Merge branch 'master' into chore/iot-actions-ownership

packages/@aws-cdk/aws-ec2/lib/security-group.ts

@@ -68,6 +68,8 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {
   public readonly connections: Connections = new Connections({ securityGroups: [this] });
   public readonly defaultPort?: Port;
 
+  private peerAsTokenCount: number = 0;
+
   constructor(scope: Construct, id: string, props?: ResourceProps) {
     super(scope, id, props);
 
@@ -83,7 +85,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {
       description = `from ${peer.uniqueId}:${connection}`;
     }
 
-    const [scope, id] = determineRuleScope(this, peer, connection, 'from', remoteRule);
+    const [scope, id] = this.determineRuleScope(peer, connection, 'from', remoteRule);
 
     // Skip duplicates
     if (scope.node.tryFindChild(id) === undefined) {
@@ -101,7 +103,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {
       description = `to ${peer.uniqueId}:${connection}`;
     }
 
-    const [scope, id] = determineRuleScope(this, peer, connection, 'to', remoteRule);
+    const [scope, id] = this.determineRuleScope(peer, connection, 'to', remoteRule);
 
     // Skip duplicates
     if (scope.node.tryFindChild(id) === undefined) {
@@ -121,75 +123,82 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {
   public toEgressRuleConfig(): any {
     return { destinationSecurityGroupId: this.securityGroupId };
   }
-}
 
-/**
- * Determine where to parent a new ingress/egress rule
- *
- * A SecurityGroup rule is parented under the group it's related to, UNLESS
- * we're in a cross-stack scenario with another Security Group. In that case,
- * we respect the 'remoteRule' flag and will parent under the other security
- * group.
- *
- * This is necessary to avoid cyclic dependencies between stacks, since both
- * ingress and egress rules will reference both security groups, and a naive
- * parenting will lead to the following situation:
- *
- *   ╔════════════════════╗         ╔════════════════════╗
- *   ║  ┌───────────┐     ║         ║    ┌───────────┐   ║
- *   ║  │  GroupA   │◀────╬─┐   ┌───╬───▶│  GroupB   │   ║
- *   ║  └───────────┘     ║ │   │   ║    └───────────┘   ║
- *   ║        ▲           ║ │   │   ║          ▲         ║
- *   ║        │           ║ │   │   ║          │         ║
- *   ║        │           ║ │   │   ║          │         ║
- *   ║  ┌───────────┐     ║ └───┼───╬────┌───────────┐   ║
- *   ║  │  EgressA  │─────╬─────┘   ║    │ IngressB  │   ║
- *   ║  └───────────┘     ║         ║    └───────────┘   ║
- *   ║                    ║         ║                    ║
- *   ╚════════════════════╝         ╚════════════════════╝
- *
- * By having the ability to switch the parent, we avoid the cyclic reference by
- * keeping all rules in a single stack.
- *
- * If this happens, we also have to change the construct ID, because
- * otherwise we might have two objects with the same ID if we have
- * multiple reversed security group relationships.
- *
- *   ╔═══════════════════════════════════╗
- *   ║┌───────────┐                      ║
- *   ║│  GroupB   │                      ║
- *   ║└───────────┘                      ║
- *   ║      ▲                            ║
- *   ║      │              ┌───────────┐ ║
- *   ║      ├────"from A"──│ IngressB  │ ║
- *   ║      │              └───────────┘ ║
- *   ║      │              ┌───────────┐ ║
- *   ║      ├─────"to B"───│  EgressA  │ ║
- *   ║      │              └───────────┘ ║
- *   ║      │              ┌───────────┐ ║
- *   ║      └─────"to B"───│  EgressC  │ ║  <-- oops
- *   ║                     └───────────┘ ║
- *   ╚═══════════════════════════════════╝
- */
-function determineRuleScope(
-  group: SecurityGroupBase,
-  peer: IPeer,
-  connection: Port,
-  fromTo: 'from' | 'to',
-  remoteRule?: boolean): [SecurityGroupBase, string] {
-
-  if (remoteRule && SecurityGroupBase.isSecurityGroup(peer) && differentStacks(group, peer)) {
-    // Reversed
-    const reversedFromTo = fromTo === 'from' ? 'to' : 'from';
-    return [peer, `${group.uniqueId}:${connection} ${reversedFromTo}`];
-  } else {
-    // Regular (do old ID escaping to in order to not disturb existing deployments)
-    return [group, `${fromTo} ${renderPeer(peer)}:${connection}`.replace('/', '_')];
+  /**
+   * Determine where to parent a new ingress/egress rule
+   *
+   * A SecurityGroup rule is parented under the group it's related to, UNLESS
+   * we're in a cross-stack scenario with another Security Group. In that case,
+   * we respect the 'remoteRule' flag and will parent under the other security
+   * group.
+   *
+   * This is necessary to avoid cyclic dependencies between stacks, since both
+   * ingress and egress rules will reference both security groups, and a naive
+   * parenting will lead to the following situation:
+   *
+   *   ╔════════════════════╗         ╔════════════════════╗
+   *   ║  ┌───────────┐     ║         ║    ┌───────────┐   ║
+   *   ║  │  GroupA   │◀────╬─┐   ┌───╬───▶│  GroupB   │   ║
+   *   ║  └───────────┘     ║ │   │   ║    └───────────┘   ║
+   *   ║        ▲           ║ │   │   ║          ▲         ║
+   *   ║        │           ║ │   │   ║          │         ║
+   *   ║        │           ║ │   │   ║          │         ║
+   *   ║  ┌───────────┐     ║ └───┼───╬────┌───────────┐   ║
+   *   ║  │  EgressA  │─────╬─────┘   ║    │ IngressB  │   ║
+   *   ║  └───────────┘     ║         ║    └───────────┘   ║
+   *   ║                    ║         ║                    ║
+   *   ╚════════════════════╝         ╚════════════════════╝
+   *
+   * By having the ability to switch the parent, we avoid the cyclic reference by
+   * keeping all rules in a single stack.
+   *
+   * If this happens, we also have to change the construct ID, because
+   * otherwise we might have two objects with the same ID if we have
+   * multiple reversed security group relationships.
+   *
+   *   ╔═══════════════════════════════════╗
+   *   ║┌───────────┐                      ║
+   *   ║│  GroupB   │                      ║
+   *   ║└───────────┘                      ║
+   *   ║      ▲                            ║
+   *   ║      │              ┌───────────┐ ║
+   *   ║      ├────"from A"──│ IngressB  │ ║
+   *   ║      │              └───────────┘ ║
+   *   ║      │              ┌───────────┐ ║
+   *   ║      ├─────"to B"───│  EgressA  │ ║
+   *   ║      │              └───────────┘ ║
+   *   ║      │              ┌───────────┐ ║
+   *   ║      └─────"to B"───│  EgressC  │ ║  <-- oops
+   *   ║                     └───────────┘ ║
+   *   ╚═══════════════════════════════════╝
+   */
+
+  protected determineRuleScope(
+    peer: IPeer,
+    connection: Port,
+    fromTo: 'from' | 'to',
+    remoteRule?: boolean): [SecurityGroupBase, string] {
+
+    if (remoteRule && SecurityGroupBase.isSecurityGroup(peer) && differentStacks(this, peer)) {
+      // Reversed
+      const reversedFromTo = fromTo === 'from' ? 'to' : 'from';
+      return [peer, `${this.uniqueId}:${connection} ${reversedFromTo}`];
+    } else {
+      // Regular (do old ID escaping to in order to not disturb existing deployments)
+      return [this, `${fromTo} ${this.renderPeer(peer)}:${connection}`.replace('/', '_')];
+    }
   }
-}
 
-function renderPeer(peer: IPeer) {
-  return Token.isUnresolved(peer.uniqueId) ? '{IndirectPeer}' : peer.uniqueId;
+  private renderPeer(peer: IPeer) {
+    if (Token.isUnresolved(peer.uniqueId)) {
+      // Need to return a unique value each time a peer
+      // is an unresolved token, else the duplicate skipper
+      // in `sg.addXxxRule` can detect unique rules as duplicates
+      return this.peerAsTokenCount++ ? `'{IndirectPeer${this.peerAsTokenCount}}'` : '{IndirectPeer}';
+    } else {
+      return peer.uniqueId;
+    }
+  }
 }
 
 function differentStacks(group1: SecurityGroupBase, group2: SecurityGroupBase) {
@@ -565,13 +574,12 @@ export class SecurityGroup extends SecurityGroupBase {
    */
   private removeNoTrafficRule() {
     if (this.disableInlineRules) {
-      const [scope, id] = determineRuleScope(
-        this,
+      const [scope, id] = this.determineRuleScope(
         NO_TRAFFIC_PEER,
         NO_TRAFFIC_PORT,
         'to',
-        false);
-
+        false,
+      );
       scope.node.tryRemoveChild(id);
     } else {
       const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, MATCH_NO_TRAFFIC));

packages/@aws-cdk/aws-ec2/test/security-group.test.ts

@@ -208,6 +208,30 @@ describe('security group', () => {
 
   });
 
+  test('can add multiple rules using tokens on same security group', () => {
+    // GIVEN
+    const stack = new Stack(undefined, 'TestStack', { env: { account: '12345678', region: 'dummy' } });
+    const vpc = new Vpc(stack, 'VPC');
+    const sg = new SecurityGroup(stack, 'SG', { vpc });
+
+    const p1 = Lazy.string({ produce: () => 'dummyid1' });
+    const p2 = Lazy.string({ produce: () => 'dummyid2' });
+    const peer1 = Peer.prefixList(p1);
+    const peer2 = Peer.prefixList(p2);
+
+    // WHEN
+    sg.addIngressRule(peer1, Port.tcp(5432), 'Rule 1');
+    sg.addIngressRule(peer2, Port.tcp(5432), 'Rule 2');
+
+    // THEN -- no crash
+    expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', {
+      Description: 'Rule 1',
+    });
+    expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', {
+      Description: 'Rule 2',
+    });
+  });
+
   test('if tokens are used in ports, `canInlineRule` should be false to avoid cycles', () => {
     // GIVEN
     const p1 = Lazy.number({ produce: () => 80 });

packages/@aws-cdk/aws-servicecatalog/README.md

@@ -30,6 +30,8 @@ enables organizations to create and manage catalogs of products for their end us
   - [Granting access to a portfolio](#granting-access-to-a-portfolio)
   - [Sharing a portfolio with another AWS account](#sharing-a-portfolio-with-another-aws-account)
 - [Product](#product)
+  - [Creating a product from a local asset](#creating-a-product-from-local-asset)
+  - [Creating a product from a stack](#creating-a-product-from-a-stack)
   - [Adding a product to a portfolio](#adding-a-product-to-a-portfolio)
 - [TagOptions](#tag-options)
 - [Constraints](#constraints)
@@ -125,10 +127,12 @@ const product = new servicecatalog.CloudFormationProduct(this, 'MyFirstProduct',
       cloudFormationTemplate: servicecatalog.CloudFormationTemplate.fromUrl(
         'https://raw.githubusercontent.com/awslabs/aws-cloudformation-templates/master/aws/services/ServiceCatalog/Product.yaml'),
     },
-  ]
+  ],
 });
 ```
 
+### Creating a product from a local asset
+
 A `CloudFormationProduct` can also be created using a Cloudformation template from an Asset.
 Assets are files that are uploaded to an S3 Bucket before deployment.
 `CloudFormationTemplate.fromAsset` can be utilized to create a Product by passing the path to a local template file on your disk:
@@ -149,7 +153,38 @@ const product = new servicecatalog.CloudFormationProduct(this, 'MyFirstProduct',
       productVersionName: "v2",
       cloudFormationTemplate: servicecatalog.CloudFormationTemplate.fromAsset(path.join(__dirname, 'development-environment.template.json')),
     },
-  ]
+  ],
+});
+```
+
+### Creating a product from a stack
+
+You can define a service catalog `CloudFormationProduct` entirely within CDK using a service catalog `ProductStack`.
+A separate child stack for your product is created and you can add resources like you would for any other CDK stack,
+such as an S3 Bucket, IAM roles, and EC2 instances. This stack is passed in as a product version to your
+product.  This will not create a separate stack during deployment. 
+
+```ts
+import * as s3 from '@aws-cdk/aws-s3';
+import * as cdk from '@aws-cdk/core';
+
+class S3BucketProduct extends servicecatalog.ProductStack {
+  constructor(scope: cdk.Construct, id: string) {
+    super(scope, id);
+
+    new s3.Bucket(this, 'BucketProduct');
+  }
+}
+
+const product = new servicecatalog.CloudFormationProduct(this, 'MyFirstProduct', {
+  productName: "My Product",
+  owner: "Product Owner",
+  productVersions: [
+    {
+      productVersionName: "v1",
+      cloudFormationTemplate: servicecatalog.CloudFormationTemplate.fromProductStack(new S3BucketProduct(this, 'S3BucketProduct')),
+    },
+  ],
 });
 ```
 

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

@@ -1,5 +1,6 @@
 import * as s3_assets from '@aws-cdk/aws-s3-assets';
 import { hashValues } from './private/util';
+import { ProductStack } from './product-stack';
 
 // keep this import separate from other imports to reduce chance for merge conflicts with v2-main
 // eslint-disable-next-line no-duplicate-imports, import/order
@@ -26,6 +27,13 @@ export abstract class CloudFormationTemplate {
     return new CloudFormationAssetTemplate(path, options);
   }
 
+  /**
+   * Creates a product with the resources defined in the given product stack.
+   */
+  public static fromProductStack(productStack: ProductStack): CloudFormationTemplate {
+    return new CloudFormationProductStackTemplate(productStack);
+  }
+
   /**
    * Called when the product is initialized to allow this object to bind
    * to the stack, add resources and have fun.
@@ -88,3 +96,21 @@ class CloudFormationAssetTemplate extends CloudFormationTemplate {
     };
   }
 }
+
+/**
+ * Template from a CDK defined product stack.
+ */
+class CloudFormationProductStackTemplate extends CloudFormationTemplate {
+  /**
+   * @param stack A service catalog product stack.
+  */
+  constructor(public readonly productStack: ProductStack) {
+    super();
+  }
+
+  public bind(_scope: Construct): CloudFormationTemplateConfig {
+    return {
+      httpUrl: this.productStack._getTemplateUrl(),
+    };
+  }
+}

packages/@aws-cdk/aws-servicecatalog/lib/index.ts

@@ -3,6 +3,7 @@ export * from './constraints';
 export * from './cloudformation-template';
 export * from './portfolio';
 export * from './product';
+export * from './product-stack';
 export * from './tag-options';
 
 // AWS::ServiceCatalog CloudFormation Resources:

packages/@aws-cdk/aws-servicecatalog/lib/private/product-stack-synthesizer.ts

@@ -0,0 +1,34 @@
+import * as cdk from '@aws-cdk/core';
+
+/**
+ * Deployment environment for an AWS Service Catalog product stack.
+ *
+ * Interoperates with the StackSynthesizer of the parent stack.
+ */
+export class ProductStackSynthesizer extends cdk.StackSynthesizer {
+  private stack?: cdk.Stack;
+
+  public bind(stack: cdk.Stack): void {
+    if (this.stack !== undefined) {
+      throw new Error('A Stack Synthesizer can only be bound once, create a new instance to use with a different Stack');
+    }
+    this.stack = stack;
+  }
+
+  public addFileAsset(_asset: cdk.FileAssetSource): cdk.FileAssetLocation {
+    throw new Error('Service Catalog Product Stacks cannot use Assets');
+  }
+
+  public addDockerImageAsset(_asset: cdk.DockerImageAssetSource): cdk.DockerImageAssetLocation {
+    throw new Error('Service Catalog Product Stacks cannot use Assets');
+  }
+
+  public synthesize(session: cdk.ISynthesisSession): void {
+    if (!this.stack) {
+      throw new Error('You must call bindStack() first');
+    }
+    // Synthesize the template, but don't emit as a cloud assembly artifact.
+    // It will be registered as an S3 asset of its parent instead.
+    this.synthesizeStackTemplate(this.stack, session);
+  }
+}