From 3e40e33df670424f2edfdcc4ff71e821146baefa Mon Sep 17 00:00:00 2001 From: Gustavo Diaz Date: Wed, 2 Sep 2026 19:00:18 +0000 Subject: [PATCH] feat: restore nested references after Create/Update 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. Both paths now hand the resource manager a copy and keep `desired` as the reference source. Update already did this with reconcileDesired; Create was passing `desired` itself, and generated sdkCreate only deep-copies the resource it is given partway through -- a custom_implementation returns before that point and a sdk_create_pre_build_request hook runs before it -- so either could mutate what the user declared. The copy is taken after setResourceManaged and EnsureTags so it carries the finalizer and controller tags. The restoration runs before the error from Create or Update is inspected. A resource manager may hand back a non-nil resource alongside a requeue error while an asynchronous operation is in flight, and many do; that object reaches the caller either way, so it should carry the declared references on every path. The restoration is not hooked into patchResourceMetadataAndSpec because the late-initialization patch uses the AWS-observed object as its base, which carries no references. Which shapes are covered is a property of the generated method rather than of this interface. Pairs with aws-controllers-k8s/code-generator#738, which generates the method. Issue aws-controllers-k8s/community#2431 Issue aws-controllers-k8s/community#2361 --- pkg/runtime/reconciler.go | 67 ++++++- pkg/runtime/reconciler_test.go | 319 +++++++++++++++++++++++++++++++++ pkg/types/reference_manager.go | 43 +++++ 3 files changed, 428 insertions(+), 1 deletion(-) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index bcdc0bdd..1f2712cc 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -856,9 +856,28 @@ func (r *resourceReconciler) createResource( } } + // Hand Create a copy, never `desired` itself, so that `desired` remains a + // record of what the user declared. Generated sdkCreate only deep-copies the + // resource it is given partway through: a `custom_implementation` returns + // before that point and a sdk_create_pre_build_request hook runs before it, so + // either can mutate the object it receives. This mirrors updateResource, which + // hands Update `reconcileDesired` for the same reason. + // + // Taken after the block above, so the copy carries the finalizer and the + // controller tags that setResourceManaged and EnsureTags just applied. + reconcileDesired := desired.DeepCopy() + rlog.Enter("rm.Create") - latest, err = rm.Create(ctx, desired) + latest, err = rm.Create(ctx, reconcileDesired) rlog.Exit("rm.Create", err) + + // Restore the references before inspecting the error. A resource manager is + // free to return a non-nil resource alongside a requeue error while an + // asynchronous create is in flight, and many do; that object is handed back to + // the caller either way, so it should carry the declared references on every + // path rather than only the success one. + latest = r.ensureReferences(rm, desired, latest) + if err != nil { // Here we're deciding to set a resource as unmanaged // if the error is an AWS API Error. This will ensure @@ -966,6 +985,43 @@ func setStatusWithoutConditions(dst, src acktypes.AWSResource) { dst.ReplaceConditions(conditions) } +// ensureReferences restores, onto an object a resource manager built from an AWS +// API response, the cross-resource reference (*Ref) fields it is missing, taking +// them from the declared resource. Only reference fields are written; every +// concrete value still comes from the service. +// +// A nested *Ref is dropped when the manager rebuilds its containing struct, which +// disables ClearResolvedReferences and lets the spec patch delete the declared +// *Ref and store 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. See acktypes.ReferenceEnsurer and +// aws-controllers-k8s/community#2361 and #2431. +// +// Called after Create and after Update rather than at the spec-patch chokepoint, +// because only on those paths is `desired` the user's declared, reference-resolved +// spec: the late-initialization patch uses the AWS-observed object as its base. +// +// Only references reached through structs are restored; see +// acktypes.ReferenceEnsurer for the list case and for the ReadOne-derived paths +// this does not cover. +// +// Returns `latest` unchanged when the resource manager does not implement the +// optional interface, which is every controller generated before it existed. +func (r *resourceReconciler) ensureReferences( + rm acktypes.AWSResourceManager, + desired acktypes.AWSResource, + latest acktypes.AWSResource, +) acktypes.AWSResource { + if ackcompare.IsNil(desired) || ackcompare.IsNil(latest) { + return latest + } + e, ok := rm.(acktypes.ReferenceEnsurer) + if !ok { + return latest + } + return e.EnsureReferences(desired, latest) +} + // updateResource calls one or more AWS APIs to modify the backend AWS resource // and patches the CR's Metadata and Spec back to the Kubernetes API. // @@ -1044,6 +1100,15 @@ func (r *resourceReconciler) updateResource( rlog.Enter("rm.Update") updated, err = rm.Update(ctx, reconcileDesired, latest, delta) rlog.Exit("rm.Update", err, "latest", latest) + + // Source the references from `desired`, not `reconcileDesired`: the latter + // was handed to Update, and a manager may mutate what it is given -- + // apigateway's ApiKey sdkUpdate assigns desired.ko.Spec.StageKeys straight + // from the response -- so it is no longer a record of what the user + // declared. Restored before the error is inspected, for the same reason as + // on the create path. + updated = r.ensureReferences(rm, desired, updated) + if err != nil { return updated, err } diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index d8103b39..810b1470 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -2786,3 +2786,322 @@ func TestReconcilerUpdate_PassesCopyOfDesiredToUpdate(t *testing.T) { // ACK.ReferencesResolved is not lost. desiredCopy.AssertCalled(t, "ReplaceConditions", inFlight) } + +// referenceEnsuringRM decorates a mocked resource manager with the optional +// acktypes.ReferenceEnsurer interface, which generated references.go implements +// for a resource carrying a nested cross-resource reference. The generated mocks +// are built from AWSResourceManager and so do not carry the method, which is what +// keeps controllers generated before it existed working unchanged. +type referenceEnsuringRM struct { + *ackmocks.AWSResourceManager + + calls []ensureCall + returns acktypes.AWSResource +} + +type ensureCall struct { + from acktypes.AWSResource + to acktypes.AWSResource +} + +func (rm *referenceEnsuringRM) EnsureReferences( + from acktypes.AWSResource, + to acktypes.AWSResource, +) acktypes.AWSResource { + rm.calls = append(rm.calls, ensureCall{from: from, to: to}) + if rm.returns != nil { + return rm.returns + } + return to +} + +// TestReconcilerUpdate_EnsuresReferencesAfterUpdate verifies the restoration runs +// immediately after rm.Update returns, is handed the DECLARED resource as its +// source, and that the object it returns is what goes on to be patched. +// +// The source matters: `reconcileDesired` is what Update is given, and a manager +// may mutate what it is handed -- apigateway's ApiKey sdkUpdate assigns +// desired.ko.Spec.StageKeys straight from the response -- so only `desired` is a +// reliable record of what the user declared. +func TestReconcilerUpdate_EnsuresReferencesAfterUpdate(t *testing.T) { + require := require.New(t) + ctx := context.TODO() + + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + + desired, _, _ := resourceMocks() + latest, _, _ := resourceMocks() + // updateResource deep-copies desired and transplants the observed status onto + // the copy; resourceMocks' DeepCopy returns the receiver, so wire these here. + desired.On("Conditions").Return([]*ackv1alpha1.Condition{}) + desired.On("ReplaceConditions", mock.Anything).Return() + // What rm.Update hands back: rebuilt from the response, nested refs dropped. + updatedByAWS, _, _ := resourceMocks() + // What the generated method returns: the above, with references restored. + restored, _, _ := resourceMocks() + + inner := &ackmocks.AWSResourceManager{} + inner.On("Update", ctx, mock.Anything, latest, delta).Return(updatedByAWS, nil) + inner.On("ClearResolvedReferences", mock.Anything).Return( + func(r acktypes.AWSResource) acktypes.AWSResource { return r }, + ) + inner.On("FilterSystemTags", mock.Anything, mock.Anything) + rm := &referenceEnsuringRM{AWSResourceManager: inner, returns: restored} + + rmf, rd := managedResourceManagerFactoryMocks(desired, latest) + rd.On("IsManaged", mock.Anything).Return(true) + rd.On("Delta", mock.Anything, mock.Anything).Return(delta) + + r, kc, _ := reconcilerMocks(rmf) + kc.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rr, ok := r.(*resourceReconciler) + require.True(ok) + + out, err := rr.updateResource(ctx, rm, desired, latest) + require.NoError(err) + + require.Len(rm.calls, 1, "restoration must run once, right after Update") + require.Same(acktypes.AWSResource(desired), rm.calls[0].from, + "source must be the declared resource, not the copy handed to Update") + require.Same(acktypes.AWSResource(updatedByAWS), rm.calls[0].to, + "target must be the object Update returned") + + // The restored object is what gets cleaned and patched; otherwise the + // restoration would be computed and thrown away. + inner.AssertCalled(t, "ClearResolvedReferences", acktypes.AWSResource(restored)) + inner.AssertNotCalled(t, "ClearResolvedReferences", acktypes.AWSResource(updatedByAWS)) + require.Same(acktypes.AWSResource(restored), out) +} + +// TestReconcilerUpdate_LateInitializeIsNotAffectedByEnsureReferences pins why the +// restoration lives on the Create/Update paths rather than in +// patchResourceMetadataAndSpec. +// +// lateInitializeResource patches with the AWS-observed `latest` as its BASE, not +// the declared resource. Hooked into the shared patch path, the restoration would +// have been handed a source carrying no references at all, so it must never be +// invoked there. +func TestReconcilerUpdate_LateInitializeIsNotAffectedByEnsureReferences(t *testing.T) { + require := require.New(t) + ctx := context.TODO() + + desired, _, _ := resourceMocks() + latest, _, _ := resourceMocks() + lateInited, _, _ := resourceMocks() + + inner := &ackmocks.AWSResourceManager{} + inner.On("LateInitialize", ctx, latest).Return(lateInited, nil) + inner.On("ClearResolvedReferences", mock.Anything).Return( + func(r acktypes.AWSResource) acktypes.AWSResource { return r }, + ) + inner.On("FilterSystemTags", mock.Anything, mock.Anything) + rm := &referenceEnsuringRM{AWSResourceManager: inner} + + rmf, rd := managedResourceManagerFactoryMocks(desired, latest) + rd.On("IsManaged", mock.Anything).Return(true) + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + rd.On("Delta", mock.Anything, mock.Anything).Return(delta) + + r, kc, _ := reconcilerMocks(rmf) + kc.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rr, ok := r.(*resourceReconciler) + require.True(ok) + + out, err := rr.lateInitializeResource(ctx, rm, desired, latest) + require.NoError(err) + + require.Empty(rm.calls, + "reference restoration must not run on the late-init patch") + require.Same(acktypes.AWSResource(lateInited), out, + "the late-initialized object must survive to be patched") +} + +// TestReconcilerCreate_EnsuresReferencesAfterCreate verifies the same restoration +// on the create path, where the corruption is otherwise introduced on the very +// first reconcile. +func TestReconcilerCreate_EnsuresReferencesAfterCreate(t *testing.T) { + require := require.New(t) + ctx := context.TODO() + + desired, _, _ := resourceMocks() + createdByAWS, _, _ := resourceMocks() + observed, _, _ := resourceMocks() + restored, _, _ := resourceMocks() + + inner := &ackmocks.AWSResourceManager{} + inner.On("Create", ctx, desired).Return(createdByAWS, nil) + inner.On("ReadOne", ctx, mock.Anything).Return(observed, nil) + inner.On("ClearResolvedReferences", mock.Anything).Return( + func(r acktypes.AWSResource) acktypes.AWSResource { return r }, + ) + inner.On("FilterSystemTags", mock.Anything, mock.Anything) + rm := &referenceEnsuringRM{AWSResourceManager: inner, returns: restored} + + rmf, rd := managedResourceManagerFactoryMocks(desired, createdByAWS) + // Already managed, so createResource goes straight to rm.Create. + rd.On("IsManaged", mock.Anything).Return(true) + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + rd.On("Delta", mock.Anything, mock.Anything).Return(delta) + + r, kc, _ := reconcilerMocks(rmf) + kc.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rr, ok := r.(*resourceReconciler) + require.True(ok) + + _, err := rr.createResource(ctx, rm, desired) + require.NoError(err) + + require.Len(rm.calls, 1, "restoration must run once, right after Create") + // resourceMocks wires DeepCopy to return the receiver, so this cannot tell a + // snapshot from the original; TestReconcilerCreate_EnsuresReferencesFromSnapshot + // pins that separately. + require.Same(acktypes.AWSResource(desired), rm.calls[0].from) + require.Same(acktypes.AWSResource(createdByAWS), rm.calls[0].to) +} + +// TestReconcilerUpdate_WithoutEnsurerIsUnaffected verifies the type assertion +// degrades cleanly: a manager that does not implement the optional interface -- +// every controller generated before it existed -- takes the original path, with +// the object Update returned flowing through untouched. +func TestReconcilerUpdate_WithoutEnsurerIsUnaffected(t *testing.T) { + require := require.New(t) + ctx := context.TODO() + + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + + desired, _, _ := resourceMocks() + latest, _, _ := resourceMocks() + // updateResource deep-copies desired and transplants the observed status onto + // the copy; resourceMocks' DeepCopy returns the receiver, so wire these here. + desired.On("Conditions").Return([]*ackv1alpha1.Condition{}) + desired.On("ReplaceConditions", mock.Anything).Return() + updatedByAWS, _, _ := resourceMocks() + + rm := &ackmocks.AWSResourceManager{} + rm.On("Update", ctx, mock.Anything, latest, delta).Return(updatedByAWS, nil) + rm.On("ClearResolvedReferences", mock.Anything).Return( + func(r acktypes.AWSResource) acktypes.AWSResource { return r }, + ) + rm.On("FilterSystemTags", mock.Anything, mock.Anything) + + rmf, rd := managedResourceManagerFactoryMocks(desired, latest) + rd.On("IsManaged", mock.Anything).Return(true) + rd.On("Delta", mock.Anything, mock.Anything).Return(delta) + + r, kc, _ := reconcilerMocks(rmf) + kc.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rr, ok := r.(*resourceReconciler) + require.True(ok) + + out, err := rr.updateResource(ctx, rm, desired, latest) + require.NoError(err) + + rm.AssertCalled(t, "ClearResolvedReferences", acktypes.AWSResource(updatedByAWS)) + require.Same(acktypes.AWSResource(updatedByAWS), out) +} + +// TestReconcilerCreate_PassesCopyToCreate pins that createResource hands Create a +// copy and keeps `desired` as the reference source, mirroring updateResource. +// +// Generated sdkCreate only deep-copies the resource it is given partway through: a +// `custom_implementation` returns before that point and a +// sdk_create_pre_build_request hook runs before it, so either can mutate the object +// it receives. Two controllers use the first route and ten the second. None of +// those resources has a struct-nested reference today, so none gets a generated +// EnsureReferences, but the restoration must not depend on that continuing to hold. +func TestReconcilerCreate_PassesCopyToCreate(t *testing.T) { + require := require.New(t) + ctx := context.TODO() + + // A distinct copy, so "which object went where?" is answerable. + desiredCopy, _, _ := resourceMocks() + desired := resourceMockReturningCopy(desiredCopy) + createdByAWS, _, _ := resourceMocks() + observed, _, _ := resourceMocks() + + inner := &ackmocks.AWSResourceManager{} + inner.On("Create", ctx, desiredCopy).Return(createdByAWS, nil) + inner.On("ReadOne", ctx, mock.Anything).Return(observed, nil) + inner.On("ClearResolvedReferences", mock.Anything).Return( + func(r acktypes.AWSResource) acktypes.AWSResource { return r }, + ) + inner.On("FilterSystemTags", mock.Anything, mock.Anything) + rm := &referenceEnsuringRM{AWSResourceManager: inner} + + rmf, rd := managedResourceManagerFactoryMocks(desired, createdByAWS) + rd.On("IsManaged", mock.Anything).Return(true) + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + rd.On("Delta", mock.Anything, mock.Anything).Return(delta) + + r, kc, _ := reconcilerMocks(rmf) + kc.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rr, ok := r.(*resourceReconciler) + require.True(ok) + + _, err := rr.createResource(ctx, rm, desired) + require.NoError(err) + + // Create gets the copy, never the stored object. + inner.AssertCalled(t, "Create", ctx, acktypes.AWSResource(desiredCopy)) + inner.AssertNotCalled(t, "Create", ctx, acktypes.AWSResource(desired)) + + // The reference source is the untouched `desired`. + require.Len(rm.calls, 1) + require.Same(acktypes.AWSResource(desired), rm.calls[0].from, + "references must be sourced from `desired`, which Create never saw") +} + +// TestReconcilerCreate_EnsuresReferencesOnCreateError pins that the restoration +// runs even when Create returns an error. +// +// A resource manager may hand back a non-nil resource alongside a requeue error +// while an asynchronous create is in flight, and many do. That object is returned +// to the caller either way, so it has to carry the declared references on the +// error path too, not only on the success path. +func TestReconcilerCreate_EnsuresReferencesOnCreateError(t *testing.T) { + require := require.New(t) + ctx := context.TODO() + + desired, _, _ := resourceMocks() + partial, _, _ := resourceMocks() + restored, _, _ := resourceMocks() + + createErr := requeue.NeededAfter(errors.New("still creating"), time.Second) + + inner := &ackmocks.AWSResourceManager{} + inner.On("Create", ctx, mock.Anything).Return(partial, createErr) + inner.On("ClearResolvedReferences", mock.Anything).Return( + func(r acktypes.AWSResource) acktypes.AWSResource { return r }, + ) + inner.On("FilterSystemTags", mock.Anything, mock.Anything) + rm := &referenceEnsuringRM{AWSResourceManager: inner, returns: restored} + + rmf, rd := managedResourceManagerFactoryMocks(desired, partial) + rd.On("IsManaged", mock.Anything).Return(true) + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + rd.On("Delta", mock.Anything, mock.Anything).Return(delta) + + r, kc, _ := reconcilerMocks(rmf) + kc.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rr, ok := r.(*resourceReconciler) + require.True(ok) + + out, err := rr.createResource(ctx, rm, desired) + require.Error(err) + + require.Len(rm.calls, 1, + "restoration must run even though Create returned an error") + require.Same(acktypes.AWSResource(desired), rm.calls[0].from) + require.Same(acktypes.AWSResource(partial), rm.calls[0].to) + require.Same(acktypes.AWSResource(restored), out, + "the restored object must be what is handed back on the error path") + + // ReadOne is never reached, so the error short-circuit still holds. + inner.AssertNotCalled(t, "ReadOne", mock.Anything, mock.Anything) +} diff --git a/pkg/types/reference_manager.go b/pkg/types/reference_manager.go index b89044da..82c1abf1 100644 --- a/pkg/types/reference_manager.go +++ b/pkg/types/reference_manager.go @@ -36,3 +36,46 @@ type ReferenceManager interface { // values. ClearResolvedReferences(AWSResource) AWSResource } + +// ReferenceEnsurer restores cross-resource reference (`*Ref`) fields onto an +// object a resource manager built from an AWS API response. +// +// A `*Ref` is generated as a sibling of the concrete field it resolves into -- +// `spec.vpcConfig.subnetRefs` next to `spec.vpcConfig.subnetIDs`. An API response +// has no concept of a reference, so rebuilding the containing struct drops every +// `*Ref` inside it. A top-level `*Ref` survives, because generated set-output code +// deep-copies the incoming object and overwrites only the concrete field. +// +// Losing it disables ClearResolvedReferences, which suppresses a resolved value +// only while the sibling `*Ref` is visible. The spec patch then deletes the +// declared `*Ref` and stores the resolved value in its place. Reconciliation +// carries on until the manifest is applied again, at which point the restored +// `*Ref` sits beside the stored value and validateReferenceFields rejects the pair +// ("both resource reference wrapper and ID cannot be used together"), stopping +// reconciliation. See aws-controllers-k8s/community#2361 and #2431. +// +// Kept separate from ReferenceManager and reached through a type assertion, so +// controllers generated before the method existed still satisfy +// AWSResourceManager. They opt in by regenerating. +// +// What actually gets restored is a property of the generated method, not of this +// interface. Today the generator emits an assignment only for a reference reached +// through structs: one reached through a list has no fixed address, and pairing an +// element the service reported with an element the user declared has no sound key, +// so those references behave as they did before. Given a declared notion of element +// identity the generated code could cover them too, without this interface +// changing. +// +// The reconciler calls this after Create and after Update, the two points where a +// manager returns an object whose spec is about to be patched back. The +// ReadOne-derived spec writes -- the adoption branch of Sync, and deleteResource +// -- need the same treatment and do not have it yet. +type ReferenceEnsurer interface { + // EnsureReferences returns a copy of `latest` with any reference field it is + // missing restored from `desired`. Only reference fields are written. + // + // `desired` must be the DECLARED resource with its references resolved, and must + // not be an object that has been through a resource manager: managers may mutate + // what they are handed, and some write API response values into it. + EnsureReferences(desired AWSResource, latest AWSResource) AWSResource +}