fix(cognito-identitypool): providerUrl causes error when mappingKey is not provided and it is a token #21191

SamStephens · 2022-07-18T02:24:15Z

This property is for use when the identityProvider is a Token. By default identityProvider is used as the key in the role mapping
hash, but Cloudformation only allows concrete strings to be used as hash keys.

In particular this feature is a requirement to allow a previously defined CDK UserPool to be used as an identityProvider.

closes #19222

Please note that the integ test results will need updating. I attempted to run the tests, and received the error

Error: ENOENT: no such file or directory, open '/home/sam/aws-cdk/packages/aws-cdk/lib/init-templates/v1/info.json'
  ERROR      integ.identitypool 0.535s
      Command exited with status 1

I've used npm to update to the latest CDK CLI. I appear to not be the only person facing this issue; see #21056 (comment)

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

This property is for use when the identityProvider is a Token. By default identityProvider is used as the key in the role mapping hash, but Cloudformation only allows concrete strings to be used as hash keys.

gitpod-io · 2022-07-18T02:24:23Z

A couple of missing import statements

I couldn't run them locally, now I've seen them fail the CodeBuild build I can fix the results based on my changes.

SamStephens · 2022-07-18T04:38:01Z

Build failure doesn't appear to be due to anything I've done; it appears to be in @aws-cdk/aws-appsync, which I haven't touched:

@aws-cdk/aws-appsync: Verifying integration test snapshots...
@aws-cdk/aws-appsync: /codebuild/output/src316876632/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.js:377
@aws-cdk/aws-appsync:             throw Error('API key expiration must be between 1 and 365 days.');
@aws-cdk/aws-appsync:             ^
@aws-cdk/aws-appsync: Error: API key expiration must be between 1 and 365 days.
@aws-cdk/aws-appsync:     at GraphqlApi.createAPIKey (/codebuild/output/src316876632/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.js:377:19)
@aws-cdk/aws-appsync:     at new GraphqlApi (/codebuild/output/src316876632/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.js:189:40)
@aws-cdk/aws-appsync:     at Object.<anonymous> (/codebuild/output/src316876632/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/test/integ.auth-apikey.js:23:13)
@aws-cdk/aws-appsync:     at Module._compile (internal/modules/cjs/loader.js:1085:14)
@aws-cdk/aws-appsync:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
@aws-cdk/aws-appsync:     at Module.load (internal/modules/cjs/loader.js:950:32)
@aws-cdk/aws-appsync:     at Function.Module._load (internal/modules/cjs/loader.js:790:12)
@aws-cdk/aws-appsync:     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:75:12)
@aws-cdk/aws-appsync:     at internal/main/run_main_module.js:17:47
@aws-cdk/aws-appsync:   ERROR      integ.auth-apikey 0.5s
@aws-cdk/aws-appsync:       Command exited with status 1

Also, move the documentation of the new feature to a more relevant part of the README.md.

TheRealAmazonKendra

I think this is more of a fix than a feat but it's almost perfect as is. Just one suggested change inline.

TheRealAmazonKendra · 2022-07-30T01:46:40Z

packages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-role-attachment.ts

