Skip to content

Commit 8977d0d

Browse files
liwewang-amazonliwewang-amazon
authored
fix(servicecatalogappregistry): RAM Share is replaced on every change to Application (#24760)
`Application.shareApplication` can be called multiple times which generates different resources within the same `Application` construct. The share construct id generation was based on child length of `Application` construct. This makes every change to the `Application` construct can unnecessarily replace the RAM share when the share actually needs no update. Similar to the solution for `addAttributeGroup` introduced in the [previous update](#24672), this commit starts to require both construct id and share name in the `Application.shareApplication` method input, which are used to create RAM share construct. For `ApplicationAssociator`, `Application.shareApplication` is called for cross-account stack associations. Share construct id depends on the node unique id of the application in `ApplicationAssociator` and the node unique id of the stack to be associated. This reduces unnecessary share replacement in `ApplicationAssociator` stack. BREAKING CHANGE: This commit involves share replacement during the deployment of `ApplicationAssociator` due to share construct id update. After this change, frequent share replacements due to structural change in `Application` construct should be avoided. `Application.shareApplication` starts to require construct id (first argument) and share name (added in `ShareOption`) as input. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent bc3db02 commit 8977d0d

19 files changed

+111
-83
lines changed

‎packages/@aws-cdk/aws-servicecatalogappregistry/README.md

+8-4
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,8 @@ import * as iam from '@aws-cdk/aws-iam';
289289
declare const application: appreg.Application;
290290
declare const myRole: iam.IRole;
291291
declare const myUser: iam.IUser;
292-
application.shareApplication({
292+
application.shareApplication('MyShareId', {
293+
name:'MyShare',
293294
accounts: ['123456789012'],
294295
organizationArns: ['arn:aws:organizations::123456789012:organization/o-my-org-id'],
295296
roles: [myRole],
@@ -302,7 +303,8 @@ E.g., sharing an application with multiple accounts and allowing the accounts to
302303
```ts
303304
import * as iam from '@aws-cdk/aws-iam';
304305
declare const application: appreg.Application;
305-
application.shareApplication({
306+
application.shareApplication('MyShareId', {
307+
name: 'MyShare',
306308
accounts: ['123456789012', '234567890123'],
307309
sharePermission: appreg.SharePermission.ALLOW_ACCESS,
308310
});
@@ -315,7 +317,8 @@ import * as iam from '@aws-cdk/aws-iam';
315317
declare const attributeGroup: appreg.AttributeGroup;
316318
declare const myRole: iam.IRole;
317319
declare const myUser: iam.IUser;
318-
attributeGroup.shareAttributeGroup({
320+
attributeGroup.shareAttributeGroup('MyShareId', {
321+
name: 'MyShare',
319322
accounts: ['123456789012'],
320323
organizationArns: ['arn:aws:organizations::123456789012:organization/o-my-org-id'],
321324
roles: [myRole],
@@ -328,7 +331,8 @@ E.g., sharing an application with multiple accounts and allowing the accounts to
328331
```ts
329332
import * as iam from '@aws-cdk/aws-iam';
330333
declare const attributeGroup: appreg.AttributeGroup;
331-
attributeGroup.shareAttributeGroup({
334+
attributeGroup.shareAttributeGroup('MyShareId', {
335+
name: 'MyShare',
332336
accounts: ['123456789012', '234567890123'],
333337
sharePermission: appreg.SharePermission.ALLOW_ACCESS,
334338
});

‎packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

+6-6
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { CfnResourceShare } from '@aws-cdk/aws-ram';
22
import * as cdk from '@aws-cdk/core';
3-
import { Names } from '@aws-cdk/core';
43
import { Construct } from 'constructs';
54
import { StageStackAssociator } from './aspects/stack-associator';
65
import { AttributeGroup, IAttributeGroup } from './attribute-group';
@@ -90,9 +89,10 @@ export interface IApplication extends cdk.IResource {
9089
/**
9190
* Share this application with other IAM entities, accounts, or OUs.
9291
*
92+
* @param id The construct name for the share.
9393
* @param shareOptions The options for the share.
9494
*/
95-
shareApplication(shareOptions: ShareOptions): void;
95+
shareApplication(id: string, shareOptions: ShareOptions): void;
9696

9797
/**
9898
* Associate this application with all stacks under the construct node.
@@ -205,13 +205,13 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
205205
* Share an application with accounts, organizations and OUs, and IAM roles and users.
206206
* The application will become available to end users within those principals.
207207
*
208+
* @param id The construct name for the share.
208209
* @param shareOptions The options for the share.
209210
*/
210-
public shareApplication(shareOptions: ShareOptions): void {
211+
public shareApplication(id: string, shareOptions: ShareOptions): void {
211212
const principals = getPrincipalsforSharing(shareOptions);
212-
const shareName = `RAMShare${hashValues(Names.nodeUniqueId(this.node), this.node.children.length.toString())}`;
213-
new CfnResourceShare(this, shareName, {
214-
name: shareName,
213+
new CfnResourceShare(this, id, {
214+
name: shareOptions.name,
215215
allowExternalPrincipals: false,
216216
principals: principals,
217217
resourceArns: [this.applicationArn],

‎packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts

+5-3
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
import { IAspect, Stack, Stage, Annotations } from '@aws-cdk/core';
1+
import { IAspect, Stack, Stage, Annotations, Names } from '@aws-cdk/core';
22
import { IConstruct } from 'constructs';
33
import { IApplication } from '../application';
44
import { ApplicationAssociator } from '../application-associator';
5-
import { SharePermission } from '../common';
5+
import { hashValues, SharePermission } from '../common';
66
import { isRegionUnresolved, isAccountUnresolved } from '../private/utils';
77

88
export interface StackAssociatorBaseProps {
@@ -112,7 +112,9 @@ abstract class StackAssociatorBase implements IAspect {
112112

113113
if (node.account != this.application.env.account && !this.sharedAccounts.has(node.account)) {
114114
if (this.associateCrossAccountStacks) {
115-
this.application.shareApplication({
115+
const shareId = `ApplicationShare${hashValues(Names.nodeUniqueId(this.application.node), Names.nodeUniqueId(node.node))}`;
116+
this.application.shareApplication(shareId, {
117+
name: shareId,
116118
accounts: [node.account],
117119
sharePermission: SharePermission.ALLOW_ACCESS,
118120
});

‎packages/@aws-cdk/aws-servicecatalogappregistry/lib/attribute-group.ts

+5-6
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { CfnResourceShare } from '@aws-cdk/aws-ram';
22
import * as cdk from '@aws-cdk/core';
3-
import { Names } from '@aws-cdk/core';
43
import { Construct } from 'constructs';
54
import { IApplication } from './application';
65
import { getPrincipalsforSharing, hashValues, ShareOptions, SharePermission } from './common';
@@ -29,9 +28,10 @@ export interface IAttributeGroup extends cdk.IResource {
2928
/**
3029
* Share the attribute group resource with other IAM entities, accounts, or OUs.
3130
*
31+
* @param id The construct name for the share.
3232
* @param shareOptions The options for the share.
3333
*/
34-
shareAttributeGroup(shareOptions: ShareOptions): void;
34+
shareAttributeGroup(id: string, shareOptions: ShareOptions): void;
3535
}
3636

3737
/**
@@ -77,11 +77,10 @@ abstract class AttributeGroupBase extends cdk.Resource implements IAttributeGrou
7777
}
7878
}
7979

80-
public shareAttributeGroup(shareOptions: ShareOptions): void {
80+
public shareAttributeGroup(id: string, shareOptions: ShareOptions): void {
8181
const principals = getPrincipalsforSharing(shareOptions);
82-
const shareName = `RAMShare${hashValues(Names.nodeUniqueId(this.node), this.node.children.length.toString())}`;
83-
new CfnResourceShare(this, shareName, {
84-
name: shareName,
82+
new CfnResourceShare(this, id, {
83+
name: shareOptions.name,
8584
allowExternalPrincipals: false,
8685
principals: principals,
8786
resourceArns: [this.attributeGroupArn],

‎packages/@aws-cdk/aws-servicecatalogappregistry/lib/common.ts

+5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ export enum SharePermission {
2020
* The options that are passed into a share of an Application or Attribute Group.
2121
*/
2222
export interface ShareOptions {
23+
/**
24+
* Name of the share.
25+
*
26+
*/
27+
readonly name: string;
2328
/**
2429
* A list of AWS accounts that the application will be shared with.
2530
*

‎packages/@aws-cdk/aws-servicecatalogappregistry/test/application.test.ts

+21-13
Original file line numberDiff line numberDiff line change
@@ -269,32 +269,36 @@ describe('Application', () => {
269269

270270
test('fails for sharing application without principals', () => {
271271
expect(() => {
272-
application.shareApplication({});
272+
application.shareApplication('MyShareId', {
273+
name: 'MyShare',
274+
});
273275
}).toThrow(/An entity must be provided for the share/);
274276
});
275277

276278
test('share application with an organization', () => {
277-
application.shareApplication({
279+
application.shareApplication('MyShareId', {
280+
name: 'MyShare',
278281
organizationArns: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
279282
});
280283

281284
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
282285
AllowExternalPrincipals: false,
283-
Name: 'RAMSharee6e0e560e6f8',
286+
Name: 'MyShare',
284287
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
285288
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
286289
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
287290
});
288291
});
289292

290293
test('share application with an account', () => {
291-
application.shareApplication({
294+
application.shareApplication('MyShareId', {
295+
name: 'MyShare',
292296
accounts: ['123456789012'],
293297
});
294298

295299
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
296300
AllowExternalPrincipals: false,
297-
Name: 'RAMSharee6e0e560e6f8',
301+
Name: 'MyShare',
298302
Principals: ['123456789012'],
299303
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
300304
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
@@ -304,13 +308,14 @@ describe('Application', () => {
304308
test('share application with an IAM role', () => {
305309
const myRole = iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::123456789012:role/myRole');
306310

307-
application.shareApplication({
311+
application.shareApplication('MyShareId', {
312+
name: 'MyShare',
308313
roles: [myRole],
309314
});
310315

311316
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
312317
AllowExternalPrincipals: false,
313-
Name: 'RAMSharee6e0e560e6f8',
318+
Name: 'MyShare',
314319
Principals: ['arn:aws:iam::123456789012:role/myRole'],
315320
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
316321
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
@@ -320,43 +325,46 @@ describe('Application', () => {
320325
test('share application with an IAM user', () => {
321326
const myUser = iam.User.fromUserArn(stack, 'MyUser', 'arn:aws:iam::123456789012:user/myUser');
322327

323-
application.shareApplication({
328+
application.shareApplication('MyShareId', {
329+
name: 'MyShare',
324330
users: [myUser],
325331
});
326332

327333
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
328334
AllowExternalPrincipals: false,
329-
Name: 'RAMSharee6e0e560e6f8',
335+
Name: 'MyShare',
330336
Principals: ['arn:aws:iam::123456789012:user/myUser'],
331337
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
332338
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
333339
});
334340
});
335341

336342
test('share application with organization, give explicit read only access to an application', () => {
337-
application.shareApplication({
343+
application.shareApplication('MyShareId', {
344+
name: 'MyShare',
338345
organizationArns: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
339346
sharePermission: appreg.SharePermission.READ_ONLY,
340347
});
341348

342349
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
343350
AllowExternalPrincipals: false,
344-
Name: 'RAMSharee6e0e560e6f8',
351+
Name: 'MyShare',
345352
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
346353
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
347354
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
348355
});
349356
});
350357

351358
test('share application with organization, allow access to associate resources and attribute group with an application', () => {
352-
application.shareApplication({
359+
application.shareApplication('MyShareId', {
360+
name: 'MyShare',
353361
organizationArns: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
354362
sharePermission: appreg.SharePermission.ALLOW_ACCESS,
355363
});
356364

357365
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
358366
AllowExternalPrincipals: false,
359-
Name: 'RAMSharee6e0e560e6f8',
367+
Name: 'MyShare',
360368
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
361369
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
362370
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationAllowAssociation'],

‎packages/@aws-cdk/aws-servicecatalogappregistry/test/attribute-group.test.ts

+21-13
Original file line numberDiff line numberDiff line change
@@ -212,32 +212,36 @@ describe('Attribute Group', () => {
212212

213213
test('fails for sharing attribute group without principals', () => {
214214
expect(() => {
215-
attributeGroup.shareAttributeGroup({});
215+
attributeGroup.shareAttributeGroup('MyShareId', {
216+
name: 'MyShare',
217+
});
216218
}).toThrow(/An entity must be provided for the share/);
217219
});
218220

219221
test('share attribute group with an organization', () => {
220-
attributeGroup.shareAttributeGroup({
222+
attributeGroup.shareAttributeGroup('MyShareId', {
223+
name: 'MyShare',
221224
organizationArns: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
222225
});
223226

224227
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
225228
AllowExternalPrincipals: false,
226-
Name: 'RAMShare76d2681489c0',
229+
Name: 'MyShare',
227230
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
228231
ResourceArns: [{ 'Fn::GetAtt': ['MyAttributeGroup99099500', 'Arn'] }],
229232
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupReadOnly'],
230233
});
231234
});
232235

233236
test('share attribute group with an account', () => {
234-
attributeGroup.shareAttributeGroup({
237+
attributeGroup.shareAttributeGroup('MyShareId', {
238+
name: 'MyShare',
235239
accounts: ['123456789012'],
236240
});
237241

238242
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
239243
AllowExternalPrincipals: false,
240-
Name: 'RAMShare76d2681489c0',
244+
Name: 'MyShare',
241245
Principals: ['123456789012'],
242246
ResourceArns: [{ 'Fn::GetAtt': ['MyAttributeGroup99099500', 'Arn'] }],
243247
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupReadOnly'],
@@ -247,13 +251,14 @@ describe('Attribute Group', () => {
247251
test('share attribute group with an IAM role', () => {
248252
const myRole = iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::123456789012:role/myRole');
249253

250-
attributeGroup.shareAttributeGroup({
254+
attributeGroup.shareAttributeGroup('MyShareId', {
255+
name: 'MyShare',
251256
roles: [myRole],
252257
});
253258

254259
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
255260
AllowExternalPrincipals: false,
256-
Name: 'RAMShare76d2681489c0',
261+
Name: 'MyShare',
257262
Principals: ['arn:aws:iam::123456789012:role/myRole'],
258263
ResourceArns: [{ 'Fn::GetAtt': ['MyAttributeGroup99099500', 'Arn'] }],
259264
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupReadOnly'],
@@ -263,43 +268,46 @@ describe('Attribute Group', () => {
263268
test('share attribute group with an IAM user', () => {
264269
const myUser = iam.User.fromUserArn(stack, 'MyUser', 'arn:aws:iam::123456789012:user/myUser');
265270

266-
attributeGroup.shareAttributeGroup({
271+
attributeGroup.shareAttributeGroup('MyShareId', {
272+
name: 'MyShare',
267273
users: [myUser],
268274
});
269275

270276
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
271277
AllowExternalPrincipals: false,
272-
Name: 'RAMShare76d2681489c0',
278+
Name: 'MyShare',
273279
Principals: ['arn:aws:iam::123456789012:user/myUser'],
274280
ResourceArns: [{ 'Fn::GetAtt': ['MyAttributeGroup99099500', 'Arn'] }],
275281
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupReadOnly'],
276282
});
277283
});
278284

279285
test('share attribute group with organization, give explicit read only access to the attribute group', () => {
280-
attributeGroup.shareAttributeGroup({
286+
attributeGroup.shareAttributeGroup('MyShareId', {
287+
name: 'MyShare',
281288
organizationArns: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
282289
sharePermission: appreg.SharePermission.READ_ONLY,
283290
});
284291

285292
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
286293
AllowExternalPrincipals: false,
287-
Name: 'RAMShare76d2681489c0',
294+
Name: 'MyShare',
288295
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
289296
ResourceArns: [{ 'Fn::GetAtt': ['MyAttributeGroup99099500', 'Arn'] }],
290297
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupReadOnly'],
291298
});
292299
});
293300

294301
test('share attribute group with organization, give access to mutate attribute groups', () => {
295-
attributeGroup.shareAttributeGroup({
302+
attributeGroup.shareAttributeGroup('MyShareId', {
303+
name: 'MyShare',
296304
organizationArns: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
297305
sharePermission: appreg.SharePermission.ALLOW_ACCESS,
298306
});
299307

300308
Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
301309
AllowExternalPrincipals: false,
302-
Name: 'RAMShare76d2681489c0',
310+
Name: 'MyShare',
303311
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
304312
ResourceArns: [{ 'Fn::GetAtt': ['MyAttributeGroup99099500', 'Arn'] }],
305313
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupAllowAssociation'],

‎packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/integ-servicecatalogappregistry-application.assets.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
{
22
"version": "31.0.0",
33
"files": {
4-
"5fbf2a286122f4bc412b1730f96351e289444b1122006f36e4ade8fae8442765": {
4+
"461d235e9497deb16b9209be4a927c7d0dc7aa06d668e38bfb19a90db8e4a4b2": {
55
"source": {
66
"path": "integ-servicecatalogappregistry-application.template.json",
77
"packaging": "file"
88
},
99
"destinations": {
1010
"current_account-current_region": {
1111
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
12-
"objectKey": "5fbf2a286122f4bc412b1730f96351e289444b1122006f36e4ade8fae8442765.json",
12+
"objectKey": "461d235e9497deb16b9209be4a927c7d0dc7aa06d668e38bfb19a90db8e4a4b2.json",
1313
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
1414
}
1515
}

‎packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/integ-servicecatalogappregistry-application.template.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,10 @@
7979
}
8080
}
8181
},
82-
"TestApplicationRAMShare004736f08f8e57044D5D": {
82+
"TestApplicationMyShareIdE1044482": {
8383
"Type": "AWS::RAM::ResourceShare",
8484
"Properties": {
85-
"Name": "RAMShare004736f08f8e",
85+
"Name": "MyShare",
8686
"AllowExternalPrincipals": false,
8787
"PermissionArns": [
8888
"arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly"

0 commit comments

Comments
 (0)