refactor(cli): aws-auth uses modern messaging infrastructure everywhere #292

mrgrain · 2025-03-28T20:03:35Z

Updates aws-auth module to use modern messaging infrastructure.

Testing

Existing unit and integration tests are passing
Successfully run the auth test suite, with the exception of tests/test-non-commercial-region.sh which was skipped.
Manually confirmed that the AccountAccessKeyCache is still written
Manually executed common CLI operations and I did not spot any additional output (as expected)

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

mrgrain · 2025-03-28T20:06:01Z

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

this is a weird change, but i think the abstraction was kind of wrong already.

It wasn't an abstraction, it was a set of factory functions grouped in a namespace 😆

mrgrain · 2025-03-28T20:06:21Z

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

    };
  }

-  public static proxyAgent(options: SdkHttpOptions) {


pulled out into a separate class. this had otherwise nothing to do with this file

packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts

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

codecov-commenter · 2025-03-28T20:09:11Z

Codecov Report

Attention: Patch coverage is 83.67347% with 40 lines in your changes missing coverage. Please review.

Project coverage is 85.62%. Comparing base (445fd71) to head (14f03f3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/aws-cdk/lib/api/aws-auth/proxy-agent.ts	65.45%	19 Missing ⚠️
packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts	77.41%	7 Missing ⚠️
...ages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts	93.02%	6 Missing ⚠️
...helpers/src/api/io/private/testing/test-io-host.ts	33.33%	4 Missing ⚠️
...ges/aws-cdk/lib/api/aws-auth/credential-plugins.ts	70.00%	3 Missing ⚠️
packages/aws-cdk/lib/notices.ts	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
+ Coverage   85.54%   85.62%   +0.07%     
==========================================
  Files         222      223       +1     
  Lines       36926    37003      +77     
  Branches     4458     4491      +33     
==========================================
+ Hits        31588    31682      +94     
+ Misses       5248     5222      -26     
- Partials       90       99       +9

Flag	Coverage Δ
suite.unit	`85.62% <83.67%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mrgrain · 2025-03-28T20:11:21Z

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

    }
    return fs.readFileSync(filename, { encoding: 'utf-8' });
  } catch (e: any) {
-    debug(e);


I'm removing this debug. This only happens of the file exists but cannot be read, which seems super unliekly. The helper is called from only two places. If someone feels strongly about it, I can implement this in the call-sites.

Given this is used in our proxy support, I feel we need any bit of information while debugging strange behavior there. I don't mind dropping it from the user agent construction.

mrgrain · 2025-03-28T20:12:39Z

packages/aws-cdk/lib/notices.ts

+
+    const options: RequestOptions = {
+      agent: await new ProxyAgentProvider(this.ioHelper).create(this.options),
+    };
+
+    const notices = await new Promise<Notice[]>((resolve, reject) => {


bit of a rejig here to allow sending a message after the promise resolved

Could just have done a .then() at the end 🤣

Happy to change it, I don't mind either way. I've received feedback in the past that mixing promise styles is confusing.

mrgrain · 2025-03-28T20:13:40Z

packages/aws-cdk/test/api/_helpers/hotswap-test-setup.ts

  ): Promise<SuccessfulDeployStackResult | undefined> {
    let hotswapProps = hotswapPropertyOverrides || new HotswapPropertyOverrides();
-    return deployments.tryHotswapDeployment(this, asIoHelper(ioHost, 'deploy'), assetParams, currentCfnStack, stackArtifact, hotswapMode, hotswapProps);
+    return deployments.tryHotswapDeployment(this, asIoHelper(ioHost, 'deploy'), assetParams, currentCfnStack, stackArtifact, hotswapMode as any, hotswapProps);


this was a type error before. tryHotswapDeployment only takes two of the 3 enum values and in the CLI code this is enforced. Tests also don't call the method with the third value.

mrgrain · 2025-03-28T20:17:01Z

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

 */
 @traceMemberMethods
 export class SDK {
-  private static readonly accountCache = new AccountAccessKeyCache();


this doesn't need to be a singleton. The class is always reading/writing from a file anywhere and does not cache values. If we ever wanted to, we can make the cache a singleton but keep the class as an instance per SDK.

It could also be made a constructor argument. Same point as for the proxy agent.

It could also be made a constructor argument. Same point as for the proxy agent.

True. There is opportunity for refactoring. Right now I want to keep the changes to the minimum to get this in.

otaviomacedo · 2025-04-02T13:29:00Z

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

-  public static requestHandlerBuilder(options: SdkHttpOptions = {}): NodeHttpHandlerOptions {
-    const agent = this.proxyAgent(options);
+  public async requestHandlerBuilder(options: SdkHttpOptions = {}): Promise<NodeHttpHandlerOptions> {
+    const agent = await new ProxyAgentProvider(this.ioHelper).create(options);


Since we are changing these things, it would be better to pass then agent as a constructor argument. Let the SDK Provider, which is the factory class, to also instantiate this.

I tried, but it has a bunch of knock-on effects that I don't want to deal with right now.

otaviomacedo · 2025-04-02T13:30:46Z

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

 */
 @traceMemberMethods
 export class SDK {
-  private static readonly accountCache = new AccountAccessKeyCache();


It could also be made a constructor argument. Same point as for the proxy agent.

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

packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts

iliapolo · 2025-04-02T13:34:02Z

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

    }
    return fs.readFileSync(filename, { encoding: 'utf-8' });
  } catch (e: any) {
-    debug(e);


Given this is used in our proxy support, I feel we need any bit of information while debugging strange behavior there. I don't mind dropping it from the user agent construction.

rix0rrr · 2025-04-02T14:05:53Z

packages/aws-cdk/lib/api/aws-auth/account-cache.ts

+   * The default path used for the accounts access key cache
+   */
+  public static get DEFAULT_PATH(): string {
+    // needs to be a getter because cdkCacheDir can be set via env variable and might change


...during tests?

(Trying to wrap my head around how an env var might change across the execution)

during tests, but in toolkit-lib it could be at any point of the code execution.

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

mrgrain temporarily deployed to integ-approval March 28, 2025 20:03 — with GitHub Actions Inactive

github-actions bot added the p2 label Mar 28, 2025

aws-cdk-automation requested a review from a team March 28, 2025 20:05

mrgrain commented Mar 28, 2025

View reviewed changes

packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts Show resolved Hide resolved

mrgrain commented Mar 28, 2025

View reviewed changes

packages/aws-cdk/lib/api/aws-auth/sdk.ts Outdated Show resolved Hide resolved

mrgrain commented Mar 28, 2025

View reviewed changes

mrgrain changed the title ~~refactor(cli): aws-use uses modern messaging infrastructure everywhere~~ refactor(cli): aws-auth uses modern messaging infrastructure everywhere Mar 28, 2025

mrgrain temporarily deployed to run-tests March 31, 2025 08:22 — with GitHub Actions Inactive

mrgrain marked this pull request as ready for review April 1, 2025 17:59

aws-cdk-automation enabled auto-merge April 1, 2025 17:59

mrgrain disabled auto-merge April 2, 2025 11:55

otaviomacedo reviewed Apr 2, 2025

View reviewed changes

iliapolo reviewed Apr 2, 2025

View reviewed changes

rix0rrr reviewed Apr 2, 2025

View reviewed changes

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts Outdated Show resolved Hide resolved

mrgrain force-pushed the mrgrain/refactor/messaging-aws-auth branch from a839899 to a78e710 Compare April 2, 2025 15:54

mrgrain temporarily deployed to integ-approval April 2, 2025 15:54 — with GitHub Actions Inactive

mrgrain temporarily deployed to run-tests April 2, 2025 16:10 — with GitHub Actions Inactive

iliapolo approved these changes Apr 2, 2025

View reviewed changes

rix0rrr approved these changes Apr 2, 2025

View reviewed changes

one last change

14f03f3

mrgrain temporarily deployed to integ-approval April 2, 2025 16:42 — with GitHub Actions Inactive

mrgrain temporarily deployed to run-tests April 2, 2025 16:53 — with GitHub Actions Inactive

mrgrain temporarily deployed to run-tests April 2, 2025 16:54 — with GitHub Actions Inactive

kaizencc added this pull request to the merge queue Apr 2, 2025

Merged via the queue into main with commit 3e8359c Apr 2, 2025
20 checks passed

kaizencc deleted the mrgrain/refactor/messaging-aws-auth branch April 2, 2025 19:35

refactor(cli): aws-auth uses modern messaging infrastructure everywhere #292

refactor(cli): aws-auth uses modern messaging infrastructure everywhere #292

Uh oh!

Conversation

mrgrain commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

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!

Uh oh!

codecov-commenter commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Apr 2, 2025 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mrgrain commented Mar 28, 2025 •

edited

Loading

codecov-commenter commented Mar 28, 2025 •

edited

Loading

rix0rrr Apr 2, 2025 •

edited

Loading