Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/generate/ack/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ var (
"GoCodeRequiredFieldsMissingFromReadOneInput": func(r *ackmodel.CRD, koVarName string, indentLevel int) string {
return code.CheckRequiredFieldsMissingFromShape(r, ackmodel.OpTypeGet, koVarName, indentLevel)
},
"GoCodeRequiredFieldsMissingFromReadManyInput": func(r *ackmodel.CRD, koVarName string, indentLevel int) string {
return code.CheckRequiredFieldsMissingFromShape(r, ackmodel.OpTypeList, koVarName, indentLevel)
},
"GoCodeRequiredFieldsMissingFromGetAttributesInput": func(r *ackmodel.CRD, koVarName string, indentLevel int) string {
return code.CheckRequiredFieldsMissingFromShape(r, ackmodel.OpTypeGetAttributes, koVarName, indentLevel)
},
Expand Down
73 changes: 73 additions & 0 deletions pkg/generate/code/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"strings"

awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
"github.com/gertd/go-pluralize"

ackgenconfig "github.com/aws-controllers-k8s/code-generator/pkg/generate/config"
"github.com/aws-controllers-k8s/code-generator/pkg/model"
Expand Down Expand Up @@ -74,6 +75,10 @@ func CheckRequiredFieldsMissingFromShape(
switch opType {
case model.OpTypeGet:
op = r.Ops.ReadOne
case model.OpTypeList:
op = r.Ops.ReadMany
return checkRequiredFieldsMissingFromShapeReadMany(
r, koVarName, indentLevel, op, op.InputRef.Shape)
case model.OpTypeGetAttributes:
op = r.Ops.GetAttributes
case model.OpTypeSetAttributes:
Expand Down Expand Up @@ -152,6 +157,74 @@ func checkRequiredFieldsMissingFromShape(
return fmt.Sprintf("%sreturn %s\n", indent, missingCondition)
}

// checkRequiredFieldsMissingFromShapeReadMany is a special-case handling
// of those APIs where there is no ReadOne operation and instead the only way to
// grab information for a single object is to call the ReadMany/List operation
// with one of more filtering fields-- specifically identifier(s). This method
// locates an identifier field in the shape that can be populated with an
// identifier value from the CR.
//
//
// As an example, DescribeVpcs EC2 API call doesn't have a ReadOne operation or
// required fields. However, the input shape has a VpcIds field which can be
// populated using a VpcId, a field in the VPC CR's Status. Therefore, require
// the VpcId field to be present to ensure the returned array from the API call
// consists only of the desired Vpc.
//
// Sample Output:
//
// return r.ko.Status.VPCID == nil
Comment on lines +160 to +176
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great explanation. ++

func checkRequiredFieldsMissingFromShapeReadMany(
r *model.CRD,
koVarName string,
indentLevel int,
op *awssdkmodel.Operation,
shape *awssdkmodel.Shape,
) string {
indent := strings.Repeat("\t", indentLevel)
result := fmt.Sprintf("%sreturn false", indent)

shapeIdentifiers := FindIdentifiersInShape(r, shape)
crIdentifiers := FindIdentifiersInCRD(r)
if len(shapeIdentifiers) == 0 || len(crIdentifiers) == 0 {
return result
}

pluralize := pluralize.NewClient()
reqIdentifier := ""
for _, si := range shapeIdentifiers {
for _, ci := range crIdentifiers {
if strings.EqualFold(pluralize.Singular(si),
pluralize.Singular(ci)) {
// The CRD identifiers being used for comparison reflect the
// *original* field names in the API model shape.
// Field renames are handled below in the call to
// getSanitizedMemberPath.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if reqIdentifier == "" {
reqIdentifier = ci
} else {
// If there are multiple identifiers, then prioritize the
// 'Id' identifier. Checking 'Id' to determine resource
// creation should be safe as the field is usually
// present in CR.Status.
if !strings.HasSuffix(reqIdentifier, "Id") ||
!strings.HasSuffix(reqIdentifier, "Ids") {
reqIdentifier = ci
}
}
}
}
}

resVarPath, err := getSanitizedMemberPath(reqIdentifier, r, op, koVarName)
if err != nil {
return result
}

result = fmt.Sprintf("%s == nil", resVarPath)
return fmt.Sprintf("%sreturn %s\n", indent, result)
}

// getSanitizedMemberPath takes a shape member field, checks for renames, checks
// for existence in Spec and Status, then constructs and returns the var path.
// Returns error if memberName is not present in either Spec or Status.
Expand Down
21 changes: 21 additions & 0 deletions pkg/generate/code/check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,24 @@ func TestCheckRequiredFields_RenamedSpecField(t *testing.T) {
strings.TrimSpace(gotCode),
)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted a test where the Shape/CR have multiple identifiers, but the Id gets prioritized. The only one I could think of was SecurityGroups. The DescribeInput can take either Name or Id, and the SecurityGroup CR has both Name in the Spec and Id in the Status.

In this scenario Id should be a required field. However, when trying to test this I uncovered an edge case in the FindIdentifiers helper funcs. No identifiers are returned for SecurityGroup CR or the DescribeInput because the fields are not securityGroupName/securityGroupId, but groupName/groupId. 😒

I can address this edge case in a different PR, but if anyone else has a good API to test the above case I'd love to add and test it now!

func TestCheckRequiredFields_StatusField_ReadMany(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewModelForService(t, "ec2")

crd := testutil.GetCRDByName(t, g, "Vpc")
require.NotNil(crd)

expRequiredFieldsCode := `
return r.ko.Status.VPCID == nil
`
gotCode := code.CheckRequiredFieldsMissingFromShape(
crd, model.OpTypeList, "r.ko", 1,
)
assert.Equal(
strings.TrimSpace(expRequiredFieldsCode),
strings.TrimSpace(gotCode),
)
}
27 changes: 16 additions & 11 deletions pkg/generate/code/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,24 @@ import (
)

// FindIdentifiersInShape returns the identifier fields of a given shape which
// can be singular or plural. Errors iff multiple identifier fields detected
// in the shape.
// can be singular or plural.
func FindIdentifiersInShape(
r *model.CRD,
shape *awssdkmodel.Shape) []string {
var identifiers []string
if r == nil || shape == nil {
return identifiers
}
identifierLookup := []string{
"Id",
"Ids",
r.Names.Original + "Id",
r.Names.Original + "Ids",
"Name",
"Names",
r.Names.Original + "Name",
r.Names.Original + "Names",
r.Names.Original + "Id",
r.Names.Original + "Ids",
}
var identifiers []string

for _, memberName := range shape.MemberNames() {
if util.InStrings(memberName, identifierLookup) {
Expand All @@ -47,22 +49,25 @@ func FindIdentifiersInShape(
return identifiers
}

// FindIdentifiersInCRD returns the identifier field of a given CRD which
// can be singular or plural. Errors iff multiple identifier fields detected
// in the CRD.
// FindIdentifiersInCRD returns the identifier fields of a given CRD which
// can be singular or plural. Note, these fields will be the *original* field
// names from the API model shape, not renamed field names.
func FindIdentifiersInCRD(
r *model.CRD) []string {
var identifiers []string
if r == nil {
return identifiers
}
identifierLookup := []string{
"Id",
"Ids",
r.Names.Original + "Id",
r.Names.Original + "Ids",
"Name",
"Names",
r.Names.Original + "Name",
r.Names.Original + "Names",
r.Names.Original + "Id",
r.Names.Original + "Ids",
}
var identifiers []string

for _, id := range identifierLookup {
_, found := r.SpecFields[id]
Expand Down
20 changes: 20 additions & 0 deletions templates/pkg/resource/sdk_find_read_many.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ func (rm *resourceManager) sdkFind(
{{- if $hookCode := Hook .CRD "sdk_read_many_pre_build_request" }}
{{ $hookCode }}
{{- end }}
// If any required fields in the input shape are missing, AWS resource is
// not created yet. Return NotFound here to indicate to callers that the
// resource isn't yet created.
if rm.requiredFieldsMissingFromReadManyInput(r) {
return nil, ackerr.NotFound
}

input, err := rm.newListRequestPayload(r)
if err != nil {
return nil, err
Expand Down Expand Up @@ -51,6 +58,19 @@ func (rm *resourceManager) sdkFind(
return &resource{ko}, nil
}

// requiredFieldsMissingFromReadManyInput returns true if there are any fields
// for the ReadMany Input shape that are required but not present in the
// resource's Spec or Status
func (rm *resourceManager) requiredFieldsMissingFromReadManyInput(
r *resource,
) bool {
{{- if $customCheckMethod := .CRD.GetCustomCheckRequiredFieldsMissingMethod .CRD.Ops.ReadMany }}
return rm.{{ $customCheckMethod }}(r)
{{- else }}
{{ GoCodeRequiredFieldsMissingFromReadManyInput .CRD "r.ko" 1 }}
{{- end }}
}

// newListRequestPayload returns SDK-specific struct for the HTTP request
// payload of the List API call for the resource
func (rm *resourceManager) newListRequestPayload(
Expand Down