[extension/awslogsencodingextension] Add support for WAF logs #40443
Conversation
|
LGTM - @axw please take a look |
| // no need to keep parsing the webACLID value for the resource attributes | ||
| // after parsing the first log, since they are all the same | ||
| setKey = false |
There was a problem hiding this comment.
nit : since the key is extracted once, you can extract it outside of the scanner and avoid the if checks as well as the boolean condition.
For example, you can define var log wafLog outside of the loop and use the last log entry to derive the Cloud key details.
There was a problem hiding this comment.
This sounds like a good idea. Be careful if putting wafLog outside the loop as the Unmarshal call will not zero out the struct - it will unmarshal over the top, and may leave fields set from the previous iteration. So we should either explicitly zero the struct, or just record the webACLID field in a var outside the loop.
There was a problem hiding this comment.
Looks great. Really just one question about whether it's safe to assume that the Web ACL ID is identical for all items in the record.
I think that's probably correct in all cases, mostly looking for some documentation that says this. Assuming that's the case, I think @Kavindu-Dodan makes a good point about doing the setKeyAttributes just once after the loop.
The only other thing I'd like to see is a TODO in the docs with a list of the WAF log fields that still need translating.
| @@ -128,3 +128,29 @@ extensions: | |||
| | TLS version | `tls.protocol.version` | | |||
| | Access point ARN | `aws.s3.access_point.arn` | | |||
| | aclRequired | `aws.s3.acl_required` | | |||
|
|
|||
|
|
|||
| #### AWS WAF log record fields | |||
There was a problem hiding this comment.
Should we leave a TODO below the table for fields that are currently untranslated?
| // no need to keep parsing the webACLID value for the resource attributes | ||
| // after parsing the first log, since they are all the same |
There was a problem hiding this comment.
Is this guaranteed?
https://docs.aws.amazon.com/waf/latest/developerguide/logging-s3.html and https://docs.aws.amazon.com/waf/latest/developerguide/logging-cw-logs.html state that they group by Web ACL name -- is that guaranteed to be 1:1 with Web ACL ID?
Also, is it possible that they would be mixed when sending to Firehose (https://docs.aws.amazon.com/waf/latest/developerguide/logging-kinesis.html)?
Description
Add support for WAF log.
Link to tracking issue
Fixes #39407.
Testing
Unit tests added.
Documentation
Documentation updated.