fix(codebuild): cannot use immutable roles for Project

packages/@aws-cdk/aws-codebuild/lib/project.ts

@@ -7,7 +7,7 @@ import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
-import { Aws, CfnResource, Construct, Duration, IResource, Lazy, PhysicalName, Resource, Stack } from '@aws-cdk/core';
+import { Aws, Construct, Duration, IResource, Lazy, PhysicalName, Resource, Stack } from '@aws-cdk/core';
 import { IArtifacts } from './artifacts';
 import { BuildSpec } from './build-spec';
 import { Cache } from './cache';
@@ -973,10 +973,9 @@ export class Project extends ProjectBase {
     this.role.attachInlinePolicy(policy);
 
     // add an explicit dependency between the EC2 Policy and this Project -
-    // otherwise, creating the Project fails,
-    // as it requires these permissions to be already attached to the Project's Role
-    const cfnPolicy = policy.node.findChild('Resource') as CfnResource;
-    project.addDependsOn(cfnPolicy);
+    // otherwise, creating the Project fails, as it requires these permissions
+    // to be already attached to the Project's Role
+    project.node.addDependency(policy);
   }
 
   private validateCodePipelineSettings(artifacts: IArtifacts) {

packages/@aws-cdk/aws-codebuild/test/test.project.ts

@@ -1,4 +1,4 @@
-import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert';
+import { countResources, expect, haveResource, haveResourceLike, not, ResourcePart } from '@aws-cdk/assert';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as s3 from '@aws-cdk/aws-s3';
@@ -271,4 +271,34 @@ export = {
 
     test.done();
   },
-};
+
+  'can use an imported Role with mutable = false for a Project within a VPC'(test: Test) {
+    const stack = new cdk.Stack();
+
+    const importedRole = iam.Role.fromRoleArn(stack, 'Role',
+        'arn:aws:iam::1234567890:role/service-role/codebuild-bruiser-service-role', {
+      mutable: false,
+    });
+    const vpc = new ec2.Vpc(stack, 'Vpc');
+
+    new codebuild.Project(stack, 'Project', {
+      source: codebuild.Source.gitHubEnterprise({
+        httpsCloneUrl: 'https://mygithub-enterprise.com/myuser/myrepo',
+      }),
+      role: importedRole,
+      vpc,
+    });
+
+    expect(stack).to(countResources('AWS::IAM::Policy', 0));
+
+    // Check that the CodeBuild project does not have a DependsOn
+    expect(stack).to(haveResource('AWS::CodeBuild::Project', (res: any) => {
+      if (res.DependsOn && res.DependsOn.length > 0) {
+        throw new Error(`CodeBuild project should have no DependsOn, but got: ${JSON.stringify(res, undefined, 2)}`);
+      }
+      return true;
+    }, ResourcePart.CompleteDefinition));
+
+    test.done();
+  },
+};

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

@@ -56,6 +56,22 @@ export interface PolicyProps {
    * @default - No statements.
    */
   readonly statements?: PolicyStatement[];
+
+  /**
+   * Whether an `AWS::IAM::Policy` must be created
+   *
+   * Unless set to `true`, this `Policy` construct will not materialize to an
+   * `AWS::IAM::Policy` CloudFormation resource in case it would have no effect
+   * (for example, if it remains unattached to an IAM identity or if it has no
+   * statements). This is generally desired behavior, since it prevents
+   * creating invalid--and hence undeployable--CloudFormation templates.
+   *
+   * In cases where you know the policy must be created and it is actually
+   * an error if no statements have been added to it, you can se this to `true`.
+   *
+   * @default false
+   */
+  readonly mustCreate?: boolean;
 }
 
 /**
@@ -79,16 +95,12 @@ export class Policy extends Resource implements IPolicy {
    */
   public readonly document = new PolicyDocument();
 
-  /**
-   * The name of this policy.
-   *
-   * @attribute
-   */
-  public readonly policyName: string;
-
+  private readonly _policyName: string;
   private readonly roles = new Array<IRole>();
   private readonly users = new Array<IUser>();
   private readonly groups = new Array<IGroup>();
