[@aws-cdk/aws-appmesh] AppMesh Stable Changes

[MrArnoldPalmer]

AppMesh API Revisions

1. Currently VirtualNode defaults the value of the logging file path parameter to /dev/stdout and is not configurable through the construct's props. Remove this default and allow customers to configure log location for their specific use case.

2. Change addListeners method signature to not be variadic since VirtualNode only has one listener. Depending on Feature Request: Add support for multiple listeners on Virtual Node and Virtual Router aws-app-mesh-roadmap#120 we may or may not actually want to change this API.

3. Change model so that VirtualService always uses mesh from either router or node and requires mesh property to exist on VirtualRouter and VirtualNode for importing. VirtualNode and VirtualRouter always have to be in same mesh as VirtualService.

---

This is 🐛 Bug Report

[dfezzie]

There has been work on this issue so I am providing an update

1. This is now configurable and no longer defaulted
2. This is the intended behavior. App Mesh has a feature request to support multiple listeners:
3. VirtualService is required to have the mesh property when importing. The proposal to ensure the targets of the VS are in the same mesh is valid. This could also be applied for the backends of VirtualNodes and targets of VirtualGateway routes.

[skinny85]

For 2., we actually did change addListeners() - we renamed it addListener(), moved it to VirtualNode (from IVirtualNode), and it also now takes a single VirtualNodeListener as the argument. So I think we can call that "done" 🙂.

[skinny85]

For 3., with your recent changes @dfezzie , both IVirtualRouter and IVirtualNode now allow retrieving the Mesh they belong to. Given that, I wonder if forcing providing the same Mesh again when creating a new VirtualService (not importing one, as providing a Mesh there makes perfect sense) is a good experience.

Additionally, I'm not crazy about the "one of virtualRouter or virtualNode is required" when creating a new VirtualService. Perhaps this is another place to use the union-like pattern? @dfezzie what do you think?

[dfezzie]

Ah good call on 2. That is complete 😄

For 3, I think adding that enforcement is valid and would prevent customers misconfiguring their mesh. If it makes sense in terms of CDK API design we could easily implement it.

I agree on the switching the VirtualService to using a union type for the provider. I just took a quick look and we are also doing something similar for VirtualNode service discovery where we accept CloudMap and DNS at the top level. We should change that as well!

[skinny85]

Before I forget, if we're going to make the changes to VirtualService to use a union-like class, I would also remove the addVirtualService() method from IMesh, and probably add addVirtualService() to both IVirtualRouter and IVirtualNode.

[dfezzie]

Before I forget, if we're going to make the changes to VirtualService to use a union-like class, I would also remove the addVirtualService() method from IMesh, and probably add addVirtualService() to both IVirtualRouter and IVirtualNode.

I don't think removing the addVirtualService() from IMesh and adding it to IVirtualRouter and IVirtualNode is necessary. You can define a IVirtualService without a provider and you can have IVirtualRouter and IVirtualNode without an associated IVirtualService. I think simply adding the union-class would suffice to make the class more ergonomic.

[skinny85]

You can define a IVirtualService without a provider

Hmm, I thought providing one of VirtualNode or VirtualRouter was actually required. This complicates things a tiny bit then 😜.

How about we have 3 options in the union-like class for the provider in IVirtualService: one taking a IVirtualNode, one taking a IVirtualRouter, and the third one just taking in an IMesh?

This way, we can have the addVirtualService() method on all 3 of IMesh, IVirtualNode and IVirtualRouter.

How does this sound @dfezzie ? 🙂

[dfezzie]

You can define a IVirtualService without a provider

Hmm, I thought providing one of VirtualNode or VirtualRouter was actually required. This complicates things a tiny bit then 😜.

How about we have 3 options in the union-like class for the provider in IVirtualService: one taking a IVirtualNode, one taking a IVirtualRouter, and the third one just taking in an IMesh?

This way, we can have the addVirtualService() method on all 3 of IMesh, IVirtualNode and IVirtualRouter.

How does this sound @dfezzie ? 🙂

So, I've been thinking about this. I think having addVirtualService methods on IVirtualNode and IVirtualRouter is semantically awkward. I like to think of a Virtual Service as the abstraction of the "Service" that other teams would interact with. You would start with a Virtual Service as the entry point into a microservice infrastructure and that would not change. The provider behind it on the other hand would be mutable as you may swap from one Virtual Node to another, or add routes to your Virtual Router.

[skinny85]

@dfezzie I have no problem with that. It just seemed like an easy set of methods that we could get basically for free. But if they don't fit the model of the service, that's fine 🙂.

Does the same comment apply to addVirtualService() in IMesh, or is that different?

[dfezzie]

Adding a Virtual Service to the mesh is appropriate. The mesh is a container for the other subresources so it makes sense to have the utility method there

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.