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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions packages/@aws-cdk/aws-appmesh/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -196,16 +196,19 @@ const node = new appmesh.VirtualNode(this, 'node', {
cdk.Tags.of(node).add('Environment', 'Dev');
```

Create a `VirtualNode` with the constructor and add backend virtual service.
Create a `VirtualNode` with the constructor, specifying a backend, and adding another backend virtual service later.

```ts
declare const mesh: appmesh.Mesh;
declare const router: appmesh.VirtualRouter;
declare const service: cloudmap.Service;

const virtualServiceName1 = 'service1.domain.local';
const virtualServiceName2 = 'service2.domain.local';
const node = new appmesh.VirtualNode(this, 'node', {
mesh,
serviceDiscovery: appmesh.ServiceDiscovery.cloudMap(service),
backends: [appmesh.Backend.virtualServiceName(virtualServiceName1)],
listeners: [appmesh.VirtualNodeListener.http({
port: 8080,
healthCheck: appmesh.HealthCheck.http({
Expand All @@ -222,17 +225,21 @@ const node = new appmesh.VirtualNode(this, 'node', {
accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
});

const virtualService = new appmesh.VirtualService(this, 'service-1', {
const virtualService1 = new appmesh.VirtualService(this, 'service-1', {
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(router),
virtualServiceName: 'service1.domain.local',
virtualServiceName: virtualServiceName1,
});
const virtualService2 = new appmesh.VirtualService(this, 'service-2', {
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(router),
virtualServiceName: virtualServiceName2,
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 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.

});

node.addBackend(appmesh.Backend.virtualService(virtualService));
wplucinsky marked this conversation as resolved.
Show resolved Hide resolved
node.addBackend(appmesh.Backend.virtualServiceName(virtualServiceName2));
```

The `listeners` property can be left blank and added later with the `node.addListener()` method. The `serviceDiscovery` property must be specified when specifying a listener.

The `backends` property can be added with `node.addBackend()`. In the example, we define a virtual service and add it to the virtual node to allow egress traffic to other nodes.
wplucinsky marked this conversation as resolved.
Show resolved Hide resolved
The `backends` property can be added in the constructor or with `node.addBackend()`. In the example, we define two virtual services and add one to the virtual node in the constructor and one via `addBackend` to allow egress traffic to other nodes.

The `backendDefaults` property is added to the node while creating the virtual node. These are the virtual node's default settings for all backends.

Expand Down
36 changes: 36 additions & 0 deletions packages/@aws-cdk/aws-appmesh/lib/shared-interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,13 @@ export abstract class Backend {
return new VirtualServiceBackend(virtualService, props.tlsClientPolicy);
}

/**
* Construct a Virtual Service backend via a Virtual Service name
*/
public static virtualServiceName(virtualServiceName: string, props: VirtualServiceBackendOptions = {}): Backend {
return new VirtualServiceNameBackend(virtualServiceName, props.tlsClientPolicy);
}
Comment on lines +221 to +223
Copy link
Contributor

@skinny85 skinny85 Dec 6, 2021

Choose a reason for hiding this comment

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

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

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

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


/**
* Return backend config
*/
Expand Down Expand Up @@ -250,6 +257,35 @@ class VirtualServiceBackend extends Backend {
}
}

/**
* Represents the properties needed to define a Virtual Service backend via a Virtual Service name
*/
class VirtualServiceNameBackend extends Backend {

constructor (private readonly virtualServiceName: string,
private readonly tlsClientPolicy: TlsClientPolicy | undefined) {
super();
}
Comment on lines +265 to +268
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
constructor (private readonly virtualServiceName: string,
private readonly tlsClientPolicy: TlsClientPolicy | undefined) {
super();
}
constructor(
private readonly virtualServiceName: string,
private readonly tlsClientPolicy: TlsClientPolicy | undefined,
) {
super();
}


/**
* Return config for a Virtual Service backend
*/
public bind(scope: Construct): BackendConfig {
return {
virtualServiceBackend: {
virtualService: {
virtualServiceName: this.virtualServiceName,
clientPolicy: this.tlsClientPolicy
? {
tls: renderTlsClientPolicy(scope, this.tlsClientPolicy),
}
: undefined,
},
},
};
}
}

/**
* Connection pool properties for HTTP listeners
*/
Expand Down
45 changes: 45 additions & 0 deletions packages/@aws-cdk/aws-appmesh/test/virtual-node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,53 @@ describe('virtual node', () => {
},
MeshOwner: ABSENT,
});
});

test('should add resource with service backends via virtual service names', () => {
// GIVEN
const stack = new cdk.Stack();

// WHEN
const mesh = new appmesh.Mesh(stack, 'mesh', {
meshName: 'test-mesh',
});

const service1Name = 'service1.domain.local';
new appmesh.VirtualService(stack, 'service-1', {
virtualServiceName: service1Name,
virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
});
const service2Name = 'service2.domain.local';
new appmesh.VirtualService(stack, 'service-2', {
virtualServiceName: service2Name,
virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
});

const node = new appmesh.VirtualNode(stack, 'test-node', {
mesh,
serviceDiscovery: appmesh.ServiceDiscovery.dns('test'),
backends: [appmesh.Backend.virtualServiceName(service1Name)],
});

node.addBackend(appmesh.Backend.virtualServiceName(service2Name));

// THEN
expect(stack).toHaveResourceLike('AWS::AppMesh::VirtualNode', {
Spec: {
Backends: [
{
VirtualService: {
VirtualServiceName: service1Name,
},
},
{
VirtualService: {
VirtualServiceName: service2Name,
},
},
],
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

});
});

Expand Down