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.

Sign up for GitHub

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

Jump to bottom

feat(appmesh): change VirtualService provider to a union-like class #11978

Merged
merged 20 commits into from
Jan 28, 2021
Merged

feat(appmesh): change VirtualService provider to a union-like class #11978

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
bec6a09
feat(appmesh): changes VirtualService provider to a union-like class
Dec 10, 2020
b824a40
Apply suggestions from code review
dfezzie Dec 15, 2020
a717a44
enforce provider and VS in same mesh
Dec 15, 2020
c46bdb8
Merge branch 'master' into refactor/virtualServiceTarget
Jan 21, 2021
db7c5a2
Merge branch 'master' into refactor/virtualServiceTarget
Jan 22, 2021
9f99c47
Update to require a provider
Jan 22, 2021
be05561
Remove construct reference with qualifier
Jan 22, 2021
fb2dcda
Update appmesh ecs addon
Jan 22, 2021
cb566fc
Add back VirtualServiceProps and enforce Virtual Service mesh matches…
Jan 22, 2021
56fb779
Update integ test to use imported ACM cert
Jan 22, 2021
ae5052e
Remove ACM and ACM PCA from integ tests
Jan 22, 2021
adcf421
Merge branch 'master' into refactor/virtualServiceTarget
Jan 22, 2021
e43c332
Add back some integ code I removed for some reason
Jan 22, 2021
e678b9e
Addresses PR comments
Jan 25, 2021
14aef5e
Remove addVirtualService method from IMesh and IMesh attribute from V…
Jan 26, 2021
a37c6db
Deploy integ test
Jan 26, 2021
ec39db1
Minor cosmetic changes
Jan 27, 2021
f6afe7b
Update VirtualServiceProps docs
Jan 27, 2021
575ded5
Remove Mesh.addVirtualService() from ReadMe
skinny85 Jan 27, 2021
93fb617
Merge branch 'master' into refactor/virtualServiceTarget
mergify[bot] Jan 27, 2021
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/appmesh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ export class AppMeshExtension extends ServiceExtension {
// virtual service -> virtual router -> virtual node
this.virtualService = new appmesh.VirtualService(this.scope, `${this.parentService.id}-virtual-service`, {
mesh: this.mesh,
virtualRouter: this.virtualRouter,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(this.virtualRouter),
virtualServiceName: serviceName,
});
}
Expand Down
6 changes: 2 additions & 4 deletions packages/@aws-cdk/aws-appmesh/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,22 +110,20 @@ Adding a virtual router as the provider:

```ts
mesh.addVirtualService('virtual-service', {
virtualRouter: router,
virtualServiceName: 'my-service.default.svc.cluster.local',
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(router),
});
```

Adding a virtual node as the provider:

```ts
mesh.addVirtualService('virtual-service', {
virtualNode: node,
virtualServiceName: `my-service.default.svc.cluster.local`,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualNode(node),
});
```

**Note** that only one must of `virtualNode` or `virtualRouter` must be chosen.

## Adding a VirtualNode

A `virtual node` acts as a logical pointer to a particular task group, such as an Amazon ECS service or a Kubernetes deployment.
Expand Down
113 changes: 73 additions & 40 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,11 @@ export interface VirtualServiceBaseProps {
readonly virtualServiceName?: string;

/**
* The VirtualRouter which the VirtualService uses as provider
* The VirtualNode or VirtualRouter which the VirtualService uses as its provider
*
* @default - At most one of virtualRouter and virtualNode is allowed.
* @default - No provider
*/
readonly virtualRouter?: IVirtualRouter;

/**
* The VirtualNode attached to the virtual service
*
* @default - At most one of virtualRouter and virtualNode is allowed.
*/
readonly virtualNode?: IVirtualNode;
readonly virtualServiceProvider?: VirtualServiceProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

2 changes here:

  1. Let's move this property to VirtualServiceProps below.
  2. Let's remove the mesh property from VirtualServiceProps (no reason to provide it redundantly when a VirtualRouter or VirtualNode were provided).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Do we not need the provider in the base props? How else will we define what type of resource backs the virtual service?

  2. Since the provider is optional, I don't think we can remove the mesh property. We still need to know what mesh the VS is created as a part of. We can enforce the mesh properties on the VS and Provider do match.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We do not, because the provider will be evident from the resource that the addService method will be on. For example, for VirtualNode, it will be:
public addService(props: VirtualServiceBaseProps): IVirtualService: {
  return new VirtualService(this, 'VirtualService', {
    ...props,
    provider: ServiceProvider.virtualNode(this),
  });
}

And similarly for Mesh and VirtualRouter.

  1. The provider should be required, of course 🙂.

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 I've provided a comment on #9490 that argues we should not have an addService method on the IVirtualNode and IVirtualRouter classes. Will wait on your response before I do anything here.


/**
* Client policy for this Virtual Service
Expand Down Expand Up @@ -135,33 +128,24 @@ export class VirtualService extends cdk.Resource implements IVirtualService {

public readonly clientPolicy?: ClientPolicy;

private readonly virtualServiceProvider?: CfnVirtualService.VirtualServiceProviderProperty;

constructor(scope: Construct, id: string, props: VirtualServiceProps) {
super(scope, id, {
physicalName: props.virtualServiceName || cdk.Lazy.string({ produce: () => cdk.Names.uniqueId(this) }),
});

if (props.virtualNode && props.virtualRouter) {
throw new Error('Must provide only one of virtualNode or virtualRouter for the provider');
}
const provider = props.virtualServiceProvider?.bind(this);
dfezzie marked this conversation as resolved.
Show resolved Hide resolved

this.mesh = props.mesh;
this.clientPolicy = props.clientPolicy;

// Check which provider to use node or router (or neither)
if (props.virtualRouter) {
this.virtualServiceProvider = this.addVirtualRouter(props.virtualRouter.virtualRouterName);
}
if (props.virtualNode) {
this.virtualServiceProvider = this.addVirtualNode(props.virtualNode.virtualNodeName);
}

const svc = new CfnVirtualService(this, 'Resource', {
meshName: this.mesh.meshName,
virtualServiceName: this.physicalName,
spec: {
provider: this.virtualServiceProvider,
provider: provider ? {
virtualNode: provider?.virtualNodeProvider,
virtualRouter: provider?.virtualRouterProvider,
}: undefined,
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
},
});

Expand All @@ -172,22 +156,6 @@ export class VirtualService extends cdk.Resource implements IVirtualService {
resourceName: this.physicalName,
});
}

private addVirtualRouter(name: string): CfnVirtualService.VirtualServiceProviderProperty {
return {
virtualRouter: {
virtualRouterName: name,
},
};
}

private addVirtualNode(name: string): CfnVirtualService.VirtualServiceProviderProperty {
return {
virtualNode: {
virtualNodeName: name,
},
};
}
}

/**
Expand All @@ -211,3 +179,68 @@ export interface VirtualServiceAttributes {
*/
readonly clientPolicy?: ClientPolicy;
}

