fix: Preserve nested struct references on readback#718
Conversation
Cross-resource references nested inside struct fields were dropped from the resource spec after the controller read the resource back from AWS. References are preserved across reconciles by ClearResolvedReferences, which only clears a concrete value when the sibling *Ref field is present on the latest resource. Setting a resource's fields from an API response rebuilds nested struct fields from scratch and reassigns the parent struct, which discards the *Ref values the user supplied. The response carries only the concrete value (e.g. an ARN) and has no concept of the *Ref field, so the references were lost. Top-level references are unaffected because the *Ref field is a sibling that is never touched when setting fields from the response. Generate code that copies nested *Ref values back from the original object into the rebuilt resource after the response is mapped, creating parent structs where needed. This makes nested references behave like top-level references. The code is emitted for the create, update, read-one, and read-many output paths. Top-level references are skipped (already preserved) and references nested within lists are skipped (their concrete values are restored by index during resolution).
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gustavodiaz7722 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 |
Manual Validation ResultsBuilt this branch's Test subject
What the regeneration producedRegenerating the controller with this branch added preservation blocks to // readback (sdkFind)
if r.ko.Spec.LambdaConfig != nil && r.ko.Spec.LambdaConfig.PreSignUpRef != nil {
if ko.Spec.LambdaConfig == nil {
ko.Spec.LambdaConfig = &svcapitypes.LambdaConfigType{}
}
ko.Spec.LambdaConfig.PreSignUpRef = r.ko.Spec.LambdaConfig.PreSignUpRef
}
// update (sdkUpdate)
if desired.ko.Spec.LambdaConfig != nil && desired.ko.Spec.LambdaConfig.PreSignUpRef != nil {
...
ko.Spec.LambdaConfig.PreSignUpRef = desired.ko.Spec.LambdaConfig.PreSignUpRef
}ResultsEach field PASSes only if:
Create / readback ( Update path — changed a non-reference mutable field ( AWS-side confirmation ( {
"MinLen": 12,
"Lambda": {
"PreSignUp": "arn:aws:lambda:us-west-2:<acct>:function:ack-test-cognito-fn",
"CustomMessage": "arn:aws:lambda:us-west-2:<acct>:function:ack-test-cognito-fn",
"PostConfirmation": "arn:aws:lambda:us-west-2:<acct>:function:ack-test-cognito-fn",
"PreAuthentication": "arn:aws:lambda:us-west-2:<acct>:function:ack-test-cognito-fn",
"PostAuthentication": "arn:aws:lambda:us-west-2:<acct>:function:ack-test-cognito-fn"
}
}No perpetual diff after convergence ( Scope noteI also regenerated Reproduction
|
Move the per-field *Ref restoration out of the inlined SDK templates and into a single generated preserveReferenceFields method in references.go, alongside the other reference passes (ResolveReferences, ClearResolvedReferences). The create, update, read-one, and read-many paths now call rm.preserveReferenceFields(<original>, ko) instead of carrying a duplicated block, which keeps the generated sdk.go lean and puts the logic next to the related reference helpers. No behavior change: the generated copy logic is identical, only its location and the source/target variable names (from/to) differ.
When the reference is set, its resolved concrete value was applied to AWS and read back, so the target's ancestor structs already exist. Instead of constructing empty ancestor structs on the target, nil-check them in the guard condition and assign directly. This drops the referenceParentGoType helper and its imports.
| {{ $hookCode }} | ||
| {{- end }} | ||
| {{ GoCodeSetUpdateOutput .CRD "resp" "ko" 1 }} | ||
| {{- if .CRD.HasReferenceFields }} |
There was a problem hiding this comment.
There's a good number of resources that use a fully custom update function which won't be impacted by updating this template. Would it be possible to apply this in the ACK runtime?
There was a problem hiding this comment.
Good point, we should create a universal fix that will apply regardless if we have custom update function. I've refactored current implementation to use optional interface approach in runtime.
| {{ $hookCode }} | ||
| {{- end }} | ||
| {{ GoCodeSetReadOneOutput .CRD "resp" "ko" 1 }} | ||
| {{- if .CRD.HasReferenceFields }} |
There was a problem hiding this comment.
Note, some fields may be set in either in the setOutputCustomMethodName function or in a sdk_read_one_post_set_output hook.
Generate an exported PreserveReferenceValues method on the resource manager instead of injecting the *Ref copy into each SDK output path. The ACK runtime calls this method from its single spec-patch chokepoint, which also covers resources with fully custom sdk implementations that bypass the generated output-setting code (and so never reached the previously injected call). The method satisfies the runtime's optional ReferenceValuesPreserver interface. Remove the per-template calls from the create, update, read-one, and read-many paths.
When a resource has references but none are nested inside structs (all top-level), the preservation body is empty and the generated method declared `fromKO` without using it, failing to compile (e.g. the IAM controller's Group, Role, User, and InstanceProfile resources). Capture the generated body in the template and emit the method only when it is non-empty. Resources with only top-level references no longer get the method, which is correct: top-level references are preserved by the caller's deep copy and the runtime simply skips the optional interface.
|
/retest |
2 participants
Description
Cross-resource references that live inside a nested struct field are dropped from the resource's
.specafter the controller reads the resource back from AWS. References resolve correctly (ACK.ReferencesResolved=True) and the resolved value is applied to AWS, but the*Reffield disappears from the spec after readback.Root cause
References are preserved across reconciles by the generated
ClearResolvedReferences, which is called onlatestin the runtime'spatchResourceMetadataAndSpec. It only clears a concrete value when the sibling*Reffield is present on the resource.Setting a resource's fields from an API response (
SetResource) rebuilds nested struct fields from scratch and reassigns the parent struct:Because the response carries only the concrete value (e.g. an ARN) and has no concept of the
*Reffield, the*Refvalues the user supplied are discarded. With no*Reffield onlatest,ClearResolvedReferencescan't clear the concrete value and the subsequent merge patch writes the concrete value while dropping the*Reffield.Top-level references are unaffected, because the
*Reffield is a sibling that is never touched when setting fields from the response. Nested references break because the whole parent struct is reassigned. This is the same class of issue seen with ECRencryptionConfiguration.kmsKeyRef, EKScomputeConfig.nodeRoleRef, CloudFrontdistributionConfig.webACLRef, and OpenSearchcognitoOptions.*Ref.Change
Add a
PreserveReferenceFieldscode generator that emits code to copy nested*Refvalues back from the original object into the rebuilt resource after the response is mapped, creating parent structs as needed:This makes nested references behave like top-level references:
latestcarries both the*Refand the concrete value, soClearResolvedReferencesdoes its job and the patch preserves the reference.The code is emitted for the create, update, read-one, and read-many output paths, guarded by
HasReferenceFields. Top-level references are skipped (already preserved) and references nested within lists are skipped (their concrete values are restored by index during resolution).Testing
go build ./...,gofmt, andgo vetare clean.Test_ReferenceFieldsPreservation_NestedReference; the fullpkg/generate/...suite passes.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.