OCPSTRAT-3567: Replace console allow-all NetworkPolicy with scoped policies - #1227
OCPSTRAT-3567: Replace console allow-all NetworkPolicy with scoped policies#1227redhat-chai-bot wants to merge 3 commits into
Conversation
… explicit policies Replace the namespace-wide allow-all NetworkPolicy in the openshift-console namespace with four explicit, auditable policies: 1. default-deny: Deny all ingress and egress for all pods in the namespace. 2. allow-ingress-console-ui: Allow ingress from the OpenShift router (openshift-ingress namespace) on ports 8443 and 8444, and from the monitoring stack (openshift-monitoring namespace) on port 8443 for Prometheus metrics scraping. 3. allow-ingress-downloads: Allow ingress from the OpenShift router (openshift-ingress namespace) on port 8080 for CLI binary downloads. 4. allow-egress-console-ui: Allow all egress from console UI pods only. Broad egress is required because the console backend communicates with the Kubernetes API server (address varies per cluster), dynamically registered ConsolePlugin backend services (arbitrary namespaces/ports), external Helm chart repositories, and OAuth/OIDC endpoints. The downloads server pods are intentionally excluded and have no egress allowance under the default-deny policy. Namespace selectors use the well-known labels network.openshift.io/ policy-group set by the cluster-ingress-operator and cluster-monitoring- operator on the openshift-ingress and openshift-monitoring namespaces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@redhat-chai-bot: This pull request references OCPSTRAT-3567 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 initiative to target the "5.1.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: Team Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used🔀 Multi-repo context openshift/consoleLinked repositories findingsopenshift/console
WalkthroughThe console namespace changes from allow-all to default-deny behavior. New policies allow console UI ingress, console UI egress, and downloads ingress from defined namespace groups and ports. Tests validate selectors and ports. ChangesConsole network policy enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change scopes console namespace network access with default-deny policies and explicit workload allowances. No unresolved merge-readiness risk is identified. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@redhat-chai-bot Can you add some unit tests to check that the ports/selectors are validated against actual deployments? |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot 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 |
|
/test e2e-aws-console |
| explicitly allowed policies. Companion policies grant ingress | ||
| from the OpenShift router and monitoring stack, and egress for | ||
| the console UI pods. |
There was a problem hiding this comment.
| explicitly allowed policies. Companion policies grant ingress | |
| from the OpenShift router and monitoring stack, and egress for | |
| the console UI pods. | |
| explicitly allowed policies. |
I think we should drop the "Companion policies grant ingress..." sentence from this policy. The previous "...ensuring that pods only communicate through explicitly allowed policies" is a sufficient hint that other NetworkPolicies exist, and cluster-admins can go read the descriptions on those policies to figure out what they do. That way, we don't have to remember to come back and bump this description as other NetworkPolicies are added, removed, or otherwise evolved.
There was a problem hiding this comment.
Agreed. I’ll remove the companion-policy inventory from the default-deny description and keep the stable generic isolation description, so it won’t need follow-up edits as policies evolve.
AI-generated. Review for accuracy.
| "github.com/openshift/library-go/pkg/operator/resource/resourceread" | ||
| ) | ||
|
|
||
| func TestNetworkPolicyAllowRulesMatchWorkloadManifests(t *testing.T) { |
There was a problem hiding this comment.
I dunno if this test-coverage is worth it. If we flub one of the matches, won't the blocking e2e presubmits turn up things like "hey, I can't access the Console UI?" or "TargetDown from errors from the console metrics?" etc.?
There was a problem hiding this comment.
That’s a fair concern. The e2e presubmits are still the end-to-end guard, but this test is a cheap deterministic check that catches selector/target-port drift before a cluster run. It also covers the custom-route redirect and downloads paths that a normal console UI flow may not exercise. I’ll keep it as a focused manifest-contract test rather than relying on e2e alone.
AI-generated. Review for accuracy.
There was a problem hiding this comment.
The e2e presubmits are still the end-to-end guard, but this test is a cheap deterministic check that catches selector/target-port drift before a cluster run.
Selector/target-port drift seems unlikely, and I'm not sure there's any garuntee that unit test success gates the launching of the e2e runs, vs. those happening in parallel.
It also covers the custom-route redirect and downloads paths that a normal console UI flow may not exercise.
Seems like those paths should have some e2e test coverage too then, because "NetworkPolicy metadata matches!" isn't sufficient to confirm they work end to end.
There was a problem hiding this comment.
Agreed — this unit test does not gate or replace the e2e jobs, and it does not prove that the routes work end to end. Its narrower purpose is to validate the intended manifest contract early and cheaply; it is not a runtime-coverage claim. I won’t add e2e coverage or expand the egress design in this PR. Since the original review request asked for this exact ports/selectors check and the test is local/static with the full suite passing, I’m leaving it in place unless maintainers prefer to drop the contract check entirely.
AI-generated. Review for accuracy.
|
@redhat-chai-bot: The following test 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. |
Summary
Replace the namespace-wide
allow-allNetworkPolicy inopenshift-consolewith explicit policies that reduce unintended ingress while preserving required console backend egress.Validation
make verifymake test-unitTracking: OCPSTRAT-3567
AI-generated. Review for accuracy.
@spadgett requested in Slack thread
Summary by CodeRabbit