Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fao89: This pull request references OTA-1608 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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: 1
🤖 Fix all issues with AI agents
In `@config/networkpolicies/operand-egress-openshift-api.yaml`:
- Around line 23-31: The egress rule in operand-egress-openshift-api.yaml uses
only namespaceSelector (matchLabels: kubernetes.io/metadata.name: default) which
allows traffic to all pods in the default namespace on ports 443/6443; restrict
this by adding a podSelector that matches the API server pod/service endpoints
(or replace the target with an ipBlock for the API server IP) so only the
kubernetes API is reachable; update the "to:" block to include a podSelector
with the API server label (or swap to ipBlock) while keeping the existing ports
entries to narrow the allowed targets.
🧹 Nitpick comments (4)
config/networkpolicies/operand-ingress-prometheus.yaml (1)
31-34: Overly permissive ingress rule lacks podSelector.This rule allows ingress from any pod in the
openshift-monitoringnamespace, unlike the other two rules that specifically target Prometheus pods. If this is intentional as a fallback for different label configurations, consider adding a comment explaining why. Otherwise, add apodSelectorto restrict access to Prometheus pods only.♻️ Suggested fix to restrict to Prometheus pods
# Alternative monitoring namespace patterns - namespaceSelector: matchLabels: name: openshift-monitoring + podSelector: + matchLabels: + app.kubernetes.io/name: prometheusconfig/networkpolicies/operator-ingress-prometheus.yaml (1)
31-33: Overly permissive ingress rule lacks podSelector.Same issue as in
operand-ingress-prometheus.yaml— this rule allows ingress from any pod in theopenshift-monitoringnamespace rather than restricting to Prometheus pods.♻️ Suggested fix
# Alternative monitoring namespace patterns - namespaceSelector: matchLabels: name: openshift-monitoring + podSelector: + matchLabels: + app.kubernetes.io/name: prometheusconfig/networkpolicies/operand-egress-internal-registry.yaml (1)
35-42: Redundant and overly permissive egress rule.This rule allows egress to any pod in
openshift-image-registryon port 5000, which makes the more specific rule above (lines 24-33 targetingdocker-registry: default) redundant. If the intent is to reach the registry Service (not pods directly), NetworkPolicies don't filter Service traffic differently — traffic still goes to pods. Consider removing this rule or consolidating with the first rule.♻️ Suggested consolidation - remove redundant rule
ports: - protocol: TCP port: 5000 # Internal registry port - - # Allow access to image registry service - - to: - - namespaceSelector: - matchLabels: - kubernetes.io/metadata.name: openshift-image-registry - ports: - - protocol: TCP - port: 5000 # Allow access to registry console (if enabled)config/networkpolicies/operand-egress-cluster-proxy.yaml (1)
49-53: Rule allows direct egress to any destination on port 443.This rule permits HTTPS traffic to any IP, which duplicates coverage already provided by
operand-egress-external-registries.yamland may not be needed here. The comment mentions "HTTPS tunneling through proxy," but NetworkPolicy evaluates the actual packet destination—when using a CONNECT proxy, the connection goes to the proxy IP on proxy ports (3128/8080), which is already covered by the rules above.Consider removing this rule to avoid redundancy and maintain tighter egress control.
♻️ Suggested fix
# Allow access to proxy pods in openshift-ingress or similar namespaces - to: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: openshift-ingress podSelector: matchLabels: app.kubernetes.io/component: proxy ports: - protocol: TCP port: 3128 - protocol: TCP port: 8080 - - # Allow CONNECT method for HTTPS tunneling through proxy - - to: [] # Proxy may forward to any destination - ports: - - protocol: TCP - port: 443 # HTTPS through proxy tunnel
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fao89 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/networkpolicies/operator-egress-openshift-api.yaml`:
- Around line 22-30: The egress rule currently uses only namespaceSelector
(matchLabels: kubernetes.io/metadata.name: default) which permits traffic to all
pods in the default namespace on ports 443/6443; update the rule to narrow the
target by adding a podSelector that matches the API server pod label (or
replace/augment the destination with an ipBlock for the API server endpoint) so
only the kube-apiserver service/pod is reachable on ports 443 and 6443; ensure
you modify the same egress "to" entry and keep the ports (protocol: TCP, port:
443 and 6443) unchanged.
Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/networkpolicies/operand-egress-external-registries.yaml`:
- Around line 22-36: The egress rules currently use unrestricted destinations
via the two entries with "- to: []" (the egress block allowing ports 443, 80,
8080, 3128), which is overly permissive; change those "- to: []" entries to
explicit destination restrictions by replacing them with targeted ipBlock CIDRs
or specific IP ranges/hosts for your container registries or proxy endpoints (or
use namespace/peer selectors if applicable) for each port group (HTTPS 443, HTTP
80, proxy 8080/3128); if full open egress is required for disconnected/proxy
scenarios, add a clear comment and a configurable allowlist variable so the
permissiveness is explicit and auditable.
🧹 Nitpick comments (1)
config/networkpolicies/operator-egress-openshift-api.yaml (1)
34-49: Consider adding podSelector for consistency.The rules for
openshift-kube-apiserverandopenshift-oauth-apiservernamespaces use onlynamespaceSelector, allowing traffic to all pods in those namespaces. While these are system namespaces with limited attack surface, addingpodSelectorconstraints would provide defense-in-depth consistent with the fix applied to the default namespace rule.
| egress: | ||
| # Allow HTTPS registry access (direct or via proxy) | ||
| - to: [] # Allow to any destination for registry access | ||
| ports: | ||
| - protocol: TCP | ||
| port: 443 # HTTPS for secure registry access | ||
| - protocol: TCP | ||
| port: 80 # HTTP for some registries/proxies | ||
| # Allow proxy access if configured | ||
| - to: [] | ||
| ports: | ||
| - protocol: TCP | ||
| port: 8080 # Common proxy port | ||
| - protocol: TCP | ||
| port: 3128 # Squid proxy port No newline at end of file |
There was a problem hiding this comment.
Egress to any destination is overly permissive.
Using to: [] allows egress to any IP address on these ports (80, 443, 8080, 3128), not just container registries. This effectively permits operand pods to reach any external endpoint on common HTTP/proxy ports, which is a significant security surface.
Consider whether you can restrict destinations to known registry CIDRs or use more specific ipBlock rules. If full flexibility is required for disconnected/proxy environments, ensure this permissiveness is an accepted risk.
🤖 Prompt for AI Agents
In `@config/networkpolicies/operand-egress-external-registries.yaml` around lines
22 - 36, The egress rules currently use unrestricted destinations via the two
entries with "- to: []" (the egress block allowing ports 443, 80, 8080, 3128),
which is overly permissive; change those "- to: []" entries to explicit
destination restrictions by replacing them with targeted ipBlock CIDRs or
specific IP ranges/hosts for your container registries or proxy endpoints (or
use namespace/peer selectors if applicable) for each port group (HTTPS 443, HTTP
80, proxy 8080/3128); if full open egress is required for disconnected/proxy
scenarios, add a clear comment and a configurable allowlist variable so the
permissiveness is explicit and auditable.
|
/test ci/prow/ci-bundle-cincinnati-bundle |
|
/test ci-bundle-cincinnati-bundle |
|
@fao89: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Signed-off-by: Fabricio Aguiar fabricio.aguiar@gmail.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED