Support for nested resource references (#314)

Issue #, if available: aws-controllers-k8s/community#1212

This PR aims to add support for adding ResourceReferences for nested fields. I have tested these changes locally by generating and compiling *eks-controller* and *apigatewayv2-controller*, and also added unit tests.

In a nut-shell, this PR mainly updates the reference resolution to use `field.Path` instead of `field.Names` with proper nil checks. This PR also updates the TypeDefs to add new *AWSResourceReference* attribute, instead of only supporting Reference fields at top-level. More detailed desciption about the changes is present below.

Note: I have broken all the changes into multiple commits and i recommend to review this PR commit-by-commit. : )

Description of changes:
* Add new reusable go code generator method for producing Nil check logical condition on a *fieldPath*
* Wrap the *ReferenceFieldValidation* with proper Nil check for a fieldPath and correctly use *field.Path* for accessing the value instead of `field.Names.Camel`
* Include Nil checks inside `ReferenceFieldPresent` method for the `field.Path` and capture the ReferenceFieldPresent condition for each field inside parentheses to make it more readable.
* Make the ResourceFieldConfig lookup case-insensitive for better developer experience . This will allow developers to not worry about case-sensitivity when creating FieldConfig keys inside `generator.yaml`
* Update `crd.HasReference` method to work for nested fields as well.
* Update *TypeDef* generation inside `model.go` to also include new *AWSResourceReference* attribute if a nested field *ReferencesConfig* is present.
* Update `references.go.tpl` to correctly use *field.Path* instead of *field.Names* to resolve references.
* Update `references.go.tpl` to support string type-alias by performing type-casting before resolving a reference.(issue# aws-controllers-k8s/community#1213)

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

Author: vijtrip2

pkg/generate/ack/controller.go

@@ -158,6 +158,9 @@ var (
 		"GoCodeContainsReferences": func(r *ackmodel.CRD, sourceVarName string) string {
 			return code.ReferenceFieldsPresent(r, sourceVarName)
 		},
+		"CheckNilFieldPath": func(f *ackmodel.Field, sourceVarName string) string {
+			return code.CheckNilFieldPath(f, sourceVarName)
+		},
 	}
 )
 

pkg/generate/code/check.go

@@ -19,6 +19,7 @@ import (
 
 	awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
 
+	"github.com/aws-controllers-k8s/code-generator/pkg/fieldpath"
 	ackgenconfig "github.com/aws-controllers-k8s/code-generator/pkg/generate/config"
 	"github.com/aws-controllers-k8s/code-generator/pkg/model"
 )
@@ -192,3 +193,26 @@ func checkRequiredFieldsMissingFromShapeReadMany(
 	result = fmt.Sprintf("%s == nil", resVarPath)
 	return fmt.Sprintf("%sreturn %s\n", indent, result)
 }
+
+// CheckNilFieldPath returns the condition statement for Nil check
+// on a field path. This nil check on field path is useful to avoid
+// nil pointer panics when accessing a field value.
+//
+// This function only outputs the logical condition and not the "if" block
+// so that the output can be reused in many templates, where
+// logic inside "if" block can be different.
+//
+// Example Output for fieldpath "JWTConfiguration.Issuer.SomeField" is
+// "ko.Spec.JWTConfiguration == nil || ko.Spec.JWTConfiguration.Issuer == nil"
+func CheckNilFieldPath(field *model.Field, sourceVarName string) string {
+	out := ""
+	fp := fieldpath.FromString(field.Path)
+	// remove fieldName from fieldPath before adding nil checks
+	fp.Pop()
+	fieldNamePrefix := ""
+	for fp.Size() > 0 {
+		fieldNamePrefix = fmt.Sprintf("%s.%s", fieldNamePrefix, fp.PopFront())
+		out += fmt.Sprintf(" || %s%s == nil", sourceVarName, fieldNamePrefix)
+	}
+	return strings.TrimPrefix(out, " || ")
+}

pkg/generate/code/check_test.go

@@ -136,3 +136,22 @@ func TestCheckRequiredFields_StatusField_ReadMany(t *testing.T) {
 		strings.TrimSpace(gotCode),
 	)
 }
+
+func TestCheckNilFieldPath(t *testing.T) {
+	// Empty FieldPath
+	field := model.Field{Path: ""}
+	assert.Equal(t, "", code.CheckNilFieldPath(&field, "ko.Spec"))
+	// FieldPath only has fieldName
+	field.Path = "JWTConfiguration"
+	assert.Equal(t, "", code.CheckNilFieldPath(&field, "ko.Spec"))
+	// Nested FieldPath
+	field.Path = "JWTConfiguration.Issuer"
+	assert.Equal(t,
+		"ko.Spec.JWTConfiguration == nil",
+		code.CheckNilFieldPath(&field, "ko.Spec"))
+	// Multi Level Nested FieldPath
+	field.Path = "JWTConfiguration.Issuer.FieldName"
+	assert.Equal(t,
+		"ko.Spec.JWTConfiguration == nil || ko.Spec.JWTConfiguration.Issuer == nil",
+		code.CheckNilFieldPath(&field, "ko.Spec"))
+}

pkg/generate/code/resource_reference.go

@@ -17,6 +17,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/aws-controllers-k8s/code-generator/pkg/fieldpath"
 	"github.com/aws-controllers-k8s/code-generator/pkg/model"
 )
 
@@ -35,32 +36,54 @@ func ReferenceFieldsValidation(
 	indentLevel int,
 ) string {
 	out := ""
+	fieldAccessPrefix := fmt.Sprintf("%s%s", sourceVarName,
+		crd.Config().PrefixConfig.SpecField)
 	// Sorted fieldnames are used for consistent code-generation
 	for _, fieldName := range crd.SortedFieldNames() {
 		field := crd.Fields[fieldName]
+		var fIndent string
 		if field.HasReference() {
-			indent := strings.Repeat("\t", indentLevel)
+			fIndentLevel := indentLevel
+			fp := fieldpath.FromString(field.Path)
+			// remove fieldName from fieldPath before adding nil checks
+			fp.Pop()
+			fieldNamePrefix := crd.Config().PrefixConfig.SpecField
+			// this loop outputs a nil-guard for each level of nested field path
+			for fp.Size() > 0 {
+				fIndent = strings.Repeat("\t", fIndentLevel)
+				fieldNamePrefix = fmt.Sprintf("%s.%s", fieldNamePrefix, fp.PopFront())
+				out += fmt.Sprintf("%sif %s%s != nil {\n", fIndent, sourceVarName, fieldNamePrefix)
+				fIndentLevel++
+			}
+			fIndent = strings.Repeat("\t", fIndentLevel)
 			// Validation to make sure both target field and reference are
 			// not present at the same time in desired resource
-			out += fmt.Sprintf("%sif %s.Spec.%s != nil"+
-				" && %s.Spec.%s != nil {\n", indent, sourceVarName,
-				field.GetReferenceFieldName().Camel, sourceVarName, field.Names.Camel)
+			out += fmt.Sprintf("%sif %s.%s != nil"+
+				" && %s.%s != nil {\n", fIndent, fieldAccessPrefix,
+				field.ReferenceFieldPath(), fieldAccessPrefix, field.Path)
 			out += fmt.Sprintf("%s\treturn "+
 				"ackerr.ResourceReferenceAndIDNotSupportedFor(\"%s\", \"%s\")\n",
-				indent, field.Names.Camel, field.GetReferenceFieldName().Camel)
-			out += fmt.Sprintf("%s}\n", indent)
+				fIndent, field.Path, field.ReferenceFieldPath())
+
+			// Close out all the curly braces with proper indentation
+			for fIndentLevel >= indentLevel {
+				fIndent = strings.Repeat("\t", fIndentLevel)
+				out += fmt.Sprintf("%s}\n", fIndent)
+				fIndentLevel--
+			}
+
+			fIndent = strings.Repeat("\t", indentLevel)
 
 			// If the field is required, make sure either Ref or original
 			// field is present in the resource
 			if field.IsRequired() {
-				out += fmt.Sprintf("%sif %s.Spec.%s == nil &&"+
-					" %s.Spec.%s == nil {\n", indent, sourceVarName,
-					field.GetReferenceFieldName().Camel, sourceVarName,
-					field.Names.Camel)
+				out += fmt.Sprintf("%sif %s.%s == nil &&"+
+					" %s.%s == nil {\n", fIndent, fieldAccessPrefix,
+					field.ReferenceFieldPath(), fieldAccessPrefix, field.Path)
 				out += fmt.Sprintf("%s\treturn "+
 					"ackerr.ResourceReferenceOrIDRequiredFor(\"%s\", \"%s\")\n",
-					indent, field.Names.Camel, field.GetReferenceFieldName().Camel)
-				out += fmt.Sprintf("%s}\n", indent)
+					fIndent, field.Path, field.ReferenceFieldPath())
+				out += fmt.Sprintf("%s}\n", fIndent)
 			}
 		}
 	}
@@ -71,18 +94,31 @@ func ReferenceFieldsValidation(
 // a non-nil reference field is present in a resource. This checks helps in deciding
 // whether ACK.ReferencesResolved condition should be added to resource status
 // Sample Code:
-// return false || ko.Spec.APIRef != nil
+// return false || (ko.Spec.APIRef != nil)
 func ReferenceFieldsPresent(
 	crd *model.CRD,
 	sourceVarName string,
 ) string {
 	out := "false"
+	fieldAccessPrefix := fmt.Sprintf("%s%s", sourceVarName,
+		crd.Config().PrefixConfig.SpecField)
 	// Sorted fieldnames are used for consistent code-generation
 	for _, fieldName := range crd.SortedFieldNames() {
 		field := crd.Fields[fieldName]
-		if field.IsReference() {
-			out += fmt.Sprintf(" || %s.Spec.%s != nil", sourceVarName,
-				field.Names.Camel)
+		if field.HasReference() {
+			out += " || ("
+			fp := fieldpath.FromString(field.Path)
+			// remove fieldName from fieldPath before adding nil checks
+			// for nested fieldPath
+			fp.Pop()
+			fieldNamePrefix := ""
+			for fp.Size() > 0 {
+				fieldNamePrefix = fmt.Sprintf("%s.%s", fieldNamePrefix, fp.PopFront())
+				out += fmt.Sprintf("%s%s != nil && ", fieldAccessPrefix, fieldNamePrefix)
+			}
+			out += fmt.Sprintf("%s.%s != nil", fieldAccessPrefix,
+				field.ReferenceFieldPath())
+			out += ")"
 		}
 	}
 	return out

pkg/generate/code/resource_reference_test.go

@@ -86,6 +86,27 @@ func Test_ReferenceFieldsValidation_SliceOfReferences(t *testing.T) {
 	assert.Equal(expected, code.ReferenceFieldsValidation(crd, "ko", 1))
 }
 
+func Test_ReferenceFieldsValidation_NestedReference(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 ko.Spec.JWTConfiguration != nil {
+		if ko.Spec.JWTConfiguration.IssuerRef != nil && ko.Spec.JWTConfiguration.Issuer != nil {
+			return ackerr.ResourceReferenceAndIDNotSupportedFor("JWTConfiguration.Issuer", "JWTConfiguration.IssuerRef")
+		}
+	}
+`
+	assert.Equal(expected, code.ReferenceFieldsValidation(crd, "ko", 1))
+}
+
 func Test_ReferenceFieldsPresent_NoReferenceConfig(t *testing.T) {
 	assert := assert.New(t)
 	require := require.New(t)
@@ -112,7 +133,7 @@ func Test_ReferenceFieldsPresent_SingleReference(t *testing.T) {
 
 	crd := testutil.GetCRDByName(t, g, "Integration")
 	require.NotNil(crd)
-	expected := "false || ko.Spec.APIRef != nil"
+	expected := "false || (ko.Spec.APIRef != nil)"
 	assert.Equal(expected, code.ReferenceFieldsPresent(crd, "ko"))
 }
 
@@ -129,6 +150,21 @@ func Test_ReferenceFieldsPresent_SliceOfReferences(t *testing.T) {
 	// just to test code generation for slices of reference
 	crd := testutil.GetCRDByName(t, g, "VpcLink")
 	require.NotNil(crd)
-	expected := "false || ko.Spec.SecurityGroupRefs != nil || ko.Spec.SubnetRefs != nil"
+	expected := "false || (ko.Spec.SecurityGroupRefs != nil) || (ko.Spec.SubnetRefs != nil)"
+	assert.Equal(expected, code.ReferenceFieldsPresent(crd, "ko"))
+}
+
+func Test_ReferenceFieldsPresent_NestedReference(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 := "false || (ko.Spec.JWTConfiguration != nil && ko.Spec.JWTConfiguration.IssuerRef != nil)"
 	assert.Equal(expected, code.ReferenceFieldsPresent(crd, "ko"))
 }

pkg/generate/config/resource.go

@@ -14,6 +14,8 @@
 package config
 
 import (
+	"strings"
+
 	"github.com/aws-controllers-k8s/code-generator/pkg/util"
 )
 
@@ -392,6 +394,30 @@ func (c *Config) ResourceFields(resourceName string) map[string]*FieldConfig {
 	return resourceConfig.Fields
 }
 
+// ResourceFieldByPath returns the FieldConfig for a field from
+// "resourceName" crd, where field.Path matches the passed "fieldPath" parameter.
+// This method performs the case-insensitive resource and fieldPath lookup.
+func (c *Config) ResourceFieldByPath(resourceName string, fieldPath string) *FieldConfig {
+	var resourceConfig ResourceConfig
+	if c == nil {
+		return nil
+	}
+
+	for resName, resConfig := range c.Resources {
+		if strings.EqualFold(resName, resourceName) {
+			resourceConfig = resConfig
+			break
+		}
+	}
+
+	for fPath, fConfig := range resourceConfig.Fields {
+		if strings.EqualFold(fPath, fieldPath) {
+			return fConfig
+		}
+	}
+	return nil
+}
+
 // GetCompareIgnoredFields returns the list of field path to ignore when
 // comparing two differnt objects
 func (c *Config) GetCompareIgnoredFields(resName string) []string {

pkg/model/crd.go

@@ -199,8 +199,7 @@ func (r *CRD) AddSpecField(
 	shapeRef *awssdkmodel.ShapeRef,
 ) {
 	fPath := memberNames.Camel
-	fConfigs := r.cfg.ResourceFields(r.Names.Original)
-	fConfig := fConfigs[memberNames.Original]
+	fConfig := r.cfg.ResourceFieldByPath(r.Names.Original, fPath)
 	f := NewField(r, fPath, memberNames, shapeRef, fConfig)
 	if fConfig != nil && fConfig.Print != nil {
 		r.addSpecPrintableColumn(f)
@@ -225,8 +224,7 @@ func (r *CRD) AddStatusField(
 	shapeRef *awssdkmodel.ShapeRef,
 ) {
 	fPath := memberNames.Camel
-	fConfigs := r.cfg.ResourceFields(r.Names.Original)
-	fConfig := fConfigs[memberNames.Original]
+	fConfig := r.cfg.ResourceFieldByPath(r.Names.Original, fPath)
 	f := NewField(r, fPath, memberNames, shapeRef, fConfig)
 	if fConfig != nil && fConfig.Print != nil {
 		r.addStatusPrintableColumn(f)
@@ -787,7 +785,7 @@ func (r *CRD) HasMember(
 // field. Otherwise returns false
 func (r *CRD) HasReferenceFields() bool {
 	for _, field := range r.Fields {
-		if field.IsReference() {
+		if field.HasReference() {
 			return true
 		}
 	}

pkg/model/field.go

@@ -178,6 +178,21 @@ func (f *Field) ReferencedResourceNamePlural() string {
 	return referencedResourceName
 }
 
+// ReferenceFieldPath returns the fieldPath for the corresponding
+// Reference field. It replaces the fieldName with ReferenceFieldName
+// at the end of fieldPath
+func (f *Field) ReferenceFieldPath() string {
+	fieldPathPrefix := strings.TrimSuffix(f.Path, f.Names.Camel)
+	return fmt.Sprintf("%s%s", fieldPathPrefix, f.GetReferenceFieldName().Camel)
+}
+
+// FieldPathWithUnderscore replaces the period in fieldPath with
+// underscore. This method is useful for generating go method
+// name from the fieldPath.
+func (f *Field) FieldPathWithUnderscore() string {
+	return strings.ReplaceAll(f.Path, ".", "_")
+}
+
 // NewReferenceField returns a pointer to a new Field object.
 // The go-type of field is either slice of '*AWSResourceReferenceWrapper' or
 // '*AWSResourceReferenceWrapper' depending on whether 'shapeRef' parameter

pkg/model/field_test.go

@@ -195,3 +195,35 @@ func TestGetReferenceFieldName(t *testing.T) {
 		assert.Equal(tc.expectedReferenceFieldName, referenceFieldName, msg)
 	}
 }
+
+func TestReferenceFieldPath(t *testing.T) {
+	assert := assert.New(t)
+
+	field := model.Field{}
+	//Non nested fieldPath
+	field.Path = "MyField"
+	field.Names = names.New("MyField")
+	assert.Equal("MyFieldRef", field.ReferenceFieldPath())
+
+	// Nested fieldPath
+	field.Path = "subPathA.subPathB.MyField"
+	field.Names = names.New("MyField")
+	assert.Equal("subPathA.subPathB.MyFieldRef", field.ReferenceFieldPath())
+}
+
+func TestFieldPathWithUnderscore(t *testing.T) {
+	assert := assert.New(t)
+
+	field := model.Field{}
+	//Empty fieldPath
+	field.Path = ""
+	assert.Equal("", field.FieldPathWithUnderscore())
+
+	//Non nested fieldPath
+	field.Path = "MyField"
+	assert.Equal("MyField", field.FieldPathWithUnderscore())
+
+	// Nested fieldPath
+	field.Path = "subPathA.subPathB.MyField"
+	assert.Equal("subPathA_subPathB_MyField", field.FieldPathWithUnderscore())
+}