Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: cdk diff prints upgrade bootstrap warning even when current version exceeds the recommended version #29938

Merged
merged 12 commits into from
Apr 26, 2024
Merged

fix: cdk diff prints upgrade bootstrap warning even when current version exceeds the recommended version #29938

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6584df3
fix: cdk diff prints upgrade bootstrap warning even when current vers…
vinayak-kukreja Apr 23, 2024
53833ac
make updates
vinayak-kukreja Apr 23, 2024
8b45d2c
make updates
vinayak-kukreja Apr 23, 2024
9257166
changing PR direction
vinayak-kukreja Apr 23, 2024
8208671
Merge branch 'main' into vkukreja/doc-update
vinayak-kukreja Apr 23, 2024
931e7a2
Merge branch 'main' into vkukreja/doc-update
vinayak-kukreja Apr 25, 2024
d5802b3
add bootstrap version old thrown error to stderr stream
vinayak-kukreja Apr 25, 2024
a820099
update error import
vinayak-kukreja Apr 25, 2024
16f04c8
update wording
vinayak-kukreja Apr 25, 2024
24bbb1a
update wording
vinayak-kukreja Apr 25, 2024
e79ab0f
Merge branch 'main' into vkukreja/doc-update
vinayak-kukreja Apr 26, 2024
34c6f06
Merge branch 'main' into vkukreja/doc-update
mergify[bot] Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions packages/aws-cdk/lib/api/deployments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,14 +533,16 @@ export class Deployments {
// try to assume the lookup role
const warningMessage = `Could not assume ${arns.lookupRoleArn}, proceeding anyway.`;
const upgradeMessage = `(To get rid of this warning, please upgrade to bootstrap version >= ${stack.lookupRole?.requiresBootstrapStackVersion})`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are unable to assume the lookup role and are not looking for the bootstrap version in SSM of the user's account, then there is no way we are sure about this recommendation. And, we should not just be recommending this without knowing it for a fact that they are on an older version of bootstrap.

We already error out if the user is using an older version of bootstrap than the recommended version.

try {
const stackSdk = await this.cachedSdkForEnvironment(resolvedEnvironment, Mode.ForReading, {
assumeRoleArn: arns.lookupRoleArn,
assumeRoleExternalId: stack.lookupRole?.assumeRoleExternalId,
});

const envResources = this.environmentResources.for(resolvedEnvironment, stackSdk.sdk);
const stackSdk = await this.cachedSdkForEnvironment(resolvedEnvironment, Mode.ForReading, {
assumeRoleArn: arns.lookupRoleArn,
assumeRoleExternalId: stack.lookupRole?.assumeRoleExternalId,
});

const envResources = this.environmentResources.for(resolvedEnvironment, stackSdk.sdk);
const stackBootstrapVersion = await envResources.getBootstrapVersion();

try {
// if we succeed in assuming the lookup role, make sure we have the correct bootstrap stack version
if (stackSdk.didAssumeRole && stack.lookupRole?.bootstrapStackVersionSsmParameter && stack.lookupRole.requiresBootstrapStackVersion) {
const version = await envResources.versionFromSsmParameter(stack.lookupRole.bootstrapStackVersionSsmParameter);
Expand All @@ -549,15 +551,18 @@ export class Deployments {
}
// we may not have assumed the lookup role because one was not provided
// if that is the case then don't print the upgrade warning
} else if (!stackSdk.didAssumeRole && stack.lookupRole?.requiresBootstrapStackVersion) {
} else if (!stackSdk.didAssumeRole && stack.lookupRole?.requiresBootstrapStackVersion &&
stack.lookupRole?.requiresBootstrapStackVersion > stackBootstrapVersion) {
warning(upgradeMessage);
}
return { ...stackSdk, resolvedEnvironment, envResources };
} catch (e: any) {
debug(e);
// only print out the warnings if the lookupRole exists AND there is a required
// bootstrap version, otherwise the warnings will print `undefined`
if (stack.lookupRole && stack.lookupRole.requiresBootstrapStackVersion) {
if (stack.lookupRole &&
stack.lookupRole.requiresBootstrapStackVersion &&
stack.lookupRole.requiresBootstrapStackVersion > stackBootstrapVersion) {
warning(warningMessage);
warning(upgradeMessage);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/aws-cdk/lib/api/environment-resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,11 @@ export class EnvironmentResources {

return { repositoryUri };
}

public async getBootstrapVersion() {
const bootstrapStack = await this.lookupToolkit();
return bootstrapStack.version;
}
}

export class NoBootstrapStackEnvironmentResources extends EnvironmentResources {
Expand Down
Loading