fix(elbv2): race condition for Lambda backends

Lambda Targets can get registered into a TargetGroup before the
`lambda:Invoke` permissions are added. Since the TargetGroup does
a permission test, deployment will fail.

Fixes #4663.

Adding a long-overdue mechanism to the IAM library to depend on Grants
that just got created, to ensure permissions are created before the
resources that depend on them (in effect: `Grant`s are made
`IDependable`).

Relates to the root cause of #7236.

packages/@aws-cdk/aws-ecr/lib/repository.ts

@@ -42,7 +42,7 @@ export interface IRepository extends IResource {
   /**
    * Add a policy statement to the repository's resource policy
    */
-  addToResourcePolicy(statement: iam.PolicyStatement): void;
+  addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult;
 
   /**
    * Grant the given principal identity permissions to perform the actions on this repository
@@ -115,7 +115,7 @@ export abstract class RepositoryBase extends Resource implements IRepository {
   /**
    * Add a policy statement to the repository's resource policy
    */
-  public abstract addToResourcePolicy(statement: iam.PolicyStatement): void;
+  public abstract addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult;
 
   /**
    * The URI of this repository (represents the latest image):
@@ -343,8 +343,9 @@ export class Repository extends RepositoryBase {
       public readonly repositoryName = attrs.repositoryName;
       public readonly repositoryArn = attrs.repositoryArn;
 
-      public addToResourcePolicy(_statement: iam.PolicyStatement) {
+      public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
         // dropped
+        return { statementAdded: false };
       }
     }
 
@@ -366,8 +367,9 @@ export class Repository extends RepositoryBase {
       public repositoryName = repositoryName;
       public repositoryArn = repositoryArn;
 
-      public addToResourcePolicy(_statement: iam.PolicyStatement): void {
+      public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
         // dropped
+        return { statementAdded: false };
       }
     }
 
@@ -379,8 +381,9 @@ export class Repository extends RepositoryBase {
       public repositoryName = repositoryName;
       public repositoryArn = Repository.arnForLocalRepository(repositoryName, scope);
 
-      public addToResourcePolicy(_statement: iam.PolicyStatement): void {
+      public addToResourcePolicy(_statement: iam.PolicyStatement): iam.AddToResourcePolicyResult  {
         // dropped
+        return { statementAdded: false };
       }
     }
 
@@ -462,11 +465,12 @@ export class Repository extends RepositoryBase {
     }
   }
 
-  public addToResourcePolicy(statement: iam.PolicyStatement) {
+  public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
     if (this.policyDocument === undefined) {
       this.policyDocument = new iam.PolicyDocument();
     }
     this.policyDocument.addStatements(statement);
+    return { statementAdded: false, policyDependable: this.policyDocument };
   }
 
   /**

packages/@aws-cdk/aws-elasticloadbalancingv2-targets/lib/lambda-target.ts

@@ -18,7 +18,8 @@ export class LambdaTarget implements elbv2.IApplicationLoadBalancerTarget {
    * load balancer.
    */
   public attachToApplicationTargetGroup(targetGroup: elbv2.IApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
-    this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));
+    const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));
+    grant.applyBefore(targetGroup);
     return this.attach(targetGroup);
   }
 
@@ -29,7 +30,8 @@ export class LambdaTarget implements elbv2.IApplicationLoadBalancerTarget {
    * load balancer.
    */
   public attachToNetworkTargetGroup(targetGroup: elbv2.INetworkTargetGroup): elbv2.LoadBalancerTargetProps {
-    this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));
+    const grant = this.fn.grantInvoke(new iam.ServicePrincipal('elasticloadbalancing.amazonaws.com'));
+    grant.applyBefore(targetGroup);
     return this.attach(targetGroup);
   }
 

packages/@aws-cdk/aws-elasticloadbalancingv2-targets/test/integ.lambda-target.expected.json

@@ -409,7 +409,10 @@
           }
         ],
         "TargetType": "lambda"
