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

@@ -41,7 +41,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
@@ -114,7 +114,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):
@@ -335,8 +335,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 };
       }
     }
 
@@ -358,8 +359,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 };
       }
     }
 
@@ -371,8 +373,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 };
       }
     }
 
@@ -424,11 +427,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-iam/lib/grant.ts

@@ -1,6 +1,7 @@
 import * as cdk from '@aws-cdk/core';
 import { PolicyStatement } from './policy-statement';
 import { IGrantable, IPrincipal } from './principals';
+import { ConcreteDependable } from '@aws-cdk/core';
 
 /**
  * Basic options for a grant operation
@@ -100,7 +101,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 +130,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,
+      policyApplied: resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined,
+    });
   }
 
   /**
@@ -147,8 +152,9 @@ export class Grant {
     });
 
     const addedToPrincipal = options.grantee.grantPrincipal.addToPolicy(statement);
+    const policyApplied = addedToPrincipal ? guessPolicyDependable(options.grantee.grantPrincipal) : new cdk.ConcreteDependable();
 
-    return new Grant({ principalStatement: addedToPrincipal ? statement : undefined, options });
+    return new Grant({ principalStatement: addedToPrincipal ? statement : undefined, options, policyApplied });
   }
 
   /**
@@ -172,9 +178,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,
+      policyApplied: resourceDependable ? new CompositeDependable(result, resourceDependable) : result,
+    });
   }
 
   /**
@@ -218,6 +230,12 @@ export class Grant {
     this.options = props.options;
     this.principalStatement = props.principalStatement;
     this.resourceStatement = props.resourceStatement;
+
+    cdk.DependableTrait.implement(this, {
+      get dependencyRoots() {
+        return props.policyApplied ? cdk.DependableTrait.get(props.policyApplied).dependencyRoots : [];
+      },
+    });
   }
 
   /**
@@ -236,6 +254,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 +275,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 policyApplied?: cdk.IDependable;
 }
 
 /**
@@ -255,5 +291,80 @@ 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 as added
+   */
+  readonly statementAdded: boolean;
+
+  /**
+   * Dependable which allows depending on the policy change being applied
+   *
+   * @default - The resource itself applies the policy change, if `didAddStatement`
+   * is true. Otherwise, no dependable.
+   */
+  readonly policyDependable?: cdk.IDependable;
+}
+
+/**
+ * Guess the policy object that just got modified from a principal
+ *
+ * This is a mega-extreme hack. We SHOULD have gotten the policy that
+ * was modified back from the principal when doing `addToPolicy()`. However,
+ * that API was cast in stone to return a `boolean` and cannot be changed
+ * anymore since it's stable.
+ *
+ * It's also occupying prime real estate, in that `addToPolicy` is the name
+ * we'd want this thing to have, so deprecating it feels silly and wasteful.
+ *
+ * So we do nasty things here in order to take a good guess at finding the
+ * Policy object, and hope we are correct. We should at least be fine for
+ * the CDK built-in Principal types that have mutable Policies (Role,
+ * User, Group, which are the main ones that matter).
+ */
+function guessPolicyDependable(principal: IPrincipal): cdk.IDependable {
+  // Role, Group and User Constructs have a default `Policy` object.
+  if (cdk.Construct.isConstruct(principal)) {
+    const pol = principal.node.tryFindChild('Policy');
+    if (pol) { return pol; }
+
+    // LazyRole wraps another role, but that may not be reified yet.
+    // It is itself a construct, so we can return it. We may capture too much
+    // (resource and any of its policies), but not too little. It's also
+    // no worse than what we used to have before Grants being IDependable,
+    // where users would ALWAYS have to depend on the Role (in order to depend
+    // on its Policy).
+    return principal;
+  }
+
+  // PrincipalWithCondition wraps another principal.
+  if ((principal as any).principal) {
+    return guessPolicyDependable((principal as any).principal);
+  }
+
+  // Give up (empty dependable)
+  return new cdk.ConcreteDependable();
 }
