Introduce new method after all

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

@@ -1,7 +1,6 @@
 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
@@ -135,7 +134,7 @@ export class Grant implements cdk.IDependable {
     return new Grant({
       resourceStatement: statement,
       options,
-      policyApplied: resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined,
+      policyDependable: resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined,
     });
   }
 
@@ -151,10 +150,16 @@ export class Grant implements cdk.IDependable {
       resources: options.resourceArns,
     });
 
-    const addedToPrincipal = options.grantee.grantPrincipal.addToPolicy(statement);
-    const policyApplied = addedToPrincipal ? guessPolicyDependable(options.grantee.grantPrincipal) : new cdk.ConcreteDependable();
+    const addedToPrincipal = options.grantee.grantPrincipal.addToIdentityPolicy(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, policyApplied });
+    return new Grant({ principalStatement: statement, options, policyDependable: addedToPrincipal.policyDependable });
   }
 
   /**
@@ -185,7 +190,7 @@ export class Grant implements cdk.IDependable {
       principalStatement: statement,
       resourceStatement: result.resourceStatement,
       options,
-      policyApplied: resourceDependable ? new CompositeDependable(result, resourceDependable) : result,
+      policyDependable: resourceDependable ? new CompositeDependable(result, resourceDependable) : result,
     });
   }
 
@@ -233,7 +238,7 @@ export class Grant implements cdk.IDependable {
 
     cdk.DependableTrait.implement(this, {
       get dependencyRoots() {
-        return props.policyApplied ? cdk.DependableTrait.get(props.policyApplied).dependencyRoots : [];
+        return props.policyDependable ? cdk.DependableTrait.get(props.policyDependable).dependencyRoots : [];
       },
     });
   }
@@ -281,7 +286,7 @@ interface GrantProps {
    *
    * Used to add dependencies on grants
    */
-  readonly policyApplied?: cdk.IDependable;
+  readonly policyDependable?: cdk.IDependable;
 }
 
 /**
@@ -299,59 +304,19 @@ export interface IResourceWithPolicy extends cdk.IConstruct {
  */
 export interface AddToResourcePolicyResult {
   /**
-   * Whether the statement as added
+   * Whether the statement was 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.
+   * @default - If `statementAdded` is true, the resource object itself.
+   * 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
  *

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 { AddToIdentityPolicyResult, 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 addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult {
     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.addToIdentityPolicy(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 { AddToIdentityPolicyResult, 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 addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult {
     if (this.role) {
-      return this.role.addToPolicy(statement);
+      return this.role.addToIdentityPolicy(statement);
     } else {
       this.statements.push(statement);
-      return true;
+      return { statementAdded: true, policyDependable: this };
     }
   }
 
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.addToIdentityPolicy(statement).statementAdded;
+  }
+
   /**
    * Attaches a policy to this role.
    * @param policy The policy to attach

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

@@ -46,8 +46,32 @@ export interface IPrincipal extends IGrantable {
    *
    * @returns true if the statement was added, false if the principal in
    * question does not have a policy document to add the statement to.
+   *
+   * @deprecated Use `addToIdentityPolicy` instead.
    */
   addToPolicy(statement: PolicyStatement): boolean;
+
+  /**
+   * Add to the policy of this principal.
+   */
+  addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult;
+}
+
+/**
+ * Result of calling `addToIdentityPolicy`
+ */
+export interface AddToIdentityPolicyResult {
+  /**
+   * Whether the statement was added to the identity's policies.
+   */
+  readonly statementAdded: boolean;
+
+  /**
+   * Dependable which allows depending on the policy change being applied
+   *
+   * @default - Required if `statementAdded` is true.
+   */
+  readonly policyDependable?: cdk.IDependable;
 }
 
 /**
@@ -66,10 +90,14 @@ export abstract class PrincipalBase implements IPrincipal {
    */
   public readonly assumeRoleAction: string = 'sts:AssumeRole';
 
-  public addToPolicy(_statement: PolicyStatement): boolean {
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.addToIdentityPolicy(statement).statementAdded;
+  }
+
+  public addToIdentityPolicy(_statement: PolicyStatement): AddToIdentityPolicyResult {
     // This base class is used for non-identity principals. None of them
     // have a PolicyDocument to add to.
-    return false;
+    return { statementAdded: false };
   }
 
   public toString() {
@@ -153,7 +181,11 @@ export class PrincipalWithConditions implements IPrincipal {
   }
 
   public addToPolicy(statement: PolicyStatement): boolean {
-    return this.principal.addToPolicy(statement);
+    return this.addToIdentityPolicy(statement).statementAdded;
+  }
+
+  public addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult {
+    return this.principal.addToIdentityPolicy(statement);
   }
 
   public toString() {

packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts

@@ -1,9 +1,9 @@
-import { Construct, DependableTrait } from '@aws-cdk/core';
+import { ConcreteDependable, Construct, DependableTrait } from '@aws-cdk/core';
 import { Grant } from '../grant';
 import { IManagedPolicy } from '../managed-policy';
 import { Policy } from '../policy';
 import { PolicyStatement } from '../policy-statement';
-import { IPrincipal } from '../principals';
+import { AddToIdentityPolicyResult, IPrincipal } from '../principals';
 import { IRole } from '../role';
 
 /**
@@ -44,9 +44,13 @@ export class ImmutableRole extends Construct implements IRole {
     // do nothing
   }
 
-  public addToPolicy(_statement: PolicyStatement): boolean {
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.addToIdentityPolicy(statement).statementAdded;
+  }
+
+  public addToIdentityPolicy(_statement: PolicyStatement): AddToIdentityPolicyResult {
     // Not really added, but for the purposes of consumer code pretend that it was.
-    return true;
+    return { statementAdded: true, policyDependable: new ConcreteDependable() };
   }
 
   public grant(grantee: IPrincipal, ...actions: string[]): Grant {

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

@@ -6,7 +6,7 @@ import { IManagedPolicy } from './managed-policy';
 import { Policy } from './policy';
 import { PolicyDocument } from './policy-document';
 import { PolicyStatement } from './policy-statement';
-import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
+import { AddToIdentityPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
 import { ImmutableRole } from './private/immutable-role';
 import { AttachedPolicies } from './util';
 
@@ -192,12 +192,16 @@ export class Role extends Resource implements IRole {
       private defaultPolicy?: Policy;
 
       public addToPolicy(statement: PolicyStatement): boolean {
+        return this.addToIdentityPolicy(statement).statementAdded;
+      }
+
+      public addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult {
         if (!this.defaultPolicy) {
           this.defaultPolicy = new Policy(this, 'Policy');
           this.attachInlinePolicy(this.defaultPolicy);
         }
         this.defaultPolicy.addStatements(statement);
-        return true;
+        return { statementAdded: true, policyDependable: this.defaultPolicy };
       }
 
       public attachInlinePolicy(policy: Policy): void {
@@ -351,13 +355,17 @@ export class Role extends 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 addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult {
     if (!this.defaultPolicy) {
       this.defaultPolicy = new Policy(this, 'DefaultPolicy');
       this.attachInlinePolicy(this.defaultPolicy);
     }
     this.defaultPolicy.addStatements(statement);
-    return true;
+    return { statementAdded: true, policyDependable: this.defaultPolicy };
+  }
+
+  public addToPolicy(statement: PolicyStatement): boolean {
+    return this.addToIdentityPolicy(statement).statementAdded;
   }
 
   /**