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 all commits
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
27 changes: 16 additions & 11 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 @@ -13,7 +13,7 @@ import { StackActivityProgress } from './util/cloudformation/stack-activity-moni
import { replaceEnvPlaceholders } from './util/placeholders';
import { makeBodyParameterAndUpload } from './util/template-body-parameter';
import { Tag } from '../cdk-toolkit';
import { debug, warning } from '../logging';
import { debug, warning, error } from '../logging';
import { buildAssets, publishAssets, BuildAssetsOptions, PublishAssetsOptions, PublishingAws, EVENT_TO_LOGGER } from '../util/asset-publishing';

/**
Expand Down Expand Up @@ -532,8 +532,9 @@ 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 {
// Trying to assume lookup role and cache the sdk for the environment
const stackSdk = await this.cachedSdkForEnvironment(resolvedEnvironment, Mode.ForReading, {
assumeRoleArn: arns.lookupRoleArn,
assumeRoleExternalId: stack.lookupRole?.assumeRoleExternalId,
Expand All @@ -545,22 +546,26 @@ export class Deployments {
if (stackSdk.didAssumeRole && stack.lookupRole?.bootstrapStackVersionSsmParameter && stack.lookupRole.requiresBootstrapStackVersion) {
const version = await envResources.versionFromSsmParameter(stack.lookupRole.bootstrapStackVersionSsmParameter);
if (version < stack.lookupRole.requiresBootstrapStackVersion) {
throw new Error(`Bootstrap stack version '${stack.lookupRole.requiresBootstrapStackVersion}' is required, found version '${version}'.`);
throw new Error(`Bootstrap stack version '${stack.lookupRole.requiresBootstrapStackVersion}' is required, found version '${version}'. To get rid of this error, please upgrade to bootstrap version >= ${stack.lookupRole.requiresBootstrapStackVersion}`);
}
// 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) {
warning(upgradeMessage);
} else if (!stackSdk.didAssumeRole) {
const lookUpRoleExists = stack.lookupRole ? true : false;
warning(`Lookup role ${ lookUpRoleExists ? 'exists but' : 'does not exist, hence'} was not assumed. Proceeding with default credentials.`);
}
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) {

// only print out the warnings if the lookupRole exists
if (stack.lookupRole) {
warning(warningMessage);
warning(upgradeMessage);
}

// This error should be shown even if debug mode is off
if (e instanceof Error && e.message.includes('Bootstrap stack version')) {
error(e.message);
}

throw (e);
}
}
Expand Down
6 changes: 2 additions & 4 deletions packages/aws-cdk/test/cdk-toolkit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ describe('readCurrentTemplate', () => {
// THEN
expect(flatten(stderrMock.mock.calls)).toEqual(expect.arrayContaining([
expect.stringMatching(/Could not assume bloop-lookup:here:123456789012/),
expect.stringMatching(/please upgrade to bootstrap version >= 5/),
Copy link
Contributor

Choose a reason for hiding this comment

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

This one has an invalid bootstrap version (1 when >= 5 was expected) and an upgrade message should be emitted here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird. Without successfully assuming the lookup role, how will we know what bootstrap version is in SSM. Maybe cached? Taking a look

But, if we are able to assume the role, we do throw in such scenario instead of a warning: https://github.com/aws/aws-cdk/blob/vkukreja/doc-update/packages/aws-cdk/lib/api/deployments.ts#L546-L549

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it was throwing the expected error but since we catch it and do nothing, it will only show up if we have --debug flag enabled. I believe it needs to be shown and hence adding it with error

expect.stringContaining("Bootstrap stack version '5' is required, found version '1'. To get rid of this error, please upgrade to bootstrap version >= 5"),
]));
expect(requestedParameterName!).toEqual('/bootstrap/parameter');
expect(mockForEnvironment.mock.calls.length).toEqual(3);
Expand Down Expand Up @@ -276,7 +276,6 @@ describe('readCurrentTemplate', () => {
// THEN
expect(flatten(stderrMock.mock.calls)).toEqual(expect.arrayContaining([
expect.stringMatching(/Could not assume bloop-lookup:here:123456789012/),
expect.stringMatching(/please upgrade to bootstrap version >= 5/),
]));
expect(mockForEnvironment.mock.calls.length).toEqual(3);
expect(mockForEnvironment.mock.calls[0][2]).toEqual({
Expand Down Expand Up @@ -315,7 +314,6 @@ describe('readCurrentTemplate', () => {
expect(mockCloudExecutable.sdkProvider.sdk.ssm).not.toHaveBeenCalled();
expect(flatten(stderrMock.mock.calls)).toEqual(expect.arrayContaining([
expect.stringMatching(/Could not assume bloop-lookup:here:123456789012/),
expect.stringMatching(/please upgrade to bootstrap version >= 5/),
]));
expect(mockForEnvironment.mock.calls.length).toEqual(3);
expect(mockForEnvironment.mock.calls[0][2]).toEqual({
Expand Down Expand Up @@ -350,7 +348,7 @@ describe('readCurrentTemplate', () => {

// THEN
expect(flatten(stderrMock.mock.calls)).toEqual(expect.arrayContaining([
expect.stringMatching(/please upgrade to bootstrap version >= 5/),
expect.stringMatching(/Lookup role exists but was not assumed. Proceeding with default credentials./),
]));
expect(mockCloudExecutable.sdkProvider.sdk.ssm).not.toHaveBeenCalled();
expect(mockForEnvironment.mock.calls.length).toEqual(3);
Expand Down
Loading