WIP feat(ecs): Enable Cloudmap Service Discovery #4

SoManyHs · 2019-02-05T07:55:11Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

rix0rrr · 2019-02-05T14:02:32Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

+   */
+  public addNamespace(name: string): cloudmap.PrivateDnsNamespace {
+    const sdNamespace = new cloudmap.PrivateDnsNamespace(this, 'DefaultServiceDiscoveryNamespace', {
+      name,


Is it possible to have this autogenerated?

Does it make sense to default it to some particular value that we can derive from another value?

Does it make sense to add a boolean to the constructor props to call this automatically?

Have name generated? No, that is usually the one thing customers would want to specify -- it's the domain name, so like foo.com

not sure what you mean by the boolean in the constructor props?

I was thinking:

class ClusterProps { // ... /** * Domain name for the private DNS namespace to create * * @default No namespace is created by default */ privateDnsNamespaceDomainName?: string }

Which would imply a call to addNamespace.

But looking at this, I do have a couple more questions:

Would you ever have more than one namespace? (Right now you can't call this more than once)

Would it always be a private namespace? (Right now doesn't look like you could do a public or HTTP namespace even if you wanted to)

Is cluster.addNamespace() descriptive enough? (Maybe cluster.addCloudMapNamespace()?).

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

rix0rrr · 2019-02-05T14:05:55Z

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

+  /**
+   * Enable CloudMap service discovery for the service
+   */
+  public enableServiceDiscovery(props: ServiceDiscoveryProps): cloudmap.Service {


Does use of this method require that the Cluster has addNamespace() called?

It does, but since addNamespace() adds the namespaceId (or namespace as a member) on the cluster we can check the value there, if you're concerned about validation.

I am, please do.

By the way, I think we're standardizing on FooOptions for arguments to a method (FooProps is for arguments to a constructor)

rix0rrr · 2019-02-05T14:10:20Z

packages/@aws-cdk/aws-servicediscovery/lib/private-dns-namespace.ts

+/**
+ * Define a Service Discovery HTTP Namespace
+ */
+export class PrivateDnsNamespace extends cdk.Construct {


Is there are more generic INamespace interface to be extracted between HttpNamespace and PrivateDnsNamespace, or does that not make sense?

rix0rrr · 2019-02-05T14:10:34Z

packages/@aws-cdk/aws-servicediscovery/lib/private-dns-namespace.ts

+    const ns = new CfnPrivateDnsNamespace(this, 'Resource', {
+      name: props.name,
+      description: props.description,
+      vpc: props.vpc.vpcId // NOTE: not sure this is right


I think it is.

rix0rrr · 2019-02-05T14:11:23Z

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

+  /**
+   * The ID of the namespace that you want to use for DNS configuration.
+   */
+  namespaceId: string;


Feels like you should want to pass in an INamespace here.

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

packages/@aws-cdk/aws-ecs/lib/cluster.ts

rix0rrr

My big question is: why do we register a namespace on a Cluster at all? Seems like supplying an INamespace to a Service should be enough, no?

rix0rrr · 2019-02-11T09:36:40Z

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

+  /**
+   * Enable CloudMap service discovery for the service
+   */
+  public enableServiceDiscovery(props: ServiceDiscoveryProps): cloudmap.Service {


I am, please do.

rix0rrr · 2019-02-11T09:37:50Z

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

+  /**
+   * Enable CloudMap service discovery for the service
+   */
+  public enableServiceDiscovery(props: ServiceDiscoveryProps): cloudmap.Service {


By the way, I think we're standardizing on FooOptions for arguments to a method (FooProps is for arguments to a constructor)

rix0rrr · 2019-02-11T09:38:58Z

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

+    }
+
+    const cloudmapService = new cloudmap.Service(this, 'CloudmapService', {
+      namespaceId: props.namespaceId, // FIXME use INamespace instead -- validate if cluster has called addNamespace set?


Why are you not taking the namespaceId from the cluster in any case? It's there, why make the user pass it in again? And in case there can be multiple namespaces why store it on the cluster at all?

Why does cluster.addNamespace() exist, anyway?

rix0rrr · 2019-02-11T09:42:41Z

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

+    // add Cloudmap service to the ECS Service's serviceRegistry
+    this.serviceRegistries.push({
+      registryArn: serviceArn,
+      containerName: this.taskDefinition.defaultContainer!.node.id,


Check for existence of a default container. It might not exist if addContainer() hasn't been called yet.

packages/@aws-cdk/aws-servicediscovery/lib/namespace.ts

rix0rrr · 2019-02-11T09:49:11Z

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

+   * The DNS type of the record that you want AWS Cloud Map to create. Supported record types
+   * include A, AAAA, A_AAAA, CNAME, and SRV.
+   */
+  dnsType: DnsRecordType;


What if namespace is of HTTP type? Does that still have a dnsType?

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

rix0rrr · 2019-02-11T09:50:49Z

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

+      {
+        dnsRecords: [
+          {
+            type: dnsType,


Validate that dnsType is set when you make it optioanl.

Wait, namespaceId (and routingPolicy) also both seem to not be set in case of an HTTP namespace.

How can this work?

rix0rrr · 2019-02-11T09:53:00Z

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

+
+export interface DnsConfig {
+  dnsRecords: DnsRecord[],
+    namespaceId: string,


Not really a nice interface to export. If it's a reference to some other resource, we don't reference it by string-id by by interface (INamespace here)

Hm, I was only using it to make it easier in the constructor (line 97), but perhaps exporting the interface this way isn't the way to go?

rix0rrr · 2019-02-11T09:53:23Z

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

+  /**
+   * The time to live for the record
+   */
+  ttl: string,


ttlSec. WHy is it a string and not a number?

fix public DNS namespace

SoManyHs force-pushed the master branch from 8a0d58b to 299fb6a Compare February 5, 2019 07:56

SoManyHs force-pushed the ecs-sd branch from 1b49318 to c764b63 Compare February 5, 2019 07:58

rix0rrr reviewed Feb 5, 2019

View reviewed changes

SoManyHs force-pushed the ecs-sd branch 4 times, most recently from 0238252 to 1a766b5 Compare February 11, 2019 09:04

rix0rrr reviewed Feb 11, 2019

View reviewed changes

packages/@aws-cdk/aws-ecs/lib/cluster.ts Outdated Show resolved Hide resolved

rix0rrr reviewed Feb 11, 2019

View reviewed changes

SoManyHs force-pushed the ecs-sd branch 6 times, most recently from f65e6ee to 3ac3e0d Compare February 19, 2019 06:51

SoManyHs mentioned this pull request Feb 20, 2019

feat(servicediscovery): AWS Cloud Map construct library aws/aws-cdk#1804

Merged

SoManyHs force-pushed the ecs-sd branch 2 times, most recently from a5ee563 to 314f75a Compare March 20, 2019 22:02

feat(ecs): integrate AWS Cloud Map (service discovery)

2e5d054

fix public DNS namespace

SoManyHs force-pushed the ecs-sd branch from 314f75a to 2e5d054 Compare March 21, 2019 08:33

SoManyHs closed this Mar 21, 2019

SoManyHs mentioned this pull request Mar 21, 2019

feat(ecs): integrate AWS Cloud Map (service discovery) aws/aws-cdk#2065

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP feat(ecs): Enable Cloudmap Service Discovery #4

WIP feat(ecs): Enable Cloudmap Service Discovery #4

SoManyHs commented Feb 5, 2019

rix0rrr Feb 5, 2019

SoManyHs Feb 6, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 5, 2019

SoManyHs Feb 6, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 5, 2019

rix0rrr Feb 5, 2019

rix0rrr Feb 5, 2019

rix0rrr left a comment

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

rix0rrr Feb 11, 2019

SoManyHs Feb 12, 2019

rix0rrr Feb 11, 2019

WIP feat(ecs): Enable Cloudmap Service Discovery #4

WIP feat(ecs): Enable Cloudmap Service Discovery #4

Conversation

SoManyHs commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment