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

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

Merged
merged 20 commits into from
Jan 28, 2021
Merged
Show file tree
Hide file tree
Changes from 13 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
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,7 @@ export class AppMeshExtension extends ServiceExtension {
// Now create a virtual service. Relationship goes like this:
// virtual service -> virtual router -> virtual node
this.virtualService = new appmesh.VirtualService(this.scope, `${this.parentService.id}-virtual-service`, {
mesh: this.mesh,
virtualRouter: this.virtualRouter,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(this.virtualRouter),
virtualServiceName: serviceName,
});
}
Expand Down
6 changes: 2 additions & 4 deletions packages/@aws-cdk/aws-appmesh/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,22 +110,20 @@ Adding a virtual router as the provider:

```ts
mesh.addVirtualService('virtual-service', {
virtualRouter: router,
virtualServiceName: 'my-service.default.svc.cluster.local',
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualRouter(router),
});
```

Adding a virtual node as the provider:

```ts
mesh.addVirtualService('virtual-service', {
virtualNode: node,
virtualServiceName: `my-service.default.svc.cluster.local`,
virtualServiceProvider: appmesh.VirtualServiceProvider.virtualNode(node),
});
```

**Note** that only one must of `virtualNode` or `virtualRouter` must be chosen.

## Adding a VirtualNode

A `virtual node` acts as a logical pointer to a particular task group, such as an Amazon ECS service or a Kubernetes deployment.
Expand Down
13 changes: 9 additions & 4 deletions packages/@aws-cdk/aws-appmesh/lib/mesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CfnMesh } from './appmesh.generated';
import { VirtualGateway, VirtualGatewayBaseProps } from './virtual-gateway';
import { VirtualNode, VirtualNodeBaseProps } from './virtual-node';
import { VirtualRouter, VirtualRouterBaseProps } from './virtual-router';
import { VirtualService, VirtualServiceBaseProps } from './virtual-service';
import { VirtualService, VirtualServiceProps, VirtualServiceProvider } from './virtual-service';

/**
* A utility enum defined for the egressFilter type property, the default of DROP_ALL,
Expand Down Expand Up @@ -49,7 +49,7 @@ export interface IMesh extends cdk.IResource {
/**
* Adds a VirtualService with the given id
*/
addVirtualService(id: string, props?: VirtualServiceBaseProps): VirtualService;
addVirtualService(id: string, props?: VirtualServiceProps): VirtualService;

/**
* Adds a VirtualNode to the Mesh
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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),
});
}

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

Copy link
Contributor

@skinny85 skinny85 Jan 25, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

id: string, props: VirtualServiceProps = { virtualServiceProvider: VirtualServiceProvider.none(this) }): VirtualService {
if (props) {
if (props.virtualServiceProvider.mesh != this) {
throw new Error('Virtual Service provider must belong to mesh');
}
}
return new VirtualService(this, id, {
...props,
mesh: this,
});
}

Expand Down
164 changes: 111 additions & 53 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export interface IVirtualService extends cdk.IResource {
/**
* The base properties which all classes in VirtualService will inherit from
*/
export interface VirtualServiceBaseProps {
export interface VirtualServiceProps {
/**
* The name of the VirtualService.
*
Expand All @@ -51,18 +51,9 @@ export interface VirtualServiceBaseProps {
readonly virtualServiceName?: string;

/**
* The VirtualRouter which the VirtualService uses as provider
*
* @default - At most one of virtualRouter and virtualNode is allowed.
*/
readonly virtualRouter?: IVirtualRouter;

/**
* The VirtualNode attached to the virtual service
*
* @default - At most one of virtualRouter and virtualNode is allowed.
* The VirtualNode or VirtualRouter which the VirtualService uses as its provider
*/
readonly virtualNode?: IVirtualNode;
readonly virtualServiceProvider: VirtualServiceProvider;

/**
* Client policy for this Virtual Service
Expand All @@ -72,16 +63,6 @@ export interface VirtualServiceBaseProps {
readonly clientPolicy?: ClientPolicy;
}

/**
* The properties applied to the VirtualService being define
*/
export interface VirtualServiceProps extends VirtualServiceBaseProps {
/**
* The Mesh which the VirtualService belongs to
*/
readonly mesh: IMesh;
}

dfezzie marked this conversation as resolved.
Show resolved Hide resolved
/**
* VirtualService represents a service inside an AppMesh
*
Expand Down Expand Up @@ -135,59 +116,39 @@ export class VirtualService extends cdk.Resource implements IVirtualService {

public readonly clientPolicy?: ClientPolicy;

private readonly virtualServiceProvider?: CfnVirtualService.VirtualServiceProviderProperty;

constructor(scope: Construct, id: string, props: VirtualServiceProps) {
super(scope, id, {
physicalName: props.virtualServiceName || cdk.Lazy.string({ produce: () => cdk.Names.uniqueId(this) }),
});

if (props.virtualNode && props.virtualRouter) {
throw new Error('Must provide only one of virtualNode or virtualRouter for the provider');
}

this.mesh = props.mesh;
this.clientPolicy = props.clientPolicy;
const provider = props.virtualServiceProvider?.bind(this);
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
this.mesh = provider.mesh;

// Check which provider to use node or router (or neither)
if (props.virtualRouter) {
this.virtualServiceProvider = this.addVirtualRouter(props.virtualRouter.virtualRouterName);
}
if (props.virtualNode) {
this.virtualServiceProvider = this.addVirtualNode(props.virtualNode.virtualNodeName);
if (provider.mesh != this.mesh) {
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(`VirtualService ${this.physicalName} and the provider must be in the same Mesh`);
}

const svc = new CfnVirtualService(this, 'Resource', {
meshName: this.mesh.meshName,
virtualServiceName: this.physicalName,
spec: {
provider: this.virtualServiceProvider,
provider: provider.virtualNodeProvider || provider.virtualRouterProvider
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
? {
virtualNode: provider?.virtualNodeProvider,
virtualRouter: provider?.virtualRouterProvider,
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
}
: undefined,
},
});

this.virtualServiceName = this.getResourceNameAttribute(svc.attrVirtualServiceName);
this.virtualServiceArn = this.getResourceArnAttribute(svc.ref, {
service: 'appmesh',
resource: `mesh/${props.mesh.meshName}/virtualService`,
resource: `mesh/${this.mesh.meshName}/virtualService`,
resourceName: this.physicalName,
});
}

private addVirtualRouter(name: string): CfnVirtualService.VirtualServiceProviderProperty {
return {
virtualRouter: {
virtualRouterName: name,
},
};
}

private addVirtualNode(name: string): CfnVirtualService.VirtualServiceProviderProperty {
return {
virtualNode: {
virtualNodeName: name,
},
};
}
}

/**
Expand All @@ -211,3 +172,100 @@ export interface VirtualServiceAttributes {
*/
readonly clientPolicy?: ClientPolicy;
}

/**
* Properties for a VirtualService provider
*/
export interface VirtualServiceProviderConfig {
/**
* Virtual Node based provider
*
* @default - none
*/
readonly virtualNodeProvider?: CfnVirtualService.VirtualNodeServiceProviderProperty;

/**
* Virtual Router based provider
*
* @default - none
*/
readonly virtualRouterProvider?: CfnVirtualService.VirtualRouterServiceProviderProperty;
skinny85 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Mesh the Provider is using
*
* @default - none
*/
readonly mesh: IMesh;
}

/**
* Represents the properties needed to define the provider for a VirtualService
*/
export abstract class VirtualServiceProvider {
/**
* Returns a VirtualNode based Provider for a VirtualService
*/
public static virtualNode(virtualNode: IVirtualNode): VirtualServiceProvider {
return new VirtualServiceProviderImpl(virtualNode, undefined);
}

/**
* Returns a VirtualRouter based Provider for a VirtualService
*/
public static virtualRouter(virtualRouter: IVirtualRouter): VirtualServiceProvider {
return new VirtualServiceProviderImpl(undefined, virtualRouter);
}

skinny85 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Returns an Empty Provider for a VirtualService. This provides no routing capabilities
* and should only be used as a placeholder
*/
public static none(mesh: IMesh): VirtualServiceProvider {
return new VirtualServiceProviderImpl(undefined, undefined, mesh);
}

/**
* Mesh the provider belongs to
*/
public abstract readonly mesh: IMesh;
dfezzie marked this conversation as resolved.
Show resolved Hide resolved

/**
* Enforces mutual exclusivity for VirtualService provider types.
*/
public abstract bind(_construct: Construct): VirtualServiceProviderConfig;
}

class VirtualServiceProviderImpl extends VirtualServiceProvider {
private readonly virtualNode?: IVirtualNode;
private readonly virtualRouter?: IVirtualRouter;
public readonly mesh: IMesh;
dfezzie marked this conversation as resolved.
Show resolved Hide resolved

constructor(virtualNode?: IVirtualNode, virtualRouter?: IVirtualRouter, mesh?: IMesh) {
super();
this.virtualNode = virtualNode;
this.virtualRouter = virtualRouter;
const providedMesh = this.virtualNode?.mesh || this.virtualRouter?.mesh || mesh;
// Only occurs if VirtualNode or VirtualRouter is not provided during construction
if (!providedMesh) {
dfezzie marked this conversation as resolved.
Show resolved Hide resolved
throw new Error('Mesh property not defined for VirtualServiceProviderImpl');
}
this.mesh = providedMesh;
}

public bind(_construct: Construct): VirtualServiceProviderConfig {
return {
mesh: this.mesh,
virtualNodeProvider: this.virtualNode
? {
virtualNodeName: this.virtualNode.virtualNodeName,
}
: undefined,
virtualRouterProvider: this.virtualRouter
? {
virtualRouterName: this.virtualRouter.virtualRouterName,
}
: undefined,
};
}
}
25 changes: 2 additions & 23 deletions packages/@aws-cdk/aws-appmesh/test/integ.mesh.expected.json
Original file line number Diff line number Diff line change
Expand Up @@ -649,17 +649,6 @@
"VirtualServiceName": "service1.domain.local"
}
},
"cert56CA94EB": {
"Type": "AWS::CertificateManager::Certificate",
"Properties": {
"DomainName":"node1.domain.local",
"DomainValidationOptions": [{
"DomainName":"node1.domain.local",
"ValidationDomain":"local"
}],
"ValidationMethod": "EMAIL"
}
},
"meshnode726C787D": {
"Type": "AWS::AppMesh::VirtualNode",
"Properties": {
Expand Down Expand Up @@ -706,16 +695,6 @@
"PortMapping": {
"Port": 8080,
"Protocol": "http"
},
"TLS": {
"Certificate": {
"ACM": {
"CertificateArn": {
"Ref": "cert56CA94EB"
}
}
},
"Mode": "STRICT"
}
}
],
Expand Down Expand Up @@ -743,8 +722,8 @@
"TLS": {
"Validation": {
"Trust": {
"ACM": {
"CertificateAuthorityArns": ["arn:aws:acm-pca:us-east-1:123456789012:certificate-authority/12345678-1234-1234-1234-123456789012"]
"File": {
"CertificateChain": "path/to/cert"
}
}
}
Expand Down
Loading