fix(pipelines): cannot publish assets to more than 35 environments (#…

…21010)

We used to use `Lazy.list()` to render the list of Role ARNs that the
Asset Publishing Role needs to assume.

The Lazy list cannot be split up, meaning the overflow rendering logic
we recently added cannot apply to this policy. Turn the lazy list into
mutating a policy statement in-place, so the policy overflow logic can
split the policy statement into multiple managed policies.

Also in this PR:

- To keep the tests behavior consistent (on whether arrays get rendered to the `Resource` field in statements or not, regardless of exactly which code path is taken), change the behavior of `PolicyStatement`, have it eagerly dedupe in memory.


----

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

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

@@ -69,20 +69,20 @@ export class PolicyStatement {
     return ret;
   }
 
-  private readonly _action = new Array<string>();
-  private readonly _notAction = new Array<string>();
+  private readonly _action = new OrderedSet<string>();
+  private readonly _notAction = new OrderedSet<string>();
   private readonly _principal: { [key: string]: any[] } = {};
   private readonly _notPrincipal: { [key: string]: any[] } = {};
-  private readonly _resource = new Array<string>();
-  private readonly _notResource = new Array<string>();
+  private readonly _resource = new OrderedSet<string>();
+  private readonly _notResource = new OrderedSet<string>();
   private readonly _condition: { [key: string]: any } = { };
   private _sid?: string;
   private _effect: Effect;
   private principalConditionsJson?: string;
 
   // Hold on to those principals
-  private readonly _principals = new Array<IPrincipal>();
-  private readonly _notPrincipals = new Array<IPrincipal>();
+  private readonly _principals = new OrderedSet<IPrincipal>();
+  private readonly _notPrincipals = new OrderedSet<IPrincipal>();
   private _frozen = false;
 
   constructor(props: PolicyStatementProps = {}) {
@@ -193,12 +193,15 @@ export class PolicyStatement {
    */
   public addPrincipals(...principals: IPrincipal[]) {
     this.assertNotFrozen('addPrincipals');
-    this._principals.push(...principals);
-    if (Object.keys(principals).length > 0 && Object.keys(this._notPrincipal).length > 0) {
+    if (principals.length > 0 && this._notPrincipals.length > 0) {
       throw new Error('Cannot add \'Principals\' to policy statement if \'NotPrincipals\' have been added');
     }
     for (const principal of principals) {
       this.validatePolicyPrincipal(principal);
+    }
+
+    const added = this._principals.push(...principals);
+    for (const principal of added) {
       const fragment = principal.policyFragment;
       mergePrincipal(this._principal, fragment.principalJson);
       this.addPrincipalConditions(fragment.conditions);
@@ -215,12 +218,15 @@ export class PolicyStatement {
    */
   public addNotPrincipals(...notPrincipals: IPrincipal[]) {
     this.assertNotFrozen('addNotPrincipals');
-    this._notPrincipals.push(...notPrincipals);
-    if (Object.keys(notPrincipals).length > 0 && Object.keys(this._principal).length > 0) {
+    if (notPrincipals.length > 0 && this._principals.length > 0) {
       throw new Error('Cannot add \'NotPrincipals\' to policy statement if \'Principals\' have been added');
     }
     for (const notPrincipal of notPrincipals) {
       this.validatePolicyPrincipal(notPrincipal);
+    }
+
+    const added = this._notPrincipals.push(...notPrincipals);
+    for (const notPrincipal of added) {
       const fragment = notPrincipal.policyFragment;
       mergePrincipal(this._notPrincipal, fragment.principalJson);
       this.addPrincipalConditions(fragment.conditions);
@@ -426,14 +432,14 @@ export class PolicyStatement {
    */
   public toStatementJson(): any {
     return normalizeStatement({
-      Action: this._action,
-      NotAction: this._notAction,
+      Action: this._action.direct(),
+      NotAction: this._notAction.direct(),
       Condition: this._condition,
       Effect: this.effect,
       Principal: this._principal,
       NotPrincipal: this._notPrincipal,
-      Resource: this._resource,
-      NotResource: this._notResource,
+      Resource: this._resource.direct(),
+      NotResource: this._notResource.direct(),
       Sid: this.sid,
     });
   }
@@ -503,7 +509,7 @@ export class PolicyStatement {
    */
   public validateForResourcePolicy(): string[] {
     const errors = this.validateForAnyPolicy();
-    if (Object.keys(this._principal).length === 0 && Object.keys(this._notPrincipal).length === 0) {
+    if (this._principals.length === 0 && this._notPrincipals.length === 0) {
       errors.push('A PolicyStatement used in a resource-based policy must specify at least one IAM principal.');
     }
     return errors;
@@ -516,10 +522,10 @@ export class PolicyStatement {
    */
   public validateForIdentityPolicy(): string[] {
     const errors = this.validateForAnyPolicy();
-    if (Object.keys(this._principal).length > 0 || Object.keys(this._notPrincipal).length > 0) {
+    if (this._principals.length > 0 || this._notPrincipals.length > 0) {
       errors.push('A PolicyStatement used in an identity-based policy cannot specify any IAM principals.');
     }
-    if (Object.keys(this._resource).length === 0 && Object.keys(this._notResource).length === 0) {
+    if (this._resource.length === 0 && this._notResource.length === 0) {
       errors.push('A PolicyStatement used in an identity-based policy must specify at least one resource.');
     }
     return errors;
@@ -529,42 +535,42 @@ export class PolicyStatement {
    * The Actions added to this statement
    */
   public get actions() {
-    return [...this._action];
+    return this._action.copy();
   }
 
   /**
    * The NotActions added to this statement
    */
   public get notActions() {
-    return [...this._notAction];
+    return this._notAction.copy();
   }
 
   /**
    * The Principals added to this statement
    */
   public get principals(): IPrincipal[] {
-    return [...this._principals];
+    return this._principals.copy();
   }
 
   /**
    * The NotPrincipals added to this statement
    */
   public get notPrincipals(): IPrincipal[] {
-    return [...this._notPrincipals];
+    return this._notPrincipals.copy();
   }
 
   /**
    * The Resources added to this statement
    */
   public get resources() {
-    return [...this._resource];
+    return this._resource.copy();
   }
 
   /**
    * The NotResources added to this statement
    */
   public get notResources() {
-    return [...this._notResource];
+    return this._notResource.copy();
   }
 
   /**
@@ -821,4 +827,54 @@ export function deriveEstimateSizeOptions(scope: IConstruct): EstimateSizeOption
   }
 
   return { actionEstimate, arnEstimate };
+}
+
+/**
+ * A class that behaves both as a set and an array
+ *
+ * Used for the elements of a PolicyStatement. In practice they behave as sets,
+ * but we have thousands of snapshot tests in existence that will rely on a
+ * particular order so we can't just change the type to `Set<>` wholesale without
+ * causing a lot of churn.
+ */
+class OrderedSet<A> {
+  private readonly set = new Set<A>();
+  private readonly array = new Array<A>();
+
+  /**
+   * Add new elements to the set
+   *
+   * @param xs the elements to be added
+   *
+   * @returns the elements actually added
+   */
+  public push(...xs: readonly A[]): A[] {
+    const ret = new Array<A>();
+    for (const x of xs) {
+      if (this.set.has(x)) {
+        continue;
+      }
+      this.set.add(x);
+      this.array.push(x);
+      ret.push(x);
+    }
+    return ret;
+  }
+
+  public get length() {
+    return this.array.length;
+  }
+
+  public copy(): A[] {
+    return [...this.array];
+  }
+
+  /**
+   * Direct (read-only) access to the underlying array
+   *
+   * (Saves a copy)
+   */
+  public direct(): readonly A[] {
+    return this.array;
+  }
 }

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

@@ -347,8 +347,8 @@ describe('IAM policy document', () => {
 
     expect(stack.resolve(statement.toStatementJson())).toEqual({
       Effect: 'Allow',
-      Action: ['a'],
-      Resource: ['x'],
+      Action: 'a',
+      Resource: 'x',
     });
   });
 
@@ -364,8 +364,8 @@ describe('IAM policy document', () => {
 
     expect(stack.resolve(statement.toStatementJson())).toEqual({
       Effect: 'Allow',
-      NotAction: ['a'],
-      NotResource: ['x'],
+      NotAction: 'a',
+      NotResource: 'x',
     });
   });
 

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

@@ -215,6 +215,22 @@ describe('IAM policy statement', () => {
       .toThrow(/Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy/);
   });
 
+  test('multiple identical entries render to a scalar (instead of a singleton list)', () => {
+    const stack = new Stack();
+    const policyStatement = new PolicyStatement({
+      actions: ['aws:Action'],
+    });
+
+    policyStatement.addResources('asdf');
+    policyStatement.addResources('asdf');
+    policyStatement.addResources('asdf');
+
+    expect(stack.resolve(policyStatement.toStatementJson())).toEqual({
+      Effect: 'Allow',
+      Action: 'aws:Action',
+      Resource: 'asdf',
+    });
+  });
 
   test('a frozen policy statement cannot be modified any more', () => {
     // GIVEN

packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts

@@ -4,14 +4,15 @@ import * as cp from '@aws-cdk/aws-codepipeline';
 import * as cpa from '@aws-cdk/aws-codepipeline-actions';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
-import { Aws, CfnCapabilities, Duration, Fn, Lazy, PhysicalName, Stack } from '@aws-cdk/core';
+import { Aws, CfnCapabilities, Duration, PhysicalName, Stack } from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
 import { Construct } from 'constructs';
 import { AssetType, FileSet, IFileSetProducer, ManualApprovalStep, ShellStep, StackAsset, StackDeployment, Step } from '../blueprint';
 import { DockerCredential, dockerCredentialsInstallCommands, DockerCredentialUsage } from '../docker-credentials';
 import { GraphNodeCollection, isGraph, AGraphNode, PipelineGraph } from '../helpers-internal';
 import { PipelineBase } from '../main';
 import { AssetSingletonRole } from '../private/asset-singleton-role';
+import { CachedFnSub } from '../private/cached-fnsub';
 import { preferredCliVersion } from '../private/cli-version';
 import { appOf, assemblyBuilderOf, embeddedAsmPath, obtainScope } from '../private/construct-internals';
 import { toPosixPath } from '../private/fs';
@@ -293,16 +294,12 @@ export class CodePipeline extends PipelineBase {
   private readonly selfMutation: boolean;
   private _myCxAsmRoot?: string;
   private readonly dockerCredentials: DockerCredential[];
+  private readonly cachedFnSub = new CachedFnSub();
 
   /**
    * Asset roles shared for publishing
    */
-  private readonly assetCodeBuildRoles: Record<string, iam.IRole> = {};
-
-  /**
-   * Per asset type, the target role ARNs that need to be assumed
-   */
-  private readonly assetPublishingRoles: Record<string, Set<string>> = {};
+  private readonly assetCodeBuildRoles: Map<AssetType, AssetSingletonRole> = new Map();
 
   /**
    * This is set to the very first artifact produced in the pipeline
@@ -678,15 +675,13 @@ export class CodePipeline extends PipelineBase {
       throw new Error('All assets in a single publishing step must be of the same type');
     }
 
-    const publishingRoles = this.assetPublishingRoles[assetType] = (this.assetPublishingRoles[assetType] ?? new Set());
-    for (const asset of assets) {
-      if (asset.assetPublishingRoleArn) {
-        publishingRoles.add(asset.assetPublishingRoleArn);
-      }
-    }
-
     const role = this.obtainAssetCodeBuildRole(assets[0].assetType);
 
+    for (const roleArn of assets.flatMap(a => a.assetPublishingRoleArn ? [a.assetPublishingRoleArn] : [])) {
+      // The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted.
+      role.addAssumeRole(this.cachedFnSub.fnSub(roleArn));
+    };
+
     // The base commands that need to be run
     const script = new CodeBuildStep(node.id, {
       commands,
@@ -824,9 +819,10 @@ export class CodePipeline extends PipelineBase {
    * Modeled after the CodePipeline role and 'CodePipelineActionRole' roles.
    * Generates one role per asset type to separate file and Docker/image-based permissions.
    */
-  private obtainAssetCodeBuildRole(assetType: AssetType): iam.IRole {
-    if (this.assetCodeBuildRoles[assetType]) {
-      return this.assetCodeBuildRoles[assetType];
+  private obtainAssetCodeBuildRole(assetType: AssetType): AssetSingletonRole {
+    const existing = this.assetCodeBuildRoles.get(assetType);
+    if (existing) {
+      return existing;
     }
 
     const stack = Stack.of(this);
@@ -840,22 +836,15 @@ export class CodePipeline extends PipelineBase {
       ),
     });
 
-    // Publishing role access
-    // The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted.
-    // Lazy-evaluated so all asset publishing roles are included.
-    assetRole.addToPolicy(new iam.PolicyStatement({
-      actions: ['sts:AssumeRole'],
-      resources: Lazy.list({ produce: () => Array.from(this.assetPublishingRoles[assetType] ?? []).map(arn => Fn.sub(arn)) }),
-    }));
-
     // Grant pull access for any ECR registries and secrets that exist
     if (assetType === AssetType.DOCKER_IMAGE) {
       this.dockerCredentials.forEach(reg => reg.grantRead(assetRole, DockerCredentialUsage.ASSET_PUBLISHING));
     }
 
-    this.assetCodeBuildRoles[assetType] = assetRole;
+    this.assetCodeBuildRoles.set(assetType, assetRole);
     return assetRole;
   }
+
 }
 
 function dockerUsageFromCodeBuild(cbt: CodeBuildProjectType): DockerCredentialUsage | undefined {