feat: return error on missing required fields during adoption (#613)

fixes [#2557](aws-controllers-k8s/community#2557)

Description of changes:
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`

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

Author: michaelhtm

pkg/generate/code/set_resource.go

@@ -843,10 +843,14 @@ func identifierNameOrIDGuardConstructor(
 // the required field for a read, or returning an error here
 // and returns a `MissingNameIdentifier` error:
 //
-//	if fields[${requiredField}] == "" {
-//	 return ackerrors.MissingNameIdentifier
+//  f0, ok := fields[${requiredField}]
+//	if !ok {
+//	 return ackerrors.NewTerminalError(fmt.Errorf("required field missing: ${requiredField}"))
 //	}
 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 +860,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,30 +1262,30 @@ 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
 //
 // ```
 //
 // An example of code with additional keys:
 //
 // ```
 //
-//	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 +1299,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 +1359,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 +1381,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 +1456,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 +1477,8 @@ func PopulateResourceFromAnnotation(
 				targetVarPath,
 				sourceVarName,
 				names.New(fieldName).CamelLower,
-				indentLevel)
+				indentLevel,
+			)
 		}
 	}
 
@@ -1539,6 +1546,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 +1557,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,

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,15 +1822,15 @@ 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"))
 	}
 
 	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,