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): add sharing of applications and attribute groups #20850

Merged
merged 29 commits into from
Aug 22, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ec8709a
example RAM share start for appreg
May 6, 2022
4e5fcf4
update spacing
May 6, 2022
b2ad0eb
Add unit tests, fix typos
Jun 18, 2022
17438c5
Documentation additions for sharing
Jun 22, 2022
ea7b52d
Added integ tests for sharing
Jun 23, 2022
a986534
Fix newlines and documentation
Jun 23, 2022
c785e58
Revised hashing strategy for resource shares, updated tests, revised …
Jun 28, 2022
9300e03
Merge branch 'main' into appreg_ram_share
mackalex Jun 28, 2022
14c7f81
AWS RAM constructs to stable in order to fix dependency issue
Jun 29, 2022
3120c49
Upgrade constructs dependency
Jun 29, 2022
73d0041
Merge branch 'main' into appreg_ram_share
mackalex Jun 29, 2022
ecb00ec
Revised docstring, using Names methods for naming
Jul 1, 2022
904f751
Merge branch 'main' into appreg_ram_share
mackalex Jul 15, 2022
b63da2e
Merge branch 'main' into appreg_ram_share
mackalex Jul 19, 2022
8f951ee
Merge branch 'main' into appreg_ram_share
mackalex Jul 28, 2022
1d62528
Add share permission option and additional tests
Jul 23, 2022
c4358c5
Remove option to allow external principals, should always be false fo…
Jul 23, 2022
db7e438
Rename organizations to organizationArns, update README
Jul 27, 2022
e3e4599
Change shareResource to shareApplication and shareAttributeGroup resp…
Jul 28, 2022
5ce92cd
Allow for multiple shares per Application or AttributeGroup
Jul 30, 2022
6a32574
Revert RAM constructs to experimental per (#21208)
Jul 30, 2022
bebfc13
Added extra blank line above each @param
Jul 30, 2022
6d0fcb2
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/attribute-…
mackalex Aug 3, 2022
2c8034c
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/applicatio…
mackalex Aug 3, 2022
80b7b5c
Merge branch 'main' into appreg_ram_share
mackalex Aug 3, 2022
a422beb
Merge branch 'main' into appreg_ram_share
mackalex Aug 5, 2022
34bbfdb
Update table of contents in README
Aug 6, 2022
0570d3c
Merge branch 'main' into appreg_ram_share
mackalex Aug 10, 2022
19a87dc
Merge branch 'main' into appreg_ram_share
mergify[bot] Aug 22, 2022
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ram/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
"engines": {
"node": ">= 14.15.0"
},
"stability": "experimental",
"stability": "stable",
mackalex marked this conversation as resolved.
Show resolved Hide resolved
"maturity": "cfn-only",
"awscdkio": {
"announce": false
Expand Down
54 changes: 54 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,57 @@ const myStack = new Stack(app, 'MyStack');
declare const application: appreg.Application;
application.associateStack(myStack);
```

## Sharing

You can share your AppRegistry applications and attribute groups with AWS Organizations, Organizational Units (OUs), AWS accounts within an organization, as well as IAM roles and users. AppRegistry requires that AWS Organizations is enabled in an account before deploying a share of an application or attribute group.

### Sharing an application

```ts
import * as iam from '@aws-cdk/aws-iam';
declare const application: appreg.Application;
declare const myRole: iam.IRole;
declare const myUser: iam.IUser;
application.shareResource({
mackalex marked this conversation as resolved.
Show resolved Hide resolved
accounts: ['123456789012'],
organizations: ['arn:aws:organizations::123456789012:organization/o-my-org-id'],
mackalex marked this conversation as resolved.
Show resolved Hide resolved
roles: [myRole],
users: [myUser]
});
```

E.g., sharing an application with multiple accounts:

```ts
import * as iam from '@aws-cdk/aws-iam';
declare const application: appreg.Application;
application.shareResource({
accounts: ['123456789012', '234567890123'],
});
```

### Sharing an attribute group
mackalex marked this conversation as resolved.
Show resolved Hide resolved

```ts
import * as iam from '@aws-cdk/aws-iam';
mackalex marked this conversation as resolved.
Show resolved Hide resolved
declare const attributeGroup: appreg.AttributeGroup;
declare const myRole: iam.IRole;
declare const myUser: iam.IUser;
attributeGroup.shareResource({
accounts: ['123456789012'],
organizations: ['arn:aws:organizations::123456789012:organization/o-my-org-id'],
roles: [myRole],
users: [myUser]
});
```

E.g., sharing an attribute group with multiple accounts:

```ts
import * as iam from '@aws-cdk/aws-iam';
declare const attributeGroup: appreg.AttributeGroup;
attributeGroup.shareResource({
accounts: ['123456789012', '234567890123'],
});
```
35 changes: 25 additions & 10 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import * as crypto from 'crypto';
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 { IAttributeGroup } from './attribute-group';
import { getPrincipalsforSharing, hashValues, ShareOptions } from './common';
import { InputValidator } from './private/validation';
import { CfnApplication, CfnAttributeGroupAssociation, CfnResourceAssociation } from './servicecatalogappregistry.generated';

Expand Down Expand Up @@ -32,6 +34,12 @@ export interface IApplication extends cdk.IResource {
* @param stack a CFN stack
*/
associateStack(stack: cdk.Stack): void;

/**
* Share this resource with other IAM entities, accounts, or OUs.
* @param shareOptions The options for the share.
*/
shareResource(shareOptions: ShareOptions): void;
}

/**
Expand Down Expand Up @@ -88,6 +96,22 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
}
}

/**
* Share application resource with target accounts.
* The application will become available to end users within targets.
* @param shareOptions The options for the share.
mackalex marked this conversation as resolved.
Show resolved Hide resolved
*/
public shareResource(shareOptions: ShareOptions): void {
const principals = getPrincipalsforSharing(shareOptions);
const shareName = `RAMShare${Names.uniqueResourceName(this, {})}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

The CFN docs don't specify a limit to this name's length, so it's probably fine to ignore the limit parameter on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it only needs to be unique in the scope of this.

But, it will not be unique if shareResource() is called more than once. Is that a supported operation? If not, I'd prefer an explicit check and error--otherwise there will be a bug report saying «if I call shareResource() twice I get a "construct already exists" error», and we would not have gotten that bug report if the error message was "you can only call shareResource() once"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gathered more information on this and dove into the AppReg console experience for sharing. AppReg applications and attribute groups can have more than one "Share" per the console. I committed a new resource hashing strategy which aims to allow this behavior. I'm curious if there's a more optimal strategy than what I've committed.

new CfnResourceShare(this, shareName, {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider since we have hash for resource name we can either throw an error if someone tries to add same share twice or ignore it, like we do in other SC resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will think about this one a bit more. I think some logic to ignore it may be sufficient.

name: shareName,
allowExternalPrincipals: shareOptions.allowExternalPrincipals ?? true,
principals: principals,
resourceArns: [this.applicationArn],
});
}

/**
* Create a unique id
*/
Expand Down Expand Up @@ -156,12 +180,3 @@ export class Application extends ApplicationBase {
InputValidator.validateLength(this.node.path, 'application description', 0, 1024, props.description);
}
}

