Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CognitoIDP[UserPoolClient]: Avoid underlying provider validation failure #762

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

ytsarev
Copy link
Collaborator

@ytsarev ytsarev commented Jun 29, 2023

Description of your changes

  • In a current state the instantiation of UserPoolClient fails with

    cannot run refresh: refresh failed: reading Amazon Cognito IDP (Identity Provider) User Pool Client (): InvalidParameter: 1 
    validation error(s) found.
    - minimum field size of 1, DescribeUserPoolClientInput.ClientId.
    

    unless we explicitly set the external-name annotation. See provider-aws-cognitoidp:v0.36.0 User Pool Client Validation Error #752 for more debugging data

  • This change will preset the id with spec.forProvider.name to avoid the validation failure. The id will be populated eventually with the dynamic value from the cloud provider

  • I also tried to follow the doc at https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cognito_user_pool_client#import but it seems to be wrong in practice:

    If we use smth like

    TemplatedStringAsIdentifierWithNoName("{{ .parameters.user_pool_id }}/{{ .parameters.name }}"),
    

    it will fail with

    cannot run refresh: refresh failed: reading Amazon Cognito IDP (Identity Provider) User Pool Client (us-west-1_dMnICLKVB/): InvalidParameterException: 1 validation error detected: Value 'us-west-1_dMnICLKVB/' at 'clientId' failed to satisfy constraint: Member must satisfy regular expression pattern: [\w+]+
    

    It proves that the import doc deviates from reality and fails its own provider validation.

  • It is possible that we observe the bug in the underlying terraform aws provider.

  • This fix provides best possible solution without the fix of underlying provider and associated costs.

  • Fixes provider-aws-cognitoidp:v0.36.0 User Pool Client Validation Error #752

I have:

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

How has this code been tested

e2e with example from examples/cognitoidp/userpooluicustomization.yaml

I will also execute uptest separately below

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 29, 2023

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

@jeanduplessis
Copy link
Collaborator

@ytsarev please run make generate locally and push any diffs up in this PR

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 29, 2023

@jeanduplessis Thanks for the catch! I actually didn't expect that and don't like what the generator did, investigating further.

* In a current state the instantiation of `UserPoolClient` fails with
```
cannot run refresh: refresh failed: reading Amazon Cognito IDP (Identity Provider) User Pool Client (): InvalidParameter: 1 validation error(s) found.
- minimum field size of 1, DescribeUserPoolClientInput.ClientId.
```
unless we explicitly set the `external-name` annotation. See crossplane-contrib#752 for more debugging data

* This change will preset the `id` with `spec.forProvider.name` to avoid
the validation failure. The `id` will be populated eventually with the
dynamic value from the cloud provider

* I also tried to follow the doc at https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cognito_user_pool_client#import
but it seems to be wrong in practice:

If we use smth like

```
TemplatedStringAsIdentifierWithNoName("{{ .parameters.user_pool_id }}/{{ .parameters.name }}"),
```

it will fail with
```
cannot run refresh: refresh failed: reading Amazon Cognito IDP (Identity Provider) User Pool Client (us-west-1_dMnICLKVB/): InvalidParameterException: 1 validation error detected: Value 'us-west-1_dMnICLKVB/' at 'clientId' failed to satisfy constraint: Member must satisfy regular expression pattern: [\w+]+
```

It proves that the import doc deviates from reality and fails its own
provider validation.

* It is possible that we observe the bug in the underlying terraform aws
provider.

* This fix provides best possible solution without the fix of underlying
provider and associated costs.

* Fixes crossplane-contrib#752

Signed-off-by: Yury Tsarev <yury@upbound.io>
@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 29, 2023

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

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 29, 2023

@jeanduplessis updated the solution with the function that does not change CRD structure/name field requirement.

@jeanduplessis jeanduplessis merged commit 59cf425 into crossplane-contrib:main Jun 29, 2023
@thekaleidoscope
Copy link
Contributor

thekaleidoscope commented Aug 9, 2023

There is an issue with this approach too I believe, the name doesn't have a regex restriction but the ClientId can only be [\w+]+

Hence, if there is name like org-name, it also errors out with

observe failed: cannot run refresh: refresh failed: reading Amazon
        Cognito IDP (Identity Provider) User Pool Client (org-name):
        InvalidParameterException: 1 validation error detected: Value
        'org-name` at 'clientId' failed to satisfy constraint: Member must
        satisfy regular expression pattern: [\w+]+

I'd suggest we have a replace operation regexReplaceAll "[^A-Za-z0-9]" .name "" to remove all non word characters in name, lmk if its sounds good, i'd be happy to raise it as a PR,

Edit: on second thought this won't be enough too, there is validation preventing capital names for internal host

a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, ‘-’ or ‘.’, and must start and end with an alphanumeric character (e.g. ‘example.com’, regex used for validation is ‘a-z0-9?(.a-z0-9?)

I think best would be to create MD5 of the name as its of regex [0-9a-f]{32} which satisfies both the requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provider-aws-cognitoidp:v0.36.0 User Pool Client Validation Error
3 participants