fix config remote state s3 and update if needs #2063

leonardobiffi · 2022-04-11T12:42:36Z

Hi, this PR is a suggestion to resolve the KMS config when the bucket is created, as the default key KMS ID is not being informed, the bucket has encryption enabled but without the KMS:

I am also suggesting an important change so that buckets that do not comply with the default encryption settings, public access blocking, bucket policy (enforce SSL only), access logging, versioning enabled are updated according to what is defined in the block of config. The disable_bucket_update attribute disable this behavior for anyone managing the bucket outside of terragrunt. This change causes the buckets to be updated when running terragrunt to conform to the config that are already made when the bucket is created.

Another fix is for the bucket policy, where currently only EnforcedTLS is being defined, the first policy configured for RootAccess is overwritten.
What I changed is to check the policy already configured and be attached to the existing one, as shown in the image below

Closes #1770
Closes #1143
Closes #1106

remote/remote_state_s3.go

remote/remote_state_s3_test.go

remote/remote_state_s3.go

denis256 · 2022-04-22T11:45:53Z

Noticed that integration tests failed, each run different tests, I was wondering merge from master(to include #2073) will help tests to pass

https://app.circleci.com/pipelines/github/gruntwork-io/terragrunt?branch=pull-request-2063&filter=all

denis256 · 2022-04-22T11:45:53Z

Noticed that integration tests failed, each run different tests, I was wondering merge from master(to include #2073) will help tests to pass

https://app.circleci.com/pipelines/github/gruntwork-io/terragrunt?branch=pull-request-2063&filter=all

leonardobiffi · 2022-04-26T11:39:50Z

@denis256 Hi,
Did this change in the integration test work?
Do I need to change something in PR?

denis256 · 2022-04-26T17:25:20Z

@leonardobiffi try to merge the last changes from master and we will see if it will help to pass more integration tests

…config-remote-state-s3

denis256 · 2022-04-27T15:26:38Z

Build passed 👍

leonardobiffi · 2022-05-02T11:13:03Z

@denis256 when this PR will be release?

denis256 · 2022-05-03T09:02:16Z

Hi, I think this week, I was doing more tests to see if will appear any issue when switching from older versions

leonardobiffi · 2022-05-03T11:58:22Z

@denis256 awesome, let me known if anything is wrong

yorinasub17

This LGTM as well! Thanks for the contribution! I had one small request, but once that's addressed, we can move forward with this.

cc @denis256

yorinasub17 · 2022-05-04T14:40:43Z

remote/remote_state_s3.go

+type S3BucketConfig struct {
+	Versioning    string
+	SSEEncryption string
+	RootAccess    string
+	EnforcedTLS   string
+	AccessLogging string
+	PublicAccess  string
+}


It looks like value of the string is immaterial in configBucket, and is only used to track if these attributes need updating. Is that true?

If so, these should be bool instead of string and the struct should be renamed to something like S3BucketUpdatesRequired.

@yorinasub17 Hi, it does make sense. I made the suggested adjustment and the necessary corrections. Let me know if I'm missing something

yorinasub17

Updates LGTM! I'll kick off a regression build and if it passes, we can merge this in!

denis256 · 2022-05-06T09:05:55Z

Noticed that still fail test TestTerragruntStackCommands, 3 runs - 3 failures:

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

text = "[I am a mgmt vpc template. I have no dependencies.]"
=== CONT  TestTerragruntStackCommands
    integration_test.go:3716: Failed to run Terragrunt command 'terragrunt apply-all --terragrunt-non-interactive --terragrunt-working-dir /tmp/terragrunt-test076015731/fixture-stack/mgmt' due to error: 2 errors occurred:
        	* Cannot process module Module /tmp/terragrunt-test076015731/fixture-stack/mgmt/bastion-host (excluded: false, assume applied: false, dependencies: [/tmp/terragrunt-test076015731/fixture-stack/mgmt/vpc, /tmp/terragrunt-test076015731/fixture-stack/mgmt/kms-master-key]) because one of its dependencies, Module /tmp/terragrunt-test076015731/fixture-stack/mgmt/kms-master-key (excluded: false, assume applied: false, dependencies: []), finished with an error: NoSuchPublicAccessBlockConfiguration: The public access block configuration was not found
        	status code: 404, request id: 0QH1FH92JDDZNA0B, host id: +mMt9VWzhriq9ozRqh2/W0ko1XcwmIZy97r3j1tpTgDN/yjcU32ru4Tsm9W+vrhjYKsLaNgwFHA=
        	* NoSuchPublicAccessBlockConfiguration: The public access block configuration was not found
        	status code: 404, request id: 0QH1FH92JDDZNA0B, host id: +mMt9VWzhriq9ozRqh2/W0ko1XcwmIZy97r3j1tpTgDN/yjcU32ru4Tsm9W+vrhjYKsLaNgwFHA=
        
        
        
        Stdout: (see log output above)
        
        Stderr: (see log output above)

Can be a concurrency issue? when multiple tests try to change S3 configuration?

leonardobiffi · 2022-05-06T12:27:02Z

Noticed that still fail test TestTerragruntStackCommands, 3 runs - 3 failures:

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

text = "[I am a mgmt vpc template. I have no dependencies.]"
=== CONT  TestTerragruntStackCommands
    integration_test.go:3716: Failed to run Terragrunt command 'terragrunt apply-all --terragrunt-non-interactive --terragrunt-working-dir /tmp/terragrunt-test076015731/fixture-stack/mgmt' due to error: 2 errors occurred:
        	* Cannot process module Module /tmp/terragrunt-test076015731/fixture-stack/mgmt/bastion-host (excluded: false, assume applied: false, dependencies: [/tmp/terragrunt-test076015731/fixture-stack/mgmt/vpc, /tmp/terragrunt-test076015731/fixture-stack/mgmt/kms-master-key]) because one of its dependencies, Module /tmp/terragrunt-test076015731/fixture-stack/mgmt/kms-master-key (excluded: false, assume applied: false, dependencies: []), finished with an error: NoSuchPublicAccessBlockConfiguration: The public access block configuration was not found
        	status code: 404, request id: 0QH1FH92JDDZNA0B, host id: +mMt9VWzhriq9ozRqh2/W0ko1XcwmIZy97r3j1tpTgDN/yjcU32ru4Tsm9W+vrhjYKsLaNgwFHA=
        	* NoSuchPublicAccessBlockConfiguration: The public access block configuration was not found
        	status code: 404, request id: 0QH1FH92JDDZNA0B, host id: +mMt9VWzhriq9ozRqh2/W0ko1XcwmIZy97r3j1tpTgDN/yjcU32ru4Tsm9W+vrhjYKsLaNgwFHA=
        
        
        
        Stdout: (see log output above)
        
        Stderr: (see log output above)

Can be a concurrency issue? when multiple tests try to change S3 configuration?

@denis256 Hi, i fixed this error and the test passed. Can you validate?

See here this error message: https://github.com/aws/aws-sdk-go/blob/main/service/s3control/errors.go#L50

denis256 · 2022-05-06T17:15:22Z

I see the build started to pass, I'm still somehow suspect since the last change is to shallow NoSuchPublicAccessBlockConfiguration error, and before last change individual run of TestTerragruntStackCommands passed but it failed when the entire integration_test.go was executed

leonardobiffi · 2022-05-09T16:28:21Z

@denis256 Hi,
The integration_test.go still failing because of update in s3?
I couldn`t see circleCI pipeline

denis256 · 2022-05-09T18:00:48Z

Hi, tests are passing all, I was wondering if is a good solution to shallow error NoSuchPublicAccessBlockConfiguration

leonardobiffi · 2022-05-09T20:57:42Z

@denis256
Looking at this aws doc, I understood that this error happens for accounts that don't have the option to configure S3 PublicAccess at the account level

https://docs.aws.amazon.com/sdk-for-go/api/service/s3control/#S3Control.GetPublicAccessBlock
https://docs.aws.amazon.com/AmazonS3/latest/userguide/configuring-block-public-access-account.html

What do you think?

denis256 · 2022-05-10T10:20:05Z

Hello,
error make sense to handle for buckets without PublicAccess configured,
but it doesn't explain why individual execution of TestTerragruntStackCommands pass successfully, but run of entire integration tests fails on TestTerragruntStackCommands (all this before commit with handling of error), so far I think it is an issue of tests not of implementation and can be handled in other PR

denis256 · 2022-05-11T12:11:21Z

Released in https://github.com/gruntwork-io/terragrunt/releases/tag/v0.37.0

Leonardo Biffi added 4 commits April 4, 2022 11:15

some fixes for staticheck

7701c4b

check if bucket is config and updated

a09d14a

check if policy is set

71e2b86

update doc reference

15728ef

leonardobiffi requested review from yorinasub17, rhoboat, zackproser and denis256 as code owners April 11, 2022 12:42

denis256 reviewed Apr 19, 2022

View reviewed changes

remote/remote_state_s3.go Outdated Show resolved Hide resolved

remote/remote_state_s3_test.go Outdated Show resolved Hide resolved

remote/remote_state_s3.go Outdated Show resolved Hide resolved

fix check bucket policy and nested blocks

266828a

Merge branch 'master' of github.com:gruntwork-io/terragrunt into fix/…

950c29c

…config-remote-state-s3

leonardobiffi requested a review from denis256 April 27, 2022 11:19

denis256 previously approved these changes Apr 27, 2022

View reviewed changes

yorinasub17 reviewed May 4, 2022

View reviewed changes

change struct bucket updates required

6cba585

leonardobiffi dismissed denis256’s stale review via 6cba585 May 4, 2022 15:16

yorinasub17 previously approved these changes May 5, 2022

View reviewed changes

fix err public access block

bba4bea

leonardobiffi dismissed yorinasub17’s stale review via bba4bea May 6, 2022 12:24

denis256 approved these changes May 10, 2022

View reviewed changes

denis256 merged commit 05b1216 into gruntwork-io:master May 11, 2022

rickys-splunk mentioned this pull request May 12, 2022

v0.37.0: Failure to parse S3 backend policy document #2104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix config remote state s3 and update if needs #2063

fix config remote state s3 and update if needs #2063

leonardobiffi commented Apr 11, 2022

denis256 commented Apr 22, 2022

denis256 commented Apr 22, 2022

leonardobiffi commented Apr 26, 2022

denis256 commented Apr 26, 2022

denis256 commented Apr 27, 2022

leonardobiffi commented May 2, 2022

denis256 commented May 3, 2022

leonardobiffi commented May 3, 2022

yorinasub17 left a comment

yorinasub17 May 4, 2022

leonardobiffi May 4, 2022

yorinasub17 left a comment

denis256 commented May 6, 2022 •

edited

Loading

leonardobiffi commented May 6, 2022 •

edited

Loading

denis256 commented May 6, 2022

leonardobiffi commented May 9, 2022

denis256 commented May 9, 2022

leonardobiffi commented May 9, 2022

denis256 commented May 10, 2022 •

edited

Loading

denis256 commented May 11, 2022

fix config remote state s3 and update if needs #2063

fix config remote state s3 and update if needs #2063

Conversation

leonardobiffi commented Apr 11, 2022

denis256 commented Apr 22, 2022

denis256 commented Apr 22, 2022

leonardobiffi commented Apr 26, 2022

denis256 commented Apr 26, 2022

denis256 commented Apr 27, 2022

leonardobiffi commented May 2, 2022

denis256 commented May 3, 2022

leonardobiffi commented May 3, 2022

yorinasub17 left a comment

Choose a reason for hiding this comment

yorinasub17 May 4, 2022

Choose a reason for hiding this comment

leonardobiffi May 4, 2022

Choose a reason for hiding this comment

yorinasub17 left a comment

Choose a reason for hiding this comment

denis256 commented May 6, 2022 • edited Loading

leonardobiffi commented May 6, 2022 • edited Loading

denis256 commented May 6, 2022

leonardobiffi commented May 9, 2022

denis256 commented May 9, 2022

leonardobiffi commented May 9, 2022

denis256 commented May 10, 2022 • edited Loading

denis256 commented May 11, 2022

denis256 commented May 6, 2022 •

edited

Loading

leonardobiffi commented May 6, 2022 •

edited

Loading

denis256 commented May 10, 2022 •

edited

Loading