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(appmesh): change VirtualRouter's Listener to a union-like class #11277

Merged
merged 17 commits into from
Nov 7, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-appmesh/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export * from './route';
export * from './shared-interfaces';
export * from './virtual-node';
export * from './virtual-router';
export * from './virtual-router-listener';
export * from './virtual-service';
export * from './virtual-gateway';
export * from './virtual-gateway-listener';
Expand Down
122 changes: 122 additions & 0 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-router-listener.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import * as cdk from '@aws-cdk/core';
import { CfnVirtualRouter } from './appmesh.generated';
import { Protocol } from './shared-interfaces';

/**
* Represents the properties needed to define Listeners for a VirtualRouter
*/
export interface RouterListenerProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be *Options, so RouterListenerOptions. *Props are reserved for construction properties of Constructs. Apologies I did not notice this before (we'll probably have to go back and change a few of these in other places in the AppMesh module).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Should updating other places be done in this in this PR or separate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it separate.

/**
* Port to listen for connections on
*
* @default - 8080
*/
readonly port?: number
}

/**
* Properties for a VirtualRouter listener
*/
export interface VirtualRouterListenerConfig {
/**
* Single listener config for a VirtualRouter
*/
readonly listener: CfnVirtualRouter.VirtualRouterListenerProperty;
}

/**
* Represents the properties needed to define listeners for a VirtualRouter
*/
export abstract class VirtualRouterListener {
/**
* Returns an HTTP Listener for a VirtualRouter
*/
public static httpVirtualRouterListener(props: RouterListenerProps = {}): VirtualRouterListener {
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
return new HttpVirtualRouterListener(props);
}

/**
* Returns an HTTP2 Listener for a VirtualRouter
*/
public static http2GatewayListener(props: RouterListenerProps = {}): VirtualRouterListener {
return new Http2VirtualRouterListener(props);
}

/**
* Returns a GRPC Listener for a VirtualRouter
*/
public static grpcGatewayListener(props: RouterListenerProps = {}): VirtualRouterListener {
return new GrpcVirtualRouterListener(props);
}

/**
* Returns a TCP Listener for a VirtualRouter
*/
public static tcpGatewayListener(props: RouterListenerProps = {}): VirtualRouterListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance any of these will have protocol-specific properties later? Because frankly, in its current form, I don't think this abstraction is buying us anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Virtual Routers specifically do not currently have any protocol specific properties. The main reason I think we should do this change is for consistency. If we have listeners on Virtual Nodes and Virtual Gateways, having listeners for your virtual routers that are defined in a different way seems like poor UX. If you don't think it is worth the change, we can scrap this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency is not a bad argument.

How about this. Let's make port?: number an optional positional argument to each of the static factory methods in this class, and get rid of RouterListenerProps completely. Then, creating a Listener will be as simple as calling VirtualRouterListener.http(8081). I think that's a reasonable compromise between consistency and ease of use.

Thoughts on this idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Simpler for sure. Made the update

return new TcpVirtualRouterListener(props);
}

/**
* Protocol the listener implements
*/
protected abstract protocol: Protocol;

/**
* Port to listen for connections on
*/
protected abstract port: number;

/**
* Called when the VirtualRouterListener type is initialized. Can be used to enforce
* mutual exclusivity
*/
public abstract bind(scope: cdk.Construct): VirtualRouterListenerConfig;
}

class HttpVirtualRouterListener extends VirtualRouterListener {
/**
* Protocol the listener implements
*/
protected protocol: Protocol = Protocol.HTTP;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you instead make the base class abstract with a public abstract readonly protocol: Protocol property, and then each of the subclasses will implement protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can further simplify this by only having one subclass for all 4 protocols.

/**
* Port to listen for connections on
*/
protected port: number;
dfezzie marked this conversation as resolved.
Show resolved Hide resolved

constructor(props: RouterListenerProps = {}) {
super();
this.port = props.port ? props.port : 8080;
}

bind(_scope: cdk.Construct): VirtualRouterListenerConfig {
return {
listener: {
portMapping: {
port: this.port,
protocol: this.protocol,
},
},
};
}
}

class Http2VirtualRouterListener extends HttpVirtualRouterListener {
constructor(props: RouterListenerProps = {}) {
super(props);
this.protocol = Protocol.HTTP2;
}
}

class GrpcVirtualRouterListener extends HttpVirtualRouterListener {
constructor(props: RouterListenerProps = {}) {
super(props);
this.protocol = Protocol.GRPC;
}
}

class TcpVirtualRouterListener extends HttpVirtualRouterListener {
constructor(props: RouterListenerProps = {}) {
super(props);
this.protocol = Protocol.TCP;
}
}
32 changes: 9 additions & 23 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Construct } from 'constructs';
import { CfnVirtualRouter } from './appmesh.generated';
import { IMesh, Mesh } from './mesh';
import { Route, RouteBaseProps } from './route';
import { PortMapping, Protocol } from './shared-interfaces';
import { VirtualRouterListener } from './virtual-router-listener';

/**
* Interface which all VirtualRouter based classes MUST implement
Expand All @@ -27,11 +27,6 @@ export interface IVirtualRouter extends cdk.IResource {
* The service mesh that the virtual router resides in
*/
readonly mesh: IMesh;

/**
* Add a single route to the router
*/
addRoute(id: string, props: RouteBaseProps): Route;
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -43,7 +38,7 @@ export interface VirtualRouterBaseProps {
*
* @default - A listener on HTTP port 8080
*/
readonly listener?: Listener;
readonly listeners?: VirtualRouterListener[];
dfezzie marked this conversation as resolved.
Show resolved Hide resolved

/**
* The name of the VirtualRouter
Expand All @@ -53,16 +48,6 @@ export interface VirtualRouterBaseProps {
readonly virtualRouterName?: string;
}

/**
* A single listener for
*/
export interface Listener {
/**
* Listener port for the virtual router
*/
readonly portMapping: PortMapping;
}

abstract class VirtualRouterBase extends cdk.Resource implements IVirtualRouter {
/**
* The name of the VirtualRouter
Expand Down Expand Up @@ -149,8 +134,11 @@ export class VirtualRouter extends VirtualRouterBase {
});

this.mesh = props.mesh;

this.addListener(props.listener || { portMapping: { port: 8080, protocol: Protocol.HTTP } });
if (props.listeners && props.listeners.length) {
props.listeners.forEach(listener => this.addListener(listener));
} else {
this.addListener(VirtualRouterListener.httpVirtualRouterListener());
}

const router = new CfnVirtualRouter(this, 'Resource', {
virtualRouterName: this.physicalName,
Expand All @@ -171,10 +159,8 @@ export class VirtualRouter extends VirtualRouterBase {
/**
* Add port mappings to the router
*/
private addListener(listener: Listener) {
this.listeners.push({
portMapping: listener.portMapping,
});
private addListener(listener: VirtualRouterListener) {
this.listeners.push(listener.bind(this).listener);
}
}

Expand Down
9 changes: 3 additions & 6 deletions packages/@aws-cdk/aws-appmesh/test/integ.mesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,9 @@ const namespace = new cloudmap.PrivateDnsNamespace(stack, 'test-namespace', {

const mesh = new appmesh.Mesh(stack, 'mesh');
const router = mesh.addVirtualRouter('router', {
listener: {
portMapping: {
port: 8080,
protocol: appmesh.Protocol.HTTP,
},
},
listeners: [
appmesh.VirtualRouterListener.httpVirtualRouterListener(),
],
});

const virtualService = mesh.addVirtualService('service', {
Expand Down
27 changes: 9 additions & 18 deletions packages/@aws-cdk/aws-appmesh/test/test.mesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,9 @@ export = {
});

mesh.addVirtualRouter('router', {
listener: {
portMapping: {
port: 8080,
protocol: appmesh.Protocol.HTTP,
},
},
listeners: [
appmesh.VirtualRouterListener.httpVirtualRouterListener(),
],
});

// THEN
Expand Down Expand Up @@ -147,12 +144,9 @@ export = {
});

const testRouter = mesh.addVirtualRouter('router', {
listener: {
portMapping: {
port: 8080,
protocol: appmesh.Protocol.HTTP,
},
},
listeners: [
appmesh.VirtualRouterListener.httpVirtualRouterListener(),
],
});

// THEN
Expand All @@ -178,12 +172,9 @@ export = {
});

const testRouter = mesh.addVirtualRouter('test-router', {
listener: {
portMapping: {
port: 8080,
protocol: appmesh.Protocol.HTTP,
},
},
listeners: [
appmesh.VirtualRouterListener.httpVirtualRouterListener(),
],
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
});

mesh.addVirtualService('service', {
Expand Down
57 changes: 57 additions & 0 deletions packages/@aws-cdk/aws-appmesh/test/test.virtual-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,63 @@ import { Test } from 'nodeunit';
import * as appmesh from '../lib';

export = {
'When creating a VirtualRouter': {
'should have protocol variant listeners'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const mesh = new appmesh.Mesh(stack, 'mesh', {
meshName: 'test-mesh',
});
// WHEN
mesh.addVirtualRouter('http-router-listener', {
listeners: [
appmesh.VirtualRouterListener.httpVirtualRouterListener(),
],
virtualRouterName: 'http-router-listener',
});

mesh.addVirtualRouter('http2-router-listener', {
listeners: [
appmesh.VirtualRouterListener.http2GatewayListener(),
],
virtualRouterName: 'http2-router-listener',
});

mesh.addVirtualRouter('grpc-router-listener', {
listeners: [
appmesh.VirtualRouterListener.grpcGatewayListener(),
],
virtualRouterName: 'grpc-router-listener',
});

mesh.addVirtualRouter('tcp-router-listener', {
listeners: [
appmesh.VirtualRouterListener.tcpGatewayListener(),
],
virtualRouterName: 'tcp-router-listener',
});

// THEN
const expectedPorts = [appmesh.Protocol.HTTP, appmesh.Protocol.HTTP2, appmesh.Protocol.GRPC, appmesh.Protocol.TCP];
expectedPorts.forEach(protocol => {
expect(stack).to(haveResourceLike('AWS::AppMesh::VirtualRouter', {
VirtualRouterName: `${protocol}-router-listener`,
Spec: {
Listeners: [
{
PortMapping: {
Port: 8080,
Protocol: protocol,
},
},
],
},
}));
});

test.done();
},
},
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
'When adding route to existing VirtualRouter': {
'should create route resource'(test: Test) {
// GIVEN
Expand Down