From c968c96d863105e9cfb85814aa0fd9089f9c336b Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 4 Oct 2019 11:34:52 +0200 Subject: [PATCH] fix(cloudmap): fix CloudMap Service import, expose ECS CloudMap Service (#4313) - Fix the missing initialization of `namespace` on CloudMap's `Service.fromServiceAttributes()`. - Expose the created CloudMap Service on ECS services so that `fromServiceAttributes()` doesn't need to be called at all. - Fix a number of property initialization errors across the library. Fixes #4286. BREAKING CHANGE: `cloudmap.Service.fromServiceAttributes` takes a newly required argument `namespace`. --- allowed-breaking-changes.txt | 2 +- .../lib/pipeline-deploy-stack-action.ts | 6 +- .../@aws-cdk/aws-apigateway/lib/restapi.ts | 3 +- .../aws-appmesh/lib/virtual-router.ts | 41 +++++- .../aws-appmesh/test/test.virtual-router.ts | 13 ++ .../@aws-cdk/aws-codebuild/lib/project.ts | 2 +- .../application-load-balanced-service-base.ts | 2 +- .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 129 ++++++++++-------- packages/@aws-cdk/aws-ecs/lib/images/ecr.ts | 4 +- .../lib/log-drivers/generic-log-driver.ts | 8 +- .../test/fargate/test.fargate-service.ts | 4 +- .../aws-eks/test/integ.eks-kubectl.lit.ts | 1 - .../test/alb/test.security-groups.ts | 9 +- .../aws-servicediscovery/lib/service.ts | 3 +- .../lib/sagemaker-train-task.ts | 6 +- 15 files changed, 149 insertions(+), 84 deletions(-) diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index a2441dc2607d1..402968212c43d 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -23,7 +23,7 @@ incompatible-argument:@aws-cdk/aws-apigateway.ProxyResource.addProxy incompatible-argument:@aws-cdk/aws-apigateway.Resource.addProxy incompatible-argument:@aws-cdk/aws-apigateway.ResourceBase.addProxy incompatible-argument:@aws-cdk/aws-apigateway.IResource.addProxy +incompatible-argument:@aws-cdk/aws-servicediscovery.Service.fromServiceAttributes removed:@aws-cdk/core.ConstructNode.addReference removed:@aws-cdk/core.ConstructNode.references removed:@aws-cdk/core.OutgoingReference - diff --git a/packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts b/packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts index bef929a4e2d01..abbb184e25238 100644 --- a/packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts +++ b/packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts @@ -93,7 +93,7 @@ export class PipelineDeployStackAction implements codepipeline.IAction { /** * The role used by CloudFormation for the deploy action */ - private _deploymentRole: iam.IRole; + private _deploymentRole?: iam.IRole; private readonly stack: cdk.Stack; private readonly prepareChangeSetAction: cpactions.CloudFormationCreateReplaceChangeSetAction; @@ -147,6 +147,10 @@ export class PipelineDeployStackAction implements codepipeline.IAction { } public get deploymentRole(): iam.IRole { + if (!this._deploymentRole) { + throw new Error(`Use this action in a pipeline first before accessing 'deploymentRole'`); + } + return this._deploymentRole; } diff --git a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts index dd7549d6de647..99b2193ccc4b0 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts @@ -202,7 +202,7 @@ export class RestApi extends Resource implements IRestApi { * If `deploy` is disabled, you will need to explicitly assign this value in order to * set up integrations. */ - public deploymentStage: Stage; + public deploymentStage!: Stage; /** * The domain name mapped to this API, if defined through the `domainName` @@ -242,6 +242,7 @@ export class RestApi extends Resource implements IRestApi { } this.root = new RootResource(this, props, resource.attrRootResourceId); + this.restApiRootResourceId = resource.attrRootResourceId; if (props.domainName) { this.domainName = this.addDomainName('CustomDomain', props.domainName); diff --git a/packages/@aws-cdk/aws-appmesh/lib/virtual-router.ts b/packages/@aws-cdk/aws-appmesh/lib/virtual-router.ts index 5733db4768746..26782017d945f 100644 --- a/packages/@aws-cdk/aws-appmesh/lib/virtual-router.ts +++ b/packages/@aws-cdk/aws-appmesh/lib/virtual-router.ts @@ -1,7 +1,7 @@ import cdk = require('@aws-cdk/core'); import { CfnVirtualRouter } from './appmesh.generated'; -import { IMesh } from './mesh'; +import { IMesh, Mesh } from './mesh'; import { Route, RouteBaseProps } from './route'; import { PortMapping, Protocol } from './shared-interfaces'; @@ -119,6 +119,13 @@ export class VirtualRouter extends VirtualRouterBase { return new ImportedVirtualRouter(scope, id, { meshName, virtualRouterName }); } + /** + * Import an existing virtual router given attributes + */ + public static fromVirtualRouterAttributes(scope: cdk.Construct, id: string, attrs: VirtualRouterAttributes): IVirtualRouter { + return new ImportedVirtualRouter(scope, id, attrs); + } + /** * The name of the VirtualRouter */ @@ -174,7 +181,7 @@ export class VirtualRouter extends VirtualRouterBase { /** * Interface with properties ncecessary to import a reusable VirtualRouter */ -interface VirtualRouterAttributes { +export interface VirtualRouterAttributes { /** * The name of the VirtualRouter */ @@ -188,6 +195,11 @@ interface VirtualRouterAttributes { /** * The AppMesh mesh the VirtualRouter belongs to */ + readonly mesh?: IMesh; + + /** + * The name of the AppMesh mesh the VirtualRouter belongs to + */ readonly meshName?: string; } @@ -205,14 +217,21 @@ class ImportedVirtualRouter extends VirtualRouterBase { */ public readonly virtualRouterArn: string; - /** - * The AppMesh mesh the VirtualRouter belongs to - */ - public readonly mesh: IMesh; + private _mesh?: IMesh; constructor(scope: cdk.Construct, id: string, props: VirtualRouterAttributes) { super(scope, id); + if (props.mesh) { + this._mesh = props.mesh; + } + if (props.meshName) { + if (props.mesh) { + throw new Error(`Supply either 'mesh' or 'meshName', but not both`); + } + this._mesh = Mesh.fromMeshName(this, 'Mesh', props.meshName); + } + if (props.virtualRouterArn) { this.virtualRouterArn = props.virtualRouterArn; this.virtualRouterName = cdk.Fn.select(2, cdk.Fn.split('/', cdk.Stack.of(scope).parseArn(props.virtualRouterArn).resourceName!)); @@ -227,4 +246,14 @@ class ImportedVirtualRouter extends VirtualRouterBase { throw new Error('Need either arn or both names'); } } + + /** + * The AppMesh mesh the VirtualRouter belongs to + */ + public get mesh(): IMesh { + if (!this._mesh) { + throw new Error(`Please supply either 'mesh' or 'meshName' when calling 'fromVirtualRouterAttributes'`); + } + return this._mesh; + } } diff --git a/packages/@aws-cdk/aws-appmesh/test/test.virtual-router.ts b/packages/@aws-cdk/aws-appmesh/test/test.virtual-router.ts index f475178703504..6df480368da34 100644 --- a/packages/@aws-cdk/aws-appmesh/test/test.virtual-router.ts +++ b/packages/@aws-cdk/aws-appmesh/test/test.virtual-router.ts @@ -303,4 +303,17 @@ export = { test.done(); }, }, + + 'can import a virtual router'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + const vr = appmesh.VirtualRouter.fromVirtualRouterName(stack, 'Router', 'MyMesh', 'MyRouter'); + + // THEN + test.ok(vr.mesh !== undefined); + + test.done(); + }, }; diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index bff6c5f75f976..d46dacaa29c46 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -175,7 +175,7 @@ abstract class ProjectBase extends Resource implements IProject { * May be unset, in which case this Project is not configured to use a VPC. * @internal */ - protected _connections: ec2.Connections; + protected _connections: ec2.Connections | undefined; /** * Access the Connections object. diff --git a/packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts b/packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts index 18d87de9b85eb..3ddbfd71bff7e 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts +++ b/packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts @@ -233,7 +233,7 @@ export abstract class ApplicationLoadBalancedServiceBase extends cdk.Construct { /** * Certificate Manager certificate to associate with the load balancer. */ - public readonly certificate: ICertificate; + public readonly certificate?: ICertificate; /** * The cluster that hosts the service. diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index 0b0317511c830..0efe699da805c 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -221,6 +221,13 @@ export abstract class BaseService extends Resource } } + /** + * The CloudMap service created for this service, if any. + */ + public get cloudMapService(): cloudmap.IService | undefined { + return this.cloudmapService; + } + /** * This method is called to attach this service to an Application Load Balancer. * @@ -303,6 +310,68 @@ export abstract class BaseService extends Resource }); } + /** + * Enable CloudMap service discovery for the service + * + * @returns The created CloudMap service + */ + public enableCloudMap(options: CloudMapOptions): cloudmap.Service { + const sdNamespace = this.cluster.defaultCloudMapNamespace; + if (sdNamespace === undefined) { + throw new Error("Cannot enable service discovery if a Cloudmap Namespace has not been created in the cluster."); + } + + // Determine DNS type based on network mode + const networkMode = this.taskDefinition.networkMode; + if (networkMode === NetworkMode.NONE) { + throw new Error("Cannot use a service discovery if NetworkMode is None. Use Bridge, Host or AwsVpc instead."); + } + + // Bridge or host network mode requires SRV records + let dnsRecordType = options.dnsRecordType; + + if (networkMode === NetworkMode.BRIDGE || networkMode === NetworkMode.HOST) { + if (dnsRecordType === undefined) { + dnsRecordType = cloudmap.DnsRecordType.SRV; + } + if (dnsRecordType !== cloudmap.DnsRecordType.SRV) { + throw new Error("SRV records must be used when network mode is Bridge or Host."); + } + } + + // Default DNS record type for AwsVpc network mode is A Records + if (networkMode === NetworkMode.AWS_VPC) { + if (dnsRecordType === undefined) { + dnsRecordType = cloudmap.DnsRecordType.A; + } + } + + // 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 a containerName and containerPort combination + const containerName = dnsRecordType === cloudmap.DnsRecordType.SRV ? this.taskDefinition.defaultContainer!.containerName : undefined; + const containerPort = dnsRecordType === cloudmap.DnsRecordType.SRV ? this.taskDefinition.defaultContainer!.containerPort : undefined; + + const cloudmapService = new cloudmap.Service(this, 'CloudmapService', { + namespace: sdNamespace, + name: options.name, + dnsRecordType: dnsRecordType!, + customHealthCheck: { failureThreshold: options.failureThreshold || 1 } + }); + + const serviceArn = cloudmapService.serviceArn; + + // add Cloudmap service to the ECS Service's serviceRegistry + this.addServiceRegistry({ + arn: serviceArn, + containerName, + containerPort + }); + + this.cloudmapService = cloudmapService; + + return cloudmapService; + } + /** * This method returns the specified CloudWatch metric name for this service. */ @@ -430,66 +499,6 @@ export abstract class BaseService extends Resource this.serviceRegistries.push(sr); } - /** - * Enable CloudMap service discovery for the service - */ - private enableCloudMap(options: CloudMapOptions): cloudmap.Service { - const sdNamespace = this.cluster.defaultCloudMapNamespace; - if (sdNamespace === undefined) { - throw new Error("Cannot enable service discovery if a Cloudmap Namespace has not been created in the cluster."); - } - - // Determine DNS type based on network mode - const networkMode = this.taskDefinition.networkMode; - if (networkMode === NetworkMode.NONE) { - throw new Error("Cannot use a service discovery if NetworkMode is None. Use Bridge, Host or AwsVpc instead."); - } - - // Bridge or host network mode requires SRV records - let dnsRecordType = options.dnsRecordType; - - if (networkMode === NetworkMode.BRIDGE || networkMode === NetworkMode.HOST) { - if (dnsRecordType === undefined) { - dnsRecordType = cloudmap.DnsRecordType.SRV; - } - if (dnsRecordType !== cloudmap.DnsRecordType.SRV) { - throw new Error("SRV records must be used when network mode is Bridge or Host."); - } - } - - // Default DNS record type for AwsVpc network mode is A Records - if (networkMode === NetworkMode.AWS_VPC) { - if (dnsRecordType === undefined) { - dnsRecordType = cloudmap.DnsRecordType.A; - } - } - - // 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 a containerName and containerPort combination - const containerName = dnsRecordType === cloudmap.DnsRecordType.SRV ? this.taskDefinition.defaultContainer!.containerName : undefined; - const containerPort = dnsRecordType === cloudmap.DnsRecordType.SRV ? this.taskDefinition.defaultContainer!.containerPort : undefined; - - const cloudmapService = new cloudmap.Service(this, 'CloudmapService', { - namespace: sdNamespace, - name: options.name, - dnsRecordType: dnsRecordType!, - customHealthCheck: { failureThreshold: options.failureThreshold || 1 } - }); - - const serviceArn = cloudmapService.serviceArn; - - // add Cloudmap service to the ECS Service's serviceRegistry - this.addServiceRegistry({ - arn: serviceArn, - containerName, - containerPort - }); - - this.cloudmapService = cloudmapService; - - return cloudmapService; - } - /** * Return the default grace period when load balancers are configured and * healthCheckGracePeriod is not already set diff --git a/packages/@aws-cdk/aws-ecs/lib/images/ecr.ts b/packages/@aws-cdk/aws-ecs/lib/images/ecr.ts index 4ed2eb5283d03..30d6bab8f670c 100644 --- a/packages/@aws-cdk/aws-ecs/lib/images/ecr.ts +++ b/packages/@aws-cdk/aws-ecs/lib/images/ecr.ts @@ -21,13 +21,15 @@ export class EcrImage extends ContainerImage { */ constructor(private readonly repository: ecr.IRepository, private readonly tag: string) { super(); + + this.imageName = this.repository.repositoryUriForTag(this.tag); } public bind(_scope: Construct, containerDefinition: ContainerDefinition): ContainerImageConfig { this.repository.grantPull(containerDefinition.taskDefinition.obtainExecutionRole()); return { - imageName: this.repository.repositoryUriForTag(this.tag) + imageName: this.imageName }; } } diff --git a/packages/@aws-cdk/aws-ecs/lib/log-drivers/generic-log-driver.ts b/packages/@aws-cdk/aws-ecs/lib/log-drivers/generic-log-driver.ts index 28f78fbcd1a42..9cd7beb6ae95d 100644 --- a/packages/@aws-cdk/aws-ecs/lib/log-drivers/generic-log-driver.ts +++ b/packages/@aws-cdk/aws-ecs/lib/log-drivers/generic-log-driver.ts @@ -51,17 +51,17 @@ export class GenericLogDriver extends LogDriver { * * @param props the generic log driver configuration options. */ - constructor(private readonly props: GenericLogDriverProps) { + constructor(props: GenericLogDriverProps) { super(); + + this.logDriver = props.logDriver; + this.options = props.options || {}; } /** * Called when the log driver is configured on a container. */ public bind(_scope: Construct, _containerDefinition: ContainerDefinition): LogDriverConfig { - this.logDriver = this.props.logDriver; - this.options = this.props.options || {}; - return { logDriver: this.logDriver, options: removeEmpty(this.options), diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts index 489c4283e90da..2a82bf0a2ae3c 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts @@ -100,7 +100,7 @@ export = { image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"), }); - new ecs.FargateService(stack, "FargateService", { + const svc = new ecs.FargateService(stack, "FargateService", { cluster, taskDefinition, desiredCount: 2, @@ -125,6 +125,8 @@ export = { }); // THEN + test.ok(svc.cloudMapService !== undefined); + expect(stack).to(haveResource("AWS::ECS::Service", { TaskDefinition: { Ref: "FargateTaskDefC6FB60B4" diff --git a/packages/@aws-cdk/aws-eks/test/integ.eks-kubectl.lit.ts b/packages/@aws-cdk/aws-eks/test/integ.eks-kubectl.lit.ts index f1e6c848f1850..992df6d1b38b2 100644 --- a/packages/@aws-cdk/aws-eks/test/integ.eks-kubectl.lit.ts +++ b/packages/@aws-cdk/aws-eks/test/integ.eks-kubectl.lit.ts @@ -17,7 +17,6 @@ class VpcStack extends TestStack { class ClusterStack extends TestStack { public readonly cluster: Cluster; - public readonly instanceRoleExportName: string; constructor(scope: Construct, id: string, props: { vpc: ec2.Vpc }) { super(scope, id); diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.security-groups.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.security-groups.ts index 417cbe8d09d83..e29fc7498d9d0 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.security-groups.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.security-groups.ts @@ -281,7 +281,7 @@ class TestFixture { public readonly stack: cdk.Stack; public readonly vpc: ec2.Vpc; public readonly lb: elbv2.ApplicationLoadBalancer; - public readonly listener: elbv2.ApplicationListener; + public readonly _listener: elbv2.ApplicationListener | undefined; constructor(createListener?: boolean) { this.app = new cdk.App(); @@ -293,7 +293,12 @@ class TestFixture { createListener = createListener === undefined ? true : createListener; if (createListener) { - this.listener = this.lb.addListener('Listener', { port: 80, open: false }); + this._listener = this.lb.addListener('Listener', { port: 80, open: false }); } } + + public get listener(): elbv2.ApplicationListener { + if (this._listener === undefined) { throw new Error('Did not create a listener'); } + return this._listener; + } } diff --git a/packages/@aws-cdk/aws-servicediscovery/lib/service.ts b/packages/@aws-cdk/aws-servicediscovery/lib/service.ts index 3c5f3eb35b783..3f8294763a7e4 100644 --- a/packages/@aws-cdk/aws-servicediscovery/lib/service.ts +++ b/packages/@aws-cdk/aws-servicediscovery/lib/service.ts @@ -138,6 +138,7 @@ abstract class ServiceBase extends Resource implements IService { } export interface ServiceAttributes { + readonly namespace: INamespace; readonly serviceName: string; readonly serviceId: string; readonly serviceArn: string; @@ -152,7 +153,7 @@ export class Service extends ServiceBase { public static fromServiceAttributes(scope: Construct, id: string, attrs: ServiceAttributes): IService { class Import extends ServiceBase { - public namespace: INamespace; + public namespace: INamespace = attrs.namespace; public serviceId = attrs.serviceId; public serviceArn = attrs.serviceArn; public dnsRecordType = attrs.dnsRecordType; diff --git a/packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-train-task.ts b/packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-train-task.ts index 9ca6d3af60a4e..63e17a8fda208 100644 --- a/packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-train-task.ts +++ b/packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-train-task.ts @@ -108,10 +108,10 @@ export class SagemakerTrainTask implements iam.IGrantable, ec2.IConnectable, sfn */ private readonly stoppingCondition: StoppingCondition; - private readonly vpc: ec2.IVpc; - private securityGroup: ec2.ISecurityGroup; + private readonly vpc?: ec2.IVpc; + private securityGroup?: ec2.ISecurityGroup; private readonly securityGroups: ec2.ISecurityGroup[] = []; - private readonly subnets: string[]; + private readonly subnets?: string[]; private readonly integrationPattern: sfn.ServiceIntegrationPattern; private _role?: iam.IRole; private _grantPrincipal?: iam.IPrincipal;