feat: sdkv3 upgrade for CLI (wip) by TheRealAmazonKendra · Pull Request #31624 · aws/aws-cdk

TheRealAmazonKendra · 2024-10-02T17:30:26Z

Issue # (if applicable)

Closes #.

Reason for this change

Description of changes

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

otaviomacedo · 2024-10-03T12:56:08Z

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts

-      // ECS and EKS instances also run on EC2 boxes but the creds represent something different.
-      // Same behavior as upstream code.
-      sources.push(() => new AWS.EC2MetadataCredentials());
+      return fromIni({


Currently, SSO has precedence over process, but in fromIni, it's the other way around. It's highly unlikely that someone mistakenly configures both, but I think we should keep the current behavior.

So, fromIni actually covers SSO in v3. It didn't work with V2, which is why we manually put it ahead of Ini.

Sure, but we are changing the behavior now. If someone has both credential_process and sso_* set in their config file, the current version of the CLI will use the SSO credentials. With this change, after they upgrade, the CLI will start using the process credentials. Maybe this is ok, but but we need at least to document this somewhere.

otaviomacedo · 2024-10-03T13:16:09Z

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts

+    return shouldPrioritizeEnv()
+      ? createCredentialChain(fromEnv(), nodeProviderChain).expireAfter(60 * 60_000)
+      : nodeProviderChain;
+  }


[feedback on readability] when I was reviewing this, I found it a bit hard to follow how the env variables fit into the chain. It's surprising to see a method whose name starts with "should" that modifies environment variables, that then gets picked up by fromEnv() (or not).

I suggest we implement our own provider that reads from both AWS_* and AMAZON_* sets of variables. And this provider is always put at the beginning of the chain. Up to you.

otaviomacedo · 2024-10-03T13:23:50Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

-    // we can determine whether the AssumeRole call succeeds or not.
    try {
-      await sdk.forceCredentialRetrieval();
+      // Force retrieval here


We can remove this whole try-catch now, can't we?

Yes. Removing it.

packages/aws-cdk/lib/api/aws-auth/sdk.ts

otaviomacedo · 2024-10-04T08:38:05Z

packages/aws-cdk/test/api/sdk-provider.test.ts

-          config: {
-            'profile ecs': { role_arn: 'arn:aws:iam::12356789012:role/Assumable', credential_source: 'EcsContainer', $account: '22222' },
+      // GIVEN
+      const calls = jest.spyOn(console, 'debug');


Shouldn't we be using the mocked metadata service?

otaviomacedo · 2024-10-04T08:48:45Z

packages/aws-cdk/test/api/fake-sts.ts

 * SDK, so we pretend to be the STS server and have an in-memory database
 * of users and roles.
+ *
+ * With the v3 upgrade, this is only now half way being used as


being used as?

I died half way though writing this comment. There is no way of knowing the ending.

But also, the "as" is that the mock sdk client made some of it unnecessary. I'll fix this comment to clarify.

packages/aws-cdk/test/context-providers/load-balancers.test.ts

otaviomacedo · 2024-10-04T09:24:02Z

packages/aws-cdk/test/util/awscli-compatible.test.ts

Instead of deleting these tests, can't we keep them and use a mocked STS and a mocked token file?

I don't know why it's outdated, but the question still applies.

The reason I deleted these instead of trying to rewrite them is that previously we wrote our own functionality for whether or not to call these, now we're just using the credential provider that the sdk provides so we'd essentially be unit testing their code, not ours.

packages/@aws-cdk/integ-runner/package.json

otaviomacedo · 2024-10-04T10:09:44Z

packages/aws-cdk/test/api/bootstrap.test.ts

  name: 'mock',
 };

+const templateBody = toYAML(deserializeStructure(serializeStructure(legacyBootstrapTemplate({}), true)));


What's the difference between this and:

const templateBody = serializeStructure(legacyBootstrapTemplate({}), false);

?

This is silly looking but unfortunately necessary. If we only serialize it, the template body isn't formatted correctly due to the escape characters that get added in. If we skip the deserialization/serialization part and just do toYAML, we get the same output.

otaviomacedo

We should also add some CLI integration tests for the credential loading scenarios.

…ns (#31658) #31624 is too big so I'm splitting out the parts that I can without causing type conflicts. This upgrades all sdk v3 versions to the most recent version used in every region by lambda's node18 and node20 runtime bundle. Closes #<issue number here>. ### Reason for this change ### Description of changes ### Description of how you validated changes ### Checklist - [ ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

TheRealAmazonKendra · 2024-10-08T23:50:11Z

This is succeeding in our build pipeline but not in the PR Build. What in the world is going on.

aws-cdk-automation · 2024-10-09T00:42:08Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 28a5722
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2024-10-09T00:44:19Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

TheRealAmazonKendra · 2024-10-10T07:13:39Z

We should also add some CLI integration tests for the credential loading scenarios.

Working on it!

aws-cdk-automation · 2024-10-10T07:15:17Z

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/31624/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

github-actions · 2024-11-20T12:48:07Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

github-actions bot added the p2 label Oct 2, 2024

aws-cdk-automation requested a review from a team October 2, 2024 17:30

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 2, 2024

TheRealAmazonKendra had a problem deploying to test-pipeline October 2, 2024 17:31 — with GitHub Actions Failure

aws-cdk-automation requested changes Oct 2, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Oct 2, 2024

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 78798f5 to fd34c5c Compare October 3, 2024 10:14

TheRealAmazonKendra had a problem deploying to test-pipeline October 3, 2024 10:15 — with GitHub Actions Failure

otaviomacedo reviewed Oct 3, 2024

View reviewed changes

otaviomacedo reviewed Oct 4, 2024

View reviewed changes

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from fd34c5c to 54a35cf Compare October 4, 2024 10:36

TheRealAmazonKendra had a problem deploying to test-pipeline October 4, 2024 10:37 — with GitHub Actions Failure

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 54a35cf to 8f7b25c Compare October 4, 2024 11:03

TheRealAmazonKendra temporarily deployed to test-pipeline October 4, 2024 11:05 — with GitHub Actions Inactive

otaviomacedo reviewed Oct 4, 2024

View reviewed changes

TheRealAmazonKendra had a problem deploying to test-pipeline October 4, 2024 18:47 — with GitHub Actions Failure

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 66ed30f to 723afa7 Compare October 4, 2024 18:57

TheRealAmazonKendra temporarily deployed to test-pipeline October 4, 2024 18:58 — with GitHub Actions Inactive

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 723afa7 to f56fe7f Compare October 4, 2024 22:33

TheRealAmazonKendra had a problem deploying to test-pipeline October 4, 2024 22:34 — with GitHub Actions Failure

TheRealAmazonKendra mentioned this pull request Oct 4, 2024

chore: upgrade sdkv3 to the newest version used by lambda distributions #31658

Merged

1 task

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from f56fe7f to 4acc721 Compare October 7, 2024 19:55

TheRealAmazonKendra had a problem deploying to test-pipeline October 7, 2024 19:56 — with GitHub Actions Failure

TheRealAmazonKendra temporarily deployed to test-pipeline October 7, 2024 21:00 — with GitHub Actions Inactive

TheRealAmazonKendra temporarily deployed to test-pipeline October 8, 2024 08:54 — with GitHub Actions Inactive

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 16ffad3 to 714b314 Compare October 8, 2024 09:05

TheRealAmazonKendra had a problem deploying to test-pipeline October 8, 2024 09:06 — with GitHub Actions Failure

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 714b314 to de3c43d Compare October 8, 2024 09:09

TheRealAmazonKendra temporarily deployed to test-pipeline October 8, 2024 09:10 — with GitHub Actions Inactive

TheRealAmazonKendra changed the title ~~WIP: temp commit for sdkv3 upgrade~~ feat: sdkv3 upgrade for CLI (wip) Oct 8, 2024

TheRealAmazonKendra added the pr-linter/exempt-readme The PR linter will not require README changes label Oct 8, 2024

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from de3c43d to 9b9c312 Compare October 8, 2024 09:26

TheRealAmazonKendra had a problem deploying to test-pipeline October 8, 2024 09:27 — with GitHub Actions Failure

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from 9b9c312 to c36eb47 Compare October 8, 2024 18:46

TheRealAmazonKendra had a problem deploying to test-pipeline October 8, 2024 18:47 — with GitHub Actions Failure

TheRealAmazonKendra had a problem deploying to test-pipeline October 8, 2024 19:27 — with GitHub Actions Failure

TheRealAmazonKendra had a problem deploying to test-pipeline October 8, 2024 20:15 — with GitHub Actions Failure

TheRealAmazonKendra temporarily deployed to test-pipeline October 8, 2024 22:21 — with GitHub Actions Inactive

TheRealAmazonKendra added 5 commits October 8, 2024 17:04

update configuration files and package versions

d6ed395

cli changes

509c2bd

other resulting changes

b475621

changes from rebase and build

4d1c009

fixed most integ tests - migrate and sam still issue

3bb1c21

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/sdkv3-for-aws-cdk branch from e46a6ca to 3bb1c21 Compare October 9, 2024 00:06

TheRealAmazonKendra temporarily deployed to test-pipeline October 9, 2024 00:07 — with GitHub Actions Inactive

skip tests in regression testing

28a5722

TheRealAmazonKendra temporarily deployed to test-pipeline October 9, 2024 00:28 — with GitHub Actions Inactive

mergify bot mentioned this pull request Oct 9, 2024

feat(cli): upgrade aws-sdk to sdkv3 #31702

Merged

28 tasks

TheRealAmazonKendra temporarily deployed to test-pipeline October 9, 2024 00:48 — with GitHub Actions Inactive

github-actions bot mentioned this pull request Nov 1, 2024

Monthly PR metrics report #31972

Closed

rix0rrr closed this Nov 20, 2024

rix0rrr deleted the TheRealAmazonKendra/sdkv3-for-aws-cdk branch November 20, 2024 12:47

github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2024

Conversation

TheRealAmazonKendra commented Oct 2, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otaviomacedo Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otaviomacedo left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra commented Oct 8, 2024

Uh oh!

aws-cdk-automation commented Oct 9, 2024

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Oct 9, 2024

Uh oh!

TheRealAmazonKendra commented Oct 10, 2024

Uh oh!

aws-cdk-automation commented Oct 10, 2024

Uh oh!

github-actions bot commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

otaviomacedo Oct 3, 2024 •

edited

Loading