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): add listener timeout to Virtual Nodes #10793

Merged
merged 27 commits into from Nov 11, 2020

Conversation

sshver
Copy link
Contributor

@sshver sshver commented Oct 8, 2020

Adds listener timeout to Virtual Nodes.

BREAKING CHANGE: IVirtualNode no longer has the addBackends() method. A backend can be added to VirtualNode using the addBackend() method which accepts a single IVirtualService

  • appmesh: IVirtualNode no longer has the addListeners() method. A listener can be added to VirtualNode using the addListener() method which accepts a single VirtualNodeListener
  • appmesh: VirtualNode no longer has a default listener. It is valid to have a VirtualNode without any listeners
  • appmesh: the construction property listener of VirtualNode has been renamed to listeners, and its type changed to an array of listeners
  • appmesh: the struct VirtualNodeListener has been removed. To create Virtual Node listeners, use the static factory methods of the VirtualNodeListener class

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

@skinny85 skinny85 self-assigned this Oct 8, 2020
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @sshver !

Before I do a full review, I wanted to ask one thing I noticed. In the VirtualNodeListener, there is a portMapping property, that has a protocol property. I can't help but notice that the 4 values for this Protocol enum correspond to the timeout values here.

I assume that you can only set the timeout for a protocol that you've set in portMapping, and doing it for a different protocol is either an error, or is silently ignored? If that's the case, I wonder whether we're missing a bigger opportunity here to model this in a richer way. Just off the top of my head, what if Protocol was a more sophisticated type instead of just a simple enum, and you could do things like:

const node = new VirtualNode(this, 'node', {
  listener: {
    portMapping: {
      port: 8080,
      protocol: Protocol.http({
        idleTimeout: Duration.minutes(1),
        requestTimeout: Duration.minutes(2),
      }),
    },
    // ...

Let me know what you think of this direction!

@dfezzie
Copy link
Contributor

dfezzie commented Oct 12, 2020

Thanks for the contribution @sshver !

Before I do a full review, I wanted to ask one thing I noticed. In the VirtualNodeListener, there is a portMapping property, that has a protocol property. I can't help but notice that the 4 values for this Protocol enum correspond to the timeout values here.

I assume that you can only set the timeout for a protocol that you've set in portMapping, and doing it for a different protocol is either an error, or is silently ignored? If that's the case, I wonder whether we're missing a bigger opportunity here to model this in a richer way. Just off the top of my head, what if Protocol was a more sophisticated type instead of just a simple enum, and you could do things like:

const node = new VirtualNode(this, 'node', {
  listener: {
    portMapping: {
      port: 8080,
      protocol: Protocol.http({
        idleTimeout: Duration.minutes(1),
        requestTimeout: Duration.minutes(2),
      }),
    },
    // ...

Let me know what you think of this direction!

We do use Protocol throughout the API with varying context. For example, you can use Protocol in the health check portion of virtual nodes, where the idle and perRequest may not necessarily map.

How could you distinguish between places where you can use the properties?

@skinny85
Copy link
Contributor

Before I answer that @dfezzie , can you answer my original question?

I assume that you can only set the timeout for a protocol that you've set in portMapping, and doing it for a different protocol is either an error, or is silently ignored?

@sshver
Copy link
Contributor Author

sshver commented Oct 13, 2020

Before I answer that @dfezzie , can you answer my original question?

I assume that you can only set the timeout for a protocol that you've set in portMapping, and doing it for a different protocol is either an error, or is silently ignored?

The timeout should be set up for a protocol, the listener uses. Doing it for a different protocol is an error. This was a miss from my end. I have updated the pull request and fixed this error.

@skinny85
Copy link
Contributor

skinny85 commented Oct 13, 2020

Thanks @sshver and @dfezzie . Here's my issue.

So in the current modeling, we have a bunch of completely separate properties (portMapping, healthCheck, timeout ) in VirtualNodeListener. Separate properties usually imply they can be set separately - but that is not the case here. When I set protocol in portMapping to HTTP, I suddenly have a constraint that I also have to use http for the timeout key (I also expect there is a similar constraint with healthCheck).

I'm wondering whether we can capture these invariants into a class that makes it obvious at compile-time what those different constraints are. Something like this:

new appmesh.VirtualNode(this, 'Node', {
  listener: appmesh.Listener.httpListener({
    timeout: {
      // only HTTP timeout properties are valid here!
      // ...
    },
    healthCheck: {
      // no need to specify Protocol here - we know it's HTTP!
    },
    // ...
  },
});

And we would have methods like tcpListener(), grpcListener(), etc.

Thoughts on this?

@dfezzie
Copy link
Contributor

dfezzie commented Oct 14, 2020

@skinny85 I was thinking along these lines as well. I think it is a good pattern for our service to adopt in the CDK as we have multiple instances where we are setting different properties based on the protocol. I implemented this to see how it would work for Gateway Routes as I was working on those during this thread and it works well. It is definitely easier to reason about as a user of CDK and makes implementation a bit simpler as well.

It would require us rewriting a few of the implementations as they stand, but I'm thinking it may be worth the effort

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Nice work @sshver ! Some notes below, mainly I don't like the change from a single listener to having many of them in VirtualNode, and also the duplication present in the VirtualNodeListener hierarchy must be addressed.

packages/@aws-cdk/aws-appmesh/README.md Show resolved Hide resolved
},
listeners: [appmesh.VirtualNodeListener.httpNodeListener({
port: containerextension.trafficPort,
})],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? What if the protocol is different than HTTP?

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 have added a function to add the listener to the virtual node based on protocol.

/**
* Represents the properties needed to define a Listeners for a VirtualNode
*/
export interface VirtualNodeListenerProps {
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 please move VirtualNodeListener and all its related types (*NodeListenerProps, * Timeout, etc.) to its own file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created virtual-node-listener and moved the related item to that file.

/**
* Returns an HealthCheck for a VirtualNode
*/
public static renderHealthCheck(pm: PortMapping, hc: HealthCheck | undefined): CfnVirtualNode.HealthCheckProperty | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

This absolutely should not be part of the public API of this module.

Remember that in TypeScript you can have free-floating functions. Please move this to its own file in the lib/private subdirectory of the module, which is the convention we use for module-private APIs like this.

/**
* Represents the properties required to define a GRPC Listener for a VirtualNode.
*/
export class GrpcNodeListener extends VirtualNodeListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - should not be exported.

/**
* Returns the ListenerTimeoutProperty for GRPC protocol
*/
public static renderTimeout(tm: GrpcTimeout): CfnVirtualNode.ListenerTimeoutProperty {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - this is a utility, not a public API of this class.

/**
* Represents the properties required to define a TCP Listener for a VirtualNode.
*/
export class TcpNodeListener extends VirtualNodeListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - should not be exported.

/**
* Returns the ListenerTimeoutProperty for TCP protocol
*/
public static renderTimeout(tm: TcpTimeout): CfnVirtualNode.ListenerTimeoutProperty | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - should be a utility, not a public API of this class.

(BTW, there's a ton of duplication in these. You might want to think about DRYing this whole VirtualNodeListener hierarchy up)

mergify bot pushed a commit that referenced this pull request Oct 29, 2020
----

This is a draft PR to resolve #9533

Takes an approach for creating protocol specific Gateway Routes as described in #10793 

This is a draft as I am seeking feedback on the implementation and approach for creating per protocol variants of App Mesh Resources.

Before merging:

- [x] Approach for per protocol variants defined
- [x] Update Gateway Listeners to follow the same pattern
- [x] Add more integ tests

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot dismissed skinny85’s stale review October 30, 2020 12:46

Pull request has been modified.

@@ -6,3 +6,4 @@ export * from './shared-interfaces';
export * from './virtual-node';
export * from './virtual-router';
export * from './virtual-service';
export * from './virtual-listener';
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 we should rename this to virtual-node-listener. No reason to not be specific

/**
* Returns an HealthCheck for a VirtualNode
*/
function renderHealthCheck(pm: PortMapping, hc: HealthCheck | undefined): CfnVirtualNode.HealthCheckProperty | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be made a protected function of VirtualNodeListener and then you can use the protocol and port of the class instead of having to pass in the PortMapping interface.

/**
* Returns the ListenerTimeoutProperty for HTTP protocol
*/
function renderTimeout(tm: any, pr: Protocol): CfnVirtualNode.ListenerTimeoutProperty {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a protected function of VirtualNodeListener? That way you can just reference the class properties and not pass parameters to this function

Comment on lines 149 to 151
if (listeners.length + this.listeners.length > 1) {
throw new Error('VirtualNode may have at most one listener');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this offline, but wanted to put it here.

We are going to accept the array of listeners without validation. This is to future proof against when we lift the server side validation for a single listener. We do not want to require customers to upgrade their CDK to use multiple listeners

@@ -258,7 +215,7 @@ export class VirtualNode extends VirtualNodeBase {
this.mesh = props.mesh;

this.addBackends(...props.backends || []);
this.addListeners(...props.listener ? [props.listener] : []);
this.addListeners(...props.listener ? [props.listener] : [VirtualNodeListener.httpNodeListener()]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not default to a listener for virtual nodes. It is valid to create a virtual node without a listener. For example, we may have a service that polls SQS to do some work, and therefore would not need a listener

@sshver sshver requested a review from skinny85 November 3, 2020 00:30
@gitpod-io
Copy link

gitpod-io bot commented Nov 3, 2020

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

We're getting closer! A few more comments.

@@ -259,17 +259,33 @@ export class AppMeshExtension extends ServiceExtension {
throw new Error('You must add a CloudMap namespace to the ECS cluster in order to use the AppMesh extension');
}

function addListener(protocol: appmesh.Protocol, port: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please specify the return type explicitly.

@@ -259,17 +259,33 @@ export class AppMeshExtension extends ServiceExtension {
throw new Error('You must add a CloudMap namespace to the ECS cluster in order to use the AppMesh extension');
}

function addListener(protocol: appmesh.Protocol, port: number) {
if (protocol === appmesh.Protocol.HTTP2) {
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 use a switch instead?

switch (protocol) {
  case appmesh.Protocol.HTTP2: return appmesh.VirtualNodeListener.http2NodeListener({ port });
  // ...
}

This way, the compiler will guarantee that all enum cases are covered.

*
* @default - none
*/
readonly idle?: cdk.Duration;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe all these types are used only in the virtual-node-listener.ts file. Can you move them there please?

/**
* Returns an HTTP Listener for a VirtualNode
*/
public static httpNodeListener(props: HttpNodeListenerProps = {}): VirtualNodeListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here as in #11277 (comment). Should we call these http / http2 / grpc / etc.?

/**
* Returns an HTTP2 Listener for a VirtualNode
*/
public static http2NodeListener(props: HttpNodeListenerProps = {}): VirtualNodeListener {
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 move these *Props interfaces above the VirtualNodeListener class please?

Also, we generally call them Props only when they're used for Construct properties. For static methods like these, they should have the suffix 'Options' (so, HttpNodeListenerOptions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These *NodeListenerProps are being used to initialize the constructor of the *NodeListener classes as the static method returns *NodeListener object and the props are used to initialize the constructor. Wouldn't it be okay to call them *Props here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, because VirtualNodeListener is not a construct. We use *Props only for construct properties.

protocol,
path: '/',
},
})],
Copy link
Contributor

Choose a reason for hiding this comment

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

I gotta say, I don't understand why do we need changes to these tests...?

@@ -71,15 +70,12 @@ export = {
meshName: 'test-mesh',
});

const node = mesh.addVirtualNode('test-node', {
new appmesh.VirtualNode(stack, 'test-node', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change...?

packages/@aws-cdk/aws-appmesh/test/test.virtual-node.ts Outdated Show resolved Hide resolved
@@ -275,7 +272,6 @@ export = {
'Fn::GetAtt': ['meshACDFE68E', 'MeshName'],
},
Spec: {
// Specifically: no Listeners and Backends
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this...?

- Removed empty listeners as we will not have a default listener. Listeners can be added to a node after the node is created.
@mergify mergify bot dismissed skinny85’s stale review November 5, 2020 19:50

Pull request has been modified.

- Renamed Interface from *Props to *Options.
@@ -113,19 +273,15 @@ export = {

const node = mesh.addVirtualNode('test-node', {
dnsHostName: 'test.domain.local',
listener: {},
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 have removed the empty listener as we will not have a default listener for a node. It would be valid to have a node without listener and then add it later.

Reference

@sshver sshver requested a review from skinny85 November 6, 2020 18:48
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks good! We're almost there, just needs some finishing touches.

Also make sure to update the PR description, so that it contains all of the breaking changes made in this PR (which is a lot): https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#step-4-commit

accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
});

cdk.Tag.add(node, 'Environment', 'Dev');
```

The listeners property can be left blank and added later with the `node.addListeners()` method. The `healthcheck` property is optional but if specifying a listener, the `portMappings` must contain at least one property.
The listeners property can be left blank and added later with the `node.addListeners()` method. The `healthcheck` and `timeout` properties are optional but if specifying a listener, the `port` must be added.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The listeners property can be left blank and added later with the `node.addListeners()` method. The `healthcheck` and `timeout` properties are optional but if specifying a listener, the `port` must be added.
The `listeners` property can be left blank and added later with the `node.addListener()` method. The `healthcheck` and `timeout` properties are optional but if specifying a listener, the `port` must be added.

/**
* Represents the properties needed to define a Listeners for a VirtualNode
*/
interface VirtualNodeListenerOptions {
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 call this VirtualNodeListenerCommonOptions.

/**
* Represent the HTTP Node Listener prorperty
*/
export interface HttpNodeListenerOptions extends VirtualNodeListenerOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be called HttpVirtualNodeListenerOptions?

/**
* Represent the GRPC Node Listener prorperty
*/
export interface GrpcNodeListenerOptions extends VirtualNodeListenerOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here (GrpcVirtualNodeListenerOptions).

validateHealthChecks(healthCheck);

return healthCheck;
protected readonly listeners = new Array<VirtualNodeListenerConfig>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here? This should be in VirtualNode, right?

@@ -122,69 +113,11 @@ abstract class VirtualNodeBase extends cdk.Resource implements IVirtualNode {
public abstract readonly virtualNodeArn: string;

protected readonly backends = new Array<CfnVirtualNode.BackendProperty>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This field should be moved to VirtualNode, it's not used in this class.

/**
* Add a Virtual Services that this node is expected to send outbound traffic to
*/
public addBackends(...props: IVirtualService[]) {
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 make addBackends() consistent with addListener() (call it addBackend(), make it take a single IVirtualService).

We will need a breaking change in the PR description for this too.

Comment on lines 99 to 100
},
'when a listener is added with timeout': {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be an empty line between tests on the same level:

Suggested change
},
'when a listener is added with timeout': {
},
'when a listener is added with timeout': {

Same comment for all new tests added.

Comment on lines 123 to 124
expect(stack).to(
haveResourceLike('AWS::AppMesh::VirtualNode', {
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 on the same line, because otherwise the indentation is weird:

Suggested change
expect(stack).to(
haveResourceLike('AWS::AppMesh::VirtualNode', {
expect(stack).to(haveResourceLike('AWS::AppMesh::VirtualNode', {

Same comment for all other added tests in this PR.

@mergify mergify bot dismissed skinny85’s stale review November 10, 2020 01:37

Pull request has been modified.

@sshver sshver requested a review from skinny85 November 10, 2020 03:58
skinny85
skinny85 previously approved these changes Nov 10, 2020
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks fantastic @sshver !

@skinny85 skinny85 changed the title feat(aws-appmesh): adds listener timeout to Virtual Nodes feat(appmesh): add listener timeout to Virtual Nodes Nov 10, 2020
@mergify mergify bot dismissed skinny85’s stale review November 10, 2020 20:34

Pull request has been modified.

skinny85
skinny85 previously approved these changes Nov 10, 2020
@mergify mergify bot dismissed skinny85’s stale review November 11, 2020 17:06

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: cf15f6e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants