Skip to content

Remove empty string as default value for VerificationMode #59

Merged
narph merged 5 commits intoelastic:mainfrom
narph:update-tls-mode
Jun 27, 2022
Merged

Remove empty string as default value for VerificationMode #59
narph merged 5 commits intoelastic:mainfrom
narph:update-tls-mode

Conversation

@narph
Copy link
Copy Markdown
Contributor

@narph narph commented Jun 7, 2022

  • Bug

What does this PR do?

Original issue elastic/elastic-agent#184
Elastic agent accepts empty string value for ssl.verification_mode (which is equivalent to full), however Endpoint is logging an error since it is not a supported option, but, it's also setting the verification mode to full, so verification modes match.

The PR is removing empty string as a default/supported verification mode option. Default mode will be full.

Why is it important?

Avoids conflicts with other stakeholders.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.md

Related issues

@narph narph requested a review from a team as a code owner June 7, 2022 11:46
@narph narph requested review from belimawr and fearful-symmetry and removed request for a team June 7, 2022 11:46
@narph narph mentioned this pull request Jun 7, 2022
Merged
6 tasks
@narph narph self-assigned this Jun 7, 2022
@narph narph changed the title remove empty option Remove empty string as default value for VerificationMode Jun 7, 2022
@narph narph added v8.4.0 Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Jun 7, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jun 7, 2022 •
edited
Loading

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

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

Expand to view the summary

Build stats

  • Start Time: 2022-06-23T12:24:52.951+0000

  • Duration: 62 min 51 sec

Test stats 🧪

Test Results
Failed 0
Passed 716
Skipped 5
Total 721

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@narph
Copy link
Copy Markdown
Contributor Author

narph commented Jun 7, 2022

/test

2 similar comments
@narph
Copy link
Copy Markdown
Contributor Author

narph commented Jun 8, 2022

/test

@narph
Copy link
Copy Markdown
Contributor Author

narph commented Jun 8, 2022

/test

fearful-symmetry
fearful-symmetry approved these changes Jun 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@fearful-symmetry fearful-symmetry left a comment

Choose a reason for hiding this comment

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

Changes look good to me, although I'm assuming the change itself won't involve any user-facing breaking changes(?)

belimawr
belimawr requested changes Jun 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

@narph, the PR looks great, but to be on the safe side, I'd like to see test case for when the verification_mode is set to an empty string, e.g: test parsing a YAML like this:

verification_mode: ""

@narph narph requested a review from belimawr June 23, 2022 10:43
@narph
Copy link
Copy Markdown
Contributor Author

narph commented Jun 23, 2022

@narph, the PR looks great, but to be on the safe side, I'd like to see test case for when the verification_mode is set to an empty string, e.g: test parsing a YAML like this:

verification_mode: ""

I've added some tests for this, including one with the scenario you suggested above but I assume by default verification_mode is not added in the configuration file instead of having a empty string value. Correct me if I am wrong here.

@belimawr
Copy link
Copy Markdown
Contributor

belimawr commented Jun 27, 2022

@narph, the PR looks great, but to be on the safe side, I'd like to see test case for when the verification_mode is set to an empty string, e.g: test parsing a YAML like this:

verification_mode: ""

I've added some tests for this, including one with the scenario you suggested above but I assume by default verification_mode is not added in the configuration file instead of having a empty string value. Correct me if I am wrong here.

Yes, you're right, but some agent policies ended up having verification_mode: "", so I wanted to be extra cautious here.

belimawr
belimawr approved these changes Jun 27, 2022
View reviewed changes
transport/tlscommon/types_test.go
"testing"

"github.com/stretchr/testify/assert"

Copy link
Copy Markdown
Contributor

@belimawr belimawr Jun 27, 2022

Choose a reason for hiding this comment

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

Suggested change

@narph narph merged commit b5be848 into elastic:main Jun 27, 2022
@narph narph deleted the update-tls-mode branch June 27, 2022 16:10
@cmacknz cmacknz mentioned this pull request Jun 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@belimawr belimawr belimawr approved these changes

@fearful-symmetry fearful-symmetry fearful-symmetry approved these changes

Assignees

@narph narph

Labels

Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@narph @elasticmachine @belimawr @fearful-symmetry