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