From 06fb9f93dec448f8536fd19e116f4a2a7b163ee4 Mon Sep 17 00:00:00 2001 From: Calvin Combs <66279577+comcalvi@users.noreply.github.com> Date: Mon, 25 Oct 2021 15:37:13 -0700 Subject: [PATCH] chore(cli): do not hardcode AWS::URLSuffix in hotswapping (#17104) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk/lib/api/aws-auth/sdk.ts | 20 ++++++ packages/aws-cdk/lib/api/deploy-stack.ts | 26 ++----- .../aws-cdk/lib/api/hotswap-deployments.ts | 3 +- .../evaluate-cloudformation-template.ts | 14 +++- .../aws-cdk/test/api/deploy-stack.test.ts | 3 +- .../api/hotswap/hotswap-deployments.test.ts | 69 +++++++++++++++++++ .../test/api/hotswap/hotswap-test-setup.ts | 4 ++ packages/aws-cdk/test/util/mock-sdk.ts | 12 ++++ 8 files changed, 126 insertions(+), 25 deletions(-) diff --git a/packages/aws-cdk/lib/api/aws-auth/sdk.ts b/packages/aws-cdk/lib/api/aws-auth/sdk.ts index 91fcdc2fede7d..c9c64d5e10ec1 100644 --- a/packages/aws-cdk/lib/api/aws-auth/sdk.ts +++ b/packages/aws-cdk/lib/api/aws-auth/sdk.ts @@ -5,6 +5,20 @@ import { cached } from '../../util/functions'; import { AccountAccessKeyCache } from './account-cache'; import { Account } from './sdk-provider'; +// We need to map regions to domain suffixes, and the SDK already has a function to do this. +// It's not part of the public API, but it's also unlikely to go away. +// +// Reuse that function, and add a safety check so we don't accidentally break if they ever +// refactor that away. + +/* eslint-disable @typescript-eslint/no-require-imports */ +const regionUtil = require('aws-sdk/lib/region_config'); +/* eslint-enable @typescript-eslint/no-require-imports */ + +if (!regionUtil.getEndpointSuffix) { + throw new Error('This version of AWS SDK for JS does not have the \'getEndpointSuffix\' function!'); +} + export interface ISDK { /** * The region this SDK has been instantiated for @@ -22,6 +36,8 @@ export interface ISDK { */ currentAccount(): Promise; + getEndpointSuffix(region: string): string; + lambda(): AWS.Lambda; cloudFormation(): AWS.CloudFormation; ec2(): AWS.EC2; @@ -190,6 +206,10 @@ export class SDK implements ISDK { } } + public getEndpointSuffix(region: string): string { + return regionUtil.getEndpointSuffix(region); + } + /** * Return a wrapping object for the underlying service object * diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts index 6b5afe9673ead..ab662d34b517c 100644 --- a/packages/aws-cdk/lib/api/deploy-stack.ts +++ b/packages/aws-cdk/lib/api/deploy-stack.ts @@ -18,20 +18,6 @@ import { } from './util/cloudformation'; import { StackActivityMonitor, StackActivityProgress } from './util/cloudformation/stack-activity-monitor'; -// We need to map regions to domain suffixes, and the SDK already has a function to do this. -// It's not part of the public API, but it's also unlikely to go away. -// -// Reuse that function, and add a safety check so we don't accidentally break if they ever -// refactor that away. - -/* eslint-disable @typescript-eslint/no-require-imports */ -const regionUtil = require('aws-sdk/lib/region_config'); -/* eslint-enable @typescript-eslint/no-require-imports */ - -if (!regionUtil.getEndpointSuffix) { - throw new Error('This version of AWS SDK for JS does not have the \'getEndpointSuffix\' function!'); -} - type TemplateBodyParameter = { TemplateBody?: string TemplateURL?: string @@ -247,8 +233,7 @@ export async function deployStack(options: DeployStackOptions): Promise { + toolkitInfo: ToolkitInfo, + sdk: ISDK): Promise { // If the template has already been uploaded to S3, just use it from there. if (stack.stackTemplateAssetObjectUrl) { - return { TemplateURL: restUrlFromManifest(stack.stackTemplateAssetObjectUrl, resolvedEnvironment) }; + return { TemplateURL: restUrlFromManifest(stack.stackTemplateAssetObjectUrl, resolvedEnvironment, sdk) }; } // Otherwise, pass via API call (if small) or upload here (if large) @@ -553,7 +539,7 @@ function compareTags(a: Tag[], b: Tag[]): boolean { * and reformats s3://.../... urls into S3 REST URLs (which CloudFormation * expects) */ -function restUrlFromManifest(url: string, environment: cxapi.Environment): string { +function restUrlFromManifest(url: string, environment: cxapi.Environment, sdk: ISDK): string { const doNotUseMarker = '**DONOTUSE**'; // This URL may contain placeholders, so still substitute those. url = cxapi.EnvironmentPlaceholders.replace(url, { @@ -576,6 +562,6 @@ function restUrlFromManifest(url: string, environment: cxapi.Environment): strin const bucketName = s3Url[1]; const objectKey = s3Url[2]; - const urlSuffix: string = regionUtil.getEndpointSuffix(environment.region); + const urlSuffix: string = sdk.getEndpointSuffix(environment.region); return `https://s3.${environment.region}.${urlSuffix}/${bucketName}/${objectKey}`; } diff --git a/packages/aws-cdk/lib/api/hotswap-deployments.ts b/packages/aws-cdk/lib/api/hotswap-deployments.ts index 19b8405a7de19..08a22d3944437 100644 --- a/packages/aws-cdk/lib/api/hotswap-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap-deployments.ts @@ -36,8 +36,7 @@ export async function tryHotswapDeployment( account: resolvedEnv.account, region: resolvedEnv.region, partition: (await sdk.currentAccount()).partition, - // ToDo make this better: - urlSuffix: 'amazonaws.com', + urlSuffix: sdk.getEndpointSuffix, listStackResources, }); diff --git a/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts b/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts index 59d8d7df19445..5dc9f948a4250 100644 --- a/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts +++ b/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts @@ -16,7 +16,7 @@ export interface EvaluateCloudFormationTemplateProps { readonly account: string; readonly region: string; readonly partition: string; - readonly urlSuffix: string; + readonly urlSuffix: (region: string) => string; readonly listStackResources: ListStackResources; } @@ -27,6 +27,8 @@ export class EvaluateCloudFormationTemplate { private readonly account: string; private readonly region: string; private readonly partition: string; + private readonly urlSuffix: (region: string) => string; + private cachedUrlSuffix: string | undefined; constructor(props: EvaluateCloudFormationTemplateProps) { this.stackResources = props.listStackResources; @@ -35,12 +37,12 @@ export class EvaluateCloudFormationTemplate { 'AWS::AccountId': props.account, 'AWS::Region': props.region, 'AWS::Partition': props.partition, - 'AWS::URLSuffix': props.urlSuffix, ...props.parameters, }; this.account = props.account; this.region = props.region; this.partition = props.partition; + this.urlSuffix = props.urlSuffix; } public async findPhysicalNameFor(logicalId: string): Promise { @@ -184,6 +186,14 @@ export class EvaluateCloudFormationTemplate { private async findRefTarget(logicalId: string): Promise { // first, check to see if the Ref is a Parameter who's value we have + if (logicalId === 'AWS::URLSuffix') { + if (!this.cachedUrlSuffix) { + this.cachedUrlSuffix = this.urlSuffix(this.region); + } + + return this.cachedUrlSuffix; + } + const parameterTarget = this.context[logicalId]; if (parameterTarget) { return parameterTarget; diff --git a/packages/aws-cdk/test/api/deploy-stack.test.ts b/packages/aws-cdk/test/api/deploy-stack.test.ts index eb551ac528ec4..13e4640ff01ec 100644 --- a/packages/aws-cdk/test/api/deploy-stack.test.ts +++ b/packages/aws-cdk/test/api/deploy-stack.test.ts @@ -29,6 +29,7 @@ const FAKE_STACK_TERMINATION_PROTECTION = testStack({ let sdk: MockSdk; let sdkProvider: MockSdkProvider; let cfnMocks: MockedObject>; + beforeEach(() => { jest.resetAllMocks(); @@ -62,7 +63,7 @@ beforeEach(() => { updateTerminationProtection: jest.fn((_o) => ({ StackId: 'stack-id' })), }; sdk.stubCloudFormation(cfnMocks as any); - + sdk.stubGetEndpointSuffix(() => 'amazonaws.com'); }); function standardDeployStackArguments(): DeployStackOptions { diff --git a/packages/aws-cdk/test/api/hotswap/hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/hotswap-deployments.test.ts index 26a8d08c27290..5d059df860cb8 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-deployments.test.ts @@ -4,13 +4,16 @@ import * as setup from './hotswap-test-setup'; let cfnMockProvider: setup.CfnMockProvider; let mockUpdateLambdaCode: (params: Lambda.Types.UpdateFunctionCodeRequest) => Lambda.Types.FunctionConfiguration; let mockUpdateMachineDefinition: (params: StepFunctions.Types.UpdateStateMachineInput) => StepFunctions.Types.UpdateStateMachineOutput; +let mockGetEndpointSuffix: () => string; beforeEach(() => { cfnMockProvider = setup.setupHotswapTests(); mockUpdateLambdaCode = jest.fn(); mockUpdateMachineDefinition = jest.fn(); + mockGetEndpointSuffix = jest.fn(() => 'amazonaws.com'); cfnMockProvider.setUpdateFunctionCodeMock(mockUpdateLambdaCode); cfnMockProvider.setUpdateStateMachineMock(mockUpdateMachineDefinition); + cfnMockProvider.stubGetEndpointSuffix(mockGetEndpointSuffix); }); test('returns a deployStackResult with noOp=true when it receives an empty set of changes', async () => { @@ -240,3 +243,69 @@ test('can correctly reference AWS::Partition in hotswappable changes', async () S3Key: 'new-key', }); }); + +test('can correctly reference AWS::URLSuffix in hotswappable changes', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + FunctionName: { + 'Fn::Join': ['', [ + 'my-function-', + { Ref: 'AWS::URLSuffix' }, + '-', + { Ref: 'AWS::URLSuffix' }, + ]], + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }, + FunctionName: { + 'Fn::Join': ['', [ + 'my-function-', + { Ref: 'AWS::URLSuffix' }, + '-', + { Ref: 'AWS::URLSuffix' }, + ]], + }, + }, + Metadata: { + 'aws:asset:path': 'new-path', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ + FunctionName: 'my-function-amazonaws.com-amazonaws.com', + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }); + expect(mockGetEndpointSuffix).toHaveBeenCalledTimes(1); +}); diff --git a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts index c41d67752b791..87e06465c61dd 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -97,4 +97,8 @@ export class CfnMockProvider { ): Promise { return deployments.tryHotswapDeployment(this.mockSdkProvider, assetParams, currentCfnStack, stackArtifact); } + + public stubGetEndpointSuffix(stub: () => string) { + this.mockSdkProvider.stubGetEndpointSuffix(stub); + } } diff --git a/packages/aws-cdk/test/util/mock-sdk.ts b/packages/aws-cdk/test/util/mock-sdk.ts index 7b9b4f6fb8b1a..c9afd8cd13baa 100644 --- a/packages/aws-cdk/test/util/mock-sdk.ts +++ b/packages/aws-cdk/test/util/mock-sdk.ts @@ -109,6 +109,10 @@ export class MockSdkProvider extends SdkProvider { public stubStepFunctions(stubs: SyncHandlerSubsetOf) { (this.sdk as any).stepFunctions = jest.fn().mockReturnValue(partialAwsService(stubs)); } + + public stubGetEndpointSuffix(stub: () => string) { + this.sdk.getEndpointSuffix = stub; + } } export class MockSdk implements ISDK { @@ -125,6 +129,7 @@ export class MockSdk implements ISDK { public readonly secretsManager = jest.fn(); public readonly kms = jest.fn(); public readonly stepFunctions = jest.fn(); + public readonly getEndpointSuffix = jest.fn(); public currentAccount(): Promise { return Promise.resolve({ accountId: '123456789012', partition: 'aws' }); @@ -150,6 +155,13 @@ export class MockSdk implements ISDK { public stubSsm(stubs: SyncHandlerSubsetOf) { this.ssm.mockReturnValue(partialAwsService(stubs)); } + + /** + * Replace the getEndpointSuffix client with the given object + */ + public stubGetEndpointSuffix(stub: () => string) { + this.getEndpointSuffix.mockReturnValue(stub()); + } } /**