-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: add cert rotator #869
Conversation
e690d43
to
234af41
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #869 +/- ##
==========================================
- Coverage 55.03% 54.55% -0.48%
==========================================
Files 80 81 +1
Lines 4739 4797 +58
==========================================
+ Hits 2608 2617 +9
- Misses 1848 1896 +48
- Partials 283 284 +1
☔ View full report in Codecov by Sentry. |
0cc0f2f
to
9d80500
Compare
d6b5213
to
c69efa3
Compare
As discussed offline, lets validate if Ratify throws error on startup when customer provides a expired cert. |
56ce934
to
e7f0521
Compare
25ca8c8
to
ee401fc
Compare
a2e65b3
to
349e80d
Compare
349e80d
to
6ed43f3
Compare
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.
PR looks great Binbin, could you provide more details on logs and test coverage?
Have we covered cases for:
- rotation customer provided cert
- rotation for ratify auto generated cert
- Are there any customer provided cert that we don't support rotation that we should document?
- Once a cert is rotated by the rotator, what does ratify log look like?
- Have we validated ratify 's configuration matrix with cert rotater disabled?
thanks!
13915d1
to
2a325ee
Compare
} | ||
|
||
@test "cert rotator test" { | ||
helm uninstall ratify --namespace gatekeeper-system |
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 tried to use helm upgrade
which works without providing --set featureFlags.RATIFY_CERT_ROTATION=true
flag. But it always failed to deploy Ratify pod with it. I checked logs which shows the pod is already up but helm regards it as not ready. What's more interesting is that the same helm upgrade
command works on my local machine.
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.
oh that's interesting. Maybe there's another resource other than the pod that is failing to be upgraded due some cpu/mem limitation? Either way I'm personally ok with your solution.
Signed-off-by: Binbin Li <libinbin@microsoft.com>
2a325ee
to
7cab6f8
Compare
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
Signed-off-by: Binbin Li <libinbin@microsoft.com>
Description
What this PR does / why we need it:
This is a following PR of #831. Add cert-rotator in Ratify to support rotating TLS related certificates automatically. The overall design doc can be found here: https://hackmd.io/@akashsinghal/BySB4VbHh
externaldata.gatekeeper.sh/v1beta1
.CERT_ROTATION
so that this feature is behind FF.Ratify_NAME
env var to Ratify as cert-rotator needs it for validation and cert generation.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 #834
Type of change
Please delete options that are not relevant.
main
branch)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Adding a new e2e test which will be provided an expiring cert in deployment, which will trigger a cert rotatation immediately.
Checklist:
Post Merge Requirements
Helm Chart Change