From 4d1336b1eb0113699f231618a472fd40a0f49f77 Mon Sep 17 00:00:00 2001 From: Gustavo Diaz Date: Wed, 2 Sep 2026 17:47:07 +0000 Subject: [PATCH] feat: generate EnsureReferences for nested refs 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 behaves as it does today: neither position nor resolved value is a sound key for pairing an observed element with a declared one. This codifies a pattern eks/cluster, lambda/function and opensearchservice/domain already hand-maintain in set-output hooks, with stricter guards. 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#2431 Issue aws-controllers-k8s/community#2361 --- pkg/generate/ack/controller.go | 3 + pkg/generate/code/resource_reference.go | 163 ++++++++++++++ pkg/generate/code/resource_reference_test.go | 211 ++++++++++++++++++ .../generator-with-reference-in-map.yaml | 22 ++ templates/pkg/resource/references.go.tpl | 37 +++ 5 files changed, 436 insertions(+) create mode 100644 pkg/testdata/models/apis/apigatewayv2/0000-00-00/generator-with-reference-in-map.yaml diff --git a/pkg/generate/ack/controller.go b/pkg/generate/ack/controller.go index 7f2de46c7..50c336e55 100644 --- a/pkg/generate/ack/controller.go +++ b/pkg/generate/ack/controller.go @@ -227,6 +227,9 @@ var ( "GoCodeClearResolvedReferences": func(f *ackmodel.Field, targetVarName string, indentLevel int) (string, error) { return code.ClearResolvedReferencesForField(f, targetVarName, indentLevel) }, + "GoCodeEnsureReferences": func(r *ackmodel.CRD, sourceVarName string, targetVarName string, indentLevel int) (string, error) { + return code.EnsureReferences(r, sourceVarName, targetVarName, indentLevel) + }, "GoCodeConvertToACKTags": func(r *ackmodel.CRD, sourceVarName string, targetVarName string, keyOrderVarName string, indentLevel int) (string, error) { return code.GoCodeConvertToACKTags(r, sourceVarName, targetVarName, keyOrderVarName, indentLevel) }, diff --git a/pkg/generate/code/resource_reference.go b/pkg/generate/code/resource_reference.go index 93024238c..e06492449 100644 --- a/pkg/generate/code/resource_reference.go +++ b/pkg/generate/code/resource_reference.go @@ -503,3 +503,166 @@ func getReferencedStateForField(field *model.Field, indentLevel int) string { return out } + +// hasCollectionAncestor reports whether a collection lies on the path to a field's +// reference (`*Ref`) sibling. +// +// It returns true for a list. A map ancestor is rejected with an error instead: a +// reference cannot be addressed through a map at all, so there is nothing sensible +// to generate. +// +// Only ancestors count. A reference field that is itself a list (`*Refs`, whose +// concrete sibling is a list of scalars) is the leaf rather than part of the path, +// so nothing has to be indexed to reach it. +// +// EnsureReferences uses this to skip such a reference; see its doc comment. +func hasCollectionAncestor(field *model.Field) (bool, error) { + r := field.CRD + refFieldPath, err := field.ReferenceFieldPath() + if err != nil { + return false, err + } + fp := fieldpath.FromString(refFieldPath) + for depth := 0; depth < fp.Size()-1; depth++ { + curFP := fp.CopyAt(depth).String() + cur, ok := r.Fields[curFP] + if !ok { + return false, fmt.Errorf( + "resource %q: unable to find field with path %q", r.Kind, curFP, + ) + } + if cur.ShapeRef.Shape.Type == "map" { + return false, fmt.Errorf( + "resource %q, field %q: references cannot be within a map", + r.Kind, field.Path, + ) + } + if cur.ShapeRef.Shape.Type == "list" { + return true, nil + } + } + return false, nil +} + +// EnsureReferences returns Go code that restores, from a source object into a +// target object, the cross-resource reference (`*Ref`) fields the target is +// missing. +// +// A `*Ref` is 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. See +// aws-controllers-k8s/community#2361 and #2431. +// +// Only a reference reached through STRUCTS is emitted. It has one fixed address, +// so exactly that field is assigned and every value the service reported stands. +// +// A TOP-LEVEL reference is skipped: generated set-output code starts from a +// DeepCopy of the object it was handed and overwrites only the concrete field, and +// the `*Ref` is a sibling of that field rather than part of it, so nothing rebuilds +// it. That holds for the generated paths; a hand-written set-output hook that +// rebuilds the object wholesale could still drop it, in which case the hook has to +// carry the reference across itself. +// +// A reference reached through a LIST is also skipped, and behaves as it does today. +// It has no fixed address, so restoring it means pairing an element the service +// reported with an element the user declared, and neither available key is sound: +// +// - Position is not reliable, because an AWS response need not preserve the order +// of the request. +// - The resolved value is not reliable either, because it is not always unique. +// A reference resolves to whatever path `references.path` names, and while most +// name an AWS-assigned identifier, 75 of the roughly 470 references configured +// across the controllers resolve to a `Spec.*` path that carries no uniqueness +// guarantee. `sqs/Queue.Policy` and `sns/Topic.Policy`, for instance, resolve +// `iam/Policy` via `Spec.PolicyDocument` -- the policy document itself -- so two +// separate IAM policies granting the same thing resolve to the same value. +// +// Replacing the whole outermost list avoids having to pair anything, but discards +// whatever the service populated inside it, which for an element carrying +// AWS-assigned members (ec2's `NetworkACL.Associations`) means losing them from the +// stored spec. +// +// A sound per-element restore needs a declared notion of element identity -- a set +// of fields named in `generator.yaml` that uniquely identify an entry -- which is +// left to a follow-up. +// +// Sample output: +// +// if desiredKO.Spec.JWTConfiguration != nil && latestKO.Spec.JWTConfiguration != nil && desiredKO.Spec.JWTConfiguration.IssuerRef != nil && latestKO.Spec.JWTConfiguration.IssuerRef == nil { +// latestKO.Spec.JWTConfiguration.IssuerRef = desiredKO.Spec.JWTConfiguration.IssuerRef +// } +func EnsureReferences( + r *model.CRD, + sourceVarName string, + targetVarName string, + indentLevel int, +) (string, error) { + out := "" + indent := strings.Repeat("\t", indentLevel) + specField := r.Config().PrefixConfig.SpecField + + for _, fieldName := range r.SortedFieldNames() { + field := r.Fields[fieldName] + if !field.HasReference() { + continue + } + refName, err := field.GetReferenceFieldName() + if err != nil { + return "", err + } + refFieldPath, err := field.ReferenceFieldPath() + if err != nil { + return "", err + } + fp := fieldpath.FromString(refFieldPath) + + // A top-level reference has no parent to be rebuilt. + if fp.Size() < 2 { + continue + } + + // A reference behind a list has no fixed address to assign to; see the doc + // comment. + inList, err := hasCollectionAncestor(field) + if err != nil { + return "", err + } + if inList { + continue + } + + // Struct-only path: guard every ancestor on both objects, then assign + // just the reference when the target lacks it. + srcAccess := sourceVarName + specField + tgtAccess := targetVarName + specField + conds := make([]string, 0, fp.Size()*2) + for depth := 0; depth < fp.Size()-1; depth++ { + srcAccess = fmt.Sprintf("%s.%s", srcAccess, fp.At(depth)) + tgtAccess = fmt.Sprintf("%s.%s", tgtAccess, fp.At(depth)) + conds = append(conds, fmt.Sprintf("%s != nil", srcAccess)) + conds = append(conds, fmt.Sprintf("%s != nil", tgtAccess)) + } + srcRef := fmt.Sprintf("%s.%s", srcAccess, refName.Camel) + tgtRef := fmt.Sprintf("%s.%s", tgtAccess, refName.Camel) + if field.ShapeRef.Shape.Type == "list" { + // A list-of-references field is one value at a fixed address, so it + // is copied whole; length stands in for nil, as it does in + // ClearResolvedReferences for the same shape. + conds = append(conds, fmt.Sprintf("len(%s) > 0", srcRef)) + conds = append(conds, fmt.Sprintf("len(%s) == 0", tgtRef)) + } else { + conds = append(conds, fmt.Sprintf("%s != nil", srcRef)) + conds = append(conds, fmt.Sprintf("%s == nil", tgtRef)) + } + out += fmt.Sprintf("%sif %s {\n", indent, strings.Join(conds, " && ")) + out += fmt.Sprintf("%s\t%s = %s\n", indent, tgtRef, srcRef) + out += fmt.Sprintf("%s}\n", indent) + } + + return out, nil +} diff --git a/pkg/generate/code/resource_reference_test.go b/pkg/generate/code/resource_reference_test.go index 34e6d0d71..7e32c4b8f 100644 --- a/pkg/generate/code/resource_reference_test.go +++ b/pkg/generate/code/resource_reference_test.go @@ -624,3 +624,214 @@ func Test_ClearResolvedReferencesForField_SingleReference_WithinMultipleSlices(t require.NoError(err) assert.Equal(expected, got) } + +func Test_EnsureReferences_TopLevelReference_EmitsNothing(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-reference.yaml", + }) + + // Integration's only reference is the top-level APIID/APIRef pair; VpcLink's + // are top-level lists of references. A top-level *Ref has no parent that could + // be rebuilt, so it always survives and nothing needs emitting. + for _, kind := range []string{"Integration", "VpcLink"} { + crd := testutil.GetCRDByName(t, g, kind) + require.NotNil(crd) + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.NoError(err) + assert.Equal("", got, "resource %s", kind) + } +} + +func Test_EnsureReferences_StructPath_AssignsOnlyTheReference(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-nested-reference.yaml", + }) + + // Reached through a struct, so the reference has one fixed address: guard the + // ancestors on both objects and assign just that field. + crd := testutil.GetCRDByName(t, g, "Authorizer") + require.NotNil(crd) + expected := + ` if desiredKO.Spec.JWTConfiguration != nil && latestKO.Spec.JWTConfiguration != nil && desiredKO.Spec.JWTConfiguration.IssuerRef != nil && latestKO.Spec.JWTConfiguration.IssuerRef == nil { + latestKO.Spec.JWTConfiguration.IssuerRef = desiredKO.Spec.JWTConfiguration.IssuerRef + } +` + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.NoError(err) + assert.Equal(expected, got) +} + +func Test_EnsureReferences_StructPath_ListOfReferences(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "eks", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-nested-reference.yaml", + }) + + // The reference field is itself a list (*Refs) but sits in a struct at a fixed + // address, so the list is the leaf rather than part of the path. It is copied + // whole, guarded on length, as ClearResolvedReferences treats the same shape. + // This is the shape community#2431 was filed for. + crd := testutil.GetCRDByName(t, g, "Cluster") + require.NotNil(crd) + expected := + ` if desiredKO.Spec.ResourcesVPCConfig != nil && latestKO.Spec.ResourcesVPCConfig != nil && len(desiredKO.Spec.ResourcesVPCConfig.SecurityGroupRefs) > 0 && len(latestKO.Spec.ResourcesVPCConfig.SecurityGroupRefs) == 0 { + latestKO.Spec.ResourcesVPCConfig.SecurityGroupRefs = desiredKO.Spec.ResourcesVPCConfig.SecurityGroupRefs + } +` + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.NoError(err) + assert.Equal(expected, got) + // The concrete sibling is never read or written. + assert.NotContains(got, "SecurityGroupIDs") +} + +func Test_EnsureReferences_ListPath_IsSkipped(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "ec2", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-nested-references.yaml", + }) + + // RouteTable's references are two inside spec.Routes plus a top-level VPCID. + // The top-level one needs no help and the list-nested ones are skipped, so + // nothing is emitted and the template's `if $ensureReferences` guard leaves + // RouteTable without the method entirely. + crd := testutil.GetCRDByName(t, g, "RouteTable") + require.NotNil(crd) + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.NoError(err) + assert.Equal("", got) +} + +func Test_EnsureReferences_MixedShapes_EmitsOnlyTheStructPath(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "s3", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-nested-references.yaml", + }) + + // Bucket carries one of each shape, pinning that they are treated differently + // within a single resource: Logging.LoggingEnabled.TargetBucket is reached + // through structs alone, while + // Notification.LambdaFunctionConfigurations[].Filter.Key.FilterRules[].Value + // sits two lists deep. + crd := testutil.GetCRDByName(t, g, "Bucket") + require.NotNil(crd) + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.NoError(err) + + // The struct path is restored, writing nothing but the reference itself. + expected := + ` if desiredKO.Spec.Logging != nil && latestKO.Spec.Logging != nil && desiredKO.Spec.Logging.LoggingEnabled != nil && latestKO.Spec.Logging.LoggingEnabled != nil && desiredKO.Spec.Logging.LoggingEnabled.TargetBucketRef != nil && latestKO.Spec.Logging.LoggingEnabled.TargetBucketRef == nil { + latestKO.Spec.Logging.LoggingEnabled.TargetBucketRef = desiredKO.Spec.Logging.LoggingEnabled.TargetBucketRef + } +` + assert.Equal(expected, got) + + // The list-nested reference contributes nothing, and in particular the + // containing list is not assigned. + assert.NotContains(got, "LambdaFunctionConfigurations") + assert.NotContains(got, "FilterRules") + + // Nothing is iterated or indexed on the way there. + assert.NotContains(got, "range") + assert.NotContains(got, "[f0idx]") + assert.NotContains(got, "[f1idx]") +} + +func Test_EnsureReferences_RespectsIndentLevel(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-nested-reference.yaml", + }) + + crd := testutil.GetCRDByName(t, g, "Authorizer") + require.NotNil(crd) + expected := + ` if desiredKO.Spec.JWTConfiguration != nil && latestKO.Spec.JWTConfiguration != nil && desiredKO.Spec.JWTConfiguration.IssuerRef != nil && latestKO.Spec.JWTConfiguration.IssuerRef == nil { + latestKO.Spec.JWTConfiguration.IssuerRef = desiredKO.Spec.JWTConfiguration.IssuerRef + } +` + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 3) + require.NoError(err) + assert.Equal(expected, got) +} + +func Test_EnsureReferences_ReferenceWithinMap_IsRejected(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-reference-in-map.yaml", + }) + + // Stage's RouteSettings is a RouteSettingsMap, so LoggingLevel is reachable + // only by inventing a map key -- worse than the list case, which at least has + // positions. Generation must fail rather than silently miss the reference. + crd := testutil.GetCRDByName(t, g, "Stage") + require.NotNil(crd) + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.Error(err) + assert.Contains(err.Error(), "references cannot be within a map") + assert.Equal("", got, "nothing may be emitted when generation fails") +} + +func Test_EnsureReferences_MissingAncestorField_IsRejected(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + g := testutil.NewModelForServiceWithOptions(t, "s3", + &testutil.TestingModelOptions{ + GeneratorConfigFile: "generator-with-nested-references.yaml", + }) + + crd := testutil.GetCRDByName(t, g, "Bucket") + require.NotNil(crd) + + // With the model intact the struct-nested reference under Logging is emitted. + // Establishing that first keeps the negative case below from being vacuous. + before, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.NoError(err) + require.Contains(before, "latestKO.Spec.Logging.LoggingEnabled.TargetBucketRef") + + // Drop the `Logging` ancestor, leaving the reference field that walks through + // it. No generator.yaml can produce this -- the model always registers the + // ancestors of a field it registers -- so reaching the guard means breaking + // that invariant directly. The guard is what turns an inconsistent model into + // a build failure naming the path instead of a nil dereference. The model is + // built fresh per test, so the mutation cannot leak. + require.Contains(crd.Fields, "Logging") + delete(crd.Fields, "Logging") + + got, err := code.EnsureReferences(crd, "desiredKO", "latestKO", 1) + require.Error(err) + assert.Contains(err.Error(), `unable to find field with path "Logging"`) + assert.Contains(err.Error(), `resource "Bucket"`) + assert.Equal("", got, "nothing may be emitted when generation fails") +} diff --git a/pkg/testdata/models/apis/apigatewayv2/0000-00-00/generator-with-reference-in-map.yaml b/pkg/testdata/models/apis/apigatewayv2/0000-00-00/generator-with-reference-in-map.yaml new file mode 100644 index 000000000..d26de1713 --- /dev/null +++ b/pkg/testdata/models/apis/apigatewayv2/0000-00-00/generator-with-reference-in-map.yaml @@ -0,0 +1,22 @@ +resources: + Stage: + fields: + # A nonsense reference, to test the case of a reference reached through a + # MAP. RouteSettings is a RouteSettingsMap, so LoggingLevel is reachable + # only by inventing a map key; code generation must reject it. + RouteSettings.LoggingLevel: + references: + resource: Api + path: Spec.Name +ignore: + resource_names: + - ApiMapping + - Authorizer + - Deployment + - DomainName + - Integration + - IntegrationResponse + - Model + - Route + - RouteResponse + - VpcLink diff --git a/templates/pkg/resource/references.go.tpl b/templates/pkg/resource/references.go.tpl index 6f9506b76..243f9c9d9 100644 --- a/templates/pkg/resource/references.go.tpl +++ b/templates/pkg/resource/references.go.tpl @@ -58,6 +58,43 @@ func (rm *resourceManager) ClearResolvedReferences(res acktypes.AWSResource) (ac return &resource{ko} } +{{ if .CRD.HasReferenceFields -}} +{{ $ensureReferences := GoCodeEnsureReferences .CRD "desiredKO" "latestKO" 1 -}} +{{ if $ensureReferences -}} +// EnsureReferences restores, onto a copy of `latest`, the cross-resource +// reference (*Ref) fields it is missing, taking them from `desired`. Only +// reference fields are written, so every concrete value on `latest` stands. +// +// A *Ref is a sibling of the concrete field it resolves into, so rebuilding the +// containing struct from an AWS API response drops 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. +// +// Only references reached through structs are restored. A top-level *Ref needs no +// help, since every write path starts from a DeepCopy of the object it was handed. +// A *Ref reached through a list is not restored and remains subject to the above: +// it has no fixed address, and replacing the whole list instead would discard +// whatever the service populated inside it. +// +// `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 +// the resource they are handed, and some write API response values into it. +func (rm *resourceManager) EnsureReferences( + desired acktypes.AWSResource, + latest acktypes.AWSResource, +) acktypes.AWSResource { + // Deep copy the source as well, so a reference handed over below does not + // alias the caller's declared object. + desiredKO := rm.concreteResource(desired).ko.DeepCopy() + latestKO := rm.concreteResource(latest).ko.DeepCopy() + +{{ $ensureReferences }} + return &resource{latestKO} +} + +{{ end -}} +{{ end -}} // ResolveReferences finds if there are any Reference field(s) present // inside AWSResource passed in the parameter and attempts to resolve those // reference field(s) into their respective target field(s). It returns a