[WIP]: Feature Implementation of Egress proxy network policy.#144
Open
siddhibhor-56 wants to merge 7 commits intoopenshift:mainfrom
Open
[WIP]: Feature Implementation of Egress proxy network policy.#144siddhibhor-56 wants to merge 7 commits intoopenshift:mainfrom
siddhibhor-56 wants to merge 7 commits intoopenshift:mainfrom
Conversation
…for EP-1998 Introduce NetworkPolicyManagementMode type with Managed and Unmanaged values, and add NetworkPolicyAllowProxyEgressAll field to ProxyConfig. When set to Managed (default), the operator automatically creates and maintains the eso-sys-proxy-egress-core NetworkPolicy allowing ESO pods to reach the proxy server whenever an effective proxy is configured via getProxyConfiguration(). When set to Unmanaged, administrators manage proxy egress NetworkPolicies themselves. The field is defined at spec.appConfig.proxy.networkPolicyAllowProxyEgressAll on ExternalSecretsConfig. CRD YAMLs for both ExternalSecretsConfig and ExternalSecretsManager are updated. Six integration test cases cover the valid values, default injection, invalid value rejection, and mutability. Ref: openshift/enhancements#1998
Add createOrApplyProxyEgressNetworkPolicy and supporting functions to manage the eso-sys-proxy-egress-core NetworkPolicy. The policy is created when spec.appConfig.proxy.networkPolicyAllowProxyEgressAll is Managed (default) and an effective proxy is configured; deleted when either condition is unmet. Also introduces: - eso-user- prefix for user-defined NetworkPolicy K8s names so they are unambiguously operator-owned and prunable - cleanupMigratedNetworkPolicies to prune stale NPs on every reconcile and remove legacy unprefixed NPs on the first post-upgrade reconcile - Constants proxyEgressNetworkPolicyName, migrationCompleteAnnotation, userNetworkPolicyPrefix Tests added for shouldManageProxyEgress, getProxyPort, createOrApplyProxyEgressNetworkPolicy, cleanupMigratedNetworkPolicies, and updated existing tests for the eso-user- prefix change. Ref: openshift/enhancements#1998 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dition Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a new 'Proxy Egress NetworkPolicy' context to the E2E suite covering EP #1998 controller behavior: - No proxy configured → eso-sys-proxy-egress-core absent - Proxy + Managed → eso-sys-proxy-egress-core created with correct port - Managed → Unmanaged transition → NP deleted - User-defined NP names get eso-user- prefix in K8s object name - Migration complete annotation set after first reconcile Ref: openshift/enhancements#1998 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new NetworkPolicyManagementMode type and a ChangesNetwork Policy Management System
Sequence DiagramsequenceDiagram
participant Controller as Controller
participant ESC as ExternalSecretsConfig
participant K8s as KubernetesAPI
participant Parser as ProxyParser
Controller->>ESC: Read proxy config & management mode
ESC-->>Controller: Return config & mode
alt Proxy configured AND Managed
Controller->>Parser: Parse proxy URL(s) (https preferred)
Parser-->>Controller: Return proxy port
Controller->>K8s: Create/Update `eso-sys-proxy-egress-core` with egress rule for port
K8s-->>Controller: Confirm created/updated
else Proxy missing OR Unmanaged
Controller->>K8s: Delete `eso-sys-proxy-egress-core` if exists
K8s-->>Controller: Confirm deleted/absent
end
Controller->>K8s: Reconcile user NetworkPolicies with `eso-user-` prefix
K8s-->>Controller: User policies reconciled
alt First reconcile (no migration annotation)
Controller->>K8s: List & delete legacy unprefixed policies
K8s-->>Controller: Legacy policies removed
Controller->>ESC: Patch migration-complete annotation
ESC-->>Controller: Annotation persisted
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented 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
Comment on lines
+78
to
+80
| // userNetworkPolicyPrefix is prepended to user-defined NetworkPolicy names from spec.networkPolicies[]. | ||
| // The user writes name: allow-external-secrets-egress; the K8s object is eso-user-allow-external-secrets-egress. | ||
| userNetworkPolicyPrefix = "eso-user-" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import re, pathlib
prefix = "eso-user-"
crd_path = "config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml"
text = pathlib.Path(crd_path).read_text()
m = re.search(r'networkPolicies:\n(?:.*\n)*?\s+name:\n(?:.*\n)*?\s+maxLength:\s*(\d+)', text)
crd_max = int(m.group(1)) if m else None
print("CRD maxLength for user policy name:", crd_max)
print("Prefix length:", len(prefix))
print("Max safe user name length with prefix:", 253 - len(prefix))
if crd_max is not None and crd_max > 253 - len(prefix):
print("MISMATCH: prefixed name can exceed Kubernetes metadata.name limit (253).")
PYRepository: openshift/external-secrets-operator
Length of output: 252
🏁 Script executed:
# 1. Find where userNetworkPolicyPrefix is used
rg "userNetworkPolicyPrefix" -A 5 -B 2Repository: openshift/external-secrets-operator
Length of output: 7102
🏁 Script executed:
# 2. Search for NetworkPolicy creation/name generation logic
rg -i "networkpolicy" --type go -A 3 -B 1 | head -100Repository: openshift/external-secrets-operator
Length of output: 11502
🏁 Script executed:
# 3. Check CRD validation for networkPolicies more thoroughly
cat config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml | grep -A 30 "networkPolicies:" | head -50Repository: openshift/external-secrets-operator
Length of output: 2202
🏁 Script executed:
# 4. Look for any existing name validation or truncation logic
rg "maxUserNameLen|truncat|255|253" --type goRepository: openshift/external-secrets-operator
Length of output: 50392
🏁 Script executed:
# Get the buildNetworkPolicyFromConfig implementation
sed -n '/func (r \*Reconciler) buildNetworkPolicyFromConfig/,/^func /p' pkg/controller/external_secrets/networkpolicy.go | head -80Repository: openshift/external-secrets-operator
Length of output: 1196
🏁 Script executed:
# Check if there's any name length validation in the codebase
rg "len.*Name|Name.*len|maxLength|validate.*name" pkg/controller/external_secrets --type go -A 2 -B 2Repository: openshift/external-secrets-operator
Length of output: 1356
🏁 Script executed:
# Get the full networkPolicies CRD definition to see if there's validation we missed
cat config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml | grep -A 80 "networkPolicies:" | grep -A 60 "name:"Repository: openshift/external-secrets-operator
Length of output: 61
🏁 Script executed:
# Search for the name field in networkPolicies section of the CRD
grep -A 150 "networkPolicies:" config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml | grep -A 30 "name:"Repository: openshift/external-secrets-operator
Length of output: 61
🏁 Script executed:
# Check if there's any existing validation or constraints on the combined name
rg "userNetworkPolicyPrefix.*Name|Name.*userNetworkPolicyPrefix" pkg/controller/external_secrets --type go -B 5 -A 5Repository: openshift/external-secrets-operator
Length of output: 5934
🏁 Script executed:
# Look for any validation functions or checks that might constrain names
rg "validation|Validate|validate" pkg/controller/external_secrets/networkpolicy.go --type goRepository: openshift/external-secrets-operator
Length of output: 61
🏁 Script executed:
# Read the CRD file directly and find the networkPolicies section
cat config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml | head -500 | tail -100Repository: openshift/external-secrets-operator
Length of output: 7322
🏁 Script executed:
# Try to find the exact maxLength constraint in the CRD
rg "maxLength" config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml -B 3 -A 1Repository: openshift/external-secrets-operator
Length of output: 3513
🏁 Script executed:
# Check the operator API types to see the NetworkPolicy struct definition
find . -name "*.go" -path "*/api/*" -o -name "types.go" | xargs rg "type NetworkPolicy struct" -A 15 2>/dev/null | head -50Repository: openshift/external-secrets-operator
Length of output: 4758
Prefixing user NetworkPolicy names exceeds Kubernetes name limits.
The CRD allows user-provided policy names up to 253 characters (api/v1alpha1/external_secrets_config_types.go), but buildNetworkPolicyFromConfig directly concatenates the 9-character prefix without validation. This creates names up to 262 characters, exceeding Kubernetes' DNS-1123 subdomain limit of 253 characters, causing NetworkPolicy creation to fail at reconcile time.
Enforce a maximum user name length of 244 characters (253 - 9 for prefix) and align CRD validation accordingly. Alternatively, apply deterministic truncation or hashing before creating the NetworkPolicy object.
🤖 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/constants.go` around lines 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.
…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>
… response The networkPolicyAllowProxyEgressAll invalid-value test triggers two simultaneous validation errors (enum rejection + CEL null check), causing the API server to wrap them in brackets: `is invalid: [err1, err2]`. The previous expectedError included the `is invalid: ` prefix which no longer matches the bracketed form `is invalid: [`. Drop the object-level prefix and match only the field-level error substring, which is present in both single and multi-error responses. Ref: openshift/enhancements#1998 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ale and proxy egress NPs The EP-1998 controller implementation calls r.Delete on NetworkPolicies in two places: cleanupMigratedNetworkPolicies (pruning stale entries) and createOrApplyProxyEgressNetworkPolicy (removing the policy when proxy is not configured). Without delete permission, these calls fail with 403, causing reconcile to abort before reaching createOrApplyDeployments. This blocked the env var test from seeing LOG_LEVEL applied to the deployment. Ref: openshift/enhancements#1998 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@siddhibhor-56: 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. |
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
1 participant
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
Tests
Chores