feat: replace oauth and userpoolclient lambdas with cfn and sdk calls #12935

sobolk · 2023-07-11T23:06:55Z

Description of changes

This PR:

Incorporates changes from https://github.com/aws-amplify/amplify-cli/commits/feat/replace-lambda-callouts that replace oauth and userpoolclient custom CFN resource lambdas
Makes changes to make existing e2e pass and adds couple of them

Issue #, if available

Description of how you validated changes

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)
New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
Pull request labels are added

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

…batch-1

codecov-commenter · 2023-07-12T00:57:38Z

Codecov Report

Merging #12935 (95f0f8b) into dev (ec9a2ba) will increase coverage by 48.48%.
The diff coverage is 65.03%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##              dev   #12935       +/-   ##
===========================================
+ Coverage    0.00%   48.48%   +48.48%     
===========================================
  Files        1296      846      -450     
  Lines      149743    38161   -111582     
  Branches     1296     7793     +6497     
===========================================
+ Hits            0    18501    +18501     
+ Misses     148447    18066   -130381     
- Partials     1296     1594      +298

Impacted Files	Coverage Δ
...rmation/auth-stack-builder/auth-stack-transform.ts	`89.22% <ø> (+89.22%)`	⬆️
.../src/provider-utils/awscloudformation/constants.ts	`100.00% <ø> (+100.00%)`	⬆️
...e-walkthrough-types/awsCognito-user-input-types.ts	`100.00% <ø> (+100.00%)`	⬆️
...vice-walkthrough-types/cognito-user-input-types.ts	`100.00% <ø> (+100.00%)`	⬆️
...y-category-function/src/commands/function/build.ts	`0.00% <0.00%> (ø)`
...ify-category-function/src/events/prePushHandler.ts	`33.33% <0.00%> (+33.33%)`	⬆️
...ider-utils/awscloudformation/utils/layerHelpers.ts	`21.80% <0.00%> (+21.80%)`	⬆️
...er-utils/awscloudformation/utils/storeResources.ts	`30.38% <ø> (+30.38%)`	⬆️
...ib/S3AndCloudFront/helpers/configure-CloudFront.js	`87.06% <ø> (+87.06%)`	⬆️
...lify-category-hosting/lib/S3AndCloudFront/index.js	`89.65% <ø> (+89.65%)`	⬆️
... and 57 more

... and 1240 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Amplifiyer · 2023-07-12T20:45:48Z

codebuild_specs/wait_for_ids.json

  "l_api_2b_api_6c_api_9a",
-  "l_api_5_apigw_api_lambda_auth_1",
-  "l_api_8_function_8_schema_iterative_update_locking",
-  "l_api_9b_custom_policies_container_function_9b",
-  "l_api_connection_migration2_api_4_containers_api_secrets",
+  "l_api_3_api_6b_api_1",
+  "l_api_4_containers_api_secrets_storage_4",


Are the changes in this file intended?

this is result of yarn split-e2e-tests-codebuild which we're supposed to commit (and other cb changes too).

...y-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.ts

Amplifiyer · 2023-07-12T22:07:31Z

packages/amplify-e2e-core/src/categories/storage.ts

@@ -610,9 +610,9 @@ export function updateS3AddTriggerNewFunctionWithFunctionExisting(cwd: string, s
  });
 }

