Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from
Sep 30, 2020
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e183e79
feat(aws-appmesh): adds access logging configuration to virtual nodes
Sep 23, 2020
6ef47f2
Reworks the AcessLog class to use bind methods to future proof for mu…
Sep 25, 2020
734c731
Merge branch 'master' into feature/accesslogging
dfezzie Sep 26, 2020
76f97c2
Changes bind method to use the CFN property for access logging
Sep 29, 2020
f878242
Add use of the access log property to the README
Sep 30, 2020
bada0e1
Merge branch 'master' into feature/accesslogging
mergify[bot] Sep 30, 2020
7788b20
Remove the default access logging config from the ECS patterns integ
Sep 30, 2020
a8ac2ca
Merge branch 'master' into feature/accesslogging
dfezzie Sep 30, 2020
bb9e105
Merge branch 'master' into feature/accesslogging
mergify[bot] Sep 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions ...es/@aws-cdk-containers/ecs-service-extensions/test/integ.all-service-addons.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,13 +1303,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"AWSCloudMap": {
"NamespaceName": "production",
Expand Down Expand Up @@ -2179,13 +2172,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"AWSCloudMap": {
"NamespaceName": "production",
Expand Down Expand Up @@ -3210,13 +3196,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"AWSCloudMap": {
"NamespaceName": "production",
Expand Down
14 changes: 0 additions & 14 deletions ...@aws-cdk-containers/ecs-service-extensions/test/integ.multiple-environments.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1491,13 +1491,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"AWSCloudMap": {
"NamespaceName": "production",
Expand Down Expand Up @@ -2022,13 +2015,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"AWSCloudMap": {
"NamespaceName": "development",
Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-appmesh/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ const node = mesh.addVirtualNode('virtual-node', {
unhealthyThreshold: 2,
},
},
accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
})
```

Expand All @@ -186,12 +187,13 @@ const node = new VirtualNode(this, 'node', {
unhealthyThreshold: 2,
},
},
accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
});

cdk.Tag.add(node, 'Environment', 'Dev');
```

The listeners property can be left blank dded later with the `mesh.addListeners()` method. The `healthcheck` property is optional but if specifying a listener, the `portMappings` must contain at least one property.
The listeners property can be left blank and added later with the `mesh.addListeners()` method. The `healthcheck` property is optional but if specifying a listener, the `portMappings` must contain at least one property.

## Adding a Route

Expand Down
69 changes: 65 additions & 4 deletions packages/@aws-cdk/aws-appmesh/lib/shared-interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Duration } from '@aws-cdk/core';
import * as cdk from '@aws-cdk/core';
import { CfnVirtualNode } from './appmesh.generated';

/**
* Enum of supported AppMesh protocols
Expand Down Expand Up @@ -27,7 +28,7 @@ export interface HealthCheck {
*
* @default 5 seconds
*/
readonly interval?: Duration;
readonly interval?: cdk.Duration;
/**
* The path where the application expects any health-checks, this can also be the application path.
*
Expand All @@ -52,7 +53,7 @@ export interface HealthCheck {
*
* @default 2 seconds
*/
readonly timeout?: Duration;
readonly timeout?: cdk.Duration;
/**
* Number of failed attempts before considering the node DOWN.
*
Expand Down Expand Up @@ -92,9 +93,69 @@ export interface VirtualNodeListener {
readonly portMapping?: PortMapping;

/**
* Array fo HealthCheckProps for the node(s)
* Health checking strategy upstream nodes should use when communicating with the listener
*
* @default - no healthcheck
*/
readonly healthCheck?: HealthCheck;
}

/**
* All Properties for Envoy Access logs for mesh endpoints
*/
export interface AccessLogConfig {

/**
* VirtualNode CFN configuration for Access Logging
*
* @default - no access logging
*/
readonly virtualNodeAccessLog?: CfnVirtualNode.AccessLogProperty;
}

/**
* Configuration for Envoy Access logs for mesh endpoints
*/
export abstract class AccessLog {
/**
* Path to a file to write access logs to
*
* @default - no file based access logging
*/
public static fromFilePath(filePath: string): AccessLog {
return new FileAccessLog(filePath);
}

/**
* Called when the AccessLog type is initialized. Can be used to enforce
* mutual exclusivity with future properties
*/
public abstract bind(scope: cdk.Construct): AccessLogConfig;
}

/**
* Configuration for Envoy Access logs for mesh endpoints
*/
class FileAccessLog extends AccessLog {
/**
* Path to a file to write access logs to
*
* @default - no file based access logging
*/
public readonly filePath: string;

constructor(filePath: string) {
super();
this.filePath = filePath;
}

public bind(_scope: cdk.Construct): AccessLogConfig {
return {
virtualNodeAccessLog: {
file: {
path: this.filePath,
},
},
};
}
}
20 changes: 12 additions & 8 deletions packages/@aws-cdk/aws-appmesh/lib/virtual-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as cdk from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnVirtualNode } from './appmesh.generated';
import { IMesh } from './mesh';
import { HealthCheck, PortMapping, Protocol, VirtualNodeListener } from './shared-interfaces';
import { AccessLog, HealthCheck, PortMapping, Protocol, VirtualNodeListener } from './shared-interfaces';
import { IVirtualService } from './virtual-service';

