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

[alexbrjo]

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

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

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

/* 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

[gitpod-io]

[alexbrjo]

I'm getting 500s from refreshing this page and I see a couple of the checks failed with 500s too. Maybe a Github availability dip - I'll wait and rerun them.

[dfezzie]

Remove this line

[skinny85]

I second this motion 🙂.

[dfezzie]

There should be a newline between comments and previous variable

[skinny85]

I second this motion 🙂.

[dfezzie]

I wonder if we could just make this an interface. I'm trying to think of a situation where we would need to enforce mutual exclusivity for this field and I'm not sure if we would need this. What do you think?

[alexbrjo]

Not sure if I'm following. Do you mean both Backend and BackendDefaults would implement an interface that enforces a method to get the client policy?

[dfezzie]

Like, just defining BackendDefaults as an interface type

interface BackendDefaults {
      readonly clientPolicy?: ClientPolicy;
}

and then instead of having to call Backends.backendDefaults you could just create a new object like:

new VirtualNode(stack, 'VirtualNode', {
    ...
    backendDefaults: {
        clientPolicy: appmesh.ClientPolicy.acm({ ... }),
    }
);

[dfezzie]

My understanding is the bind() methods are used to enforce exclusivity. Like you cannot define both file and acm types for ClientPolicies. For backend defaults, I don't see us needing to enforce this mutual exclusivity

[skinny85]

Yeah, I agree. It doesn't seem like the bind() pattern is buying us anything here.

Maybe something like this is what we need?

export interface BackendDefaults {
  readonly clientPolicy?: ClientPolicy;
}

export interface Backend {
  readonly clientPolicy?: ClientPolicy;

  readonly virtualService: IVirtualService;
}

[dfezzie]

I would hesitate to remove to remove the bind() pattern for the backend itself. I could see there being other targets in the future that might not be a IVirtualService. We don't have any immediate plans for this, but adding a different type here is within the realm of reason.

[skinny85]

Fair enough. I defer to the expert(s) here 🙂.

[skinny85]

Looks nice @alexbrjo ! But I think this is one of the places where we don't need the bind() pattern, and we can just go with simple interfaces instead.

Also, make sure to include all of the breaking changes in this PR in the description - see our Contributing guide for how these should be structured.

[skinny85]

We got rid of this - if you merge from master, you should be able to remove the "signAssembly" and "assemblyOriginatorKeyFile" keys.

[skinny85]

I second this motion 🙂.

[skinny85]

I second this motion 🙂.

[skinny85]

Yeah, I agree. It doesn't seem like the bind() pattern is buying us anything here.

Maybe something like this is what we need?

export interface BackendDefaults {
  readonly clientPolicy?: ClientPolicy;
}

export interface Backend {
  readonly clientPolicy?: ClientPolicy;

  readonly virtualService: IVirtualService;
}

[skinny85]

Let's leave this ID as-is, and just start adding 'service2' below. We should minimize these changes as much as possible.

Pull request has been modified.

[alexbrjo]

Builds fail with an error in another module. Looking into it

@aws-cdk-containers/ecs-service-extensions: lib/extensions/appmesh.ts(349,33): error TS2345: Argument of type 'VirtualService' is not assignable to parameter of type 'Backend'.
@aws-cdk-containers/ecs-service-extensions:   Property 'virtualService' is missing in type 'VirtualService' but required in type 'Backend'.

[dfezzie]

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

[alexbrjo]

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.

[dfezzie]

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.

[dfezzie]

Changes LGTM at this point. 👍

I'll defer the rest of the review to Adam

[skinny85]

Looks good! Couple of minor comments/questions.

Also, I believe the ReadMe file of the module also needs updating with these changes!

Pull request has been modified.

[alexbrjo]

Merge from master, addressed all comments + noticed I didn't previously update the README. So added that

[alexbrjo]

The AWS CodeBuild CI Report succeeded (see above comment) and the logs look fine at a glance. but the "AWS CodeBuild us-east-1 (AutoBuildProject89A8053A-LhjRyN9kxr8o)" check below failed. How can I figure out why this happened?

[skinny85]

@alexbrjo Not sure why the failing build did not post the logs, but the build is failing on:

@aws-cdk-containers/ecs-service-extensions: lib/extensions/appmesh.ts(350,49): error TS2339: Property 'virtualServiceBackend' does not exist on type 'typeof Backend'.
--
981 | @aws-cdk-containers/ecs-service-extensions: Error: /codebuild/output/src634370312/src/github.com/aws/aws-cdk/node_modules/typescript/bin/tsc --build exited with error code 1
982 | @aws-cdk-containers/ecs-service-extensions: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
983 | @aws-cdk-containers/ecs-service-extensions: error Command failed with exit code 1.
984 | @aws-cdk-containers/ecs-service-extensions: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
985 | @aws-cdk-containers/ecs-service-extensions: error Command failed with exit code 1.
986 | @aws-cdk-containers/ecs-service-extensions: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
987 | lerna ERR! yarn run build+test exited 1 in '@aws-cdk-containers/ecs-service-extensions'
988 | lerna WARN complete Waiting for 5 child processes to exit. CTRL-C to exit immediately.

[skinny85]

Everything looks great @alexbrjo !

I have one suggestion - I'm curious what your opinion is on it.

[skinny85]

Looks great @alexbrjo!