MTV-2678 | RFE: Implement OpenShift Network Policies in MTV#2440
MTV-2678 | RFE: Implement OpenShift Network Policies in MTV#2440Hazanel wants to merge 3 commits intokubev2v:mainfrom
Conversation
Hazanel
requested review from
mnecas,
mrnold,
solenoci and
yaacov
as code owners
WalkthroughAdds a new feature flag spec.feature_network_policies surfaced across samples/CSVs/manifests and operator defaults; grants RBAC for NetworkPolicy, adds Ansible tasks/defaults to apply or remove NetworkPolicy resources, and introduces five new Jinja2 NetworkPolicy templates (operator, controller, API, migration workloads, validation). Changes
Sequence Diagram(s)sequenceDiagram
participant CR as ForkliftController CR
participant Ansible as Ansible role (forkliftcontroller)
participant K8s as Kubernetes API
CR->>Ansible: spec.feature_network_policies = true/false
alt feature_network_policies == true
Ansible->>K8s: Apply NetworkPolicy templates (operator, controller, api, migration, validation)
else feature_network_policies == false
Ansible->>K8s: Delete listed NetworkPolicy resources
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Hazanel
force-pushed
the
networkPolicies
branch
4 times, most recently
from
135442f to
f32d030
Compare
Hazanel
force-pushed
the
networkPolicies
branch
from
f32d030 to
817a766
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2440 +/- ##
==========================================
- Coverage 15.45% 10.10% -5.36%
==========================================
Files 112 469 +357
Lines 23377 53685 +30308
==========================================
+ Hits 3613 5424 +1811
- Misses 19479 47792 +28313
- Partials 285 469 +184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
operator/config/network-policies/README.md (1)
1-286: Answer prior review: clarify doc location and reference the JiraEchoing a prior comment: If this is not user-facing, consider linking it from the main docs so users discover it. Add a reference to MTV-2678 for traceability.
🧹 Nitpick comments (4)
operator/.kustomized_manifests (1)
5280-5294: Expose the new flag in the initialization CR for visibilityOptional but helpful: include
feature_network_policiesin the CSV initialization resource so users see the knob immediately after install."spec": { "feature_ui_plugin": "true", "feature_validation": "true", - "feature_volume_populator": "true" + "feature_volume_populator": "true", + "feature_network_policies": "false" }operator/config/network-policies/forklift-controller.yaml (1)
164-164: Add newline at end of fileFixes lint ([new-line-at-end-of-file]) and keeps diffs cleaner.
operator/config/network-policies/README.md (2)
110-110: Fix markdownlint: remove trailing punctuation in headingsMinor cleanup for MD026.
-### Network Policy Communication Flows: +### Network Policy Communication Flows ... -### ⚠️ **Important**: +### ⚠️ **Important**Also applies to: 232-232
22-23: Namespace naming is inconsistent across docs and manifestsDocs reference
konveyor-forklift, while manifests targetopenshift-mtv. Consider aligning or adding a note that the namespace is configurable and may differ by deployment mode.Also applies to: 45-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 135442fe3a99c0938ca68945b6ae299e067b11c6 and 817a766ba1a683e4220b0bb435decd2454697b56.
📒 Files selected for processing (15)
operator/.kustomized_manifests(1 hunks)operator/config/network-policies/README.md(1 hunks)operator/config/network-policies/deploy.yaml(1 hunks)operator/config/network-policies/forklift-api.yaml(1 hunks)operator/config/network-policies/forklift-controller.yaml(1 hunks)operator/config/network-policies/kustomization.yaml(1 hunks)operator/config/network-policies/migration-workloads.yaml(1 hunks)operator/config/network-policies/validation-pods.yaml(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- operator/config/network-policies/kustomization.yaml
- operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml
- operator/roles/forkliftcontroller/defaults/main.yml
- operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
- operator/config/network-policies/forklift-api.yaml
- operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
- operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
🧰 Additional context used
🪛 YAMLlint (1.37.1)
operator/config/network-policies/forklift-controller.yaml
[error] 164-164: no new line character at the end of file
(new-line-at-end-of-file)
operator/roles/forkliftcontroller/tasks/main.yml
[warning] 297-297: wrong indentation: expected 4 but found 6
(indentation)
🪛 markdownlint-cli2 (0.17.2)
operator/config/network-policies/README.md
110-110: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
232-232: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (3)
operator/.kustomized_manifests (1)
5145-5148: Good addition: feature flag is introduced and defaults to falseThe field integrates cleanly alongside existing flags and will parse correctly via
|boolin Ansible. No functional concerns here.operator/roles/forkliftcontroller/tasks/main.yml (1)
238-261: Network policy creation: OK gating; ensure templates render with the correct namespace variableCreation is properly gated on
feature_network_policies|booland usesk8smodule idempotently. Verify the templates rely onapp_namespace(used elsewhere in this role) and not an undefined variable; see the controller policy template note below.operator/config/network-policies/validation-pods.yaml (1)
16-23: Ingress: removeports: []to actually allow controller accessingress: # Allow controller communication - from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted)Likely an incorrect or invalid review comment.
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Outdated
Show resolved
Hide resolved
mnecas
left a comment
mnecas
left a comment
There was a problem hiding this comment.
Where did you got the list of those ports?
Have you tried the migrations from all providers?
This can break all of the migraitons if we miss one port.
Hazanel
force-pushed
the
networkPolicies
branch
2 times, most recently
from
cd5f2d8 to
2cf67da
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (12)
operator/config/network-policies/README.md (1)
1-5: Add reference to the Jira (MTV-2678) as requested previouslyAdd an explicit link to the Jira to provide background/rationale in the doc.
operator/roles/forkliftcontroller/tasks/main.yml (1)
289-301: Cleanup is incomplete and indentation fails yamllint; remove all created policies and fix indentOnly three policies are removed, leaving the rest orphaned when disabling the feature. Also, yamllint flags wrong indentation (Line 297).
Apply this diff:
- name: "Cleanup network policies if disabled" k8s: api_version: networking.k8s.io/v1 kind: NetworkPolicy namespace: "{{ app_namespace }}" name: "{{ item }}" state: absent - loop: - - forklift-default-deny - - forklift-controller-policy - - forklift-api-policy + loop: + - forklift-default-deny + - forklift-controller-policy + - forklift-api-policy + - forklift-virt-v2v-policy + - forklift-consumer-policy + - forklift-ovirt-populator-policy + - forklift-openstack-populator-policy + - forklift-vsphere-xcopy-populator-policy + - forklift-vddk-validation-policy + - forklift-validation-service-policy + - forklift-image-converter-policy when: not feature_network_policies|booloperator/config/network-policies/forklift-controller.yaml (4)
16-31: Ingress: removeports: [](currently blocks all)For each ingress item, omit
portsto allow all ports (or enumerate needed ports):- from: - podSelector: {} # Allow all pods in same namespace - ports: [] # CRITICAL: Allow all ports ... - from: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: openshift-console - ports: [] # CRITICAL: Allow all ports ... - from: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: openshift-monitoring - ports: [] # CRITICAL: Allow all ports
32-41: Egress within namespace: removeports: []to actually allow traffic- to: - podSelector: {} - ports: [] # CRITICAL: Allow all ports for internal communication
37-41: DNS egress needs TCP/UDP 53, notports: []- to: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: openshift-dns - ports: [] + ports: + - protocol: UDP + port: 53 + - protocol: TCP + port: 53
43-73: Use “any destination” correctly: removeto: []and keep onlyports
to: []matches nothing and blocks the rule. For destination-any rules (K8s API and external providers), omitto.- - to: [] - ports: + - ports: - protocol: TCP port: 6443 - protocol: TCP port: 443 - - to: [] - ports: + - ports: - protocol: TCP port: 80 - protocol: TCP port: 443 # vSphere/OpenStack/oVirt HTTPS - protocol: TCP port: 8080 # oVirt/OVA HTTP - protocol: TCP port: 8443 # vSphere/oVirt HTTPS alternate - protocol: TCP port: 902 # vSphere NFC - protocol: TCP port: 5000 # OpenStack Keystone - protocol: TCP port: 8774 # OpenStack Nova - protocol: TCP port: 8776 # OpenStack Cinder - protocol: TCP port: 9696 # OpenStack Neutron - protocol: TCP port: 35357 # OpenStack Admin - protocol: TCP port: 54322 # oVirt imageioAlso add a newline at EOF.
operator/config/network-policies/migration-workloads.yaml (2)
16-23: Ingress from controller: removeports: []to allow traffic- from: - podSelector: matchLabels: control-plane: controller-manager - ports: []Apply to all policies in this file.
31-39: Egress rules: replaceto: []with ports-only to mean “any destination”Using
to: []blocks the rule. Keep onlyportsfor external HTTPS/NFC/imageio, DNS, and NFS rules.Example for the first policy:
- - to: [] - ports: + - ports: - protocol: TCP port: 443 # HTTPS - protocol: TCP port: 902 # vSphere NFC - protocol: TCP port: 54322 # oVirt imageio - - to: [] - ports: + - ports: - protocol: UDP port: 53 - protocol: TCP port: 53 - protocol: TCP port: 80 - protocol: TCP port: 443 - protocol: TCP port: 8080 - protocol: TCP port: 8443 - - to: [] - ports: + - ports: - protocol: TCP port: 2049 # NFS - protocol: UDP port: 2049 # NFSReplicate the same fix across the other policies.
Also applies to: 40-54, 55-61
operator/config/network-policies/validation-pods.yaml (4)
16-23: Ingress from controller: removeports: []across all validation policies- from: - podSelector: matchLabels: control-plane: controller-manager - ports: []
24-37: Egress rules: removeto: []and keep onlyportsto match any destinationExample for vddk policy:
- - to: [] - ports: + - ports: - protocol: TCP port: 443 # vSphere API - protocol: TCP port: 902 # NFC - - to: [] - ports: + - ports: - protocol: UDP port: 53 - protocol: TCP port: 53 - protocol: TCP port: 80 - protocol: TCP port: 443 - protocol: TCP port: 8080 - protocol: TCP port: 8443Apply similarly in the other sections.
Also applies to: 38-52
100-107: K8s API/DNS/HTTP rules: removeto: [], keep ports only- - to: [] - ports: + - ports: - protocol: TCP port: 6443 - protocol: TCP port: 443 - - to: [] - ports: + - ports: - protocol: UDP port: 53 - protocol: TCP port: 53 - protocol: TCP port: 80 - protocol: TCP port: 443 - protocol: TCP port: 8080 - protocol: TCP port: 8443 - - to: [] - ports: + - ports: - protocol: TCP port: 8080Also applies to: 108-122, 123-127
147-156: Repeat the same fixes in image-converter policy
- Remove
ports: []under ingress from controller.- Replace egress
to: []with ports-only for external, DNS/common ports, and storage (NFS) sections.Also applies to: 161-176, 177-183
🧹 Nitpick comments (2)
operator/config/network-policies/README.md (1)
110-110: Fix markdownlint MD026: remove trailing punctuation from headingsRemove the trailing colons to satisfy MD026.
-### Network Policy Communication Flows: +### Network Policy Communication Flows ... -### ⚠️ **Important**: +### ⚠️ **Important**Also applies to: 232-232
operator/config/network-policies/forklift-api.yaml (1)
74-74: Trim trailing spaces and add newline at EOF
- Line 74 has trailing spaces.
- File lacks a newline at EOF.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 74c1e68d79666ae4c6436f1e666a95a1cdf9d958 and 2cf67daefe9b9f444078dc8afbb50b90331821e0.
📒 Files selected for processing (15)
operator/.kustomized_manifests(1 hunks)operator/config/network-policies/README.md(1 hunks)operator/config/network-policies/deploy.yaml(1 hunks)operator/config/network-policies/forklift-api.yaml(1 hunks)operator/config/network-policies/forklift-controller.yaml(1 hunks)operator/config/network-policies/kustomization.yaml(1 hunks)operator/config/network-policies/migration-workloads.yaml(1 hunks)operator/config/network-policies/validation-pods.yaml(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml
- operator/.kustomized_manifests
- operator/roles/forkliftcontroller/defaults/main.yml
- operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
- operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
- operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
- operator/config/network-policies/kustomization.yaml
- operator/config/network-policies/deploy.yaml
- operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
🧰 Additional context used
🪛 YAMLlint (1.37.1)
operator/config/network-policies/forklift-api.yaml
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 77-77: no new line character at the end of file
(new-line-at-end-of-file)
operator/config/network-policies/forklift-controller.yaml
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
operator/roles/forkliftcontroller/tasks/main.yml
[warning] 297-297: wrong indentation: expected 4 but found 6
(indentation)
🪛 markdownlint-cli2 (0.17.2)
operator/config/network-policies/README.md
110-110: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
232-232: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / forklift-operator-dev-preview-on-pull-request
- GitHub Check: lint
- GitHub Check: build
Hazanel
force-pushed
the
networkPolicies
branch
2 times, most recently
from
c239d0f to
1c2f359
Compare
There was a problem hiding this comment.
Actionable comments posted: 34
♻️ Duplicate comments (12)
operator/config/network-policies/forklift-api.yaml (1)
29-33: Fix webhook ingress:from: []blocks all peers; restrict to kube‑apiserver namespace
from: []matches no sources. For webhooks, restrict to the kube‑apiserver namespace.Apply this diff:
- # Allow Kubernetes API server for webhooks - - from: [] - ports: - - protocol: TCP - port: 8443 + # Allow Kubernetes API server for webhooks + - from: + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: openshift-kube-apiserver + ports: + - protocol: TCP + port: 8443operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2 (2)
1-35: Fix NetworkPolicy semantics: removeports: [](blocks all) andto: [](matches none)
ports: []matches no ports; omit the field to allow all ports.to: []matches no destinations; omittoto allow anywhere on listed ports or specify proper selectors.
Also correct the misleading comment at line 1.-# Final Working Controller Policy - with explicit ports: [] to prevent DNS timeout +# Controller NetworkPolicy — corrected ports/to semantics @@ ingress: # Allow all Forklift components to communicate with controller - from: - podSelector: {} # Allow all pods in same namespace - ports: [] # CRITICAL: Allow all ports + # All ports allowed by omitting 'ports' # Allow OpenShift console access - from: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: openshift-console - ports: [] # CRITICAL: Allow all ports + # All ports allowed by omitting 'ports' # Allow monitoring access - from: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: openshift-monitoring - ports: [] # CRITICAL: Allow all ports + # All ports allowed by omitting 'ports' egress: # Allow all egress to same namespace (including self-communication to inventory service) - to: - podSelector: {} - ports: [] # CRITICAL: Allow all ports for internal communication + # All ports allowed by omitting 'ports' - # Allow DNS resolution (essential) - - to: [] - ports: + # Allow DNS resolution (essential) to any destination + - ports: - protocol: UDP port: 53 - protocol: TCP port: 53 - # Allow Kubernetes API access - - to: [] - ports: + # Allow Kubernetes API access to any destination + - ports: - protocol: TCP port: 6443 - protocol: TCP port: 443 - # Allow external provider API access - - to: [] - ports: + # Allow external provider API access to any destination + - ports: - protocol: TCP port: 443 # vSphere/OpenStack/oVirt HTTPS - protocol: TCP port: 8443 # vSphere/oVirt HTTPS alternate - protocol: TCP port: 902 # vSphere NFC - protocol: TCP port: 5000 # OpenStack Keystone - protocol: TCP port: 8774 # OpenStack Nova - protocol: TCP port: 8776 # OpenStack Cinder - protocol: TCP port: 9696 # OpenStack Neutron - protocol: TCP - port: 35357 # OpenStack Admin + port: 35357 # OpenStack Admin - protocol: TCP - port: 8080 # oVirt/OVA HTTP + port: 8080 # oVirt/OVA HTTP - protocol: TCP - port: 54322 # oVirt imageio + port: 54322 # oVirt imageio
6-6: Wrong variable: useapp_namespace(forklift_namespace is likely undefined)This will render an empty/invalid namespace and fail creation. Use the role’s
app_namespace.- namespace: {{ forklift_namespace }} + namespace: {{ app_namespace }}operator/config/network-policies/migration-workloads.yaml (2)
16-22: Ingress: remove ‘ports: []’ to actually allow controller on all portsEmpty ports list blocks all ports.
- from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted)
30-60: Egress to external/DNS/NFS: drop ‘to: []’; keep only portsto: [] matches no destinations; omit to for “any destination.”
- - to: [] - ports: + - ports: - protocol: TCP port: 443 # HTTPS ... - - to: [] - ports: + - ports: - protocol: UDP port: 53 ... - - to: [] - ports: + - ports: - protocol: TCP port: 2049 # NFSApply to all similar blocks in this file.
operator/config/network-policies/validation-pods.yaml (2)
90-94: In-namespace allow: remove ‘ports: []’ or tighten scopeThis rule is currently a no-op (ports: [] means no ports). Either remove ports: [] to make it effective, or restrict to known pods/ports (e.g., consumer on 8080).
- - to: - - podSelector: {} - ports: [] + - to: + - podSelector: {}Or narrow to specific labels/ports if possible.
95-116: K8s API and DNS egress: remove ‘to: []’ and keep ports onlyEnsure these rules aren’t neutered by empty peer lists.
- - to: [] - ports: + - ports: - protocol: TCP port: 6443 - protocol: TCP port: 443 - - to: [] - ports: + - ports: - protocol: UDP port: 53 - protocol: TCP port: 53operator/config/network-policies/deploy.yaml (5)
19-23: Ingress: ‘from: []’ denies all sources; omit to allow any on 8443For forklift-api-policy, remove from: [] so 8443 is reachable from any source as intended for webhooks.
- - from: [] - ports: + - ports: - port: 8443 protocol: TCP
53-74: Ports-only egress with ‘to: []’: remove ‘to’ to target any destinationRules with to: [] match nothing. Keep only ports.
- ports: - port: 80 protocol: TCP - port: 443 protocol: TCP - to: [] ... - ports: - port: 2049 protocol: TCP - port: 2049 protocol: UDP - to: []Apply across all similar egress blocks in this file.
163-183: Remove ‘to: []’ on ports-only egress (image converter)- ports: - port: 443 protocol: TCP - port: 80 protocol: TCP - to: [] ... - ports: - port: 2049 protocol: TCP - port: 2049 protocol: UDP - to: []
315-334: K8s API/DNS/HTTP(S) egress: remove ‘to: []’- ports: - port: 6443 protocol: TCP - port: 443 protocol: TCP - to: [] ... - ports: - port: 8443 protocol: TCP - to: []
457-481: vSphere xcopy populator egress: remove ‘to: []’ on ports-only blocks- ports: - port: 443 protocol: TCP - port: 902 protocol: TCP - to: [] ... - ports: - port: 53 protocol: UDP - port: 53 protocol: TCP - port: 80 protocol: TCP - port: 443 protocol: TCP - port: 8080 protocol: TCP - port: 8443 protocol: TCP - to: []
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2cf67daefe9b9f444078dc8afbb50b90331821e0 and 1c2f359.
⛔ Files ignored due to path filters (1)
operator/config/network-policies/README.mdis excluded by!**/*.md
📒 Files selected for processing (15)
operator/.kustomized_manifests(1 hunks)operator/config/network-policies/configure-namespace.sh(1 hunks)operator/config/network-policies/deploy.yaml(1 hunks)operator/config/network-policies/forklift-api.yaml(1 hunks)operator/config/network-policies/forklift-controller.yaml(1 hunks)operator/config/network-policies/kustomization.yaml(1 hunks)operator/config/network-policies/migration-workloads.yaml(1 hunks)operator/config/network-policies/validation-pods.yaml(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
operator/config/network-policies/forklift-controller.yaml
[error] 11-11: wrong indentation: expected 4 but found 2
(indentation)
[error] 15-15: wrong indentation: expected 4 but found 2
(indentation)
[error] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 19-19: wrong indentation: expected 6 but found 4
(indentation)
[error] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 29-29: wrong indentation: expected 4 but found 2
(indentation)
[error] 30-30: wrong indentation: expected 6 but found 4
(indentation)
[error] 33-33: wrong indentation: expected 6 but found 4
(indentation)
[error] 38-38: wrong indentation: expected 6 but found 4
(indentation)
[error] 44-44: wrong indentation: expected 6 but found 4
(indentation)
[warning] 63-63: too few spaces before comment: expected 2
(comments)
[warning] 65-65: too few spaces before comment: expected 2
(comments)
operator/config/network-policies/deploy.yaml
[error] 8-8: wrong indentation: expected 4 but found 2
(indentation)
[error] 9-9: wrong indentation: expected 6 but found 4
(indentation)
[error] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 17-17: wrong indentation: expected 6 but found 4
(indentation)
[error] 21-21: wrong indentation: expected 6 but found 4
(indentation)
[error] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 28-28: wrong indentation: expected 6 but found 4
(indentation)
[error] 34-34: wrong indentation: expected 4 but found 2
(indentation)
[error] 43-43: wrong indentation: expected 4 but found 2
(indentation)
[error] 45-45: wrong indentation: expected 6 but found 4
(indentation)
[error] 49-49: wrong indentation: expected 6 but found 4
(indentation)
[error] 55-55: wrong indentation: expected 6 but found 4
(indentation)
[error] 69-69: wrong indentation: expected 6 but found 4
(indentation)
[error] 75-75: wrong indentation: expected 4 but found 2
(indentation)
[error] 76-76: wrong indentation: expected 6 but found 4
(indentation)
[error] 84-84: wrong indentation: expected 4 but found 2
(indentation)
[error] 94-94: wrong indentation: expected 4 but found 2
(indentation)
[error] 95-95: wrong indentation: expected 6 but found 4
(indentation)
[error] 97-97: wrong indentation: expected 6 but found 4
(indentation)
[error] 101-101: wrong indentation: expected 6 but found 4
(indentation)
[error] 106-106: wrong indentation: expected 6 but found 4
(indentation)
[error] 129-129: wrong indentation: expected 4 but found 2
(indentation)
[error] 130-130: wrong indentation: expected 6 but found 4
(indentation)
[error] 132-132: wrong indentation: expected 6 but found 4
(indentation)
[error] 136-136: wrong indentation: expected 6 but found 4
(indentation)
[error] 143-143: wrong indentation: expected 4 but found 2
(indentation)
[error] 153-153: wrong indentation: expected 4 but found 2
(indentation)
[error] 155-155: wrong indentation: expected 6 but found 4
(indentation)
[error] 159-159: wrong indentation: expected 6 but found 4
(indentation)
[error] 165-165: wrong indentation: expected 6 but found 4
(indentation)
[error] 179-179: wrong indentation: expected 6 but found 4
(indentation)
[error] 185-185: wrong indentation: expected 4 but found 2
(indentation)
[error] 186-186: wrong indentation: expected 6 but found 4
(indentation)
[error] 193-193: wrong indentation: expected 4 but found 2
(indentation)
[error] 203-203: wrong indentation: expected 4 but found 2
(indentation)
[error] 205-205: wrong indentation: expected 6 but found 4
(indentation)
[error] 209-209: wrong indentation: expected 6 but found 4
(indentation)
[error] 219-219: wrong indentation: expected 6 but found 4
(indentation)
[error] 233-233: wrong indentation: expected 4 but found 2
(indentation)
[error] 234-234: wrong indentation: expected 6 but found 4
(indentation)
[error] 242-242: wrong indentation: expected 4 but found 2
(indentation)
[error] 252-252: wrong indentation: expected 4 but found 2
(indentation)
[error] 254-254: wrong indentation: expected 6 but found 4
(indentation)
[error] 258-258: wrong indentation: expected 6 but found 4
(indentation)
[error] 264-264: wrong indentation: expected 6 but found 4
(indentation)
[error] 278-278: wrong indentation: expected 4 but found 2
(indentation)
[error] 279-279: wrong indentation: expected 6 but found 4
(indentation)
[error] 287-287: wrong indentation: expected 4 but found 2
(indentation)
[error] 297-297: wrong indentation: expected 4 but found 2
(indentation)
[error] 299-299: wrong indentation: expected 6 but found 4
(indentation)
[error] 304-304: wrong indentation: expected 6 but found 4
(indentation)
[error] 308-308: wrong indentation: expected 6 but found 4
(indentation)
[error] 313-313: wrong indentation: expected 6 but found 4
(indentation)
[error] 315-315: wrong indentation: expected 6 but found 4
(indentation)
[error] 321-321: wrong indentation: expected 6 but found 4
(indentation)
[error] 335-335: wrong indentation: expected 6 but found 4
(indentation)
[error] 339-339: wrong indentation: expected 4 but found 2
(indentation)
[error] 340-340: wrong indentation: expected 6 but found 4
(indentation)
[error] 344-344: wrong indentation: expected 6 but found 4
(indentation)
[error] 352-352: wrong indentation: expected 4 but found 2
(indentation)
[error] 362-362: wrong indentation: expected 4 but found 2
(indentation)
[error] 364-364: wrong indentation: expected 6 but found 4
(indentation)
[error] 368-368: wrong indentation: expected 6 but found 4
(indentation)
[error] 373-373: wrong indentation: expected 6 but found 4
(indentation)
[error] 386-386: wrong indentation: expected 4 but found 2
(indentation)
[error] 387-387: wrong indentation: expected 6 but found 4
(indentation)
[error] 394-394: wrong indentation: expected 4 but found 2
(indentation)
[error] 404-404: wrong indentation: expected 4 but found 2
(indentation)
[error] 406-406: wrong indentation: expected 6 but found 4
(indentation)
[error] 410-410: wrong indentation: expected 6 but found 4
(indentation)
[error] 418-418: wrong indentation: expected 6 but found 4
(indentation)
[error] 432-432: wrong indentation: expected 6 but found 4
(indentation)
[error] 438-438: wrong indentation: expected 4 but found 2
(indentation)
[error] 439-439: wrong indentation: expected 6 but found 4
(indentation)
[error] 447-447: wrong indentation: expected 4 but found 2
(indentation)
[error] 457-457: wrong indentation: expected 4 but found 2
(indentation)
[error] 459-459: wrong indentation: expected 6 but found 4
(indentation)
[error] 463-463: wrong indentation: expected 6 but found 4
(indentation)
[error] 469-469: wrong indentation: expected 6 but found 4
(indentation)
[error] 483-483: wrong indentation: expected 4 but found 2
(indentation)
[error] 484-484: wrong indentation: expected 6 but found 4
(indentation)
[error] 492-492: wrong indentation: expected 4 but found 2
(indentation)
operator/config/network-policies/forklift-api.yaml
[error] 12-12: wrong indentation: expected 4 but found 2
(indentation)
[error] 16-16: wrong indentation: expected 4 but found 2
(indentation)
[error] 17-17: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 26-26: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: wrong indentation: expected 6 but found 4
(indentation)
[error] 35-35: wrong indentation: expected 6 but found 4
(indentation)
[error] 39-39: wrong indentation: expected 6 but found 4
(indentation)
operator/config/network-policies/kustomization.yaml
[error] 6-6: wrong indentation: expected at least 1
(indentation)
operator/config/network-policies/validation-pods.yaml
[error] 13-13: wrong indentation: expected 4 but found 2
(indentation)
[error] 17-17: wrong indentation: expected 4 but found 2
(indentation)
[error] 18-18: wrong indentation: expected 6 but found 4
(indentation)
[error] 23-23: wrong indentation: expected 4 but found 2
(indentation)
[error] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 30-30: wrong indentation: expected 6 but found 4
(indentation)
[error] 36-36: wrong indentation: expected 6 but found 4
(indentation)
[error] 58-58: wrong indentation: expected 4 but found 2
(indentation)
[error] 62-62: wrong indentation: expected 4 but found 2
(indentation)
[error] 63-63: wrong indentation: expected 6 but found 4
(indentation)
[error] 68-68: wrong indentation: expected 6 but found 4
(indentation)
[error] 74-74: wrong indentation: expected 4 but found 2
(indentation)
[error] 75-75: wrong indentation: expected 6 but found 4
(indentation)
[error] 81-81: wrong indentation: expected 6 but found 4
(indentation)
[error] 88-88: wrong indentation: expected 6 but found 4
(indentation)
[error] 92-92: wrong indentation: expected 6 but found 4
(indentation)
[error] 97-97: wrong indentation: expected 6 but found 4
(indentation)
[error] 104-104: wrong indentation: expected 6 but found 4
(indentation)
[error] 119-119: wrong indentation: expected 6 but found 4
(indentation)
[error] 131-131: wrong indentation: expected 4 but found 2
(indentation)
[error] 135-135: wrong indentation: expected 4 but found 2
(indentation)
[error] 136-136: wrong indentation: expected 6 but found 4
(indentation)
[error] 141-141: wrong indentation: expected 4 but found 2
(indentation)
[error] 142-142: wrong indentation: expected 6 but found 4
(indentation)
[error] 149-149: wrong indentation: expected 6 but found 4
(indentation)
[error] 156-156: wrong indentation: expected 6 but found 4
(indentation)
[error] 171-171: wrong indentation: expected 6 but found 4
(indentation)
[error] 177-177: too many blank lines (3 > 0)
(empty-lines)
operator/config/network-policies/migration-workloads.yaml
[error] 13-13: wrong indentation: expected 4 but found 2
(indentation)
[error] 17-17: wrong indentation: expected 4 but found 2
(indentation)
[error] 18-18: wrong indentation: expected 6 but found 4
(indentation)
[error] 24-24: wrong indentation: expected 4 but found 2
(indentation)
[error] 25-25: wrong indentation: expected 6 but found 4
(indentation)
[error] 32-32: wrong indentation: expected 6 but found 4
(indentation)
[warning] 37-37: too few spaces before comment: expected 2
(comments)
[error] 41-41: wrong indentation: expected 6 but found 4
(indentation)
[error] 56-56: wrong indentation: expected 6 but found 4
(indentation)
[error] 70-70: wrong indentation: expected 4 but found 2
(indentation)
[error] 74-74: wrong indentation: expected 4 but found 2
(indentation)
[error] 75-75: wrong indentation: expected 6 but found 4
(indentation)
[error] 81-81: wrong indentation: expected 4 but found 2
(indentation)
[error] 82-82: wrong indentation: expected 6 but found 4
(indentation)
[error] 89-89: wrong indentation: expected 6 but found 4
(indentation)
[error] 96-96: wrong indentation: expected 6 but found 4
(indentation)
[error] 111-111: wrong indentation: expected 6 but found 4
(indentation)
[error] 125-125: wrong indentation: expected 4 but found 2
(indentation)
[error] 129-129: wrong indentation: expected 4 but found 2
(indentation)
[error] 130-130: wrong indentation: expected 6 but found 4
(indentation)
[error] 136-136: wrong indentation: expected 4 but found 2
(indentation)
[error] 137-137: wrong indentation: expected 6 but found 4
(indentation)
[error] 144-144: wrong indentation: expected 6 but found 4
(indentation)
[error] 151-151: wrong indentation: expected 6 but found 4
(indentation)
[error] 173-173: wrong indentation: expected 4 but found 2
(indentation)
[error] 177-177: wrong indentation: expected 4 but found 2
(indentation)
[error] 178-178: wrong indentation: expected 6 but found 4
(indentation)
[error] 184-184: wrong indentation: expected 4 but found 2
(indentation)
[error] 185-185: wrong indentation: expected 6 but found 4
(indentation)
[error] 192-192: wrong indentation: expected 6 but found 4
(indentation)
[error] 203-203: wrong indentation: expected 6 but found 4
(indentation)
[error] 225-225: wrong indentation: expected 4 but found 2
(indentation)
[error] 229-229: wrong indentation: expected 4 but found 2
(indentation)
[error] 230-230: wrong indentation: expected 6 but found 4
(indentation)
[error] 236-236: wrong indentation: expected 4 but found 2
(indentation)
[error] 237-237: wrong indentation: expected 6 but found 4
(indentation)
[error] 244-244: wrong indentation: expected 6 but found 4
(indentation)
[error] 251-251: wrong indentation: expected 6 but found 4
(indentation)
[error] 265-265: too many blank lines (3 > 0)
(empty-lines)
🔇 Additional comments (5)
operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml (1)
11-12: Feature flag sample added — looks good; confirm string vs boolean handling is intentionalSample uses quoted strings ("false") like other flags. Ansible’s
|boolfilter typically interprets "false" correctly, but mixing booleans and strings across sources can confuse users. Consider noting accepted values in docs or unifying representation to booleans in samples.operator/roles/forkliftcontroller/defaults/main.yml (1)
12-12: Defaults extended with NetworkPolicy support — align cleanup expectations
- Adding
feature_network_policies: falseand appendingNetworkPolicytoforklift_resourcesis consistent with the new feature.- Verify that generic cleanup tasks that loop over
forklift_resourceswon’t try to remove unrelated NetworkPolicy objects (e.g., if they key off labels that do not exist on these policies).Also applies to: 23-23
operator/roles/forkliftcontroller/tasks/main.yml (1)
238-261: NetworkPolicy creation tasks: good gating and structureTasks create controller, API, migration workloads, and validation policies behind
feature_network_policies|bool. LGTM.operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2 (1)
137-143: No action needed: UDP is already allowed for NFS egress
The snippet already permits both TCP and UDP on port 2049, so there’s nothing further to add here.Likely an incorrect or invalid review comment.
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (1)
18-24: Ingress from controller uses ‘ports: []’; omit ports to allow traffic- from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted)Likely an incorrect or invalid review comment.
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2
Outdated
Show resolved
Hide resolved
Hazanel
force-pushed
the
networkPolicies
branch
4 times, most recently
from
4766a7e to
3fab263
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (9)
operator/.kustomized_manifests (1)
5145-5145: Also expose this flag in the CSV initialization-resourceThe CR here includes
"feature_network_policies": "false", but the CSV’soperatorframework.io/initialization-resourcespec below still omits it. Add it for visibility/UX parity.Suggested JSON snippet in the CSV init resource:
"spec": { "feature_ui_plugin": "true", "feature_validation": "true", "feature_volume_populator": "true", + "feature_network_policies": "false" }operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2 (1)
6-6: Use the correct namespace variable
forklift_namespaceis likely undefined in this role. Use{{ app_namespace }}which other templates/tasks use.- namespace: {{ forklift_namespace }} + namespace: {{ app_namespace }}operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2 (1)
31-53: Remove duplicate 8443 entryTCP 8443 is listed twice. Keep a single entry with a clear comment.
- protocol: TCP port: 8443 # HTTPS alternate @@ - - protocol: TCP - port: 8443 # oVirt Engine HTTPSoperator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2 (6)
16-23: Ingress ‘ports: []’ blocks controller → vddk-validation trafficRemove
ports: [](allow all) or enumerate specific ports. As-is, it denies traffic.ingress: # Allow controller communication - from: - podSelector: matchLabels: control-plane: controller-manager - ports: []
24-30: Egress ‘ports: []’ blocks vddk-validation → controller trafficSame issue on egress. Omit
portsor list required port(s).egress: # Allow communication with controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: []
58-64: Ingress ‘ports: []’ blocks controller → validation serviceThis denies traffic. Remove
portsor specify the needed port(s).ingress: # Allow controller communication - from: - podSelector: matchLabels: control-plane: controller-manager - ports: []
70-76: Egress ‘ports: []’ blocks validation service → controllerSame pattern—remove or enumerate.
egress: # Allow communication with controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: []
102-108: Ingress ‘ports: []’ blocks controller → image-converterOmit
portsor list the needed port(s).ingress: # Allow controller communication - from: - podSelector: matchLabels: control-plane: controller-manager - ports: []
110-114: Egress ‘ports: []’ blocks image-converter → controllerSame issue—traffic is denied.
egress: # Allow communication with controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: []
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/network-policies.mdis excluded by!**/*.md
📒 Files selected for processing (10)
operator/.kustomized_manifests(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🔇 Additional comments (7)
operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml (1)
11-12: Flag surfaced in sample CR — looks goodConsistent with other feature flags; defaulting to "false" is safe. No issues.
operator/roles/forkliftcontroller/defaults/main.yml (2)
12-12: Sane default for the new feature flagKeeping
feature_network_policies: falseby default is prudent. The role uses|boolin conditions, so type coercion from CR string values is handled.
23-23: Include NetworkPolicy in managed resource kindsAdding
NetworkPolicytoforklift_resourcesaligns cleanup mechanics across features. No issues spotted.operator/roles/forkliftcontroller/tasks/main.yml (1)
301-322: Cleanup list is comprehensiveAll created policy names are covered, including the default policy. Good.
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (1)
24-33: Good: canonical namespace label is used for OpenShift namespacesUsing kubernetes.io/metadata.name for openshift-console and openshift-monitoring is correct and robust across clusters.
Also applies to: 38-47
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2 (2)
76-87: Good use of egress-only port rules for K8s API, DNS, external HTTP(S), and NFSLeaving out
to:makes these rules apply to any destination on the specified ports, which is the correct approach given kube-apiserver/node sources and external services.Also applies to: 115-134
18-21: Verified controller label alignmentThe controller Deployment and Pod templates in
operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2
already include the labelcontrol-plane: controller-managerunder both
metadata.labelsandspec.template.metadata.labels. All your NetworkPolicy templates reference the same selector, so the ingress/egress rules will correctly match the controller pods. No changes required.
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
operator/.kustomized_manifests (1)
4896-4977: Add missing NetworkPolicy RBAC to ClusterRolemanager-roleThe operator deploys and manages NetworkPolicy resources in:
operator/roles/forkliftcontroller/templates/security/networkpolicy-*.yml.j2operator/roles/forkliftcontroller/tasks/main.yml:303But the ClusterRole
manager-roleinoperator/.kustomized_manifestsdoes not include any rules fornetworking.k8s.io/v1networkpolicies. Without these permissions, attempts to create, update, patch or delete NetworkPolicies will be forbidden.Please add this under the
rules:section of themanager-roleClusterRole:rules: + - apiGroups: + - networking.k8s.io + resources: + - networkpolicies + verbs: + - get + - list + - watch + - create + - update + - patch + - delete
♻️ Duplicate comments (4)
operator/.kustomized_manifests (1)
5281-5294: Add feature_network_policies to CSV initialization-resource for operator UX consistencyThe CSV’s operatorframework.io/initialization-resource omits feature_network_policies (defaults to false anyway). Add it for visibility and parity with the CR.
"spec": { "feature_ui_plugin": "true", "feature_validation": "true", - "feature_volume_populator": "true" + "feature_volume_populator": "true", + "feature_network_policies": "false" }operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2 (1)
6-6: Use app_namespace variableforklift_namespace is likely undefined in this role; other templates use app_namespace. Use {{ app_namespace }} for consistency.
- namespace: {{ forklift_namespace }} + namespace: {{ app_namespace }}operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2 (1)
24-29: Remove ‘ports: []’ from controller allow rules (it denies all ports)All these ingress/egress entries intend to allow controller↔workload traffic but ports: [] blocks it.
Apply this diff across the listed blocks:
# virt-v2v egress to controller egress: # Allow communication with controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # consumer ingress from controller ingress: # Allow controller communication - from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # consumer egress to controller egress: # Allow communication with controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # ovirt-populator ingress/egress, openstack-populator ingress/egress, # vsphere-xcopy-populator ingress/egress: # repeat the same change (remove 'ports: []' under those blocks).Also applies to: 80-86, 88-93, 125-131, 132-138, 166-172, 173-179, 213-219, 220-226
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2 (1)
16-23: Remove ‘ports: []’ from controller allow rules (it denies all ports)Same pattern here: ports: [] blocks controller↔validation/image‑converter traffic.
Apply this diff across the listed blocks:
# vddk-validation ingress from controller - from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # vddk-validation egress to controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # validation-service ingress from controller - from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # validation-service egress to controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # image-converter ingress from controller - from: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted) # image-converter egress to controller - to: - podSelector: matchLabels: control-plane: controller-manager - ports: [] + # all ports allowed (ports omitted)Also applies to: 24-30, 58-64, 70-76, 101-107, 108-115
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/network-policies.mdis excluded by!**/*.md
📒 Files selected for processing (10)
operator/.kustomized_manifests(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🔇 Additional comments (5)
operator/roles/forkliftcontroller/defaults/main.yml (2)
12-12: Default added — LGTMfeature_network_policies: false default is consistent with other feature flags here. No issues.
23-23: Include of NetworkPolicy in forklift_resources — LGTMAdding NetworkPolicy to forklift_resources aligns with the new feature. Ensure RBAC allows managing these objects (see RBAC note in CSV bundle comment).
operator/.kustomized_manifests (1)
5145-5145: CR includes feature_network_policies — LGTMSpec exposes "feature_network_policies": "false" alongside other flags. Matches sample and defaults.
operator/roles/forkliftcontroller/tasks/main.yml (1)
301-321: Cleanup list looks complete and consistentAll created NetworkPolicy resources are removed when the feature is disabled, including default‑deny.
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (1)
25-33: Selectors look good; keep using canonical namespace labelUsing kubernetes.io/metadata.name for openshift-console and openshift-monitoring is correct and consistent.
Also applies to: 39-47
operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2
Outdated
Show resolved
Hide resolved
Hazanel
force-pushed
the
networkPolicies
branch
from
3fab263 to
0e62755
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2 (2)
6-6: Wrong namespace variable: use app_namespace (forklift_namespace likely undefined here)Switch to the role’s namespace variable to avoid rendering/apply failures.
Apply:
- namespace: {{ forklift_namespace }} + namespace: {{ app_namespace }}Verification script to confirm variable definitions/usage:
#!/bin/bash # Show where forklift_namespace is referenced and whether it's defined rg -n --no-heading $'forklift_namespace|app_namespace' -S echo echo "Check defaults for forklift_namespace definition:" rg -n --no-heading -S 'forklift_namespace:' operator/roles/forkliftcontroller/defaults/main.yml || echo "Not defined in defaults."
20-24: Make console ingress optional or remove if not neededUnconditionally allowing openshift-console increases surface. Gate it with a flag or drop it.
Example:
- # Allow OpenShift console access - - from: - - namespaceSelector: - matchLabels: - kubernetes.io/metadata.name: openshift-console +{% if allow_console_ingress | default(false) %} + # Allow OpenShift console access (optional) + - from: + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: openshift-console +{% endif %}operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2 (1)
31-53: Duplicate 8443 in virt‑v2v external ports — remove redundancy8443 appears twice; keep one and consolidate the comment.
- ports: - protocol: TCP port: 443 # HTTPS - protocol: TCP - port: 8443 # HTTPS alternate + port: 8443 # HTTPS alternate / oVirt Engine HTTPS - protocol: TCP port: 902 # vSphere NFC @@ - protocol: TCP - port: 8443 # oVirt Engine HTTPS + # 8443 covered aboveoperator/config/samples/forklift_v1beta1_forkliftcontroller.yaml (1)
11-12: Clarify that policies are permissive by default unless deny-all is enabledAs shipped, the “default” policy allows all traffic. Enablement alone doesn’t increase security until you flip default-deny.
- # Enable network policies for enhanced security (set to "true" to enable) + # Enable NetworkPolicies (permissive by default). Set to "true" to install policies; + # switch the default policy to deny-all to enforce restrictions.operator/.kustomized_manifests (1)
5281-5294: Also add feature_network_policies to the CSV initialization-resource specKeeps init manifest aligned with CR defaults and surfaces the flag to users.
"spec": { "feature_ui_plugin": "true", "feature_validation": "true", - "feature_volume_populator": "true" + "feature_volume_populator": "true", + "feature_network_policies": "false" }operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2 (1)
18-23: “Default-deny” is currently allow-all; flip to deny-all or gate behind a variableWith - {} rules, this policy permits all traffic and defeats other policies.
Recommend deny-all:
- ingress: - # PASS-THROUGH MODE: Allow all ingress traffic - - {} - egress: - # PASS-THROUGH MODE: Allow all egress traffic - - {} + ingress: [] # Deny all ingress by default + egress: [] # Deny all egress by defaultAlternatively, parameterize:
{% if network_policy_permissive | default(true) %} ingress: [ {} ] egress: [ {} ] {% else %} ingress: [] egress: [] {% endif %}operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2 (1)
42-45: Optional: narrow intra-namespace egress to Forklift podsto: podSelector: {} permits egress to any pod in the namespace. Restrict to Forklift pods to reduce blast radius.
- - to: - - podSelector: {} + - to: + - podSelector: + matchLabels: + app: {{ app_name }}operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (1)
37-40: Critical:from: []blocks all traffic instead of allowing any sourceThe
from: []creates an empty source selector that matches no sources, effectively denying all traffic on port 8443. For webhook traffic from the Kubernetes API server, this should allow any source.Apply this fix:
- # Allow Kubernetes API server for webhooks - - from: [] - ports: + # Allow Kubernetes API server for webhooks + - ports: - protocol: TCP port: 8443
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/network-policies.mdis excluded by!**/*.md
📒 Files selected for processing (10)
operator/.kustomized_manifests(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🔇 Additional comments (8)
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2 (1)
16-30: LGTM: fixed NetworkPolicy port semanticsEmpty ports lists removed; rules now correctly allow intended traffic. DNS/API egress specified explicitly.
Also applies to: 57-75
operator/.kustomized_manifests (1)
5145-5145: Expose feature_network_policies in the CR spec in this bundle — LGTMField added to ForkliftController spec: good.
operator/roles/forkliftcontroller/defaults/main.yml (1)
12-12: Defaults updated for NetworkPolicies — LGTMNew flag and resource type included; aligns with added templates and tasks.
Also applies to: 23-23
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2 (1)
11-28: LGTM: correct selectors and DNS/API egress
- Uses control-plane: controller-manager as selector; matches other templates.
- DNS egress uses explicit TCP/UDP 53; no empty ports lists.
- Ingress block is conditionally rendered for OCP; avoids empty list on k8s.
Also applies to: 29-41
operator/roles/forkliftcontroller/tasks/main.yml (2)
238-272: LGTM! Network policy setup is well-structured and properly guarded.The network policy creation tasks are well-organized with:
- Consistent naming convention and structure
- Proper feature flag gating with
when: feature_network_policies|bool- Logical ordering (default-deny first, then specific allow policies)
- Clear task descriptions
The implementation follows best practices by applying the baseline default-deny policy first, followed by specific allow policies.
301-321: Comprehensive cleanup implementation looks good.The cleanup task properly removes all NetworkPolicy resources when the feature is disabled. The extensive list (12 policies) indicates that some templates create multiple policies, ensuring no orphaned resources remain when toggling the feature off.
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (2)
51-75: Excellent egress rules implementation.The egress rules are comprehensive and correctly address the requirement for API pods to maintain essential connectivity when default-deny policies are enforced:
- DNS resolution (UDP/TCP 53) for name lookups
- HTTPS (443) for certificate fetching and external validation
- Kubernetes API access (6443, 443) for cluster communication
- Controller communication (8081) for internal coordination
This ensures the API service remains functional with the default-deny baseline.
1-16: Well-structured NetworkPolicy template with proper metadata.The template has a clean structure with:
- Correct Kubernetes API version and kind
- Proper use of Jinja2 templating variables
- Appropriate podSelector targeting the API service
- Both Ingress and Egress policy types declared
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
operator/.kustomized_manifests (1)
4896-4977: Add missing RBAC permissions for NetworkPoliciesThe operator currently lacks any rules granting access to
networking.k8s.io/networkpolicies. Deployments that enable or manage NetworkPolicies will fail with “RBAC: forbidden” errors.Affected location:
- operator/.kustomized_manifests (around lines 4896–4977), ClusterRole
manager-roleProposed change:
kind: ClusterRole metadata: name: manager-role rules: + - apiGroups: + - networking.k8s.io + resources: + - networkpolicies + verbs: + - get + - list + - watch + - create + - update + - patch + - delete
♻️ Duplicate comments (6)
operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml (1)
11-12: Clarify permissive-by-default behavior of the new flagEnabling the feature deploys NetworkPolicies, but until a default-deny is applied, policies here are largely pass-through. Consider expanding the comment to reflect that enabling “network policies” does not enforce deny by default.
operator/.kustomized_manifests (1)
5281-5294: Surface feature_network_policies in CSV initialization-resource for UX parityInclude the new flag so the init CR mirrors the defaults and exposes the feature explicitly.
"spec": { "feature_ui_plugin": "true", "feature_validation": "true", - "feature_volume_populator": "true" + "feature_volume_populator": "true", + "feature_network_policies": "false" }operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2 (1)
4-8: Use app_namespace instead of undefined forklift_namespaceThe role defaults expose app_namespace. forklift_namespace is likely undefined here and will render empty/incorrect.
metadata: name: forklift-controller-policy - namespace: {{ forklift_namespace }} + namespace: {{ app_namespace }}operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (1)
36-40: Ingress ‘from: []’ blocks traffic; omit ‘from’ to allow any-source on 8443
from: []matches no sources and denies webhooks. Remove ‘from’ so the 8443 rule applies to any source.- # Allow Kubernetes API server for webhooks - - from: [] - ports: + # Allow Kubernetes API server for webhooks (any source) + - ports: - protocol: TCP port: 8443operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2 (1)
12-23: “Default-deny” currently allows all traffic; flip to real deny-all or parameterize permissive modeWith podSelector: {} and ingress/egress rules set to “- {}”, this policy becomes allow-all for all pods and nullifies any restrictive policies. That defeats the goal of enabling NetworkPolicies.
Recommended fix (deny-by-default):
spec: podSelector: {} # Applies to all pods in the namespace policyTypes: - Ingress - Egress - ingress: - # PASS-THROUGH MODE: Allow all ingress traffic - - {} - egress: - # PASS-THROUGH MODE: Allow all egress traffic - - {} + ingress: [] # Deny all ingress by default + egress: [] # Deny all egress by defaultIf you need a permissive rollout, parameterize:
spec: podSelector: {} policyTypes: - Ingress - Egress {% if network_policy_permissive | default(false) %} ingress: [ {} ] egress: [ {} ] {% else %} ingress: [] egress: [] {% endif %}Also consider aligning the resource/task naming if pass-through remains (e.g., “default-allow”) to avoid operator confusion.
operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2 (1)
42-45: Narrow broad intra‑namespace egress to Forklift podsThe last rule allows egress to any pod in the namespace on all ports. Reduce lateral movement by scoping to Forklift-managed pods.
- # Allow communication with Forklift components for management - - to: - - podSelector: {} + # Allow communication with Forklift components for management (limit to Forklift pods) + - to: + - podSelector: + matchLabels: + app: {{ app_name }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/network-policies.mdis excluded by!**/*.md
📒 Files selected for processing (10)
operator/.kustomized_manifests(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(2 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-default-deny.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2(1 hunks)
🔥 Files not summarized due to errors (1)
- operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2: Error: Server error: no LLM provider could handle the message
🔇 Additional comments (7)
operator/.kustomized_manifests (1)
5145-5145: Good: CR spec includes feature_network_policiesAddition looks correct and consistent with defaults/tasks.
operator/roles/forkliftcontroller/defaults/main.yml (1)
12-12: LGTM: flag default and resource kind registered
- feature_network_policies: false default is appropriate.
- Adding NetworkPolicy to forklift_resources is correct.
Also applies to: 23-23
operator/roles/forkliftcontroller/templates/security/networkpolicy-validation.yml.j2 (1)
16-44: Validation policies look correct after fixesIngress/egress semantics are sound (no empty ports/to; correct port scoping for DNS, API, NFS, and provider access).
Also applies to: 58-87, 101-135
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2 (1)
51-76: Egress/Controller rule: verify port 8081 is correct targetIf the controller exposes a different service/port for API communications, adjust accordingly. Otherwise, this looks fine.
Would you confirm the controller’s metrics/status port and whether API needs egress to it? If not required, you can drop this rule to reduce surface.
operator/roles/forkliftcontroller/tasks/main.yml (2)
238-243: Invert Default-Deny Placement When Enabling True Deny-All
- In operator/roles/forkliftcontroller/tasks/main.yml (around line 238), you currently create the baseline
default-denyfirst, then deploy the specific allow policies (operator, controller, API, migration, validation).- While “pass-through” deny is safe in this order, switching to a true deny-all baseline requires you to apply allow policies first and only then enforce default-deny—this avoids transient outages during rollout.
- No change is needed today; just remember to reorder those tasks (allow-list → default-deny) when you flip
networkpolicy-default-deny.yml.j2to a strict deny-all policy.
301-322: Cleanup looks complete and gated correctlyVerified that every NetworkPolicy defined under
operator/roles/forkliftcontroller/templates/securityis listed in the cleanup loop intasks/main.ymland will be removed whenfeature_network_policiesis disabled (includingforklift-default-deny). LGTM.operator/roles/forkliftcontroller/templates/security/networkpolicy-operator.yml.j2 (1)
15-17: Confirm intent: Ingress is denied on Kubernetes clusterspolicyTypes always include Ingress but the ingress block is emitted only on OpenShift. On vanilla Kubernetes, this denies all ingress to operator pods. If that’s not desired, either:
- make policyTypes conditional, or
- add an explicit ingress allowlist for required ports on k8s_cluster too.
Also applies to: 18-28
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-api.yml.j2
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-controller.yml.j2
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-hooks.yml.j2
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
Hazanel
force-pushed
the
networkPolicies
branch
8 times, most recently
from
56fbb60 to
66d63ba
Compare
Hazanel
force-pushed
the
networkPolicies
branch
from
66d63ba to
4e87b2b
Compare
Hazanel
changed the title
Hazanel
force-pushed
the
networkPolicies
branch
from
4e87b2b to
5a5d59b
Compare
|
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
operator/roles/forkliftcontroller/templates/security/networkpolicy-migration-workloads.yml.j2
Outdated
Show resolved
Hide resolved
Hazanel
force-pushed
the
networkPolicies
branch
2 times, most recently
from
332a9f6 to
c86e884
Compare
Hazanel
force-pushed
the
networkPolicies
branch
from
c86e884 to
2ee15f2
Compare
|
Global network policies enablement due date is currently July 1st |
Hazanel
force-pushed
the
networkPolicies
branch
2 times, most recently
from
3f93d64 to
01df98c
Compare
Hazanel
force-pushed
the
networkPolicies
branch
from
01df98c to
0b03971
Compare
…e enablement logic Ref: https://issues.redhat.com/browse/MTV-2678 Signed-off-by: Elad Hazan <ehazan@redhat.com>
…c for enablement Resolves: None Signed-off-by: Elad Hazan <ehazan@redhat.com>
- version-aware enablement refactor - Added HyperV network policy Resolves: None Signed-off-by: Elad Hazan <ehazan@redhat.com>
Hazanel
force-pushed
the
networkPolicies
branch
from
0b03971 to
8c8649d
Compare
|
5 participants
Ref: https://issues.redhat.com/browse/MTV-2678
Signed-off-by: Elad Hazan ehazan@redhat.com
Summary by CodeRabbit
New Features
Chores