-
-
Notifications
You must be signed in to change notification settings - Fork 952
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 configuration options for AWS S3 server access logging #3006
base: master
Are you sure you want to change the base?
Add configuration options for AWS S3 server access logging #3006
Conversation
SkipAccessLoggingBucketAcl bool `mapstructure:"skip_accesslogging_bucket_acl"` | ||
SkipAccessLoggingBucketEnforcedTLS bool `mapstructure:"skip_accesslogging_bucket_enforced_tls"` | ||
SkipAccessLoggingBucketPublicAccessBlocking bool `mapstructure:"skip_accesslogging_bucket_public_access_blocking"` | ||
SkipAccessLoggingBucketSSEncryption bool `mapstructure:"skip_accesslogging_bucket_ssencryption"` |
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.
Could you please consider adding tests to ensure that the newly added fields are functioning as expected?
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.
Hi @denis256, Thx for your feedback. I tried to updated existing existing tests and added one new for method createS3LoggingInput
. Please let me know if I missed anything else.
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.
Hi @denis256 , Can I politely ask you for re-review once you have some free time please? Thx.
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.
Hi @denis256, do you think you'll have time to take a look on this PR in the upcoming days?
675daef
to
e4b533b
Compare
Description
Fixes #3003.
In our environment the S3 bucket where should be stored S3 access logs is pre-created by "central" team and we don't have permissions to do any changes. In such case we don't have to:
PartitionDateSource
.Additionally 1) adding error logging into the function
configureAccessLogBucket
. Without it I wasn't able to detect why the logging isn't configured on the source bucket. The error is returned but ignored. 2) Adding code to detect if the logging policy / configuration was change. The previous code just check if some policy is enabled.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
N/A.