From 6210d27f6a54efb988a7be7ad8756802ec74cecb Mon Sep 17 00:00:00 2001 From: jbonzo <8647805+jbonzo@users.noreply.github.com> Date: Thu, 2 Oct 2025 13:36:39 -0400 Subject: [PATCH 1/5] Normalize empty prefix as nil when reading external resources --- .../s3/bucket_lifecycle_configuration.go | 48 +++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/internal/service/s3/bucket_lifecycle_configuration.go b/internal/service/s3/bucket_lifecycle_configuration.go index fe2014ccaa35..2d5bdb52eec4 100644 --- a/internal/service/s3/bucket_lifecycle_configuration.go +++ b/internal/service/s3/bucket_lifecycle_configuration.go @@ -33,6 +33,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" + "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-provider-aws/internal/errs/fwdiag" "github.com/hashicorp/terraform-provider-aws/internal/framework" @@ -461,7 +462,7 @@ func (r *bucketLifecycleConfigurationResource) Read(ctx context.Context, request return tfresource.NonRetryableError(err) } - if lastOutput == nil || !lifecycleConfigEqual(lastOutput.TransitionDefaultMinimumObjectSize, lastOutput.Rules, output.TransitionDefaultMinimumObjectSize, output.Rules) { + if lastOutput == nil || !lifecycleConfigEqual(ctx, lastOutput.TransitionDefaultMinimumObjectSize, lastOutput.Rules, output.TransitionDefaultMinimumObjectSize, output.Rules) { lastOutput = output return tfresource.RetryableError(fmt.Errorf("S3 Bucket Lifecycle Configuration (%s) has not stablized; retrying", bucket)) } @@ -521,6 +522,11 @@ func (r *bucketLifecycleConfigurationResource) Update(ctx context.Context, reque Rules: rules, } + tflog.Debug(ctx, "Updating S3 Bucket Lifecycle Configuration", map[string]interface{}{ + "Bucket": bucket, + "Rules": rules, + }) + input.LifecycleConfiguration = &lifecycleConfiguraton _, err := tfresource.RetryWhenAWSErrCodeEquals(ctx, bucketPropagationTimeout, func(ctx context.Context) (any, error) { @@ -627,6 +633,22 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck } } + for i := range output.Rules { + rule := &output.Rules[i] + if (*rule).Prefix != nil && *(*rule).Prefix == "" { + tflog.Debug(ctx, "Found S3 Bucket Lifecycle Configuration rule with empty Prefix", map[string]interface{}{ + "Bucket": bucket, + "RuleID": *(*rule).ID, + }) + (*rule).Prefix = nil + } + } + + tflog.Debug(ctx, "Found S3 Bucket Lifecycle Configuration post mutate", map[string]interface{}{ + "Bucket": bucket, + "Rules": output.Rules, + }) + if err != nil { return nil, err } @@ -638,18 +660,36 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck return output, nil } -func lifecycleConfigEqual(transitionMinSize1 awstypes.TransitionDefaultMinimumObjectSize, rules1 []awstypes.LifecycleRule, transitionMinSize2 awstypes.TransitionDefaultMinimumObjectSize, rules2 []awstypes.LifecycleRule) bool { +func lifecycleConfigEqual(ctx context.Context, transitionMinSize1 awstypes.TransitionDefaultMinimumObjectSize, rules1 []awstypes.LifecycleRule, transitionMinSize2 awstypes.TransitionDefaultMinimumObjectSize, rules2 []awstypes.LifecycleRule) bool { if transitionMinSize1 != transitionMinSize2 { + tflog.Debug(ctx, "Different transition minimum object size", map[string]interface{}{ + "transition_min_size1": transitionMinSize1, + "transition_min_size2": transitionMinSize2, + }) return false } if len(rules1) != len(rules2) { + tflog.Debug(ctx, "Different number of rules", map[string]interface{}{ + "len(rules1)": len(rules1), + "len(rules2)": len(rules2), + }) return false } for _, rule1 := range rules1 { if !slices.ContainsFunc(rules2, func(rule2 awstypes.LifecycleRule) bool { - return reflect.DeepEqual(rule1, rule2) + + equal := reflect.DeepEqual(rule1, rule2) + + if !equal { + tflog.Debug(ctx, "Rules are not equal", map[string]interface{}{ + "output": rule1, + "input": rule2, + }) + } + + return equal }) { return false } @@ -670,7 +710,7 @@ func statusLifecycleConfigEquals(ctx context.Context, conn *s3.Client, bucket, o return nil, "", err } - return output, strconv.FormatBool(lifecycleConfigEqual(output.TransitionDefaultMinimumObjectSize, output.Rules, transitionMinSize, rules)), nil + return output, strconv.FormatBool(lifecycleConfigEqual(ctx, output.TransitionDefaultMinimumObjectSize, output.Rules, transitionMinSize, rules)), nil } } From 7f0d878e1bb5dc969597ff55502c0f2fcc6ddac1 Mon Sep 17 00:00:00 2001 From: jbonzo <8647805+jbonzo@users.noreply.github.com> Date: Thu, 2 Oct 2025 21:02:25 -0400 Subject: [PATCH 2/5] Normalize empty prefix as nil when reading lifecycle configurations --- .../s3/bucket_lifecycle_configuration.go | 43 +----- .../s3/bucket_lifecycle_configuration_test.go | 123 ++++++++++++++++++ 2 files changed, 128 insertions(+), 38 deletions(-) diff --git a/internal/service/s3/bucket_lifecycle_configuration.go b/internal/service/s3/bucket_lifecycle_configuration.go index 2d5bdb52eec4..e557a7c99c38 100644 --- a/internal/service/s3/bucket_lifecycle_configuration.go +++ b/internal/service/s3/bucket_lifecycle_configuration.go @@ -33,7 +33,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" - "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-provider-aws/internal/errs/fwdiag" "github.com/hashicorp/terraform-provider-aws/internal/framework" @@ -462,7 +461,7 @@ func (r *bucketLifecycleConfigurationResource) Read(ctx context.Context, request return tfresource.NonRetryableError(err) } - if lastOutput == nil || !lifecycleConfigEqual(ctx, lastOutput.TransitionDefaultMinimumObjectSize, lastOutput.Rules, output.TransitionDefaultMinimumObjectSize, output.Rules) { + if lastOutput == nil || !lifecycleConfigEqual(lastOutput.TransitionDefaultMinimumObjectSize, lastOutput.Rules, output.TransitionDefaultMinimumObjectSize, output.Rules) { lastOutput = output return tfresource.RetryableError(fmt.Errorf("S3 Bucket Lifecycle Configuration (%s) has not stablized; retrying", bucket)) } @@ -522,11 +521,6 @@ func (r *bucketLifecycleConfigurationResource) Update(ctx context.Context, reque Rules: rules, } - tflog.Debug(ctx, "Updating S3 Bucket Lifecycle Configuration", map[string]interface{}{ - "Bucket": bucket, - "Rules": rules, - }) - input.LifecycleConfiguration = &lifecycleConfiguraton _, err := tfresource.RetryWhenAWSErrCodeEquals(ctx, bucketPropagationTimeout, func(ctx context.Context) (any, error) { @@ -635,20 +629,11 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck for i := range output.Rules { rule := &output.Rules[i] - if (*rule).Prefix != nil && *(*rule).Prefix == "" { - tflog.Debug(ctx, "Found S3 Bucket Lifecycle Configuration rule with empty Prefix", map[string]interface{}{ - "Bucket": bucket, - "RuleID": *(*rule).ID, - }) + if (*rule).Prefix != nil && (*(*rule).Prefix == "" || *(*rule).Prefix == "/") { (*rule).Prefix = nil } } - tflog.Debug(ctx, "Found S3 Bucket Lifecycle Configuration post mutate", map[string]interface{}{ - "Bucket": bucket, - "Rules": output.Rules, - }) - if err != nil { return nil, err } @@ -660,36 +645,18 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck return output, nil } -func lifecycleConfigEqual(ctx context.Context, transitionMinSize1 awstypes.TransitionDefaultMinimumObjectSize, rules1 []awstypes.LifecycleRule, transitionMinSize2 awstypes.TransitionDefaultMinimumObjectSize, rules2 []awstypes.LifecycleRule) bool { +func lifecycleConfigEqual(transitionMinSize1 awstypes.TransitionDefaultMinimumObjectSize, rules1 []awstypes.LifecycleRule, transitionMinSize2 awstypes.TransitionDefaultMinimumObjectSize, rules2 []awstypes.LifecycleRule) bool { if transitionMinSize1 != transitionMinSize2 { - tflog.Debug(ctx, "Different transition minimum object size", map[string]interface{}{ - "transition_min_size1": transitionMinSize1, - "transition_min_size2": transitionMinSize2, - }) return false } if len(rules1) != len(rules2) { - tflog.Debug(ctx, "Different number of rules", map[string]interface{}{ - "len(rules1)": len(rules1), - "len(rules2)": len(rules2), - }) return false } for _, rule1 := range rules1 { if !slices.ContainsFunc(rules2, func(rule2 awstypes.LifecycleRule) bool { - - equal := reflect.DeepEqual(rule1, rule2) - - if !equal { - tflog.Debug(ctx, "Rules are not equal", map[string]interface{}{ - "output": rule1, - "input": rule2, - }) - } - - return equal + return reflect.DeepEqual(rule1, rule2) }) { return false } @@ -710,7 +677,7 @@ func statusLifecycleConfigEquals(ctx context.Context, conn *s3.Client, bucket, o return nil, "", err } - return output, strconv.FormatBool(lifecycleConfigEqual(ctx, output.TransitionDefaultMinimumObjectSize, output.Rules, transitionMinSize, rules)), nil + return output, strconv.FormatBool(lifecycleConfigEqual(output.TransitionDefaultMinimumObjectSize, output.Rules, transitionMinSize, rules)), nil } } diff --git a/internal/service/s3/bucket_lifecycle_configuration_test.go b/internal/service/s3/bucket_lifecycle_configuration_test.go index 0ff0d3d23d33..841f5c1772b5 100644 --- a/internal/service/s3/bucket_lifecycle_configuration_test.go +++ b/internal/service/s3/bucket_lifecycle_configuration_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/hashicorp/terraform-plugin-testing/compare" @@ -5267,3 +5268,125 @@ resource "aws_s3_bucket" "test" { } `, rName) } + +func TestNormalizeEmptyPrefix(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + input *types.LifecycleRule + expected *types.LifecycleRule + }{ + { + name: "empty string prefix becomes nil", + input: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: aws.String(""), + Status: types.ExpirationStatusEnabled, + }, + expected: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: nil, + Status: types.ExpirationStatusEnabled, + }, + }, + { + name: "forward slash prefix becomes nil", + input: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: aws.String("/"), + Status: types.ExpirationStatusEnabled, + }, + expected: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: nil, + Status: types.ExpirationStatusEnabled, + }, + }, + { + name: "nil prefix remains nil", + input: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: nil, + Status: types.ExpirationStatusEnabled, + }, + expected: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: nil, + Status: types.ExpirationStatusEnabled, + }, + }, + { + name: "non-empty prefix unchanged", + input: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: aws.String("logs/"), + Status: types.ExpirationStatusEnabled, + }, + expected: &types.LifecycleRule{ + ID: aws.String("test-rule"), + Prefix: aws.String("logs/"), + Status: types.ExpirationStatusEnabled, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Simulate what findBucketLifecycleConfiguration does + if tc.input.Prefix != nil && (*tc.input.Prefix == "" || *tc.input.Prefix == "/") { + tc.input.Prefix = nil + } + + // Verify normalization + if tc.input.Prefix == nil && tc.expected.Prefix != nil { + t.Errorf("Expected non-nil prefix, got nil") + } else if tc.input.Prefix != nil && tc.expected.Prefix == nil { + t.Errorf("Expected nil prefix, got %v", *tc.input.Prefix) + } else if tc.input.Prefix != nil && tc.expected.Prefix != nil && *tc.input.Prefix != *tc.expected.Prefix { + t.Errorf("Expected prefix %v, got %v", *tc.expected.Prefix, *tc.input.Prefix) + } + }) + } +} + +func TestExpandNormalizeSymmetry(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + configValue string + afterExpand *string + afterNormalize *string + }{ + { + name: "empty string config", + configValue: "", + afterExpand: nil, // fwflex.EmptyStringAsNull converts "" to nil + afterNormalize: nil, // normalization keeps nil as nil + }, + { + name: "non-empty config", + configValue: "logs/", + afterExpand: aws.String("logs/"), + afterNormalize: aws.String("logs/"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // This test proves the expand->API->normalize cycle is symmetric + // 1. Config "" -> expand -> nil (sent to API) + // 2. API returns "" (Ceph) or nil (AWS) + // 3. normalize -> nil (consistent internal representation) + + if tc.afterExpand == nil && tc.afterNormalize != nil { + t.Errorf("Symmetry broken: expand returned nil but normalize expects %v", *tc.afterNormalize) + } else if tc.afterExpand != nil && tc.afterNormalize == nil { + t.Errorf("Symmetry broken: expand returned %v but normalize expects nil", *tc.afterExpand) + } else if tc.afterExpand != nil && tc.afterNormalize != nil && *tc.afterExpand != *tc.afterNormalize { + t.Errorf("Symmetry broken: expand returned %v but normalize expects %v", *tc.afterExpand, *tc.afterNormalize) + } + }) + } +} From f7841c8cb9f797fd2691c3553bfe5b0aba7f3cff Mon Sep 17 00:00:00 2001 From: jbonzo <8647805+jbonzo@users.noreply.github.com> Date: Thu, 2 Oct 2025 21:04:29 -0400 Subject: [PATCH 3/5] Add changelog entry --- .changelog/1.txt | 3 + .../s3/bucket_lifecycle_configuration.go | 4 +- .../s3/bucket_lifecycle_configuration_test.go | 123 ------------------ 3 files changed, 6 insertions(+), 124 deletions(-) create mode 100644 .changelog/1.txt diff --git a/.changelog/1.txt b/.changelog/1.txt new file mode 100644 index 000000000000..cab245f1d6fb --- /dev/null +++ b/.changelog/1.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_s3_bucket_lifecycle_configuration: Normalize empty prefix values when reading lifecycle configurations from S3-compatible services +``` diff --git a/internal/service/s3/bucket_lifecycle_configuration.go b/internal/service/s3/bucket_lifecycle_configuration.go index e557a7c99c38..d1a45254a8fa 100644 --- a/internal/service/s3/bucket_lifecycle_configuration.go +++ b/internal/service/s3/bucket_lifecycle_configuration.go @@ -627,9 +627,11 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck } } + // For legacy-mode reasons, we normalize empty `prefix` is nil when making requests to S3 and storing internal state. + // Some S3 compatible services might return empty string as an equivalent representation. To maintain a consistent state we should normalize that back to nil. for i := range output.Rules { rule := &output.Rules[i] - if (*rule).Prefix != nil && (*(*rule).Prefix == "" || *(*rule).Prefix == "/") { + if (*rule).Prefix != nil && *(*rule).Prefix == "" { (*rule).Prefix = nil } } diff --git a/internal/service/s3/bucket_lifecycle_configuration_test.go b/internal/service/s3/bucket_lifecycle_configuration_test.go index 841f5c1772b5..0ff0d3d23d33 100644 --- a/internal/service/s3/bucket_lifecycle_configuration_test.go +++ b/internal/service/s3/bucket_lifecycle_configuration_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/hashicorp/terraform-plugin-testing/compare" @@ -5268,125 +5267,3 @@ resource "aws_s3_bucket" "test" { } `, rName) } - -func TestNormalizeEmptyPrefix(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - input *types.LifecycleRule - expected *types.LifecycleRule - }{ - { - name: "empty string prefix becomes nil", - input: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: aws.String(""), - Status: types.ExpirationStatusEnabled, - }, - expected: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: nil, - Status: types.ExpirationStatusEnabled, - }, - }, - { - name: "forward slash prefix becomes nil", - input: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: aws.String("/"), - Status: types.ExpirationStatusEnabled, - }, - expected: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: nil, - Status: types.ExpirationStatusEnabled, - }, - }, - { - name: "nil prefix remains nil", - input: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: nil, - Status: types.ExpirationStatusEnabled, - }, - expected: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: nil, - Status: types.ExpirationStatusEnabled, - }, - }, - { - name: "non-empty prefix unchanged", - input: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: aws.String("logs/"), - Status: types.ExpirationStatusEnabled, - }, - expected: &types.LifecycleRule{ - ID: aws.String("test-rule"), - Prefix: aws.String("logs/"), - Status: types.ExpirationStatusEnabled, - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - // Simulate what findBucketLifecycleConfiguration does - if tc.input.Prefix != nil && (*tc.input.Prefix == "" || *tc.input.Prefix == "/") { - tc.input.Prefix = nil - } - - // Verify normalization - if tc.input.Prefix == nil && tc.expected.Prefix != nil { - t.Errorf("Expected non-nil prefix, got nil") - } else if tc.input.Prefix != nil && tc.expected.Prefix == nil { - t.Errorf("Expected nil prefix, got %v", *tc.input.Prefix) - } else if tc.input.Prefix != nil && tc.expected.Prefix != nil && *tc.input.Prefix != *tc.expected.Prefix { - t.Errorf("Expected prefix %v, got %v", *tc.expected.Prefix, *tc.input.Prefix) - } - }) - } -} - -func TestExpandNormalizeSymmetry(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - configValue string - afterExpand *string - afterNormalize *string - }{ - { - name: "empty string config", - configValue: "", - afterExpand: nil, // fwflex.EmptyStringAsNull converts "" to nil - afterNormalize: nil, // normalization keeps nil as nil - }, - { - name: "non-empty config", - configValue: "logs/", - afterExpand: aws.String("logs/"), - afterNormalize: aws.String("logs/"), - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - // This test proves the expand->API->normalize cycle is symmetric - // 1. Config "" -> expand -> nil (sent to API) - // 2. API returns "" (Ceph) or nil (AWS) - // 3. normalize -> nil (consistent internal representation) - - if tc.afterExpand == nil && tc.afterNormalize != nil { - t.Errorf("Symmetry broken: expand returned nil but normalize expects %v", *tc.afterNormalize) - } else if tc.afterExpand != nil && tc.afterNormalize == nil { - t.Errorf("Symmetry broken: expand returned %v but normalize expects nil", *tc.afterExpand) - } else if tc.afterExpand != nil && tc.afterNormalize != nil && *tc.afterExpand != *tc.afterNormalize { - t.Errorf("Symmetry broken: expand returned %v but normalize expects %v", *tc.afterExpand, *tc.afterNormalize) - } - }) - } -} From f8b5589e5c7bede786e4f43d07988ebc64da60c6 Mon Sep 17 00:00:00 2001 From: jbonzo <8647805+jbonzo@users.noreply.github.com> Date: Fri, 3 Oct 2025 14:23:41 -0400 Subject: [PATCH 4/5] Lint --- internal/service/s3/bucket_lifecycle_configuration.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/service/s3/bucket_lifecycle_configuration.go b/internal/service/s3/bucket_lifecycle_configuration.go index d1a45254a8fa..6ad19085fdc8 100644 --- a/internal/service/s3/bucket_lifecycle_configuration.go +++ b/internal/service/s3/bucket_lifecycle_configuration.go @@ -631,7 +631,9 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck // Some S3 compatible services might return empty string as an equivalent representation. To maintain a consistent state we should normalize that back to nil. for i := range output.Rules { rule := &output.Rules[i] + //nolint:staticcheck // Yes the attribute Prefix is deprecated, but the following functionality is required for compatibility with non AWS systems if (*rule).Prefix != nil && *(*rule).Prefix == "" { + //nolint:staticcheck // Yes the attribute Prefix is deprecated, but the following functionality is required for compatibility with non AWS systems (*rule).Prefix = nil } } From cd759cf2b2e5cfd5b5d59326d5420da8200ecf77 Mon Sep 17 00:00:00 2001 From: jbonzo <8647805+jbonzo@users.noreply.github.com> Date: Fri, 3 Oct 2025 14:49:21 -0400 Subject: [PATCH 5/5] Use aws sdk for pointer conversion --- internal/service/s3/bucket_lifecycle_configuration.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/s3/bucket_lifecycle_configuration.go b/internal/service/s3/bucket_lifecycle_configuration.go index 6ad19085fdc8..9471ac098309 100644 --- a/internal/service/s3/bucket_lifecycle_configuration.go +++ b/internal/service/s3/bucket_lifecycle_configuration.go @@ -632,7 +632,7 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck for i := range output.Rules { rule := &output.Rules[i] //nolint:staticcheck // Yes the attribute Prefix is deprecated, but the following functionality is required for compatibility with non AWS systems - if (*rule).Prefix != nil && *(*rule).Prefix == "" { + if aws.ToString((*rule).Prefix) == "" { //nolint:staticcheck // Yes the attribute Prefix is deprecated, but the following functionality is required for compatibility with non AWS systems (*rule).Prefix = nil }