-export function addS3StorageWithIdpAuth(projectDir: string): Promise<void> {
+export function addS3StorageWithIdpAuth(projectDir: string, testingWithLatestCodebase = false): Promise<void> {


Shouldn't the default be true?

nah. The patter is "false", see https://github.com/search?q=repo%3Aaws-amplify%2Famplify-cli%20testingWithLatestCodebase&type=code . and then we rely on cci/codebuild to set this up properly ...

Amplifiyer · 2023-07-12T22:14:24Z

packages/amplify-e2e-tests/src/__tests__/auth/hosted-ui.test.ts

+};
+
+const oauthSettings = {
+  signinUrl: 'https://danielle.lol/',


…ation/auth-stack-builder/auth-cognito-stack-builder.ts Co-authored-by: Amplifiyer <[email protected]>

...mplify-migration-tests/src/__tests__/migration_tests_v12/auth-oauth-lambda-migration.test.ts

danielleadams

Looks good. I think there's 1 change we need to make, and then I will say it looks like there's future tests that were added, but I think that's okay because if the tests pass now, we can just TDD the future code.

danielleadams · 2023-07-13T17:03:01Z

codebuild_specs/e2e_workflow_generated.yml

@@ -2158,7 +2166,7 @@ batch:
        variables:
          compute-type: BUILD_GENERAL1_SMALL
          TEST_SUITE: src/__tests__/migration_tests_v10/custom-stack.migration.test.ts
-          CLI_REGION: us-west-2
+          CLI_REGION: us-east-2


Do we need all these changes to the regions?

This was outcome from yarn split-e2e-tests-codebuild. We're supposed to run in and commit outcome if test suite changes.

danielleadams · 2023-07-13T17:05:52Z

...r-utils/awscloudformation/auth-stack-builder/__snapshots__/auth-stack-transform.test.ts.snap

@@ -1987,19 +1614,44 @@ exports.handler = (event, context, callback) => {
    },
    "UserPoolClient": {
      "DependsOn": [
+        "HostedUIProvidersCustomResourceInputs",


If this is here, I think there's a depends on that needs to be removed from the stack transform.

danielleadams · 2023-07-13T17:08:15Z

...r-utils/awscloudformation/auth-stack-builder/__snapshots__/auth-stack-transform.test.ts.snap

@@ -2206,16 +1716,41 @@ exports.handler = (event, context, callback) => {
    },
    "UserPoolClientWeb": {
      "DependsOn": [
+        "HostedUIProvidersCustomResourceInputs",


Same comment here. I think a dependency needs to be removed.

Or is this because the client resources depended on the Oauth custom resource?

danielleadams · 2023-07-13T17:17:06Z

...y-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.ts

  userPoolClientRole?: iam.CfnRole;
-  userPoolClientLambdaPolicy?: iam.CfnPolicy;


I know this wasn't in the original code but if we are changing the method name, should we change userPoolClientRole to something like userPoolBaseRole?

We can't it's on customer facing type in overrides, we'd have to ship major version bump.
See https://github.com/aws-amplify/amplify-cli/blob/a3b139fb36bcb2c1c890403e5bd754beec5b7ff7/packages/amplify-cli-extensibility-helper/src/types/auth/types.ts#L33C11-L33C11 .

What we could do is deprecate old field and add new. But I'm not sure it's worth it.

danielleadams · 2023-07-13T17:23:35Z

...y-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.ts

-    this.oAuthCustomResourceInputs.node.addDependency(this.oAuthCustomResourceLogPolicy);
+    // this can be removed when hostedUI Custom resource is removed
+    this.userPoolClient?.node.addDependency(this.hostedUIProvidersCustomResourceInputs);
+    this.userPoolClientWeb?.node.addDependency(this.hostedUIProvidersCustomResourceInputs);


I don't think L790-L792 need to be added.

I'll give it a try. This was taken from branch as is, see https://github.com/danielleadams/amplify-cli/blob/test/updating-migration-tests/packages/amplify-category-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.ts#L839-L841

Changed in 74cf0dc .
When I re-recorded snapshots, these e2e tests, so confidence is high. running full e2e on the change.

I have to revert it. it caused failures across the board

https://app.circleci.com/pipelines/github/aws-amplify/amplify-cli/22063/workflows/8803c18b-c076-4eb1-9a7c-ce7b6901a3dd

CREATE_COMPLETE Thu Jul 13 2023 \u001b[39m\r\n\u001b[K\u001b[32m\tHostedUICustomResourceInputs Custom::LambdaCallout CREATE_COMPLETE Thu Jul 13 2023 \u001b[39m\r\n\u001b[?25h\r\n"] [214.859,"o","🛑 \u001b[31mThe following resources failed to deploy:\u001b[39m\r\n"] [214.859,"o","\u001b[31mResource Name:\u001b[39m UserPoolClient (AWS::Cognito::UserPoolClient)\r\n\u001b[31mEvent Type:\u001b[39m create\r\n\u001b[31mReason:\u001b[39m The provider Facebook does not exist for User Pool us-east-2_OVbZj6wvz. (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: f28ddb69-9b44-484e-87a8-ba40bb5c3cf6; Proxy: null)\r\n\r\n\r\n\u001b[31mResource Name:\u001b[39m UserPoolClientWeb (AWS::Cognito::UserPoolClient)\r\n\u001b[31mEvent Type:\u001b[39m create\r\n\u001b[31mReason:\u001b[39m The provider Facebook does not exist for User Pool us-east-2_OVbZj6wvz. (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: 74dfe154-1c80-4e5f-b050-e20c70633db7; Proxy: null)\r\n\r\n\r\n"] [214.86,"o","🛑 \u001b[31mResource is not in the state stackUpdateComplete\u001b[39m\r\n"] [214.86,"o","\u001b[0mName: UserPoolClient (AWS::Cognito::UserPoolClient), Event Type: create, Reason: The provider Facebook does not exist for User Pool us-east-2_OVbZj6wvz. (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: f28ddb69-9b44-484e-87a8-ba40bb5c3cf6; Proxy: null), IsCustomResource: false\u001b[0m\r\n\u001b[0m\u001b[0m\r\n\u001b[0mName: UserPoolClientWeb (AWS::Cognito::UserPoolClient), Event Type: create, Reason: The provider Facebook does not exist for User Pool us-east-2_OVbZj6wvz. (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: 74dfe154-1c80-4e5f-b050-e20c70633db7; Proxy: null), IsCustomResource: false\u001b[0m\r\n\u001b[0m\u001b[0m\r\n\r\n"] [214.86,"o","\u001b[0mLearn more at: https://docs.amplify.aws/cli/project/troubleshooting/\u001b[0m\r\n\r\nDeploymentFault: Resource is not in the state stackUpdateComplete\r\n at Object.run (/snapshot/repo/build/node_modules/@aws-amplify/amplify-provider-awscloudformation/lib/push-resources.js:362:11)\r\n at async /snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/extensions/amplify-helpers/push-resources.js:137:16\r\n at async Promise.all (index 0)\r\n at async providersPush (/snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/extensions/amplify-helpers/push-resources.js:133:5)\r\n at async AmplifyToolkit.pushResources (/snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/extensions/amplify-helpers/push-resources.js:107:13)\r\n at async Object.executeAmplifyCommand (/snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/index.js:194:9)\r\n at async executePluginModuleCommand (/snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/execution-manager.js:139:5)\r\n at async executeCommand (/snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/execution-manager.js:37:9)\r\n at async Object.run (/snapshot/repo/build/node_modules/@aws-amplify/cli-internal/lib/index.js:121:5)\r\n\r\nResource is not in the state stackUpdateComplete\r\n"] [214.861,"o","ResourceNotReady: Resource is not in the state stackUpdateComplete\r\n at constructor.setError (/snapshot/repo/build/node_modules/aws-sdk/lib/resource_waiter.js:182:47)\r\n at Request.CHECK_ACCEPTORS (/snapshot/repo/build/node_modules/aws-sdk/lib/resource_waiter.js:44:12)\r\n at Request.callListeners (/snapshot/repo/build/node_modules/aws-sdk/lib/sequential_executor.js:106:20)\r\n at Request.emit (/snapshot/repo/build/node_modules/aws-sdk/lib/sequential_executor.js:78:10)\r\n at Request.emit (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:686:14)\r\n at Request.transition (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:22:10)\r\n at AcceptorStateMachine.runTo (/snapshot/repo/build/node_modules/aws-sdk/lib/state_machine.js:14:12)\r\n at /snapshot/repo/build/node_modules/aws-sdk/lib/state_machine.js:26:10\r\n at Request.<anonymous> (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:38:9)\r\n at Request.<anonymous> (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:688:12)\r\n at Request.callListeners (/snapshot/repo/build/node_modules/aws-sdk/lib/sequential_executor.js:116:18)\r\n at Request.emit (/snapshot/repo/build/node_modules/aws-sdk/lib/sequential_executor.js:78:10)\r\n at Request.emit (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:686:14)\r\n at Request.transition (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:22:10)\r\n at AcceptorStateMachine.runTo (/snapshot/repo/build/node_modules/aws-sdk/lib/state_machine.js:14:12)\r\n at /snapshot/repo/build/node_modules/aws-sdk/lib/state_machine.js:26:10\r\n at Request.<anonymous> (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:38:9)\r\n at Request.<anonymous> (/snapshot/repo/build/node_modules/aws-sdk/lib/request.js:688:12)\r\n at Request.callListeners (/snapshot/repo/build/node_modules/aws-sdk/lib/sequential_executor.js:116:18)\r\n at callNextListener (/snapshot/repo/build/node_modules/aws-sdk/lib/sequential_executor.js:96:12)\r\n at IncomingMessage.onEnd (/snapshot/repo/build/node_modules/aws-sdk/lib/event_listeners.js:417:13)\r\n at IncomingMessage.emit (node:events:525:35)\r\n at IncomingMessage.emit (node:domain:489:12)\r\n at endReadableNT (node:internal/streams/readable:1359:12)"] [214.861,"o","\r\n at process.processTicksAndRejections (node:internal/process/task_queues:82:21)\r\n"]

sobolk · 2023-07-13T18:55:27Z

Looks good. I think there's 1 change we need to make, and then I will say it looks like there's future tests that were added, but I think that's okay because if the tests pass now, we can just TDD the future code.

Yes. Taking these tests eagerly was intentional.

This reverts commit 74cf0dc.

edwardfoyle · 2023-07-14T16:43:07Z

packages/amplify-migration-tests/package.json

@@ -21,6 +21,7 @@
    "migration_v4.28.2_nonmultienv_layers": "yarn setup-profile 4.28.2 && jest src/__tests__/migration_tests/lambda-layer-migration/layer-migration.test.ts --verbose --config=jest.config.js",
    "migration_v4.52.0_multienv_layers": "yarn setup-profile 4.52.0 && jest src/__tests__/migration_tests/lambda-layer-migration/layer-migration.test.ts --verbose --config=jest.config.js",
    "migration_v10.5.1": "yarn setup-profile 10.5.1 && jest --verbose --config=jest.config.js",
+    "migration_v12.0.3": "yarn setup-profile 12.0.3 && jest --verbose --config=jest.config.js",


should we test migrating from 12.1.1 (current latest)?

Current latest is soon to be replaced by 12.2.0-rc.a3b139fb36.0 . I'd refrain from jumping on moving target.
For this migration purposes 12.0.3 is as good as latest
The diffs (v12.0.3...v12.1.1 and v12.0.3...release_rc/a3b139fb36) don't reveal significant changes (both show 6 changed files in auth category - changelog, versions in package.json, test changes due to yarn migration and the userPoolConsoleUrl change - which isn't significant to this PR.).

If we ship this PR before batch 2 arrives then I'll be inclined to add one more v12 suite to cover version in-between.

sobolk added 22 commits July 7, 2023 13:39

chore: easy to port changes

c66cf28

chore: oauth lambda transplantation plus migration tests it compiles

e91bd58

chore: more changes

e93c60f

chore: more changes

211ca4e

chore: remove userpoolclientLambda

3eab8be

chore: more changes

7aeddf4

chore: generate auth secret

27566b7

chore: api

6100289

fix: that

ebadf5f

chore: tests

3bb797b

chore: merge remote-tracking branch 'upstream/dev' into auth-lambdas-…

792ce21

…batch-1

chore: tests

3915eb9

chore: tests

7731994

chore: tests

bd9926e

chore: tests

6dfe6a9

chore: tests

67fce6c

chore: tests

8652e65

chore: tests

a935a71

chore: tests

d299f66

chore: tests

29311bc

chore: tests

5691056

chore: codebuild tests

fceefa7

chore: isolate that test

52d067a

sobolk marked this pull request as ready for review July 12, 2023 03:55

sobolk requested review from a team as code owners July 12, 2023 03:55

sobolk added 2 commits July 12, 2023 10:54

chore: headless test

f76b6d4

chore: app secret test

28932cd

Amplifiyer previously approved these changes Jul 12, 2023

View reviewed changes

sobolk dismissed Amplifiyer’s stale review via b179aa4 July 12, 2023 22:34

sobolk and others added 5 commits July 12, 2023 15:34

Update packages/amplify-category-auth/src/provider-utils/awscloudform…

b179aa4

…ation/auth-stack-builder/auth-cognito-stack-builder.ts Co-authored-by: Amplifiyer <[email protected]>

chore: latest headless add auth app secret

06689c2

chore: more tests

2278758

chore: oauth headless test

b6fa5d5

chore: oauth force push test

32e9d5b

github-advanced-security bot found potential problems Jul 13, 2023

View reviewed changes

...mplify-migration-tests/src/__tests__/migration_tests_v12/auth-oauth-lambda-migration.test.ts Fixed Show fixed Hide fixed

danielleadams suggested changes Jul 13, 2023

View reviewed changes

sobolk added 4 commits July 13, 2023 12:50

chore: headless oauth migration tesT

9d63da9

chore: pr feedback

74cf0dc

chore: refresh codebuild

4d791a6

chore: revert "chore: pr feedback"

95f0f8b

This reverts commit 74cf0dc.

Amplifiyer approved these changes Jul 14, 2023

View reviewed changes

edwardfoyle approved these changes Jul 14, 2023

View reviewed changes

sobolk merged commit b3ab512 into aws-amplify:dev Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace oauth and userpoolclient lambdas with cfn and sdk calls #12935

feat: replace oauth and userpoolclient lambdas with cfn and sdk calls #12935

sobolk commented Jul 11, 2023

codecov-commenter commented Jul 12, 2023 •

edited

Loading

Amplifiyer Jul 12, 2023

sobolk Jul 12, 2023

Amplifiyer Jul 12, 2023

sobolk Jul 12, 2023

Amplifiyer Jul 12, 2023

danielleadams left a comment

danielleadams Jul 13, 2023

sobolk Jul 13, 2023

danielleadams Jul 13, 2023

danielleadams Jul 13, 2023

danielleadams Jul 13, 2023

danielleadams Jul 13, 2023

sobolk Jul 13, 2023

sobolk Jul 13, 2023

danielleadams Jul 13, 2023

sobolk Jul 13, 2023

sobolk Jul 13, 2023

sobolk Jul 13, 2023

sobolk Jul 13, 2023

sobolk commented Jul 13, 2023

edwardfoyle Jul 14, 2023

sobolk Jul 14, 2023 •

edited

Loading

		userPoolClientRole?: iam.CfnRole;
		userPoolClientLambdaPolicy?: iam.CfnPolicy;

feat: replace oauth and userpoolclient lambdas with cfn and sdk calls #12935

feat: replace oauth and userpoolclient lambdas with cfn and sdk calls #12935

Conversation

sobolk commented Jul 11, 2023

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

codecov-commenter commented Jul 12, 2023 • edited Loading

Codecov Report

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

danielleadams 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

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

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

sobolk commented Jul 13, 2023

Choose a reason for hiding this comment

sobolk Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 12, 2023 •

edited

Loading

sobolk Jul 14, 2023 •

edited

Loading