+  private readonly mustCreate: boolean;
+  private referenceTaken = false;
 
   constructor(scope: Construct, id: string, props: PolicyProps = {}) {
     super(scope, id, {
@@ -107,7 +119,8 @@ export class Policy extends Resource implements IPolicy {
       groups: undefinedIfEmpty(() => this.groups.map(g => g.groupName)),
     });
 
-    this.policyName = this.physicalName!;
+    this._policyName = this.physicalName!;
+    this.mustCreate = props.mustCreate !== undefined ? props.mustCreate : false;
 
     if (props.users) {
       props.users.forEach(u => this.attachToUser(u));
@@ -160,19 +173,60 @@ export class Policy extends Resource implements IPolicy {
     group.attachInlinePolicy(this);
   }
 
+  /**
+   * The name of this policy.
+   *
+   * @attribute
+   */
+  public get policyName(): string {
+    this.referenceTaken = true;
+    return this._policyName;
+  }
+
   protected validate(): string[] {
     const result = new Array<string>();
 
     // validate that the policy document is not empty
     if (this.document.isEmpty) {
-      result.push('Policy is empty. You must add statements to the policy');
+      if (this.mustCreate) {
+        result.push('Policy created with mustCreate=true is empty. You must add statements to the policy');
+      }
+      if (!this.mustCreate && this.referenceTaken) {
+        result.push('Policy name has been read of empty policy. You must add statements to the policy so it can exist.');
+      }
     }
 
     // validate that the policy is attached to at least one principal (role, user or group).
-    if (this.groups.length + this.users.length + this.roles.length === 0) {
-      result.push(`Policy must be attached to at least one principal: user, group or role`);
+    if (!this.isAttached) {
+      if (this.mustCreate) {
+        result.push(`Policy created with mustCreate=true must be attached to at least one principal: user, group or role`);
+      }
+      if (!this.mustCreate && this.referenceTaken) {
+        result.push('Policy name has been read of unattached policy. Attach to at least one principal: user, group or role.');
+      }
     }
 
     return result;
   }
+
+  protected prepare() {
+    // Remove the resource if it shouldn't exist. This will prevent it from being rendered to the template.
+    if (!this.shouldExist) {
+      this.node.removeChild('Resource');
+    }
+  }
+
+  /**
+   * Whether the policy resource has been attached to any identity
+   */
+  private get isAttached() {
+    return this.groups.length + this.users.length + this.roles.length > 0;
+  }
+
+  /**
+   * Whether the policy resource should be created
+   */
+  private get shouldExist() {
+    return this.mustCreate || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
+  }
 }

packages/@aws-cdk/aws-iam/test/policy.test.ts

@@ -1,22 +1,26 @@
+import { ResourcePart } from '@aws-cdk/assert';
 import '@aws-cdk/assert/jest';
-import { App, Stack } from '@aws-cdk/core';
+import { App, CfnResource, Stack } from '@aws-cdk/core';
 import { AnyPrincipal, CfnPolicy, Group, Policy, PolicyStatement, Role, ServicePrincipal, User } from '../lib';
 
 // tslint:disable:object-literal-key-quotes
 
 describe('IAM policy', () => {
-  test('fails when policy is empty', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-    new Policy(stack, 'MyPolicy');
+  let app: App;
+  let stack: Stack;
 
-    expect(() => app.synth()).toThrow(/Policy is empty/);
+  beforeEach(() => {
+    app = new App();
+    stack = new Stack(app, 'MyStack');
   });
 
-  test('policy with statements', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
+  test('fails when "mustCreate" policy is empty', () => {
+    new Policy(stack, 'MyPolicy', { mustCreate: true });
+
+    expect(() => app.synth()).toThrow(/is empty/);
+  });
 
+  test('policy with statements', () => {
     const policy = new Policy(stack, 'MyPolicy', { policyName: 'MyPolicyName' });
     policy.addStatements(new PolicyStatement({ resources: ['*'], actions: ['sqs:SendMessage'] }));
     policy.addStatements(new PolicyStatement({ resources: ['arn'], actions: ['sns:Subscribe'] }));
@@ -39,9 +43,6 @@ describe('IAM policy', () => {
   });
 
   test('policy name can be omitted, in which case the logical id will be used', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-
     const policy = new Policy(stack, 'MyPolicy');
     policy.addStatements(new PolicyStatement({ resources: ['*'], actions: ['sqs:SendMessage'] }));
     policy.addStatements(new PolicyStatement({ resources: ['arn'], actions: ['sns:Subscribe'] }));
@@ -64,10 +65,6 @@ describe('IAM policy', () => {
   });
 
   test('policy can be attached users, groups and roles and added permissions via props', () => {
-    const app = new App();
-
-    const stack = new Stack(app, 'MyStack');
-
     const user1 = new User(stack, 'User1');
     const group1 = new Group(stack, 'Group1');
     const role1 = new Role(stack, 'Role1', {
@@ -108,8 +105,6 @@ describe('IAM policy', () => {
   });
 
   test('idempotent if a principal (user/group/role) is attached twice', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
     const p = new Policy(stack, 'MyPolicy');
     p.addStatements(new PolicyStatement({ actions: ['*'], resources: ['*'] }));
 
@@ -130,10 +125,6 @@ describe('IAM policy', () => {
   });
 
   test('users, groups, roles and permissions can be added using methods', () => {
-    const app = new App();
-
-    const stack = new Stack(app, 'MyStack');
-
     const p = new Policy(stack, 'MyTestPolicy', {
       policyName: 'Foo',
     });
@@ -171,9 +162,6 @@ describe('IAM policy', () => {
   });
 
   test('policy can be attached to users, groups or role via methods on the principal', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-
     const policy = new Policy(stack, 'MyPolicy');
     const user = new User(stack, 'MyUser');
     const group = new Group(stack, 'MyGroup');
@@ -210,9 +198,6 @@ describe('IAM policy', () => {
   });
 
   test('fails if policy name is not unique within a user/group/role', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-
     // create two policies named Foo and attach them both to the same user/group/role
     const p1 = new Policy(stack, 'P1', { policyName: 'Foo' });
     const p2 = new Policy(stack, 'P2', { policyName: 'Foo' });
@@ -236,16 +221,12 @@ describe('IAM policy', () => {
     p3.attachToRole(role);
   });
 
-  test('fails if policy is not attached to a principal', () => {
-    const app = new App();
-    const stack = new Stack(app, 'MyStack');
-    new Policy(stack, 'MyPolicy');
-    expect(() => app.synth()).toThrow(/Policy must be attached to at least one principal: user, group or role/);
+  test('fails if "mustCreate" policy is not attached to a principal', () => {
+    new Policy(stack, 'MyPolicy', { mustCreate: true });
+    expect(() => app.synth()).toThrow(/attached to at least one principal: user, group or role/);
   });
 
   test("generated policy name is the same as the logical id if it's shorter than 128 characters", () => {
-    const stack = new Stack();
-
     createPolicyWithLogicalId(stack, 'Foo');
 
     expect(stack).toHaveResourceLike('AWS::IAM::Policy', {
@@ -254,8 +235,6 @@ describe('IAM policy', () => {
   });
 
   test('generated policy name only uses the last 128 characters of the logical id', () => {
-    const stack = new Stack();
-
     const logicalId128 = 'a' + dup(128 - 2) + 'a';
     const logicalIdOver128 = 'PREFIX' + logicalId128;
 
@@ -273,6 +252,64 @@ describe('IAM policy', () => {
       return r;
     }
   });
+
+  test('mustCreate=false, dependency on empty Policy never materializes', () => {
+    // GIVEN
+    const pol = new Policy(stack, 'Pol', { mustCreate: false });
+
+    const res = new CfnResource(stack, 'Resource', {
+      type: 'Some::Resource',
+    });
+
+    // WHEN
+    res.node.addDependency(pol);
+
+    // THEN
+    expect(stack).toMatchTemplate({
+      Resources: {
+        Resource: {
+          Type: 'Some::Resource',
+        }
+      }
+    });
+  });
+
+  test('mustCreate=false, dependency on attached and non-empty Policy can be taken', () => {
+    // GIVEN
+    const pol = new Policy(stack, 'Pol', { mustCreate: false });
+    pol.addStatements(new PolicyStatement({
+      actions: ['s3:*'],
+      resources: ['*'],
+    }));
+    pol.attachToUser(new User(stack, 'User'));
+
+    const res = new CfnResource(stack, 'Resource', {
+      type: 'Some::Resource',
+    });
+
+    // WHEN
+    res.node.addDependency(pol);
+
+    // THEN
+    expect(stack).toHaveResource("Some::Resource", {
+      Type: "Some::Resource",
+      DependsOn: [ "Pol0FE9AD5D" ]
+    }, ResourcePart.CompleteDefinition);
+  });
+
+  test('empty policy is OK if mustCreate=false', () => {
+    new Policy(stack, 'Pol', { mustCreate: false });
+
+    app.synth();
+    // If we got here, all OK
+  });
+
+  test('reading policyName forces a Policy to materialize', () => {
+    const pol = new Policy(stack, 'Pol', { mustCreate: false });
+    Array.isArray(pol.policyName);
+
+    expect(() => app.synth()).toThrow(/empty policy/);
+  });
 });
 
 function createPolicyWithLogicalId(stack: Stack, logicalId: string): void {

packages/@aws-cdk/core/lib/construct.ts

@@ -438,6 +438,15 @@ export class ConstructNode {
     return ret;
   }
 
+  /**
+   * Remove the child with the given name, if present.
+   *
+   * It is not an error if a child with the given name does not exist.
+   */
+  public removeChild(childName: string) {
+    delete this._children[childName];
+  }
+
   /**
    * Locks this construct from allowing more children to be added. After this
    * call, no more children can be added to this construct or to any children.