OCPBUGS-87018: Revert conditional deletion of openshift-ingress NetworkPolicy - #8662
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
jira/severity-critical
|
@csrwng: This pull request references Jira Issue OCPBUGS-87018, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
📝 WalkthroughWalkthroughThis pull request refactors how the hosted cluster reconciler manages the Sequence Diagram(s)sequenceDiagram
participant HostedClusterReconciler
participant createOrUpdate
participant KubernetesAPI
participant OpenshiftIngressNetworkPolicy
HostedClusterReconciler->>createOrUpdate: call reconcileOpenshiftIngressNetworkPolicy
createOrUpdate->>KubernetesAPI: apply NetworkPolicy resource
KubernetesAPI-->>createOrUpdate: success/failure
createOrUpdate-->>HostedClusterReconciler: result (wrapped on failure)
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 |
openshift-ci
Bot
added
area/hypershift-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@csrwng: This pull request references Jira Issue OCPBUGS-87018, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
removed
the
jira/invalid-bug
![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)
269-277: ⚡ Quick winAssert the reconciled end state, not just the
createOrUpdatecall.This no longer verifies the regression the PR is fixing. The test passes as soon as
reconcileNetworkPoliciesinvokes the stub, even if a future change deletes the policy later or fails to preserve a pre-existing object. Please keep one case that seedsopenshift-ingressin the fake client and then assert it still exists after reconcile.Also applies to: 457-459
🤖 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 269 - 277, The test currently only asserts that the createOrUpdate stub was invoked, which misses regressions where reconcileNetworkPolicies later deletes or fails to preserve an existing openshift-ingress NetworkPolicy; update the test cases to include at least one scenario that seeds the fake client with a pre-existing NetworkPolicy named "openshift-ingress", call reconcileNetworkPolicies, then fetch the NetworkPolicy from the fake client and assert it still exists and has expected fields (preserve name/namespace/important labels/ports) instead of only asserting the createOrUpdate stub was called; locate the reconciliation helper reconcileNetworkPolicies and the test table in network_policies_test.go to add the seeded object and the post-reconcile existence assertion.
🤖 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 269-277: The test currently only asserts that the createOrUpdate
stub was invoked, which misses regressions where reconcileNetworkPolicies later
deletes or fails to preserve an existing openshift-ingress NetworkPolicy; update
the test cases to include at least one scenario that seeds the fake client with
a pre-existing NetworkPolicy named "openshift-ingress", call
reconcileNetworkPolicies, then fetch the NetworkPolicy from the fake client and
assert it still exists and has expected fields (preserve
name/namespace/important labels/ports) instead of only asserting the
createOrUpdate stub was called; locate the reconciliation helper
reconcileNetworkPolicies and the test table in network_policies_test.go to add
the seeded object and the post-reconcile existence assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6444553f-ed81-4b9b-8034-5f9ad84e2cce
📒 Files selected for processing (2)
hypershift-operator/controllers/hostedcluster/network_policies.gohypershift-operator/controllers/hostedcluster/network_policies_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8662 +/- ##
==========================================
- Coverage 41.41% 41.41% -0.01%
==========================================
Files 756 756
Lines 93623 93612 -11
==========================================
- Hits 38777 38772 -5
+ Misses 52120 52116 -4
+ Partials 2726 2724 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
…k policy based on route labeling" This reverts commit 27f49d7. The original change conditionally deleted the openshift-ingress NetworkPolicy when LabelHCPRoutes was true (AWS/GCP private clusters, clusters with dedicated KAS DNS, ARO HCP). While the policy is functionally unnecessary in those cases, its absence triggers an OVN-Kubernetes bug during management cluster kube-apiserver revision rollouts. When the MC KAS rolls out, ovnkube-controller briefly loses its API connection and re-syncs. During this re-sync, it writes OVN NorthDB logical flows that reference port groups before those port groups are fully restored. ovn-controller on each node then fails to parse the flow match rules: lflow|WARN|error parsing match "... inport == @a15503844857551241179": Syntax error at `@a15503844857551241179' expecting port group name. These broken flow rules are the allow rules for NodePort/LoadBalancer ingress traffic. Without them, the namespace default-deny ingress ACL drops all NLB traffic to HCP router pods on port 8443. This causes a complete API blackout for all hosted clusters on the management cluster, lasting 20-30+ minutes until router pods are manually deleted. ACL logging from the must-gather confirms the mechanism — 11,110 drops across all 6 router pods on 3 hosted clusters, 100% targeting router IPs on port 8443, correlated exactly with the ovn-controller lflow errors during the KAS rollout window. The presence of the openshift-ingress NetworkPolicy (which uses podSelector: {} matching all pods) changes the OVN port group structure in a way that avoids this race. Manual testing confirmed that re-applying the policy prevents the outage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2d4b968 to
00101fe
Compare
|
@csrwng: This pull request references Jira Issue OCPBUGS-87018, which is valid. 3 validation(s) were run on this bug
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. |
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aks
e2e-aws
|
|
For posterity - For understanding the rationale behind why this "unrelated" change solve the problem see outcome of https://redhat.atlassian.net/browse/OCPBUGS-87020 |
|
/test e2e-aks-4-22 |
|
This confirms the flow. The The node did boot and became ready (line 1716: "Successfully waited for 1 nodes to become ready"), which means the ignition payload was served. But the annotation on the token secret that records this fact was never applied — this is a best-effort annotation update in the ignition server (start.go:280) that can silently fail. This is a pre-existing flake unrelated to the NetworkPolicy changes in PR #8662. The PR only modifies the Now I have everything I need to produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe failure is a known flaky behavior in the How
Why it failed:
Why this is unrelated to PR #8662:
Recommendations
Evidence
|
|
/retest-required |
|
/verified by @csrwng |
openshift-ci-robot
added
the
verified
|
@csrwng: This PR has been marked as verified by 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. |
|
@csrwng: all tests passed! 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. |
|
@csrwng: Jira Issue Verification Checks: Jira Issue OCPBUGS-87018 Jira Issue OCPBUGS-87018 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-06-06-100407 |
…gress NetworkPolicy Reverts the revert PR openshift#8662 to re-apply the original PR openshift#7872 behavior: conditionally create or delete the openshift-ingress NetworkPolicy based on whether routes are labeled for the HCP router (LabelHCPRoutes). The OVN-Kubernetes port group race that prompted the revert is being addressed via PR openshift#8689 (--hcp-egress-block-cidrs flag), making the blanket always-create workaround unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6 participants
Summary
openshift-ingressNetworkPolicy whenLabelHCPRouteswas trueProblem
When the management cluster kube-apiserver undergoes a revision rollout (cert rotation, config change, or forced redeployment), all hosted cluster private-router NLBs experience sustained traffic blackouts. The issue was introduced in HO v0.1.75 and does not self-heal — manual deletion of HCP
routerpods is required to restore traffic.Root Cause
During a MC KAS rollout,
ovnkube-controllerbriefly loses its API connection and re-syncs. During this re-sync, it writes OVN NorthDB logical flows that reference port groups before those port groups are fully restored.ovn-controllerfails to parse the flow match rules:These broken flows are the allow rules for NodePort/LoadBalancer ingress traffic. Without them, the namespace default-deny ingress ACL drops all NLB traffic to HCP router pods on port 8443.
Evidence from must-gather
ACL logging confirmed the mechanism:
ovn-controllerlflow parse errors during the KAS rollout windowWhy re-adding the policy helps
The exact mechanism by which the
openshift-ingressNetworkPolicy prevents the race is not yet fully understood — other policies in the namespace also usepodSelector: {}, so it is not simply the presence of a broad port group. It may be related to the cross-namespacenamespaceSelectorreference (matchingnetwork.openshift.io/policy-group: ingress) or to reducing the amount of OVN NorthDB churn during re-sync. Regardless, manual testing confirmed that re-applying the policy prevents the outage, and removing it reliably reproduces it.The underlying OVN-Kubernetes bug (non-atomic port group + logical flow updates during re-sync) should be tracked and fixed separately.
Test plan
openshift-ingressNetworkPolicy is present in HCP namespaces for private/PublicAndPrivate clusters🤖 Generated with Claude Code