fix: for target template parsing for complex matchers#5735
fix: for target template parsing for complex matchers#5735Skarlso merged 4 commits intoexternal-secrets:mainfrom
Conversation
WalkthroughController pre-fetches an existing generic target resource for CreationPolicy values (Merge, Orphan, Owner) and passes that resource as an optional base object into template application, enabling merge-style manifest construction and updated error handling. Changes
Sequence Diagram(s)sequenceDiagram
actor Ctrl as ExternalSecret Controller
participant K8s as Kubernetes API
participant Tmpl as applyTemplateToManifest
Ctrl->>K8s: GET target resource (if CreationPolicy is Merge/Orphan/Owner)
alt resource exists
K8s-->>Ctrl: 200 existing resource
Ctrl->>Tmpl: applyTemplateToManifest(ctx, es, dataMap, existingObj)
Tmpl->>Tmpl: deep copy existingObj -> baseObj
Tmpl->>Tmpl: render template and merge into baseObj
Tmpl-->>Ctrl: merged manifest
Ctrl->>K8s: UPDATE resource (using baseObj UID/resourceVersion)
else not found
K8s-->>Ctrl: 404 NotFound
Ctrl->>Tmpl: applyTemplateToManifest(ctx, es, dataMap, nil)
Tmpl->>Tmpl: build new object from template (set GVK/name/namespace)
Tmpl-->>Ctrl: new manifest
Ctrl->>K8s: CREATE resource
else fetch error
K8s-->>Ctrl: error
Ctrl->>Ctrl: mark reconcile failed / record error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
308fb38 to
2d0fed3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (1)
170-193: Consider reordering logic to avoid unnecessary assignments when existingObj is provided.When
existingObjis provided, the initial assignments on lines 176-179 (setting GVK, Name, Namespace) are immediately discarded by theobj = existingObj.DeepCopy()on line 183. While functionally correct since the existing object already contains this metadata, this could be clearer:func (r *Reconciler) applyTemplateToManifest(ctx context.Context, es *esv1.ExternalSecret, dataMap map[string][]byte, existingObj *unstructured.Unstructured) (*unstructured.Unstructured, error) { gvk := getTargetGVK(es) - obj := &unstructured.Unstructured{} - obj.SetGroupVersionKind(gvk) - obj.SetName(getTargetName(es)) - obj.SetNamespace(es.Namespace) - + var obj *unstructured.Unstructured if existingObj != nil { // use the existing object for merge behavior if it exists obj = existingObj.DeepCopy() + } else { + obj = &unstructured.Unstructured{} + obj.SetGroupVersionKind(gvk) + obj.SetName(getTargetName(es)) + obj.SetNamespace(es.Namespace) }This makes it explicit that the metadata initialization only happens for new objects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/controllers/externalsecret/externalsecret_controller.go(2 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (1)
apis/externalsecrets/v1/externalsecret_types.go (5)
ExternalSecret(676-682)ManifestReference(200-210)ExternalSecretTemplate(95-116)TemplateEngineV2(135-135)TemplateFrom(140-154)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (2)
pkg/controllers/externalsecret/externalsecret_controller.go (1)
Reconciler(138-154)apis/externalsecrets/v1/externalsecret_types.go (1)
ExternalSecret(676-682)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: Analyze project (actions, none)
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (9)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (1)
186-207: LGTM!The label and annotation handling correctly:
- Preserves existing values from the base object when provided
- Initializes empty maps when needed
- Merges template metadata on top
- Always applies the managed label
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (4)
372-372: LGTM!Test signature correctly updated to pass
nilfor the newexistingObjparameter, maintaining the original test behavior.
434-434: LGTM!Test signature correctly updated.
603-603: LGTM!Test signature correctly updated.
631-723: LGTM! Comprehensive merge behavior test.The test effectively validates that:
- Pre-existing fields (
spec.type,spec.slack.channel,spec.slack.username) are preserved- Template-derived fields (
spec.slack.api_url) are correctly added- The existing object's metadata (resourceVersion, uid) is preserved through DeepCopy
pkg/controllers/externalsecret/externalsecret_controller.go (4)
36-36: LGTM!Import added for the new
unstructured.Unstructuredusage.
650-661: LGTM!The existing resource fetch logic correctly:
- Only fetches for policies that need it (Merge, Orphan, Owner)
- Treats NotFound as a valid state (resource doesn't exist yet)
- Fails reconciliation for other errors
663-671: Verify: Template merge behavior only for CreatePolicyMerge.The
baseObjis only set forCreatePolicyMerge, meaning:
- Merge policy: Templates are applied to the existing object, preserving existing fields
- Orphan/Owner policies: Templates are applied to a fresh object (though ResourceVersion/UID are copied later for updates)
This appears intentional for "complex matchers" fix mentioned in the PR title, but please confirm this matches the expected behavior for Orphan/Owner policies.
683-700: LGTM!The creation policy handling is correct:
- Merge policy returns early if resource doesn't exist (lines 685-688)
- Orphan/Owner policies correctly propagate ResourceVersion and UID from existing resource to enable optimistic concurrency control on updates (lines 694-695)
- Creates new resource if it doesn't exist (line 698)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (1)
631-716: Good test coverage for the merge behavior fix.This test validates the core fix for issue #5587 by:
- Setting up an existing resource with pre-populated fields (
spec.type,spec.slack.channel,spec.slack.username)- Applying a template targeting a nested path (
spec.slack) with a new field (api_url)- Verifying that existing fields are preserved while the template-derived field is correctly added
Consider adding a test case for the scenario where
existingObjhas nospecfield at all, to verify the merge handles initialization of nested paths correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/controllers/externalsecret/externalsecret_controller.go(2 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/externalsecret/externalsecret_controller.go (2)
apis/externalsecrets/v1beta1/externalsecret_types.go (3)
CreatePolicyMerge(52-52)CreatePolicyOrphan(49-49)CreatePolicyOwner(45-45)apis/externalsecrets/v1/externalsecret_types.go (4)
CreatePolicyMerge(52-52)CreatePolicyOrphan(49-49)CreatePolicyOwner(45-45)ConditionReasonResourceSyncedError(636-636)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (8)
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (3)
372-372: LGTM!Correctly updated to pass
nilfor the newexistingObjparameter when no pre-existing resource should be used.
434-434: LGTM!Correctly updated to pass
nilfor the newexistingObjparameter.
603-603: LGTM!Correctly updated to pass
nilfor the newexistingObjparameter.pkg/controllers/externalsecret/externalsecret_controller_manifest.go (1)
170-194: LGTM - Clean implementation of merge behavior.The implementation correctly:
- Uses
DeepCopy()to avoid mutating the original existing object- Preserves labels and annotations from the existing resource
- Ensures non-nil maps for labels/annotations before manipulation
- Always adds the managed label for tracking
Minor observation: Lines 176-179 set GVK, Name, and Namespace on
objbefore theexistingObjcheck, which are then overwritten by the deep copy. This is harmless but slightly redundant whenexistingObjis provided.pkg/controllers/externalsecret/externalsecret_controller.go (4)
36-36: LGTM!The
unstructuredimport is correctly added to support the newbaseObjparameter of type*unstructured.Unstructured.
650-668: LGTM! Fetching existing resource for base object handling.The logic correctly:
- Fetches the existing resource for Merge/Orphan/Owner policies
- Only passes the existing resource as
baseObjfor Merge policy, enabling templates to be applied on top of existing data- Continues to use
existingin the switch statement for ResourceVersion/UID propagation in Orphan/Owner pathsThis addresses the issue where templates were creating new objects instead of merging into existing resources.
683-700: LGTM! Switch statement correctly integrates with the fetched resource.The integration is correct:
- Merge: Uses
obj(template applied on existing resource) for update, or returns early if resource doesn't exist- Orphan/Owner: Preserves ResourceVersion/UID from existing resource for updates, or creates new resource
This maintains the previous behavior for Orphan/Owner while enabling proper merge behavior for the Merge policy.
671-675:applyTemplateToManifestcorrectly handles nilbaseObj.The function implementation (lines 181-184) explicitly checks for nil and creates a fresh object when
existingObjis nil. For non-nil values (Merge policy with existing resources), it deep copies the existing object. Both code paths are properly handled.
| annotations := make(map[string]string) | ||
| if existingObj != nil { | ||
| // use the existing object for merge behavior if it exists | ||
| obj = existingObj.DeepCopy() |
There was a problem hiding this comment.
Im not exactly sure but is the existingObj set with GVK or does that still need to be set here?
There was a problem hiding this comment.
Existing Object here comes from this function:
func (r *Reconciler) getGenericResource(ctx context.Context, log logr.Logger, es *esv1.ExternalSecret) (*unstructured.Unstructured, error) {
gvk := getTargetGVK(es)
resource := &unstructured.Unstructured{}
resource.SetGroupVersionKind(gvk)There, the GVK is being set.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
624d739 to
1399c8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (1)
631-716: Excellent test coverage for the merge behavior fix.This test directly validates the fix for issue #5587 by verifying that:
- Existing fields (
spec.type,spec.slack.channel,spec.slack.username) are preserved- Template-derived fields (
spec.slack.api_url) are correctly addedThe test effectively proves the template targeting
spec.slackwithapi_url: {{ .url }}no longer produces duplicate path segments likespec.slack.slack.Consider adding assertions for metadata preservation
The existing resource has
resourceVersionanduidset. SinceDeepCopyis used, these should be preserved. Adding assertions would strengthen confidence in the implementation:+ // Verify metadata is preserved from existing resource + assert.Equal(t, "12345", result.GetResourceVersion(), "resourceVersion should be preserved") + assert.Equal(t, "test-uid-123", string(result.GetUID()), "uid should be preserved") + t.Logf("Result spec: %+v", result.Object["spec"])
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/controllers/externalsecret/externalsecret_controller.go(2 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (1)
apis/externalsecrets/v1/externalsecret_types.go (1)
ExternalSecret(676-682)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: unit-tests
- GitHub Check: check-diff
- GitHub Check: lint
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (4)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (1)
170-193: LGTM! Clean implementation of the merge behavior.The optional
existingObjparameter allows templates to be applied to an existing resource for merge scenarios. The implementation correctly:
- Uses
DeepCopyto avoid mutating the original object- Initializes labels/annotations maps when nil to prevent nil map assignment panics
- Falls back to creating a new object when
existingObjis nilpkg/controllers/externalsecret/externalsecret_controller.go (2)
650-671: LGTM! Well-structured pre-fetch and merge logic.The implementation correctly:
- Pre-fetches existing resources for all policies that may need them (Merge, Orphan, Owner)
- Only passes the existing resource as
baseObjfor Merge policy, preserving the distinct semantics:
- Merge: Apply template on top of existing resource (preserves existing fields)
- Orphan/Owner: Replace resource entirely with template output (uses existing only for ResourceVersion/UID)
- Properly handles NotFound errors without failing the reconciliation
683-699: Logic for Merge vs Orphan/Owner is correct.For Merge (lines 683-691): The
objwas built on top of the existing resource, preserving fields not touched by the template.For Orphan/Owner (lines 692-699): The
objwas built from scratch, and only theResourceVersion/UIDare copied from the existing resource to enable the update operation. This correctly implements a "replace" semantic rather than a merge.pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (1)
372-372: Updated call signature looks correct.Passing
nilfor the newexistingObjparameter maintains the original behavior for these tests.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
1399c8b to
3a35bea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (1)
631-717: Consider additional edge case tests (optional).While the current test comprehensively validates the merge behavior, you might consider adding tests for:
- Overwriting existing fields: Template sets a field that already exists in
existingObj(e.g., changingspec.slack.channel)- Creating new nested paths: Template targets a path that doesn't exist in
existingObj(e.g.,spec.newSection.field)These would provide additional confidence in the template engine's merge semantics, though they can be deferred if the engine is already tested separately.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (2)
pkg/controllers/externalsecret/externalsecret_controller.go (1)
Reconciler(138-154)apis/externalsecrets/v1/externalsecret_types.go (1)
ExternalSecret(676-682)
pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (1)
apis/externalsecrets/v1/externalsecret_types.go (7)
ExternalSecret(676-682)ExternalSecretSpec(503-535)ExternalSecretTarget(214-249)ManifestReference(200-210)ExternalSecretTemplate(95-116)TemplateEngineV2(135-135)TemplateFrom(140-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: lint
- GitHub Check: check-diff
- GitHub Check: unit-tests
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (4)
pkg/controllers/externalsecret/externalsecret_controller_manifest.go (2)
171-184: LGTM: Merge behavior implementation is correct.The function now properly supports merge-based template application by accepting an optional
existingObj. The logic correctly:
- Uses
DeepCopy()to prevent mutation of the existing resource- Falls back to creating a new object when
existingObjis nil- Preserves metadata like resourceVersion and UID from the existing resource
This addresses the issue described in #5587 where path-targeted templates were producing "unknown field" errors due to incorrect path construction.
186-193: LGTM: Safe initialization of labels and annotations.The nil checks ensure that
GetLabels()andGetAnnotations()returning nil don't cause panics when attempting to merge template metadata in subsequent lines.pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go (2)
372-372: LGTM: Test calls updated correctly.All existing test calls properly pass
nilfor the newexistingObjparameter, preserving the original behavior where templates are applied to newly created objects.Also applies to: 434-434, 603-603
631-717: Excellent test coverage for merge behavior!This test validates the fix for issue #5587 by:
- Creating an existing resource with pre-populated fields (
spec.type,spec.slack.channel,spec.slack.username)- Applying a template targeting
spec.slackwith a newapi_urlfield- Verifying that existing fields are preserved while the new field is added
- Ensuring metadata like
resourceVersionandUIDare preservedThis directly addresses the reported problem where targeting
spec.slackwould incorrectly create paths likespec.slack.slackorspec.slack.api_url.url.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/externalsecret/externalsecret_controller.go (1)
650-668: Pre-fetch and baseObj logic correctly addresses the Merge policy requirements.The logic properly:
- Pre-fetches existing resource for policies that need it (Merge/Orphan/Owner)
- Sets
baseObjonly for Merge policy when the resource exists- Keeps
baseObjnil for Owner/Orphan, ensuring templates apply to fresh objectsThis correctly addresses the concern from previous review about accidentally causing a merge for Owner policy.
One optional consideration: Line 666 could also check
existing.GetUID() != ""for extra defensive coding, though this is handled downstream at line 685 for Merge policy.🔎 Optional defensive check
var baseObj *unstructured.Unstructured -if externalSecret.Spec.Target.CreationPolicy == esv1.CreatePolicyMerge && existing != nil { +if externalSecret.Spec.Target.CreationPolicy == esv1.CreatePolicyMerge && existing != nil && existing.GetUID() != "" { baseObj = existing }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/controllers/externalsecret/externalsecret_controller.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/externalsecret/externalsecret_controller.go (2)
apis/externalsecrets/v1/externalsecret_types.go (4)
CreatePolicyMerge(52-52)CreatePolicyOrphan(49-49)CreatePolicyOwner(45-45)ConditionReasonResourceSyncedError(636-636)apis/externalsecrets/v1beta1/externalsecret_types.go (3)
CreatePolicyMerge(52-52)CreatePolicyOrphan(49-49)CreatePolicyOwner(45-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: unit-tests
- GitHub Check: check-diff
- GitHub Check: lint
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (2)
pkg/controllers/externalsecret/externalsecret_controller.go (2)
36-36: Import addition looks good.The
unstructuredimport is correctly added to support the newbaseObjparameter used in the template application logic.
670-675: PassingbaseObjtoapplyTemplateToManifestfixes template path targeting.The implementation correctly handles the nil case for non-Merge policies and applies templates onto the existing resource for Merge policy, resolving the duplicated field path segments reported in issue #5587.
|
Problem Statement
What is the problem you're trying to solve?
Related Issue
Fixes #5587
Proposed Changes
How do you like to solve the issue and why?
Format
Please ensure that your PR follows the following format for the title:
Where
scopeis optionally one of:Checklist
git commit --signoffmake testmake reviewableSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.