/**
* Generates a unique hash identfifer using SHA256 encryption algorithm
*/
function hashValues(...values: string[]): string {
const sha256 = crypto.createHash('sha256');
values.forEach(val => sha256.update(val));
return sha256.digest('hex').slice(0, 12);
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { CfnResourceShare } from '@aws-cdk/aws-ram';
import * as cdk from '@aws-cdk/core';
import { getPrincipalsforSharing, ShareOptions } from './common';
import { Construct } from 'constructs';
import { InputValidator } from './private/validation';
import { CfnAttributeGroup } from './servicecatalogappregistry.generated';
import { Names } from '@aws-cdk/core';

/**
* A Service Catalog AppRegistry Attribute Group.
Expand All @@ -18,6 +21,12 @@ export interface IAttributeGroup extends cdk.IResource {
* @attribute
*/
readonly attributeGroupId: string;

/**
* Share the attribute group resource with other IAM entities, accounts, or OUs.
* @param shareOptions The options for the share.
*/
shareResource(shareOptions: ShareOptions): void;
}

/**
Expand Down Expand Up @@ -45,6 +54,17 @@ export interface AttributeGroupProps {
abstract class AttributeGroupBase extends cdk.Resource implements IAttributeGroup {
public abstract readonly attributeGroupArn: string;
public abstract readonly attributeGroupId: string;

public shareResource(shareOptions: ShareOptions): void {
const principals = getPrincipalsforSharing(shareOptions);
const shareName = `RAMShare${Names.uniqueResourceName(this, {})}`;
mackalex marked this conversation as resolved.
Show resolved Hide resolved
new CfnResourceShare(this, shareName, {
name: shareName,
allowExternalPrincipals: shareOptions.allowExternalPrincipals ?? true,
principals: principals,
resourceArns: [this.attributeGroupArn],
});
}
}

/**
Expand Down
73 changes: 73 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/common.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import * as crypto from 'crypto';
import * as iam from '@aws-cdk/aws-iam';

/**
* The options that are passed into a share of an Application or Attribute Group.
*/
export interface ShareOptions {
/**
* When set to true, this allows sharing of applications and attribute groups
* with accounts outside of your AWS Organization. When set to false, sharing
* is restricted to only accounts and principals which belong to the organization.
*
* @default true
*/
readonly allowExternalPrincipals?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this should be true be default...pinging @rix0rrr for advice

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on what this flag does.

