feat: generate EnsureReferences to preserve nested cross-resource references - #738
Conversation
|
[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 |
e861ec6 to
5cedd00
Compare
A cross-resource reference (*Ref) is generated as a sibling of the concrete field it resolves into. A resource manager builds its return value from an AWS API response, which has no concept of a reference, so rebuilding the containing struct drops every *Ref inside it. That disables ClearResolvedReferences, which suppresses a resolved value only while the sibling *Ref is visible, so the spec patch deletes the declared *Ref and stores the resolved value in its place. The next apply of the manifest puts the *Ref back beside that value, a pair validateReferenceFields rejects, stopping reconciliation. Add an optional ReferenceEnsurer interface and invoke it on the object a resource manager hands back from Create and from Update, sourcing the references from the declared resource. It is kept separate from ReferenceManager and reached through a type assertion, so controllers generated before the method existed still satisfy AWSResourceManager and compile unchanged; they opt in by regenerating. The source is `desired`, not `reconcileDesired`: the latter is handed to Update, and a manager may mutate what it is given, so it is not a reliable record of what the user declared. The restoration is not hooked into patchResourceMetadataAndSpec because the late-initialization patch uses the AWS-observed object as its base, which carries no references. Pairs with aws-controllers-k8s/code-generator#738, which generates the method. Issue aws-controllers-k8s/community#2361 Issue aws-controllers-k8s/community#2431
A cross-resource reference (*Ref) is generated as a sibling of the concrete field it resolves into. A resource manager builds its return value from an AWS API response, which has no concept of a reference, so rebuilding the containing struct drops every *Ref inside it. That disables ClearResolvedReferences, which suppresses a resolved value only while the sibling *Ref is visible, so the spec patch deletes the declared *Ref and stores the resolved value in its place. The next apply of the manifest puts the *Ref back beside that value, a pair validateReferenceFields rejects, stopping reconciliation. Generate an EnsureReferences method that restores such a reference from the declared resource. Only a reference reached through structs is emitted, at its one fixed address, so every value the service reported stands. A top-level *Ref is skipped because it cannot be lost. One reached through a list is also skipped and stays as it is today: it has no fixed address, and replacing the whole outermost list instead would discard whatever the service populated inside it. Requires the runtime's optional ReferenceEnsurer interface, which invokes the method after Create and after Update. Controllers generated before the method existed are unaffected and opt in by regenerating. Issue aws-controllers-k8s/community#2361 Issue aws-controllers-k8s/community#2431
5cedd00 to
8ccc75d
Compare
A cross-resource reference (*Ref) is generated as a sibling of the concrete field it resolves into. A resource manager builds its return value from an AWS API response, which has no concept of a reference, so rebuilding the containing struct drops every *Ref inside it. That disables ClearResolvedReferences, which suppresses a resolved value only while the sibling *Ref is visible, so the spec patch deletes the declared *Ref and stores the resolved value in its place. The next apply of the manifest puts the *Ref back beside that value, a pair validateReferenceFields rejects, stopping reconciliation. Add an optional ReferenceEnsurer interface and invoke it on the object a resource manager hands back from Create and from Update, sourcing the references from the declared resource. It is kept separate from ReferenceManager and reached through a type assertion, so controllers generated before the method existed still satisfy AWSResourceManager and compile unchanged; they opt in by regenerating. The source is `desired`, not `reconcileDesired`: the latter is handed to Update, and a manager may mutate what it is given, so it is not a reliable record of what the user declared. The restoration is not hooked into patchResourceMetadataAndSpec because the late-initialization patch uses the AWS-observed object as its base, which carries no references. Pairs with aws-controllers-k8s/code-generator#738, which generates the method. Issue aws-controllers-k8s/community#2361 Issue aws-controllers-k8s/community#2431
|
/retest |
1 similar comment
|
/retest |
|
@gustavodiaz7722: The following tests failed, say
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. |
Summary
A cross-resource reference (
*Ref) is generated as a sibling of the concrete field it resolves into —spec.vpcConfig.subnetRefsnext tospec.vpcConfig.subnetIDs. A resource manager builds its return value from an AWS API response, which has no concept of a reference, so rebuilding the containing struct drops every*Refinside it.That disables
ClearResolvedReferences, which suppresses a resolved value only while it can still see the sibling:So the spec patch deletes the declared
*Refand stores the resolved value in its place — what aws-controllers-k8s/community#2431 reports: a declaredsecurityGroupRefsreplaced bysecurityGroupIDs.Reconciliation continues until the manifest is applied again, from Helm, Argo, Flux or
kubectl apply. That apply restores the*Refbeside the now-stored value, andvalidateReferenceFieldsrejects the pair:This PR generates an
EnsureReferencesmethod that restores the missing reference from the declared resource.Fixes aws-controllers-k8s/community#2431. Addresses the struct-nested half of aws-controllers-k8s/community#2361.
Requires the runtime change
aws-controllers-k8s/runtime#267 defines the optional
ReferenceEnsurerinterface and calls the method afterCreateand afterUpdate. Without it this method is inert but harmless. Controllers generated before it existed are unaffected and opt in by regenerating.What is emitted
spec.xRef)DeepCopy, and the*Refis a sibling of the concrete field, so nothing rebuilds itspec.a.xRef,spec.a.xRefs)spec.l[].xRef)A reference field that is itself a list (
*Refs, whose concrete sibling is a list of scalars) belongs in the struct row: the list is the leaf, not part of the path, so nothing has to be indexed to reach it.Only the reference is written, so nothing the service populated is touched.
This codifies an existing pattern
Restoring a nested
*Reffrom the declared resource is not new — three controllers already hand-maintain this assignment for want of a generated equivalent.eks/cluster:lambda/functiondoes the same forVPCConfig, andopensearchservice/domainforVPCOptions— the latter with a comment naming this issue directly:// To prevent https://github.com/aws-controllers-k8s/community/issues/2431The generated code is the same assignment with stricter guards: it nil-checks the container on both objects and only writes when the target is actually missing the reference, so it cannot clobber a reference the service did report. What changes is that every controller with struct-nested references gets the behaviour without hand-writing it.
Scope
Classifying each reference by the shape of the path to its
*Ref, across the controllers with a generatedreferences.go:Testing
Eight unit tests in
pkg/generate/code/resource_reference_test.go: top-level emits nothing, struct-nested single ref, struct-nested list-of-refs, list path emits nothing, a resource mixing struct- and list-nested, indent level, and the two rejection paths (a reference within a map, and a model missing an ancestor field).Regenerated
ec2-controllerandlambda-controller;references.godiffs are purely additive, output is gofmt-clean, and both build in full.lambda/functionrestoresCode.S3BucketRef,VPCConfig.SecurityGroupRefsandVPCConfig.SubnetRefs— the shape #2431 was filed for. Verified on a cluster that aFunctiondeclaring those refs keeps them through create and across repeated resyncs, with no resolved IDs written to the spec.Not addressed
References reached through a list. No fixed address to assign to, and no sound way to pair an observed element with a declared one: an AWS response need not preserve request order. These behave exactly as they do today.
The read path.
EnsureReferencesruns afterCreateand afterUpdate, not afterReadOne. TheAdoptionPolicy_Adoptbranch ofSyncanddeleteResourceboth patch the spec from aReadOne-derived object and so can still delete a nested*Ref; verified on a cluster for the adoption path. Thesdk_read_one_post_set_outputhooks inlambda/function,eks/clusterandopensearchservice/domainmust therefore stay — only their create-path halves are subsumed.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.