From 9994b641408c981e4725fb67d904f71a030c9773 Mon Sep 17 00:00:00 2001 From: siddhi bhor Date: Thu, 8 Jan 2026 12:23:24 +0530 Subject: [PATCH 1/6] Adds the implementation changes for annotations # Conflicts: # api/v1alpha1/external_secrets_config_types.go # api/v1alpha1/zz_generated.deepcopy.go # bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml # config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml # docs/api_reference.md # Conflicts: # pkg/controller/external_secrets/deployments_test.go --- pkg/controller/common/utils.go | 28 ++++++++++- .../external_secrets/certificate.go | 8 ++++ .../external_secrets/deployments.go | 47 +++++++++++++++++++ .../install_external_secrets.go | 11 ++++- .../external_secrets/networkpolicy.go | 8 ++++ pkg/controller/external_secrets/rbacs.go | 42 +++++++++++++++-- pkg/controller/external_secrets/secret.go | 9 ++++ .../external_secrets/serviceaccounts.go | 8 ++++ pkg/controller/external_secrets/services.go | 8 ++++ .../external_secrets/validatingwebhook.go | 9 ++++ 10 files changed, 173 insertions(+), 5 deletions(-) diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index 029daf712..833fadf46 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -71,6 +71,19 @@ func UpdateResourceLabels(obj client.Object, labels map[string]string) { obj.SetLabels(l) } +// UpdateResourceAnnotations merges provided annotations into the object's existing annotations. +// User-provided annotations take precedence over existing ones. +func UpdateResourceAnnotations(obj client.Object, annotations map[string]string) { + a := obj.GetAnnotations() + if a == nil { + a = make(map[string]string, len(annotations)) + } + for k, v := range annotations { + a[k] = v + } + obj.SetAnnotations(a) +} + func DecodeCertificateObjBytes(objBytes []byte) *certmanagerv1.Certificate { obj, err := runtime.Decode(codecs.UniversalDecoder(certmanagerv1.SchemeGroupVersion), objBytes) if err != nil { @@ -193,7 +206,15 @@ func HasObjectChanged(desired, fetched client.Object) bool { } func ObjectMetadataModified(desired, fetched client.Object) bool { - return !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) + // Check if labels have changed + if !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) { + return true + } + // Check if annotations have changed + if !reflect.DeepEqual(desired.GetAnnotations(), fetched.GetAnnotations()) { + return true + } + return false } func certificateSpecModified(desired, fetched *certmanagerv1.Certificate) bool { @@ -220,6 +241,11 @@ func deploymentSpecModified(desired, fetched *appsv1.Deployment) bool { return true } + // Check pod template annotations + if desired.Spec.Template.Annotations != nil && !reflect.DeepEqual(desired.Spec.Template.Annotations, fetched.Spec.Template.Annotations) { + return true + } + // Check volumes if !volumesEqual(desired.Spec.Template.Spec.Volumes, fetched.Spec.Template.Spec.Volumes) { return true diff --git a/pkg/controller/external_secrets/certificate.go b/pkg/controller/external_secrets/certificate.go index f68c3cfb0..c10a32625 100644 --- a/pkg/controller/external_secrets/certificate.go +++ b/pkg/controller/external_secrets/certificate.go @@ -90,6 +90,14 @@ func (r *Reconciler) getCertificateObject(esc *operatorv1alpha1.ExternalSecretsC updateNamespace(certificate, esc) common.UpdateResourceLabels(certificate, resourceLabels) + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(certificate, annotationsMap) + } + } + if err := r.updateCertificateParams(esc, certificate); err != nil { return nil, common.NewIrrecoverableError(err, "failed to update certificate resource for %s/%s deployment", getNamespace(esc), esc.GetName()) } diff --git a/pkg/controller/external_secrets/deployments.go b/pkg/controller/external_secrets/deployments.go index f66e7d11f..ea659e6b8 100644 --- a/pkg/controller/external_secrets/deployments.go +++ b/pkg/controller/external_secrets/deployments.go @@ -2,6 +2,7 @@ package external_secrets import ( "fmt" + "github.com/go-logr/logr" "os" "unsafe" @@ -103,6 +104,15 @@ func (r *Reconciler) getDeploymentObject(assetName string, esc *operatorv1alpha1 common.UpdateResourceLabels(deployment, resourceLabels) updatePodTemplateLabels(deployment, resourceLabels) + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(deployment, annotationsMap) + updatePodTemplateAnnotations(deployment, annotationsMap) + } + } + 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()) @@ -163,6 +173,43 @@ func updatePodTemplateLabels(deployment *appsv1.Deployment, labels map[string]st deployment.Spec.Template.SetLabels(l) } +// convertAnnotationsToMap converts []Annotation to map[string]string with reserved prefix filtering. +func convertAnnotationsToMap(annotations []operatorv1alpha1.Annotation, logger logr.Logger) map[string]string { + result := make(map[string]string, len(annotations)) + + reservedPrefixes := []string{"kubernetes.io/", "app.kubernetes.io/", "openshift.io/", "k8s.io/"} + + for _, ann := range annotations { + isReserved := false + for _, prefix := range reservedPrefixes { + if hasPrefix := len(ann.Key) >= len(prefix) && ann.Key[:len(prefix)] == prefix; hasPrefix { + isReserved = true + logger.V(1).Info("skipping annotation with reserved prefix", + "key", ann.Key, "prefix", prefix) + break + } + } + + if !isReserved && ann.Key != "" { + result[ann.Key] = ann.Value + } + } + + return result +} + +// updatePodTemplateAnnotations sets annotations on the pod template spec. +func updatePodTemplateAnnotations(deployment *appsv1.Deployment, annotations map[string]string) { + a := deployment.Spec.Template.GetAnnotations() + if a == nil { + a = make(map[string]string, len(annotations)) + } + for k, v := range annotations { + a[k] = v + } + deployment.Spec.Template.SetAnnotations(a) +} + func updateContainerSecurityContext(container *corev1.Container) { container.SecurityContext = &corev1.SecurityContext{ AllowPrivilegeEscalation: ptr.To(false), diff --git a/pkg/controller/external_secrets/install_external_secrets.go b/pkg/controller/external_secrets/install_external_secrets.go index f60ce85e7..aa2191edd 100644 --- a/pkg/controller/external_secrets/install_external_secrets.go +++ b/pkg/controller/external_secrets/install_external_secrets.go @@ -118,10 +118,19 @@ func (r *Reconciler) createOrApplyNamespace(esc *operatorv1alpha1.ExternalSecret namespace := getNamespace(esc) obj := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: namespace, + Name: namespace, + Labels: resourceLabels, }, } + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + obj.Annotations = annotationsMap + } + } + if err := r.Create(r.ctx, obj); err != nil { if errors.IsAlreadyExists(err) { r.log.V(4).Info("namespace already exists", "namespace", namespace) diff --git a/pkg/controller/external_secrets/networkpolicy.go b/pkg/controller/external_secrets/networkpolicy.go index 2e735c2fe..520a56563 100644 --- a/pkg/controller/external_secrets/networkpolicy.go +++ b/pkg/controller/external_secrets/networkpolicy.go @@ -137,6 +137,14 @@ func (r *Reconciler) createOrApplyNetworkPolicyFromAsset(esc *operatorv1alpha1.E updateNamespace(networkPolicy, esc) common.UpdateResourceLabels(networkPolicy, resourceLabels) + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(networkPolicy, annotationsMap) + } + } + networkPolicyName := fmt.Sprintf("%s/%s", networkPolicy.GetNamespace(), networkPolicy.GetName()) r.log.V(4).Info("Reconciling static network policy", "name", networkPolicyName) diff --git a/pkg/controller/external_secrets/rbacs.go b/pkg/controller/external_secrets/rbacs.go index 664fac6cb..e8a46e555 100644 --- a/pkg/controller/external_secrets/rbacs.go +++ b/pkg/controller/external_secrets/rbacs.go @@ -45,7 +45,7 @@ func (r *Reconciler) createOrApplyControllerRBACResources(esc *operatorv1alpha1. controllerClusterRoleServiceBindingsAssetName, controllerClusterRoleViewAssetName, } { - clusterRoleObj := r.getClusterRoleObject(asset, resourceLabels) + clusterRoleObj := r.getClusterRoleObject(esc, asset, resourceLabels) if err := r.createOrApplyClusterRole(esc, clusterRoleObj, recon); err != nil { r.log.Error(err, "failed to reconcile controller clusterrole resources") return err @@ -82,7 +82,7 @@ func (r *Reconciler) createOrApplyCertControllerRBACResources(esc *operatorv1alp return nil } - clusterRoleObj := r.getClusterRoleObject(certControllerClusterRoleAssetName, resourceLabels) + clusterRoleObj := r.getClusterRoleObject(esc, certControllerClusterRoleAssetName, resourceLabels) if err := r.createOrApplyClusterRole(esc, clusterRoleObj, recon); err != nil { r.log.Error(err, "failed to reconcile cert-controller clusterrole resources") return err @@ -135,9 +135,18 @@ func (r *Reconciler) createOrApplyClusterRole(esc *operatorv1alpha1.ExternalSecr // getClusterRoleObject is for obtaining the content of given ClusterRole static asset, and // then updating it with desired values. -func (r *Reconciler) getClusterRoleObject(assetName string, resourceLabels map[string]string) *rbacv1.ClusterRole { +func (r *Reconciler) getClusterRoleObject(esc *operatorv1alpha1.ExternalSecretsConfig, assetName string, resourceLabels map[string]string) *rbacv1.ClusterRole { clusterRole := common.DecodeClusterRoleObjBytes(assets.MustAsset(assetName)) common.UpdateResourceLabels(clusterRole, resourceLabels) + + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(clusterRole, annotationsMap) + } + } + return clusterRole } @@ -183,6 +192,15 @@ func (r *Reconciler) getClusterRoleBindingObject(esc *operatorv1alpha1.ExternalS clusterRoleBinding := common.DecodeClusterRoleBindingObjBytes(assets.MustAsset(assetName)) clusterRoleBinding.RoleRef.Name = clusterRoleName common.UpdateResourceLabels(clusterRoleBinding, resourceLabels) + + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(clusterRoleBinding, annotationsMap) + } + } + updateServiceAccountNamespaceInRBACBindingObject[*rbacv1.ClusterRoleBinding](clusterRoleBinding, serviceAccountName, getNamespace(esc)) return clusterRoleBinding } @@ -225,6 +243,15 @@ func (r *Reconciler) getRoleObject(esc *operatorv1alpha1.ExternalSecretsConfig, role := common.DecodeRoleObjBytes(assets.MustAsset(assetName)) updateNamespace(role, esc) common.UpdateResourceLabels(role, resourceLabels) + + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(role, annotationsMap) + } + } + return role } @@ -268,6 +295,15 @@ func (r *Reconciler) getRoleBindingObject(esc *operatorv1alpha1.ExternalSecretsC roleBinding.RoleRef.Name = roleName updateNamespace(roleBinding, esc) common.UpdateResourceLabels(roleBinding, resourceLabels) + + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(roleBinding, annotationsMap) + } + } + updateServiceAccountNamespaceInRBACBindingObject[*rbacv1.RoleBinding](roleBinding, serviceAccountName, roleBinding.GetNamespace()) return roleBinding } diff --git a/pkg/controller/external_secrets/secret.go b/pkg/controller/external_secrets/secret.go index 0f673ff41..b35dec8bf 100644 --- a/pkg/controller/external_secrets/secret.go +++ b/pkg/controller/external_secrets/secret.go @@ -61,5 +61,14 @@ func (r *Reconciler) getSecretObject(esc *operatorv1alpha1.ExternalSecretsConfig updateNamespace(secret, esc) common.UpdateResourceLabels(secret, resourceLabels) + + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(secret, annotationsMap) + } + } + return secret, nil } diff --git a/pkg/controller/external_secrets/serviceaccounts.go b/pkg/controller/external_secrets/serviceaccounts.go index 14ba33f86..9cb6ec943 100644 --- a/pkg/controller/external_secrets/serviceaccounts.go +++ b/pkg/controller/external_secrets/serviceaccounts.go @@ -44,6 +44,14 @@ func (r *Reconciler) createOrApplyServiceAccounts(esc *operatorv1alpha1.External updateNamespace(desired, esc) common.UpdateResourceLabels(desired, resourceLabels) + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(desired, annotationsMap) + } + } + serviceAccountName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) r.log.V(4).Info("reconciling serviceaccount resource", "name", serviceAccountName) diff --git a/pkg/controller/external_secrets/services.go b/pkg/controller/external_secrets/services.go index cb0238651..5ae81ccda 100644 --- a/pkg/controller/external_secrets/services.go +++ b/pkg/controller/external_secrets/services.go @@ -53,6 +53,14 @@ func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.Externa updateNamespace(service, esc) common.UpdateResourceLabels(service, resourceLabels) + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(service, annotationsMap) + } + } + serviceName := fmt.Sprintf("%s/%s", service.GetNamespace(), service.GetName()) r.log.V(4).Info("Reconciling service", "name", serviceName) diff --git a/pkg/controller/external_secrets/validatingwebhook.go b/pkg/controller/external_secrets/validatingwebhook.go index d5013e9de..21cdf7753 100644 --- a/pkg/controller/external_secrets/validatingwebhook.go +++ b/pkg/controller/external_secrets/validatingwebhook.go @@ -59,6 +59,15 @@ func (r *Reconciler) getValidatingWebhookObjects(esc *operatorv1alpha1.ExternalS validatingWebhook := common.DecodeValidatingWebhookConfigurationObjBytes(assets.MustAsset(assetName)) common.UpdateResourceLabels(validatingWebhook, resourceLabels) + + // Apply custom annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + common.UpdateResourceAnnotations(validatingWebhook, annotationsMap) + } + } + 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()) } From 2f4af5ed72b09530ca964d543f69f7fab353cb5c Mon Sep 17 00:00:00 2001 From: siddhi bhor Date: Wed, 21 Jan 2026 00:35:41 +0530 Subject: [PATCH 2/6] changes as per review comments --- config/manager/kustomization.yaml | 3 +- docs/api_reference.md | 19 +-- .../external_secrets/certificate.go | 2 +- .../external_secrets/certificate_test.go | 81 +++++++++++ pkg/controller/external_secrets/configmap.go | 31 ++++- .../external_secrets/deployments.go | 29 ++-- .../install_external_secrets.go | 2 +- .../external_secrets/networkpolicy.go | 2 +- .../external_secrets/networkpolicy_test.go | 55 ++++++++ pkg/controller/external_secrets/rbacs.go | 8 +- pkg/controller/external_secrets/rbacs_test.go | 127 ++++++++++++++++++ pkg/controller/external_secrets/secret.go | 2 +- .../external_secrets/secret_test.go | 55 ++++++++ .../external_secrets/service_test.go | 74 ++++++++++ .../external_secrets/serviceaccounts.go | 2 +- .../external_secrets/serviceaccounts_test.go | 64 +++++++++ pkg/controller/external_secrets/services.go | 2 +- .../external_secrets/validatingwebhook.go | 2 +- .../validatingwebhook_test.go | 71 +++++++++- 19 files changed, 578 insertions(+), 53 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 2066b0a18..d48684dd6 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -4,8 +4,7 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization images: - name: controller - newName: openshift.io/external-secrets-operator - newTag: latest + newName: quay.io/rh-ee-sbhor/external-secrets-operator generatorOptions: disableNameSuffixHash: true configMapGenerator: diff --git a/docs/api_reference.md b/docs/api_reference.md index 11cae900c..bcb5d1e20 100644 --- a/docs/api_reference.md +++ b/docs/api_reference.md @@ -16,23 +16,6 @@ Package v1alpha1 contains API Schema definitions for the operator v1alpha1 API g -#### Annotation - - - -Annotation is for adding custom annotations to the resources created for external-secrets deployment. - - - -_Appears in:_ -- [ControllerConfig](#controllerconfig) - -| Field | Description | Default | Validation | -| --- | --- | --- | --- | -| `key` _string_ | key is the annotation key. It must follow Kubernetes qualified name syntax: an optional lowercase DNS subdomain prefix (max 253 chars) followed by '/' and a name segment (max 63 chars). The name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character. Examples: 'my-key', 'example.com/my-key'. | | MaxLength: 317
MinLength: 1
Required: \{\}
| -| `value` _string_ | value is the annotation value. It can be any string, including empty. | | Optional: \{\}
| - - #### ApplicationConfig @@ -218,7 +201,7 @@ _Appears in:_ | --- | --- | --- | --- | | `certProvider` _[CertProvidersConfig](#certprovidersconfig)_ | certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook and plugins. | | Optional: \{\}
| | `labels` _object (keys:string, values:string)_ | labels to apply to all resources created for the external-secrets operand deployment.
This field can have a maximum of 20 entries. | | MaxProperties: 20
MinProperties: 0
Optional: \{\}
| -| `annotations` _[Annotation](#annotation) array_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.
Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. | | MaxItems: 20
MinItems: 0
| +| `annotations` _object (keys:string, values:string)_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment.
The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.
Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. | | MaxProperties: 20
MinProperties: 0
Optional: \{\}
| | `networkPolicies` _[NetworkPolicy](#networkpolicy) array_ | networkPolicies specifies the list of network policy configurations
to be applied to external-secrets pods.
Each entry allows specifying a name for the generated NetworkPolicy object,
along with its full Kubernetes NetworkPolicy definition.
If this field is not provided, external-secrets components will be isolated
with deny-all network policies, which will prevent proper operation. | | MaxItems: 50
MinItems: 0
Optional: \{\}
| | `componentConfigs` _[ComponentConfig](#componentconfig) array_ | componentConfigs allows specifying deployment-level configuration overrides for individual external-secrets components. This field enables fine-grained control over deployment settings for each component independently.
Each component can only have one configuration entry. | | MaxItems: 4
MinItems: 0
Optional: \{\}
| diff --git a/pkg/controller/external_secrets/certificate.go b/pkg/controller/external_secrets/certificate.go index c10a32625..8b90072f4 100644 --- a/pkg/controller/external_secrets/certificate.go +++ b/pkg/controller/external_secrets/certificate.go @@ -92,7 +92,7 @@ func (r *Reconciler) getCertificateObject(esc *operatorv1alpha1.ExternalSecretsC // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(certificate, annotationsMap) } diff --git a/pkg/controller/external_secrets/certificate_test.go b/pkg/controller/external_secrets/certificate_test.go index 580ac7ae7..daa5ff71c 100644 --- a/pkg/controller/external_secrets/certificate_test.go +++ b/pkg/controller/external_secrets/certificate_test.go @@ -441,6 +441,87 @@ func TestCreateOrApplyCertificates(t *testing.T) { }, recon: false, }, + { + name: "certificate with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + if ns.Name == "test-issuer" { + return true, nil + } + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if cert, ok := obj.(*certmanagerv1.Certificate); ok { + // Verify annotations are applied + if cert.Annotations == nil { + t.Error("certificate annotations should not be nil") + return nil + } + if cert.Annotations["cert-manager.io/issue-temporary-certificate"] != "true" { + t.Errorf("expected annotation 'cert-manager.io/issue-temporary-certificate'='true', got '%s'", + cert.Annotations["cert-manager.io/issue-temporary-certificate"]) + } + } + return nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + if ns.Name == "test-issuer" { + testIssuer().DeepCopyInto(obj.(*certmanagerv1.Issuer)) + return nil + } + return fmt.Errorf("object not found") + }) + }, + esc: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.CertProvider.CertManager.Mode = v1alpha1.Enabled + esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = "test-issuer" + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "cert-manager.io/issue-temporary-certificate": "true", + "team/owner": "security", + } + }, + recon: false, + }, + { + name: "certificate filters reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + if ns.Name == "test-issuer" { + return true, nil + } + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if cert, ok := obj.(*certmanagerv1.Certificate); ok { + // Verify only allowed annotation is present + if cert.Annotations["allowed-cert-annotation"] != "value" { + t.Errorf("expected 'allowed-cert-annotation'") + } + // Verify reserved prefixes were filtered + if _, exists := cert.Annotations["app.kubernetes.io/component"]; exists { + t.Error("reserved prefix 'app.kubernetes.io/' should have been filtered") + } + } + return nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + if ns.Name == "test-issuer" { + testIssuer().DeepCopyInto(obj.(*certmanagerv1.Issuer)) + return nil + } + return fmt.Errorf("object not found") + }) + }, + esc: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.CertProvider.CertManager.Mode = v1alpha1.Enabled + esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = "test-issuer" + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "allowed-cert-annotation": "value", + "app.kubernetes.io/component": "filtered", + } + }, + recon: false, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/configmap.go b/pkg/controller/external_secrets/configmap.go index e8ec24336..31927424a 100644 --- a/pkg/controller/external_secrets/configmap.go +++ b/pkg/controller/external_secrets/configmap.go @@ -15,7 +15,7 @@ import ( // in the operand namespace when proxy configuration is present. The ConfigMap is labeled // with the injection label required by the Cluster Network Operator (CNO), which watches // for this label and injects the cluster's trusted CA bundle into the ConfigMap's data. -// This function ensures the correct labels are present so that CNO can manage the CA bundle +// This function ensures the correct Testinglabels are present so that CNO can manage the CA bundle // content as expected. func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.ExternalSecretsConfig, resourceLabels map[string]string) error { proxyConfig := r.getProxyConfiguration(esc) @@ -39,6 +39,14 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern }, } + // Apply annotations from ControllerConfig + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) + if len(annotationsMap) > 0 { + desiredConfigMap.Annotations = annotationsMap + } + } + configMapName := fmt.Sprintf("%s/%s", desiredConfigMap.GetNamespace(), desiredConfigMap.GetName()) r.log.V(4).Info("reconciling trusted CA bundle ConfigMap resource", "name", configMapName) @@ -58,17 +66,26 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern return nil } - // ConfigMap exists, ensure it has the correct labels + // ConfigMap exists, ensure it has the correct labels and annotations // 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. + // Check if metadata (labels/annotations) has been modified. + // NOTE: CNO adds `openshift.io/owning-component: Networking / cluster-network-operator` annotations on this ConfigMap. + // validateAndFilterAnnotations function filters out reserved prefixes like openshift.io/, so we won't overwrite CNO's annotations. 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 + // Update the labels existingConfigMap.Labels = desiredConfigMap.Labels + // Merge annotations - preserve existing annotations that are not managed by us + if desiredConfigMap.Annotations != nil { + if existingConfigMap.Annotations == nil { + existingConfigMap.Annotations = make(map[string]string) + } + for k, v := range desiredConfigMap.Annotations { + existingConfigMap.Annotations[k] = v + } + } + if err := r.UpdateWithRetry(r.ctx, existingConfigMap); err != nil { return common.FromClientError(err, "failed to update %s trusted CA bundle ConfigMap resource", configMapName) } diff --git a/pkg/controller/external_secrets/deployments.go b/pkg/controller/external_secrets/deployments.go index ea659e6b8..075259755 100644 --- a/pkg/controller/external_secrets/deployments.go +++ b/pkg/controller/external_secrets/deployments.go @@ -8,6 +8,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/apis/core" @@ -106,7 +107,7 @@ func (r *Reconciler) getDeploymentObject(assetName string, esc *operatorv1alpha1 // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(deployment, annotationsMap) updatePodTemplateAnnotations(deployment, annotationsMap) @@ -173,25 +174,35 @@ func updatePodTemplateLabels(deployment *appsv1.Deployment, labels map[string]st deployment.Spec.Template.SetLabels(l) } -// convertAnnotationsToMap converts []Annotation to map[string]string with reserved prefix filtering. -func convertAnnotationsToMap(annotations []operatorv1alpha1.Annotation, logger logr.Logger) map[string]string { - result := make(map[string]string, len(annotations)) +// validateAndFilterAnnotations validates annotations using Kubernetes validation and filters out reserved prefixes. +func validateAndFilterAnnotations(annotations map[string]string, logger logr.Logger) map[string]string { + if len(annotations) == 0 { + return annotations + } + // Validate annotations using Kubernetes built-in validation + if errs := apivalidation.ValidateAnnotations(annotations, field.NewPath("annotations")); len(errs) > 0 { + logger.Error(errs.ToAggregate(), "invalid annotations detected, skipping all annotations") + return make(map[string]string) + } + + // Filter reserved prefixes + result := make(map[string]string, len(annotations)) reservedPrefixes := []string{"kubernetes.io/", "app.kubernetes.io/", "openshift.io/", "k8s.io/"} - for _, ann := range annotations { + for key, value := range annotations { isReserved := false for _, prefix := range reservedPrefixes { - if hasPrefix := len(ann.Key) >= len(prefix) && ann.Key[:len(prefix)] == prefix; hasPrefix { + if len(key) >= len(prefix) && key[:len(prefix)] == prefix { isReserved = true logger.V(1).Info("skipping annotation with reserved prefix", - "key", ann.Key, "prefix", prefix) + "key", key, "prefix", prefix) break } } - if !isReserved && ann.Key != "" { - result[ann.Key] = ann.Value + if !isReserved { + result[key] = value } } diff --git a/pkg/controller/external_secrets/install_external_secrets.go b/pkg/controller/external_secrets/install_external_secrets.go index aa2191edd..c738effd5 100644 --- a/pkg/controller/external_secrets/install_external_secrets.go +++ b/pkg/controller/external_secrets/install_external_secrets.go @@ -125,7 +125,7 @@ func (r *Reconciler) createOrApplyNamespace(esc *operatorv1alpha1.ExternalSecret // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { obj.Annotations = annotationsMap } diff --git a/pkg/controller/external_secrets/networkpolicy.go b/pkg/controller/external_secrets/networkpolicy.go index 520a56563..527312ab0 100644 --- a/pkg/controller/external_secrets/networkpolicy.go +++ b/pkg/controller/external_secrets/networkpolicy.go @@ -139,7 +139,7 @@ func (r *Reconciler) createOrApplyNetworkPolicyFromAsset(esc *operatorv1alpha1.E // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(networkPolicy, annotationsMap) } diff --git a/pkg/controller/external_secrets/networkpolicy_test.go b/pkg/controller/external_secrets/networkpolicy_test.go index 55eab12ef..84f588d38 100644 --- a/pkg/controller/external_secrets/networkpolicy_test.go +++ b/pkg/controller/external_secrets/networkpolicy_test.go @@ -181,6 +181,61 @@ func TestCreateOrApplyStaticNetworkPolicies(t *testing.T) { }, wantErr: "failed to update network policy external-secrets/deny-all-traffic: test client error", }, + { + name: "network policy with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if np, ok := obj.(*networkingv1.NetworkPolicy); ok { + // Verify annotations are applied + if np.Annotations == nil { + t.Error("networkpolicy annotations should not be nil") + return nil + } + if np.Annotations["security/policy-type"] != "deny-all" { + t.Errorf("expected annotation 'security/policy-type'='deny-all', got '%s'", + np.Annotations["security/policy-type"]) + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "security/policy-type": "deny-all", + "team/owner": "security", + } + }, + }, + { + name: "network policy filters reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if np, ok := obj.(*networkingv1.NetworkPolicy); ok { + // Verify only allowed annotation is present + if np.Annotations["custom-policy"] != "value" { + t.Errorf("expected 'custom-policy' annotation") + } + // Verify reserved prefixes were filtered + if _, exists := np.Annotations["kubernetes.io/test"]; exists { + t.Error("reserved prefix 'kubernetes.io/' should have been filtered") + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "custom-policy": "value", + "kubernetes.io/test": "filtered", + } + }, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/rbacs.go b/pkg/controller/external_secrets/rbacs.go index e8a46e555..adfd7d659 100644 --- a/pkg/controller/external_secrets/rbacs.go +++ b/pkg/controller/external_secrets/rbacs.go @@ -141,7 +141,7 @@ func (r *Reconciler) getClusterRoleObject(esc *operatorv1alpha1.ExternalSecretsC // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(clusterRole, annotationsMap) } @@ -195,7 +195,7 @@ func (r *Reconciler) getClusterRoleBindingObject(esc *operatorv1alpha1.ExternalS // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(clusterRoleBinding, annotationsMap) } @@ -246,7 +246,7 @@ func (r *Reconciler) getRoleObject(esc *operatorv1alpha1.ExternalSecretsConfig, // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(role, annotationsMap) } @@ -298,7 +298,7 @@ func (r *Reconciler) getRoleBindingObject(esc *operatorv1alpha1.ExternalSecretsC // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(roleBinding, annotationsMap) } diff --git a/pkg/controller/external_secrets/rbacs_test.go b/pkg/controller/external_secrets/rbacs_test.go index 56c565f51..5bc95ddc9 100644 --- a/pkg/controller/external_secrets/rbacs_test.go +++ b/pkg/controller/external_secrets/rbacs_test.go @@ -297,6 +297,133 @@ func TestCreateOrApplyRBACResource(t *testing.T) { { name: "rolebindings creation successful", }, + { + name: "clusterrole with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if cr, ok := obj.(*rbacv1.ClusterRole); ok { + // Verify annotations are applied + if cr.Annotations != nil { + if cr.Annotations["rbac/managed-by"] != "operator" { + t.Errorf("expected annotation 'rbac/managed-by'='operator', got '%s'", + cr.Annotations["rbac/managed-by"]) + } + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "rbac/managed-by": "operator", + "team/owner": "platform", + } + }, + }, + { + name: "clusterrolebinding with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if crb, ok := obj.(*rbacv1.ClusterRoleBinding); ok { + // Verify annotations are applied + if crb.Annotations != nil { + if crb.Annotations["binding/type"] != "cluster" { + t.Errorf("expected annotation 'binding/type'='cluster'") + } + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "binding/type": "cluster", + } + }, + }, + { + name: "role with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if role, ok := obj.(*rbacv1.Role); ok { + // Verify annotations are applied + if role.Annotations != nil { + if role.Annotations["role/purpose"] != "leader-election" { + t.Errorf("expected annotation 'role/purpose'='leader-election'") + } + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "role/purpose": "leader-election", + } + }, + }, + { + name: "rolebinding with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if rb, ok := obj.(*rbacv1.RoleBinding); ok { + // Verify annotations are applied + if rb.Annotations != nil { + if rb.Annotations["binding/scope"] != "namespace" { + t.Errorf("expected annotation 'binding/scope'='namespace'") + } + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "binding/scope": "namespace", + } + }, + }, + { + name: "rbac resources filter reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + switch obj := obj.(type) { + case *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding, *rbacv1.Role, *rbacv1.RoleBinding: + // Verify reserved prefixes were filtered + metaObj := obj.(client.Object) + annotations := metaObj.GetAnnotations() + if _, exists := annotations["openshift.io/test"]; exists { + t.Error("reserved prefix 'openshift.io/' should have been filtered") + } + if annotations["allowed-rbac"] != "value" { + t.Errorf("expected 'allowed-rbac' annotation") + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "allowed-rbac": "value", + "openshift.io/test": "filtered", + } + }, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/secret.go b/pkg/controller/external_secrets/secret.go index b35dec8bf..ebf41510e 100644 --- a/pkg/controller/external_secrets/secret.go +++ b/pkg/controller/external_secrets/secret.go @@ -64,7 +64,7 @@ func (r *Reconciler) getSecretObject(esc *operatorv1alpha1.ExternalSecretsConfig // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(secret, annotationsMap) } diff --git a/pkg/controller/external_secrets/secret_test.go b/pkg/controller/external_secrets/secret_test.go index d8a9a851c..a9c7fb369 100644 --- a/pkg/controller/external_secrets/secret_test.go +++ b/pkg/controller/external_secrets/secret_test.go @@ -184,6 +184,61 @@ func TestCreateOrApplySecret(t *testing.T) { preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { }, }, + { + name: "secret with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if secret, ok := obj.(*corev1.Secret); ok { + // Verify annotations are applied + if secret.Annotations == nil { + t.Error("secret annotations should not be nil") + return nil + } + if secret.Annotations["vault.hashicorp.com/secret-type"] != "webhook-cert" { + t.Errorf("expected annotation 'vault.hashicorp.com/secret-type'='webhook-cert', got '%s'", + secret.Annotations["vault.hashicorp.com/secret-type"]) + } + } + return nil + }) + }, + esc: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "vault.hashicorp.com/secret-type": "webhook-cert", + "security/classification": "confidential", + } + }, + }, + { + name: "secret filters reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if secret, ok := obj.(*corev1.Secret); ok { + // Verify only allowed annotation is present + if secret.Annotations["allowed-secret-annotation"] != "value" { + t.Errorf("expected 'allowed-secret-annotation'") + } + // Verify reserved prefixes were filtered + if _, exists := secret.Annotations["k8s.io/test"]; exists { + t.Error("reserved prefix 'k8s.io/' should have been filtered") + } + } + return nil + }) + }, + esc: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "allowed-secret-annotation": "value", + "k8s.io/test": "filtered", + } + }, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/service_test.go b/pkg/controller/external_secrets/service_test.go index 644b6ef0b..62a043001 100644 --- a/pkg/controller/external_secrets/service_test.go +++ b/pkg/controller/external_secrets/service_test.go @@ -114,6 +114,80 @@ func TestCreateOrApplyServices(t *testing.T) { }, wantErr: `failed to create service external-secrets/external-secrets-webhook: test client error`, }, + { + name: "service with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + var capturedService *corev1.Service + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + switch svc := obj.(type) { + case *corev1.Service: + capturedService = svc.DeepCopy() + // Verify annotations are applied + if capturedService.Annotations == nil { + t.Error("service annotations should not be nil") + return nil + } + if capturedService.Annotations["prometheus.io/scrape"] != "true" { + t.Errorf("expected annotation 'prometheus.io/scrape'='true', got '%s'", + capturedService.Annotations["prometheus.io/scrape"]) + } + if capturedService.Annotations["team/owner"] != "platform" { + t.Errorf("expected annotation 'team/owner'='platform', got '%s'", + capturedService.Annotations["team/owner"]) + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "prometheus.io/scrape": "true", + "team/owner": "platform", + } + }, + }, + { + name: "service filters reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + var capturedService *corev1.Service + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + switch svc := obj.(type) { + case *corev1.Service: + capturedService = svc.DeepCopy() + // Verify only allowed annotation is present + if capturedService.Annotations == nil { + t.Error("service annotations should not be nil") + return nil + } + if capturedService.Annotations["allowed-key"] != "allowed-value" { + t.Errorf("expected 'allowed-key' annotation, got '%s'", + capturedService.Annotations["allowed-key"]) + } + // Verify reserved prefixes were filtered + if _, exists := capturedService.Annotations["kubernetes.io/test"]; exists { + t.Error("reserved prefix 'kubernetes.io/' should have been filtered") + } + if _, exists := capturedService.Annotations["app.kubernetes.io/component"]; exists { + t.Error("reserved prefix 'app.kubernetes.io/' should have been filtered") + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "allowed-key": "allowed-value", + "kubernetes.io/test": "filtered", + "app.kubernetes.io/component": "filtered", + } + }, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/serviceaccounts.go b/pkg/controller/external_secrets/serviceaccounts.go index 9cb6ec943..a4d0bbab2 100644 --- a/pkg/controller/external_secrets/serviceaccounts.go +++ b/pkg/controller/external_secrets/serviceaccounts.go @@ -46,7 +46,7 @@ func (r *Reconciler) createOrApplyServiceAccounts(esc *operatorv1alpha1.External // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(desired, annotationsMap) } diff --git a/pkg/controller/external_secrets/serviceaccounts_test.go b/pkg/controller/external_secrets/serviceaccounts_test.go index 70809fc94..6d238555f 100644 --- a/pkg/controller/external_secrets/serviceaccounts_test.go +++ b/pkg/controller/external_secrets/serviceaccounts_test.go @@ -122,6 +122,70 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { }, wantErr: "failed to create serviceaccount external-secrets/external-secrets: test client error", }, + { + name: "serviceaccount with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if sa, ok := obj.(*corev1.ServiceAccount); ok { + // Verify annotations are applied + if sa.Annotations == nil { + t.Error("serviceaccount annotations should not be nil") + return nil + } + if sa.Annotations["vault.hashicorp.com/agent-inject"] != "true" { + t.Errorf("expected annotation 'vault.hashicorp.com/agent-inject'='true', got '%s'", + sa.Annotations["vault.hashicorp.com/agent-inject"]) + } + if sa.Annotations["team/owner"] != "platform" { + t.Errorf("expected annotation 'team/owner'='platform', got '%s'", + sa.Annotations["team/owner"]) + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "vault.hashicorp.com/agent-inject": "true", + "team/owner": "platform", + } + }, + }, + { + name: "serviceaccount filters reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if sa, ok := obj.(*corev1.ServiceAccount); ok { + // Verify only allowed annotation is present + if sa.Annotations["allowed-annotation"] != "value" { + t.Errorf("expected 'allowed-annotation', got '%s'", + sa.Annotations["allowed-annotation"]) + } + // Verify reserved prefixes were filtered + if _, exists := sa.Annotations["openshift.io/test"]; exists { + t.Error("reserved prefix 'openshift.io/' should have been filtered") + } + if _, exists := sa.Annotations["k8s.io/test"]; exists { + t.Error("reserved prefix 'k8s.io/' should have been filtered") + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "allowed-annotation": "value", + "openshift.io/test": "filtered", + "k8s.io/test": "filtered", + } + }, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/services.go b/pkg/controller/external_secrets/services.go index 5ae81ccda..e3fbaf0da 100644 --- a/pkg/controller/external_secrets/services.go +++ b/pkg/controller/external_secrets/services.go @@ -55,7 +55,7 @@ func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.Externa // Apply annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(service, annotationsMap) } diff --git a/pkg/controller/external_secrets/validatingwebhook.go b/pkg/controller/external_secrets/validatingwebhook.go index 21cdf7753..5bc96148d 100644 --- a/pkg/controller/external_secrets/validatingwebhook.go +++ b/pkg/controller/external_secrets/validatingwebhook.go @@ -62,7 +62,7 @@ func (r *Reconciler) getValidatingWebhookObjects(esc *operatorv1alpha1.ExternalS // Apply custom annotations from ControllerConfig if len(esc.Spec.ControllerConfig.Annotations) > 0 { - annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(validatingWebhook, annotationsMap) } diff --git a/pkg/controller/external_secrets/validatingwebhook_test.go b/pkg/controller/external_secrets/validatingwebhook_test.go index be17bfbdd..9673371ab 100644 --- a/pkg/controller/external_secrets/validatingwebhook_test.go +++ b/pkg/controller/external_secrets/validatingwebhook_test.go @@ -20,9 +20,10 @@ var ( func TestCreateOrApplyValidatingWebhookConfiguration(t *testing.T) { tests := []struct { - name string - preReq func(*Reconciler, *fakes.FakeCtrlClient) - wantErr string + name string + preReq func(*Reconciler, *fakes.FakeCtrlClient) + updateExternalSecretsConfig func(*v1alpha1.ExternalSecretsConfig) + wantErr string }{ { name: "validatingWebhookConfiguration reconciliation successful", @@ -97,6 +98,61 @@ func TestCreateOrApplyValidatingWebhookConfiguration(t *testing.T) { }) }, }, + { + name: "validatingWebhookConfiguration with custom annotations applied successfully", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if vwc, ok := obj.(*webhook.ValidatingWebhookConfiguration); ok { + // Verify annotations are applied + if vwc.Annotations == nil { + t.Error("validatingwebhook annotations should not be nil") + return nil + } + if vwc.Annotations["monitoring/enabled"] != "true" { + t.Errorf("expected annotation 'monitoring/enabled'='true', got '%s'", + vwc.Annotations["monitoring/enabled"]) + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "monitoring/enabled": "true", + "team/owner": "platform", + } + }, + }, + { + name: "validatingWebhookConfiguration filters reserved annotation prefixes", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if vwc, ok := obj.(*webhook.ValidatingWebhookConfiguration); ok { + // Verify only allowed annotation is present + if vwc.Annotations["custom-key"] != "custom-value" { + t.Errorf("expected 'custom-key' annotation") + } + // Verify reserved prefixes were filtered + if _, exists := vwc.Annotations["kubernetes.io/test"]; exists { + t.Error("reserved prefix 'kubernetes.io/' should have been filtered") + } + } + return nil + }) + }, + updateExternalSecretsConfig: func(esc *v1alpha1.ExternalSecretsConfig) { + esc.Spec.ControllerConfig.Annotations = map[string]string{ + "custom-key": "custom-value", + "kubernetes.io/test": "filtered", + } + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -105,10 +161,13 @@ func TestCreateOrApplyValidatingWebhookConfiguration(t *testing.T) { if tt.preReq != nil { tt.preReq(r, mock) } - r.CtrlClient = mock - externalSecretsForValidateWebhook := testExternalSecretsForValidateWebhookConfiguration() + r.CtrlClient = mock + externalSecretsForValidateWebhook := testExternalSecretsForValidateWebhookConfiguration() + if tt.updateExternalSecretsConfig != nil { + tt.updateExternalSecretsConfig(externalSecretsForValidateWebhook) + } - err := r.createOrApplyValidatingWebhookConfiguration(externalSecretsForValidateWebhook, controllerDefaultResourceLabels, false) + err := r.createOrApplyValidatingWebhookConfiguration(externalSecretsForValidateWebhook, controllerDefaultResourceLabels, false) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyValidatingWebhookConfiguration() err: %v, wantErr: %v", err, tt.wantErr) } From 895f9bdbb8d5cd93accc45b9f6fcc8af06b0d479 Mon Sep 17 00:00:00 2001 From: siddhi bhor Date: Wed, 21 Jan 2026 18:44:40 +0530 Subject: [PATCH 3/6] added the validation to skip annotations with .kubernetes --- api/v1alpha1/external_secrets_config_types.go | 4 +- .../externalsecretsconfig.testsuite.yaml | 60 ++++------------- bundle.Dockerfile | 2 +- ...ecrets-operator.clusterserviceversion.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 11 ++-- bundle/metadata/annotations.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 11 ++-- config/manager/kustomization.yaml | 2 +- pkg/controller/common/utils.go | 64 ++++++++++++++++++- .../external_secrets/deployments.go | 27 +++----- .../external_secrets/serviceaccounts_test.go | 37 ++++++++--- .../external_secrets_with_annotations.yaml | 27 ++++++++ 12 files changed, 153 insertions(+), 96 deletions(-) create mode 100644 test/e2e/testdata/external_secrets_with_annotations.yaml diff --git a/api/v1alpha1/external_secrets_config_types.go b/api/v1alpha1/external_secrets_config_types.go index 6d24f518f..560149938 100644 --- a/api/v1alpha1/external_secrets_config_types.go +++ b/api/v1alpha1/external_secrets_config_types.go @@ -116,11 +116,11 @@ type ControllerConfig struct { // annotations are for adding custom annotations to all the resources created for external-secrets deployment. // The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts. - // Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. + // Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. // +kubebuilder:validation:XValidation:rule="self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\\\\/)?([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$'))",message="Annotation keys must consist of alphanumeric characters, '-', '_' or '.', starting and ending with alphanumeric, with an optional lowercase DNS subdomain prefix and '/' (e.g., 'my-key' or 'example.com/my-key')" // +kubebuilder:validation:XValidation:rule="self.all(key, !key.contains('/') || key.split('/')[0].size() <= 253)",message="Annotation key prefix (DNS subdomain) must be no more than 253 characters" // +kubebuilder:validation:XValidation:rule="self.all(key, key.contains('/') ? key.split('/')[1].size() <= 63 : key.size() <= 63)",message="Annotation key name part must be no more than 63 characters" - // +kubebuilder:validation:XValidation:rule="self.all(key, !['kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/'].exists(p, key.startsWith(p)))",message="Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" + // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" // +kubebuilder:validation:MinProperties=0 // +kubebuilder:validation:MaxProperties=20 // +kubebuilder:validation:Optional diff --git a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml index c6dea6b5c..5fcb52da3 100644 --- a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml +++ b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml @@ -520,7 +520,7 @@ tests: controllerConfig: annotations: kubernetes.io/service-account: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with app.kubernetes.io resourceName: cluster initial: | @@ -530,7 +530,7 @@ tests: controllerConfig: annotations: app.kubernetes.io/name: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with openshift.io resourceName: cluster initial: | @@ -540,7 +540,7 @@ tests: controllerConfig: annotations: openshift.io/cluster: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with k8s.io resourceName: cluster initial: | @@ -550,8 +550,8 @@ tests: controllerConfig: annotations: k8s.io/component: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" - - name: Should allow annotation key containing but not starting with kubernetes.io + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + - name: Should not allow annotation key with subdomain of kubernetes.io resourceName: cluster initial: | apiVersion: operator.openshift.io/v1alpha1 @@ -559,30 +559,8 @@ tests: spec: controllerConfig: annotations: - custom.kubernetes.io/annotation: "allowed" - expected: | - apiVersion: operator.openshift.io/v1alpha1 - kind: ExternalSecretsConfig - spec: - controllerConfig: - annotations: - custom.kubernetes.io/annotation: "allowed" - - name: Should allow annotation key with subdomain of kubernetes.io - resourceName: cluster - initial: | - apiVersion: operator.openshift.io/v1alpha1 - kind: ExternalSecretsConfig - spec: - controllerConfig: - annotations: - monitoring.kubernetes.io/enabled: "true" - expected: | - apiVersion: operator.openshift.io/v1alpha1 - kind: ExternalSecretsConfig - spec: - controllerConfig: - annotations: - monitoring.kubernetes.io/enabled: "true" + custom.kubernetes.io/annotation: "denied" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow annotation key ending with kubernetes.io resourceName: cluster initial: | @@ -599,7 +577,7 @@ tests: controllerConfig: annotations: test-kubernetes.io/value: "allowed" - - name: Should allow annotation key with k8s in the middle + - name: Should not allow annotation key with subdomain of k8s.io resourceName: cluster initial: | apiVersion: operator.openshift.io/v1alpha1 @@ -607,14 +585,8 @@ tests: spec: controllerConfig: annotations: - company.k8s.io/annotation: "allowed" - expected: | - apiVersion: operator.openshift.io/v1alpha1 - kind: ExternalSecretsConfig - spec: - controllerConfig: - annotations: - company.k8s.io/annotation: "allowed" + service.k8s.io/annotation: "denied" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow annotation key similar but not matching reserved prefix resourceName: cluster initial: | @@ -644,7 +616,7 @@ tests: allowed-annotation: "good" kubernetes.io/forbidden: "bad" another-allowed: "good" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow exactly 20 annotations (max limit) resourceName: cluster initial: | @@ -759,16 +731,6 @@ tests: controllerConfig: annotations: custom-company.io/my_annotation.name: "allowed" - - name: Should fail with annotation key exactly matching reserved prefix - resourceName: cluster - initial: | - apiVersion: operator.openshift.io/v1alpha1 - kind: ExternalSecretsConfig - spec: - controllerConfig: - annotations: - kubernetes.io/test: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not allowed" - name: Should allow annotation key that looks similar to reserved but is not resourceName: cluster initial: | diff --git a/bundle.Dockerfile b/bundle.Dockerfile index 596156331..185d9e272 100644 --- a/bundle.Dockerfile +++ b/bundle.Dockerfile @@ -6,7 +6,7 @@ LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/ LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/ LABEL operators.operatorframework.io.bundle.package.v1=external-secrets-operator LABEL operators.operatorframework.io.bundle.channels.v1=alpha -LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.0 +LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.2 LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1 LABEL operators.operatorframework.io.metrics.project_layout=go.kubebuilder.io/v4 diff --git a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml index d3d52c960..451f9e3e0 100644 --- a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml +++ b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml @@ -242,7 +242,7 @@ metadata: operatorframework.io/suggested-namespace: external-secrets-operator operators.openshift.io/valid-subscription: '["OpenShift Kubernetes Engine", "OpenShift Container Platform", "OpenShift Platform Plus"]' - operators.operatorframework.io/builder: operator-sdk-v1.39.0 + operators.operatorframework.io/builder: operator-sdk-v1.39.2 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 repository: https://github.com/openshift/external-secrets-operator support: Red Hat, Inc. diff --git a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml index 83607c738..8b2c24561 100644 --- a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml +++ b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml @@ -1171,7 +1171,7 @@ spec: description: |- annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts. - Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. + Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. maxProperties: 20 minProperties: 0 type: object @@ -1188,11 +1188,10 @@ spec: - message: Annotation key name part must be no more than 63 characters rule: 'self.all(key, key.contains(''/'') ? key.split(''/'')[1].size() <= 63 : key.size() <= 63)' - - message: Annotation keys with reserved prefixes 'kubernetes.io/', - 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not - allowed - rule: self.all(key, !['kubernetes.io/', 'app.kubernetes.io/', - 'openshift.io/', 'k8s.io/'].exists(p, key.startsWith(p))) + - message: Annotation keys containing reserved domains 'kubernetes.io/', + 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') + are not allowed + rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) certProvider: description: certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook diff --git a/bundle/metadata/annotations.yaml b/bundle/metadata/annotations.yaml index 6937cea9b..b9cc7bdcf 100644 --- a/bundle/metadata/annotations.yaml +++ b/bundle/metadata/annotations.yaml @@ -5,7 +5,7 @@ annotations: operators.operatorframework.io.bundle.metadata.v1: metadata/ operators.operatorframework.io.bundle.package.v1: external-secrets-operator operators.operatorframework.io.bundle.channels.v1: alpha - operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.0 + operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.2 operators.operatorframework.io.metrics.mediatype.v1: metrics+v1 operators.operatorframework.io.metrics.project_layout: go.kubebuilder.io/v4 diff --git a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml index 5de68e0c3..28cbc7317 100644 --- a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml +++ b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml @@ -1171,7 +1171,7 @@ spec: description: |- annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts. - Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. + Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. maxProperties: 20 minProperties: 0 type: object @@ -1188,11 +1188,10 @@ spec: - message: Annotation key name part must be no more than 63 characters rule: 'self.all(key, key.contains(''/'') ? key.split(''/'')[1].size() <= 63 : key.size() <= 63)' - - message: Annotation keys with reserved prefixes 'kubernetes.io/', - 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' are not - allowed - rule: self.all(key, !['kubernetes.io/', 'app.kubernetes.io/', - 'openshift.io/', 'k8s.io/'].exists(p, key.startsWith(p))) + - message: Annotation keys containing reserved domains 'kubernetes.io/', + 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') + are not allowed + rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) certProvider: description: certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index d48684dd6..c00208f2c 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -4,7 +4,7 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization images: - name: controller - newName: quay.io/rh-ee-sbhor/external-secrets-operator + newName: openshift.io/external-secrets-operator generatorOptions: disableNameSuffixHash: true configMapGenerator: diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index 833fadf46..66888d8c7 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "reflect" + "strings" "sync" "sync/atomic" @@ -210,18 +211,77 @@ func ObjectMetadataModified(desired, fetched client.Object) bool { if !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) { return true } - // Check if annotations have changed - if !reflect.DeepEqual(desired.GetAnnotations(), fetched.GetAnnotations()) { + // Check if annotations have changed (ignoring system-managed annotations) + desiredAnnotates := desired.GetAnnotations() + fetchedAnnotates := FilterReservedAnnotations(fetched.GetAnnotations()) + if !reflect.DeepEqual(desiredAnnotates, fetchedAnnotates) { return true } return false } +// FilterReservedAnnotations filters out Kubernetes/OpenShift reserved domain annotations. +// This function serves two purposes: +// 1. Filter user-provided annotations to prevent using reserved domains (security) +// 2. Filter system-managed annotations during comparison to prevent reconciliation loops +// +// Reserved domains: kubernetes.io, openshift.io, k8s.io (including all subdomains) +func FilterReservedAnnotations(annotations map[string]string) map[string]string { + if len(annotations) == 0 { + return annotations + } + + // Reserved domain patterns: blocks both "kubernetes.io/*" and "*.kubernetes.io/*" + reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io"} + + result := make(map[string]string, len(annotations)) + for key, value := range annotations { + isReserved := false + + for _, domain := range reservedDomains { + // Check for direct prefix: kubernetes.io/foo + if strings.HasPrefix(key, domain+"/") { + isReserved = true + break + } + + // Check for subdomain: *.kubernetes.io/foo + // Look for ".kubernetes.io/" or ".kubernetes.io" in the key + if strings.Contains(key, "."+domain+"/") || strings.Contains(key, "."+domain) { + isReserved = true + break + } + } + + if !isReserved { + result[key] = value + } + } + + return result +} + func certificateSpecModified(desired, fetched *certmanagerv1.Certificate) bool { return !reflect.DeepEqual(desired.Spec, fetched.Spec) } func deploymentSpecModified(desired, fetched *appsv1.Deployment) bool { + if desired.Labels != nil && !reflect.DeepEqual(desired.Labels, fetched.GetLabels()) { + return true + } + + if desired.Annotations != nil { + fetchedAnnots := fetched.GetAnnotations() + if fetchedAnnots == nil { + return true + } + + fetchedAnnots = FilterReservedAnnotations(fetchedAnnots) + if !reflect.DeepEqual(desired.Annotations, fetchedAnnots) { + return true + } + } + if desired.Spec.Replicas != nil && !reflect.DeepEqual(desired.Spec.Replicas, fetched.Spec.Replicas) { return true } diff --git a/pkg/controller/external_secrets/deployments.go b/pkg/controller/external_secrets/deployments.go index 075259755..2ef4b4d17 100644 --- a/pkg/controller/external_secrets/deployments.go +++ b/pkg/controller/external_secrets/deployments.go @@ -2,10 +2,10 @@ package external_secrets import ( "fmt" - "github.com/go-logr/logr" "os" "unsafe" + "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apivalidation "k8s.io/apimachinery/pkg/api/validation" @@ -174,7 +174,7 @@ func updatePodTemplateLabels(deployment *appsv1.Deployment, labels map[string]st deployment.Spec.Template.SetLabels(l) } -// validateAndFilterAnnotations validates annotations using Kubernetes validation and filters out reserved prefixes. +// validateAndFilterAnnotations validates annotations using Kubernetes validation and filters out reserved domains using the common utility function. func validateAndFilterAnnotations(annotations map[string]string, logger logr.Logger) map[string]string { if len(annotations) == 0 { return annotations @@ -186,27 +186,18 @@ func validateAndFilterAnnotations(annotations map[string]string, logger logr.Log return make(map[string]string) } - // Filter reserved prefixes - result := make(map[string]string, len(annotations)) - reservedPrefixes := []string{"kubernetes.io/", "app.kubernetes.io/", "openshift.io/", "k8s.io/"} + // Filter reserved domains kubernetes.io/*, *.kubernetes.io/*, openshift.io/*, *.openshift.io/*, etc. + filtered := common.FilterReservedAnnotations(annotations) - for key, value := range annotations { - isReserved := false - for _, prefix := range reservedPrefixes { - if len(key) >= len(prefix) && key[:len(prefix)] == prefix { - isReserved = true - logger.V(1).Info("skipping annotation with reserved prefix", - "key", key, "prefix", prefix) - break + if len(filtered) < len(annotations) { + for key := range annotations { + if _, exists := filtered[key]; !exists { + logger.V(1).Info("filtered annotation with reserved domain", "key", key) } } - - if !isReserved { - result[key] = value - } } - return result + return filtered } // updatePodTemplateAnnotations sets annotations on the pod template spec. diff --git a/pkg/controller/external_secrets/serviceaccounts_test.go b/pkg/controller/external_secrets/serviceaccounts_test.go index 6d238555f..986dc03da 100644 --- a/pkg/controller/external_secrets/serviceaccounts_test.go +++ b/pkg/controller/external_secrets/serviceaccounts_test.go @@ -155,24 +155,37 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { }, }, { - name: "serviceaccount filters reserved annotation prefixes", + name: "serviceaccount filters reserved annotation prefixes with wildcard", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { return false, nil }) m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { if sa, ok := obj.(*corev1.ServiceAccount); ok { + if sa.Annotations == nil { + t.Error("serviceaccount annotations should not be nil") + return nil + } + // Verify only allowed annotation is present if sa.Annotations["allowed-annotation"] != "value" { t.Errorf("expected 'allowed-annotation', got '%s'", sa.Annotations["allowed-annotation"]) } - // Verify reserved prefixes were filtered - if _, exists := sa.Annotations["openshift.io/test"]; exists { - t.Error("reserved prefix 'openshift.io/' should have been filtered") + + reservedKeys := []string{ + "kubernetes.io/test", + "app.kubernetes.io/component", + "deployment.kubernetes.io/revision", + "openshift.io/test", + "console.openshift.io/route", + "k8s.io/test", } - if _, exists := sa.Annotations["k8s.io/test"]; exists { - t.Error("reserved prefix 'k8s.io/' should have been filtered") + + for _, key := range reservedKeys { + if val, exists := sa.Annotations[key]; exists && val == "filtered" { + t.Errorf("reserved annotation %q should have been filtered but found with value %q", key, val) + } } } return nil @@ -180,9 +193,15 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { }, updateExternalSecretsConfig: func(esc *operatorv1alpha1.ExternalSecretsConfig) { esc.Spec.ControllerConfig.Annotations = map[string]string{ - "allowed-annotation": "value", - "openshift.io/test": "filtered", - "k8s.io/test": "filtered", + "allowed-annotation": "value", + // Direct domain patterns + "kubernetes.io/test": "filtered", + "openshift.io/test": "filtered", + "k8s.io/test": "filtered", + // Wildcard subdomain patterns (should also be filtered) + "app.kubernetes.io/component": "filtered", + "deployment.kubernetes.io/revision": "filtered", + "console.openshift.io/route": "filtered", } }, }, diff --git a/test/e2e/testdata/external_secrets_with_annotations.yaml b/test/e2e/testdata/external_secrets_with_annotations.yaml new file mode 100644 index 000000000..06251c0ce --- /dev/null +++ b/test/e2e/testdata/external_secrets_with_annotations.yaml @@ -0,0 +1,27 @@ +apiVersion: operator.openshift.io/v1alpha1 +kind: ExternalSecretsConfig +metadata: + labels: + app.kubernetes.io/name: cluster + app.kubernetes.io/managed-by: external-secrets-operator-e2e + name: cluster +spec: + controllerConfig: + annotations: + deployment.io/revision: "10" + prometheus.io/scrape: "true" + networkPolicies: + - name: allow-external-secrets-egress + componentName: ExternalSecretsCoreController + egress: + # Allow egress to Kubernetes API server, AWS endpoints, and DNS + - to: [] + ports: + - protocol: TCP + port: 6443 # Kubernetes API + - protocol: TCP + port: 443 # HTTPS (AWS Secrets Manager) + - protocol: TCP + port: 5353 # DNS + - protocol: UDP + port: 5353 # DNS From 40e20b4379c5a2b94e7a54516c81f34696794dc1 Mon Sep 17 00:00:00 2001 From: siddhi bhor Date: Thu, 29 Jan 2026 01:09:12 +0530 Subject: [PATCH 4/6] adds restrictions for cert-manager.io --- api/v1alpha1/external_secrets_config_types.go | 4 +- .../externalsecretsconfig.testsuite.yaml | 14 ++-- bundle.Dockerfile | 2 +- ...ecrets-operator.clusterserviceversion.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 8 +- bundle/metadata/annotations.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 8 +- pkg/controller/common/utils.go | 46 ++++-------- pkg/controller/common/utils_test.go | 73 +++++++++++++++++++ 9 files changed, 107 insertions(+), 52 deletions(-) create mode 100644 pkg/controller/common/utils_test.go diff --git a/api/v1alpha1/external_secrets_config_types.go b/api/v1alpha1/external_secrets_config_types.go index 560149938..be6c8e578 100644 --- a/api/v1alpha1/external_secrets_config_types.go +++ b/api/v1alpha1/external_secrets_config_types.go @@ -116,11 +116,11 @@ type ControllerConfig struct { // annotations are for adding custom annotations to all the resources created for external-secrets deployment. // The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts. - // Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. + // Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, `cert-manager.io/` or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. // +kubebuilder:validation:XValidation:rule="self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\\\\/)?([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$'))",message="Annotation keys must consist of alphanumeric characters, '-', '_' or '.', starting and ending with alphanumeric, with an optional lowercase DNS subdomain prefix and '/' (e.g., 'my-key' or 'example.com/my-key')" // +kubebuilder:validation:XValidation:rule="self.all(key, !key.contains('/') || key.split('/')[0].size() <= 253)",message="Annotation key prefix (DNS subdomain) must be no more than 253 characters" // +kubebuilder:validation:XValidation:rule="self.all(key, key.contains('/') ? key.split('/')[1].size() <= 63 : key.size() <= 63)",message="Annotation key name part must be no more than 63 characters" - // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io|cert-manager\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" // +kubebuilder:validation:MinProperties=0 // +kubebuilder:validation:MaxProperties=20 // +kubebuilder:validation:Optional diff --git a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml index 5fcb52da3..9ed3543b1 100644 --- a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml +++ b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml @@ -520,7 +520,7 @@ tests: controllerConfig: annotations: kubernetes.io/service-account: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with app.kubernetes.io resourceName: cluster initial: | @@ -530,7 +530,7 @@ tests: controllerConfig: annotations: app.kubernetes.io/name: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with openshift.io resourceName: cluster initial: | @@ -540,7 +540,7 @@ tests: controllerConfig: annotations: openshift.io/cluster: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with k8s.io resourceName: cluster initial: | @@ -550,7 +550,7 @@ tests: controllerConfig: annotations: k8s.io/component: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should not allow annotation key with subdomain of kubernetes.io resourceName: cluster initial: | @@ -560,7 +560,7 @@ tests: controllerConfig: annotations: custom.kubernetes.io/annotation: "denied" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow annotation key ending with kubernetes.io resourceName: cluster initial: | @@ -586,7 +586,7 @@ tests: controllerConfig: annotations: service.k8s.io/annotation: "denied" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow annotation key similar but not matching reserved prefix resourceName: cluster initial: | @@ -616,7 +616,7 @@ tests: allowed-annotation: "good" kubernetes.io/forbidden: "bad" another-allowed: "good" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow exactly 20 annotations (max limit) resourceName: cluster initial: | diff --git a/bundle.Dockerfile b/bundle.Dockerfile index 185d9e272..596156331 100644 --- a/bundle.Dockerfile +++ b/bundle.Dockerfile @@ -6,7 +6,7 @@ LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/ LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/ LABEL operators.operatorframework.io.bundle.package.v1=external-secrets-operator LABEL operators.operatorframework.io.bundle.channels.v1=alpha -LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.2 +LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.0 LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1 LABEL operators.operatorframework.io.metrics.project_layout=go.kubebuilder.io/v4 diff --git a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml index 451f9e3e0..d3d52c960 100644 --- a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml +++ b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml @@ -242,7 +242,7 @@ metadata: operatorframework.io/suggested-namespace: external-secrets-operator operators.openshift.io/valid-subscription: '["OpenShift Kubernetes Engine", "OpenShift Container Platform", "OpenShift Platform Plus"]' - operators.operatorframework.io/builder: operator-sdk-v1.39.2 + operators.operatorframework.io/builder: operator-sdk-v1.39.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 repository: https://github.com/openshift/external-secrets-operator support: Red Hat, Inc. diff --git a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml index 8b2c24561..5c17ce45e 100644 --- a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml +++ b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml @@ -1171,7 +1171,7 @@ spec: description: |- annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts. - Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. + Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, `cert-manager.io/` or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. maxProperties: 20 minProperties: 0 type: object @@ -1189,9 +1189,9 @@ spec: rule: 'self.all(key, key.contains(''/'') ? key.split(''/'')[1].size() <= 63 : key.size() <= 63)' - message: Annotation keys containing reserved domains 'kubernetes.io/', - 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') - are not allowed - rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) + 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including + subdomains like '*.kubernetes.io/') are not allowed + rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io|cert-manager\\.io)/')) certProvider: description: certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook diff --git a/bundle/metadata/annotations.yaml b/bundle/metadata/annotations.yaml index b9cc7bdcf..6937cea9b 100644 --- a/bundle/metadata/annotations.yaml +++ b/bundle/metadata/annotations.yaml @@ -5,7 +5,7 @@ annotations: operators.operatorframework.io.bundle.metadata.v1: metadata/ operators.operatorframework.io.bundle.package.v1: external-secrets-operator operators.operatorframework.io.bundle.channels.v1: alpha - operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.2 + operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.0 operators.operatorframework.io.metrics.mediatype.v1: metrics+v1 operators.operatorframework.io.metrics.project_layout: go.kubebuilder.io/v4 diff --git a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml index 28cbc7317..2067aeaed 100644 --- a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml +++ b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml @@ -1171,7 +1171,7 @@ spec: description: |- annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts. - Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. + Annotation keys containing domains `kubernetes.io/`, `openshift.io/`, `cert-manager.io/` or `k8s.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. maxProperties: 20 minProperties: 0 type: object @@ -1189,9 +1189,9 @@ spec: rule: 'self.all(key, key.contains(''/'') ? key.split(''/'')[1].size() <= 63 : key.size() <= 63)' - message: Annotation keys containing reserved domains 'kubernetes.io/', - 'openshift.io/', or 'k8s.io/' (including subdomains like '*.kubernetes.io/') - are not allowed - rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) + 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including + subdomains like '*.kubernetes.io/') are not allowed + rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io|cert-manager\\.io)/')) certProvider: description: certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index 66888d8c7..c91e83ed1 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -200,9 +200,11 @@ func HasObjectChanged(desired, fetched client.Object) bool { objectModified = networkPolicySpecModified(desired.(*networkingv1.NetworkPolicy), fetched.(*networkingv1.NetworkPolicy)) case *webhook.ValidatingWebhookConfiguration: objectModified = validatingWebHookSpecModified(desired.(*webhook.ValidatingWebhookConfiguration), fetched.(*webhook.ValidatingWebhookConfiguration)) + default: - panic(fmt.Sprintf("unsupported object type: %T", desired)) + return ObjectMetadataModified(desired, fetched) } + return objectModified || ObjectMetadataModified(desired, fetched) } @@ -214,6 +216,10 @@ func ObjectMetadataModified(desired, fetched client.Object) bool { // Check if annotations have changed (ignoring system-managed annotations) desiredAnnotates := desired.GetAnnotations() fetchedAnnotates := FilterReservedAnnotations(fetched.GetAnnotations()) + // Handle nil vs empty map: both are semantically equivalent + if len(desiredAnnotates) == 0 && len(fetchedAnnotates) == 0 { + return false + } if !reflect.DeepEqual(desiredAnnotates, fetchedAnnotates) { return true } @@ -221,10 +227,6 @@ func ObjectMetadataModified(desired, fetched client.Object) bool { } // FilterReservedAnnotations filters out Kubernetes/OpenShift reserved domain annotations. -// This function serves two purposes: -// 1. Filter user-provided annotations to prevent using reserved domains (security) -// 2. Filter system-managed annotations during comparison to prevent reconciliation loops -// // Reserved domains: kubernetes.io, openshift.io, k8s.io (including all subdomains) func FilterReservedAnnotations(annotations map[string]string) map[string]string { if len(annotations) == 0 { @@ -232,22 +234,15 @@ func FilterReservedAnnotations(annotations map[string]string) map[string]string } // Reserved domain patterns: blocks both "kubernetes.io/*" and "*.kubernetes.io/*" - reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io"} + reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io", "cert-manager.io"} result := make(map[string]string, len(annotations)) for key, value := range annotations { isReserved := false for _, domain := range reservedDomains { - // Check for direct prefix: kubernetes.io/foo - if strings.HasPrefix(key, domain+"/") { - isReserved = true - break - } - - // Check for subdomain: *.kubernetes.io/foo - // Look for ".kubernetes.io/" or ".kubernetes.io" in the key - if strings.Contains(key, "."+domain+"/") || strings.Contains(key, "."+domain) { + // Checks for patterns like kubernetes.io/* and *.kubernetes.io/* only + if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") { isReserved = true break } @@ -266,22 +261,6 @@ func certificateSpecModified(desired, fetched *certmanagerv1.Certificate) bool { } func deploymentSpecModified(desired, fetched *appsv1.Deployment) bool { - if desired.Labels != nil && !reflect.DeepEqual(desired.Labels, fetched.GetLabels()) { - return true - } - - if desired.Annotations != nil { - fetchedAnnots := fetched.GetAnnotations() - if fetchedAnnots == nil { - return true - } - - fetchedAnnots = FilterReservedAnnotations(fetchedAnnots) - if !reflect.DeepEqual(desired.Annotations, fetchedAnnots) { - return true - } - } - if desired.Spec.Replicas != nil && !reflect.DeepEqual(desired.Spec.Replicas, fetched.Spec.Replicas) { return true } @@ -302,7 +281,10 @@ func deploymentSpecModified(desired, fetched *appsv1.Deployment) bool { } // Check pod template annotations - if desired.Spec.Template.Annotations != nil && !reflect.DeepEqual(desired.Spec.Template.Annotations, fetched.Spec.Template.Annotations) { + if desired.Spec.Template.Annotations != nil && !reflect.DeepEqual( + desired.Spec.Template.Annotations, + FilterReservedAnnotations(fetched.Spec.Template.Annotations), + ) { return true } diff --git a/pkg/controller/common/utils_test.go b/pkg/controller/common/utils_test.go new file mode 100644 index 000000000..ddb74d20f --- /dev/null +++ b/pkg/controller/common/utils_test.go @@ -0,0 +1,73 @@ +package common + +import ( + "reflect" + "testing" + + "github.com/google/go-cmp/cmp" + appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestFilterAnnotations(t *testing.T) { + source := map[string]string{ + "kubernetes.io/foo": "bar", + "deployment.kubernetes.io/revision": "4", + + "k8s.io/foo": "bar", + "friends.k8s.io/xyz": "infinity", + + "openshift.io/foo": "p", + "services.openshift.io/apiserver": "self", + + "cert-manager.io/secret-name": "x-cert", + "acme.cert-manager.io/dns01-challenge": "dns", + } + + // the legit annotations should be retained. + retained := map[string]string{ + "platform.openshift.site/eso": "included", + "platform-team.company.io/component": "eso", + + "kubernetes.io": "foobar", + "k8s.io": "foobar", + + "app.kubernetes.io": "none", + "none.k8s.io": "foo", + + "openshift.io": "enterprise", + "beta.openshift.io": "foobar", + + "cert-manager.io": "ready", + "acme.cert-manager.io": "cert", + } + + // include all non-rejected annotations in source. + for k, v := range retained { + source[k] = v + } + + filtered := FilterReservedAnnotations(retained) + if !reflect.DeepEqual(filtered, retained) { + t.Fatalf("expected filtered annotations mismatch: %s", cmp.Diff(filtered, retained)) + } +} +func TestDeploymentObjectChanged(t *testing.T) { + x := appsv1.Deployment{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "deployment.kubernetes.io/revision": "4", + }, + }, + } + + y := appsv1.Deployment{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{}, + }, + } + + if HasObjectChanged(&y, &x) { + t.Fatal("expected mismatch") + } +} From 009f9e5dae33bca84ac5e797a4f502baed7f7a57 Mon Sep 17 00:00:00 2001 From: siddhi bhor Date: Fri, 30 Jan 2026 16:43:41 +0530 Subject: [PATCH 5/6] adds the e2e test case --- api/v1alpha1/external_secrets_config_types.go | 3 +- .../externalsecretsconfig.testsuite.yaml | 30 +++- bundle.Dockerfile | 2 +- ...ecrets-operator.clusterserviceversion.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 5 +- bundle/metadata/annotations.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 5 +- pkg/controller/common/utils.go | 21 +-- pkg/controller/common/utils_test.go | 10 +- .../external_secrets/certificate_test.go | 14 +- .../external_secrets/serviceaccounts.go | 10 +- test/e2e/e2e_test.go | 155 +++++++++++++++++- test/e2e/testdata/external_secret.yaml | 2 + .../external_secrets_with_annotations.yaml | 27 --- test/utils/conditions.go | 24 +++ 15 files changed, 252 insertions(+), 60 deletions(-) delete mode 100644 test/e2e/testdata/external_secrets_with_annotations.yaml diff --git a/api/v1alpha1/external_secrets_config_types.go b/api/v1alpha1/external_secrets_config_types.go index be6c8e578..f582b641d 100644 --- a/api/v1alpha1/external_secrets_config_types.go +++ b/api/v1alpha1/external_secrets_config_types.go @@ -120,7 +120,8 @@ type ControllerConfig struct { // +kubebuilder:validation:XValidation:rule="self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\\\\/)?([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$'))",message="Annotation keys must consist of alphanumeric characters, '-', '_' or '.', starting and ending with alphanumeric, with an optional lowercase DNS subdomain prefix and '/' (e.g., 'my-key' or 'example.com/my-key')" // +kubebuilder:validation:XValidation:rule="self.all(key, !key.contains('/') || key.split('/')[0].size() <= 253)",message="Annotation key prefix (DNS subdomain) must be no more than 253 characters" // +kubebuilder:validation:XValidation:rule="self.all(key, key.contains('/') ? key.split('/')[1].size() <= 63 : key.size() <= 63)",message="Annotation key name part must be no more than 63 characters" - // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io|cert-manager\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^(cert-manager\\\\.io)/'))",message="Annotation keys containing reserved domain 'cert-manager.io/' are not allowed" // +kubebuilder:validation:MinProperties=0 // +kubebuilder:validation:MaxProperties=20 // +kubebuilder:validation:Optional diff --git a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml index 9ed3543b1..45c0cf1ae 100644 --- a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml +++ b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml @@ -511,6 +511,34 @@ tests: annotations: custom.company.io/team: "platform" custom.company.io/app: "external-secrets" + - name: Should allow custom annotations with cert-manager subdomain + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: ExternalSecretsConfig + spec: + controllerConfig: + annotations: + nothing.cert-manager.io: "space" + something.cert-manager.io/app: "platform" + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: ExternalSecretsConfig + spec: + controllerConfig: + annotations: + nothing.cert-manager.io: "space" + something.cert-manager.io/app: "platform" + - name: Should fail with annotation key starting with cert-manager.io + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: ExternalSecretsConfig + spec: + controllerConfig: + annotations: + cert-manager.io/inject-ca-from: "self-signed-issuer" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domain 'cert-manager.io/' are not allowed" - name: Should fail with annotation key starting with kubernetes.io resourceName: cluster initial: | @@ -761,7 +789,6 @@ tests: prometheus.io/port: "8080" argocd.argoproj.io/sync-wave: "1" vault.hashicorp.com/agent-inject: "true" - cert-manager.io/cluster-issuer: "letsencrypt" expected: | apiVersion: operator.openshift.io/v1alpha1 kind: ExternalSecretsConfig @@ -772,7 +799,6 @@ tests: prometheus.io/port: "8080" argocd.argoproj.io/sync-wave: "1" vault.hashicorp.com/agent-inject: "true" - cert-manager.io/cluster-issuer: "letsencrypt" - name: Should allow annotation with empty value resourceName: cluster initial: | diff --git a/bundle.Dockerfile b/bundle.Dockerfile index 596156331..185d9e272 100644 --- a/bundle.Dockerfile +++ b/bundle.Dockerfile @@ -6,7 +6,7 @@ LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/ LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/ LABEL operators.operatorframework.io.bundle.package.v1=external-secrets-operator LABEL operators.operatorframework.io.bundle.channels.v1=alpha -LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.0 +LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.2 LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1 LABEL operators.operatorframework.io.metrics.project_layout=go.kubebuilder.io/v4 diff --git a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml index d3d52c960..451f9e3e0 100644 --- a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml +++ b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml @@ -242,7 +242,7 @@ metadata: operatorframework.io/suggested-namespace: external-secrets-operator operators.openshift.io/valid-subscription: '["OpenShift Kubernetes Engine", "OpenShift Container Platform", "OpenShift Platform Plus"]' - operators.operatorframework.io/builder: operator-sdk-v1.39.0 + operators.operatorframework.io/builder: operator-sdk-v1.39.2 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 repository: https://github.com/openshift/external-secrets-operator support: Red Hat, Inc. diff --git a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml index 5c17ce45e..1ff16179d 100644 --- a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml +++ b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml @@ -1191,7 +1191,10 @@ spec: - message: Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed - rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io|cert-manager\\.io)/')) + rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) + - message: Annotation keys containing reserved domain 'cert-manager.io/' + are not allowed + rule: self.all(key, !key.matches('^(cert-manager\\.io)/')) certProvider: description: certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook diff --git a/bundle/metadata/annotations.yaml b/bundle/metadata/annotations.yaml index 6937cea9b..b9cc7bdcf 100644 --- a/bundle/metadata/annotations.yaml +++ b/bundle/metadata/annotations.yaml @@ -5,7 +5,7 @@ annotations: operators.operatorframework.io.bundle.metadata.v1: metadata/ operators.operatorframework.io.bundle.package.v1: external-secrets-operator operators.operatorframework.io.bundle.channels.v1: alpha - operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.0 + operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.2 operators.operatorframework.io.metrics.mediatype.v1: metrics+v1 operators.operatorframework.io.metrics.project_layout: go.kubebuilder.io/v4 diff --git a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml index 2067aeaed..60fe2239c 100644 --- a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml +++ b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml @@ -1191,7 +1191,10 @@ spec: - message: Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed - rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io|cert-manager\\.io)/')) + rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) + - message: Annotation keys containing reserved domain 'cert-manager.io/' + are not allowed + rule: self.all(key, !key.matches('^(cert-manager\\.io)/')) certProvider: description: certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index c91e83ed1..90949a3b1 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -214,7 +214,9 @@ func ObjectMetadataModified(desired, fetched client.Object) bool { return true } // Check if annotations have changed (ignoring system-managed annotations) - desiredAnnotates := desired.GetAnnotations() + // Filter both desired and fetched to avoid infinite reconcile loops caused by + // annotations managed by external controllers (e.g., cert-manager.io/inject-ca-from) + desiredAnnotates := FilterReservedAnnotations(desired.GetAnnotations()) fetchedAnnotates := FilterReservedAnnotations(fetched.GetAnnotations()) // Handle nil vs empty map: both are semantically equivalent if len(desiredAnnotates) == 0 && len(fetchedAnnotates) == 0 { @@ -226,15 +228,16 @@ func ObjectMetadataModified(desired, fetched client.Object) bool { return false } -// FilterReservedAnnotations filters out Kubernetes/OpenShift reserved domain annotations. -// Reserved domains: kubernetes.io, openshift.io, k8s.io (including all subdomains) +// FilterReservedAnnotations filters out Kubernetes/OpenShift reserved domain annotations +// and annotations managed by external controllers that should not trigger reconciliation. +// Reserved domains: kubernetes.io, openshift.io, k8s.io func FilterReservedAnnotations(annotations map[string]string) map[string]string { if len(annotations) == 0 { return annotations } // Reserved domain patterns: blocks both "kubernetes.io/*" and "*.kubernetes.io/*" - reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io", "cert-manager.io"} + reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io"} result := make(map[string]string, len(annotations)) for key, value := range annotations { @@ -246,6 +249,10 @@ func FilterReservedAnnotations(annotations map[string]string) map[string]string isReserved = true break } + if strings.HasPrefix(key, "cert-manager.io/") { + isReserved = true + break + } } if !isReserved { @@ -531,12 +538,6 @@ func validatingWebHookSpecModified(desired, fetched *webhook.ValidatingWebhookCo return true } - if desiredVal, exists := desired.GetAnnotations()[CertManagerInjectCAFromAnnotation]; exists { - if desiredVal != fetched.GetAnnotations()[CertManagerInjectCAFromAnnotation] { - return true - } - } - fetchedWebhooksMap := make(map[string]webhook.ValidatingWebhook) for _, wh := range fetched.Webhooks { fetchedWebhooksMap[wh.Name] = wh diff --git a/pkg/controller/common/utils_test.go b/pkg/controller/common/utils_test.go index ddb74d20f..179866791 100644 --- a/pkg/controller/common/utils_test.go +++ b/pkg/controller/common/utils_test.go @@ -20,8 +20,7 @@ func TestFilterAnnotations(t *testing.T) { "openshift.io/foo": "p", "services.openshift.io/apiserver": "self", - "cert-manager.io/secret-name": "x-cert", - "acme.cert-manager.io/dns01-challenge": "dns", + "cert-manager.io/secret-name": "x-cert", } // the legit annotations should be retained. @@ -38,8 +37,9 @@ func TestFilterAnnotations(t *testing.T) { "openshift.io": "enterprise", "beta.openshift.io": "foobar", - "cert-manager.io": "ready", - "acme.cert-manager.io": "cert", + "cert-manager.io": "ready", + "acme.cert-manager.io": "cert", + "acme.cert-manager.io/dns01-challenge": "dns", } // include all non-rejected annotations in source. @@ -47,7 +47,7 @@ func TestFilterAnnotations(t *testing.T) { source[k] = v } - filtered := FilterReservedAnnotations(retained) + filtered := FilterReservedAnnotations(source) if !reflect.DeepEqual(filtered, retained) { t.Fatalf("expected filtered annotations mismatch: %s", cmp.Diff(filtered, retained)) } diff --git a/pkg/controller/external_secrets/certificate_test.go b/pkg/controller/external_secrets/certificate_test.go index daa5ff71c..4f16fae75 100644 --- a/pkg/controller/external_secrets/certificate_test.go +++ b/pkg/controller/external_secrets/certificate_test.go @@ -457,9 +457,9 @@ func TestCreateOrApplyCertificates(t *testing.T) { t.Error("certificate annotations should not be nil") return nil } - if cert.Annotations["cert-manager.io/issue-temporary-certificate"] != "true" { - t.Errorf("expected annotation 'cert-manager.io/issue-temporary-certificate'='true', got '%s'", - cert.Annotations["cert-manager.io/issue-temporary-certificate"]) + if cert.Annotations["app.io/issue-temporary-certificate"] != "true" { + t.Errorf("expected annotation 'app.io/issue-temporary-certificate'='true', got '%s'", + cert.Annotations["app.io/issue-temporary-certificate"]) } } return nil @@ -476,8 +476,8 @@ func TestCreateOrApplyCertificates(t *testing.T) { esc.Spec.ControllerConfig.CertProvider.CertManager.Mode = v1alpha1.Enabled esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = "test-issuer" esc.Spec.ControllerConfig.Annotations = map[string]string{ - "cert-manager.io/issue-temporary-certificate": "true", - "team/owner": "security", + "app.io/issue-temporary-certificate": "true", + "team/owner": "security", } }, recon: false, @@ -516,8 +516,8 @@ func TestCreateOrApplyCertificates(t *testing.T) { esc.Spec.ControllerConfig.CertProvider.CertManager.Mode = v1alpha1.Enabled esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = "test-issuer" esc.Spec.ControllerConfig.Annotations = map[string]string{ - "allowed-cert-annotation": "value", - "app.kubernetes.io/component": "filtered", + "allowed-cert-annotation": "value", + "app.kubernetes.io/component": "filtered", } }, recon: false, diff --git a/pkg/controller/external_secrets/serviceaccounts.go b/pkg/controller/external_secrets/serviceaccounts.go index a4d0bbab2..6bfa80eec 100644 --- a/pkg/controller/external_secrets/serviceaccounts.go +++ b/pkg/controller/external_secrets/serviceaccounts.go @@ -65,7 +65,15 @@ func (r *Reconciler) createOrApplyServiceAccounts(esc *operatorv1alpha1.External if externalSecretsConfigCreateRecon { r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s serviceaccount already exists, possibly from a previous install", serviceAccountName) } - r.log.V(4).Info("serviceaccount exists", "name", serviceAccountName) + if common.HasObjectChanged(desired, fetched) { + r.log.V(1).Info("ServiceAccount modified, updating", "name", serviceAccountName) + if err := r.UpdateWithRetry(r.ctx, desired); err != nil { + return common.FromClientError(err, "failed to update serviceaccount %s", serviceAccountName) + } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "ServiceAccount %s updated", serviceAccountName) + } else { + r.log.V(4).Info("serviceaccount already up-to-date", "name", serviceAccountName) + } } else { if err := r.Create(r.ctx, desired); err != nil { return common.FromClientError(err, "failed to create serviceaccount %s", serviceAccountName) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 332474f2a..f954a015c 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -23,18 +23,25 @@ import ( "embed" "encoding/base64" "fmt" + "strings" "testing" "time" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1" "github.com/openshift/external-secrets-operator/test/utils" ) @@ -43,11 +50,16 @@ var testassets embed.FS const ( // test bindata - externalSecretsFile = "testdata/external_secret.yaml" + externalSecretsFile = "testdata/external_secret.yaml" externalSecretsFileWithRevisionLimit = "testdata/external_secret_with_revision_limits.yaml" - expectedSecretValueFile = "testdata/expected_value.yaml" + expectedSecretValueFile = "testdata/expected_value.yaml" ) +// ExternalSecretsConfigTemplate represents the template data for ExternalSecretsConfig +type ExternalSecretsConfigTemplate struct { + Annotations map[string]string +} + const ( // test resource names operatorNamespace = "external-secrets-operator" @@ -73,6 +85,7 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, var ( clientset *kubernetes.Clientset dynamicClient *dynamic.DynamicClient + runtimeClient client.Client loader utils.DynamicResourceLoader awsSecretName string testNamespace string @@ -88,6 +101,14 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, dynamicClient, err = dynamic.NewForConfig(cfg) Expect(err).Should(BeNil()) + // Create scheme and register types + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(operatorv1alpha1.AddToScheme(scheme)) + + runtimeClient, err = client.New(cfg, client.Options{Scheme: scheme}) + Expect(err).Should(BeNil()) + awsSecretName = fmt.Sprintf("eso-e2e-secret-%s", utils.GetRandomString(5)) namespace := &corev1.Namespace{ @@ -303,4 +324,134 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }, time.Minute, 5*time.Second).Should(Succeed()) }) }) + + Context("Annotations", func() { + It("should apply custom annotations to created resources", func() { + // Define test annotations + testAnnotations := map[string]string{ + "example.com/custom-annotation": "test-value", + "mycompany.io/owner": "platform-team", + } + + By("Updating ExternalSecretsConfig with custom annotations") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + existingCR := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, existingCR); err != nil { + return err + } + + updatedCR := existingCR.DeepCopy() + updatedCR.Spec.ControllerConfig.Annotations = testAnnotations + + return runtimeClient.Update(ctx, updatedCR) + }) + Expect(err).NotTo(HaveOccurred(), "should update ExternalSecretsConfig with annotations") + + defer func() { + // Remove the test annotations + By("Removing test annotations from ExternalSecretsConfig CR") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCR := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, currentCR); err != nil { + return err + } + for key := range testAnnotations { + delete(currentCR.Spec.ControllerConfig.Annotations, key) + } + return runtimeClient.Update(ctx, currentCR) + }) + Expect(err).NotTo(HaveOccurred(), "should remove test annotations from ExternalSecretsConfig") + }() + + By("Waiting for external-secrets operand pods to be ready") + Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ + operandCoreControllerPodPrefix, + operandCertControllerPodPrefix, + operandWebhookPodPrefix, + })).To(Succeed()) + + By("Verifying annotations are applied to Deployment resources") + Eventually(func(g Gomega) { + deployments, err := clientset.AppsV1().Deployments(operandNamespace).List(ctx, metav1.ListOptions{}) + g.Expect(err).NotTo(HaveOccurred(), "should list deployments in %s namespace", operandNamespace) + g.Expect(deployments.Items).NotTo(BeEmpty(), "should have at least one deployment") + + for _, deployment := range deployments.Items { + if !strings.HasPrefix(deployment.Name, "external-secrets") { + continue + } + + // annotations on deployment.metadata + annotations := deployment.GetAnnotations() + for key, value := range testAnnotations { + g.Expect(annotations).To(HaveKeyWithValue(key, value), + "deployment %s should have annotation %s=%s", deployment.Name, key, value) + } + + // annotations on deployment.spec.template + annotations = deployment.Spec.Template.Annotations + for key, value := range testAnnotations { + g.Expect(annotations).To(HaveKeyWithValue(key, value), + "deployment %s should have annotation %s=%s", deployment.Name, key, value) + } + } + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + + By("Verifying annotations are applied to Service resources") + Eventually(func(g Gomega) { + services, err := clientset.CoreV1().Services(operandNamespace).List(ctx, metav1.ListOptions{}) + g.Expect(err).NotTo(HaveOccurred(), "should list services in %s namespace", operandNamespace) + + for _, service := range services.Items { + if !strings.HasPrefix(service.Name, "external-secrets") { + continue + } + + annotations := service.GetAnnotations() + for key, value := range testAnnotations { + g.Expect(annotations).To(HaveKeyWithValue(key, value), + "service %s should have annotation %s=%s", service.Name, key, value) + } + } + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + + By("Verifying annotations are applied to ServiceAccount resources") + Eventually(func(g Gomega) { + serviceAccounts, err := clientset.CoreV1().ServiceAccounts(operandNamespace).List(ctx, metav1.ListOptions{}) + g.Expect(err).NotTo(HaveOccurred(), "should list service accounts in %s namespace", operandNamespace) + + for _, sa := range serviceAccounts.Items { + if !strings.HasPrefix(sa.Name, "external-secrets") { + continue + } + + annotations := sa.GetAnnotations() + for key, value := range testAnnotations { + g.Expect(annotations).To(HaveKeyWithValue(key, value), + "service account %s should have annotation %s=%s", sa.Name, key, value) + } + } + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should not allow updating annotations with reserved domain prefix", func() { + By("Getting the existing ExternalSecretsConfig CR") + existingCR := &operatorv1alpha1.ExternalSecretsConfig{} + err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, existingCR) + Expect(err).NotTo(HaveOccurred(), "should get ExternalSecretsConfig CR") + + By("Attempting to update with a reserved domain annotation") + updatedCR := existingCR.DeepCopy() + if updatedCR.Spec.ControllerConfig.Annotations == nil { + updatedCR.Spec.ControllerConfig.Annotations = make(map[string]string) + } + + // Add two reserved annotations that should be rejected + updatedCR.Spec.ControllerConfig.Annotations["deployment.kubernetes.io/revision"] = "9" + updatedCR.Spec.ControllerConfig.Annotations["k8s.io/not-allowed"] = "denied" + + err = runtimeClient.Update(ctx, updatedCR) + Expect(err).To(HaveOccurred(), "update with reserved domain annotations should fail") + }) + }) }) diff --git a/test/e2e/testdata/external_secret.yaml b/test/e2e/testdata/external_secret.yaml index 2fa38abb9..b1866026c 100644 --- a/test/e2e/testdata/external_secret.yaml +++ b/test/e2e/testdata/external_secret.yaml @@ -22,3 +22,5 @@ spec: port: 5353 # DNS - protocol: UDP port: 5353 # DNS + + diff --git a/test/e2e/testdata/external_secrets_with_annotations.yaml b/test/e2e/testdata/external_secrets_with_annotations.yaml deleted file mode 100644 index 06251c0ce..000000000 --- a/test/e2e/testdata/external_secrets_with_annotations.yaml +++ /dev/null @@ -1,27 +0,0 @@ -apiVersion: operator.openshift.io/v1alpha1 -kind: ExternalSecretsConfig -metadata: - labels: - app.kubernetes.io/name: cluster - app.kubernetes.io/managed-by: external-secrets-operator-e2e - name: cluster -spec: - controllerConfig: - annotations: - deployment.io/revision: "10" - prometheus.io/scrape: "true" - networkPolicies: - - name: allow-external-secrets-egress - componentName: ExternalSecretsCoreController - egress: - # Allow egress to Kubernetes API server, AWS endpoints, and DNS - - to: [] - ports: - - protocol: TCP - port: 6443 # Kubernetes API - - protocol: TCP - port: 443 # HTTPS (AWS Secrets Manager) - - protocol: TCP - port: 5353 # DNS - - protocol: UDP - port: 5353 # DNS diff --git a/test/utils/conditions.go b/test/utils/conditions.go index 54cb0463b..32eb22282 100644 --- a/test/utils/conditions.go +++ b/test/utils/conditions.go @@ -19,11 +19,13 @@ limitations under the License. package utils import ( + "bytes" "context" "fmt" "math/rand" "os" "strings" + "text/template" "time" corev1 "k8s.io/api/core/v1" @@ -202,3 +204,25 @@ func ReplacePatternInAsset(replacePatternString ...string) AssetFunc { return []byte(replacedFileContent), nil } } + +// TemplateAsset renders a Go template file with the provided data +func TemplateAsset(data interface{}) AssetFunc { + return func(assetName string) ([]byte, error) { + fileContent, err := os.ReadFile(assetName) + if err != nil { + return nil, err + } + + tmpl, err := template.New("asset").Parse(string(fileContent)) + if err != nil { + return nil, fmt.Errorf("failed to parse template %s: %w", assetName, err) + } + + var buf bytes.Buffer + if err := tmpl.Execute(&buf, data); err != nil { + return nil, fmt.Errorf("failed to execute template %s: %w", assetName, err) + } + + return buf.Bytes(), nil + } +} From 7a3b7dc615e809bf71ac3b827ac0eabe5f90e65b Mon Sep 17 00:00:00 2001 From: siddhi bhor Date: Fri, 30 Jan 2026 18:14:36 +0530 Subject: [PATCH 6/6] updated the e2e and the api --- api/v1alpha1/external_secrets_config_types.go | 2 +- .../externalsecretsconfig.testsuite.yaml | 14 +++++------ bundle.Dockerfile | 2 +- ...ecrets-operator.clusterserviceversion.yaml | 4 ++-- ...r.openshift.io_externalsecretsconfigs.yaml | 4 ++-- bundle/metadata/annotations.yaml | 2 +- ...r.openshift.io_externalsecretsconfigs.yaml | 4 ++-- test/e2e/e2e_test.go | 23 +++++++----------- test/utils/conditions.go | 24 ------------------- 9 files changed, 25 insertions(+), 54 deletions(-) diff --git a/api/v1alpha1/external_secrets_config_types.go b/api/v1alpha1/external_secrets_config_types.go index f582b641d..0beb2574c 100644 --- a/api/v1alpha1/external_secrets_config_types.go +++ b/api/v1alpha1/external_secrets_config_types.go @@ -120,7 +120,7 @@ type ControllerConfig struct { // +kubebuilder:validation:XValidation:rule="self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\\\\/)?([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$'))",message="Annotation keys must consist of alphanumeric characters, '-', '_' or '.', starting and ending with alphanumeric, with an optional lowercase DNS subdomain prefix and '/' (e.g., 'my-key' or 'example.com/my-key')" // +kubebuilder:validation:XValidation:rule="self.all(key, !key.contains('/') || key.split('/')[0].size() <= 253)",message="Annotation key prefix (DNS subdomain) must be no more than 253 characters" // +kubebuilder:validation:XValidation:rule="self.all(key, key.contains('/') ? key.split('/')[1].size() <= 63 : key.size() <= 63)",message="Annotation key name part must be no more than 63 characters" - // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io)/'))",message="Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" // +kubebuilder:validation:XValidation:rule="self.all(key, !key.matches('^(cert-manager\\\\.io)/'))",message="Annotation keys containing reserved domain 'cert-manager.io/' are not allowed" // +kubebuilder:validation:MinProperties=0 // +kubebuilder:validation:MaxProperties=20 diff --git a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml index 45c0cf1ae..ba33bda97 100644 --- a/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml +++ b/api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml @@ -548,7 +548,7 @@ tests: controllerConfig: annotations: kubernetes.io/service-account: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with app.kubernetes.io resourceName: cluster initial: | @@ -558,7 +558,7 @@ tests: controllerConfig: annotations: app.kubernetes.io/name: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with openshift.io resourceName: cluster initial: | @@ -568,7 +568,7 @@ tests: controllerConfig: annotations: openshift.io/cluster: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should fail with annotation key starting with k8s.io resourceName: cluster initial: | @@ -578,7 +578,7 @@ tests: controllerConfig: annotations: k8s.io/component: "test" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should not allow annotation key with subdomain of kubernetes.io resourceName: cluster initial: | @@ -588,7 +588,7 @@ tests: controllerConfig: annotations: custom.kubernetes.io/annotation: "denied" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow annotation key ending with kubernetes.io resourceName: cluster initial: | @@ -614,7 +614,7 @@ tests: controllerConfig: annotations: service.k8s.io/annotation: "denied" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow annotation key similar but not matching reserved prefix resourceName: cluster initial: | @@ -644,7 +644,7 @@ tests: allowed-annotation: "good" kubernetes.io/forbidden: "bad" another-allowed: "good" - expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including subdomains like '*.kubernetes.io/') are not allowed" + expectedError: "ExternalSecretsConfig.operator.openshift.io \"cluster\" is invalid: spec.controllerConfig.annotations: Invalid value: \"object\": Annotation keys containing reserved domains 'kubernetes.io/', 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') are not allowed" - name: Should allow exactly 20 annotations (max limit) resourceName: cluster initial: | diff --git a/bundle.Dockerfile b/bundle.Dockerfile index 185d9e272..596156331 100644 --- a/bundle.Dockerfile +++ b/bundle.Dockerfile @@ -6,7 +6,7 @@ LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/ LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/ LABEL operators.operatorframework.io.bundle.package.v1=external-secrets-operator LABEL operators.operatorframework.io.bundle.channels.v1=alpha -LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.2 +LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.39.0 LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1 LABEL operators.operatorframework.io.metrics.project_layout=go.kubebuilder.io/v4 diff --git a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml index 451f9e3e0..fba37ddfe 100644 --- a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml +++ b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml @@ -220,7 +220,7 @@ metadata: categories: Security console.openshift.io/disable-operand-delete: "true" containerImage: openshift.io/external-secrets-operator:latest - createdAt: "2026-01-13T09:33:27Z" + createdAt: "2026-01-30T12:44:22Z" features.operators.openshift.io/cnf: "false" features.operators.openshift.io/cni: "false" features.operators.openshift.io/csi: "false" @@ -242,7 +242,7 @@ metadata: operatorframework.io/suggested-namespace: external-secrets-operator operators.openshift.io/valid-subscription: '["OpenShift Kubernetes Engine", "OpenShift Container Platform", "OpenShift Platform Plus"]' - operators.operatorframework.io/builder: operator-sdk-v1.39.2 + operators.operatorframework.io/builder: operator-sdk-v1.39.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 repository: https://github.com/openshift/external-secrets-operator support: Red Hat, Inc. diff --git a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml index 1ff16179d..34626485c 100644 --- a/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml +++ b/bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml @@ -1189,8 +1189,8 @@ spec: rule: 'self.all(key, key.contains(''/'') ? key.split(''/'')[1].size() <= 63 : key.size() <= 63)' - message: Annotation keys containing reserved domains 'kubernetes.io/', - 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including - subdomains like '*.kubernetes.io/') are not allowed + 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') + are not allowed rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) - message: Annotation keys containing reserved domain 'cert-manager.io/' are not allowed diff --git a/bundle/metadata/annotations.yaml b/bundle/metadata/annotations.yaml index b9cc7bdcf..6937cea9b 100644 --- a/bundle/metadata/annotations.yaml +++ b/bundle/metadata/annotations.yaml @@ -5,7 +5,7 @@ annotations: operators.operatorframework.io.bundle.metadata.v1: metadata/ operators.operatorframework.io.bundle.package.v1: external-secrets-operator operators.operatorframework.io.bundle.channels.v1: alpha - operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.2 + operators.operatorframework.io.metrics.builder: operator-sdk-v1.39.0 operators.operatorframework.io.metrics.mediatype.v1: metrics+v1 operators.operatorframework.io.metrics.project_layout: go.kubebuilder.io/v4 diff --git a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml index 60fe2239c..a8964affa 100644 --- a/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml +++ b/config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml @@ -1189,8 +1189,8 @@ spec: rule: 'self.all(key, key.contains(''/'') ? key.split(''/'')[1].size() <= 63 : key.size() <= 63)' - message: Annotation keys containing reserved domains 'kubernetes.io/', - 'openshift.io/', 'k8s.io/', or 'cert-manager.io/' (including - subdomains like '*.kubernetes.io/') are not allowed + 'openshift.io/', 'k8s.io/' (including subdomains like '*.kubernetes.io/') + are not allowed rule: self.all(key, !key.matches('^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/')) - message: Annotation keys containing reserved domain 'cert-manager.io/' are not allowed diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index f954a015c..16f759797 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -55,11 +55,6 @@ const ( expectedSecretValueFile = "testdata/expected_value.yaml" ) -// ExternalSecretsConfigTemplate represents the template data for ExternalSecretsConfig -type ExternalSecretsConfigTemplate struct { - Annotations map[string]string -} - const ( // test resource names operatorNamespace = "external-secrets-operator" @@ -134,15 +129,6 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, loader.CreateFromFile(testassets.ReadFile, externalSecretsFile, "") }) - AfterAll(func() { - By("Deleting the externalsecrets.openshift.operator.io/cluster CR") - loader.DeleteFromFile(testassets.ReadFile, externalSecretsFile, "") - - By("Deleting the test namespace") - Expect(clientset.CoreV1().Namespaces().Delete(ctx, testNamespace, metav1.DeleteOptions{})). - NotTo(HaveOccurred(), "failed to delete test namespace") - }) - BeforeEach(func() { By("Verifying external-secrets operand pods are ready") Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ @@ -454,4 +440,13 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, Expect(err).To(HaveOccurred(), "update with reserved domain annotations should fail") }) }) + + AfterAll(func() { + By("Deleting the externalsecrets.openshift.operator.io/cluster CR") + loader.DeleteFromFile(testassets.ReadFile, externalSecretsFile, "") + + By("Deleting the test namespace") + Expect(clientset.CoreV1().Namespaces().Delete(ctx, testNamespace, metav1.DeleteOptions{})). + NotTo(HaveOccurred(), "failed to delete test namespace") + }) }) diff --git a/test/utils/conditions.go b/test/utils/conditions.go index 32eb22282..54cb0463b 100644 --- a/test/utils/conditions.go +++ b/test/utils/conditions.go @@ -19,13 +19,11 @@ limitations under the License. package utils import ( - "bytes" "context" "fmt" "math/rand" "os" "strings" - "text/template" "time" corev1 "k8s.io/api/core/v1" @@ -204,25 +202,3 @@ func ReplacePatternInAsset(replacePatternString ...string) AssetFunc { return []byte(replacedFileContent), nil } } - -// TemplateAsset renders a Go template file with the provided data -func TemplateAsset(data interface{}) AssetFunc { - return func(assetName string) ([]byte, error) { - fileContent, err := os.ReadFile(assetName) - if err != nil { - return nil, err - } - - tmpl, err := template.New("asset").Parse(string(fileContent)) - if err != nil { - return nil, fmt.Errorf("failed to parse template %s: %w", assetName, err) - } - - var buf bytes.Buffer - if err := tmpl.Execute(&buf, data); err != nil { - return nil, fmt.Errorf("failed to execute template %s: %w", assetName, err) - } - - return buf.Bytes(), nil - } -}