Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(servicecatalogappregistry): Associate an application with attribute group #24378

Merged
merged 11 commits into from
Mar 6, 2023
Merged

fix(servicecatalogappregistry): Associate an application with attribute group #24378

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3bbc9a1
Issue-23641: Fixing dynamic description bug
sangosh Jan 11, 2023
4262fdb
Changing the integration test
sangosh Jan 11, 2023
6208b23
Merge branch 'main' into main
mergify[bot] Jan 12, 2023
fe90d28
Merge branch 'aws:main' into main
santanugho Jan 19, 2023
f682b68
Merge branch 'aws:main' into main
santanugho Feb 27, 2023
ae03cb5
Associate application with attribute group
sangosh Feb 28, 2023
c3dd40a
Associate application to AG
sangosh Feb 28, 2023
fe247c8
Associate application to attribute group
sangosh Mar 1, 2023
9249be2
Merge branch 'main' into Issue-AWS242-27542
santanugho Mar 3, 2023
7a2804c
Fixing the test
sangosh Mar 3, 2023
a329869
Merge branch 'main' into Issue-AWS242-27542
mergify[bot] Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,45 @@ const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplicati
});
```

If you want to associate an Attribute Group with application created by `ApplicationAssociator`, then use as shown in the example below:

```ts
import * as cdk from "@aws-cdk/core";

const app = new App();

class CustomAppRegistryAttributeGroup extends cdk.Stack {
public readonly attributeGroup: appreg.AttributeGroup
constructor(scope: Construct, id: string, props?: cdk.StackProps) {
super(scope, id, props);
const myAttributeGroup = new appreg.AttributeGroup(app, 'MyFirstAttributeGroup', {
attributeGroupName: 'MyAttributeGroupName',
description: 'Test attribute group',
attributes: {},
});

this.attributeGroup = myAttributeGroup;
}
}
Comment on lines +109 to +121
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really expect people to define a stack for their AttributeGroup? Couldn't it go into one of the other stacks?

For example, wouldn't it make sense for a user to do:

const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplication', {
  applications: [ /* ... */],
  attributes: { 
    // ...
  },
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes is not a valid input as per ApplicationAssociatorProps. Attribute group can definitely be a part of some other stack, however customers need to explicitly associate the attribute group to the application, as ApplicationAssociator is responsible to associate the stacks within the scope, it wont automatically associate the underlying AGs within a stack.

Copy link
Contributor

@rix0rrr rix0rrr Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes is not a valid input as per ApplicationAssociatorProps

I understand.

The nice thing about software is that it is not like the laws of physics. It is in fact changeable.

  • FACT: "attributes" can not be passed to the ApplicationAssociator
  • QUESTION: would it be better/easier/more convenient/more obvious IF "attributes" could be passed to ApplicationAssociator, and the creation of the AttributeGroup happens implicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I like this idea of including attribute-group in application-associator construct. However, can we first prioritize this bug fix which will unblock our customers and they should be able to associate an attribute group to an application.

I will do some more deep dive on including attribute-group in application-associator, its pros and cons and will let you know about our plan to modify application-associator in order to include attribute-group.

Let me know if this plan works for you.


const customAttributeGroup = new CustomAppRegistryAttributeGroup(app, 'AppRegistryAttributeGroup');

const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
applicationName: 'MyAssociatedApplication',
// 'Application containing stacks deployed via CDK.' is the default
applicationDescription: 'Associated Application description',
stackName: 'MyAssociatedApplicationStack',
// AWS Account and Region that are implied by the current CLI configuration is the default
env: { account: '123456789012', region: 'us-east-1' },
})],
});

// Associate application to the attribute group.
customAttributeGroup.attributeGroup.associateWith(associatedApp.appRegistryApplication());

```

If you are using CDK Pipelines to deploy your application, the application stacks will be inside Stages, and
ApplicationAssociator will not be able to find them. Call `associateStage` on each Stage object before adding it to the
Pipeline, as shown in the example below:
Expand Down Expand Up @@ -191,6 +230,16 @@ declare const attributeGroup: appreg.AttributeGroup;
application.associateAttributeGroup(attributeGroup);
```

