Skip to content

CORENET-7114: Add HostedCluster RBAC permissions for CNO - #8826

Closed
tpantelis wants to merge 1 commit into
openshift:mainfrom
tpantelis:update_cno_rbac
Closed

CORENET-7114: Add HostedCluster RBAC permissions for CNO#8826
tpantelis wants to merge 1 commit into
openshift:mainfrom
tpantelis:update_cno_rbac

Conversation

@tpantelis

@tpantelis tpantelis commented Jun 24, 2026

Copy link
Copy Markdown

CNO needs to read hostedclusters.hypershift.openshift.io resources to fetch TLS security profile configuration from the HostedCluster spec. This was added to both the static role.yaml (for when networkType
is OVNKubernetes) and the adaptRole() function (which replaces the rules otherwise).

Summary by CodeRabbit

  • Bug Fixes
    • Improved RBAC permissions for cluster networking components to allow reading hosted cluster information (get, list, watch) when using non-OVN Kubernetes network configurations.
    • This ensures networking features can reliably access the hosted cluster details they need to operate correctly.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 24, 2026
@openshift-ci-robot

openshift-ci-robot commented Jun 24, 2026

Copy link
Copy Markdown

@tpantelis: This pull request references CORENET-7114 which is a valid jira issue.

Details

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 openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The cluster-network-operator RBAC definition is updated in both the static asset and the generated adaptRole path to allow get, list, and watch access to the hostedclusters resource.

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding HostedCluster RBAC permissions for the cluster-network-operator.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PR only updates RBAC YAML/Go; no Ginkgo test titles or test files were changed, so the naming rule is not applicable.
Test Structure And Quality ✅ Passed No Ginkgo test code was changed; this PR only updates RBAC YAML and adaptRole RBAC rules.
Topology-Aware Scheduling Compatibility ✅ Passed Changed files only add HostedCluster RBAC and TLS flag handling; no nodeSelector/affinity/spread/PDB/replica scheduling constraints were introduced.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed Only RBAC YAML/Go changed; no new Ginkgo tests or network calls were added, so IPv4/disconnected compatibility isn’t implicated.
No-Weak-Crypto ✅ Passed The PR only changes RBAC rules in Go/YAML; no weak crypto, custom crypto, or secret/token comparisons are present.
Container-Privileges ✅ Passed Diff only adds RBAC for hostedclusters and TLS args; no added privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation settings in manifests.
No-Sensitive-Data-In-Logs ✅ Passed The patch only adds HostedCluster RBAC rules; no log statements or sensitive-data strings appear in the changed files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@openshift-ci openshift-ci Bot added the area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release label Jun 24, 2026
@openshift-ci

openshift-ci Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tpantelis
Once this PR has been reviewed and has the lgtm label, please assign enxebre for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.58%. Comparing base (438c61f) to head (30d6172).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ator/controllers/hostedcontrolplane/v2/cno/rbac.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8826      +/-   ##
==========================================
- Coverage   42.59%   42.58%   -0.01%     
==========================================
  Files         768      768              
  Lines       95359    95370      +11     
==========================================
  Hits        40617    40617              
- Misses      51934    51945      +11     
  Partials     2808     2808              
Files with missing lines Coverage Δ
...ator/controllers/hostedcontrolplane/v2/cno/rbac.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

Flag Coverage Δ
cmd-support 35.46% <ø> (ø)
cpo-hostedcontrolplane 44.81% <0.00%> (-0.03%) ⬇️
cpo-other 44.94% <ø> (ø)
hypershift-operator 53.05% <ø> (ø)
other 31.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

CNO needs to read hostedclusters.hypershift.openshift.io resources
to fetch TLS security profile configuration from the HostedCluster
spec. This was added to both the static role.yaml (for when networkType
is OVNKubernetes) and the dynamic adaptRole() function (which replaces
the rules otherwise).

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
@openshift-ci

openshift-ci Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@tpantelis: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@hypershift-jira-solve-ci

hypershift-jira-solve-ci Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

I have all the evidence needed. Here is the complete analysis:

Test Failure Analysis Complete

Job Information

  • Prow Job: codecov/patch (GitHub Actions / Codecov integration, not a Prow CI job)
  • Build ID: Check Run 83259660590
  • PR: #8826CORENET-7114: Add HostedCluster RBAC permissions for CNO
  • Branch: update_cno_rbacmain

Test Failure Analysis

Error

Patch coverage is 0% with 11 lines in your changes missing coverage. Target: 42.59%.
File: control-plane-operator/controllers/hostedcontrolplane/v2/cno/rbac.go — 0.00% of diff hit

Summary

