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

SoManyHs · 2019-03-21T08:38:10Z

Fixes #1554

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

SoManyHs · 2019-03-21T08:38:34Z

Still need to add some fargate integ tests probably but just wanted to get this up for initial review.

rix0rrr · 2019-03-21T10:38:26Z

design/aws-ecs-service-discovery-integration.md

+
+## General approach
+Rather than having the customer instantiate Cloud Map constructs directly and
+thus be forced to learn the API of a separate construct library, we should


I think this is probably fine, but I would also expect to be able to pass in an INamespace if I know what I'm doing (same as we do for passing role).

Honestly, I wonder if this is not all a whole load of design pain for not a lot of gain.

If we make ecs.Service take a cloudmap.IService, are we not done?

Sure, you could do that but that would require having the customer have to know about a larger set of properties to create the service, and then also forcing them to learn which what fields from that resource need to be passed into the service as a ServiceRegistry.

As for passing in an entire INamespace, that is perhaps possible (and something I need to think about for public namespaces, since the ECS console doesn't create one for you the way it does a Private DNS namespace) but I was coming at it from the point of view that the only thing the customer cares about is a domain name (this was also validated by ECS engineers involved with cloudmap integration/service mesh).

rix0rrr · 2019-03-21T10:39:19Z

design/aws-ecs-service-discovery-integration.md

+ECS will automatically register service discovery instances that are accessible
+by IP address (IPv4 only) on the createService API call.
+
+// FIXME public namespace needs to be imported?


I'm not quite sure yet what the correct workflow is if the customer chooses to bring their own public DNS namespace here, since this is not something that would be owned by Route53.

rix0rrr · 2019-03-21T10:39:36Z

design/aws-ecs-service-discovery-integration.md

+Cloud Map service for each ECS service.
+
+The current proposal is to add a method on the ECS Cluster construct,
+`addNamespace` that would take a set of properties that include the namespace


Why addNamespace and not constructor parameters?

I suppose that is possible. I thought it might be worth making it explicit, since not everyone will want service discovery in their cluster but having an optional constructor parameter would achieve the same thing.

Referring to a previous comment, there is also still the possibility of adding more than one namespace to a cluster, although the assumption is that that would be unlikely. Not sure then if it would be better to keep a public addNamespace() method or move this to the constructor.

Maybe, but this declaration:

private _serviceDiscoveryNamespace?: cloudmap.INamespace;

Makes it impossible to have multiple namespaces anyway.

Which right now requires a check in addNamespace() by the way to ensure it's only set once.

Again, we avoid all this design-gnashing we don't know the right answers to by simplifying the API down to the bare minimum.

Right; I mention the more than one namespace issue here: https://github.com/awslabs/aws-cdk/pull/2065/files#diff-3d958921780fc2ee31330432c62f135eR20

You're right though that if we're not clear about the right answers we could avoid persisting the namespace at the cluster level (i.e. get rid off addNamespace) and pass INamespace to enableServiceDiscovery(); I do feel that the latter method does add more value however.

rix0rrr · 2019-03-21T10:40:14Z

design/aws-ecs-service-discovery-integration.md

+  /**
+   * The Amazon VPC that you want to associate the namespace with. Required for Private DNS namespaces
+   *
+   * @default VPC of the cluster for Private DNS Namespace, otherwise none


Does it make sense to ever have a VPC that's not the Cluster's VPC? I think not, right? Might as well leave it out then.

True -- technically a cluster can span more than one VPC but in the CDK we're strongly enforcing a 1:1 cluster:vpc relationship so that's fine.

rix0rrr · 2019-03-21T10:40:23Z

design/aws-ecs-service-discovery-integration.md

+  /**
+   * The domain name for the namespace, such as foo.com
+   */
+  name: string;


Call it domainName then?

I suppose name is a bit generic, though on the namespace it's namespaceName which is a bit stuttery.

It is, but in these APIs I'd rather be stuttery and explicit than ambiguous.

so namespaceName rather than domainName?

rix0rrr · 2019-03-21T10:42:30Z

design/aws-ecs-service-discovery-integration.md

+}
+```
+
+#### service#enableServiceDiscovery(options: ServiceDiscoveryOptions)


Why not have this as constructor parameters?

And I feel if you enable a namespace on the cluster, service discovery should be enabled by default, no?

I believe that not all services within a cluster will necessarily want to be hooked into Cloud Map, though would love input from @nathanpeck or @herrhound on this.

I think I leaned this way bc I liked thinking of service discovery options as its own bundle, and enabling it as a modular operation, but from the point of view of the CDK you're right in that it doesn't matter. Implementation of this is here and here so let me know if you think that all can just live in the constructor.

I was thinking the constructor would have something like this:

/** @default true */ registerInCloudMap?: boolean; cloudMapServiceOptions?: CloudMapServiceOptions;

Also we're going to have to make a decision on whether to call it "ServiceDiscovery" or "CloudMap". Right now we're doing both, which seems confusing.

Yes; it is already confusing in our docs since we mention service discovery but a lot of those pages redirect to AWS Cloud Map. Thoughts from @herrhound?

design/aws-ecs-service-discovery-integration.md

Fixes aws#1554

See 66dd228

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

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

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

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

rename set* to add* because Java move private method to after public method

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

Fixes aws#1554

See 66dd228

rename set* to add* because Java move private method to after public method

also moar tests

…/2065 AND ALSO add a test for exporting/importing

rix0rrr · 2019-04-03T08:08:50Z

Failed build was a NuGet bug: @aws-cdk/region-info: /usr/share/dotnet/sdk/2.1.402/NuGet.targets(498,5): error : Could not find a part of the path '/tmp/NuGetScratch/4e1e5ff8-80b8-4254-9097-14b892ffea35'. [/tmp/jsii-pacmak-codeUtVEsC/Amazon.CDK.RegionInfo/Amazon.CDK.RegionInfo.csproj]

eladb · 2019-04-03T08:14:36Z

Great job @SoManyHs!

SoManyHs requested a review from a team as a code owner March 21, 2019 08:38

SoManyHs force-pushed the master branch from 3586a25 to 8086085 Compare March 21, 2019 08:39

rix0rrr reviewed Mar 21, 2019

View reviewed changes

SoManyHs force-pushed the master branch 2 times, most recently from bcf1077 to dc2baf9 Compare March 26, 2019 21:27

RomainMuller assigned SoManyHs Mar 28, 2019

SoManyHs added 2 commits April 1, 2019 14:30

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

4a62ecf

Fixes aws#1554

update decdk snapshot

f9d7bee

SoManyHs force-pushed the master branch from dc2baf9 to f9d7bee Compare April 1, 2019 21:30

SoManyHs added 3 commits April 1, 2019 19:19

Make interface properties readonly

3e6a5a1

See 66dd228

Rename cluster#addNamepsace to addDefaultCloudMapNamespace

b5446d6

Put ServiceDiscoveryOptions in service constructor

596b258

rix0rrr reviewed Apr 2, 2019

View reviewed changes

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

rix0rrr reviewed Apr 2, 2019

View reviewed changes

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

rix0rrr reviewed Apr 2, 2019

View reviewed changes

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

rix0rrr reviewed Apr 2, 2019

View reviewed changes

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

Fix build errors

5ee0492

rename set* to add* because Java move private method to after public method

rix0rrr reviewed Apr 2, 2019

View reviewed changes

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

SoManyHs added 11 commits April 2, 2019 08:10

Check default namespace only added once

4697d8d

Refactor enableServiceDiscovery to base class

db01aca

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

31f0897

Fixes aws#1554

update decdk snapshot

2c2f200

Make interface properties readonly

ae3c23d

See 66dd228

Rename cluster#addNamepsace to addDefaultCloudMapNamespace

1f63711

Put ServiceDiscoveryOptions in service constructor

1f0ad5b

Fix build errors

b8e44e2

rename set* to add* because Java move private method to after public method

Check default namespace only added once

08e51e4

Refactor enableServiceDiscovery to base class

92a3072

Wrap NamespaceType enum

b73d2e9

also moar tests

SoManyHs force-pushed the master branch from f543bc6 to b73d2e9 Compare April 2, 2019 15:39

Rico Huijbers added 2 commits April 2, 2019 17:44

Add namespace export/import

0f182c5

Merge branch 'master' of github.com:SoManyHs/aws-cdk into pr/SoManyHs…

77f100a

…/2065 AND ALSO add a test for exporting/importing

rix0rrr approved these changes Apr 2, 2019

View reviewed changes

Rico Huijbers and others added 2 commits April 2, 2019 18:31

Define enum values locally

1d12ee6

Fix Namespace typecheck

77b13e3

rix0rrr merged commit 4864cc8 into aws:master Apr 3, 2019

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SoManyHs commented Mar 21, 2019 •

edited

Loading

SoManyHs commented Mar 21, 2019

rix0rrr Mar 21, 2019

rix0rrr Mar 21, 2019

SoManyHs Mar 21, 2019

rix0rrr Mar 21, 2019

SoManyHs Mar 21, 2019

rix0rrr Mar 21, 2019

SoManyHs Mar 21, 2019

SoManyHs Mar 21, 2019

rix0rrr Mar 22, 2019

SoManyHs Mar 22, 2019

rix0rrr Mar 21, 2019

SoManyHs Mar 21, 2019

rix0rrr Mar 21, 2019

SoManyHs Mar 21, 2019

rix0rrr Mar 22, 2019 •

edited

Loading

SoManyHs Mar 22, 2019

rix0rrr Mar 21, 2019

SoManyHs Mar 21, 2019

rix0rrr Mar 22, 2019

SoManyHs Mar 22, 2019

rix0rrr commented Apr 3, 2019

eladb commented Apr 3, 2019

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

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

Conversation

SoManyHs commented Mar 21, 2019 • edited Loading

Pull Request Checklist

SoManyHs commented Mar 21, 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

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 Mar 22, 2019 • edited Loading

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 commented Apr 3, 2019

eladb commented Apr 3, 2019

SoManyHs commented Mar 21, 2019 •

edited

Loading

rix0rrr Mar 22, 2019 •

edited

Loading