/**
* Properties for a VirtualService provider
*/
export interface VirtualServiceProviderConfig {
/**
* Virtual Node based provider
*
* @default - none
*/
readonly virtualNodeProvider?: CfnVirtualService.VirtualNodeServiceProviderProperty;

/**
* Virtual Router based provider
*
* @default - none
*/
readonly virtualRouterProvider?: CfnVirtualService.VirtualRouterServiceProviderProperty;
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Represents the properties needed to define the provider for a VirtualService
*/
export abstract class VirtualServiceProvider {
/**
* Returns a VirtualNode based Provider for a VirtualService
*/
public static virtualNode(virtualNode: IVirtualNode): VirtualServiceProvider {
return new VirtualServiceProviderImpl(virtualNode, undefined);
}

/**
* Returns a VirtualRouter based Provider for a VirtualService
*/
public static virtualRouter(virtualRouter: IVirtualRouter): VirtualServiceProvider {
return new VirtualServiceProviderImpl(undefined, virtualRouter);
}

skinny85 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Enforces mutual exclusivity for VirtualService provider types.
*/
public abstract bind(_construct: cdk.Construct): VirtualServiceProviderConfig;
}

class VirtualServiceProviderImpl extends VirtualServiceProvider {
private readonly virtualNode?: IVirtualNode;
private readonly virtualRouter?: IVirtualRouter;

constructor(virtualNode?: IVirtualNode, virtualRouter?: IVirtualRouter) {
super();
this.virtualNode = virtualNode;
this.virtualRouter = virtualRouter;
}

public bind(_construct: cdk.Construct): VirtualServiceProviderConfig {
return {
virtualNodeProvider: this.virtualNode? {
virtualNodeName: this.virtualNode.virtualNodeName,
}: undefined,
virtualRouterProvider: this.virtualRouter? {
virtualRouterName: this.virtualRouter.virtualRouterName,
}: undefined,
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
};
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-appmesh/test/integ.mesh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const router = mesh.addVirtualRouter('router', {
});

const virtualService = mesh.addVirtualService('service', {
virtualRouter: router,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(router),
virtualServiceName: 'service1.domain.local',
});

Expand Down
37 changes: 2 additions & 35 deletions packages/@aws-cdk/aws-appmesh/test/test.mesh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,39 +124,6 @@ export = {
},

'When adding a VirtualService to a mesh': {
'with VirtualRouter and VirtualNode as providers': {
'should throw error'(test: Test) {
// GIVEN
const stack = new cdk.Stack();

// WHEN
const mesh = new appmesh.Mesh(stack, 'mesh', {
meshName: 'test-mesh',
});

const testNode = new appmesh.VirtualNode(stack, 'test-node', {
mesh,
dnsHostName: 'test-node',
});

const testRouter = mesh.addVirtualRouter('router', {
listeners: [
appmesh.VirtualRouterListener.http(),
],
});

// THEN
test.throws(() => {
mesh.addVirtualService('service', {
virtualServiceName: 'test-service.domain.local',
virtualNode: testNode,
virtualRouter: testRouter,
});
});

test.done();
},
},
'with single virtual router provider resource': {
'should create service'(test: Test) {
// GIVEN
Expand All @@ -175,7 +142,7 @@ export = {

mesh.addVirtualService('service', {
virtualServiceName: 'test-service.domain.local',
virtualRouter: testRouter,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(testRouter),
});

// THEN
Expand Down Expand Up @@ -215,7 +182,7 @@ export = {

mesh.addVirtualService('service2', {
virtualServiceName: 'test-service.domain.local',
virtualNode: node,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualNode(node),
});

// THEN
Expand Down