feat(servicecatalog): Add provisioned product #1806

teeverr · 2023-07-14T16:53:22Z

Description of your changes

Adds new resource - ProvisionedProduct, which represents life cycle of launched product in service catalog

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Many scenarios have been tested manually In development env and also I wrote some unit tests for custom controller logic

mleahu · 2023-07-17T08:55:01Z

Hi @MisterMX!

Can you please check this PR and give us your first impression?

Thanks a lot!

MisterMX

Thank you for your contribution @teeverr. I added some remarks to the code. Can you take a look at them? Thanks!

pkg/controller/servicecatalog/provisionedproduct/setup.go

MisterMX · 2023-07-24T09:34:53Z

pkg/controller/servicecatalog/provisionedproduct/setup.go

+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && *describeRecordOutput.RecordDetail.RecordType == "UPDATE_PROVISIONED_PRODUCT":
+		cr.SetConditions(xpv1.Available().WithMessage("Provisioned Product is updating, availability depends on product"))
+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_ERROR):
+		cr.Status.SetConditions(xpv1.ReconcileError(errors.New(errProvisionedProductStatusSdkError)))


This is not a reconcile error. It only indicates a misconfiguration of the external resource. Setting ReconcileError would send the wrong message to the user (and might be overwritten by the runtime anyways). The Unavailable status condition is should be used here:

Suggested change

cr.Status.SetConditions(xpv1.ReconcileError(errors.New(errProvisionedProductStatusSdkError)))

cr.Status.SetConditions(xpv1.Unvailable().WithMessage(errProvisionedProductStatusSdkError))

It is true if provisioned product has Tainted state, but if it has Error update is not allowed, so crossplane will never reconcile that resource

If you want to prevent an update you can look up the state during Update and do nothing if it is not allowed - or you return ResourceIsUpdToDate: true in Observe. Either way, setting the ReconcileError condition here is still not correct and you should use Unvailable.

pkg/controller/servicecatalog/provisionedproduct/setup.go

teeverr · 2023-08-02T10:11:55Z

Thanks for review @MisterMX! I hope I fulfilled all of your remarks : )
p.s. The commit history was accidentally squashed in a wrong way, it makes reading history a little harder.

MisterMX · 2023-08-30T11:41:45Z

pkg/controller/servicecatalog/provisionedproduct/setup.go

+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && *describeRecordOutput.RecordDetail.RecordType == "UPDATE_PROVISIONED_PRODUCT":
+		cr.SetConditions(xpv1.Available().WithMessage("Provisioned Product is updating, availability depends on product"))
+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_ERROR):
+		cr.Status.SetConditions(xpv1.ReconcileError(errors.New(errProvisionedProductStatusSdkError)))


If you want to prevent an update you can look up the state during Update and do nothing if it is not allowed - or you return ResourceIsUpdToDate: true in Observe. Either way, setting the ReconcileError condition here is still not correct and you should use Unvailable.

MisterMX · 2023-08-30T11:43:18Z

pkg/controller/servicecatalog/provisionedproduct/setup.go

+	if err != nil {
+		return "", err
+	}
+	return *output.ProvisioningArtifactDetail.Id, nil


You might run into a panic when dereferencing a nil value. Better to check if it is nil before returning it.

MisterMX · 2023-08-30T11:43:54Z

pkg/controller/servicecatalog/provisionedproduct/setup.go

+
+	cfStackKeyValue := make(map[string]string)
+	for _, v := range cfStackParams {
+		cfStackKeyValue[*v.ParameterKey] = *v.ParameterValue


Possible nil deref. Better use pointer.StringDeref.

MisterMX · 2023-08-30T11:48:43Z

Thanks for the changes @teeverr. There are some minor issues that need to be solved before merging. Can you take a look at them?

Also, can you squash your changes into a single commit and sign it?

threadproc · 2023-09-05T07:52:47Z

Thanks for the review! I work on the team handling this on Swisscom's side, and we should have an updated PR ready soon that incorporates this feedback, and fixes several bugs and issues we've discovered so far.

threadproc · 2023-09-05T18:37:08Z

@MisterMX I've pushed a new version of this work, which addresses the feedback you provided as well as resolving the following discovered issues:

productName does not work past the first provision
updates performed when a provisioned product is in the UNDER_CHANGE state fail with an obscure error message (replaced this with a handled error and better message)
products that are UNDER_CHANGE do not reconcile with much frequency, so they are not available until they are polled next - we treat this like a ReconcileError now, since we expect them to become AVAILABLE and they should be re-reconciled until that is the case
provisioned products are not deleted when they are in ERROR or TAINTED status

As well, I did some refactoring to try to clean things up a bit and keep it organized.

…ervice Catalog Signed-off-by: Kai Parry <[email protected]>

threadproc · 2023-09-06T10:07:09Z

Fixed the linter errors, apologies.

teeverr

Thanks for your changes @threadproc
I left some comments here, the summary(and concern) of which is to understand that all of these 'refactoring' are really necessary and improve the PR and these are not changes for changes.

Because it would be more than enough to add 10-20 simple lines of code to fix 3-4 bugs.

Ps. It would be really nice to create new PR to this branch before. Because after squashing is happend it became hardly understandable, but I restored the original branch from my local backup. So I used this link in my comments/remarks for comparison and clarification

teeverr · 2023-09-06T13:23:30Z

pkg/controller/servicecatalog/provisionedproduct/setup.go

+			reconcilerOpts...))
+}
+
+func (c *customConnector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) {


What is the advantage of this func in comparison of prepareSetupExternal from the original branch
https://github.com/swisscom/provider-aws/pull/1/conflicts

teeverr · 2023-09-06T13:29:07Z

pkg/controller/servicecatalog/provisionedproduct/lifecycle.go

@@ -0,0 +1,194 @@
+package provisionedproduct


@threadproc
Before squashing this branch had the same stucture for provisioned product which already used by others 100+ resources in this provider. Are sure it's necessary to have this "lifecycle.go"?

teeverr · 2023-09-06T13:29:38Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

@@ -0,0 +1,101 @@
+package provisionedproduct


@threadproc
Original PR had the same stucture for provisioned product which already used by others 100+ resources in this provider. Are sure it's necessary to use this "utils.go"?

teeverr · 2023-09-06T13:34:11Z

pkg/controller/servicecatalog/provisionedproduct/lifecycle.go

+	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())


But this condition already set in zz_controller.go, it is necessary here?

teeverr · 2023-09-06T13:34:31Z

pkg/controller/servicecatalog/provisionedproduct/lifecycle.go

+		cr.SetConditions(xpv1.Creating())
+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && recordType == "UPDATE_PROVISIONED_PRODUCT":
+		cr.SetConditions(xpv1.Unavailable().WithMessage(msgProvisionedProductStatusSdkUnderChange))
+	case ppStatus == string(svcapitypes.ProvisionedProductStatus_SDK_UNDER_CHANGE) && recordType == "TERMINATE_PROVISIONED_PRODUCT":


But this condition already set in zz_controller.go, it is necessary here?

teeverr · 2023-09-06T14:12:28Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+func (c *custom) productOrArtifactAreChanged(ds *svcapitypes.ProvisionedProductParameters, resp *svcsdk.ProvisionedProductDetail) (bool, error) { // nolint:gocyclo
+	var productID, artifactID string
+
+	if ds.ProductID != nil {


this if is redundant

teeverr · 2023-09-06T14:12:33Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+	if ds.ProductID != nil {
+		productID = pointer.StringDeref(ds.ProductID, "")
+	}
+	if ds.ProvisioningArtifactID != nil {


this if is redundant

teeverr · 2023-09-06T14:35:50Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+		}
+	}
+
+	return pointer.StringDeref(resp.ProductId, "") != productID ||


These fields can't be nil, so deref is redundant. But as I understand @MisterMX recommends to deref any pointers, idk
Another question about this func in general... as I wrote above.

teeverr · 2023-09-06T14:36:11Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+			productID = pointer.StringDeref(describeProduct.ProductViewSummary.ProductId, "")
+		}
+
+		if artifactID == "" {


teeverr · 2023-09-06T14:39:45Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+}
+
+// productOrArtifactAreChanged will attempt to determine whether or not the currently requested SC product and provisioning artifact IDs have changed when using names instead of IDs
+func (c *custom) productOrArtifactAreChanged(ds *svcapitypes.ProvisionedProductParameters, resp *svcsdk.ProvisionedProductDetail) (bool, error) { // nolint:gocyclo


// nolint:gocyclo I think the idea of this rule is to keep code clean and protect it from some things... : )

teeverr · 2023-09-06T19:18:10Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+
+		if artifactID == "" {
+			// find the matching artifact
+			for _, artifact := range describeProduct.ProvisioningArtifacts {


O(n) here, but O(1) in original branch with another method(DescribeProvisioningArtifact). Why?

teeverr · 2023-09-06T19:18:15Z

pkg/controller/servicecatalog/provisionedproduct/utils.go

+		// API call.
+		dpInput := &svcsdk.DescribeProductInput{}
+
+		if productID != "" {


These two field are mutualy exclusive, it means this if is redundant again and even more it follows to bugs or unpected behavior, cuz if user set both he/she don't get relevant error, cuz you use productid every time.
And again the question arises what was wrong with the original code/func?

mleahu · 2023-09-07T07:48:51Z

Hi @MisterMX

Thanks a lot for your reviews and feedbacks!

We still run internal tests with this service catalog feature of provider-aws and we might still hit some bugs.

I propose to keep this PR open/draft until we're done with the testing phase. I'll keep you updated!

Thank you again! Wish a nice day!

Marius

teeverr · 2023-10-13T13:57:58Z

This PR has been replaced by this - #1908

teeverr force-pushed the feature/servicecatalog-provisioned-product-resource branch 2 times, most recently from 7f410e4 to 2577615 Compare July 14, 2023 20:17

teeverr force-pushed the feature/servicecatalog-provisioned-product-resource branch from 3945a36 to 9b399a3 Compare July 18, 2023 10:55

MisterMX requested changes Jul 24, 2023

View reviewed changes

MisterMX changed the title ~~add service catalog - provisioned product~~ feat(servicecatalog): Add provisioned product Jul 26, 2023

teeverr force-pushed the feature/servicecatalog-provisioned-product-resource branch 2 times, most recently from 413df22 to 0919706 Compare August 2, 2023 09:29

teeverr requested a review from MisterMX August 2, 2023 10:13

teeverr force-pushed the feature/servicecatalog-provisioned-product-resource branch from 0919706 to 0bd9555 Compare August 2, 2023 10:39

teeverr force-pushed the feature/servicecatalog-provisioned-product-resource branch 3 times, most recently from 34c9607 to e0cd859 Compare August 25, 2023 13:28

MisterMX requested changes Aug 30, 2023

View reviewed changes

threadproc force-pushed the feature/servicecatalog-provisioned-product-resource branch from c70dd56 to 3be0213 Compare September 5, 2023 18:32

feat(servicecatalog): add support for provisioned products from AWS S…

4482809

…ervice Catalog Signed-off-by: Kai Parry <[email protected]>

threadproc force-pushed the feature/servicecatalog-provisioned-product-resource branch from 3be0213 to 4482809 Compare September 6, 2023 10:06

teeverr commented Sep 6, 2023

View reviewed changes

teeverr mentioned this pull request Oct 13, 2023

feat(servicecatalog): Add provisioned product #1908

Merged

2 tasks

teeverr closed this Oct 13, 2023

teeverr deleted the feature/servicecatalog-provisioned-product-resource branch October 3, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servicecatalog): Add provisioned product #1806

feat(servicecatalog): Add provisioned product #1806

teeverr commented Jul 14, 2023 •

edited

Loading

mleahu commented Jul 17, 2023

MisterMX left a comment

MisterMX Jul 24, 2023

teeverr Aug 2, 2023

MisterMX Aug 30, 2023

teeverr commented Aug 2, 2023 •

edited

Loading

MisterMX Aug 30, 2023

MisterMX Aug 30, 2023

MisterMX Aug 30, 2023

MisterMX commented Aug 30, 2023

threadproc commented Sep 5, 2023

threadproc commented Sep 5, 2023

threadproc commented Sep 6, 2023

teeverr left a comment •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023

teeverr Sep 6, 2023

teeverr Sep 6, 2023

teeverr Sep 6, 2023

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023

teeverr Sep 6, 2023

teeverr Sep 6, 2023

teeverr Sep 6, 2023 •

edited

Loading

mleahu commented Sep 7, 2023

teeverr commented Oct 13, 2023

	cr.Status.SetConditions(xpv1.ReconcileError(errors.New(errProvisionedProductStatusSdkError)))
	cr.Status.SetConditions(xpv1.Unvailable().WithMessage(errProvisionedProductStatusSdkError))

feat(servicecatalog): Add provisioned product #1806

feat(servicecatalog): Add provisioned product #1806

Conversation

teeverr commented Jul 14, 2023 • edited Loading

Description of your changes

How has this code been tested

mleahu commented Jul 17, 2023

MisterMX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teeverr commented Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisterMX commented Aug 30, 2023

threadproc commented Sep 5, 2023

threadproc commented Sep 5, 2023

threadproc commented Sep 6, 2023

teeverr left a comment • edited Loading

Choose a reason for hiding this comment

teeverr Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

teeverr Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

teeverr Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teeverr Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teeverr Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

mleahu commented Sep 7, 2023

teeverr commented Oct 13, 2023

teeverr commented Jul 14, 2023 •

edited

Loading

teeverr commented Aug 2, 2023 •

edited

Loading

teeverr left a comment •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading

teeverr Sep 6, 2023 •

edited

Loading