DNM OADP-6074: add network policies for OADP #1906
Conversation
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@weshayutin: This pull request references OADP-6074 which is a valid jira issue. In 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weshayutin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
Overall looks good, I assume that these will be included in the bundle for the next release?
| name: default-deny-labelled | ||
| ``` | ||
|
|
||
| This policy implements a **default-deny approach** for pods managed by the OADP operator, meaning that by default, all network traffic is blocked except for explicitly allowed connections. |
There was a problem hiding this comment.
Will this block the OADP CLI from functioning? Did we ever enable the pull-through logs download that @kaovilai had started? Do we need to allow this in some way?
There was a problem hiding this comment.
I think we can revise the default policy later if needed. The download server has not yet been implemented.
|
/retest |
|
@shawn-hurley @kaovilai ugh.. I don't even remember writing this pr. It does appear as a trace through emails and jira this will need to be backported across all versions and released. I need to do more reading and testing. @shubham-pampattiwar @sseago additional in-depth reviews welcomed |
shubham-pampattiwar
left a comment
shubham-pampattiwar
left a comment
There was a problem hiding this comment.
@weshayutin Is this the primary issue https://issues.redhat.com/browse/OCPSTRAT-819 ?
Seems like this is a hardening requirement across all openshift components. We may want the network policy to be applied during install time. Most likely the policy should be part of the operator bundle manifest folder.
|
@shubham-pampattiwar yup.. going through the hardening now.. hold reviews |
WalkthroughAdds a kustomization and a NetworkPolicy (default-deny with specific ingress/egress allowances) targeting pods labeled Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/NetworkPolicies/README.md (2)
77-77: Convert bold section headers to proper markdown headings.Lines 77, 92, and 110 use bold text (
**...**) instead of markdown headings (###). Update these to use proper heading syntax:-**1. HTTPS Internet Access** +### HTTPS Internet Access -**2. DNS Resolution** +### DNS Resolution -**3. Kubernetes API Access** +### Kubernetes API AccessThis improves document structure and consistency.
Also applies to: 92-92, 110-110
69-69: Document the broad ingress rule rationale.Line 69 notes that the first ingress rule "allows incoming traffic from any pod within the same namespace." While this is documented, consider clarifying in the "Important Considerations" section why namespace-wide ingress is acceptable for OADP components. This helps users understand the security posture and any associated risks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
config/networkpolicy/kustomization.yaml(1 hunks)config/networkpolicy/networkpolicy.yaml(1 hunks)docs/NetworkPolicies/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/networkpolicy/networkpolicy.yamlconfig/networkpolicy/kustomization.yamldocs/NetworkPolicies/README.md
🪛 LanguageTool
docs/NetworkPolicies/README.md
[style] ~141-~141: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...Webhooks: The Kubernetes API server needs to reach the OADP operator for CR validati...
(REP_NEED_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
docs/NetworkPolicies/README.md
77-77: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
92-92: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
config/networkpolicy/kustomization.yaml (1)
7-9: Verify label alignment after kustomize application.The
commonLabelswill merge with the labels defined innetworkpolicy.yaml. Line 6 ofnetworkpolicy.yamlsetsapp.kubernetes.io/name: oadp-networkpolicy, but this kustomization will applyapp.kubernetes.io/name: oadp-operator(line 8), resulting in the latter. Confirm this label merge is intentional and aligns with your labeling strategy.config/networkpolicy/networkpolicy.yaml (1)
11-13: Verify that OADP pods are labeled withapp.kubernetes.io/managed-by: "oadp-operator".The policy's
podSelectortargets pods with this label. Confirm that all OADP operator and managed components (Velero pods, etc.) are consistently labeled with this label in the codebase, otherwise the policy will not select them and traffic rules will not apply.
| - ports: | ||
| - protocol: TCP | ||
| port: 9443 |
There was a problem hiding this comment.
Restrict the webhook ingress rule to the API server.
The ingress rule on port 9443 lacks a from clause, meaning it allows traffic from any source on this port. Since this is documented as a webhook for API server validation/mutation (line 65 of README), restrict it to the Kubernetes API server to reduce attack surface:
- ports:
- protocol: TCP
port: 9443
+ from:
+ - podSelector: {}
+ namespaceSelector:
+ matchLabels:
+ kubernetes.io/metadata.name: defaultAlternatively, if the webhook needs to accept traffic from pods within the same namespace, document this decision explicitly.
Would you like me to refine this rule further or provide additional security-hardened options?
📝 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.
| - ports: | |
| - protocol: TCP | |
| port: 9443 | |
| - ports: | |
| - protocol: TCP | |
| port: 9443 | |
| from: | |
| - podSelector: {} | |
| namespaceSelector: | |
| matchLabels: | |
| kubernetes.io/metadata.name: default |
🤖 Prompt for AI Agents
In config/networkpolicy/networkpolicy.yaml around lines 32-34, the ingress rule
allowing TCP port 9443 has no `from` clause (allowing all sources); restrict it
to only the Kubernetes API server by adding a `from` selector that targets the
API server pods or control-plane CIDR (e.g., namespaceSelector + podSelector
matching the kube-system API server pods, or an ipBlock matching the cluster
control-plane IPs); alternatively, if the webhook must accept traffic from
same-namespace pods, add a `from` clause limiting sources to that namespace
(namespaceSelector matching the webhook namespace) and document the decision.
|
|
||
| ## OADP NetworkPolicy Configuration | ||
|
|
||
| The `network_policy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does: |
There was a problem hiding this comment.
Correct the policy filename reference.
Line 18 refers to the policy as network_policy.yaml, but the actual filename is networkpolicy.yaml (no underscore). Update the reference:
-The `network_policy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does:
+The `networkpolicy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does:Also applies to: 23-23
🤖 Prompt for AI Agents
In docs/NetworkPolicies/README.md around lines 18 and 23, the README incorrectly
references the policy file as "network_policy.yaml"; update both occurrences to
the actual filename "networkpolicy.yaml" (remove the underscore) so the README
matches the repository file name and any links or commands referencing it work
correctly.
| Check if the NetworkPolicy is applied: | ||
| ```bash | ||
| kubectl get networkpolicy -n <oadp-namespace> | ||
| kubectl describe networkpolicy default-deny-labelled -n <oadp-namespace> |
There was a problem hiding this comment.
Correct the NetworkPolicy name reference.
Line 160 references the policy as default-deny-labelled, but the actual policy name is oadp-default-deny-labelled. Update the reference:
-kubectl describe networkpolicy default-deny-labelled -n <oadp-namespace>
+kubectl describe networkpolicy oadp-default-deny-labelled -n <oadp-namespace>📝 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.
| kubectl describe networkpolicy default-deny-labelled -n <oadp-namespace> | |
| kubectl describe networkpolicy oadp-default-deny-labelled -n <oadp-namespace> |
🤖 Prompt for AI Agents
In docs/NetworkPolicies/README.md around line 160, the kubectl command
references the wrong NetworkPolicy name; change `default-deny-labelled` to the
actual policy name `oadp-default-deny-labelled` so the command reads `kubectl
describe networkpolicy oadp-default-deny-labelled -n <oadp-namespace>`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
config/networkpolicy/networkpolicy.yaml (1)
32-34: Port 9443 webhook ingress rule lacks source restriction — restricting to API server traffic is strongly recommended.An unrestricted webhook ingress rule allows communication from any pod to the webhook server, which increases the attack surface. For webhook security, the minimal ingress rule should restrict traffic to only the Kubernetes API server by using an IP block or pod selector.
Apply this diff to restrict the webhook to the API server:
- ports: - protocol: TCP port: 9443 + from: + - ipBlock: + cidr: <API_SERVER_IP>/32 # Replace with your control-plane IP + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: kube-system + podSelector: + matchLabels: + component: kube-apiserverAlternatively, if restricting by IP is not feasible in your environment, document this security decision explicitly with a comment explaining why unrestricted access is acceptable for your threat model.
docs/NetworkPolicies/README.md (1)
18-18: Correct filename and policy name references throughout the documentation.Multiple references to the policy and file use incorrect names:
- Lines 18, 170: Refer to
network_policy.yamlinstead ofnetworkpolicy.yaml- Lines 23, 182: Reference policy name as
default-deny-labelledinstead ofoadp-default-deny-labelledApply these diffs:
-The `network_policy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does: +The `networkpolicy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does:-name: default-deny-labelled +name: oadp-default-deny-labelled-kubectl apply -f network_policy.yaml -n <oadp-namespace> +kubectl apply -f networkpolicy.yaml -n <oadp-namespace>-kubectl describe networkpolicy default-deny-labelled -n <oadp-namespace> +kubectl describe networkpolicy oadp-default-deny-labelled -n <oadp-namespace>Also applies to: 23-23, 170-170, 182-182
🧹 Nitpick comments (1)
docs/NetworkPolicies/README.md (1)
77-77: Use proper Markdown headings instead of bold emphasis.Lines 77 ("1. General Network Access"), 94 ("2. DNS Resolution"), and 110 ("3. Kubernetes API Access (Explicit)") use bold emphasis to simulate headings. Replace with Markdown level-3 headings for proper document structure:
-**1. General Network Access** +### 1. General Network Access-**2. DNS Resolution** +### 2. DNS Resolution-**3. Kubernetes API Access (Explicit)** +### 3. Kubernetes API Access (Explicit)Also applies to: 94-94, 110-110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
config/networkpolicy/networkpolicy.yaml(1 hunks)docs/NetworkPolicies/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/networkpolicy/networkpolicy.yamldocs/NetworkPolicies/README.md
🪛 LanguageTool
docs/NetworkPolicies/README.md
[style] ~163-~163: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...Webhooks: The Kubernetes API server needs to reach the OADP operator for CR validati...
(REP_NEED_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
docs/NetworkPolicies/README.md
77-77: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
config/networkpolicy/networkpolicy.yaml (1)
1-79: Policy structure and egress rules are well-designed.The policy correctly implements a default-deny model with explicit allowlists for DNS, API servers, and cloud storage access. The egress rules properly scope access to necessary namespaces and IP ranges, reducing the attack surface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
docs/NetworkPolicies/README.md (2)
18-18: Correct the policy filename reference.Lines 18 and 178 reference the policy file as
network_policy.yaml, but the actual filename isnetworkpolicy.yaml(no underscore). Update both occurrences for accuracy:-The `network_policy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does: +The `networkpolicy.yaml` file in this directory defines a NetworkPolicy specifically designed for OADP components. Here's what it does:-kubectl apply -f network_policy.yaml -n <oadp-namespace> +kubectl apply -f networkpolicy.yaml -n <oadp-namespace>Also applies to: 178-178
23-23: Correct the NetworkPolicy name reference.The documentation references the policy as
default-deny-labelled, but the actual policy name isoadp-default-deny-labelled(defined in networkpolicy.yaml line 4). Update lines 23 and 190:-name: default-deny-labelled +name: oadp-default-deny-labelled-kubectl describe networkpolicy default-deny-labelled -n <oadp-namespace> +kubectl describe networkpolicy oadp-default-deny-labelled -n <oadp-namespace>Also applies to: 190-190
config/networkpolicy/networkpolicy.yaml (1)
36-38: Restrict the webhook ingress rule to the API server.The ingress rule on port 9443 allows traffic from any source due to the missing
fromclause. Since this port is for API server validation/mutation webhooks, restrict it to the Kubernetes API server to reduce attack surface.Apply this diff to restrict webhook traffic to the API server:
- ports: - protocol: TCP port: 9443 + from: + - podSelector: {} + namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: defaultAlternatively, if the webhook must accept traffic from same-namespace pods, document this decision explicitly and adjust the selector accordingly.
🧹 Nitpick comments (1)
docs/NetworkPolicies/README.md (1)
77-77: Use Markdown headings instead of bold emphasis.Lines 77, 94, and 118 use bold emphasis (
**...**) for section headers. For better document structure and semantic meaning, convert these to proper Markdown headings (####):-**1. General Network Access** +#### 1. General Network Access-**2. DNS Resolution** +#### 2. DNS Resolution-**3. Kubernetes API Access (Explicit)** +#### 3. Kubernetes API Access (Explicit)Also applies to: 94-94, 118-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
config/networkpolicy/networkpolicy.yaml(1 hunks)docs/NetworkPolicies/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/networkpolicy/networkpolicy.yamldocs/NetworkPolicies/README.md
🪛 LanguageTool
docs/NetworkPolicies/README.md
[style] ~171-~171: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...Webhooks: The Kubernetes API server needs to reach the OADP operator for CR validati...
(REP_NEED_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
docs/NetworkPolicies/README.md
77-77: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
118-118: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
|
/retest ai-retester: The e2e tests failed because the comment for /pull/1906 |
|
/retest ai-retester: The comment for /pull/1906 |
|
/retest ai-retester: The comment for /pull/1906 |
|
/retest ai-retester: The E2E test failed because the comment for /pull/1906 |
|
@weshayutin: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
after a lot of testing and retesting... the networkpolicy is responsible for the backuprepository NOT getting created. Other communication to the s3 bucket is working so it's a little wierd. debug logs are here: https://termbin.com/k9wv |
|
5 participants
Why the changes were made
https://issues.redhat.com/browse/OADP-6074
How to test the changes made
For now the only way to test is to use the networkpolicy yaml in the docs directory and attempt backup and restore operations.