feat(cli): diff now uses the lookup Role for new-style synthesis by corymhall · Pull Request #18277 · aws/aws-cdk

corymhall · 2022-01-05T13:48:45Z

This PR exposes information on the bootstrap lookup role on the
CloudFormation stack artifact. This enables the CLI to assume the lookup
role during cli operations in order to lookup information in the stack
account.

Along with the ARN of the lookup role, this also exposes a
requiresBootstrapStackVersion property which is set to 8 (the
version the lookup role was given ReadOnlyAccess), and the
bootstrapStackVersionSsmParameter which is needed to lookup the
bootstrap version if a user has renamed the bootstrap stack.

This allows us to first check whether the lookupRole exists and has the
correct permissions prior to using it.

This also updates the diff capability in the CLI (run as part of cdk diff or cdk deploy)
to use this new functionality. It now will try to assume the lookupRole and if it doesn't exist or
if the bootstrap stack version is not valid, then it will fallback to using the deployRole (what it uses
currently).

This PR also updates the forEnvironment function to return whether or not it is returning the
default credentials. This allows the calling function to decide whether or not it actually wants
to use the default credentials.

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

stack artifact This PR exposes information on the bootstrap lookup role on the CloudFormation stack artifact. This enables the CLI to assume the lookup role during cli operations in order to lookup information in the stack account. Along with the ARN of the lookup role, this also exposes a `requiresBootstrapStackVersion` property which is set to `8` (the version the lookup role was given ReadOnlyAccess), and the `bootstrapStackVersionSsmParameter` which is needed to lookup the bootstrap version if a user has renamed the bootstrap stack. This allows us to first check whether the lookupRole exists and has the correct permissions prior to using it.

gitpod-io · 2022-01-05T13:48:47Z

rix0rrr

Can you also add the changes to the synthesizer that will emit the new values, and the changes to the CLI that will consume the new values?

packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts

rix0rrr · 2022-01-05T14:35:39Z

the changes to the CLI that will consume the new values?

At least for cdk diff for example

skinny85 · 2022-01-05T20:44:46Z

the changes to the CLI that will consume the new values?

At least for cdk diff for example

Yes, +1000 this!

Let's re-shape this PR to, instead of a chore:, be a feat(cli): diff now uses the lookup Role for new-style synthesis.

This will allow for a beautiful transition into Calvin's PR for supporting diff in Nested Stacks (#18207).

corymhall · 2022-01-05T20:53:37Z

the changes to the CLI that will consume the new values?

At least for cdk diff for example

I'm actually not sure that the approach that I've currently implemented will work. I am providing the requiresBootstrapStackVersion so that on the consuming side we can check whether or not we can use the lookup role. This creates a chicken/egg issue because we need to first lookup the bootstrap version (with the lookup role) in order to determine whether we can use the lookup role.

The only approach I can think of is to:

Try to assume the lookup role and if it fails fall back to something else (deploy-role, default credentials, etc)
If it can be assumed then lookup the bootstrap version to verify we have a version where the lookup role has the correct permissions.

Alternatively this feels like a big enough change that we could just bump the required bootstrap version in the CLI to 8. @rix0rrr let me know what you think.

rix0rrr · 2022-01-06T12:51:22Z

I am really loath to bump the required bootstrap version. We've had extremely negative feedback to that in the past.

I think your solution is the best one. The most backwards-compatible algorithm I can think of is:

Try to assume the role (if present).
- If it fails, print a warning that we're going to be using current credentials, include instructions on bootstrapping, and proceed anyway.
- If it succeeds, use the role we just assumed to do a post-hoc validation of the bootstrap version, same way we do it for deploy: check SSM parameter value by a parameter name that's in the assembly.
  - In this case we don't have to do the fallback to looking up the default stack name, we should always have an SSM parameter name (it was just there in the previous assembly because of backwards compat concerns).
  - It's probably fine to make the requiredBootstrapVersion optional, I guess I can see situations in which people might not want to have this check at all.

For cdk diff, the "most" backwards compatible would be to, instead of falling back to current credentials, is to fall back to the deploy-role instead.

For cdk watch that is not necessary.

…mplate also updated forEnvironment to return whether or not it is returning default credentials.

rix0rrr

Thanks for the awesome work! Got some style niggles, but this is almost there

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

packages/aws-cdk/lib/api/cloudformation-deployments.ts

rix0rrr · 2022-01-07T12:51:49Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+  /**
+   * Read a version from an SSM parameter, cached
+   */
+  protected async versionFromSsmParameter(sdk: ISDK, parameterName: string): Promise<number> {


Is this code copy/pasted? Can we get rid of the duplication?

In order to remove the duplication I had to make this method a public method on ToolkitInfo. Let me know what you think.

packages/aws-cdk/lib/api/cloudformation-deployments.ts

rix0rrr · 2022-01-07T12:55:02Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+    if (!stack.lookupRole) {
+      throw new Error(`The stack ${stack.displayName} does not have the lookupRole configured`);


Is this worth an error? What is it checking?

Is it checking that the caller of prepareSdkWithLookupRoleFor should not have called this function if there's not a lookup role?

Is it worth requiring every caller of this function to add an additional if ? Can we not just use the "current credentials" and simplify the call path for all callers?

Or is it checking that all stacks have a lookupRole ?

What about old versions of the cloud assembly, or legacy-synthesized stacks?

packages/aws-cdk/lib/api/cloudformation-deployments.ts

rix0rrr · 2022-01-07T13:02:52Z

packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts

      requiresBootstrapStackVersion: MIN_BOOTSTRAP_STACK_VERSION,
      bootstrapStackVersionSsmParameter: this.bootstrapStackVersionSsmParameter,
      additionalDependencies: [artifactId],
+      lookupRole: this.lookupRoleArn ? {


It's probably useful if people can disable this. I can imagine environments in which the lookup role has been neutered, or people prefer using current credentials to access their stacks.

Add a property to switch this off?

Add a disableLookupRole property.

rix0rrr · 2022-01-07T13:12:20Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+  /**
+   * Try to use the bootstrap lookupRole. There are two scenarios that are handled here
+   *  1. The lookup role may not exist (it was added in bootstrap stack version 7)
+   *  2. The lookup role may not have the correct permissions (ReadOnlyAccess was added in


Sheesh, fortunately at version 7 it already has the required SSM parameter permissions... 😅

packages/aws-cdk/test/cdk-toolkit.test.ts

rix0rrr

Conditionally shipped, I'd like to see a minor tweak to the user-facing flag

rix0rrr · 2022-01-10T08:39:45Z

packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts

+  /**
+   * Whether or not to disable use of the lookup role
+   *
+   * @default - false
+   */
+  readonly disableLookupRole?: boolean;


In general, I hate to use flags that are named after a negative. In frail human heads it becomes very hard to reason about (at least in mine 😅).

Can we turn this into something like this (adding some explanation of what the flag does as well):

Suggested change

/**

* Whether or not to disable use of the lookup role

*

* @default - false

*/

readonly disableLookupRole?: boolean;

/**

* Use the bootstrapped lookup role for (read-only) stack operations

*

* Use the lookup role when performing a `cdk diff`. If set to `false`, the

* `deploy role` credentials will be used to perform a `cdk diff.`

*

* Requires bootstrap stack version 8.

*

* @default true

*/

readonly useLookupRoleForStackOperations?: boolean;

mergify · 2022-01-10T14:14:41Z

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

aws-cdk-automation · 2022-01-10T15:15:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 3c97283
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-01-10T15:16:25Z

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

…#18277) This PR exposes information on the bootstrap lookup role on the CloudFormation stack artifact. This enables the CLI to assume the lookup role during cli operations in order to lookup information in the stack account. Along with the ARN of the lookup role, this also exposes a `requiresBootstrapStackVersion` property which is set to `8` (the version the lookup role was given ReadOnlyAccess), and the `bootstrapStackVersionSsmParameter` which is needed to lookup the bootstrap version if a user has renamed the bootstrap stack. This allows us to first check whether the lookupRole exists and has the correct permissions prior to using it. This also updates the `diff` capability in the CLI (run as part of `cdk diff` or `cdk deploy`) to use this new functionality. It now will try to assume the lookupRole and if it doesn't exist or if the bootstrap stack version is not valid, then it will fallback to using the deployRole (what it uses currently). This PR also updates the `forEnvironment` function to return whether or not it is returning the default credentials. This allows the calling function to decide whether or not it actually wants to use the default credentials. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

corymhall requested a review from rix0rrr January 5, 2022 13:48

github-actions bot assigned rix0rrr Jan 5, 2022

github-actions bot added the @aws-cdk/cloud-assembly-schema label Jan 5, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 5, 2022

corymhall mentioned this pull request Jan 5, 2022

feat(cli): watch streams resources' CloudWatch logs to the terminal #18159

Merged

adding docstring

050ad04

rix0rrr requested changes Jan 5, 2022

View reviewed changes

packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts Outdated Show resolved Hide resolved

corymhall added 2 commits January 5, 2022 14:44

emit from default stack synthesizer

fadd38a

moving bootstraprole definition to cloudassembly

440abfb

diff now attempts to use the lookup role to read the current stack te…

2d85755

…mplate also updated forEnvironment to return whether or not it is returning default credentials.

corymhall changed the title ~~chore(cloud-assembly-schema): expose lookupRole on the cloudformation stack artifact~~ feat(cli): diff now uses the lookup Role for new-style synthesis Jan 6, 2022

github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Jan 6, 2022

corymhall requested a review from rix0rrr January 6, 2022 18:15

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Jan 6, 2022

rix0rrr requested changes Jan 7, 2022

View reviewed changes

rix0rrr reviewed Jan 7, 2022

View reviewed changes

packages/aws-cdk/test/cdk-toolkit.test.ts Show resolved Hide resolved

updates based on review comments

51478ba

corymhall requested a review from rix0rrr January 7, 2022 16:25

adding ability to specify an externalId for the lookupRole

13c6145

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jan 10, 2022

rix0rrr approved these changes Jan 10, 2022

View reviewed changes

changing disableLookupRole to useLookupRoleForStackOperations

29c64cd

corymhall removed the pr/do-not-merge This PR should not be merged at this time. label Jan 10, 2022

Merge branch 'master' into corymhall/lookuprole

3c97283

mergify bot merged commit 2256680 into aws:master Jan 10, 2022

Plasma mentioned this pull request Jan 26, 2022

(cli): "To get rid of this warning, please upgrade to boostrap version >= undefined" #18633

Closed

natevick mentioned this pull request Mar 11, 2022

(cdk-diff): >=2.11.0 not authorized to perform: cloudformation:ListStackResources #19351

Closed

		if (!stack.lookupRole) {
		throw new Error(`The stack ${stack.displayName} does not have the lookupRole configured`);

-  /**
-   * Whether or not to disable use of the lookup role
-   *
-   * @default - false
-   */
-  readonly disableLookupRole?: boolean;
+  /**
+   * Use the bootstrapped lookup role for (read-only) stack operations
+   *
+   * Use the lookup role when performing a `cdk diff`. If set to `false`, the
+   * `deploy role` credentials will be used to perform a `cdk diff.`
+   *
+   * Requires bootstrap stack version 8.
+   *
+   * @default true
+   */
+  readonly useLookupRoleForStackOperations?: boolean;

Conversation

corymhall commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Jan 5, 2022

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rix0rrr commented Jan 5, 2022

Uh oh!

skinny85 commented Jan 5, 2022

Uh oh!

corymhall commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rix0rrr commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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!

rix0rrr left a comment

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!

mergify bot commented Jan 10, 2022

Uh oh!

aws-cdk-automation commented Jan 10, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

corymhall commented Jan 5, 2022 •

edited

Loading

corymhall commented Jan 5, 2022 •

edited

Loading

rix0rrr commented Jan 6, 2022 •

edited

Loading