Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libbeat/common/transport: fix log message about TLS #30063

Merged
merged 5 commits into from
Feb 9, 2022
Merged

libbeat/common/transport: fix log message about TLS #30063

merged 5 commits into from
Feb 9, 2022

Conversation

belimawr
Copy link
Contributor

@belimawr belimawr commented Jan 27, 2022 •
edited
Loading

What does this PR do?

This commit fixes the log message issued by the test output command.
Our current TLS verification relies on more than the value of
tlsConfig.InsecureSkipVerify, so the previous implementation would
log that TLS was disabled when it was not.

This commit fixes it by checking the value of config.Verification.

Why is it important?

It fix a misleading log message

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

How to test this PR locally

  1. Configure any output with TLS enabled and ssl.verification_mode != strict (it always worked for strict).
  2. Run the test output command from your beat
  3. You should see the log: server's certificate chain verification is enabled

Related issues

## Use cases

## Screenshots

## Logs

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 27, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 27, 2022

This pull request does not have a backport label. Could you fix it @belimawr? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 27, 2022
@belimawr belimawr added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jan 27, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 27, 2022
@jlind23 jlind23 requested a review from a team January 27, 2022 14:54
@belimawr
libbeat/common/transport: fix log message about TLS
0250eef 
This commit fixes the log message issued by the `test output` command.
Our current TLS verification relies on more than the value of
`tlsConfig.InsecureSkipVerify`, so the previous implementation would
log that TLS was disabled when it was not.

This commit fixes it by checking the value of `config.Verification`.
@belimawr belimawr marked this pull request as ready for review January 27, 2022 16:58
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@belimawr belimawr requested review from kvch and ph January 27, 2022 16:58
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 27, 2022 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Reason: null

  • Start Time: 2022-02-03T10:11:56.740+0000

  • Duration: 19 min 52 sec

  • Commit: 2641d8e

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

kvch
kvch reviewed Jan 28, 2022
View reviewed changes
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
kvch
kvch reviewed Jan 28, 2022
View reviewed changes
Copy link
Contributor

@kvch kvch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you address the comments, I will approve.

@belimawr
Copy link
Contributor Author

@kvch @cmacknz I've addressed your comments on 6273038.

@cmacknz I was quite detailed on the comment, let me know if it was too much 😅

cmacknz
cmacknz approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great comment.

@belimawr @cmacknz
fix typo
6cf7c2a 
Co-authored-by: Craig MacKenzie <[email protected]>
@belimawr
Copy link
Contributor Author

@ruflin should we backport?

ruflin
ruflin approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, lets backport.

@belimawr belimawr added the backport-7.17 Automated backport to the 7.17 branch with mergify label Jan 31, 2022
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Jan 31, 2022
@belimawr belimawr added backport-skip Skip notification from the automated backport with mergify backport-v8.0.0 Automated backport with mergify labels Jan 31, 2022
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Jan 31, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 1, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-tls-message upstream/fix-tls-message
git merge upstream/master
git push upstream fix-tls-message

@belimawr belimawr merged commit e208c22 into elastic:main Feb 9, 2022
@belimawr belimawr deleted the fix-tls-message branch February 9, 2022 09:47
mergify bot pushed a commit that referenced this pull request Feb 9, 2022
@belimawr
libbeat/common/transport: fix log message about TLS (#30063)
293814d 
This commit fixes the log message issued by the `test output` command.
Our current TLS verification relies on more than the value of
`tlsConfig.InsecureSkipVerify`, so the previous implementation would
log that TLS was disabled when it was not.

This commit fixes it by checking the value of `config.Verification`.

Co-authored-by: Craig MacKenzie <[email protected]>
(cherry picked from commit e208c22)
@mergify mergify bot mentioned this pull request Feb 9, 2022
Closed
mergify bot pushed a commit that referenced this pull request Feb 9, 2022
@belimawr
libbeat/common/transport: fix log message about TLS (#30063)
2737b7a 
This commit fixes the log message issued by the `test output` command.
Our current TLS verification relies on more than the value of
`tlsConfig.InsecureSkipVerify`, so the previous implementation would
log that TLS was disabled when it was not.

This commit fixes it by checking the value of `config.Verification`.

Co-authored-by: Craig MacKenzie <[email protected]>
(cherry picked from commit e208c22)
@mergify mergify bot mentioned this pull request Feb 9, 2022
Closed
@cmacknz cmacknz mentioned this pull request Feb 9, 2022
Closed
cmacknz added a commit to cmacknz/beats that referenced this pull request Feb 10, 2022
@cmacknz
Revert "libbeat/common/transport: fix log message about TLS (elastic#…
5a87840 
…30063)"

This reverts commit e208c22.

This commit caused the agent to crash on startup on Windows with a nil
pointer dereference.
cmacknz added a commit that referenced this pull request Feb 10, 2022
@cmacknz
Revert "libbeat/common/transport: fix log message about TLS (#30063)" (…
7ccb649 
…#30320)

This reverts commit e208c22.

This commit caused the agent to crash on startup on Windows with a nil
pointer dereference.
@belimawr belimawr mentioned this pull request Feb 10, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvch kvch kvch approved these changes

@cmacknz cmacknz cmacknz approved these changes

@ruflin ruflin ruflin approved these changes

@ph ph Awaiting requested review from ph

Assignees

@belimawr belimawr

Labels
backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.0.0 Automated backport with mergify review Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@belimawr @elasticmachine @ruflin @cmacknz @kvch