feat(ec2): change log format in Vpc flow logs #22430

watany-dev · 2022-10-09T07:43:49Z

refer to #16279 and #16279 (comment). The difference is below

I don't think you need custom for LogFormatField. "custom" in the document below does not mean that the user can specify any Key. It means that you can specify any Key of "Available fields" with a space-separated string.
https://docs.aws.amazon.com/vpc/latest/userguide/flow-logs.html#flow-logs-fields

fixes #19316

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-10-09T07:43:56Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

corymhall

Just a couple of comments. Thanks for picking this up!

corymhall · 2022-10-21T15:14:03Z

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

+/**
+ * The following table describes all of the available fields for a flow log record.
+ */
+export enum LogFormatField {


I don't think you need custom for LogFormatField. "custom" in the document below does not mean that the user can specify any Key. It means that you can specify any Key of "Available fields" with a space-separated string.
docs.aws.amazon.com/vpc/latest/userguide/flow-logs.html#flow-logs-fields

The reason we want to have the custom option is to allow the user to specify new fields that we
have not added support for yet. We don't want to block users from using new values as soon as they
are released. If you don't like the custom methed we can do without that and the
user can just do something like:

customLogFormatFields: [ LogFormatField.SRC_PORT, new LogFormatField('${new-field}'), ]

Fixed. Added Usage to Readme.md.

corymhall · 2022-10-21T15:14:53Z

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

+  /**
+    * The VPC Flow Logs version.
+    */


Suggested change

/**

* The VPC Flow Logs version.

*/

/**

* The VPC Flow Logs version.

*/

nit - can you fix all the formatting of these docstrings so they are consistent.

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

Co-authored-by: Cory Hall <[email protected]>

Pull request has been modified.

TheRealAmazonKendra · 2022-11-02T19:01:36Z

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

+  /**
+   * The default format.
+   */
+  public static readonly ALL_DEFAULT_FIELDS = new LogFormatField('${version} ${account-id} ${interface-id} ${srcaddr} ${dstaddr} ${srcport} ${dstport} ${protocol} ${packets} ${bytes} ${start} ${end} ${action} ${log-status} ${vpc-id} ${subnet-id} ${instance-id} ${tcp-flags} ${type} ${pkt-srcaddr} ${pkt-dstaddr} ${region} ${az-id} ${sublocation-type} ${sublocation-id} ${pkt-src-aws-service} ${pkt-dst-aws-service} ${flow-direction} ${traffic-path}');


Edge case question here. Since this will be a manual update in the future, what happens if a customer uses this and then also adds a custom one because it's not in here yet, and then we update this to include it (so now it's a duplicate). Will that cause an error? Will it de-duplicate it? Or will it just have the field twice?

In this implementation, the expanded string of each parameter is defined as a space-separated string. Thus there will be two fields.

According to https://docs.aws.amazon.com/vpc/latest/userguide/flow-logs.html#flow-log-records, the default fields are all those with version '2' in that table, i.e.:

version

account-id

interface-id

srcaddr

dstaddr

srcport

dstport

protocol

packets

bytes

start

end

action

log-status

(Hence, not the rest)

rix0rrr · 2022-11-09T14:43:29Z

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

+  /**
+   * The default format.
+   */
+  public static readonly ALL_DEFAULT_FIELDS = new LogFormatField('${version} ${account-id} ${interface-id} ${srcaddr} ${dstaddr} ${srcport} ${dstport} ${protocol} ${packets} ${bytes} ${start} ${end} ${action} ${log-status} ${vpc-id} ${subnet-id} ${instance-id} ${tcp-flags} ${type} ${pkt-srcaddr} ${pkt-dstaddr} ${region} ${az-id} ${sublocation-type} ${sublocation-id} ${pkt-src-aws-service} ${pkt-dst-aws-service} ${flow-direction} ${traffic-path}');


According to https://docs.aws.amazon.com/vpc/latest/userguide/flow-logs.html#flow-log-records, the default fields are all those with version '2' in that table, i.e.:

version

account-id

interface-id

srcaddr

dstaddr

srcport

dstport

protocol

packets

bytes

start

end

action

log-status

(Hence, not the rest)

rix0rrr · 2022-11-09T14:45:27Z

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

+    return new LogFormatField(`\${${field}}`);
+  }
+
+  constructor(public readonly value: string) {}


Make this constructor at least protected.

I am actually using public value to pass strings like this to CfnFlowlog. Is there a good way to improve this?

customLogFormat = props.logFormat.map(elm => { return elm.value; }).join(' ');

That is fine. The constructor itself should be protected is what I mean.

Suggested change

constructor(public readonly value: string) {}

protected constructor(public readonly value: string) {}

My mistake, my bad.

rix0rrr · 2022-11-09T14:46:51Z

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

+  /**
+   * The custom format. For users to specify unsupported fields.
+   */
+  public static custom(field: string): LogFormatField {


A factory called custom() probably shouldn't add any unrequested decoration. Probably call this factory field(), and add one called custom() that just uses the user string verbatim.

It is certainly more natural that way. Corrected.

packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

rix0rrr · 2022-11-09T15:01:03Z

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

@@ -1321,6 +1321,36 @@ vpc.addFlowLog('FlowLogCloudWatch', {
 });
 ```

+You can also custom format flow logs.


Please add a section heading as well.

Co-authored-by: Rico Huijbers <[email protected]>

Pull request has been modified.

Co-authored-by: Rico Huijbers <[email protected]>

Pull request has been modified.

rix0rrr · 2022-11-10T15:17:53Z

@aws-cdk/aws-ec2:   UNCHANGED  integ.instance-multipart-userdata 1.383s
@aws-cdk/aws-ec2:   UNCHANGED  integ.share-vpcs.lit 1.255s
@aws-cdk/aws-ec2:   NEW        integ.vpc-flow-logs-customformat 0.529s
@aws-cdk/aws-ec2:   UNCHANGED  integ.ports 1.637s
@aws-cdk/aws-ec2:   UNCHANGED  integ.vpc-flow-logs-interval 1.316s
@aws-cdk/aws-ec2:   UNCHANGED  integ.client-vpn-endpoint 2.519s
@aws-cdk/aws-ec2:   UNCHANGED  integ.vpc-ipam 1.34s

wat

watany-dev · 2022-11-11T07:43:02Z

tested integ.vpc-flow-logs-customformat

mergify · 2022-11-11T09:11:52Z

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

mergify · 2022-11-11T09:50:36Z

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

aws-cdk-automation · 2022-11-11T09:50:39Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 54f261a
Result: SUCCEEDED
Build Logs (available for 30 days)

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

github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Oct 9, 2022

aws-cdk-automation requested a review from a team October 9, 2022 07:44

github-actions bot added p2 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Oct 9, 2022

aws-cdk-automation previously requested changes Oct 9, 2022

View reviewed changes

fixes aws#19316. refer to aws#16279

0fb256a

watany-dev force-pushed the add-flowlog-logformat branch from 4d2aa5e to 0fb256a Compare October 15, 2022 05:50

feat(aws-ec2): support for custom logformat in vpc flow logs

71448e9

watany-dev changed the title ~~feat(ec2): Add Option LogFormatField in flowlogs~~ feat(ec2): support for custom logformat in vpc flow logs Oct 15, 2022

watany-dev marked this pull request as ready for review October 16, 2022 03:37

corymhall self-assigned this Oct 21, 2022

corymhall previously requested changes Oct 21, 2022

View reviewed changes

Update packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

9b82ecd

Co-authored-by: Cory Hall <[email protected]>

review: comment style

3d2cb00

watany-dev force-pushed the add-flowlog-logformat branch 3 times, most recently from 3b819e2 to a535340 Compare October 22, 2022 08:33

support custom method

8d24152

watany-dev force-pushed the add-flowlog-logformat branch from a535340 to 8d24152 Compare October 22, 2022 09:06

watany-dev requested a review from corymhall October 23, 2022 05:07

corymhall removed their assignment Nov 2, 2022

TheRealAmazonKendra reviewed Nov 2, 2022

View reviewed changes

watany-dev requested review from TheRealAmazonKendra and removed request for corymhall November 8, 2022 14:16

rix0rrr previously requested changes Nov 9, 2022

View reviewed changes

rix0rrr reviewed Nov 9, 2022

View reviewed changes

classname LogFormatField to LogFormat

c9fa295

Co-authored-by: Rico Huijbers <[email protected]>

watany-dev and others added 8 commits November 10, 2022 01:11

Update packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

10662ec

Co-authored-by: Rico Huijbers <[email protected]>

Update packages/@aws-cdk/aws-ec2/lib/vpc-flow-logs.ts

b93faa6

Co-authored-by: Rico Huijbers <[email protected]>

fix property names

b622cff

fixed ALL DEFAULT FIELDS only version '2'

b0e6194

adding section heading

6bbcf2f

remove unrequested decoration

4573e4c

test remove unrequested decoration

9b8ce2e

Update vpc-flow-logs.ts

50c770a

rix0rrr changed the title ~~feat(ec2): support for custom logformat in vpc flow logs~~ feat(ec2): control of log format in Vpc flow logs Nov 10, 2022

rix0rrr previously approved these changes Nov 10, 2022

View reviewed changes

Merge branch 'main' into add-flowlog-logformat

c0ca1ea

rix0rrr changed the title ~~feat(ec2): control of log format in Vpc flow logs~~ feat(ec2): change log format in Vpc flow logs Nov 10, 2022

rix0rrr previously approved these changes Nov 10, 2022

View reviewed changes

rix0rrr self-assigned this Nov 10, 2022

watany-dev force-pushed the add-flowlog-logformat branch from 84d4f9c to c0ca1ea Compare November 11, 2022 05:08

fix integ

b740547

rix0rrr approved these changes Nov 11, 2022

View reviewed changes

Merge branch 'main' into add-flowlog-logformat

54f261a

mergify bot merged commit 26779f8 into aws:main Nov 11, 2022

watany-dev deleted the add-flowlog-logformat branch November 11, 2022 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): change log format in Vpc flow logs #22430

feat(ec2): change log format in Vpc flow logs #22430

watany-dev commented Oct 9, 2022 •

edited

Loading

gitpod-io bot commented Oct 9, 2022

aws-cdk-automation left a comment

corymhall left a comment

corymhall Oct 21, 2022

watany-dev Oct 22, 2022 •

edited

Loading

corymhall Oct 21, 2022

TheRealAmazonKendra Nov 2, 2022

watany-dev Nov 3, 2022

rix0rrr Nov 9, 2022

watany-dev Nov 9, 2022

rix0rrr Nov 9, 2022

rix0rrr Nov 9, 2022

watany-dev Nov 9, 2022

rix0rrr Nov 10, 2022

watany-dev Nov 10, 2022

rix0rrr Nov 9, 2022

watany-dev Nov 9, 2022

rix0rrr Nov 9, 2022

watany-dev Nov 9, 2022

rix0rrr commented Nov 10, 2022

watany-dev commented Nov 11, 2022

mergify bot commented Nov 11, 2022

mergify bot commented Nov 11, 2022

aws-cdk-automation commented Nov 11, 2022

	constructor(public readonly value: string) {}
	protected constructor(public readonly value: string) {}

feat(ec2): change log format in Vpc flow logs #22430

feat(ec2): change log format in Vpc flow logs #22430

Conversation

watany-dev commented Oct 9, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Oct 9, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watany-dev Oct 22, 2022 • 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

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

rix0rrr commented Nov 10, 2022

watany-dev commented Nov 11, 2022

mergify bot commented Nov 11, 2022

mergify bot commented Nov 11, 2022

aws-cdk-automation commented Nov 11, 2022

AWS CodeBuild CI Report

watany-dev commented Oct 9, 2022 •

edited

Loading

watany-dev Oct 22, 2022 •

edited

Loading