fix(appmesh): allow the virtualServiceName field for virtual node backends to be supplied as a string

[wplucinsky]

Addresses a circular dependency issue where the virtual service must be created before it could be added as a backend to a virtual node, which then did not allow that virtual service to set that virtual node as its provider. The App Mesh API does not have a restriction that the virtual service is created before the backend is registered so now there is the ability to supply just the virtual service name when creating a backend.

Fixes #17322

Example of the new possibility:

const stack = new cdk.Stack();
const mesh = new appmesh.Mesh(stack, 'mesh', {
  meshName: 'test-mesh',
});
const virtualServiceName = 'service.domain.local';
const node = new appmesh.VirtualNode(stack, 'node', {
  mesh,
  serviceDiscovery: appmesh.ServiceDiscovery.dns('example.com'),
  backends: [appmesh.Backend.virtualServiceName(virtualServiceName)],
});
new appmesh.VirtualService(stack, 'service', {
  virtualServiceName,
  virtualServiceProvider: appmesh.VirtualServiceProvider.virtualNode(node),
});

---

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

[gitpod-io]

[Seiya6329]

Overall looks great! Few comments on README changes.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: bc5e553
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[Seiya6329]

Looks great! Let's wait for Adam's feedback as well especially since we are making the changes to READMe file.

[skinny85]

Thanks for the contribution @wplucinsky! I'd prefer if we solved this problem in a different way, my proposal is in the PR comments.

[skinny85]

Can you actually create a separate example, explaining what problem does the new method in Backend solve? It probably deserves its own section, honestly. Let's leave this example unchanged.

[skinny85]

Suggested change

	constructor (private readonly virtualServiceName: string,
	private readonly tlsClientPolicy: TlsClientPolicy | undefined) {
	super();
	}
	constructor(
	private readonly virtualServiceName: string,
	private readonly tlsClientPolicy: TlsClientPolicy | undefined,
	) {
	super();
	}

[skinny85]

I don't love this API. In my opinion, it's kind of confusing, easy to misuse (for example, what if I do Backend.virtualServiceName(virtualService.virtualServiceName)? This will still not work), does not work with our automatic VirtualService naming, and the docs do not explain at all what problem does this method solve.

I'd prefer if we added a new property to IVirtualService, let's call it virtualServicePhysicalName, and fill it with the simple string name (not with a Token value that results in { Ref: 'VirtualServiceLogicalId' } being rendered in the template). Then, change the API here to take an IVirtualService instead of a string.

Let's also make sure to explain in the JSDocs of this method how is it different than Backend.virtualService().

[wplucinsky]

I believe using Backend.virtualServiceName(virtualService.virtualServiceName) would offer the same functionality that currently exists, but would not add the dependency automatically. The user would be required to specify that the Virtual Node depends on that Virtual Service.

Won't the token value still be needed if the new physicalName field is added? The virtualServiceName is currently an optional field in IVirtualService, but required by the App Mesh service. Therefore it, and the physcialName field, will be have a fallback to be generated using cdk.Lazy.string({ produce: () => cdk.Names.uniqueId(this) }).

[skinny85]

I believe using Backend.virtualServiceName(virtualService.virtualServiceName) would offer the same functionality that currently exists, but would not add the dependency automatically. The user would be required to specify that the Virtual Node depends on that Virtual Service.

No, it would not work (which makes sense, because virtualService.virtualServiceName is exactly how the current Backend.virtualService() method is implemented - so, that would mean this new Backend.virtualServiceName() method is not needed!). That's because virtualService.virtualServiceName results in a { 'Fn::Get': 'VirtualServiceLogicalId.VirtualServiceName' } being rendered in the resulting CloudFormation template, and that creates a dependency order (the resource with { 'Fn::Get': 'VirtualServiceLogicalId.VirtualServiceName' } will have a dependency on VirtualServiceLogicalId). So, the cycle will still happen.

Won't the token value still be needed if the new physicalName field is added? The virtualServiceName is currently an optional field in IVirtualService, but required by the App Mesh service. Therefore it, and the physcialName field, will be have a fallback to be generated using cdk.Lazy.string({ produce: () => cdk.Names.uniqueId(this) }).

I was a little imprecise with my wording. What I meant is that virtualServicePhysicalName is also a Token, but one that doesn't resolve to a CloudFormation expression like virtualServiceName does, but a Token that resolves to a specific string - the name of the VirtualService (either assigned by the customer using the virtualServiceName property when creating the VirtualService, or generated by the CDK).

[skinny85]

I don't think this unit test correctly reflects the problem we're trying to solve here (the cycle between VirtualService and VirtualNode).

[skinny85]

Actually, can you add an integ test that shows the usage of the new Backend.virtualServiceName() method? I want to confirm it deploys correctly (that the dependency from the Virtual Service to the Virtual Node is in fact not needed).

[wplucinsky]

New PR: #18265