Skip to content

Commit

Permalink
fix: cdk diff prints upgrade bootstrap warning even when current vers…
Browse files Browse the repository at this point in the history
…ion exceeds the recommended version (#29938)

Closes #28888

### Reason for this change

Currently, we just check for if a minimum bootstrap version is needed for a functionality and give a warning. But, we do not check the bootstrap version of the stack itself. This gives a confusing warning for upgrading to a bootstrap version (example: version 8) when the stack bootstrap version is already higher than the recommended version.

### Description of changes

We cannot know successfully what the bootstrap version is in the AWS account without the lookup role. We are getting this warning while trying to assume the lookup role and failing to assume it. 

I am removing upgrade related warnings since we are emitting them without any confirmation of the user's account bootstrap version. This will lead to confusion. Instead, I am trying to make some of the existing error messages more clear.

### Description of how you validated changes

Updated unit tests. 
Will run this through test pipeline.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
vinayak-kukreja authored Apr 26, 2024
1 parent f00a79e commit 28b0080
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 15 deletions.
27 changes: 16 additions & 11 deletions packages/aws-cdk/lib/api/deployments.ts
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})`;

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
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/),
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

0 comments on commit 28b0080

Please sign in to comment.