ESO-186: Make the operator HTTP(s) proxy aware #62
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
chiragkyal
force-pushed
the
http-proxy
branch
from
155cd28 to
cac8b3f
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughEnable operator proxy-awareness and trusted CA bundle support: derive proxy config (ESC → manager global → OLM env), inject/remove proxy env vars into all containers and initContainers, ensure a trusted CA ConfigMap and mount it into Deployments, watch ConfigMaps for metadata changes, update manifests, and add unit tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
pkg/controller/external_secrets/rbacs_test.go (1)
172-183: Wrong asset helper used for ClusterRole name check.Comparing against a ClusterRoleBinding name will mask failures.
- case *rbacv1.ClusterRole: - if obj.GetName() == testClusterRoleBinding(certControllerClusterRoleBindingAssetName).GetName() { + case *rbacv1.ClusterRole: + if obj.GetName() == testClusterRole(certControllerClusterRoleAssetName).GetName() {pkg/controller/common/utils.go (4)
62-68: Potential nil-map panic in UpdateResourceLabels.
obj.GetLabels()can be nil; assigning into it panics.func UpdateResourceLabels(obj client.Object, labels map[string]string) { - l := obj.GetLabels() + l := obj.GetLabels() + if l == nil { + l = map[string]string{} + } for k, v := range labels { l[k] = v } obj.SetLabels(l) }
284-286: Possible nil dereference on SecurityContext compare.Dereferences
*fetchedContainer.SecurityContextwithout nil check when desired is non‑nil and fetched is nil.- // SecurityContext nil check - if desiredContainer.SecurityContext != nil && !reflect.DeepEqual(*desiredContainer.SecurityContext, *fetchedContainer.SecurityContext) { - return true - } + // SecurityContext nil checks + if (desiredContainer.SecurityContext == nil) != (fetchedContainer.SecurityContext == nil) { + return true + } + if desiredContainer.SecurityContext != nil && fetchedContainer.SecurityContext != nil && + !reflect.DeepEqual(*desiredContainer.SecurityContext, *fetchedContainer.SecurityContext) { + return true + }
292-295: Resource comparison logic likely inverted.Currently flags a change when desired resources are empty and fetched aren’t. Usually we want to update only when desired explicitly differs.
- if reflect.DeepEqual(desiredContainer.Resources, corev1.ResourceRequirements{}) && - !reflect.DeepEqual(desiredContainer.Resources, fetchedContainer.Resources) { - return true - } + if !reflect.DeepEqual(desiredContainer.Resources, fetchedContainer.Resources) && + !reflect.DeepEqual(desiredContainer.Resources, corev1.ResourceRequirements{}) { + return true + }If the intended behavior was to clear resources when desired is empty, please confirm and we can gate that behind an explicit flag.
385-387: Function name vs behavior mismatch: IsESMSpecEmpty returns true when spec is non‑empty.This can propagate logic errors.
-func IsESMSpecEmpty(esm *operatorv1alpha1.ExternalSecretsManager) bool { - return esm != nil && !reflect.DeepEqual(esm.Spec, operatorv1alpha1.ExternalSecretsManagerSpec{}) -} +func IsESMSpecEmpty(esm *operatorv1alpha1.ExternalSecretsManager) bool { + if esm == nil { + return true + } + return reflect.DeepEqual(esm.Spec, operatorv1alpha1.ExternalSecretsManagerSpec{}) +}If callers rely on the old (inverted) semantics, consider introducing
IsESMSpecNonEmptyand migrating call sites.pkg/controller/external_secrets/controller.go (1)
412-414: Wrong Reason for Ready=false fatal case.ReasonReady on a non‑ready condition is misleading. Use ReasonFailed.
- readyCond.Status = metav1.ConditionFalse - readyCond.Reason = operatorv1alpha1.ReasonReady + readyCond.Status = metav1.ConditionFalse + readyCond.Reason = operatorv1alpha1.ReasonFailedpkg/controller/external_secrets/install_external_secrets.go (2)
30-46: Guard against nil ControllerConfig before accessing LabelsPotential NPE: esc.Spec.ControllerConfig may be nil.
Apply:
- if len(esc.Spec.ControllerConfig.Labels) != 0 { - for k, v := range esc.Spec.ControllerConfig.Labels { + if esc.Spec.ControllerConfig != nil && len(esc.Spec.ControllerConfig.Labels) != 0 { + for k, v := range esc.Spec.ControllerConfig.Labels {
106-121: createOrApplyNamespace only creates; it never applies labels on existing namespacesThis regresses "apply" semantics; label drift won’t be corrected.
Apply:
if err := r.Create(r.ctx, obj); err != nil { if errors.IsAlreadyExists(err) { - r.log.V(4).Info("namespace already exists", "namespace", namespace) - return nil + // Ensure labels are up to date on existing namespace + existing := &corev1.Namespace{} + if getErr := r.Get(r.ctx, types.NamespacedName{Name: namespace}, existing); getErr != nil { + return getErr + } + common.UpdateResourceLabels(existing, resourceLabels) + if updateErr := r.UpdateWithRetry(r.ctx, existing); updateErr != nil { + return updateErr + } + r.log.V(4).Info("namespace already exists; labels ensured", "namespace", namespace) + return nil } return err }Add missing import:
k8s.io/apimachinery/pkg/typespkg/controller/external_secrets/certificate.go (1)
164-183: Handle unsupported Issuer kind to avoid passing a nil object to Exists()Currently, an unknown kind yields a nil client.Object and risks a panic/pathological behavior.
var object client.Object switch issuerRef.Kind { case clusterIssuerKind: object = &certmanagerv1.ClusterIssuer{} case issuerKind: object = &certmanagerv1.Issuer{} + default: + return false, fmt.Errorf("unsupported issuer kind %q", issuerRef.Kind) } - ifExists, err := r.UncachedClient.Exists(r.ctx, namespacedName, object) + ifExists, err := r.UncachedClient.Exists(r.ctx, namespacedName, object)pkg/controller/external_secrets_manager/controller.go (1)
67-70: Missing RBAC for ExternalSecretsConfigThe controller reads and watches externalsecretsconfigs; add permissions or manager will fail with RBAC errors.
Add:
// +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsconfigs,verbs=get;list;watch
🧹 Nitpick comments (56)
test/apis/README.md (1)
1-1: Fix bare URL and stabilize branch reference.Use a Markdown link and avoid hardcoding the branch name to reduce link rot.
-Refer to https://github.com/openshift/api/tree/master/tests for more details. +Refer to the OpenShift API tests for more details: https://github.com/openshift/api/tree/main/testspkg/controller/external_secrets/constants.go (1)
48-51: Correct comment to reflect actual field path.Spec.Namespace doesn’t exist; Namespace lives under ControllerConfig.
- // will be created, when ExternalSecretsConfig.Spec.Namespace is not set. + // will be created when ExternalSecretsConfig.Spec.ControllerConfig.Namespace is not set.api/v1alpha1/tests/externalsecretsmanager.operator.openshift.io/externalsecretsmanager.testsuite.yaml (2)
210-219: Add acceptance test for noProxy at boundary length.You test “too long”; add a case that accepts exactly 4096 bytes to lock the boundary.
+ - name: Should accept no proxy list at maximum length boundary + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: ExternalSecretsManager + spec: + globalConfig: + logLevel: 1 + proxy: + noProxy: "<string of exactly 4096 bytes>" + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: ExternalSecretsManager + spec: + globalConfig: + logLevel: 1 + proxy: + noProxy: "<string of exactly 4096 bytes>"
92-99: Consider invalid-scheme tests for proxy URLs.Add negative cases for schemes other than http/https (e.g., socks5://) if the CRD enforces scheme.
Also applies to: 244-253
pkg/controller/external_secrets/serviceaccounts_test.go (1)
124-124: Duplicate test name; rename for clarity.Two tests share the same title. Rename this one to reflect the deletion path.
- name: "cert-controller serviceaccount skipped when cert-manager enabled", + name: "cert-controller serviceaccount deletion fails when cert-manager enabled",config/samples/operator_v1alpha1_externalsecrets.yaml (1)
2-8: Align sample with rename and proxy feature
- Consider renaming the file to operator_v1alpha1_externalsecretsconfig.yaml for consistency.
- Add a minimal Proxy configuration example in spec to showcase the PR’s HTTP proxy support.
test/e2e/testdata/external_secret.yaml (1)
2-8: E2E testdata naming and coverage
- Consider renaming to external_secrets_config.yaml for clarity.
- Add a case exercising Proxy settings to validate the new HTTP proxy awareness end‑to‑end.
README.md (1)
28-31: Fix typo in controller bullet“external-scerets” → “external-secrets”.
- * reconciling the `externalsecretsmanagers.openshift.operator.io` resource for the global configurations and updates the `external-scerets` deployment accordingly. + * reconciling the `externalsecretsmanagers.openshift.operator.io` resource for the global configurations and updates the `external-secrets` deployment accordingly.hack/test-apis.sh (1)
16-19: Shell robustness: quote HOME and comparisonAvoid unbound/word-splitting issues; quote HOME and use
:default.-export HOME=${HOME:=/tmp/kubebuilder-testing} -if [ $HOME == "/" ]; then +export HOME="${HOME:-/tmp/kubebuilder-testing}" +if [ "$HOME" = "/" ]; then export HOME=/tmp/kubebuilder-testing fitest/apis/suite_test.go (1)
46-50: Register project APIs with the scheme.Typed client encodes/decodes your CRs only if the scheme knows them.
- testScheme = scheme.Scheme + testScheme = scheme.Scheme + // register operator APIs + Expect(operatorv1alpha1.AddToScheme(testScheme)).To(Succeed())And add imports:
@@ - "testing" + "testing" + "context" @@ - "k8s.io/client-go/discovery" + "k8s.io/client-go/discovery" + operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"If these vars aren’t declared elsewhere, also declare at file scope:
+var ( + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + testScheme = scheme.Scheme +)With import:
+ "k8s.io/client-go/rest"pkg/controller/external_secrets/serviceaccounts.go (2)
40-42: Fix misleading delete error message.Always mentions “cert-controller” even when deleting another SA.
- return fmt.Errorf("failed to delete cert-controller serviceaccount: %w", err) + return fmt.Errorf("failed to delete serviceaccount %q: %w", serviceAccount.assetName, err)
15-15: Optional: rename boolean param for clarity.
externalsecretsCreateReconnow mismatches type naming. ConsidercreateReconcileorescCreateRecon.api/v1alpha1/meta.go (3)
58-63: Enforce immutability of Resources via CEL (comment says “Cannot be updated”).Add an XValidation rule so the API enforces it.
- // Cannot be updated. + // Cannot be updated. + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="resources is immutable"
91-111: Proxy config shape is fine; consider follow-ups.
- NoProxy as a string is acceptable, but a string list (slice) avoids comma-splitting issues later.
- Optionally add URL pattern checks for http(s)Proxy.
Would you like a follow-up PR to add patterns and a helper to translate ProxyConfig to env vars (HTTP_PROXY/HTTPS_PROXY/NO_PROXY and lowercase variants)?
72-84: Hard caps on Tolerations/NodeSelector may be too restrictive.MaxItems/MaxProperties of 10 can block legitimate configurations. Consider removing or raising.
pkg/controller/common/utils.go (1)
432-437: Generic RemoveFinalizer error messages hardcode resource kind.Errors always mention
externalsecretsconfig...even when removing from other types.- return fmt.Errorf("failed to create %q externalsecretsconfig.openshift.operator.io object with finalizers removed", namespacedName) + return fmt.Errorf("failed to update %q with finalizers removed", namespacedName) @@ - return fmt.Errorf("failed to remove finalizers on %q externalsecretsconfig.openshift.operator.io with %w", namespacedName, err) + return fmt.Errorf("failed to remove finalizers on %q: %w", namespacedName, err)Optionally specialize by inspecting the object’s GVK.
Makefile (1)
266-269: Optional: add a simple clean target (static checkmake warning).Helps CI hygiene.
.PHONY: ginkgo ginkgo: $(LOCALBIN) ## Download ginkgo locally if necessary. $(call go-install-tool,$(GINKGO),github.com/onsi/ginkgo/v2/ginkgo) +.PHONY: clean +clean: ## Remove local build artifacts. + rm -rf $(LOCALBIN) cover.out distpkg/controller/crd_annotator/controller_test.go (2)
171-177: Use updated resource name in NotFound error.Reflect the CRD rename for accuracy.
- return errors.NewNotFound(schema.GroupResource{ - Group: operatorv1alpha1.GroupVersion.Group, - Resource: "externalsecrets", - }, commontest.TestExternalSecretsConfigResourceName) + return errors.NewNotFound(schema.GroupResource{ + Group: operatorv1alpha1.GroupVersion.Group, + Resource: "externalsecretsconfigs", + }, commontest.TestExternalSecretsConfigResourceName)
418-423: Consider asserting Get() error in status checks.Ignoring the error can hide regressions when the resource isn’t created.
- r.CtrlClient.Get(r.ctx, key, esc) + if err := r.CtrlClient.Get(r.ctx, key, esc); err != nil && tt.wantErr == "" { + t.Fatalf("failed to fetch ESC for status assertions: %v", err) + }pkg/controller/external_secrets/services.go (1)
42-79: Param name nit: keep naming consistent with ExternalSecretsConfig.Rename externalsecretsCreateRecon → externalSecretsConfigCreateRecon for clarity.
-func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.ExternalSecretsConfig, assetName string, resourceLabels map[string]string, externalsecretsCreateRecon bool) error { +func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.ExternalSecretsConfig, assetName string, resourceLabels map[string]string, externalSecretsConfigCreateRecon bool) error { @@ - if exists { - if externalsecretsCreateRecon { + if exists { + if externalSecretsConfigCreateRecon {api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
104-130: Duplicate test case—consolidate to one.“addInjectorAnnotations true when enabled is false” appears twice with the same expectation. Please drop one to reduce noise.
pkg/controller/external_secrets/secret_test.go (2)
181-185: Test doesn’t enable cert‑manager but claims creation is skipped.This case never toggles esc.Spec.ApplicationConfig.CertManagerConfig.Enabled to "true", and no client behavior is stubbed. Make intent explicit and stub a successful delete.
- { - name: "secret creation skipped when cert-manager config is enabled", - preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - }, - }, + { + name: "secret creation skipped when cert-manager config is enabled", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + return nil + }) + }, + esc: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ApplicationConfig = v1alpha1.ApplicationConfig{ + CertManagerConfig: &v1alpha1.CertManagerConfig{ Enabled: "true" }, + } + }, + },
224-227: Consider adding a drift test if Secret comparison switches to full object.If you adopt HasObjectChanged in production, add a test where Secret data differs and ensure update path triggers.
pkg/controller/external_secrets/controller.go (3)
327-365: Resource strings use the wrong API group order.Logs/comments reference externalsecretsconfig.openshift.operator.io; the group is operator.openshift.io (plural form for resources).
- // deployment to reflect desired state configured in `externalsecretsconfig.openshift.operator.io`. + // deployment to reflect desired state configured in `externalsecretsconfigs.operator.openshift.io`. @@ - // Fetch the externalsecretsconfig.openshift.operator.io CR + // Fetch the externalsecretsconfigs.operator.openshift.io CR @@ - r.log.V(1).Info("externalsecretsconfig.openshift.operator.io object not found, skipping reconciliation", "request", req) + r.log.V(1).Info("externalsecretsconfigs.operator.openshift.io object not found, skipping reconciliation", "request", req) @@ - return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsconfig.openshift.operator.io %q during reconciliation: %w", req.NamespacedName, err) + return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsconfigs.operator.openshift.io %q during reconciliation: %w", req.NamespacedName, err) @@ - r.log.V(1).Info("externalsecretsconfig.openshift.operator.io is marked for deletion", "namespace", req.NamespacedName) + r.log.V(1).Info("externalsecretsconfigs.operator.openshift.io is marked for deletion", "resource", req.NamespacedName) @@ - return ctrl.Result{}, fmt.Errorf("clean up failed for %q externalsecretsconfig.openshift.operator.io instance deletion: %w", req.NamespacedName, err) + return ctrl.Result{}, fmt.Errorf("clean up failed for %q externalsecretsconfigs.operator.openshift.io instance deletion: %w", req.NamespacedName, err) @@ - return ctrl.Result{}, fmt.Errorf("failed to update %q externalsecretsconfig.openshift.operator.io with finalizers: %w", req.NamespacedName, err) + return ctrl.Result{}, fmt.Errorf("failed to update %q externalsecretsconfigs.operator.openshift.io with finalizers: %w", req.NamespacedName, err)
367-379: Same group order issue for ExternalSecretsManager.Use externalsecretsmanagers.operator.openshift.io.
- // Fetch the externalsecretsmanager.openshift.operator.io CR + // Fetch the externalsecretsmanagers.operator.openshift.io CR @@ - r.log.V(1).Info("externalsecretsmanager.openshift.operator.io object not found, continuing without it") + r.log.V(1).Info("externalsecretsmanagers.operator.openshift.io object not found, continuing without it") @@ - return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsmanager.openshift.operator.io %q during reconciliation: %w", esmNamespacedName, err) + return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsmanagers.operator.openshift.io %q during reconciliation: %w", esmNamespacedName, err)
458-463: Event message uses wrong group order.Minor copy fix.
- r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "RemoveDeployment", "%s/%s externalsecretsconfig.openshift.operator.io marked for deletion, remove reference in deployment and remove all resources created for deployment", esc.GetNamespace(), esc.GetName()) + r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "RemoveDeployment", "%s/%s externalsecretsconfigs.operator.openshift.io marked for deletion, remove reference in deployment and remove all resources created for deployment", esc.GetNamespace(), esc.GetName())config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml (2)
133-138: Tighten CSV description grammar.Minor copy fixes for clarity.
- description: External Secrets Operator for Red Hat OpenShift deploys and manages - `external-secrets` application in OpenShift clusters. `external-secrets` provides - an uniformed interface to fetch secrets stored in external providers like AWS - Secrets Manager, HashiCorp Vault, Google Secrets Manager, Azure Key Vault, IBM - Cloud Secrets Manager to name a few, stores them as secrets in OpenShift. It provides - APIs to define authentication and the details of the secret to fetch. + description: External Secrets Operator for Red Hat OpenShift deploys and manages + the `external-secrets` application in OpenShift clusters. `external-secrets` provides + a uniform interface to fetch secrets stored in external providers such as AWS + Secrets Manager, HashiCorp Vault, Google Secret Manager, Azure Key Vault, and IBM + Cloud Secrets Manager, and stores them as Secrets in OpenShift. It provides + APIs to define authentication and the details of the secret to fetch.
10-10: Align createdAt with bundle CSV.Base CSV has createdAt: 2023-03-03; bundle CSV uses 2025-09-18. Consider syncing for consistency.
pkg/controller/external_secrets/validatingwebhook.go (1)
57-73: Error message phrasing and potential nil-guard.Message says “for %s external secrets”; suggest neutral wording and avoid esc.GetName() in the error path in case esc is nil in future edits.
- if err := updateValidatingWebhookAnnotation(esc, validatingWebhook); err != nil { - return nil, fmt.Errorf("failed to update validatingWebhook resource for %s external secrets: %s", esc.GetName(), err.Error()) - } + if err := updateValidatingWebhookAnnotation(esc, validatingWebhook); err != nil { + return nil, fmt.Errorf("failed to update validatingWebhook annotations: %w", err) + }test/apis/generator.go (5)
47-58: Deterministic suite order.Map iteration randomizes test order. Sort file paths before loading to stabilize runs.
var out []SuiteSpec - for path := range suiteFiles { + paths := make([]string, 0, len(suiteFiles)) + for p := range suiteFiles { + paths = append(paths, p) + } + sort.Strings(paths) + for _, path := range paths { suite, err := loadSuiteFile(path) if err != nil { return nil, fmt.Errorf("could not set up test suite: %w", err) } out = append(out, suite) }Add import:
import ( "bytes" "fmt" + "sort" "os" "path/filepath" "strings"
396-404: GenerateName ergonomics.Kubernetes commonly expects generateName prefixes to end with “-”. Add it if missing for readability.
- if useGenerateName { - u.SetGenerateName(resourceName) + if useGenerateName { + gen := resourceName + if !strings.HasSuffix(gen, "-") { + gen += "-" + } + u.SetGenerateName(gen) } else { u.SetName(resourceName) }
256-260: Comment typo.“sential” → “sentinel”.
- // Use an eventually here, so that we retry until the sential correctly applies. + // Use Eventually here so that we retry until the sentinel correctly applies.
555-585: Redundant continue.The continue after the if/break is dead code; remove for clarity.
if currCRD.Name == crdName { crdFilesToCheck = append(crdFilesToCheck, filename) break } - continue
14-15: Prefer maintained YAML lib.Switch from github.com/ghodss/yaml (archived) to sigs.k8s.io/yaml.
- "github.com/ghodss/yaml" + sigsYaml "sigs.k8s.io/yaml"- if err := yaml.Unmarshal(raw, &s); err != nil { + if err := sigsYaml.Unmarshal(raw, &s); err != nil {- if err := yaml.Unmarshal(raw, crd); err != nil { + if err := sigsYaml.Unmarshal(raw, crd); err != nil {- if err := yaml.Unmarshal(patchedData.Bytes(), crd); err != nil { + if err := sigsYaml.Unmarshal(patchedData.Bytes(), crd); err != nil {Also applies to: 431-434, 548-550
pkg/controller/external_secrets/certificate_test.go (1)
467-481: Helper reuse suggestion.Small DRY win: factor a helper to set Enabled/IssuerRef to cut repetition in table entries.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (2)
331-336: Fix bundle CSV description grammar.Same copy edits as base CSV.
- description: External Secrets Operator for Red Hat OpenShift deploys and manages - `external-secrets` application in OpenShift clusters. `external-secrets` provides - an uniformed interface to fetch secrets stored in external providers like AWS - Secrets Manager, HashiCorp Vault, Google Secrets Manager, Azure Key Vault, IBM - Cloud Secrets Manager to name a few, stores them as secrets in OpenShift. It provides - APIs to define authentication and the details of the secret to fetch. + description: External Secrets Operator for Red Hat OpenShift deploys and manages + the `external-secrets` application in OpenShift clusters. `external-secrets` provides + a uniform interface to fetch secrets stored in external providers such as AWS + Secrets Manager, HashiCorp Vault, Google Secret Manager, Azure Key Vault, and IBM + Cloud Secrets Manager, and stores them as Secrets in OpenShift. It provides + APIs to define authentication and the details of the secret to fetch.
500-523: RBAC for Proxy object (if read at runtime).If the operator reads cluster proxy config (config.openshift.io/v1, Resource=proxies), add get/list/watch to clusterPermissions. If not, ignore.
I can propose the RBAC stanza once confirmed.
pkg/controller/external_secrets/install_external_secrets.go (1)
100-100: Align log message with new API nameUse "externalsecretsconfig" for clarity.
- r.log.V(4).Info("finished reconciliation of external-secrets", "namespace", esc.GetNamespace(), "name", esc.GetName()) + r.log.V(4).Info("finished reconciliation of externalsecretsconfig", "namespace", esc.GetNamespace(), "name", esc.GetName())pkg/controller/external_secrets/certificate.go (1)
5-5: Avoid reflect; use explicit nil/field checksReflect here adds complexity and a false‑positive path. Prefer direct checks.
-import "reflect"pkg/controller/external_secrets_manager/controller.go (3)
91-96: Defensive type assertions in predicateHard type assertions can panic; guard them.
- oldObj := e.ObjectOld.(*operatorv1alpha1.ExternalSecretsConfig) - newObj := e.ObjectNew.(*operatorv1alpha1.ExternalSecretsConfig) + oldObj, ok1 := e.ObjectOld.(*operatorv1alpha1.ExternalSecretsConfig) + newObj, ok2 := e.ObjectNew.(*operatorv1alpha1.ExternalSecretsConfig) + if !ok1 || !ok2 { + return false + }
49-50: Fix finalizer comment to match resourceComment references the wrong group/resource.
- // finalizer name for external-secrets.openshift.operator.io resource. + // finalizer name for externalsecretsmanager.openshift.operator.io resource.
102-103: Optional: Map ESC updates directly to the singleton ESM requestReduces chance of accidental enqueueing with unexpected names.
Use EnqueueRequestsFromMapFunc to always enqueue NamespacedName{Name: common.ExternalSecretsManagerObjectName}.
pkg/controller/external_secrets/deployments.go (2)
247-249: Fix duplicated error field paths (“spec.affinity.affinity…”, “spec.tolerations.tolerations…”)Call sites already include the field segment; the validators append it again, duplicating path components.
- if err := validateAffinityRules(affinity, field.NewPath("spec", "affinity")); err != nil { + if err := validateAffinityRules(affinity, field.NewPath("spec")); err != nil { return err }- if err := validateTolerationsConfig(tolerations, field.NewPath("spec", "tolerations")); err != nil { + if err := validateTolerationsConfig(tolerations, field.NewPath("spec")); err != nil { return err }Then update the unit test expectations to reflect correct paths (see related comment).
Also applies to: 269-271
290-294: Align tolerations validator with caller to avoid double “tolerations” in pathIf you keep the callers as-is, remove the extra Child here. Prefer aligning with the pattern used by nodeSelector.
func validateTolerationsConfig(tolerations []corev1.Toleration, fldPath *field.Path) error { // convert corev1.Tolerations to core.Tolerations, required for validation. convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations)) - return corevalidation.ValidateTolerations(convTolerations, fldPath.Child("tolerations")).ToAggregate() + return corevalidation.ValidateTolerations(convTolerations, fldPath.Child("tolerations")).ToAggregate() }If you applied the call-site fix, change this to
fldPath.Child("tolerations")->fldPathinstead.test/apis/vars.go (1)
68-71: Fix typo in comment (“resourec” → “resource”)Small doc nit.
- // 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.pkg/controller/external_secrets/deployments_test.go (3)
414-418: Remove duplicated conditional in status image assertionNested
if tt.wantErr == ""is redundant.- if tt.wantErr == "" { - if tt.wantErr == "" { - if externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { - t.Errorf(...) - } - } - } + if tt.wantErr == "" && externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { + t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, "test-image") + }
424-722: Add tests: proxy removal and initContainers coverage
- Add a case where proxies were previously present, then ESC/ESM unset and OLM envs are absent; expect env vars removed from all containers.
- Add a case with initContainers to assert proxy env propagation/removal there too.
I can draft these test cases to align with the proposed code changes—want me to push them?
231-231: Error path field duplication is codified in expectationsTests expect
spec.tolerations.tolerations[...]andspec.affinity.affinity.... If you adopt the field-path fix in code, update these expected strings accordingly.Also applies to: 312-313
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (2)
1018-1021: Validation inconsistency for TLS secret reference fieldThe comment mentions that the TLS key pair secret is required, but the field is marked as optional without any validation to ensure it's provided when Bitwarden is enabled.
Consider adding a validation rule to ensure the secret reference is provided when Bitwarden is enabled:
x-kubernetes-validations: - message: secretRef must be provided when Bitwarden is enabled rule: self.enabled != 'true' || has(self.secretRef)
1108-1113: Consider simplifying the validation expressionThe validation expression is complex and could be simplified for better readability.
- - message: addInjectorAnnotations can only be set when enabled - is set to 'true'. - rule: 'has(self.addInjectorAnnotations) && self.addInjectorAnnotations - != ''false'' ? self.enabled != ''false'' : true' + - message: addInjectorAnnotations can only be set when enabled is set to 'true'. + rule: '!has(self.addInjectorAnnotations) || self.addInjectorAnnotations == ''false'' || self.enabled == ''true'''pkg/controller/external_secrets/utils.go (1)
102-110: Consider handling additional log levels explicitlyThe log level mapping only handles specific cases (0-2, 4-5) but doesn't handle level 3. This might cause unexpected behavior.
func getLogLevel(config operatorv1alpha1.ExternalSecretsConfigSpec) string { switch config.ApplicationConfig.LogLevel { case 0, 1, 2: return zapcore.Level(config.ApplicationConfig.LogLevel).String() + case 3: + return zapcore.WarnLevel.String() case 4, 5: return zapcore.DebugLevel.String() } return zapcore.InfoLevel.String() }docs/api_reference.md (1)
37-40: Fix bare URLs in markdown documentationThe documentation contains bare URLs that should be properly formatted as markdown links for better readability.
-| `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#resourcerequirements-v1-core)_ | resources is for defining the resource requirements.<br />Cannot be updated.<br />ref: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | | Optional: \{\} <br /> | -| `affinity` _[Affinity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#affinity-v1-core)_ | affinity is for setting scheduling affinity rules.<br />ref: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/ | | Optional: \{\} <br /> | -| `tolerations` _[Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#toleration-v1-core) array_ | tolerations is for setting the pod tolerations.<br />ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ | | MaxItems: 10 <br />MinItems: 0 <br />Optional: \{\} <br /> | -| `nodeSelector` _object (keys:string, values:string)_ | nodeSelector is for defining the scheduling criteria using node labels.<br />ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/ | | MaxProperties: 10 <br />MinProperties: 0 <br />Optional: \{\} <br /> | +| `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#resourcerequirements-v1-core)_ | resources is for defining the resource requirements.<br />Cannot be updated.<br />ref: [Managing Resources for Containers](https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/) | | Optional: \{\} <br /> | +| `affinity` _[Affinity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#affinity-v1-core)_ | affinity is for setting scheduling affinity rules.<br />ref: [Assigning Pods to Nodes](https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/) | | Optional: \{\} <br /> | +| `tolerations` _[Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#toleration-v1-core) array_ | tolerations is for setting the pod tolerations.<br />ref: [Taints and Tolerations](https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/) | | MaxItems: 10 <br />MinItems: 0 <br />Optional: \{\} <br /> | +| `nodeSelector` _object (keys:string, values:string)_ | nodeSelector is for defining the scheduling criteria using node labels.<br />ref: [Assigning Pods to Nodes](https://kubernetes.io/docs/concepts/configuration/assign-pod-node/) | | MaxProperties: 10 <br />MinProperties: 0 <br />Optional: \{\} <br /> |api/v1alpha1/external_secrets_config_types.go (1)
182-183: Consider using constants for cert-manager validationThe validation rules use hardcoded strings for 'issuer', 'clusterissuer', and 'cert-manager.io'. Consider defining these as constants for maintainability.
While the current validation works, you might want to define constants in the code for these values to ensure consistency across the codebase and make future updates easier.
pkg/controller/crd_annotator/controller.go (3)
215-235: Thread the request context; avoid r.ctx inside reconciliation pathUsing a background context bypasses cancellation/shutdown and deadlines. Pass ctx through to all client calls.
Apply this diff in this function:
-func (r *Reconciler) processReconcileRequest(esc *operatorv1alpha1.ExternalSecretsConfig, req types.NamespacedName) (ctrl.Result, error) { +func (r *Reconciler) processReconcileRequest(ctx context.Context, esc *operatorv1alpha1.ExternalSecretsConfig, req types.NamespacedName) (ctrl.Result, error) { @@ - if err := r.Get(r.ctx, req, crd); err != nil { + if err := r.Get(ctx, req, crd); err != nil {And update the call site in Reconcile:
return r.processReconcileRequest(ctx, esc, req.NamespacedName)
223-231: Preserve status updates on NotFound by avoiding early returnCurrently, a NotFound CRD short-circuits before updateCondition, so Status isn’t updated. Update the condition before returning.
Apply this diff:
- if errors.IsNotFound(err) { - r.log.V(1).Info("crd managed by OLM is not found, skipping reconciliation", "crd", req) - return ctrl.Result{}, nil - } + if errors.IsNotFound(err) { + r.log.V(1).Info("crd managed by OLM is not found, skipping reconciliation", "crd", req) + if uerr := r.updateCondition(esc, nil); uerr != nil { + return ctrl.Result{}, uerr + } + return ctrl.Result{}, nil + }
272-276: Avoid taking the address of the range loop variablePassing &crd uses the address of the loop variable; prefer indexing to take the address of the slice element.
- for _, crd := range managedCRDList.Items { - if err := r.updateAnnotations(&crd); err != nil { + for i := range managedCRDList.Items { + if err := r.updateAnnotations(&managedCRDList.Items[i]); err != nil { return fmt.Errorf("failed to update annotations in %q: %w", crd.GetName(), err) } }
📜 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 (53)
Makefile(5 hunks)PROJECT(1 hunks)README.md(1 hunks)api/v1alpha1/external_secrets_config_types.go(4 hunks)api/v1alpha1/external_secrets_manager_types.go(4 hunks)api/v1alpha1/meta.go(3 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(6 hunks)bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml(9 hunks)bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml(5 hunks)config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml(9 hunks)config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml(5 hunks)config/crd/kustomization.yaml(1 hunks)config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml(2 hunks)config/rbac/role.yaml(2 hunks)config/samples/operator_v1alpha1_externalsecrets.yaml(1 hunks)docs/api_reference.md(10 hunks)go.mod(4 hunks)hack/test-apis.sh(1 hunks)pkg/controller/common/constants.go(1 hunks)pkg/controller/common/utils.go(4 hunks)pkg/controller/commontest/utils.go(3 hunks)pkg/controller/crd_annotator/controller.go(8 hunks)pkg/controller/crd_annotator/controller_test.go(16 hunks)pkg/controller/external_secrets/certificate.go(5 hunks)pkg/controller/external_secrets/certificate_test.go(13 hunks)pkg/controller/external_secrets/constants.go(2 hunks)pkg/controller/external_secrets/controller.go(9 hunks)pkg/controller/external_secrets/deployments.go(15 hunks)pkg/controller/external_secrets/deployments_test.go(14 hunks)pkg/controller/external_secrets/install_external_secrets.go(3 hunks)pkg/controller/external_secrets/rbacs.go(11 hunks)pkg/controller/external_secrets/rbacs_test.go(6 hunks)pkg/controller/external_secrets/secret.go(3 hunks)pkg/controller/external_secrets/secret_test.go(3 hunks)pkg/controller/external_secrets/service_test.go(3 hunks)pkg/controller/external_secrets/serviceaccounts.go(4 hunks)pkg/controller/external_secrets/serviceaccounts_test.go(5 hunks)pkg/controller/external_secrets/services.go(4 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(4 hunks)pkg/controller/external_secrets_manager/controller_test.go(7 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(1 hunks)test/e2e/testdata/external_secret.yaml(1 hunks)tools/tools.go(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
test/apis/README.md
1-1: Bare URL used
(MD034, no-bare-urls)
docs/api_reference.md
37-37: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
39-39: Bare URL used
(MD034, no-bare-urls)
40-40: Bare URL used
(MD034, no-bare-urls)
98-98: Bare URL used
(MD034, no-bare-urls)
99-99: Bare URL used
(MD034, no-bare-urls)
100-100: Bare URL used
(MD034, no-bare-urls)
101-101: Bare URL used
(MD034, no-bare-urls)
364-364: Bare URL used
(MD034, no-bare-urls)
365-365: Bare URL used
(MD034, no-bare-urls)
366-366: Bare URL used
(MD034, no-bare-urls)
367-367: Bare URL used
(MD034, no-bare-urls)
🪛 checkmake (0.2.2)
Makefile
[warning] 395-395: Missing required phony target "clean"
(minphony)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
Outdated
Show resolved
Hide resolved
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml
Outdated
Show resolved
Hide resolved
config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
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/deployments.go (1)
117-121: Do not require Bitwarden image unless reconciling Bitwarden deployment.Today, missing RELATED_IMAGE_BITWARDEN_SDK_SERVER fails all deployment reconciliations even if Bitwarden is disabled.
- bitwardenImage := os.Getenv(bitwardenImageEnvVarName) - if bitwardenImage == "" { - return nil, common.NewIrrecoverableError(fmt.Errorf("%s environment variable with bitwarden-sdk-server image not set", bitwardenImageEnvVarName), "failed to update image in %s deployment object", deployment.GetName()) - } @@ case bitwardenDeploymentAssetName: - deployment.Labels["app.kubernetes.io/version"] = bitwardenImageVersionEnvVarName - updateBitwardenServerContainerSpec(deployment, bitwardenImage) + bitwardenImage := os.Getenv(bitwardenImageEnvVarName) + if bitwardenImage == "" { + return nil, common.NewIrrecoverableError(fmt.Errorf("%s environment variable with bitwarden-sdk-server image not set", bitwardenImageEnvVarName), "failed to update image in %s deployment object", deployment.GetName()) + } + deployment.Labels["app.kubernetes.io/version"] = bitwardenImageVersionEnvVarName + updateBitwardenServerContainerSpec(deployment, bitwardenImage)Also applies to: 129-133
🧹 Nitpick comments (11)
pkg/controller/external_secrets/configmap.go (2)
46-47: Use reconciler context instead of context.TODO().Leverage r.ctx for cancellation, tracing, and consistency with the rest of the controller.
- err := r.Get(context.TODO(), types.NamespacedName{Name: trustedCABundleConfigMapName, Namespace: namespace}, existingConfigMap) + err := r.Get(r.ctx, types.NamespacedName{Name: trustedCABundleConfigMapName, Namespace: namespace}, existingConfigMap) @@ - if err := r.Create(context.TODO(), configMap); err != nil { + if err := r.Create(r.ctx, configMap); err != nil { @@ - if err := r.Update(context.TODO(), existingConfigMap); err != nil { + if err := r.Update(r.ctx, existingConfigMap); err != nil {Also applies to: 51-52, 76-77
65-72: Minor: avoid recomputing expectedLabels.You already compute expectedLabels at Line 30. Reuse it to reduce noise.
- expectedLabels = getTrustedCABundleLabels(resourceLabels) needsUpdate := false for k, expectedValue := range expectedLabels {pkg/controller/external_secrets/install_external_secrets.go (1)
111-127: createOrApplyNamespace doesn't “apply” when Namespace exists.If the Namespace pre-exists, labels are never reconciled. Apply label changes when AlreadyExists.
func (r *Reconciler) createOrApplyNamespace(esc *operatorv1alpha1.ExternalSecretsConfig, resourceLabels map[string]string) error { namespace := getNamespace(esc) obj := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: namespace, Labels: resourceLabels, }, } if err := r.Create(r.ctx, obj); err != nil { if errors.IsAlreadyExists(err) { - r.log.V(4).Info("namespace already exists", "namespace", namespace) - return nil + // Fetch and patch labels if they differ + existing := &corev1.Namespace{} + if getErr := r.Get(r.ctx, types.NamespacedName{Name: namespace}, existing); getErr != nil { + return getErr + } + if !reflect.DeepEqual(existing.Labels, resourceLabels) { + if existing.Labels == nil { + existing.Labels = map[string]string{} + } + for k, v := range resourceLabels { + existing.Labels[k] = v + } + if updErr := r.Update(r.ctx, existing); updErr != nil { + return updErr + } + } + return nil } return err } return nil }Note: add imports if missing:
- "reflect"
- "k8s.io/apimachinery/pkg/types"
pkg/controller/external_secrets/controller.go (2)
225-228: Fix wrong object in error message.Uses ownObject in the esm informer error; should log esmObject.
- if err != nil { - return nil, fmt.Errorf("failed to add informer for %s resource: %w", ownObject.GetObjectKind().GroupVersionKind().String(), err) - } + if err != nil { + return nil, fmt.Errorf("failed to add informer for %s resource: %w", esmObject.GetObjectKind().GroupVersionKind().String(), err) + }
270-277: Tighten ConfigMap predicate to avoid cross‑namespace noise.Name-only match can enqueue on similarly named CMs in other namespaces. Prefer filtering by the injection label you control.
- // predicate function for ConfigMaps to include both managed resources and trusted CA bundle ConfigMap - configMapPredicate := predicate.NewPredicateFuncs(func(object client.Object) bool { - if object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue { - return true - } - return object.GetName() == trustedCABundleConfigMapName - }) + // Predicate for ConfigMaps: include controller-managed resources and any CM with the injection label + configMapPredicate := predicate.NewPredicateFuncs(func(object client.Object) bool { + labels := object.GetLabels() + if labels != nil && labels[requestEnqueueLabelKey] == requestEnqueueLabelValue { + return true + } + return labels != nil && labels[trustedCABundleInjectLabel] == "true" + })pkg/controller/external_secrets/deployments_test.go (2)
425-759: Add coverage: initContainers, lowercase envs, and removal of stale proxy vars.Current tests validate only containers and uppercase envs. Please add cases to assert:
- initContainers receive the same proxy env and mounts
- lowercase http_proxy/https_proxy/no_proxy are set
- when no proxy is configured from ESC/ESM/OLM, any existing proxy envs are removed
761-774: Make env var comparison order-insensitive.Container.Env ordering is not guaranteed. Sort by Name before comparing to reduce test flakiness.
func validateEnvironmentVariables(t *testing.T, deployment *appsv1.Deployment, expectedContainerEnvVars map[string][]corev1.EnvVar) { for containerName, expectedEnvVars := range expectedContainerEnvVars { container := findContainer(deployment, containerName) if container == nil { t.Errorf("Container %s not found in deployment", containerName) return } - if !reflect.DeepEqual(container.Env, expectedEnvVars) { + sort.Slice(container.Env, func(i, j int) bool { return container.Env[i].Name < container.Env[j].Name }) + sort.Slice(expectedEnvVars, func(i, j int) bool { return expectedEnvVars[i].Name < expectedEnvVars[j].Name }) + if !reflect.DeepEqual(container.Env, expectedEnvVars) { t.Errorf("Container %s environment variables mismatch.\nExpected: %+v\nActual: %+v", containerName, expectedEnvVars, container.Env) } } }Note: add
import "sort"at top.pkg/controller/external_secrets/deployments.go (3)
185-187: Guard against nil r.esm in fallbacks.Avoid potential NPEs by checking r.esm != nil before dereferencing in resource/affinity/toleration/nodeSelector fallbacks.
- } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Resources != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Resources != nil { @@ - } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.NodeSelector != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.NodeSelector != nil { @@ - } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Affinity != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Affinity != nil { @@ - } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Tolerations != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Tolerations != nil {Also applies to: 217-219, 239-241, 262-263
411-424: Proxies: support removal, initContainers, and lowercase envs.
- Remove stale proxy envs when no proxy config resolves from ESC/ESM/OLM
- Apply to initContainers as well
- Set lowercase variants for broader image compatibility
func (r *Reconciler) updateProxyEnvironmentVariables(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) error { proxyConfig := r.getProxyConfiguration(esc) - if proxyConfig == nil { - return nil - } - - // Apply proxy environment variables to all containers - for i := range deployment.Spec.Template.Spec.Containers { - container := &deployment.Spec.Template.Spec.Containers[i] - r.setProxyEnvVars(container, proxyConfig) - } + // Helper to process both containers and initContainers + process := func(envSetter func(c *corev1.Container)) { + for i := range deployment.Spec.Template.Spec.Containers { + envSetter(&deployment.Spec.Template.Spec.Containers[i]) + } + for i := range deployment.Spec.Template.Spec.InitContainers { + envSetter(&deployment.Spec.Template.Spec.InitContainers[i]) + } + } + if proxyConfig == nil { + process(func(c *corev1.Container) { r.removeProxyEnvVars(c) }) + return nil + } + process(func(c *corev1.Container) { r.setProxyEnvVars(c, proxyConfig) }) return nil } @@ // Set proxy environment variables setEnvVar("HTTP_PROXY", proxyConfig.HTTPProxy) setEnvVar("HTTPS_PROXY", proxyConfig.HTTPSProxy) setEnvVar("NO_PROXY", proxyConfig.NoProxy) + // Lowercase variants + setEnvVar("http_proxy", proxyConfig.HTTPProxy) + setEnvVar("https_proxy", proxyConfig.HTTPSProxy) + setEnvVar("no_proxy", proxyConfig.NoProxy)Add helper outside the shown ranges:
// removeProxyEnvVars deletes all proxy-related env vars from a container. func (r *Reconciler) removeProxyEnvVars(container *corev1.Container) { if len(container.Env) == 0 { return } toDrop := map[string]struct{}{ "HTTP_PROXY": {}, "HTTPS_PROXY": {}, "NO_PROXY": {}, "http_proxy": {}, "https_proxy": {}, "no_proxy": {}, } dst := container.Env[:0] for _, e := range container.Env { if _, drop := toDrop[e.Name]; !drop { dst = append(dst, e) } } container.Env = dst }Also applies to: 426-459
509-524: Mount trusted CA bundle in initContainers too.Parity with main containers ensures any init work honors the proxy CA.
- for i := range deployment.Spec.Template.Spec.Containers { + for i := range deployment.Spec.Template.Spec.Containers { container := &deployment.Spec.Template.Spec.Containers[i] @@ } + + // Add volume mounts to all init containers + for i := range deployment.Spec.Template.Spec.InitContainers { + container := &deployment.Spec.Template.Spec.InitContainers[i] + volumeMountExists := false + for j, volumeMount := range container.VolumeMounts { + if volumeMount.Name == trustedCABundleVolumeName { + container.VolumeMounts[j] = trustedCAVolumeMount + volumeMountExists = true + break + } + } + if !volumeMountExists { + container.VolumeMounts = append(container.VolumeMounts, trustedCAVolumeMount) + } + }pkg/controller/external_secrets/constants.go (1)
52-66: Add derived file path constant for the injected CA bundleConfirmed usage of ConfigMapName, InjectLabel, VolumeName, MountPath and KeyName; no direct full-path references exist—derive once instead of recomputing.
// trustedCABundleKeyName is the key in the ConfigMap that contains the CA bundle trustedCABundleKeyName = "ca-bundle.crt" + // trustedCABundleFilePath is the full path to the injected CA bundle file + trustedCABundleFilePath = trustedCABundleMountPath + "/" + trustedCABundleKeyName
📜 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 (7)
pkg/controller/external_secrets/configmap.go(1 hunks)pkg/controller/external_secrets/constants.go(2 hunks)pkg/controller/external_secrets/controller.go(9 hunks)pkg/controller/external_secrets/deployments.go(15 hunks)pkg/controller/external_secrets/deployments_test.go(15 hunks)pkg/controller/external_secrets/install_external_secrets.go(3 hunks)pkg/controller/external_secrets/utils.go(4 hunks)
🔇 Additional comments (6)
pkg/controller/external_secrets/deployments.go (1)
410-415: Guard getProxyConfiguration against nil r.esm.If not already fixed, ensure r.esm is checked inside getProxyConfiguration to avoid panics. This was flagged earlier.
Additional snippet (outside range):
func (r *Reconciler) getProxyConfiguration(esc *operatorv1alpha1.ExternalSecretsConfig) *operatorv1alpha1.ProxyConfig { // ESC overrides if esc != nil && esc.Spec.ApplicationConfig.CommonConfigs.Proxy != nil { return esc.Spec.ApplicationConfig.CommonConfigs.Proxy } // ESM fallback if r != nil && r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.CommonConfigs.Proxy != nil { return r.esm.Spec.GlobalConfig.CommonConfigs.Proxy } // OLM env fallback... // ... return nil }pkg/controller/external_secrets/install_external_secrets.go (1)
55-57: Don't swallow namespace reconcile errors.Propagate the error so the loop can requeue and status can reflect failure. This was flagged earlier.
if err := r.createOrApplyNamespace(esc, resourceLabels); err != nil { - r.log.Error(err, "failed to create namespace") + r.log.Error(err, "failed to reconcile namespace resource") + return err }pkg/controller/external_secrets/controller.go (2)
429-433: Status updates short‑circuited; Ready may not update.Evaluate both SetStatusCondition calls, then update once. Same issue was previously noted.
- if apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) || - apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) { + changed := apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) + changed = apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) || changed + if changed { errUpdate = r.updateCondition(esc, err) err = utilerrors.NewAggregate([]error{err, errUpdate}) }
456-459: Same short‑circuit bug on success path.- if apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) || - apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) { - errUpdate = r.updateCondition(esc, nil) - } + changed := apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) + changed = apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) || changed + if changed { + errUpdate = r.updateCondition(esc, nil) + }pkg/controller/external_secrets/constants.go (2)
48-51: LGTM: default namespace comment clarified.Comment correctly reflects ExternalSecretsConfig.Spec.Namespace fallback.
19-21: Fix finalizer domain and keep a legacy finalizer for upgrade paths.The string uses openshift.operator.io (reversed). Use operator.openshift.io and add a legacy constant so existing CRs don’t get stranded on upgrade. Also update the stale comment.
Apply:
- // finalizer name for external-secrets.openshift.operator.io resource. - finalizer = "externalsecretsconfig.openshift.operator.io/" + ControllerName + // finalizer name for ExternalSecretsConfig resources. + // NOTE: keep legacy finalizer for upgrade cleanup of existing CRs. + finalizer = "externalsecretsconfig.operator.openshift.io/" + ControllerName + legacyFinalizer = "externalsecrets.openshift.operator.io/" + ControllerNameRun to find and update add/remove checks to handle both finalizers (prefer new; remove both):
#!/bin/bash # Find finalizer string literals and controller-runtime helpers usage rg -nC2 -P 'externalsecrets.*openshift\.operator\.io/|-finalizer|-Finalizer|controllerutil\.(AddFinalizer|RemoveFinalizer|ContainsFinalizer)\('
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (2)
331-337: Fix user‑facing grammar and clarity in description
- “an uniformed interface” → “a unified interface”
- Remove double space before “AWS”
- Clarify that secrets are stored as Kubernetes Secrets.
Apply:
- description: External Secrets Operator for Red Hat OpenShift deploys and manages - `external-secrets` application in OpenShift clusters. `external-secrets` provides - an uniformed interface to fetch secrets stored in external providers like AWS - Secrets Manager, HashiCorp Vault, Google Secrets Manager, Azure Key Vault, IBM - Cloud Secrets Manager to name a few, stores them as secrets in OpenShift. It provides - APIs to define authentication and the details of the secret to fetch. + description: External Secrets Operator for Red Hat OpenShift deploys and manages + the `external-secrets` application in OpenShift clusters. `external-secrets` provides + a unified interface to fetch secrets stored in external providers like AWS + Secrets Manager, HashiCorp Vault, Google Secrets Manager, Azure Key Vault, and IBM + Cloud Secrets Manager, and stores them as Kubernetes Secrets in OpenShift. It provides + APIs to define authentication and the details of the secret to fetch.
500-523: Add patch verb to externalsecretsconfigs ClusterRole
Controller patches ExternalSecretsConfig (finalizers); addpatchto the main resource’s verbs. No patch needed on the status subresource.--- bundle/manifests/external-secrets-operator.clusterserviceversion.yaml @@ -502,7 +502,8 @@ - operator.openshift.io resources: - externalsecretsconfigs - verbs: - - create - - get - - list - - update - - watch + verbs: + - create + - get + - list + - patch + - update + - watch
🧹 Nitpick comments (4)
config/manager/trusted-ca-patch.yaml (2)
11-15: Explicitly set SSL_CERT_FILE for portability.To ensure all base images/apps pick up the injected CA, set SSL_CERT_FILE to the mounted bundle.
Apply this diff:
- name: manager + env: + - name: SSL_CERT_FILE + value: /etc/pki/tls/certs/ca-bundle.crt volumeMounts: - name: trusted-ca-bundle mountPath: /etc/pki/tls/certs readOnly: truePlease confirm the controller image base (UBI/distroless/etc.). If it’s not UBI/RHEL‑like, we may need SSL_CERT_DIR instead or a different path.
10-14: Harden container security context (addresses Checkov hints).You can set restrictive defaults here without affecting functionality.
Apply this diff:
- name: manager + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsNonRoot: true + seccompProfile: + type: RuntimeDefaultbundle/manifests/external-secrets-operator.clusterserviceversion.yaml (2)
179-189: alm-example: ExternalSecretsConfig entry looks correctKind/name align with singleton pattern; empty spec is fine for a minimal example.
Consider showcasing a minimal ProxyConfig in the example to demonstrate HTTP(S) proxy usage.
668-685: Mount CA bundle via subPath to avoid shadowing system certs
Use subPath to mount only ca-bundle.crt instead of the entire certs directory. The ConfigMap external-secrets-operator-trusted-ca-bundle (with config.openshift.io/inject-trusted-cabundle: "true") is present.- volumeMounts: - - mountPath: /etc/pki/tls/certs - name: trusted-ca-bundle - readOnly: true + volumeMounts: + - mountPath: /etc/pki/tls/certs/ca-bundle.crt + name: trusted-ca-bundle + subPath: ca-bundle.crt + readOnly: true
📜 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)
bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml(1 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(7 hunks)config/manager/kustomization.yaml(1 hunks)config/manager/trusted-ca-patch.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/manager/kustomization.yaml
- bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
config/manager/trusted-ca-patch.yaml
[medium] 1-21: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-21: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (2)
config/manager/trusted-ca-patch.yaml (1)
16-21: LGTM on volume wiring and key mapping.Mounting ca-bundle.crt read-only at /etc/pki/tls/certs is correct for OpenShift’s injected bundle.
If a kustomize ConfigMapGenerator is also used for this bundle, ensure there is no duplication and either:
- use the static, annotated CM (no nameSuffixHash), or
- rely solely on the generator and verify kustomize rewrites the name reference in volumes.configMap.name.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
267-276: CRD ownership/description rename to ExternalSecretsConfig is consistentKind, name, and singleton wording align with the API rebrand.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/controller/external_secrets/deployments.go (5)
113-120: Bitwarden image required unconditionally and version label bug
- getDeploymentObject fails for non-bitwarden assets when BITWARDEN image env is unset.
- The version label is set to the env var name constant instead of its value.
Scope the bitwarden image requirement to the bitwarden asset and set the label to the actual version value.
Apply this diff:
image := os.Getenv(externalsecretsImageEnvVarName) if image == "" { return nil, common.NewIrrecoverableError(fmt.Errorf("%s environment variable with externalsecrets image not set", externalsecretsImageEnvVarName), "failed to update image in %s deployment object", deployment.GetName()) } - bitwardenImage := os.Getenv(bitwardenImageEnvVarName) - if bitwardenImage == "" { - return nil, common.NewIrrecoverableError(fmt.Errorf("%s environment variable with bitwarden-sdk-server image not set", bitwardenImageEnvVarName), "failed to update image in %s deployment object", deployment.GetName()) - } logLevel := getLogLevel(esc.Spec) switch assetName { case controllerDeploymentAssetName: updateContainerSpec(deployment, esc, image, logLevel) case webhookDeploymentAssetName: updateWebhookContainerSpec(deployment, image, logLevel) case certControllerDeploymentAssetName: updateCertControllerContainerSpec(deployment, image, logLevel) case bitwardenDeploymentAssetName: - deployment.Labels["app.kubernetes.io/version"] = bitwardenImageVersionEnvVarName - updateBitwardenServerContainerSpec(deployment, bitwardenImage) + // Bitwarden image/env are only required for this asset. + bitwardenImage := os.Getenv(bitwardenImageEnvVarName) + if bitwardenImage == "" { + return nil, common.NewIrrecoverableError(fmt.Errorf("%s environment variable with bitwarden-sdk-server image not set", bitwardenImageEnvVarName), "failed to update image in %s deployment object", deployment.GetName()) + } + if deployment.Labels == nil { + deployment.Labels = map[string]string{} + } + if v := os.Getenv(bitwardenImageVersionEnvVarName); v != "" { + deployment.Labels["app.kubernetes.io/version"] = v + } + updateBitwardenServerContainerSpec(deployment, bitwardenImage) }Also applies to: 124-133
180-202: Nil deref risk: r.esm checks missingAccesses r.esm.Spec.GlobalConfig without guarding r.esm != nil; can panic.
Apply this diff:
func (r *Reconciler) updateResourceRequirement(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) error { rscReqs := corev1.ResourceRequirements{} if esc.Spec.ApplicationConfig.Resources != nil { esc.Spec.ApplicationConfig.Resources.DeepCopyInto(&rscReqs) - } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Resources != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Resources != nil { r.esm.Spec.GlobalConfig.Resources.DeepCopyInto(&rscReqs) } else { return nil }
211-231: Nil deref risk: r.esm checks missing (node selector)Same issue here.
Apply this diff:
- } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.NodeSelector != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.NodeSelector != nil { nodeSelector = r.esm.Spec.GlobalConfig.NodeSelector }
233-253: Nil deref risk: r.esm checks missing (affinity)Same issue here.
Apply this diff:
- } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Affinity != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Affinity != nil { affinity = r.esm.Spec.GlobalConfig.Affinity }
255-275: Nil deref risk: r.esm checks missing (tolerations)Same issue here.
Apply this diff:
- } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Tolerations != nil { + } else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Tolerations != nil { tolerations = r.esm.Spec.GlobalConfig.Tolerations }
🧹 Nitpick comments (5)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
331-336: Minor wording nit in description“an uniformed interface” → “a unified interface”.
Apply this diff:
- an uniformed interface to fetch secrets stored in external providers like AWS + a unified interface to fetch secrets stored in external providers like AWSpkg/controller/external_secrets/deployments_test.go (3)
755-768: Tests compare env vars by order; consider order-insensitive comparisonDeepEqual requires exact ordering. Consider comparing by map keyed by Name to reduce brittleness.
770-787: Volumes check is strict; consider contains-based assertionDeepEqual on full slice fails if unrelated volumes are present. Prefer asserting the presence and equality of expected volumes.
789-817: VolumeMounts check is strict; consider contains-based assertion per containerFilter is good; still DeepEqual enforces order. Consider sort-by-name or contains check.
pkg/controller/external_secrets/deployments.go (1)
461-522: Trusted CA bundle: make optional and mount for initContainers tooAvoid startup failures if the ConfigMap is not yet present; ensure initContainers also trust the CA.
Apply this diff:
trustedCAVolume := corev1.Volume{ Name: trustedCABundleVolumeName, VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ Name: trustedCABundleConfigMapName, }, + Optional: ptr.To(true), }, }, } @@ for i := range deployment.Spec.Template.Spec.Containers { container := &deployment.Spec.Template.Spec.Containers[i] @@ if !volumeMountExists { container.VolumeMounts = append(container.VolumeMounts, trustedCAVolumeMount) } } + + // Also mount in initContainers + for i := range deployment.Spec.Template.Spec.InitContainers { + c := &deployment.Spec.Template.Spec.InitContainers[i] + volumeMountExists := false + for j, vm := range c.VolumeMounts { + if vm.Name == trustedCABundleVolumeName { + c.VolumeMounts[j] = trustedCAVolumeMount + volumeMountExists = true + break + } + } + if !volumeMountExists { + c.VolumeMounts = append(c.VolumeMounts, trustedCAVolumeMount) + } + }
📜 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)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(7 hunks)config/manager/trusted-ca-patch.yaml(1 hunks)pkg/controller/external_secrets/deployments.go(15 hunks)pkg/controller/external_secrets/deployments_test.go(15 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
config/manager/trusted-ca-patch.yaml
[medium] 1-18: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-18: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (7)
config/manager/trusted-ca-patch.yaml (3)
5-5: Remove hardcoded namespace from the patch.Delete Line 5 to keep the patch namespace-agnostic for overlays.
Apply this diff:
metadata: name: controller-manager - namespace: system
16-18: Align ConfigMap name with injected bundle and make it optional.Use the actual injected CM name and avoid startup failures if it’s not present yet.
Apply this diff:
volumes: - name: trusted-ca-bundle configMap: - name: trusted-ca-bundle + name: external-secrets-operator-trusted-ca-bundle + optional: true + items: + - key: ca-bundle.crt + path: ca-bundle.crt
1-18: Verify base image CA path assumptions. No occurrences of SSL_CERT_FILE, REQUESTS_CA_BUNDLE, or alternate CA bundle filenames were found; confirm the controller image reads CAs from /etc/pki/tls/certs/ca-bundle.crt or adjust the mountPath or set SSL_CERT_FILE/REQUESTS_CA_BUNDLE to match its CA path.bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (3)
213-213: Proxy-aware annotation: goodEnables OLM to inject HTTP(S)_PROXY/NO_PROXY; aligns with the PR goal.
637-637: Pin operator image (avoid :latest) in DeploymentThe install spec still uses :latest; pin to a digest to match supply‑chain best practices.
Apply this diff (replace with the real digest/tag):
- image: openshift.io/external-secrets-operator:latest + image: openshift.io/external-secrets-operator@sha256:<digest>
206-206: Pin operator image (avoid :latest) in CSV metadataUsing a mutable tag harms reproducibility and compliance. Pin to an immutable digest.
Apply this diff (replace with the real digest/tag):
- containerImage: openshift.io/external-secrets-operator:latest + containerImage: openshift.io/external-secrets-operator@sha256:<digest>pkg/controller/external_secrets/deployments.go (1)
410-424: Proxy envs: add lowercase variants and cover initContainersMany images honor lowercase envs; also initContainers need the same proxy envs. Keep “no change” behavior when no proxy is resolved to avoid breaking existing tests.
Apply this diff:
func (r *Reconciler) updateProxyEnvironmentVariables(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) error { proxyConfig := r.getProxyConfiguration(esc) if proxyConfig == nil { return nil } - // Apply proxy environment variables to all containers - for i := range deployment.Spec.Template.Spec.Containers { - container := &deployment.Spec.Template.Spec.Containers[i] - r.setProxyEnvVars(container, proxyConfig) - } + // Apply proxy environment variables to all containers and initContainers + for i := range deployment.Spec.Template.Spec.Containers { + r.setProxyEnvVars(&deployment.Spec.Template.Spec.Containers[i], proxyConfig) + } + for i := range deployment.Spec.Template.Spec.InitContainers { + r.setProxyEnvVars(&deployment.Spec.Template.Spec.InitContainers[i], proxyConfig) + } return nil } @@ // Set proxy environment variables setEnvVar("HTTP_PROXY", proxyConfig.HTTPProxy) setEnvVar("HTTPS_PROXY", proxyConfig.HTTPSProxy) setEnvVar("NO_PROXY", proxyConfig.NoProxy) + // Lowercase variants for broader compatibility + setEnvVar("http_proxy", proxyConfig.HTTPProxy) + setEnvVar("https_proxy", proxyConfig.HTTPSProxy) + setEnvVar("no_proxy", proxyConfig.NoProxy) }Also applies to: 426-459
|
|
||
| // setProxyEnvVars sets proxy environment variables on a container. | ||
| func (r *Reconciler) setProxyEnvVars(container *corev1.Container, proxyConfig *operatorv1alpha1.ProxyConfig) { | ||
| if proxyConfig == nil { |
There was a problem hiding this comment.
We should for proxy ENV vars in operator pod set by OLM, when cluster-wide egress proxy is configured.
There was a problem hiding this comment.
Sorry, I could not understand it correctly.
We are doing the necessary checks in getProxyConfiguration function and feeding the value in proxyConfig before callingsetProxyEnvVars.
If proxyConfig is non-nil value here, then we will set the env vars in the operand pods.
In case we need any modification to this logic, let me know.
There was a problem hiding this comment.
My suggestion was to handle this case.
There was a problem hiding this comment.
Okay, yeah we are already considering OLM Env vars when cluster-wide proxy is enabled.
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
config/manager/trusted-ca-patch.yaml (3)
5-5: Remove hardcoded namespace to support kustomize overlays.The hardcoded
namespace: systemwill cause the patch to miss the target Deployment when overlays change the namespace. Remove this line to make the patch namespace-agnostic.Apply this diff:
metadata: name: controller-manager - namespace: system
12-14: Mounting entire directory masks system CA certificates.Mounting the entire
/etc/pki/tls/certsdirectory will hide system CA certificates, breaking TLS trust for connections that rely on system CAs. UsesubPathto mount only theca-bundle.crtfile.Apply this diff:
volumeMounts: - name: trusted-ca-bundle - mountPath: /etc/pki/tls/certs + mountPath: /etc/pki/tls/certs/ca-bundle.crt + subPath: ca-bundle.crt readOnly: true
18-18: ConfigMap name mismatch.The volume references ConfigMap
"trusted-ca-bundle", but the actual ConfigMap created is named"external-secrets-operator-trusted-ca-bundle"(as seen in the manifest and constants). This will cause the volume to mount an empty/non-existent ConfigMap at runtime.Apply this diff:
configMap: - name: trusted-ca-bundle + name: external-secrets-operator-trusted-ca-bundlepkg/controller/external_secrets/utils.go (1)
127-155: Guard against nil ApplicationConfig to prevent panic.Line 133 dereferences
esc.Spec.ApplicationConfigwithout checking for nil, which will panic for CRs that omitApplicationConfigand rely solely on ESM or OLM defaults.Apply this diff:
func (r *Reconciler) getProxyConfiguration(esc *operatorv1alpha1.ExternalSecretsConfig) *operatorv1alpha1.ProxyConfig { var proxyConfig *operatorv1alpha1.ProxyConfig // Check ExternalSecretsConfig first - if esc.Spec.ApplicationConfig.Proxy != nil { // TODO: check if esc.Spec.ApplicationConfig != nil is required + if esc.Spec.ApplicationConfig != nil && esc.Spec.ApplicationConfig.Proxy != nil { proxyConfig = esc.Spec.ApplicationConfig.Proxy } else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Proxy != nil {pkg/controller/external_secrets/deployments.go (1)
413-460: Proxy env vars still linger and miss init containersWe still can’t turn the proxy off once it was set: when
proxyConfigbecomes nil (or a field goes empty), the old environment variables stay on the pod. We also never touch initContainers and only export upper-case keys, so several common images keep using stale settings. Please fold in cleanup + init container coverage and set both casings.func (r *Reconciler) updateProxyEnvironmentVariables(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) error { - proxyConfig := r.getProxyConfiguration(esc) - if proxyConfig == nil { - return nil - } - - // Apply proxy environment variables to all containers - for i := range deployment.Spec.Template.Spec.Containers { - container := &deployment.Spec.Template.Spec.Containers[i] - r.setProxyEnvVars(container, proxyConfig) - } + proxyConfig := r.getProxyConfiguration(esc) + + process := func(envSetter func(*corev1.Container)) { + for i := range deployment.Spec.Template.Spec.Containers { + envSetter(&deployment.Spec.Template.Spec.Containers[i]) + } + for i := range deployment.Spec.Template.Spec.InitContainers { + envSetter(&deployment.Spec.Template.Spec.InitContainers[i]) + } + } + if proxyConfig == nil { + process(r.removeProxyEnvVars) + return nil + } + + process(func(c *corev1.Container) { r.setProxyEnvVars(c, proxyConfig) }) return nil } func (r *Reconciler) setProxyEnvVars(container *corev1.Container, proxyConfig *operatorv1alpha1.ProxyConfig) { @@ if container.Env == nil { container.Env = []corev1.EnvVar{} } + removeEnvVar := func(name string) { + for i := 0; i < len(container.Env); i++ { + if container.Env[i].Name == name { + container.Env = append(container.Env[:i], container.Env[i+1:]...) + i-- + } + } + } setEnvVar := func(name, value string) { if value == "" { - return + removeEnvVar(name) + return } @@ // Set proxy environment variables setEnvVar("HTTP_PROXY", proxyConfig.HTTPProxy) setEnvVar("HTTPS_PROXY", proxyConfig.HTTPSProxy) setEnvVar("NO_PROXY", proxyConfig.NoProxy) + setEnvVar("http_proxy", proxyConfig.HTTPProxy) + setEnvVar("https_proxy", proxyConfig.HTTPSProxy) + setEnvVar("no_proxy", proxyConfig.NoProxy) } + +// removeProxyEnvVars deletes all proxy-related env vars from a container. +func (r *Reconciler) removeProxyEnvVars(container *corev1.Container) { + if len(container.Env) == 0 { + return + } + toDrop := map[string]struct{}{ + "HTTP_PROXY": {}, "HTTPS_PROXY": {}, "NO_PROXY": {}, + "http_proxy": {}, "https_proxy": {}, "no_proxy": {}, + } + dst := container.Env[:0] + for _, env := range container.Env { + if _, drop := toDrop[env.Name]; drop { + continue + } + dst = append(dst, env) + } + container.Env = dst +}Please refresh the accompanying tests as well.
📜 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 (13)
bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml(1 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(3 hunks)config/default/manager_metrics_patch.yaml(0 hunks)config/manager/kustomization.yaml(1 hunks)config/manager/trusted-ca-patch.yaml(1 hunks)config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml(1 hunks)pkg/controller/external_secrets/configmap.go(1 hunks)pkg/controller/external_secrets/constants.go(1 hunks)pkg/controller/external_secrets/controller.go(4 hunks)pkg/controller/external_secrets/deployments.go(2 hunks)pkg/controller/external_secrets/deployments_test.go(2 hunks)pkg/controller/external_secrets/install_external_secrets.go(1 hunks)pkg/controller/external_secrets/utils.go(2 hunks)
💤 Files with no reviewable changes (1)
- config/default/manager_metrics_patch.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/external_secrets/configmap.go
- config/manager/kustomization.yaml
- bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
config/manager/trusted-ca-patch.yaml
[medium] 1-18: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-18: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (11)
config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml (1)
16-16: LGTM! Proxy-aware mode enabled.The annotation change correctly enables proxy-aware behavior for the operator, aligning with the PR objectives.
pkg/controller/external_secrets/constants.go (1)
52-65: LGTM! Constants are well-defined.The new trusted CA bundle constants follow naming conventions and use appropriate values for OpenShift CA injection.
bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml (1)
1-9: LGTM! ConfigMap correctly configured for CA injection.The ConfigMap manifest is properly structured with the
config.openshift.io/inject-trusted-cabundle: "true"label that triggers OpenShift's CNO to inject cluster-wide CA certificates.pkg/controller/external_secrets/install_external_secrets.go (1)
85-88: LGTM! CA bundle ConfigMap ensured before deployments.The call to
ensureTrustedCABundleConfigMapis correctly placed beforecreateOrApplyDeployments, ensuring the CA bundle ConfigMap exists when deployments need to mount it. Error handling is consistent with other reconciliation steps.pkg/controller/external_secrets/deployments_test.go (2)
424-752: Test coverage looks comprehensive.The test function covers important scenarios including proxy precedence, partial configurations, updates to existing environment variables, and no-change cases. The test structure is clear and maintainable.
754-837: Helper functions are well-designed.The validation helper functions (
validateEnvironmentVariables,validateVolumes,validateVolumeMounts,findContainer,filterTrustedCAMounts) provide clear separation of concerns and make the tests more readable.pkg/controller/external_secrets/controller.go (5)
77-77: LGTM! ConfigMap added to managed resources.Adding
&corev1.ConfigMap{}tocontrollerManagedResourcesenables the controller to watch ConfigMap resources, which is necessary for trusted CA bundle management.
255-262: LGTM! Simplified mapFunc aligns with predicate-based filtering.The simplified
mapFuncthat always enqueues reconciliation is correct because the predicate now handles all filtering logic. Objects that reachmapFunchave already been filtered by the appropriate predicates.
270-280: LGTM! ConfigMap predicate correctly handles both managed and CA bundle ConfigMaps.The
configMapPredicateproperly filters ConfigMaps to include:
- Managed resources (with the standard label)
- The trusted CA bundle ConfigMap (by name)
This allows the controller to reconcile when the trusted CA bundle ConfigMap is created or updated, even if it doesn't have the managed resource label.
296-297: LGTM! ConfigMap watch uses the correct predicate.The ConfigMap watch correctly uses
configMapResourcePredicateinstead ofmanagedResourcePredicate, enabling reconciliation for both managed ConfigMaps and the trusted CA bundle ConfigMap.
467-467: TODO: Consider implementing CA bundle ConfigMap cleanup.The TODO comment correctly identifies that cleanup logic for the trusted CA bundle ConfigMap should be added to the
cleanUpfunction. When the ExternalSecretsConfig is deleted, the associated CA bundle ConfigMap should also be removed if no longer needed.Consider implementing the cleanup logic to remove the trusted CA bundle ConfigMap when appropriate, or open a follow-up issue to track this work.
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/controller/external_secrets/deployments_test.go (1)
431-431: Verify ConfigMap name matches the manifest.Based on a previous review comment, the ConfigMap name should be
"external-secrets-operator-trusted-ca-bundle"to match the OLM manifest atbundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml. The current value"external-secrets-trusted-ca-bundle"may cause runtime failures.Run the following script to verify the ConfigMap name across the codebase:
#!/bin/bash # Search for all references to the trusted CA bundle ConfigMap name rg -n 'external-secrets.*trusted-ca-bundle' --type yaml --type gopkg/controller/external_secrets/deployments.go (2)
412-432: Missing cleanup when proxy configuration is disabled.When
proxyConfigisnil, the function returns early without removing previously set proxy environment variables. This means once proxy is configured, it cannot be disabled by removing the configuration. The same issue exists inupdateTrustedCABundleVolumesfor CA bundle volumes/mounts.To support proxy removal, the function should clean up when
proxyConfigis nil:func (r *Reconciler) updateProxyEnvironmentVariables(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) error { proxyConfig := r.getProxyConfiguration(esc) - if proxyConfig == nil { - return nil - } - // Apply proxy environment variables to all containers - for i := range deployment.Spec.Template.Spec.Containers { - container := &deployment.Spec.Template.Spec.Containers[i] - r.setProxyEnvVars(container, proxyConfig) - } - - // Apply proxy environment variables to all init containers - for i := range deployment.Spec.Template.Spec.InitContainers { - initContainer := &deployment.Spec.Template.Spec.InitContainers[i] - r.setProxyEnvVars(initContainer, proxyConfig) - } + process := func(c *corev1.Container) { + if proxyConfig == nil { + r.removeProxyEnvVars(c) + } else { + r.setProxyEnvVars(c, proxyConfig) + } + } + + for i := range deployment.Spec.Template.Spec.Containers { + process(&deployment.Spec.Template.Spec.Containers[i]) + } + for i := range deployment.Spec.Template.Spec.InitContainers { + process(&deployment.Spec.Template.Spec.InitContainers[i]) + } return nil }Add the removal helper:
// removeProxyEnvVars removes all proxy-related environment variables from a container. func (r *Reconciler) removeProxyEnvVars(container *corev1.Container) { if len(container.Env) == 0 { return } proxyEnvVars := map[string]struct{}{ "HTTP_PROXY": {}, "HTTPS_PROXY": {}, "NO_PROXY": {}, "http_proxy": {}, "https_proxy": {}, "no_proxy": {}, } filtered := container.Env[:0] for _, e := range container.Env { if _, isProxy := proxyEnvVars[e.Name]; !isProxy { filtered = append(filtered, e) } } container.Env = filtered }
473-528: Missing cleanup of CA bundle volumes when proxy is disabled.Similar to the environment variables issue, when
proxyConfigisnil, the function returns early without removing the trusted CA bundle volume and its mounts. This leaves stale volumes mounted even after proxy configuration is removed.Apply symmetric cleanup logic:
func (r *Reconciler) updateTrustedCABundleVolumes(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) error { proxyConfig := r.getProxyConfiguration(esc) - // Only add trusted CA bundle if proxy is configured - if proxyConfig == nil { - return nil - } + removeVolumeMount := func(container *corev1.Container) { + filtered := container.VolumeMounts[:0] + for _, vm := range container.VolumeMounts { + if vm.Name != trustedCABundleVolumeName { + filtered = append(filtered, vm) + } + } + container.VolumeMounts = filtered + } + + if proxyConfig == nil { + // Remove volume mounts from all containers and init containers + for i := range deployment.Spec.Template.Spec.Containers { + removeVolumeMount(&deployment.Spec.Template.Spec.Containers[i]) + } + for i := range deployment.Spec.Template.Spec.InitContainers { + removeVolumeMount(&deployment.Spec.Template.Spec.InitContainers[i]) + } + // Remove the volume itself + filtered := deployment.Spec.Template.Spec.Volumes[:0] + for _, v := range deployment.Spec.Template.Spec.Volumes { + if v.Name != trustedCABundleVolumeName { + filtered = append(filtered, v) + } + } + deployment.Spec.Template.Spec.Volumes = filtered + return nil + } // Add the trusted CA bundle volume to the pod spec trustedCAVolume := corev1.Volume{
🧹 Nitpick comments (1)
pkg/controller/external_secrets/deployments_test.go (1)
424-830: Add test case for proxy removal scenario.The test suite comprehensively covers proxy configuration being added and updated, but lacks a test case for when proxy configuration is removed (i.e., when
proxyConfigtransitions from being set tonil). This scenario should verify that proxy environment variables and CA bundle volumes/mounts are cleaned up.Add a test case similar to:
{ name: "Proxy removal cleans up environment variables and volumes", deployment: &appsv1.Deployment{ Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ { Name: "external-secrets", Env: []corev1.EnvVar{ {Name: "HTTP_PROXY", Value: "http://old-proxy:8080"}, {Name: "EXISTING_VAR", Value: "existing-value"}, }, }, }, Volumes: []corev1.Volume{ { Name: "trusted-ca-bundle", VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ Name: "external-secrets-trusted-ca-bundle", }, }, }, }, }, }, }, }, }, externalSecretsConfig: &v1alpha1.ExternalSecretsConfig{}, externalSecretsManager: &v1alpha1.ExternalSecretsManager{}, olmEnvVars: map[string]string{}, expectedContainerEnvVars: map[string][]corev1.EnvVar{ "external-secrets": { {Name: "EXISTING_VAR", Value: "existing-value"}, }, }, expectedVolumes: []corev1.Volume{}, expectedVolumeMounts: map[string][]corev1.VolumeMount{}, },
📜 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 (5)
pkg/controller/external_secrets/configmap.go(1 hunks)pkg/controller/external_secrets/constants.go(1 hunks)pkg/controller/external_secrets/deployments.go(3 hunks)pkg/controller/external_secrets/deployments_test.go(2 hunks)pkg/controller/external_secrets/utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/external_secrets/constants.go
- pkg/controller/external_secrets/configmap.go
🔇 Additional comments (5)
pkg/controller/external_secrets/utils.go (1)
127-155: LGTM! Proxy configuration precedence is correctly implemented.The three-tier precedence (ExternalSecretsConfig → ExternalSecretsManager → OLM environment variables) is clearly implemented and aligns with the design. The nil checks for nested fields are appropriate given that
ApplicationConfigis a struct andr.esmis always initialized.pkg/controller/external_secrets/deployments_test.go (1)
832-922: LGTM! Well-structured test helpers.The validation helper functions are well-organized and provide clear, detailed error messages for test failures. The separation of concerns (environment variables, volumes, volume mounts) makes the tests easy to understand and maintain.
pkg/controller/external_secrets/deployments.go (3)
400-410: LGTM! Clean orchestration of proxy configuration updates.The function correctly delegates to specialized handlers for environment variables and CA bundle volumes, with proper error handling and context.
434-471: LGTM! Comprehensive proxy environment variable handling.The function correctly handles both uppercase and lowercase variants of proxy environment variables, which improves compatibility across different tools and libraries. The update-or-append logic avoids duplicates.
530-544: LGTM! Clean helper for adding volume mounts.The function correctly checks for existing mounts and either updates or appends, avoiding duplicates.
openshift-merge-robot
removed
the
needs-rebase
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@chiragkyal: This pull request references ESO-186 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. |
chiragkyal
changed the title
chiragkyal
changed the title
| mapFunc := func(ctx context.Context, obj client.Object) []reconcile.Request { | ||
| r.log.V(4).Info("received reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace()) | ||
|
|
||
| objLabels := obj.GetLabels() |
There was a problem hiding this comment.
This is required for Secret resource filtering scenario.
There was a problem hiding this comment.
If we modify the predicate for Secret resource from builder.WithPredicates(predicate.LabelChangedPredicate{}) to managedResourcePredicate, then this won't be required, right?
Moreover, there is a dedicated check for the label change; so I think the above predicate can be modified.
external-secrets-operator/pkg/controller/external_secrets/secret.go
Lines 39 to 47 in e3fe1cf
There was a problem hiding this comment.
The reason for using predicate.LabelChangedPredicate{} is because we are not interested in the data. If both predicates can be combined then no issues.
There was a problem hiding this comment.
Combined both the predicates and modified to use
case &corev1.Secret{}, &corev1.ConfigMap{}:
mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{}, managedResources))| case &corev1.Secret{}: | ||
| mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) | ||
| case &corev1.ConfigMap{}: | ||
| mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), configMapResourcePredicate) |
There was a problem hiding this comment.
We can do something like below for current configmap usage too, and later this would need an update. Or we can just use the managedResources and default case works well.
| case &corev1.Secret{}: | |
| mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) | |
| case &corev1.ConfigMap{}: | |
| mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), configMapResourcePredicate) | |
| case &corev1.Secret{}, &corev1.ConfigMap{}: | |
| mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) |
There was a problem hiding this comment.
Updated to use managedResourcePredicate for the configmap; since we are already checking the label modification through if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap) line.
case &corev1.Secret{}:
mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{}))
case &corev1.ConfigMap{}:
mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate)There was a problem hiding this comment.
Modified to use
case &corev1.Secret{}, &corev1.ConfigMap{}:
mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{}, managedResources))|
|
||
| // setProxyEnvVars sets proxy environment variables on a container. | ||
| func (r *Reconciler) setProxyEnvVars(container *corev1.Container, proxyConfig *operatorv1alpha1.ProxyConfig) { | ||
| if proxyConfig == nil { |
There was a problem hiding this comment.
My suggestion was to handle this case.
|
Please update the PR description. |
|
@chiragkyal: This pull request references ESO-186 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/controller/external_secrets/controller.go (2)
413-417: Status updates short-circuited; Ready may not update.Using
||causes the secondSetStatusConditionto be skipped if the first returns true. Evaluate both, then update.- if apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) || - apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) { - errUpdate = r.updateCondition(esc, err) - err = utilerrors.NewAggregate([]error{err, errUpdate}) - } + changed := apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) + changed = apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) || changed + if changed { + errUpdate = r.updateCondition(esc, err) + err = utilerrors.NewAggregate([]error{err, errUpdate}) + }
440-443: Same short-circuit bug on success path.Apply the same fix here.
- if apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) || - apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) { - errUpdate = r.updateCondition(esc, nil) - } + changed := apimeta.SetStatusCondition(&esc.Status.Conditions, degradedCond) + changed = apimeta.SetStatusCondition(&esc.Status.Conditions, readyCond) || changed + if changed { + errUpdate = r.updateCondition(esc, nil) + }
🧹 Nitpick comments (1)
pkg/controller/external_secrets/controller.go (1)
287-288: ConfigMap case is redundant; let default case handle it.The explicit ConfigMap case uses
managedResourcePredicate, which is identical to what the default case uses (line 290). This case can be removed to reduce duplication.Apply this diff to remove the redundant case:
case &corev1.Secret{}: mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) - case &corev1.ConfigMap{}: - mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate) default: mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate) }
📜 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 (3)
pkg/controller/external_secrets/configmap.go(1 hunks)pkg/controller/external_secrets/controller.go(3 hunks)pkg/controller/external_secrets/install_external_secrets.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/external_secrets/install_external_secrets.go
🔇 Additional comments (4)
pkg/controller/external_secrets/controller.go (2)
77-77: LGTM! ConfigMap added to managed resources.Adding
&corev1.ConfigMap{}tocontrollerManagedResourcesis correct and necessary for the new trusted CA bundle ConfigMap feature. This ensures the controller creates an informer and cache for ConfigMap resources.
255-262: LGTM! mapFunc simplification is correct.The simplified mapFunc that always returns a reconcile request is correct because the cache (configured in
BuildCustomClientat lines 178-180) already filters allcontrollerManagedResourcesby therequestEnqueueLabel. Therefore, any object reaching this mapFunc has already been filtered by label, making the label check redundant.pkg/controller/external_secrets/configmap.go (2)
14-79: LGTM! Trusted CA bundle ConfigMap management is well-implemented.The implementation correctly:
- Creates the ConfigMap only when proxy configuration is present
- Leaves the
Datafield unset, as it's managed by CNO (Cluster Network Operator)- Applies the required injection label (
config.openshift.io/inject-trusted-cabundle: "true") for CNO to populate the CA bundle- Updates only labels when the ConfigMap exists, avoiding conflicts with CNO's data management
- Includes appropriate error handling, logging, and event recording
The comment on lines 62-64 appropriately documents the potential race condition with CNO annotations, which is a good safeguard for future maintenance.
81-89: LGTM! Label merging helper is clean and correct.The
getTrustedCABundleLabelshelper correctly merges resource labels with the injection label required by CNO.
|
@chiragkyal: The following test failed, say
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. |
|
/retest |
| } | ||
| case &corev1.Secret{}: | ||
| mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) | ||
| case &corev1.ConfigMap{}: |
There was a problem hiding this comment.
This can be removed, default will handle the same.
There was a problem hiding this comment.
Modified to use
case &corev1.Secret{}, &corev1.ConfigMap{}:
mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{}, managedResources))
|
/label docs-approved |
openshift-ci
bot
added
docs-approved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
pkg/controller/external_secrets/configmap.go(1 hunks)pkg/controller/external_secrets/deployments.go(3 hunks)pkg/controller/external_secrets/deployments_test.go(2 hunks)
| namespace := getNamespace(esc) | ||
| expectedLabels := getTrustedCABundleLabels(resourceLabels) | ||
|
|
||
| desiredConfigMap := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: trustedCABundleConfigMapName, | ||
| Namespace: namespace, | ||
| Labels: expectedLabels, | ||
| }, | ||
| } | ||
|
|
||
| configMapName := fmt.Sprintf("%s/%s", desiredConfigMap.GetNamespace(), desiredConfigMap.GetName()) | ||
| r.log.V(4).Info("reconciling trusted CA bundle ConfigMap resource", "name", configMapName) | ||
|
|
||
| // Check if the ConfigMap already exists | ||
| existingConfigMap := &corev1.ConfigMap{} | ||
| exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), existingConfigMap) | ||
| if err != nil { | ||
| return common.FromClientError(err, "failed to check %s trusted CA bundle ConfigMap resource already exists", configMapName) | ||
| } | ||
|
|
||
| if !exist { | ||
| // Create the ConfigMap | ||
| if err := r.Create(r.ctx, desiredConfigMap); err != nil { | ||
| return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) | ||
| } | ||
| r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s created", configMapName) | ||
| return nil | ||
| } | ||
|
|
||
| // ConfigMap exists, ensure it has the correct labels | ||
| // Do not update the data of the ConfigMap since it is managed by CNO | ||
| // Check if metadata (labels) has been modified. | ||
| // NOTE: Currently ObjectMetadataModified only checks labels, but if it's extended | ||
| // in the future to check annotations as well, CNO may race with this update since | ||
| // CNO adds `openshift.io/owning-component: Networking / cluster-network-operator` annotations on this ConfigMap. | ||
| if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap) { | ||
| r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName) | ||
| // Update the labels since | ||
| existingConfigMap.Labels = desiredConfigMap.Labels | ||
|
|
There was a problem hiding this comment.
Use the CA-bundle injection annotation, not a label
CNO injects trust bundles only when the ConfigMap carries the annotation config.openshift.io/inject-trusted-cabundle: "true". Populating it under metadata.labels (as done here via getTrustedCABundleLabels) means the operand ConfigMap will never be patched with the cluster CA, so every proxy/TLS flow that relies on this bundle will fail. Please move the injection marker to metadata.annotations (and ensure the reconcile path preserves any annotations CNO adds) so the bundle actually gets injected.
Apply the following diff:
- expectedLabels := getTrustedCABundleLabels(resourceLabels)
+ expectedLabels := maps.Clone(resourceLabels)
+ expectedAnnotations := map[string]string{
+ trustedCABundleInjectAnnotation: "true",
+ }
desiredConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: trustedCABundleConfigMapName,
Namespace: namespace,
- Labels: expectedLabels,
+ Labels: expectedLabels,
+ Annotations: expectedAnnotations,
},
}Then, when the ConfigMap already exists, make sure you merge the annotation without dropping any others before calling UpdateWithRetry:
- if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap) {
+ needsUpdate := common.ObjectMetadataModified(desiredConfigMap, existingConfigMap)
+
+ if existingConfigMap.Annotations == nil {
+ existingConfigMap.Annotations = map[string]string{}
+ }
+ if existingConfigMap.Annotations[trustedCABundleInjectAnnotation] != "true" {
+ existingConfigMap.Annotations[trustedCABundleInjectAnnotation] = "true"
+ needsUpdate = true
+ }
+
+ if needsUpdate {
r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName)
- // Update the labels since
+ // Update the labels
existingConfigMap.Labels = desiredConfigMap.Labels(You'll need to import maps from the standard library and define trustedCABundleInjectAnnotation alongside the existing constants.)
📝 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.
| namespace := getNamespace(esc) | |
| expectedLabels := getTrustedCABundleLabels(resourceLabels) | |
| desiredConfigMap := &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: trustedCABundleConfigMapName, | |
| Namespace: namespace, | |
| Labels: expectedLabels, | |
| }, | |
| } | |
| configMapName := fmt.Sprintf("%s/%s", desiredConfigMap.GetNamespace(), desiredConfigMap.GetName()) | |
| r.log.V(4).Info("reconciling trusted CA bundle ConfigMap resource", "name", configMapName) | |
| // Check if the ConfigMap already exists | |
| existingConfigMap := &corev1.ConfigMap{} | |
| exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), existingConfigMap) | |
| if err != nil { | |
| return common.FromClientError(err, "failed to check %s trusted CA bundle ConfigMap resource already exists", configMapName) | |
| } | |
| if !exist { | |
| // Create the ConfigMap | |
| if err := r.Create(r.ctx, desiredConfigMap); err != nil { | |
| return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) | |
| } | |
| r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s created", configMapName) | |
| return nil | |
| } | |
| // ConfigMap exists, ensure it has the correct labels | |
| // Do not update the data of the ConfigMap since it is managed by CNO | |
| // Check if metadata (labels) has been modified. | |
| // NOTE: Currently ObjectMetadataModified only checks labels, but if it's extended | |
| // in the future to check annotations as well, CNO may race with this update since | |
| // CNO adds `openshift.io/owning-component: Networking / cluster-network-operator` annotations on this ConfigMap. | |
| if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap) { | |
| r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName) | |
| // Update the labels since | |
| existingConfigMap.Labels = desiredConfigMap.Labels | |
| namespace := getNamespace(esc) | |
| // Use the base resourceLabels as labels, and set the injection annotation | |
| expectedLabels := maps.Clone(resourceLabels) | |
| expectedAnnotations := map[string]string{ | |
| trustedCABundleInjectAnnotation: "true", | |
| } | |
| desiredConfigMap := &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: trustedCABundleConfigMapName, | |
| Namespace: namespace, | |
| Labels: expectedLabels, | |
| Annotations: expectedAnnotations, | |
| }, | |
| } | |
| configMapName := fmt.Sprintf("%s/%s", desiredConfigMap.GetNamespace(), desiredConfigMap.GetName()) | |
| r.log.V(4).Info("reconciling trusted CA bundle ConfigMap resource", "name", configMapName) | |
| // Check if the ConfigMap already exists | |
| existingConfigMap := &corev1.ConfigMap{} | |
| exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), existingConfigMap) | |
| if err != nil { | |
| return common.FromClientError(err, "failed to check %s trusted CA bundle ConfigMap resource already exists", configMapName) | |
| } | |
| if !exist { | |
| // Create the ConfigMap | |
| if err := r.Create(r.ctx, desiredConfigMap); err != nil { | |
| return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) | |
| } | |
| r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s created", configMapName) | |
| return nil | |
| } | |
| // Determine if labels drifted, then ensure the inject annotation is present | |
| needsUpdate := common.ObjectMetadataModified(desiredConfigMap, existingConfigMap) | |
| if existingConfigMap.Annotations == nil { | |
| existingConfigMap.Annotations = map[string]string{} | |
| } | |
| if existingConfigMap.Annotations[trustedCABundleInjectAnnotation] != "true" { | |
| existingConfigMap.Annotations[trustedCABundleInjectAnnotation] = "true" | |
| needsUpdate = true | |
| } | |
| if needsUpdate { | |
| r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName) | |
| // Update the labels | |
| existingConfigMap.Labels = desiredConfigMap.Labels | |
| // (the update call goes here) | |
| } |
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/configmap.go around lines 31 to 71, the code
currently puts the CNO CA-bundle injection marker into metadata.labels which
prevents CNO from injecting the trust bundle; change the implementation to place
the marker key "config.openshift.io/inject-trusted-cabundle" with value "true"
under desiredConfigMap.ObjectMeta.Annotations (define a constant
trustedCABundleInjectAnnotation), not Labels, and ensure you still set the
expected labels on ObjectMeta.Labels; when reconciling an existing ConfigMap
merge annotations rather than overwriting them (preserve any annotations CNO
added) — use the standard library maps helpers to clone/merge annotation maps so
existingConfigMap.Annotations = merged(existingConfigMap.Annotations,
desiredConfigMap.Annotations) before calling UpdateWithRetry, and keep updating
labels as before.
There was a problem hiding this comment.
This is not true, label is expected and not annotation.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenShift Cluster Network Operator CNO CA bundle injection config.openshift.io/inject-trusted-cabundle label or annotation
💡 Result:
Use a label. The CNO (and other operators) request trusted-CA injection by creating an empty ConfigMap with the label
config.openshift.io/inject-trusted-cabundle="true". (Do not combine that label with the service.beta.openshift.io/inject-cabundle annotation — the latter is a different mechanism for service CA injection.) [1][2]
@bharath-b-rh You're absolutely right, and I apologize for the incorrect review comment.
The CNO does indeed use a label (config.openshift.io/inject-trusted-cabundle: "true"), not an annotation, to trigger CA bundle injection. The current implementation in the PR is correct. [1][2]
The suggestion to move it to annotations was incorrect and should be disregarded. Thank you for catching that!
|
/retest |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@chiragkyal: This pull request references ESO-186 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. |
openshift-merge-robot
added
the
needs-rebase
Operator should manage the trusted-ca-configmap for operand Signed-off-by: chiragkyal <[email protected]>
Enable proxy-aware feature in CSV Signed-off-by: chiragkyal <[email protected]>
Signed-off-by: chiragkyal <[email protected]>
Signed-off-by: chiragkyal <[email protected]>
Signed-off-by: chiragkyal <[email protected]>
chiragkyal
force-pushed
the
http-proxy
branch
from
0b2d6ac to
3386a27
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controller/external_secrets/configmap.go (1)
14-81: LGTM! Trusted CA bundle ConfigMap reconciliation well-implemented.The method correctly:
- Creates the ConfigMap only when proxy configuration exists
- Leaves the
datafield untouched (CNO manages CA bundle content)- Updates only labels when they drift from desired state
- Uses consistent error handling patterns
The TODO at line 25 about ConfigMap removal when proxy is removed is reasonable for follow-up work. Consider tracking this in a follow-up issue to ensure cleanup behavior is implemented.
Do you want me to help draft a follow-up issue to track the ConfigMap removal functionality mentioned in the TODO?
📜 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 (13)
bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml(1 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(3 hunks)config/default/manager_metrics_patch.yaml(0 hunks)config/manager/kustomization.yaml(1 hunks)config/manager/trusted-ca-patch.yaml(1 hunks)config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml(1 hunks)pkg/controller/external_secrets/configmap.go(1 hunks)pkg/controller/external_secrets/constants.go(1 hunks)pkg/controller/external_secrets/controller.go(3 hunks)pkg/controller/external_secrets/deployments.go(3 hunks)pkg/controller/external_secrets/deployments_test.go(2 hunks)pkg/controller/external_secrets/install_external_secrets.go(1 hunks)pkg/controller/external_secrets/utils.go(2 hunks)
💤 Files with no reviewable changes (1)
- config/default/manager_metrics_patch.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
- bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml
- pkg/controller/external_secrets/constants.go
🧰 Additional context used
🪛 Checkov (3.2.334)
config/manager/trusted-ca-patch.yaml
[medium] 1-18: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-18: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (19)
config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml (1)
16-16: LGTM! Proxy-aware flag correctly updated.The annotation correctly marks the operator as proxy-aware, which aligns with the proxy configuration and CA bundle support implemented throughout this PR.
config/manager/trusted-ca-patch.yaml (1)
1-18: LGTM! Trusted CA bundle mount correctly configured.The patch appropriately mounts the trusted CA bundle ConfigMap into the controller-manager container. Past review discussions confirmed:
namespace: systemis a standard kustomize placeholder- Directory mount (vs subPath) enables automatic ConfigMap updates
- Mount path
/etc/pki/tls/certsis the standard location for Go applicationsThe static analysis warnings about privilege escalation are generic pod security hints unrelated to this CA bundle mount.
pkg/controller/external_secrets/utils.go (1)
127-155: LGTM! Proxy configuration precedence correctly implemented.The method properly implements the documented precedence order:
- ExternalSecretsConfig.Spec.ApplicationConfig.Proxy
- ExternalSecretsManager.Spec.GlobalConfig.Proxy
- OLM environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY)
Returning
nilwhen no proxy configuration exists is the correct behavior, allowing consumers to conditionally apply proxy settings.pkg/controller/external_secrets/install_external_secrets.go (1)
75-78: LGTM! Trusted CA bundle ConfigMap reconciliation properly integrated.The call is correctly positioned in the reconciliation flow (after secrets, before RBAC resources) and follows the established error-handling pattern. The method internally checks for proxy configuration before creating the ConfigMap, ensuring it's only created when needed.
config/manager/kustomization.yaml (1)
9-23: LGTM! Kustomization correctly generates trusted CA bundle ConfigMap.The configuration properly:
- Generates an empty ConfigMap with the CNO injection label (
config.openshift.io/inject-trusted-cabundle: "true")- Disables name suffix hashing to maintain consistent naming
- Applies the trusted-ca-patch.yaml to the controller-manager Deployment
- Includes appropriate management labels
pkg/controller/external_secrets/deployments_test.go (1)
604-1228: Excellent test coverage for proxy configuration!The test suite comprehensively validates:
- Precedence order (ExternalSecretsConfig → ExternalSecretsManager → OLM env vars)
- Init container handling
- Partial proxy configurations
- Environment variable updates
- Volume and volume mount management
- Cleanup when proxy configuration is removed
The helper functions (
validateEnvironmentVariables,validateVolumes,validateVolumeMounts,findContainer) are well-structured and reusable. This thorough testing significantly increases confidence in the proxy feature implementation.pkg/controller/external_secrets/controller.go (2)
77-77: LGTM! ConfigMap added to managed resources.Adding ConfigMap to the controller-managed resources enables the controller to watch for trusted CA bundle ConfigMap changes and trigger reconciliation when the CNO updates the bundle content.
252-262: LGTM! Watch configuration correctly handles ConfigMaps.The changes properly:
- Unconditionally enqueue reconciliation requests (filtering handled by upstream predicates)
- Watch ConfigMap metadata changes (efficient for label-based reconciliation)
- Apply combined predicates (
LabelChangedPredicate+managedResources) to both Secret and ConfigMap resourcesThis ensures the trusted CA bundle ConfigMap triggers reconciliation when its labels change, while avoiding unnecessary reconciliations for unrelated ConfigMaps.
Also applies to: 285-286
pkg/controller/external_secrets/configmap.go (1)
83-91: LGTM! Label merging helper is clean and correct.The helper properly creates a new map and merges resource labels with the CNO injection label without mutating the input. The implementation is straightforward and serves its purpose well.
pkg/controller/external_secrets/deployments.go (10)
6-6: LGTM!The new imports are used appropriately for lowercase proxy environment variables and efficient set-based filtering.
Also applies to: 12-12
148-150: LGTM!The proxy configuration is properly integrated into the deployment construction flow with appropriate error handling.
432-442: LGTM!The orchestration function properly delegates to sub-functions and wraps errors with context.
444-469: LGTM!The function correctly handles both containers and init containers, and properly supports both adding and removing proxy environment variables based on configuration presence. This addresses previous review feedback about init container coverage and removal symmetry.
471-508: LGTM!The function correctly sets both uppercase and lowercase proxy environment variables for broader compatibility, properly handles deduplication, and skips empty values. This addresses previous review feedback about lowercase variants.
536-548: LGTM!The function correctly delegates to add or remove operations based on proxy configuration presence, ensuring proper volume lifecycle management. This addresses previous review feedback about trusted CA volume symmetry.
599-623: LGTM!The function correctly removes the trusted CA bundle volume and mounts from both containers and init containers, ensuring proper cleanup when proxy configuration is removed.
625-650: LGTM!
The volume mount helpers work as expected, and getProxyConfiguration in pkg/controller/external_secrets/utils.go correctly applies ESC → ESM → OLM env var precedence.
510-534: LGTM. removeProxyEnvVars efficiently removes both uppercase and lowercase proxy env vars; the constants are declared in pkg/controller/external_secrets/constants.go.
550-597: approve addTrustedCABundleVolumes implementationtrustedCABundleVolumeName, trustedCABundleConfigMapName, and trustedCABundleMountPath are defined in pkg/controller/external_secrets/constants.go; no further changes required.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/common/utils.go (2)
298-301: Fix potential nil-pointer deref on SecurityContext (critical).Dereferencing fetchedContainer.SecurityContext without a nil check can panic when fetched has no securityContext.
Apply this diff:
- // SecurityContext nil check - if desiredContainer.SecurityContext != nil && !reflect.DeepEqual(*desiredContainer.SecurityContext, *fetchedContainer.SecurityContext) { - return true - } + // SecurityContext + if !equality.Semantic.DeepEqual(desiredContainer.SecurityContext, fetchedContainer.SecurityContext) { + return true + }Add import:
import ( "context" "fmt" "reflect" "sync" "sync/atomic" + "k8s.io/apimachinery/pkg/api/equality"
283-296: Use semantic deep-equal for probes and include liveness/startup.Only checking HTTPGet.Path misses many probe fields and probe types; add StartupProbe/LivenessProbe checks.
Apply this diff:
- // ReadinessProbe nil checks - if (desiredContainer.ReadinessProbe == nil) != (fetchedContainer.ReadinessProbe == nil) { - return true - } - if desiredContainer.ReadinessProbe != nil && fetchedContainer.ReadinessProbe != nil { - if (desiredContainer.ReadinessProbe.HTTPGet == nil) != (fetchedContainer.ReadinessProbe.HTTPGet == nil) { - return true - } - if desiredContainer.ReadinessProbe.HTTPGet != nil && fetchedContainer.ReadinessProbe.HTTPGet != nil { - if desiredContainer.ReadinessProbe.HTTPGet.Path != fetchedContainer.ReadinessProbe.HTTPGet.Path { - return true - } - } - } + // Probes + if !equality.Semantic.DeepEqual(desiredContainer.ReadinessProbe, fetchedContainer.ReadinessProbe) { + return true + } + if !equality.Semantic.DeepEqual(desiredContainer.LivenessProbe, fetchedContainer.LivenessProbe) { + return true + } + if !equality.Semantic.DeepEqual(desiredContainer.StartupProbe, fetchedContainer.StartupProbe) { + return true + }
🧹 Nitpick comments (9)
pkg/controller/external_secrets/deployments.go (3)
548-595: LGTM: Comprehensive CA bundle volume addition.The function correctly adds the trusted CA bundle volume and mounts it to all containers and init containers with proper deduplication and read-only access.
Optional: Consider pre-allocating the volumes slice when checking existence:
// Instead of appending to existing slice, could check existence first volumeExists := false volumeIndex := -1 for i, volume := range deployment.Spec.Template.Spec.Volumes { if volume.Name == trustedCABundleVolumeName { volumeExists = true volumeIndex = i break } } if volumeExists { deployment.Spec.Template.Spec.Volumes[volumeIndex] = trustedCAVolume } else { deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, trustedCAVolume) }However, the current implementation is clear and performs well in practice, so this is purely optional.
597-621: LGTM: Complete CA bundle removal logic.The function correctly removes the CA bundle volume and all associated mounts from both containers and init containers.
Optional micro-optimization: Pre-allocate
filteredVolumesslice capacity:- var filteredVolumes []corev1.Volume + filteredVolumes := make([]corev1.Volume, 0, len(deployment.Spec.Template.Spec.Volumes))This matches the pattern used in
removeProxyEnvVars(line 526) but is a very minor performance improvement.
639-648: LGTM: Straightforward volume mount removal.The helper correctly filters out the CA bundle volume mount.
Optional micro-optimization: Pre-allocate
filteredVolumeMountsslice capacity similar to line 600:- var filteredVolumeMounts []corev1.VolumeMount + filteredVolumeMounts := make([]corev1.VolumeMount, 0, len(container.VolumeMounts))This is consistent with the pattern used in
removeTrustedCABundleVolumesandremoveProxyEnvVars, but is a very minor improvement.pkg/controller/common/utils.go (6)
266-281: Compare container ports by all relevant fields, not just ContainerPort.Current logic ignores Name, Protocol, HostPort, HostIP, which can lead to missed updates.
Apply this diff:
- // Check ports - if len(desiredContainer.Ports) != len(fetchedContainer.Ports) { - return true - } - for _, fetchedPort := range fetchedContainer.Ports { - matched := false - for _, desiredPort := range desiredContainer.Ports { - if fetchedPort.ContainerPort == desiredPort.ContainerPort { - matched = true - break - } - } - } - if !matched { - return true - } + // Check ports (order-insensitive; compare all relevant fields) + if !portsEqual(desiredContainer.Ports, fetchedContainer.Ports) { + return true + }Add helper:
func portsEqual(a, b []corev1.ContainerPort) bool { if len(a) != len(b) { return false } aa := append([]corev1.ContainerPort(nil), a...) bb := append([]corev1.ContainerPort(nil), b...) less := func(x, y corev1.ContainerPort) bool { if x.ContainerPort != y.ContainerPort { return x.ContainerPort < y.ContainerPort } if x.Protocol != y.Protocol { return x.Protocol < y.Protocol } return x.Name < y.Name } sort.Slice(aa, func(i, j int) bool { return less(aa[i], aa[j]) }) sort.Slice(bb, func(i, j int) bool { return less(bb[i], bb[j]) }) return equality.Semantic.DeepEqual(aa, bb) }
254-258: Also compare Command (and optionally EnvFrom) for completeness.Args are checked, but Command can differ and should trigger reconcile; EnvFrom often used with ConfigMaps/Secrets.
Apply this diff:
- if !reflect.DeepEqual(desiredContainer.Args, fetchedContainer.Args) || + if !reflect.DeepEqual(desiredContainer.Command, fetchedContainer.Command) || + !reflect.DeepEqual(desiredContainer.Args, fetchedContainer.Args) || desiredContainer.Name != fetchedContainer.Name || desiredContainer.Image != fetchedContainer.Image || desiredContainer.ImagePullPolicy != fetchedContainer.ImagePullPolicy { return true } + // Optional: EnvFrom + if !equality.Semantic.DeepEqual(desiredContainer.EnvFrom, fetchedContainer.EnvFrom) { + return true + }
233-237: Match containers by name, not index, to avoid ordering churn.Iterating by index assumes identical ordering, causing false positives if order differs. Compare by Name maps.
Illustrative refactor:
- for i := range desired.Spec.Template.Spec.Containers { - if containerSpecModified(&desired.Spec.Template.Spec.Containers[i], &fetched.Spec.Template.Spec.Containers[i]) { - return true - } - } + fetchedByName := map[string]*corev1.Container{} + for i := range fetched.Spec.Template.Spec.Containers { + c := &fetched.Spec.Template.Spec.Containers[i] + fetchedByName[c.Name] = c + } + for i := range desired.Spec.Template.Spec.Containers { + dc := &desired.Spec.Template.Spec.Containers[i] + fc, ok := fetchedByName[dc.Name] + if !ok || containerSpecModified(dc, fc) { + return true + } + } @@ - for i := range desired.Spec.Template.Spec.InitContainers { - if containerSpecModified(&desired.Spec.Template.Spec.InitContainers[i], &fetched.Spec.Template.Spec.InitContainers[i]) { - return true - } - } + fetchedInitByName := map[string]*corev1.Container{} + for i := range fetched.Spec.Template.Spec.InitContainers { + c := &fetched.Spec.Template.Spec.InitContainers[i] + fetchedInitByName[c.Name] = c + } + for i := range desired.Spec.Template.Spec.InitContainers { + dc := &desired.Spec.Template.Spec.InitContainers[i] + fc, ok := fetchedInitByName[dc.Name] + if !ok || containerSpecModified(dc, fc) { + return true + } + }Also applies to: 243-247
209-215: Volume comparison: redundant length check and asymmetric nil handling.
- The explicit length check is redundant (volumesEqual already checks length).
- If desired.Volumes is nil but fetched has extra volumes, no diff is detected.
Either rely solely on volumesEqual and make it nil-safe, or add a symmetric check.
Apply this diff:
- // Check volumes - if desired.Spec.Template.Spec.Volumes != nil && len(desired.Spec.Template.Spec.Volumes) != len(fetched.Spec.Template.Spec.Volumes) { - return true - } - if desired.Spec.Template.Spec.Volumes != nil && !volumesEqual(desired.Spec.Template.Spec.Volumes, fetched.Spec.Template.Spec.Volumes) { + // Check volumes + if !volumesEqual(desired.Spec.Template.Spec.Volumes, fetched.Spec.Template.Spec.Volumes) { return true }And update helper to be nil-safe (see next comment).
316-369: Broaden volumesEqual coverage and make it nil-safe.
- Not nil-safe: when both are nil, they should be equal; when one is nil and the other not, they differ.
- ConfigMap volume ignores Items, Optional, DefaultMode.
- Other common types (Projected, PersistentVolumeClaim, HostPath) are not handled.
Apply this diff:
-func volumesEqual(desired, fetched []corev1.Volume) bool { - if len(desired) != len(fetched) { +func volumesEqual(desired, fetched []corev1.Volume) bool { + if len(desired) == 0 && len(fetched) == 0 { + return true + } + if len(desired) != len(fetched) { return false } @@ - // Check ConfigMap volume + // Check ConfigMap volume if desiredVol.ConfigMap != nil { if fetchedVol.ConfigMap == nil { return false } - if desiredVol.ConfigMap.Name != fetchedVol.ConfigMap.Name { + if desiredVol.ConfigMap.Name != fetchedVol.ConfigMap.Name { return false } + if !equality.Semantic.DeepEqual(desiredVol.ConfigMap.Items, fetchedVol.ConfigMap.Items) { + return false + } + if !equality.Semantic.DeepEqual(desiredVol.ConfigMap.Optional, fetchedVol.ConfigMap.Optional) { + return false + } + if !equality.Semantic.DeepEqual(desiredVol.ConfigMap.DefaultMode, fetchedVol.ConfigMap.DefaultMode) { + return false + } } @@ - // Add other volume types as needed (PVC, HostPath, etc.) + // PVC + if desiredVol.PersistentVolumeClaim != nil { + if fetchedVol.PersistentVolumeClaim == nil { + return false + } + if desiredVol.PersistentVolumeClaim.ClaimName != fetchedVol.PersistentVolumeClaim.ClaimName { + return false + } + } + // Projected + if desiredVol.Projected != nil { + if fetchedVol.Projected == nil { + return false + } + if !equality.Semantic.DeepEqual(desiredVol.Projected, fetchedVol.Projected) { + return false + } + } + // HostPath + if desiredVol.HostPath != nil { + if fetchedVol.HostPath == nil { + return false + } + if !equality.Semantic.DeepEqual(desiredVol.HostPath, fetchedVol.HostPath) { + return false + } + } }
252-258: Minor: keep comparisons comprehensive but focused.Given the proxy/CA work, ensure these fields are covered to avoid silent drift:
- Add WorkingDir and Lifecycle comparisons.
- Consider comparing TerminationMessage{Path,Policy} if set in desired.
Example:
+ if desiredContainer.WorkingDir != fetchedContainer.WorkingDir { + return true + } + if !equality.Semantic.DeepEqual(desiredContainer.Lifecycle, fetchedContainer.Lifecycle) { + return true + }
📜 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 (3)
pkg/controller/common/utils.go(3 hunks)pkg/controller/external_secrets/constants.go(1 hunks)pkg/controller/external_secrets/deployments.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/external_secrets/constants.go
🔇 Additional comments (7)
pkg/controller/external_secrets/deployments.go (7)
146-148: LGTM: Clean integration of proxy configuration.The proxy configuration step is properly integrated into the deployment construction flow with consistent error handling.
430-442: LGTM: Well-structured orchestration function.The function efficiently calls
getProxyConfigurationonce and passes the result to both env var and volume management functions, addressing the concern about multiple calls.
444-467: LGTM: Comprehensive env var management.The function correctly handles both containers and init containers, with symmetric add/remove behavior based on proxy configuration presence. This addresses all previous concerns about init container coverage and removal logic.
469-506: LGTM: Robust env var setting with proper deduplication.The function correctly sets both uppercase and lowercase proxy environment variables, handles empty values gracefully, and prevents duplicate entries through proper update logic.
508-533: LGTM: Efficient removal logic with switch-based filtering.The function uses a switch statement (as suggested in previous review) and pre-allocates the filtered slice for optimal performance. The implementation cleanly removes all proxy-related environment variables.
535-546: LGTM: Clean delegation for CA bundle management.The function provides clear conditional logic to add or remove CA bundle volumes based on proxy configuration presence, maintaining symmetry between add and remove operations.
623-637: LGTM: Clean helper for adding volume mounts.The helper function properly handles both updating existing mounts and adding new ones, preventing duplicates.
Signed-off-by: chiragkyal <[email protected]>
chiragkyal
force-pushed
the
http-proxy
branch
from
66fbc75 to
661b156
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/controller/common/utils.go (4)
314-316: Nil-deref risk when comparing SecurityContextIf desiredContainer.SecurityContext != nil but fetchedContainer.SecurityContext == nil, dereferencing fetched panics. Compare nil-safely.
Apply:
- // SecurityContext nil check - if desiredContainer.SecurityContext != nil && !reflect.DeepEqual(*desiredContainer.SecurityContext, *fetchedContainer.SecurityContext) { - return true - } + // SecurityContext compare (nil-safe) + if (desiredContainer.SecurityContext == nil) != (fetchedContainer.SecurityContext == nil) { + return true + } + if desiredContainer.SecurityContext != nil && !equality.Semantic.DeepEqual(desiredContainer.SecurityContext, fetchedContainer.SecurityContext) { + return true + }Add import if not present:
+ "k8s.io/apimachinery/pkg/api/equality"
281-296: Port comparison ignores protocol/name; can miss diffsOnly matching on ContainerPort can hide protocol/name changes.
- // Check ports - if len(desiredContainer.Ports) != len(fetchedContainer.Ports) { + // Check ports (match by port+protocol+name) + if len(desiredContainer.Ports) != len(fetchedContainer.Ports) { return true } - for _, fetchedPort := range fetchedContainer.Ports { - matched := false - for _, desiredPort := range desiredContainer.Ports { - if fetchedPort.ContainerPort == desiredPort.ContainerPort { - matched = true - break - } - } - if !matched { - return true - } - } + type key struct{ port int32; proto corev1.Protocol; name string } + want := make(map[key]struct{}, len(desiredContainer.Ports)) + for _, p := range desiredContainer.Ports { + want[key{p.ContainerPort, p.Protocol, p.Name}] = struct{}{} + } + for _, p := range fetchedContainer.Ports { + if _, ok := want[key{p.ContainerPort, p.Protocol, p.Name}]; !ok { + return true + } + }
194-199: AutomountServiceAccountToken comparison is one‑sidedIf desired.AutomountServiceAccountToken is nil but fetched has a value, changes aren’t detected.
- if desired.Spec.Template.Spec.ServiceAccountName != fetched.Spec.Template.Spec.ServiceAccountName || - desired.Spec.Template.Spec.AutomountServiceAccountToken != nil { - if !reflect.DeepEqual(desired.Spec.Template.Spec.AutomountServiceAccountToken, fetched.Spec.Template.Spec.AutomountServiceAccountToken) { - return true - } - } + if desired.Spec.Template.Spec.ServiceAccountName != fetched.Spec.Template.Spec.ServiceAccountName { + return true + } + if !equality.Semantic.DeepEqual(desired.Spec.Template.Spec.AutomountServiceAccountToken, fetched.Spec.Template.Spec.AutomountServiceAccountToken) { + return true + }Add import if needed:
+ "k8s.io/apimachinery/pkg/api/equality"
480-483: IsESMSpecEmpty returns inverted result vs its nameThe function claims to return whether the Spec is empty but returns true when it’s non-empty and esm != nil.
-func IsESMSpecEmpty(esm *operatorv1alpha1.ExternalSecretsManager) bool { - return esm != nil && !reflect.DeepEqual(esm.Spec, operatorv1alpha1.ExternalSecretsManagerSpec{}) -} +func IsESMSpecEmpty(esm *operatorv1alpha1.ExternalSecretsManager) bool { + if esm == nil { + return true + } + return reflect.DeepEqual(esm.Spec, operatorv1alpha1.ExternalSecretsManagerSpec{}) +}pkg/controller/external_secrets/deployments.go (1)
405-416: Critical: updateSecretVolumeConfig sets SecretName on the first volume and breaksThe loop breaks unconditionally, and the SecretName is assigned without checking the volume name, corrupting the first volume and skipping the target.
-func updateSecretVolumeConfig(deployment *appsv1.Deployment, volumeName, secretName string) { - volumeExists := false - for i := range deployment.Spec.Template.Spec.Volumes { - if deployment.Spec.Template.Spec.Volumes[i].Name == volumeName { - volumeExists = true - } - if deployment.Spec.Template.Spec.Volumes[i].Secret == nil { - deployment.Spec.Template.Spec.Volumes[i].Secret = &corev1.SecretVolumeSource{} - } - deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = secretName - break - } +func updateSecretVolumeConfig(deployment *appsv1.Deployment, volumeName, secretName string) { + volumeExists := false + for i := range deployment.Spec.Template.Spec.Volumes { + if deployment.Spec.Template.Spec.Volumes[i].Name == volumeName { + if deployment.Spec.Template.Spec.Volumes[i].Secret == nil { + deployment.Spec.Template.Spec.Volumes[i].Secret = &corev1.SecretVolumeSource{} + } + deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = secretName + volumeExists = true + break + } + } if !volumeExists { deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, corev1.Volume{ Name: volumeName, VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: secretName, }, }, }) } }
♻️ Duplicate comments (2)
pkg/controller/external_secrets/deployments.go (1)
430-442: Reconcile safety depends on order-insensitive comparisons in utilsThis method updates Env and VolumeMounts; utils.containerSpecModified currently uses reflect.DeepEqual for Env/VolumeMounts/Resources which is order-sensitive and may cause noisy updates.
See companion suggestion in pkg/controller/common/utils.go to switch to semantic, order-insensitive comparisons.
pkg/controller/common/utils.go (1)
276-279: Make Env, VolumeMounts, and Resources comparisons order- and default-safe to avoid reconcile loopsCurrent reflect.DeepEqual is order-sensitive and non-semantic; quantity strings and slice orders can cause spurious diffs.
- // Check environment variables - if !reflect.DeepEqual(desiredContainer.Env, fetchedContainer.Env) { + // Check environment variables (order-insensitive) + if !envsEqual(desiredContainer.Env, fetchedContainer.Env) { return true } @@ - // Check volume mounts - if !reflect.DeepEqual(desiredContainer.VolumeMounts, fetchedContainer.VolumeMounts) { + // Check volume mounts (order-insensitive) + if !volumeMountsEqual(desiredContainer.VolumeMounts, fetchedContainer.VolumeMounts) { return true } @@ - // Check resources - if !reflect.DeepEqual(desiredContainer.Resources, fetchedContainer.Resources) { + // Check resources (semantic) + if !equality.Semantic.DeepEqual(desiredContainer.Resources, fetchedContainer.Resources) { return true }Add helpers (place below volumesEqual):
// order-insensitive env compare func envsEqual(a, b []corev1.EnvVar) bool { if len(a) != len(b) { return false } aa := append([]corev1.EnvVar(nil), a...) bb := append([]corev1.EnvVar(nil), b...) sort.Slice(aa, func(i, j int) bool { return aa[i].Name < aa[j].Name }) sort.Slice(bb, func(i, j int) bool { return bb[i].Name < bb[j].Name }) return equality.Semantic.DeepEqual(aa, bb) } // order-insensitive volumeMount compare func volumeMountsEqual(a, b []corev1.VolumeMount) bool { if len(a) != len(b) { return false } aa := append([]corev1.VolumeMount(nil), a...) bb := append([]corev1.VolumeMount(nil), b...) sort.Slice(aa, func(i, j int) bool { return aa[i].MountPath < aa[j].MountPath }) sort.Slice(bb, func(i, j int) bool { return bb[i].MountPath < bb[j].MountPath }) return equality.Semantic.DeepEqual(aa, bb) }Imports:
+ "sort" + "k8s.io/apimachinery/pkg/api/equality"Also applies to: 318-326
📜 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 (3)
pkg/controller/common/utils.go(3 hunks)pkg/controller/external_secrets/constants.go(1 hunks)pkg/controller/external_secrets/deployments.go(2 hunks)
🔇 Additional comments (8)
pkg/controller/external_secrets/constants.go (2)
52-64: LGTM! Trusted CA bundle constants are well-defined.The constants follow good naming conventions and are appropriately documented. The mount path
/etc/pki/tls/certsis correct for RHEL-based systems (OpenShift). The reference to the Go source code is helpful for understanding the path choice.Minor note: The comment references
go1.24.4which may not be available yet. Consider using a stable Go version tag (e.g.,go1.23.0) or simply removing the specific version from the URL to point to the latest source.
66-74: LGTM! Proxy constants provide good compatibility coverage.Defining both uppercase and lowercase variants ensures compatibility with applications and libraries that expect either convention. This approach is cleaner than runtime conversion and addresses the previous review feedback effectively.
pkg/controller/external_secrets/deployments.go (6)
146-148: Good placement for proxy wiringHooking updateProxyConfiguration after node/affinity/tolerations ensures a fully built desired spec before env/volume mutations.
445-467: InitContainers covered; symmetric set/remove logicupdateProxyEnvironmentVariables correctly applies to both containers and initContainers and removes stale vars when unset.
470-506: Lowercase + uppercase proxy envs handled; dedup via in-place updatesetProxyEnvVars correctly updates existing entries and adds both casings. LGTM.
509-533: Efficient proxy env removal with switch and constsremoveProxyEnvVars is clean and O(n). No concerns.
535-546: CA bundle add/remove toggled by proxy presenceThe orchestration is symmetric and clear.
549-595: CA bundle volume + mounts applied to all containers and initContainersIdempotent add/update and clean removal. Looks solid.
Also applies to: 597-621, 623-648
| func volumesEqual(desired, fetched []corev1.Volume) bool { | ||
| if len(desired) == 0 && len(fetched) == 0 { | ||
| return true | ||
| } | ||
| if len(desired) != len(fetched) { | ||
| return false | ||
| } | ||
|
|
||
| // Create a map of fetched volumes by name for easier lookup | ||
| fetchedMap := make(map[string]corev1.Volume) | ||
| for _, v := range fetched { | ||
| fetchedMap[v.Name] = v | ||
| } | ||
|
|
||
| // Check each desired volume exists and matches in fetched | ||
| for _, desiredVol := range desired { | ||
| fetchedVol, exists := fetchedMap[desiredVol.Name] | ||
| if !exists { | ||
| return false | ||
| } | ||
|
|
||
| // Compare volume sources | ||
| // Check ConfigMap volume | ||
| if desiredVol.ConfigMap != nil { | ||
| if fetchedVol.ConfigMap == nil { | ||
| return false | ||
| } | ||
| if desiredVol.ConfigMap.Name != fetchedVol.ConfigMap.Name { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Check Secret volume | ||
| if desiredVol.Secret != nil { | ||
| if fetchedVol.Secret == nil { | ||
| return false | ||
| } | ||
| if desiredVol.Secret.SecretName != fetchedVol.Secret.SecretName { | ||
| return false | ||
| } | ||
| if desiredVol.Secret.Items != nil && !reflect.DeepEqual(desiredVol.Secret.Items, fetchedVol.Secret.Items) { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Check EmptyDir volume | ||
| if desiredVol.EmptyDir != nil { | ||
| if fetchedVol.EmptyDir == nil { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Add other volume types as needed (PVC, HostPath, etc.) | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
volumesEqual misses fields and is asymmetric; may fail to detect differences
ConfigMap: ignores Items/defaultMode/Optional; Secret: Items check only when desired.Items != nil; EmptyDir: ignores Medium/SizeLimit. Type mismatches aren’t handled explicitly.
func volumesEqual(desired, fetched []corev1.Volume) bool {
@@
- // Compare volume sources
- // Check ConfigMap volume
- if desiredVol.ConfigMap != nil {
- if fetchedVol.ConfigMap == nil {
- return false
- }
- if desiredVol.ConfigMap.Name != fetchedVol.ConfigMap.Name {
- return false
- }
- }
+ // Compare volume sources (type and key fields)
+ if (desiredVol.ConfigMap == nil) != (fetchedVol.ConfigMap == nil) {
+ return false
+ }
+ if desiredVol.ConfigMap != nil {
+ d, f := desiredVol.ConfigMap, fetchedVol.ConfigMap
+ if d.Name != f.Name || !boolPtrEqual(d.Optional, f.Optional) || !int32PtrEqual(d.DefaultMode, f.DefaultMode) {
+ return false
+ }
+ if !cmItemsEqual(d.Items, f.Items) {
+ return false
+ }
+ }
@@
- // Check Secret volume
- if desiredVol.Secret != nil {
- if fetchedVol.Secret == nil {
- return false
- }
- if desiredVol.Secret.SecretName != fetchedVol.Secret.SecretName {
- return false
- }
- if desiredVol.Secret.Items != nil && !reflect.DeepEqual(desiredVol.Secret.Items, fetchedVol.Secret.Items) {
- return false
- }
- }
+ if (desiredVol.Secret == nil) != (fetchedVol.Secret == nil) {
+ return false
+ }
+ if desiredVol.Secret != nil {
+ d, f := desiredVol.Secret, fetchedVol.Secret
+ if d.SecretName != f.SecretName || !boolPtrEqual(d.Optional, f.Optional) || !int32PtrEqual(d.DefaultMode, f.DefaultMode) {
+ return false
+ }
+ if !cmItemsEqual(d.Items, f.Items) { // same schema as ConfigMapKeySelector items
+ return false
+ }
+ }
@@
- // Check EmptyDir volume
- if desiredVol.EmptyDir != nil {
- if fetchedVol.EmptyDir == nil {
- return false
- }
- }
+ if (desiredVol.EmptyDir == nil) != (fetchedVol.EmptyDir == nil) {
+ return false
+ }
+ if desiredVol.EmptyDir != nil {
+ d, f := desiredVol.EmptyDir, fetchedVol.EmptyDir
+ if d.Medium != f.Medium {
+ return false
+ }
+ if (d.SizeLimit == nil) != (f.SizeLimit == nil) {
+ return false
+ }
+ if d.SizeLimit != nil && f.SizeLimit != nil && d.SizeLimit.Cmp(*f.SizeLimit) != 0 {
+ return false
+ }
+ }Add helpers outside the function:
func boolPtrEqual(a, b *bool) bool { if a==nil || b==nil { return a==b }; return *a==*b }
func int32PtrEqual(a, b *int32) bool { if a==nil || b==nil { return a==b }; return *a==*b }
func cmItemsEqual(a, b []corev1.KeyToPath) bool {
if len(a) != len(b) { return false }
aa := append([]corev1.KeyToPath(nil), a...)
bb := append([]corev1.KeyToPath(nil), b...)
sort.Slice(aa, func(i, j int) bool { return aa[i].Key < aa[j].Key || (aa[i].Key==aa[j].Key && aa[i].Path < aa[j].Path) })
sort.Slice(bb, func(i, j int) bool { return bb[i].Key < bb[j].Key || (bb[i].Key==bb[j].Key && bb[i].Path < bb[j].Path) })
return equality.Semantic.DeepEqual(aa, bb)
}Imports:
+ "sort"
+ "k8s.io/apimachinery/pkg/api/equality"🤖 Prompt for AI Agents
In pkg/controller/common/utils.go around lines 331-387, volumesEqual is
asymmetric and misses key fields (ConfigMap: Items/defaultMode/Optional; Secret:
defaultMode/Optional and Items only checked when desired non-nil; EmptyDir:
Medium/SizeLimit) and doesn’t explicitly detect type mismatches; add helper
functions outside the function to compare pointer values (boolPtrEqual,
int32PtrEqual) and to compare KeyToPath slices in an order-insensitive way
(sort.Slice then equality.Semantic.DeepEqual), import sort and
k8s.io/apimachinery/pkg/api/equality, then update volumesEqual to: (1) treat nil
vs non-nil as unequal for each volume source type, (2) compare all relevant
fields for ConfigMap (Name, Items, DefaultMode, Optional) and Secret
(SecretName, Items, DefaultMode, Optional) using the helpers, (3) compare
EmptyDir Medium and SizeLimit, (4) explicitly handle other volume types and type
mismatches, and (5) use the KeyToPath comparator to ensure Items equality
regardless of order.
5 participants
Description
This PR implements HTTP(s) proxy support for ESO, enabling the operator to function in environments where access requires proxy configuration.
If a cluster-wide proxy is configured, OLM automatically updates the operator deployments with
HTTP_PROXY,HTTPS_PROXY, andNO_PROXYenvironment variables. These variables will then be propagated to all the operand deployments by the operator.Proxy configurations can also be set specific to
external-secretsinexternalsecretsconfigs.operator.openshift.io, or in common configurations for all operands managed by the operator inexternalsecretsmanager.operator.openshift.io.Changes
OLM creates a new configmap
external-secrets-operator-trusted-ca-bundlewithconfig.openshift.io/inject-trusted-cabundle: "true"for OpenShift CNO injection and mounts it in the Operator pod.bundle/manifests/external-secrets-operator-trusted-ca-bundle_v1_configmap.yaml- New trusted CA bundle ConfigMapOperator configures
HTTP_PROXY,HTTPS_PROXY, andNO_PROXYenvironment variables on all containers and init containers for operand deployments.Operator creates and manages a ConfigMap (
external-secrets-trusted-ca-bundle) with the injection label so that CNO can inject the trusted CA bundles. It also mounts this configmap in the operand containers./etc/pki/tls/certs(standard Go certificate path)Enhanced controller to watch ConfigMap changes and trigger reconciliation.
Updates CSV to mark the operator as proxy-aware (
features.operators.openshift.io/proxy-aware: "true")Configuration Hierarchy
The proxy configuration follows this precedence order:
ExternalSecretsConfig.Spec.ApplicationConfig.CommonConfigs.ProxyExternalSecretsManager.Spec.GlobalConfig.CommonConfigs.Proxy