ESO-101: Restructures and renames ExternalSecrets API to ExternalSecretsConfig #54
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-101 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.20.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-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
bharath-b-rh
changed the title
openshift-merge-robot
added
the
needs-rebase
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughRenamed the cluster-scoped API from ExternalSecrets to ExternalSecretsConfig, introduced new ExternalSecretsConfig types and shared meta types, removed legacy ExternalSecrets types, updated CRDs/CSVs/samples/RBAC/controllers/tests, regenerated deepcopy, and added an API test generator and test tooling. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@bharath-b-rh: This pull request references ESO-101 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
pkg/controller/common/utils.go (1)
454-460: Race: Now.Do can execute f twice under contentionBoth callers can pass the initial check, one after another, and each will call f. Re-check after locking.
func (n *Now) Do(f func()) { - n.done.Load() - if n.done.Load() == 0 { - n.Lock() - defer n.Unlock() - - defer n.done.Store(1) - f() - } + if n.done.Load() == 1 { + return + } + n.Lock() + defer n.Unlock() + if n.done.Load() == 1 { + return + } + n.done.Store(1) + f() }pkg/controller/crd_annotator/controller.go (1)
272-276: Avoid taking the address of the range variable (Go vet issue)Passing &crd takes the address of the loop variable, which is reused each iteration. Use the slice element’s address instead.
- for _, crd := range managedCRDList.Items { - if err := r.updateAnnotations(&crd); err != nil { + for i := range managedCRDList.Items { + crd := &managedCRDList.Items[i] + if err := r.updateAnnotations(crd); err != nil { return fmt.Errorf("failed to update annotations in %q: %w", crd.GetName(), err) } }config/manifests/bases/external-secrets-operator.clusterserviceversion.yaml (1)
133-138: Fix grammar and clarity in the operator descriptionUser-facing copy has minor grammar issues (“an uniformed interface”, double-space, punctuation). Clean it up for docs quality.
- 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 external secret to fetch.pkg/controller/external_secrets/utils.go (1)
47-56: Bug: aggregated error drops prependErr and duplicates the status error.When updateStatus fails and prependErr is non-nil, you aggregate err and errUpdate, but omit prependErr. Return prependErr + the wrapped update error instead.
Apply:
func (r *Reconciler) updateCondition(esc *operatorv1alpha1.ExternalSecretsConfig, prependErr error) error { if err := r.updateStatus(r.ctx, esc); err != nil { errUpdate := fmt.Errorf("failed to update %s/%s status: %w", esc.GetNamespace(), esc.GetName(), err) if prependErr != nil { - return utilerrors.NewAggregate([]error{err, errUpdate}) + return utilerrors.NewAggregate([]error{prependErr, errUpdate}) } return errUpdate } return prependErr }pkg/controller/external_secrets/rbacs_test.go (1)
171-176: Bug: Comparing ClusterRole name against ClusterRoleBinding asset constant.This can silently miss the intended match if names diverge. Compare using the ClusterRole test asset/constant.
Apply:
- m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { - switch obj.(type) { - case *rbacv1.ClusterRole: - if obj.GetName() == testClusterRoleBinding(certControllerClusterRoleBindingAssetName).GetName() { - return commontest.TestClientError - } - } - return nil - }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + switch obj.(type) { + case *rbacv1.ClusterRole: + if obj.GetName() == testClusterRole(certControllerClusterRoleAssetName).GetName() { + return commontest.TestClientError + } + } + return nil + })pkg/controller/external_secrets/secret_test.go (1)
175-178: Test case never enables cert-manager; assertion is ineffectiveThe "secret creation skipped when cert-manager config is enabled" test doesn't set CertManagerConfig.Enabled to "true", so it doesn't actually exercise the skip path and may accidentally pass. Make the test fail if Create is called and explicitly enable cert-manager.
- { - 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.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + return fmt.Errorf("unexpected Create call when cert-manager is enabled") + }) + }, + esc: func(esc *v1alpha1.ExternalSecretsConfig) { + if esc.Spec.CertManagerConfig == nil { + esc.Spec.CertManagerConfig = &v1alpha1.CertManagerConfig{} + } + esc.Spec.CertManagerConfig.Enabled = "true" + }, + },bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
331-336: Fix grammar in user-facing descriptionUse “a uniform interface” instead of “an uniformed interface”.
- 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 + description: External Secrets Operator for Red Hat OpenShift deploys and manages + `external-secrets` application in OpenShift clusters. `external-secrets` provides + a uniform interface to fetch secrets stored in external providers like AWSpkg/controller/external_secrets/deployments_test.go (1)
364-368: Fix redundant if and mismatched expected value in test error message.Remove duplicate condition and print the actual expected value.
- if tt.wantErr == "" { - if tt.wantErr == "" { - if externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { - t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", externalsecrets.Status.ExternalSecretsImage, "test-image") - } - } - } + if tt.wantErr == "" && externalsecrets.Status.ExternalSecretsImage != commontest.TestExternalSecretsImageName { + t.Errorf("createOrApplyDeployments() got image in status: %v, want: %v", + externalsecrets.Status.ExternalSecretsImage, commontest.TestExternalSecretsImageName) + }pkg/controller/external_secrets_manager/controller.go (1)
67-70: Missing RBAC for ExternalSecretsConfig.The controller watches/gets ExternalSecretsConfig but lacks RBAC; this will fail on clusters.
// +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsmanagers,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsmanagers/status,verbs=get;update;patch // +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsmanagers/finalizers,verbs=update +// +kubebuilder:rbac:groups=operator.openshift.io,resources=externalsecretsconfigs,verbs=get;list;watchpkg/controller/external_secrets/controller.go (2)
407-421: Always set non-empty Condition.Message to satisfy CRD.To align with the CRD (or if keeping message required), ensure both Ready and Degraded conditions always have a non-empty Message.
if isFatal { degradedCond.Status = metav1.ConditionTrue degradedCond.Reason = operatorv1alpha1.ReasonFailed degradedCond.Message = fmt.Sprintf("reconciliation failed with irrecoverable error, not retrying: %v", err) readyCond.Status = metav1.ConditionFalse readyCond.Reason = operatorv1alpha1.ReasonReady + readyCond.Message = fmt.Sprintf("operator not ready due to irrecoverable error: %v", err) } else { degradedCond.Status = metav1.ConditionFalse degradedCond.Reason = operatorv1alpha1.ReasonReady + degradedCond.Message = "reconciliation in progress after a recoverable error" readyCond.Status = metav1.ConditionFalse readyCond.Reason = operatorv1alpha1.ReasonInProgress readyCond.Message = fmt.Sprintf("reconciliation failed, retrying: %v", err) } ... // Successful reconciliation degradedCond := metav1.Condition{ Type: operatorv1alpha1.Degraded, Status: metav1.ConditionFalse, Reason: operatorv1alpha1.ReasonReady, + Message: "no degradation detected", ObservedGeneration: observedGeneration, } readyCond := metav1.Condition{ Type: operatorv1alpha1.Ready, Status: metav1.ConditionTrue, Reason: operatorv1alpha1.ReasonReady, Message: "reconciliation successful", ObservedGeneration: observedGeneration, }Also applies to: 436-451
99-115: Minimize RBAC on CRDs: avoid delete unless strictly required.Granting delete on apiextensions.k8s.io/customresourcedefinitions is risky. The operator typically needs get/list/watch/update/patch; delete should be avoided unless you own and must tear down CRDs.
-// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch;create;update;patchIf delete is necessary, scope it to the specific CRD names with resourceNames=… instead of wildcarding.
pkg/controller/external_secrets/deployments.go (1)
110-117: Don't require Bitwarden image for non-Bitwarden assets; fix label value.
- getDeploymentObject errors if BITWARDEN image is unset even when Bitwarden is disabled or when reconciling non-Bitwarden deployments. This blocks normal reconciles.
- The version label is set to the env var name, not its value.
Apply:
- 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()) - } + image := os.Getenv(externalsecretsImageEnvVarName)And in the switch:
case controllerDeploymentAssetName: + 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()) + } updateContainerSpec(deployment, esc, image, logLevel) case webhookDeploymentAssetName: + 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()) + } updateWebhookContainerSpec(deployment, image, logLevel) case certControllerDeploymentAssetName: + 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()) + } updateCertControllerContainerSpec(deployment, image, logLevel) 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()) + } + if ver := os.Getenv(bitwardenImageVersionEnvVarName); ver != "" { + deployment.Labels["app.kubernetes.io/version"] = ver + } + updateBitwardenServerContainerSpec(deployment, bitwardenImage)Also applies to: 127-130
🧹 Nitpick comments (63)
README.md (1)
28-30: Fix typo and align list marker style (markdownlint MD004).Use dashes for sub-bullets and correct “external-scerets” → “external-secrets”.
- * reconciling the `externalsecretsconfig.openshift.operator.io` resource. - * installing and managing the `external-secrets` application based on the user defined configurations in `externalsecretsconfig.openshift.operator.io` resource. - * reconciling the `externalsecretsmanagers.openshift.operator.io` resource for the global configurations and updates the `external-scerets` deployment accordingly. + - reconciling the `externalsecretsconfig.openshift.operator.io` resource. + - installing and managing the `external-secrets` application based on the user defined configurations in `externalsecretsconfig.openshift.operator.io` resource. + - reconciling the `externalsecretsmanagers.openshift.operator.io` resource for the global configurations and updates the `external-secrets` deployment accordingly.config/samples/operator_v1alpha1_externalsecrets.yaml (1)
2-2: Rename the sample file to match the updated Kind
The fileconfig/samples/operator_v1alpha1_externalsecrets.yamlshould be renamed tooperator_v1alpha1_externalsecretsconfig.yamlto align withkind: ExternalSecretsConfig.test/e2e/testdata/external_secret.yaml (1)
2-9: Fixture updated to ExternalSecretsConfig — OK.Minor nit: label app.kubernetes.io/name typically reflects the app (“external-secrets-operator” or “externalsecretsconfig”), not the object name “cluster”.
- labels: - app.kubernetes.io/name: cluster + labels: + app.kubernetes.io/name: externalsecretsconfigpkg/controller/common/constants.go (1)
12-14: Provide a deprecated alias to avoid downstream breakages.If any external consumers still reference ExternalSecretsObjectName, keep a compat alias.
// ExternalSecretsConfigObjectName is the default name of the externalsecretsconfig.openshift.operator.io CR. ExternalSecretsConfigObjectName = "cluster" + // Deprecated: use ExternalSecretsConfigObjectName. + ExternalSecretsObjectName = ExternalSecretsConfigObjectNamepkg/controller/external_secrets/validatingwebhook_test.go (1)
17-19: Rename test resource name to match new kindThe constant still uses "externalsecret-validate". Consider aligning with the new resource naming to reduce confusion.
Apply:
- testValidateWebhookConfigurationResourceName = "externalsecret-validate" + testValidateWebhookConfigurationResourceName = "externalsecretsconfig-validate"pkg/controller/external_secrets/secret.go (1)
14-22: Consistent naming for cert-manager togglecreateOrApplySecret uses isCertManagerConfigEnabled while common exposes IsInjectCertManagerAnnotationEnabled(esc). Consider consolidating to a single helper to reduce drift.
pkg/controller/common/utils.go (1)
430-435: Fix error message wording in RemoveFinalizerThe message mentions “failed to create … with finalizers removed” while removing finalizers. Clarify.
- return fmt.Errorf("failed to create %q externalsecretsconfig.openshift.operator.io object with finalizers removed", namespacedName) + return fmt.Errorf("failed to remove finalizer %q from %q", finalizer, namespacedName)pkg/controller/external_secrets/serviceaccounts.go (2)
15-16: Param naming driftexternalsecretsCreateRecon still carries the old name. Consider rename to createRecon or installRecon.
61-71: ServiceAccount drift isn’t reconciledIf only labels/metadata change, we skip updates. Consider updating when metadata differs, mirroring other resources.
- if exist { + if exist { + if common.ObjectMetadataModified(desired, fetched) { + if err := r.UpdateWithRetry(r.ctx, desired); err != nil { + return common.FromClientError(err, "failed to update serviceaccount %s", serviceAccountName) + } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Updated serviceaccount %s to desired state", serviceAccountName) + } if externalsecretsCreateRecon { r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s serviceaccount already exists, possibly from a previous install", serviceAccountName) }pkg/controller/external_secrets/install_external_secrets.go (3)
43-46: Message wording: “unallowed” → “disallowed”Align log text with disallowedLabelMatcher naming.
- r.log.V(1).Info("skip adding unallowed label configured in externalsecretsconfig.operator.openshift.io", "label", k, "value", v) + r.log.V(1).Info("skip adding disallowed label configured in externalsecretsconfig.operator.openshift.io", "label", k, "value", v)Also consider the same change for the ESM log above in this function.
55-57: Abort reconciliation if namespace creation failsCurrently we log and continue; downstream creates will keep failing. Prefer returning the error to requeue.
if err := r.createOrApplyNamespace(esc, resourceLabels); err != nil { - r.log.Error(err, "failed to create namespace") + r.log.Error(err, "failed to create namespace; aborting reconciliation") + return err }
23-26: GVK in error message may be emptyGetObjectKind().GroupVersionKind() can be unset on typed objects; consider using a constant kind string to avoid empty messages.
pkg/controller/crd_annotator/controller.go (2)
192-205: Fix resource group/name in log and comment stringsSeveral messages use “externalsecretsconfig.openshift.operator.io” (reversed group) and singular resource. Use “externalsecretsconfigs.operator.openshift.io” for clarity and grep-ability.
Example fixes:
- // 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", "key", key) + r.log.V(1).Info("externalsecretsconfigs.operator.openshift.io object not found, skipping reconciliation", "key", key) - return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsconfig.openshift.operator.io %q during reconciliation: %w", key, err) + return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsconfigs.operator.openshift.io %q during reconciliation: %w", key, err)And in updateStatus:
- // updateStatus is for updating the status subresource of externalsecretsconfig.openshift.operator.io. + // updateStatus updates the status subresource of externalsecretsconfigs.operator.openshift.io. - r.log.V(4).Info("updating externalsecretsconfig.openshift.operator.io status", "request", namespacedName) + r.log.V(4).Info("updating externalsecretsconfigs.operator.openshift.io status", "request", namespacedName) - return fmt.Errorf("failed to fetch externalsecretsconfig.openshift.operator.io %q for status update: %w", namespacedName, err) + return fmt.Errorf("failed to fetch externalsecretsconfigs.operator.openshift.io %q for status update: %w", namespacedName, err) - return fmt.Errorf("failed to update externalsecretsconfig.openshift.operator.io %q status: %w", namespacedName, err) + return fmt.Errorf("failed to update externalsecretsconfigs.operator.openshift.io %q status: %w", namespacedName, err)Also applies to: 304-317
66-69: Prefer request-scoped context over r.ctx for API callsUsing a global background context hinders cancellation on shutdown and per-reconcile timeouts. Thread the Reconcile ctx through helpers.
Minimal direction:
- Add a ctx parameter to processReconcileRequest and updateCondition.
- Use that ctx for all r.Get/List/Patch/StatusUpdate calls.
Also applies to: 237-239
pkg/controller/external_secrets/service_test.go (1)
59-66: Optional: add a test for explicit "false" (or default) to ensure no Bitwarden service creationCovers the negative path for the Bitwarden service when disabled by spec.
pkg/controller/external_secrets/certificate_test.go (2)
73-81: Minor robustness: avoid calling helper in wantErr to derive nameUsing testExternalSecretsConfigForCertificate() inside wantErr may drift if the helper changes. Consider hard-coding the expected name constant or deriving it from commontest.
Example:
- wantErr: fmt.Sprintf("failed to update certificate resource for %s/%s deployment: issuerRef.Name not present", commontest.TestExternalSecretsNamespace, testExternalSecretsConfigForCertificate().GetName()), + wantErr: fmt.Sprintf("failed to update certificate resource for %s/%s deployment: issuerRef.Name not present", commontest.TestExternalSecretsNamespace, commontest.TestExternalSecretsName),
221-225: Reduce repeated setup with small helpersRepeated lines enable CertManager and set IssuerRef.Name in many cases. Introduce a tiny helper to reduce duplication and improve intent.
func enableCertManagerWithIssuer(esc *v1alpha1.ExternalSecretsConfig, name string) { if esc.Spec.CertManagerConfig == nil { esc.Spec.CertManagerConfig = &v1alpha1.CertManagerConfig{IssuerRef: &v1alpha1.ObjectReference{}} } esc.Spec.CertManagerConfig.Enabled = "true" esc.Spec.CertManagerConfig.IssuerRef.Name = name }Also applies to: 287-291, 333-342, 381-390, 428-432
pkg/controller/external_secrets_manager/controller_test.go (1)
161-181: Pluralize resource in ServerTimeout and align expected error for consistency.
Keeps test expectations consistent with K8s plural resource naming.Apply this diff:
- case *operatorv1alpha1.ExternalSecretsConfig: - return errors.NewServerTimeout(operatorv1alpha1.Resource("externalsecretsconfig"), "Get", int(5)) + case *operatorv1alpha1.ExternalSecretsConfig: + return errors.NewServerTimeout(operatorv1alpha1.Resource("externalsecretsconfigs"), "Get", 5) @@ - wantErr: `failed to fetch externalsecretsconfig.openshift.operator.io "/cluster" during reconciliation: The Get operation against externalsecretsconfig.operator.openshift.io could not be completed at this time, please try again.`, + wantErr: `failed to fetch externalsecretsconfigs.openshift.operator.io "/cluster" during reconciliation: The Get operation against externalsecretsconfigs.operator.openshift.io could not be completed at this time, please try again.`,pkg/controller/external_secrets/services.go (2)
15-16: Unify flag name: externalsecretsCreateRecon → externalSecretsConfigCreateRecon.
Purely a naming consistency fix; no behavior change.Apply this diff:
-func (r *Reconciler) createOrApplyServices(esc *operatorv1alpha1.ExternalSecretsConfig, resourceLabels map[string]string, externalSecretsConfigCreateRecon bool) error { +func (r *Reconciler) createOrApplyServices(esc *operatorv1alpha1.ExternalSecretsConfig, resourceLabels map[string]string, externalSecretsConfigCreateRecon bool) error { @@ -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 externalsecretsCreateRecon { + if externalSecretsConfigCreateRecon {Also applies to: 43-43, 61-64
63-64: Consider downgrading AlreadyExists to Normal.
Avoid warning noise for benign idempotency.Apply this diff:
- r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s already exists", serviceName) + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "ResourceAlreadyExists", "%s already exists", serviceName)pkg/controller/crd_annotator/controller_test.go (2)
170-175: Use renamed resource in NotFound: externalsecretsconfigs.
Keeps resource names consistent across tests.Apply this diff:
- 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)
416-429: Strengthen assertions: ensure expected conditions are present, not just equal when present.
Current loop skips missing conditions silently.Apply this diff:
- for _, c1 := range esc.Status.Conditions { - for _, c2 := range tt.expectedStatusCondition { - if c1.Type == c2.Type { - if c1.Status != c2.Status || c1.Reason != c2.Reason { - t.Errorf("Reconcile() condition: %+v, expectedStatusCondition: %+v", c1, c2) - } - } - } - } + got := make(map[string]metav1.Condition, len(esc.Status.Conditions)) + for _, c := range esc.Status.Conditions { + got[c.Type] = c + } + for _, exp := range tt.expectedStatusCondition { + if g, ok := got[exp.Type]; !ok { + t.Errorf("missing expected condition type %q", exp.Type) + } else if g.Status != exp.Status || g.Reason != exp.Reason { + t.Errorf("Reconcile() condition: %+v, expectedStatusCondition: %+v", g, exp) + } + }pkg/controller/external_secrets/utils.go (2)
97-101: Comment nit: function mentions CertManager instead of Bitwarden.Apply:
-// isBitwardenConfigEnabled returns whether CertManagerConfig is enabled in ExternalSecretsConfig CR Spec. +// isBitwardenConfigEnabled returns whether BitwardenSecretManagerProvider is enabled in ExternalSecretsConfig Spec.
112-114: Provide fallback for OperatingNamespace.If Spec.OperatingNamespace is empty, consider falling back to getNamespace(esc) (or a defined default) to avoid creating namespaceless objects.
Apply:
func getOperatingNamespace(esc *operatorv1alpha1.ExternalSecretsConfig) string { - return esc.Spec.OperatingNamespace + if esc.Spec.OperatingNamespace != "" { + return esc.Spec.OperatingNamespace + } + return getNamespace(esc) }pkg/controller/external_secrets/serviceaccounts_test.go (2)
75-81: Avoid clobbering defaults by replacing the entire Spec.Overwriting esc.Spec may reset defaults like Namespace/OperatingNamespace. Set only the field you need.
Apply:
-esc.Spec = operatorv1alpha1.ExternalSecretsConfigSpec{ - BitwardenSecretManagerProvider: &operatorv1alpha1.BitwardenSecretManagerProvider{ - Enabled: "true", - }, -} +if esc.Spec.BitwardenSecretManagerProvider == nil { + esc.Spec.BitwardenSecretManagerProvider = &operatorv1alpha1.BitwardenSecretManagerProvider{} +} +esc.Spec.BitwardenSecretManagerProvider.Enabled = "true"
96-103: Same: mutate only the cert-manager knob, not the whole Spec.Apply:
-esc.Spec = operatorv1alpha1.ExternalSecretsConfigSpec{ - CertManagerConfig: &operatorv1alpha1.CertManagerConfig{ - Enabled: "true", - }, -} +if esc.Spec.CertManagerConfig == nil { + esc.Spec.CertManagerConfig = &operatorv1alpha1.CertManagerConfig{} +} +esc.Spec.CertManagerConfig.Enabled = "true"pkg/controller/external_secrets/rbacs_test.go (2)
268-285: Avoid overwriting Spec in success-path tests too.Mirror the targeted mutation approach to preserve defaults.
Apply:
-esc.Spec = operatorv1alpha1.ExternalSecretsConfigSpec{ - CertManagerConfig: &operatorv1alpha1.CertManagerConfig{ - Enabled: "true", - }, -} +if esc.Spec.CertManagerConfig == nil { + esc.Spec.CertManagerConfig = &operatorv1alpha1.CertManagerConfig{} +} +esc.Spec.CertManagerConfig.Enabled = "true"
279-285: Same targeted mutation here.Apply:
-esc.Spec = operatorv1alpha1.ExternalSecretsConfigSpec{ - CertManagerConfig: &operatorv1alpha1.CertManagerConfig{ - Enabled: "true", - }, -} +if esc.Spec.CertManagerConfig == nil { + esc.Spec.CertManagerConfig = &operatorv1alpha1.CertManagerConfig{} +} +esc.Spec.CertManagerConfig.Enabled = "true"pkg/controller/external_secrets/secret_test.go (1)
195-197: Tighten error assertion for readabilityThe current boolean expression is hard to follow. Prefer explicit branches.
- if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { - t.Errorf("createOrApplySecret() err: %v, wantErr: %v", err, tt.wantErr) - } + if tt.wantErr == "" && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if tt.wantErr != "" && (err == nil || err.Error() != tt.wantErr) { + t.Fatalf("error mismatch: got %v, want %v", err, tt.wantErr) + }pkg/controller/external_secrets/validatingwebhook.go (1)
65-67: Wrap underlying error and drop stale wordingUse %w to preserve the original error and remove “external secrets” wording which seems leftover.
- 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 resource for %s: %w", esc.GetName(), err) + }bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
992-1005: Prefer consistent boolean typing across the APISeveral boolean-like fields (e.g., bitwardenSecretManagerProvider.enabled) are strings with "true"/"false". Consider native booleans (with pointers for tri-state) for consistency and better UX.
api/v1alpha1/external_secrets_manager_types.go (1)
74-76: Remove duplicate comment lineThe “Feature is for enabling the optional features.” comment is duplicated.
-// Feature is for enabling the optional features. // Feature is for enabling the optional features. type Feature struct {api/v1alpha1/meta.go (3)
58-64: Reconsider tight MaxProperties caps (20 labels, 10 node selectors).If not product requirements, these hard caps can cause surprising validation failures for users.
Would you like me to propose relaxed limits or remove them entirely?
Also applies to: 84-91
101-112: Validate proxy URL scheme minimally.Add a light Pattern to ensure http(s) scheme without over-restricting.
// httpProxy is the URL of the proxy for HTTP requests. - // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:Pattern=`^https?://` // +kubebuilder:validation:MaxLength=4096 // +kubebuilder:validation:Optional HTTPProxy string `json:"httpProxy,omitempty"` // httpsProxy is the URL of the proxy for HTTPS requests. - // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:Pattern=`^https?://` // +kubebuilder:validation:MaxLength=4096 // +kubebuilder:validation:Optional HTTPSProxy string `json:"httpsProxy,omitempty"`
8-9: Doc nit: make description CRD-agnostic.Use “operator-managed deployments” or “the External Secrets controller” instead of “external-secrets deployment.”
-// ConditionalStatus holds information of the current state of the external-secrets deployment +// ConditionalStatus holds information about the current state of operator-managed deploymentspkg/controller/external_secrets/deployments_test.go (1)
349-349: Minor: rename local var to reflect new type.Use “esc” for clarity.
- externalsecrets := commontest.TestExternalSecretsConfig() + esc := commontest.TestExternalSecretsConfig()And update subsequent references accordingly.
pkg/controller/external_secrets_manager/controller.go (5)
48-50: Comment accuracy: fix API group/kind in finalizer note.- // finalizer name for external-secrets.openshift.operator.io resource. + // finalizer name for externalsecretsmanager.openshift.operator.io resource.
139-155: Avoid storing request-scoped ESC on the reconciler; keep it local.Reduces shared-state risk when controller concurrency >1.
- // Fetch the externalsecretsconfig.openshift.operator.io CR - r.esc = new(operatorv1alpha1.ExternalSecretsConfig) + // Fetch the externalsecretsconfig.openshift.operator.io CR + esc := new(operatorv1alpha1.ExternalSecretsConfig) key = types.NamespacedName{ Name: common.ExternalSecretsConfigObjectName, } - if err := r.Get(ctx, key, r.esc); err != nil { + if err := r.Get(ctx, key, esc); err != nil { if errors.IsNotFound(err) { // NotFound errors, would mean the object hasn't been created yet and // not required to reconcile yet. r.log.V(1).Info("externalsecretsconfig.openshift.operator.io object not found, skipping reconciliation", "key", key) return ctrl.Result{}, nil } return ctrl.Result{}, fmt.Errorf("failed to fetch externalsecretsconfig.openshift.operator.io %q during reconciliation: %w", key, err) } - return r.processReconcileRequest(esm) + return r.processReconcileRequest(esm, esc)
158-166: Pass ESC into the handler instead of reading from struct.-func (r *Reconciler) processReconcileRequest(esm *operatorv1alpha1.ExternalSecretsManager) (ctrl.Result, error) { +func (r *Reconciler) processReconcileRequest(esm *operatorv1alpha1.ExternalSecretsManager, esc *operatorv1alpha1.ExternalSecretsConfig) (ctrl.Result, error) { statusUpdated := false if esm.Status.ControllerStatuses == nil { esm.Status.ControllerStatuses = make([]operatorv1alpha1.ControllerStatus, 0) } - if r.esc != nil && len(r.esc.Status.Conditions) > 0 { - for _, esCond := range r.esc.Status.Conditions { + if esc != nil && len(esc.Status.Conditions) > 0 { + for _, esCond := range esc.Status.Conditions { if r.updateStatusCondition(esm, externalSecretsControllerId, esCond) { statusUpdated = true } } }
171-175: Use the request-scoped context, not r.ctx.Prevents stale context use.
- if statusUpdated { - if err := r.updateStatus(r.ctx, esm); err != nil { + if statusUpdated { + if err := r.updateStatus(ctx, esm); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update externalsecretsmanager.openshift.operator.io status: %w", err) } }
99-104: Optional: map ESC events explicitly to the singleton ESM.Using EnqueueRequestForObject enqueues ESC keys; since Reconcile ignores req and always fetches the singleton ESM, map directly for clarity.
If you agree, I can propose the import + mapping via EnqueueRequestsFromMapFunc that enqueues {Name: common.ExternalSecretsManagerObjectName}.
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (2)
996-1005: Consider booleans over string enums for on/off flags.Using string enums "true"/"false" for enabled-like fields complicates UX and validation. Prefer type: boolean with proper defaults and rules; keep enums only if backward-compatibility demands strings.
Example change:
- enabled: - default: "false" - enum: ["true","false"] - type: string + enabled: + default: false + type: booleanIf strings are required for compatibility, document the rationale in the CRD description.
Also applies to: 1026-1036, 1048-1056
1082-1090: issuerRef unconditional requirement—verify intent.issuerRef is required whenever certManagerConfig is present, even when enabled is "false". If the intent is “issuerRef required only when enabled”, encode that with a CEL rule instead of unconditional required.
- required: - - enabled - - issuerRef + required: + - enabled + x-kubernetes-validations: + - rule: "self.enabled == 'true' ? has(self.issuerRef) : true" + message: "issuerRef must be set when certManagerConfig.enabled == 'true'"pkg/controller/external_secrets/certificate.go (1)
149-158: ClusterIssuer lookup uses NamespacedName with a namespace.Namespacing a cluster-scoped resource in NamespacedName is harmless but noisy in logs. Consider empty namespace for ClusterIssuer to improve clarity.
func (r *Reconciler) getIssuer(issuerRef v1.ObjectReference, namespace string) (bool, error) { - namespacedName := types.NamespacedName{Name: issuerRef.Name, Namespace: namespace} + nn := types.NamespacedName{Name: issuerRef.Name} + if issuerRef.Kind == issuerKind { + nn.Namespace = namespace + } ... - ifExists, err := r.UncachedClient.Exists(r.ctx, namespacedName, object) + ifExists, err := r.UncachedClient.Exists(r.ctx, nn, object) - return ifExists, fmt.Errorf("failed to fetch %q issuer: %w", namespacedName, err) + return ifExists, fmt.Errorf("failed to fetch %q issuer: %w", nn, err) }Also applies to: 163-182
pkg/controller/external_secrets/deployments.go (1)
198-206: Unsafe conversions couple us to internal k/k API; consider safer validation.validateResourceRequirements uses unsafe pointer casts to internal core types. This is brittle across k8s bumps. Prefer:
- Using apiserver-side validation via an admission webhook (if applicable), or
- Converting via k8s API conversion functions, or
- Validating shapes locally without importing k8s.io/kubernetes internal packages.
Optional if the repo standardizes on this pattern.
api/v1alpha1/external_secrets_config_types.go (1)
99-101: Consider booleans over string "true"/"false" for Enabled flags.Using strings complicates validations and XValidations. If backward-compat is not a constraint, prefer bool with proper defaults and immutability guards. If strings are required, current defaults are fine.
If API stability allows, I can provide a patch converting these fields to bool with migration notes.
Also applies to: 127-137, 144-156
pkg/controller/external_secrets/rbacs.go (8)
23-35: Rename plumbed correctly; verify behavior under operatingNamespace.Signatures/calls moved from ES → ESC cleanly. One concern: when spec.operatingNamespace is set, do we still intend to reconcile cluster-scoped RBAC (ClusterRole/ClusterRoleBinding) here? If not, gate those creations behind the same condition used to disable cluster-scoped features.
79-98: Cert-manager gating reads clearer now; name nit.Logic is correct. Consider renaming isCertManagerConfigEnabled → useCertManagerForCerts for readability (it returns “skip cert-controller RBAC” when true).
101-136: Event severity for “already exists” might be too noisy.Emitting Warning for pre-existing resources during reconcile can alarm operators. Consider Normal to reduce noise.
Apply this diff if you agree:
- r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s clusterrole resource already exists, maybe from previous installation", clusterRoleName) + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "ResourceAlreadyExists", "%s clusterrole resource already exists, maybe from previous installation", clusterRoleName)
149-183: Same event severity nit for ClusterRoleBinding.Recommend Normal over Warning for “already exists”.
- r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s clusterrolebinding resource already exists, maybe from previous installation", clusterRoleBindingName) + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "ResourceAlreadyExists", "%s clusterrolebinding resource already exists, maybe from previous installation", clusterRoleBindingName)
199-229: Role create/apply path OK; same Warning→Normal suggestion.Flow mirrors ClusterRole with namespace-aware key; propose Normal for “already exists”.
- r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s role resource already exists, maybe from previous installation", roleName) + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "ResourceAlreadyExists", "%s role resource already exists, maybe from previous installation", roleName)
244-275: RoleBinding path consistent; Warning→Normal suggestion.Same event severity nit as above; otherwise LGTM.
- r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s rolebinding resource already exists, maybe from previous installation", roleBindingName) + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "ResourceAlreadyExists", "%s rolebinding resource already exists, maybe from previous installation", roleBindingName)
282-289: Subject namespace update relies on asset shape.We only update the Namespace on an existing ServiceAccount subject that matches name. If the asset ever omits that subject (or renames), this becomes a silent no-op. Consider asserting at least one SA subject matched and log V(1) when none found.
func (r *Reconciler) getRoleBindingObject(esc *operatorv1alpha1.ExternalSecretsConfig, assetName, roleName, serviceAccountName string, resourceLabels map[string]string) *rbacv1.RoleBinding { roleBinding := common.DecodeRoleBindingObjBytes(assets.MustAsset(assetName)) roleBinding.RoleRef.Name = roleName updateNamespace(roleBinding, esc) common.UpdateResourceLabels(roleBinding, resourceLabels) - updateServiceAccountNamespaceInRBACBindingObject[*rbacv1.RoleBinding](roleBinding, serviceAccountName, roleBinding.GetNamespace()) + if updated := updateServiceAccountNamespaceInRBACBindingObject[*rbacv1.RoleBinding](roleBinding, serviceAccountName, roleBinding.GetNamespace()); !updated { + r.log.V(1).Info("no matching ServiceAccount subject found in rolebinding", "rb", roleBinding.GetName(), "sa", serviceAccountName) + } return roleBinding }And return a bool from the helper (see next comment).
293-306: Generic helper: return whether an update occurred.Current helper always returns void; returning a bool allows call sites to detect drift in asset shape.
-func updateServiceAccountNamespaceInRBACBindingObject[Object *rbacv1.RoleBinding | *rbacv1.ClusterRoleBinding](obj Object, serviceAccount, newNamespace string) { +func updateServiceAccountNamespaceInRBACBindingObject[Object *rbacv1.RoleBinding | *rbacv1.ClusterRoleBinding](obj Object, serviceAccount, newNamespace string) bool { var subjects *[]rbacv1.Subject switch o := any(obj).(type) { case *rbacv1.ClusterRoleBinding: subjects = &o.Subjects case *rbacv1.RoleBinding: subjects = &o.Subjects } + updated := false for i := range *subjects { if (*subjects)[i].Kind == roleBindingSubjectKind && (*subjects)[i].Name == serviceAccount { (*subjects)[i].Namespace = newNamespace + updated = true } } + return updated }docs/api_reference.md (9)
49-54: Use boolean, not string, for enabled.Docs show
enabled _string_with Enum [true false]. This is confusing versusFeature.enabled _boolean_below. Prefertype: booleanfor enabled flags (or clarify intentional string usage). Please verify CRD/schema and fix generator/doc accordingly.
57-79: Fix bare URLs (MD034) and keep content identical.Wrap raw links in angle brackets to satisfy markdownlint.
-| `logLevel` _integer_ | logLevel supports value range as per [kubernetes logging guidelines](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#what-method-to-use). | 1 | Maximum: 5 <br />Minimum: 1 <br />Optional: {} <br /> | +| `logLevel` _integer_ | logLevel supports value range as per [kubernetes logging guidelines](<https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#what-method-to-use>). | 1 | Maximum: 5 <br />Minimum: 1 <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: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | | 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: <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 /> | +| `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 /> | +| `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 /> | +| `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 /> |
177-201: Spec consistency with operatingNamespace and RBAC.Docs say operatingNamespace disables cluster-scoped CRs. Verify controllers/RBAC do not unnecessarily create cluster-scoped permissions when this is set (see rbacs.go discussion).
189-201: Fix bare URLs in Spec table (MD034).Same angle-bracket treatment as earlier for the “resources/affinity/tolerations/nodeSelector” refs.
304-306: Remove duplicate sentence.“Feature is for enabling the optional features.” appears twice.
-Feature is for enabling the optional features. -Feature is for enabling the optional features. +Feature is for enabling the optional features.
315-316: Boolean doesn’t need Enum annotation.For
enabled _boolean_, drop “Enum: [true false]” to avoid implying string enums.-| `enabled` _boolean_ | enabled determines if feature should be turned on. | | Enum: [true false] <br />Required: {} <br /> | +| `enabled` _boolean_ | enabled determines if feature should be turned on. | | Required: {} <br /> |
331-337: Fix bare URLs in GlobalConfig (MD034).Apply the same angle-bracket wrapping for the four references here.
358-377: Clarify env var names for proxy.State that values are surfaced as HTTP_PROXY/HTTPS_PROXY/NO_PROXY (and lowercase variants) in managed pods.
-ProxyConfig is for setting the proxy configurations which will be made available -in operand containers managed by the operator as environment variables. +ProxyConfig sets proxy configuration exposed to managed containers via the standard +environment variables HTTP_PROXY, HTTPS_PROXY, and NO_PROXY (and lowercase variants).
405-410: Fix bare URL in WebhookConfig (MD034).Wrap the Duration link with angle brackets.
-| `certificateCheckInterval` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#duration-v1-meta)_ | CertificateCheckInterval is for configuring the polling interval to check the certificate<br />validity. | 5m | Optional: {} <br /> | +| `certificateCheckInterval` _[Duration](<https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#duration-v1-meta>)_ | CertificateCheckInterval is for configuring the polling interval to check the certificate<br />validity. | 5m | Optional: {} <br /> |
📜 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 (45)
PROJECT(1 hunks)README.md(1 hunks)api/v1alpha1/external_secrets_config_types.go(7 hunks)api/v1alpha1/external_secrets_manager_types.go(3 hunks)api/v1alpha1/meta.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(8 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(7 hunks)bundle/manifests/operator.openshift.io_externalsecrets.yaml(0 hunks)bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml(1 hunks)bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml(5 hunks)config/crd/bases/operator.openshift.io_externalsecrets.yaml(0 hunks)config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml(1 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(11 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(4 hunks)pkg/controller/external_secrets/certificate_test.go(16 hunks)pkg/controller/external_secrets/constants.go(2 hunks)pkg/controller/external_secrets/controller.go(9 hunks)pkg/controller/external_secrets/deployments.go(14 hunks)pkg/controller/external_secrets/deployments_test.go(11 hunks)pkg/controller/external_secrets/install_external_secrets.go(3 hunks)pkg/controller/external_secrets/rbacs.go(10 hunks)pkg/controller/external_secrets/rbacs_test.go(5 hunks)pkg/controller/external_secrets/secret.go(3 hunks)pkg/controller/external_secrets/secret_test.go(2 hunks)pkg/controller/external_secrets/service_test.go(3 hunks)pkg/controller/external_secrets/serviceaccounts.go(4 hunks)pkg/controller/external_secrets/serviceaccounts_test.go(4 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/e2e/e2e_test.go(1 hunks)test/e2e/testdata/external_secret.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- config/crd/bases/operator.openshift.io_externalsecrets.yaml
- bundle/manifests/operator.openshift.io_externalsecrets.yaml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
28-28: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
29-29: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
docs/api_reference.md
74-74: Bare URL used
(MD034, no-bare-urls)
75-75: Bare URL used
(MD034, no-bare-urls)
76-76: Bare URL used
(MD034, no-bare-urls)
77-77: Bare URL used
(MD034, no-bare-urls)
197-197: Bare URL used
(MD034, no-bare-urls)
198-198: Bare URL used
(MD034, no-bare-urls)
199-199: Bare URL used
(MD034, no-bare-urls)
200-200: Bare URL used
(MD034, no-bare-urls)
333-333: Bare URL used
(MD034, no-bare-urls)
334-334: Bare URL used
(MD034, no-bare-urls)
335-335: Bare URL used
(MD034, no-bare-urls)
336-336: Bare URL used
(MD034, no-bare-urls)
bharath-b-rh
force-pushed
the
eso-101
branch
from
cada4da to
91951f4
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
bharath-b-rh
force-pushed
the
eso-101
branch
from
91951f4 to
993d42d
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@bharath-b-rh: This pull request references ESO-101 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. |
|
/test e2e-operator |
bharath-b-rh
force-pushed
the
eso-101
branch
3 times, most recently
from
7008969 to
beafe53
Compare
Signed-off-by: Bharath B <[email protected]>
bharath-b-rh
force-pushed
the
eso-101
branch
2 times, most recently
from
9192d65 to
ad4b6d7
Compare
| Tolerations []corev1.Toleration `json:"tolerations,omitempty"` | ||
|
|
||
| // nodeSelector is for defining the scheduling criteria using node labels. | ||
| // ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/ |
There was a problem hiding this comment.
[claude-generated] The ControllerStatus struct change from 'name,omitempty' to 'name' is correct since name is a required field, but ensure this doesn't break existing instances during upgrades.
| Name string `json:"name"` | ||
| } | ||
|
|
||
| // CommonConfigs are the common configurations available for all the operands managed by the operator. |
There was a problem hiding this comment.
[claude-generated] Good refactoring to extract CommonConfigs into a shared type. This reduces duplication between ApplicationConfig and GlobalConfig.
| // +kubebuilder:validation:Optional | ||
| Resources *corev1.ResourceRequirements `json:"resources,omitempty"` | ||
|
|
||
| // affinity is for setting scheduling affinity rules. |
There was a problem hiding this comment.
[claude-generated] The Mode type definition is clean and well-documented. Consider adding validation to prevent empty string values.
| HELM ?= $(LOCALBIN)/helm | ||
| REFERENCE_DOC_GENERATOR ?= $(LOCALBIN)/crd-ref-docs | ||
| GOVULNCHECK ?= $(LOCALBIN)/govulncheck | ||
| GINKGO ?= $(LOCALBIN)/ginkgo |
There was a problem hiding this comment.
[claude-generated] Good documentation of the ignored vulnerabilities with links to vulnerability details. The added vulnerabilities (GO-2025-3956, GO-2025-3915) appear to be in vendored dependencies, which is appropriate to ignore if they don't affect the operator code.
|
|
||
| .PHONY: ginkgo | ||
| ginkgo: $(LOCALBIN) ## Download ginkgo locally if necessary. | ||
| $(call go-install-tool,$(GINKGO),github.com/onsi/ginkgo/v2/ginkgo) |
There was a problem hiding this comment.
[claude-generated] The test structure changes properly separate API integration tests from unit tests. The test-apis target using ginkgo is well-implemented for running focused API validation tests.
There was a problem hiding this comment.
/lgtm
For https://pkg.go.dev/vuln/GO-2025-3915 and other vulnerabilities that we can fix, we can create another PR
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=63 | ||
| // +kubebuilder:validation:Optional | ||
| OperatingNamespace string `json:"operatingNamespace,omitempty"` |
There was a problem hiding this comment.
As mentioned in above claude comment, would it be better to validate OperatingNamespace as per RFC 1123 ?
There was a problem hiding this comment.
operatingNamespace is as per the RFC 1123 DNS Labels, which allows max of 63 characters for namespace.
| if exist && recon { | ||
| r.eventRecorder.Eventf(es, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s clusterrole resource already exists, maybe from previous installation", clusterRoleName) | ||
| r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s clusterrole resource already exists, maybe from previous installation", clusterRoleName) | ||
| } | ||
| if exist && common.HasObjectChanged(obj, fetched) { | ||
| r.log.V(1).Info("clusterrole has been modified, updating to desired state", "name", clusterRoleName) | ||
| if err := r.UpdateWithRetry(r.ctx, obj); err != nil { | ||
| return common.FromClientError(err, "failed to update %s clusterrole resource", clusterRoleName) | ||
| } | ||
| r.eventRecorder.Eventf(es, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s reconciled back to desired state", clusterRoleName) | ||
| r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s reconciled back to desired state", clusterRoleName) | ||
| } else { | ||
| r.log.V(4).Info("clusterrole resource already exists and is in expected state", "name", clusterRoleName) | ||
| } | ||
| if !exist { | ||
| if err := r.Create(r.ctx, obj); err != nil { | ||
| return common.FromClientError(err, "failed to create %s clusterrole resource", clusterRoleName) | ||
| } | ||
| r.eventRecorder.Eventf(es, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s created", clusterRoleName) | ||
| r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s created", clusterRoleName) | ||
| } |
There was a problem hiding this comment.
@bharath-b-rh is the above code-rabbit comment true?
| } | ||
|
|
||
| if ifExists, err := r.UncachedClient.Exists(r.ctx, namespacedName, object); err != nil { | ||
| ifExists, err := r.UncachedClient.Exists(r.ctx, namespacedName, object) |
There was a problem hiding this comment.
As we are thinking of periodic reconciliation, do we need to use the UncachedClient here?
There was a problem hiding this comment.
We are not caching the Issuer/ClusterIssuer objects, hence uncachedClient is used.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
Makefile (1)
133-134: Fix the test exclusion regex.
grep -vE 'test/[e2e|apis|utils]'still uses a character class and ends up filtering unrelated packages (any path starting with those characters), so most tests are skipped. Please switch to a proper grouped alternation.-test-unit: vet ## Run unit tests. - go test $$(go list ./... | grep -vE 'test/[e2e|apis|utils]') -coverprofile cover.out +test-unit: vet ## Run unit tests. + go test $$(go list ./... | grep -Ev '/test/(e2e|apis|utils)($|/)') -coverprofile cover.outbundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
345-350: Minor: Description grammar could be improved.The description text has minor grammatical issues flagged in past reviews, though this doesn't affect functionality.
Consider this revision:
- description: External Secrets Operator for Red Hat OpenShift deploys and manages - `external-secrets` application in OpenShift clusters. `external-secrets` provides - an uniform 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 from 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 secret retrieval details.Based on past review comments.
pkg/controller/external_secrets/certificate_test.go (1)
469-487: Replace hardcoded namespace with test constant.The fixture sets
OperatingNamespace: "test-ns"(line 480), but test mocks and assertions usecommontest.TestExternalSecretsNamespace. This mismatch can cause false negatives in tests.Apply this diff:
func testExternalSecretsConfigForCertificate() *v1alpha1.ExternalSecretsConfig { esc := commontest.TestExternalSecretsConfig() esc.Spec = v1alpha1.ExternalSecretsConfigSpec{ ControllerConfig: v1alpha1.ControllerConfig{ CertProvider: &v1alpha1.CertProvidersConfig{ CertManager: &v1alpha1.CertManagerConfig{ IssuerRef: &v1alpha1.ObjectReference{}, }, }, }, ApplicationConfig: v1alpha1.ApplicationConfig{ - OperatingNamespace: "test-ns", + OperatingNamespace: commontest.TestExternalSecretsNamespace, }, Plugins: v1alpha1.PluginsConfig{ BitwardenSecretManagerProvider: &v1alpha1.BitwardenSecretManagerProvider{}, }, } return esc }Based on past review comments.
api/v1alpha1/external_secrets_config_types.go (1)
88-89: Fix kubebuilder marker syntax throughout the file.Multiple kubebuilder markers still use
:=instead of=, which will prevent controller-gen from parsing them correctly. Past reviews flagged these issues, but they remain in the current code.Apply these fixes:
// operatingNamespace -// +kubebuilder:validation:MinLength:=1 -// +kubebuilder:validation:MaxLength:=63 +// +kubebuilder:validation:MinLength=1 +// +kubebuilder:validation:MaxLength=63 // Labels map -// +kubebuilder:validation:MinProperties:=0 -// +kubebuilder:validation:MaxProperties:=20 +// +kubebuilder:validation:MinProperties=0 +// +kubebuilder:validation:MaxProperties=20 // periodicReconcileInterval -// +kubebuilder:default:=300 -// +kubebuilder:validation:Minimum:=120 -// +kubebuilder:validation:Maximum:=18000 +// +kubebuilder:default=300 +// +kubebuilder:validation:Minimum=120 +// +kubebuilder:validation:Maximum=18000 // BitwardenSecretManagerProvider Mode -// +kubebuilder:validation:Enum:=Enabled;Disabled -// +kubebuilder:default:=Disabled +// +kubebuilder:validation:Enum=Enabled;Disabled +// +kubebuilder:default=Disabled // CertificateCheckInterval -// +kubebuilder:default:="5m" +// +kubebuilder:default="5m" // CertManagerConfig Mode -// +kubebuilder:validation:Enum:=Enabled;Disabled +// +kubebuilder:validation:Enum=Enabled;Disabled // InjectAnnotations -// +kubebuilder:validation:Enum:="true";"false" -// +kubebuilder:default:="false" +// +kubebuilder:validation:Enum="true";"false" +// +kubebuilder:default="false" // CertificateDuration -// +kubebuilder:default:="8760h" +// +kubebuilder:default="8760h" // CertificateRenewBefore -// +kubebuilder:default:="30m" +// +kubebuilder:default="30m"After fixing, regenerate CRDs with
make manifeststo ensure controller-gen succeeds.Based on past review comments.
Also applies to: 110-111, 118-120, 130-131, 145-145, 159-160, 166-167, 182-183, 186-187
pkg/controller/external_secrets/deployments.go (3)
245-247: Fix duplicate field path in affinity validation.The field path construction results in
spec.affinity.affinitybecausefield.NewPath("spec", "affinity")is passed and the helper appends.Child("affinity")again.Apply this fix:
- if err := validateAffinityRules(affinity, field.NewPath("spec", "affinity")); err != nil { + if err := validateAffinityRules(affinity, field.NewPath("spec")); err != nil {Based on past review comments.
267-269: Fix duplicate field path in tolerations validation.Same issue as affinity: the path becomes
spec.tolerations.tolerations.Apply this fix:
- if err := validateTolerationsConfig(tolerations, field.NewPath("spec", "tolerations")); err != nil { + if err := validateTolerationsConfig(tolerations, field.NewPath("spec")); err != nil {Based on past review comments.
131-134: Version label uses env var name literal, not the value.Line 132 sets the version label to the constant
bitwardenImageVersionEnvVarName(the env var key name) instead of reading the environment variable's value.Apply this fix:
case bitwardenDeploymentAssetName: - deployment.Labels["app.kubernetes.io/version"] = os.Getenv(bitwardenImageVersionEnvVarName) + deployment.Labels["app.kubernetes.io/version"] = os.Getenv(bitwardenImageVersionEnvVarName)Wait, looking at the code again - line 132 in the annotated code shows:
deployment.Labels["app.kubernetes.io/version"] = os.Getenv(bitwardenImageVersionEnvVarName)This IS calling
os.Getenv(), so the past review comment may have been addressed. Let me re-read...Actually, the past review comment stated the issue was on lines 131-134, and said it was setting to
bitwardenImageVersionEnvVarNamedirectly withoutos.Getenv. But the current code (line 132) showsos.Getenv(bitwardenImageVersionEnvVarName), which is correct.This appears to have been fixed already.
[actions]
Approve this as fixed.
-->pkg/controller/external_secrets/utils.go (1)
108-115: Fix the logLevel mapping to match documented semantics.With the CRD enforcing
spec.applicationConfig.logLevel∈ [1,5], the current switch turns1 → "warn",2 → "error",3 → "info", and only4/5 → "debug". Users dialing up the verbosity actually downgrade the operator to error-only logging, which is the opposite of the documented “[Kubernetes logging guidelines]” behaviour. Please realign this mapping (or the validation/doc) so that each allowed value produces the intended zap level—e.g. treat1as info and progressively more verbose for higher numbers—and keep the fallback for out-of-range inputs.
🧹 Nitpick comments (1)
go.mod (1)
9-9: Prefer maintained YAML helper instead of ghodss
github.com/ghodss/yamlhas been archived for years;sigs.k8s.io/yamlis the maintained drop-in replacement already in the tree. Please switch to the maintained module instead of introducing another direct dependency on an unmaintained fork.
📜 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 (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (14)
Makefile(5 hunks)api/v1alpha1/external_secrets_config_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(8 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(8 hunks)bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml(8 hunks)config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml(8 hunks)config/manager/manager.yaml(1 hunks)docs/api_reference.md(10 hunks)go.mod(3 hunks)pkg/controller/external_secrets/certificate.go(3 hunks)pkg/controller/external_secrets/certificate_test.go(13 hunks)pkg/controller/external_secrets/deployments.go(13 hunks)pkg/controller/external_secrets/install_external_secrets.go(3 hunks)pkg/controller/external_secrets/utils.go(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/api_reference.md
35-35: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
110-110: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
112-112: Bare URL used
(MD034, no-bare-urls)
113-113: Bare URL used
(MD034, no-bare-urls)
354-354: Bare URL used
(MD034, no-bare-urls)
355-355: Bare URL used
(MD034, no-bare-urls)
356-356: Bare URL used
(MD034, no-bare-urls)
357-357: Bare URL used
(MD034, no-bare-urls)
381-381: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (9)
config/manager/manager.yaml (1)
97-97: LGTM: imagePullPolicy addition is reasonable.Adding
imagePullPolicy: Alwaysensures the latest image is pulled, which is appropriate for development and CI environments. For production deployments, operators may override this via Kustomize or OLM bundle configurations.pkg/controller/external_secrets/install_external_secrets.go (2)
55-58: LGTM: Error handling fixed.Namespace creation errors now return immediately (line 57), preventing cascading failures as recommended in past reviews.
107-123: LGTM: Simplified namespace creation logic.The previous Get-Then-Update approach that could panic with nil pointers (flagged in past reviews) has been replaced with a simpler Create-only flow that properly handles
AlreadyExistserrors.bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
198-198: LGTM: CSV correctly updated for API rename.All references to the API resource have been consistently updated from
ExternalSecretstoExternalSecretsConfig, including:
- Example manifests (line 198)
- CRD ownership declarations (lines 287-290)
- RBAC resource names (lines 516, 526, 533)
Also applies to: 287-290, 516-516, 526-527, 533-533
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (3)
7-24: LGTM: CRD metadata correctly reflects API rename.The CRD definition properly establishes:
- Resource name:
externalsecretsconfigs.operator.openshift.io- Short names:
esc,externalsecretsconfig,esconfig- Categories for discoverability
- Printer columns for
kubectl getoutputAlso applies to: 27-36
1258-1264: LGTM: Conditional validation for issuerRef.The x-kubernetes-validations correctly enforce that
issuerRefis required only when cert-manager mode is Enabled, addressing the concern from past reviews about unconditional requirements.
1-1419: Regenerate CRD after fixing kubebuilder markers.This generated CRD file will need to be regenerated (via
make manifestsor similar) after fixing the kubebuilder marker syntax issues inapi/v1alpha1/external_secrets_config_types.goto ensure all validation constraints are properly included.pkg/controller/external_secrets/deployments.go (2)
78-78: LGTM: Proper use of controller-runtime client.The switch to
client.ObjectKeyFromObject(deployment)correctly leverages controller-runtime utilities for object key construction.
131-134: LGTM: Version label correctly reads environment variable.The code properly uses
os.Getenv(bitwardenImageVersionEnvVarName)to read the version from the environment variable, addressing the concern from past reviews.
|
@bharath-b-rh: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
/label px-approved |
openshift-ci
bot
added
px-approved
|
@emmajiafan Adding qe-approved label to unblock other PRs, since one round of pre-merge tests were performed. |
openshift-ci
bot
added
the
qe-approved
|
@bharath-b-rh: This pull request references ESO-101 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. |
|
/cherrypick release-1.0 |
|
@bharath-b-rh: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@bharath-b-rh: new pull request created: #69 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…etsConfig (openshift#54) * ESO-101: Adds vendor packages required for API integration tests Signed-off-by: Bharath B <[email protected]> * ESO-101: Adds API integration test suite Signed-off-by: Bharath B <[email protected]> * ESO-101: Restructures and renames ExternalSecrets API to ExternalSecretsConfig Signed-off-by: Bharath B <[email protected]> * ESO-101: incorporate AI code review suggestions * ESO-101: fixes GO-2025-3915 vulnerability Signed-off-by: Bharath B <[email protected]> * ESO-101: Removes default value from required type field * ESO-101: Uses logLevel configured in ExternalSecretsManager object too --------- Signed-off-by: Bharath B <[email protected]>
6 participants
PR has following changes
externalsecretsconfigs.operator.openshift.ioandexternalsecretsmanagers.operator.openshift.ioAPIs with changes proposed in ESO-101: Revisits and restructures external-secrets-operator APIs for GA enhancements#1835 for GA.