feat: generate EnsureReferences to preserve nested cross-resource references

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)
 		},

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
+}

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")
+}