Fix: Add garbage collection to handle Approved-Unissued CSRs #133116
Conversation
|
|
|
Welcome @264nm! |
|
Hi @264nm. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 264nm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2fd7180 to
036f4bc
Compare
|
Force pushed an amend commit so the CLA emails lined up properly.. Let me know what you guys think. |
|
thanks for the PR, it's in the review queue but won't make it before 1.34 code freeze tomorrow, will take a look some time after 1.34 wraps up |
|
/triage accepted |
| // isApprovedUnissuedPastDeadline checks if the certificate has an Approved status but | ||
| // no certificate has been issued, and the approval time has passed the deadline | ||
| // that pending requests are maintained for. | ||
| func isApprovedUnissuedPastDeadline(logger klog.Logger, csr *capi.CertificateSigningRequest) bool { |
There was a problem hiding this comment.
this is a pre-existing issue with some of the other check functions, but let's improve this to hoist the checks that don't change out of the loop, e.g.:
func isApprovedUnissuedPastDeadline(logger klog.Logger, csr *capi.CertificateSigningRequest) bool {
if isIssued(csr) {
return false
}
for _, c := range csr.Status.Conditions {
if c.Type == capi.CertificateApproved && isOlderThan(c.LastUpdateTime, pendingExpiration) {
...can make the same change to isIssuedPastDeadline
func isIssuedPastDeadline(logger klog.Logger, csr *capi.CertificateSigningRequest) bool {
if !isIssued(csr) {
return false
}
for _, c := range csr.Status.Conditions {
if c.Type == capi.CertificateApproved && isOlderThan(c.LastUpdateTime, approvedExpiration) {
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently there is no mechanism to account for garbage collection of Approved-But-Unissued kubelet client CSRs which may happen in scenarios like HashiVault managed certificate signing, which over time will lead to excessive etcd space consumption. This implements the suggestion in issue #132947 where a condition is added to the cleaner which will treat certificates in this state, in the same way it treats
PendingCSRs so they can get garbage collected every 24 hours.Which issue(s) this PR is related to:
#132947
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: