-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support to TLSSetting to not only read from file path, but from memory #7676
Conversation
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
I have been poking at the unit test issue and I noticed this isn't the only PR getting it. #7486 EDIT: I can recreate with |
Found the related issue I think #7537 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I feel like there's an opportunity to optimize the periodic reloading of certs if the PEM-encoded string was provided, but I recognize that this PR solves a concrete problem already.
config/configtls/configtls.go
Outdated
|
||
switch { | ||
case c.hasCAFile() && c.hasCAPem(): | ||
return nil, fmt.Errorf("failed to load CA CertPool: CA File and PEM cannot both be provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to get a native speaker to check this wording, as it sounds a bit strange to my ears. I would use something like:
failed to load CA CertPool: provide either a CA file or the PEM-encoded string, but not both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated some of the verbiage to match the suggested pattern. Let me know if you think more needs to be changed. It's no big deal on my end.
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Tests need fixing (not related to #7537 AFAICT)
|
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #7676 +/- ##
==========================================
- Coverage 91.31% 91.02% -0.30%
==========================================
Files 296 295 -1
Lines 14485 14617 +132
==========================================
+ Hits 13227 13305 +78
- Misses 995 1048 +53
- Partials 263 264 +1
☔ View full report in Codecov by Sentry. |
Tests are failing on Windows:
|
Need a fix on windows. |
Signed-off-by: Erik Baranowski <39704712+erikbaranowski@users.noreply.github.com>
Fixed test in windows |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
hey all, is there anything else needed on this PR before merge? |
Description: This implements a version of the proposed changes requested in the linked issue.
Related: A PR accomplishing similar was accepted into Prometheus recently.
Link to tracking Issue: #7313
Testing: Added tests for a variety of scenarios around filepath vs in memory certs.
Documentation: Added new parameters to the README