ROSAENG-8224: feat(operator): use MC machine network CIDR to block HCP egress to management KAS#8689
ROSAENG-8224: feat(operator): use MC machine network CIDR to block HCP egress to management KAS#8689Ajpantuso wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
|
@Ajpantuso: This pull request references ROSAENG-8224 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis pull request adds a new UseMCMachineNetworkForKASBlock option wired from installer CLI and operator startup into HostedClusterReconciler. The reconciler now precomputes a kasBlock []string per reconcile: when enabled it reads kube-system/cluster-config-v1 install-config to return management cluster machineNetwork CIDRs, otherwise it falls back to /32 CIDRs derived from KAS endpoints. NetworkPolicy builders and tests were updated to accept and consume the kasBlock exceptions list instead of Endpoints objects. Sequence Diagram(s)sequenceDiagram
participant InstallerCLI
participant HyperShiftOperatorDeployment
participant HostedClusterReconciler
participant ManagementConfigMap
participant KubernetesEndpoints
participant NetworkPolicy
InstallerCLI->>HyperShiftOperatorDeployment: set --use-mc-machine-network-for-kas-block=%t
HyperShiftOperatorDeployment->>HostedClusterReconciler: operator receives flag
HostedClusterReconciler->>ManagementConfigMap: GET kube-system/cluster-config-v1 (if flag)
ManagementConfigMap->>HostedClusterReconciler: return install-config YAML (machineNetwork CIDRs)
HostedClusterReconciler->>KubernetesEndpoints: GET Endpoints (fallback)
KubernetesEndpoints->>HostedClusterReconciler: return endpoint IPs
HostedClusterReconciler->>NetworkPolicy: reconcile with IPBlock.Except using machineNetwork CIDRs or /32 endpoint CIDRs + pod CIDRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ajpantuso 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 |
openshift-ci
Bot
added
area/hypershift-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/network_policies_test.go (1)
1697-1707: ⚡ Quick winAssert management pod CIDR is still included in
Except.Line 1701 validates the feature-toggle CIDRs, but it doesn’t confirm the existing management cluster pod CIDR remains present. Add one assertion so this test also catches regressions where new KAS block CIDRs replace (instead of extend) existing exceptions.
Suggested test hardening
exceptCIDRs := collectExceptCIDRs(privateRouterPolicy) + g.Expect(exceptCIDRs).To(ContainElement("10.128.0.0/14"), "private-router Except list should retain management cluster pod CIDR") for _, cidr := range tc.expectedExceptCIDRs { g.Expect(exceptCIDRs).To(ContainElement(cidr), "private-router Except list should contain %s", cidr) }Based on learnings: "Unit test any code changes and additions and include e2e tests when changes impact consumer behaviour."
🤖 Prompt for 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. In `@hypershift-operator/controllers/hostedcluster/network_policies_test.go` around lines 1697 - 1707, The test currently checks feature-toggle CIDRs but doesn't assert the management cluster pod CIDR is still present; after collecting exceptCIDRs (from collectExceptCIDRs(privateRouterPolicy)) add an assertion that the known management pod CIDR (e.g., managementPodCIDR or the variable used elsewhere in this test file) is contained in exceptCIDRs using g.Expect(exceptCIDRs).To(ContainElement(managementPodCIDR)) to catch regressions where exceptions get replaced instead of extended; place this assertion near the existing checks for tc.expectedExceptCIDRs and tc.expectCIDRNotPresent so it runs for each test case referencing createdNetworkPolicies["private-router"] and privateRouterPolicy.
🤖 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.
Nitpick comments:
In `@hypershift-operator/controllers/hostedcluster/network_policies_test.go`:
- Around line 1697-1707: The test currently checks feature-toggle CIDRs but
doesn't assert the management cluster pod CIDR is still present; after
collecting exceptCIDRs (from collectExceptCIDRs(privateRouterPolicy)) add an
assertion that the known management pod CIDR (e.g., managementPodCIDR or the
variable used elsewhere in this test file) is contained in exceptCIDRs using
g.Expect(exceptCIDRs).To(ContainElement(managementPodCIDR)) to catch regressions
where exceptions get replaced instead of extended; place this assertion near the
existing checks for tc.expectedExceptCIDRs and tc.expectCIDRNotPresent so it
runs for each test case referencing createdNetworkPolicies["private-router"] and
privateRouterPolicy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eadd8666-ca8d-475c-8303-c5739a6cf3c8
📒 Files selected for processing (6)
cmd/install/assets/hypershift_operator.gocmd/install/install.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/network_policies.gohypershift-operator/controllers/hostedcluster/network_policies_test.gohypershift-operator/main.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8689 +/- ##
==========================================
+ Coverage 41.43% 41.45% +0.01%
==========================================
Files 756 756
Lines 93647 93687 +40
==========================================
+ Hits 38802 38837 +35
- Misses 52124 52128 +4
- Partials 2721 2722 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Ajpantuso
force-pushed
the
apantuso/rosaeng-8224
branch
2 times, most recently
from
3b45a90 to
8df0806
Compare
openshift-ci
Bot
added
area/platform/azure
Ajpantuso
force-pushed
the
apantuso/rosaeng-8224
branch
from
8df0806 to
2827d9d
Compare
|
I now have the complete picture. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe gitlint CI check failed because the commit Root CauseThe commit message for SHA There is no body text following the title. Gitlint rule B6 (body-is-missing) requires that every commit (except merge commits, which are ignored by default) includes a body message — i.e., descriptive text separated from the title by a blank line. The repository's The error Notably, the PR description on GitHub does contain a well-written explanation of the change, but this text was not included in the git commit message body itself. RecommendationsAmend the commit to add a body describing what the change does and why. For example: git commit --amendThen edit the message to include a body after the title, separated by a blank line: After amending, force-push the branch to re-trigger CI: git push --force-with-leaseEvidence
|
2 participants
What this PR does / why we need it:
Adds a
--use-mc-machine-network-for-kas-blockflag to the HyperShift Operator.When enabled, the operator reads the management cluster's machine network
CIDR(s) from
kube-system/cluster-config-v1(theinstall-configkey) anduses them as the
Exceptentries in the egress IPBlock rules of theprivate-routerandmanagement-kasNetworkPolicies, instead of theper-pod
/32CIDRs derived from thedefault/kubernetesEndpoints object.If the ConfigMap is absent or unparseable, the operator falls back to the
existing
kasEndpointsToCIDRs()behavior and logs a warning.The flag is wired end-to-end:
hypershift install --use-mc-machine-network-for-kas-blocksets it on the operator Deployment.
This PR will remain in draft until live testing is completed to ensure no regressions.
Which issue(s) this PR fixes:
Fixes ROSAENG-8224
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Tests