Skip to content

feat: restore nested cross-resource references after Create and Update - #267

Open
gustavodiaz7722 wants to merge 1 commit into
aws-controllers-k8s:mainfrom
gustavodiaz7722:feat/ensure-references
Open

feat: restore nested cross-resource references after Create and Update#267
gustavodiaz7722 wants to merge 1 commit into
aws-controllers-k8s:mainfrom
gustavodiaz7722:feat/ensure-references

Conversation

@gustavodiaz7722

@gustavodiaz7722 gustavodiaz7722 commented Aug 27, 2026

Copy link
Copy Markdown
Member

Summary

A cross-resource reference (*Ref) is generated as a sibling of the concrete field it resolves into — spec.vpcConfig.subnetRefs next to spec.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 *Ref inside it.

That disables ClearResolvedReferences, which suppresses a resolved value only while it can still see the sibling:

if ko.Spec.VPCConfig != nil {
	if len(ko.Spec.VPCConfig.SubnetRefs) > 0 {   // false once the *Ref is gone
		ko.Spec.VPCConfig.SubnetIDs = nil
	}
}

So the spec patch deletes the declared *Ref and stores the resolved value in its place — what aws-controllers-k8s/community#2431 reports: a declared securityGroupRefs replaced by securityGroupIDs.

Reconciliation continues until the manifest is applied again, from Helm, Argo, Flux or kubectl apply. That apply restores the *Ref beside the now-stored value, and validateReferenceFields rejects the pair:

message: Reference resolution failed
reason: 'both resource reference wrapper and ID cannot be used together:
         VPCConfig.SubnetIDs,VPCConfig.SubnetRefs'

This PR adds an optional ReferenceEnsurer interface and invokes it on the object a resource manager hands back from Create and from Update, sourcing the references from the declared resource.

Fixes aws-controllers-k8s/community#2431. Pairs with aws-controllers-k8s/code-generator#738, which generates the method.

Backwards compatible

ReferenceEnsurer is deliberately separate from ReferenceManager and reached through a type assertion, so every controller generated before the method existed still satisfies AWSResourceManager and compiles unchanged. Those controllers take the existing path untouched and opt in by regenerating. TestReconcilerUpdate_WithoutEnsurerIsUnaffected pins that.

Why these two call sites

The restoration runs where a manager returns an object whose spec is about to be patched back, not at patchResourceMetadataAndSpec, because the source has to be the declared, reference-resolved resource — and that is only in scope on these paths.

lateInitializeResource patches with the AWS-observed latest as its base, so hooked into the shared patch path the restoration would have been handed a source carrying no references at all. TestReconcilerUpdate_LateInitializeIsNotAffectedByEnsureReferences keeps that boundary asserted.

Why desired and not reconcileDesired

reconcileDesired is what gets handed to rm.Update, and a resource manager may mutate the object it is given. apigateway's ApiKey sdkUpdate assigns desired.ko.Spec.StageKeys straight from the UpdateApiKey response (sdk.go:331), so by the time Update returns it is no longer a record of what the user declared. applyIgnoredFields likewise merges observed values into it for a resource carrying the ignore-field-drift annotation. Only desired is clean, and TestReconcilerUpdate_EnsuresReferencesAfterUpdate asserts it is what gets passed.

What the generated method does

Detailed in aws-controllers-k8s/code-generator#738. Summarised here because it bounds this PR's blast radius:

  • top-level *Ref — nothing emitted; it cannot be lost, since every write path starts from a DeepCopy of the object it was handed.
  • reached through structs — only the reference field is assigned, so every concrete value the service reported stands. This is the whole of what is emitted, and it codifies a pattern eks/cluster, lambda/function and opensearchservice/domain already hand-maintain in sdk_*_post_set_output hooks.
  • reached through a list — nothing emitted; no fixed address to assign to, and no sound way to pair an observed element with a declared one. These behave exactly as they do today.

So 117 struct-nested references across 37 resources in 25 controllers change behaviour; the 315 top-level and 38 list-nested ones are untouched.

Testing

Four tests in pkg/runtime/reconciler_test.go:

