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

Remove empty string as default value for VerificationMode #59

Merged
merged 5 commits into from
Jun 27, 2022

Conversation

narph
Copy link
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 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
Collaborator

elasticmachine commented Jun 7, 2022

❕ 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
Contributor Author

narph commented Jun 7, 2022

/test

2 similar comments
@narph
Copy link
Contributor Author

narph commented Jun 8, 2022

/test

@narph
Copy link
Contributor Author

narph commented Jun 8, 2022

/test

Copy link
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(?)

Copy link
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
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
Contributor

@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.

@@ -20,6 +20,8 @@ package tlscommon
import (
"testing"

"github.com/stretchr/testify/assert"

Copy link
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants