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): Move Client Policy from Virtual Service to backend structure #12943

Merged
merged 12 commits into from
Mar 12, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ export class AppMeshExtension extends ServiceExtension {
// Next update the app mesh config so that the local Envoy
// proxy on this service knows how to route traffic to
// nodes from the other service.
this.virtualNode.addBackend(otherAppMesh.virtualService);
this.virtualNode.addBackend({ virtualService: otherAppMesh.virtualService });
}

private routeSpec(weightedTargets: appmesh.WeightedTarget[], serviceName: string): appmesh.RouteSpec {
Expand Down
31 changes: 31 additions & 0 deletions packages/@aws-cdk/aws-appmesh/lib/shared-interfaces.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import * as cdk from '@aws-cdk/core';
import { CfnVirtualGateway, CfnVirtualNode } from './appmesh.generated';
import { ClientPolicy } from './client-policy';
import { IVirtualService } from './virtual-service';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
Expand Down Expand Up @@ -194,3 +196,32 @@ class FileAccessLog extends AccessLog {
}
}

/**
* Represents the properties needed to define a backend
*/
export interface Backend {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still partial to leaving this as a bind() class. We could have other backend types in the future

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 couldn't imagine any fundamentally different outbound traffic (other backend types). I could only think of additional handling logic for the outbound traffic (client side throttling), which would be implemented by adding properties to the backend.

Also the backends property in the API is a list and I don't think that leaves room to create other backend types in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

The backend property is not a list though. We could easily introduce a different type here in the future, such as using a Virtual Gateway as a target or some sort of egress type.

/**
* The Virtual Service this backend points to
*/
readonly virtualService: IVirtualService;

/**
* Client policy for a backend
*
* @default none
*/
readonly clientPolicy?: ClientPolicy;
}

/**
* Represents the properties needed to define backend defaults
*/
export interface BackendDefaults {
/**
* Client policy for backend defaults
*
* @default none
*/
readonly clientPolicy?: ClientPolicy;
}

9 changes: 5 additions & 4 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import * as cdk from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnVirtualGateway } from './appmesh.generated';
import { ClientPolicy } from './client-policy';
import { GatewayRoute, GatewayRouteBaseProps } from './gateway-route';
import { IMesh, Mesh } from './mesh';
import { AccessLog } from './shared-interfaces';
import { AccessLog, BackendDefaults } from './shared-interfaces';
import { VirtualGatewayListener, VirtualGatewayListenerConfig } from './virtual-gateway-listener';

/**
Expand Down Expand Up @@ -66,7 +65,7 @@ export interface VirtualGatewayBaseProps {
*
* @default - No Config
*/
readonly backendsDefaultClientPolicy?: ClientPolicy;
readonly backendDefaults?: BackendDefaults;
}

/**
Expand Down Expand Up @@ -180,7 +179,9 @@ export class VirtualGateway extends VirtualGatewayBase {
meshName: this.mesh.meshName,
spec: {
listeners: this.listeners.map(listener => listener.listener),
backendDefaults: props.backendsDefaultClientPolicy?.bind(this),
backendDefaults: props.backendDefaults !== undefined ? {
clientPolicy: props.backendDefaults?.clientPolicy?.bind(this).clientPolicy,
} : undefined,
alexbrjo marked this conversation as resolved.
Show resolved Hide resolved
logging: accessLogging !== undefined ? {
accessLog: accessLogging.virtualGatewayAccessLog,
} : undefined,
Expand Down
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-node.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import * as cdk from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnVirtualNode } from './appmesh.generated';
import { ClientPolicy } from './client-policy';
import { IMesh, Mesh } from './mesh';
import { ServiceDiscovery } from './service-discovery';
import { AccessLog } from './shared-interfaces';
import { AccessLog, BackendDefaults, Backend } from './shared-interfaces';
import { VirtualNodeListener, VirtualNodeListenerConfig } from './virtual-node-listener';
import { IVirtualService } from './virtual-service';

/**
* Interface which all VirtualNode based classes must implement
Expand Down Expand Up @@ -61,7 +59,7 @@ export interface VirtualNodeBaseProps {
*
* @default - No backends
*/
readonly backends?: IVirtualService[];
readonly backends?: Backend[];

/**
* Initial listener for the virtual node
Expand All @@ -82,7 +80,7 @@ export interface VirtualNodeBaseProps {
*
* @default - No Config
*/
readonly backendsDefaultClientPolicy?: ClientPolicy;
readonly backendDefaults?: BackendDefaults;
}

/**
Expand Down Expand Up @@ -185,7 +183,9 @@ export class VirtualNode extends VirtualNodeBase {
spec: {
backends: cdk.Lazy.anyValue({ produce: () => this.backends }, { omitEmptyArray: true }),
listeners: cdk.Lazy.anyValue({ produce: () => this.listeners.map(listener => listener.listener) }, { omitEmptyArray: true }),
backendDefaults: props.backendsDefaultClientPolicy?.bind(this),
backendDefaults: props.backendDefaults !== undefined ? {
clientPolicy: props.backendDefaults?.clientPolicy?.bind(this).clientPolicy,
} : undefined,
alexbrjo marked this conversation as resolved.
Show resolved Hide resolved
serviceDiscovery: {
dns: serviceDiscovery?.dns,
awsCloudMap: serviceDiscovery?.cloudmap,
Expand Down Expand Up @@ -214,11 +214,11 @@ export class VirtualNode extends VirtualNodeBase {
/**
* Add a Virtual Services that this node is expected to send outbound traffic to
*/
public addBackend(virtualService: IVirtualService) {
public addBackend(backend: Backend) {
this.backends.push({
virtualService: {
virtualServiceName: virtualService.virtualServiceName,
clientPolicy: virtualService.clientPolicy?.bind(this).clientPolicy,
virtualServiceName: backend.virtualService.virtualServiceName,
clientPolicy: backend.clientPolicy?.bind(this).clientPolicy,
},
});
}
Expand Down
24 changes: 0 additions & 24 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import * as cdk from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnVirtualService } from './appmesh.generated';
import { ClientPolicy } from './client-policy';
import { IMesh, Mesh } from './mesh';
import { IVirtualNode } from './virtual-node';
import { IVirtualRouter } from './virtual-router';
Expand All @@ -28,11 +27,6 @@ export interface IVirtualService extends cdk.IResource {
* The Mesh which the VirtualService belongs to
*/
readonly mesh: IMesh;

/**
* Client policy for this Virtual Service
*/
readonly clientPolicy?: ClientPolicy;
}

/**
Expand All @@ -50,13 +44,6 @@ export interface VirtualServiceProps {
*/
readonly virtualServiceName?: string;

/**
* Client policy for this Virtual Service
*
* @default - none
*/
readonly clientPolicy?: ClientPolicy;

/**
* The VirtualNode or VirtualRouter which the VirtualService uses as its provider
*/
Expand Down Expand Up @@ -90,7 +77,6 @@ export class VirtualService extends cdk.Resource implements IVirtualService {
return new class extends cdk.Resource implements IVirtualService {
readonly virtualServiceName = attrs.virtualServiceName;
readonly mesh = attrs.mesh;
readonly clientPolicy = attrs.clientPolicy;
readonly virtualServiceArn = cdk.Stack.of(this).formatArn({
service: 'appmesh',
resource: `mesh/${attrs.mesh.meshName}/virtualService`,
Expand All @@ -114,14 +100,11 @@ export class VirtualService extends cdk.Resource implements IVirtualService {
*/
public readonly mesh: IMesh;

public readonly clientPolicy?: ClientPolicy;

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

this.clientPolicy = props.clientPolicy;
const providerConfig = props.virtualServiceProvider.bind(this);
this.mesh = providerConfig.mesh;

Expand Down Expand Up @@ -160,13 +143,6 @@ export interface VirtualServiceAttributes {
* The Mesh which the VirtualService belongs to
*/
readonly mesh: IMesh;

/**
* Client policy for this Virtual Service
*
* @default - none
*/
readonly clientPolicy?: ClientPolicy;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-appmesh/test/integ.mesh.expected.json
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@
"VirtualService": {
"VirtualServiceName": {
"Fn::GetAtt": [
"service27C65CF7D",
alexbrjo marked this conversation as resolved.
Show resolved Hide resolved
"service2B2862B6B",
"VirtualServiceName"
]
}
Expand Down Expand Up @@ -711,7 +711,7 @@
"VirtualService": {
"VirtualServiceName": {
"Fn::GetAtt": [
"service3859EB104",
"service3529E2EB1",
"VirtualServiceName"
]
}
Expand Down Expand Up @@ -969,7 +969,7 @@
"VirtualServiceName": "service1.domain.local"
}
},
"service27C65CF7D": {
"service2B2862B6B": {
"Type": "AWS::AppMesh::VirtualService",
"Properties": {
"MeshName": {
Expand All @@ -982,7 +982,7 @@
"VirtualServiceName": "service2.domain.local"
}
},
"service3859EB104": {
"service3529E2EB1": {
"Type": "AWS::AppMesh::VirtualService",
"Properties": {
"MeshName": {
Expand Down
45 changes: 29 additions & 16 deletions packages/@aws-cdk/aws-appmesh/test/integ.mesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ const virtualService = new appmesh.VirtualService(stack, 'service', {
virtualServiceName: 'service1.domain.local',
});

const virtualService2 = new appmesh.VirtualService(stack, 'service2', {
virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
virtualServiceName: 'service2.domain.local',
});

const virtualService3 = new appmesh.VirtualService(stack, 'service3', {
virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
virtualServiceName: 'service3.domain.local',
});

const node = mesh.addVirtualNode('node', {
serviceDiscovery: appmesh.ServiceDiscovery.dns(`node1.${namespace.namespaceName}`),
listeners: [appmesh.VirtualNodeListener.http({
Expand All @@ -37,15 +47,15 @@ const node = mesh.addVirtualNode('node', {
},
})],
backends: [
virtualService,
{
virtualService: virtualService,
},
],
});

node.addBackend(new appmesh.VirtualService(stack, 'service-2', {
virtualServiceName: 'service2.domain.local',
virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
}),
);
node.addBackend({
virtualService: virtualService2,
});

router.addRoute('route-1', {
routeSpec: appmesh.RouteSpec.http({
Expand Down Expand Up @@ -78,14 +88,15 @@ const node2 = mesh.addVirtualNode('node2', {
unhealthyThreshold: 2,
},
})],
backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
certificateChain: 'path/to/cert',
}),
backends: [
new appmesh.VirtualService(stack, 'service-3', {
virtualServiceName: 'service3.domain.local',
virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
backendDefaults: {
clientPolicy: appmesh.ClientPolicy.fileTrust({
certificateChain: 'path/to/cert',
}),
},
backends: [
{
virtualService: virtualService3,
},
],
});

Expand All @@ -102,9 +113,11 @@ const node3 = mesh.addVirtualNode('node3', {
unhealthyThreshold: 2,
},
})],
backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
certificateChain: 'path-to-certificate',
}),
backendDefaults: {
clientPolicy: appmesh.ClientPolicy.fileTrust({
certificateChain: 'path-to-certificate',
}),
},
accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
});

Expand Down
2 changes: 0 additions & 2 deletions packages/@aws-cdk/aws-appmesh/test/test.health-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ export = {
// THEN
test.doesNotThrow(() => toThrow(min));
test.doesNotThrow(() => toThrow(max));
// falsy, falls back to portMapping.port
// test.throws(() => toThrow(min - 1), /below the minimum threshold/);
test.throws(() => toThrow(max + 1), /above the maximum threshold/);

test.done();
Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-appmesh/test/test.mesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,9 @@ export = {
port: 8080,
})],
backends: [
service1,
{
virtualService: service1,
},
],
});

Expand Down
8 changes: 5 additions & 3 deletions packages/@aws-cdk/aws-appmesh/test/test.virtual-gateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,11 @@ export = {
new appmesh.VirtualGateway(stack, 'virtual-gateway', {
virtualGatewayName: 'virtual-gateway',
mesh: mesh,
backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
certificateChain: 'path-to-certificate',
}),
backendDefaults: {
clientPolicy: appmesh.ClientPolicy.fileTrust({
certificateChain: 'path-to-certificate',
}),
},
});

// THEN
Expand Down
Loading