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(ecs): integrate AWS Cloud Map (service discovery) #2065

Merged
merged 21 commits into from
Apr 3, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
216 changes: 216 additions & 0 deletions design/aws-ecs-service-discovery-integration.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
# AWS ECS - Support for AWS Cloud Map (Service Discovery) Integration

To address issue [#1554](https://github.com/awslabs/aws-cdk/issues/1554), the
ECS construct library should provide a way to set
[`serviceRegistries`](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_CreateService.html#ECS-CreateService-request-serviceRegistries)
on their ECS service at the L1 level.


## 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

allow the customer to pass in the domain name and other configuration minimally
needed to instantiate a Cloud Map namespace within an ECS cluster and create a
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why addNamespace and not constructor parameters?

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

type (Public DNS, Private DNS or Http) and namespace name (domain name).

While it is possible to use more than one namespace for services in an ECS
cluster, realistically, we would expect ECS customers to only have one
namespace per given cluster. A Cloud Map service within that namespace would
then be created for each ECS service using service discovery in the cluster,
and would be discoverable by service name within that namespace, e.g.
frontend.mydomain.com, backend.mydomain.com, etc.

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?
Copy link
Contributor

Choose a reason for hiding this comment

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

?

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'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.


## Code changes

The following are the new methods/interfaces that would be required for the proposed approach:

#### Cluster#addNamespace(options: NamespaceOptions)

This will allow adding a Cloud Map namespace, which will be accessible as a
member on the cluster. In the case of a Private DNS Namespace, a Route53 hosted
zone will be created for the customer.

```ts
export interface NamespaceOptions {
/**
* The domain name for the namespace, such as foo.com
*/
name: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Call it domainName then?

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 suppose name is a bit generic, though on the namespace it's namespaceName which is a bit stuttery.

Copy link
Contributor

@rix0rrr rix0rrr Mar 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so namespaceName rather than domainName?


/**
* The type of CloudMap Namespace to create in your cluster
*
* @default PrivateDns
*/
type?: cloudmap.NamespaceType;

/**
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

*/
vpc?: ec2.IVpcNetwork;
}
```

#### service#enableServiceDiscovery(options: ServiceDiscoveryOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


This method would create a Cloud Map service, whose arn would then be passed as the serviceRegistry when the ECS service is created.

Other fields in the service registry are optionally needed depending on the
network mode of the task definition used by the ECS service.

- If the task definition uses the bridge or host network mode, a containerName
and containerPort combination are needed. These will be taken from the
defaultContainer on the task definition.

- If the task definition uses the awsvpc network mode and a type SRV DNS record
is used, you must specify either a containerName and containerPort
combination. These will be taken from the defaultContainer on the task definition.
NOTE: In this case, the API also allows you to simply pass in "port" at the
mutual exclusion of the `containerName` and `containerPort` combination, but
for simplicity we are only including `containerName` and `containerPort` and
not `port`.

NOTE: warn about creating service with public namespace?

If the customer wishes to have maximum configurability for their service, we will also add

```ts
export interface ServiceDiscoveryOptions {
/**
* Name of the cloudmap service to attach to the ECS Service
*
* @default CloudFormation-generated name
*/
name?: string,

/**
* The DNS type of the record that you want AWS Cloud Map to create. Supported record types
* include A or SRV.

* @default: A
*/
dnsRecordType?: cloudmap.DnsRecordType.A | cloudmap.DnsRecordType.SRV,

/**
* The amount of time, in seconds, that you want DNS resolvers to cache the settings for this
* record.
*
* @default 60
*/
dnsTtlSec?: number;

/**
* The number of 30-second intervals that you want Cloud Map to wait after receiving an
* UpdateInstanceCustomHealthStatus request before it changes the health status of a service instance.
* NOTE: This is used for a Custom HealthCheckCustomConfig
*/
failureThreshold?: number,
}
```

A full example would look like the following:

```
const vpc = new ec2.VpcNetwork(stack, 'Vpc', { maxAZs: 2 });

// Cloud Map Namespace
const namespace = new servicediscovery.PrivateDnsNamespace(stack, 'MyNamespace', {
name: 'mydomain.com',
vpc,
});

// Cloud Map Service

const cloudMapService = namespace.createService('MyCloudMapService', {
dnsRecordType: servicediscovery.DnsRecordType.A,
dnsTtlSec: 300,
customHealthCheck: {
failureThreshold = 1
}
});

// ECS Cluster
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
SoManyHs marked this conversation as resolved.
Show resolved Hide resolved

cluster.addCapacity('DefaultAutoScalingGroup', {
instanceType: new ec2.InstanceType('t2.micro')
});

cluster.addNamespace({ name: "foo.com" })

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

const container = taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
memoryLimitMiB: 256,
});

container.addPortMappings({
containerPort: 80,
hostPort: 8080,
protocol: ecs.Protocol.Tcp
});

const ecsService = new ecs.Ec2Service(stack, "MyECSService", {
cluster,
taskDefinition,
});

ecsService.enableServiceDiscovery(
dnsRecordType: servicediscovery.DnsRecordType.A,
dnsTtlSec: 300,
customHealthCheck: {
failureThreshold = 1
}
)

```
#### Service Discovery Considerations
##### See: (https://docs.aws.amazon.com/AmazonECS/latest/developerguide/service-discovery.html)

The following should be considered when using service discovery:

Service discovery is supported for tasks using the Fargate launch type if they are using platform version v1.1.0 or later. For more information, see AWS Fargate Platform Versions.

The Create Service workflow in the Amazon ECS console only supports registering services into private DNS namespaces. When a AWS Cloud Map private DNS namespace is created, a Route 53 private hosted zone will be created automatically.

Amazon ECS does not support registering services into public DNS namespaces.

The DNS records created for a service discovery service always register with the private IP address for the task, rather than the public IP address, even when public namespaces are used.

Service discovery requires that tasks specify either the awsvpc, bridge, or host network mode (none is not supported).

If the task definition your service task specifies uses the awsvpc network mode, you can create any combination of A or SRV records for each service task. If you use SRV records, a port is required.

If the task definition that your service task specifies uses the bridge or host network mode, an SRV record is the only supported DNS record type. Create an SRV record for each service task. The SRV record must specify a container name and container port combination from the task definition.

DNS records for a service discovery service can be queried within your VPC. They use the following format: <service discovery service name>.<service discovery namespace>. For more information, see Step 3: Verify Service Discovery.

When doing a DNS query on the service name, A records return a set of IP addresses that correspond to your tasks. SRV records return a set of IP addresses and ports per task.

You can configure service discovery for an ECS service that is behind a load balancer, but service discovery traffic is always routed to the task and not the load balancer.

Service discovery does not support the use of Classic Load Balancers.

It is recommended to use container-level health checks managed by Amazon ECS for your service discovery service.

HealthCheckCustomConfig—Amazon ECS manages health checks on your behalf. Amazon ECS uses information from container and health checks, as well as your task state, to update the health with AWS Cloud Map. This is specified using the --health-check-custom-config parameter when creating your service discovery service. For more information, see HealthCheckCustomConfig in the AWS Cloud Map API Reference.

If you are using the Amazon ECS console, the workflow creates one service discovery service per ECS service. It maps all of the task IP addresses as A records, or task IP addresses and port as SRV records.

Service discovery can only be configured when first creating a service. Updating existing services to configure service discovery for the first time or change the current configuration is not supported.

The AWS Cloud Map resources created when service discovery is used must be cleaned up manually. For more information, see Step 4: Clean Up in the Tutorial: Creating a Service Using Service Discovery topic.


95 changes: 95 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,22 @@ import cloudwatch = require('@aws-cdk/aws-cloudwatch');
import ec2 = require('@aws-cdk/aws-ec2');
import elbv2 = require('@aws-cdk/aws-elasticloadbalancingv2');
import iam = require('@aws-cdk/aws-iam');
import cloudmap = require('@aws-cdk/aws-servicediscovery');
import cdk = require('@aws-cdk/cdk');
import { NetworkMode, TaskDefinition } from '../base/task-definition';
import { ICluster } from '../cluster';
import { CfnService } from '../ecs.generated';
import { ScalableTaskCount } from './scalable-task-count';

/**
* Basic service properties
*/
export interface BaseServiceProps {
/**
* Cluster where service will be deployed
*/
readonly cluster: ICluster;

/**
* Number of desired copies of running tasks
*
Expand Down Expand Up @@ -50,6 +57,11 @@ export interface BaseServiceProps {
* @default ??? FIXME
*/
readonly healthCheckGracePeriodSeconds?: number;

/**
* Options for enabling AWS Cloud Map service discovery for the service
*/
readonly serviceDiscoveryOptions?: ServiceDiscoveryOptions;
}

/**
Expand Down Expand Up @@ -83,8 +95,12 @@ export abstract class BaseService extends cdk.Construct
*/
public readonly taskDefinition: TaskDefinition;

protected cloudmapService: cloudmap.Service;
protected cluster: ICluster;
protected loadBalancers = new Array<CfnService.LoadBalancerProperty>();
protected networkConfiguration?: CfnService.NetworkConfigurationProperty;
protected serviceRegistries = new Array<CfnService.ServiceRegistryProperty>();

private readonly resource: CfnService;
private scalableTaskCount?: ScalableTaskCount;

Expand All @@ -109,11 +125,13 @@ export abstract class BaseService extends cdk.Construct
healthCheckGracePeriodSeconds: props.healthCheckGracePeriodSeconds,
/* role: never specified, supplanted by Service Linked Role */
networkConfiguration: new cdk.Token(() => this.networkConfiguration),
serviceRegistries: new cdk.Token(() => this.serviceRegistries),
...additionalProps
});
this.serviceArn = this.resource.serviceArn;
this.serviceName = this.resource.serviceName;
this.clusterName = clusterName;
this.cluster = props.cluster;
}

