From 6584df32d640ab2216bdb82303b9ded6564fcb55 Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Tue, 23 Apr 2024 11:57:01 -0700 Subject: [PATCH 1/8] fix: cdk diff prints upgrade bootstrap warning even when current version exceeds the recommended version Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 21 ++++++++++++------- .../aws-cdk/lib/api/environment-resources.ts | 5 +++++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index f407ffd774776..9fc549e319c77 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -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})`; - 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); @@ -549,7 +551,8 @@ 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 }; @@ -557,7 +560,9 @@ export class Deployments { 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); } diff --git a/packages/aws-cdk/lib/api/environment-resources.ts b/packages/aws-cdk/lib/api/environment-resources.ts index 0b262a13ad919..00ed0f7159820 100644 --- a/packages/aws-cdk/lib/api/environment-resources.ts +++ b/packages/aws-cdk/lib/api/environment-resources.ts @@ -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 { From 53833ac9d4520b6ce4d10623198c2cb01c023f4a Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Tue, 23 Apr 2024 13:15:59 -0700 Subject: [PATCH 2/8] make updates Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 9fc549e319c77..9265abd94f486 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -534,15 +534,17 @@ export class Deployments { 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})`; - 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(); + let stackBootstrapVersion; try { + const stackSdk = await this.cachedSdkForEnvironment(resolvedEnvironment, Mode.ForReading, { + assumeRoleArn: arns.lookupRoleArn, + assumeRoleExternalId: stack.lookupRole?.assumeRoleExternalId, + }); + + const envResources = this.environmentResources.for(resolvedEnvironment, stackSdk.sdk); + stackBootstrapVersion = await envResources.getBootstrapVersion(); + // 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); @@ -560,11 +562,12 @@ export class Deployments { 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 && - stack.lookupRole.requiresBootstrapStackVersion > stackBootstrapVersion) { + if (stack.lookupRole && stack.lookupRole.requiresBootstrapStackVersion) { warning(warningMessage); - warning(upgradeMessage); + + if (stackBootstrapVersion && stack.lookupRole.requiresBootstrapStackVersion > stackBootstrapVersion) { + warning(upgradeMessage); + } } throw (e); } From 8b45d2c3ff4cc3db0912092278420387a8b90bbb Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Tue, 23 Apr 2024 13:18:14 -0700 Subject: [PATCH 3/8] make updates Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 9265abd94f486..382e43d5aa9e5 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -562,10 +562,12 @@ export class Deployments { 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) { warning(warningMessage); - if (stackBootstrapVersion && stack.lookupRole.requiresBootstrapStackVersion > stackBootstrapVersion) { + if (stack.lookupRole.requiresBootstrapStackVersion && + stackBootstrapVersion && + stack.lookupRole.requiresBootstrapStackVersion > stackBootstrapVersion) { warning(upgradeMessage); } } From 92571660344b43180aa521249d61cf45a1b76c6a Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Tue, 23 Apr 2024 16:10:36 -0700 Subject: [PATCH 4/8] changing PR direction Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 23 ++++--------------- .../aws-cdk/lib/api/environment-resources.ts | 5 ---- packages/aws-cdk/test/cdk-toolkit.test.ts | 5 +--- 3 files changed, 6 insertions(+), 27 deletions(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 382e43d5aa9e5..4b12d9e77d19b 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -532,9 +532,6 @@ 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})`; - - let stackBootstrapVersion; try { const stackSdk = await this.cachedSdkForEnvironment(resolvedEnvironment, Mode.ForReading, { @@ -543,33 +540,23 @@ export class Deployments { }); const envResources = this.environmentResources.for(resolvedEnvironment, stackSdk.sdk); - stackBootstrapVersion = await envResources.getBootstrapVersion(); // 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); 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 && - stack.lookupRole?.requiresBootstrapStackVersion > stackBootstrapVersion) { - warning(upgradeMessage); + } else if (!stackSdk.didAssumeRole) { + const lookUpRoleExists = stack.lookupRole ? true : false; + warning(`Lookup role ${ lookUpRoleExists ? 'exists but' : 'does not exist, hence'} could not be 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` + // only print out the warnings if the lookupRole exists if (stack.lookupRole) { warning(warningMessage); - - if (stack.lookupRole.requiresBootstrapStackVersion && - stackBootstrapVersion && - stack.lookupRole.requiresBootstrapStackVersion > stackBootstrapVersion) { - warning(upgradeMessage); - } } throw (e); } diff --git a/packages/aws-cdk/lib/api/environment-resources.ts b/packages/aws-cdk/lib/api/environment-resources.ts index 00ed0f7159820..0b262a13ad919 100644 --- a/packages/aws-cdk/lib/api/environment-resources.ts +++ b/packages/aws-cdk/lib/api/environment-resources.ts @@ -171,11 +171,6 @@ export class EnvironmentResources { return { repositoryUri }; } - - public async getBootstrapVersion() { - const bootstrapStack = await this.lookupToolkit(); - return bootstrapStack.version; - } } export class NoBootstrapStackEnvironmentResources extends EnvironmentResources { diff --git a/packages/aws-cdk/test/cdk-toolkit.test.ts b/packages/aws-cdk/test/cdk-toolkit.test.ts index 7b17498d4087a..8dca3ee903d8c 100644 --- a/packages/aws-cdk/test/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cdk-toolkit.test.ts @@ -241,7 +241,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(requestedParameterName!).toEqual('/bootstrap/parameter'); expect(mockForEnvironment.mock.calls.length).toEqual(3); @@ -276,7 +275,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({ @@ -315,7 +313,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({ @@ -350,7 +347,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 could not be assumed. Proceeding with default credentials/), ])); expect(mockCloudExecutable.sdkProvider.sdk.ssm).not.toHaveBeenCalled(); expect(mockForEnvironment.mock.calls.length).toEqual(3); From d5802b36ce50c64270a688558734ae16679b0fa2 Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Thu, 25 Apr 2024 11:22:44 -0700 Subject: [PATCH 5/8] add bootstrap version old thrown error to stderr stream Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 9 +++++++++ packages/aws-cdk/test/cdk-toolkit.test.ts | 1 + 2 files changed, 10 insertions(+) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 4b12d9e77d19b..2ac7c0d928ff1 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -1,3 +1,4 @@ +import { error } from 'console'; import * as cxapi from '@aws-cdk/cx-api'; import * as cdk_assets from 'cdk-assets'; import { AssetManifest, IManifestEntry } from 'cdk-assets'; @@ -534,6 +535,7 @@ export class Deployments { const warningMessage = `Could not assume ${arns.lookupRoleArn}, proceeding anyway.`; 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, @@ -554,10 +556,17 @@ export class Deployments { return { ...stackSdk, resolvedEnvironment, envResources }; } catch (e: any) { debug(e); + // only print out the warnings if the lookupRole exists if (stack.lookupRole) { warning(warningMessage); } + + // 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); } } diff --git a/packages/aws-cdk/test/cdk-toolkit.test.ts b/packages/aws-cdk/test/cdk-toolkit.test.ts index 8dca3ee903d8c..8245342e2b541 100644 --- a/packages/aws-cdk/test/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cdk-toolkit.test.ts @@ -241,6 +241,7 @@ describe('readCurrentTemplate', () => { // THEN expect(flatten(stderrMock.mock.calls)).toEqual(expect.arrayContaining([ expect.stringMatching(/Could not assume bloop-lookup:here:123456789012/), + 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); From a82009979fef2df097d61812672850371f4cb731 Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Thu, 25 Apr 2024 11:26:07 -0700 Subject: [PATCH 6/8] update error import Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 2ac7c0d928ff1..6fb797bd04855 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -1,4 +1,3 @@ -import { error } from 'console'; import * as cxapi from '@aws-cdk/cx-api'; import * as cdk_assets from 'cdk-assets'; import { AssetManifest, IManifestEntry } from 'cdk-assets'; @@ -14,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'; /** From 16f04c8a96ada7759dd9dfc56f09164dfcc7071d Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Thu, 25 Apr 2024 11:29:40 -0700 Subject: [PATCH 7/8] update wording Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 6fb797bd04855..5c0ac2110fed2 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -550,7 +550,7 @@ export class Deployments { } } else if (!stackSdk.didAssumeRole) { const lookUpRoleExists = stack.lookupRole ? true : false; - warning(`Lookup role ${ lookUpRoleExists ? 'exists but' : 'does not exist, hence'} could not be assumed. Proceeding with default credentials.`); + warning(`Lookup role ${ lookUpRoleExists ? 'exists but' : 'does not exist, hence'} was not be assumed. Proceeding with default credentials.`); } return { ...stackSdk, resolvedEnvironment, envResources }; } catch (e: any) { From 24bbb1ad3ddf74ffa691e1ee342fe5abca3699fb Mon Sep 17 00:00:00 2001 From: Vinayak Kukreja Date: Thu, 25 Apr 2024 11:31:02 -0700 Subject: [PATCH 8/8] update wording Signed-off-by: Vinayak Kukreja --- packages/aws-cdk/lib/api/deployments.ts | 2 +- packages/aws-cdk/test/cdk-toolkit.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index 5c0ac2110fed2..ee357e01b525c 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -550,7 +550,7 @@ export class Deployments { } } else if (!stackSdk.didAssumeRole) { const lookUpRoleExists = stack.lookupRole ? true : false; - warning(`Lookup role ${ lookUpRoleExists ? 'exists but' : 'does not exist, hence'} was not be assumed. Proceeding with default credentials.`); + warning(`Lookup role ${ lookUpRoleExists ? 'exists but' : 'does not exist, hence'} was not assumed. Proceeding with default credentials.`); } return { ...stackSdk, resolvedEnvironment, envResources }; } catch (e: any) { diff --git a/packages/aws-cdk/test/cdk-toolkit.test.ts b/packages/aws-cdk/test/cdk-toolkit.test.ts index 8245342e2b541..cb3f9a19bd973 100644 --- a/packages/aws-cdk/test/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cdk-toolkit.test.ts @@ -348,7 +348,7 @@ describe('readCurrentTemplate', () => { // THEN expect(flatten(stderrMock.mock.calls)).toEqual(expect.arrayContaining([ - expect.stringMatching(/Lookup role exists but could not be assumed. Proceeding with default credentials/), + 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);