CNTRLPLANE-2845: add service-ca operator and workload network policies#324
Conversation
|
@liouk: This pull request references CNTRLPLANE-2845 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds four new Kubernetes NetworkPolicy manifests: two for the openshift-service-ca namespace and two for the openshift-service-ca-operator namespace, defining ingress (TCP 8443), egress (DNS to openshift-dns and catch-all/API access), and default-deny-all policies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
@liouk: This pull request references CNTRLPLANE-2845 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. |
|
@liouk: This pull request references CNTRLPLANE-2845 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
manifests/08_networkpolicy.yaml (1)
16-21: Ingress policy lacks explicit source restrictions despite documented design intentWhile the comment in lines 1-2 explains that
podSelector: {}is intentional for this core operator namespace, the ingress rule on port 8443 still lacks explicit source restrictions (nofromfield). Although metrics scraping is controlled via the ServiceMonitor and RBAC RoleBinding for prometheus-k8s in openshift-monitoring, adding explicit network-layer controls (namespace and pod selectors in thefromfield) would provide defense-in-depth. Consider restricting ingress to the openshift-monitoring namespace and prometheus-k8s pod to align with the intentional scraper configuration defined in the ServiceMonitor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/08_networkpolicy.yaml` around lines 16 - 21, The NetworkPolicy currently allows ingress to podSelector: {} on port 8443 with no `from` clause; add explicit source restrictions to enforce defense-in-depth by updating the ingress rule for port 8443 to include a `from` that limits traffic to the openshift-monitoring namespace and the prometheus-k8s pod (use a namespaceSelector matching metadata.name: openshift-monitoring and a podSelector matching labels used by prometheus-k8s), ensuring the ServiceMonitor and RBAC remain the primary controls while the NetworkPolicy explicitly restricts network-layer access to only the prometheus scraper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/0000_10_openshift_service-ca_01_networkpolicy.yaml`:
- Around line 36-37: The egress entry "egress: - {}" currently allows all
outbound traffic; update the NetworkPolicy to restrict egress to the Kubernetes
API server only by replacing the empty rule with an explicit rule that targets
the API server (e.g., an egress rule matching destination port 443/6443 and
either an IPBlock for the cluster API service CIDR or a peer that selects the
API server pods/namespace via namespaceSelector/podSelector); specifically,
remove the "egress: - {}" wildcard and add an egress rule that uses to: -
ipBlock: {cidr: <api-server-cidr>} ports: - protocol: TCP port: 443 (and 6443 if
used) or alternatively use to: - namespaceSelector: {matchLabels:
{kubernetes.io/metadata.name: kube-system}} + podSelector matching the
kube-apiserver label so only API-server egress is permitted.
In `@manifests/08_networkpolicy.yaml`:
- Around line 36-37: The egress rule currently uses the open matcher "- {}"
which permits all destinations; replace that with a restricted egress that
targets the Kubernetes API server using an explicit "to" entry and port (e.g.,
port 6443) or, if unrestricted egress is intended, update the comment to state
that traffic is intentionally allowed to all destinations. Locate the egress
rule with the "- {}" entry in the NetworkPolicy manifest and either replace it
with a "to" selector/ipBlock + port specification for the API server or change
the comment to reflect the open egress policy.
---
Nitpick comments:
In `@manifests/08_networkpolicy.yaml`:
- Around line 16-21: The NetworkPolicy currently allows ingress to podSelector:
{} on port 8443 with no `from` clause; add explicit source restrictions to
enforce defense-in-depth by updating the ingress rule for port 8443 to include a
`from` that limits traffic to the openshift-monitoring namespace and the
prometheus-k8s pod (use a namespaceSelector matching metadata.name:
openshift-monitoring and a podSelector matching labels used by prometheus-k8s),
ensuring the ServiceMonitor and RBAC remain the primary controls while the
NetworkPolicy explicitly restricts network-layer access to only the prometheus
scraper.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
manifests/0000_10_openshift_service-ca_01_networkpolicy.yamlmanifests/08_networkpolicy.yaml
| # Kubernetes API server; no port restriction since it may run on non-standard ports | ||
| - {} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes NetworkPolicy egress rule empty object {} behavior specification
💡 Result:
In a NetworkPolicy, each entry in spec.egress is an allow rule; traffic is allowed if it matches at least one egress rule across all policies selecting the pod (rules are additive). If a pod becomes “egress-isolated” (a selecting policy has policyTypes: [Egress]), then only traffic allowed by those egress rules is permitted. [1]
What egress: - {} means
An empty egress rule object ({}) has no to and no ports, so it imposes no destination or port constraints—it matches all destinations on all ports. Kubernetes documents this as the standard “allow all egress traffic” policy:
spec:
podSelector: {}
egress:
- {}
policyTypes:
- EgressWith this in place, “no additional policy or policies can cause any outgoing connection from those pods to be denied” (because NetworkPolicy is allow-list + additive; there is no explicit “deny” rule). [2]
Contrast: “default deny egress”
If you select pods and set policyTypes: [Egress] but provide no egress rules, that creates egress isolation with zero allowed egress, i.e. deny-all egress:
spec:
podSelector: {}
policyTypes:
- Egress
``` [2]
**So:** `egress: - {}` == **allow all egress** (for selected pods), while omitting egress rules under an egress-isolating policy == **deny all egress**. [1][2]
**Sources**: Kubernetes NetworkPolicy concept docs (v1.33) [1][2].
---
🏁 Script executed:
```shell
cat -n manifests/0000_10_openshift_service-ca_01_networkpolicy.yaml | sed -n '16,37p'Repository: openshift/service-ca-operator
Length of output: 797
egress: - {} permits unrestricted outbound traffic, not API-server-specific egress
Line 37 allows egress to any destination on any port/protocol. The comment on line 36 indicates intent to allow Kubernetes API server traffic only, but the empty egress rule {} overrides this constraint and permits all outbound destinations. Replace with an explicit rule targeting API server endpoints if that is the intended scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifests/0000_10_openshift_service-ca_01_networkpolicy.yaml` around lines 36
- 37, The egress entry "egress: - {}" currently allows all outbound traffic;
update the NetworkPolicy to restrict egress to the Kubernetes API server only by
replacing the empty rule with an explicit rule that targets the API server
(e.g., an egress rule matching destination port 443/6443 and either an IPBlock
for the cluster API service CIDR or a peer that selects the API server
pods/namespace via namespaceSelector/podSelector); specifically, remove the
"egress: - {}" wildcard and add an egress rule that uses to: - ipBlock: {cidr:
<api-server-cidr>} ports: - protocol: TCP port: 443 (and 6443 if used) or
alternatively use to: - namespaceSelector: {matchLabels:
{kubernetes.io/metadata.name: kube-system}} + podSelector matching the
kube-apiserver label so only API-server egress is permitted.
| # Kubernetes API server; no port restriction since it may run on non-standard ports | ||
| - {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n manifests/08_networkpolicy.yaml | head -50Repository: openshift/service-ca-operator
Length of output: 1693
🌐 Web query:
Kubernetes NetworkPolicy egress empty object behavior v1 API
💡 Result:
In the networking.k8s.io/v1 NetworkPolicy API, an “empty object” egress rule means “match everything”:
-
spec.egress: - {}creates one egress rule with notoand noportsconstraints, so it matches all destinations on all ports → effectively allow all egress (for the selected pods, for policies that apply to Egress). This is the documented “allow all egress” pattern. [1] -
Within an egress rule:
- If
tois missing or empty, the rule matches all destinations. [2] - If
portsis missing or empty, the rule matches all ports. [3]
- If
By contrast:
- If a policy applies to egress (e.g.,
policyTypes: ["Egress"]) but has no egress rules (spec.egressomitted) or an empty egress list (spec.egress: []), then the selected pods become egress-isolated and no egress is allowed (default-deny for egress). [1]
Examples
# Allow all egress
spec:
podSelector: {}
policyTypes: ["Egress"]
egress:
- {}# Deny all egress (egress isolation with zero allowed rules)
spec:
podSelector: {}
policyTypes: ["Egress"]
# egress: [] # (equivalent intent: no allowed egress rules)
# (or omit egress entirely)Sources: Kubernetes NetworkPolicy docs (allow-all / default-deny examples) [1], API semantics for egress.to [2] and egress.ports [3].
[1] (v1-33.docs.kubernetes.io)
[2] (v1-31.kubernetes.ac.cn)
[3] (docs.okd.io)
Citations:
- 1: https://v1-33.docs.kubernetes.io/docs/concepts/services-networking/network-policies/?utm_source=openai
- 2: https://v1-31.kubernetes.ac.cn/docs/reference/kubernetes-api/policy-resources/network-policy-v1/?utm_source=openai
- 3: https://docs.okd.io/4.10/rest_api/network_apis/networkpolicy-networking-k8s-io-v1.html?utm_source=openai
- {} in egress grants unrestricted access to all destinations, not API-server-only access
The empty egress rule on line 37 allows traffic to any destination on any port. This contradicts the intent stated in the line 36 comment. Either restrict the rule to the Kubernetes API server using an explicit to selector and port, or update the comment to reflect that unrestricted egress is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifests/08_networkpolicy.yaml` around lines 36 - 37, The egress rule
currently uses the open matcher "- {}" which permits all destinations; replace
that with a restricted egress that targets the Kubernetes API server using an
explicit "to" entry and port (e.g., port 6443) or, if unrestricted egress is
intended, update the comment to state that traffic is intentionally allowed to
all destinations. Locate the egress rule with the "- {}" entry in the
NetworkPolicy manifest and either replace it with a "to" selector/ipBlock + port
specification for the API server or change the comment to reflect the open
egress policy.
|
/retitle CNTRLPLANE-2845: add service-ca operator and workload network policies |
|
@liouk: This pull request references CNTRLPLANE-2845 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/0000_10_openshift_service-ca_01_networkpolicy.yaml`:
- Around line 7-10: The annotations block in the allow NetworkPolicy is missing
the include.release.openshift.io/ibm-cloud-managed: "true" key which makes it
inconsistent with the default-deny policy and will block traffic in IBM Cloud
managed environments; update the annotations in the manifest (the annotations
map that currently contains include.release.openshift.io/hypershift,
include.release.openshift.io/self-managed-high-availability, and
include.release.openshift.io/single-node-developer) to also include
include.release.openshift.io/ibm-cloud-managed: "true" so both policies share
the same IBM Cloud managed annotation.
In `@manifests/01_networkpolicy_00_service-ca-operator.yaml`:
- Around line 7-10: Add the missing annotation
include.release.openshift.io/ibm-cloud-managed: "true" to the annotations block
in the allow policy manifest (01_networkpolicy_00_service-ca-operator.yaml) so
it matches the default-deny policy; update the annotations map containing
include.release.openshift.io/hypershift,
include.release.openshift.io/self-managed-high-availability, and
include.release.openshift.io/single-node-developer by adding the
include.release.openshift.io/ibm-cloud-managed key with the string value "true".
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
manifests/0000_10_openshift_service-ca_01_networkpolicy.yamlmanifests/0000_10_openshift_service-ca_02_networkpolicy_default-deny-all.yamlmanifests/01_networkpolicy_00_service-ca-operator.yamlmanifests/01_networkpolicy_01_default-deny-all.yaml
| annotations: | ||
| include.release.openshift.io/hypershift: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" |
There was a problem hiding this comment.
Missing ibm-cloud-managed annotation creates inconsistency.
The default-deny policy for this namespace (0000_10_openshift_service-ca_02_networkpolicy_default-deny-all.yaml) includes include.release.openshift.io/ibm-cloud-managed: "true", but this allow policy does not. This would cause traffic to be blocked in IBM Cloud managed environments.
🔧 Proposed fix
annotations:
include.release.openshift.io/hypershift: "true"
+ include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| annotations: | |
| include.release.openshift.io/hypershift: "true" | |
| include.release.openshift.io/self-managed-high-availability: "true" | |
| include.release.openshift.io/single-node-developer: "true" | |
| annotations: | |
| include.release.openshift.io/hypershift: "true" | |
| include.release.openshift.io/ibm-cloud-managed: "true" | |
| include.release.openshift.io/self-managed-high-availability: "true" | |
| include.release.openshift.io/single-node-developer: "true" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifests/0000_10_openshift_service-ca_01_networkpolicy.yaml` around lines 7
- 10, The annotations block in the allow NetworkPolicy is missing the
include.release.openshift.io/ibm-cloud-managed: "true" key which makes it
inconsistent with the default-deny policy and will block traffic in IBM Cloud
managed environments; update the annotations in the manifest (the annotations
map that currently contains include.release.openshift.io/hypershift,
include.release.openshift.io/self-managed-high-availability, and
include.release.openshift.io/single-node-developer) to also include
include.release.openshift.io/ibm-cloud-managed: "true" so both policies share
the same IBM Cloud managed annotation.
| annotations: | ||
| include.release.openshift.io/hypershift: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" |
There was a problem hiding this comment.
Missing ibm-cloud-managed annotation creates inconsistency.
The default-deny policy in this namespace (01_networkpolicy_01_default-deny-all.yaml) includes include.release.openshift.io/ibm-cloud-managed: "true", but this allow policy does not. In IBM Cloud managed environments, the default-deny would be applied while this allow policy would be skipped, blocking all traffic to the operator pod.
🔧 Proposed fix
annotations:
include.release.openshift.io/hypershift: "true"
+ include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifests/01_networkpolicy_00_service-ca-operator.yaml` around lines 7 - 10,
Add the missing annotation include.release.openshift.io/ibm-cloud-managed:
"true" to the annotations block in the allow policy manifest
(01_networkpolicy_00_service-ca-operator.yaml) so it matches the default-deny
policy; update the annotations map containing
include.release.openshift.io/hypershift,
include.release.openshift.io/self-managed-high-availability, and
include.release.openshift.io/single-node-developer by adding the
include.release.openshift.io/ibm-cloud-managed key with the string value "true".
|
/retest-required |
|
Holding to run payload jobs once review is completed. /hold |
|
/verified by @kaleemsiddiqu |
|
@kaleemsiddiqu: The 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. |
| name: service-ca | ||
| namespace: openshift-service-ca | ||
| annotations: | ||
| include.release.openshift.io/hypershift: "true" |
| name: default-deny-all | ||
| namespace: openshift-service-ca | ||
| annotations: | ||
| include.release.openshift.io/hypershift: "true" |
There was a problem hiding this comment.
Remove hypershift label and ibm-cloud-managed
| name: service-ca-operator | ||
| namespace: openshift-service-ca-operator | ||
| annotations: | ||
| include.release.openshift.io/hypershift: "true" |
| name: default-deny-all | ||
| namespace: openshift-service-ca-operator | ||
| annotations: | ||
| include.release.openshift.io/hypershift: "true" |
There was a problem hiding this comment.
remove hypershift label and ibm-cloud-managed
|
@liouk: This pull request references CNTRLPLANE-2845 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. |
|
/lgtm |
Verify that NetworkPolicy resources introduced in PR openshift#324 are correctly deployed in both the openshift-service-ca-operator and openshift-service-ca namespaces. Tests check: - existence of allow and default-deny-all policies in each namespace - pod selector targets the correct app label - ingress allows TCP 8443 for metrics scraping - egress allows port 5353 for DNS resolution - both Ingress and Egress policyTypes are declared
Verify that NetworkPolicy resources introduced in PR openshift#324 are correctly deployed in both the openshift-service-ca-operator and openshift-service-ca namespaces. Tests check: - existence of allow and default-deny-all policies in each namespace - pod selector targets the correct app label - ingress allows TCP 8443 for metrics scraping - egress allows port 5353 for DNS resolution - both Ingress and Egress policyTypes are declared
Verify that NetworkPolicy resources introduced in PR openshift#324 are correctly deployed in both the openshift-service-ca-operator and openshift-service-ca namespaces. Tests check: - existence of allow and default-deny-all policies in each namespace - pod selector targets the correct app label - ingress allows TCP 8443 for metrics scraping - egress allows port 5353 for DNS resolution - both Ingress and Egress policyTypes are declared
E2E Test Results for Network Policy ChangesTested against a live cluster with the NetworkPolicy manifests from this PR applied, using the companion test PR #337. Test binary:
All 4 tests passed. The NetworkPolicies exist in both namespaces with correct pod selectors, ingress on TCP 8443 (metrics), egress on port 5353 (DNS), and both |
|
/verified by @wangke19 |
|
@wangke19: 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: benluddy, dusk125, 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 |
|
/payload 4.22 nightly blocking |
|
@liouk: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6b9cf080-28f2-11f1-83d6-de0dddf695db-0 trigger 65 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6b9cf080-28f2-11f1-83d6-de0dddf695db-1 |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-upgrade-ovn-single-node periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-fips periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-fips-no-nat-instance periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ipi-ovn-ipv6 |
|
@liouk: trigger 4 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2eca08f0-2924-11f1-9375-4c2a457bbf12-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-fips-no-nat-instance |
|
@liouk: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c66dd730-29b6-11f1-95d2-69081b0f76a0-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-fips-no-nat-instance |
|
@liouk: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4703eca0-29f2-11f1-9ff4-5d16cfd60a17-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-fips-no-nat-instance |
|
@liouk: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/25a085a0-2cd1-11f1-8e72-8668e4fa92ea-0 |
|
After a few retries of the single blocking job failing, I'll cancel the hold; the job failure is unrelated to the changes of this PR. /hold cancel |
|
/retest-required |
|
/retest-required |
|
@liouk: 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. |
Note on manifest placement: The network policies for the operand namespace (
openshift-service-ca) are placed inmanifests/as CVO-managed resources rather than inbindata/as operator-managed resources.This is because the vendored version of
library-goin this repo does not includeApplyNetworkPolicysupport inApplyDirectly. The latestlibrary-goon master does support it (seenetworking.goand theNetworkPolicycase ingeneric.go).If we bump the
library-godependency in the future, we can move the controller policy tobindata/v4.0.0/controller/and have the operator manage it directly, consistent with how other operand resources (namespace, RBAC, etc.) are managed. For now, CVO will reconcile the policies and recreate them if deleted.