OCPBUGS-16313: pkg/operator: correctly fetch CA for AWS minter #575
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We just need to read *one* and only *one* ConfigMap, so add a Role to let us and use a live client to just read the thing. Signed-off-by: Steve Kuznetsov <[email protected]>
openshift-ci-robot
added
jira/valid-reference
|
@stevekuznetsov: This pull request references Jira Issue OCPBUGS-16313, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #575 +/- ##
==========================================
- Coverage 47.72% 47.66% -0.06%
==========================================
Files 93 94 +1
Lines 11682 11696 +14
==========================================
Hits 5575 5575
- Misses 5480 5494 +14
Partials 627 627
|
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abutcher, stevekuznetsov 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 |
openshift-ci
bot
added
the
approved
|
@stevekuznetsov: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Merged
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@abutcher: This pull request references Jira Issue OCPBUGS-16313, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
@stevekuznetsov: Jira Issue OCPBUGS-16313: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-16313 has been moved to the MODIFIED state. In response to this:
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. |
wking
added a commit
to wking/cloud-credential-operator
that referenced
this pull request
Jan 3, 2025
Even when awsSTSIAMRoleARN is empty, we want the label so that pkg/cmd/operator's NewOperator's filteredWatchPossible label-selector can find these Secrets. Then the controller will notice if they're deleted (so it can update the CredentialsRequest status to point that out) or when they haven't been changed (so it can avoid "I can't find the Secret!" overly-frequent bumping in the hasRecentlySynced calculation, because it thinks crSecretExists=false). And we want the annotation, so it's clear why the Secret needs to exist (because of the annotation-referenced CredentialsRequest). The risk here is that we might end up contending over label/annotation presence with the external controller that is populating the 'credentials' data inside the Secret. But the alternative of an unfiltered Secret informer in the client is still too resource-intensive, as described in the filteredWatchPossible comment and the a58a09c (*: use a filtered LIST + WATCH on Secrets for AWS STS, 2023-06-29, openshift#545) commit that added the filteredWatchPossible logic. Additional labels and annotations are properties that external controllers should be able to accept. For example, [1] has ArgoCD discussing: apiVersion: argoproj.io/v1alpha1 kind: ApplicationSet spec: # (...) preservedFields: annotations: ["my-custom-annotation"] labels: ["my-custom-label"] to ignore annotations and labels injected by external-to-ArgoCD controllers, which is what the CCO-specific annotation/label I'm touching now would be. Moving to 48d6ccc (pkg/operator: correctly fetch CA for AWS minter, 2023-07-19, openshift#575)'s LiveClient avoids confusing CreateOrPatch. With the cached .Client, it would have: 1. Failed to retrive an unlabeled Secret, because the externally-created Secret lacked the label that the Client's filteredWatchPossible informer is filtered on. 2. Thought that it should Create a new Secret. 3. Had that Create attempt fail on 'secrets "$NAME" already exists'. With the LiveClient, that becomes: 1. Successfully retrived an unlabeled Secret, with the uncached reader. 2. Thought that it should Patch the Secret. 3. Successfully Patch the Secret. 4. Once the Patch sets the label, future attempts to Get the Secret through the filtered informer cache will succeed. [1]: https://argo-cd.readthedocs.io/en/release-2.13/operator-manual/applicationset/Controlling-Resource-Modification/#preserving-changes-made-to-an-applications-annotations-and-labels
wking
added a commit
to wking/cloud-credential-operator
that referenced
this pull request
Jan 4, 2025
Even when awsSTSIAMRoleARN is empty, we want the label so that pkg/cmd/operator's NewOperator's filteredWatchPossible label-selector can find these Secrets. Then the controller will notice if they're deleted (so it can update the CredentialsRequest status to point that out) or when they haven't been changed (so it can avoid "I can't find the Secret!" overly-frequent bumping in the hasRecentlySynced calculation, because it thinks crSecretExists=false). And we want the annotation, so it's clear why the Secret needs to exist (because of the annotation-referenced CredentialsRequest). The risk here is that we might end up contending over label/annotation presence with the external controller that is populating the 'credentials' data inside the Secret. But the alternative of an unfiltered Secret informer in the client is still too resource-intensive, as described in the filteredWatchPossible comment and the a58a09c (*: use a filtered LIST + WATCH on Secrets for AWS STS, 2023-06-29, openshift#545) commit that added the filteredWatchPossible logic. Additional labels and annotations are properties that external controllers should be able to accept. For example, [1] has ArgoCD discussing: apiVersion: argoproj.io/v1alpha1 kind: ApplicationSet spec: # (...) preservedFields: annotations: ["my-custom-annotation"] labels: ["my-custom-label"] to ignore annotations and labels injected by external-to-ArgoCD controllers, which is what the CCO-specific annotation/label I'm touching now would be. Moving to 48d6ccc (pkg/operator: correctly fetch CA for AWS minter, 2023-07-19, openshift#575)'s LiveClient avoids confusing CreateOrPatch. With the cached .Client, it would have: 1. Failed to retrive an unlabeled Secret, because the externally-created Secret lacked the label that the Client's filteredWatchPossible informer is filtered on. 2. Thought that it should Create a new Secret. 3. Had that Create attempt fail on 'secrets "$NAME" already exists'. With the LiveClient, that becomes: 1. Successfully retrived an unlabeled Secret, with the uncached reader. 2. Thought that it should Patch the Secret. 3. Successfully Patch the Secret. 4. Once the Patch sets the label, future attempts to Get the Secret through the filtered informer cache will succeed. [1]: https://argo-cd.readthedocs.io/en/release-2.13/operator-manual/applicationset/Controlling-Resource-Modification/#preserving-changes-made-to-an-applications-annotations-and-labels
wking
added a commit
to wking/cloud-credential-operator
that referenced
this pull request
Jan 4, 2025
Even when awsSTSIAMRoleARN is empty, we want the label so that pkg/cmd/operator's NewOperator's filteredWatchPossible label-selector can find these Secrets. Then the controller will notice if they're deleted (so it can update the CredentialsRequest status to point that out) or when they haven't been changed (so it can avoid "I can't find the Secret!" overly-frequent bumping in the hasRecentlySynced calculation, because it thinks crSecretExists=false). And we want the annotation, so it's clear why the Secret needs to exist (because of the annotation-referenced CredentialsRequest). The risk here is that we might end up contending over label/annotation presence with the external controller that is populating the 'credentials' data inside the Secret. But the alternative of an unfiltered Secret informer in the client is still too resource-intensive, as described in the filteredWatchPossible comment and the a58a09c (*: use a filtered LIST + WATCH on Secrets for AWS STS, 2023-06-29, openshift#545) commit that added the filteredWatchPossible logic. Additional labels and annotations are properties that external controllers should be able to accept. For example, [1] has ArgoCD discussing: apiVersion: argoproj.io/v1alpha1 kind: ApplicationSet spec: # (...) preservedFields: annotations: ["my-custom-annotation"] labels: ["my-custom-label"] to ignore annotations and labels injected by external-to-ArgoCD controllers, which is what the CCO-specific annotation/label I'm touching now would be. Moving to 48d6ccc (pkg/operator: correctly fetch CA for AWS minter, 2023-07-19, openshift#575)'s LiveClient avoids confusing CreateOrPatch. With the cached .Client, it would have: 1. Failed to retrive an unlabeled Secret, because the externally-created Secret lacked the label that the Client's filteredWatchPossible informer is filtered on. 2. Thought that it should Create a new Secret. 3. Had that Create attempt fail on 'secrets "$NAME" already exists'. With the LiveClient, that becomes: 1. Successfully retrived an unlabeled Secret, with the uncached reader. 2. Thought that it should Patch the Secret. 3. Successfully Patch the Secret. 4. Once the Patch sets the label, future attempts to Get the Secret through the filtered informer cache will succeed. [1]: https://argo-cd.readthedocs.io/en/release-2.13/operator-manual/applicationset/Controlling-Resource-Modification/#preserving-changes-made-to-an-applications-annotations-and-labels
stephenfin
added a commit
to shiftstack/cloud-credential-operator
that referenced
this pull request
Jun 9, 2025
This was first introduced in commit 48d6ccc as a resolution to OCPBUGS-16313 [1][2], which was itself introduced by the removal of configmaps read access from the cluster role used by CCO. However, non-caching clients are expensive and with the change introduced in the previous commit, which restricted caching to specific config maps, plus the existing role allowing access to these config maps, their use should no longer be necessary. [1] openshift#575 [2] https://issues.redhat.com/browse/OCPBUGS-16313 Signed-off-by: Stephen Finucane <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We just need to read one and only one ConfigMap, so add a Role to let us and use a live client to just read the thing.