API Implementation of Egress proxy network policy.#144
Open
siddhibhor-56 wants to merge 1 commit into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 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 |
|
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:
WalkthroughThis PR adds controlled NetworkPolicy provisioning for proxy egress traffic in External Secrets Operator. It introduces a new ChangesProxy Egress NetworkPolicy Provisioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/e2e_test.go (1)
869-872: 💤 Low valueConsider using
Consistentlyto verify the unprefixed policy never exists.The synchronous
Getat line 870 might pass spuriously if checked at the exact moment before the controller has fully reconciled. UsingConsistentlywould provide stronger guarantees that the unprefixed policy is never created.♻️ Suggested improvement
- By(fmt.Sprintf("Verifying unprefixed name '%s' does not exist as a separate object", userPolicyName)) - _, err = clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, userPolicyName, metav1.GetOptions{}) - Expect(err).To(HaveOccurred(), "unprefixed '%s' should not exist as a K8s object", userPolicyName) + By(fmt.Sprintf("Verifying unprefixed name '%s' does not exist as a separate object", userPolicyName)) + Consistently(func(g Gomega) { + _, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, userPolicyName, metav1.GetOptions{}) + g.Expect(err).To(HaveOccurred(), "unprefixed '%s' should not exist as a K8s object", userPolicyName) + }, 10*time.Second, 2*time.Second).Should(Succeed())🤖 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 `@test/e2e/e2e_test.go` around lines 869 - 872, Replace the one-off synchronous Get call that checks for the absence of the unprefixed policy with a Gomega Consistently assertion so the test verifies the object never appears over time; specifically, wrap the call to clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, userPolicyName, metav1.GetOptions{}) (and its returned error) in a Consistently block (using an appropriate timeout and poll interval) and assert that the error always occurs (Expect(err).To(HaveOccurred())), referencing userPolicyName, operandNamespace, ctx and the Get call to locate the code to change.pkg/controller/external_secrets/networkpolicy.go (1)
248-253: 💤 Low valueConsider handling NotFound error gracefully during deletion.
If the NetworkPolicy is deleted by another actor between the
Existscheck and theDeletecall, this could return an error. While the reconciliation will likely succeed on retry, ignoringNotFounderrors during deletion is a common resilience pattern.♻️ Suggested improvement
if exists { r.log.V(1).Info("Proxy egress policy no longer needed, deleting", "name", npName) if err := r.Delete(r.ctx, fetched); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } return common.FromClientError(err, "failed to delete proxy egress network policy %s", npName) } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s deleted", npName) }🤖 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 `@pkg/controller/external_secrets/networkpolicy.go` around lines 248 - 253, The delete path for the proxy egress NetworkPolicy should treat a NotFound error as non-fatal: after calling r.Delete(fetched) in the block that logs "Proxy egress policy no longer needed, deleting" (use symbols r.Delete, fetched, npName, r.log, r.eventRecorder), check the returned error with apierrors.IsNotFound(err) (import "k8s.io/apimachinery/pkg/api/errors") and ignore/log it (e.g., r.log.V(1).Info("NetworkPolicy already deleted", "name", npName)) instead of returning; only wrap-and-return other errors via common.FromClientError so transient races where another actor already deleted the NetworkPolicy do not cause a reconcile failure.
🤖 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
`@api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml`:
- Around line 1729-1814: The test cases named like "Should accept
networkPolicyAllowProxyEgressAll set to Managed/Unmanaged", "Should default
networkPolicyAllowProxyEgressAll to Managed when proxy is set without the
field", "Should fail with invalid value for networkPolicyAllowProxyEgressAll",
and "Should accept proxy config with all fields including
networkPolicyAllowProxyEgressAll" are currently placed under onUpdate without an
updated manifest; move these create-style cases into onCreate (or add an
explicit updated manifest per case) so they exercise create/default behavior
rather than update behavior—locate the entries with resourceName: cluster and
kind: ExternalSecretsConfig in the test suite and either change their parent
block to onCreate or provide an updated: | YAML payload for each to represent
the post-update state.
In `@pkg/controller/external_secrets/constants.go`:
- Around line 78-80: The code concatenates userNetworkPolicyPrefix ("eso-user-")
to user-provided names in buildNetworkPolicyFromConfig which can exceed
Kubernetes' 253-char DNS-1123 subdomain limit; enforce a max user name length of
244 characters (253 - len("eso-user-")) or apply deterministic
truncation/hashing before concatenation. Update buildNetworkPolicyFromConfig to
validate the incoming name length (or truncate/hash it) and produce a final name
<=253 chars, and align the CRD validation (external_secrets_config_types.go) to
cap the user-provided name to 244 characters so reconcilers never try to create
too-long NetworkPolicy names.
---
Nitpick comments:
In `@pkg/controller/external_secrets/networkpolicy.go`:
- Around line 248-253: The delete path for the proxy egress NetworkPolicy should
treat a NotFound error as non-fatal: after calling r.Delete(fetched) in the
block that logs "Proxy egress policy no longer needed, deleting" (use symbols
r.Delete, fetched, npName, r.log, r.eventRecorder), check the returned error
with apierrors.IsNotFound(err) (import "k8s.io/apimachinery/pkg/api/errors") and
ignore/log it (e.g., r.log.V(1).Info("NetworkPolicy already deleted", "name",
npName)) instead of returning; only wrap-and-return other errors via
common.FromClientError so transient races where another actor already deleted
the NetworkPolicy do not cause a reconcile failure.
In `@test/e2e/e2e_test.go`:
- Around line 869-872: Replace the one-off synchronous Get call that checks for
the absence of the unprefixed policy with a Gomega Consistently assertion so the
test verifies the object never appears over time; specifically, wrap the call to
clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx,
userPolicyName, metav1.GetOptions{}) (and its returned error) in a Consistently
block (using an appropriate timeout and poll interval) and assert that the error
always occurs (Expect(err).To(HaveOccurred())), referencing userPolicyName,
operandNamespace, ctx and the Get call to locate the code to change.
🪄 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: 453d2949-13e2-4cd7-88cb-44c93795a333
📒 Files selected for processing (8)
api/v1alpha1/meta.goapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsmanagers.yamlpkg/controller/external_secrets/constants.gopkg/controller/external_secrets/networkpolicy.gopkg/controller/external_secrets/networkpolicy_test.gotest/e2e/e2e_test.go
ambient-code Bot
pushed a commit
to siddhibhor-56/external-secrets-operator
that referenced
this pull request
May 8, 2026
…etworkPolicyAllowProxyEgressAll - Move 5 ESC proxy egress tests from onUpdate to onCreate (they lacked required 'updated' field, causing 'Object Kind is missing' errors) - Add logLevel: 1 to expected in ESC onUpdate proxy egress test (API server defaults logLevel when appConfig is present) - Add networkPolicyAllowProxyEgressAll: Managed to 5 ESM test expected blocks where proxy is configured (field defaulted by kubebuilder marker) Fixes 11 CI unit test failures in PR openshift#144. Ref: openshift/enhancements#1998 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ambient-code
Bot
force-pushed
the
feature/ep-1998-e2e-proxy-egress-np
branch
from
720831b to
bd96e90
Compare
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 `@test/e2e/e2e_test.go`:
- Around line 725-738: The helper restoreESCProxyConfig currently force-clears
proxy and NetworkPolicies by setting updatedCR.Spec.ApplicationConfig.Proxy =
nil and updatedCR.Spec.ControllerConfig.NetworkPolicies = nil; change it to
capture the original values before modifying the CR and restore those captured
values in the retry.Update loop (use variables like originalProxy and
originalNetworkPolicies captured from existingCR) and invoke this restore via
defer so the test restores prior state instead of erasing it; update references
in the retry.RetryOnConflict closure (existingCR, updatedCR,
runtimeClient.Update) to assign the captured originals back to
updatedCR.Spec.ApplicationConfig.Proxy and
updatedCR.Spec.ControllerConfig.NetworkPolicies rather than nil.
- Around line 742-745: The test currently treats any error from
clientset.NetworkingV1().NetworkPolicies(...).Get as success; change the
Consistently check (the block referencing ctx, operandNamespace,
proxyEgressNPName) to assert the error is a NotFound error explicitly using
k8serrors.IsNotFound(err) instead of HaveOccurred; import k8serrors
(k8s.io/apimachinery/pkg/api/errors) if missing and replace
g.Expect(err).To(HaveOccurred(), ...) with an assertion like
g.Expect(k8serrors.IsNotFound(err)).To(BeTrue(), "%s should not exist when no
proxy is configured", proxyEgressNPName) and apply the same pattern to the other
occurrences mentioned.
🪄 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: 6163de1d-9774-4ebf-b283-9716a4cc705f
📒 Files selected for processing (10)
api/v1alpha1/meta.goapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsmanagers.yamlpkg/controller/client/client.gopkg/controller/client/fakes/fake_ctrl_client.gopkg/controller/external_secrets/constants.gopkg/controller/external_secrets/networkpolicy.gopkg/controller/external_secrets/networkpolicy_test.gotest/e2e/e2e_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/client/fakes/fake_ctrl_client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/external_secrets/networkpolicy_test.go
- pkg/controller/external_secrets/networkpolicy.go
ambient-code
Bot
force-pushed
the
feature/ep-1998-e2e-proxy-egress-np
branch
from
bd96e90 to
5cea3eb
Compare
siddhibhor-56
changed the title
Contributor
Author
|
/unhold |
siddhibhor-56
changed the title
| // +optional | ||
| NoProxy string `json:"noProxy,omitempty"` | ||
|
|
||
| // networkPolicyProvisioning controls whether the operator automatically creates a NetworkPolicy |
Contributor
There was a problem hiding this comment.
Please keep API changes aligned with what's present in EP.
Contributor
Author
There was a problem hiding this comment.
Updated according to the EP
Contributor
Author
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
siddhibhor-56
force-pushed
the
feature/ep-1998-e2e-proxy-egress-np
branch
5 times, most recently
from
6b314d6 to
5c213ad
Compare
…1998 Introduce ManagementState type with Managed and Unmanaged values, and add NetworkPolicyProvisioning field to ProxyConfig. When set to Managed (default), the operator will automatically create and maintain a NetworkPolicy allowing ESO pods to reach the configured proxy server. When set to Unmanaged, administrators manage proxy egress NetworkPolicies themselves. The field is defined at spec.appConfig.proxy.networkPolicyProvisioning on ExternalSecretsConfig. CRD YAMLs for both ExternalSecretsConfig and ExternalSecretsManager are updated. Six API test cases cover valid values, default injection, invalid value rejection, and mutability. Ref: openshift/enhancements#1998 Co-authored-by: Cursor <cursoragent@cursor.com>
siddhibhor-56
force-pushed
the
feature/ep-1998-e2e-proxy-egress-np
branch
from
5c213ad to
bd8d79a
Compare
|
@siddhibhor-56: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ep Link
Tested using custom workflow workflow link on ambient code platform.
Summary by CodeRabbit
New Features
networkPolicyProvisioningconfiguration field to proxy settings in both ExternalSecretsConfig and ExternalSecretsManager, allowing users to control automatic operator management of proxy egress NetworkPolicies (defaults toManaged;Unmanagedoption available).Tests