### Associating an attribute group with application

You can associate an application with an attribute group with `associateWith`:

```ts
declare const application: appreg.Application;
declare const attributeGroup: appreg.AttributeGroup;
attributeGroup.associateWith(application);
```

### Associating application with a Stack

You can associate a stack with an application with the `associateStack()` API:
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
/**
* Associate an attribute group with application
* If the attribute group is already associated, it will ignore duplicate request.
*
* @deprecated Use `AttributeGroup.associateWith` instead.
*/
public associateAttributeGroup(attributeGroup: IAttributeGroup): void {
if (!this.associatedAttributeGroups.has(attributeGroup.node.addr)) {
Expand Down
35 changes: 34 additions & 1 deletion packages/@aws-cdk/aws-servicecatalogappregistry/lib/attribute-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ import { CfnResourceShare } from '@aws-cdk/aws-ram';
import * as cdk from '@aws-cdk/core';
import { Names } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { IApplication } from './application';
import { getPrincipalsforSharing, hashValues, ShareOptions, SharePermission } from './common';
import { InputValidator } from './private/validation';
import { CfnAttributeGroup } from './servicecatalogappregistry.generated';
import { CfnAttributeGroup, CfnAttributeGroupAssociation } from './servicecatalogappregistry.generated';

const ATTRIBUTE_GROUP_READ_ONLY_RAM_PERMISSION_ARN = 'arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupReadOnly';
const ATTRIBUTE_GROUP_ALLOW_ACCESS_RAM_PERMISSION_ARN = 'arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryAttributeGroupAllowAssociation';
Expand Down Expand Up @@ -58,6 +59,23 @@ export interface AttributeGroupProps {
abstract class AttributeGroupBase extends cdk.Resource implements IAttributeGroup {
public abstract readonly attributeGroupArn: string;
public abstract readonly attributeGroupId: string;
private readonly associatedApplications: Set<string> = new Set();

/**
* Associate an application with attribute group
* If the attribute group is already associated, it will ignore duplicate request.
*/
public associateWith(application: IApplication): void {
if (!this.associatedApplications.has(application.node.addr)) {
const hashId = this.generateUniqueHash(application.node.addr);
new CfnAttributeGroupAssociation(this, `ApplicationAttributeGroupAssociation${hashId}`, {
application: application.stack === cdk.Stack.of(this) ? application.applicationId : application.applicationName ?? application.applicationId,
attributeGroup: this.attributeGroupId,
});

this.associatedApplications.add(application.node.addr);
}
}

public shareAttributeGroup(shareOptions: ShareOptions): void {
const principals = getPrincipalsforSharing(shareOptions);
Expand Down Expand Up @@ -85,6 +103,11 @@ abstract class AttributeGroupBase extends cdk.Resource implements IAttributeGrou
return shareOptions.sharePermission ?? ATTRIBUTE_GROUP_READ_ONLY_RAM_PERMISSION_ARN;
}
}

/**
* Create a unique hash
*/
protected abstract generateUniqueHash(resourceAddress: string): string;
}

/**
Expand All @@ -109,6 +132,10 @@ export class AttributeGroup extends AttributeGroupBase implements IAttributeGrou
class Import extends AttributeGroupBase {
public readonly attributeGroupArn = attributeGroupArn;
public readonly attributeGroupId = attributeGroupId!;

protected generateUniqueHash(resourceAddress: string): string {
return hashValues(this.attributeGroupArn, resourceAddress);
}
}

return new Import(scope, id, {
Expand All @@ -118,6 +145,7 @@ export class AttributeGroup extends AttributeGroupBase implements IAttributeGrou

public readonly attributeGroupArn: string;
public readonly attributeGroupId: string;
private readonly nodeAddress: string;

constructor(scope: Construct, id: string, props: AttributeGroupProps) {
super(scope, id);
Expand All @@ -132,6 +160,11 @@ export class AttributeGroup extends AttributeGroupBase implements IAttributeGrou

this.attributeGroupArn = attributeGroup.attrArn;
this.attributeGroupId = attributeGroup.attrId;
this.nodeAddress = cdk.Names.nodeUniqueId(attributeGroup.node);
}

protected generateUniqueHash(resourceAddress: string): string {
return hashValues(this.nodeAddress, resourceAddress);
}

private validateAttributeGroupProps(props: AttributeGroupProps) {
Expand Down
47 changes: 47 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/test/application-associator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,38 @@ describe('Scope based Associations with Application within Same Account', () =>
});
});
});

describe('Associate attribute group with Application', () => {
let app: cdk.App;
beforeEach(() => {
app = new cdk.App({
context: {
'@aws-cdk/core:newStyleStackSynthesis': false,
},
});
});

test('Associate Attribute Group with application created by ApplicationAssociator', () => {

const customAttributeGroup = new CustomAppRegistryAttributeGroup(app, 'AppRegistryAttributeGroup');

const appAssociator = new appreg.ApplicationAssociator(app, 'TestApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
applicationName: 'TestAssociatedApplication',
stackName: 'TestAssociatedApplicationStack',
})],
});

customAttributeGroup.attributeGroup.associateWith(appAssociator.appRegistryApplication());
Template.fromStack(customAttributeGroup.attributeGroup.stack).resourceCountIs('AWS::ServiceCatalogAppRegistry::AttributeGroupAssociation', 1);
Template.fromStack(customAttributeGroup.attributeGroup.stack).hasResourceProperties('AWS::ServiceCatalogAppRegistry::AttributeGroupAssociation', {
Application: 'TestAssociatedApplication',
AttributeGroup: { 'Fn::GetAtt': ['MyFirstAttributeGroupDBC21379', 'Id'] },
});

});
});

describe('Scope based Associations with Application with Cross Region/Account', () => {
let app: cdk.App;
beforeEach(() => {
Expand Down Expand Up @@ -211,3 +243,18 @@ class AppRegistrySampleStack extends cdk.Stack {
super(scope, id, props);
}
}

class CustomAppRegistryAttributeGroup extends cdk.Stack {
public readonly attributeGroup: appreg.AttributeGroup;

constructor(scope: Construct, id: string, props?: cdk.StackProps) {
super(scope, id, props);
const myAttributeGroup = new appreg.AttributeGroup(this, 'MyFirstAttributeGroup', {
attributeGroupName: 'MyFirstAttributeGroupName',
description: 'Test attribute group',
attributes: {},
});

this.attributeGroup = myAttributeGroup;
}
}
24 changes: 24 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/test/attribute-group.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,30 @@ describe('Attribute Group', () => {
});
});

describe('Associate application to an attribute group', () => {
let attributeGroup: appreg.AttributeGroup;

beforeEach(() => {
attributeGroup = new appreg.AttributeGroup(stack, 'MyAttributeGroupForAssociation', {
attributeGroupName: 'MyAttributeGroupForAssociation',
attributes: {},
});
});

test('Associate an application to an attribute group', () => {
const application = new appreg.Application(stack, 'MyApplication', {
applicationName: 'MyTestApplication',
});
attributeGroup.associateWith(application);
Template.fromStack(stack).hasResourceProperties('AWS::ServiceCatalogAppRegistry::AttributeGroupAssociation', {
Application: { 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Id'] },
AttributeGroup: { 'Fn::GetAtt': ['MyAttributeGroupForAssociation6B3E1329', 'Id'] },
});

});

});

describe('Resource sharing of an attribute group', () => {
let attributeGroup: appreg.AttributeGroup;

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"29.0.0"}
{"version":"30.1.0"}
6 changes: 3 additions & 3 deletions ...est/integ.application.js.snapshot/integ-servicecatalogappregistry-application.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "29.0.0",
"version": "30.1.0",
"files": {
"8bf01e42edcc7e9cd4c65b9db9e52d2d6564f931bc84e7f564a1a4c43e499d6e": {
"0d4e060fe5da6b164b9df46b0dc0cd20e7962c6cb531ffe08e6e5b99418f13de": {
"source": {
"path": "integ-servicecatalogappregistry-application.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "8bf01e42edcc7e9cd4c65b9db9e52d2d6564f931bc84e7f564a1a4c43e499d6e.json",
"objectKey": "0d4e060fe5da6b164b9df46b0dc0cd20e7962c6cb531ffe08e6e5b99418f13de.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...t/integ.application.js.snapshot/integ-servicecatalogappregistry-application.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"Type": "AWS::ServiceCatalogAppRegistry::Application",
"Properties": {
"Name": "TestApplication",
"Description": "Test application description"
"Description": "My application description"
}
},
"TestApplicationResourceAssociationd232b63e52a8414E905D": {
Expand Down
2 changes: 1 addition & 1 deletion ...ages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "29.0.0",
"version": "30.1.0",
"testCases": {
"integ.application": {
"stacks": [
Expand Down
4 changes: 2 additions & 2 deletions ...s/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "29.0.0",
"version": "30.1.0",
"artifacts": {
"integ-servicecatalogappregistry-application.assets": {
"type": "cdk:asset-manifest",
Expand All @@ -17,7 +17,7 @@
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/8bf01e42edcc7e9cd4c65b9db9e52d2d6564f931bc84e7f564a1a4c43e499d6e.json",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/0d4e060fe5da6b164b9df46b0dc0cd20e7962c6cb531ffe08e6e5b99418f13de.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/tree.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"aws:cdk:cloudformation:type": "AWS::ServiceCatalogAppRegistry::Application",
"aws:cdk:cloudformation:props": {
"name": "TestApplication",
"description": "Test application description"
"description": "My application description"
}
},
"constructInfo": {
Expand Down Expand Up @@ -249,7 +249,7 @@
"path": "Tree",
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.1.209"
"version": "10.1.252"
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const stack = new cdk.Stack(app, 'integ-servicecatalogappregistry-application');

const application = new appreg.Application(stack, 'TestApplication', {
applicationName: 'TestApplication',
description: 'Test application description',
description: 'My application description',
});

const attributeGroup = new appreg.AttributeGroup(stack, 'TestAttributeGroup', {
Expand Down
2 changes: 1 addition & 1 deletion ...ges/@aws-cdk/aws-servicecatalogappregistry/test/integ.attribute-group.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"22.0.0"}
{"version":"30.1.0"}
6 changes: 3 additions & 3 deletions ...g.attribute-group.js.snapshot/integ-servicecatalogappregistry-attribute-group.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "22.0.0",
"version": "30.1.0",
"files": {
"3dece22dad73361a79cb380f2880362a20ffc5c0cc75ddc6707e26b5a88cf93f": {
"9d37fdefa4311937f8f73f9556f1d9a03a2874545a0a262fd42bfde3823ab551": {
"source": {
"path": "integ-servicecatalogappregistry-attribute-group.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "3dece22dad73361a79cb380f2880362a20ffc5c0cc75ddc6707e26b5a88cf93f.json",
"objectKey": "9d37fdefa4311937f8f73f9556f1d9a03a2874545a0a262fd42bfde3823ab551.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...attribute-group.js.snapshot/integ-servicecatalogappregistry-attribute-group.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
"beta": "time2"
}
},
"Name": "myAttributeGroupTest",
"Description": "my attribute group description"
"Name": "myFirstAttributeGroup",
"Description": "test attribute group description"
}
},
"TestAttributeGroupRAMSharec67f7d80e5baA10EFB4E": {
Expand Down
2 changes: 1 addition & 1 deletion .../@aws-cdk/aws-servicecatalogappregistry/test/integ.attribute-group.js.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "22.0.0",
"version": "30.1.0",
"testCases": {
"integ.attribute-group": {
"stacks": [
Expand Down
4 changes: 2 additions & 2 deletions ...ws-cdk/aws-servicecatalogappregistry/test/integ.attribute-group.js.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "22.0.0",
"version": "30.1.0",
"artifacts": {
"integ-servicecatalogappregistry-attribute-group.assets": {
"type": "cdk:asset-manifest",
Expand All @@ -17,7 +17,7 @@
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/3dece22dad73361a79cb380f2880362a20ffc5c0cc75ddc6707e26b5a88cf93f.json",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/9d37fdefa4311937f8f73f9556f1d9a03a2874545a0a262fd42bfde3823ab551.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
Expand Down
Loading