The codecov/patch check failed because all 11 new Go lines added to rbac.go (the hostedclusters RBAC rule in the adaptRole function) have zero unit test coverage. This is not a product bug or a test regression — it is a code coverage gap. The entire adaptRole function already had 0% coverage before this PR; the new lines simply inherited that pre-existing gap. The function is indirectly validated via YAML fixture comparison tests (zz_fixture_TestControlPlaneComponents_cluster_network_operator_role.yaml across 5 platform variants), but those fixtures are YAML data files that do not count as Go code coverage. No Go unit test directly invokes adaptRole(). Notably, codecov/patch is typically informational and not a merge-blocking required status check in the openshift/hypershift repository — PRs can merge despite this failure.

Root Cause

The codecov/patch check enforces that new/modified Go lines in a PR are exercised by unit tests. The PR adds 11 lines to the adaptRole() function in control-plane-operator/controllers/hostedcontrolplane/v2/cno/rbac.go, appending a new RBAC PolicyRule granting CNO get/list/watch on hostedclusters resources.

The root cause of the 0% patch coverage is that no Go unit test exists that calls adaptRole(). The only test file in the package (component_test.go) tests a different function (platformHasCloudNetworkConfigController). The adaptRole function is registered as a manifest adapter in NewComponent() via .WithManifestAdapter("role.yaml", component.WithAdaptFunction(adaptRole)), and its output is validated by YAML fixture comparison tests (the 5 zz_fixture_*.yaml files updated in this PR). However, the Go coverage tooling does not trace execution through the fixture-based test framework back to the adaptRole function — it requires a direct Go function call path.

Since the function had 0% coverage before this PR, there was no way for the 11 added lines to achieve coverage without also adding a dedicated unit test for adaptRole. This is a pre-existing coverage gap, not something introduced by this PR.

Recommendations
  1. This check is likely non-blocking. Verify with the repo maintainers, but codecov/patch is typically informational in openshift/hypershift. The PR should be mergeable via Prow (/lgtm + /approve) regardless of this Codecov status.

  2. Optional: Add a unit test for adaptRole to eliminate the coverage gap. A minimal test would:

    func TestAdaptRole(t *testing.T) {
        role := &rbacv1.Role{}
        cpContext := component.WorkloadContext{
            HCP: &hyperv1.HostedControlPlane{
                Spec: hyperv1.HostedControlPlaneSpec{
                    Networking: hyperv1.ClusterNetworking{
                        NetworkType: hyperv1.Other, // non-OVN to exercise the rules path
                    },
                },
            },
        }
        err := adaptRole(cpContext, role)
        g := NewWithT(t)
        g.Expect(err).ToNot(HaveOccurred())
        g.Expect(role.Rules).To(ContainElement(/* hostedclusters rule */))
    }
  3. No product risk. The 5 updated YAML fixture files confirm the role manifest output is correct across all platform variants (default, AROSwift, GCP, IBMCloud, TechPreviewNoUpgrade). The functional correctness of the RBAC change is validated — only the Go coverage instrumentation is missing.

Evidence
Evidence Detail
Failing check codecov/patch — 0% patch coverage (11 lines missing), target 42.59%
Affected file control-plane-operator/controllers/hostedcontrolplane/v2/cno/rbac.go — 11 new lines in adaptRole()
Pre-existing coverage rbac.go had 0% coverage before this PR — the file's overall coverage is 0.00% <0.00%> (ø) (no delta)
Existing test file component_test.go only tests platformHasCloudNetworkConfigController, not adaptRole
Fixture validation 5 YAML fixture files updated and passing (zz_fixture_TestControlPlaneComponents_cluster_network_operator_role.yaml across AROSwift, GCP, IBMCloud, TechPreviewNoUpgrade, default) — confirms functional correctness
Static asset v2/assets/cluster-network-operator/role.yaml also updated with matching RBAC rule (YAML files excluded from coverage per codecov.yml ignore: **/*.yaml)
Project coverage impact Overall project coverage dropped by -0.01% (42.59% → 42.58%) — 11 new misses on 95,370 total lines
Check type codecov/patch is a GitHub status check, not a Prow job — typically informational, not merge-blocking

@tpantelis

Copy link
Copy Markdown
Author

/cc @muraee @enxebre Can you review this? This is related to OCP 5.0 TLS profile compliance.

@enxebre

enxebre commented Jun 25, 2026

Copy link
Copy Markdown
Member

This is a namespaced role, nothing within the cp namespace can read HCs.
You probably want to read the hostecontrolplane and the role already has permissions for that https://github.com/openshift/hypershift/pull/8826/changes#diff-b048481108577eb0a812287c603ba2badf629cde351eb6cb821669fbc1c57742L47-R52

@tpantelis tpantelis closed this Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants