-
Notifications
You must be signed in to change notification settings - Fork 732
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
fix: enable cert rotation for audit by default #2875
Conversation
@maxsmythe this issue can also be resolve with removing Additionally, I raised this PR against master thinking we will cherry-pick in respective releases from the master. Let me know if that is not the case. I will close this PR and open a new one against 3.11 release in that case. cc: @sozercan |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #2875 +/- ##
==========================================
- Coverage 53.13% 53.10% -0.04%
==========================================
Files 135 135
Lines 11790 11790
==========================================
- Hits 6265 6261 -4
- Misses 5041 5044 +3
- Partials 484 485 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This would be a breaking change for users. Why not add the flag to audit by-default? Can we only disable cert rotation if audit has external data enabled? I'm guessing via an if statement in the Helm chart? |
what do you mean?
When we upgrade to 3.11 |
@maxsmythe @sozercan changed it to not disable cert rotation for audit by default. |
does this have any side effects since we'll be generating certificates twice (in audit and in controller-manager)? |
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
the only one I could think of was if there could be a race condition between audit and controller pod to write certs to secret. But I didn't notice anything concerning when I tested the change. |
Actually we'd be generating certs 4 times (3 controller-manager pods, 1 audit pod). Worst case side effect should be an extra write to the API server for each pod, all but the first write should fail due to optimistic concurrency check failure (stale resourceVersion). |
Is #2121 (comment) no longer an issue?
|
Couldn't say. "weird issues" is too vague to know what was noticed, if it can be reproduced, or whether it's gone. I'd suggest trying it out and seeing if we notice anything "weird". There's nothing inherent in the concept that should be broken. |
I can try running this with external data enabled to see if I face any errors. |
Tested it! I didn't notice anything weird or face issues in running the audit or webhook. |
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.
LGTM
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.
LGTM
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #2516
Special notes for your reviewer: