feat(servicecatalog): Add provisioned product

[teeverr]

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

It is a new PR, instead of this one

[MisterMX]

LGTM. @teeverr can you rebase it on the latest master, fix the conflicts and squash your changes?

[teeverr]

LGTM. @teeverr can you rebase it on the latest master, fix the conflicts and squash your changes?

@MisterMX Done, I rebased it on the latest master and squashed my commits.

[teeverr]

@MisterMX a final build, which included changes from the master, didn't pass tests.
Only after reverting of this commit everything where stabilized

Context:

• ProvisionedProduct has mutual exclusive params: productID and productName, provisioningArtifactID and provisioningArtifactName
• ProvisionedProduct uses ack-generated create/delete/observe/update funcs

Expected behaviour: ProvisionedProduct custom resource has absolutely the same set of spec.forProvider parameters that set in the manifest which was applied, plus lately initialized params in this case it is only one - acceptLanguage
But if apply a manifest:

---
apiVersion: servicecatalog.aws.crossplane.io/v1alpha1
kind: ProvisionedProduct
metadata:
  name: awesome-s3-bucket
spec:
  forProvider:
    region: us-east-1
    productName: s3
    provisioningArtifactName: v1.0.0
    provisioningParameters:
      - key: BucketName
        value: "awesome-s3-bucket"
      - key: BucketVersioning
        value: "enabled"
      - key: TransferAcceleration
        value: "suspended"
      - key: EncryptionType
        value: "SSE-KMS"
      - key: KMSArn
        value: "none"
  providerConfigRef:
    name: provider-aws

The custom resource will look liks this:

apiVersion: servicecatalog.aws.crossplane.io/v1alpha1
kind: ProvisionedProduct
...
spec:
  forProvider:
    acceptLanguage: en # it is okay, added by late init
    pathID: lpv3-aerpfedhecscw # it is not okay, this param is not specified in the manifest
    productID: prod-bz6oapwxnu67u # it is not okay, this param is not specified in the manifest
    productName: s3
    provisioningArtifactID: pa-7lzz6kt44qevo # it is not okay, this param is not specified in the manifest
    provisioningArtifactName: v1.0.0
    provisioningParameters:
      - key: BucketName
        value: "awesome-s3-bucket"
      - key: BucketVersioning
        value: "enabled"
      - key: TransferAcceleration
        value: "suspended"
      - key: EncryptionType
        value: "SSE-KMS"
      - key: KMSArn
        value: "none"
    region: us-east-1
  providerConfigRef:
    name: provider-aws

It happens only if the controller creates a new resource, if it takes control over an existing resource(by external name) everything will be ok. And it started after crossplane-runtime had been updated to 1.14.1.
So it looks like Create func of controller enriches current/desired state by all params from resp, as result custom resource has mutual exclusive params and it breaks isUpTodate func.

Probably this crossplane-runtime update also affected other resources in the provider

[MisterMX]

@teeverr the effects you see have nothing to do with the crossplane-runtime as it does not modify your spec. The fields are modified in the Create method which is generated by ACK: https://github.com/crossplane-contrib/provider-aws/pull/1908/files#diff-e76b8787ede5bd7470b087cadc20b14a091d05b51bc9d2a02b4bd7a8d9dc7a65R140

[teeverr]

@MisterMX I didn't say crossplay-runtime modifies spec, but probably it indirectly affects Create method which uses this package. I didn't research this problem deeply, but I found that the same zz_controller.go(which inlcludes Create method) has different behaviour with packages from commit and without. Which is not okay, I assume.
Probably another package from the commit above affects custom resource specs. Maybe github.com/crossplane/crossplane-tools?

[MisterMX]

https://github.com/crossplane/crossplane-runtime only implements the underlying, fundemental controller logic. The way managed resources are reconciled hasn't changed afaik.

https://github.com/crossplane/crossplane-tools is a code generator (angryjet) that generates resolver functions that are necessary to satisfy the resource.Managed interface. But it does not cover external API specific fields that inside spec.forProvider.

As I said, the fields you mentioned are set in the ACK generated Create in zz_controller.go (https://github.com/crossplane-contrib/provider-aws/pull/1908/files#diff-e76b8787ede5bd7470b087cadc20b14a091d05b51bc9d2a02b4bd7a8d9dc7a65R156). This also explains why the behaviour only occurs for new resources and not during imports of existing once (fields are not overwritten in Update).

To solve this I guess it should suffice to ignore the respective fields in the generator-config from AWS response. Something like:

ignore:
  field_paths:
    - ProvisionedProductOutput.RecordDetail.ProvisioningArtifactId

[teeverr]

@MisterMX yes, ignore.filed_paths helped, custom resource doesn't have these fields anymore if they're not explicitly set. Thanks!
But in the same time I had to delete

  fields:
      LastPathID:
        is_read_only: true
        from:
          operation: DescribeRecord
          path: RecordDetail.PathId

from generator-config, because it used the same type, it's not critical but nice to know what pathId your resource used. Looks like there is only one option to return it back - write custom Create method

It's really weird, because Create had if statements like this before the commit

       if resp.RecordDetail.PathId != nil {
               cr.Spec.ForProvider.LastPathID = resp.RecordDetail.PathId
       } else {
               cr.Spec.ForProvider.LastPathID = nil
       }

but fields in spec.forProvider wheren't filled implicitly. So as I already texted, probably it affected, something else.

Anyway, it looks like service catalog is good for now. Thanks for your help : ) I will squash a commit history tomorrow, when tests be successfully passed 

[teeverr]

@MisterMX
Done, commits are squashed

[MisterMX]

@teeverr there is some diff generated because of #1924 and #1943. Can you rebase on the latest master?

[teeverr]

@MisterMX done!

[MisterMX]

LGTM. Thank you very much for all the hard work @teeverr!