+
+/**
+ * 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-kms/lib/alias.ts

@@ -73,8 +73,8 @@ abstract class AliasBase extends Resource implements IAlias {
     return this.aliasTargetKey.addAlias(alias);
   }
 
-  public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp?: boolean): void {
-    this.aliasTargetKey.addToResourcePolicy(statement, allowNoOp);
+  public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp?: boolean): iam.AddToResourcePolicyResult {
+    return this.aliasTargetKey.addToResourcePolicy(statement, allowNoOp);
   }
 
   public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {

packages/@aws-cdk/aws-kms/lib/key.ts

@@ -107,15 +107,16 @@ abstract class KeyBase extends Resource implements IKey {
    * defined (i.e. external key), the operation will fail. Otherwise, it will
    * no-op.
    */
-  public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp = true) {
+  public addToResourcePolicy(statement: iam.PolicyStatement, allowNoOp = true): iam.AddToResourcePolicyResult {
     const stack = Stack.of(this);
 
     if (!this.policy) {
-      if (allowNoOp) { return; }
+      if (allowNoOp) { return { statementAdded: false }; }
       throw new Error(`Unable to add statement to IAM resource policy for KMS key: ${JSON.stringify(stack.resolve(this.keyArn))}`);
     }
 
     this.policy.addStatements(statement);
+    return { statementAdded: true, policyDependable: this.policy };
   }
 
   /**

packages/@aws-cdk/aws-lambda/lib/function-base.ts

@@ -283,6 +283,8 @@ export abstract class FunctionBase extends Resource implements IFunction {
             principal: grantee.grantPrincipal!,
             action: 'lambda:InvokeFunction',
           });
+
+          return { statementAdded: true, policyDependable: this.node.findChild(identifier) } as iam.AddToResourcePolicyResult;
         },
         node: this.node,
       },

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -1,7 +1,7 @@
 import * as events from '@aws-cdk/aws-events';
 import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
-import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core';
+import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, ConcreteDependable } from '@aws-cdk/core';
 import { EOL } from 'os';
 import { BucketPolicy } from './bucket-policy';
 import { IBucketNotificationDestination } from './destination';
@@ -73,7 +73,7 @@ export interface IBucket extends IResource {
    * contents. Use `bucketArn` and `arnForObjects(keys)` to obtain ARNs for
    * this bucket or objects.
    */
-  addToResourcePolicy(permission: iam.PolicyStatement): void;
+  addToResourcePolicy(permission: iam.PolicyStatement): iam.AddToResourcePolicyResult;
 
   /**
    * The https URL of an S3 object. For example:
@@ -408,14 +408,17 @@ abstract class BucketBase extends Resource implements IBucket {
    * contents. Use `bucketArn` and `arnForObjects(keys)` to obtain ARNs for
    * this bucket or objects.
    */
-  public addToResourcePolicy(permission: iam.PolicyStatement) {
+  public addToResourcePolicy(permission: iam.PolicyStatement): iam.AddToResourcePolicyResult {
     if (!this.policy && this.autoCreatePolicy) {
       this.policy = new BucketPolicy(this, 'Policy', { bucket: this });
     }
 
     if (this.policy) {
       this.policy.document.addStatements(permission);
+      return { statementAdded: true, policyDependable: this.policy };
     }
+
+    return { statementAdded: false };
   }
 
   /**

packages/@aws-cdk/aws-secretsmanager/lib/secret.ts

@@ -163,14 +163,16 @@ abstract class SecretBase extends Resource implements ISecret {
     });
   }
 
-  public addToResourcePolicy(statement: iam.PolicyStatement) {
+  public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
     if (!this.policy && this.autoCreatePolicy) {
       this.policy = new ResourcePolicy(this, 'Policy', { secret: this });
     }
 
     if (this.policy) {
       this.policy.document.addStatements(statement);
+      return { statementAdded: true, policyDependable: this.policy };
     }
+    return { statementAdded: false };
   }
 
   public denyAccountRootDelete() {

packages/@aws-cdk/aws-sns/lib/topic-base.ts

@@ -83,14 +83,16 @@ export abstract class TopicBase extends Resource implements ITopic {
    * will be automatically created upon the first call to `addToPolicy`. If
    * the topic is improted (`Topic.import`), then this is a no-op.
    */
-  public addToResourcePolicy(statement: iam.PolicyStatement) {
+  public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
     if (!this.policy && this.autoCreatePolicy) {
       this.policy = new TopicPolicy(this, 'Policy', { topics: [ this ] });
     }
 
     if (this.policy) {
       this.policy.document.addStatements(statement);
+      return { statementAdded: true, policyDependable: this.policy };
     }
+    return { statementAdded: false };
   }
 
   /**