OCPBUGS-61175: Create NetworkPolicy for Manila CSI driver - #424
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@mpatlasov: This pull request references Jira Issue OCPBUGS-61175, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpatlasov 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 |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@mpatlasov: This pull request references Jira Issue OCPBUGS-61175, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (wduan@redhat.com), skipping review request. 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. |
The operator needs permissions to publish and monitor NPs in the custom namespace `openshift-manila-csi-driver`. See openshift/csi-operator#424 for details.
|
/testwith openshift/csi-operator/main/e2e-openstack-manila-csi openshift/cluster-storage-operator#615 |
2 similar comments
|
/testwith openshift/csi-operator/main/e2e-openstack-manila-csi openshift/cluster-storage-operator#615 |
|
/testwith openshift/csi-operator/main/e2e-openstack-manila-csi openshift/cluster-storage-operator#615 |
|
/testwith openshift/csi-operator/main/e2e-openstack-manila-csi openshift/cluster-storage-operator#615 |
The operator needs permissions to publish and monitor NPs in the custom namespace `openshift-manila-csi-driver`. See openshift/csi-operator#424 for details.
|
/testwith openshift/csi-operator/main/e2e-openstack-manila-csi openshift/cluster-storage-operator#615 |
c558a95 to
10f7c82
Compare
mpatlasov
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
|
and |
|
/hold My previous comment about outdated CSO image was red-herring. The latest fail of This means that openshift/cluster-storage-operator#615 helped to standalone case, but not to hypershift one. Need to investigate why. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/testwith openshift/csi-operator/main/hypershift-e2e-openstack-aws-csi-manila openshift/cluster-storage-operator#626 |
|
/testwith openshift/csi-operator/main/hypershift-e2e-openstack-aws-csi-manila openshift/cluster-storage-operator#626 |
|
/testwith openshift/csi-operator/main/hypershift-e2e-openstack-aws-csi-manila openshift/hypershift#6930 |
|
/testwith openshift/csi-operator/main/hypershift-e2e-openstack-aws-csi-manila openshift/hypershift#6930,openshift/cluster-storage-operator#626 |
|
/testwith openshift/csi-operator/main/hypershift-e2e-openstack-aws-csi-manila openshift/hypershift#6930 openshift/cluster-storage-operator#626 |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
📝 WalkthroughWalkthroughThe change adds four OpenStack Manila Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test hypershift-e2e-openstack-aws-csi-manila |
Manila csi driver controller pods reside in a custome namespace (`openshift-manila-csi-driver`). Hence, Manila csi driver operator must create NetworkPolicy for them expicitly.
10f7c82 to
0f64d6d
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/test hypershift-e2e-openstack-aws-csi-manila |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml`:
- Around line 3-5: Prefix each NetworkPolicy metadata.name with ${ASSET_PREFIX}:
update
assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml:3-5
to ${ASSET_PREFIX}-allow-all-egress,
assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml:3-5
to ${ASSET_PREFIX}-allow-egress-to-api-server,
assets/overlays/openstack-manila/base/network-policy-allow-to-dns.yaml:3-5 to
${ASSET_PREFIX}-allow-to-dns, and
assets/overlays/openstack-manila/base/network-policy-allow-ingress-to-metrics.yaml:3-5
to ${ASSET_PREFIX}-allow-ingress-to-metrics-range.
In
`@assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml`:
- Around line 16-19: Update the egress rules in the controller-selected network
policies, replacing the broad allow-all-egress behavior with topology-specific
API-server peers. Retain TCP port 6443 for API-server access and add only the
required DNS and metrics exceptions, ensuring no unrestricted TCP 1–65535 rule
remains.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca7337fc-7771-4934-a3cd-5eea109cda2d
⛔ Files ignored due to path filters (10)
assets/overlays/openstack-manila/generated/hypershift/manifests.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/hypershift/network-policy-allow-all-egress.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/hypershift/network-policy-allow-egress-to-api-server.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/hypershift/network-policy-allow-ingress-to-metrics.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/hypershift/network-policy-allow-to-dns.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/manifests.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/network-policy-allow-all-egress.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/network-policy-allow-egress-to-api-server.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/network-policy-allow-ingress-to-metrics.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/network-policy-allow-to-dns.yamlis excluded by!**/generated/**
📒 Files selected for processing (5)
assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yamlassets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yamlassets/overlays/openstack-manila/base/network-policy-allow-ingress-to-metrics.yamlassets/overlays/openstack-manila/base/network-policy-allow-to-dns.yamlpkg/driver/openstack-manila/openstack_manila.go
| metadata: | ||
| name: allow-all-egress | ||
| namespace: ${NAMESPACE} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use ${ASSET_PREFIX} in each NetworkPolicy name.
The asset templates use generic resource names. Prefix each name with ${ASSET_PREFIX} to identify the owning driver consistently.
assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml#L3-L5: change the name to${ASSET_PREFIX}-allow-all-egress.assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml#L3-L5: change the name to${ASSET_PREFIX}-allow-egress-to-api-server.assets/overlays/openstack-manila/base/network-policy-allow-to-dns.yaml#L3-L5: change the name to${ASSET_PREFIX}-allow-to-dns.assets/overlays/openstack-manila/base/network-policy-allow-ingress-to-metrics.yaml#L3-L5: change the name to${ASSET_PREFIX}-allow-ingress-to-metrics-range.
📍 Affects 4 files
assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml#L3-L5(this comment)assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml#L3-L5assets/overlays/openstack-manila/base/network-policy-allow-to-dns.yaml#L3-L5assets/overlays/openstack-manila/base/network-policy-allow-ingress-to-metrics.yaml#L3-L5
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml`
around lines 3 - 5, Prefix each NetworkPolicy metadata.name with
${ASSET_PREFIX}: update
assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml:3-5
to ${ASSET_PREFIX}-allow-all-egress,
assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml:3-5
to ${ASSET_PREFIX}-allow-egress-to-api-server,
assets/overlays/openstack-manila/base/network-policy-allow-to-dns.yaml:3-5 to
${ASSET_PREFIX}-allow-to-dns, and
assets/overlays/openstack-manila/base/network-policy-allow-ingress-to-metrics.yaml:3-5
to ${ASSET_PREFIX}-allow-ingress-to-metrics-range.
Source: Coding guidelines
| egress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 6443 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find existing API-server NetworkPolicy patterns and the labels that select Manila pods.
rg -n -C 6 'allow-egress-to-api-server|port:\s*6443|api-server:\s*allow|kubernetes\.default' \
--glob '*.{yaml,yml}' --glob '*.go' .
# Inspect Manila controller patches for the selected label and deployment topology.
fd -t f . assets pkg | rg 'openstack-manila|network-policy|controller'Repository: openshift/csi-operator
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Manila base policies ---'
for f in \
assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml \
assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml \
assets/overlays/openstack-manila/base/network-policy-allow-to-dns.yaml
do
echo "--- $f"
cat -n "$f"
done
printf '%s\n' '--- Manila controller labels and asset registration ---'
sed -n '1,115p' pkg/driver/openstack-manila/openstack_manila.go
cat -n assets/overlays/openstack-manila/patches/controller_add_driver.yaml
printf '%s\n' '--- Exact policy selectors and egress peers ---'
python3 - <<'PY'
from pathlib import Path
files = [
Path("assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml"),
Path("assets/overlays/openstack-manila/base/network-policy-allow-all-egress.yaml"),
Path("assets/overlays/openstack-manila/base/network-policy-allow-to-dns.yaml"),
]
for path in files:
text = path.read_text()
print(path)
print(" selects_api_server_label =", "openshift.storage.network-policy.api-server: allow" in text)
print(" selects_all_egress_label =", "openshift.storage.network-policy.all-egress: allow" in text)
print(" has_to_peer =", any(line.lstrip().startswith("to:") for line in text.splitlines()))
print(" has_port_6443 =", "port: 6443" in text)
PYRepository: openshift/csi-operator
Length of output: 14444
Security Misconfiguration (CWE-284)
Reachability: Internal · Exploitability: Moderate
Restrict Manila controller egress.
Both policies select the controller pods. allow-all-egress already permits TCP ports 1–65535, so adding a to peer only to the API-server policy will not enforce isolation. Replace the broad egress policy with topology-specific API-server peers and the required DNS and metrics exceptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@assets/overlays/openstack-manila/base/network-policy-allow-egress-to-api-server.yaml`
around lines 16 - 19, Update the egress rules in the controller-selected network
policies, replacing the broad allow-all-egress behavior with topology-specific
API-server peers. Retain TCP port 6443 for API-server access and add only the
required DNS and metrics exceptions, ensuring no unrestricted TCP 1–65535 rule
remains.
Source: Path instructions
|
@mpatlasov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Manila csi driver controller pods reside in a custome namespace (
openshift-manila-csi-driver). Hence, Manila csi driver operator must create NetworkPolicy for them expicitly.