-
Notifications
You must be signed in to change notification settings - Fork 115
CNTRLPLANE-2610: Create network policies for AUTH components #825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@liouk: This pull request references CNTRLPLANE-2610 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 epic to target the "4.22.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. |
WalkthroughThis pull request introduces Kubernetes NetworkPolicy resources for OpenShift authentication components across multiple namespaces. It adds default-deny-all policies and component-specific traffic rules for oauth-apiserver, oauth-openshift, and authentication-operator, along with code registration and corresponding test output files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
|
/jira refresh |
|
@liouk: This pull request references CNTRLPLANE-2610 which is a valid jira issue. 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. |
9711c7b to
d058feb
Compare
|
@liouk: This pull request references CNTRLPLANE-2610 which is a valid jira issue. 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
d058feb to
97ba0b8
Compare
|
@liouk: This pull request references CNTRLPLANE-2610 which is a valid jira issue. 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. |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
/retest |
|
I'd like a review from a member of each of the auth and network policy feature teams -- holding until we get both. Holding PR until we get:
/hold |
|
@liouk: This pull request references CNTRLPLANE-2610 which is a valid jira issue. 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. |
| libraryoutputresources.ExactResource("networking.k8s.io", "v1", "networkpolicies", "openshift-authentication-operator", "authentication-operator-networkpolicy"), | ||
| libraryoutputresources.ExactResource("networking.k8s.io", "v1", "networkpolicies", "openshift-authentication-operator", "default-deny-all"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does MOM need to also know the manifests for deploying the cluster-authentication-operator itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually good point, I don't believe OM needs these operator manifests.
97ba0b8 to
2321ed0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bindata/oauth-openshift/networkpolicy_oauth-server.yaml`:
- Around line 44-68: The NetworkPolicy in networkpolicy_oauth-server.yaml
currently uses a wide namespaceSelector: {} allowing ingress to port 6443 from
any namespace and an egress rule with only ports: - protocol: TCP (no `to`)
permitting all TCP egress; tighten or document this: replace namespaceSelector:
{} with a scoped namespaceSelector/podSelector or ipBlock that targets known
oauth-proxy/sidecar namespaces or pods (reference the ingress block targeting
port 6443), and restrict the egress rule (the egress entry listing ports with
protocol: TCP) by adding specific `to:` destinations (podSelector,
namespaceSelector or ipBlock) for kube-apiserver and configured IDPs;
alternatively, if the broad scope is intentional, add an explicit comment in the
manifest near the ingress (port 6443) and the TCP-only egress rule explaining
the rationale and approved clients/endpoints.
🧹 Nitpick comments (1)
bindata/oauth-apiserver/networkpolicy_oauth-apiserver.yaml (1)
77-80: Egress rule allows all TCP ports to any destination.This rule permits unrestricted TCP egress, which is quite permissive. The comment indicates this is for kube-apiserver communication, but kube-apiserver typically runs on port 6443. Consider whether this could be tightened to specific ports (e.g., 6443 for API server) to reduce attack surface, or document why unrestricted TCP is required.
everettraven
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM in general.
We should probably run payload jobs for a sanity check that this won't cause component readiness issues and payload build failures.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, liouk 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 |
Indeed -- I intend to get this reviewed and tested by QE first before running a payload job, to avoid having to repeat it unnecessarily. |
|
lgtm as well! |
2321ed0 to
ec97772
Compare
|
New changes are detected. LGTM label has been removed. |
|
Pushed changes to better align with the "Egress to the APIServer" guidance. |
| annotations: | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| include.release.openshift.io/ibm-cloud-managed: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cesar warned against having these applied in hypershift since they already do their own network policies in the hosted control planes. I would suggest removing this include.release.openshift.io/ibm-cloud-managed: "true".
| annotations: | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| include.release.openshift.io/ibm-cloud-managed: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same: #825 (comment)
37dd6ce to
af9a504
Compare
af9a504 to
e9f60f8
Compare
|
Pushed fixes as per comments from @dusk125 and also more alignment with guidance (allow all ingress to metrics, not just prometheus pods). |
This PR adds network policies to the authentication operator, oauth-server and oauth-apiserver. For each component, there are two policies:
All known and required connections must be reflected to respective allow rules.
Note that, in case of pods that require traffic to/from hostNetwork pods (such as the kube-apiserver), we need to allow all ingress/egress TCP traffic; NetworkPolicies do not affect pods on hostNetwork, but we still need a rule to allow ingress/egress from/to them.
In some cases there might be some overlap in the policy rules, but this is intentional for the sake of documentation/future reference.