removal of forProvider.name, fix http resp code 400 in the start of creating and some small enchantments

apis/servicecatalog/generator-config.yaml

@@ -14,8 +14,11 @@ ignore:
     - ProvisionProductInput.ProvisionToken
     - UpdateProvisioningParameter.UsePreviousValue
     - UpdateProvisioningPreferences.StackSetOperationType
+    - ProvisionProductInput.ProvisionedProductName
     - UpdateProvisionedProductInput.ProvisionedProductId
+    - UpdateProvisionedProductInput.ProvisionedProductName
     - TerminateProvisionedProductInput.ProvisionedProductId
+    - TerminateProvisionedProductInput.ProvisionedProductName
   shape_names:
     - ProvisionedProductPlanSummary
     - ProvisionedProductPlanDetails
@@ -95,9 +98,4 @@ resources:
         is_read_only: true
         from:
           operation: DescribeProvisionedProduct
-          path: ProvisionedProductDetail.StatusMessage
-    renames:
-      operations:
-        ProvisionProduct:
-          input_fields:
-            ProvisionedProductName: Name
+          path: ProvisionedProductDetail.StatusMessage

package/crds/servicecatalog.aws.crossplane.io_provisionedproducts.yaml

@@ -73,11 +73,6 @@ spec:
                     description: "The language code. \n * en - English (default) \n
                       * jp - Japanese \n * zh - Chinese"
                     type: string
-                  name:
-                    description: A user-friendly name for the provisioned product.
-                      This value must be unique for the Amazon Web Services account
-                      and cannot be updated after the product is provisioned.
-                    type: string
                   notificationARNs:
                     description: Passed to CloudFormation. The SNS topic ARNs to which
                       to publish stack-related events.
@@ -162,7 +157,6 @@ spec:
                       type: object
                     type: array
                 required:
-                - name
                 - region
                 type: object
               managementPolicies:
@@ -419,6 +413,9 @@ spec:
                   provisionedProductID:
                     description: The identifier of the provisioned product.
                     type: string
+                  provisionedProductName:
+                    description: The user-friendly name of the provisioned product.
+                    type: string
                   provisionedProductType:
                     description: The type of provisioned product. The supported values
                       are CFN_STACK and CFN_STACKSET.

pkg/controller/servicecatalog/provisionedproduct/setup.go

@@ -21,15 +21,12 @@ import (
 	"fmt"
 	"strings"
 
-	"k8s.io/apimachinery/pkg/types"
-
 	awsconfig "github.com/aws/aws-sdk-go-v2/config"
 	cfsdkv2 "github.com/aws/aws-sdk-go-v2/service/cloudformation"
 	cfsdkv2types "github.com/aws/aws-sdk-go-v2/service/cloudformation/types"
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	svcsdk "github.com/aws/aws-sdk-go/service/servicecatalog"
-	awsclient "github.com/crossplane-contrib/provider-aws/pkg/clients"
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/connection"
 	"github.com/crossplane/crossplane-runtime/pkg/controller"
@@ -40,12 +37,14 @@ import (
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	svcapitypes "github.com/crossplane-contrib/provider-aws/apis/servicecatalog/v1alpha1"
 	"github.com/crossplane-contrib/provider-aws/apis/v1alpha1"
+	awsclient "github.com/crossplane-contrib/provider-aws/pkg/clients"
 	clientset "github.com/crossplane-contrib/provider-aws/pkg/clients/servicecatalog"
 	"github.com/crossplane-contrib/provider-aws/pkg/features"
 )
@@ -58,11 +57,11 @@ const (
 	msgProvisionedProductStatusSdkPlanInProgress = "provisioned product is awaiting plan approval"
 	msgProvisionedProductStatusSdkError          = "provisioned product has status ERROR"
 
-	errUpdatePending                        = "provisioned product is already under change, not updating"
 	errCouldNotGetProvisionedProductOutputs = "could not get provisioned product outputs"
 	errCouldNotGetCFParameters              = "could not get cloudformation stack parameters"
 	errCouldNotDescribeRecord               = "could not describe record"
 	errCouldNotLookupProduct                = "could not lookup product"
+	errCreatExternalNameIsNotValid          = "external name is not equal provisioned product name"
 	errAwsAPICodeInvalidParametersException = "Last Successful Provisioning Record doesn't exist."
 )
 
@@ -106,12 +105,12 @@ func SetupProvisionedProduct(mgr ctrl.Manager, o controller.Options) error {
 func prepareSetupExternal(cfClient *cfsdkv2.Client, kube client.Client) func(*external) {
 	return func(e *external) {
 		c := &custom{client: &clientset.CustomServiceCatalogClient{CfClient: cfClient, Client: e.client}, kube: kube}
-		e.isUpToDate = c.isUpToDate
+		e.preCreate = preCreate
+		e.preUpdate = c.preUpdate
 		e.lateInitialize = c.lateInitialize
+		e.isUpToDate = c.isUpToDate
+		e.preObserve = c.preObserve
 		e.postObserve = c.postObserve
-		e.preUpdate = c.preUpdate
-		e.preCreate = preCreate
-		e.postCreate = c.postCreate
 		e.preDelete = preDelete
 	}
 }
@@ -132,12 +131,31 @@ func (c *custom) lateInitialize(spec *svcapitypes.ProvisionedProductParameters,
 	return nil
 }
 
-func (c *custom) isUpToDate(_ context.Context, ds *svcapitypes.ProvisionedProduct, resp *svcsdk.DescribeProvisionedProductOutput) (bool, string, error) {
+func preCreate(_ context.Context, cr *svcapitypes.ProvisionedProduct, input *svcsdk.ProvisionProductInput) error {
+	input.ProvisionToken = aws.String(genIdempotencyToken())
+	if cr.GetName() != meta.GetExternalName(cr) {
+		return errors.New(errCreatExternalNameIsNotValid)
+	}
+	input.ProvisionedProductName = aws.String(meta.GetExternalName(cr))
+	return nil
+}
+
+func (c *custom) preUpdate(_ context.Context, cr *svcapitypes.ProvisionedProduct, input *svcsdk.UpdateProvisionedProductInput) error {
+	input.UpdateToken = aws.String(genIdempotencyToken())
+	if cr.GetName() == meta.GetExternalName(cr) {
+		input.ProvisionedProductName = aws.String(meta.GetExternalName(cr))
+	} else {
+		input.ProvisionedProductId = aws.String(meta.GetExternalName(cr))
+	}
+	return nil
+}
+
+func (c *custom) isUpToDate(ctx context.Context, ds *svcapitypes.ProvisionedProduct, resp *svcsdk.DescribeProvisionedProductOutput) (bool, string, error) { // nolint:gocyclo
 	// If the product is undergoing change, we want to assume that it is not up-to-date. This will force this resource
 	// to be queued for an update (which will be skipped due to UNDER_CHANGE), and once that update fails, we will
 	// recheck the status again. This will allow us to quickly transition from UNDER_CHANGE to AVAILABLE without having
 	// to wait for the entire polling interval to pass before re-checking the status.
-	if pointer.StringDeref(ds.Status.AtProvider.Status, "") == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) {
+	if pointer.StringDeref(resp.ProvisionedProductDetail.Status, "") == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) {
 		return true, "", nil
 	}
 
@@ -148,10 +166,8 @@ func (c *custom) isUpToDate(_ context.Context, ds *svcapitypes.ProvisionedProduc
 		// is wrong with the provisioned product (error on creation, tainted, etc)
 		// We will be able to handle those specific cases in postObserve
 		var aerr awserr.Error
-		if ok := errors.As(err, &aerr); ok {
-			if aerr.Code() == svcsdk.ErrCodeInvalidParametersException && aerr.Message() == errAwsAPICodeInvalidParametersException {
-				return false, "", nil
-			}
+		if ok := errors.As(err, &aerr); ok && aerr.Code() == svcsdk.ErrCodeInvalidParametersException && aerr.Message() == errAwsAPICodeInvalidParametersException {
+			return false, "", nil
 		}
 		return false, "", errors.Wrap(err, errCouldNotGetProvisionedProductOutputs)
 	}
@@ -161,26 +177,35 @@ func (c *custom) isUpToDate(_ context.Context, ds *svcapitypes.ProvisionedProduc
 		return false, "", errors.Wrap(err, errCouldNotGetCFParameters)
 	}
 
-	productOrArtifactAreChanged, err := c.productOrArtifactAreChanged(&ds.Spec.ForProvider, resp.ProvisionedProductDetail)
+	productOrArtifactIsChanged, err := c.productOrArtifactIsChanged(&ds.Spec.ForProvider, resp.ProvisionedProductDetail)
 	if err != nil {
 		return false, "", errors.Wrap(err, "could not discover if product or artifact ids have changed")
 	}
-	provisioningParamsAreChanged, err := c.provisioningParamsAreChanged(cfStackParameters, ds)
+	provisioningParamsAreChanged, err := c.provisioningParamsAreChanged(ctx, cfStackParameters, ds)
 	if err != nil {
 		return false, "", errors.Wrap(err, "could not compare provisioning parameters with previous ones")
 	}
 
-	if productOrArtifactAreChanged || provisioningParamsAreChanged {
+	if productOrArtifactIsChanged || provisioningParamsAreChanged {
 		return false, "", nil
 	}
 	return true, "", nil
 }
 
+func (c *custom) preObserve(_ context.Context, cr *svcapitypes.ProvisionedProduct, input *svcsdk.DescribeProvisionedProductInput) error {
+	if cr.GetName() == meta.GetExternalName(cr) {
+		input.Name = aws.String(meta.GetExternalName(cr))
+	} else {
+		input.Id = aws.String(meta.GetExternalName(cr))
+	}
+	return nil
+
+}
+
 func (c *custom) postObserve(_ context.Context, cr *svcapitypes.ProvisionedProduct, resp *svcsdk.DescribeProvisionedProductOutput, obs managed.ExternalObservation, err error) (managed.ExternalObservation, error) {
 	if err != nil {
 		return managed.ExternalObservation{}, err
 	}
-	meta.SetExternalName(cr, *cr.Spec.ForProvider.Name)
 
 	describeRecordInput := svcsdk.DescribeRecordInput{Id: resp.ProvisionedProductDetail.LastRecordId}
 	describeRecordOutput, err := c.client.DescribeRecord(&describeRecordInput)
@@ -214,63 +239,34 @@ func (c *custom) postObserve(_ context.Context, cr *svcapitypes.ProvisionedProdu
 	return obs, nil
 }
 
-func preCreate(_ context.Context, cr *svcapitypes.ProvisionedProduct, obj *svcsdk.ProvisionProductInput) error {
-	obj.ProvisionToken = aws.String(genIdempotencyToken())
-
-	// We want to specifically set this to match the forProvider name, as that
-	// is what we use to track the provisioned product
-	if cr.Spec.ForProvider.Name != nil {
-		meta.SetExternalName(cr, *cr.Spec.ForProvider.Name)
-	}
-
-	return nil
-}
-
-func (c *custom) postCreate(_ context.Context, cr *svcapitypes.ProvisionedProduct, obj *svcsdk.ProvisionProductOutput, cre managed.ExternalCreation, err error) (managed.ExternalCreation, error) {
-	if err != nil {
-		return cre, err
-	}
-
-	// We are expected to set the external-name annotation upon creation since
-	// it can differ from the metadata.name for the ProvisionedProduct
-	if obj.RecordDetail != nil && obj.RecordDetail.ProvisionedProductName != nil {
-		meta.SetExternalName(cr, *obj.RecordDetail.ProvisionedProductName)
-	}
-
-	return cre, nil
-}
-
-func (c *custom) preUpdate(_ context.Context, cr *svcapitypes.ProvisionedProduct, input *svcsdk.UpdateProvisionedProductInput) error {
-	if pointer.StringDeref(cr.Status.AtProvider.Status, "") == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) {
-		return errors.New(errUpdatePending)
-	}
-	input.UpdateToken = aws.String(genIdempotencyToken())
-	input.ProvisionedProductName = aws.String(meta.GetExternalName(cr))
-	return nil
-}
-
-func preDelete(_ context.Context, cr *svcapitypes.ProvisionedProduct, obj *svcsdk.TerminateProvisionedProductInput) (bool, error) {
+func preDelete(_ context.Context, cr *svcapitypes.ProvisionedProduct, input *svcsdk.TerminateProvisionedProductInput) (bool, error) {
 	if pointer.StringDeref(cr.Status.AtProvider.Status, "") == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) {
 		return true, nil
 	}
-	obj.TerminateToken = aws.String(genIdempotencyToken())
-	obj.ProvisionedProductName = aws.String(meta.GetExternalName(cr))
+	input.TerminateToken = aws.String(genIdempotencyToken())
+	if cr.GetName() == meta.GetExternalName(cr) {
+		input.ProvisionedProductName = aws.String(meta.GetExternalName(cr))
+	} else {
+		input.ProvisionedProductId = aws.String(meta.GetExternalName(cr))
+	}
 	return false, nil
 }
 
 func setConditions(describeRecordOutput *svcsdk.DescribeRecordOutput, resp *svcsdk.DescribeProvisionedProductOutput, cr *svcapitypes.ProvisionedProduct) {
-	recordType := pointer.StringDeref(describeRecordOutput.RecordDetail.RecordType, "UPDATE_PROVISIONED_PRODUCT")
-
 	ppStatus := aws.StringValue(resp.ProvisionedProductDetail.Status)
 	switch {
 	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_AVAILABLE):
 		cr.SetConditions(xpv1.Available())
-	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && recordType == "PROVISION_PRODUCT":
-		cr.SetConditions(xpv1.Creating())
-	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && recordType == "UPDATE_PROVISIONED_PRODUCT":
-		cr.SetConditions(xpv1.Available().WithMessage(msgProvisionedProductStatusSdkUnderChange))
-	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && recordType == "TERMINATE_PROVISIONED_PRODUCT":
-		cr.SetConditions(xpv1.Deleting())
+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE):
+		recordType := pointer.StringDeref(describeRecordOutput.RecordDetail.RecordType, "UPDATE_PROVISIONED_PRODUCT")
+		switch {
+		case recordType == "PROVISION_PRODUCT":
+			cr.SetConditions(xpv1.Creating())
+		case recordType == "UPDATE_PROVISIONED_PRODUCT":
+			cr.SetConditions(xpv1.Available().WithMessage(msgProvisionedProductStatusSdkUnderChange))
+		case recordType == "TERMINATE_PROVISIONED_PRODUCT":
+			cr.SetConditions(xpv1.Deleting())
+		}
 	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_PLAN_IN_PROGRESS):
 		cr.SetConditions(xpv1.Unavailable().WithMessage(msgProvisionedProductStatusSdkPlanInProgress))
 	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_ERROR):
@@ -280,29 +276,34 @@ func setConditions(describeRecordOutput *svcsdk.DescribeRecordOutput, resp *svcs
 	}
 }
 
-func (c *custom) provisioningParamsAreChanged(cfStackParams []cfsdkv2types.Parameter, ds *svcapitypes.ProvisionedProduct) (bool, error) {
+func (c *custom) provisioningParamsAreChanged(ctx context.Context, cfStackParams []cfsdkv2types.Parameter, ds *svcapitypes.ProvisionedProduct) (bool, error) {
 	nn := types.NamespacedName{
 		Name: ds.GetName(),
 	}
-	mr := svcapitypes.ProvisionedProduct{}
-	err := c.kube.Get(context.TODO(), nn, &mr)
+	xr := svcapitypes.ProvisionedProduct{}
+	err := c.kube.Get(ctx, nn, &xr)
 	if err != nil {
 		return false, err
 	}
-	if len(mr.Status.AtProvider.LastProvisioningParameters) != len(ds.Spec.ForProvider.ProvisioningParameters) {
+	// Product should be updated if amount of provisioning params from desired stats lesser than the amount from previous reconciliation loop
+	if len(xr.Status.AtProvider.LastProvisioningParameters) > len(ds.Spec.ForProvider.ProvisioningParameters) {
 		return true, nil
 	}
 
 	cfStackKeyValue := make(map[string]string)
 	for _, v := range cfStackParams {
-		cfStackKeyValue[*v.ParameterKey] = pointer.StringDeref(v.ParameterValue, "")
+		if v.ParameterKey != nil {
+			cfStackKeyValue[*v.ParameterKey] = pointer.StringDeref(v.ParameterValue, "")
+		}
 	}
 
 	for _, v := range ds.Spec.ForProvider.ProvisioningParameters {
 		// In this statement/comparison, the provider ignores spaces from the left and right of the parameter value from
 		// the desired state. Because on cloudformation side spaces are also trimmed
 		if cfv, ok := cfStackKeyValue[*v.Key]; ok && strings.TrimSpace(pointer.StringDeref(v.Value, "")) == cfv {
 			continue
+		} else if !ok {
+			return false, errors.Errorf("provisioning parameter %s is not present in cloud formation stack", *v.Key)
 		} else {
 			return true, nil
 		}
@@ -311,7 +312,7 @@ func (c *custom) provisioningParamsAreChanged(cfStackParams []cfsdkv2types.Param
 	return false, nil
 }
 
-func (c *custom) productOrArtifactAreChanged(ds *svcapitypes.ProvisionedProductParameters, resp *svcsdk.ProvisionedProductDetail) (bool, error) {
+func (c *custom) productOrArtifactIsChanged(ds *svcapitypes.ProvisionedProductParameters, resp *svcsdk.ProvisionedProductDetail) (bool, error) {
 	// ProvisioningArtifactID and ProvisioningArtifactName are mutual exclusive params, the same about ProductID and ProductName
 	// But if describe a provisioned product aws api will return only IDs, so it's impossible to compare names with ids
 	// Conditional statement below works only if desired state includes ProvisioningArtifactID and ProductID
@@ -343,7 +344,7 @@ func (c *custom) getArtifactID(ds *svcapitypes.ProvisionedProductParameters) (st
 		Id:   ds.ProductID,
 		Name: ds.ProductName,
 	}
-	// DescribeProvisioningArtifact methods fits much better, but it has a bug
+	// DescribeProvisioningArtifact method fits much better, but it has a bug
 	output, err := c.client.DescribeProduct(&input)
 	if err != nil {
 		return "", errors.Wrap(err, errCouldNotLookupProduct)
@@ -352,7 +353,7 @@ func (c *custom) getArtifactID(ds *svcapitypes.ProvisionedProductParameters) (st
 	for _, artifact := range output.ProvisioningArtifacts {
 		if pointer.StringDeref(ds.ProvisioningArtifactName, "") == *artifact.Name ||
 			pointer.StringDeref(ds.ProvisioningArtifactID, "") == *artifact.Id {
-			return pointer.StringDeref(artifact.Id, ""), nil
+			return *artifact.Id, nil
 		}
 	}
 	return "", errors.Wrap(errors.New("artifact not found"), errCouldNotLookupProduct)