Skip to content

Conversation

@mattedallo
Copy link

Add read permissions (i.e. get, list, watch) for network-related resources to the cluster-reader ClusterRole aggregation. The resources are:

  • egressrouters.network.operator.openshift.io
  • network-attachment-definitions.k8s.cni.cncf.io
  • networks.operator.openshift.io

Fixes: OCPBUGS-35387

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Add read permissions (i.e. get, list, watch) for network-related resources to the
cluster-reader ClusterRole aggregation. The resources are:
- egressrouters.network.operator.openshift.io
- network-attachment-definitions.k8s.cni.cncf.io
- networks.operator.openshift.io

Fixes: OCPBUGS-35387
@mattedallo mattedallo force-pushed the add-network-rbac-cluster-reader branch from e8f6663 to 93ab506 Compare October 27, 2025 16:33
@mattedallo mattedallo marked this pull request as ready for review October 27, 2025 16:39
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2025
@mattedallo
Copy link
Author

/retest

@mattedallo mattedallo changed the title rbac: Add network resources to cluster-reader role OCPBUGS-35387: rbac: Add network resources to cluster-reader role Oct 29, 2025
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 29, 2025
@openshift-ci-robot
Copy link
Contributor

@mattedallo: This pull request references Jira Issue OCPBUGS-35387, which is invalid:

  • expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Add read permissions (i.e. get, list, watch) for network-related resources to the cluster-reader ClusterRole aggregation. The resources are:

  • egressrouters.network.operator.openshift.io
  • network-attachment-definitions.k8s.cni.cncf.io
  • networks.operator.openshift.io

Fixes: OCPBUGS-35387

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.

@mattedallo
Copy link
Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 29, 2025
@openshift-ci-robot
Copy link
Contributor

