Skip to content

Commit 0e73b7f

Browse files
lev-oklev-ok
authored
fix: AWS S3 backend access logging bucket not versioning (#4246)
1 parent 8d8e92d commit 0e73b7f

File tree

3 files changed

+74
-18
lines changed

3 files changed

+74
-18
lines changed

‎internal/remotestate/backend/s3/client.go

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ func (client *Client) UpdateS3BucketIfNecessary(ctx context.Context, bucketName
184184
if bucketUpdatesRequired.Versioning {
185185
if client.SkipBucketVersioning {
186186
client.logger.Debugf("Versioning is disabled for the remote state S3 bucket %s using 'skip_bucket_versioning' config.", bucketName)
187-
} else if err := client.EnableVersioningForS3Bucket(); err != nil {
187+
} else if err := client.EnableVersioningForS3Bucket(bucketName); err != nil {
188188
return err
189189
}
190190
}
@@ -295,6 +295,12 @@ func (client *Client) configureAccessLogBucket(ctx context.Context, opts *option
295295
}
296296
}
297297

298+
if client.SkipBucketVersioning {
299+
client.logger.Debugf("Versioning is disabled for the remote state S3 bucket %s using 'skip_bucket_versioning' config.", client.AccessLoggingBucketName)
300+
} else if err := client.EnableVersioningForS3Bucket(client.AccessLoggingBucketName); err != nil {
301+
return err
302+
}
303+
298304
return nil
299305
}
300306

@@ -479,7 +485,7 @@ func (client *Client) CreateS3BucketWithVersioningSSEncryptionAndAccessLogging(c
479485

480486
if client.SkipBucketVersioning {
481487
client.logger.Debugf("Versioning is disabled for the remote state S3 bucket %s using 'skip_bucket_versioning' config.", cfg.Bucket)
482-
} else if err := client.EnableVersioningForS3Bucket(); err != nil {
488+
} else if err := client.EnableVersioningForS3Bucket(cfg.Bucket); err != nil {
483489
return err
484490
}
485491

@@ -939,21 +945,19 @@ func (client *Client) EnableEnforcedTLSAccesstoS3Bucket(bucket string) error {
939945
// Helper function to check if the enforced TLS policy is enabled for the bucket
940946

941947
// EnableVersioningForS3Bucket enables versioning for the S3 bucket specified in the given config.
942-
func (client *Client) EnableVersioningForS3Bucket() error {
943-
cfg := &client.RemoteStateConfigS3
944-
945-
client.logger.Debugf("Enabling versioning on S3 bucket %s", cfg.Bucket)
948+
func (client *Client) EnableVersioningForS3Bucket(bucketName string) error {
949+
client.logger.Debugf("Enabling versioning on S3 bucket %s", bucketName)
946950
input := s3.PutBucketVersioningInput{
947-
Bucket: aws.String(cfg.Bucket),
951+
Bucket: aws.String(bucketName),
948952
VersioningConfiguration: &s3.VersioningConfiguration{Status: aws.String(s3.BucketVersioningStatusEnabled)},
949953
}
950954

951955
_, err := client.PutBucketVersioning(&input)
952956
if err != nil {
953-
return errors.Errorf("error enabling versioning on S3 bucket %s: %w", cfg.Bucket, err)
957+
return errors.Errorf("error enabling versioning on S3 bucket %s: %w", bucketName, err)
954958
}
955959

956-
client.logger.Debugf("Enabled versioning on S3 bucket %s", cfg.Bucket)
960+
client.logger.Debugf("Enabled versioning on S3 bucket %s", bucketName)
957961

958962
return nil
959963
}

‎test/fixtures/s3-backend/common.hcl

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ feature "enable_lock_table_ssencryption" {
66
default = false
77
}
88

9+
feature "access_logging_bucket" {
10+
default = ""
11+
}
12+
913
feature "key_prefix" {
1014
default = ""
1115
}
@@ -22,7 +26,8 @@ remote_state {
2226
region = "__FILL_IN_REGION__"
2327
dynamodb_table = "__FILL_IN_LOCK_TABLE_NAME__"
2428

25-
skip_bucket_versioning = feature.disable_versioning.value
29+
skip_bucket_versioning = feature.disable_versioning.value
2630
enable_lock_table_ssencryption = feature.enable_lock_table_ssencryption.value
31+
accesslogging_bucket_name = feature.access_logging_bucket.value
2732
}
2833
}

‎test/integration_aws_test.go

Lines changed: 55 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func TestAwsBootstrapBackend(t *testing.T) {
8080

8181
require.NoError(t, err)
8282

83-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, nil)
83+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, true, nil)
8484
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, false)
8585
},
8686
},
@@ -92,7 +92,7 @@ func TestAwsBootstrapBackend(t *testing.T) {
9292

9393
require.NoError(t, err)
9494

95-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, nil)
95+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, true, nil)
9696
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, true)
9797
},
9898
},
@@ -104,7 +104,7 @@ func TestAwsBootstrapBackend(t *testing.T) {
104104

105105
require.NoError(t, err)
106106

107-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, nil)
107+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, true, nil)
108108
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, false)
109109
},
110110
},
@@ -161,7 +161,7 @@ func TestAwsBootstrapBackendLegacyBehavior(t *testing.T) {
161161
_, stderr, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --all --non-interactive --log-level debug --working-dir "+rootPath+" apply")
162162
require.NoError(t, err)
163163

164-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, nil)
164+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, true, nil)
165165
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, false)
166166

