feat(appmesh): add Virtual Gateways and Gateway Routes #10879

dfezzie · 2020-10-14T22:18:15Z

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:

Approach for per protocol variants defined
Update Gateway Listeners to follow the same pattern
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

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

skinny85

This is quite a doozy, thanks for submitting it @dfezzie!

Not sure I got through the entire PR, but I think I had enough comments to get us started 🙂.

skinny85 · 2020-10-15T00:09:55Z

packages/@aws-cdk/aws-appmesh/lib/utils.ts

@@ -0,0 +1,41 @@
+import * as cdk from '@aws-cdk/core';


We settled on a convention that these sort of module-private utilities would go to a subdirectory of lib called private - this way, it's immediately obvious this is not part of the public API of the module.

Example of how it's used in CodePipeline.

packages/@aws-cdk/aws-appmesh/lib/virtual-node.ts

skinny85 · 2020-10-15T00:28:31Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

+  /**
+   * What protocol the route uses
+   */
+  readonly routeType: Protocol.GRPC | Protocol.HTTP | Protocol.HTTP2;


I wonder - is the reason the type is not simply Protocol because there's no TCP Gateway?

Yeah, I wanted to reuse Protocol for this, but scope it down to not include TCP. Would you suggest creating a separate enum?

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

skinny85 · 2020-10-15T00:35:00Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

+ *
+ * @see https://docs.aws.amazon.com/app-mesh/latest/userguide/gateway-routes.html
+ */
+export class GatewayGrpcRoute extends GatewayRoute {


The fact that these are empty suggests to me the modeling is not quite right here 🙂

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

skinny85 · 2020-10-15T00:55:02Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

+
+    if (props.mesh) {
+      this.mesh = props.mesh;
+    } else if (props.meshName) {


I wouldn't allow passing meshName here - instead, require an IMesh, and the customer can call Mesh.fromMeshName() here directly. I think supplying both options adds more complication that it's worth.

Will address in separate PR

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

gitpod-io · 2020-10-16T21:05:46Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

skinny85

We're getting there - nice work @dfezzie !

skinny85 · 2020-10-21T21:25:33Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

+  /**
+   * The VirtualGateway the GatewayRoute belongs to
+   *
+   * @attribute


I think @attribute should not be here...? (These are only for CloudFormation attributes)

I was following the other resources which have them present in the other IResource interface.

Looks like lambda is doing this too: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-lambda/lib/function-base.ts#L19

Right. Lambda is doing it for functionName, which is an attribute 🙂.

I don't have any problems with gatewayRouteName and gatewayRouteArn being @attributes in IGatewayRoute. I just don't think virtualGateway should be one.

ahh got it. I thought you meant having the attribute tag in general

@attribute is still here 🙂

@attribute is still here 🙂🙂

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

skinny85 · 2020-10-21T21:43:58Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

+/**
+* Represents the properties needed to define a GRPC Listener for Virtual Gateway
+*/
+export class GrpcGatewayListener extends VirtualGatewayListener {


Same here - make this class module-private?

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

dfezzie · 2020-10-27T17:44:46Z

Looks great! Some minor comments (mainly styling) below.

Make sure to also update the "Breaking changes" section of the PR, as a lot of stuff is being changed (I'm also fine if you'd rather do these from*() method changes in a separate PR).

I ended up reverting the from*() method commit. I'll pull it into a separate PR once this is merged!

skinny85

We're getting closer. I would still like to see a simplified version of the imported classes, and I'm very confused about VirtualGateway accepting multiple listeners, but then having validation that you can have only one...

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/gateway-route-spec.ts

skinny85 · 2020-10-27T18:09:32Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

+   * Utility method to add a single listener to this VirtualGateway
+   */
+  public addListener(listener: VirtualGatewayListener) {
+    if (this.listeners.length > 0) {


How does this work for imported VirtualGateways?

I think this method should only be defined on VirtualGateway, not on IVirtualGateway?

I went ahead and removed these methods. We can add them back later!

skinny85 · 2020-10-27T18:11:12Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

+   *
+   * @default - Single HTTP listener on port 8080
+   */
+  readonly listeners?: VirtualGatewayListener[];


Considering the condition checked below that a VirtualGateway can only have a single listener, why is this an array?

We plan to support multiple listeners in the future

So you have to decide between one of two options here.

Because you only support a single listener right now, change this property to readonly listener: VirtualGatewayListener, and we'll add an additional property later that allows you to specify multiple ones.

Leave this as readonly listeners: VirtualGatewayListener[], but remove the validation below that checks there's only one, so that when you lift this limitation on the server side, CDK customers will be able to take advantage of it without the need to upgrade their CDK version.

Right now, this is a "worst of both worlds" kind of situation: you force providing an array, but don't allow specifying more than one because of the validation baked into the class. So the array serves no purpose from what I can see.

I'm leaving the decision to you which of the above options you want to go with 🙂.

I'm leaning more towards option 2.

Option 1 would require customers having to think about two separate fields, listener and listeners when creating a gateway/virtual node.

Option 2 has the side affect of requiring server side validation, but you raise a good point on CDK versioning. I would prefer customers have immediate access when we change the server side validation.

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

packages/@aws-cdk/aws-appmesh/package.json

… rendering as part of abstract class

Pull request has been modified.

dfezzie · 2020-10-28T04:54:22Z

We're getting closer. I would still like to see a simplified version of the imported classes, and I'm very confused about VirtualGateway accepting multiple listeners, but then having validation that you can have only one...

We plan on supporting multiple listeners in the future: aws/aws-app-mesh-roadmap#120

I went ahead and removed the addListeners methods since you cannot create a Virtual Gateway without a listener and since you cannot add more than one, those methods are useless.

skinny85

We're an inch away 🙂. I want to give a better story around the whole "one listener today" - "many listeners in the future" scenario, because I'm not a huge fan of how this is handled in the current revision. But otherwise, looks awesome!

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway-listener.ts

skinny85 · 2020-10-28T23:23:48Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

+  /**
+   * The VirtualGateway the GatewayRoute belongs to
+   *
+   * @attribute


@attribute is still here 🙂🙂

skinny85 · 2020-10-28T23:34:48Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

+   *
+   * @default - Single HTTP listener on port 8080
+   */
+  readonly listeners?: VirtualGatewayListener[];


So you have to decide between one of two options here.

Because you only support a single listener right now, change this property to readonly listener: VirtualGatewayListener, and we'll add an additional property later that allows you to specify multiple ones.

Leave this as readonly listeners: VirtualGatewayListener[], but remove the validation below that checks there's only one, so that when you lift this limitation on the server side, CDK customers will be able to take advantage of it without the need to upgrade their CDK version.

Right now, this is a "worst of both worlds" kind of situation: you force providing an array, but don't allow specifying more than one because of the validation baked into the class. So the array serves no purpose from what I can see.

I'm leaving the decision to you which of the above options you want to go with 🙂.

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

Pull request has been modified.

skinny85

Awesome - thanks @dfezzie !

The PR build is currently broken for unrelated reasons, but we'll get this merged as soon as that's fixed.

Thanks for your patience with this change, and with me 😊

Pull request has been modified.

aws-cdk-automation · 2020-10-29T21:49:14Z

AWS CodeBuild CI Report

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

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

mergify · 2020-10-29T21:49:41Z

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

…butes() (#11266) Follow Up from #10879 BREAKING CHANGE: all `fromResourceName()` methods in the AppMesh module have been replaced with `fromResourceAttributes()` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…11277) Follows the pattern set in #10879 and transitions the VirtualRouter listener to use protocol specific listeners. BREAKING CHANGE: VirtualRouter's Listeners are no longer a struct; use the static factory methods of the `VirtualNodeListener` class to obtain instances of them * **appmesh:** VirtualRouter accepts a list of listeners instead of a single listener ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Dominic Fezzie added 5 commits September 30, 2020 15:34

Initial VGW commit

5cab38c

Update with upstream changes

71ac7dd

Add initial round of tests

0cf538a

Fixes a bug with default paths on health checks

1849af9

Implements Gateway Routes

4e26790

dfezzie commented Oct 14, 2020

View reviewed changes

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts Outdated Show resolved Hide resolved

Adds routeType to GatewayRouteBaseProps

9c7dc48

skinny85 previously requested changes Oct 15, 2020

View reviewed changes

SomayaB changed the title ~~feat(module): adds Virtual Gateways and Gateway Routes~~ feat(appmesh): adds Virtual Gateways and Gateway Routes Oct 15, 2020

SomayaB assigned skinny85 Oct 15, 2020

Update packages/@aws-cdk/aws-appmesh/lib/virtual-node.ts

0d47976

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Dominic Fezzie added 2 commits October 16, 2020 12:51

Reworks Gateway Routes to use bind classes for protocol spec variants

463f507

Merge branch 'master' into gateway

860a40a

Dominic Fezzie added 3 commits October 16, 2020 14:06

Adds addListener to VGW

d2c59bb

Adds gateway listener protocol variant classes

4976d8f

Adds addListener method to IVirtualGateway

ce2f325

dfezzie commented Oct 16, 2020

View reviewed changes

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts Outdated Show resolved Hide resolved

dfezzie requested a review from skinny85 October 16, 2020 23:16

Dominic Fezzie added 5 commits October 20, 2020 20:28

Merge branch 'master' into gateway

45690a9

Fixes some build issues

2939e5c

adds utility method to add multiple gateway routes to a VirtualGateway

a242bfd

adds more tests to gateway routes

a97a5c5

adds integ tests for gateway routes

0016549

dfezzie marked this pull request as ready for review October 21, 2020 06:45

Dominic Fezzie added 2 commits October 21, 2020 14:39

Updates README and adds tests for adding multiple gateway routes

5a54ab5

Merge branch 'master' into gateway

2b36223

skinny85 previously requested changes Oct 21, 2020

View reviewed changes

Apply suggestions from code review

ca1d0b3

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

dfezzie requested a review from skinny85 October 26, 2020 22:42

skinny85 previously requested changes Oct 27, 2020

View reviewed changes

Dominic Fezzie added 3 commits October 27, 2020 21:15

addresses smaller changes from review

4e432c2

moves gateway listener classes to separate file and adds health check…

e550d53

… rendering as part of abstract class

removes addListener() methods from virtual gateways

7813bbc

removes the ListenerBase interface

8670dbd

Merge branch 'master' into gateway

c01eb4c

dfezzie requested a review from skinny85 October 28, 2020 17:50

skinny85 previously requested changes Oct 28, 2020

View reviewed changes

remove validation for only having 1 listener on virtual gateways

5efc195

Merge branch 'master' into gateway

1eda686

dfezzie requested a review from skinny85 October 29, 2020 19:12

skinny85 previously approved these changes Oct 29, 2020

View reviewed changes

skinny85 changed the title ~~feat(appmesh): adds Virtual Gateways and Gateway Routes~~ feat(appmesh): add Virtual Gateways and Gateway Routes Oct 29, 2020

Merge branch 'master' into gateway

519fc23

skinny85 approved these changes Oct 29, 2020

View reviewed changes

mergify bot merged commit 79200e7 into aws:master Oct 29, 2020

This was referenced Nov 3, 2020

feat(appmesh): remove from*Name() methods and replace with from*Attributes() #11266

Merged

feat(appmesh): change VirtualRouter's Listener to a union-like class #11277

Merged

sshver mentioned this pull request Nov 17, 2020

Add listener TLS certificates for VirtualNodes and VirtualGateways alexbrjo/aws-cdk#1

Merged

dfezzie mentioned this pull request Nov 18, 2020

feat(appmesh): change Route's spec to a union-like class #11343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appmesh): add Virtual Gateways and Gateway Routes #10879

feat(appmesh): add Virtual Gateways and Gateway Routes #10879

dfezzie commented Oct 14, 2020 •

edited

Loading

skinny85 left a comment

skinny85 Oct 15, 2020

skinny85 Oct 15, 2020

dfezzie Oct 15, 2020

skinny85 Oct 15, 2020

skinny85 Oct 15, 2020

dfezzie Oct 26, 2020

gitpod-io bot commented Oct 16, 2020 •

edited

Loading

skinny85 left a comment

skinny85 Oct 21, 2020

dfezzie Oct 21, 2020

skinny85 Oct 21, 2020

dfezzie Oct 21, 2020

skinny85 Oct 27, 2020

skinny85 Oct 28, 2020

dfezzie Oct 29, 2020

skinny85 Oct 21, 2020

dfezzie commented Oct 27, 2020

skinny85 left a comment

skinny85 Oct 27, 2020

dfezzie Oct 28, 2020

skinny85 Oct 27, 2020

dfezzie Oct 28, 2020

skinny85 Oct 28, 2020 •

edited

Loading

dfezzie Oct 29, 2020

dfezzie commented Oct 28, 2020

skinny85 left a comment

skinny85 Oct 28, 2020

skinny85 Oct 28, 2020 •

edited

Loading

skinny85 left a comment

aws-cdk-automation commented Oct 29, 2020

mergify bot commented Oct 29, 2020

feat(appmesh): add Virtual Gateways and Gateway Routes #10879

feat(appmesh): add Virtual Gateways and Gateway Routes #10879

Conversation

dfezzie commented Oct 14, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitpod-io bot commented Oct 16, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfezzie commented Oct 27, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfezzie commented Oct 28, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 29, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 29, 2020

dfezzie commented Oct 14, 2020 •

edited

Loading

gitpod-io bot commented Oct 16, 2020 •

edited

Loading

skinny85 Oct 28, 2020 •

edited

Loading

skinny85 Oct 28, 2020 •

edited

Loading