-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
feat(aws-ec2): add support for custom logformat in vpc flow logs #16279
Conversation
@peterwoodworth Thanks for taking a look. Should I document this in a Readme like the PR linter says or it's not necessary? |
No need to update the readme anymore :) I've marked this as p2, so it may be a while before it gets reviewed. Are you aware of escape hatches which help you work around this issue in the meantime? |
Yup, I'm alright with escape hatches for now. |
924c117
to
ebfd5f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :)
@mnanchev I fixed the test that had a conflict. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xavierbuspatrol thanks for the PR and sorry it took so long to review!
* | ||
* @default - No custom log format options provided. | ||
*/ | ||
readonly logFormat?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the log format expects specific values, I think it makes sense to turn this into something that is better typed and can provide useful information to the user. Maybe something like
readonly customLogFormatFields?: LogFormatField[];
And then LogFormat
could be something like
export class LogFormatField {
/**
* The source port of the traffic.
*/
public static readonly SRC_PORT = new LogFormatField('${srcport'});
public static custom(field: string): LogFormatField {
return new LogFormatField(`\${${field}}`);
}
constructor(public readonly value: string) {}
}
new FlowLog(stack, 'FlowLogs', {
customLogFormatFields: [
LogFormatField.SRC_PORT,
LogFormatField.custom('dstport'),
...
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback, will work on it
This PR has been in CHANGES REQUESTED for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
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: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*
Adds support for a custom
logFormat
in the vpc flow logs options.See "Custom Format" on this page: https://docs.aws.amazon.com/vpc/latest/userguide/flow-logs.html
fixes #19316