  • As currently stated, it automatically shares your app with everyone. That doesn't seem great... what's the thought behind that @arcrank ?
  • If the actual behavior is, this turns on the possibility of calling some other method later which shares, then it's fine, but also the documentation is completely failing to explain that.

Also, the docstring says "Explicitly" but proceeds to turn on the flag implicitly, which makes that word pretty redundant.

I'd suggest rewriting the docstring to be clear and specific and actionable.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is default behavior, all it means is that you can share with accounts not in your AWS Org. Since this is really AWS Org specific (this flag is meaningless if the caller account itself is not in an aws org) maybe we can rename it align with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised the docstring to be more clear on this. +1 to @arcrank on this. The default behavior is explained further in step 8 of RAM's resource sharing getting started guide: https://docs.aws.amazon.com/ram/latest/userguide/getting-started-sharing.html

Copy link
Contributor

Choose a reason for hiding this comment

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

I would further explain the behavior in these docs to address @rix0rrr's second point; is there a method that must be called to actually share this resource? This docstring should tell the user not only that this sharing can happen, but how it happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the behavior relates to the organization, why don't we call it allowPrincipalsOutsideOrganization ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the discussion on this. After conferring with the team, this property has been removed from ShareOptions as it is always set to false in AppRegistry's sharing experience with RAM and not surfaced to the customer.


/**
* A list of AWS accounts that the application will be shared with.
*
* @default - No accounts specified for share
*/
readonly accounts?: string[];

/**
* A list of AWS Organization or Organizational Units (OUs) ARNs that the application will be shared with.
*
* @default - No AWS Organizations or OUs specified for share
*/
readonly organizations?: string[];

/**
* A list of AWS IAM roles that the application will be shared with.
*
* @default - No IAM roles specified for share
*/
readonly roles?: iam.IRole[];

/**
* A list of AWS IAM users that the application will be shared with.
*
* @default - No IAM Users specified for share
*/
readonly users?: iam.IUser[];
}

/**
* Generates a unique hash identfifer using SHA256 encryption algorithm.
*/
export function hashValues(...values: string[]): string {
const sha256 = crypto.createHash('sha256');
values.forEach(val => sha256.update(val));
return sha256.digest('hex').slice(0, 12);
}

