Skip to content

Commit

Permalink
fix(iam): AWS Managed Policy ARNs are not deduped (#17623)
Browse files Browse the repository at this point in the history
Managed Policy ARNs should be deduped when added to a Role,
otherwise the deployment is going to fail.

Remove the unnecessary use of `Lazy.uncachedString` to make sure that
the ARNs of two `ManagedPolicy.fromAwsManagedPolicyName()` policies
are consistent.

Fixes #17552.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Dec 10, 2021
1 parent 450f7ca commit ed4a4b4
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 15 deletions.
19 changes: 8 additions & 11 deletions packages/@aws-cdk/aws-iam/lib/managed-policy.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ArnFormat, IResolveContext, Lazy, Resource, Stack } from '@aws-cdk/core';
import { ArnFormat, Resource, Stack, Arn, Aws } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { IGroup } from './group';
import { CfnManagedPolicy } from './iam.generated';
Expand Down Expand Up @@ -156,16 +156,13 @@ export class ManagedPolicy extends Resource implements IManagedPolicy {
*/
public static fromAwsManagedPolicyName(managedPolicyName: string): IManagedPolicy {
class AwsManagedPolicy implements IManagedPolicy {
public readonly managedPolicyArn = Lazy.uncachedString({
produce(ctx: IResolveContext) {
return Stack.of(ctx.scope).formatArn({
service: 'iam',
region: '', // no region for managed policy
account: 'aws', // the account for a managed policy is 'aws'
resource: 'policy',
resourceName: managedPolicyName,
});
},
public readonly managedPolicyArn = Arn.format({
partition: Aws.PARTITION,
service: 'iam',
region: '', // no region for managed policy
account: 'aws', // the account for a managed policy is 'aws'
resource: 'policy',
resourceName: managedPolicyName,
});
}
return new AwsManagedPolicy();
Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-iam/test/managed-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -614,3 +614,10 @@ describe('managed policy', () => {
});
});
});

test('ARN for two instances of the same AWS Managed Policy is the same', () => {
const mp1 = ManagedPolicy.fromAwsManagedPolicyName('foo/bar');
const mp2 = ManagedPolicy.fromAwsManagedPolicyName('foo/bar');

expect(mp1.managedPolicyArn).toEqual(mp2.managedPolicyArn);
});
14 changes: 10 additions & 4 deletions packages/@aws-cdk/core/lib/arn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,16 @@ export class Arn {
* the 'scope' is attached to. If all ARN pieces are supplied, the supplied scope
* can be 'undefined'.
*/
public static format(components: ArnComponents, stack: Stack): string {
const partition = components.partition ?? stack.partition;
const region = components.region ?? stack.region;
const account = components.account ?? stack.account;
public static format(components: ArnComponents, stack?: Stack): string {
const partition = components.partition ?? stack?.partition;
const region = components.region ?? stack?.region;
const account = components.account ?? stack?.account;

// Catch both 'null' and 'undefined'
if (partition == null || region == null || account == null) {
throw new Error(`Arn.format: partition (${partition}), region (${region}), and account (${account}) must all be passed if stack is not passed.`);
}

const sep = components.sep ?? (components.arnFormat === ArnFormat.COLON_RESOURCE_NAME ? ':' : '/');

const values = [
Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/core/test/arn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ describe('arn', () => {

});

test('cannot rely on defaults when stack not known', () => {
expect(() =>
Arn.format({
service: 'sqs',
resource: 'myqueuename',
})).toThrow(/must all be passed if stack is not/);
});

test('create from components with specific values for the various components', () => {
const stack = new Stack();

Expand Down

0 comments on commit ed4a4b4

Please sign in to comment.