Add the removal of unrequired certificates #1705
Conversation
jetstack-bot
added
dco-signoff: yes
jetstack-bot
added
size/L
| } | ||
|
|
||
| func isUnrequiredCertificate(crt *v1alpha1.Certificate, ing *extv1beta1.Ingress) bool { | ||
| if !metav1.IsControlledBy(crt, ing) { |
There was a problem hiding this comment.
I think it might be possible for us to be cleverer with the label selector in findUnrequiredCertificates in order fo filter the list of certificate's we consider to only those that are controlled/owned by the given Ingress resource. I'm not too certain though, but #sig-api-machinery on the k8s slack will be able to help.
It may not be important to do now, but it could be interesting for you to look into. If you don't want to do it now, could you add a TODO here so we don't forget to consider this option in future? 😄
There was a problem hiding this comment.
Certificates take all the same labels as their owning Ingress, and I imagine an Ingress doesn't add a label onto itself identifying itself so I think we would want to add another label onto new Certificates which links it to a certain Ingress in order to use a more sophisticated selector - maybe the Ingress' UID?
I will add a TODO for now.
|
Looks like we may have created a new test flake with the new webhook support 😅 this PR looks good to me. I'll add the appropriate labels in case you want to get this merged (just comment /lgtm |
jetstack-bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheukwing, munnerz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jetstack-bot
added
lgtm
|
/retest |
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
|
/retest |
|
/lgtm |
|
/hold cancel |
jetstack-bot
removed
the
do-not-merge/hold
What this PR does / why we need it:
When the
SecretNameof a TLS entry in an ingress is changed/modified, the previous certificate is deleted.This fixes issues with work queues being filled with unrequired certificates.
Which issue this PR fixes: fixes #912
Special notes for your reviewer:
The change to
sync_test.gowill need to be modified for simplicity to follow the changes in pull request #1670 (w.r.t. owner references).Release note: