-
-
Notifications
You must be signed in to change notification settings - Fork 995
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
Encrypt the bucket and log access on the AWS S3 Bucket of the TFState #645
Encrypt the bucket and log access on the AWS S3 Bucket of the TFState #645
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.
This looks great, thank you!
Two questions:
-
Could you update the README to indicate that encryption and access logging will be turned on by default and can be enabled with the new
skip_xxx
params? -
How did you test these changes?
remote/remote_state_s3.go
Outdated
Bucket: aws.String(config.Bucket), | ||
BucketLoggingStatus: &s3.BucketLoggingStatus{ | ||
LoggingEnabled: &s3.LoggingEnabled{ | ||
TargetBucket: aws.String(config.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.
Are you writing the access logs back to the same bucket? If so, may be worth mentioning that in the log statement.
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.
Yes, in the same bucket. OK - will do.
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.
@brikis98 Done.
DynamotableTags []map[string]string `mapstructure:"dynamodb_table_tags"` | ||
SkipBucketVersioning bool `mapstructure:"skip_bucket_versioning"` | ||
SkipBucketSSEncryption bool `mapstructure:"skip_bucket_ssencryption"` | ||
SkipBucketAccessLogging bool `mapstructure:"skip_bucket_accesslogging"` |
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.
Is this just GitHub rendering or is the indentation not quite right here? Try running make fmt
if you haven't already.
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.
Weird - I use "Atom" v1.34.0 on Win10Home - looks nicely there. Installed MS VSC (latest) - modified it there, looks nicely - but again it's not fine in GitHub.
Will try make fmt
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.
@brikis98 Should be fixed.
Used the - https://github.com/Microsoft/vscode-go/wiki/Go-tools-that-the-Go-extension-depends-on - for correct formatting in MS VSC 1.30.2 (latest).
goreturns or goimports for formatting code
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.
On GitHub it still looks wrong though.
Tried using GitHub and then paste the code from there - but the Go tools revert it back locally to the currently uploaded code.
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.
Try making whitespace visible in your editor? In vscode, View -> Toggle Render Whitespace
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.
Fixed it remotely on GitHub already.
Thank you for the nice advice though - it'll be useful in the future.
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.
Nah - reverting - I'll use the formatting from the Go Tools. Checked the whitespaces as well - it should be fine when reverted.
It doesn't sound right to use GitHub remotely in a browser to fix such formatting issues.
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.
The funny part - after reverting it looks fine on GitHub as well... #GitHubSaySorryPlease
(1) OK - will do. |
Whoops, hit enter too soon! What I was trying to write:
Since Terragrunt talks to AWS, and our CI builds have credentials for our AWS account, we can't automatically run tests on external PRs: https://circleci.com/docs/2.0/oss/#pass-secrets-to-builds-from-forked-pull-requests So, we ask contributors to run tests themselves. You can test it manually and/or automatically. Once I merge, all the CI tests will run, but I want to have some sense of confidence so I don't merge broken code into |
[root@localhost remote]# go test -v -parallel 128 # _/root/go-project/terragrunt/remote [_/root/go-project/terragrunt/remote.test] ./remote_state_s3.go:449:4: unknown field 'ServerSideEncryption' in struct literal of type s3.ServerSideEncryptionConfiguration ./remote_state_s3.go:450:4: unknown field 'SSEKMSKeyId' in struct literal of type s3.ServerSideEncryptionConfiguration FAIL _/root/go-project/terragrunt/remote [build failed]
[root@localhost remote]# go test -v -parallel 128 # _/root/go-project/terragrunt/remote ./remote_state_s3.go:459: Logger.Printf format %s reads arg #2, but call has 1 arg FAIL _/root/go-project/terragrunt/remote [build failed]
@brikis98 Tested with:
Haven't tested it with real AWS account yet - fixed 3 bugs and it should be fine now. |
@brikis98 I guess I might be a geek... that couldn't resist the temptation - tested with an AWS account:
|
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.
OK, code changes and tests look great!
If you can update the README accordingly, this is good to merge. Thanks!
@brikis98 Perfect - will update the README accordingly today in the evening. Thank you / Спасибо! |
Co-Authored-By: Xtigyro <Xtigyro@users.noreply.github.com>
Co-Authored-By: Xtigyro <Xtigyro@users.noreply.github.com>
I'll retest it as well today in the evening - before coming back here and letting you know the status. |
@brikis98 Tests - still OK. |
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.
Fantastic, thank you. I'll merge now and let the tests run. If they pass, I'll issue a new release and share the link here.
This is a follow-up to #645. The tests were failing with the error: ``` InvalidTargetBucketForLogging: You must give the log-delivery group WRITE and READ_ACP permissions to the target bucket ``` This PR is an attempt to grant those permissions.
Encrypt the bucket and log the access on the AWS S3 Bucket of the Terraform State - both bucket-wide.