/**
* Reformats share targets into a collapsed list necessary for handler.
* @param options The share target options
* @returns flat list of target ARNs
*/
export function getPrincipalsforSharing(options: ShareOptions): string[] {
const principals = [
...options.accounts ?? [],
...options.organizations ?? [],
...options.users ? options.users.map(user => user.userArn) : [],
...options.roles ? options.roles.map(role => role.roleArn) : [],
];

if (principals.length == 0) {
throw new Error('An entity must be provided for the share');
}

return principals;
mackalex marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export * from './application';
export * from './attribute-group';
export * from './common';

// AWS::ServiceCatalogAppRegistry CloudFormation Resources:
export * from './servicecatalogappregistry.generated';
4 changes: 4 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,14 @@
},
"dependencies": {
"@aws-cdk/core": "0.0.0",
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-ram": "0.0.0",
"constructs": "^10.0.0"
},
"peerDependencies": {
"@aws-cdk/core": "0.0.0",
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-ram": "0.0.0",
"constructs": "^10.0.0"
},
"engines": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"17.0.0"}
{"version":"20.0.0"}
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "17.0.0",
"version": "20.0.0",
"files": {
"d561cf6d9aa2d98689712d70accb1c3f56f2a54d6cbb1268d35bd72e05675791": {
"d38cefd07b083dfb1814889c0bbb741d1a53333b08007b245296279d3e3510a2": {
"source": {
"path": "integ-servicecatalogappregistry-application.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "d561cf6d9aa2d98689712d70accb1c3f56f2a54d6cbb1268d35bd72e05675791.json",
"objectKey": "d38cefd07b083dfb1814889c0bbb741d1a53333b08007b245296279d3e3510a2.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,29 @@
}
}
},
"TestApplicationRAMShareintegservicecatalogappregistryapplicationTestApplicationF7821DB233482C38": {
"Type": "AWS::RAM::ResourceShare",
"Properties": {
"Name": "RAMShareintegservicecatalogappregistryapplicationTestApplicationF7821DB2",
"AllowExternalPrincipals": true,
"Principals": [
{
"Fn::GetAtt": [
"MyRoleF48FFE04",
"Arn"
]
}
],
"ResourceArns": [
{
"Fn::GetAtt": [
"TestApplication2FBC585F",
"Arn"
]
}
]
}
},
"TestAttributeGroupB1CB284F": {
"Type": "AWS::ServiceCatalogAppRegistry::AttributeGroup",
"Properties": {
Expand All @@ -61,6 +84,38 @@
"Name": "myAttributeGroupTest",
"Description": "my attribute group description"
}
},
"MyRoleF48FFE04": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::",
{
"Ref": "AWS::AccountId"
},
":root"
]
]
}
}
}
],
"Version": "2012-10-17"
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "17.0.0",
"version": "20.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
Expand Down Expand Up @@ -33,11 +33,32 @@
"data": "TestApplicationAttributeGroupAssociation4ba7f5842818B8EE1C6F"
}
],
"/integ-servicecatalogappregistry-application/TestApplication/RAMShareintegservicecatalogappregistryapplicationTestApplicationF7821DB2": [
{
"type": "aws:cdk:logicalId",
"data": "TestApplicationRAMShareintegservicecatalogappregistryapplicationTestApplicationF7821DB233482C38"
}
],
"/integ-servicecatalogappregistry-application/TestAttributeGroup/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "TestAttributeGroupB1CB284F"
}
],
"/integ-servicecatalogappregistry-application/MyRole/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "MyRoleF48FFE04"
}
],
"TestApplicationRAMShareb83647b4f06a5A88B554": [
{
"type": "aws:cdk:logicalId",
"data": "TestApplicationRAMShareb83647b4f06a5A88B554",
"trace": [
"!!DESTRUCTIVE_CHANGES: WILL_DESTROY"
]
}
]
},
"displayName": "integ-servicecatalogappregistry-application"
Expand Down
Loading