/**
Expand Down Expand Up @@ -161,6 +179,14 @@ export abstract class BaseService extends cdk.Construct
});
}

/**
* Associate Service Discovery (Cloud Map) service
*/
public addServiceRegistry(registry: ServiceRegistry) {
SoManyHs marked this conversation as resolved.
Show resolved Hide resolved
const sr = this.renderServiceRegistry(registry);
this.serviceRegistries.push(sr);
}

/**
* Return the given named metric for this Service
*/
Expand Down Expand Up @@ -195,6 +221,14 @@ export abstract class BaseService extends cdk.Construct
};
}

private renderServiceRegistry(registry: ServiceRegistry): CfnService.ServiceRegistryProperty {
return {
registryArn: registry.arn,
containerName: registry.containerName,
containerPort: registry.containerPort,
};
}

/**
* Shared logic for attaching to an ELBv2
*/
Expand Down Expand Up @@ -236,3 +270,64 @@ export abstract class BaseService extends cdk.Construct
* The port range to open up for dynamic port mapping
*/
const EPHEMERAL_PORT_RANGE = new ec2.TcpPortRange(32768, 65535);

/**
* Options for enabling service discovery on an ECS service
*/
export interface ServiceDiscoveryOptions {
/**
* Name of the cloudmap service to attach to the ECS Service
*
* @default CloudFormation-generated name
*/
readonly name?: string,

/**
* The DNS type of the record that you want AWS Cloud Map to create. Supported record types include A or SRV.
*
* @default: A
*/
readonly dnsRecordType?: cloudmap.DnsRecordType.A | cloudmap.DnsRecordType.SRV,

/**
* The amount of time, in seconds, that you want DNS resolvers to cache the settings for this record.
*
* @default 60
*/
readonly dnsTtlSec?: number;

/**
* The number of 30-second intervals that you want Cloud Map to wait after receiving an
* UpdateInstanceCustomHealthStatus request before it changes the health status of a service instance.
* NOTE: This is used for HealthCheckCustomConfig
*/
readonly failureThreshold?: number,
}

/**
* Service Registry for ECS service
*/
export interface ServiceRegistry {
/**
* Arn of the Cloud Map Service that will register a Cloud Map Instance for your ECS Service
*/
readonly arn: string;

/**
* The container name value, already specified in the task definition, to be used for your service discovery service.
* If the task definition that your service task specifies uses the bridge or host network mode,
* you must specify a containerName and containerPort combination from the task definition.
* If the task definition that your service task specifies uses the awsvpc network mode and a type SRV DNS record is
* used, you must specify either a containerName and containerPort combination or a port value, but not both.
*/
readonly containerName?: string;

/**
* The container port value, already specified in the task definition, to be used for your service discovery service.
* If the task definition that your service task specifies uses the bridge or host network mode,
* you must specify a containerName and containerPort combination from the task definition.
* If the task definition that your service task specifies uses the awsvpc network mode and a type SRV DNS record is
* used, you must specify either a containerName and containerPort combination or a port value, but not both.
*/
readonly containerPort?: number;
}
Loading