fix(cli): IAM Policy changes not deploying with --hotswap-fallback (#…

…28185)

The hotswappable resource detectors failed to correctly identify `AWS::IAM::Policy` resources as not-hotswappable.

When `--hotswap-fallback` was used and the only change to the stack was with `AWS::IAM::Policy`, this caused the deploy command to first report IAM changes, and then report `no changes` on the stack.

<img width="1076" alt="image" src="https://github.com/aws/aws-cdk/assets/379814/d77320bc-fc8d-4b70-b710-2c28467d07e5">

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/aws-cdk/lib/api/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<ChangeHotswapResult> => {
+    // 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 () => [],
 };

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<ChangeHotswapResult> {
   // 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<ChangeHotswapResult> {
-  const roles = change.newValue.Properties?.Roles;
-  if (!roles) {
-    return reportNonHotswappableResource(
-      change,
-      'This IAM Policy does not have have any Roles',
-    );
+): Promise<boolean> {
+  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 {

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);
+    }
+  });
+});

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,
           },