From be597f36fbe306652dbb306416fce575fe2420cb Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Tue, 16 Nov 2021 17:06:27 -0800 Subject: [PATCH 01/27] stash --- packages/aws-cdk/lib/api/aws-auth/sdk.ts | 5 + .../aws-cdk/lib/api/hotswap-deployments.ts | 22 +- packages/aws-cdk/lib/api/hotswap/common.ts | 9 + .../lib/api/hotswap/lambda-functions.ts | 3 + .../lib/api/hotswap/s3-bucket-deployments.ts | 132 +++++ .../test/api/hotswap/hotswap-test-setup.ts | 13 + .../s3-bucket-hotswap-deployments.test.ts | 458 ++++++++++++++++++ packages/aws-cdk/test/util/mock-sdk.ts | 5 + 8 files changed, 646 insertions(+), 1 deletion(-) create mode 100644 packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts create mode 100644 packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts diff --git a/packages/aws-cdk/lib/api/aws-auth/sdk.ts b/packages/aws-cdk/lib/api/aws-auth/sdk.ts index c9c64d5e10ec1..674109c081be5 100644 --- a/packages/aws-cdk/lib/api/aws-auth/sdk.ts +++ b/packages/aws-cdk/lib/api/aws-auth/sdk.ts @@ -50,6 +50,7 @@ export interface ISDK { secretsManager(): AWS.SecretsManager; kms(): AWS.KMS; stepFunctions(): AWS.StepFunctions; + iam(): AWS.IAM; } /** @@ -154,6 +155,10 @@ export class SDK implements ISDK { return this.wrapServiceErrorHandling(new AWS.StepFunctions(this.config)); } + public iam(): AWS.IAM { + return this.wrapServiceErrorHandling(new AWS.IAM(this.config)); + } + public async currentAccount(): Promise { // Get/refresh if necessary before we can access `accessKeyId` await this.forceCredentialRetrieval(); diff --git a/packages/aws-cdk/lib/api/hotswap-deployments.ts b/packages/aws-cdk/lib/api/hotswap-deployments.ts index 08a22d3944437..7e1abc09163cd 100644 --- a/packages/aws-cdk/lib/api/hotswap-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap-deployments.ts @@ -8,6 +8,7 @@ import { isHotswappableEcsServiceChange } from './hotswap/ecs-services'; import { EvaluateCloudFormationTemplate } from './hotswap/evaluate-cloudformation-template'; import { isHotswappableLambdaFunctionChange } from './hotswap/lambda-functions'; import { isHotswappableStateMachineChange } from './hotswap/stepfunctions-state-machines'; +import { isHotswappableS3BucketDeploymentChange } from './hotswap/s3-bucket-deployments'; import { CloudFormationStack } from './util/cloudformation'; /** @@ -66,13 +67,16 @@ async function findAllHotswappableChanges( if (resourceHotswapEvaluation === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT) { foundNonHotswappableChange = true; - } else if (resourceHotswapEvaluation === ChangeHotswapImpact.IRRELEVANT) { + } else if (resourceHotswapEvaluation === ChangeHotswapImpact.IRRELEVANT || resourceHotswapEvaluation === ChangeHotswapImpact.NONE) { // empty 'if' just for flow-aware typing to kick in... } else { + /*eslint-disable*/ + console.log('pushing') promises.push([ isHotswappableLambdaFunctionChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), isHotswappableStateMachineChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), isHotswappableEcsServiceChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), + isHotswappableS3BucketDeploymentChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), ]); } }); @@ -101,6 +105,20 @@ async function findAllHotswappableChanges( continue; } + // this is a roundabout way to continue out of this above for loop + let foundNone = false; + + // NONE means that this change is a false change made with another change + for (const result of hotswapDetectionResults) { + if (result === ChangeHotswapImpact.NONE) { + foundNone = true; + } + } + + if (foundNone) { + continue; + } + // no hotswappable changes found, so any REQUIRES_FULL_DEPLOYMENTs imply a non-hotswappable change for (const result of hotswapDetectionResults) { if (result === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT) { @@ -138,6 +156,8 @@ async function applyAllHotswappableChanges( sdk: ISDK, hotswappableChanges: HotswapOperation[], ): Promise { return Promise.all(hotswappableChanges.map(hotswapOperation => { + /*eslint-disable*/ + console.log('applying out') return hotswapOperation.apply(sdk); })); } diff --git a/packages/aws-cdk/lib/api/hotswap/common.ts b/packages/aws-cdk/lib/api/hotswap/common.ts index 1e482d112aef4..62c3aa3f82410 100644 --- a/packages/aws-cdk/lib/api/hotswap/common.ts +++ b/packages/aws-cdk/lib/api/hotswap/common.ts @@ -30,6 +30,15 @@ export enum ChangeHotswapImpact { * (for example, it's a change to the CDKMetadata resource). */ IRRELEVANT = 'irrelevant', + + /** + * This result means that the given change should not be acted on during a + * hotswap deployment. + * (for example, it's a change to an AWS::IAM::Policy made when a Custom::CDKBucketDeployment + * is being hotswapped). + */ + + NONE = 'none', } export type ChangeHotswapResult = HotswapOperation | ChangeHotswapImpact; diff --git a/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts b/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts index 6aae68738acca..eeb58ca612ae4 100644 --- a/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts +++ b/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts @@ -28,6 +28,9 @@ export async function isHotswappableLambdaFunctionChange( return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } + /*eslint-disable*/ + console.log(functionName) + return new LambdaFunctionHotswapOperation({ physicalName: functionName, code: lambdaCodeChange, diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts new file mode 100644 index 0000000000000..5fc79f15a7418 --- /dev/null +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -0,0 +1,132 @@ +import { ISDK } from '../aws-auth'; +import { /*assetMetadataChanged,*/ ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; +import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; + +/** + * Returns `false` if the change cannot be short-circuited, + * `true` if the change is irrelevant from a short-circuit perspective + * (like a change to CDKMetadata), + * or a LambdaFunctionResource if the change can be short-circuited. + */ +/*eslint-disable*/ +export async function isHotswappableS3BucketDeploymentChange( + logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, +): Promise { + /*const s3BucketDeployment =*/ await isS3BucketDeploymentOnlyChange(change, evaluateCfnTemplate); + + //if (typeof s3BucketDeployment === 'string') { + // return s3BucketDeployment; + //} + if (change.newValue.Type === 'AWS::IAM::Policy') { + return ChangeHotswapImpact.NONE; + } + + logicalId; + + const deployment = { + SourceBucketNames: '', + SourceObjectKeys: '', + DestinationBucketName: '', + }; + + console.log(change.newValue.Type) + console.log(change.newValue.Properties) + + // note that this gives the ARN of the lambda, not the name. This is fine though, because thankfully the invoke sdk call will take either + const functionName = await establishResourcePhysicalName(logicalId, change.newValue.Properties?.ServiceToken, evaluateCfnTemplate); + if (!functionName) { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + + console.log('function name') + console.log(functionName) + + return new S3BucketDeploymentHotswapOperation({ + FunctionName: functionName, + s3BucketDeployment: deployment, + }); +} + +/** + * Returns `ChangeHotswapImpact.IRRELEVANT` if the change is not for a AWS::Lambda::Function, + * but doesn't prevent short-circuiting + * (like a change to CDKMetadata resource), + * `ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT` if the change is to a AWS::Lambda::Function, + * but not only to its Code property, + * or a LambdaFunctionCode if the change is to a AWS::Lambda::Function, + * and only affects its Code property. + */ +async function isS3BucketDeploymentOnlyChange( + change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, +): Promise { + const newResourceType = change.newValue.Type; + evaluateCfnTemplate; + /*eslint-disable*/ + console.log(newResourceType) + + //if (newResourceType !== 'Custom::CDKBucketDeployment') { + // return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + //} + + /* + let s3Bucket = '', s3Key = ''; + let foundCodeDifference = false; + // Make sure only the code in the Lambda function changed + const propertyUpdates = change.propertyUpdates; + for (const updatedPropName in propertyUpdates) { + const updatedProp = propertyUpdates[updatedPropName]; + for (const newPropName in updatedProp.newValue) { + switch (newPropName) { + case 'S3Bucket': + foundCodeDifference = true; + s3Bucket = await evaluateCfnTemplate.evaluateCfnExpression(updatedProp.newValue[newPropName]); + break; + case 'S3Key': + foundCodeDifference = true; + s3Key = await evaluateCfnTemplate.evaluateCfnExpression(updatedProp.newValue[newPropName]); + break; + default: + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + } + } + + return foundCodeDifference + ? { + s3Bucket, + s3Key, + } + : ChangeHotswapImpact.IRRELEVANT; + */ + + return ChangeHotswapImpact.IRRELEVANT; +} + +interface S3BucketDeployment { + SourceBucketNames: any, + SourceObjectKeys: any, + DestinationBucketName: any, + +} + +interface S3BucketDeploymentResource { + FunctionName: string, + s3BucketDeployment: S3BucketDeployment, +} + +class S3BucketDeploymentHotswapOperation implements HotswapOperation { + constructor(private readonly s3BucketDeploymentResource: S3BucketDeploymentResource) { + } + + public async apply(sdk: ISDK): Promise { + console.log('applying invoke') + return sdk.lambda().invoke({ + FunctionName: this.s3BucketDeploymentResource.FunctionName, + Payload: { + SourceBucketNames: this.s3BucketDeploymentResource.s3BucketDeployment.SourceBucketNames, + SourceObjectKeys: this.s3BucketDeploymentResource.s3BucketDeployment.SourceObjectKeys, + DestinationBucketName: this.s3BucketDeploymentResource.s3BucketDeployment.DestinationBucketName, + }, + }).promise(); + } +} 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 87e06465c61dd..353f314cabdce 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -2,6 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api'; import { CloudFormation } from 'aws-sdk'; import * as AWS from 'aws-sdk'; import * as lambda from 'aws-sdk/clients/lambda'; +import * as iam from 'aws-sdk/clients/iam'; import * as stepfunctions from 'aws-sdk/clients/stepfunctions'; import { DeployStackResult } from '../../../lib/api'; import * as deployments from '../../../lib/api/hotswap-deployments'; @@ -87,6 +88,18 @@ export class CfnMockProvider { }); } + public setLambdaInvocationMock(mockInvoke: (input: lambda.InvocationRequest) => lambda.InvocationResponse) { + this.mockSdkProvider.stubLambda({ + invoke: mockInvoke, + }); + } + + public setIAMCreatePolicyVersionMock(mockCreatePolicyVersion: (input: iam.CreatePolicyVersionRequest) => iam.CreatePolicyVersionResponse) { + this.mockSdkProvider.stubIAM({ + createPolicyVersion: mockCreatePolicyVersion, + }); + } + public stubEcs(stubs: SyncHandlerSubsetOf, additionalProperties: { [key: string]: any } = {}): void { this.mockSdkProvider.stubEcs(stubs, additionalProperties); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts new file mode 100644 index 0000000000000..3fdb5e38ec8ae --- /dev/null +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -0,0 +1,458 @@ +import { Lambda } from 'aws-sdk'; +import * as setup from './hotswap-test-setup'; + +let mockLambdaInvoke: (params: Lambda.Types.InvocationRequest) => Lambda.Types.InvocationResponse; +let cfnMockProvider: setup.CfnMockProvider; + +beforeEach(() => { + cfnMockProvider = setup.setupHotswapTests(); + mockLambdaInvoke = jest.fn(); + cfnMockProvider.setLambdaInvocationMock(mockLambdaInvoke); +}); +/*eslint-disable*/ + +/* +test('returns undefined when a new Lambda function is added to the Stack', async () => { + // GIVEN + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); +}); +*/ + +test('calls the lambdaInvoke() API when it receives only an asset difference in an s3 bucket deployment', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Func: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + SourceBucketNames: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + SourceObjectKeys: 'my-function', + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Func: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + SourceBucketNames: { + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }, + SourceObjectKeys: 'my-function', + }, + Metadata: { + 'aws:asset:path': 'new-path', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: ' ', + Payload: { + SourceBucketNames: {}, + SourceObjectKeys: {}, + DestinationBucketName: {}, + }, + }); +}); + + +/* +test("correctly evaluates the function's name when it references a different resource from the template", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Bucket: { + Type: 'AWS::S3::Bucket', + }, + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + FunctionName: { + 'Fn::Join': ['-', [ + 'lambda', + { Ref: 'Bucket' }, + 'function', + ]], + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + setup.pushStackResourceSummaries(setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'mybucket')); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Bucket: { + Type: 'AWS::S3::Bucket', + }, + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }, + FunctionName: { + 'Fn::Join': ['-', [ + 'lambda', + { Ref: 'Bucket' }, + 'function', + ]], + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ + FunctionName: 'lambda-mybucket-function', + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }); + }); + + test("correctly falls back to taking the function's name from the current stack if it can't evaluate it in the template", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Parameters: { + Param1: { Type: 'String' }, + AssetBucketParam: { Type: 'String' }, + }, + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { Ref: 'AssetBucketParam' }, + S3Key: 'current-key', + }, + FunctionName: { Ref: 'Param1' }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-function')); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Parameters: { + Param1: { Type: 'String' }, + AssetBucketParam: { Type: 'String' }, + }, + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { Ref: 'AssetBucketParam' }, + S3Key: 'new-key', + }, + FunctionName: { Ref: 'Param1' }, + }, + Metadata: { + 'aws:asset:path': 'new-path', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact, { AssetBucketParam: 'asset-bucket' }); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ + FunctionName: 'my-function', + S3Bucket: 'asset-bucket', + S3Key: 'new-key', + }); + }); + + test("will not perform a hotswap deployment if it cannot find a Ref target (outside the function's name)", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Parameters: { + Param1: { Type: 'String' }, + }, + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { 'Fn::Sub': '${Param1}' }, + S3Key: 'current-key', + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-func')); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Parameters: { + Param1: { Type: 'String' }, + }, + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { 'Fn::Sub': '${Param1}' }, + S3Key: 'new-key', + }, + }, + Metadata: { + 'aws:asset:path': 'new-path', + }, + }, + }, + }, + }); + + // THEN + await expect(() => + cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), + ).rejects.toThrow(/Parameter or resource 'Param1' could not be found for evaluation/); + }); + + test("will not perform a hotswap deployment if it doesn't know how to handle a specific attribute (outside the function's name)", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Bucket: { + Type: 'AWS::S3::Bucket', + }, + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, + S3Key: 'current-key', + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + setup.pushStackResourceSummaries( + setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-func'), + setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'my-bucket'), + ); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Bucket: { + Type: 'AWS::S3::Bucket', + }, + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, + S3Key: 'new-key', + }, + }, + Metadata: { + 'aws:asset:path': 'new-path', + }, + }, + }, + }, + }); + + // THEN + await expect(() => + cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), + ).rejects.toThrow("We don't support the 'UnknownAttribute' attribute of the 'AWS::S3::Bucket' resource. This is a CDK limitation. Please report it at https://github.com/aws/aws-cdk/issues/new/choose"); + }); + + test('calls the updateLambdaCode() API when it receives a code difference in a Lambda function with no name', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + }, + Metadata: { + 'aws:asset:path': 'current-path', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }, + }, + Metadata: { + 'aws:asset:path': 'current-path', + }, + }, + }, + }, + }); + + // WHEN + setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'mock-function-resource-id')); + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ + FunctionName: 'mock-function-resource-id', + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }); + }); + + test('does not call the updateLambdaCode() API when it receives a change that is not a code difference in a Lambda function', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + PackageType: 'Zip', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + PackageType: 'Image', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockUpdateLambdaCode).not.toHaveBeenCalled(); + }); + + test('does not call the updateLambdaCode() API when a resource with type that is not AWS::Lambda::Function but has the same properties is changed', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Func: { + Type: 'AWS::NotLambda::NotAFunction', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'current-key', + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Func: { + Type: 'AWS::NotLambda::NotAFunction', + Properties: { + Code: { + S3Bucket: 'current-bucket', + S3Key: 'new-key', + }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockUpdateLambdaCode).not.toHaveBeenCalled(); + });*/ \ No newline at end of file diff --git a/packages/aws-cdk/test/util/mock-sdk.ts b/packages/aws-cdk/test/util/mock-sdk.ts index c9afd8cd13baa..efb8378ce36b3 100644 --- a/packages/aws-cdk/test/util/mock-sdk.ts +++ b/packages/aws-cdk/test/util/mock-sdk.ts @@ -110,6 +110,10 @@ export class MockSdkProvider extends SdkProvider { (this.sdk as any).stepFunctions = jest.fn().mockReturnValue(partialAwsService(stubs)); } + public stubIAM(stubs: SyncHandlerSubsetOf) { + (this.sdk as any).iam = jest.fn().mockReturnValue(partialAwsService(stubs)); + } + public stubGetEndpointSuffix(stub: () => string) { this.sdk.getEndpointSuffix = stub; } @@ -129,6 +133,7 @@ export class MockSdk implements ISDK { public readonly secretsManager = jest.fn(); public readonly kms = jest.fn(); public readonly stepFunctions = jest.fn(); + public readonly iam = jest.fn(); public readonly getEndpointSuffix = jest.fn(); public currentAccount(): Promise { From bb2b80f72c574572bc9d5aa85f5b689d52f05401 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 17 Nov 2021 16:51:48 -0800 Subject: [PATCH 02/27] added tests and got a successful hotswap --- .../aws-cdk/lib/api/hotswap-deployments.ts | 9 +- .../lib/api/hotswap/s3-bucket-deployments.ts | 64 ++- .../s3-bucket-hotswap-deployments.test.ts | 445 ++++++++++-------- 3 files changed, 299 insertions(+), 219 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap-deployments.ts b/packages/aws-cdk/lib/api/hotswap-deployments.ts index 7e1abc09163cd..bf307426e67c5 100644 --- a/packages/aws-cdk/lib/api/hotswap-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap-deployments.ts @@ -71,7 +71,7 @@ async function findAllHotswappableChanges( // empty 'if' just for flow-aware typing to kick in... } else { /*eslint-disable*/ - console.log('pushing') + //console.log('pushing') promises.push([ isHotswappableLambdaFunctionChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), isHotswappableStateMachineChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), @@ -111,6 +111,9 @@ async function findAllHotswappableChanges( // NONE means that this change is a false change made with another change for (const result of hotswapDetectionResults) { if (result === ChangeHotswapImpact.NONE) { + console.log('-------------------------') + console.log('-------------------------') + console.log('-------------------------') foundNone = true; } } @@ -122,6 +125,7 @@ async function findAllHotswappableChanges( // no hotswappable changes found, so any REQUIRES_FULL_DEPLOYMENTs imply a non-hotswappable change for (const result of hotswapDetectionResults) { if (result === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT) { + console.log('there') foundNonHotswappableChange = true; } } @@ -138,6 +142,7 @@ async function findAllHotswappableChanges( export function isCandidateForHotswapping(change: cfn_diff.ResourceDifference): HotswappableChangeCandidate | ChangeHotswapImpact { // a resource has been removed OR a resource has been added; we can't short-circuit that change if (!change.newValue || !change.oldValue) { + console.log('here') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } @@ -157,7 +162,7 @@ async function applyAllHotswappableChanges( ): Promise { return Promise.all(hotswappableChanges.map(hotswapOperation => { /*eslint-disable*/ - console.log('applying out') + //console.log('applying out') return hotswapOperation.apply(sdk); })); } diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 5fc79f15a7418..fc1f010b1f394 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -2,6 +2,8 @@ import { ISDK } from '../aws-auth'; import { /*assetMetadataChanged,*/ ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; +export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; + /** * Returns `false` if the change cannot be short-circuited, * `true` if the change is irrelevant from a short-circuit perspective @@ -17,29 +19,43 @@ export async function isHotswappableS3BucketDeploymentChange( //if (typeof s3BucketDeployment === 'string') { // return s3BucketDeployment; //} + if (change.newValue.Type === 'AWS::IAM::Policy') { return ChangeHotswapImpact.NONE; } - logicalId; + if (change.newValue.Type !== 'Custom::CDKBucketDeployment') { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } - const deployment = { - SourceBucketNames: '', - SourceObjectKeys: '', - DestinationBucketName: '', - }; - console.log(change.newValue.Type) - console.log(change.newValue.Properties) + logicalId; + + //console.log(change.newValue.Type) + //console.log(change.newValue.Properties) // note that this gives the ARN of the lambda, not the name. This is fine though, because thankfully the invoke sdk call will take either const functionName = await establishResourcePhysicalName(logicalId, change.newValue.Properties?.ServiceToken, evaluateCfnTemplate); if (!functionName) { + console.log('wherence') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - console.log('function name') - console.log(functionName) + const sourceBucketNames = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceBucketNames) + const sourceObjectKeys = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceObjectKeys); + const destinationBucketName = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketName); + + ////console.log('function name') + //console.log(functionName) + + const deployment = { + SourceBucketNames: sourceBucketNames, + SourceObjectKeys: sourceObjectKeys, + DestinationBucketName: destinationBucketName, + }; + + //console.log('deployment') + //console.log(deployment) return new S3BucketDeploymentHotswapOperation({ FunctionName: functionName, @@ -60,9 +76,10 @@ async function isS3BucketDeploymentOnlyChange( change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, ): Promise { const newResourceType = change.newValue.Type; + newResourceType; evaluateCfnTemplate; /*eslint-disable*/ - console.log(newResourceType) + //console.log(newResourceType) //if (newResourceType !== 'Custom::CDKBucketDeployment') { // return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; @@ -119,14 +136,27 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { } public async apply(sdk: ISDK): Promise { - console.log('applying invoke') + //console.log('applying invoke') + //console.log(this.s3BucketDeploymentResource.s3BucketDeployment) + ////console.log(this.s3BucketDeploymentResource.s3BucketDeployment.SourceObjectKeys) return sdk.lambda().invoke({ FunctionName: this.s3BucketDeploymentResource.FunctionName, - Payload: { - SourceBucketNames: this.s3BucketDeploymentResource.s3BucketDeployment.SourceBucketNames, - SourceObjectKeys: this.s3BucketDeploymentResource.s3BucketDeployment.SourceObjectKeys, - DestinationBucketName: this.s3BucketDeploymentResource.s3BucketDeployment.DestinationBucketName, - }, + // Lambda refuses to take a direct JSON object and requires it to be stringify()'d + Payload: JSON.stringify({ + RequestType: 'Update', // Required by CloudFormation to invoke this Lambda + ResponseURL: REQUIRED_BY_CFN, + PhysicalResourceId: REQUIRED_BY_CFN, + StackId: REQUIRED_BY_CFN, + RequestId: REQUIRED_BY_CFN, + LogicalResourceId: REQUIRED_BY_CFN, + // Required by CloudFormation; this contains the props for the lambda + ResourceProperties: { + SourceBucketNames: this.s3BucketDeploymentResource.s3BucketDeployment.SourceBucketNames, + SourceObjectKeys: this.s3BucketDeploymentResource.s3BucketDeployment.SourceObjectKeys, + DestinationBucketName: this.s3BucketDeploymentResource.s3BucketDeployment.DestinationBucketName, + // TODO: DestinationBucketKeyPrefix: 'web/static'; we should get 'web/static' from the diff + }, + }), }).promise(); } } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 3fdb5e38ec8ae..1dc89118df0b2 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -1,24 +1,34 @@ import { Lambda } from 'aws-sdk'; +import { REQUIRED_BY_CFN } from '../../../lib/api/hotswap/s3-bucket-deployments'; import * as setup from './hotswap-test-setup'; let mockLambdaInvoke: (params: Lambda.Types.InvocationRequest) => Lambda.Types.InvocationResponse; let cfnMockProvider: setup.CfnMockProvider; +let payload: { [key: string]: any}; beforeEach(() => { cfnMockProvider = setup.setupHotswapTests(); mockLambdaInvoke = jest.fn(); cfnMockProvider.setLambdaInvocationMock(mockLambdaInvoke); + + payload = { + RequestType: 'Update', + ResponseURL: REQUIRED_BY_CFN, + PhysicalResourceId: REQUIRED_BY_CFN, + StackId: REQUIRED_BY_CFN, + RequestId: REQUIRED_BY_CFN, + LogicalResourceId: REQUIRED_BY_CFN, + }; }); /*eslint-disable*/ -/* -test('returns undefined when a new Lambda function is added to the Stack', async () => { +test('returns undefined when a new S3 Deployment is added to the Stack', async () => { // GIVEN const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { - Func: { - Type: 'AWS::Lambda::Function', + Deployment: { + Type: 'Custom::CDKBucketDeployment', }, }, }, @@ -30,20 +40,23 @@ test('returns undefined when a new Lambda function is added to the Stack', async // THEN expect(deployStackResult).toBeUndefined(); }); -*/ test('calls the lambdaInvoke() API when it receives only an asset difference in an s3 bucket deployment', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { - Func: { + S3Deployment: { Type: 'Custom::CDKBucketDeployment', Properties: { - SourceBucketNames: { - S3Bucket: 'current-bucket', - S3Key: 'current-key', - }, - SourceObjectKeys: 'my-function', + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-old' + ], + DestinationBucketName: 'dest-bucket', + //DestinationBucketKeyPrefix }, Metadata: { 'aws:asset:path': 'old-path', @@ -54,17 +67,21 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { - Func: { + S3Deployment: { Type: 'Custom::CDKBucketDeployment', Properties: { - SourceBucketNames: { - S3Bucket: 'current-bucket', - S3Key: 'new-key', - }, - SourceObjectKeys: 'my-function', + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-new' + ], + DestinationBucketName: 'dest-bucket', + //DestinationBucketKeyPrefix }, Metadata: { - 'aws:asset:path': 'new-path', + 'aws:asset:path': 'old-path', }, }, }, @@ -76,91 +93,132 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in // THEN expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + }; + expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: ' ', - Payload: { - SourceBucketNames: {}, - SourceObjectKeys: {}, - DestinationBucketName: {}, - }, + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), }); }); - -/* -test("correctly evaluates the function's name when it references a different resource from the template", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - Bucket: { - Type: 'AWS::S3::Bucket', +test("correctly evaluates the service token when it references a lambda found in the template", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Lambda: { + Type: 'AWS::Lambda::Function', + }, + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-old' + ], + DestinationBucketName: 'dest-bucket', + //DestinationBucketKeyPrefix }, - Func: { + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + setup.pushStackResourceSummaries(setup.stackSummaryOf('Lambda', 'AWS::Lambda::Function', 'my-deployment-lambda')); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Lambda: { Type: 'AWS::Lambda::Function', + }, + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'current-key', - }, - FunctionName: { + ServiceToken: { 'Fn::Join': ['-', [ 'lambda', - { Ref: 'Bucket' }, + { Ref: 'Lambda' }, 'function', ]], }, + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-old' + ], + DestinationBucketName: 'dest-bucket', + //DestinationBucketKeyPrefix }, Metadata: { 'aws:asset:path': 'old-path', }, }, }, - }); - setup.pushStackResourceSummaries(setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'mybucket')); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Bucket: { - Type: 'AWS::S3::Bucket', - }, - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'new-key', - }, - FunctionName: { - 'Fn::Join': ['-', [ - 'lambda', - { Ref: 'Bucket' }, - 'function', - ]], - }, - }, - Metadata: { - 'aws:asset:path': 'old-path', - }, - }, - }, - }, - }); + }, + }); - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + }; - // THEN - expect(deployStackResult).not.toBeUndefined(); - expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ - FunctionName: 'lambda-mybucket-function', - S3Bucket: 'current-bucket', - S3Key: 'new-key', - }); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'lambda-my-deployment-lambda-function', + Payload: JSON.stringify(payload), }); +}); - test("correctly falls back to taking the function's name from the current stack if it can't evaluate it in the template", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ + +/* +// TODO +test("correctly falls back to taking the service token from the current stack if it can't evaluate it in the template", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Parameters: { + Param1: { Type: 'String' }, + AssetBucketParam: { Type: 'String' }, + }, + Resources: { + Func: { + Type: 'AWS::Lambda::Function', + Properties: { + Code: { + S3Bucket: { Ref: 'AssetBucketParam' }, + S3Key: 'current-key', + }, + FunctionName: { Ref: 'Param1' }, + }, + Metadata: { + 'aws:asset:path': 'old-path', + }, + }, + }, + }); + setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-function')); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { Parameters: { Param1: { Type: 'String' }, AssetBucketParam: { Type: 'String' }, @@ -171,156 +229,142 @@ test("correctly evaluates the function's name when it references a different res Properties: { Code: { S3Bucket: { Ref: 'AssetBucketParam' }, - S3Key: 'current-key', + S3Key: 'new-key', }, FunctionName: { Ref: 'Param1' }, }, Metadata: { - 'aws:asset:path': 'old-path', - }, - }, - }, - }); - setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-function')); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Parameters: { - Param1: { Type: 'String' }, - AssetBucketParam: { Type: 'String' }, - }, - Resources: { - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: { Ref: 'AssetBucketParam' }, - S3Key: 'new-key', - }, - FunctionName: { Ref: 'Param1' }, - }, - Metadata: { - 'aws:asset:path': 'new-path', - }, + 'aws:asset:path': 'new-path', }, }, }, - }); + }, + }); - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact, { AssetBucketParam: 'asset-bucket' }); + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact, { AssetBucketParam: 'asset-bucket' }); - // THEN - expect(deployStackResult).not.toBeUndefined(); - expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ - FunctionName: 'my-function', - S3Bucket: 'asset-bucket', - S3Key: 'new-key', - }); + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'my-function', + S3Bucket: 'asset-bucket', + S3Key: 'new-key', }); +}); +*/ - test("will not perform a hotswap deployment if it cannot find a Ref target (outside the function's name)", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ +test("will not perform a hotswap deployment if it cannot find a Ref target (outside the function's name)", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Parameters: { + BucketParam: { Type: 'String' }, + }, + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'my-lambda', + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-old' + ], + DestinationBucketName: { 'Fn::Sub': '${BucketParam}' }, + //DestinationBucketKeyPrefix + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { Parameters: { - Param1: { Type: 'String' }, + BucketParam: { Type: 'String' }, }, Resources: { - Func: { - Type: 'AWS::Lambda::Function', + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', Properties: { - Code: { - S3Bucket: { 'Fn::Sub': '${Param1}' }, - S3Key: 'current-key', - }, - }, - Metadata: { - 'aws:asset:path': 'old-path', + ServiceToken: 'my-lambda', + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-new' + ], + DestinationBucketName: { 'Fn::Sub': '${BucketParam}' }, + //DestinationBucketKeyPrefix }, }, }, - }); - setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-func')); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Parameters: { - Param1: { Type: 'String' }, - }, - Resources: { - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: { 'Fn::Sub': '${Param1}' }, - S3Key: 'new-key', - }, - }, - Metadata: { - 'aws:asset:path': 'new-path', - }, - }, + }, + }); + + // THEN + await expect(() => + cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), + ).rejects.toThrow(/Parameter or resource 'BucketParam' could not be found for evaluation/); +}); + +test("will not perform a hotswap deployment if it doesn't know how to handle a specific attribute (outside the function's name)", async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Bucket: { + Type: 'AWS::S3::Bucket', + }, + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'my-lambda', + SourceBucketNames: [ + 'src-bucket' + ], + SourceObjectKeys: [ + 'src-key-old' + ], + DestinationBucketName: 'website-bucket', + //DestinationBucketKeyPrefix }, }, - }); - - // THEN - await expect(() => - cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), - ).rejects.toThrow(/Parameter or resource 'Param1' could not be found for evaluation/); + }, }); - - test("will not perform a hotswap deployment if it doesn't know how to handle a specific attribute (outside the function's name)", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ + setup.pushStackResourceSummaries( + setup.stackSummaryOf('S3Deployment', 'Custom::CDKBucketDeployment', 'my-s3-deployment'), + setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'asset-bucket'), + ); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { Resources: { Bucket: { Type: 'AWS::S3::Bucket', }, - Func: { - Type: 'AWS::Lambda::Function', + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', Properties: { - Code: { - S3Bucket: { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, - S3Key: 'current-key', - }, - }, - Metadata: { - 'aws:asset:path': 'old-path', + ServiceToken: 'my-lambda', + SourceBucketNames: [ + { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, + ], + SourceObjectKeys: [ + 'src-key-old' + ], + DestinationBucketName: 'website-bucket', + //DestinationBucketKeyPrefix }, }, }, - }); - setup.pushStackResourceSummaries( - setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-func'), - setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'my-bucket'), - ); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Bucket: { - Type: 'AWS::S3::Bucket', - }, - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, - S3Key: 'new-key', - }, - }, - Metadata: { - 'aws:asset:path': 'new-path', - }, - }, - }, - }, - }); - - // THEN - await expect(() => - cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), - ).rejects.toThrow("We don't support the 'UnknownAttribute' attribute of the 'AWS::S3::Bucket' resource. This is a CDK limitation. Please report it at https://github.com/aws/aws-cdk/issues/new/choose"); + }, }); + // THEN + await expect(() => + cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), + ).rejects.toThrow("We don't support the 'UnknownAttribute' attribute of the 'AWS::S3::Bucket' resource. This is a CDK limitation. Please report it at https://github.com/aws/aws-cdk/issues/new/choose"); +}); + + /* test('calls the updateLambdaCode() API when it receives a code difference in a Lambda function with no name', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -371,6 +415,7 @@ test("correctly evaluates the function's name when it references a different res }); }); + /* test('does not call the updateLambdaCode() API when it receives a change that is not a code difference in a Lambda function', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ From 3ca6d7c1cd1401d95effa445da2bdf931d5f7b15 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Thu, 18 Nov 2021 17:40:58 -0800 Subject: [PATCH 03/27] added support for all except the IAM policy change referring to a different deployment --- packages/aws-cdk/lib/api/aws-auth/sdk.ts | 5 - .../aws-cdk/lib/api/hotswap-deployments.ts | 16 +- packages/aws-cdk/lib/api/hotswap/common.ts | 1 - .../lib/api/hotswap/lambda-functions.ts | 7 +- .../lib/api/hotswap/s3-bucket-deployments.ts | 132 +---- .../test/api/hotswap/hotswap-test-setup.ts | 7 - .../s3-bucket-hotswap-deployments.test.ts | 466 +++++++++++++----- 7 files changed, 358 insertions(+), 276 deletions(-) diff --git a/packages/aws-cdk/lib/api/aws-auth/sdk.ts b/packages/aws-cdk/lib/api/aws-auth/sdk.ts index 674109c081be5..c9c64d5e10ec1 100644 --- a/packages/aws-cdk/lib/api/aws-auth/sdk.ts +++ b/packages/aws-cdk/lib/api/aws-auth/sdk.ts @@ -50,7 +50,6 @@ export interface ISDK { secretsManager(): AWS.SecretsManager; kms(): AWS.KMS; stepFunctions(): AWS.StepFunctions; - iam(): AWS.IAM; } /** @@ -155,10 +154,6 @@ export class SDK implements ISDK { return this.wrapServiceErrorHandling(new AWS.StepFunctions(this.config)); } - public iam(): AWS.IAM { - return this.wrapServiceErrorHandling(new AWS.IAM(this.config)); - } - public async currentAccount(): Promise { // Get/refresh if necessary before we can access `accessKeyId` await this.forceCredentialRetrieval(); diff --git a/packages/aws-cdk/lib/api/hotswap-deployments.ts b/packages/aws-cdk/lib/api/hotswap-deployments.ts index bf307426e67c5..9beff373c4715 100644 --- a/packages/aws-cdk/lib/api/hotswap-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap-deployments.ts @@ -7,8 +7,8 @@ import { ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, ListStackRe import { isHotswappableEcsServiceChange } from './hotswap/ecs-services'; import { EvaluateCloudFormationTemplate } from './hotswap/evaluate-cloudformation-template'; import { isHotswappableLambdaFunctionChange } from './hotswap/lambda-functions'; -import { isHotswappableStateMachineChange } from './hotswap/stepfunctions-state-machines'; import { isHotswappableS3BucketDeploymentChange } from './hotswap/s3-bucket-deployments'; +import { isHotswappableStateMachineChange } from './hotswap/stepfunctions-state-machines'; import { CloudFormationStack } from './util/cloudformation'; /** @@ -70,8 +70,6 @@ async function findAllHotswappableChanges( } else if (resourceHotswapEvaluation === ChangeHotswapImpact.IRRELEVANT || resourceHotswapEvaluation === ChangeHotswapImpact.NONE) { // empty 'if' just for flow-aware typing to kick in... } else { - /*eslint-disable*/ - //console.log('pushing') promises.push([ isHotswappableLambdaFunctionChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), isHotswappableStateMachineChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate), @@ -105,19 +103,13 @@ async function findAllHotswappableChanges( continue; } - // this is a roundabout way to continue out of this above for loop + // NONE means that this change is an artifact of old-style synthesis and should be ignored let foundNone = false; - - // NONE means that this change is a false change made with another change for (const result of hotswapDetectionResults) { if (result === ChangeHotswapImpact.NONE) { - console.log('-------------------------') - console.log('-------------------------') - console.log('-------------------------') foundNone = true; } } - if (foundNone) { continue; } @@ -125,7 +117,6 @@ async function findAllHotswappableChanges( // no hotswappable changes found, so any REQUIRES_FULL_DEPLOYMENTs imply a non-hotswappable change for (const result of hotswapDetectionResults) { if (result === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT) { - console.log('there') foundNonHotswappableChange = true; } } @@ -142,7 +133,6 @@ async function findAllHotswappableChanges( export function isCandidateForHotswapping(change: cfn_diff.ResourceDifference): HotswappableChangeCandidate | ChangeHotswapImpact { // a resource has been removed OR a resource has been added; we can't short-circuit that change if (!change.newValue || !change.oldValue) { - console.log('here') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } @@ -161,8 +151,6 @@ async function applyAllHotswappableChanges( sdk: ISDK, hotswappableChanges: HotswapOperation[], ): Promise { return Promise.all(hotswappableChanges.map(hotswapOperation => { - /*eslint-disable*/ - //console.log('applying out') return hotswapOperation.apply(sdk); })); } diff --git a/packages/aws-cdk/lib/api/hotswap/common.ts b/packages/aws-cdk/lib/api/hotswap/common.ts index 62c3aa3f82410..dd86cfd7e213a 100644 --- a/packages/aws-cdk/lib/api/hotswap/common.ts +++ b/packages/aws-cdk/lib/api/hotswap/common.ts @@ -37,7 +37,6 @@ export enum ChangeHotswapImpact { * (for example, it's a change to an AWS::IAM::Policy made when a Custom::CDKBucketDeployment * is being hotswapped). */ - NONE = 'none', } diff --git a/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts b/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts index eeb58ca612ae4..ae09cc34e2038 100644 --- a/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts +++ b/packages/aws-cdk/lib/api/hotswap/lambda-functions.ts @@ -3,8 +3,8 @@ import { assetMetadataChanged, ChangeHotswapImpact, ChangeHotswapResult, Hotswap import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; /** - * Returns `false` if the change cannot be short-circuited, - * `true` if the change is irrelevant from a short-circuit perspective + * Returns `ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT` if the change cannot be short-circuited, + * `ChangeHotswapImpact.IRRELEVANT` if the change is irrelevant from a short-circuit perspective * (like a change to CDKMetadata), * or a LambdaFunctionResource if the change can be short-circuited. */ @@ -28,9 +28,6 @@ export async function isHotswappableLambdaFunctionChange( return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - /*eslint-disable*/ - console.log(functionName) - return new LambdaFunctionHotswapOperation({ physicalName: functionName, code: lambdaCodeChange, diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index fc1f010b1f394..0f847120dec15 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -1,25 +1,18 @@ import { ISDK } from '../aws-auth'; -import { /*assetMetadataChanged,*/ ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; +import { ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; -export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; - /** - * Returns `false` if the change cannot be short-circuited, - * `true` if the change is irrelevant from a short-circuit perspective - * (like a change to CDKMetadata), - * or a LambdaFunctionResource if the change can be short-circuited. + * This means that the value is required to exist by CloudFormation's API (or our S3 Bucket Deployment Lambda) + * but the actual value specified is irrelevant */ -/*eslint-disable*/ +export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; + export async function isHotswappableS3BucketDeploymentChange( logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, ): Promise { - /*const s3BucketDeployment =*/ await isS3BucketDeploymentOnlyChange(change, evaluateCfnTemplate); - - //if (typeof s3BucketDeployment === 'string') { - // return s3BucketDeployment; - //} - + // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, + // meaning that the changes made to the Policy are artificial if (change.newValue.Type === 'AWS::IAM::Policy') { return ChangeHotswapImpact.NONE; } @@ -28,133 +21,56 @@ export async function isHotswappableS3BucketDeploymentChange( return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - - logicalId; - - //console.log(change.newValue.Type) - //console.log(change.newValue.Properties) - - // note that this gives the ARN of the lambda, not the name. This is fine though, because thankfully the invoke sdk call will take either + // note that this gives the ARN of the lambda, not the name. This is fine though, the invoke() sdk call will take either const functionName = await establishResourcePhysicalName(logicalId, change.newValue.Properties?.ServiceToken, evaluateCfnTemplate); if (!functionName) { - console.log('wherence') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - const sourceBucketNames = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceBucketNames) + const sourceBucketNames = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceBucketNames); const sourceObjectKeys = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceObjectKeys); const destinationBucketName = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketName); + const destinationBucketKeyPrefix = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketKeyPrefix); - ////console.log('function name') - //console.log(functionName) - - const deployment = { + return new S3BucketDeploymentHotswapOperation({ + FunctionName: functionName, SourceBucketNames: sourceBucketNames, SourceObjectKeys: sourceObjectKeys, DestinationBucketName: destinationBucketName, - }; - - //console.log('deployment') - //console.log(deployment) - - return new S3BucketDeploymentHotswapOperation({ - FunctionName: functionName, - s3BucketDeployment: deployment, + DestinationBucketKeyPrefix: destinationBucketKeyPrefix, }); } -/** - * Returns `ChangeHotswapImpact.IRRELEVANT` if the change is not for a AWS::Lambda::Function, - * but doesn't prevent short-circuiting - * (like a change to CDKMetadata resource), - * `ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT` if the change is to a AWS::Lambda::Function, - * but not only to its Code property, - * or a LambdaFunctionCode if the change is to a AWS::Lambda::Function, - * and only affects its Code property. - */ -async function isS3BucketDeploymentOnlyChange( - change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, -): Promise { - const newResourceType = change.newValue.Type; - newResourceType; - evaluateCfnTemplate; - /*eslint-disable*/ - //console.log(newResourceType) - - //if (newResourceType !== 'Custom::CDKBucketDeployment') { - // return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - //} - - /* - let s3Bucket = '', s3Key = ''; - let foundCodeDifference = false; - // Make sure only the code in the Lambda function changed - const propertyUpdates = change.propertyUpdates; - for (const updatedPropName in propertyUpdates) { - const updatedProp = propertyUpdates[updatedPropName]; - for (const newPropName in updatedProp.newValue) { - switch (newPropName) { - case 'S3Bucket': - foundCodeDifference = true; - s3Bucket = await evaluateCfnTemplate.evaluateCfnExpression(updatedProp.newValue[newPropName]); - break; - case 'S3Key': - foundCodeDifference = true; - s3Key = await evaluateCfnTemplate.evaluateCfnExpression(updatedProp.newValue[newPropName]); - break; - default: - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } - } - } - - return foundCodeDifference - ? { - s3Bucket, - s3Key, - } - : ChangeHotswapImpact.IRRELEVANT; - */ - - return ChangeHotswapImpact.IRRELEVANT; -} - interface S3BucketDeployment { + FunctionName: string, SourceBucketNames: any, SourceObjectKeys: any, DestinationBucketName: any, - -} - -interface S3BucketDeploymentResource { - FunctionName: string, - s3BucketDeployment: S3BucketDeployment, + DestinationBucketKeyPrefix: string, } class S3BucketDeploymentHotswapOperation implements HotswapOperation { - constructor(private readonly s3BucketDeploymentResource: S3BucketDeploymentResource) { + constructor(private readonly s3BucketDeployment: S3BucketDeployment) { } public async apply(sdk: ISDK): Promise { - //console.log('applying invoke') - //console.log(this.s3BucketDeploymentResource.s3BucketDeployment) - ////console.log(this.s3BucketDeploymentResource.s3BucketDeployment.SourceObjectKeys) + const deployment = this.s3BucketDeployment; + return sdk.lambda().invoke({ - FunctionName: this.s3BucketDeploymentResource.FunctionName, + FunctionName: deployment.FunctionName, // Lambda refuses to take a direct JSON object and requires it to be stringify()'d Payload: JSON.stringify({ - RequestType: 'Update', // Required by CloudFormation to invoke this Lambda + RequestType: 'Update', ResponseURL: REQUIRED_BY_CFN, PhysicalResourceId: REQUIRED_BY_CFN, StackId: REQUIRED_BY_CFN, RequestId: REQUIRED_BY_CFN, LogicalResourceId: REQUIRED_BY_CFN, - // Required by CloudFormation; this contains the props for the lambda ResourceProperties: { - SourceBucketNames: this.s3BucketDeploymentResource.s3BucketDeployment.SourceBucketNames, - SourceObjectKeys: this.s3BucketDeploymentResource.s3BucketDeployment.SourceObjectKeys, - DestinationBucketName: this.s3BucketDeploymentResource.s3BucketDeployment.DestinationBucketName, - // TODO: DestinationBucketKeyPrefix: 'web/static'; we should get 'web/static' from the diff + SourceBucketNames: deployment.SourceBucketNames, + SourceObjectKeys: deployment.SourceObjectKeys, + DestinationBucketName: deployment.DestinationBucketName, + DestinationBucketKeyPrefix: deployment.DestinationBucketKeyPrefix, }, }), }).promise(); 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 353f314cabdce..92891c71dcb1b 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -2,7 +2,6 @@ import * as cxapi from '@aws-cdk/cx-api'; import { CloudFormation } from 'aws-sdk'; import * as AWS from 'aws-sdk'; import * as lambda from 'aws-sdk/clients/lambda'; -import * as iam from 'aws-sdk/clients/iam'; import * as stepfunctions from 'aws-sdk/clients/stepfunctions'; import { DeployStackResult } from '../../../lib/api'; import * as deployments from '../../../lib/api/hotswap-deployments'; @@ -94,12 +93,6 @@ export class CfnMockProvider { }); } - public setIAMCreatePolicyVersionMock(mockCreatePolicyVersion: (input: iam.CreatePolicyVersionRequest) => iam.CreatePolicyVersionResponse) { - this.mockSdkProvider.stubIAM({ - createPolicyVersion: mockCreatePolicyVersion, - }); - } - public stubEcs(stubs: SyncHandlerSubsetOf, additionalProperties: { [key: string]: any } = {}): void { this.mockSdkProvider.stubEcs(stubs, additionalProperties); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 1dc89118df0b2..2e4d8e099e946 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -20,7 +20,6 @@ beforeEach(() => { LogicalResourceId: REQUIRED_BY_CFN, }; }); -/*eslint-disable*/ test('returns undefined when a new S3 Deployment is added to the Stack', async () => { // GIVEN @@ -50,16 +49,12 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in Properties: { ServiceToken: 'a-lambda-arn', SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-old' + 'src-key-old', ], DestinationBucketName: 'dest-bucket', - //DestinationBucketKeyPrefix - }, - Metadata: { - 'aws:asset:path': 'old-path', }, }, }, @@ -72,16 +67,12 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in Properties: { ServiceToken: 'a-lambda-arn', SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-new' + 'src-key-new', ], DestinationBucketName: 'dest-bucket', - //DestinationBucketKeyPrefix - }, - Metadata: { - 'aws:asset:path': 'old-path', }, }, }, @@ -109,7 +100,7 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in }); }); -test("correctly evaluates the service token when it references a lambda found in the template", async () => { +test('correctly evaluates the service token when it references a lambda found in the template', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { @@ -121,13 +112,12 @@ test("correctly evaluates the service token when it references a lambda found in Properties: { ServiceToken: 'a-lambda-arn', SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-old' + 'src-key-old', ], DestinationBucketName: 'dest-bucket', - //DestinationBucketKeyPrefix }, Metadata: { 'aws:asset:path': 'old-path', @@ -153,13 +143,12 @@ test("correctly evaluates the service token when it references a lambda found in ]], }, SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-old' + 'src-key-old', ], DestinationBucketName: 'dest-bucket', - //DestinationBucketKeyPrefix }, Metadata: { 'aws:asset:path': 'old-path', @@ -266,13 +255,12 @@ test("will not perform a hotswap deployment if it cannot find a Ref target (outs Properties: { ServiceToken: 'my-lambda', SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-old' + 'src-key-old', ], DestinationBucketName: { 'Fn::Sub': '${BucketParam}' }, - //DestinationBucketKeyPrefix }, }, }, @@ -288,13 +276,12 @@ test("will not perform a hotswap deployment if it cannot find a Ref target (outs Properties: { ServiceToken: 'my-lambda', SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-new' + 'src-key-new', ], DestinationBucketName: { 'Fn::Sub': '${BucketParam}' }, - //DestinationBucketKeyPrefix }, }, }, @@ -319,13 +306,12 @@ test("will not perform a hotswap deployment if it doesn't know how to handle a s Properties: { ServiceToken: 'my-lambda', SourceBucketNames: [ - 'src-bucket' + 'src-bucket', ], SourceObjectKeys: [ - 'src-key-old' + 'src-key-old', ], DestinationBucketName: 'website-bucket', - //DestinationBucketKeyPrefix }, }, }, @@ -348,10 +334,9 @@ test("will not perform a hotswap deployment if it doesn't know how to handle a s { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, ], SourceObjectKeys: [ - 'src-key-old' + 'src-key-old', ], DestinationBucketName: 'website-bucket', - //DestinationBucketKeyPrefix }, }, }, @@ -364,140 +349,349 @@ test("will not perform a hotswap deployment if it doesn't know how to handle a s ).rejects.toThrow("We don't support the 'UnknownAttribute' attribute of the 'AWS::S3::Bucket' resource. This is a CDK limitation. Please report it at https://github.com/aws/aws-cdk/issues/new/choose"); }); - /* - test('calls the updateLambdaCode() API when it receives a code difference in a Lambda function with no name', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ +test('does not call the invoke() API when a resource with type that is not Custom::CDKBucketDeployment but has the same properties is changed', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + S3Deployment: { + Type: 'Custom::NotCDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { Resources: { - Func: { - Type: 'AWS::Lambda::Function', + S3Deployment: { + Type: 'Custom::NotCDKBucketDeployment', Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'current-key', - }, - }, - Metadata: { - 'aws:asset:path': 'current-path', + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', }, }, }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'new-key', + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); +}); + +test('calls the lambdaInvoke() API when it receives an asset difference in an s3 bucket deployment and an IAM Policy difference using old-style synthesis', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Effect: 'Allow', + Resource: [ + { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':s3:::', + { + Ref: 'bucket-name', + }, + ], + ], + }, + ], }, + ], + }, + }, + }, + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket-old', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Effect: 'Allow', + Resource: [ + { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':s3:::', + { + Ref: 'new-bucket-name', + }, + ], + ], + }, + ], + }, + ], }, - Metadata: { - 'aws:asset:path': 'current-path', - }, + }, + }, + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', }, }, }, - }); - - // WHEN - setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'mock-function-resource-id')); - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).not.toBeUndefined(); - expect(mockUpdateLambdaCode).toHaveBeenCalledWith({ - FunctionName: 'mock-function-resource-id', - S3Bucket: 'current-bucket', - S3Key: 'new-key', - }); + }, }); - /* - test('does not call the updateLambdaCode() API when it receives a change that is not a code difference in a Lambda function', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), + }); +}); + +test('can use the DestinationBucketKeyPrefix property', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-old-prefix', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { Resources: { - Func: { - Type: 'AWS::Lambda::Function', + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'current-key', - }, - PackageType: 'Zip', + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-new-prefix', }, }, }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'current-key', + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-new-prefix', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), + }); +}); + +// TODO +test('does not call the lambdaInvoke() API when the difference in the s3 deployment is not referred to in the IAM policy change', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Effect: 'Allow', + Resource: [ + { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':s3:::', + { + Ref: 'bucket-name', + }, + ], + ], + }, + ], }, - PackageType: 'Image', - }, + ], }, }, }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).toBeUndefined(); - expect(mockUpdateLambdaCode).not.toHaveBeenCalled(); + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket-old', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + }, + }, + }, }); - - test('does not call the updateLambdaCode() API when a resource with type that is not AWS::Lambda::Function but has the same properties is changed', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { Resources: { - Func: { - Type: 'AWS::NotLambda::NotAFunction', + Policy: { + Type: 'AWS::IAM::Policy', Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'current-key', + PolicyDocument: { + Statement: [ + { + Effect: 'Allow', + Resource: [ + { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':s3:::', + { + Ref: 'new-bucket-name', + }, + ], + ], + }, + ], + }, + ], }, }, - Metadata: { - 'aws:asset:path': 'old-path', - }, }, - }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Func: { - Type: 'AWS::NotLambda::NotAFunction', - Properties: { - Code: { - S3Bucket: 'current-bucket', - S3Key: 'new-key', - }, - }, - Metadata: { - 'aws:asset:path': 'old-path', - }, + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', }, }, }, - }); + }, + }); - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - // THEN - expect(deployStackResult).toBeUndefined(); - expect(mockUpdateLambdaCode).not.toHaveBeenCalled(); - });*/ \ No newline at end of file + // THEN + expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), + }); +}); \ No newline at end of file From f37bf6dec2686642bc8fd700cd69c3039be128ce Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Fri, 19 Nov 2021 14:14:30 -0800 Subject: [PATCH 04/27] IAM Policy is now bound to the S3 deployment change --- .../lib/api/hotswap/s3-bucket-deployments.ts | 35 +++- .../s3-bucket-hotswap-deployments.test.ts | 197 ++++-------------- 2 files changed, 72 insertions(+), 160 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 0f847120dec15..fda24e9ce78fe 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -8,13 +8,28 @@ import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-templa */ export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; +let resourceList:any[] = []; + export async function isHotswappableS3BucketDeploymentChange( logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, ): Promise { // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, // meaning that the changes made to the Policy are artificial if (change.newValue.Type === 'AWS::IAM::Policy') { - return ChangeHotswapImpact.NONE; + /*eslint-disable*/ + const statements = change.newValue.Properties?.PolicyDocument.Statement; + for (const statement of statements) { + if (resourceList.length === 0) { + resourceList.push(statement.Resource); + return ChangeHotswapImpact.NONE; + } else if (evaluateCfnTemplate.evaluateCfnExpression(statement.Resource) === resourceList[0]) { + return ChangeHotswapImpact.NONE; + } + } + + if (resourceList.length > 0) { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } } if (change.newValue.Type !== 'Custom::CDKBucketDeployment') { @@ -32,13 +47,19 @@ export async function isHotswappableS3BucketDeploymentChange( const destinationBucketName = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketName); const destinationBucketKeyPrefix = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketKeyPrefix); + if (resourceList === []) { + resourceList = [destinationBucketName]; + } else if (destinationBucketName !in resourceList) { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + return new S3BucketDeploymentHotswapOperation({ - FunctionName: functionName, - SourceBucketNames: sourceBucketNames, - SourceObjectKeys: sourceObjectKeys, - DestinationBucketName: destinationBucketName, - DestinationBucketKeyPrefix: destinationBucketKeyPrefix, - }); + FunctionName: functionName, + SourceBucketNames: sourceBucketNames, + SourceObjectKeys: sourceObjectKeys, + DestinationBucketName: destinationBucketName, + DestinationBucketKeyPrefix: destinationBucketKeyPrefix, + }); } interface S3BucketDeployment { diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 2e4d8e099e946..0a47a287eab0e 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -179,70 +179,6 @@ test('correctly evaluates the service token when it references a lambda found in }); }); - -/* -// TODO -test("correctly falls back to taking the service token from the current stack if it can't evaluate it in the template", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Parameters: { - Param1: { Type: 'String' }, - AssetBucketParam: { Type: 'String' }, - }, - Resources: { - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: { Ref: 'AssetBucketParam' }, - S3Key: 'current-key', - }, - FunctionName: { Ref: 'Param1' }, - }, - Metadata: { - 'aws:asset:path': 'old-path', - }, - }, - }, - }); - setup.pushStackResourceSummaries(setup.stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-function')); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Parameters: { - Param1: { Type: 'String' }, - AssetBucketParam: { Type: 'String' }, - }, - Resources: { - Func: { - Type: 'AWS::Lambda::Function', - Properties: { - Code: { - S3Bucket: { Ref: 'AssetBucketParam' }, - S3Key: 'new-key', - }, - FunctionName: { Ref: 'Param1' }, - }, - Metadata: { - 'aws:asset:path': 'new-path', - }, - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact, { AssetBucketParam: 'asset-bucket' }); - - // THEN - expect(deployStackResult).not.toBeUndefined(); - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'my-function', - S3Bucket: 'asset-bucket', - S3Key: 'new-key', - }); -}); -*/ - test("will not perform a hotswap deployment if it cannot find a Ref target (outside the function's name)", async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -406,24 +342,16 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 PolicyDocument: { Statement: [ { - Effect: 'Allow', - Resource: [ - { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':s3:::', - { - Ref: 'bucket-name', - }, - ], - ], - }, + Action: [ + 's3:GetObject*', ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, }, ], }, @@ -439,7 +367,7 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 SourceObjectKeys: [ 'src-key-old', ], - DestinationBucketName: 'dest-bucket', + DestinationBucketName: 'WebsiteBucketOld', }, }, }, @@ -453,24 +381,16 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 PolicyDocument: { Statement: [ { - Effect: 'Allow', - Resource: [ - { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':s3:::', - { - Ref: 'new-bucket-name', - }, - ], - ], - }, + Action: [ + 's3:GetObject*', ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, }, ], }, @@ -486,7 +406,7 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 SourceObjectKeys: [ 'src-key-new', ], - DestinationBucketName: 'dest-bucket', + DestinationBucketName: 'WebsiteBucketNew', }, }, }, @@ -505,7 +425,7 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 SourceObjectKeys: [ 'src-key-new', ], - DestinationBucketName: 'dest-bucket', + DestinationBucketName: 'WebsiteBucketNew', }; expect(mockLambdaInvoke).toHaveBeenCalledWith({ @@ -577,7 +497,7 @@ test('can use the DestinationBucketKeyPrefix property', async () => { }); }); -// TODO + test('does not call the lambdaInvoke() API when the difference in the s3 deployment is not referred to in the IAM policy change', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -588,24 +508,16 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym PolicyDocument: { Statement: [ { - Effect: 'Allow', - Resource: [ - { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':s3:::', - { - Ref: 'bucket-name', - }, - ], - ], - }, + Action: [ + 's3:GetObject*', ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, }, ], }, @@ -621,7 +533,7 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym SourceObjectKeys: [ 'src-key-old', ], - DestinationBucketName: 'dest-bucket', + DestinationBucketName: 'DifferentBucketOld', }, }, }, @@ -635,24 +547,16 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym PolicyDocument: { Statement: [ { - Effect: 'Allow', - Resource: [ - { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':s3:::', - { - Ref: 'new-bucket-name', - }, - ], - ], - }, + Action: [ + 's3:GetObject*', ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, }, ], }, @@ -668,7 +572,7 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym SourceObjectKeys: [ 'src-key-new', ], - DestinationBucketName: 'dest-bucket', + DestinationBucketName: 'DifferentBucketNew', }, }, }, @@ -679,19 +583,6 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); // THEN - expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - }; - - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), - }); + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); \ No newline at end of file From f9eb43daa8a883c44210c91b9ba7a25bac1f3024 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 22 Nov 2021 16:58:10 -0800 Subject: [PATCH 05/27] all tests passing --- .../evaluate-cloudformation-template.ts | 5 + .../lib/api/hotswap/s3-bucket-deployments.ts | 66 +- .../test/api/hotswap/hotswap-test-setup.ts | 3 +- .../s3-bucket-hotswap-deployments.test.ts | 587 +++++++++++++++--- 4 files changed, 557 insertions(+), 104 deletions(-) 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 5dc9f948a4250..fe6f615bc7cf5 100644 --- a/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts +++ b/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts @@ -50,6 +50,11 @@ export class EvaluateCloudFormationTemplate { return stackResources.find(sr => sr.LogicalResourceId === logicalId)?.PhysicalResourceId; } + public async findLogicallNameFor(physicalId: string): Promise { + const stackResources = await this.stackResources.listStackResources(); + return stackResources.find(sr => sr.PhysicalResourceId === physicalId)?.LogicalResourceId; + } + public findReferencesTo(logicalId: string): Array { const ret = new Array(); for (const [resourceLogicalId, resourceDef] of Object.entries(this.template?.Resources ?? {})) { diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index fda24e9ce78fe..f316f67c1bdad 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -8,27 +8,59 @@ import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-templa */ export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; -let resourceList:any[] = []; - export async function isHotswappableS3BucketDeploymentChange( logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, ): Promise { // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, // meaning that the changes made to the Policy are artificial + + // TODO: there's a case where this should break with two policies and two s3 deployments + + // broken case: two policies, two s3 deployments, both of them are valid changes. + // with one pairing referring to bucketA, the other referring to bucketB + // resourceList adds bucketA + // S3 deployment that refs bucketB is processed next + // now that s3 deployment checks if resourceList is empty + // its not + // so it requires full deployment + // wow the newest test seems to catch this case + + // ok not quite it actually catches a different case: + // the two IAM policies are evaluated first, so buketA is placed and + // policy B sees bucketA and balks + + // solution: completely rework this broken code + // all of the IAM::Policy handling should be handled right here, not later + // for each IAM change, compute the diff between change.oldValue and change.newValue + // that diff is the updated policy doc resource + // find all references to the updated resource + // if all of those references are s3 deployments, return NONE + // otherwise, return REQUIRES_FULL_DEPLOYMENT + + // better solution: find the Role that the policy applies to and that the s3 deployment references + /*eslint-disable*/ if (change.newValue.Type === 'AWS::IAM::Policy') { - /*eslint-disable*/ - const statements = change.newValue.Properties?.PolicyDocument.Statement; - for (const statement of statements) { - if (resourceList.length === 0) { - resourceList.push(statement.Resource); - return ChangeHotswapImpact.NONE; - } else if (evaluateCfnTemplate.evaluateCfnExpression(statement.Resource) === resourceList[0]) { - return ChangeHotswapImpact.NONE; + const roles = change.newValue.Properties?.Roles; + for (const role of roles) { + const roleLogicalName = await evaluateCfnTemplate.findLogicallNameFor(await evaluateCfnTemplate.evaluateCfnExpression(role)); + if (!roleLogicalName) { + // TODO: what should be done here? prolly throw an error since a ref target was not found + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - } - if (resourceList.length > 0) { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalName); + + for (const roleRef of roleRefs) { + if (roleRef.Type === 'AWS::Lambda::Function') { + const lambdaRefs = evaluateCfnTemplate.findReferencesTo(roleRef.LogicalId); + for (const lambdaRef of lambdaRefs) { + if (lambdaRef.Type === 'Custom::CDKBucketDeployment') { + // TODO: likely need extra validation here + return ChangeHotswapImpact.NONE; + } + } + } + } } } @@ -47,19 +79,13 @@ export async function isHotswappableS3BucketDeploymentChange( const destinationBucketName = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketName); const destinationBucketKeyPrefix = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketKeyPrefix); - if (resourceList === []) { - resourceList = [destinationBucketName]; - } else if (destinationBucketName !in resourceList) { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } - return new S3BucketDeploymentHotswapOperation({ FunctionName: functionName, SourceBucketNames: sourceBucketNames, SourceObjectKeys: sourceObjectKeys, DestinationBucketName: destinationBucketName, DestinationBucketKeyPrefix: destinationBucketKeyPrefix, - }); + }); } interface S3BucketDeployment { 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 92891c71dcb1b..54f8731560afb 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -41,7 +41,8 @@ export function pushStackResourceSummaries(...items: CloudFormation.StackResourc currentCfnStackResources.push(...items); } -export function setCurrentCfnStackTemplate(template: Template) { +export function setCurrentCfnStackTemplate(_template: Template) { + let template = JSON.parse(JSON.stringify(_template)); currentCfnStack.setTemplate(template); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 0a47a287eab0e..7ebe18ab96b26 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -332,7 +332,71 @@ test('does not call the invoke() API when a resource with type that is not Custo expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); -test('calls the lambdaInvoke() API when it receives an asset difference in an s3 bucket deployment and an IAM Policy difference using old-style synthesis', async () => { +test('can use the DestinationBucketKeyPrefix property', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-old-prefix', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-new-prefix', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-new-prefix', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), + }); +}); + +/* +test('does not call the lambdaInvoke() API when the difference in the s3 deployment is not referred to in the IAM policy change', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { @@ -367,7 +431,7 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 SourceObjectKeys: [ 'src-key-old', ], - DestinationBucketName: 'WebsiteBucketOld', + DestinationBucketName: 'DifferentBucketOld', }, }, }, @@ -406,7 +470,7 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 SourceObjectKeys: [ 'src-key-new', ], - DestinationBucketName: 'WebsiteBucketNew', + DestinationBucketName: 'DifferentBucketNew', }, }, }, @@ -417,92 +481,55 @@ test('calls the lambdaInvoke() API when it receives an asset difference in an s3 const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); // THEN - expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'WebsiteBucketNew', - }; - - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), - }); + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); -test('can use the DestinationBucketKeyPrefix property', async () => { +// todo: it's not the website bucket they share, it's the sourcebucket +test('calls the lambdaInvoke() API when it receives an asset difference in two different s3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { - S3Deployment: { + PolicyA: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { + Ref: 'ServiceRole', + }, + ], + PolicyDocument: { + Statement: [ + { + Action: [ + 's3:GetObject*', + ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOldA', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + S3DeploymentA: { Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', SourceBucketNames: [ - 'src-bucket', + 'src-bucket-old', ], SourceObjectKeys: [ 'src-key-old', ], - DestinationBucketName: 'dest-bucket', - DestinationBucketKeyPrefix: 'my-key/some-old-prefix', + DestinationBucketName: 'WebsiteBucketOldA', }, }, - }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - DestinationBucketKeyPrefix: 'my-key/some-new-prefix', - }, - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - DestinationBucketKeyPrefix: 'my-key/some-new-prefix', - }; - - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), - }); -}); - - -test('does not call the lambdaInvoke() API when the difference in the s3 deployment is not referred to in the IAM policy change', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - Policy: { + PolicyB: { Type: 'AWS::IAM::Policy', Properties: { PolicyDocument: { @@ -514,7 +541,7 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym Effect: 'Allow', Resource: { 'Fn::GetAtt': [ - 'WebsiteBucketOld', + 'WebsiteBucketOldB', 'Arn', ], }, @@ -523,7 +550,7 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym }, }, }, - S3Deployment: { + S3DeploymentB: { Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', @@ -533,15 +560,19 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym SourceObjectKeys: [ 'src-key-old', ], - DestinationBucketName: 'DifferentBucketOld', + DestinationBucketName: 'WebsiteBucketOldB', }, }, }, }); + setup.pushStackResourceSummaries( + setup.stackSummaryOf('WebsiteBucketNewA', 'AWS::S3::Bucket', 'bucket-A'), + setup.stackSummaryOf('WebsiteBucketNewB', 'AWS::S3::Bucket', 'bucket-B'), + ); const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { - Policy: { + PolicyA: { Type: 'AWS::IAM::Policy', Properties: { PolicyDocument: { @@ -553,7 +584,7 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym Effect: 'Allow', Resource: { 'Fn::GetAtt': [ - 'WebsiteBucketNew', + 'WebsiteBucketNewA', 'Arn', ], }, @@ -562,7 +593,7 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym }, }, }, - S3Deployment: { + S3DeploymentA: { Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', @@ -572,7 +603,41 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym SourceObjectKeys: [ 'src-key-new', ], - DestinationBucketName: 'DifferentBucketNew', + DestinationBucketName: 'WebsiteBucketNewA', + }, + }, + PolicyB: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Action: [ + 's3:GetObject*', + ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNewB', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + S3DeploymentB: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNewB', }, }, }, @@ -583,6 +648,362 @@ test('does not call the lambdaInvoke() API when the difference in the s3 deploym const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); // THEN - expect(deployStackResult).toBeUndefined(); - expect(mockLambdaInvoke).not.toHaveBeenCalled(); + expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNewA', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), + }); + + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNewB', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'a-lambda-arn', + Payload: JSON.stringify(payload), + }); +}); +*/ + +describe('old-style synthesis', () => { + let serviceRole: any; + let policy: any; + let policy2: any; + let deploymentLambda: any; + let s3Deployment: any; + beforeEach(() => { + serviceRole = { + Type: 'AWS::IAM::Role', + Properties: { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { + Service: 'lambda.amazonaws.com', + }, + }, + ], + Version: '2012-10-17', + }, + }, + }; + + policy = { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: [ + 's3:GetObject*', + ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }; + + policy2 = { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: [ + 's3:GetObject*', + ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }; + + deploymentLambda = { + Type: 'AWS::Lambda::Function', + Role: { + 'Fn::GetAtt': [ + 'ServiceRole', + 'Arn', + ], + }, + }; + + s3Deployment = { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { + 'Fn::GetAtt': [ + 'S3DeploymentLambda', + 'Arn', + ], + }, + SourceBucketNames: [ + 'src-bucket-old', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'WebsiteBucketOld', + }, + }; + + setup.pushStackResourceSummaries( + setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), + setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), + ); + }); + + test('calls the lambdaInvoke() API when it receives an asset difference in an s3 bucket deployment and an IAM Policy difference using old-style synthesis', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + ServiceRole: serviceRole, + Policy: policy, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + }, + }); + + s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; + s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; + s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; + + /*eslint-disable*/ + + policy.Properties.PolicyDocument.Statement.Resource = { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }; + /*eslint-disable*/ + + console.log(s3Deployment); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + ServiceRole: serviceRole, + Policy: policy, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNew', + }; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', + Payload: JSON.stringify(payload), + }); + }); + + test('does not call the lambdaInvoke() API when the difference in the s3 deployment is referred to in one IAM policy change but not another', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + Policy2: policy2, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + }, + }); + setup.pushStackResourceSummaries( + setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), + setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), + ); + + policy.Properties.PolicyDocument.Statement.Resource = { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }; + + policy2.Properties.PolicyDocument.Statement.Resource = { + 'Fn::GetAtt': [ + 'DifferentBucketNew', + 'Arn', + ], + }; + policy2.Properties.Roles = [ + 'different-role', + ] + + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + Policy2: policy2, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); + }); + + test('calls the lambdaInvoke() API when it receives an asset difference in two s3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { + // GIVEN + let s3Deployment2 = { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { + 'Fn::GetAtt': [ + 'S3DeploymentLambda', + 'Arn', + ], + }, + SourceBucketNames: [ + 'src-bucket-old', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'DifferentBucketOld', + }, + }; + + setup.setCurrentCfnStackTemplate({ + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + Policy2: policy2, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + S3Deployment2: s3Deployment2, + }, + }); + setup.pushStackResourceSummaries( + setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), + setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), + ); + + policy.Properties.PolicyDocument.Statement.Resource = { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }; + + policy2.Properties.PolicyDocument.Statement.Resource = { + 'Fn::GetAtt': [ + 'DifferentBucketNew', + 'Arn', + ], + }; + //policy2.Properties.Roles = [ + // 'different-role', + //] + + + s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; + s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; + s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; + + s3Deployment2.Properties.SourceBucketNames = ['src-bucket-new']; + s3Deployment2.Properties.SourceObjectKeys = ['src-key-new']; + s3Deployment2.Properties.DestinationBucketName = 'DifferentBucketNew'; + + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + Policy2: policy2, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + S3Deployment2: s3Deployment2, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + + payload.ResourceProperties = { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNew', + }; + + let payload2 = JSON.parse(JSON.stringify(payload)); + payload2.ResourceProperties.DestinationBucketName = 'DifferentBucketNew';; + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', + Payload: JSON.stringify(payload), + }); + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', + Payload: JSON.stringify(payload2), + }); + }); }); \ No newline at end of file From f2675df02cc2a36d1695934f77ca0089bf8dc197 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 22 Nov 2021 17:14:38 -0800 Subject: [PATCH 06/27] removed NONE, added an empty hotswap operation --- .../aws-cdk/lib/api/hotswap-deployments.ts | 13 +- packages/aws-cdk/lib/api/hotswap/common.ts | 8 - .../lib/api/hotswap/s3-bucket-deployments.ts | 52 +-- .../test/api/hotswap/hotswap-test-setup.ts | 2 +- .../s3-bucket-hotswap-deployments.test.ts | 313 +----------------- 5 files changed, 28 insertions(+), 360 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap-deployments.ts b/packages/aws-cdk/lib/api/hotswap-deployments.ts index 9beff373c4715..e57d4c0fe1f33 100644 --- a/packages/aws-cdk/lib/api/hotswap-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap-deployments.ts @@ -67,7 +67,7 @@ async function findAllHotswappableChanges( if (resourceHotswapEvaluation === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT) { foundNonHotswappableChange = true; - } else if (resourceHotswapEvaluation === ChangeHotswapImpact.IRRELEVANT || resourceHotswapEvaluation === ChangeHotswapImpact.NONE) { + } else if (resourceHotswapEvaluation === ChangeHotswapImpact.IRRELEVANT) { // empty 'if' just for flow-aware typing to kick in... } else { promises.push([ @@ -103,17 +103,6 @@ async function findAllHotswappableChanges( continue; } - // NONE means that this change is an artifact of old-style synthesis and should be ignored - let foundNone = false; - for (const result of hotswapDetectionResults) { - if (result === ChangeHotswapImpact.NONE) { - foundNone = true; - } - } - if (foundNone) { - continue; - } - // no hotswappable changes found, so any REQUIRES_FULL_DEPLOYMENTs imply a non-hotswappable change for (const result of hotswapDetectionResults) { if (result === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT) { diff --git a/packages/aws-cdk/lib/api/hotswap/common.ts b/packages/aws-cdk/lib/api/hotswap/common.ts index dd86cfd7e213a..1e482d112aef4 100644 --- a/packages/aws-cdk/lib/api/hotswap/common.ts +++ b/packages/aws-cdk/lib/api/hotswap/common.ts @@ -30,14 +30,6 @@ export enum ChangeHotswapImpact { * (for example, it's a change to the CDKMetadata resource). */ IRRELEVANT = 'irrelevant', - - /** - * This result means that the given change should not be acted on during a - * hotswap deployment. - * (for example, it's a change to an AWS::IAM::Policy made when a Custom::CDKBucketDeployment - * is being hotswapped). - */ - NONE = 'none', } export type ChangeHotswapResult = HotswapOperation | ChangeHotswapImpact; diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index f316f67c1bdad..80b0567295cc6 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -13,50 +13,23 @@ export async function isHotswappableS3BucketDeploymentChange( ): Promise { // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, // meaning that the changes made to the Policy are artificial - - // TODO: there's a case where this should break with two policies and two s3 deployments - - // broken case: two policies, two s3 deployments, both of them are valid changes. - // with one pairing referring to bucketA, the other referring to bucketB - // resourceList adds bucketA - // S3 deployment that refs bucketB is processed next - // now that s3 deployment checks if resourceList is empty - // its not - // so it requires full deployment - // wow the newest test seems to catch this case - - // ok not quite it actually catches a different case: - // the two IAM policies are evaluated first, so buketA is placed and - // policy B sees bucketA and balks - - // solution: completely rework this broken code - // all of the IAM::Policy handling should be handled right here, not later - // for each IAM change, compute the diff between change.oldValue and change.newValue - // that diff is the updated policy doc resource - // find all references to the updated resource - // if all of those references are s3 deployments, return NONE - // otherwise, return REQUIRES_FULL_DEPLOYMENT - - // better solution: find the Role that the policy applies to and that the s3 deployment references - /*eslint-disable*/ if (change.newValue.Type === 'AWS::IAM::Policy') { const roles = change.newValue.Properties?.Roles; for (const role of roles) { const roleLogicalName = await evaluateCfnTemplate.findLogicallNameFor(await evaluateCfnTemplate.evaluateCfnExpression(role)); if (!roleLogicalName) { - // TODO: what should be done here? prolly throw an error since a ref target was not found return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalName); - for (const roleRef of roleRefs) { if (roleRef.Type === 'AWS::Lambda::Function') { const lambdaRefs = evaluateCfnTemplate.findReferencesTo(roleRef.LogicalId); for (const lambdaRef of lambdaRefs) { + // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an + // artifact of old-style synthesis if (lambdaRef.Type === 'Custom::CDKBucketDeployment') { - // TODO: likely need extra validation here - return ChangeHotswapImpact.NONE; + return new EmptyHotswapOperation(); } } } @@ -80,11 +53,11 @@ export async function isHotswappableS3BucketDeploymentChange( const destinationBucketKeyPrefix = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketKeyPrefix); return new S3BucketDeploymentHotswapOperation({ - FunctionName: functionName, - SourceBucketNames: sourceBucketNames, - SourceObjectKeys: sourceObjectKeys, - DestinationBucketName: destinationBucketName, - DestinationBucketKeyPrefix: destinationBucketKeyPrefix, + FunctionName: functionName, + SourceBucketNames: sourceBucketNames, + SourceObjectKeys: sourceObjectKeys, + DestinationBucketName: destinationBucketName, + DestinationBucketKeyPrefix: destinationBucketKeyPrefix, }); } @@ -123,3 +96,12 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { }).promise(); } } + +class EmptyHotswapOperation implements HotswapOperation { + constructor() { + } + + public async apply(sdk: ISDK): Promise { + return new Promise(() => { sdk; }); + } +} 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 54f8731560afb..bdf209bbe0c7f 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -42,7 +42,7 @@ export function pushStackResourceSummaries(...items: CloudFormation.StackResourc } export function setCurrentCfnStackTemplate(_template: Template) { - let template = JSON.parse(JSON.stringify(_template)); + let template = JSON.parse(JSON.stringify(_template)); // deep copy the _template, so our tests can mutate one template instead of creating two currentCfnStack.setTemplate(template); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 7ebe18ab96b26..4b1b7bacba8a9 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -395,292 +395,6 @@ test('can use the DestinationBucketKeyPrefix property', async () => { }); }); -/* -test('does not call the lambdaInvoke() API when the difference in the s3 deployment is not referred to in the IAM policy change', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - Policy: { - Type: 'AWS::IAM::Policy', - Properties: { - PolicyDocument: { - Statement: [ - { - Action: [ - 's3:GetObject*', - ], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket-old', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'DifferentBucketOld', - }, - }, - }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Policy: { - Type: 'AWS::IAM::Policy', - Properties: { - PolicyDocument: { - Statement: [ - { - Action: [ - 's3:GetObject*', - ], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'DifferentBucketNew', - }, - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).toBeUndefined(); - expect(mockLambdaInvoke).not.toHaveBeenCalled(); -}); - -// todo: it's not the website bucket they share, it's the sourcebucket -test('calls the lambdaInvoke() API when it receives an asset difference in two different s3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - PolicyA: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { - Ref: 'ServiceRole', - }, - ], - PolicyDocument: { - Statement: [ - { - Action: [ - 's3:GetObject*', - ], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOldA', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - S3DeploymentA: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket-old', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'WebsiteBucketOldA', - }, - }, - PolicyB: { - Type: 'AWS::IAM::Policy', - Properties: { - PolicyDocument: { - Statement: [ - { - Action: [ - 's3:GetObject*', - ], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOldB', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - S3DeploymentB: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket-old', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'WebsiteBucketOldB', - }, - }, - }, - }); - setup.pushStackResourceSummaries( - setup.stackSummaryOf('WebsiteBucketNewA', 'AWS::S3::Bucket', 'bucket-A'), - setup.stackSummaryOf('WebsiteBucketNewB', 'AWS::S3::Bucket', 'bucket-B'), - ); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - PolicyA: { - Type: 'AWS::IAM::Policy', - Properties: { - PolicyDocument: { - Statement: [ - { - Action: [ - 's3:GetObject*', - ], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNewA', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - S3DeploymentA: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'WebsiteBucketNewA', - }, - }, - PolicyB: { - Type: 'AWS::IAM::Policy', - Properties: { - PolicyDocument: { - Statement: [ - { - Action: [ - 's3:GetObject*', - ], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNewB', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - S3DeploymentB: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'WebsiteBucketNewB', - }, - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'WebsiteBucketNewA', - }; - - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), - }); - - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'WebsiteBucketNewB', - }; - - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), - }); -}); -*/ - describe('old-style synthesis', () => { let serviceRole: any; let policy: any; @@ -806,17 +520,13 @@ describe('old-style synthesis', () => { s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - /*eslint-disable*/ - policy.Properties.PolicyDocument.Statement.Resource = { 'Fn::GetAtt': [ 'WebsiteBucketOld', 'Arn', ], }; - /*eslint-disable*/ - console.log(s3Deployment); const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { @@ -880,7 +590,7 @@ describe('old-style synthesis', () => { }; policy2.Properties.Roles = [ 'different-role', - ] + ]; const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { @@ -937,24 +647,20 @@ describe('old-style synthesis', () => { setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), ); - + policy.Properties.PolicyDocument.Statement.Resource = { 'Fn::GetAtt': [ 'WebsiteBucketNew', 'Arn', ], }; - + policy2.Properties.PolicyDocument.Statement.Resource = { 'Fn::GetAtt': [ 'DifferentBucketNew', 'Arn', ], }; - //policy2.Properties.Roles = [ - // 'different-role', - //] - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; @@ -963,7 +669,7 @@ describe('old-style synthesis', () => { s3Deployment2.Properties.SourceBucketNames = ['src-bucket-new']; s3Deployment2.Properties.SourceObjectKeys = ['src-key-new']; s3Deployment2.Properties.DestinationBucketName = 'DifferentBucketNew'; - + const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { @@ -976,10 +682,10 @@ describe('old-style synthesis', () => { }, }, }); - + // WHEN const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - + // THEN expect(deployStackResult).not.toBeUndefined(); @@ -993,17 +699,16 @@ describe('old-style synthesis', () => { DestinationBucketName: 'WebsiteBucketNew', }; - let payload2 = JSON.parse(JSON.stringify(payload)); - payload2.ResourceProperties.DestinationBucketName = 'DifferentBucketNew';; - expect(mockLambdaInvoke).toHaveBeenCalledWith({ FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', Payload: JSON.stringify(payload), }); + payload.ResourceProperties.DestinationBucketName = 'DifferentBucketNew';; + expect(mockLambdaInvoke).toHaveBeenCalledWith({ FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', - Payload: JSON.stringify(payload2), + Payload: JSON.stringify(payload), }); }); }); \ No newline at end of file From e382c521ced579ef6170b0401534a7a5fa8aef12 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 22 Nov 2021 17:15:33 -0800 Subject: [PATCH 07/27] updated comment --- packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 80b0567295cc6..1b82715b8479e 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -12,7 +12,7 @@ export async function isHotswappableS3BucketDeploymentChange( logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, ): Promise { // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, - // meaning that the changes made to the Policy are artificial + // meaning that the changes made to the Policy are artifacts that can be safely ignored if (change.newValue.Type === 'AWS::IAM::Policy') { const roles = change.newValue.Properties?.Roles; for (const role of roles) { From 8aaae291976e882d3c16f7c83d4f641f6f3e2602 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 22 Nov 2021 17:19:56 -0800 Subject: [PATCH 08/27] added readme --- packages/aws-cdk/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index 1bd9e491e34e4..9a07cb7c70d17 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -364,6 +364,7 @@ Hotswapping is currently supported for the following changes - Code asset changes of AWS Lambda functions. - Definition changes of AWS Step Functions State Machines. - Container asset changes of AWS ECS Services. +- Website asset changes of AWS S3 Bucket Deployments **âš  Note #1**: This command deliberately introduces drift in CloudFormation stacks in order to speed up deployments. For this reason, only use it for development purposes. From b662ae438884f49da33581f60eacb37820c3ac01 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 22 Nov 2021 17:26:20 -0800 Subject: [PATCH 09/27] removed unneeded mocking of IAM SDK calls --- packages/aws-cdk/test/util/mock-sdk.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/aws-cdk/test/util/mock-sdk.ts b/packages/aws-cdk/test/util/mock-sdk.ts index efb8378ce36b3..2cd98aceb2496 100644 --- a/packages/aws-cdk/test/util/mock-sdk.ts +++ b/packages/aws-cdk/test/util/mock-sdk.ts @@ -110,10 +110,6 @@ export class MockSdkProvider extends SdkProvider { (this.sdk as any).stepFunctions = jest.fn().mockReturnValue(partialAwsService(stubs)); } - public stubIAM(stubs: SyncHandlerSubsetOf) { - (this.sdk as any).iam = jest.fn().mockReturnValue(partialAwsService(stubs)); - } - public stubGetEndpointSuffix(stub: () => string) { this.sdk.getEndpointSuffix = stub; } From 45edf7edc6f9ac48bc1f6bbef906f9dbab92fcba Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 22 Nov 2021 17:28:18 -0800 Subject: [PATCH 10/27] removed unneeded iam call --- packages/aws-cdk/test/util/mock-sdk.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/aws-cdk/test/util/mock-sdk.ts b/packages/aws-cdk/test/util/mock-sdk.ts index 2cd98aceb2496..c9afd8cd13baa 100644 --- a/packages/aws-cdk/test/util/mock-sdk.ts +++ b/packages/aws-cdk/test/util/mock-sdk.ts @@ -129,7 +129,6 @@ export class MockSdk implements ISDK { public readonly secretsManager = jest.fn(); public readonly kms = jest.fn(); public readonly stepFunctions = jest.fn(); - public readonly iam = jest.fn(); public readonly getEndpointSuffix = jest.fn(); public currentAccount(): Promise { From 240b70c2c8304782a3d6e236323e86cc0f480801 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Tue, 23 Nov 2021 11:12:29 -0800 Subject: [PATCH 11/27] incorporated review comments and fixed broken tests that didn't refer to the Statement[0].Resource --- .../evaluate-cloudformation-template.ts | 4 +- .../lib/api/hotswap/s3-bucket-deployments.ts | 57 ++++++++++--------- .../test/api/hotswap/hotswap-test-setup.ts | 4 +- .../s3-bucket-hotswap-deployments.test.ts | 15 ++--- 4 files changed, 43 insertions(+), 37 deletions(-) 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 fe6f615bc7cf5..011a9b28fc394 100644 --- a/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts +++ b/packages/aws-cdk/lib/api/hotswap/evaluate-cloudformation-template.ts @@ -50,9 +50,9 @@ export class EvaluateCloudFormationTemplate { return stackResources.find(sr => sr.LogicalResourceId === logicalId)?.PhysicalResourceId; } - public async findLogicallNameFor(physicalId: string): Promise { + public async findLogicalIdForPhysicalName(physicalName: string): Promise { const stackResources = await this.stackResources.listStackResources(); - return stackResources.find(sr => sr.PhysicalResourceId === physicalId)?.LogicalResourceId; + return stackResources.find(sr => sr.PhysicalResourceId === physicalName)?.LogicalResourceId; } public findReferencesTo(logicalId: string): Array { diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 1b82715b8479e..8fdd9641958e5 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -7,6 +7,7 @@ import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-templa * but the actual value specified is irrelevant */ export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; +/*eslint-disable*/ export async function isHotswappableS3BucketDeploymentChange( logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, @@ -15,15 +16,20 @@ export async function isHotswappableS3BucketDeploymentChange( // meaning that the changes made to the Policy are artifacts that can be safely ignored if (change.newValue.Type === 'AWS::IAM::Policy') { const roles = change.newValue.Properties?.Roles; + if (!roles) { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } for (const role of roles) { - const roleLogicalName = await evaluateCfnTemplate.findLogicallNameFor(await evaluateCfnTemplate.evaluateCfnExpression(role)); - if (!roleLogicalName) { + const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(await evaluateCfnTemplate.evaluateCfnExpression(role)); + if (!roleLogicalId) { return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalName); + const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalId); for (const roleRef of roleRefs) { - if (roleRef.Type === 'AWS::Lambda::Function') { + if (roleRef.Type !== 'AWS::Lambda::Function' && roleRef.Type !== 'AWS::IAM::Policy') { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } else if (roleRef.Type === 'AWS::Lambda::Function') { const lambdaRefs = evaluateCfnTemplate.findReferencesTo(roleRef.LogicalId); for (const lambdaRef of lambdaRefs) { // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an @@ -31,6 +37,8 @@ export async function isHotswappableS3BucketDeploymentChange( if (lambdaRef.Type === 'Custom::CDKBucketDeployment') { return new EmptyHotswapOperation(); } + + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } } @@ -47,37 +55,34 @@ export async function isHotswappableS3BucketDeploymentChange( return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - const sourceBucketNames = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceBucketNames); - const sourceObjectKeys = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.SourceObjectKeys); - const destinationBucketName = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketName); - const destinationBucketKeyPrefix = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.DestinationBucketKeyPrefix); + const props = change.newValue.Properties; return new S3BucketDeploymentHotswapOperation({ - FunctionName: functionName, - SourceBucketNames: sourceBucketNames, - SourceObjectKeys: sourceObjectKeys, - DestinationBucketName: destinationBucketName, - DestinationBucketKeyPrefix: destinationBucketKeyPrefix, + functionName: functionName, + sourceBucketNames: await evaluateCfnTemplate.evaluateCfnExpression(props?.SourceBucketNames), + sourceObjectKeys: await evaluateCfnTemplate.evaluateCfnExpression(props?.SourceObjectKeys), + destinationBucketName: await evaluateCfnTemplate.evaluateCfnExpression(props?.DestinationBucketName), + destinationBucketKeyPrefix: await evaluateCfnTemplate.evaluateCfnExpression(props?.DestinationBucketKeyPrefix), }); } -interface S3BucketDeployment { - FunctionName: string, - SourceBucketNames: any, - SourceObjectKeys: any, - DestinationBucketName: any, - DestinationBucketKeyPrefix: string, +interface S3BucketDeploymentHotswapOperationOptions { + functionName: string; + sourceBucketNames: string[]; + sourceObjectKeys: string[]; + destinationBucketName: string; + destinationBucketKeyPrefix: string; } class S3BucketDeploymentHotswapOperation implements HotswapOperation { - constructor(private readonly s3BucketDeployment: S3BucketDeployment) { + constructor(private readonly s3BucketDeployment: S3BucketDeploymentHotswapOperationOptions) { } public async apply(sdk: ISDK): Promise { const deployment = this.s3BucketDeployment; return sdk.lambda().invoke({ - FunctionName: deployment.FunctionName, + FunctionName: deployment.functionName, // Lambda refuses to take a direct JSON object and requires it to be stringify()'d Payload: JSON.stringify({ RequestType: 'Update', @@ -87,10 +92,10 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { RequestId: REQUIRED_BY_CFN, LogicalResourceId: REQUIRED_BY_CFN, ResourceProperties: { - SourceBucketNames: deployment.SourceBucketNames, - SourceObjectKeys: deployment.SourceObjectKeys, - DestinationBucketName: deployment.DestinationBucketName, - DestinationBucketKeyPrefix: deployment.DestinationBucketKeyPrefix, + SourceBucketNames: deployment.sourceBucketNames, + SourceObjectKeys: deployment.sourceObjectKeys, + DestinationBucketName: deployment.destinationBucketName, + DestinationBucketKeyPrefix: deployment.destinationBucketKeyPrefix, }, }), }).promise(); @@ -102,6 +107,6 @@ class EmptyHotswapOperation implements HotswapOperation { } public async apply(sdk: ISDK): Promise { - return new Promise(() => { sdk; }); + return Promise.resolve(sdk); } } 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 bdf209bbe0c7f..9fa8a7429fd6b 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -88,9 +88,9 @@ export class CfnMockProvider { }); } - public setLambdaInvocationMock(mockInvoke: (input: lambda.InvocationRequest) => lambda.InvocationResponse) { + public setInvokeLambdaMock(mockInvokeLambda: (input: lambda.InvocationRequest) => lambda.InvocationResponse) { this.mockSdkProvider.stubLambda({ - invoke: mockInvoke, + invoke: mockInvokeLambda, }); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 4b1b7bacba8a9..48d1fc19164b2 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -9,7 +9,7 @@ let payload: { [key: string]: any}; beforeEach(() => { cfnMockProvider = setup.setupHotswapTests(); mockLambdaInvoke = jest.fn(); - cfnMockProvider.setLambdaInvocationMock(mockLambdaInvoke); + cfnMockProvider.setInvokeLambdaMock(mockLambdaInvoke); payload = { RequestType: 'Update', @@ -520,9 +520,9 @@ describe('old-style synthesis', () => { s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - policy.Properties.PolicyDocument.Statement.Resource = { + policy.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ - 'WebsiteBucketOld', + 'WebsiteBucketNewasdf', 'Arn', ], }; @@ -557,6 +557,7 @@ describe('old-style synthesis', () => { FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', Payload: JSON.stringify(payload), }); + policy2; }); test('does not call the lambdaInvoke() API when the difference in the s3 deployment is referred to in one IAM policy change but not another', async () => { @@ -575,14 +576,14 @@ describe('old-style synthesis', () => { setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), ); - policy.Properties.PolicyDocument.Statement.Resource = { + policy.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ 'WebsiteBucketNew', 'Arn', ], }; - policy2.Properties.PolicyDocument.Statement.Resource = { + policy2.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ 'DifferentBucketNew', 'Arn', @@ -648,14 +649,14 @@ describe('old-style synthesis', () => { setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), ); - policy.Properties.PolicyDocument.Statement.Resource = { + policy.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ 'WebsiteBucketNew', 'Arn', ], }; - policy2.Properties.PolicyDocument.Statement.Resource = { + policy2.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ 'DifferentBucketNew', 'Arn', From 22c91311f7c027c865f788a88737e6533569f502 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Tue, 23 Nov 2021 11:41:58 -0800 Subject: [PATCH 12/27] removed eslint-disable --- packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 8fdd9641958e5..ded88603db554 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -7,7 +7,6 @@ import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-templa * but the actual value specified is irrelevant */ export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; -/*eslint-disable*/ export async function isHotswappableS3BucketDeploymentChange( logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, From 29993e8010faf68c8ddc1a7f01ae7087e0d02c54 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Tue, 23 Nov 2021 13:27:49 -0800 Subject: [PATCH 13/27] updated the way properties are passed to the lambda and removed needless constructor() definition --- .../lib/api/hotswap/s3-bucket-deployments.ts | 23 ++++--------------- 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index ded88603db554..a24e113809d29 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -54,23 +54,18 @@ export async function isHotswappableS3BucketDeploymentChange( return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - const props = change.newValue.Properties; + const props = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties); + delete props.ServiceToken; return new S3BucketDeploymentHotswapOperation({ functionName: functionName, - sourceBucketNames: await evaluateCfnTemplate.evaluateCfnExpression(props?.SourceBucketNames), - sourceObjectKeys: await evaluateCfnTemplate.evaluateCfnExpression(props?.SourceObjectKeys), - destinationBucketName: await evaluateCfnTemplate.evaluateCfnExpression(props?.DestinationBucketName), - destinationBucketKeyPrefix: await evaluateCfnTemplate.evaluateCfnExpression(props?.DestinationBucketKeyPrefix), + properties: props, }); } interface S3BucketDeploymentHotswapOperationOptions { functionName: string; - sourceBucketNames: string[]; - sourceObjectKeys: string[]; - destinationBucketName: string; - destinationBucketKeyPrefix: string; + properties: any; } class S3BucketDeploymentHotswapOperation implements HotswapOperation { @@ -90,21 +85,13 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { StackId: REQUIRED_BY_CFN, RequestId: REQUIRED_BY_CFN, LogicalResourceId: REQUIRED_BY_CFN, - ResourceProperties: { - SourceBucketNames: deployment.sourceBucketNames, - SourceObjectKeys: deployment.sourceObjectKeys, - DestinationBucketName: deployment.destinationBucketName, - DestinationBucketKeyPrefix: deployment.destinationBucketKeyPrefix, - }, + ResourceProperties: deployment.properties, }), }).promise(); } } class EmptyHotswapOperation implements HotswapOperation { - constructor() { - } - public async apply(sdk: ISDK): Promise { return Promise.resolve(sdk); } From af9f35a27aa677959550567b5538cf88b3afde46 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 23 Nov 2021 13:36:31 -0800 Subject: [PATCH 14/27] Send all requests to the Custom Resource Lambda, instead of just the select ones. --- .../lib/api/hotswap/s3-bucket-deployments.ts | 41 ++++++++++++------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index a24e113809d29..89f32b008a078 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -54,29 +54,22 @@ export async function isHotswappableS3BucketDeploymentChange( return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - const props = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties); - delete props.ServiceToken; - - return new S3BucketDeploymentHotswapOperation({ - functionName: functionName, - properties: props, + const customResourceProperties = await evaluateCfnTemplate.evaluateCfnExpression({ + ...change.newValue.Properties, + ServiceToken: undefined, }); -} -interface S3BucketDeploymentHotswapOperationOptions { - functionName: string; - properties: any; + return new S3BucketDeploymentHotswapOperation(functionName, customResourceProperties); } class S3BucketDeploymentHotswapOperation implements HotswapOperation { - constructor(private readonly s3BucketDeployment: S3BucketDeploymentHotswapOperationOptions) { + constructor(private readonly functionName: string, private readonly customResourceProperties: any) { } public async apply(sdk: ISDK): Promise { - const deployment = this.s3BucketDeployment; - + const resourceProperties = stringifyObject(this.customResourceProperties); return sdk.lambda().invoke({ - FunctionName: deployment.functionName, + FunctionName: this.functionName, // Lambda refuses to take a direct JSON object and requires it to be stringify()'d Payload: JSON.stringify({ RequestType: 'Update', @@ -85,12 +78,30 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { StackId: REQUIRED_BY_CFN, RequestId: REQUIRED_BY_CFN, LogicalResourceId: REQUIRED_BY_CFN, - ResourceProperties: deployment.properties, + ResourceProperties: resourceProperties, }), }).promise(); } } +function stringifyObject(obj: any): any { + if (obj == null) { + return obj; + } + if (Array.isArray(obj)) { + return obj.map(stringifyObject); + } + if (typeof obj !== 'object') { + return obj.toString(); + } + + const ret: { [k: string]: any } = {}; + for (const [k, v] of Object.entries(obj)) { + ret[k] = stringifyObject(v); + } + return ret; +} + class EmptyHotswapOperation implements HotswapOperation { public async apply(sdk: ISDK): Promise { return Promise.resolve(sdk); From 3e7f2d8ca1ccff47b967f82b5aa35aa29e5fbf0e Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 24 Nov 2021 09:16:44 -0800 Subject: [PATCH 15/27] incorporated adam's code and fixed the failing lambda --- packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 89f32b008a078..5eb65972d4812 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -67,7 +67,9 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { } public async apply(sdk: ISDK): Promise { + // JSON.stringify() doesn't turn the actual objects to strings, but the lambda expects strings const resourceProperties = stringifyObject(this.customResourceProperties); + return sdk.lambda().invoke({ FunctionName: this.functionName, // Lambda refuses to take a direct JSON object and requires it to be stringify()'d From f9853fcfdd565704f3ede9b16405ac90074b8117 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 24 Nov 2021 09:35:48 -0800 Subject: [PATCH 16/27] added test to confirm that undefined roles do not make the cli crash --- .../lib/api/hotswap/s3-bucket-deployments.ts | 1 + .../s3-bucket-hotswap-deployments.test.ts | 89 +++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 5eb65972d4812..9561754cdc5a3 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -18,6 +18,7 @@ export async function isHotswappableS3BucketDeploymentChange( if (!roles) { return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } + for (const role of roles) { const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(await evaluateCfnTemplate.evaluateCfnExpression(role)); if (!roleLogicalId) { diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 48d1fc19164b2..9a3f78eec9468 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -395,6 +395,95 @@ test('can use the DestinationBucketKeyPrefix property', async () => { }); }); +test('does not call the invokeLambda() api if the updated Policy has no Roles', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + }, + }, + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Action: [ + 's3:GetObject*', + ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + }, + }, + PolicyDocument: { + Statement: [ + { + Action: [ + 's3:GetObject*', + ], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); +}); + describe('old-style synthesis', () => { let serviceRole: any; let policy: any; From 3dc72e8de1b3d7fc8c69e2cb2c995380c7e9f461 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 24 Nov 2021 10:49:57 -0800 Subject: [PATCH 17/27] refs to the lambda that are not s3 deployments result in full deployment --- .../lib/api/hotswap/s3-bucket-deployments.ts | 8 +- .../s3-bucket-hotswap-deployments.test.ts | 75 +++++++++++++++++-- 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 9561754cdc5a3..89371d06b4256 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -34,12 +34,12 @@ export async function isHotswappableS3BucketDeploymentChange( for (const lambdaRef of lambdaRefs) { // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an // artifact of old-style synthesis - if (lambdaRef.Type === 'Custom::CDKBucketDeployment') { - return new EmptyHotswapOperation(); + if (lambdaRef.Type !== 'Custom::CDKBucketDeployment') { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } + + return new EmptyHotswapOperation(); } } } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 9a3f78eec9468..287a7134f3f62 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -660,10 +660,6 @@ describe('old-style synthesis', () => { S3Deployment: s3Deployment, }, }); - setup.pushStackResourceSummaries( - setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), - setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), - ); policy.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ @@ -702,6 +698,73 @@ describe('old-style synthesis', () => { expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); + test('does not call the lambdaInvoke() API when the lambda that references the role is referred to by something other than an s3 deployment', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + ServiceRole: serviceRole, + Policy: policy, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + Endpoint: { + Type: 'AWS::Lambda::Permission', + Properties: { + Action: 'lambda:InvokeFunction', + FunctionName: { + 'Fn::GetAtt': [ + 'S3DeploymentLambda', + 'Arn', + ], + }, + Principal: 'apigateway.amazonaws.com', + }, + }, + }, + }); + + policy.Properties.PolicyDocument.Statement[0].Resource = { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }; + + s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; + s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; + s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; + + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + ServiceRole: serviceRole, + Policy: policy, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + Endpoint: { + Type: 'AWS::Lambda::Permission', + Properties: { + Action: 'lambda:InvokeFunction', + FunctionName: { + 'Fn::GetAtt': [ + 'S3DeploymentLambda', + 'Arn', + ], + }, + Principal: 'apigateway.amazonaws.com', + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); + }); + test('calls the lambdaInvoke() API when it receives an asset difference in two s3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { // GIVEN let s3Deployment2 = { @@ -733,10 +796,6 @@ describe('old-style synthesis', () => { S3Deployment2: s3Deployment2, }, }); - setup.pushStackResourceSummaries( - setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), - setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), - ); policy.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ From 13476ada3699869da7bac8a4c810870c60ef1f13 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 24 Nov 2021 10:57:17 -0800 Subject: [PATCH 18/27] refactored IAM Policy Handling --- .../lib/api/hotswap/s3-bucket-deployments.ts | 64 ++++++++++--------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 89371d06b4256..fad8f0474039e 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -14,35 +14,7 @@ export async function isHotswappableS3BucketDeploymentChange( // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, // meaning that the changes made to the Policy are artifacts that can be safely ignored if (change.newValue.Type === 'AWS::IAM::Policy') { - const roles = change.newValue.Properties?.Roles; - if (!roles) { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } - - for (const role of roles) { - const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(await evaluateCfnTemplate.evaluateCfnExpression(role)); - if (!roleLogicalId) { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } - - const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalId); - for (const roleRef of roleRefs) { - if (roleRef.Type !== 'AWS::Lambda::Function' && roleRef.Type !== 'AWS::IAM::Policy') { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } else if (roleRef.Type === 'AWS::Lambda::Function') { - const lambdaRefs = evaluateCfnTemplate.findReferencesTo(roleRef.LogicalId); - for (const lambdaRef of lambdaRefs) { - // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an - // artifact of old-style synthesis - if (lambdaRef.Type !== 'Custom::CDKBucketDeployment') { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } - } - - return new EmptyHotswapOperation(); - } - } - } + return isOldStyleSynthesisArtifact(change, evaluateCfnTemplate); } if (change.newValue.Type !== 'Custom::CDKBucketDeployment') { @@ -87,6 +59,40 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { } } +async function isOldStyleSynthesisArtifact( + change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, +): Promise { + const roles = change.newValue.Properties?.Roles; + if (!roles) { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + + for (const role of roles) { + const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(await evaluateCfnTemplate.evaluateCfnExpression(role)); + if (!roleLogicalId) { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + + const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalId); + for (const roleRef of roleRefs) { + if (roleRef.Type !== 'AWS::Lambda::Function' && roleRef.Type !== 'AWS::IAM::Policy') { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } else if (roleRef.Type === 'AWS::Lambda::Function') { + const lambdaRefs = evaluateCfnTemplate.findReferencesTo(roleRef.LogicalId); + for (const lambdaRef of lambdaRefs) { + // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an + // artifact of old-style synthesis + if (lambdaRef.Type !== 'Custom::CDKBucketDeployment') { + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + } + } + } + } + + return new EmptyHotswapOperation(); +} + function stringifyObject(obj: any): any { if (obj == null) { return obj; From cf1e73fa7c94adee3a90450529863bc294961fca Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 24 Nov 2021 11:02:30 -0800 Subject: [PATCH 19/27] updated test to have an additional role to verify that early returns work correctly --- .../test/api/hotswap/s3-bucket-hotswap-deployments.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 287a7134f3f62..4398816321f0a 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -646,7 +646,6 @@ describe('old-style synthesis', () => { FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', Payload: JSON.stringify(payload), }); - policy2; }); test('does not call the lambdaInvoke() API when the difference in the s3 deployment is referred to in one IAM policy change but not another', async () => { @@ -675,6 +674,7 @@ describe('old-style synthesis', () => { ], }; policy2.Properties.Roles = [ + { Ref: 'ServiceRole' }, 'different-role', ]; From a2788c29d80176ad399bb0e61ffb007f215d02b6 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 1 Dec 2021 18:24:39 -0800 Subject: [PATCH 20/27] improved tests and caught case of multiple role references --- packages/aws-cdk/README.md | 2 +- .../lib/api/hotswap/s3-bucket-deployments.ts | 26 +- .../test/api/hotswap/hotswap-test-setup.ts | 4 +- .../s3-bucket-hotswap-deployments.test.ts | 337 +++++++++++------- 4 files changed, 225 insertions(+), 144 deletions(-) diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index 9a07cb7c70d17..ac399df3008df 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -364,7 +364,7 @@ Hotswapping is currently supported for the following changes - Code asset changes of AWS Lambda functions. - Definition changes of AWS Step Functions State Machines. - Container asset changes of AWS ECS Services. -- Website asset changes of AWS S3 Bucket Deployments +- Website asset changes of AWS S3 Bucket Deployments. **âš  Note #1**: This command deliberately introduces drift in CloudFormation stacks in order to speed up deployments. For this reason, only use it for development purposes. diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index fad8f0474039e..d524bc2bdaf0c 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -2,6 +2,7 @@ import { ISDK } from '../aws-auth'; import { ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; +/*eslint-disable*/ /** * This means that the value is required to exist by CloudFormation's API (or our S3 Bucket Deployment Lambda) * but the actual value specified is irrelevant @@ -14,16 +15,18 @@ export async function isHotswappableS3BucketDeploymentChange( // In old-style synthesis, the policy used by the lambda to copy assets Ref's the assets directly, // meaning that the changes made to the Policy are artifacts that can be safely ignored if (change.newValue.Type === 'AWS::IAM::Policy') { - return isOldStyleSynthesisArtifact(change, evaluateCfnTemplate); + return changeIsForS3DeployCustomResourcePolicy(logicalId, change, evaluateCfnTemplate); } if (change.newValue.Type !== 'Custom::CDKBucketDeployment') { + console.log('ah 1') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } // note that this gives the ARN of the lambda, not the name. This is fine though, the invoke() sdk call will take either const functionName = await establishResourcePhysicalName(logicalId, change.newValue.Properties?.ServiceToken, evaluateCfnTemplate); if (!functionName) { + console.log('ah 2') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } @@ -59,33 +62,44 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { } } -async function isOldStyleSynthesisArtifact( - change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, +async function changeIsForS3DeployCustomResourcePolicy( + logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, ): Promise { const roles = change.newValue.Properties?.Roles; if (!roles) { + console.log('ah 3') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } for (const role of roles) { const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(await evaluateCfnTemplate.evaluateCfnExpression(role)); if (!roleLogicalId) { + console.log('ah 4') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalId); for (const roleRef of roleRefs) { - if (roleRef.Type !== 'AWS::Lambda::Function' && roleRef.Type !== 'AWS::IAM::Policy') { - return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; - } else if (roleRef.Type === 'AWS::Lambda::Function') { + if (roleRef.Type === 'AWS::Lambda::Function') { const lambdaRefs = evaluateCfnTemplate.findReferencesTo(roleRef.LogicalId); for (const lambdaRef of lambdaRefs) { // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an // artifact of old-style synthesis if (lambdaRef.Type !== 'Custom::CDKBucketDeployment') { + console.log('ah 5') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } + } else if (roleRef.Type === 'AWS::IAM::Policy') { + if (roleRef.LogicalId !== logicalId) { + console.log(roleRef.LogicalId) + console.log(logicalId) + console.log('ah 6') + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; + } + } else if (roleRef.Type !== 'AWS::IAM::Policy') { + console.log('ah 7') + return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } } 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 9fa8a7429fd6b..73a4f17549ad9 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -42,8 +42,8 @@ export function pushStackResourceSummaries(...items: CloudFormation.StackResourc } export function setCurrentCfnStackTemplate(_template: Template) { - let template = JSON.parse(JSON.stringify(_template)); // deep copy the _template, so our tests can mutate one template instead of creating two - currentCfnStack.setTemplate(template); + const templateDeepCopy = JSON.parse(JSON.stringify(_template)); // deep copy the _template, so our tests can mutate one template instead of creating two + currentCfnStack.setTemplate(templateDeepCopy); } export function stackSummaryOf(logicalId: string, resourceType: string, physicalResourceId: string): CloudFormation.StackResourceSummary { diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 4398816321f0a..b6916e72e205d 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -4,21 +4,20 @@ import * as setup from './hotswap-test-setup'; let mockLambdaInvoke: (params: Lambda.Types.InvocationRequest) => Lambda.Types.InvocationResponse; let cfnMockProvider: setup.CfnMockProvider; -let payload: { [key: string]: any}; + +const payloadWithoutCustomResProps = { + RequestType: 'Update', + ResponseURL: REQUIRED_BY_CFN, + PhysicalResourceId: REQUIRED_BY_CFN, + StackId: REQUIRED_BY_CFN, + RequestId: REQUIRED_BY_CFN, + LogicalResourceId: REQUIRED_BY_CFN, +}; beforeEach(() => { cfnMockProvider = setup.setupHotswapTests(); mockLambdaInvoke = jest.fn(); cfnMockProvider.setInvokeLambdaMock(mockLambdaInvoke); - - payload = { - RequestType: 'Update', - ResponseURL: REQUIRED_BY_CFN, - PhysicalResourceId: REQUIRED_BY_CFN, - StackId: REQUIRED_BY_CFN, - RequestId: REQUIRED_BY_CFN, - LogicalResourceId: REQUIRED_BY_CFN, - }; }); test('returns undefined when a new S3 Deployment is added to the Stack', async () => { @@ -38,6 +37,7 @@ test('returns undefined when a new S3 Deployment is added to the Stack', async ( // THEN expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); test('calls the lambdaInvoke() API when it receives only an asset difference in an s3 bucket deployment', async () => { @@ -55,6 +55,7 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in 'src-key-old', ], DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-old-prefix', }, }, }, @@ -73,6 +74,7 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in 'src-key-new', ], DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-new-prefix', }, }, }, @@ -84,19 +86,22 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in // THEN expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - }; expect(mockLambdaInvoke).toHaveBeenCalledWith({ FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), + Payload: JSON.stringify({ + ...payloadWithoutCustomResProps, + ResourceProperties: { + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + DestinationBucketKeyPrefix: 'my-key/some-new-prefix', + }, + }), }); }); @@ -160,22 +165,23 @@ test('correctly evaluates the service token when it references a lambda found in // WHEN const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'dest-bucket', - }; - // THEN expect(deployStackResult).not.toBeUndefined(); expect(mockLambdaInvoke).toHaveBeenCalledWith({ FunctionName: 'lambda-my-deployment-lambda-function', - Payload: JSON.stringify(payload), + Payload: JSON.stringify({ + ...payloadWithoutCustomResProps, + ResourceProperties: { + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-old', + ], + DestinationBucketName: 'dest-bucket', + }, + }), }); }); @@ -332,69 +338,6 @@ test('does not call the invoke() API when a resource with type that is not Custo expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); -test('can use the DestinationBucketKeyPrefix property', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'dest-bucket', - DestinationBucketKeyPrefix: 'my-key/some-old-prefix', - }, - }, - }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - DestinationBucketKeyPrefix: 'my-key/some-new-prefix', - }, - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - DestinationBucketKeyPrefix: 'my-key/some-new-prefix', - }; - - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'a-lambda-arn', - Payload: JSON.stringify(payload), - }); -}); - test('does not call the invokeLambda() api if the updated Policy has no Roles', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -438,22 +381,22 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: 'a-lambda-arn', + SourceBucketNames: [ + 'src-bucket', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'dest-bucket', + }, + }, Policy: { Type: 'AWS::IAM::Policy', Properties: { - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'dest-bucket', - }, - }, PolicyDocument: { Statement: [ { @@ -463,7 +406,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Effect: 'Allow', Resource: { 'Fn::GetAtt': [ - 'WebsiteBucketOld', + 'WebsiteBucketNew', 'Arn', ], }, @@ -611,7 +554,7 @@ describe('old-style synthesis', () => { policy.Properties.PolicyDocument.Statement[0].Resource = { 'Fn::GetAtt': [ - 'WebsiteBucketNewasdf', + 'WebsiteBucketNew', 'Arn', ], }; @@ -632,19 +575,20 @@ describe('old-style synthesis', () => { // THEN expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: 'WebsiteBucketNew', - }; - expect(mockLambdaInvoke).toHaveBeenCalledWith({ FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', - Payload: JSON.stringify(payload), + Payload: JSON.stringify({ + ...payloadWithoutCustomResProps, + ResourceProperties: { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNew', + }, + }), }); }); @@ -772,7 +716,7 @@ describe('old-style synthesis', () => { Properties: { ServiceToken: { 'Fn::GetAtt': [ - 'S3DeploymentLambda', + 'S3DeploymentLambda2', 'Arn', ], }, @@ -786,12 +730,18 @@ describe('old-style synthesis', () => { }, }; + policy2.Properties.Roles = [ + { Ref: 'ServiceRole2' }, + ]; + setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, + ServiceRole2: serviceRole, Policy1: policy, Policy2: policy2, S3DeploymentLambda: deploymentLambda, + S3DeploymentLambda2: deploymentLambda, S3Deployment: s3Deployment, S3Deployment2: s3Deployment2, }, @@ -823,9 +773,11 @@ describe('old-style synthesis', () => { template: { Resources: { ServiceRole: serviceRole, + ServiceRole2: serviceRole, Policy1: policy, Policy2: policy2, S3DeploymentLambda: deploymentLambda, + S3DeploymentLambda2: deploymentLambda, S3Deployment: s3Deployment, S3Deployment2: s3Deployment2, }, @@ -833,31 +785,146 @@ describe('old-style synthesis', () => { }); // WHEN + setup.pushStackResourceSummaries( + setup.stackSummaryOf('S3DeploymentLambda2', 'AWS::Lambda::Function', 'my-deployment-lambda-2'), + setup.stackSummaryOf('ServiceRole2', 'AWS::IAM::Role', 'my-service-role-2'), + ); + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).not.toBeUndefined(); - payload.ResourceProperties = { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', + Payload: JSON.stringify({ + ...payloadWithoutCustomResProps, + ResourceProperties: { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'WebsiteBucketNew', + }, + }), + }); + + expect(mockLambdaInvoke).toHaveBeenCalledWith({ + FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda-2', + Payload: JSON.stringify({ + ...payloadWithoutCustomResProps, + ResourceProperties: { + SourceBucketNames: [ + 'src-bucket-new', + ], + SourceObjectKeys: [ + 'src-key-new', + ], + DestinationBucketName: 'DifferentBucketNew', + }, + }), + }); + }); + + test('does not call the lambdaInvoke() API when it receives an asset difference in an s3 bucket deployment that references two different policies', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + Policy2: policy2, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + }, + }); + + policy.Properties.PolicyDocument.Statement[0].Resource = { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', ], - DestinationBucketName: 'WebsiteBucketNew', }; - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', - Payload: JSON.stringify(payload), + s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; + s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; + s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; + + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + Policy2: policy2, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + }, + }, }); - payload.ResourceProperties.DestinationBucketName = 'DifferentBucketNew';; + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', - Payload: JSON.stringify(payload), + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); + }); + + test('does not call the lambdaInvoke() API when a policy is referenced by a resource that is not an s3 deployment', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + NotADeployment: { + Type: 'AWS::Not::S3Deployment', + Properties: { + Prop: { + Ref: 'ServiceRole', + }, + }, + }, + }, }); + + policy.Properties.PolicyDocument.Statement[0].Resource = { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }; + + s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; + s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; + s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; + + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + ServiceRole: serviceRole, + Policy1: policy, + S3DeploymentLambda: deploymentLambda, + S3Deployment: s3Deployment, + NotADeployment: { + Type: 'AWS::Not::S3Deployment', + Properties: { + Prop: { + Ref: 'ServiceRole', + }, + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); }); \ No newline at end of file From 4866367b359786af304d72bb87656c73672cf193 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Thu, 2 Dec 2021 11:21:12 -0800 Subject: [PATCH 21/27] refactor --- .../lib/api/hotswap/s3-bucket-deployments.ts | 2 + .../s3-bucket-hotswap-deployments.test.ts | 266 +++--------------- 2 files changed, 37 insertions(+), 231 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index d524bc2bdaf0c..a8315c1fa4833 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -93,12 +93,14 @@ async function changeIsForS3DeployCustomResourcePolicy( } else if (roleRef.Type === 'AWS::IAM::Policy') { if (roleRef.LogicalId !== logicalId) { console.log(roleRef.LogicalId) + debugger; console.log(logicalId) console.log('ah 6') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } else if (roleRef.Type !== 'AWS::IAM::Policy') { console.log('ah 7') + debugger; return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index b6916e72e205d..b4f6f740ced52 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -20,26 +20,6 @@ beforeEach(() => { cfnMockProvider.setInvokeLambdaMock(mockLambdaInvoke); }); -test('returns undefined when a new S3 Deployment is added to the Stack', async () => { - // GIVEN - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Deployment: { - Type: 'Custom::CDKBucketDeployment', - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).toBeUndefined(); - expect(mockLambdaInvoke).not.toHaveBeenCalled(); -}); - test('calls the lambdaInvoke() API when it receives only an asset difference in an s3 bucket deployment', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -48,12 +28,8 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-old'], DestinationBucketName: 'dest-bucket', DestinationBucketKeyPrefix: 'my-key/some-old-prefix', }, @@ -67,12 +43,8 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'dest-bucket', DestinationBucketKeyPrefix: 'my-key/some-new-prefix', }, @@ -92,12 +64,8 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in Payload: JSON.stringify({ ...payloadWithoutCustomResProps, ResourceProperties: { - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'dest-bucket', DestinationBucketKeyPrefix: 'my-key/some-new-prefix', }, @@ -105,6 +73,7 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in }); }); +/* test('correctly evaluates the service token when it references a lambda found in the template', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -116,17 +85,12 @@ test('correctly evaluates the service token when it references a lambda found in Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], + SourceBucketNames: ['src-bucket'], SourceObjectKeys: [ 'src-key-old', ], DestinationBucketName: 'dest-bucket', }, - Metadata: { - 'aws:asset:path': 'old-path', - }, }, }, }); @@ -141,23 +105,14 @@ test('correctly evaluates the service token when it references a lambda found in Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: { - 'Fn::Join': ['-', [ - 'lambda', - { Ref: 'Lambda' }, - 'function', - ]], + 'Fn::GetAtt': ['Lambda', 'Arn'], }, - SourceBucketNames: [ - 'src-bucket', - ], + SourceBucketNames: ['src-bucket'], SourceObjectKeys: [ 'src-key-old', ], DestinationBucketName: 'dest-bucket', }, - Metadata: { - 'aws:asset:path': 'old-path', - }, }, }, }, @@ -169,13 +124,11 @@ test('correctly evaluates the service token when it references a lambda found in // THEN expect(deployStackResult).not.toBeUndefined(); expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'lambda-my-deployment-lambda-function', + FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', Payload: JSON.stringify({ ...payloadWithoutCustomResProps, ResourceProperties: { - SourceBucketNames: [ - 'src-bucket', - ], + SourceBucketNames: ['src-bucket'], SourceObjectKeys: [ 'src-key-old', ], @@ -184,112 +137,7 @@ test('correctly evaluates the service token when it references a lambda found in }), }); }); - -test("will not perform a hotswap deployment if it cannot find a Ref target (outside the function's name)", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Parameters: { - BucketParam: { Type: 'String' }, - }, - Resources: { - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'my-lambda', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: { 'Fn::Sub': '${BucketParam}' }, - }, - }, - }, - }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Parameters: { - BucketParam: { Type: 'String' }, - }, - Resources: { - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'my-lambda', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], - DestinationBucketName: { 'Fn::Sub': '${BucketParam}' }, - }, - }, - }, - }, - }); - - // THEN - await expect(() => - cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), - ).rejects.toThrow(/Parameter or resource 'BucketParam' could not be found for evaluation/); -}); - -test("will not perform a hotswap deployment if it doesn't know how to handle a specific attribute (outside the function's name)", async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - Bucket: { - Type: 'AWS::S3::Bucket', - }, - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'my-lambda', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'website-bucket', - }, - }, - }, - }); - setup.pushStackResourceSummaries( - setup.stackSummaryOf('S3Deployment', 'Custom::CDKBucketDeployment', 'my-s3-deployment'), - setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'asset-bucket'), - ); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Bucket: { - Type: 'AWS::S3::Bucket', - }, - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'my-lambda', - SourceBucketNames: [ - { 'Fn::GetAtt': ['Bucket', 'UnknownAttribute'] }, - ], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'website-bucket', - }, - }, - }, - }, - }); - - // THEN - await expect(() => - cfnMockProvider.tryHotswapDeployment(cdkStackArtifact), - ).rejects.toThrow("We don't support the 'UnknownAttribute' attribute of the 'AWS::S3::Bucket' resource. This is a CDK limitation. Please report it at https://github.com/aws/aws-cdk/issues/new/choose"); -}); +*/ test('does not call the invoke() API when a resource with type that is not Custom::CDKBucketDeployment but has the same properties is changed', async () => { // GIVEN @@ -299,12 +147,8 @@ test('does not call the invoke() API when a resource with type that is not Custo Type: 'Custom::NotCDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-old'], DestinationBucketName: 'dest-bucket', }, }, @@ -317,12 +161,8 @@ test('does not call the invoke() API when a resource with type that is not Custo Type: 'Custom::NotCDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'dest-bucket', }, }, @@ -346,12 +186,8 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-old', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-old'], DestinationBucketName: 'dest-bucket', }, }, @@ -361,9 +197,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', PolicyDocument: { Statement: [ { - Action: [ - 's3:GetObject*', - ], + Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { 'Fn::GetAtt': [ @@ -385,12 +219,8 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: [ - 'src-bucket', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'dest-bucket', }, }, @@ -400,9 +230,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', PolicyDocument: { Statement: [ { - Action: [ - 's3:GetObject*', - ], + Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { 'Fn::GetAtt': [ @@ -461,9 +289,7 @@ describe('old-style synthesis', () => { PolicyDocument: { Statement: [ { - Action: [ - 's3:GetObject*', - ], + Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { 'Fn::GetAtt': [ @@ -486,9 +312,7 @@ describe('old-style synthesis', () => { PolicyDocument: { Statement: [ { - Action: [ - 's3:GetObject*', - ], + Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { 'Fn::GetAtt': [ @@ -521,12 +345,8 @@ describe('old-style synthesis', () => { 'Arn', ], }, - SourceBucketNames: [ - 'src-bucket-old', - ], - SourceObjectKeys: [ - 'src-key-old', - ], + SourceBucketNames: ['src-bucket-old'], + SourceObjectKeys: ['src-key-old'], DestinationBucketName: 'WebsiteBucketOld', }, }; @@ -580,12 +400,8 @@ describe('old-style synthesis', () => { Payload: JSON.stringify({ ...payloadWithoutCustomResProps, ResourceProperties: { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket-new'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'WebsiteBucketNew', }, }), @@ -720,12 +536,8 @@ describe('old-style synthesis', () => { 'Arn', ], }, - SourceBucketNames: [ - 'src-bucket-old', - ], - SourceObjectKeys: [ - 'src-key-old', - ], + SourceBucketNames: ['src-bucket-old'], + SourceObjectKeys: ['src-key-old'], DestinationBucketName: 'DifferentBucketOld', }, }; @@ -800,12 +612,8 @@ describe('old-style synthesis', () => { Payload: JSON.stringify({ ...payloadWithoutCustomResProps, ResourceProperties: { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket-new'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'WebsiteBucketNew', }, }), @@ -816,12 +624,8 @@ describe('old-style synthesis', () => { Payload: JSON.stringify({ ...payloadWithoutCustomResProps, ResourceProperties: { - SourceBucketNames: [ - 'src-bucket-new', - ], - SourceObjectKeys: [ - 'src-key-new', - ], + SourceBucketNames: ['src-bucket-new'], + SourceObjectKeys: ['src-key-new'], DestinationBucketName: 'DifferentBucketNew', }, }), From 7a2b96e28138abce031beb7861ca329dbd905c3f Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Thu, 2 Dec 2021 16:24:52 -0800 Subject: [PATCH 22/27] inlined policies --- .../lib/api/hotswap/s3-bucket-deployments.ts | 2 - .../s3-bucket-hotswap-deployments.test.ts | 490 ++++++++++++++---- 2 files changed, 398 insertions(+), 94 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index a8315c1fa4833..d524bc2bdaf0c 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -93,14 +93,12 @@ async function changeIsForS3DeployCustomResourcePolicy( } else if (roleRef.Type === 'AWS::IAM::Policy') { if (roleRef.LogicalId !== logicalId) { console.log(roleRef.LogicalId) - debugger; console.log(logicalId) console.log('ah 6') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } else if (roleRef.Type !== 'AWS::IAM::Policy') { console.log('ah 7') - debugger; return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index b4f6f740ced52..2d933d77129bb 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -258,7 +258,6 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', describe('old-style synthesis', () => { let serviceRole: any; let policy: any; - let policy2: any; let deploymentLambda: any; let s3Deployment: any; beforeEach(() => { @@ -303,29 +302,7 @@ describe('old-style synthesis', () => { }, }; - policy2 = { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'DifferentBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }; - + // TODO: inline policy and policy2 where possible. Also rename the Foo1 and Foo2 IDs to be FooOld and FooNew deploymentLambda = { Type: 'AWS::Lambda::Function', Role: { @@ -362,7 +339,28 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy: policy, + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, }, @@ -372,18 +370,32 @@ describe('old-style synthesis', () => { s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - policy.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }; - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, - Policy: policy, + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, }, @@ -413,37 +425,104 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy1: policy, - Policy2: policy2, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + Policy2: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, }, }); - policy.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }; - - policy2.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'DifferentBucketNew', - 'Arn', - ], - }; - policy2.Properties.Roles = [ - { Ref: 'ServiceRole' }, - 'different-role', - ]; - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, - Policy1: policy, - Policy2: policy2, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + Policy2: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + 'different-role', + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, }, @@ -463,7 +542,28 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy: policy, + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, Endpoint: { @@ -482,13 +582,6 @@ describe('old-style synthesis', () => { }, }); - policy.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }; - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; @@ -497,7 +590,28 @@ describe('old-style synthesis', () => { template: { Resources: { ServiceRole: serviceRole, - Policy: policy, + Policy: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, Endpoint: { @@ -542,16 +656,54 @@ describe('old-style synthesis', () => { }, }; - policy2.Properties.Roles = [ - { Ref: 'ServiceRole2' }, - ]; - setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, ServiceRole2: serviceRole, - Policy1: policy, - Policy2: policy2, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + Policy2: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole2' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3DeploymentLambda2: deploymentLambda, S3Deployment: s3Deployment, @@ -559,20 +711,6 @@ describe('old-style synthesis', () => { }, }); - policy.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }; - - policy2.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'DifferentBucketNew', - 'Arn', - ], - }; - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; @@ -586,8 +724,50 @@ describe('old-style synthesis', () => { Resources: { ServiceRole: serviceRole, ServiceRole2: serviceRole, - Policy1: policy, - Policy2: policy2, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + Policy2: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole2' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3DeploymentLambda2: deploymentLambda, S3Deployment: s3Deployment, @@ -637,8 +817,50 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy1: policy, - Policy2: policy2, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + Policy2: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, }, @@ -659,8 +881,50 @@ describe('old-style synthesis', () => { template: { Resources: { ServiceRole: serviceRole, - Policy1: policy, - Policy2: policy2, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, + Policy2: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, }, @@ -680,7 +944,28 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy1: policy, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, NotADeployment: { @@ -709,7 +994,28 @@ describe('old-style synthesis', () => { template: { Resources: { ServiceRole: serviceRole, - Policy1: policy, + Policy1: { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }, S3DeploymentLambda: deploymentLambda, S3Deployment: s3Deployment, NotADeployment: { From 43b9e535cfbf467da583363b8f1096f421f79ae5 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Fri, 3 Dec 2021 14:16:20 -0800 Subject: [PATCH 23/27] improved testing --- .../s3-bucket-hotswap-deployments.test.ts | 716 +++++------------- 1 file changed, 184 insertions(+), 532 deletions(-) diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 2d933d77129bb..960eba5249838 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -73,72 +73,6 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in }); }); -/* -test('correctly evaluates the service token when it references a lambda found in the template', async () => { - // GIVEN - setup.setCurrentCfnStackTemplate({ - Resources: { - Lambda: { - Type: 'AWS::Lambda::Function', - }, - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: ['src-bucket'], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'dest-bucket', - }, - }, - }, - }); - setup.pushStackResourceSummaries(setup.stackSummaryOf('Lambda', 'AWS::Lambda::Function', 'my-deployment-lambda')); - const cdkStackArtifact = setup.cdkStackArtifactOf({ - template: { - Resources: { - Lambda: { - Type: 'AWS::Lambda::Function', - }, - S3Deployment: { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: { - 'Fn::GetAtt': ['Lambda', 'Arn'], - }, - SourceBucketNames: ['src-bucket'], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'dest-bucket', - }, - }, - }, - }, - }); - - // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); - - // THEN - expect(deployStackResult).not.toBeUndefined(); - expect(mockLambdaInvoke).toHaveBeenCalledWith({ - FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', - Payload: JSON.stringify({ - ...payloadWithoutCustomResProps, - ResourceProperties: { - SourceBucketNames: ['src-bucket'], - SourceObjectKeys: [ - 'src-key-old', - ], - DestinationBucketName: 'dest-bucket', - }, - }), - }); -}); -*/ - test('does not call the invoke() API when a resource with type that is not Custom::CDKBucketDeployment but has the same properties is changed', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ @@ -256,78 +190,157 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', }); describe('old-style synthesis', () => { - let serviceRole: any; - let policy: any; - let deploymentLambda: any; - let s3Deployment: any; - beforeEach(() => { - serviceRole = { - Type: 'AWS::IAM::Role', - Properties: { - AssumeRolePolicyDocument: { - Statement: [ - { - Action: 'sts:AssumeRole', - Effect: 'Allow', - Principal: { - Service: 'lambda.amazonaws.com', - }, + const serviceRole = { + Type: 'AWS::IAM::Role', + Properties: { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { + Service: 'lambda.amazonaws.com', }, - ], - Version: '2012-10-17', - }, + }, + ], + Version: '2012-10-17', }, - }; + }, + }; - policy = { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, + const policyOld = { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, + }, + }, + }; + + const policyNew = { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketNew', + 'Arn', + ], }, - ], - }, + }, + ], }, - }; + }, + }; + + const policy2Old = { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'WebsiteBucketOld', + 'Arn', + ], + }, + }, + ], + }, + }, + }; + + const policy2New = { + Type: 'AWS::IAM::Policy', + Properties: { + Roles: [ + { Ref: 'ServiceRole2' }, + ], + PolicyDocument: { + Statement: [ + { + Action: ['s3:GetObject*'], + Effect: 'Allow', + Resource: { + 'Fn::GetAtt': [ + 'DifferentBucketNew', + 'Arn', + ], + }, + }, + ], + }, + }, + }; + + const deploymentLambda = { + Type: 'AWS::Lambda::Function', + Role: { + 'Fn::GetAtt': [ + 'ServiceRole', + 'Arn', + ], + }, + }; - // TODO: inline policy and policy2 where possible. Also rename the Foo1 and Foo2 IDs to be FooOld and FooNew - deploymentLambda = { - Type: 'AWS::Lambda::Function', - Role: { + const s3DeploymentOld = { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { 'Fn::GetAtt': [ - 'ServiceRole', + 'S3DeploymentLambda', 'Arn', ], }, - }; + SourceBucketNames: ['src-bucket-old'], + SourceObjectKeys: ['src-key-old'], + DestinationBucketName: 'WebsiteBucketOld', + }, + }; - s3Deployment = { - Type: 'Custom::CDKBucketDeployment', - Properties: { - ServiceToken: { - 'Fn::GetAtt': [ - 'S3DeploymentLambda', - 'Arn', - ], - }, - SourceBucketNames: ['src-bucket-old'], - SourceObjectKeys: ['src-key-old'], - DestinationBucketName: 'WebsiteBucketOld', + const s3DeploymentNew = { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { + 'Fn::GetAtt': [ + 'S3DeploymentLambda', + 'Arn', + ], }, - }; + SourceBucketNames: ['src-bucket-new'], + SourceObjectKeys: ['src-key-new'], + DestinationBucketName: 'WebsiteBucketNew', + }, + }; + beforeEach(() => { setup.pushStackResourceSummaries( setup.stackSummaryOf('S3DeploymentLambda', 'AWS::Lambda::Function', 'my-deployment-lambda'), setup.stackSummaryOf('ServiceRole', 'AWS::IAM::Role', 'my-service-role'), @@ -339,65 +352,19 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy: policyOld, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentOld, }, }); - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; - s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; - s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, - Policy: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy: policyNew, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentNew, }, }, }); @@ -425,52 +392,10 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - Policy2: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'DifferentBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyOld, + Policy2: policy2Old, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentOld, }, }); @@ -478,30 +403,8 @@ describe('old-style synthesis', () => { template: { Resources: { ServiceRole: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyNew, Policy2: { - Type: 'AWS::IAM::Policy', Properties: { Roles: [ { Ref: 'ServiceRole' }, @@ -524,7 +427,7 @@ describe('old-style synthesis', () => { }, }, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentNew, }, }, }); @@ -542,30 +445,9 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy: policyOld, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentOld, Endpoint: { Type: 'AWS::Lambda::Permission', Properties: { @@ -582,38 +464,13 @@ describe('old-style synthesis', () => { }, }); - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; - s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; - s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, - Policy: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy: policyNew, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentNew, Endpoint: { Type: 'AWS::Lambda::Permission', Properties: { @@ -641,7 +498,7 @@ describe('old-style synthesis', () => { test('calls the lambdaInvoke() API when it receives an asset difference in two s3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { // GIVEN - let s3Deployment2 = { + const s3Deployment2Old = { Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: { @@ -656,122 +513,46 @@ describe('old-style synthesis', () => { }, }; + const s3Deployment2New = { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { + 'Fn::GetAtt': [ + 'S3DeploymentLambda2', + 'Arn', + ], + }, + SourceBucketNames: ['src-bucket-new'], + SourceObjectKeys: ['src-key-new'], + DestinationBucketName: 'DifferentBucketNew', + }, + }; + setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, ServiceRole2: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - Policy2: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole2' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'DifferentBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyOld, + Policy2: policy2Old, S3DeploymentLambda: deploymentLambda, S3DeploymentLambda2: deploymentLambda, - S3Deployment: s3Deployment, - S3Deployment2: s3Deployment2, + S3Deployment: s3DeploymentOld, + S3Deployment2: s3Deployment2Old, }, }); - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; - s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; - s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - - s3Deployment2.Properties.SourceBucketNames = ['src-bucket-new']; - s3Deployment2.Properties.SourceObjectKeys = ['src-key-new']; - s3Deployment2.Properties.DestinationBucketName = 'DifferentBucketNew'; const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, ServiceRole2: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - Policy2: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole2' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'DifferentBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyNew, + Policy2: policy2New, S3DeploymentLambda: deploymentLambda, S3DeploymentLambda2: deploymentLambda, - S3Deployment: s3Deployment, - S3Deployment2: s3Deployment2, + S3Deployment: s3DeploymentNew, + S3Deployment2: s3Deployment2New, }, }, }); @@ -786,7 +567,6 @@ describe('old-style synthesis', () => { // THEN expect(deployStackResult).not.toBeUndefined(); - expect(mockLambdaInvoke).toHaveBeenCalledWith({ FunctionName: 'arn:aws:lambda:here:123456789012:function:my-deployment-lambda', Payload: JSON.stringify({ @@ -817,94 +597,19 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, - Policy2: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'DifferentBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyOld, + Policy2: policy2Old, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentOld, }, }); - policy.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }; - - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; - s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; - s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyNew, Policy2: { - Type: 'AWS::IAM::Policy', Properties: { Roles: [ { Ref: 'ServiceRole' }, @@ -916,7 +621,7 @@ describe('old-style synthesis', () => { Effect: 'Allow', Resource: { 'Fn::GetAtt': [ - 'DifferentBucketOld', + 'DifferentBucketNew', 'Arn', ], }, @@ -926,7 +631,7 @@ describe('old-style synthesis', () => { }, }, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentNew, }, }, }); @@ -944,30 +649,9 @@ describe('old-style synthesis', () => { setup.setCurrentCfnStackTemplate({ Resources: { ServiceRole: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyOld, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentOld, NotADeployment: { Type: 'AWS::Not::S3Deployment', Properties: { @@ -979,45 +663,13 @@ describe('old-style synthesis', () => { }, }); - policy.Properties.PolicyDocument.Statement[0].Resource = { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }; - - s3Deployment.Properties.SourceBucketNames = ['src-bucket-new']; - s3Deployment.Properties.SourceObjectKeys = ['src-key-new']; - s3Deployment.Properties.DestinationBucketName = 'WebsiteBucketNew'; - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { ServiceRole: serviceRole, - Policy1: { - Type: 'AWS::IAM::Policy', - Properties: { - Roles: [ - { Ref: 'ServiceRole' }, - ], - PolicyDocument: { - Statement: [ - { - Action: ['s3:GetObject*'], - Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], - }, - }, - ], - }, - }, - }, + Policy1: policyNew, S3DeploymentLambda: deploymentLambda, - S3Deployment: s3Deployment, + S3Deployment: s3DeploymentNew, NotADeployment: { Type: 'AWS::Not::S3Deployment', Properties: { From df69b2684525f5f0d98a5fa78cdfecab94475f32 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Fri, 3 Dec 2021 15:00:29 -0800 Subject: [PATCH 24/27] updated tests to incorporate latest from master --- .../lib/api/hotswap/s3-bucket-deployments.ts | 13 +++------- .../s3-bucket-hotswap-deployments.test.ts | 24 +++++++++---------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index d524bc2bdaf0c..859b3d16c9240 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -2,7 +2,6 @@ import { ISDK } from '../aws-auth'; import { ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; -/*eslint-disable*/ /** * This means that the value is required to exist by CloudFormation's API (or our S3 Bucket Deployment Lambda) * but the actual value specified is irrelevant @@ -19,14 +18,12 @@ export async function isHotswappableS3BucketDeploymentChange( } if (change.newValue.Type !== 'Custom::CDKBucketDeployment') { - console.log('ah 1') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } // note that this gives the ARN of the lambda, not the name. This is fine though, the invoke() sdk call will take either const functionName = await establishResourcePhysicalName(logicalId, change.newValue.Properties?.ServiceToken, evaluateCfnTemplate); if (!functionName) { - console.log('ah 2') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } @@ -39,6 +36,8 @@ export async function isHotswappableS3BucketDeploymentChange( } class S3BucketDeploymentHotswapOperation implements HotswapOperation { + public readonly service = 'custom-s3-deployment'; + constructor(private readonly functionName: string, private readonly customResourceProperties: any) { } @@ -67,14 +66,12 @@ async function changeIsForS3DeployCustomResourcePolicy( ): Promise { const roles = change.newValue.Properties?.Roles; if (!roles) { - console.log('ah 3') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } for (const role of roles) { const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(await evaluateCfnTemplate.evaluateCfnExpression(role)); if (!roleLogicalId) { - console.log('ah 4') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } @@ -86,19 +83,14 @@ async function changeIsForS3DeployCustomResourcePolicy( // If S3Deployment -> Lambda -> Role and IAM::Policy -> Role, then this IAM::Policy change is an // artifact of old-style synthesis if (lambdaRef.Type !== 'Custom::CDKBucketDeployment') { - console.log('ah 5') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } } else if (roleRef.Type === 'AWS::IAM::Policy') { if (roleRef.LogicalId !== logicalId) { - console.log(roleRef.LogicalId) - console.log(logicalId) - console.log('ah 6') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } else if (roleRef.Type !== 'AWS::IAM::Policy') { - console.log('ah 7') return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } @@ -126,6 +118,7 @@ function stringifyObject(obj: any): any { } class EmptyHotswapOperation implements HotswapOperation { + readonly service = 'AWS::IAM::Policy'; public async apply(sdk: ISDK): Promise { return Promise.resolve(sdk); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 960eba5249838..3f80f747b29dd 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -3,7 +3,7 @@ import { REQUIRED_BY_CFN } from '../../../lib/api/hotswap/s3-bucket-deployments' import * as setup from './hotswap-test-setup'; let mockLambdaInvoke: (params: Lambda.Types.InvocationRequest) => Lambda.Types.InvocationResponse; -let cfnMockProvider: setup.CfnMockProvider; +let hotswapMockSdkProvider: setup.HotswapMockSdkProvider; const payloadWithoutCustomResProps = { RequestType: 'Update', @@ -15,9 +15,9 @@ const payloadWithoutCustomResProps = { }; beforeEach(() => { - cfnMockProvider = setup.setupHotswapTests(); + hotswapMockSdkProvider = setup.setupHotswapTests(); mockLambdaInvoke = jest.fn(); - cfnMockProvider.setInvokeLambdaMock(mockLambdaInvoke); + hotswapMockSdkProvider.setInvokeLambdaMock(mockLambdaInvoke); }); test('calls the lambdaInvoke() API when it receives only an asset difference in an s3 bucket deployment', async () => { @@ -54,7 +54,7 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).not.toBeUndefined(); @@ -105,7 +105,7 @@ test('does not call the invoke() API when a resource with type that is not Custo }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).toBeUndefined(); @@ -182,7 +182,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).toBeUndefined(); @@ -370,7 +370,7 @@ describe('old-style synthesis', () => { }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).not.toBeUndefined(); @@ -433,7 +433,7 @@ describe('old-style synthesis', () => { }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).toBeUndefined(); @@ -489,7 +489,7 @@ describe('old-style synthesis', () => { }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).toBeUndefined(); @@ -563,7 +563,7 @@ describe('old-style synthesis', () => { setup.stackSummaryOf('ServiceRole2', 'AWS::IAM::Role', 'my-service-role-2'), ); - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).not.toBeUndefined(); @@ -637,7 +637,7 @@ describe('old-style synthesis', () => { }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).toBeUndefined(); @@ -683,7 +683,7 @@ describe('old-style synthesis', () => { }); // WHEN - const deployStackResult = await cfnMockProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); // THEN expect(deployStackResult).toBeUndefined(); From 302a9558d915b87b9f0f9d9d3f6e944e59339c89 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 8 Dec 2021 19:06:11 -0800 Subject: [PATCH 25/27] addressed review comments --- .../lib/api/hotswap/s3-bucket-deployments.ts | 19 ++- .../test/api/hotswap/hotswap-test-setup.ts | 4 +- .../s3-bucket-hotswap-deployments.test.ts | 132 ++++++++++++------ 3 files changed, 96 insertions(+), 59 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts index 859b3d16c9240..49ba4ce129759 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -1,5 +1,5 @@ import { ISDK } from '../aws-auth'; -import { ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate, establishResourcePhysicalName } from './common'; +import { ChangeHotswapImpact, ChangeHotswapResult, HotswapOperation, HotswappableChangeCandidate/*, establishResourcePhysicalName*/ } from './common'; import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; /** @@ -22,7 +22,7 @@ export async function isHotswappableS3BucketDeploymentChange( } // note that this gives the ARN of the lambda, not the name. This is fine though, the invoke() sdk call will take either - const functionName = await establishResourcePhysicalName(logicalId, change.newValue.Properties?.ServiceToken, evaluateCfnTemplate); + const functionName = await evaluateCfnTemplate.evaluateCfnExpression(change.newValue.Properties?.ServiceToken); if (!functionName) { return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } @@ -42,9 +42,6 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { } public async apply(sdk: ISDK): Promise { - // JSON.stringify() doesn't turn the actual objects to strings, but the lambda expects strings - const resourceProperties = stringifyObject(this.customResourceProperties); - return sdk.lambda().invoke({ FunctionName: this.functionName, // Lambda refuses to take a direct JSON object and requires it to be stringify()'d @@ -55,15 +52,15 @@ class S3BucketDeploymentHotswapOperation implements HotswapOperation { StackId: REQUIRED_BY_CFN, RequestId: REQUIRED_BY_CFN, LogicalResourceId: REQUIRED_BY_CFN, - ResourceProperties: resourceProperties, + ResourceProperties: stringifyObject(this.customResourceProperties), // JSON.stringify() doesn't turn the actual objects to strings, but the lambda expects strings }), }).promise(); } } async function changeIsForS3DeployCustomResourcePolicy( - logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, -): Promise { + iamPolicyLogicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, +): Promise { const roles = change.newValue.Properties?.Roles; if (!roles) { return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; @@ -87,10 +84,10 @@ async function changeIsForS3DeployCustomResourcePolicy( } } } else if (roleRef.Type === 'AWS::IAM::Policy') { - if (roleRef.LogicalId !== logicalId) { + if (roleRef.LogicalId !== iamPolicyLogicalId) { return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } - } else if (roleRef.Type !== 'AWS::IAM::Policy') { + } else { return ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT; } } @@ -118,7 +115,7 @@ function stringifyObject(obj: any): any { } class EmptyHotswapOperation implements HotswapOperation { - readonly service = 'AWS::IAM::Policy'; + readonly service = 'empty'; public async apply(sdk: ISDK): Promise { return Promise.resolve(sdk); } 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 83942a33b42ae..056e4728ff354 100644 --- a/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts +++ b/packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts @@ -41,8 +41,8 @@ export function pushStackResourceSummaries(...items: CloudFormation.StackResourc currentCfnStackResources.push(...items); } -export function setCurrentCfnStackTemplate(_template: Template) { - const templateDeepCopy = JSON.parse(JSON.stringify(_template)); // deep copy the _template, so our tests can mutate one template instead of creating two +export function setCurrentCfnStackTemplate(template: Template) { + const templateDeepCopy = JSON.parse(JSON.stringify(template)); // deep copy the template, so our tests can mutate one template instead of creating two currentCfnStack.setTemplate(templateDeepCopy); } diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 3f80f747b29dd..9b67592abe52e 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -20,7 +20,7 @@ beforeEach(() => { hotswapMockSdkProvider.setInvokeLambdaMock(mockLambdaInvoke); }); -test('calls the lambdaInvoke() API when it receives only an asset difference in an s3 bucket deployment', async () => { +test('calls the lambdaInvoke() API when it receives only an asset difference in an S3 bucket deployment and evaluates CFN expressions in S3 Deployment Properties', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { @@ -44,7 +44,12 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in Properties: { ServiceToken: 'a-lambda-arn', SourceBucketNames: ['src-bucket'], - SourceObjectKeys: ['src-key-new'], + SourceObjectKeys: { + 'Fn::Split': [ + '-', + 'key1-key2-key3', + ], + }, DestinationBucketName: 'dest-bucket', DestinationBucketKeyPrefix: 'my-key/some-new-prefix', }, @@ -65,7 +70,7 @@ test('calls the lambdaInvoke() API when it receives only an asset difference in ...payloadWithoutCustomResProps, ResourceProperties: { SourceBucketNames: ['src-bucket'], - SourceObjectKeys: ['src-key-new'], + SourceObjectKeys: ['key1', 'key2', 'key3'], DestinationBucketName: 'dest-bucket', DestinationBucketKeyPrefix: 'my-key/some-new-prefix', }, @@ -80,10 +85,7 @@ test('does not call the invoke() API when a resource with type that is not Custo S3Deployment: { Type: 'Custom::NotCDKBucketDeployment', Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: ['src-bucket'], SourceObjectKeys: ['src-key-old'], - DestinationBucketName: 'dest-bucket', }, }, }, @@ -94,10 +96,7 @@ test('does not call the invoke() API when a resource with type that is not Custo S3Deployment: { Type: 'Custom::NotCDKBucketDeployment', Properties: { - ServiceToken: 'a-lambda-arn', - SourceBucketNames: ['src-bucket'], SourceObjectKeys: ['src-key-new'], - DestinationBucketName: 'dest-bucket', }, }, }, @@ -115,14 +114,16 @@ test('does not call the invoke() API when a resource with type that is not Custo test('does not call the invokeLambda() api if the updated Policy has no Roles', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ + Parameters: { + WebsiteBucketParamOld: { Type: 'String' }, + WebsiteBucketParamNew: { Type: 'String' }, + }, Resources: { S3Deployment: { Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: ['src-bucket'], SourceObjectKeys: ['src-key-old'], - DestinationBucketName: 'dest-bucket', }, }, Policy: { @@ -134,10 +135,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], + Ref: 'WebsiteBucketParamOld', }, }, ], @@ -148,14 +146,16 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', }); const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { + Parameters: { + WebsiteBucketParamOld: { Type: 'String' }, + WebsiteBucketParamNew: { Type: 'String' }, + }, Resources: { S3Deployment: { Type: 'Custom::CDKBucketDeployment', Properties: { ServiceToken: 'a-lambda-arn', - SourceBucketNames: ['src-bucket'], SourceObjectKeys: ['src-key-new'], - DestinationBucketName: 'dest-bucket', }, }, Policy: { @@ -167,10 +167,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], + Ref: 'WebsiteBucketParamNew', }, }, ], @@ -189,7 +186,56 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); +test('throws an error when the serviceToken fails evaluation in the template', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { + Ref: 'BadLamba', + }, + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-old'], + DestinationBucketName: 'dest-bucket', + }, + }, + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + S3Deployment: { + Type: 'Custom::CDKBucketDeployment', + Properties: { + ServiceToken: { + Ref: 'BadLamba', + }, + SourceBucketNames: ['src-bucket'], + SourceObjectKeys: ['src-key-new'], + DestinationBucketName: 'dest-bucket', + }, + }, + }, + }, + }); + + // WHEN + await expect(() => + hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact), + ).rejects.toThrow(/Parameter or resource 'BadLamba' could not be found for evaluation/); + + expect(mockLambdaInvoke).not.toHaveBeenCalled(); +}); + describe('old-style synthesis', () => { + const parameters = { + WebsiteBucketParamOld: { Type: 'String' }, + WebsiteBucketParamNew: { Type: 'String' }, + DifferentBucketParamNew: { Type: 'String' }, + }; + const serviceRole = { Type: 'AWS::IAM::Role', Properties: { @@ -220,10 +266,7 @@ describe('old-style synthesis', () => { Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], + Ref: 'WebsiteBucketParamOld', }, }, ], @@ -243,10 +286,7 @@ describe('old-style synthesis', () => { Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketNew', - 'Arn', - ], + Ref: 'WebsiteBucketParamNew', }, }, ], @@ -266,10 +306,7 @@ describe('old-style synthesis', () => { Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { - 'Fn::GetAtt': [ - 'WebsiteBucketOld', - 'Arn', - ], + Ref: 'WebsiteBucketParamOld', }, }, ], @@ -289,10 +326,7 @@ describe('old-style synthesis', () => { Action: ['s3:GetObject*'], Effect: 'Allow', Resource: { - 'Fn::GetAtt': [ - 'DifferentBucketNew', - 'Arn', - ], + Ref: 'DifferentBucketParamOld', }, }, ], @@ -347,10 +381,11 @@ describe('old-style synthesis', () => { ); }); - test('calls the lambdaInvoke() API when it receives an asset difference in an s3 bucket deployment and an IAM Policy difference using old-style synthesis', async () => { + test('calls the lambdaInvoke() API when it receives an asset difference in an S3 bucket deployment and an IAM Policy difference using old-style synthesis', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { + Parameters: parameters, ServiceRole: serviceRole, Policy: policyOld, S3DeploymentLambda: deploymentLambda, @@ -361,6 +396,7 @@ describe('old-style synthesis', () => { const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { + Parameters: parameters, ServiceRole: serviceRole, Policy: policyNew, S3DeploymentLambda: deploymentLambda, @@ -370,7 +406,7 @@ describe('old-style synthesis', () => { }); // WHEN - const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact, { WebsiteBucketParamOld: 'WebsiteBucketOld', WebsiteBucketParamNew: 'WebsiteBucketNew' }); // THEN expect(deployStackResult).not.toBeUndefined(); @@ -387,7 +423,7 @@ describe('old-style synthesis', () => { }); }); - test('does not call the lambdaInvoke() API when the difference in the s3 deployment is referred to in one IAM policy change but not another', async () => { + test('does not call the lambdaInvoke() API when the difference in the S3 deployment is referred to in one IAM policy change but not another', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { @@ -440,7 +476,7 @@ describe('old-style synthesis', () => { expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); - test('does not call the lambdaInvoke() API when the lambda that references the role is referred to by something other than an s3 deployment', async () => { + test('does not call the lambdaInvoke() API when the lambda that references the role is referred to by something other than an S3 deployment', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { @@ -496,7 +532,7 @@ describe('old-style synthesis', () => { expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); - test('calls the lambdaInvoke() API when it receives an asset difference in two s3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { + test('calls the lambdaInvoke() API when it receives an asset difference in two S3 bucket deployments and IAM Policy differences using old-style synthesis', async () => { // GIVEN const s3Deployment2Old = { Type: 'Custom::CDKBucketDeployment', @@ -541,10 +577,10 @@ describe('old-style synthesis', () => { }, }); - const cdkStackArtifact = setup.cdkStackArtifactOf({ template: { Resources: { + Parameters: parameters, ServiceRole: serviceRole, ServiceRole2: serviceRole, Policy1: policyNew, @@ -563,7 +599,11 @@ describe('old-style synthesis', () => { setup.stackSummaryOf('ServiceRole2', 'AWS::IAM::Role', 'my-service-role-2'), ); - const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact, { + WebsiteBucketParamOld: 'WebsiteBucketOld', + WebsiteBucketParamNew: 'WebsiteBucketNew', + DifferentBucketParamNew: 'WebsiteBucketNew', + }); // THEN expect(deployStackResult).not.toBeUndefined(); @@ -592,7 +632,7 @@ describe('old-style synthesis', () => { }); }); - test('does not call the lambdaInvoke() API when it receives an asset difference in an s3 bucket deployment that references two different policies', async () => { + test('does not call the lambdaInvoke() API when it receives an asset difference in an S3 bucket deployment that references two different policies', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { @@ -644,7 +684,7 @@ describe('old-style synthesis', () => { expect(mockLambdaInvoke).not.toHaveBeenCalled(); }); - test('does not call the lambdaInvoke() API when a policy is referenced by a resource that is not an s3 deployment', async () => { + test('does not call the lambdaInvoke() API when a policy is referenced by a resource that is not an S3 deployment', async () => { // GIVEN setup.setCurrentCfnStackTemplate({ Resources: { From 3750683ed43e25d7bae0f5af7e04725454070ae5 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Thu, 9 Dec 2021 07:41:35 -0800 Subject: [PATCH 26/27] added PolicyName property for correctness --- .../test/api/hotswap/s3-bucket-hotswap-deployments.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 9b67592abe52e..4a0d906960795 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -129,6 +129,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Policy: { Type: 'AWS::IAM::Policy', Properties: { + PolicyName: 'my-policy', PolicyDocument: { Statement: [ { @@ -161,6 +162,7 @@ test('does not call the invokeLambda() api if the updated Policy has no Roles', Policy: { Type: 'AWS::IAM::Policy', Properties: { + PolicyName: 'my-policy', PolicyDocument: { Statement: [ { @@ -257,6 +259,7 @@ describe('old-style synthesis', () => { const policyOld = { Type: 'AWS::IAM::Policy', Properties: { + PolicyName: 'my-policy', Roles: [ { Ref: 'ServiceRole' }, ], @@ -277,6 +280,7 @@ describe('old-style synthesis', () => { const policyNew = { Type: 'AWS::IAM::Policy', Properties: { + PolicyName: 'my-policy', Roles: [ { Ref: 'ServiceRole' }, ], @@ -297,6 +301,7 @@ describe('old-style synthesis', () => { const policy2Old = { Type: 'AWS::IAM::Policy', Properties: { + PolicyName: 'my-policy', Roles: [ { Ref: 'ServiceRole' }, ], @@ -317,6 +322,7 @@ describe('old-style synthesis', () => { const policy2New = { Type: 'AWS::IAM::Policy', Properties: { + PolicyName: 'my-policy', Roles: [ { Ref: 'ServiceRole2' }, ], From afcb3f5624e61d8c3aefd64ee36ba9e1955893b6 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Thu, 9 Dec 2021 07:44:25 -0800 Subject: [PATCH 27/27] policies that appear in the same template should have different names --- .../api/hotswap/s3-bucket-hotswap-deployments.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts index 4a0d906960795..3a015308fcadc 100644 --- a/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/s3-bucket-hotswap-deployments.test.ts @@ -259,7 +259,7 @@ describe('old-style synthesis', () => { const policyOld = { Type: 'AWS::IAM::Policy', Properties: { - PolicyName: 'my-policy', + PolicyName: 'my-policy-old', Roles: [ { Ref: 'ServiceRole' }, ], @@ -280,7 +280,7 @@ describe('old-style synthesis', () => { const policyNew = { Type: 'AWS::IAM::Policy', Properties: { - PolicyName: 'my-policy', + PolicyName: 'my-policy-new', Roles: [ { Ref: 'ServiceRole' }, ], @@ -301,7 +301,7 @@ describe('old-style synthesis', () => { const policy2Old = { Type: 'AWS::IAM::Policy', Properties: { - PolicyName: 'my-policy', + PolicyName: 'my-policy-old-2', Roles: [ { Ref: 'ServiceRole' }, ], @@ -322,7 +322,7 @@ describe('old-style synthesis', () => { const policy2New = { Type: 'AWS::IAM::Policy', Properties: { - PolicyName: 'my-policy', + PolicyName: 'my-policy-new-2', Roles: [ { Ref: 'ServiceRole2' }, ],