Test Asserts
TestReconcilerUpdate_EnsuresReferencesAfterUpdate runs once after Update, sourced from desired, and the returned object is what gets patched
TestReconcilerCreate_EnsuresReferencesAfterCreate same on the create path
TestReconcilerUpdate_LateInitializeIsNotAffectedByEnsureReferences never invoked on the late-init patch
TestReconcilerUpdate_WithoutEnsurerIsUnaffected a manager not implementing the interface is untouched

Full runtime suite passes. Verified on a cluster against real generated code (lambda and ec2 regenerated against code-generator#738): a Function declaring vpcConfig.securityGroupRefs/subnetRefs keeps both in its stored spec through create and across repeated resyncs, with no resolved IDs written and no Reference resolution failed condition.

Not addressed

The read path. This runs after Create and after Update, not after ReadOne. Two spec writes consume a ReadOne-derived object and so can still delete a nested *Ref: the AdoptionPolicy_Adopt branch of Sync, and deleteResource. Both use the stored CR as the patch base and the ReadOne result as the target. Confirmed on a cluster for the adoption path, which is why the existing sdk_read_one_post_set_output hooks in the three controllers above must stay. Covering it is a separate change: the unresolved desired on those paths is a valid source, since a *Ref is user-declared and resolution only fills the concrete sibling.

The delta. It is computed against the raw ReadOne result, which this does not touch. A dropped struct-nested *Ref was invisible to the delta — the generated delta compares *Ref fields only at the top level — so it never drove a redundant Update and there is nothing left to fix for the shapes this PR covers.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@gustavodiaz7722

Copy link
Copy Markdown
Member Author

/retest

Comment thread pkg/types/reference_manager.go Outdated
Comment thread pkg/types/reference_manager.go Outdated
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
@knottnt

knottnt commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

/retest

ack-prow Bot pushed a commit that referenced this pull request Sep 8, 2026
Description of changes:

`unit-test` is currently failing on every PR, including unmodified `main`. It
fails in the `mocks` target, which `make test` depends on, so no test runs:

```
building mocks for pkg/types ...
internal error: package "k8s.io/apimachinery/pkg/apis/meta/v1" without types was imported from "github.com/aws-controllers-k8s/runtime/pkg/types"
make: *** [Makefile:24: mocks] Error 1
```

`scripts/install-mockery.sh` builds mockery from source with whatever Go the CI
image provides, and aws-controllers-k8s/test-infra#1084 moved `go_version` from
1.26.5 to 1.27.1 on 2026-09-02. mockery v2.53.3 pins
`golang.org/x/tools v0.30.0`, whose `go/packages` predates Go 1.27 and cannot
type-check its standard library, which is what produces the `without types`
loader error.

The timeline matches: the last `unit-test` run before the image bump passed
(PR #267, 2026-09-02T19:04Z), and runs after it fail.

v2.53.7 pins `golang.org/x/tools v0.49.0`, which handles Go 1.27.

The mocks are regenerated so the committed output matches the new version. The
only content changes are the generated-by header and import grouping — no mock
behaviour changes.

This also drops `mocks/pkg/types/resolved_reference_manager.go`. Its
`ResolvedReferenceManager` interface no longer exists in `pkg/types`, so mockery
does not generate it and nothing references it. It survived because the `mocks`
target overwrites files rather than starting from a clean directory, so a
`make clean-mocks && make mocks` cycle would otherwise always leave the tree
dirty.

### Testing

Reproduced and verified locally against both Go versions, building mockery from
source exactly as the job does:

| Go | mockery | `make mocks` |
| --- | --- | --- |
| 1.26.0 | v2.53.3 | passes |
| 1.27.1 | v2.53.3 | fails with the error above |
| 1.27.1 | v2.53.7 | passes |

Under Go 1.27.1 with this change, all six mock sets generate and the full
`go test ./...` passes. I also confirmed v2.53.3 fails on unmodified `main` under
Go 1.27.1, so this is not specific to any open PR.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@ack-prow

ack-prow Bot commented Sep 11, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gustavodiaz7722, knottnt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow Bot added the approved label Sep 11, 2026
@knottnt

knottnt commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

/retest

1 similar comment
@gustavodiaz7722

Copy link
Copy Markdown
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The ACK Lambda Controller modifies the object spec

2 participants