diff --git a/packages/aws-cdk/lib/api/hotswap-deployments.ts b/packages/aws-cdk/lib/api/hotswap-deployments.ts index 86fc0d43aa3c6..0188f2fbd1cf3 100644 --- a/packages/aws-cdk/lib/api/hotswap-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap-deployments.ts @@ -6,10 +6,10 @@ import { DeployStackResult } from './deploy-stack'; import { EvaluateCloudFormationTemplate } from './evaluate-cloudformation-template'; import { isHotswappableAppSyncChange } from './hotswap/appsync-mapping-templates'; import { isHotswappableCodeBuildProjectChange } from './hotswap/code-build-projects'; -import { ICON, ChangeHotswapResult, HotswapMode, HotswappableChange, NonHotswappableChange, HotswappableChangeCandidate, ClassifiedResourceChanges, reportNonHotswappableChange } from './hotswap/common'; +import { ICON, ChangeHotswapResult, HotswapMode, HotswappableChange, NonHotswappableChange, HotswappableChangeCandidate, ClassifiedResourceChanges, reportNonHotswappableChange, reportNonHotswappableResource } from './hotswap/common'; import { isHotswappableEcsServiceChange } from './hotswap/ecs-services'; import { isHotswappableLambdaFunctionChange } from './hotswap/lambda-functions'; -import { isHotswappableS3BucketDeploymentChange } from './hotswap/s3-bucket-deployments'; +import { skipChangeForS3DeployCustomResourcePolicy, isHotswappableS3BucketDeploymentChange } from './hotswap/s3-bucket-deployments'; import { isHotswappableStateMachineChange } from './hotswap/stepfunctions-state-machines'; import { loadCurrentTemplateWithNestedStacks, NestedStackNames } from './nested-stack-helpers'; import { CloudFormationStack } from './util/cloudformation'; @@ -35,7 +35,16 @@ const RESOURCE_DETECTORS: { [key: string]: HotswapDetector } = { 'AWS::CodeBuild::Project': isHotswappableCodeBuildProjectChange, 'AWS::StepFunctions::StateMachine': isHotswappableStateMachineChange, 'Custom::CDKBucketDeployment': isHotswappableS3BucketDeploymentChange, - 'AWS::IAM::Policy': isHotswappableS3BucketDeploymentChange, + 'AWS::IAM::Policy': async ( + logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, + ): Promise => { + // If the policy is for a S3BucketDeploymentChange, we can ignore the change + if (await skipChangeForS3DeployCustomResourcePolicy(logicalId, change, evaluateCfnTemplate)) { + return []; + } + + return reportNonHotswappableResource(change, 'This resource type is not supported for hotswap deployments'); + }, 'AWS::CDK::Metadata': async () => [], }; 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 1dae7f665f010..4ccfa3a5f72d4 100644 --- a/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts +++ b/packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts @@ -1,4 +1,4 @@ -import { ChangeHotswapResult, HotswappableChangeCandidate, reportNonHotswappableResource } from './common'; +import { ChangeHotswapResult, HotswappableChangeCandidate } from './common'; import { ISDK } from '../aws-auth'; import { EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-template'; @@ -9,14 +9,11 @@ import { EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-templ export const REQUIRED_BY_CFN = 'required-to-be-present-by-cfn'; export async function isHotswappableS3BucketDeploymentChange( - logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, + _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 artifacts that can be safely ignored const ret: ChangeHotswapResult = []; - if (change.newValue.Type === 'AWS::IAM::Policy') { - return changeIsForS3DeployCustomResourcePolicy(logicalId, change, evaluateCfnTemplate); - } if (change.newValue.Type !== 'Custom::CDKBucketDeployment') { return []; @@ -60,60 +57,54 @@ export async function isHotswappableS3BucketDeploymentChange( return ret; } -async function changeIsForS3DeployCustomResourcePolicy( +export async function skipChangeForS3DeployCustomResourcePolicy( iamPolicyLogicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate, -): Promise { - const roles = change.newValue.Properties?.Roles; - if (!roles) { - return reportNonHotswappableResource( - change, - 'This IAM Policy does not have have any Roles', - ); +): Promise { + if (change.newValue.Type !== 'AWS::IAM::Policy') { + return false; + } + const roles: string[] = change.newValue.Properties?.Roles; + + // If no roles are referenced, the policy is definitely not used for a S3Deployment + if (!roles || !roles.length) { + return false; } + // Check if every role this policy is referenced by is only used for a S3Deployment for (const role of roles) { const roleArn = await evaluateCfnTemplate.evaluateCfnExpression(role); const roleLogicalId = await evaluateCfnTemplate.findLogicalIdForPhysicalName(roleArn); + + // We must assume this role is used for something else, because we can't check it if (!roleLogicalId) { - return reportNonHotswappableResource( - change, - `could not find logicalId for role with name '${roleArn}'`, - ); + return false; } - const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalId); - for (const roleRef of roleRefs) { + // Find all interesting reference to the role + const roleRefs = evaluateCfnTemplate.findReferencesTo(roleLogicalId) + // we are not interested in the reference from the original policy - it always exists + .filter(roleRef => !(roleRef.Type == 'AWS::IAM::Policy' && roleRef.LogicalId === iamPolicyLogicalId)); + + // Check if the role is only used for S3Deployment + // We know this is the case, if S3Deployment -> Lambda -> Role is satisfied for every reference + // And we have at least one reference. + const isRoleOnlyForS3Deployment = roleRefs.length >= 1 && roleRefs.every(roleRef => { 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 reportNonHotswappableResource( - change, - `found an AWS::IAM::Policy that has Role '${roleLogicalId}' that is referred to by AWS::Lambda::Function '${roleRef.LogicalId}' that is referred to by ${lambdaRef.Type} '${lambdaRef.LogicalId}', which does not have type 'Custom::CDKBucketDeployment'`, - ); - } - } - } else if (roleRef.Type === 'AWS::IAM::Policy') { - if (roleRef.LogicalId !== iamPolicyLogicalId) { - return reportNonHotswappableResource( - change, - `found an AWS::IAM::Policy that has Role '${roleLogicalId}' that is referred to by AWS::IAM::Policy '${roleRef.LogicalId}' that is not the policy of the s3 bucket deployment`, - ); - } - } else { - return reportNonHotswappableResource( - change, - `found a resource which refers to the role '${roleLogicalId}' that is not of type AWS::Lambda::Function or AWS::IAM::Policy, so the bucket deployment cannot be hotswapped`, - ); + // Every reference must be to the custom resource and at least one reference must be present + return lambdaRefs.length >= 1 && lambdaRefs.every(lambdaRef => lambdaRef.Type === 'Custom::CDKBucketDeployment'); } + return false; + }); + + // We have determined this role is used for something else, so we can't skip the change + if (!isRoleOnlyForS3Deployment) { + return false; } } - // this doesn't block the hotswap, but it also isn't a hotswappable change by itself. Return - // an empty change to signify this. - return []; + // We have checked that any use of this policy is only for S3Deployment and we can safely skip it + return true; } function stringifyObject(obj: any): any { diff --git a/packages/aws-cdk/test/api/hotswap/iam-policy-hotswap-deployment.test.ts b/packages/aws-cdk/test/api/hotswap/iam-policy-hotswap-deployment.test.ts new file mode 100644 index 0000000000000..0f36d8d09efff --- /dev/null +++ b/packages/aws-cdk/test/api/hotswap/iam-policy-hotswap-deployment.test.ts @@ -0,0 +1,131 @@ +/* eslint-disable import/order */ +import * as setup from './hotswap-test-setup'; +import { HotswapMode } from '../../../lib/api/hotswap/common'; + +let hotswapMockSdkProvider: setup.HotswapMockSdkProvider; + +beforeEach(() => { + hotswapMockSdkProvider = setup.setupHotswapTests(); +}); + +describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hotswapMode) => { + test('A change to an IAM Policy results in a full deployment for HOTSWAP and a noOp for HOTSWAP_ONLY', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + RoleOne: { + Type: 'AWS::IAM::Role', + Properties: { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { + Service: 'sqs.amazonaws.com', + }, + }, + ], + Version: '2012-10-17', + }, + }, + }, + RoleDefaultPolicy: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Action: [ + 'sqs:ChangeMessageVisibility', + 'sqs:DeleteMessage', + 'sqs:GetQueueAttributes', + 'sqs:GetQueueUrl', + 'sqs:ReceiveMessage', + ], + Effect: 'Allow', + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + PolicyName: 'roleDefaultPolicy', + Roles: [ + { + Ref: 'RoleOne', + }, + ], + }, + }, + }, + }); + setup.pushStackResourceSummaries({ + LogicalResourceId: 'RoleOne', + PhysicalResourceId: 'RoleOne', + ResourceType: 'AWS::IAM::Role', + ResourceStatus: 'CREATE_COMPLETE', + LastUpdatedTimestamp: new Date(), + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + RoleOne: { + Type: 'AWS::IAM::Role', + Properties: { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { + Service: 'sqs.amazonaws.com', + }, + }, + ], + Version: '2012-10-17', + }, + }, + }, + RoleDefaultPolicy: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyDocument: { + Statement: [ + { + Action: [ + 'sqs:DeleteMessage', + ], + Effect: 'Allow', + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + PolicyName: 'roleDefaultPolicy', + Roles: [ + { + Ref: 'RoleOne', + }, + ], + }, + }, + }, + }, + }); + + if (hotswapMode === HotswapMode.FALL_BACK) { + // WHEN + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact); + + // THEN + expect(deployStackResult).toBeUndefined(); + } else if (hotswapMode === HotswapMode.HOTSWAP_ONLY) { + // WHEN + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(deployStackResult?.noOp).toEqual(true); + } + }); +}); 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 105f00e0aeac2..b0a9662b1d1ac 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 @@ -618,6 +618,26 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot 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 deploymentLambda2Old = { + Type: 'AWS::Lambda::Function', + Role: { + 'Fn::GetAtt': [ + 'ServiceRole', + 'Arn', + ], + }, + }; + + const deploymentLambda2New = { + Type: 'AWS::Lambda::Function', + Role: { + 'Fn::GetAtt': [ + 'ServiceRole2', + 'Arn', + ], + }, + }; + const s3Deployment2Old = { Type: 'Custom::CDKBucketDeployment', Properties: { @@ -655,7 +675,7 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot Policy1: policyOld, Policy2: policy2Old, S3DeploymentLambda: deploymentLambda, - S3DeploymentLambda2: deploymentLambda, + S3DeploymentLambda2: deploymentLambda2Old, S3Deployment: s3DeploymentOld, S3Deployment2: s3Deployment2Old, }, @@ -670,7 +690,7 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot Policy1: policyNew, Policy2: policy2New, S3DeploymentLambda: deploymentLambda, - S3DeploymentLambda2: deploymentLambda, + S3DeploymentLambda2: deploymentLambda2New, S3Deployment: s3DeploymentNew, S3Deployment2: s3Deployment2New, },