167167
assert.Contains(t, stderr, "Use the explicit `--backend-bootstrap` flag to automatically provision backend resources before they're needed.")
@@ -204,7 +204,7 @@ func TestAwsBootstrapBackendWithoutVersioning(t *testing.T) {
204204
_, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --all --non-interactive --log-level debug --strict-control require-explicit-bootstrap --working-dir "+rootPath+" --feature disable_versioning=true --backend-bootstrap apply")
205205
require.NoError(t, err)
206206

207-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, nil)
207+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, false, nil)
208208
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, false)
209209

210210
_, _, err = helpers.RunTerragruntCommandWithOutput(t, "terragrunt --non-interactive --log-level debug --working-dir "+rootPath+" --feature disable_versioning=true backend delete --all")
@@ -214,6 +214,36 @@ func TestAwsBootstrapBackendWithoutVersioning(t *testing.T) {
214214
require.NoError(t, err)
215215
}
216216

217+
func TestAwsBootstrapBackendWithAccessLogging(t *testing.T) {
218+
t.Parallel()
219+
220+
helpers.CleanupTerraformFolder(t, testFixtureS3Backend)
221+
tmpEnvPath := helpers.CopyEnvironment(t, testFixtureS3Backend)
222+
rootPath := util.JoinPath(tmpEnvPath, testFixtureS3Backend)
223+
224+
testID := strings.ToLower(helpers.UniqueID())
225+
226+
s3BucketName := "terragrunt-test-bucket-" + testID
227+
s3AccessLogsBucketName := "terragrunt-test-bucket-" + testID + "-access-logs"
228+
dynamoDBName := "terragrunt-test-dynamodb-" + testID
229+
230+
defer func() {
231+
deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region)
232+
deleteS3Bucket(t, s3AccessLogsBucketName, helpers.TerraformRemoteStateS3Region)
233+
cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region)
234+
}()
235+
236+
commonConfigPath := util.JoinPath(rootPath, "common.hcl")
237+
helpers.CopyTerragruntConfigAndFillPlaceholders(t, commonConfigPath, commonConfigPath, s3BucketName, dynamoDBName, helpers.TerraformRemoteStateS3Region)
238+
239+
_, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --all --non-interactive --log-level debug --strict-control require-explicit-bootstrap --working-dir "+rootPath+" --feature access_logging_bucket="+s3AccessLogsBucketName+" --backend-bootstrap apply")
240+
require.NoError(t, err)
241+
242+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, true, nil)
243+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3AccessLogsBucketName, true, nil)
244+
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, false)
245+
}
246+
217247
func TestAwsMigrateBackendWithoutVersioning(t *testing.T) {
218248
t.Parallel()
219249

@@ -238,7 +268,7 @@ func TestAwsMigrateBackendWithoutVersioning(t *testing.T) {
238268
_, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --non-interactive --log-level debug --strict-control require-explicit-bootstrap --working-dir "+unitPath+" --feature disable_versioning=true --backend-bootstrap apply -- -auto-approve")
239269
require.NoError(t, err)
240270

241-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, nil)
271+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, false, nil)
242272
validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t, helpers.TerraformRemoteStateS3Region, dynamoDBName, nil, false)
243273

