ESO-222: Implementation of Network Policy for external secrets Operator #67
Conversation
WalkthroughAdds two Kubernetes NetworkPolicy manifests under config/network-policy and updates kustomization.yaml to include them alongside the existing allow-metrics-traffic.yaml. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/external_secrets/utils.go (1)
48-57: Bug: prependErr is dropped when updateStatus fails.When updateStatus errors and prependErr is non-nil, the aggregate excludes prependErr, losing the original error context.
errUpdate := fmt.Errorf("failed to update %s/%s status: %w", externalsecrets.GetNamespace(), externalsecrets.GetName(), err) if prependErr != nil { - return utilerrors.NewAggregate([]error{err, errUpdate}) + return utilerrors.NewAggregate([]error{prependErr, errUpdate}) }
🧹 Nitpick comments (3)
config/network-policy/deny-all.yaml (1)
5-5: Avoid hardcoding namespace for portabilityLet the overlay set the namespace via kustomize. Remove metadata.namespace to keep the resource reusable across overlays.
Apply this diff:
- namespace: external-secrets-operatorconfig/network-policy/allow-network-traffic.yaml (1)
5-5: Avoid hardcoding namespace for portabilityLet kustomize set the namespace; drop metadata.namespace.
Apply this diff:
- namespace: external-secrets-operatorpkg/controller/external_secrets/utils.go (1)
94-106: Remove commented-out dead code.This legacy block is now superseded and adds noise. It’s version-controlled; safe to delete.
-//// isCertManagerConfigEnabled returns whether CertManagerConfig is enabled in ExternalSecrets CR Spec. -//func isCertManagerConfigEnabled(es *operatorv1alpha1.ExternalSecrets) bool { -// return es.Spec != (operatorv1alpha1.ExternalSecretsSpec{}) && es.Spec.ExternalSecretsConfig != nil && -// es.Spec.ExternalSecretsConfig.CertManagerConfig != nil && -// common.ParseBool(es.Spec.ExternalSecretsConfig.CertManagerConfig.Enabled) -//} -// -//// isBitwardenConfigEnabled returns whether CertManagerConfig is enabled in ExternalSecrets CR Spec. -//func isBitwardenConfigEnabled(es *operatorv1alpha1.ExternalSecrets) bool { -// return es.Spec != (operatorv1alpha1.ExternalSecretsSpec{}) && es.Spec.ExternalSecretsConfig != nil && es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider != nil && -// common.ParseBool(es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider.Enabled) -//}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
config/crd/bases/operator.openshift.io_externalsecrets.yaml(0 hunks)config/default/kustomization.yaml(1 hunks)config/network-policy/allow-network-traffic.yaml(1 hunks)config/network-policy/deny-all.yaml(1 hunks)config/network-policy/kustomization.yaml(1 hunks)pkg/controller/external_secrets/utils.go(1 hunks)
💤 Files with no reviewable changes (1)
- config/crd/bases/operator.openshift.io_externalsecrets.yaml
🔇 Additional comments (4)
config/network-policy/kustomization.yaml (1)
3-4: Kustomization entries look goodThe new resources are correctly referenced; order doesn’t affect policy behavior.
config/network-policy/allow-network-traffic.yaml (1)
7-9: Verify pod label selector matches the operator podsUsing app: external-secrets-operator may not match the Deployment’s pod template labels (often control-plane: controller-manager in Kubebuilder scaffolding). Mismatch means policies won’t apply.
Use the script shared on config/default/kustomization.yaml to confirm and adjust matchLabels accordingly.
pkg/controller/external_secrets/utils.go (2)
109-113: Nil-safe, clearer implementation — confirmed: omitted/empty Enabled evaluates to false.ParseBool returns true only for "true" (pkg/controller/common/utils.go:380); CertManagerConfig.Enabled is a string (api/v1alpha1/external_secrets_types.go:179), so empty/omitted -> false.
115-121: Confirm Bitwarden Enabled defaults to disabledCRD has +kubebuilder:default:="false" for Bitwarden.Enabled and common.ParseBool(val) returns true only for "true" — unspecified or default "false" evaluates to false.
config/default/kustomization.yaml
Outdated
| # Only CR(s) which requires webhooks and are applied on namespaces labeled with 'webhooks: enabled' will | ||
| # be able to communicate with the Webhook Server. | ||
| #- ../network-policy | ||
| - ../network-policy |
There was a problem hiding this comment.
🧩 Analysis chain
Enabling network policies: verify selector alignment and traffic impact
Including ../network-policy will enforce the new deny/allow rules. Ensure the podSelector labels in those policies match the operator Deployment labels; otherwise, policies won’t apply or, worse, could over/under-block traffic.
Run this to inspect Deployment labels and reconcile with the NetworkPolicy podSelectors:
🏁 Script executed:
#!/bin/bash
# Show deployments and pod template labels under config/
rg -nC5 -g '!**/vendor/**' -P '^\s*kind:\s*Deployment\b' config | sed -n '1,200p'
echo '---'
# List any places labeling pods with "app:" or "control-plane:" for the operator
rg -nC3 -P 'labels:\s*$|^\s*app:\s*external-secrets-operator\b|^\s*control-plane:\s*controller-manager\b' configLength of output: 46592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== config/default/kustomization.yaml ==="
sed -n '1,200p' config/default/kustomization.yaml || true
echo
echo "=== config/network-policy/deny-all.yaml ==="
sed -n '1,200p' config/network-policy/deny-all.yaml || true
echo
echo "=== config/network-policy/allow-network-traffic.yaml ==="
sed -n '1,200p' config/network-policy/allow-network-traffic.yaml || true
echo
echo "=== config/network-policy/allow-metrics-traffic.yaml ==="
sed -n '1,200p' config/network-policy/allow-metrics-traffic.yaml || true
echo
echo "=== config/manager/manager.yaml ==="
sed -n '1,240p' config/manager/manager.yaml || true
echo
echo "=== config/default/metrics_service.yaml ==="
sed -n '1,200p' config/default/metrics_service.yaml || trueLength of output: 11582
NetworkPolicy namespace mismatch — policies won't apply to the Deployment
Deployment runs in namespace "system" with label app: external-secrets-operator (config/manager/manager.yaml). deny-all.yaml and allow-network-traffic.yaml are scoped to namespace "external-secrets-operator" (config/network-policy/*) so they will NOT affect the Deployment; allow-metrics-traffic.yaml is in "system" and will apply.
- Action: align namespaces so policies target the same namespace as the Deployment — either change config/network-policy/deny-all.yaml and config/network-policy/allow-network-traffic.yaml to namespace: system, or move the Deployment to external-secrets-operator.
- Action: re-evaluate deny-all and allow-network-traffic semantics to avoid inadvertently blocking API/metrics/webhook traffic (files: config/network-policy/deny-all.yaml, config/network-policy/allow-network-traffic.yaml, config/network-policy/allow-metrics-traffic.yaml, config/manager/manager.yaml; config/default/kustomization.yaml includes ../network-policy).
🤖 Prompt for AI Agents
In config/default/kustomization.yaml around line 34, the included network-policy
manifests target namespace "external-secrets-operator" while the Deployment in
config/manager/manager.yaml runs in namespace "system", so those NetworkPolicies
won't apply; update the NetworkPolicy YAMLs (config/network-policy/deny-all.yaml
and config/network-policy/allow-network-traffic.yaml) to namespace: system so
they target the Deployment's namespace (or alternatively change the Deployment
namespace to external-secrets-operator if that's desired), and then re-evaluate
and adjust the deny-all and allow-network-traffic rules to ensure they don't
block necessary API/metrics/webhook traffic (compare with
allow-metrics-traffic.yaml and modify podSelector/ingress/egress rules and
namespaceSelector as needed).
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| egress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 6443 # Required: Kubernetes API server | ||
| ingress: |
There was a problem hiding this comment.
Egress rule is too restrictive; will break DNS and external provider access
Allowing only TCP 6443 egress blocks DNS (53) and external APIs (443), which external-secrets-operator typically needs to fetch secrets. If you must keep egress policy, add DNS and 443 (and any others you require). Otherwise, remove egress from policyTypes and drop the egress block.
Two options:
- Preferred (ingress-only policy):
policyTypes:
- - Ingress
- - Egress
+ - Ingress
- egress:
- - ports:
- - protocol: TCP
- port: 6443 # Required: Kubernetes API server- If keeping egress, minimally allow DNS and HTTPS:
egress:
- - ports:
- - protocol: TCP
- port: 6443 # Required: Kubernetes API server
+ - to:
+ - namespaceSelector:
+ matchLabels:
+ kubernetes.io/metadata.name: kube-system
+ podSelector:
+ matchLabels:
+ k8s-app: kube-dns
+ ports:
+ - protocol: UDP
+ port: 53
+ - protocol: TCP
+ port: 53
+ - ports:
+ - protocol: TCP
+ port: 443📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| policyTypes: | |
| - Ingress | |
| - Egress | |
| egress: | |
| - ports: | |
| - protocol: TCP | |
| port: 6443 # Required: Kubernetes API server | |
| ingress: | |
| policyTypes: | |
| - Ingress | |
| ingress: |
| policyTypes: | |
| - Ingress | |
| - Egress | |
| egress: | |
| - ports: | |
| - protocol: TCP | |
| port: 6443 # Required: Kubernetes API server | |
| ingress: | |
| policyTypes: | |
| - Ingress | |
| - Egress | |
| egress: | |
| - to: | |
| - namespaceSelector: | |
| matchLabels: | |
| kubernetes.io/metadata.name: kube-system | |
| podSelector: | |
| matchLabels: | |
| k8s-app: kube-dns | |
| ports: | |
| - protocol: UDP | |
| port: 53 | |
| - protocol: TCP | |
| port: 53 | |
| - ports: | |
| - protocol: TCP | |
| port: 443 | |
| ingress: |
🤖 Prompt for AI Agents
In config/network-policy/allow-network-traffic.yaml around lines 10 to 17, the
egress block only allows TCP 6443 which blocks DNS and external HTTPS calls;
either remove "Egress" from policyTypes and delete the entire egress section to
make this an ingress-only policy, or expand the egress rules to minimally allow
DNS and HTTPS by adding UDP 53 and TCP 53 for DNS plus TCP 443 for external APIs
(and any other provider ports you require); implement one of these two changes
so DNS resolution and external-secrets/operator HTTPS calls are not blocked.
| # Optional: expose metrics (8443 and 8080 based on user configuration) | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8080 No newline at end of file |
There was a problem hiding this comment.
Ingress is overly permissive; it allows any source to metrics ports
Without a from selector, any pod can scrape 8443/8080. Either:
- Remove these rules and rely on the existing allow-metrics-traffic.yaml, or
- Restrict sources to namespaces labeled metrics: "enabled".
Also, 8080 may be unused if HTTPS metrics are enabled.
Preferred fix (de-duplicate and restrict):
ingress:
- # Optional: expose metrics (8443 and 8080 based on user configuration)
- - ports:
- - protocol: TCP
- port: 8443
- - ports:
- - protocol: TCP
- port: 8080
+ - from:
+ - namespaceSelector:
+ matchLabels:
+ metrics: "enabled"
+ ports:
+ - protocol: TCP
+ port: 8443If you keep 8080, mirror the same from selector.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Optional: expose metrics (8443 and 8080 based on user configuration) | |
| - ports: | |
| - protocol: TCP | |
| port: 8443 | |
| - ports: | |
| - protocol: TCP | |
| port: 8080 | |
| - from: | |
| - namespaceSelector: | |
| matchLabels: | |
| metrics: "enabled" | |
| ports: | |
| - protocol: TCP | |
| port: 8443 |
🤖 Prompt for AI Agents
config/network-policy/allow-network-traffic.yaml around lines 18-24: the current
ingress entries expose metrics ports (8443 and 8080) to all sources because they
lack a from selector; either remove these port rules and rely on
allow-metrics-traffic.yaml to avoid duplication, or restrict each ingress to
only allow pods/namespaces labeled metrics: "enabled" by adding an appropriate
from selector; if HTTPS metrics (8443) is used and 8080 is unused, remove 8080,
otherwise ensure 8080 has the same from selector as 8443 so both ports are
consistently restricted.
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app: external-secrets-operator | ||
| policyTypes: | ||
| - Ingress | ||
| - Egress No newline at end of file |
There was a problem hiding this comment.
Default deny egress will likely break the operator (DNS, cloud providers, webhooks)
Denying all egress without explicit exceptions will block:
- DNS (TCP/UDP 53 to kube-dns)
- External secret providers (e.g., AWS/GCP/Vault over 443)
- Calls to the kube-apiserver when not explicitly permitted
If the goal is to “protect metrics/webhook,” prefer ingress-only default deny. Otherwise, explicitly add egress allows (DNS + required external endpoints).
Suggested minimal fix (ingress-only deny):
spec:
podSelector:
matchLabels:
app: external-secrets-operator
policyTypes:
- Ingress
- - Egress📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec: | |
| podSelector: | |
| matchLabels: | |
| app: external-secrets-operator | |
| policyTypes: | |
| - Ingress | |
| - Egress | |
| spec: | |
| podSelector: | |
| matchLabels: | |
| app: external-secrets-operator | |
| policyTypes: | |
| - Ingress |
🤖 Prompt for AI Agents
In config/network-policy/deny-all.yaml around lines 6 to 12, the NetworkPolicy
currently denies all egress which will break the operator (DNS, cloud provider
APIs, webhooks); either change the policy to be ingress-only by removing or
replacing the policyTypes entry so only Ingress is enforced, OR keep Egress but
add explicit egress allow rules for UDP/TCP port 53 to kube-dns (cluster DNS
CIDR / kube-dns label), TCP 443 to the operator’s required external endpoints
(cloud provider metadata/API and secret backends), and TCP to the
kube-apiserver, ensuring the selectors and CIDRs match cluster addresses.
siddhibhor-56
force-pushed
the
network-policy-new
branch
from
9341c2e to
8d0e3cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/network-policy/kustomization.yaml (1)
2-4: Consider reordering for clarity.The resources are functionally correct, but listing
deny-all.yamlbefore the allow policies would better reflect the conceptual model of "deny by default, then explicitly allow." While Kubernetes applies all NetworkPolicies additively regardless of order, this arrangement improves readability.Apply this diff to reorder the resources:
resources: +- deny-all.yaml - allow-metrics-traffic.yaml - allow-network-traffic.yaml -- deny-all.yaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
config/default/kustomization.yaml(1 hunks)config/network-policy/allow-network-traffic.yaml(1 hunks)config/network-policy/deny-all.yaml(1 hunks)config/network-policy/kustomization.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/network-policy/deny-all.yaml
- config/network-policy/allow-network-traffic.yaml
- config/default/kustomization.yaml
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/external_secrets_manager/controller.go (2)
68-71: Missing RBAC for ExternalSecretsConfig.Controller reads and watches ExternalSecretsConfig but lacks RBAC for it. Add get/list/watch to avoid 403s at runtime.
Apply this diff:
// +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsmanagers,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsmanagers/status,verbs=get;update;patch // +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsmanagers/finalizers,verbs=update +// +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsconfigs,verbs=get;list;watch
124-139: Handle legacy finalizers to support upgrades.Finalizer value changed; deletions will hang if old finalizer remains. Remove legacy values during reconcile/delete.
Apply this diff:
if !esm.DeletionTimestamp.IsZero() { r.log.V(1).Info("externalsecretsmanagers.operator.openshift.io is marked for deletion", "key", key) - if err := common.RemoveFinalizer(ctx, esm, r.CtrlClient, finalizer); err != nil { + // Remove current and legacy finalizers to avoid stuck deletions after upgrades. + if err := common.RemoveFinalizer(ctx, esm, r.CtrlClient, finalizer); err != nil { return ctrl.Result{}, err } + // Best-effort removal of legacy finalizer(s). + _ = common.RemoveFinalizer(ctx, esm, r.CtrlClient, "externalsecretsmanager.openshift.operator.io/"+ControllerName) r.log.V(1).Info("removed finalizer, cleanup complete", "key", key) return ctrl.Result{}, nil } @@ - // Set finalizers on the externalsecretsmanagers.operator.openshift.io resource + // Set finalizers on the externalsecretsmanagers.operator.openshift.io resource and clean legacy values if err := common.AddFinalizer(ctx, esm, r.CtrlClient, finalizer); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update %q externalsecretsmanagers.operator.openshift.io with finalizers: %w", key, err) } + // Clean legacy finalizer if present (non-fatal). + _ = common.RemoveFinalizer(ctx, esm, r.CtrlClient, "externalsecretsmanager.openshift.operator.io/"+ControllerName)
♻️ Duplicate comments (1)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1020-1025: Namespace value validation (tracked in base CRD).operatingNamespace length bounds are set, but no DNS-1123 label pattern. I’ve proposed adding the pattern on the base CRD; please ensure this bundle stays in sync after updating the base.
🧹 Nitpick comments (24)
pkg/controller/client/client.go (1)
65-65: LGTM! Idiomatic key extraction.Using
client.ObjectKeyFromObject(obj)is the standard controller-runtime approach and eliminates manual key construction. This improves maintainability and aligns with best practices.hack/test-apis.sh (1)
18-18: Simplify HOME directory comparison.The comparison
[ $HOME == "/" ]on line 18 works correctly withset -o nounset, but could be more robust.Apply this diff for clarity:
-if [ $HOME == "/" ]; then +if [ "${HOME}" = "/" ]; thenUsing
=instead of==is more portable, and quoting${HOME}is a best practice.config/manager/manager.yaml (1)
96-97: Pin image tag and align imagePullPolicy
- Replace
controller:latestwith a specific semantic version or digest for production.- Use
imagePullPolicy: IfNotPresentwhen deploying immutable tags.go.mod (1)
18-18: Replace archived yaml-patch with a maintained alternativeThe modules
github.com/vmware-archive/yaml-patch(go.mod line 18)github.com/krishicks/yaml-patch(go.mod line 161)are archived and no longer receive updates. Migrate to one of these actively maintained libraries based on your use case:
- sigs.k8s.io/kustomize/kyaml
- k8s.io/apimachinery/pkg/util/strategicpatch
- github.com/evanphx/json-patch + sigs.k8s.io/yaml
- github.com/mikefarah/yq/v4
Choose by whether you need programmatic in-process APIs, strategic-merge vs. RFC6902 semantics, or a CLI-based workflow.
api/v1alpha1/tests/externalsecretsmanager.operator.openshift.io/externalsecretsmanager.testsuite.yaml (1)
92-99: Make ExpectedError matching less brittleSince ExpectedError is matched as a substring, consider trimming dynamic prefixes/suffixes (e.g., “: Invalid value: …”) and keep only stable parts like:
- “must be 'cluster'”
- “Too many: 21: must have at most 20 items”
This reduces flakiness across API server versions.
Also applies to: 128-138
config/rbac/role.yaml (2)
161-170: Add patch verb for externalsecretsconfigs for parity with managersControllers often use Patch or SSA. externalsecretsmanagers already includes patch; externalsecretsconfigs lacks it. Add patch for consistency/future-proofing.
resources: - externalsecretsconfigs verbs: - create + - patch - get - list - update - watch
178-184: Add patch verb to externalsecretsconfigs/status to match managers/statusManagers/status includes patch; configs/status should too to avoid forbidden errors on status patch.
resources: - externalsecretsconfigs/status verbs: - get + - patch - updatepkg/controller/external_secrets/certificate_test.go (1)
22-24: Unify webhook certificate name constant to avoid driftUse the package constant (serviceExternalSecretWebhookName) everywhere instead of redefining testValidateCertificateResourceName.
-var ( - testValidateCertificateResourceName = "external-secrets-webhook" -) +// use serviceExternalSecretWebhookName directly for consistency @@ - wantErr: fmt.Sprintf("failed to check %s/%s certificate resource already exists: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.TestClientError), + wantErr: fmt.Sprintf("failed to check %s/%s certificate resource already exists: %s", commontest.TestExternalSecretsNamespace, serviceExternalSecretWebhookName, commontest.TestClientError), @@ - wantErr: fmt.Sprintf("failed to update %s/%s certificate resource: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.TestClientError), + wantErr: fmt.Sprintf("failed to update %s/%s certificate resource: %s", commontest.TestExternalSecretsNamespace, serviceExternalSecretWebhookName, commontest.TestClientError), @@ - wantErr: fmt.Sprintf("failed to create %s/%s certificate resource: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.TestClientError), + wantErr: fmt.Sprintf("failed to create %s/%s certificate resource: %s", commontest.TestExternalSecretsNamespace, serviceExternalSecretWebhookName, commontest.TestClientError),Also applies to: 127-128, 180-181, 267-268
test/apis/vars.go (3)
68-71: Typo in comment“resourec” → “resource”.
-// ExpectedError defines the error string that should be returned when the initial resourec is invalid. +// ExpectedError defines the error string that should be returned when the initial resource is invalid.
47-49: Comment wording nit“OnUpdate defines a list of on create style tests.” → “on update style tests.”
-// OnUpdate defines a list of on create style tests. +// OnUpdate defines a list of on update style tests.
130-143: Prefer json.RawMessage or any for Patch.ValuePointer to interface{} is awkward for (un)marshalling. Consider:
- Value json.RawMessage (most flexible), or
- Value any with
omitempty.-import "k8s.io/apimachinery/pkg/runtime" +import ( + "encoding/json" + "k8s.io/apimachinery/pkg/runtime" +) -type Patch struct { +type Patch struct { // Op is the operation to be performed. Common operations include "add", "remove", "replace", "move", "copy", and "test". Op string `json:"op"` // Path is a JSON Pointer that indicates the location in the YAML document where the operation is to be performed. Path string `json:"path"` - // Value is the value to be used within the operation. This field is required for operations like "add" and "replace". - Value *interface{} `json:"value"` + // Value is the raw value for operations like "add" and "replace". + Value json.RawMessage `json:"value,omitempty"` }pkg/controller/external_secrets/secret.go (1)
49-55: Event reason consistency (optional)Consider reason “Created” for the create path to distinguish from “Reconciled” on updates.
- r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s created", secretName) + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Created", "secret resource %s created", secretName)Makefile (1)
133-135: Fix the regex used to exclude test packages in test-unit.
grep -vE 'test/[e2e|apis|utils]'uses a character class and filters the wrong paths. Use grouping:- go test $$(go list ./... | grep -vE 'test/[e2e|apis|utils]') -coverprofile cover.out + go test $$(go list ./... | grep -vE '(^|/)test/(e2e|apis|utils)(/|$$)') -coverprofile cover.outpkg/controller/external_secrets/serviceaccounts.go (1)
56-66: Reconcile updates for existing ServiceAccounts (parity with Services).Current flow creates but doesn’t update when drift is detected. Add an update path like Services to keep labels/fields in sync.
- if exist { - if externalSecretsConfigCreateRecon { - r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s serviceaccount already exists, possibly from a previous install", serviceAccountName) - } - r.log.V(4).Info("serviceaccount exists", "name", serviceAccountName) - } else { + if exist { + if externalSecretsConfigCreateRecon { + r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s serviceaccount already exists, possibly from a previous install", serviceAccountName) + } + if common.HasObjectChanged(desired, fetched) { + r.log.V(1).Info("serviceaccount modified, updating", "name", serviceAccountName) + if err := r.UpdateWithRetry(r.ctx, desired); err != nil { + return common.FromClientError(err, "failed to update serviceaccount %s", serviceAccountName) + } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Updated serviceaccount %s", serviceAccountName) + } else { + r.log.V(4).Info("serviceaccount already up-to-date", "name", serviceAccountName) + } + } else { if err := r.Create(r.ctx, desired); err != nil { return common.FromClientError(err, "failed to create serviceaccount %s", serviceAccountName) } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Created serviceaccount %s", serviceAccountName) }pkg/controller/external_secrets/certificate.go (1)
163-182: Use empty namespace when checking ClusterIssuer existence.ClusterIssuer is cluster-scoped; pass an empty namespace to avoid surprises with custom client helpers.
-func (r *Reconciler) getIssuer(issuerRef v1.ObjectReference, namespace string) (bool, error) { - namespacedName := types.NamespacedName{ - Name: issuerRef.Name, - Namespace: namespace, - } +func (r *Reconciler) getIssuer(issuerRef v1.ObjectReference, namespace string) (bool, error) { + var namespacedName types.NamespacedName + if issuerRef.Kind == clusterIssuerKind { + namespacedName = types.NamespacedName{Name: issuerRef.Name} + } else { + namespacedName = types.NamespacedName{Name: issuerRef.Name, Namespace: namespace} + }test/apis/generator.go (1)
256-259: Typo in comment: "sential" should be "sentinel".Apply this diff:
- # Use an eventually here, so that we retry until the sential correctly applies. + # Use an eventually here, so that we retry until the sentinel correctly applies.api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
95-102: Harden error assertions to reduce brittleness.The expectedError strings mirror upstream validation text (including paths like and aggregator notes). Prefer substring or regex assertions (if the harness supports it) against stable fragments, e.g., "singleton" or specific field constraints, to avoid flakes across Kubernetes version bumps.
Also applies to: 112-125, 140-151, 200-216, 233-234, 242-252, 281-297, 311-327, 341-366, 374-384, 393-408
pkg/controller/external_secrets_manager/controller.go (1)
111-121: Event on read failure may reference a non-fetched object.Recording an event on esm after Get error can produce noisy/invalid events. Consider logging only, or emit a controller-level event.
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1027-1053: Optional: strengthen proxy URL validations.If feasible, add CEL to require http(s):// prefixes and basic URL shape for httpProxy/httpsProxy and a simple CSV-host/CIDR/IP list check for noProxy.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (3)
582-591: Add patch verb for ExternalSecretsConfig RBAC.Controllers commonly use PATCH (server‑side apply/strategic merges). Recommend adding patch.
- externalsecretsconfigs verbs: - create - get - list + - patch - update - watch
600-605: Optional: allow PATCH on status.Not required, but enabling patch on status can simplify partial updates if used.
verbs: - - get - - update + - get + - patch + - update
727-727: Reconsider imagePullPolicy: Always.Always pulls can fail on disconnected/air‑gapped clusters and increase startup time. If not strictly needed, prefer IfNotPresent for GA channels.
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (2)
1020-1025: Validate operatingNamespace as a DNS‑1123 label.Add the standard namespace pattern to prevent invalid values.
operatingNamespace: description: |- operatingNamespace is for restricting the external-secrets operations to the provided namespace. When configured `ClusterSecretStore` and `ClusterExternalSecret` are implicitly disabled. maxLength: 63 minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string
1027-1053: Optional: add basic URL/csv validations to proxy fields.Consider CEL to enforce http(s):// prefixes and simple host/IP/CIDR tokenization for noProxy (comma‑separated).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (125)
go.sumis excluded by!**/*.sumvendor/github.com/ghodss/yaml/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/ghodss/yaml/.travis.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/ghodss/yaml/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/ghodss/yaml/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/ghodss/yaml/fields.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/ghodss/yaml/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/core_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/decorator_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/build/build_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/dependencies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_t_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/around_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/focus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/group.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/testingtproxy/testing_t_proxy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/junit_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/teamcity_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/around_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/semver_filter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/container.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/operation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/patch.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/pathfinder.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/vmware-archive/yaml-patch/placeholder_wrapper.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/term_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/astutil/enclosing.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/inspector.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/typeof.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/builder.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/imports/source_modindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/directories.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/index.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/modindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/symbols.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/types.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/envtest/komega/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/envtest/komega/default.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/envtest/komega/equalobject.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/envtest/komega/interfaces.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/envtest/komega/komega.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/multi.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (65)
Makefile(6 hunks)PROJECT(1 hunks)README.md(1 hunks)api/v1alpha1/conditions.go(2 hunks)api/v1alpha1/external_secrets_config_types.go(1 hunks)api/v1alpha1/external_secrets_manager_types.go(4 hunks)api/v1alpha1/external_secrets_types.go(0 hunks)api/v1alpha1/groupversion_info.go(1 hunks)api/v1alpha1/meta.go(2 hunks)api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml(1 hunks)api/v1alpha1/tests/externalsecretsmanager.operator.openshift.io/externalsecretsmanager.testsuite.yaml(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(8 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(8 hunks)bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml(8 hunks)bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml(6 hunks)config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml(8 hunks)config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml(6 hunks)config/crd/kustomization.yaml(1 hunks)config/manager/manager.yaml(1 hunks)config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml(3 hunks)config/rbac/role.yaml(2 hunks)config/samples/kustomization.yaml(1 hunks)config/samples/operator_v1alpha1_externalsecretsconfig.yaml(1 hunks)docs/api_reference.md(10 hunks)go.mod(3 hunks)hack/test-apis.sh(1 hunks)pkg/controller/client/client.go(1 hunks)pkg/controller/common/constants.go(1 hunks)pkg/controller/common/utils.go(5 hunks)pkg/controller/commontest/utils.go(4 hunks)pkg/controller/crd_annotator/controller.go(8 hunks)pkg/controller/crd_annotator/controller_test.go(16 hunks)pkg/controller/external_secrets/certificate.go(3 hunks)pkg/controller/external_secrets/certificate_test.go(13 hunks)pkg/controller/external_secrets/constants.go(2 hunks)pkg/controller/external_secrets/controller.go(8 hunks)pkg/controller/external_secrets/deployments.go(12 hunks)pkg/controller/external_secrets/deployments_test.go(12 hunks)pkg/controller/external_secrets/install_external_secrets.go(3 hunks)pkg/controller/external_secrets/rbacs.go(6 hunks)pkg/controller/external_secrets/rbacs_test.go(4 hunks)pkg/controller/external_secrets/secret.go(2 hunks)pkg/controller/external_secrets/secret_test.go(2 hunks)pkg/controller/external_secrets/service_test.go(3 hunks)pkg/controller/external_secrets/serviceaccounts.go(2 hunks)pkg/controller/external_secrets/serviceaccounts_test.go(4 hunks)pkg/controller/external_secrets/services.go(3 hunks)pkg/controller/external_secrets/suite_test.go(0 hunks)pkg/controller/external_secrets/utils.go(3 hunks)pkg/controller/external_secrets/validatingwebhook.go(4 hunks)pkg/controller/external_secrets/validatingwebhook_test.go(1 hunks)pkg/controller/external_secrets_manager/controller.go(7 hunks)pkg/controller/external_secrets_manager/controller_test.go(7 hunks)pkg/controller/external_secrets_manager/externalsecretsmanager.go(1 hunks)pkg/operator/setup_manager.go(2 hunks)test/apis/README.md(1 hunks)test/apis/generator.go(1 hunks)test/apis/suite_test.go(1 hunks)test/apis/vars.go(1 hunks)test/e2e/e2e_test.go(4 hunks)test/e2e/testdata/aws_external_secret.yaml(1 hunks)test/e2e/testdata/aws_secret_store.yaml(1 hunks)test/e2e/testdata/external_secret.yaml(1 hunks)test/e2e/testdata/push_secret.yaml(1 hunks)tools/tools.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/controller/external_secrets/suite_test.go
- api/v1alpha1/external_secrets_types.go
✅ Files skipped from review due to trivial changes (3)
- pkg/controller/external_secrets_manager/externalsecretsmanager.go
- api/v1alpha1/conditions.go
- test/apis/README.md
🔇 Additional comments (89)
test/e2e/testdata/aws_external_secret.yaml (1)
5-5: LGTM! Test data improvements.The label update to
aws-external-secretprovides better specificity, and using the${CLUSTERSECRETSTORE_NAME}template variable improves test configurability.Also applies to: 12-12
test/e2e/testdata/push_secret.yaml (1)
5-5: LGTM! Consistent test data improvements.The changes mirror those in
aws_external_secret.yaml, providing better resource naming and test configurability through template variables.Also applies to: 12-12
tools/tools.go (1)
14-14: LGTM! Ginkgo v2 dependency tracking added.Adding the Ginkgo v2 tool dependency is appropriate and aligns with the test infrastructure updates mentioned in the AI summary.
pkg/controller/external_secrets/constants.go (1)
49-49: LGTM! Comment updated for API structure change.The comment correctly reflects the new ExternalSecretsConfig API structure.
PROJECT (1)
19-19: LGTM! Kubebuilder scaffolding updated.The PROJECT file correctly reflects the rename from ExternalSecrets to ExternalSecretsConfig in the Kubebuilder configuration.
pkg/controller/common/constants.go (2)
15-15: LGTM! Comment updated.The comment correctly reflects the updated CR name in the operator domain.
12-13: No remaining references toExternalSecretsObjectName
A repository-wide search found zero occurrences of the old constant name—rename toExternalSecretsConfigObjectNameis complete.pkg/operator/setup_manager.go (3)
23-32: LGTM! Controller wiring updated for ExternalSecretsConfig.The variable rename from
externalsecretstoexternalSecretsConfigand the subsequent usage correctly reflect the migration to the new ExternalSecretsConfig resource type.
34-45: LGTM! CertManager check updated.The conditional logic correctly uses the renamed
externalSecretsConfigvariable for the CertManager installation check.
52-55: LGTM! Log message updated.The log message correctly references the updated resource name
externalsecretsmanagers.operator.openshift.io(plural form).test/e2e/testdata/aws_secret_store.yaml (1)
5-7: LGTM! Test templating improves flexibility.The changes align the label value with the resource identifier and introduce a template variable for the resource name, which enables better test parameterization.
pkg/controller/crd_annotator/controller_test.go (5)
10-10: LGTM! Import added for NotFound error construction.The
schemapackage import is correctly added to support theerrors.NewNotFoundcalls with the updated resource type.
31-42: LGTM! Helper function updated consistently.The function has been correctly renamed and updated to work with
ExternalSecretsConfig, maintaining the same logic for configuring CRD annotation settings.
77-80: LGTM! Type switches updated consistently.All type switch cases have been correctly updated to handle
*operatorv1alpha1.ExternalSecretsConfiginstead of the previous type, with proper DeepCopyInto usage maintained.Also applies to: 106-110, 146-147, 174-178, 198-201, 220-224, 252-256, 289-293, 318-322, 349-353, 382-386
162-162: LGTM! Error messages updated to reference correct resource.Error messages now correctly reference
externalsecretsconfigs.operator.openshift.ioinstead of the old resource name, aligning with the API rename.Also applies to: 403-403
420-425: LGTM! Status verification correctly updated.The status verification logic has been properly updated to work with
ExternalSecretsConfigobjects and uses the correct constant for the resource name.pkg/controller/common/utils.go (5)
59-68: LGTM! Prevents panic on nil label map.The nil check initializes the labels map when it's nil, preventing potential panics during label assignment. This is a defensive programming improvement.
384-389: LGTM! New mode evaluation helper added.The
EvalModefunction provides a clean abstraction for evaluating mode values, returningtrueonly forEnabledmode as documented.
397-401: LGTM! Function signature updated for ExternalSecretsConfig.The function correctly accepts
*operatorv1alpha1.ExternalSecretsConfigand navigates the updated nested configuration path to check if cert-manager annotation injection is enabled.
405-434: LGTM! Finalizer logic updated with best practices.The function now uses
client.ObjectKeyFromObject(obj)which is preferred over manual NamespacedName construction. The switch case correctly handles*operatorv1alpha1.ExternalSecretsConfigin the post-update fetch-and-copy logic.
438-450: LGTM! Finalizer removal updated consistently.The function correctly uses
client.ObjectKeyFromObject(obj)and updates error messages to be more descriptive with the namespacedName.pkg/controller/external_secrets/serviceaccounts_test.go (3)
30-33: LGTM! Test function signature updated.The function parameter has been correctly renamed from
updateExternalSecretsObjtoupdateExternalSecretsConfigto reflect the type change toExternalSecretsConfig.
75-83: LGTM! Test configurations updated for new API structure.The test cases correctly configure the new nested structure:
- Bitwarden plugin config via
Spec.Plugins.BitwardenSecretManagerProvider- Cert-manager config via
Spec.ControllerConfig.CertProvider.CertManagerThese paths align with the new
ExternalSecretsConfigAPI structure.Also applies to: 98-108
136-141: LGTM! Test execution updated consistently.The test now correctly creates an
ExternalSecretsConfigobject usingcommontest.TestExternalSecretsConfig()and passes it tocreateOrApplyServiceAccounts, maintaining the test's original intent while using the renamed type.api/v1alpha1/zz_generated.deepcopy.go (1)
1-549: Auto-generated deepcopy code.This file is auto-generated by controller-gen and contains DeepCopy implementations for the API types. The changes reflect the rename from ExternalSecrets to ExternalSecretsConfig and the addition of new configuration types. As an auto-generated file, it should not be manually edited.
test/apis/suite_test.go (2)
59-70: LGTM: Server version validation is robust.The Kubernetes server version check correctly validates that the test environment meets the minimum version requirement (1.25) for CEL support. The parsing logic handles version suffixes (e.g., "1.25+abc123") appropriately.
37-74: All referenced test variables and functions are defined:testEnv,cfg,testScheme,k8sClient,ctxare declared in test/apis/vars.go;LoadTestSuiteSpecsandGenerateTestSuiteare implemented in test/apis/generator.go.pkg/controller/external_secrets/install_external_secrets.go (1)
23-26: LGTM: Function signature updated to ExternalSecretsConfig.The function signature has been correctly updated from
ExternalSecretstoExternalSecretsConfig, aligning with the broader API surface migration across the codebase.README.md (1)
25-36: LGTM: Documentation accurately reflects the API surface changes.The README has been properly updated to reflect the renamed CRD from
externalsecretsmanagers.openshift.operator.iotoexternalsecretsmanagers.operator.openshift.ioand the introduction ofexternalsecretsconfigs.operator.openshift.io. The descriptions clearly explain the controller responsibilities and singleton pattern.pkg/controller/external_secrets/validatingwebhook_test.go (1)
119-131: LGTM: Test helper correctly migrated to ExternalSecretsConfig.The test helper function has been properly updated to use
ExternalSecretsConfiginstead ofExternalSecrets, with the spec structure correctly adjusted to match the new API surface. The use ofcommontest.TestExternalSecretsConfig()promotes test consistency.pkg/controller/external_secrets/service_test.go (1)
127-131: LGTM: Test updated to use ExternalSecretsConfig API.The test has been correctly refactored to use
ExternalSecretsConfiginstead ofExternalSecrets, with proper configuration of thePluginsConfig.BitwardenSecretManagerProviderfield. The test structure remains clear and follows the established pattern.go.mod (1)
9-9: LGTM: Dependency updates align with test infrastructure requirements.The dependency updates are consistent with the PR's introduction of Ginkgo-based test infrastructure and Kubernetes API surface changes:
- Ginkgo v2.25.3 and Gomega v1.38.2 support the new test suite.
- Kubernetes v1.32.8 and controller-runtime v0.20.5 align with the API version requirements.
- ghodss/yaml and yaml-patch libraries support CRD YAML processing.
Based on learnings: controller-runtime v0.20.x is built against k8s.io/* v1.32 series and requires Go 1.23. Ensure your CI environment uses Go 1.23+.
Also applies to: 14-15, 26-28
pkg/controller/external_secrets/certificate_test.go (1)
469-487: Helper sets OperatingNamespace = "test-ns"; ensure it matches commontest namespaceMany test expectations use commontest.TestExternalSecretsNamespace. Please ensure OperatingNamespace equals that value to prevent subtle namespace mismatches.
pkg/controller/external_secrets/secret.go (1)
14-22: Skip when cert-manager enabled: LGTMEarly-return on cert-manager mode is correct and avoids conflicting secret management.
config/rbac/role.yaml (1)
63-72: Validate removal of deployments delete permissionIf the operator deletes Deployments during reconcile/uninstall, this will fail. Please verify no code paths call Delete on Deployments.
pkg/controller/external_secrets/services.go (1)
14-48: Services reconciliation updates look good.Correct parameterization to ExternalSecretsConfig, proper existence checks, and event recording. No issues.
Also applies to: 50-86
pkg/controller/commontest/utils.go (1)
11-26: Test fixtures align with ExternalSecretsConfig rename.Constants and helper return types look correct.
Also applies to: 33-40, 42-49
pkg/controller/external_secrets/validatingwebhook.go (1)
15-51: ValidatingWebhook reconciliation migration looks correct.Good use of esc context, object keys, and annotation management.
Also applies to: 54-87
pkg/controller/external_secrets/certificate.go (1)
19-21: Retain serviceExternalSecretWebhookName: it’s referenced in certificate_test.go at multiple locations and isn’t unused.pkg/controller/external_secrets/deployments.go (11)
15-15: LGTM! Import addition is correct.The import of
sigs.k8s.io/controller-runtime/pkg/clientis necessary for theclient.ObjectKeyFromObjectusage at line 75.
23-62: LGTM! Function signature and logic correctly updated.The function signature now accepts
esc *operatorv1alpha1.ExternalSecretsConfiginstead of the previous type, and all downstream calls have been properly updated to useesc. The conditional logic for cert-manager and Bitwarden deployments correctly references the new helper functionsisCertManagerConfigEnabled(esc)andisBitwardenConfigEnabled(esc).
120-125: LGTM! Webhook certificate check interval logic is correct.The logic correctly extracts the check interval from
esc.Spec.ApplicationConfig.WebhookConfig.CertificateCheckIntervalwith appropriate nil checks and falls back to a default value of "5m". The Duration.String() conversion is appropriate.
176-197: LGTM! Resource requirements update correctly handles both ESC and global config.The function signature has been updated to accept
esc *operatorv1alpha1.ExternalSecretsConfig, and the logic appropriately checksesc.Spec.ApplicationConfig.Resourcesfirst before falling back to the global config. The validation and application logic remains correct.
207-226: LGTM! Node selector update correctly handles both ESC and global config.The function signature and logic have been properly updated to accept
escand checkesc.Spec.ApplicationConfig.NodeSelectorfirst before falling back to the global config. The validation remains correct.
229-248: LGTM! Affinity rules update correctly handles both ESC and global config.The function signature and logic have been properly updated to accept
escand checkesc.Spec.ApplicationConfig.Affinityfirst before falling back to the global config. The validation remains correct.
251-270: LGTM! Pod tolerations update correctly handles both ESC and global config.The function signature and logic have been properly updated to accept
escand checkesc.Spec.ApplicationConfig.Tolerationsfirst before falling back to the global config. The validation remains correct.
291-300: LGTM! Status update function correctly updates image information.The function properly updates the status fields for both ExternalSecrets and Bitwarden SDK images when they differ from the environment variables, and correctly calls the status update method.
303-328: LGTM! Container spec update correctly uses ESC for namespace.The function signature has been updated to accept
esc *operatorv1alpha1.ExternalSecretsConfig, and the namespace is correctly obtained viagetOperatingNamespace(esc). All other logic remains correct.
331-352: LGTM! Webhook container spec correctly uses check interval parameter.The function signature has been updated to accept the
checkIntervalparameter, and it's correctly applied to the webhook arguments. The logic remains correct.
75-75: ObjectKeyFromObject usage is correct. updateNamespace(obj, esc) sets the namespace and the embedded Deployment asset defines metadata.name, so client.ObjectKeyFromObject(deployment) yields the same NamespacedName as before.test/apis/generator.go (6)
25-58: LGTM! Test suite discovery logic is correct.The function properly walks the provided paths, filters for
.testsuite.yamlfiles intestsdirectories, and loads them intoSuiteSpecstructs. The error handling is appropriate.
60-93: LGTM! Suite file loading and validation is correct.The function properly reads, unmarshals, and validates the suite spec, including checking for the required
crdNamefield and populating thePerTestRuntimeInfoandVersionfields with appropriate defaults. Error messages are clear and helpful.
95-146: LGTM! Test suite generation is well-structured.The function correctly iterates over CRD filenames, installs CRDs using envtest, generates test names, and sets up BeforeEach/AfterEach hooks for test isolation. The use of
OncePerOrderedis appropriate for CRD installation/cleanup, and the resource cleanup in AfterEach ensures test isolation.
148-203: LGTM! OnCreate test generation is correct.The function properly generates table-driven tests for OnCreate scenarios, handling resource names, generateName, namespaces, and expected errors. The assertion logic correctly compares the created object with the expected state, ignoring autogenerated metadata.
205-363: LGTM! OnUpdate test generation handles complex scenarios correctly.The function properly handles CRD patching with sentinel fields, status updates, and validates both spec and status update errors. The use of
Eventuallyfor CRD schema propagation is appropriate, and the cleanup of sentinel fields ensures tests don't pollute the CRD schema.
365-586: LGTM! Helper functions are well-implemented.The helper functions for creating unstructured objects, loading CRDs, filtering versions, generating suite names, and applying patches are all correctly implemented. The error handling is appropriate throughout.
api/v1alpha1/meta.go (7)
4-4: LGTM! Import addition is correct.The import of
corev1types is necessary for theCommonConfigsstruct fields that referencecorev1.ResourceRequirements,corev1.Affinity, andcorev1.Toleration.
8-16: LGTM! ConditionalStatus type is well-defined.The type correctly uses kubebuilder markers for patch strategy and list type, ensuring proper merge behavior for condition arrays.
18-37: LGTM! ObjectReference validation markers are appropriate.The validation markers correctly enforce:
- Name: MinLength 1, MaxLength 253, Required
- Kind: MinLength 1, MaxLength 253, Optional
- Group: MinLength 1, MaxLength 253, Optional
These constraints align with Kubernetes naming conventions.
39-46: LGTM! SecretReference validation markers are appropriate.The validation markers correctly enforce MinLength 1, MaxLength 253, and Required for the secret name, aligning with Kubernetes naming conventions.
48-89: LGTM! CommonConfigs type is well-structured.The struct correctly defines common configuration fields with appropriate validation markers:
- LogLevel: Default 1, Min 1, Max 5 (aligns with Kubernetes logging guidelines)
- Resources: Optional resource requirements
- Affinity: Optional affinity rules
- Tolerations: Optional, MinItems 0, MaxItems 50
- NodeSelector: Optional, MinProperties 0, MaxProperties 50
- Proxy: Optional proxy configuration
The field limits (50 for tolerations and nodeSelector) are reasonable to prevent excessive configuration complexity.
91-113: LGTM! ProxyConfig type is well-defined.The struct correctly defines proxy configuration fields with appropriate length constraints:
- HTTPProxy: MaxLength 2048
- HTTPSProxy: MaxLength 2048
- NoProxy: MaxLength 4096
These limits are reasonable for typical proxy URL and no-proxy list configurations.
115-124: LGTM! Mode type and constants are correctly defined.The string-based Mode type with Enabled/Disabled constants is a clean approach for representing operational state.
pkg/controller/external_secrets_manager/controller_test.go (5)
71-94: LGTM! Test case correctly uses ExternalSecretsConfig.The test case properly constructs an
ExternalSecretsConfigobject with the correct object name (common.ExternalSecretsConfigObjectName), and the DeepCopyInto call is correct.
131-137: LGTM! Error message correctly references the new resource name.The error message has been properly updated to reference
externalsecretsmanagers.operator.openshift.ioand uses the correct resource name.
140-159: LGTM! Test case correctly handles ExternalSecretsConfig not found.The test case properly handles the NotFound scenario for
ExternalSecretsConfigand returns the appropriate error from the mock.
161-181: LGTM! Test case and error message correctly updated.The test case properly handles the ServerTimeout scenario for
ExternalSecretsConfig, and the error message has been correctly updated to referenceexternalsecretsconfigs.operator.openshift.io.
201-224: LGTM! Test case correctly uses ExternalSecretsConfig with status.The test case properly constructs an
ExternalSecretsConfigobject with status conditions, and the DeepCopyInto call is correct.pkg/controller/external_secrets/controller.go (7)
81-91: LGTM! Reconciler comment correctly updated.The comment has been properly updated to reference
ExternalSecretsConfiginstead of the previous type.
93-103: LGTM! RBAC markers correctly updated.The kubebuilder RBAC markers have been properly updated to reference
externalsecretsconfigsinstead of the previous resource name, maintaining the same permissions (get, list, watch, create, update for the main resource, and get/update for status and finalizers).
181-182: LGTM! BuildCustomClient correctly references ExternalSecretsConfig.The
ownObjectvariable has been correctly updated to use&operatorv1alpha1.ExternalSecretsConfig{}instead of the previous type.
260-278: LGTM! SetupWithManager correctly watches ExternalSecretsConfig.The controller setup has been properly updated to watch
&operatorv1alpha1.ExternalSecretsConfig{}and use the correctcommon.ExternalSecretsConfigObjectNamein the reconcile request mapping.
327-374: LGTM! Reconcile function correctly handles ExternalSecretsConfig.The Reconcile function has been properly updated to:
- Fetch
ExternalSecretsConfiginstead of the previous type- Use updated variable names (esc instead of es)
- Reference the correct resource name in log messages and error messages
- Handle deletion and finalizers correctly
376-448: LGTM! processReconcileRequest signature and logic correctly updated.The function signature has been updated to accept
esc *operatorv1alpha1.ExternalSecretsConfig, and all logic has been properly updated to use the new parameter and status structure. Condition handling remains correct.
450-461: LGTM! cleanUp function correctly updated.The function signature and implementation have been properly updated to:
- Accept
esc *operatorv1alpha1.ExternalSecretsConfig- Use the correct resource name in event messages
- Handle finalizer removal correctly
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
80-95: Defaulting dependencies — verify defaulters/webhooks are wired.Expected outputs include defaulted fields (e.g., controllerConfig.periodicReconcileInterval, certManager.certificateDuration/renewBefore, appConfig.logLevel, injectAnnotations=false). Ensure the mutating/defaulter logic is implemented and registered; otherwise these tests will fail.
Also applies to: 153-164, 175-186, 429-449, 468-486, 531-539
docs/api_reference.md (1)
187-208: API docs alignment looks good.The documented defaults/validations (Mode required, logLevel bounds, ProxyConfig limits, singleton semantics) match the types and tests.
Also applies to: 210-226, 228-244, 246-262, 339-359, 361-377, 379-385, 392-406, 408-426, 441-458
api/v1alpha1/external_secrets_manager_types.go (1)
24-38: Types and validations align with docs/CRD.Labels constraints, inline CommonConfigs, and required controller status fields look consistent.
Also applies to: 55-73, 75-88, 90-106
bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml (1)
7-16: CRD schema updates are consistent.Added labels, categories, shortNames, printer columns, and validation constraints match the Go types and docs.
Please ensure the generated CRD in config/crd/bases is in sync and that CSV picks up the new categories/shortNames.
Also applies to: 20-31, 59-67, 995-1004, 1006-1011, 1018-1023, 1024-1050, 1159-1215
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (2)
7-10: LGTM: labels/name alignment for the new CRD.Accurate rename to externalsecretsconfigs.operator.openshift.io with consistent labels.
1406-1409: LGTM: singleton enforcement via CEL.The metadata.name == 'cluster' rule matches the singleton intent.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
197-206: LGTM: owned CRD sample updated to ExternalSecretsConfig.Sample object reflects singleton name “cluster”.
config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml (3)
7-9: LGTM: CRD labeling.Consistent app labels for discoverability.
14-24: LGTM: discoverability improvements.categories/shortNames additions and AGE printer column are sensible.
994-1004: LGTM: labels map constraints.maxProperties and granular map type are appropriate.
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (4)
7-11: LGTM: CRD rename and labels.Correct resource name and labeling for ExternalSecretsConfig.
1323-1331: LGTM: cross‑field validation for Bitwarden plugin prerequisites.CEL rule correctly couples secretRef or certManager mode when plugin is Enabled.
1406-1409: LGTM: singleton enforcement via CEL..metadata.name == 'cluster' rule is correct for singleton behavior.
20-24: Verify shortNames for collisions
Confirm “esc”, “externalsecretsconfig” and “esconfig” don’t overlap with any existing CRD shortNames in this repo or in the external-secrets upstream.
...ha1/tests/externalsecretsmanager.operator.openshift.io/externalsecretsmanager.testsuite.yaml
Show resolved
Hide resolved
| k8s.io/kubernetes v1.32.8 | ||
| k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 | ||
| sigs.k8s.io/controller-runtime v0.20.4 | ||
| sigs.k8s.io/controller-runtime v0.20.5-0.20250517180713-32e5e9e948a5 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify pre-release controller-runtime stability.
The controller-runtime dependency is pinned to a pre-release version v0.20.5-0.20250517180713-32e5e9e948a5 (a specific commit). While this aligns with Kubernetes v0.32.x dependencies, pre-release versions may contain bugs or breaking changes.
Verify the stability and change log of this controller-runtime version:
Based on learnings: controller-runtime v0.20.x requires Go 1.23 and is compatible with k8s.io/* v1.32 series, which matches your setup. Ensure you're on Go 1.23+ and verify this specific commit has been tested.
🌐 Web query:
sigs.k8s.io/controller-runtime v0.20.5 release notes and stability status
💡 Result:
Short answer
- There is no release tagged v0.20.5 for sigs.k8s.io/controller-runtime on the official GitHub releases list (latest v0.20.x shown is v0.20.4). (checked Oct 8, 2025). [1]
- Stability: controller-runtime v0.20 (the v0.20.x line) is a maintained minor series based on k8s.io/* v0.32 and requires Go >= 1.23; it's a production-grade library maintained by the kubernetes-sigs project (see compatibility/minimum Go info). [2]
Sources
- kubernetes-sigs/controller-runtime — Releases (shows v0.20.4 as latest v0.20.x). [GitHub]. [1]
- sigs.k8s.io/controller-runtime package / compatibility & minimum Go version (CR v0.20 → k8s.io/* v0.32, Go >= 1.23). [pkg.go.dev]. [2]
If you want, I can: (a) watch the repo and notify when v0.20.5 appears, or (b) pull the full changelog for v0.20.4.
Pin controller-runtime to latest official v0.20.x
go.mod currently references a pseudo-version for v0.20.5 (no matching GitHub tag). Replace with v0.20.4 unless you specifically require that commit—in which case document and test its changes. Controller-runtime v0.20.x is stable and requires Go 1.23+.
🤖 Prompt for AI Agents
In go.mod around line 28, the controller-runtime dependency is pinned to a
pseudo-version (v0.20.5-...32e5e9e948a5) that has no matching tag; replace it
with the official v0.20.4 tag (sigs.k8s.io/controller-runtime v0.20.4) unless
you intentionally need that specific commit—in that case add a short comment in
the repo documenting why the pseudo-version is required, include the commit hash
and expected behavior, and add tests validating those changes; after updating
run go mod tidy and run the test suite to ensure no breakages and confirm your
Go toolchain is 1.23+ as required by controller-runtime v0.20.x.
| echo ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${REPO_ROOT}/test/apis | ||
| ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${REPO_ROOT}/test/apis |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote variable expansions to prevent word splitting.
The unquoted variable expansions ${GINKGO}, ${GINKGO_ARGS}, ${GINKGO_EXTRA_ARGS}, and ${REPO_ROOT} on lines 27-28 may cause unexpected word splitting or globbing if these variables contain spaces or special characters.
Apply this diff to quote the expansions:
-echo ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${REPO_ROOT}/test/apis
-${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${REPO_ROOT}/test/apis
+echo "${GINKGO}" ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} "${REPO_ROOT}"/test/apis
+"${GINKGO}" ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} "${REPO_ROOT}"/test/apisNote: GINKGO_ARGS and GINKGO_EXTRA_ARGS are intentionally left unquoted to allow word splitting for passing multiple flags.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${REPO_ROOT}/test/apis | |
| ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${REPO_ROOT}/test/apis | |
| echo "${GINKGO}" ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} "${REPO_ROOT}"/test/apis | |
| "${GINKGO}" ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} "${REPO_ROOT}"/test/apis |
🤖 Prompt for AI Agents
In hack/test-apis.sh around lines 27-28, the variable expansions are unquoted
which can cause word splitting or globbing; wrap GINKGO and REPO_ROOT in double
quotes on both the echo and the command lines (use "${GINKGO}" and
"${REPO_ROOT}") while intentionally leaving GINKGO_ARGS and GINKGO_EXTRA_ARGS
unquoted so they can still split into multiple flags.
| // Reconciler reconciles externalsecretsmanagers.operator.openshift.io CR. | ||
| type Reconciler struct { | ||
| operatorclient.CtrlClient | ||
| Scheme *runtime.Scheme | ||
| ctx context.Context | ||
| eventRecorder record.EventRecorder | ||
| log logr.Logger | ||
| now *common.Now | ||
| externalSecrets *operatorv1alpha1.ExternalSecrets | ||
| Scheme *runtime.Scheme | ||
| ctx context.Context | ||
| eventRecorder record.EventRecorder | ||
| log logr.Logger | ||
| now *common.Now | ||
| esc *operatorv1alpha1.ExternalSecretsConfig | ||
| } |
There was a problem hiding this comment.
Avoid shared mutable state in Reconciler; pass ESC locally.
Storing request-scoped ESC on the struct can race if controller concurrency >1. Pass it down instead.
Apply this diff:
type Reconciler struct {
operatorclient.CtrlClient
Scheme *runtime.Scheme
ctx context.Context
eventRecorder record.EventRecorder
log logr.Logger
now *common.Now
- esc *operatorv1alpha1.ExternalSecretsConfig
}
@@
- // Fetch the externalsecretsconfigs.operator.openshift.io CR
- r.esc = new(operatorv1alpha1.ExternalSecretsConfig)
+ // Fetch the externalsecretsconfigs.operator.openshift.io CR
+ esc := new(operatorv1alpha1.ExternalSecretsConfig)
key = types.NamespacedName{
Name: common.ExternalSecretsConfigObjectName,
}
- if err := r.Get(ctx, key, r.esc); err != nil {
+ if err := r.Get(ctx, key, esc); err != nil {
@@
- return r.processReconcileRequest(esm)
+ return r.processReconcileRequest(esm, esc)
}
-// processReconcileRequest is the reconciliation handler to manage the resources.
-func (r *Reconciler) processReconcileRequest(esm *operatorv1alpha1.ExternalSecretsManager) (ctrl.Result, error) {
+// processReconcileRequest is the reconciliation handler to manage the resources.
+func (r *Reconciler) processReconcileRequest(esm *operatorv1alpha1.ExternalSecretsManager, esc *operatorv1alpha1.ExternalSecretsConfig) (ctrl.Result, error) {
@@
- if r.esc != nil && len(r.esc.Status.Conditions) > 0 {
- for _, esCond := range r.esc.Status.Conditions {
+ if esc != nil && len(esc.Status.Conditions) > 0 {
+ for _, esCond := range esc.Status.Conditions {Also applies to: 140-156, 159-170
| // finalizer name for externalsecretsconfigs.operator.openshift.io resource. | ||
| finalizer = "externalsecretsconfigs.operator.openshift.io/" + ControllerName |
There was a problem hiding this comment.
🧩 Analysis chain
Verify finalizer migration across the codebase.
The finalizer name has been updated to match the ExternalSecretsConfig rename. Ensure that any existing resources with the old finalizer (externalsecrets.openshift.operator.io/external-secrets-controller) are properly migrated or that the controller can handle both during the transition.
🏁 Script executed:
#!/bin/bash
# Description: Search for any remaining references to the old finalizer name
# Search for old finalizer references
rg -n "externalsecrets\.openshift\.operator\.io" --type yaml --type go -g '!*.md'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search entire repository for old finalizer references
rg -n "externalsecrets\.openshift\.operator\.io"Length of output: 189
Update e2e tests to use the new finalizer name
Lines 111 and 116 in test/e2e/e2e_test.go still reference externalsecrets.openshift.operator.io; update these to externalsecretsconfigs.operator.openshift.operator.io (or, better, use the shared finalizer constant).
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/constants.go around lines 19-20, the
finalizer is defined as an unexported constant
("externalsecretsconfigs.operator.openshift.io/"+ControllerName) while e2e tests
(test/e2e/e2e_test.go lines ~111 and ~116) still reference the old literal;
export the constant (rename to Finalizer) so tests can import it, update the
tests to import the package and reference the shared Finalizer constant instead
of the hard-coded string (or, if you prefer a quick fix, change the two literals
in the tests to "externalsecretsconfigs.operator.openshift.operator.io" to match
the new name).
| if tt.wantErr == "" && externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { | ||
| t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, "test-image") | ||
| } |
There was a problem hiding this comment.
Align expected message with actual expected value.
The assertion compares against commontest.TestExternalSecretsImageName but prints want test-image. Update the message.
- t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, "test-image")
+ t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, commontest.TestExternalSecretsImageName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if tt.wantErr == "" && externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { | |
| t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, "test-image") | |
| } | |
| if tt.wantErr == "" && externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { | |
| t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, commontest.TestExternalSecretsImageName) | |
| } |
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/deployments_test.go around lines 363 to 365,
the test assertion compares externalsecrets.Status.ExternalSecretsImage to
commontest.TestExternalSecretsImageName but the error message prints the literal
"test-image"; change the error message to reference
commontest.TestExternalSecretsImageName so the printed expected value matches
the actual expected constant, e.g., replace the hardcoded "test-image" in the
t.Errorf call with commontest.TestExternalSecretsImageName.
| By("Creating kubernetes secret to be used in PushSecret") | ||
| secretsAssetFunc := utils.ReplacePatternInAsset(awsSecretValuePattern, base64.StdEncoding.EncodeToString(expectedSecretValue)) | ||
| loader.CreateFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace) | ||
| defer loader.DeleteFromFile(testassets.ReadFile, awsSecretToPushFile, testNamespace) | ||
|
|
||
| By("Creating ClusterSecretStore") | ||
| cssAssetFunc := utils.ReplacePatternInAsset(awsClusterSecretStoreNamePattern, clusterSecretStoreResourceName) | ||
| loader.CreateFromFile(cssAssetFunc, clusterSecretStoreFile, testNamespace) | ||
| defer loader.DeleteFromFile(cssAssetFunc, clusterSecretStoreFile, testNamespace) | ||
|
|
||
| By("Waiting for ClusterSecretStore to become Ready") | ||
| Expect(utils.WaitForESOResourceReady(ctx, dynamicClient, | ||
| schema.GroupVersionResource{ | ||
| Group: externalSecretsGroupName, | ||
| Version: v1APIVersion, | ||
| Resource: clusterSecretStoresKind, | ||
| }, | ||
| "", clusterSecretStoreResourceName, time.Minute, | ||
| )).To(Succeed()) | ||
|
|
||
| By("Creating PushSecret") | ||
| assetFunc := utils.ReplacePatternInAsset(awsSecretNamePattern, awsSecretName, | ||
| awsClusterSecretStoreNamePattern, clusterSecretStoreResourceName) | ||
| loader.CreateFromFile(assetFunc, pushSecretFile, testNamespace) | ||
| defer loader.DeleteFromFile(testassets.ReadFile, pushSecretFile, testNamespace) | ||
|
|
||
| By("Waiting for PushSecret to become Ready") | ||
| Expect(utils.WaitForESOResourceReady(ctx, dynamicClient, | ||
| schema.GroupVersionResource{ | ||
| Group: externalSecretsGroupName, | ||
| Version: v1alpha1APIVersion, | ||
| Resource: PushSecretsKind, | ||
| }, | ||
| testNamespace, pushSecretResourceName, time.Minute, | ||
| )).To(Succeed()) | ||
|
|
||
| By("Creating ExternalSecret") | ||
| loader.CreateFromFile(assetFunc, externalSecretFile, testNamespace) | ||
| defer loader.DeleteFromFile(testassets.ReadFile, externalSecretFile, testNamespace) | ||
|
|
There was a problem hiding this comment.
Cleanup must reuse the templated asset readers
The deletes run with the raw testassets.ReadFile, so the ${…} placeholders never get substituted. As a result, DeleteFromFile issues a delete for the literal placeholder names and silently leaves the real resources behind—risking test flakes when rerunning the suite. Please reuse the same templated asset funcs you used for creation.
- loader.CreateFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace)
- defer loader.DeleteFromFile(testassets.ReadFile, awsSecretToPushFile, testNamespace)
+ loader.CreateFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace)
+ defer loader.DeleteFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace)
…
- loader.CreateFromFile(assetFunc, pushSecretFile, testNamespace)
- defer loader.DeleteFromFile(testassets.ReadFile, pushSecretFile, testNamespace)
+ loader.CreateFromFile(assetFunc, pushSecretFile, testNamespace)
+ defer loader.DeleteFromFile(assetFunc, pushSecretFile, testNamespace)
…
- loader.CreateFromFile(assetFunc, externalSecretFile, testNamespace)
- defer loader.DeleteFromFile(testassets.ReadFile, externalSecretFile, testNamespace)
+ loader.CreateFromFile(assetFunc, externalSecretFile, testNamespace)
+ defer loader.DeleteFromFile(assetFunc, externalSecretFile, testNamespace)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| By("Creating kubernetes secret to be used in PushSecret") | |
| secretsAssetFunc := utils.ReplacePatternInAsset(awsSecretValuePattern, base64.StdEncoding.EncodeToString(expectedSecretValue)) | |
| loader.CreateFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace) | |
| defer loader.DeleteFromFile(testassets.ReadFile, awsSecretToPushFile, testNamespace) | |
| By("Creating ClusterSecretStore") | |
| cssAssetFunc := utils.ReplacePatternInAsset(awsClusterSecretStoreNamePattern, clusterSecretStoreResourceName) | |
| loader.CreateFromFile(cssAssetFunc, clusterSecretStoreFile, testNamespace) | |
| defer loader.DeleteFromFile(cssAssetFunc, clusterSecretStoreFile, testNamespace) | |
| By("Waiting for ClusterSecretStore to become Ready") | |
| Expect(utils.WaitForESOResourceReady(ctx, dynamicClient, | |
| schema.GroupVersionResource{ | |
| Group: externalSecretsGroupName, | |
| Version: v1APIVersion, | |
| Resource: clusterSecretStoresKind, | |
| }, | |
| "", clusterSecretStoreResourceName, time.Minute, | |
| )).To(Succeed()) | |
| By("Creating PushSecret") | |
| assetFunc := utils.ReplacePatternInAsset(awsSecretNamePattern, awsSecretName, | |
| awsClusterSecretStoreNamePattern, clusterSecretStoreResourceName) | |
| loader.CreateFromFile(assetFunc, pushSecretFile, testNamespace) | |
| defer loader.DeleteFromFile(testassets.ReadFile, pushSecretFile, testNamespace) | |
| By("Waiting for PushSecret to become Ready") | |
| Expect(utils.WaitForESOResourceReady(ctx, dynamicClient, | |
| schema.GroupVersionResource{ | |
| Group: externalSecretsGroupName, | |
| Version: v1alpha1APIVersion, | |
| Resource: PushSecretsKind, | |
| }, | |
| testNamespace, pushSecretResourceName, time.Minute, | |
| )).To(Succeed()) | |
| By("Creating ExternalSecret") | |
| loader.CreateFromFile(assetFunc, externalSecretFile, testNamespace) | |
| defer loader.DeleteFromFile(testassets.ReadFile, externalSecretFile, testNamespace) | |
| By("Creating kubernetes secret to be used in PushSecret") | |
| secretsAssetFunc := utils.ReplacePatternInAsset( | |
| awsSecretValuePattern, | |
| base64.StdEncoding.EncodeToString(expectedSecretValue), | |
| ) | |
| loader.CreateFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace) | |
| defer loader.DeleteFromFile(secretsAssetFunc, awsSecretToPushFile, testNamespace) | |
| By("Creating ClusterSecretStore") | |
| cssAssetFunc := utils.ReplacePatternInAsset( | |
| awsClusterSecretStoreNamePattern, | |
| clusterSecretStoreResourceName, | |
| ) | |
| loader.CreateFromFile(cssAssetFunc, clusterSecretStoreFile, testNamespace) | |
| defer loader.DeleteFromFile(cssAssetFunc, clusterSecretStoreFile, testNamespace) | |
| By("Waiting for ClusterSecretStore to become Ready") | |
| Expect(utils.WaitForESOResourceReady(ctx, dynamicClient, | |
| schema.GroupVersionResource{ | |
| Group: externalSecretsGroupName, | |
| Version: v1APIVersion, | |
| Resource: clusterSecretStoresKind, | |
| }, | |
| "", clusterSecretStoreResourceName, time.Minute, | |
| )).To(Succeed()) | |
| By("Creating PushSecret") | |
| assetFunc := utils.ReplacePatternInAsset( | |
| awsSecretNamePattern, awsSecretName, | |
| awsClusterSecretStoreNamePattern, clusterSecretStoreResourceName, | |
| ) | |
| loader.CreateFromFile(assetFunc, pushSecretFile, testNamespace) | |
| defer loader.DeleteFromFile(assetFunc, pushSecretFile, testNamespace) | |
| By("Waiting for PushSecret to become Ready") | |
| Expect(utils.WaitForESOResourceReady(ctx, dynamicClient, | |
| schema.GroupVersionResource{ | |
| Group: externalSecretsGroupName, | |
| Version: v1alpha1APIVersion, | |
| Resource: PushSecretsKind, | |
| }, | |
| testNamespace, pushSecretResourceName, time.Minute, | |
| )).To(Succeed()) | |
| By("Creating ExternalSecret") | |
| loader.CreateFromFile(assetFunc, externalSecretFile, testNamespace) | |
| defer loader.DeleteFromFile(assetFunc, externalSecretFile, testNamespace) |
🤖 Prompt for AI Agents
In test/e2e/e2e_test.go around lines 168 to 207 the cleanup calls use the raw
testassets.ReadFile so placeholders are never substituted; update each defer
DeleteFromFile to pass the same templated asset function used for creation
(secretsAssetFunc, cssAssetFunc, assetFunc respectively) instead of
testassets.ReadFile so DeleteFromFile deletes the actual templated resources.
siddhibhor-56
force-pushed
the
network-policy-new
branch
from
04fcadb to
06caf45
Compare
siddhibhor-56
force-pushed
the
network-policy-new
branch
from
06caf45 to
7f050c9
Compare
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-222 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.21.0" version, but no target version was set. In 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. |
|
@siddhibhor-56: This pull request references ESO-222 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.21.0" version, but no target version was set. In 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. |
|
@siddhibhor-56: This pull request references ESO-222 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.21.0" version, but no target version was set. In 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. |
|
@siddhibhor-56: This pull request references ESO-222 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.21.0" version, but no target version was set. In 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. |
|
/label docs-approved /cherrypick release-1.0 |
|
@bharath-b-rh: once the present PR merges, I will cherry-pick it on top of In 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 kubernetes-sigs/prow repository. |
openshift-ci
bot
added
docs-approved
|
@siddhibhor-56: This pull request references ESO-222 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.21.0" version, but no target version was set. In 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. |
Co-authored-by: Bharath B <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, siddhibhor-56 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@siddhibhor-56: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@bharath-b-rh: new pull request created: #76 In 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 kubernetes-sigs/prow repository. |
4 participants
Added network policies in config/network-policy/ for the operator namespace:
- Applies to all pods in the operator namespace
- Blocks all ingress and egress traffic by default
- Permits necessary traffic for operator functionality
- Allows egress to Kubernetes API server
- Allows metrics scraping (if configured)