Skip to content

Simplify TLS config deserialization#31168

Merged
faec merged 2 commits into
elastic:mainfrom
faec:tls-client-auth
Apr 13, 2022
Merged

Simplify TLS config deserialization#31168
faec merged 2 commits into
elastic:mainfrom
faec:tls-client-auth

Conversation

@faec

@faec faec commented Apr 5, 2022

Copy link
Copy Markdown

What does this PR do?

A quick cleanup to tlsClientAuth.Unpack.

Fixes #12602.

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.

@faec faec added cleanup Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Apr 5, 2022
@faec faec requested a review from a team as a code owner April 5, 2022 18:46
@faec faec self-assigned this Apr 5, 2022
@elasticmachine

Copy link
Copy Markdown
Contributor

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

@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 Apr 5, 2022
@cmacknz cmacknz requested a review from belimawr April 5, 2022 19:21
@elasticmachine

elasticmachine commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

💚 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: 2022-04-05T20:04:59.854+0000

  • Duration: 85 min 35 sec

Test stats 🧪

Test Results
Failed 0
Passed 22281
Skipped 1934
Total 24215

💚 Flaky test report

Tests succeeded.

🤖 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!)

@belimawr belimawr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I only have one question/consideration.

It seems the "default" behaviour of when this config is not defined having it default to tlsClientAuthRequired is lost.

I'm not an expert on how our config lib works, but it seems that it now would return an error instead of using the default value.

@faec

faec commented Apr 11, 2022

Copy link
Copy Markdown
Author

It seems the "default" behaviour of when this config is not defined having it default to tlsClientAuthRequired is lost.

That's the reason for this cleanup -- that isn't really the default behavior, but this code makes it look like it is. The in == nil branch in Unpack is never called because the actual target field is a string (and if it isn't because of a bad config, it still won't read as nil). The real value is set in libbeat/common/transport/tlscommon/server_config.go, this is just the initial config parsing.

@faec faec merged commit d101ba1 into elastic:main Apr 13, 2022
@faec faec deleted the tls-client-auth branch April 13, 2022 14:07
v1v added a commit to v1v/beats that referenced this pull request Apr 18, 2022
…er-tar-gz

* upstream/main: (139 commits)
  [Automation] Update elastic stack version to 8.3.0-c655cda8 for testing (elastic#31322)
  Define a queue metrics reporter interface  (elastic#31289)
  [Oracle Module] Change tablespace metricset collection period (elastic#31259)
  libbeat/reader/syslog: relax timestamp parsing to allow leading zero (elastic#31254)
  [Automation] Update elastic stack version to 8.3.0-55ba6f37 for testing (elastic#31311)
  [libbeat] Remove unused fields and functions in the memory queue (elastic#31302)
  [libbeat] Cleaning up some unneeded helper types (elastic#31290)
  Readme for kibana module (elastic#31276)
  [Automation] Update elastic stack version to 8.3.0-4be61f32 for testing (elastic#31296)
  x-pack/winlogbeat/module/routing/ingest: fix typo for channel name (elastic#31291)
  Small pipeline cleanup removing some unused data fields (elastic#31288)
  removing info log (elastic#30971)
  Simplify TLS config deserialization (elastic#31168)
  Detect new files under known paths in filestream input (elastic#31268)
  Add support for port mapping in docker hints (elastic#31243)
  Update qa-labels.yml (elastic#31260)
  libbeat: log debug for `proxy_url` and fixed docs (elastic#31130)
  [heartbeat][docs] Add note about ensuring correct index settings for uptime (elastic#31146)
  [Automation] Update elastic stack version to 8.3.0-2c8f9574 for testing (elastic#31256)
  [Filebeat] fix m365_defender pipeline bug (elastic#31227)
  ...
kush-elastic pushed a commit to kush-elastic/beats that referenced this pull request May 2, 2022
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change Unpack method for tlsClientAuth

4 participants