+      } else {
+        const providerUrl = prop.providerUrl.value;
+        if (Token.isUnresolved(providerUrl)) {
+          throw new Error('mappingKey must be provided when providerUrl.value is an unresolved token');


Suggested change

throw new Error('mappingKey must be provided when providerUrl.value is an unresolved token');

throw new Error('mappingKey must be provided when providerUrl.value is a token');

I'm happy to make this change; that was wording I've copied from other parts of the CDK such as

aws-cdk/packages/@aws-cdk/aws-ssm/lib/util.ts

Line 51 in 74318c7

throw new Error('Unable to determine ARN separator for SSM parameter since the parameter name is an unresolved token. Use "fromAttributes" and specify "simpleName" explicitly');

and

aws-cdk/packages/@aws-cdk/aws-iam/lib/principals.ts

Line 335 in cea1039

throw new Error(`multiple "${operator}" conditions cannot be merged if one of them contains an unresolved token`);

.

I couldn't decide if this was a fix or a feature; I fell on the side of this being a feature as it does actually provide new functionality, rather than altering existing functionality; it's just that the new feature addresses a gap that was previously present in the CDK's capabilities. Once again, I'm happy to defer to your judgment here.

Ah, I've made the change manually, rather than applying your commit suggestion; that's Github functionality I hadn't seen before.

Pull request has been modified.

@TheRealAmazonKendra

Update the wording of the error message when providerUrl.value is a token as per request of @TheRealAmazonKendra

aws-cdk-automation · 2022-07-30T04:21:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5ffb940
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-07-30T04:22:09Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…s not provided and it is a token (aws#21191) This property is for use when the identityProvider is a Token. By default identityProvider is used as the key in the role mapping hash, but Cloudformation only allows concrete strings to be used as hash keys. In particular this feature is a requirement to allow a previously defined CDK UserPool to be used as an identityProvider. closes aws#19222 Please note that the integ test results will need updating. I attempted to run the tests, and received the error ``` Error: ENOENT: no such file or directory, open '/home/sam/aws-cdk/packages/aws-cdk/lib/init-templates/v1/info.json' ERROR integ.identitypool 0.535s Command exited with status 1 ``` I've used `npm` to update to the latest CDK CLI. I appear to not be the only person facing this issue; see aws#21056 (comment) ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add mappingKey to role mapping

4ce08e4

This property is for use when the identityProvider is a Token. By default identityProvider is used as the key in the role mapping hash, but Cloudformation only allows concrete strings to be used as hash keys.

aws-cdk-automation requested a review from a team July 18, 2022 02:24

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 18, 2022

SamStephens added 2 commits July 18, 2022 14:57

Add missing dependencies

0eb4701

A couple of missing import statements

Update integ results

bbc6ae8

I couldn't run them locally, now I've seen them fail the CodeBuild build I can fix the results based on my changes.

SamStephens added 2 commits July 19, 2022 09:55

Merge branch 'main' into main

2524a24

Add missing IdentityPoolProviderUrl import to README.md

192876d

Also, move the documentation of the new feature to a more relevant part of the README.md.

TheRealAmazonKendra changed the title ~~feat(aws-cognito-identitypool): Add mappingKey to role mapping~~ fix(cognito-identitypool): providerUrl causes error when mappingKey is not provided and it is a token Jul 30, 2022

TheRealAmazonKendra previously requested changes Jul 30, 2022

View reviewed changes

Merge branch 'main' into main

68e96cc

SamStephens added 2 commits July 30, 2022 15:23

Fix providerUrl.value is token error message

49af778

Update the wording of the error message when providerUrl.value is a token as per request of @TheRealAmazonKendra

Fix test missed from previous commit

5ffb940

TheRealAmazonKendra approved these changes Jul 30, 2022

View reviewed changes

mergify bot merged commit d91c904 into aws:main Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cognito-identitypool): providerUrl causes error when mappingKey is not provided and it is a token #21191

fix(cognito-identitypool): providerUrl causes error when mappingKey is not provided and it is a token #21191

SamStephens commented Jul 18, 2022

gitpod-io bot commented Jul 18, 2022

SamStephens commented Jul 18, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jul 30, 2022

SamStephens Jul 30, 2022

SamStephens Jul 30, 2022

aws-cdk-automation commented Jul 30, 2022

mergify bot commented Jul 30, 2022

	throw new Error('mappingKey must be provided when providerUrl.value is an unresolved token');
	throw new Error('mappingKey must be provided when providerUrl.value is a token');

fix(cognito-identitypool): providerUrl causes error when mappingKey is not provided and it is a token #21191

fix(cognito-identitypool): providerUrl causes error when mappingKey is not provided and it is a token #21191

Conversation

SamStephens commented Jul 18, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 18, 2022

SamStephens commented Jul 18, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 30, 2022

Choose a reason for hiding this comment

SamStephens Jul 30, 2022

Choose a reason for hiding this comment

SamStephens Jul 30, 2022

Choose a reason for hiding this comment

aws-cdk-automation commented Jul 30, 2022

AWS CodeBuild CI Report

mergify bot commented Jul 30, 2022