Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Short Rotation Period For Certificates #1670
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
base: master
Are you sure you want to change the base?
Add Short Rotation Period For Certificates #1670
Changes from all commits
397766a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a specific reason for leaving this up to the components?
The dev flag already means an upper bound on the rotation time:
i.e:
So within 6 hrs all of the components of interest should have cycled through a rotation.
So why don't we just explicitly dictate the duration (e.g 3hrs) for the relevant components?
I'm guessing we likely don't want all components to rotate at the same time (although not sure if we expect to tolerate any API disruption from these rotations).
But if the motivation for letting components choose, is to stagger the rotations, then they could still overlap rotations if they end up choosing the same "shortened" duration or change it later for whatever reason.
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.
Cert durations depend on functionality, as some certs are relatively painless to replace and some require kube-apiserver revision rollout. So setting all certs to say 30 mins would break our distruption tests. Also we don't know yet how the certs are interacting and what are the effects of rotations, so I'd rather not dictate exact certificate durations in this enhancement.
That said we do want set an upper limit - if the certificate is rotatable it must be rotated during that test, so cert validity duration is capped at 8 hours (but the less the better as its useful to observe several rotations).
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.
Fair enough. We can hold off on the configurability of the "short" durations until we feel like we actually need to tune that per test run. Or if bumping the hardcoded rotation periods per component proves to be too unwieldy when iterating in CI.
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.
Details are scant. To achieve the same benefit, this featuregate needs to be enabled in Default during pre-RC builds and moved to TechPreview after RC.0. Can we get this spelled out?
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.
There wasn't any benefit, "developer branch fast rotation" has never worked in CI - see https://github.com/openshift/enhancements/pull/1670/files#diff-1695a5e93f0f7e139919d0e0fac08ce0ea6d442932ff1cc7fab792ef1c616cf1R31-R35
Added more details on how this will be tested in CI and promotion criteria
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.
The point is not coverage in CI. The point is every pre-production cluster in existence that runs longer than a day. Which covers a wide variety of clusters internal to this group and external to this group including education, TAMs, test platform, and various test environments. Losing this capability is enormous. CI was an objective, but we gained tremendous benefit without ever having a rotation in CI.
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 never materialized into bugs though (especially into availability tracking), has it? Testing fast rotation in CI however will have the same effect alongside with a better set of debugging information.
In any case, this featuregate doesn't forbid specific components to do pre-release shorter cert rotations
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.
Added clarification that existing devrotation code can be used alongside short rotation featuregate