OCPBUGS-74627: Fix infrastructure resource name filtering in watch predicate#985
OCPBUGS-74627: Fix infrastructure resource name filtering in watch predicate#985jstuever wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jstuever: This pull request references Jira Issue OCPBUGS-74627, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe Infrastructure event predicate in the credentialsrequest controller is tightened to only target the Infrastructure resource named "cluster" and to require presence of AWS resource tags on Create and Update events. Two unit tests were added to validate tag-detection and tag-update helper functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstuever The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
jstuever
force-pushed
the
OCPBUGS-74627
branch
2 times, most recently
from
23e6604 to
7258b69
Compare
OCPBUGS-74627: CCO should only process the infrastructure resource named "cluster", not other infrastructure resources like "cloud-provider-config" that may exist in the cluster. This change updates the infraResourcePredicate to check that the infrastructure resource name is "cluster" before processing create and update events. This prevents CCO from incorrectly processing infrastructure resources with other names. Also adds comprehensive tests to verify the predicate correctly filters infrastructure resources by name. Assisted-by: Claude Sonnet 4.5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/credentialsrequest/credentialsrequest_controller.go (1)
257-268: Deduplicate the Infrastructure resource name.Line 262 and Line 268 add another
"cluster"literal. The same singleton name is already hardcoded inpkg/operator/utils/utils.go, so keeping it inline here makes future drift between the watch predicate and the lookup path more likely. Prefer a dedicated constant for the Infrastructure resource name and reuse it in both places.♻️ Suggested cleanup
const ( controllerName = "credreq" labelControllerName = controllerName + "_labeller" + infrastructureResourceName = "cluster" namespaceMissing = "NamespaceMissing" namespaceExists = "NamespaceExists" @@ - return e.Object.GetName() == "cluster" && hasResourceTags(e.Object) + return e.Object.GetName() == infrastructureResourceName && hasResourceTags(e.Object) @@ - return e.ObjectNew.GetName() == "cluster" && areTagsUpdated(e.ObjectOld, e.ObjectNew) + return e.ObjectNew.GetName() == infrastructureResourceName && areTagsUpdated(e.ObjectOld, e.ObjectNew)Also update the
types.NamespacedName{Name: "cluster"}lookup inpkg/operator/utils/utils.goto use the same constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/credentialsrequest/credentialsrequest_controller.go` around lines 257 - 268, The predicate uses the literal "cluster" in infraResourcePredicate (TypedFuncs[*configv1.Infrastructure] CreateFunc and UpdateFunc) which duplicates the same singleton name used elsewhere; replace these literals with a shared constant (e.g., InfrastructureName or InfrastructureResourceName) defined in the utils package and reference that constant in infraResourcePredicate, ensuring CreateFunc and UpdateFunc compare against the constant and UpdateFunc continues to call areTagsUpdated(e.ObjectOld, e.ObjectNew); also update the lookup that uses types.NamespacedName{Name: "cluster"} in utils.go to use the same constant so both the watch predicate and lookup share the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/credentialsrequest/credentialsrequest_controller_test.go`:
- Around line 2078-2285: The tests currently miss cases for Infrastructure
objects whose .ObjectMeta.Name is not "cluster"; add regression cases to
TestHasResourceTags and TestAreTagsUpdated that use an infra with Name set to a
non-"cluster" value (e.g., "cloud-provider-config") and assert the expected
boolean results from hasResourceTags and areTagsUpdated, and also add a small
test that invokes the controller's Infrastructure predicate (the new predicate
added in the credentialsrequest controller) to ensure it rejects non-"cluster"
Infrastructure objects so the predicate behavior is explicitly verified.
---
Nitpick comments:
In `@pkg/operator/credentialsrequest/credentialsrequest_controller.go`:
- Around line 257-268: The predicate uses the literal "cluster" in
infraResourcePredicate (TypedFuncs[*configv1.Infrastructure] CreateFunc and
UpdateFunc) which duplicates the same singleton name used elsewhere; replace
these literals with a shared constant (e.g., InfrastructureName or
InfrastructureResourceName) defined in the utils package and reference that
constant in infraResourcePredicate, ensuring CreateFunc and UpdateFunc compare
against the constant and UpdateFunc continues to call
areTagsUpdated(e.ObjectOld, e.ObjectNew); also update the lookup that uses
types.NamespacedName{Name: "cluster"} in utils.go to use the same constant so
both the watch predicate and lookup share the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b905de42-8666-4137-a10b-485698957e8c
📒 Files selected for processing (2)
pkg/operator/credentialsrequest/credentialsrequest_controller.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.go
|
/jira backport release-4.21,release-4.20 |
|
/jira refresh |
|
@jstuever: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: once the present PR merges, I will cherry-pick it on top of DetailsIn 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-sigs/prow repository. |
openshift-ci-robot
added
jira/valid-bug
|
@jstuever: This pull request references Jira Issue OCPBUGS-74627, 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: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@jstuever: This pull request references Jira Issue OCPBUGS-74627, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/credentialsrequest/credentialsrequest_controller.go`:
- Around line 257-268: Add a predicate-level regression test that asserts
infraResourcePredicate only allows events for Infrastructure objects named
"cluster" (and rejects others like "cloud-provider-config"); specifically,
construct TypedCreateEvent and TypedUpdateEvent inputs and verify
infraResourcePredicate.CreateFunc returns true only when e.Object.GetName() ==
"cluster" and hasResourceTags(e.Object) is true, and
infraResourcePredicate.UpdateFunc returns true only when e.ObjectNew.GetName()
== "cluster" and areTagsUpdated(e.ObjectOld, e.ObjectNew) is true; include
negative cases where name != "cluster" to ensure the name gate prevents
processing regardless of tag helpers.
- Around line 267-268: The UpdateFunc predicate currently enqueues only when
areTagsUpdated(e.ObjectOld, e.ObjectNew) is true, which misses present->absent
transitions; update the predicate in the UpdateFunc for
event.TypedUpdateEvent[*configv1.Infrastructure] so it returns true when the
object is the "cluster" AND either areTagsUpdated(old,new) OR tags were removed
(i.e., detect a present->absent change). Implement this by adding a small helper
(e.g., tagsRemoved(old, new)) or by extending areTagsUpdated to treat removal as
an update, and reference the UpdateFunc closure, areTagsUpdated, and the
TypedUpdateEvent[*configv1.Infrastructure] event to locate where to change the
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98dcd8ff-7b29-4ec6-a3ff-5e78bcb3cdcc
📒 Files selected for processing (2)
pkg/operator/credentialsrequest/credentialsrequest_controller.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/credentialsrequest/credentialsrequest_controller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #985 +/- ##
==========================================
+ Coverage 46.20% 46.30% +0.09%
==========================================
Files 98 98
Lines 12253 12253
==========================================
+ Hits 5662 5674 +12
+ Misses 5941 5929 -12
Partials 650 650
🚀 New features to boost your workflow:
|
|
@jstuever: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
3 participants
OCPBUGS-74627: CCO should only process the infrastructure resource named "cluster", not other infrastructure resources like "cloud-provider-config" that may exist in the cluster.
This change updates the infraResourcePredicate to check that the infrastructure resource name is "cluster" before processing create and update events. This prevents CCO from incorrectly processing infrastructure resources with other names.
Also adds comprehensive tests to verify the predicate correctly filters infrastructure resources by name.
Assisted-by: Claude Sonnet 4.5
Summary by CodeRabbit
Bug Fixes
Tests