Added BackendDefaults to Virtual Node #3

sshver · 2020-10-29T06:41:13Z

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

dfezzie · 2020-10-29T21:53:11Z

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

+}
+
+/**
+ * Default Configuration for Virtual Nodes


I think we should be more descriptive here.

Default configuration that is applied to all backends for the virtual node. Any configuration defined will be overwritten by configurations specified for a particular backend.

Added descriptive comment.

dfezzie · 2020-10-29T22:17:13Z

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

+  readonly enforce?: boolean
+
+  /**
+   * TLS enforced on these ports. If not specified it is enforced on all ports.
+   *
+   * @default - none
+   */
+  readonly ports?: number[]
+
+  /**
+   * Certificate discovery method, ACM-PCA or Local file hosting.
+   *
+   * @default - none
+   */
+  readonly certificateType: string;
+
+  /**
+   * Certificate File path or Certificate ARN.
+   *
+   * @default - none
+   */
+  readonly certificate: string[];


These should not be top level properties. These are specific to the client configuration with regard to TLS. There may be other types of backend defaults we could apply in the future.

You could model this more like

interface BackendDefaults { readonly tlsClientPolicy: TLSClientPolicy; } interface TLSClientPolicy { /** * @default - True */ readonly enforce?: boolean; ... readonly validation: TLSClientValidation; }

TLSClientValidation could be modeled as a bind class to enforce the trust is one of file, acmpca, or sds (in the future). We may want to accept the Certificate Authority construct from ACMPCA, which is not yet built. Adam can help with that, we'll just need to coordinate with him

We also want to be aware of the changes that are going to be made with aws/aws-app-mesh-roadmap#34 and be able to accommodate them easily

updated the model.

dfezzie · 2020-10-29T22:17:44Z

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

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


The default in our API is true, let's mirror that here

set enforce to true by default.

…faults

dfezzie · 2020-11-05T06:14:48Z

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

 */
 export interface BackendDefaults {
+  /**
+   * Client policy for TLS


So the Props name is reserved for class constructors. (Found this out in a PR I have out now)

Let's have this named TLSClientPolicyOptions

dfezzie

Nice work! This will set up an easy win for adding the ClientPolicy to backends!

dfezzie · 2020-11-05T06:16:43Z

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

+  /**
+   * Client policy for TLS
+   */
+  readonly tlsClientPolicy: TLSClientPolicyProps;


Maybe we can reword this to be more descriptive

TLS Connections with downstream server will always be enforced if True

Something to that effect.

dfezzie · 2020-11-05T06:17:56Z

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


  /**
   * TLS enforced on these ports. If not specified it is enforced on all ports.
   *


Since we haven't introduced SDS yet, let's remove the reference here

We can also reword this to

Policy used to determine if the TLS certificate the server presents is accepted

dfezzie · 2020-11-05T06:18:43Z

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

   */
-  readonly ports?: number[]
+  readonly ports?: number[];



Needs to be optional

Shouldn't it be required ? I found this reference doc where in it says this field in required

dfezzie · 2020-11-05T06:24:31Z

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


+/**
+ * Defines the TLS validation context trust.
+ */


We could shorten this to fileTrust(...)

We could also potentially accept just the path to the certificate chain instead of having a props interface. Makes it simpler to use for customers, but has the drawback of not being as extensible in the future, but there are plenty of ways around that.

dfezzie · 2020-11-05T06:27:06Z

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

+export abstract class TLSClientValidation {
  /**
-   * Certificate File path or Certificate ARN.
-   *


I want to specify this is acm-pca somehow, but it doesn't flow great :P You have any ideas?

dfezzie · 2020-11-05T06:29:20Z

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

+
+  /**
+   * Returns Trust context based on trust type.
+   */


We should specify ACM PCA here

Amazon Resource Names (ARN) of trusted ACM Private Certificate Authorities

dfezzie · 2020-11-05T06:29:56Z

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

+        file: {
+          certificateChain: this.certificateChain,
+        },
+      },


This does not need to be exported. Same for above

dfezzie · 2020-11-05T06:30:35Z

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

+
+  /**
+   * TLS validation context trust for AWS Certicate Manager (ACM) certificate.
+   */


Bind should return a Config object here

dfezzie · 2020-11-05T06:37:28Z

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

-                  certificateChain: certificate[0],
-                },
-              },
-            },


This could be simplified to just backendDefaults.tlsClientPolicy.ports since it accepts undefined.

dfezzie · 2020-11-05T06:39:28Z

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

-              } : {
-                file: {
-                  certificateChain: certificate[0],
-                },


I think enforce: backendDefaults.tlsClientPolicy.enforce ?? true would work here?

…faults

dfezzie

Looks good to me. Just a few comments.

Any reason not to implement this for Virtual Gateways?

dfezzie · 2020-11-13T17:23:52Z

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

+ */
+export abstract class TLSClientValidation {
+  /**
+   * TLS validation context trust for a local file


nit: TLS validation provided from the local file system

This struct tells the envoy where to fetch the validation context from

same comment below

dfezzie · 2020-11-13T17:27:37Z

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

  readonly accessLog?: AccessLog;
+
+  /**
+   * Default Configuration Virtual Node uses to communicate with Vritual Service


typo: Vritual => Virtual

dfezzie · 2020-11-13T17:29:08Z

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

   */
  public readonly mesh: IMesh;

+  private readonly backendDefaults = new Array<CfnVirtualNode.BackendDefaultsProperty>();


Why use an Array here? This could just be CfnVirtualNode.BackendDefaultsProperty

If the backendDefaults is of type CfnVirtualNode.BackendDefaultsProperty and a private member of VirtualNode, I would have to initialize it say, with an empty object because it cannot be undefined. There are no options to omit the empty object and all the resources would have backendDefaults: {}, if not provided by the client. Whereas if we use Array we could set omitEmptyArray true.

Or something like this would work as well.

if (props.backendDefaults) { this.backendDefaults = this.addBackendDefaults(props.backendDefaults); } else { this.backendDefaults = {}; } const node = new CfnVirtualNode(this, 'Resource', { backendDefaults: Object.keys(this.backendDefaults).length != 0 ? this.backendDefaults : undefined, . . . });

dfezzie · 2020-11-13T17:30:05Z

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

+  /**
+   * Adds Default Backend Configuration for virtual node to communicate with Virtual Services.
+   */
+  public addBackendDefaults(clientPolicy: ClientPolicy) {


Why would we need this method?

I was under the impression that the backendDefault can be added to virtual node after it has been created. Hence a public method.

As discussed offline, I have made this method private.

dfezzie · 2020-11-13T17:32:59Z

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

+      this.backendDefaults.push({
+        clientPolicy: {
+          tls: {
+            enforce: clientPolicy.tlsClientPolicy.enforce ?? true,


Technically we could just do clientPolicy.tlsClientPolicy.enforce and accept the undefined since the API defaults to true. Not sure what the right thing to do here for CDK is

alexbrjo · 2020-11-17T01:01:32Z

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

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


Backend Client Policies won't be the only shapes that support validation contexts. Listeners could support validation contexts in the future (ex. mTLS).

Unless there is a limitation that forces us to have 2 different shapes for client validation and server validation, I'd recommend changing the name here to a more general TLSValidationContext. And creating a separate file for client policy shapes.

Fair point. I have updated the PR.

…hapes that support validation context

…faults

sshver · 2020-11-18T22:57:50Z

Looks good to me. Just a few comments.

Any reason not to implement this for Virtual Gateways?

Added backend defaults for Virtual Gateways in the latest commit.

2. Updated bind() to return client policy

…faults

Added BackendDefaults to Virtual Node

5326284

dfezzie suggested changes Oct 29, 2020

View reviewed changes

sshver added 2 commits November 3, 2020 10:59

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

e476ae7

…faults

Modeled TLSClientValidation to enforce trust in file or acm cetificate

dbc5870

dfezzie reviewed Nov 5, 2020

View reviewed changes

dfezzie suggested changes Nov 5, 2020

View reviewed changes

sshver added 5 commits November 10, 2020 09:29

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

dfezzie suggested changes Nov 13, 2020

View reviewed changes

alexbrjo reviewed Nov 17, 2020

View reviewed changes

sshver added 2 commits November 18, 2020 12:06

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

sshver added 14 commits November 18, 2020 15:26

Updated comments and removed newlines

7ca64da

1. Addressed comments

01be91b

2. Updated bind() to return client policy

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

7e2ddbc

…faults

Added skeleton L2 for ACMPCA and addressed comments

49824b1

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

0306a36

…faults

Fixed minor comments

045d2a7

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

2cad882

…faults

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

Updated README and fixed build errors

41d2c12

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

d18fdc4

…faults

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

4ec4b21

…faults

sshver and others added 4 commits December 7, 2020 08:34

Minor changes

8b0b863

Updated README.md

bd2d85e

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

8b707ce

…faults

Merge branch 'master' into feature/backendDefaults

de939c3

Added BackendDefaults to Virtual Node #3

Are you sure you want to change the base?

Added BackendDefaults to Virtual Node #3

Uh oh!

Conversation

sshver commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfezzie Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfezzie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfezzie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sshver Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sshver commented Nov 18, 2020

Uh oh!

Reviewers

dfezzie Nov 5, 2020 •

edited

Loading

sshver Nov 17, 2020 •

edited

Loading