/**
Expand Down Expand Up @@ -90,6 +90,13 @@ export interface VirtualNodeBaseProps {
* @default - No listeners
*/
readonly listener?: VirtualNodeListener;

/**
* Access Logging Configuration for the virtual node
*
* @default - No access logging
*/
readonly accessLog?: AccessLog;
}

/**
Expand Down Expand Up @@ -252,6 +259,7 @@ export class VirtualNode extends VirtualNodeBase {

this.addBackends(...props.backends || []);
this.addListeners(...props.listener ? [props.listener] : []);
const accessLogging = props.accessLog?.bind(this);

const node = new CfnVirtualNode(this, 'Resource', {
virtualNodeName: this.physicalName,
Expand All @@ -267,13 +275,9 @@ export class VirtualNode extends VirtualNodeBase {
attributes: renderAttributes(props.cloudMapServiceInstanceAttributes),
} : undefined,
},
logging: {
accessLog: {
file: {
path: '/dev/stdout',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@skinny85 skinny85 Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Don't default this field (this way, not setting it in the CDK = passing null through CloudFormation).
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bcelenza bcelenza Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are actually two questions here:

  1. Should we enable access logging by default?
  2. 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.

},
},
},
logging: accessLogging !== undefined ? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

accessLog: accessLogging.virtualNodeAccessLog,
} : undefined,
},
});

Expand Down
14 changes: 0 additions & 14 deletions packages/@aws-cdk/aws-appmesh/test/integ.mesh.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,13 +672,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"DNS": {
"Hostname": "node1.domain.local"
Expand Down Expand Up @@ -727,13 +720,6 @@
}
}
],
"Logging": {
"AccessLog": {
"File": {
"Path": "/dev/stdout"
}
}
},
"ServiceDiscovery": {
"DNS": {
"Hostname": "node2.domain.local"
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-appmesh/test/integ.mesh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ const node3 = mesh.addVirtualNode('node3', {
unhealthyThreshold: 2,
},
},
accessLog: appmesh.AccessLog.fromFilePath('/dev/stdout'),
});

router.addRoute('route-2', {
Expand Down
7 changes: 0 additions & 7 deletions packages/@aws-cdk/aws-appmesh/test/test.mesh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,13 +276,6 @@ export = {
},
Spec: {
// Specifically: no Listeners and Backends
Logging: {
AccessLog: {
File: {
Path: '/dev/stdout',
},
},
},
ServiceDiscovery: {
DNS: {
Hostname: 'test.domain.local',
Expand Down