From 6f0d5cd060ff7517c3deabad62782dd2e0bc9543 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Fri, 25 Jul 2025 10:16:33 -0700 Subject: [PATCH] feat: return error on missing required fields During resource Adoption, the ACK controllers only returned an error when there was a missing PrimaryKey. If a Resource requires more than 1 field to describe a Resource (eg. Lambda alias requires Name and FunctionName), the controller only verifies the Primary identifier (ARN, ID, Name) and nothing else. With this change, we look into the SDK model to retrieve the required fields and return an error if all are not defined during `PopulateResourceFromAnnotation` --- pkg/generate/code/set_resource.go | 65 ++++++++++++++------------ pkg/generate/code/set_resource_test.go | 22 ++++----- 2 files changed, 47 insertions(+), 40 deletions(-) diff --git a/pkg/generate/code/set_resource.go b/pkg/generate/code/set_resource.go index f2f73664..eb5884ab 100644 --- a/pkg/generate/code/set_resource.go +++ b/pkg/generate/code/set_resource.go @@ -847,6 +847,9 @@ func identifierNameOrIDGuardConstructor( // return ackerrors.MissingNameIdentifier // } func requiredFieldGuardContructor( + // requiredFieldVarName is the variable where the requiredField value + // will be stored + requiredFieldVarName string, // String representing the fields map that contains the required // fields for adoption sourceVarName string, @@ -856,7 +859,7 @@ func requiredFieldGuardContructor( indentLevel int, ) string { indent := strings.Repeat("\t", indentLevel) - out := fmt.Sprintf("%stmp, ok := %s[\"%s\"]\n", indent, sourceVarName, requiredField) + out := fmt.Sprintf("%s%s, ok := %s[\"%s\"]\n", indent, requiredFieldVarName, sourceVarName, requiredField) out += fmt.Sprintf("%sif !ok {\n", indent) out += fmt.Sprintf("%s\treturn ackerrors.NewTerminalError(fmt.Errorf(\"required field missing: %s\"))\n", indent, requiredField) out += fmt.Sprintf("%s}\n", indent) @@ -1258,11 +1261,11 @@ func SetResourceIdentifiers( // // ``` // -// tmp, ok := field["brokerID"] +// primaryKey, ok := field["brokerID"] // if !ok { -// return ackerrors.MissingNameIdentifier +// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: brokerID")) // } -// r.ko.Status.BrokerID = &tmp +// r.ko.Status.BrokerID = &primaryKey // // ``` // @@ -1270,18 +1273,18 @@ func SetResourceIdentifiers( // // ``` // -// tmp, ok := field["resourceID"] -// if !ok { -// return ackerrors.MissingNameIdentifier -// } -// -// r.ko.Spec.ResourceID = &tmp +// primaryKey, ok := field["resourceID"] +// if !ok { +// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: resourceID")) +// } // -// f0, f0ok := fields["scalableDimension"] +// r.ko.Spec.ResourceID = &primaryKey // -// if f0ok { -// r.ko.Spec.ScalableDimension = &f0 -// } +// f0, ok := fields["scalableDimension"] +// if !ok { +// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: scalableDimension")) +// } +// r.ko.Spec.ScalableDimension = &f0 // // f1, f1ok := fields["serviceNamespace"] // @@ -1295,17 +1298,17 @@ func SetResourceIdentifiers( // ``` // // tmpArn, ok := field["arn"] -// if !ok { -// return ackerrors.MissingNameIdentifier +// if !ok { +// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: arn")) // } // if r.ko.Status.ACKResourceMetadata == nil { // r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{} // } // arn := ackv1alpha1.AWSResourceName(tmp) // -// r.ko.Status.ACKResourceMetadata.ARN = &arn +// r.ko.Status.ACKResourceMetadata.ARN = &arn // -// f0, f0ok := fields["modelPackageName"] +// f0, f0ok := fields["modelPackageName"] // // if f0ok { // r.ko.Spec.ModelPackageName = &f0 @@ -1355,10 +1358,10 @@ func PopulateResourceFromAnnotation( out := "\n" // Check if the CRD defines the primary keys primaryKeyConditionalOut := "\n" - primaryKeyConditionalOut += requiredFieldGuardContructor(sourceVarName, "arn", indentLevel) + primaryKeyConditionalOut += requiredFieldGuardContructor("resourceARN", sourceVarName, "arn", indentLevel) arnOut += ackResourceMetadataGuardConstructor(fmt.Sprintf("%s.Status", targetVarName), indentLevel) arnOut += fmt.Sprintf( - "%sarn := ackv1alpha1.AWSResourceName(tmp)\n", + "%sarn := ackv1alpha1.AWSResourceName(resourceARN)\n", indent, ) arnOut += fmt.Sprintf( @@ -1377,9 +1380,10 @@ func PopulateResourceFromAnnotation( isPrimarySet := primaryField != nil if isPrimarySet { memberPath, _ := findFieldInCR(cfg, r, primaryField.Names.Original) - primaryKeyOut += requiredFieldGuardContructor(sourceVarName, primaryField.Names.CamelLower, indentLevel) + primaryKeyOut += requiredFieldGuardContructor("primaryKey", sourceVarName, primaryField.Names.CamelLower, indentLevel) targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath) primaryKeyOut += setResourceIdentifierPrimaryIdentifierAnn(cfg, r, + "&primaryKey", primaryField, targetVarPath, sourceVarName, @@ -1451,18 +1455,19 @@ func PopulateResourceFromAnnotation( switch targetField.ShapeRef.Shape.Type { case "list", "structure", "map": panic("primary identifier '" + targetField.Path + "' must be a scalar type since NameOrID is a string") - default: - break } targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath) - if isPrimaryIdentifier { - primaryKeyOut += requiredFieldGuardContructor(sourceVarName, targetField.Names.CamelLower, indentLevel) + if inputShape.IsRequired(memberName) || isPrimaryIdentifier { + requiredFieldVarName := fmt.Sprintf("f%d", memberIndex) + primaryKeyOut += requiredFieldGuardContructor(requiredFieldVarName, sourceVarName, targetField.Names.CamelLower, indentLevel) primaryKeyOut += setResourceIdentifierPrimaryIdentifierAnn(cfg, r, + fmt.Sprintf("&%s", requiredFieldVarName), targetField, targetVarPath, sourceVarName, - indentLevel) + indentLevel, + ) } else { additionalKeyOut += setResourceIdentifierAdditionalKeyAnn( cfg, r, @@ -1471,7 +1476,8 @@ func PopulateResourceFromAnnotation( targetVarPath, sourceVarName, names.New(fieldName).CamelLower, - indentLevel) + indentLevel, + ) } } @@ -1539,6 +1545,8 @@ func setResourceIdentifierPrimaryIdentifier( func setResourceIdentifierPrimaryIdentifierAnn( cfg *ackgenconfig.Config, r *model.CRD, + // The variable used for required key + requiredFieldVarName string, // The field that will be set on the target variable targetField *model.Field, // The variable name that we want to set a value to @@ -1548,12 +1556,11 @@ func setResourceIdentifierPrimaryIdentifierAnn( // Number of levels of indentation to use indentLevel int, ) string { - adaptedMemberPath := fmt.Sprintf("&tmp") qualifiedTargetVar := fmt.Sprintf("%s.%s", targetVarName, targetField.Path) return setResourceForScalar( qualifiedTargetVar, - adaptedMemberPath, + requiredFieldVarName, targetField.ShapeRef, indentLevel, false, diff --git a/pkg/generate/code/set_resource_test.go b/pkg/generate/code/set_resource_test.go index b833ea67..1d0fea43 100644 --- a/pkg/generate/code/set_resource_test.go +++ b/pkg/generate/code/set_resource_test.go @@ -204,7 +204,6 @@ func TestSetResource_APIGWv2_Route_ReadOne(t *testing.T) { ) } - func TestSetResource_SageMaker_Domain_ReadOne(t *testing.T) { assert := assert.New(t) require := require.New(t) @@ -1800,11 +1799,11 @@ func TestSetResource_EKS_Cluster_PopulateResourceFromAnnotation(t *testing.T) { require.NotNil(crd) expected := ` - tmp, ok := fields["name"] + f0, ok := fields["name"] if !ok { return ackerrors.NewTerminalError(fmt.Errorf("required field missing: name")) } - r.ko.Spec.Name = &tmp + r.ko.Spec.Name = &f0 ` assert.Equal( @@ -1823,7 +1822,7 @@ func TestSetResource_SageMaker_ModelPackage_PopulateResourceFromAnnotation(t *te require.NotNil(crd) expected := ` - tmp, ok := identifier["arn"] + resourceARN, ok := identifier["arn"] if !ok { return ackerrors.NewTerminalError(fmt.Errorf("required field missing: arn")) } @@ -1831,7 +1830,7 @@ func TestSetResource_SageMaker_ModelPackage_PopulateResourceFromAnnotation(t *te if r.ko.Status.ACKResourceMetadata == nil { r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{} } - arn := ackv1alpha1.AWSResourceName(tmp) + arn := ackv1alpha1.AWSResourceName(resourceARN) r.ko.Status.ACKResourceMetadata.ARN = &arn ` assert.Equal( @@ -1850,16 +1849,17 @@ func TestSetResource_APIGWV2_ApiMapping_PopulateResourceFromAnnotation(t *testin require.NotNil(crd) expected := ` - tmp, ok := fields["apiMappingID"] + f0, ok := fields["apiMappingID"] if !ok { return ackerrors.NewTerminalError(fmt.Errorf("required field missing: apiMappingID")) } - r.ko.Status.APIMappingID = &tmp - - f1, f1ok := fields["domainName"] - if f1ok { - r.ko.Spec.DomainName = aws.String(f1) + r.ko.Status.APIMappingID = &f0 + f1, ok := fields["domainName"] + if !ok { + return ackerrors.NewTerminalError(fmt.Errorf("required field missing: domainName")) } + r.ko.Spec.DomainName = &f1 + ` assert.Equal( expected,