Repository navigation
Conversation
rjenkins
left a comment
rjenkins
left a comment
There was a problem hiding this comment.
Looks good, left a couple suggestions/comments and questions for my own understanding.
| /// The log format string (e.g., "${version} ${account-id} ${interface-id} ...") | ||
| pub log_format: String, | ||
| /// The destination type (e.g., "cloud-watch-logs", "s3") | ||
| pub destination_type: String, |
There was a problem hiding this comment.
Any good reason not to use an enum here?
There was a problem hiding this comment.
Actually, on inspection there's not really a good reason to save it in this struct. We already filter to only Cloudwatch destinations, so this should never be anything else. Removing entirely.
|
|
||
| // Get the log group name | ||
| let log_group_name = match flow_log.log_group_name() { | ||
| Some(name) => name, |
There was a problem hiding this comment.
You could probably just make this a none check? Is the Some(name) => name needed?
There was a problem hiding this comment.
So the match block is combining an unwrap with a conditional statement on it being None. We could alternatively do an is_none() check here and then an unwrap() later when we need it (since we know it's not none), but this handles both here. Maybe there's a cleaner way to do this though?
| } | ||
| }; | ||
|
|
||
| let log_format = match flow_log.log_format().map(|s| s.to_string()) { |
There was a problem hiding this comment.
Just curious what does this return if it's not a string already?
There was a problem hiding this comment.
The AWS API uses &str, so the map of to_string() is converting that to String, which we want for later.
| /// | ||
| /// Returns information about the current state of the cache including | ||
| /// the number of cached entries and whether fetching is currently disabled. | ||
| pub fn cache_stats(&self) -> CacheStats { |
There was a problem hiding this comment.
Is this only used for testing?
There was a problem hiding this comment.
Oh yes, I've marked these as cfg(test) now
| put_request = put_request.if_match(etag); | ||
| } else { | ||
| debug!("Using conditional write with if-none-match"); | ||
| put_request = put_request.if_none_match("*"); |
There was a problem hiding this comment.
Why do we not want to write if there is an existing file?
There was a problem hiding this comment.
So it's probably overkill for the use case, but I wanted to make sure we never overwrite a cache in S3. Instead, if there's any conflict I want to read-merge-update.
It's probably less important for the flow log use case since we save all entries at once. For log group tag caching, which I want to eventually use this module for, we may be updating individual entries in the cache file separately, so the merging case is more important.
This supports forwarding VPC flow logs from CloudWatch log groups. If the forwarder lambda function has permissions to
ecs:DescribeFlowLogs, then on startup we query flow logs from EC2. The flow logs define the formatting of the log output, which if we can query, is used to split out the named attribute fields from the log message. If we don't have access then the log is just forwarded as the full string.The flow log definitions are refreshed periodically and they are cached to S3. The S3 cache provides for faster cold start times and reduces calls to the EC2 APIs to refresh the data, in theory the Ec2 calls are slower than querying S3. If the IAM permissions do not exist for this, we enter a backoff period and avoid repeated calls.
We enrich the resource record with any tags found on the flog logs, allowing it to have more info on the source.
Resolves: #9