(bugfix) Fix cognito user pool client orphaned resources

[mbbush]

Description of your changes

The cognito idp user pool client is one of the handful of resources in terraform-provider-aws v4.67.0 that use the terraform plugin framework, and therefore that we still reconcile by forking a CLI terraform process.

There are three problems with the cognito user pool client:

1. Creation fails if the spec.forProvider.name doesn't match the regex for the clientId
2. Whenever the provider pod restarts, crossplane will either create a duplicate resource, orphaning the existing one, or get into a Synced: False state, depending on whether spec.forProvider.name matches the clientId regex.
3. Observe-only resources fail, because terraform import expects an argument that is not the same as the terraform id.

This PR fixes the first two issues, and makes the third issue no worse.

Context

This resource has two idiosyncrasies that make it difficult.

1. Calling terraform apply -refresh-only with an id of "" returns an error, because the terraform provider passes through the empty-string id to the aws sdk, which (correctly) complains that trying to find a user pool client with id "" is invalid. I suspect this is because the resource uses the terraform plugin framework, which introduced the concept of a null value as distinct from a zero value. I suspect that if we had some way to specify a null value for the id, it would work fine, but the type in upjet is string not *string so I don't see how to do that without major changes. Maybe the upjet developers will need to add a transformation layer from zero values back to null when integrating with the terraform provider framework, or perhaps they'll come up with a better idea.

2. The argument needed to terraform import the resource is different than the terraform id, but the terraform id is needed to run terraform apply -refresh-only. The provider uses terraform import for resources with management policy Observe and terraform apply -refresh-only for other management policies. Unless someone has a clever way to identify which case we're in, in the context of an external name configuration, we can only support one of these use cases.

#762 was an attempt to work around the "id":"" issue, but it introduced new problems.

My solution for the first issue is to simply use a static client id of "invalidnonemptystring", which satisfies the client id regex [\w+]+, so doesn't cause an error when we run terraform apply -refresh-only, but also doesn't match any actual existing client id (the aws-generated ids that I've seen are all 26 lowercase alphanumeric characters long). This feels like getting the right result by doing the wrong thing, but I can't think of any actual problems it would cause, nor can I come up with a better idea.

For the second issue, I've chosen to write an external name config that supports assuming full control of existing resources, and does not support observing existing resources only, as that seems like the lesser of two evils (especially since it's necessary for recovering from a provider pod crash restart). It also has the advantage of keeping the terraform id set to the value of the terraform id (except when we don't have an id, so set it to "invalidnonemptystring" to get the necessary NotFound response).

Ultimately I hope that when we reconcile this through the terraform plugin framework, we'll be able to ignore the format expected by terraform import and just use the id and user pool id parameters. It looks like that's exactly the implementation used in the Read method on the terraform provider.

Fixes #828
Fixes #807
Fixes #1049

I have:

• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Uptest passes. It did not pass on main with the previous config, because the import step resulted in creating a second user pool client.

Creating an observe-only resource fails. This is not a regression, as it fails in the same way with the current version of the provider, and I don't believe it is fixable without breaking other, more important functionality.

The external name format is remaining the same: the alphanumeric client id.

When I ran uptest locally for the existing examples/cognitoidp/userpooluicustomization.yaml, I got an error about the domain already being registered, so I updated the example to use a random domain name and now it passes consistently.

[mbbush]

/test-examples="examples/cognitoidp/userpoolclient.yaml"

[mbbush]

/test-examples="examples/cognitoidp/userpooluicustomization.yaml"

[mbbush]

Before I rebased and included #1046, setting this was a non-breaking change, since reference fields and identifier fields were both omitted from spec.initProvider. Now, if I set it here, then upjet removes userPoolId, userPoolIdRef and userPoolIdSelector from spec.initProvider.

For this resource, not having those fields in initProvider seems totally fine (if they change, terraform will require destroying and recreating the resource anyways), and it is necessary to know the user pool id in some form in order to read the state of the resource from AWS, so it seems to conceptually fit as an identifier field.

This is a bugfix, so I think the best compromise is to leave it commented out, with a TODO to add it back when we do a major release.

[mbbush]

/test-examples="examples/cognitoidp/userpoolclient-with-dashes.yaml, examples/cognitoidp/userpoolclient-observe.yaml"

[mbbush]

/test-examples="examples/cognitoidp/userpoolclient.yaml, examples/cognitoidp/userpoolclient-with-dashes.yaml"

[mbbush]

@ulucinar @turkenf @sergenyalcin I'd really like to get this merged and (ideally) released promptly. This resource is basically unusable without this fix.

[turkenf]

/test-examples="examples/cognitoidp/userpoolclient.yaml"

[turkenf]

/test-examples="examples/cognitoidp/userpoolclient-with-dashes.yaml"

[turkenf]

Thank you for this PR @mbbush, I left a few comments to you for consider.

[turkenf]

I think there is no need to add another example here that only contains dashes in the name, instead we can give the name in the main example(examples/cognitoidp/userpoolclient.yaml) with dashes and remove this example, what do you think?

[turkenf]

Could you please remove this separator?

[turkenf]

Could you please add the example-id?

Suggested change

	metadata:
	metadata:
	annotations:
	meta.upbound.io/example-id: cognitoidp/v1beta1/userpoolclient

[turkenf]

Suggested change

	metadata:
	metadata:
	annotations:
	meta.upbound.io/example-id: cognitoidp/v1beta1/userpoolclient

[turkenf]

Thank you for your work on this PR @mbbush, the improvements noted in the comments above are not prohibitive in moving this PR forward and can be fixed later.

[ulucinar]

Thank you @mbbush, lgtm.

[github-actions]

Backport failed for release-0.46, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-0.46
git worktree add -d .worktree/backport-1021-to-release-0.46 origin/release-0.46
cd .worktree/backport-1021-to-release-0.46
git checkout -b backport-1021-to-release-0.46
ancref=$(git merge-base 8f8e547a8fbba3cac6e3b1ffe137478818b4b06e dc6a1aa2ccc11eccddef0ce18e21cc6b1b875c87)
git cherry-pick -x $ancref..dc6a1aa2ccc11eccddef0ce18e21cc6b1b875c87

[github-actions]

Successfully created backport PR #1059 for release-0.47.