ESO-281:Adds the Implementation Logic for Annotations #97
ESO-281:Adds the Implementation Logic for Annotations #97siddhibhor-56 wants to merge 6 commits intoopenshift:mainfrom
Conversation
WalkthroughConverted controller annotations from an array type to a map, added annotation validation and filtering for reserved domains, introduced utilities to merge/filter annotations, and propagated validated annotations across many reconciled resources with accompanying unit and e2e tests updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
siddhibhor-56
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/external_secrets/networkpolicy.go (1)
180-207: Custom network policies are missing annotation application.The
buildNetworkPolicyFromConfigfunction appliesresourceLabels(line 195) but does not apply annotations fromControllerConfig.Annotations. This creates an inconsistency where static network policies receive annotations but custom/user-defined network policies do not.🔧 Suggested fix
func (r *Reconciler) buildNetworkPolicyFromConfig(esc *operatorv1alpha1.ExternalSecretsConfig, npConfig operatorv1alpha1.NetworkPolicy, resourceLabels map[string]string) (*networkingv1.NetworkPolicy, error) { namespace := getNamespace(esc) // Determine pod selector based on component name podSelector, err := r.getPodSelectorForComponent(npConfig.ComponentName) if err != nil { return nil, fmt.Errorf("failed to determine pod selector for network policy %s: %w", npConfig.Name, err) } + // Build annotations map + var annotations map[string]string + if len(esc.Spec.ControllerConfig.Annotations) > 0 { + annotations = convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) + } + // Build the NetworkPolicy object networkPolicy := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: npConfig.Name, Namespace: namespace, Labels: resourceLabels, + Annotations: annotations, }, Spec: networkingv1.NetworkPolicySpec{ PodSelector: podSelector, PolicyTypes: []networkingv1.PolicyType{ networkingv1.PolicyTypeEgress, }, Egress: npConfig.Egress, }, } return networkPolicy, nil }
🤖 Fix all issues with AI agents
In @bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml:
- Around line 1177-1182: Update the description text in the manifest so it
exactly matches the validation rule that forbids annotation key prefixes with
trailing slashes: change the listed forbidden prefixes to include the trailing
slashes and correct the app.kubernetes entry to app.kubernetes.io/; ensure the
description now states that keys with prefixes `kubernetes.io/`,
`app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed to match the
validation rule used elsewhere in this resource.
In @docs/api_reference.md:
- Line 201: The documentation for the `annotations` field incorrectly lists
prefix `app.kubernetes` and omits trailing slashes; update the description for
`annotations` (the `annotations` _[Annotation](#annotation) array_ entry) so
each disallowed prefix matches the validation rule including trailing slashes:
use `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, and `k8s.io/`.
Ensure the sentence and example text reflect the trailing slash format for all
listed prefixes.
🧹 Nitpick comments (2)
pkg/controller/external_secrets/deployments.go (1)
173-196: Consider usingstrings.HasPrefixand centralizing the helper.The manual prefix checking is correct but verbose. Additionally, this function appears to be duplicated across multiple files.
♻️ Optional improvements
1. Use
strings.HasPrefixfor clarity:+import "strings" + 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 { + if strings.HasPrefix(ann.Key, prefix) { isReserved = true logger.V(1).Info("skipping annotation with reserved prefix", "key", ann.Key, "prefix", prefix) break } }2. Move to common package to eliminate duplication:
Since this function appears in multiple controller files with identical logic, consider moving it to
pkg/controller/common/utils.goalongsideUpdateResourceAnnotations.pkg/controller/external_secrets/install_external_secrets.go (1)
126-132: Consider usingcommon.UpdateResourceAnnotationsfor consistency.While direct assignment works correctly for newly created namespaces, using
common.UpdateResourceAnnotations(like other resources in this PR) would maintain a consistent pattern across the codebase.♻️ Optional refactor for consistency
// 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 + common.UpdateResourceAnnotations(obj, annotationsMap) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (16)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/zz_generated.deepcopy.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/common/utils.gopkg/controller/external_secrets/certificate.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.gopkg/controller/external_secrets/install_external_secrets.gopkg/controller/external_secrets/networkpolicy.gopkg/controller/external_secrets/rbacs.gopkg/controller/external_secrets/secret.gopkg/controller/external_secrets/serviceaccounts.gopkg/controller/external_secrets/services.gopkg/controller/external_secrets/validatingwebhook.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/external_secrets/secret.gopkg/controller/external_secrets/services.gopkg/controller/external_secrets/serviceaccounts.gopkg/controller/common/utils.gopkg/controller/external_secrets/validatingwebhook.gopkg/controller/external_secrets/certificate.godocs/api_reference.mdpkg/controller/external_secrets/deployments.goconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/zz_generated.deepcopy.gopkg/controller/external_secrets/install_external_secrets.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/external_secrets_config_types.gopkg/controller/external_secrets/networkpolicy.gopkg/controller/external_secrets/deployments_test.gopkg/controller/external_secrets/rbacs.go
🧬 Code graph analysis (10)
pkg/controller/external_secrets/secret.go (2)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
pkg/controller/external_secrets/services.go (2)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
pkg/controller/external_secrets/serviceaccounts.go (2)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
pkg/controller/external_secrets/validatingwebhook.go (2)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
pkg/controller/external_secrets/certificate.go (2)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
pkg/controller/external_secrets/deployments.go (2)
api/v1alpha1/external_secrets_config_types.go (2)
ControllerConfig(103-142)Annotation(153-156)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/external_secrets_config_types.go (2)
Annotation(153-156)KVPair(145-149)
pkg/controller/external_secrets/install_external_secrets.go (1)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)
pkg/controller/external_secrets/networkpolicy.go (2)
api/v1alpha1/external_secrets_config_types.go (1)
ControllerConfig(103-142)pkg/controller/common/utils.go (1)
UpdateResourceAnnotations(76-85)
pkg/controller/external_secrets/deployments_test.go (4)
pkg/controller/external_secrets/controller.go (1)
Reconciler(85-94)pkg/controller/crd_annotator/controller.go (1)
Reconciler(65-69)pkg/controller/client/fakes/fake_ctrl_client.go (1)
FakeCtrlClient(12-135)api/v1alpha1/external_secrets_config_types.go (4)
ExternalSecretsConfig(43-55)ControllerConfig(103-142)Annotation(153-156)KVPair(145-149)
🔇 Additional comments (21)
api/v1alpha1/zz_generated.deepcopy.go (1)
30-44: LGTM - Auto-generated deep copy implementations are correct.The generated code properly handles the new
AnnotationandKVPairtypes. Since both types contain only string fields (value types), the shallow copy approach (*out = *in,copy()) is appropriate and correct.Also applies to: 233-237, 495-508
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1204: LGTM - CRD schema for annotations is well-defined.The annotations field schema correctly:
- Uses
x-kubernetes-list-type: mapwithkeyas the map key for proper merge semantics- Enforces
maxItems: 20limit- Includes CEL validation to block reserved Kubernetes/OpenShift prefixes
- Documents merge behavior and precedence clearly
pkg/controller/external_secrets/certificate.go (1)
93-99: LGTM - Annotation application follows the established pattern.The annotation application logic correctly:
- Checks for non-empty annotations before processing
- Converts annotations to map (which filters reserved prefixes based on the PR context)
- Only applies if the resulting map is non-empty
- Uses the shared
UpdateResourceAnnotationsutilitypkg/controller/external_secrets/networkpolicy.go (1)
140-146: LGTM for static network policies.Annotation application for static network policies follows the consistent pattern used across other resources.
pkg/controller/external_secrets/services.go (1)
56-62: LGTM - Annotation application follows the established pattern.The Service annotation logic is consistent with other resource controllers in this PR.
pkg/controller/external_secrets/secret.go (1)
64-71: LGTM - Annotation application follows the established pattern.The Secret annotation logic is consistent with other resource controllers in this PR.
pkg/controller/external_secrets/serviceaccounts.go (1)
47-53: LGTM - Annotation application follows the established pattern.The ServiceAccount annotation logic is consistent with other resource controllers in this PR.
pkg/controller/external_secrets/validatingwebhook.go (1)
62-69: LGTM! Consistent annotation handling.The annotation application logic correctly converts and filters user-provided annotations before applying them to the ValidatingWebhook. The ordering is appropriate—user annotations are set first, followed by operator-managed cert-manager annotations.
pkg/controller/external_secrets/deployments.go (3)
5-5: LGTM! Appropriate import for logging.The logr import is correctly added to support structured logging within the annotation conversion helper.
107-114: LGTM! Dual annotation application.Correctly applies annotations to both the Deployment resource and its PodTemplate, ensuring consistency between the deployment metadata and the pods it creates.
198-208: LGTM! Consistent with label handling pattern.The function correctly mirrors the approach used for
updatePodTemplateLabels, ensuring annotations are properly merged into the pod template specification.pkg/controller/external_secrets/deployments_test.go (2)
565-630: LGTM! Comprehensive annotation test coverage.The test case properly validates that custom annotations are applied to both the Deployment resource and its PodTemplate, covering multiple annotation patterns including namespace-prefixed keys.
632-688: LGTM! Good coverage of reserved prefix filtering.The test correctly validates that user-provided annotations with reserved prefixes are filtered out. The handling of potentially existing default annotations is appropriate.
api/v1alpha1/external_secrets_config_types.go (3)
116-123: LGTM! Well-defined validation rules.The Annotations field is properly constrained with a maximum of 20 items and includes CRD-level validation to prevent reserved Kubernetes prefixes, providing defense in depth alongside runtime filtering.
144-149: LGTM! Clean, reusable structure.The KVPair type provides a simple, generic key-value pair structure that can be reused across different configuration contexts.
151-156: LGTM! Appropriate type distinction.The Annotation type correctly embeds KVPair inline, providing semantic distinction while reusing the underlying structure.
pkg/controller/common/utils.go (3)
74-85: LGTM!The
UpdateResourceAnnotationsfunction correctly mirrors the pattern ofUpdateResourceLabels, properly merging annotations with user-specified values taking precedence.
209-217: LGTM!Correctly extends metadata change detection to include annotations in addition to labels.
244-247: LGTM!Appropriately checks pod template annotations for deployment modifications.
pkg/controller/external_secrets/rbacs.go (2)
196-203: LGTM!Consistent annotation application across ClusterRoleBinding, Role, and RoleBinding objects.
Also applies to: 247-254, 299-306
141-149: Annotation filtering and consistent application across RBAC objects is solid.The
convertAnnotationsToMapfunction properly filters reserved prefixes (kubernetes.io/,app.kubernetes.io/,openshift.io/,k8s.io/) and logs skipped annotations. The pattern is applied consistently across all RBAC and other resource getters.
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
Outdated
Show resolved
Hide resolved
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-281 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
|
||
| // Apply annotations from ControllerConfig | ||
| if len(esc.Spec.ControllerConfig.Annotations) > 0 { | ||
| annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) |
There was a problem hiding this comment.
Annotations validation is handled in CRD, but it would be good have a validation here as well.
k8s.io/apimachinery/pkg/api/validation.ValidateAnnotations()
|
|
||
| // Apply annotations from ControllerConfig | ||
| if len(esc.Spec.ControllerConfig.Annotations) > 0 { | ||
| annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) |
There was a problem hiding this comment.
we are calling convertAnnotationsToMap() for every managed resource. Instead it should be at one place and passed around like resourcelabels. Either we can add a new arg to each func, or modify the existing resourcelabels arg to accommodate both labels and annotations.
| // Apply annotations from ControllerConfig | ||
| if len(esc.Spec.ControllerConfig.Annotations) > 0 { | ||
| annotationsMap := convertAnnotationsToMap(esc.Spec.ControllerConfig.Annotations, r.log) | ||
| if len(annotationsMap) > 0 { |
There was a problem hiding this comment.
The if condition should be moved to UpdateResourceAnnotations(), instead of checking it in every caller.
| } | ||
|
|
||
| // convertAnnotationsToMap converts []Annotation to map[string]string with reserved prefix filtering. | ||
| func convertAnnotationsToMap(annotations []operatorv1alpha1.Annotation, logger logr.Logger) map[string]string { |
There was a problem hiding this comment.
Better to have it in the common utils.
siddhibhor-56
force-pushed
the
eso-annotations
branch
from
0f2c8f3 to
c094e40
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@config/manager/kustomization.yaml`:
- Around line 6-7: The kustomization change replaces the controller image
registry (name: controller, newName:
quay.io/rh-ee-sbhor/external-secrets-operator) but leaves the tag unpinned,
resulting in a mutable :latest image; update the kustomization to include an
explicit newTag or newDigest for the controller image (referencing the same
image name used in the diff) so deployments use an immutable, reproducible image
reference — add newTag: <version> or newDigest: sha256:<digest> alongside name:
controller and newName to pin the controller image.
In `@pkg/controller/external_secrets/configmap.go`:
- Around line 69-88: ObjectMetadataModified is comparing the full annotation
maps so CNO-injected openshift.io/* annotations (which are removed from
desiredConfigMap by validateAndFilterAnnotations) cause the controller to always
think metadata changed; fix by comparing only the managed/filtered annotations
when deciding to update: create filtered copies of desiredConfigMap.Annotations
and existingConfigMap.Annotations using the same validateAndFilterAnnotations
logic (or a helper like filterManagedAnnotations) and then call
common.ObjectMetadataModified (or a new comparator) using
desiredConfigMap.Labels and the filtered annotation map versus
existingConfigMap.Labels and its filtered annotation map so external/unmanaged
annotations do not trigger a reconcile loop for ConfigMap (refer to
ObjectMetadataModified, desiredConfigMap, existingConfigMap,
validateAndFilterAnnotations, configMapName).
In `@pkg/controller/external_secrets/rbacs_test.go`:
- Around line 307-315: The test currently skips verification when annotations
are nil; update the block that inspects obj.(*rbacv1.ClusterRole) (variable cr)
so it explicitly fails if cr.Annotations is nil (e.g., t.Fatalf or t.Errorf +
return) and only then asserts cr.Annotations["rbac/managed-by"] == "operator";
apply the same explicit-nil-fails change to the other annotation checks that
inspect ClusterRole and Role/RoleBinding annotations (the blocks checking
"rbac/managed-by" and related keys at the other locations).
In `@pkg/controller/external_secrets/validatingwebhook_test.go`:
- Around line 164-170: Indentation mismatch: align the block that sets
r.CtrlClient = mock, initializes externalSecretsForValidateWebhook, applies
tt.updateExternalSecretsConfig, and calls
r.createOrApplyValidatingWebhookConfiguration to use the same double-tab
indentation as the surrounding test code (i.e., match the indentation style used
in the lines around the test). Locate the statements around r.CtrlClient = mock
and adjust their leading tabs so they are consistent with the surrounding block
containing the test setup and call to
createOrApplyValidatingWebhookConfiguration.
🧹 Nitpick comments (2)
pkg/controller/external_secrets/deployments.go (1)
174-207: Consider usingstrings.HasPrefixfor cleaner prefix matching.The manual prefix check at line 193 could be simplified using the standard library function for better readability.
♻️ Suggested improvement
+import "strings" + // 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 key, value := range annotations { isReserved := false for _, prefix := range reservedPrefixes { - if len(key) >= len(prefix) && key[:len(prefix)] == prefix { + if strings.HasPrefix(key, prefix) { isReserved = true logger.V(1).Info("skipping annotation with reserved prefix", "key", key, "prefix", prefix) break } } if !isReserved { result[key] = value } } return result }pkg/controller/external_secrets/rbacs.go (1)
141-148: Consider extracting repeated annotation handling into a helper function.The same annotation application pattern is repeated across
getClusterRoleObject,getClusterRoleBindingObject,getRoleObject, andgetRoleBindingObject. This could be extracted to reduce duplication and improve maintainability.♻️ Suggested helper function
// applyControllerAnnotations applies validated annotations from ControllerConfig to the given object. func (r *Reconciler) applyControllerAnnotations(obj client.Object, esc *operatorv1alpha1.ExternalSecretsConfig) { if len(esc.Spec.ControllerConfig.Annotations) > 0 { annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) if len(annotationsMap) > 0 { common.UpdateResourceAnnotations(obj, annotationsMap) } } }Then each getter simplifies to:
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 := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) - if len(annotationsMap) > 0 { - common.UpdateResourceAnnotations(clusterRole, annotationsMap) - } - } - + r.applyControllerAnnotations(clusterRole, esc) return clusterRole }Also applies to: 195-202, 246-253, 298-305
config/manager/kustomization.yaml
Outdated
| - name: controller | ||
| newName: openshift.io/external-secrets-operator | ||
| newTag: latest | ||
| newName: quay.io/rh-ee-sbhor/external-secrets-operator |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect controller image references and tags/digests
rg -n --type=yaml -C2 'image:' config/manager
echo "---"
rg -n --type=yaml -C2 'images:' config/manager/kustomization.yamlRepository: openshift/external-secrets-operator
Length of output: 541
🏁 Script executed:
#!/bin/bash
# Check for other kustomization files that might patch the image
find config -name 'kustomization.yaml' -o -name 'kustomization.yml'
echo "---"
# Search for any newTag or digest references to controller
rg -n 'newTag|digest' config/ --type=yamlRepository: openshift/external-secrets-operator
Length of output: 429
🏁 Script executed:
#!/bin/bash
# Check config/default/kustomization.yaml which is typically the production overlay
cat config/default/kustomization.yaml | grep -A5 -B5 'images\|controller' || echo "No image references found"Repository: openshift/external-secrets-operator
Length of output: 759
🏁 Script executed:
#!/bin/bash
# Check the full config/default/kustomization.yaml structure
cat config/default/kustomization.yamlRepository: openshift/external-secrets-operator
Length of output: 5310
Pin the controller image tag/digest after changing registries.
Line 7 changes the registry to quay.io/rh-ee-sbhor/external-secrets-operator but leaves the tag resolution to the base image (controller:latest from config/manager/manager.yaml), which results in the unpinned mutable tag quay.io/rh-ee-sbhor/external-secrets-operator:latest. This introduces non-reproducible and potentially unsafe deployments. Add an explicit newTag or digest.
🔧 Suggested fix (example)
images:
- name: controller
newName: quay.io/rh-ee-sbhor/external-secrets-operator
+ newTag: vX.Y.Z
+ # alternatively:
+ # digest: sha256:...🤖 Prompt for AI Agents
In `@config/manager/kustomization.yaml` around lines 6 - 7, The kustomization
change replaces the controller image registry (name: controller, newName:
quay.io/rh-ee-sbhor/external-secrets-operator) but leaves the tag unpinned,
resulting in a mutable :latest image; update the kustomization to include an
explicit newTag or newDigest for the controller image (referencing the same
image name used in the diff) so deployments use an immutable, reproducible image
reference — add newTag: <version> or newDigest: sha256:<digest> alongside name:
controller and newName to pin the controller image.
| // 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 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid a reconcile loop caused by CNO-added annotations.
ObjectMetadataModified now compares full annotation maps. Since CNO injects openshift.io/* and validateAndFilterAnnotations strips those from desiredConfigMap, Line 74 will stay true and this will update on every reconcile even when managed annotations haven’t changed.
Consider comparing only managed (desired) annotations so extra external annotations don’t trigger updates.
💡 Suggested fix (compare only managed annotations)
@@
-import (
- "fmt"
+import (
+ "fmt"
+ "reflect"
@@
- // 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) {
+ // Check if metadata (labels/managed annotations) has been modified.
+ labelsChanged := !reflect.DeepEqual(desiredConfigMap.Labels, existingConfigMap.Labels)
+ annotationsChanged := false
+ if desiredConfigMap.Annotations != nil {
+ for k, v := range desiredConfigMap.Annotations {
+ if existingConfigMap.Annotations == nil || existingConfigMap.Annotations[k] != v {
+ annotationsChanged = true
+ break
+ }
+ }
+ }
+ if exist && (labelsChanged || annotationsChanged) {🤖 Prompt for AI Agents
In `@pkg/controller/external_secrets/configmap.go` around lines 69 - 88,
ObjectMetadataModified is comparing the full annotation maps so CNO-injected
openshift.io/* annotations (which are removed from desiredConfigMap by
validateAndFilterAnnotations) cause the controller to always think metadata
changed; fix by comparing only the managed/filtered annotations when deciding to
update: create filtered copies of desiredConfigMap.Annotations and
existingConfigMap.Annotations using the same validateAndFilterAnnotations logic
(or a helper like filterManagedAnnotations) and then call
common.ObjectMetadataModified (or a new comparator) using
desiredConfigMap.Labels and the filtered annotation map versus
existingConfigMap.Labels and its filtered annotation map so external/unmanaged
annotations do not trigger a reconcile loop for ConfigMap (refer to
ObjectMetadataModified, desiredConfigMap, existingConfigMap,
validateAndFilterAnnotations, configMapName).
| 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"]) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Test doesn't fail when annotations are unexpectedly nil.
If annotations weren't applied due to a bug, cr.Annotations would be nil and the test would silently pass. The assertion should explicitly fail when annotations are missing.
💚 Proposed fix
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"])
- }
+ if cr.Annotations == nil {
+ t.Error("expected annotations to be applied, got nil")
+ } else if cr.Annotations["rbac/managed-by"] != "operator" {
+ t.Errorf("expected annotation 'rbac/managed-by'='operator', got '%s'",
+ cr.Annotations["rbac/managed-by"])
}
}
return nil
})The same pattern applies to the other annotation tests at lines 333-340, 356-365, and 380-389.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if 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"]) | |
| } | |
| } | |
| } | |
| if cr, ok := obj.(*rbacv1.ClusterRole); ok { | |
| // Verify annotations are applied | |
| if cr.Annotations == nil { | |
| t.Error("expected annotations to be applied, got nil") | |
| } else if cr.Annotations["rbac/managed-by"] != "operator" { | |
| t.Errorf("expected annotation 'rbac/managed-by'='operator', got '%s'", | |
| cr.Annotations["rbac/managed-by"]) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/controller/external_secrets/rbacs_test.go` around lines 307 - 315, The
test currently skips verification when annotations are nil; update the block
that inspects obj.(*rbacv1.ClusterRole) (variable cr) so it explicitly fails if
cr.Annotations is nil (e.g., t.Fatalf or t.Errorf + return) and only then
asserts cr.Annotations["rbac/managed-by"] == "operator"; apply the same
explicit-nil-fails change to the other annotation checks that inspect
ClusterRole and Role/RoleBinding annotations (the blocks checking
"rbac/managed-by" and related keys at the other locations).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/v1alpha1/external_secrets_config_types.go`:
- Around line 117-123: The XValidation rule that rejects annotation keys using
contains('kubernetes.io/') is too broad and disallows keys like
"mykubernetes.io/..." — update the reserved-domain rule for the annotations
validation to check only the prefix (key.split('/')[0]) and ensure that prefix
is not exactly or a subdomain of the reserved domains; for example, replace the
current contains(...) check with logic that extracts prefix := key.split('/')[0]
(or handles absence of a prefix when no '/') and asserts !(prefix ==
"kubernetes.io" || prefix.endsWith(".kubernetes.io") || prefix == "openshift.io"
|| prefix.endsWith(".openshift.io") || prefix == "k8s.io" ||
prefix.endsWith(".k8s.io")). Ensure this change is applied to the XValidation
rule on the annotations field so only reserved domains and their subdomains are
rejected.
In `@pkg/controller/common/utils.go`:
- Around line 209-221: In ObjectMetadataModified, normalize annotation maps
before comparing to avoid nil vs empty-map mismatches: ensure
desired.GetAnnotations() and the result of
filterSystemManagedAnnotations(fetched.GetAnnotations()) are both non-nil (e.g.,
convert nil to an empty map) or both nil before calling reflect.DeepEqual;
update the comparison in the ObjectMetadataModified function (and any helper
usage of filterSystemManagedAnnotations) so annotations are canonicalized (nil →
empty map or vice versa) prior to equality check to prevent spurious diffs.
18eba9c to
6a18217
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/controller/common/utils.go`:
- Around line 223-261: FilterReservedAnnotations is too broad because it
searches the whole key for reserved domain substrings; instead, split the
annotation key at the first '/' and only inspect the left-hand part (the domain
portion) against reservedDomains. Replace the strings.Contains checks with exact
equality (domain) or suffix match for subdomains (e.g.,
strings.HasSuffix(domainPart, "."+domain)) while keeping the existing direct
prefix check logic; update the loop that iterates reservedDomains accordingly
and ensure result preserves non-reserved entries.
In `@pkg/controller/external_secrets/deployments_test.go`:
- Around line 633-705: The reserved-annotation assertion in the
validateDeployment test currently only fails when the annotation value equals
"value", so change the check in the validateDeployment closure (where
reservedKeys is defined) to assert on presence regardless of value: for each key
in reservedKeys, test existence with something like "if _, exists :=
annotations[key]; exists { t.Errorf(... ) }" so any reserved annotation is
flagged even if its value is not "value".
In `@test/e2e/testdata/external_secrets_with_annotations.yaml`:
- Around line 24-27: The test network policy data only includes DNS port 5353;
update the DNS egress rules in external_secrets_with_annotations.yaml to also
allow port 53 for both TCP and UDP. Locate the DNS egress entries (the protocol:
TCP/UDP blocks listing port: 5353) and add corresponding entries with port: 53
so both "port: 53" and "port: 5353" are present for TCP and UDP to match the
reference networkpolicy_allow-dns.yaml.
♻️ Duplicate comments (1)
pkg/controller/common/utils.go (1)
209-221: Normalize annotation maps before DeepEqual comparisons.Nil vs empty maps can produce false diffs once reserved keys are filtered, causing unnecessary updates. Consider normalizing both sides (and avoiding the early nil return in deploymentSpecModified).
🔧 Suggested normalization to avoid nil/empty mismatches
func ObjectMetadataModified(desired, fetched client.Object) bool { // Check if labels have changed if !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) { return true } // Check if annotations have changed (ignoring system-managed annotations) desiredAnnotates := desired.GetAnnotations() fetchedAnnotates := FilterReservedAnnotations(fetched.GetAnnotations()) + if desiredAnnotates == nil { + desiredAnnotates = map[string]string{} + } + if fetchedAnnotates == nil { + fetchedAnnotates = map[string]string{} + } if !reflect.DeepEqual(desiredAnnotates, fetchedAnnotates) { return true } return false } @@ - 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.Annotations != nil { + fetchedAnnots := FilterReservedAnnotations(fetched.GetAnnotations()) + if fetchedAnnots == nil { + fetchedAnnots = map[string]string{} + } + if !reflect.DeepEqual(desired.Annotations, fetchedAnnots) { + return true + } + }Also applies to: 273-281
🧹 Nitpick comments (2)
pkg/controller/external_secrets/deployments.go (2)
174-198: Consider movingvalidateAndFilterAnnotationsto common utils.This function performs validation and filtering that appears to be needed across multiple resource types (Deployments, Secrets, Services, etc.). Centralizing it in the
commonpackage would:
- Avoid duplication across controllers
- Enable single-point validation before passing annotations to reconciliation functions (similar to how
resourceLabelsis handled)Based on past review comments, annotations should be validated/filtered once and passed around rather than calling this for every managed resource.
108-115: Nested length checks add verbosity.The pattern of checking
len(esc.Spec.ControllerConfig.Annotations) > 0before callingvalidateAndFilterAnnotations, then checkinglen(annotationsMap) > 0before applying, is verbose. Consider moving the empty-check logic intoUpdateResourceAnnotationsso callers don't need to guard against empty maps.♻️ Suggested simplification
If
UpdateResourceAnnotationshandles empty maps gracefully (no-op), the caller code simplifies to:annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) common.UpdateResourceAnnotations(deployment, annotationsMap) updatePodTemplateAnnotations(deployment, annotationsMap)
| name: "deployment reconciliation filters reserved annotation prefixes", | ||
| preReq: func(r *Reconciler, m *fakes.FakeCtrlClient, capturedDeployment **appsv1.Deployment) { | ||
| m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { | ||
| switch o := obj.(type) { | ||
| case *appsv1.Deployment: | ||
| deployment := testDeployment(controllerDeploymentAssetName) | ||
| deployment.DeepCopyInto(o) | ||
| } | ||
| return true, nil | ||
| }) | ||
| m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { | ||
| switch o := obj.(type) { | ||
| case *appsv1.Deployment: | ||
| *capturedDeployment = o.DeepCopy() | ||
| } | ||
| return nil | ||
| }) | ||
| }, | ||
| updateExternalSecretsConfig: func(esc *v1alpha1.ExternalSecretsConfig) { | ||
| esc.Spec.ControllerConfig.Annotations = map[string]string{ | ||
| "allowed-annotation": "allowed", | ||
| "kubernetes.io/forbidden": "value", | ||
| "app.kubernetes.io/managed-by": "value", | ||
| "deployment.kubernetes.io/revision": "100", | ||
| "pod.kubernetes.io/name": "test", | ||
| "openshift.io/test": "value", | ||
| "console.openshift.io/route": "value", | ||
| } | ||
| }, | ||
| validateDeployment: func(t *testing.T, deployment *appsv1.Deployment) { | ||
| if deployment == nil { | ||
| t.Error("deployment should not be nil") | ||
| return | ||
| } | ||
|
|
||
| annotations := deployment.GetAnnotations() | ||
| if annotations == nil { | ||
| t.Error("deployment annotations should not be nil") | ||
| return | ||
| } | ||
|
|
||
| // Allowed annotation should be present | ||
| if annotations["allowed-annotation"] != "allowed" { | ||
| t.Errorf("allowed-annotation should be present with value 'allowed', got: %v", | ||
| annotations["allowed-annotation"]) | ||
| } | ||
|
|
||
| // Verify ALL reserved domain patterns are filtered | ||
| reservedKeys := []string{ | ||
| "kubernetes.io/forbidden", | ||
| "app.kubernetes.io/managed-by", | ||
| "deployment.kubernetes.io/revision", | ||
| "pod.kubernetes.io/name", | ||
| "openshift.io/test", | ||
| "console.openshift.io/route", | ||
| } | ||
|
|
||
| for _, key := range reservedKeys { | ||
| if val, exists := annotations[key]; exists && val == "value" { | ||
| t.Errorf("reserved annotation %q should have been filtered but found with value %q", key, val) | ||
| } | ||
| } | ||
|
|
||
| // Verify only the allowed annotation exists (excluding any default annotations) | ||
| userAnnotationCount := 0 | ||
| for key := range annotations { | ||
| if key == "allowed-annotation" { | ||
| userAnnotationCount++ | ||
| } | ||
| } | ||
| if userAnnotationCount != 1 { | ||
| t.Errorf("expected exactly 1 user annotation, found user annotations: %v", annotations) | ||
| } |
There was a problem hiding this comment.
Reserved-annotation test can miss non‑value entries.
The loop only errors when the value equals "value", so keys like deployment.kubernetes.io/revision or pod.kubernetes.io/name could slip through if present. Check for existence regardless of value.
🔧 Tighten reserved-key assertions
for _, key := range reservedKeys {
- if val, exists := annotations[key]; exists && val == "value" {
- t.Errorf("reserved annotation %q should have been filtered but found with value %q", key, val)
- }
+ if val, exists := annotations[key]; exists {
+ t.Errorf("reserved annotation %q should have been filtered but found with value %q", key, val)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: "deployment reconciliation filters reserved annotation prefixes", | |
| preReq: func(r *Reconciler, m *fakes.FakeCtrlClient, capturedDeployment **appsv1.Deployment) { | |
| m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { | |
| switch o := obj.(type) { | |
| case *appsv1.Deployment: | |
| deployment := testDeployment(controllerDeploymentAssetName) | |
| deployment.DeepCopyInto(o) | |
| } | |
| return true, nil | |
| }) | |
| m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { | |
| switch o := obj.(type) { | |
| case *appsv1.Deployment: | |
| *capturedDeployment = o.DeepCopy() | |
| } | |
| return nil | |
| }) | |
| }, | |
| updateExternalSecretsConfig: func(esc *v1alpha1.ExternalSecretsConfig) { | |
| esc.Spec.ControllerConfig.Annotations = map[string]string{ | |
| "allowed-annotation": "allowed", | |
| "kubernetes.io/forbidden": "value", | |
| "app.kubernetes.io/managed-by": "value", | |
| "deployment.kubernetes.io/revision": "100", | |
| "pod.kubernetes.io/name": "test", | |
| "openshift.io/test": "value", | |
| "console.openshift.io/route": "value", | |
| } | |
| }, | |
| validateDeployment: func(t *testing.T, deployment *appsv1.Deployment) { | |
| if deployment == nil { | |
| t.Error("deployment should not be nil") | |
| return | |
| } | |
| annotations := deployment.GetAnnotations() | |
| if annotations == nil { | |
| t.Error("deployment annotations should not be nil") | |
| return | |
| } | |
| // Allowed annotation should be present | |
| if annotations["allowed-annotation"] != "allowed" { | |
| t.Errorf("allowed-annotation should be present with value 'allowed', got: %v", | |
| annotations["allowed-annotation"]) | |
| } | |
| // Verify ALL reserved domain patterns are filtered | |
| reservedKeys := []string{ | |
| "kubernetes.io/forbidden", | |
| "app.kubernetes.io/managed-by", | |
| "deployment.kubernetes.io/revision", | |
| "pod.kubernetes.io/name", | |
| "openshift.io/test", | |
| "console.openshift.io/route", | |
| } | |
| for _, key := range reservedKeys { | |
| if val, exists := annotations[key]; exists && val == "value" { | |
| t.Errorf("reserved annotation %q should have been filtered but found with value %q", key, val) | |
| } | |
| } | |
| // Verify only the allowed annotation exists (excluding any default annotations) | |
| userAnnotationCount := 0 | |
| for key := range annotations { | |
| if key == "allowed-annotation" { | |
| userAnnotationCount++ | |
| } | |
| } | |
| if userAnnotationCount != 1 { | |
| t.Errorf("expected exactly 1 user annotation, found user annotations: %v", annotations) | |
| } | |
| name: "deployment reconciliation filters reserved annotation prefixes", | |
| preReq: func(r *Reconciler, m *fakes.FakeCtrlClient, capturedDeployment **appsv1.Deployment) { | |
| m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { | |
| switch o := obj.(type) { | |
| case *appsv1.Deployment: | |
| deployment := testDeployment(controllerDeploymentAssetName) | |
| deployment.DeepCopyInto(o) | |
| } | |
| return true, nil | |
| }) | |
| m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { | |
| switch o := obj.(type) { | |
| case *appsv1.Deployment: | |
| *capturedDeployment = o.DeepCopy() | |
| } | |
| return nil | |
| }) | |
| }, | |
| updateExternalSecretsConfig: func(esc *v1alpha1.ExternalSecretsConfig) { | |
| esc.Spec.ControllerConfig.Annotations = map[string]string{ | |
| "allowed-annotation": "allowed", | |
| "kubernetes.io/forbidden": "value", | |
| "app.kubernetes.io/managed-by": "value", | |
| "deployment.kubernetes.io/revision": "100", | |
| "pod.kubernetes.io/name": "test", | |
| "openshift.io/test": "value", | |
| "console.openshift.io/route": "value", | |
| } | |
| }, | |
| validateDeployment: func(t *testing.T, deployment *appsv1.Deployment) { | |
| if deployment == nil { | |
| t.Error("deployment should not be nil") | |
| return | |
| } | |
| annotations := deployment.GetAnnotations() | |
| if annotations == nil { | |
| t.Error("deployment annotations should not be nil") | |
| return | |
| } | |
| // Allowed annotation should be present | |
| if annotations["allowed-annotation"] != "allowed" { | |
| t.Errorf("allowed-annotation should be present with value 'allowed', got: %v", | |
| annotations["allowed-annotation"]) | |
| } | |
| // Verify ALL reserved domain patterns are filtered | |
| reservedKeys := []string{ | |
| "kubernetes.io/forbidden", | |
| "app.kubernetes.io/managed-by", | |
| "deployment.kubernetes.io/revision", | |
| "pod.kubernetes.io/name", | |
| "openshift.io/test", | |
| "console.openshift.io/route", | |
| } | |
| for _, key := range reservedKeys { | |
| if val, exists := annotations[key]; exists { | |
| t.Errorf("reserved annotation %q should have been filtered but found with value %q", key, val) | |
| } | |
| } | |
| // Verify only the allowed annotation exists (excluding any default annotations) | |
| userAnnotationCount := 0 | |
| for key := range annotations { | |
| if key == "allowed-annotation" { | |
| userAnnotationCount++ | |
| } | |
| } | |
| if userAnnotationCount != 1 { | |
| t.Errorf("expected exactly 1 user annotation, found user annotations: %v", annotations) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/controller/external_secrets/deployments_test.go` around lines 633 - 705,
The reserved-annotation assertion in the validateDeployment test currently only
fails when the annotation value equals "value", so change the check in the
validateDeployment closure (where reservedKeys is defined) to assert on presence
regardless of value: for each key in reservedKeys, test existence with something
like "if _, exists := annotations[key]; exists { t.Errorf(... ) }" so any
reserved annotation is flagged even if its value is not "value".
| - protocol: TCP | ||
| port: 5353 # DNS | ||
| - protocol: UDP | ||
| port: 5353 # DNS |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for DNS port configurations
rg -n "port:\s*(53|5353)" --type yaml -C 2Repository: openshift/external-secrets-operator
Length of output: 1810
🏁 Script executed:
# Also check for DNS-related egress rules with any port
rg -n "DNS|dns" --type yaml -C 3Repository: openshift/external-secrets-operator
Length of output: 24059
🏁 Script executed:
# Check what this test file is about
cat -n test/e2e/testdata/external_secrets_with_annotations.yaml | head -40Repository: openshift/external-secrets-operator
Length of output: 1128
Include port 53 in addition to port 5353 for DNS egress.
The reference network policy (bindata/external-secrets/networkpolicy_allow-dns.yaml) allows both port 5353 (OpenShift DNS daemonset) and port 53 (standard DNS). This test data includes only port 5353, which is incomplete. Add port 53 to match the production configuration and ensure DNS resolution works correctly in all scenarios.
🤖 Prompt for AI Agents
In `@test/e2e/testdata/external_secrets_with_annotations.yaml` around lines 24 -
27, The test network policy data only includes DNS port 5353; update the DNS
egress rules in external_secrets_with_annotations.yaml to also allow port 53 for
both TCP and UDP. Locate the DNS egress entries (the protocol: TCP/UDP blocks
listing port: 5353) and add corresponding entries with port: 53 so both "port:
53" and "port: 5353" are present for TCP and UDP to match the reference
networkpolicy_allow-dns.yaml.
fc0cd78 to
b65ff41
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
753-775: cert-manager.io entry now conflicts with the reserved-domain rule.
Line 764 still expectscert-manager.io/cluster-issuerto be accepted, but the CRD now rejects that domain. Remove it from the “allowed” case or add a failing test that asserts the rejection.🩹 Suggested edit (remove from allowed list)
vault.hashicorp.com/agent-inject: "true" - cert-manager.io/cluster-issuer: "letsencrypt"
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
752-775: Test includes annotation that should be rejected per the validation rules.The test expects
cert-manager.io/cluster-issuerto be allowed, but the error messages throughout this file (e.g., line 523) explicitly listcert-manager.io/as a reserved domain that is not allowed. This test will fail.Either remove the
cert-manager.io/cluster-issuerannotation from this test case, or if cert-manager.io annotations should genuinely be allowed, the error messages need to be updated to remove cert-manager.io from the reserved domains list.🐛 Proposed fix to remove the disallowed annotation
- name: Should allow common third-party annotation keys resourceName: cluster initial: | apiVersion: operator.openshift.io/v1alpha1 kind: ExternalSecretsConfig spec: controllerConfig: annotations: prometheus.io/scrape: "true" 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 spec: controllerConfig: annotations: prometheus.io/scrape: "true" prometheus.io/port: "8080" argocd.argoproj.io/sync-wave: "1" vault.hashicorp.com/agent-inject: "true" - cert-manager.io/cluster-issuer: "letsencrypt"
🧹 Nitpick comments (1)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
580-589: Consider adding subdomain tests for openshift.io and cert-manager.io for completeness.Tests exist for subdomains of
kubernetes.ioandk8s.io, but not foropenshift.ioorcert-manager.io. While the validation logic likely handles all domains uniformly, explicit tests would ensure complete coverage.
siddhibhor-56
force-pushed
the
eso-annotations
branch
from
b65ff41 to
2c1bdb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
753-775:cert-manager.iois now disallowed but still listed as allowedThe CRD rule now rejects
cert-manager.io/keys; this test should expect failure or remove the entry to avoid invalid objects passing here.🔧 Proposed fix (remove from allowlist test)
prometheus.io/scrape: "true" prometheus.io/port: "8080" argocd.argoproj.io/sync-wave: "1" vault.hashicorp.com/agent-inject: "true" - cert-manager.io/cluster-issuer: "letsencrypt" @@ prometheus.io/scrape: "true" prometheus.io/port: "8080" argocd.argoproj.io/sync-wave: "1" vault.hashicorp.com/agent-inject: "true" - cert-manager.io/cluster-issuer: "letsencrypt"
🤖 Fix all issues with AI agents
In `@pkg/controller/common/utils_test.go`:
- Around line 12-53: In TestFilterAnnotations the test calls
FilterReservedAnnotations(retained) so reserved keys are never removed; change
the invocation to call FilterReservedAnnotations(source) (in
TestFilterAnnotations) so the mixed map with reserved keys is filtered, then
assert the returned map equals retained; keep the same retained expected map and
compare filtered to retained using reflect.DeepEqual/cmp.Diff.
# 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
siddhibhor-56
force-pushed
the
eso-annotations
branch
from
2c1bdb9 to
372554b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
753-775:⚠️ Potential issue | 🟠 MajorTest expectation conflicts with the cert-manager.io disallowlist.
ControllerConfig.Annotations rejectscert-manager.io/keys, so this case should expect failure or drop the key.✅ Suggested adjustment
@@ - cert-manager.io/cluster-issuer: "letsencrypt" @@ - cert-manager.io/cluster-issuer: "letsencrypt"
🤖 Fix all issues with AI agents
In `@config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml`:
- Around line 1191-1197: The CEL rule for cert-manager domain restriction is
missing a closing parenthesis causing an invalid CRD schema; update the rule
string that currently reads "rule: self.all(key,
!key.matches('^(cert-manager\\.io)/')" by adding the missing closing parenthesis
so the expression is balanced (i.e., ends with "'))"); ensure the corrected rule
is applied where the cert-manager domain restriction rule is defined.
In `@docs/api_reference.md`:
- Line 204: Update the `annotations` property docs to include the
`cert-manager.io/` reserved domain (and its subdomains) in the list of
disallowed prefixes; specifically, amend the sentence that lists forbidden
prefixes (`kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, `k8s.io/`) to
also include `cert-manager.io/` and clarify that subdomains of these prefixes
are prohibited, so the `annotations` documentation accurately reflects the
validation enforced by the code.
In `@pkg/controller/common/utils.go`:
- Around line 231-255: The FilterReservedAnnotations function currently only
blocks the exact "cert-manager.io/" prefix but not subdomains like
"*.cert-manager.io/"; update the reserved checks so cert-manager subdomains are
filtered the same way as other domains by either adding "cert-manager.io" to the
reservedDomains slice (so the existing strings.HasPrefix(key, domain+"/") ||
strings.Contains(key, "."+domain+"/") logic covers it) or by extending the
explicit cert-manager check to also test strings.Contains(key,
".cert-manager.io/") before marking isReserved in FilterReservedAnnotations.
In `@pkg/controller/external_secrets/certificate_test.go`:
- Around line 444-483: The test "certificate with custom annotations applied
successfully" asserts the reserved key
"cert-manager.io/issue-temporary-certificate" is allowed, but controller
validation disallows cert-manager.io/*; update the test to use a non-reserved
annotation instead of changing validation. Concretely, in the test's preReq
CreateCalls assertion (inside the preReq func) and in the esc closure
(esc.Spec.ControllerConfig.Annotations), replace the reserved key with a
non-reserved key (e.g., "example.com/issue-temporary-certificate" or reuse
"team/owner") so the m.CreateCalls check and the provided esc annotations match
the allowed key. Ensure the assertion in the CreateCalls refers to that new key
name.
In `@pkg/controller/external_secrets/networkpolicy.go`:
- Around line 140-146: The ControllerConfig annotations are only applied to
static NetworkPolicy objects; ensure the same validated annotations from
esc.Spec.ControllerConfig.Annotations are applied to NetworkPolicies created by
buildNetworkPolicyFromConfig as well: call
validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) when
building custom policies (or accept an annotations map parameter in
buildNetworkPolicyFromConfig) and then invoke
common.UpdateResourceAnnotations(networkPolicy, annotationsMap) on the
NetworkPolicy returned/constructed by buildNetworkPolicyFromConfig so global
annotations are present on custom resources too.
🧹 Nitpick comments (2)
test/utils/conditions.go (1)
208-227: Fail fast on missing template variables.
text/templatedefaults can silently emit<no value>/zero values, which can hide fixture mistakes in e2e tests. Consider enablingmissingkey=errorto surface issues early.Suggested change
- tmpl, err := template.New("asset").Parse(string(fileContent)) + tmpl, err := template.New("asset").Option("missingkey=error").Parse(string(fileContent))test/e2e/e2e_test.go (1)
58-61: Remove unused type definition.
ExternalSecretsConfigTemplateis defined at lines 58–59 but never referenced anywhere in the codebase. Remove this struct definition.
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml
Outdated
Show resolved
Hide resolved
| | `certProvider` _[CertProvidersConfig](#certprovidersconfig)_ | certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook and plugins. | | Optional: \{\} <br /> | | ||
| | `labels` _object (keys:string, values:string)_ | labels to apply to all resources created for the external-secrets operand deployment.<br />This field can have a maximum of 20 entries. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> | | ||
| | `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.<br />Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. | | MaxItems: 20 <br />MinItems: 0 <br /> | | ||
| | `annotations` _object (keys:string, values:string)_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment.<br />The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.<br />Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> | |
There was a problem hiding this comment.
Document the cert-manager.io/ reserved domain in annotations.
The validation disallows cert-manager.io/ (and subdomains), but the docs omit it.
📝 Suggested doc update
-| `annotations` _object (keys:string, values:string)_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment.<br />The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.<br />Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> |
+| `annotations` _object (keys:string, values:string)_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment.<br />The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.<br />Annotation keys containing reserved domains `kubernetes.io/`, `openshift.io/`, `k8s.io/`, or `cert-manager.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `annotations` _object (keys:string, values:string)_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment.<br />The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.<br />Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> | | |
| | `annotations` _object (keys:string, values:string)_ | annotations are for adding custom annotations to all the resources created for external-secrets deployment.<br />The annotations are merged with any default annotations set by the operator. User-specified annotations take precedence over defaults in case of conflicts.<br />Annotation keys containing reserved domains `kubernetes.io/`, `openshift.io/`, `k8s.io/`, or `cert-manager.io/` (including subdomains like `*.kubernetes.io/`) are not allowed. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 204-204: Table column style
Table pipe has extra space to the right for style "compact"
(MD060, table-column-style)
[warning] 204-204: Table column style
Table pipe has extra space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@docs/api_reference.md` at line 204, Update the `annotations` property docs to
include the `cert-manager.io/` reserved domain (and its subdomains) in the list
of disallowed prefixes; specifically, amend the sentence that lists forbidden
prefixes (`kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, `k8s.io/`) to
also include `cert-manager.io/` and clarify that subdomains of these prefixes
are prohibited, so the `annotations` documentation accurately reflects the
validation enforced by the code.
| // 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"} | ||
|
|
||
| result := make(map[string]string, len(annotations)) | ||
| for key, value := range annotations { | ||
| isReserved := false | ||
|
|
||
| for _, domain := range reservedDomains { | ||
| // Checks for patterns like kubernetes.io/* and *.kubernetes.io/* only | ||
| if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") { | ||
| isReserved = true | ||
| break | ||
| } | ||
| if strings.HasPrefix(key, "cert-manager.io/") { | ||
| isReserved = true | ||
| break | ||
| } |
There was a problem hiding this comment.
Filter *.cert-manager.io/ subdomains as reserved.
Validation disallows cert-manager subdomains, but the filter only blocks the exact cert-manager.io/ prefix.
🔧 Suggested fix
-// Reserved domains: kubernetes.io, openshift.io, k8s.io
+// Reserved domains: kubernetes.io, openshift.io, k8s.io, cert-manager.io
@@
-reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io"}
+reservedDomains := []string{"kubernetes.io", "openshift.io", "k8s.io", "cert-manager.io"}
@@
- for _, domain := range reservedDomains {
- // Checks for patterns like kubernetes.io/* and *.kubernetes.io/* only
- if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") {
- isReserved = true
- break
- }
- if strings.HasPrefix(key, "cert-manager.io/") {
- isReserved = true
- break
- }
- }
+ for _, domain := range reservedDomains {
+ // Checks for patterns like kubernetes.io/* and *.kubernetes.io/*
+ if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") {
+ isReserved = true
+ break
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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"} | |
| result := make(map[string]string, len(annotations)) | |
| for key, value := range annotations { | |
| isReserved := false | |
| for _, domain := range reservedDomains { | |
| // Checks for patterns like kubernetes.io/* and *.kubernetes.io/* only | |
| if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") { | |
| isReserved = true | |
| break | |
| } | |
| if strings.HasPrefix(key, "cert-manager.io/") { | |
| isReserved = true | |
| break | |
| } | |
| // 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, cert-manager.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"} | |
| result := make(map[string]string, len(annotations)) | |
| for key, value := range annotations { | |
| isReserved := false | |
| for _, domain := range reservedDomains { | |
| // Checks for patterns like kubernetes.io/* and *.kubernetes.io/* | |
| if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") { | |
| isReserved = true | |
| break | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/controller/common/utils.go` around lines 231 - 255, The
FilterReservedAnnotations function currently only blocks the exact
"cert-manager.io/" prefix but not subdomains like "*.cert-manager.io/"; update
the reserved checks so cert-manager subdomains are filtered the same way as
other domains by either adding "cert-manager.io" to the reservedDomains slice
(so the existing strings.HasPrefix(key, domain+"/") || strings.Contains(key,
"."+domain+"/") logic covers it) or by extending the explicit cert-manager check
to also test strings.Contains(key, ".cert-manager.io/") before marking
isReserved in FilterReservedAnnotations.
| // Apply annotations from ControllerConfig | ||
| if len(esc.Spec.ControllerConfig.Annotations) > 0 { | ||
| annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log) | ||
| if len(annotationsMap) > 0 { | ||
| common.UpdateResourceAnnotations(networkPolicy, annotationsMap) | ||
| } | ||
| } |
There was a problem hiding this comment.
Apply ControllerConfig annotations to custom NetworkPolicies too.
This only updates static assets; custom NetworkPolicies built via buildNetworkPolicyFromConfig never receive global annotations, so user-specified annotations won’t apply to those resources.
Proposed fix
func (r *Reconciler) buildNetworkPolicyFromConfig(esc *operatorv1alpha1.ExternalSecretsConfig, npConfig operatorv1alpha1.NetworkPolicy, resourceLabels map[string]string) (*networkingv1.NetworkPolicy, error) {
@@
networkPolicy := &networkingv1.NetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: npConfig.Name,
Namespace: namespace,
Labels: resourceLabels,
},
@@
}
+
+ if len(esc.Spec.ControllerConfig.Annotations) > 0 {
+ annotationsMap := validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log)
+ if len(annotationsMap) > 0 {
+ common.UpdateResourceAnnotations(networkPolicy, annotationsMap)
+ }
+ }
return networkPolicy, nil
}🤖 Prompt for AI Agents
In `@pkg/controller/external_secrets/networkpolicy.go` around lines 140 - 146, The
ControllerConfig annotations are only applied to static NetworkPolicy objects;
ensure the same validated annotations from esc.Spec.ControllerConfig.Annotations
are applied to NetworkPolicies created by buildNetworkPolicyFromConfig as well:
call validateAndFilterAnnotations(esc.Spec.ControllerConfig.Annotations, r.log)
when building custom policies (or accept an annotations map parameter in
buildNetworkPolicyFromConfig) and then invoke
common.UpdateResourceAnnotations(networkPolicy, annotationsMap) on the
NetworkPolicy returned/constructed by buildNetworkPolicyFromConfig so global
annotations are present on custom resources too.
372554b to
009f9e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/v1alpha1/external_secrets_config_types.go`:
- Around line 123-124: The validation annotation message for the XValidation
rule on the kubebuilder tag that uses the regex
'^([^/]*\\.)?(kubernetes\\.io|k8s\\.io|openshift\\.io)/' is inaccurate because
it mentions 'cert-manager.io/' even though cert-manager.io is not included in
that regex; update the message text on that annotation (the comment containing
"+kubebuilder:validation:XValidation:rule=\"self.all(key,
!key.matches('^([^/]*\\\\.)?(kubernetes\\\\.io|k8s\\\\.io|openshift\\\\.io)/'))\",message=...")
to remove any reference to 'cert-manager.io/' so it reflects only the domains
actually checked, leaving the separate cert-manager.io rule (the next
XValidation annotation) unchanged.
🧹 Nitpick comments (2)
test/e2e/e2e_test.go (1)
58-61: Unused type definition.
ExternalSecretsConfigTemplateis defined but not used anywhere in this file. Consider removing it if it's not needed.♻️ Suggested removal
-// ExternalSecretsConfigTemplate represents the template data for ExternalSecretsConfig -type ExternalSecretsConfigTemplate struct { - Annotations map[string]string -} -pkg/controller/common/utils.go (1)
246-256: Move cert-manager.io check outside the loop for efficiency.The
cert-manager.io/prefix check on lines 252-255 is inside thereservedDomainsloop, causing it to be evaluated 3 times per annotation key. This is a minor inefficiency but easy to fix.♻️ Suggested fix
for key, value := range annotations { isReserved := false + // Check cert-manager.io separately (exact prefix only, no subdomain) + if strings.HasPrefix(key, "cert-manager.io/") { + isReserved = true + } + for _, domain := range reservedDomains { // Checks for patterns like kubernetes.io/* and *.kubernetes.io/* only if strings.HasPrefix(key, domain+"/") || strings.Contains(key, "."+domain+"/") { isReserved = true break } - if strings.HasPrefix(key, "cert-manager.io/") { - isReserved = true - break - } } if !isReserved {
siddhibhor-56
force-pushed
the
eso-annotations
branch
2 times, most recently
from
4ce0658 to
c58041a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/e2e_test.go`:
- Around line 319-446: The test currently overwrites
Spec.ControllerConfig.Annotations on the ExternalSecretsConfig (via
runtimeClient.Get + updatedCR := existingCR.DeepCopy();
updatedCR.Spec.ControllerConfig.Annotations = testAnnotations) which can drop
pre-existing annotations; change it to capture originalAnnotations :=
existingCR.Spec.ControllerConfig.Annotations (nil-safe), merge the
testAnnotations into a copy when updating (instead of assigning), and in the
deferred cleanup restore the originalAnnotations (replacing the whole map)
rather than only deleting test keys so pre-existing annotations are preserved;
reference runtimeClient, ExternalSecretsConfig, existingCR, updatedCR,
Spec.ControllerConfig.Annotations and the defer cleanup block when making the
change.
| 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") | ||
| }) |
There was a problem hiding this comment.
Preserve and restore existing annotations to avoid test pollution.
The update replaces the entire annotations map, and cleanup only removes test keys, which can drop pre-existing config and affect later tests. Capture the original annotations, merge the test keys, and restore on cleanup.
🛠️ Suggested fix
- 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)
- })
+ var originalAnnotations map[string]string
+ err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ existingCR := &operatorv1alpha1.ExternalSecretsConfig{}
+ if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, existingCR); err != nil {
+ return err
+ }
+ if originalAnnotations == nil {
+ originalAnnotations = map[string]string{}
+ for k, v := range existingCR.Spec.ControllerConfig.Annotations {
+ originalAnnotations[k] = v
+ }
+ }
+
+ updatedCR := existingCR.DeepCopy()
+ if updatedCR.Spec.ControllerConfig.Annotations == nil {
+ updatedCR.Spec.ControllerConfig.Annotations = map[string]string{}
+ }
+ for key, value := range testAnnotations {
+ updatedCR.Spec.ControllerConfig.Annotations[key] = value
+ }
+
+ return runtimeClient.Update(ctx, updatedCR)
+ })
@@
- for key := range testAnnotations {
- delete(currentCR.Spec.ControllerConfig.Annotations, key)
- }
+ if currentCR.Spec.ControllerConfig.Annotations == nil {
+ currentCR.Spec.ControllerConfig.Annotations = map[string]string{}
+ }
+ for key := range currentCR.Spec.ControllerConfig.Annotations {
+ delete(currentCR.Spec.ControllerConfig.Annotations, key)
+ }
+ for key, value := range originalAnnotations {
+ currentCR.Spec.ControllerConfig.Annotations[key] = value
+ }🤖 Prompt for AI Agents
In `@test/e2e/e2e_test.go` around lines 319 - 446, The test currently overwrites
Spec.ControllerConfig.Annotations on the ExternalSecretsConfig (via
runtimeClient.Get + updatedCR := existingCR.DeepCopy();
updatedCR.Spec.ControllerConfig.Annotations = testAnnotations) which can drop
pre-existing annotations; change it to capture originalAnnotations :=
existingCR.Spec.ControllerConfig.Annotations (nil-safe), merge the
testAnnotations into a copy when updating (instead of assigning), and in the
deferred cleanup restore the originalAnnotations (replacing the whole map)
rather than only deleting test keys so pre-existing annotations are preserved;
reference runtimeClient, ExternalSecretsConfig, existingCR, updatedCR,
Spec.ControllerConfig.Annotations and the defer cleanup block when making the
change.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1168-1197:⚠️ Potential issue | 🟠 MajorBlock cert-manager subdomains in annotation validation.
The rule on Line 1197 only rejects
cert-manager.io/, sofoo.cert-manager.io/would pass even though reserved-domain filtering is intended. Align it with the other domain-based rule.🛠️ Proposed fix
- rule: self.all(key, !key.matches('^(cert-manager\.io)/')) + rule: self.all(key, !key.matches('^([^/]*\\.)?cert-manager\\.io/'))bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1168-1197:⚠️ Potential issue | 🟠 MajorKeep manifest validation strict for cert-manager subdomains.
The manifest rule also only blocks
cert-manager.io/, sofoo.cert-manager.io/slips through. Please mirror the domain-based restriction used for other reserved domains.🛠️ Proposed fix
- rule: self.all(key, !key.matches('^(cert-manager\.io)/')) + rule: self.all(key, !key.matches('^([^/]*\\.)?cert-manager\\.io/'))
siddhibhor-56
force-pushed
the
eso-annotations
branch
from
c58041a to
7a3b7dc
Compare
|
@siddhibhor-56: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
I haven't taken a look at coderabbit provided suggestions. Could you please address valid suggestions by it.
| 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" |
There was a problem hiding this comment.
nit: no actual change here, not required to commit
| images: | ||
| - name: controller | ||
| newName: openshift.io/external-secrets-operator | ||
| newTag: latest |
There was a problem hiding this comment.
why is this field removed?
| for k, v := range annotations { | ||
| a[k] = v | ||
| } |
There was a problem hiding this comment.
This will be flagged by linter. We need make similar change here as well.
| custom.company.io/team: "platform" | ||
| custom.company.io/app: "external-secrets" | ||
| - name: Should fail with annotation key starting with kubernetes.io | ||
| - name: Should allow custom annotations with cert-manager subdomain |
There was a problem hiding this comment.
I think below scenarios could be also added
Should not allow annotation key with subdomain of openshift.io
Should not allow annotation key with deep subdomain of kubernetes.io
Should allow annotation key similar to k8s.io but different domain
Should allow annotation key similar to cert-manager.io but different domain
Should allow reserved domain without trailing slash
|
|
||
| default: | ||
| panic(fmt.Sprintf("unsupported object type: %T", desired)) | ||
| return ObjectMetadataModified(desired, fetched) |
There was a problem hiding this comment.
Why is this change required?
There was a problem hiding this comment.
The change from panic to ObjectMetadataModified in the default case is intentional as without this change Service Account resource panics and cannot update annotations, which is not handled by the switch case.
There was a problem hiding this comment.
Then we need to add a case for ServiceAccount isn't, instead of changing the behavior?
| return make(map[string]string) | ||
| } | ||
|
|
||
| // Filter reserved domains kubernetes.io/*, *.kubernetes.io/*, openshift.io/*, *.openshift.io/*, etc. |
There was a problem hiding this comment.
IMO, this is not required.
| if a == nil { | ||
| a = make(map[string]string, len(annotations)) | ||
| } | ||
| for k, v := range annotations { |
There was a problem hiding this comment.
Same linter suggestion here as well.
| // 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 { |
There was a problem hiding this comment.
I thought we were going to refactor here, to pass common arg which contains both labels and annotations.
| common.UpdateResourceLabels(clusterRole, resourceLabels) | ||
|
|
||
| // Apply annotations from ControllerConfig | ||
| if len(esc.Spec.ControllerConfig.Annotations) > 0 { |
There was a problem hiding this comment.
This code is repeating at multiple places, which can be avoided with refactoring I feel.
| common.UpdateResourceLabels(service, resourceLabels) | ||
|
|
||
| // Apply annotations from ControllerConfig | ||
| if len(esc.Spec.ControllerConfig.Annotations) > 0 { |
There was a problem hiding this comment.
Please refactor this repeating everywhere.
3 participants
This PR adds support for custom annotations in
ExternalSecretsConfig, allowing users to add custom metadata annotations to all resources created by the external-secrets-operator.