@mattedallo: This pull request references Jira Issue OCPBUGS-35387, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.21.0) matches configured target version for branch (4.21.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

/jira refresh

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.

@mattedallo
Copy link
Author

/retest-required

@pliurh
Copy link
Contributor

pliurh commented Oct 30, 2025

@mattedallo did you check if the cluster-reader role gets the permissions in an OCP cluster with the patch?

@mattedallo
Copy link
Author

Hi @pliurh , Yes I tested it.
Let me paste the tests I made here and in the bug:

$ oc get clusterversion
NAME      VERSION                              AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.19.0-0.nightly-2025-10-22-141259   True        False         4m58s   Cluster version is 4.19.0-0.nightly-2025-10-22-141259


$ oc new-project test-project
Now using project "test-project" on server "https://api.ci-ln-9wmpp2t-76ef8.origin-ci-int-aws.dev.rhcloud.com:6443".

$ oc create sa test-user -n test-project
serviceaccount/test-user created


$ oc auth can-i list network-attachment-definitions.k8s.cni.cncf.io --as=system:serviceaccount:test-project:test-user
no

$ oc auth can-i list egressrouters.network.operator.openshift.io --as=system:serviceaccount:test-project:test-user
no

$ oc auth can-i list networks.operator.openshift.io --as=system:serviceaccount:test-project:test-user
Warning: resource 'networks' is not namespace scoped in group 'operator.openshift.io'

no


$ oc adm policy add-cluster-role-to-user cluster-reader -z test-user -n test-project
clusterrole.rbac.authorization.k8s.io/cluster-reader added: "test-user"

$ oc auth can-i list network-attachment-definitions.k8s.cni.cncf.io --as=system:serviceaccount:test-project:test-user
yes

$ oc auth can-i list egressrouters.network.operator.openshift.io --as=system:serviceaccount:test-project:test-user
yes

$ oc auth can-i list networks.operator.openshift.io --as=system:serviceaccount:test-project:test-user
Warning: resource 'networks' is not namespace scoped in group 'operator.openshift.io'

yes

@mattedallo
Copy link
Author

/retest

@arkadeepsen
Copy link
Member

/lgtm

/cc @kyrtapz

@openshift-ci openshift-ci bot requested a review from kyrtapz November 4, 2025 14:23
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

Two new read-only RBAC rules were appended to the OVN-Kubernetes cluster-reader ClusterRole: one for egressrouters (apiGroup network.operator.openshift.io) and one for network-attachment-definitions (apiGroup k8s.cni.cncf.io), both granting get, list, and watch.

Changes

Cohort / File(s) Summary
RBAC Cluster-Reader Configuration
bindata/network/ovn-kubernetes/common/007-rbac-cluster-reader.yaml
Added two read-only RBAC rules: egressrouters in network.operator.openshift.io and network-attachment-definitions in k8s.cni.cncf.io; both grant get, list, and watch.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Additive, localized change in a single manifest file.
  • Review focus: ensure no duplicate rules, correct apiGroup/resource names, and YAML formatting/indentation.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 93ab506 and a981f15.

📒 Files selected for processing (1)
  • bindata/network/ovn-kubernetes/common/007-rbac-cluster-reader.yaml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • bindata/network/ovn-kubernetes/common/007-rbac-cluster-reader.yaml
🔇 Additional comments (2)
bindata/network/ovn-kubernetes/common/007-rbac-cluster-reader.yaml (2)

40-46: Verify this aligns with resolved architectural concerns.

The past review comments show explicit feedback rejecting the addition of operator.openshift.io resources to cluster-reader. Reviewer pliurh stated: "I don't think we shall break the pattern," given that no other operator.openshift.io resources are exposed to cluster-reader across the OpenShift platform.

This rule adds egressrouters.network.operator.openshift.io to cluster-reader, which would be the first operator.openshift.io resource exposed this way. Clarify whether this change intentionally overrides that architectural guidance or if an alternative approach was agreed upon.


47-53: Verify consistency with PR description.

The PR description states three resources are being added: egressrouters.network.operator.openshift.io, network-attachment-definitions.k8s.cni.cncf.io, and networks.operator.openshift.io. However, the YAML only contains two rules; the networks.operator.openshift.io resource is missing.

Confirm whether this omission is intentional (due to the architectural concerns flagged in past reviews) or if a rule for networks should be added.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example:

"Create a concise high-level summary as a bullet-point list. Then include a Markdown table showing lines added and removed by each contributing author."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Comment @coderabbitai help to get the list of available commands and usage tips.

@mattedallo
Copy link
Author

@kyrtapz when you have time please take a look. Thanks

- get
- list
- watch
- apiGroups: ["operator.openshift.io"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am ok with the other two I am not really sure what is the general approach for operator.openshift.io CRDs across OCP.
From what I've seen there are no other operator CRDs added to the cluster-reader.
@pliurh @mattedallo were you able to find that out?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyrtapz Thank you for raising this concern. I've checked the RBAC patterns across OpenShift operators, and I indeed it seems no operator.openshift.io resources are currently exposed to cluster-reader across the entire platform. Our proposed change would be the first exception to this pattern.

Investigation Findings / My understanding

Listing my findings/understanding below, most of them might be trivial for a seasoned Red Hat engineer but helps me validate if my understanding is correct.

Pattern Confirmed Across All Operators

I verified that zero out of 21 operator.openshift.io/v1 resources have aggregate-to-cluster-reader or aggregate-to-view labels.

For comparison, here are operators that do have cluster-reader access, but for different API groups (NOT operator.openshift.io):

  • machine-config-operator: Exposes machineconfiguration.openshift.io and config.openshift.io (source)
  • cluster-samples-operator: Exposes samples.operator.openshift.io (different API group) (source)
  • cluster-config-operator: Exposes only config.openshift.io (source)
  • machine-api-operator: Exposes machine.openshift.io (source)

The Current Architectural Pattern

The consistent pattern across OpenShift is:

  • config.openshift.io → Exposed to cluster-reader (user-facing, high-level configuration)
  • Custom operator API groups (e.g., samples.operator.openshift.io, machineconfiguration.openshift.io) → Exposed when appropriate
  • operator.openshift.io → NOT exposed to cluster-reader (implementation details, cluster-admin only)

Security Considerations

On the security side, looking at the operator.openshift.io/Network CRD data that would become readable does not seem harmful:

  • ✅ No credential/secret fields in the API definition
  • ✅ Mostly configuration data (CIDRs, MTU, ports, feature flags)
  • ⚠️ RawCNIConfig is freeform JSON (could be misused, but user error not design flaw AFAIU)
  • ⚠️ Infrastructure topology info (collectors, syslog destinations)

Still, that doesn't mean it won't include something that shouldn't be readable by a cluster-reader in the future, especially if the architectural assumption is that this is an internal, non-customer-visible CRD.

How troubleshoot has been made possible so far?

Mostly making use of must-gather

Questions

As someone new to the team, I want to make sure I understand the tradeoffs here:

  1. Breaking the pattern: Is the debugging benefit sufficient to justify being the first and only operator.openshift.io resource exposed to cluster-reader across the platform?

  2. Security review: Should we assess the risk of exposing implementation details like RawCNIConfig and in general braking the current pattern?

  3. Alternatives: Could we instead:

    • Enhance config.openshift.io/Network.Status with additional troubleshooting fields?
    • Create a dedicated troubleshooting API?

I'm happy to pursue whichever direction the team thinks is best.

@pliurh please let me know your thoughts as you might have additional context on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we shall break the pattern.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyrtapz I removed the operator.openshift.io CRD based on the discussion above.
PTAL
Thanks

…uster-reader

Signed-off-by: Matteo Dallaglio <mdallagl@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2025
Remove networks.operator.openshift.io resources from the cluster-reader
ClusterRole aggregation to maintain consistency with the platform-wide
architectural pattern.

After investigation, no operator.openshift.io resources are currently
exposed to cluster-reader across any OpenShift operators. The consistent
pattern is:
- config.openshift.io resources ARE exposed (user-facing configuration)
- operator.openshift.io resources are NOT exposed (implementation details)

While the Network resource doesn't contain sensitive data, exposing it
would make CNO the first and only exception to this pattern. The team
decided to preserve architectural consistency rather than break this
established pattern.

Related: OCPBUGS-35387
Signed-off-by: Matteo Dallaglio <mdallagl@redhat.com>
@mattedallo
Copy link
Author

/retest

1 similar comment
@mattedallo
Copy link
Author

/retest

@pliurh
Copy link
Contributor

pliurh commented Nov 19, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkadeepsen, mattedallo, pliurh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 19, 2025

@mattedallo: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-scos-e2e-aws-ovn 93ab506 link false /test okd-scos-e2e-aws-ovn
ci/prow/security a981f15 link false /test security
ci/prow/e2e-aws-ovn-upgrade a981f15 link true /test e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn-windows a981f15 link true /test e2e-aws-ovn-windows
ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw a981f15 link true /test e2e-metal-ipi-ovn-dualstack-bgp-local-gw

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants