-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add waf v2 logs #23397
base: epic/23361-migrate-aws-waf-v2
Are you sure you want to change the base?
Add waf v2 logs #23397
Conversation
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.
LGTM
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 👍 just some questions/requests.
wodles/aws/buckets_s3/waf.py
Outdated
def check_waf_type(self): | ||
try: |
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.
Add a docstring here to describe this method since it could be a bit confusing. What does it mean when it returns True? Is it native or using Kinesis? and what when it returns False? It can be inferred, but it is better to explain it.
wodles/aws/buckets_s3/waf.py
Outdated
|
||
def check_waf_type(self): | ||
try: | ||
return 'CommonPrefixes' in self.client.list_objects_v2(Bucket=self.bucket, Prefix=f'{self.prefix}AWSLogs', Delimiter='/', MaxKeys=1) |
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.
Too long line
return 'CommonPrefixes' in self.client.list_objects_v2(Bucket=self.bucket, Prefix=f'{self.prefix}AWSLogs', Delimiter='/', MaxKeys=1) | |
return 'CommonPrefixes' in self.client.list_objects_v2(Bucket=self.bucket, Prefix=f'{self.prefix}AWSLogs', | |
Delimiter='/', MaxKeys=1) |
wodles/aws/buckets_s3/waf.py
Outdated
|
||
def get_full_prefix(self, account_id, account_region, acl_name=None): | ||
if self.type == WAF_NATIVE: | ||
path = self.client.list_objects_v2(Bucket=self.bucket) |
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.
There is an imported module named path
, it is usually better to choose a different name the variable so it doesn't shadow the outer scope
path = self.client.list_objects_v2(Bucket=self.bucket) | |
bucket_path = self.client.list_objects_v2(Bucket=self.bucket) |
wodles/aws/buckets_s3/waf.py
Outdated
if 'Contents' in path: | ||
for obj in path['Contents']: | ||
log_key = obj['Key'] | ||
parts = log_key.split("/") | ||
acl_name = parts[parts.index("WAFLogs") + 2] |
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.
I need to better understand what this is for. Would it work if only the first object in path['Contents']
is used? As it is now, it still iterates all items inside path['Contents']
, but only uses the last one. In this part:
acl_name = parts[parts.index("WAFLogs") + 2]
only the parts.index("WAFLogs")
of the last object found in the loop will be used. Additionally, if Contents
is empty, the loop will not iterate and parts
will be undefined.
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.
Update
As we talked about in the huddle, with the new change, access the last Key
of Contents
to obtain the acl_name
wodles/aws/buckets_s3/waf.py
Outdated
def iter_regions_and_accounts(self, account_id, regions): | ||
if self.type == WAF_NATIVE: | ||
AWSBucket.iter_regions_and_accounts(self, account_id, regions) | ||
else: | ||
print(WAF_DEPRECATED_MESSAGE.format(release="5.0", url=WAF_URL)) | ||
self.check_prefix = True | ||
AWSCustomBucket.iter_regions_and_accounts(self, account_id, regions) |
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.
def iter_regions_and_accounts(self, account_id, regions): | |
if self.type == WAF_NATIVE: | |
AWSBucket.iter_regions_and_accounts(self, account_id, regions) | |
else: | |
print(WAF_DEPRECATED_MESSAGE.format(release="5.0", url=WAF_URL)) | |
self.check_prefix = True | |
AWSCustomBucket.iter_regions_and_accounts(self, account_id, regions) | |
def iter_regions_and_accounts(self, account_id, regions): | |
if self.type == WAF_NATIVE: | |
AWSBucket.iter_regions_and_accounts(self, account_id, regions) | |
else: | |
print(WAF_DEPRECATED_MESSAGE.format(release="5.0", url=WAF_URL)) | |
self.check_prefix = True | |
AWSCustomBucket.iter_regions_and_accounts(self, account_id, regions) |
Description
This PR closes #22572. It modified
WAF
to implementWAF v2
logs and updated related unit tests.Test