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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIn the ChangesRelated Objects Extension
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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
smulje
force-pushed
the
fix-OCPBUGS-65488
branch
2 times, most recently
from
c94e52e to
fad0e27
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/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.
smulje
force-pushed
the
fix-OCPBUGS-65488
branch
2 times, most recently
from
554c8ab to
d2c6769
Compare
| Name: "operatorpkis.network.operator.openshift.io", | ||
| }) | ||
|
|
||
| relatedObjects = append(relatedObjects, configv1.ObjectReference{ |
There was a problem hiding this comment.
we already capture this in the must-gather. is there any other reason to include it into relatedObjects?
| relatedObjects = append(relatedObjects, configv1.ObjectReference{ | ||
| Group: "apiextensions.k8s.io", | ||
| Resource: "customresourcedefinitions", | ||
| Name: "operatorpkis.network.operator.openshift.io", |
There was a problem hiding this comment.
Why do we need the CRD in relatedObjects ? we already have related operatorpkis added there.
$ oc get co network -oyaml|grep operatorpkis -3
- group: network.operator.openshift.io
name: ovn
namespace: openshift-ovn-kubernetes
resource: operatorpkis
- group: network.operator.openshift.io
name: signer
namespace: openshift-ovn-kubernetes
resource: operatorpkis
|
|
||
| relatedObjects = append(relatedObjects, configv1.ObjectReference{ | ||
| Group: "apiextensions.k8s.io", | ||
| Resource: "customresourcedefinitions", |
There was a problem hiding this comment.
Wy this CRD is required?
|
Hi @arghosh93 Thank you for the review. You're right about the CRDs. I've removed all three of CRD and kept just the ClusterRoleBindings. I compared the oc adm inspect clusteroperator/network output with what's actually in relatedObjects to verify this. For operatorpkis, like you mentioned, we already have the CR instances listed When inspect collects those CRs, it can access the CRD schema anyway, so adding the CRD separately doesn't help. Same thing for the networks CRD. The two ClusterRoleBindings are different though. I checked the actual inspect output and cluster-network-operator and default-account-cluster-network-operator are missing. These are cluster-scoped RBAC resources that don't get auto-collected . They're defined in the manifests but not being collected right now. |
Add cluster-network-operator and default-account-cluster-network-operator ClusterRoleBindings to relatedObjects for oc adm inspect collection. These cluster-scoped RBAC resources are not auto-collected and are currently missing from inspect output. They are essential for debugging RBAC/permission issues with the cluster-network-operator ServiceAccount. Fixes: OCPBUGS-65488 Signed-off-by: Swati Mulje <smulje@redhat.com>
smulje
force-pushed
the
fix-OCPBUGS-65488
branch
from
080e5dd to
cd2ec6b
Compare
|
@smulje: 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
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:
Summary by CodeRabbit