OCPBUGS-65488: Add cluster-scoped RBAC and CRDs to network ClusterOperator relatedObjects#3013
OCPBUGS-65488: Add cluster-scoped RBAC and CRDs to network ClusterOperator relatedObjects#3013smulje wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-low
|
@smulje: This pull request references Jira Issue OCPBUGS-65488, 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 PR updates the network operator’s ClusterOperator status to include additional related Kubernetes resources, and mirrors those references in controller reconciliation before status is written. ChangesNetwork operator related objects
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @smulje. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smulje The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manifests/0000_70_cluster-network-operator_05_clusteroperator.yaml`:
- Around line 10-25: The controller is overwriting
ClusterOperator.status.relatedObjects with only objects from network.Render(...)
(via pkg/controller/operconfig/operconfig_controller.go ->
r.status.SetRelatedObjects) and status_manager.set() calls
deleteRelatedObjectsNotRendered(co), which removes the three CRDs and two
ClusterRoleBindings declared in this manifest; fix by either ensuring the
computed relatedObjects (in operconfig_controller.go / network.Render) includes
the two ClusterRoleBinding names (cluster-network-operator and
default-account-cluster-network-operator) and the three CRD names
(egressrouters.network.operator.openshift.io,
operatorpkis.network.operator.openshift.io, networks.operator.openshift.io), or
modify pkg/controller/statusmanager/status_manager.go (set() /
deleteRelatedObjectsNotRendered) to merge/preserve CVO-managed relatedObjects by
adding those CRD names to keepCRDs or by changing deletion logic to union
existing co.Status.RelatedObjects with rendered status.relatedObjects for known
CVO-managed entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44c044d7-a087-48f6-b5c7-a964b9c23d7f
📒 Files selected for processing (1)
manifests/0000_70_cluster-network-operator_05_clusteroperator.yaml
…Objects Add missing cluster-scoped resources to the network ClusterOperator's relatedObjects field in both the manifest and the controller code: - ClusterRoleBinding: cluster-network-operator - ClusterRoleBinding: default-account-cluster-network-operator - CRD: egressrouters.network.operator.openshift.io - CRD: operatorpkis.network.operator.openshift.io - CRD: networks.operator.openshift.io These resources are deployed as static manifests by CVO but were not included in relatedObjects, causing 'oc adm inspect clusteroperator/network' to fail collecting them for debugging purposes. Changes: 1. Updated ClusterOperator manifest to include relatedObjects for CVO bootstrap 2. Updated operconfig controller to add these resources to the computed relatedObjects list, preventing deleteRelatedObjectsNotRendered() from removing them during reconciliation Without the controller change, the manifest-only fix would be overwritten at runtime when the controller recomputes and sets relatedObjects. Signed-off-by: Swati Mulje <smulje@redhat.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
@smulje: The following test 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. |
|
/test e2e-aws-ovn-upgrade |
There was a problem hiding this comment.
I understand that all resources belonging to https://github.com/openshift/cluster-network-operator/tree/master/manifests are not being tracked at relatedObjects of network Cluster operator CR. It is logical as files in the manifests directory are supposed to be deployed and reconciled by cluster-version-operator and not CNO. So, it also does not make sense to have those in relatedObjects of network CO.
did you check whether all objects under manifest directory of CNO should be in relatedObjects of network CO? or, we only care about the objects that you added in your PR? Did you have any conversation regarding this with anyone?
| include.release.openshift.io/ibm-cloud-managed: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| status: | ||
| relatedObjects: |
There was a problem hiding this comment.
Why you are adding here and also adding at operconfig_controller.go? We should never set status at the manifest I think.
3 participants
Summary
Adds missing cluster-scoped resources to the network ClusterOperator's
relatedObjectsfield to enableoc adm inspect clusteroperator/networkto collect all relevant resources for debugging.Associated Bug:
https://redhat.atlassian.net/browse/OCPBUGS-65488
Problem
While checking
oc adm inspect clusteroperatoroutput in CI, several cluster-scoped resources deployed via static manifests were missing from the network ClusterOperator'srelatedObjects:cluster-network-operatordefault-account-cluster-network-operatoregressrouters.network.operator.openshift.io,operatorpkis.network.operator.openshift.io,networks.operator.openshift.ioThis caused
oc adm inspectto fail collecting these resources, making debugging more difficult.Solution
Updated
manifests/0000_70_cluster-network-operator_05_clusteroperator.yamlto include these cluster-scoped resources in thestatus.relatedObjectsfield.Testing
Manual Verification
oc adm inspect clusteroperator/networkBefore Fix:
After Fix: