feat(cli-plugin-contract): introduce a public contract between CLI and plugins #32111

otaviomacedo · 2024-11-13T14:20:24Z

The contract between the CLI and credential provider plugins is not publicly defined.

Extract the types involved in the CLI-plugin communication into a new package, @aws-cdk/cli-plugin-contract, and update all references in the CLI code.

Closes #32099.

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

…d plugins

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.

codecov · 2024-11-13T14:43:42Z

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.48%. Comparing base (421d327) to head (33e3e2f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32111      +/-   ##
==========================================
+ Coverage   77.17%   77.48%   +0.30%     
==========================================
  Files         105      104       -1     
  Lines        7169     7164       -5     
  Branches     1315     1313       -2     
==========================================
+ Hits         5533     5551      +18     
+ Misses       1455     1431      -24     
- Partials      181      182       +1

Flag	Coverage Δ
suite.unit	`77.48% <93.75%> (+0.30%)`	⬆️

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

Components	Coverage Δ
packages/aws-cdk	`77.48% <93.75%> (+0.30%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

mrgrain · 2024-11-13T15:47:01Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+   *
+   * Guaranteed to be called only if canProvideCredentails() returned true at some point.
+   */
+  getProvider(accountId: string, mode: Mode): Promise<AwsCredentials>;


We change this to a strongly typed string, we can make this package types only.

Suggested change

getProvider(accountId: string, mode: Mode): Promise<AwsCredentials>;

getProvider(accountId: string, mode: 'ForReading' | 'ForWriting'): Promise<AwsCredentials>;

This would be a breaking change for plugins, which are expecting a number (ForReading = 0, ForWriting = 1).

getProvider(accountId: string, mode: 0 | 1): Promise<AwsCredentials>; getProviderEx(accountId: string, mode: 'ForReading' | 'ForWriting'): Promise<AwsCredentials>;

?

This reverts commit 16d3d4d.

otaviomacedo · 2024-11-25T11:21:24Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+/**
+ * A list of credential provider sources
+ */
+export interface CredentialProviderSourceRepository {


I'd happily rename this interface if someone has a better suggestion.

PluginHost ?

It's technically also for lookup plugins, for example.

If we call it PluginHost, what should we call the class that implements it in the CLI?

otaviomacedo · 2024-11-25T11:28:49Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+  getPromise?: () => Promise<void>;
+}
+
+export enum Mode {


I've considered making this a const enum but, given the documented pitfalls, I've decided against it. This value will probably be used only once during the execution of the plugin, which itself is part of much bigger CDK command execution. So the cost of additional indirection when accessing enum values is negligible.

The goal of that was more to avoid having plugins take a runtime dependency on this package. They should be able to take only a devDependency on this package.

What is the concrete pitfall you're concerned about?

All three of them. If you look at the approaches to avoid them, they boil down to "do not use this feature if an external package is going to consume this enum", which is exactly the case here.

aws-cdk-automation · 2024-11-25T11:33:55Z

➡️ 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.

rix0rrr · 2024-11-25T12:36:17Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+/**
+ * A list of credential provider sources
+ */
+export interface CredentialProviderSourceRepository {


PluginHost ?

It's technically also for lookup plugins, for example.

rix0rrr · 2024-11-25T12:36:56Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+   * Refreshes the current credentials. This function only exists for
+   * legacy reasons, to be compatible with the `AWS.Credentials` class.
+   * Plugins that use the AWS SDK v3 don't need this.


Not necessarily true. If you want credentials that need to refresh themselves (session credentials, for example), how else would they work?

rix0rrr · 2024-11-25T12:37:29Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+  /**
+   *   A Date when the identity or credential will no longer be accepted.
+   */
+  readonly expiration?:	Date;


Is this actually used?

rix0rrr · 2024-11-25T12:37:33Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+  /**
+   * AWS accountId.
+   */
+  readonly accountId?: string;


Is this actually used?

rix0rrr · 2024-11-25T12:37:35Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+  /**
+   * AWS credential scope for this set of credentials.
+   */
+  readonly credentialScope?: string;


Is this actually used?

Not directly by the CLI, but can be used by the SDK.

rix0rrr · 2024-11-25T12:38:31Z

packages/@aws-cdk/cli-plugin-contract/lib/index.ts

+  getPromise?: () => Promise<void>;
+}
+
+export enum Mode {


The goal of that was more to avoid having plugins take a runtime dependency on this package. They should be able to take only a devDependency on this package.

What is the concrete pitfall you're concerned about?

rix0rrr · 2024-11-25T12:41:16Z

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

@@ -65,13 +66,13 @@ export class CredentialPlugins {
      // Otherwise it must have returned credentials.
      const credentials = (providerOrCreds as any).resolvePromise


The as any should no longer be necessary, should it?

I don't see resolvePromise() in the typing package. Where is it coming from? What is this object?

True. It should be part of the interface.

rix0rrr · 2024-11-25T12:42:04Z

packages/aws-cdk/package.json

@@ -102,6 +102,7 @@
    "xml-js": "^1.6.11"
  },
  "dependencies": {
+    "@aws-cdk/cli-plugin-contract": "0.0.0",


devDependency please

rix0rrr · 2024-11-25T12:42:32Z

packages/aws-cdk/package.json

+  "peerDependencies": {
+    "@aws-cdk/cli-plugin-contract": "0.0.0"


dependency + peerDependency together doesn't mean anything

aws-cdk-automation · 2024-11-25T16:16:15Z

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/32111/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.

aws-cdk-automation · 2024-11-25T16:44:20Z

AWS CodeBuild CI Report

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

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

The CLI immediately invokes credential *providers* to produce *credentials*, and passes the credentials around instead of the providers. This means that if short-lived credentials expire (like session credentials from roles), there is no way to refresh them. CLI calls will start to fail if that happens. To fix this, instead of resolving providers to credentials, pass providers around instead. Implications for auth plugins ------------- This widens the plugin protocol: the new plugin protocol *forced* a translation to V3 credentials, and had no way to return V3 providers. While it is now possible to return V3 Credential Providers from the plugin protocol, plugin writers cannot easily take advantage of that protocol because there have been ~8 CLI releases that only support V3 credentials and will fail at runtime of V3 providers are returned. To support this, pass a new options argument into `getProvider()`: this will indicate whether V3 Providers are supported or not. Plugins can return a provider if the CLI indicates that it supports V3 providers, and avoid doing that if the CLI indicates it won't. That way, plugins can be rewritten to take advantage of returning V3 providers without crashing on CLI versions `2.167.0..(this releases)`. This also affects #32111 in which the plugin contract is being moved.

The CLI immediately invokes credential *providers* to produce *credentials*, and passes the credentials around instead of the providers. This means that if short-lived credentials expire (like session credentials from roles), there is no way to refresh them. CLI calls will start to fail if that happens. To fix this, instead of resolving providers to credentials, pass providers around instead. Implications for auth plugins ------------- This widens the plugin protocol: the new plugin protocol *forced* a translation to V3 credentials, and had no way to return V3 providers. While it is now possible to return V3 Credential Providers from the plugin protocol, plugin writers cannot easily take advantage of that protocol because there have been ~8 CLI releases that only support V3 credentials and will fail at runtime of V3 providers are returned. To support this, pass a new options argument into `getProvider()`: this will indicate whether V3 Providers are supported or not. Plugins can return a provider if the CLI indicates that it supports V3 providers, and avoid doing that if the CLI indicates it won't. That way, plugins can be rewritten to take advantage of returning V3 providers without crashing on CLI versions `2.167.0..(this releases)`. This also affects #32111 in which the plugin contract is being moved. Closes #32287. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

otaviomacedo added 3 commits November 13, 2024 12:07

feat(cli-plugin-contract): introduce a public contract between CLI an…

797f920

…d plugins

README update

af8d279

Fixes

819ca45

aws-cdk-automation requested a review from a team November 13, 2024 14:20

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 13, 2024

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

otaviomacedo temporarily deployed to test-pipeline November 13, 2024 14:21 — with GitHub Actions Inactive

aws-cdk-automation requested changes Nov 13, 2024

View reviewed changes

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

otaviomacedo added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 13, 2024

yarn build && yarn test

54dac39

otaviomacedo had a problem deploying to test-pipeline November 13, 2024 14:32 — with GitHub Actions Failure

New test

d7b671a

otaviomacedo temporarily deployed to test-pipeline November 13, 2024 15:40 — with GitHub Actions Inactive

mrgrain reviewed Nov 13, 2024

View reviewed changes

const enum

16d3d4d

otaviomacedo requested a deployment to test-pipeline November 15, 2024 15:53 — with GitHub Actions Waiting

otaviomacedo added 2 commits November 25, 2024 10:37

Revert "const enum"

3aa719b

This reverts commit 16d3d4d.

More details on getPromise()

2dad460

otaviomacedo requested a deployment to test-pipeline November 25, 2024 10:44 — with GitHub Actions Waiting

Add expiration field.

ddd6733

otaviomacedo requested a deployment to test-pipeline November 25, 2024 11:18 — with GitHub Actions Waiting

Formatting

390e99f

otaviomacedo requested a deployment to test-pipeline November 25, 2024 11:21 — with GitHub Actions Waiting

otaviomacedo commented Nov 25, 2024

View reviewed changes

otaviomacedo marked this pull request as ready for review November 25, 2024 11:29

otaviomacedo requested a review from a team as a code owner November 25, 2024 11:29

Merge branch 'main' into otaviom/cli-plugin-contract

29b1d95

otaviomacedo temporarily deployed to test-pipeline November 25, 2024 11:31 — with GitHub Actions Inactive

rix0rrr requested changes Nov 25, 2024

View reviewed changes

otaviomacedo added 2 commits November 25, 2024 14:36

Remove as any

236dd39

Merge branch 'main' into otaviom/cli-plugin-contract

33e3e2f

otaviomacedo requested a deployment to test-pipeline November 25, 2024 16:14 — with GitHub Actions Waiting

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

Monthly PR metrics report #32340

Open

rix0rrr mentioned this pull request Dec 2, 2024

fix(cli): short-lived credentials are not refreshed #32354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli-plugin-contract): introduce a public contract between CLI and plugins #32111

feat(cli-plugin-contract): introduce a public contract between CLI and plugins #32111

otaviomacedo commented Nov 13, 2024

aws-cdk-automation left a comment

codecov bot commented Nov 13, 2024 •

edited

Loading

mrgrain Nov 13, 2024 •

edited

Loading

otaviomacedo Nov 13, 2024

rix0rrr Nov 25, 2024

otaviomacedo Nov 25, 2024

rix0rrr Nov 25, 2024

otaviomacedo Nov 25, 2024

otaviomacedo Nov 25, 2024

rix0rrr Nov 25, 2024

otaviomacedo Nov 25, 2024

aws-cdk-automation commented Nov 25, 2024

rix0rrr Nov 25, 2024

rix0rrr Nov 25, 2024

rix0rrr Nov 25, 2024

rix0rrr Nov 25, 2024

rix0rrr Nov 25, 2024

otaviomacedo Nov 25, 2024

rix0rrr Nov 25, 2024

rix0rrr Nov 25, 2024

otaviomacedo Nov 25, 2024

rix0rrr Nov 25, 2024

rix0rrr Nov 25, 2024

aws-cdk-automation commented Nov 25, 2024

aws-cdk-automation commented Nov 25, 2024

	getProvider(accountId: string, mode: Mode): Promise<AwsCredentials>;
	getProvider(accountId: string, mode: 'ForReading' \| 'ForWriting'): Promise<AwsCredentials>;

		@@ -65,13 +66,13 @@ export class CredentialPlugins {
		// Otherwise it must have returned credentials.
		const credentials = (providerOrCreds as any).resolvePromise

		"peerDependencies": {
		"@aws-cdk/cli-plugin-contract": "0.0.0"

feat(cli-plugin-contract): introduce a public contract between CLI and plugins #32111

Are you sure you want to change the base?

feat(cli-plugin-contract): introduce a public contract between CLI and plugins #32111

Conversation

otaviomacedo commented Nov 13, 2024

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

mrgrain Nov 13, 2024 • edited Loading

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

aws-cdk-automation commented Nov 25, 2024

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

aws-cdk-automation commented Nov 25, 2024

aws-cdk-automation commented Nov 25, 2024

AWS CodeBuild CI Report

codecov bot commented Nov 13, 2024 •

edited

Loading

mrgrain Nov 13, 2024 •

edited

Loading