-
Notifications
You must be signed in to change notification settings - Fork 83
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
[WIP] Add filtering label to secret and enable secret informer filtering by default #1084
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nak3 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 |
Codecov Report
@@ Coverage Diff @@
## main #1084 +/- ##
==========================================
- Coverage 80.00% 79.57% -0.43%
==========================================
Files 18 18
Lines 1430 1449 +19
==========================================
+ Hits 1144 1153 +9
- Misses 229 237 +8
- Partials 57 59 +2
|
@skonto @dprotaso Could you please take a look if this approach is alright? I will add tests if no problem. |
@@ -89,10 +95,31 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre | |||
} | |||
|
|||
secret, err := translator.secretGetter(ingressTLS.SecretNamespace, ingressTLS.SecretName) | |||
if err != nil { | |||
if apierrors.IsNotFound(err) { |
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.
So we are trying to make the UX easier, non-breaking and we add the label ourselves.
Are there any other secrets that might need the same?
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.
Other secrets are deployed by us and so we have already added the label like https://github.com/knative/serving/blob/main/config/core/300-secret.yaml#L34 by default. So we don't need it.
So, is it okay for this approach? |
PR needs rebase. 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/test-infra repository. |
Closing due to no feedback. |
Changes
Currently users have to set
ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID
to enable secret informer filtering.It has a big pain that they have to add a label to their every single secret on their DomainMapping's BYO certs.
This patch changes the controller to add the label to the secret specified by DomainMapping.
As a consequence, it can enable
ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID
by default./kind enhancement
Release Note
TODO: