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

dfezzie · 2020-12-10T07:17:20Z

BREAKING CHANGE: the properties virtualRouter and virtualNode of VirtualServiceProps have been replaced with the union-like class VirtualServiceProvider

appmesh: the method addVirtualService has been removed from IMesh

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

gitpod-io · 2020-12-10T07:17:23Z

skinny85

Looks great @dfezzie ! A few comments here, mainly related to this comment that I made: #9490 (comment) .

Please also add addVirtualService() methods to both IVirtualNode and IVirtualRouter.

Also, I'm fairly sure that's the last point that's left of #9490, so I would add Fixes #9490 to the PR description 🙂.

skinny85 · 2020-12-10T20:18:40Z

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

- * @default - At most one of virtualRouter and virtualNode is allowed.
- */
- readonly virtualNode?: IVirtualNode;
+ readonly virtualServiceProvider?: VirtualServiceProvider;


2 changes here:

Let's move this property to VirtualServiceProps below.

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

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

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.

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.

The provider should be required, of course 🙂.

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.

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

Co-authored-by: Adam Ruka <adamruka@amazon.com>

Pull request has been modified.

skinny85

Putting in "Request changes" to clear this from my ToDo list. @dfezzie make sure to re-request my review when this is ready!

dfezzie · 2020-12-15T17:50:26Z

Putting in "Request changes" to clear this from my ToDo list. @dfezzie make sure to re-request my review when this is ready!

I ended up getting sidetracked and haven't addressed all the feedback quite yet

skinny85 · 2020-12-15T19:07:18Z

Putting in "Request changes" to clear this from my ToDo list. @dfezzie make sure to re-request my review when this is ready!

I ended up getting sidetracked and haven't addressed all the feedback quite yet

No worries, it's all good 🙂

dfezzie · 2021-01-21T05:18:20Z

Sorry for the long delay. Holidays snuck up on me and then this PR fell off my radar 😅

I've provided a comment on #9490 about the changes requested. Depending on your response there, we may be able to call this PR good to go.

Pull request has been modified.

skinny85

@dfezzie I have no problems with not having addService() methods in IVirtualNode and IVirtualRouter. If that's the case, I think you might want to get rid of the existing addVirtualService() method in IMesh as well...?

Having said that, I do feel strongly that requiring the user to provide the Mesh twice when creating a Virtual Service with a provider is a mistake (as both a VirtualNode and VirtualRouter already have a Mesh). It's just poor usability.

If you're very opposed to my idea of having a third none() VirtualServiceProvider, let's at least make IMesh an optional argument when creating a VirtualService, and we will not not require it when a provider has been given. How does that sound?

Pull request has been modified.

… provider mesh

dfezzie · 2021-01-22T07:41:50Z

I've added the none() method to the class. Gives the customer flexibility to define a Virtual Service without having to provide a provider out of the gate.

I'm having issues deploying the integ test to my own account, seeing

1 validation error detected: Value 'local' at 'domainValidationOptions.1.member.validationDomain' failed to satisfy constraint: Member must satisfy regular expression pattern: ^(\*\.)?(((?!-)[A-Za-z0-9-]{0,62}[A-Za-z0-9])\.)+((?!-)[A-Za-z0-9-]{1,62}[A-Za-z0-9])$ (Service: AWSCertificateManager

Not sure if there's a bug with the deprecated props in acm.Certificate or something weird with my setup. I'll dig into it a bit more tomorrow if the build here doesn't pass

skinny85 · 2021-01-22T15:06:22Z

@dfezzie for deploying the integ.mesh.ts integ test, maybe reach out to @alexbrjo ? I think he was the last person to modify them.

dfezzie · 2021-01-22T22:53:36Z

I had to remove ACMPCA and ACM from the integ tests as App Mesh does validate the existence of the resources, and as a result you cannot just import from a fake ARN. I think that should fix the last of the issues with the build.

dfezzie · 2021-01-22T23:00:17Z

packages/@aws-cdk/aws-appmesh/test/integ.mesh.ts

- backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
- certificateChain: 'path-to-certificate',
- }),


Not sure when I removed this. Will add back.

skinny85

Looks almost perfect @dfezzie! Just a tiny change in Mesh, and this will be ready to be shipped.

skinny85 · 2021-01-25T19:32:11Z

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

@@ -89,10 +89,15 @@ abstract class MeshBase extends cdk.Resource implements IMesh {
 /**
 * Adds a VirtualService with the given id
 */
- public addVirtualService(id: string, props: VirtualServiceBaseProps = {}): VirtualService {
+ public addVirtualService(


Sorry, I don't understand these changes 😕.

The whole point of having addVirtualService() on Mesh is so that you don't have to provide the Mesh again. But with your changes, you can do something like:

const mesh = new appmesh.Mesh(...); mesh.addVirtualService({ virtualServiceProvider.none(mesh), });

which just doesn't make sense to me.

I would revert all of these changes here, and this method should be:

Suggested change

public addVirtualService(

public addVirtualService(id: string, props: VirtualServiceBaseProps = {}): VirtualService {

return new VirtualService(this, id, {

...props,

virtualServiceProvider: VirtualServiceProvider.none(this),

});

}

I guess I was thinking of it more as a utility method, a quick way to add a new Virtual Service to your mesh, similar to how we add other resources like Virtual Nodes. If you cannot specify a provider, then essentially the method is useless as it will never produce a usable Virtual Service. If you created a VS as a placeholder and then went to add a provider you would have to remove it from the addVirtualService method into a new VirtualService(...) call. How it is here, you could have addVirtualService('VirtualService') in one synthesized stack which uses the none method as the default. Later, you could come along and add a different provider if you so choose.

We can remove the method all together if you feel it does not fit well.

I guess I was thinking of it more as a utility method, a quick way to add a new Virtual Service to your mesh

Right, but if you want to specify any property of the created Service, then it doesn't really save anything, right?

mesh.addVirtualService('Service', { virtualServiceName: 'MyService', provider: VirtualServiceProvider.none(mesh), });

Doesn't save anything over

new VirtualService(this, 'Service', { virtualServiceName: 'MyService', provider: VirtualServiceProvider.none(mesh), });

I think it doesn't make too much sense to over-emphasize the "add a VirtualService to the Mesh with only default properties" use case - it seems to just lead to an awkward API.

, similar to how we add other resources like Virtual Nodes.

Notice that all other methods in the AppMesh module like addVirtualNode(), addVirtualGateway(), etc. do not allow you to specify the main owning resource, like the Mesh, again.

I don't think this inconsistency is worth it. I would either not allow passing virtualServiceProvider in addVirtualService(), or get rid of this method completely, whichever you prefer (if you go with the later, don't forget to add a BREAKING CHANGE note to the PR description).

For now, let's remove it. You're right that there is no real difference between the two method calls

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

packages/@aws-cdk/aws-appmesh/test/test.virtual-service.ts

Pull request has been modified.

dfezzie · 2021-01-25T23:24:11Z

Addressed a few comments, waiting for a response before making any final updates 👍

…irtualServiceProvider class

packages/@aws-cdk/aws-appmesh/test/test.virtual-service.ts

skinny85

Looks great! Couple of cosmetic changes before we merge this in.

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

Pull request has been modified.

skinny85

Looks great @dfezzie!

mergify · 2021-01-27T23:07:09Z

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 · 2021-01-27T23:43:48Z

AWS CodeBuild CI Report

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

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

mergify · 2021-01-27T23:44:14Z

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

feat(appmesh): changes VirtualService provider to a union-like class

bec6a09

github-actions bot added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Dec 10, 2020

github-actions bot assigned skinny85 Dec 10, 2020

skinny85 previously requested changes Dec 10, 2020

View reviewed changes

Apply suggestions from code review

b824a40

Co-authored-by: Adam Ruka <adamruka@amazon.com>

skinny85 changed the title ~~feat(appmesh): changes VirtualService provider to a union-like class~~ feat(appmesh): change VirtualService provider to a union-like class Dec 15, 2020

enforce provider and VS in same mesh

a717a44

skinny85 previously requested changes Dec 15, 2020

View reviewed changes

Merge branch 'master' into refactor/virtualServiceTarget

c46bdb8

dfezzie requested a review from skinny85 January 21, 2021 05:24

skinny85 previously requested changes Jan 21, 2021

View reviewed changes

Dominic Fezzie added 2 commits January 21, 2021 21:34

Merge branch 'master' into refactor/virtualServiceTarget

db7c5a2

Update to require a provider

9f99c47

Dominic Fezzie added 3 commits January 21, 2021 22:23

Remove construct reference with qualifier

be05561

Update appmesh ecs addon

fb2dcda

Add back VirtualServiceProps and enforce Virtual Service mesh matches…

cb566fc

… provider mesh

Dominic Fezzie added 2 commits January 22, 2021 14:10

Update integ test to use imported ACM cert

56fb779

Remove ACM and ACM PCA from integ tests

ae5052e

Merge branch 'master' into refactor/virtualServiceTarget

adcf421

dfezzie requested a review from skinny85 January 22, 2021 22:56

dfezzie commented Jan 22, 2021

View reviewed changes

Add back some integ code I removed for some reason

e43c332

skinny85 previously requested changes Jan 25, 2021

View reviewed changes

Addresses PR comments

e678b9e

dfezzie requested a review from skinny85 January 25, 2021 23:16

Remove addVirtualService method from IMesh and IMesh attribute from V…

14aef5e

…irtualServiceProvider class

dfezzie commented Jan 26, 2021

View reviewed changes

packages/@aws-cdk/aws-appmesh/test/test.virtual-service.ts Show resolved Hide resolved

Deploy integ test

a37c6db

skinny85 previously requested changes Jan 26, 2021

View reviewed changes

Dominic Fezzie added 2 commits January 27, 2021 14:23

Minor cosmetic changes

ec39db1

Update VirtualServiceProps docs

f6afe7b

dfezzie requested a review from skinny85 January 27, 2021 22:25

Remove Mesh.addVirtualService() from ReadMe

575ded5

skinny85 approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into refactor/virtualServiceTarget

93fb617

nija-at changed the title ~~feat(appmesh): change VirtualService provider to a union-like class~~ feat(appmesh): change VirtualService provider to a union-like clas Jan 28, 2021

nija-at changed the title ~~feat(appmesh): change VirtualService provider to a union-like clas~~ feat(appmesh): change VirtualService provider to a union-like class Jan 28, 2021

mergify bot merged commit dfc765a into aws:master Jan 28, 2021

dfezzie deleted the refactor/virtualServiceTarget branch January 28, 2021 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dfezzie commented Dec 10, 2020 •

edited by skinny85

Loading

gitpod-io bot commented Dec 10, 2020 •

edited

Loading

skinny85 left a comment

skinny85 Dec 10, 2020

dfezzie Dec 15, 2020

skinny85 Dec 15, 2020

dfezzie Jan 21, 2021

skinny85 left a comment

dfezzie commented Dec 15, 2020

skinny85 commented Dec 15, 2020

dfezzie commented Jan 21, 2021

skinny85 left a comment

dfezzie commented Jan 22, 2021

skinny85 commented Jan 22, 2021

dfezzie commented Jan 22, 2021

dfezzie Jan 22, 2021

skinny85 left a comment

skinny85 Jan 25, 2021

dfezzie Jan 25, 2021

skinny85 Jan 25, 2021 •

edited

Loading

dfezzie Jan 26, 2021

dfezzie commented Jan 25, 2021

skinny85 left a comment

skinny85 left a comment

mergify bot commented Jan 27, 2021

aws-cdk-automation commented Jan 27, 2021

mergify bot commented Jan 27, 2021

- public addVirtualService(
+ public addVirtualService(id: string, props: VirtualServiceBaseProps = {}): VirtualService {
+ return new VirtualService(this, id, {
+ ...props,
+ virtualServiceProvider: VirtualServiceProvider.none(this),
+ });
+ }

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

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

Conversation

dfezzie commented Dec 10, 2020 • edited by skinny85 Loading

gitpod-io bot commented Dec 10, 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

skinny85 left a comment

Choose a reason for hiding this comment

dfezzie commented Dec 15, 2020

skinny85 commented Dec 15, 2020

dfezzie commented Jan 21, 2021

skinny85 left a comment

Choose a reason for hiding this comment

dfezzie commented Jan 22, 2021

skinny85 commented Jan 22, 2021

dfezzie commented Jan 22, 2021

Choose a reason for hiding this comment

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

skinny85 Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfezzie commented Jan 25, 2021

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 27, 2021

aws-cdk-automation commented Jan 27, 2021

AWS CodeBuild CI Report

mergify bot commented Jan 27, 2021

dfezzie commented Dec 10, 2020 •

edited by skinny85

Loading

gitpod-io bot commented Dec 10, 2020 •

edited

Loading

skinny85 Jan 25, 2021 •

edited

Loading