feat(aws-appmesh): adds access logging configuration to Virtual Nodes #10490

dfezzie · 2020-09-23T04:43:26Z

Addresses the first point on #9490 by allow access logging to be configured through props

Introduces a new AccessLog shared-interface as it can be reused in Virtual Gateways and Virtual Nodes
Removes the default access logging to stdout in Virtual Nodes and allows it to be configured via props

BREAKING CHANGE: VirtualNode no longer has accessLog set to "/dev/stdout" by default

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

skinny85

LGTM! One small comment, feel free to remove the pr/do-not-merge label if you decide to tackle it, and the PR will get merged automatically.

Thanks for the contribution!

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

bcelenza · 2020-09-24T00:03:03Z

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

+  /**
+   * Path to a file to write access logs to
+   */
+  readonly filePath: string;


Question (for @skinny85 I think): We represent the options in this struct as a "union" in our APIs, which means only one value (e.g. filePath or something else) can be provided. In the future if we add a new option that's not a filePath, is this the right interface/approach?

If that is the case, we have a different pattern in the CDK for that: the union-like class. Basically, you type this property as an abstract class:

export abstract class AccessLog { // ... } export interface VirtualNodeBaseProps { // .. readonly accessLog?: AccessLog; }

And then have static factory methods that create instances of it:

export abstract class AccessLog { public static fromFilePath(filePath: string): AccessLog { // ... } // ... }

If, in the future, you need other types of AccessLog, you just add new static factory methods to the AccessLog class.

Does this make sense?

bcelenza · 2020-09-24T00:03:43Z

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

          accessLog: {
            file: {
-              path: '/dev/stdout',


Would defaulting the filePath to /dev/stdout be reasonable? It's the most common configuration we expect for folks using access logs.

Let me ask you this.

Is it reasonable to leave this property unset (by that I mean null, as this property is optional in CloudFormation)? Is it a legitimate thing that customers might want to do?

Path itself cannot be null (required in CFN: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appmesh-virtualnode-fileaccesslog.html). However, nulling the access log field in total is acceptable, and just means that no access logging will be configured for the resource

So, is it reasonable for a customer to want to turn off logging by setting this (entire) property to null? Because if it is, we need to to take this requirement into account when designing this functionality.

I see 2 ways of allowing that:

Don't default this field (this way, not setting it in the CDK = passing null through CloudFormation).

If you want to go with the union-like pattern that I mentioned in this comment, you can have a "magic" AccessLog static factory method like none:

export abstract class AccessLog { public static none(): AccessLog { ... } // ... }

which will set this field in the implementation of VirtualNode to null. In this case, keep the default for this field same it is today (so, { file: { path: '/dev/stdout' } }).

Which one to choose between the two (assuming you're both on-board with the "union-like" approach here) I think depends on what you see the common case being. If file: { path: '/dev/stdout' } is a sensible default that many customers will be happy with, I would go with nr 2. If disabling logging is a common thing that a lot customers want (for example: does it have some cost implications?), though, I would go with nr 1.

For Virtual Nodes, wouldn't the equivalent to having no access logging be:

new VirtualNode(stack, 'testVn', { mesh: mesh, virtualNodeName: 'exampleVirtualNodeName', })

Then if you wanted to enable the default access logging you could do something like:

new VirtualNode(stack, 'testVnWithDefaultAccessLogging', { mesh: mesh, virtualNodeName: 'exampleVirtualNodeName', accessLog: {}, })

or are the two equivalent?

I think we're talking about 2 different defaults here 🙂.

Today, this:

new VirtualNode(stack, 'testVn', { mesh: mesh, virtualNodeName: 'exampleVirtualNodeName', });

will result in the CDK default for logging of { file: { path: '/dev/stdout' } }. I believe we're discussing in this comment whether to keep this default, or to get rid of it (see @bcelenza's original comment).

If you do this:

new VirtualNode(stack, 'testVnWithDefaultAccessLogging', { mesh: mesh, virtualNodeName: 'exampleVirtualNodeName', accessLog: {}, });

you will get the service default instead, which I believe is "no logging is enabled" (but I'm sure you know it better than I do 🙂)

Yeah, there are actually two questions here:

Should we enable access logging by default?

Should the default path of that access log be /dev/stdout?

Having given this some thought, my view is the answer to both is no. With /dev/stdout, the access logs are mixed in with the logs from the proxy process, so I think it's important folks understand the impact of that (namely they need to parse two formats from the same sink). Also, /dev/stdout is *nix specific and while we don't support Windows now, I can't definitively say we wouldn't support it in the future.

…tual exclusive properties

Pull request has been modified.

skinny85

I have a suggestion for AccessLog.bind() that I think will simplify this greatly 🙂.

skinny85 · 2020-09-29T19:53:53Z

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

+   *
+   * @default - no file based access logging
+   */
+  readonly filePath?: string;


Here's my suggestion 🙂.

To make bind() in AccessLog more flexible, let's actually have AccessLogConfig have an (optional) property called accessLog with the type CfnVirtualNode.AccessLogProperty.

Yes, I know we say we don't use L1 properties in our L2s, but that only applies to input properties. This is an output property. Complicated, I know 🙂.

Then, in VritualNode, you can simply do:

const accessLoggingConfig = props.accessLog?.bind(this); const node = new CfnVirtualNode(this, 'Resource', { // ... logging: { accessLog: accessLoggingConfig.accessLog, }, // ...

This has the huge benefit that customers don't have to wait for you to implement new access logging types - they have an "escape hatch" of updating their L1 layer, and then extending AccessLog themselves.

Would this pattern support Virtual Gateways as well? https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appmesh-virtualgateway-virtualgatewaylogging.html

Would you implement the bind method to also set a CfnVirtualGateway.VirtualGatewayAccessLogProperty; property on the AccessLogConfig as well?

If that's your plan, then I would eventually shoot for AccessLogConfig to have both properties!

export interface AccessLogConfig { readonly virtualNodeLogging?: CfnVirtualNode.AccessLogProperty; readonly virtualGatewayLogging?: CfnVirtualGateway.VirtualGatewayAccessLogProperty; } export class AccessLog { public static fromFilePath(filePath: string): AccessLog { return new FilePathAccessLog(filePath); } public abstract bind(scope: Construct): AccessLogConfig; } class FilePathAccessLog extends AccessLog { public constructor(private readonly filePath: string) { } public bind(_scope: Construct): AccessLogConfig { return { virtualNodeLogging: { file { path: this.filePath }, }, virtualGatewayLogging: { file: { path: this.filePath }, }, }; } }

And then both VirtualNode and VirtualGateway read their respective fields after calling bind().

The only way I could see this backfiring is if you were 100% convinced you would not ever have any differences between CfnVirtualNode.AccessLogProperty and CfnVirtualGateway.VirtualGatewayAccessLogProperty (but if that's the case, I would question why would you have 2 separate property types for them in CloudFormation).

skinny85 · 2020-09-29T19:54:53Z

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

+/**
+ * Configuration for Envoy Access logs for mesh endpoints
+ */
+export class FileAccessLog extends AccessLog {


Let's actually not export this class from this file, and keep it module-private (just remove the export keyword).

skinny85 · 2020-09-29T19:55:29Z

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

@@ -267,13 +275,13 @@ export class VirtualNode extends VirtualNodeBase {
            attributes: renderAttributes(props.cloudMapServiceInstanceAttributes),
          } : undefined,
        },
-        logging: {
+        logging: accessLogging !== undefined ? {


If you take my suggestion from above, this code should be simplified greatly 🙂.

Pull request has been modified.

skinny85

Looks great!

One more thing - can you please update the ReadMe with the new class? Just using it in the existing example that creates a VirtualNode class instance to fill the accessLog property of it will be enough 🙂.

Thanks!

skinny85

Awesome, thanks for the first (of hopefully many 🙂) contributions @dfezzie !

mergify · 2020-09-30T16:58:37Z

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

Pull request has been modified.

mergify · 2020-09-30T21:24:00Z

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

mergify · 2020-09-30T22:07:28Z

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-09-30T22:07:30Z

AWS CodeBuild CI Report

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

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

feat(aws-appmesh): adds access logging configuration to virtual nodes

e183e79

SomayaB assigned skinny85 Sep 23, 2020

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Sep 23, 2020

skinny85 previously approved these changes Sep 23, 2020

View reviewed changes

packages/@aws-cdk/aws-appmesh/lib/shared-interfaces.ts Outdated Show resolved Hide resolved

bcelenza reviewed Sep 24, 2020

View reviewed changes

Reworks the AcessLog class to use bind methods to future proof for mu…

6ef47f2

…tual exclusive properties

Merge branch 'master' into feature/accesslogging

734c731

dfezzie requested a review from skinny85 September 29, 2020 15:48

skinny85 previously requested changes Sep 29, 2020

View reviewed changes

Changes bind method to use the CFN property for access logging

76f97c2

dfezzie requested a review from skinny85 September 29, 2020 21:29

skinny85 reviewed Sep 29, 2020

View reviewed changes

Add use of the access log property to the README

f878242

dfezzie requested a review from skinny85 September 30, 2020 03:55

bcelenza approved these changes Sep 30, 2020

View reviewed changes

skinny85 previously approved these changes Sep 30, 2020

View reviewed changes

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Sep 30, 2020

mergify bot and others added 2 commits September 30, 2020 16:59

Merge branch 'master' into feature/accesslogging

bada0e1

Remove the default access logging config from the ECS patterns integ

7788b20

Merge branch 'master' into feature/accesslogging

a8ac2ca

dfezzie requested a review from skinny85 September 30, 2020 20:52

skinny85 approved these changes Sep 30, 2020

View reviewed changes

Merge branch 'master' into feature/accesslogging

bb9e105

mergify bot merged commit e96b5aa into aws:master Sep 30, 2020

dfezzie deleted the feature/accesslogging branch September 30, 2020 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-appmesh): adds access logging configuration to Virtual Nodes #10490

feat(aws-appmesh): adds access logging configuration to Virtual Nodes #10490

dfezzie commented Sep 23, 2020 •

edited by skinny85

Loading

skinny85 left a comment

bcelenza Sep 24, 2020

skinny85 Sep 24, 2020 •

edited

Loading

bcelenza Sep 24, 2020

skinny85 Sep 24, 2020

bcelenza Sep 24, 2020

skinny85 Sep 24, 2020 •

edited

Loading

dfezzie Sep 24, 2020

skinny85 Sep 24, 2020

bcelenza Sep 26, 2020 •

edited

Loading

skinny85 left a comment

skinny85 Sep 29, 2020

dfezzie Sep 29, 2020

skinny85 Sep 29, 2020 •

edited

Loading

skinny85 Sep 29, 2020 •

edited

Loading

skinny85 Sep 29, 2020

skinny85 left a comment

skinny85 left a comment

mergify bot commented Sep 30, 2020

mergify bot commented Sep 30, 2020

mergify bot commented Sep 30, 2020

aws-cdk-automation commented Sep 30, 2020

feat(aws-appmesh): adds access logging configuration to Virtual Nodes #10490

feat(aws-appmesh): adds access logging configuration to Virtual Nodes #10490

Conversation

dfezzie commented Sep 23, 2020 • edited by skinny85 Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Sep 24, 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

skinny85 Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcelenza Sep 26, 2020 • edited Loading

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

skinny85 Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

skinny85 Sep 29, 2020 • edited Loading

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

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 30, 2020

mergify bot commented Sep 30, 2020

mergify bot commented Sep 30, 2020

aws-cdk-automation commented Sep 30, 2020

AWS CodeBuild CI Report

dfezzie commented Sep 23, 2020 •

edited by skinny85

Loading

skinny85 Sep 24, 2020 •

edited

Loading

skinny85 Sep 24, 2020 •

edited

Loading

bcelenza Sep 26, 2020 •

edited

Loading

skinny85 Sep 29, 2020 •

edited

Loading

skinny85 Sep 29, 2020 •

edited

Loading