244274
_, _, err = helpers.RunTerragruntCommandWithOutput(t, "terragrunt --non-interactive --log-level debug --working-dir "+rootPath+" --feature disable_versioning=true backend migrate unit1 unit2")
@@ -471,7 +501,7 @@ func TestAwsWorksWithLocalTerraformVersion(t *testing.T) {
471501
var expectedS3Tags = map[string]string{
472502
"owner": "terragrunt integration test",
473503
"name": "Terraform state storage"}
474-
validateS3BucketExistsAndIsTagged(t, helpers.TerraformRemoteStateS3Region, s3BucketName, expectedS3Tags)
504+
validateS3BucketExistsAndIsTaggedAndVersioning(t, helpers.TerraformRemoteStateS3Region, s3BucketName, true, expectedS3Tags)
475505

476506
var expectedDynamoDBTableTags = map[string]string{
477507
"owner": "terragrunt integration test",
@@ -1590,6 +1620,21 @@ func assertS3Tags(t *testing.T, expectedTags map[string]string, bucketName strin
15901620
assert.Equal(t, expectedTags, actualTags, "Did not find expected tags on s3 bucket.")
15911621
}
15921622

1623+
func assertS3BucketVersioning(t *testing.T, bucketName string, versioning bool, client *s3.S3) {
1624+
t.Helper()
1625+
1626+
res, err := client.GetBucketVersioning(&s3.GetBucketVersioningInput{Bucket: aws.String(bucketName)})
1627+
require.NoError(t, err)
1628+
require.NotNil(t, res)
1629+
1630+
if versioning {
1631+
require.NotNil(t, res.Status)
1632+
assert.Equal(t, *res.Status, s3.BucketVersioningStatusEnabled, "Versioning is not enabled for the remote state S3 bucket %s", bucketName)
1633+
} else {
1634+
require.Nil(t, res.Status)
1635+
}
1636+
}
1637+
15931638
// Check that the DynamoDB table of the given name and region exists. Terragrunt should create this table during the test.
15941639
// Also check if table got tagged properly
15951640
func validateDynamoDBTableExistsAndIsTaggedAndIsSSEncrypted(t *testing.T, awsRegion string, tableName string, expectedTags map[string]string, expectedSSEncrypted bool) {
@@ -1651,7 +1696,7 @@ func doesDynamoDBTableItemExist(t *testing.T, awsRegion string, tableName, key s
16511696

16521697
// Check that the S3 Bucket of the given name and region exists. Terragrunt should create this bucket during the test.
16531698
// Also check if bucket got tagged properly and that public access is disabled completely.
1654-
func validateS3BucketExistsAndIsTagged(t *testing.T, awsRegion string, bucketName string, expectedTags map[string]string) {
1699+
func validateS3BucketExistsAndIsTaggedAndVersioning(t *testing.T, awsRegion string, bucketName string, versioning bool, expectedTags map[string]string) {
16551700
t.Helper()
16561701

16571702
client := helpers.CreateS3ClientForTest(t, awsRegion)
@@ -1663,6 +1708,8 @@ func validateS3BucketExistsAndIsTagged(t *testing.T, awsRegion string, bucketNam
16631708
assertS3Tags(t, expectedTags, bucketName, client)
16641709
}
16651710

1711+
assertS3BucketVersioning(t, bucketName, versioning, client)
1712+
16661713
assertS3PublicAccessBlocks(t, client, bucketName)
16671714
}
16681715

0 commit comments

Comments
 (0)