feat(appmesh): add ClientPolicy to VirtualNode, VirtualGateway and VirtualService #11563

sshver · 2020-11-18T23:46:43Z

Adds backend defaults to Virtual Node and Virtual Gateways.
Adds validation context for the backend defined on the Virtual Node.

Before merging (TODO):

Update README

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

…faults

…hapes that support validation context

…faults

gitpod-io · 2020-11-18T23:46:46Z

skinny85

Looks good @sshver ! Some initial comments / questions.

skinny85 · 2020-11-19T00:42:31Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+/**
+ * TLS Connections with downstream server will always be enforced if True
+ */
+export interface TLSClientPolicyOptions {


This should be called TlsClientPolicyOptions.

skinny85 · 2020-11-19T00:42:54Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+  /**
+   * TLS enforced if True.
+   *
+   * @default - True


Suggested change

* @default - True

* @default true

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

skinny85 · 2020-11-19T00:43:47Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+/**
+ * ACM Trust Properties
+ */
+export interface ACMTrustOptions {


Suggested change

export interface ACMTrustOptions {

export interface AcmTrustOptions {

skinny85 · 2020-11-19T00:44:46Z

packages/@aws-cdk/aws-appmesh/lib/validation-context.ts

+/**
+ * Defines the TLS validation context trust.
+ */
+export abstract class TLSValidationContext {


Suggested change

export abstract class TLSValidationContext {

export abstract class TlsValidationContext {

skinny85 · 2020-11-19T00:49:55Z

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

@@ -151,6 +159,7 @@ export class VirtualGateway extends VirtualGatewayBase {
  public readonly mesh: IMesh;

  protected readonly listeners = new Array<VirtualGatewayListenerConfig>();
+  private readonly backendDefaults: CfnVirtualGateway.VirtualGatewayBackendDefaultsProperty;


Why does this need to be a field? It's only set and read in the constructor of the class?

I have updated the PR to use an array here. This gives me an option set omitEmptyArray true and this would remove backendDefault if not defined.

skinny85 · 2020-11-19T00:51:51Z

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

@@ -166,13 +175,20 @@ export class VirtualGateway extends VirtualGatewayBase {
      props.listeners.forEach(listener => this.listeners.push(listener.bind(this)));
    }

+    if (props.backendDefaults) {
+      this.backendDefaults = this.addBackendDefaults(props.backendDefaults);


I don't think this is a good method name. add should only be used if there can be multiples of the thing added. Since this doesn't seem to be the case here, I would go for something like setBackendDefaults.

Updated the function name to setBackendDefaults

skinny85 · 2020-11-19T00:56:29Z

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

+      clientPolicy: {
+        tls: {
+          enforce: clientPolicy.tlsClientPolicy.enforce ?? true,
+          validation: clientPolicy.tlsClientPolicy.validation.bind(this).tlsValidation,


Why is the tslValidation set in bind(), but the rest are properties of the ClientPolicy? Wondering why the different approach.

Earlier, I was trying to separate out the validation context as it could be used in future for Listener TLS.
But now I have a single file client-policy.ts and now the clientPolicy properties are being set in bind().

This would make the API more intuitive.

new appmesh.VirtualGateway(stack, 'virtual-gateway', { virtualGatewayName: 'virtual-gateway', mesh: mesh, backendDefaults: appmesh.ClientPolicy.fileTrust({ certificateChain: ['path-to-certificate'], enforceTls: false, }), });

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

skinny85 · 2020-11-19T00:59:02Z

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

@@ -92,6 +99,13 @@ const node3 = mesh.addVirtualNode('node3', {
      unhealthyThreshold: 2,
    },
  })],
+  backendDefaults: {
+    tlsClientPolicy: {
+      validation: appmesh.TLSValidationContext.fileTrust({


We're testing fileTrust twice here. Should we use a different static factory method for the second time?

2. Updated bind() to return client policy

Pull request has been modified.

…faults

skinny85

Looks good @sshver , but I still have some questions 🙂

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

skinny85 · 2020-11-24T17:53:08Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+  /**
+   * Amazon Resource Names (ARN) of trusted ACM Private Certificate Authorities
+   */
+  readonly certificateAuthorityArns: string[];


Should these be L2 resources, instead of strings?

If L2s for these don't exist, we should add them.

Added a skeleton L2 for ACMPCA.

skinny85 · 2020-11-24T17:53:24Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+  /**
+   * Path to the Certificate Chain file on the file system where the Envoy is deployed.
+   */
+  readonly certificateChain: string[];


Why is this an array...?

Updated to string

skinny85 · 2020-11-24T17:55:38Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+}
+
+class ClientPolicyImpl extends ClientPolicy {
+  constructor (private readonly enforce: boolean = true,


No point in having the default argument here.

Removed the default arguments

skinny85 · 2020-11-24T17:56:05Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+    };
+  }
+
+  private renderValidation(): CfnVirtualNode.TlsValidationContextProperty {


Just inline this method, I don't see what value it adds.

Removed the method renderValidation and added inline.

skinny85 · 2020-11-24T18:12:15Z

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

      },
    });
  }
 }

+


Unnecessary empty line.

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

skinny85 · 2020-11-24T18:19:59Z

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

+  /**
+   * Client policy for Virtual Service
+   */


You don't need the second comment if the property is coming from an interface.

skinny85 · 2020-11-24T18:20:58Z

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

@@ -96,6 +100,10 @@ const node3 = mesh.addVirtualNode('node3', {
      unhealthyThreshold: 2,
    },
  })],
+  backendDefaults: appmesh.ClientPolicy.fileTrust({


Can we test the other static factory method here, acmTrust?

skinny85 · 2020-11-24T18:23:34Z

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

+            Listeners: [
+              {
+                HealthCheck: {
+                  HealthyThreshold: 2,
+                  IntervalMillis: 5000,
+                  Port: 80,
+                  Protocol: 'tcp',
+                  TimeoutMillis: 3000,
+                  UnhealthyThreshold: 2,
+                },
+                PortMapping: {
+                  Port: 80,
+                  Protocol: 'tcp',
+                },
+                Timeout: {
+                  TCP: {
+                    Idle: {
+                      Unit: 'ms',
+                      Value: 10000,
+                    },
+                  },
+                },
+              },
+            ],


Is there a reason this is included in this test? I think we're only interested in the below Backends part, right?

Removed Listeners and kept backend part in test.

…faults

Pull request has been modified.

sshver · 2020-11-30T21:30:15Z

In reply to comment,
I have created a shared interface BackendDefaults with clientPolicy as a property. The interface is used in VirtualNode and VirtualGateway to represent the backendDefault property. The ClientPolicy is a part of VirtualServiceBackend object. (Reference). Therefore I have updated the VirtualService and added a property clientPolicy of type ClientPolicy

skinny85

Looks great @sshver ! Nice work on adding the skeleton ACM PCA resources, I have a feeling that skeleton will get filled out fast after we merge this 😉.

A few minor comments before I give my "ShipIt".

skinny85 · 2020-11-30T22:24:55Z

packages/@aws-cdk/aws-acmpca/lib/acmpca.ts

@@ -0,0 +1,63 @@
+import * as cdk from '@aws-cdk/core';


This file should be called certificate-authority.ts.

skinny85 · 2020-11-30T22:28:38Z

packages/@aws-cdk/aws-acmpca/lib/acmpca.ts

+/**
+ * Basic Properties of a Certificate
+ */
+export interface CertificateAuthorityProps {


Let's remove these Props for now. Having fromCertificateAuthorityArn() is good enough for now.

skinny85 · 2020-11-30T22:30:58Z

packages/@aws-cdk/aws-acmpca/lib/acmpca.ts

+/**
+ * Interface which all CertificateAuthority based class must implement
+ */
+export abstract class CertificateAuthorityBase extends cdk.Resource implements ICertificateAuthority {


I have a feeling this class is superfluous at this stage, and can be safely removed once you incorporate my remaining comments for this file.

But even if you decide to keep it, it should be module-private (remove the export modifier here).

skinny85 · 2020-11-30T22:32:12Z

packages/@aws-cdk/aws-acmpca/lib/acmpca.ts

+   */
+  public readonly certificateAuthorityArn: string;
+
+  constructor(scope: Construct, id: string, props: CertificateAuthorityProps) {


Just make this constructor the same as for cdk.Resource, and make it private. Just having fromCertificateAuthorityArn() is good enough at this stage (and passing an ARN to a resource managed by the CDK doesn't really make much sense anyway).

skinny85 · 2020-11-30T22:32:44Z

packages/@aws-cdk/aws-acmpca/lib/acmpca.ts

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


Nope, that's not how this property should be used (it should be the actual name, not the ARN).

Just leave this out for now.

skinny85 · 2020-11-30T23:08:10Z

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

@@ -1,7 +1,7 @@
 import { expect, haveResourceLike } from '@aws-cdk/assert';
+import { CertificateAuthority } from '@aws-cdk/aws-acmpca';


Same here - no unqualified imports outside your module.

Suggested change

import { CertificateAuthority } from '@aws-cdk/aws-acmpca';

import * as acmpca from '@aws-cdk/aws-acmpca';

skinny85 · 2020-11-30T23:08:48Z

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

+                  Validation: {
+                    Trust: {
+                      ACM: {
+                        CertificateAuthorityArns: ['arn:aws:acm-pca:us-east-1:123456789012:certificate-authority/12345678-1234-1234-1234-123456789012'],


Use the variable from above here instead of repeating the value?

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

skinny85 · 2020-11-30T23:09:28Z

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

+          meshName: 'test-mesh',
+        });
+
+        const arn = 'arn:aws:acm-pca:us-east-1:123456789012:certificate-authority/12345678-1234-1234-1234-123456789012';


Rename to certificateAuthorityArn?

skinny85 · 2020-11-30T23:14:14Z

Also, the ReadMe changes are missing 🙂.

…faults

Pull request has been modified.

…faults

skinny85

We're almost there! A few minor changes before we merge this in, mainly concerning naming and documentation.

skinny85 · 2020-12-04T22:05:50Z

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

+  }),
+});
+
+node.addBackend(virtualService);


This is a weird example, right? Because if I understand correctly, because the VirtualService defines its own clientPolicy, the backendDefaultsClientPolicy on VirtualNode will be ignored?

If my understanding is correct, I would re-structure the example to be less artificial.

skinny85 · 2020-12-04T22:06:33Z

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

@@ -159,8 +159,22 @@ const node = mesh.addVirtualNode('virtual-node', {
      unhealthyThreshold: 2,
    },
  })],
+  backendDefaultsClientPolicy: appmesh.ClientPolicy.fileTrust({
+    certificateChain: 'path-to-certificate',


Can we put a more realistic path to the certificate here? Where are they usually stored? Right now, this looks kind of weird as an example (it's not even a file path...).

skinny85 · 2020-12-04T22:06:41Z

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

+  virtualServiceName: 'service1.domain.local',
+  mesh,
+  clientPolicy: appmesh.ClientPolicy.fileTrust({
+    certificateChain: 'path-to-certificate',


Same comment for this path as above.

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

skinny85 · 2020-12-04T22:08:08Z

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

 });
 ```

 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 `backendDefaults` property are added to the node while creating the virtual gateway. These are virtual gateway's service backends client policy defaults.


Old name used here (it should be backendDefaultsClientPolicy, not backendDefaults).

skinny85 · 2020-12-04T22:47:01Z

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

@@ -165,14 +172,14 @@ export class VirtualGateway extends VirtualGatewayBase {
    } else {
      props.listeners.forEach(listener => this.listeners.push(listener.bind(this)));
    }
-


I would leave this as-is.

skinny85 · 2020-12-04T22:48:39Z

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

+   *
+   * @default - No Config
+   */
+  readonly backendDefaultsClientPolicy?: ClientPolicy;


I think this name is slightly incorrect. There are multiple 'backends', but a single 'default'. So I would call this backendsDefaultClientPolicy.

skinny85 · 2020-12-04T22:50:11Z

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

+   *
+   * @default - No Config
+   */
+  readonly backendDefaultsClientPolicy?: ClientPolicy;


Same comment here as for VirtualGateway.

Suggested change

readonly backendDefaultsClientPolicy?: ClientPolicy;

readonly backendsDefaultClientPolicy?: ClientPolicy;

skinny85 · 2020-12-04T22:50:59Z

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

@@ -187,14 +195,16 @@ export class VirtualNode extends VirtualNodeBase {

    props.backends?.forEach(backend => this.addBackend(backend));
    props.listeners?.forEach(listener => this.addListener(listener));
+


I would leave this as-is.

skinny85 · 2020-12-04T22:54:15Z

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

+          },
+        },
+      }));
+      test.done();


I usually like to leave an empty line before the last test.done(), makes the tests read slightly better in my view.

…faults

skinny85 · 2020-12-04T23:23:21Z

packages/@aws-cdk/aws-appmesh/lib/client-policy.ts

+  public bind(_scope: cdk.Construct): ClientPolicyConfig {
+    if (this.certificateType === CertificateType.ACMPCA && this.certificateAuthorityArns?.map(certificateArn =>
+      certificateArn.certificateAuthorityArn).length === 0) {
+      throw new Error('Certificate Authority ARN is required but empty');


Let's make this a little more clearer what and where was the problem:

Suggested change

throw new Error('Certificate Authority ARN is required but empty');

throw new Error('You must provide at least one Certificate Authority when creating an ACM Trust ClientPolicy');

…faults

Pull request has been modified.

skinny85

Looks great @sshver , thanks! 😊

mergify · 2020-12-07T19:31:19Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-12-07T20:42:28Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: de939c3
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-12-07T20:46:13Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…rtualService (aws#11563) Adds backend defaults to Virtual Node and Virtual Gateways. Adds validation context for the backend defined on the Virtual Node. Before merging (TODO): - [ ] Update README ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

sshver added 11 commits October 28, 2020 23:11

Added BackendDefaults to Virtual Node

5326284

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

e476ae7

…faults

Modeled TLSClientValidation to enforce trust in file or acm cetificate

dbc5870

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

5b4c97b

…faults

Added check to have only one backendDefault per virtual node

b4b7f95

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

ce35692

…faults

adding backend validation context

4aea038

created a seperate module validation-context

25208a4

Created a seperate client-policy file as client policy wont be only s…

5b260be

…hapes that support validation context

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

a55a305

…faults

Updated comments and removed newlines

7ca64da

skinny85 previously requested changes Nov 19, 2020

View reviewed changes

SomayaB assigned MrArnoldPalmer and skinny85 and unassigned MrArnoldPalmer Nov 20, 2020

1. Addressed comments

01be91b

2. Updated bind() to return client policy

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

7e2ddbc

…faults

sshver requested a review from skinny85 November 24, 2020 16:11

skinny85 previously requested changes Nov 24, 2020

View reviewed changes

sshver added 2 commits November 30, 2020 12:23

Added skeleton L2 for ACMPCA and addressed comments

49824b1

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

0306a36

…faults

sshver requested a review from skinny85 November 30, 2020 21:30

skinny85 previously requested changes Nov 30, 2020

View reviewed changes

sshver added 2 commits December 2, 2020 13:38

Fixed minor comments

045d2a7

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

2cad882

…faults

sshver mentioned this pull request Dec 3, 2020

Add listener TLS certificates for VirtualNodes and VirtualGateways alexbrjo/aws-cdk#1

Merged

sshver added 4 commits December 3, 2020 14:11

Fixed comments

20c0e96

Added validation check to ensure certificateAuthorityArns is not empty

d36fc42

Minor fix

4e07587

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

da25261

…faults

sshver requested a review from skinny85 December 4, 2020 01:35

sshver added 2 commits December 4, 2020 10:21

Updated README and fixed build errors

41d2c12

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

d18fdc4

…faults

skinny85 changed the title ~~feat(appmesh): adds Backend Defaults and Backend Validation Context~~ feat(appmesh): add ClientPolicy to VirtualNode, VirtualGateway and VirtualService Dec 4, 2020

github-actions bot added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Dec 4, 2020

skinny85 previously requested changes Dec 4, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

4ec4b21

…faults

skinny85 mentioned this pull request Dec 4, 2020

feat(appmesh): add listener TLS certificates for VirtualNodes and VirtualGateways #11863

Merged

skinny85 reviewed Dec 4, 2020

View reviewed changes

sshver added 3 commits December 7, 2020 08:34

Minor changes

8b0b863

Updated README.md

bd2d85e

Merge remote-tracking branch 'upstream/master' into feature/backendDe…

8b707ce

…faults

sshver requested a review from skinny85 December 7, 2020 17:41

skinny85 approved these changes Dec 7, 2020

View reviewed changes

Merge branch 'master' into feature/backendDefaults

de939c3

mergify bot merged commit bfee58c into aws:master Dec 7, 2020

	export interface ACMTrustOptions {
	export interface AcmTrustOptions {

	export abstract class TLSValidationContext {
	export abstract class TlsValidationContext {

		@@ -1,7 +1,7 @@
		import { expect, haveResourceLike } from '@aws-cdk/assert';
		import { CertificateAuthority } from '@aws-cdk/aws-acmpca';

	import { CertificateAuthority } from '@aws-cdk/aws-acmpca';
	import * as acmpca from '@aws-cdk/aws-acmpca';

	readonly backendDefaultsClientPolicy?: ClientPolicy;
	readonly backendsDefaultClientPolicy?: ClientPolicy;

		@@ -187,14 +195,16 @@ export class VirtualNode extends VirtualNodeBase {

		props.backends?.forEach(backend => this.addBackend(backend));
		props.listeners?.forEach(listener => this.addListener(listener));

	throw new Error('Certificate Authority ARN is required but empty');
	throw new Error('You must provide at least one Certificate Authority when creating an ACM Trust ClientPolicy');

feat(appmesh): add ClientPolicy to VirtualNode, VirtualGateway and VirtualService #11563

feat(appmesh): add ClientPolicy to VirtualNode, VirtualGateway and VirtualService #11563

Conversation

sshver commented Nov 18, 2020

gitpod-io bot commented Nov 18, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshver commented Nov 30, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Nov 30, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 7, 2020

aws-cdk-automation commented Dec 7, 2020

AWS CodeBuild CI Report

mergify bot commented Dec 7, 2020

gitpod-io bot commented Nov 18, 2020 •

edited

Loading

skinny85 Nov 30, 2020 •

edited

Loading