-      }
+      },
+      "DependsOn": [
+        "FunInvokeServicePrincipalelasticloadbalancingamazonawscomD2CAC0C4"
+      ]
     },
     "FunServiceRole3CC876D7": {
       "Type": "AWS::IAM::Role",

packages/@aws-cdk/aws-elasticloadbalancingv2-targets/test/lambda-target.test.ts

@@ -1,23 +1,28 @@
-import { expect, haveResource } from '@aws-cdk/assert';
+import { expect, haveResource, ResourcePart } from '@aws-cdk/assert';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
 import * as lambda from '@aws-cdk/aws-lambda';
 import { Stack } from '@aws-cdk/core';
 import * as targets from '../lib';
 
-test('Can create target groups with lambda targets', () => {
-  // GIVEN
-  const stack = new Stack();
+let stack: Stack;
+let listener: elbv2.ApplicationListener;
+let fn: lambda.Function;
+
+beforeEach(() => {
+  stack = new Stack();
   const vpc = new ec2.Vpc(stack, 'Stack');
   const lb = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc });
-  const listener = lb.addListener('Listener', { port: 80 });
+  listener = lb.addListener('Listener', { port: 80 });
 
-  const fn = new lambda.Function(stack, 'Fun', {
+  fn = new lambda.Function(stack, 'Fun', {
     code: lambda.Code.inline('foo'),
     runtime: lambda.Runtime.PYTHON_3_6,
     handler: 'index.handler',
   });
+});
 
+test('Can create target groups with lambda targets', () => {
   // WHEN
   listener.addTargets('Targets', {
     targets: [new targets.LambdaTarget(fn)],
@@ -31,3 +36,15 @@ test('Can create target groups with lambda targets', () => {
     ],
   }));
 });
+
+test('Lambda targets create dependency on Invoke permission', () => {
+  // WHEN
+  listener.addTargets('Targets', {
+    targets: [new targets.LambdaTarget(fn)],
+  });
+
+  // THEN
+  expect(stack).to(haveResource('AWS::ElasticLoadBalancingV2::TargetGroup', (def: any) => {
+    return (def.DependsOn ?? []).includes('FunInvokeServicePrincipalelasticloadbalancingamazonawscomD2CAC0C4');
+  }, ResourcePart.CompleteDefinition));
+});

packages/@aws-cdk/aws-iam/lib/grant.ts

@@ -100,7 +100,7 @@ export interface GrantOnPrincipalAndResourceOptions extends CommonGrantOptions {
  * This class is not instantiable by consumers on purpose, so that they will be
  * required to call the Grant factory functions.
  */
-export class Grant {
+export class Grant implements cdk.IDependable {
   /**
    * Grant the given permissions to the principal
    *
@@ -129,9 +129,13 @@ export class Grant {
       principals: [options.grantee!.grantPrincipal],
     });
 
-    options.resource.addToResourcePolicy(statement);
+    const resourceResult = options.resource.addToResourcePolicy(statement);
 
-    return new Grant({ resourceStatement: statement, options });
+    return new Grant({
+      resourceStatement: statement,
+      options,
+      policyDependable: resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined,
+    });
   }
 
   /**
@@ -146,9 +150,16 @@ export class Grant {
       resources: options.resourceArns,
     });
 
-    const addedToPrincipal = options.grantee.grantPrincipal.addToPolicy(statement);
+    const addedToPrincipal = options.grantee.grantPrincipal.addToPrincipalPolicy(statement);
+    if (!addedToPrincipal.statementAdded) {
+      return new Grant({ principalStatement: undefined, options });
+    }
+
+    if (!addedToPrincipal.policyDependable) {
+      throw new Error('Contract violation: when Principal returns statementAdded=true, it should return a dependable');
+    }
 
-    return new Grant({ principalStatement: addedToPrincipal ? statement : undefined, options });
+    return new Grant({ principalStatement: statement, options, policyDependable: addedToPrincipal.policyDependable });
   }
 
   /**
@@ -172,9 +183,15 @@ export class Grant {
       principals: [options.resourcePolicyPrincipal || options.grantee!.grantPrincipal],
     });
 
-    options.resource.addToResourcePolicy(statement);
+    const resourceResult = options.resource.addToResourcePolicy(statement);
+    const resourceDependable = resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined;
 
-    return new Grant({ principalStatement: statement, resourceStatement: result.resourceStatement, options });
+    return new Grant({
+      principalStatement: statement,
+      resourceStatement: result.resourceStatement,
+      options,
+      policyDependable: resourceDependable ? new CompositeDependable(result, resourceDependable) : result,
+    });
   }
 
   /**
@@ -218,6 +235,12 @@ export class Grant {
     this.options = props.options;
     this.principalStatement = props.principalStatement;
     this.resourceStatement = props.resourceStatement;
+
+    cdk.DependableTrait.implement(this, {
+      get dependencyRoots() {
+        return props.policyDependable ? cdk.DependableTrait.get(props.policyDependable).dependencyRoots : [];
+      },
+    });
   }
 
   /**
@@ -236,6 +259,17 @@ export class Grant {
       throw new Error(`${describeGrant(this.options)} could not be added on either identity or resource policy.`);
     }
   }
+
+  /**
+   * Make sure this grant is applied before the given constructs are deployed
+   *
+   * The same as construct.node.addDependency(grant), but slightly nicer to read.
+   */
+  public applyBefore(...constructs: cdk.IConstruct[]) {
+    for (const construct of constructs) {
+      construct.node.addDependency(this);
+    }
+  }
 }
 
 function describeGrant(options: CommonGrantOptions) {
@@ -246,6 +280,13 @@ interface GrantProps {
   readonly options: CommonGrantOptions;
   readonly principalStatement?: PolicyStatement;
   readonly resourceStatement?: PolicyStatement;
+
+  /**
+   * Constructs whose deployment applies the grant
+   *
+   * Used to add dependencies on grants
+   */
+  readonly policyDependable?: cdk.IDependable;
 }
 
 /**
@@ -255,5 +296,40 @@ export interface IResourceWithPolicy extends cdk.IConstruct {
   /**
    * Add a statement to the resource's resource policy
    */
-  addToResourcePolicy(statement: PolicyStatement): void;
+  addToResourcePolicy(statement: PolicyStatement): AddToResourcePolicyResult;
 }
+
+/**
+ * Result of calling addToResourcePolicy
+ */
+export interface AddToResourcePolicyResult {
+  /**
+   * Whether the statement was added
+   */
+  readonly statementAdded: boolean;
+
+  /**
+   * Dependable which allows depending on the policy change being applied
+   *
+   * @default - If `statementAdded` is true, the resource object itself.
+   * Otherwise, no dependable.
+   */
+  readonly policyDependable?: cdk.IDependable;
+}
+
+/**
+ * Composite dependable
+ *
+ * Not as simple as eagerly getting the dependency roots from the
+ * inner dependables, as they may be mutable so we need to defer
+ * the query.
+ */
+export class CompositeDependable implements cdk.IDependable {
+  constructor(...dependables: cdk.IDependable[]) {
+    cdk.DependableTrait.implement(this, {
+      get dependencyRoots(): cdk.IConstruct[] {
+        return Array.prototype.concat.apply([], dependables.map(d => cdk.DependableTrait.get(d).dependencyRoots));
+      },
+    });
+  }
+}

packages/@aws-cdk/aws-iam/lib/group.ts

@@ -4,7 +4,7 @@ import { IIdentity } from './identity-base';
 import { IManagedPolicy } from './managed-policy';
 import { Policy } from './policy';
 import { PolicyStatement } from './policy-statement';
-import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
+import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
 import { IUser } from './user';
 import { AttachedPolicies } from './util';
 
@@ -104,14 +104,18 @@ abstract class GroupBase extends Resource implements IGroup {
   /**
    * Adds an IAM statement to the default policy.
    */
-  public addToPolicy(statement: PolicyStatement): boolean {
+  public addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult {
     if (!this.defaultPolicy) {
       this.defaultPolicy = new Policy(this, 'DefaultPolicy');
       this.defaultPolicy.attachToGroup(this);
     }
 
     this.defaultPolicy.addStatements(statement);
-    return true;
+    return { statementAdded: true, policyDependable: this.defaultPolicy };
+  }
+
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.addToPrincipalPolicy(statement).statementAdded;
   }
 }
 

packages/@aws-cdk/aws-iam/lib/lazy-role.ts

@@ -3,7 +3,7 @@ import { Grant } from './grant';
 import { IManagedPolicy } from './managed-policy';
 import { Policy } from './policy';
 import { PolicyStatement } from './policy-statement';
-import { IPrincipal, PrincipalPolicyFragment } from './principals';
+import { AddToPrincipalPolicyResult, IPrincipal, PrincipalPolicyFragment } from './principals';
 import { IRole, Role, RoleProps } from './role';
 
 /**
@@ -43,15 +43,19 @@ export class LazyRole extends cdk.Resource implements IRole {
    * If there is no default policy attached to this role, it will be created.
    * @param statement The permission statement to add to the policy document
    */
-  public addToPolicy(statement: PolicyStatement): boolean {
+  public addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult {
     if (this.role) {
-      return this.role.addToPolicy(statement);
+      return this.role.addToPrincipalPolicy(statement);
     } else {
       this.statements.push(statement);
-      return true;
+      return { statementAdded: true, policyDependable: this };
     }
   }
 
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.addToPrincipalPolicy(statement).statementAdded;
+  }
+
   /**
    * Attaches a policy to this role.
    * @param policy The policy to attach