fix(appmesh): Move Client Policy from Virtual Service to backend structure (#12943)

@sshver:
 
> Client Policies are inherently not related to the Virtual Service. It should be thought of as the client (the VN) telling envoy what connections they want to allow to the server (the Virtual Service). The server shouldn't be the one to define what policies are used to enforce connections with itself.

## Description of changes
I refactored the client policy from Virtual Service to a separate backend structure. This mirrors how our API is designed. Also ran `npm run lint -- --fix` and removed some comments to fix lint warnings.

```ts
/* Old backend defaults */
backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
  certificateChain: 'path-to-certificate',
}),

/* result of this PR */
backendDefaults: {
  clientPolicy: appmesh.ClientPolicy.fileTrust({
    certificateChain: 'path-to-certificate',
  }),
},
```

```ts
/* Old Virtual Service with client policy */
const service1 = new appmesh.VirtualService(stack, 'service-1', {
  virtualServiceName: 'service1.domain.local',
  virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
  clientPolicy: appmesh.ClientPolicy.fileTrust({
    certificateChain: 'path-to-certificate',
    ports: [8080, 8081],
  }),
});

/* result of this PR; client policy is defined in the Virtual Node */
const service1 = new appmesh.VirtualService(stack, 'service-1', {
  virtualServiceName: 'service1.domain.local',
  virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
});

const node = new appmesh.VirtualNode(stack, 'test-node', {
  mesh,
  serviceDiscovery: appmesh.ServiceDiscovery.dns('test'),
});

node.addBackend({
  virtualService: service1,
  clientPolicy: appmesh.ClientPolicy.fileTrust({
    certificateChain: 'path-to-certificate',
    ports: [8080, 8081],
  }),
});
```

BREAKING CHANGE: Backend, backend default and Virtual Service client policies structures are being altered
* **appmesh**: you must use the backend default interface to define backend defaults in `VirtualGateway`.
  The property name also changed from `backendsDefaultClientPolicy` to `backendDefaults`
* **appmesh**:  you must use the backend default interface to define backend defaults in `VirtualNode`,
  (the property name also changed from `backendsDefaultClientPolicy` to `backendDefaults`),
  and the `Backend` class to define a backend
* **appmesh**: you can no longer attach a client policy to a `VirtualService`

Resolves #11996

----

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

Author: alexbrjo

packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/appmesh.ts

@@ -347,7 +347,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(appmesh.Backend.virtualService(otherAppMesh.virtualService));
   }
 
   private routeSpec(weightedTargets: appmesh.WeightedTarget[], serviceName: string): appmesh.RouteSpec {

packages/@aws-cdk/aws-appmesh/README.md

@@ -186,9 +186,11 @@ const node = new VirtualNode(this, 'node', {
       idle: cdk.Duration.seconds(5),
     },
   })],
-  backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
-    certificateChain: '/keys/local_cert_chain.pem',
-  }),
+  backendDefaults: {
+    clientPolicy: appmesh.ClientPolicy.fileTrust({
+      certificateChain: '/keys/local_cert_chain.pem',
+    }),
+  },
   accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
 });
 
@@ -230,14 +232,14 @@ const virtualService = new appmesh.VirtualService(stack, 'service-1', {
   }),
 });
 
-node.addBackend(virtualService);
+node.addBackend(appmesh.Backend.virtualService(virtualService));
 ```
 
 The `listeners` property can be left blank and added later with the `node.addListener()` method. The `healthcheck` and `timeout` properties are optional but if specifying a listener, the `port` must be added.
 
 The `backends` property can be added with `node.addBackend()`. We define a virtual service and add it to the virtual node to allow egress traffic to other node.
 
-The `backendsDefaultClientPolicy` property are added to the node while creating the virtual node. These are virtual node's service backends client policy defaults.
+The `backendDefaults` property are added to the node while creating the virtual node. These are virtual node's default settings for all backends.
 
 ## Adding TLS to a listener
 
@@ -437,10 +439,12 @@ const gateway = new appmesh.VirtualGateway(stack, 'gateway', {
       interval: cdk.Duration.seconds(10),
     },
   })],
-  backendsDefaultClientPolicy: appmesh.ClientPolicy.acmTrust({
-    certificateAuthorities: [acmpca.CertificateAuthority.fromCertificateAuthorityArn(stack, 'certificate', certificateAuthorityArn)],
-    ports: [8080, 8081],
-  }),
+  backendDefaults: {
+    clientPolicy: appmesh.ClientPolicy.acmTrust({
+      certificateAuthorities: [acmpca.CertificateAuthority.fromCertificateAuthorityArn(stack, 'certificate', certificateAuthorityArn)],
+      ports: [8080, 8081],
+    }),
+  },
   accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
   virtualGatewayName: 'virtualGateway',
 });
@@ -464,7 +468,7 @@ const gateway = mesh.addVirtualGateway('gateway', {
 The listeners field can be omitted which will default to an HTTP Listener on port 8080.
 A gateway route can be added using the `gateway.addGatewayRoute()` method.
 
-The `backendsDefaultClientPolicy` property are added to the node while creating the virtual gateway. These are virtual gateway's service backends client policy defaults.
+The `backendDefaults` property is added to the node while creating the virtual gateway. These are virtual gateway's default settings for all backends.
 
 ## Adding a Gateway Route
 

packages/@aws-cdk/aws-appmesh/lib/shared-interfaces.ts

@@ -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
@@ -194,3 +196,80 @@ class FileAccessLog extends AccessLog {
   }
 }
 
+/**
+ * Represents the properties needed to define backend defaults
+ */
+export interface BackendDefaults {
+  /**
+   * Client policy for backend defaults
+   *
+   * @default none
+   */
+  readonly clientPolicy?: ClientPolicy;
+}
+
+/**
+ * Represents the properties needed to define a Virtual Service backend
+ */
+export interface VirtualServiceBackendOptions {
+
+  /**
+   * Client policy for the backend
+   *
+   * @default none
+   */
+  readonly clientPolicy?: ClientPolicy;
+}
+
+/**
+ * Properties for a backend
+ */
+export interface BackendConfig {
+  /**
+   * Config for a Virtual Service backend
+   */
+  readonly virtualServiceBackend: CfnVirtualNode.BackendProperty;
+}
+
+
+/**
+ * Contains static factory methods to create backends
+ */
+export abstract class Backend {
+  /**
+   * Construct a Virtual Service backend
+   */
+  public static virtualService(virtualService: IVirtualService, props: VirtualServiceBackendOptions = {}): Backend {
+    return new VirtualServiceBackend(virtualService, props.clientPolicy);
+  }
+
+  /**
+   * Return backend config
+   */
+  public abstract bind(_scope: Construct): BackendConfig;
+}
+
+/**
+ * Represents the properties needed to define a Virtual Service backend
+ */
+class VirtualServiceBackend extends Backend {
+
+  constructor (private readonly virtualService: IVirtualService,
+    private readonly clientPolicy: ClientPolicy | undefined) {
+    super();
+  }
+
+  /**
+   * Return config for a Virtual Service backend
+   */
+  public bind(_scope: Construct): BackendConfig {
+    return {
+      virtualServiceBackend: {
+        virtualService: {
+          virtualServiceName: this.virtualService.virtualServiceName,
+          clientPolicy: this.clientPolicy?.bind(_scope).clientPolicy,
+        },
+      },
+    };
+  }
+}

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

@@ -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';
 
 /**
@@ -66,7 +65,7 @@ export interface VirtualGatewayBaseProps {
    *
    * @default - No Config
    */
-  readonly backendsDefaultClientPolicy?: ClientPolicy;
+  readonly backendDefaults?: BackendDefaults;
 }
 
 /**
@@ -180,7 +179,11 @@ 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,
         logging: accessLogging !== undefined ? {
           accessLog: accessLogging.virtualGatewayAccessLog,
         } : undefined,

packages/@aws-cdk/aws-appmesh/lib/virtual-node.ts

@@ -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
@@ -61,7 +59,7 @@ export interface VirtualNodeBaseProps {
    *
    * @default - No backends
    */
-  readonly backends?: IVirtualService[];
+  readonly backends?: Backend[];
 
   /**
    * Initial listener for the virtual node
@@ -82,7 +80,7 @@ export interface VirtualNodeBaseProps {
    *
    * @default - No Config
    */
-  readonly backendsDefaultClientPolicy?: ClientPolicy;
+  readonly backendDefaults?: BackendDefaults;
 }
 
 /**
@@ -185,7 +183,11 @@ 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,
         serviceDiscovery: {
           dns: serviceDiscovery?.dns,
           awsCloudMap: serviceDiscovery?.cloudmap,
@@ -214,13 +216,8 @@ export class VirtualNode extends VirtualNodeBase {
   /**
    * Add a Virtual Services that this node is expected to send outbound traffic to
    */
-  public addBackend(virtualService: IVirtualService) {
-    this.backends.push({
-      virtualService: {
-        virtualServiceName: virtualService.virtualServiceName,
-        clientPolicy: virtualService.clientPolicy?.bind(this).clientPolicy,
-      },
-    });
+  public addBackend(backend: Backend) {
+    this.backends.push(backend.bind(this).virtualServiceBackend);
   }
 }
 

packages/@aws-cdk/aws-appmesh/lib/virtual-service.ts

@@ -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';
@@ -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;
 }
 
 /**
@@ -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
    */
@@ -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`,
@@ -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;
 
@@ -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;
 }
 
 /**

packages/@aws-cdk/aws-appmesh/test/integ.mesh.ts

@@ -36,16 +36,15 @@ const node = mesh.addVirtualNode('node', {
       path: '/check-path',
     },
   })],
-  backends: [
-    virtualService,
-  ],
+  backends: [appmesh.Backend.virtualService(virtualService)],
 });
 
-node.addBackend(new appmesh.VirtualService(stack, 'service-2', {
-  virtualServiceName: 'service2.domain.local',
-  virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
-}),
-);
+node.addBackend(appmesh.Backend.virtualService(
+  new appmesh.VirtualService(stack, 'service-2', {
+    virtualServiceName: 'service2.domain.local',
+    virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
+  }),
+));
 
 router.addRoute('route-1', {
   routeSpec: appmesh.RouteSpec.http({
@@ -78,15 +77,17 @@ const node2 = mesh.addVirtualNode('node2', {
       unhealthyThreshold: 2,
     },
   })],
-  backendsDefaultClientPolicy: appmesh.ClientPolicy.fileTrust({
-    certificateChain: 'path/to/cert',
-  }),
-  backends: [
+  backendDefaults: {
+    clientPolicy: appmesh.ClientPolicy.fileTrust({
+      certificateChain: 'path/to/cert',
+    }),
+  },
+  backends: [appmesh.Backend.virtualService(
     new appmesh.VirtualService(stack, 'service-3', {
       virtualServiceName: 'service3.domain.local',
       virtualServiceProvider: appmesh.VirtualServiceProvider.none(mesh),
     }),
-  ],
+  )],
 });
 
 const node3 = mesh.addVirtualNode('node3', {
@@ -102,9 +103,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'),
 });
 

packages/@aws-cdk/aws-appmesh/test/test.health-check.ts

@@ -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();