OTA-1639: Restrict osus egress namespace#265
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@nader-ziada: This pull request references OTA-1639 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 "5.0.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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors internal-registry detection with constants and a namespaceFromInternalHost parser. Updates newNetworkPolicy to compute a registryEgress rule, set a description annotation, and restrict egress To to a namespace selector when the registry hostname is cluster-internal. Tests added for parsing and egress namespace behavior. ChangesCluster-internal registry egress filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nader-ziada The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🧹 Nitpick comments (2)
controllers/new.go (1)
395-405: 💤 Low valueRefactor to avoid duplicating registry extraction.
The registry is extracted from
instance.Spec.Releaseson line 405, but the same extraction already happens insideegressPortson line 322. Consider extracting the registry once and passing it to both functions, or returning it fromegressPorts.♻️ Suggested refactor
-func egressPorts(releases string) []int32 { +func egressPorts(releases string) ([]int32, string) { seen := map[int32]bool{} var ports []int32 add := func(p int32) { @@ -331,7 +331,7 @@ func egressPorts(releases string) []int32 { } } - return ports + return ports, registry }Then update the caller:
func (k *kubeResources) newNetworkPolicy(instance *cv1.UpdateService) *networkingv1.NetworkPolicy { - ports := egressPorts(instance.Spec.Releases) + ports, registry := egressPorts(instance.Spec.Releases) egressPolicyPorts := make([]networkingv1.NetworkPolicyPort, len(ports)) for i, p := range ports { egressPolicyPorts[i] = networkingv1.NetworkPolicyPort{ @@ -402,7 +402,6 @@ func (k *kubeResources) newNetworkPolicy(instance *cv1.UpdateService) *networki } } - registry := strings.SplitN(instance.Spec.Releases, "/", 2)[0] registryEgress := networkingv1.NetworkPolicyEgressRule{ Ports: egressPolicyPorts, }🤖 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 `@controllers/new.go` around lines 395 - 405, The code duplicates extraction of the registry from instance.Spec.Releases in newNetworkPolicy and inside egressPorts; refactor to extract registry once and pass it where needed (or have egressPorts return it). Specifically, modify calls so newNetworkPolicy computes registry := strings.SplitN(instance.Spec.Releases, "/", 2)[0] once and then pass that registry into egressPorts (or change egressPorts to return (ports, registry)), updating the function signatures and call sites accordingly (egressPorts, newNetworkPolicy) to remove the duplicate extraction.controllers/testdata/resources/zz_fixture_Test_newKubeResources_network_policy.yaml (1)
3-6: 💤 Low valueDescription mentions "proxy ports" but only registry port 443 is shown.
The annotation describes egress to "the registry and proxy ports", but the egress rules only show port 443. If this fixture represents a scenario with no proxy configured, or a cluster-internal registry (where proxy ports are excluded per the PR design), the description could be misleading.
Consider clarifying the description to match the actual egress rules present in this test fixture.
🤖 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 `@controllers/testdata/resources/zz_fixture_Test_newKubeResources_network_policy.yaml` around lines 3 - 6, Update the kubernetes.io/description annotation so it accurately matches the egress rules in this NetworkPolicy: either remove mention of "proxy ports" and state that egress is limited to the registry (port 443) only, or explicitly list the proxy ports in the egress spec to match the current description; locate the annotation line with key kubernetes.io/description in the NetworkPolicy manifest and edit the text accordingly so description and actual egress ports are consistent.
🤖 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 `@controllers/new.go`:
- Around line 367-393: The code treats malformed hosts like "foo.svc" as
cluster-internal; fix this by making proxy-port skipping depend on successful
namespace extraction: change isClusterInternal to call
namespaceFromInternalHost(host) and return true only when that call yields a
non-empty namespace; keep namespaceFromInternalHost's current strict parsing
(service.namespace.svc(.cluster.local) -> namespace or "" for malformed) so
malformed hosts are not treated as internal. Also add validation on the
spec.releases field (the Releases/Spec type) to enforce the expected hostname
format/pattern (add a kubebuilder validation pattern) so malformed internal
hostnames are rejected before reconciliation.
---
Nitpick comments:
In `@controllers/new.go`:
- Around line 395-405: The code duplicates extraction of the registry from
instance.Spec.Releases in newNetworkPolicy and inside egressPorts; refactor to
extract registry once and pass it where needed (or have egressPorts return it).
Specifically, modify calls so newNetworkPolicy computes registry :=
strings.SplitN(instance.Spec.Releases, "/", 2)[0] once and then pass that
registry into egressPorts (or change egressPorts to return (ports, registry)),
updating the function signatures and call sites accordingly (egressPorts,
newNetworkPolicy) to remove the duplicate extraction.
In
`@controllers/testdata/resources/zz_fixture_Test_newKubeResources_network_policy.yaml`:
- Around line 3-6: Update the kubernetes.io/description annotation so it
accurately matches the egress rules in this NetworkPolicy: either remove mention
of "proxy ports" and state that egress is limited to the registry (port 443)
only, or explicitly list the proxy ports in the egress spec to match the current
description; locate the annotation line with key kubernetes.io/description in
the NetworkPolicy manifest and edit the text accordingly so description and
actual egress ports are consistent.
🪄 Autofix (Beta)
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c923eaf8-e876-407e-8cf3-58203bdd4bf5
📒 Files selected for processing (4)
controllers/new.gocontrollers/new_test.gocontrollers/testdata/resources/zz_fixture_Test_newKubeResources_network_policy.yamlcontrollers/updateservice_controller_test.go
nader-ziada
force-pushed
the
restrict-osus-egress-namespace
branch
3 times, most recently
from
62075c3 to
1806ef7
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
nader-ziada
force-pushed
the
restrict-osus-egress-namespace
branch
2 times, most recently
from
0fe565d to
5b7d0c6
Compare
|
/retest |
|
@hongkailiu ready for review |
|
/retest |
|
@nader-ziada: 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. |
openshift-ci
Bot
added
the
needs-rebase
…tries When spec.releases points to a cluster-internal registry (.svc), the NetworkPolicy now restricts egress to only that registry's namespace using a namespaceSelector, blocking all cluster-external traffic. Signed-off-by: Nader Ziada <nziada@redhat.com>
nader-ziada
force-pushed
the
restrict-osus-egress-namespace
branch
from
5b7d0c6 to
711054d
Compare
openshift-ci
Bot
removed
the
needs-rebase
2 participants
When spec.releases points to a cluster-internal registry (.svc), the NetworkPolicy now restricts egress to only that registry's namespace using a namespaceSelector, blocking all cluster-external traffic.
blocked by #264 being merged firstSummary by CodeRabbit
Improvements
Tests