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.

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

feat(servicecatalogappregistry): application-associator L2 Construct #22024

Merged
merged 5 commits into from
Sep 30, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
58 changes: 32 additions & 26 deletions packages/@aws-cdk/aws-servicecatalogappregistry/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ enables organizations to create and manage repositores of applications and assoc
## Table Of Contents

- [Application](#application)
- [Automatic-Application](#automatic-application)
- [Register-Application](#register-application)
- [Attribute-Group](#attribute-group)
- [Associations](#associations)
- [Associating application with an attribute group](#attribute-group-association)
Expand Down Expand Up @@ -65,33 +65,39 @@ const importedApplication = appreg.Application.fromApplicationArn(
);
```

## Automatic-Application
## Register-Application

An AppRegistry L2 construct to automatically create an application with the given name and description.
The application name must be unique at the account level and it's immutable.
`AutomaticApplication` L2 construct will automatically associate all stacks in the given scope, however
in case of a `Pipeline` stack, stage underneath the pipeline will not automatically be associated and
needs to be associated separately.
If cross account stack is detected, then this construct will automatically share the application to consumer accounts.
Cross account feature will only work for non environment agnostic stacks.

Following will create an Application named `MyAutoApplication` in account `123456789012` and region `us-east-1`
and will associate all stacks in the `App` scope to `MyAutoApplication`.
If you want to create an Application named `MyRegisteredApplication` in account `123456789012` and region `us-east-1`
and want to associate all stacks in the `App` scope to `MyRegisteredApplication`, then use as shown in the example below:

```ts
const app = new App();
const autoApp = new appreg.AutomaticApplication(app, 'AutoApplication', {
applicationName: 'MyAutoApplication',
description: 'Testing auto application',
const registeredApp = new appreg.RegisterApplication(app, 'RegisterApplication', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I am sure the team thought deeply about a name of this class. Still, I think having a class name expressing an action instead of an object looks unintuitive. Perhaps that should be a factory method on the application class? Something like appreg.Application.registerStacks(app)?

Related topics in AWS Construct Library Design Guidelines:

  • Construct class - "The name of resource constructs must be identical to the name of the resource in the AWS API, which should be consistent with the resource name in the AWS CloudFormation spec"
  • Factories

P.S.: Another reference can be cdk-nag project.

applicationName: 'MyRegisteredApplication',
description: 'Testing registered application',
stackProps: {
stackName: 'MyAutoApplicationStack',
stackName: 'MyRegisteredApplicationStack',
env: {account: '123456789012', region: 'us-east-1'},
},
});
```

In case of a Pipeline stack, you need to pass the reference of `AutomaticApplication` to pipeline stack and associate
each stage as shown below:
If you want to re-use an existing Application with ARN: `arn:aws:servicecatalog:us-east-1:123456789012:/applications/applicationId`
and want to associate all stacks in the `App` scope to your imported application, then use as shown in the example below:

```ts
const app = new App();
const registeredApp = new appreg.RegisterApplication(app, 'RegisterApplication', {
applicationArnValue: 'arn:aws:servicecatalog:us-east-1:123456789012:/applications/applicationId',
stackProps: {
stackName: 'MyRegisteredApplicationStack',
},
});
```

If you are using CDK Pipelines to deploy your application, the application stacks will be inside Stages, and
RegisterApplication 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:

```ts
import * as cdk from "@aws-cdk/core";
Expand All @@ -104,28 +110,28 @@ class ApplicationPipelineStack extends cdk.Stack {
constructor(scope: cdk.App, id: string, props: ApplicationPipelineStackProps) {
super(scope, id, props);

//associate the stage to automatic application.
props.application.associateStage(beta, this.stackName);
//associate the stage to register application.
props.application.associateStage(beta);
pipeline.addStage(beta);
}
};

interface ApplicationPipelineStackProps extends cdk.StackProps {
application: appreg.AutomaticApplication;
application: appreg.RegisterApplication;
};

const app = new App();
const autoApp = new appreg.AutomaticApplication(app, 'AutoApplication', {
applicationName: 'MyPipelineAutoApplication',
description: 'Testing pipeline auto app',
const registeredApp = new appreg.RegisterApplication(app, 'RegisterApplication', {
Copy link
Contributor

@rix0rrr rix0rrr Sep 28, 2022

Choose a reason for hiding this comment

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

I also don't particularly like this new name. It should be a noun.

ApplicationRegisterer then, if you want to use that word. Though I kinda don't like that either.

Copy link
Contributor

Choose a reason for hiding this comment

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

RegistryApplication ?

applicationName: 'MyPipelineRegisteredApplication',
description: 'Testing pipeline registered app',
stackProps: {
stackName: 'MyPipelineAutoApplicationStack',
stackName: 'MyPipelineRegisteredApplicationStack',
env: {account: '123456789012', region: 'us-east-1'},
},
});

const cdkPipeline = new ApplicationPipelineStack(app, 'CDKApplicationPipelineStack', {
application: autoApp,
application: registeredApp,
env: {account: '123456789012', region: 'us-east-1'},
});
```
Expand Down
22 changes: 17 additions & 5 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Construct } from 'constructs';
import { StageStackAssociator } from './aspects/stack-associator';
import { IAttributeGroup } from './attribute-group';
import { getPrincipalsforSharing, hashValues, ShareOptions, SharePermission } from './common';
import { isAccountUnresolved } from './private/utils';
import { InputValidator } from './private/validation';
import { CfnApplication, CfnAttributeGroupAssociation, CfnResourceAssociation } from './servicecatalogappregistry.generated';

Expand Down Expand Up @@ -43,7 +44,7 @@ export interface IApplication extends cdk.IResource {
/**
* Associate this application with a CloudFormation stack.
*
* @deprecated Use `associateApplicationWithResource` instead.
* @deprecated Use `associateApplicationWithStack` instead.
* @param stack a CFN stack
*/
associateStack(stack: cdk.Stack): void;
Expand All @@ -53,7 +54,7 @@ export interface IApplication extends cdk.IResource {
*
* @param stack a CFN stack
*/
associateApplicationWithResource(stack: cdk.Stack): void;
associateApplicationWithStack(stack: cdk.Stack): void;

/**
* Share this application with other IAM entities, accounts, or OUs.
Expand Down Expand Up @@ -116,7 +117,7 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
* If the resource is already associated, it will ignore duplicate request.
* A stack can only be associated with one application.
*
* @deprecated Use `associateApplicationWithResource` instead.
* @deprecated Use `associateApplicationWithStack` instead.
*/
public associateStack(stack: cdk.Stack): void {
if (!this.associatedResources.has(stack.node.addr)) {
Expand All @@ -136,7 +137,7 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
* If the stack is already associated, it will ignore duplicate request.
* A stack can only be associated with one application.
*/
public associateApplicationWithResource(stack: cdk.Stack): void {
public associateApplicationWithStack(stack: cdk.Stack): void {
if (!this.associatedResources.has(stack.node.addr)) {
new CfnResourceAssociation(stack, 'AppRegistryAssociation', {
application: stack === cdk.Stack.of(this) ? this.applicationId : this.applicationName ?? this.applicationId,
Expand All @@ -145,7 +146,7 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
});

this.associatedResources.add(stack.node.addr);
if (stack !== cdk.Stack.of(this) && this.env.account === stack.account && !this.isStageScope(stack)) {
if (stack !== cdk.Stack.of(this) && this.isSameAccount(stack) && !this.isStageScope(stack)) {
stack.addDependency(cdk.Stack.of(this));
}
}
Expand Down Expand Up @@ -200,9 +201,20 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
}
}

/**
* Checks whether a stack is defined in a Stage or not.
*/
private isStageScope(stack : cdk.Stack): boolean {
return !(stack.node.scope instanceof cdk.App) && (stack.node.scope instanceof cdk.Stage);
}

/**
* Verifies if application and the visited node is deployed in different account.
*/
private isSameAccount(stack: cdk.Stack): boolean {
return isAccountUnresolved(this.env.account, stack.account) || this.env.account === stack.account;
}

}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { Pipeline } from '@aws-cdk/aws-codepipeline';
import { IAspect, Stack, Token, Annotations } from '@aws-cdk/core';
import { CodePipeline } from '@aws-cdk/pipelines';
import { IAspect, Stack, Stage, Annotations } from '@aws-cdk/core';
import { IConstruct } from 'constructs';
import { IApplication } from '../application';
import { AutomaticApplication } from '../automatic-application';
import { SharePermission } from '../common';
import { isRegionUnresolved, isAccountUnresolved } from '../private/utils';
import { RegisterApplication } from '../register-application';

/**
* Aspect class, this will visit each node from the provided construct once.
Expand All @@ -14,26 +13,21 @@ import { SharePermission } from '../common';
*/
abstract class StackAssociatorBase implements IAspect {
protected abstract readonly application: IApplication;
protected abstract readonly automaticApplication?: AutomaticApplication;
protected abstract readonly registerApplication?: RegisterApplication;

protected readonly sharedAccounts: Set<string> = new Set();
protected pipelineStages: Set<string> = new Set();

public visit(node: IConstruct): void {
if (node instanceof CodePipeline) {
this.pipelineStages = this.getPipelineStages(node);
}

// verify if a stage in a particular stack is associated to Application.
if (this.automaticApplication != undefined && node instanceof Pipeline) {
this.pipelineStages.forEach(( customStage ) => {
var stageAssociated = this.automaticApplication?.isStageAssociated(customStage, node.stack.stackName);
if (!stageAssociated) {
this.error(node, 'Associate Stage: ' + customStage + ' to ensure all stacks in your cdk app are associated with AppRegistry. '
+ 'You can use AutomaticApplication.associateStage to associate any stage.');
node.node.children.forEach((childNode) => {
if (Stage.isStage(childNode)) {
var stageAssociated = this.registerApplication?.isStageAssociated(childNode);
if (stageAssociated === false) {
this.error(childNode, 'Associate Stage: ' + childNode.stageName + ' to ensure all stacks in your cdk app are associated with AppRegistry. '
+ 'You can use RegisterApplication.associateStage to associate any stage.');
}
});
}
}
});

if (Stack.isStack(node)) {
this.handleCrossRegionStack(node);
Expand All @@ -48,7 +42,7 @@ abstract class StackAssociatorBase implements IAspect {
* @param node A Stage stack.
*/
private associate(node: Stack): void {
this.application.associateApplicationWithResource(node);
this.application.associateApplicationWithStack(node);
}

/**
Expand Down Expand Up @@ -80,7 +74,7 @@ abstract class StackAssociatorBase implements IAspect {
* @param node Cfn stack.
*/
private handleCrossRegionStack(node: Stack): void {
if (this.isRegionUnresolved(this.application.env.region, node.region)) {
if (isRegionUnresolved(this.application.env.region, node.region)) {
this.warning(node, 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
return;
}
Expand All @@ -99,7 +93,7 @@ abstract class StackAssociatorBase implements IAspect {
* @param node Cfn stack.
*/
private handleCrossAccountStack(node: Stack): void {
if (this.isAccountUnresolved(this.application.env.account!, node.account)) {
if (isAccountUnresolved(this.application.env.account!, node.account)) {
this.warning(node, 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
return;
}
Expand All @@ -113,58 +107,22 @@ abstract class StackAssociatorBase implements IAspect {
this.sharedAccounts.add(node.account);
}
}

/**
* Verifies if application or the visited node is region agnostic.
*
* @param applicationRegion Region of the application.
* @param nodeRegion Region of the visited node.
*/
private isRegionUnresolved(applicationRegion: string, nodeRegion: string): boolean {
return Token.isUnresolved(applicationRegion) || Token.isUnresolved(nodeRegion);
}

/**
* Verifies if application or the visited node is account agnostic.
*
* @param applicationAccount Account of the application.
* @param nodeAccount Account of the visited node.
*/
private isAccountUnresolved(applicationAccount: string, nodeAccount: string): boolean {
return Token.isUnresolved(applicationAccount) || Token.isUnresolved(nodeAccount);
}

/**
* Get custom defined stage names for the given pipeline.
*
* @param pipeline Code Pipeline.
*/
private getPipelineStages(pipeline: CodePipeline): Set<string> {
const stageNames = new Set<string>();
pipeline.waves.forEach( ( wave ) => {
wave.stages.forEach( ( stage ) => {
stageNames.add(stage.stageName);
});
});

return stageNames;
}
}

export class AutomaticApplicationStageStackAssociator extends StackAssociatorBase {
export class RegisterApplicationStageStackAssociator extends StackAssociatorBase {
protected readonly application: IApplication;
protected readonly automaticApplication?: AutomaticApplication;
protected readonly registerApplication?: RegisterApplication;

constructor(app: AutomaticApplication) {
constructor(app: RegisterApplication) {
super();
this.application = app.appRegistryApplication;
this.automaticApplication = app;
this.registerApplication = app;
}
}

export class StageStackAssociator extends StackAssociatorBase {
protected readonly application: IApplication;
protected readonly automaticApplication?: AutomaticApplication;
protected readonly registerApplication?: RegisterApplication;

constructor(app: IApplication) {
super();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { CfnResourceShare } from '@aws-cdk/aws-ram';
import * as cdk from '@aws-cdk/core';
import { getPrincipalsforSharing, hashValues, ShareOptions, SharePermission } from './common';
import { Names } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { getPrincipalsforSharing, hashValues, ShareOptions, SharePermission } from './common';
import { InputValidator } from './private/validation';
import { CfnAttributeGroup } from './servicecatalogappregistry.generated';
import { Names } from '@aws-cdk/core';

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
Loading