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.

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

Enable TLS by default in shipper output #34425

Closed

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Jan 30, 2023

What does this PR do?

closes #34321

This changes the default config for the shipper output so TLS is enabled by default, which is what elastic-agent and shipper expect.

How to test:

      env:
        - name: GRPC_GO_LOG_VERBOSITY_LEVEL
          value: 99
        - name: GRPC_GO_LOG_SEVERITY_LEVEL
          value: info
        - name: GRPC_TRACE
          value: all

@fearful-symmetry fearful-symmetry added bug Team:Elastic-Agent Label for the Agent team labels Jan 30, 2023
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner January 30, 2023 20:14
@fearful-symmetry fearful-symmetry self-assigned this Jan 30, 2023
@fearful-symmetry fearful-symmetry requested review from rdner and cmacknz and removed request for a team January 30, 2023 20:14
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jan 30, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 30, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @fearful-symmetry? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 30, 2023

💚 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

  • Start Time: 2023-02-01T18:21:43.766+0000

  • Duration: 67 min 32 sec

Test stats 🧪

Test Results
Failed 0
Passed 25313
Skipped 1962
Total 27275

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the 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!)

Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

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

The description needs manual testing steps, how can we check that the shipper is actually running with TLS? The config option might be as well ignored.

@fearful-symmetry
Copy link
Contributor Author

@rdner so, I'm sort of going off of @blakerouse 's comment in the issue, which is that TLS should be enabled, unless the agent explicitly disabled it. Unless I'm misunderstanding your question?

@rdner
Copy link
Member

rdner commented Feb 1, 2023

@fearful-symmetry my point was rather that would be nice to have steps in the description to test the change. Just to make sure the configuration parameter is not lost in some propagation.

If the TLS on is the default, then we need to verify that the flag actually disables it when set to false.

I just don't see anything in this PR that checks or verifies that, unless I've missed it.

@fearful-symmetry
Copy link
Contributor Author

@rdner

my point was rather that would be nice to have steps in the description to test the change. Just to make sure the configuration parameter is not lost in some propagation.

Ah, sorry, didn't understand what you meant, sorry. I can try to add some testing instructions, but at this point, there's so many bugs and in-flight PRs needed for the shipper that I don't think it's particularly easy to test.

I agree though, gonna at least see if I can add some go tests.

@fearful-symmetry
Copy link
Contributor Author

Alright, added a test and a some more to the description.

@@ -53,6 +53,7 @@ type Config struct {
func defaultConfig() Config {
enabled := true
return Config{
// agent will expect that TLS is enabled by default, will disable explicitly
Copy link
Member

Choose a reason for hiding this comment

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

will disable explicitly

Looks confusing because of the enabled := true above. What does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, it should say "the agent will disabled it explicitly"

@fearful-symmetry
Copy link
Contributor Author

Brief update: Since we currently don't have a timeline for when we want to make changes to the rest of the TLS stack, I'm thinking of leaving this PR unmerged until elastic/elastic-agent-shipper#224 is dealt with, as otherwise we'd need to come up with another workaround to disable TLS until we fix that.

@jlind23
Copy link
Collaborator

jlind23 commented Feb 6, 2023

@leehinman @pierrehilbert @fearful-symmetry as elastic/elastic-agent-shipper#224 is still not planned yet, can I move #34321 out fo the current sprint as otherwise it will stay unmerged?

@pierrehilbert
Copy link
Collaborator

I was expecting to discuss about this on tomorrow in the shipper area but yes I think we can move this one out of the sprint

@mergify
Copy link
Contributor

mergify bot commented Feb 27, 2023

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 shipper-tls-config upstream/shipper-tls-config
git merge upstream/main
git push upstream shipper-tls-config

@cmacknz
Copy link
Member

cmacknz commented May 9, 2023

Closing, we can reopen when we revisit this

@cmacknz cmacknz closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Beats shipper output will not enable TLS
6 participants