Skip to content

Commit 7f0d878

Browse files
jbonzojbonzo
committed
Normalize empty prefix as nil when reading lifecycle configurations
1 parent 6210d27 commit 7f0d878

File tree

2 files changed

+128
-38
lines changed

2 files changed

+128
-38
lines changed

‎internal/service/s3/bucket_lifecycle_configuration.go‎

Lines changed: 5 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ import (
3333
"github.com/hashicorp/terraform-plugin-framework/schema/validator"
3434
"github.com/hashicorp/terraform-plugin-framework/types"
3535
"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
36-
"github.com/hashicorp/terraform-plugin-log/tflog"
3736
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"
3837
"github.com/hashicorp/terraform-provider-aws/internal/errs/fwdiag"
3938
"github.com/hashicorp/terraform-provider-aws/internal/framework"
@@ -462,7 +461,7 @@ func (r *bucketLifecycleConfigurationResource) Read(ctx context.Context, request
462461
return tfresource.NonRetryableError(err)
463462
}
464463

465-
if lastOutput == nil || !lifecycleConfigEqual(ctx, lastOutput.TransitionDefaultMinimumObjectSize, lastOutput.Rules, output.TransitionDefaultMinimumObjectSize, output.Rules) {
464+
if lastOutput == nil || !lifecycleConfigEqual(lastOutput.TransitionDefaultMinimumObjectSize, lastOutput.Rules, output.TransitionDefaultMinimumObjectSize, output.Rules) {
466465
lastOutput = output
467466
return tfresource.RetryableError(fmt.Errorf("S3 Bucket Lifecycle Configuration (%s) has not stablized; retrying", bucket))
468467
}
@@ -522,11 +521,6 @@ func (r *bucketLifecycleConfigurationResource) Update(ctx context.Context, reque
522521
Rules: rules,
523522
}
524523

525-
tflog.Debug(ctx, "Updating S3 Bucket Lifecycle Configuration", map[string]interface{}{
526-
"Bucket": bucket,
527-
"Rules": rules,
528-
})
529-
530524
input.LifecycleConfiguration = &lifecycleConfiguraton
531525

532526
_, err := tfresource.RetryWhenAWSErrCodeEquals(ctx, bucketPropagationTimeout, func(ctx context.Context) (any, error) {
@@ -635,20 +629,11 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck
635629

636630
for i := range output.Rules {
637631
rule := &output.Rules[i]
638-
if (*rule).Prefix != nil && *(*rule).Prefix == "" {
639-
tflog.Debug(ctx, "Found S3 Bucket Lifecycle Configuration rule with empty Prefix", map[string]interface{}{
640-
"Bucket": bucket,
641-
"RuleID": *(*rule).ID,
642-
})
632+
if (*rule).Prefix != nil && (*(*rule).Prefix == "" || *(*rule).Prefix == "/") {
643633
(*rule).Prefix = nil
644634
}
645635
}
646636

647-
tflog.Debug(ctx, "Found S3 Bucket Lifecycle Configuration post mutate", map[string]interface{}{
648-
"Bucket": bucket,
649-
"Rules": output.Rules,
650-
})
651-
652637
if err != nil {
653638
return nil, err
654639
}
@@ -660,36 +645,18 @@ func findBucketLifecycleConfiguration(ctx context.Context, conn *s3.Client, buck
660645
return output, nil
661646
}
662647

663-
func lifecycleConfigEqual(ctx context.Context, transitionMinSize1 awstypes.TransitionDefaultMinimumObjectSize, rules1 []awstypes.LifecycleRule, transitionMinSize2 awstypes.TransitionDefaultMinimumObjectSize, rules2 []awstypes.LifecycleRule) bool {
648+
func lifecycleConfigEqual(transitionMinSize1 awstypes.TransitionDefaultMinimumObjectSize, rules1 []awstypes.LifecycleRule, transitionMinSize2 awstypes.TransitionDefaultMinimumObjectSize, rules2 []awstypes.LifecycleRule) bool {
664649
if transitionMinSize1 != transitionMinSize2 {
665-
tflog.Debug(ctx, "Different transition minimum object size", map[string]interface{}{
666-
"transition_min_size1": transitionMinSize1,
667-
"transition_min_size2": transitionMinSize2,
668-
})
669650
return false
670651
}
671652

672653
if len(rules1) != len(rules2) {
673-
tflog.Debug(ctx, "Different number of rules", map[string]interface{}{
674-
"len(rules1)": len(rules1),
675-
"len(rules2)": len(rules2),
676-
})
677654
return false
678655
}
679656

680657
for _, rule1 := range rules1 {
681658
if !slices.ContainsFunc(rules2, func(rule2 awstypes.LifecycleRule) bool {
682-
683-
equal := reflect.DeepEqual(rule1, rule2)
684-
685-
if !equal {
686-
tflog.Debug(ctx, "Rules are not equal", map[string]interface{}{
687-
"output": rule1,
688-
"input": rule2,
689-
})
690-
}
691-
692-
return equal
659+
return reflect.DeepEqual(rule1, rule2)
693660
}) {
694661
return false
695662
}
@@ -710,7 +677,7 @@ func statusLifecycleConfigEquals(ctx context.Context, conn *s3.Client, bucket, o
710677
return nil, "", err
711678
}
712679

713-
return output, strconv.FormatBool(lifecycleConfigEqual(ctx, output.TransitionDefaultMinimumObjectSize, output.Rules, transitionMinSize, rules)), nil
680+
return output, strconv.FormatBool(lifecycleConfigEqual(output.TransitionDefaultMinimumObjectSize, output.Rules, transitionMinSize, rules)), nil
714681
}
715682
}
716683

‎internal/service/s3/bucket_lifecycle_configuration_test.go‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"testing"
1111
"time"
1212

13+
"github.com/aws/aws-sdk-go-v2/aws"
1314
"github.com/aws/aws-sdk-go-v2/service/s3"
1415
"github.com/aws/aws-sdk-go-v2/service/s3/types"
1516
"github.com/hashicorp/terraform-plugin-testing/compare"
@@ -5267,3 +5268,125 @@ resource "aws_s3_bucket" "test" {
52675268
}
52685269
`, rName)
52695270
}
5271+
5272+
func TestNormalizeEmptyPrefix(t *testing.T) {
5273+
t.Parallel()
5274+
5275+
testCases := []struct {
5276+
name string
5277+
input *types.LifecycleRule
5278+
expected *types.LifecycleRule
5279+
}{
5280+
{
5281+
name: "empty string prefix becomes nil",
5282+
input: &types.LifecycleRule{
5283+
ID: aws.String("test-rule"),
5284+
Prefix: aws.String(""),
5285+
Status: types.ExpirationStatusEnabled,
5286+
},
5287+
expected: &types.LifecycleRule{
5288+
ID: aws.String("test-rule"),
5289+
Prefix: nil,
5290+
Status: types.ExpirationStatusEnabled,
5291+
},
5292+
},
5293+
{
5294+
name: "forward slash prefix becomes nil",
5295+
input: &types.LifecycleRule{
5296+
ID: aws.String("test-rule"),
5297+
Prefix: aws.String("/"),
5298+
Status: types.ExpirationStatusEnabled,
5299+
},
5300+
expected: &types.LifecycleRule{
5301+
ID: aws.String("test-rule"),
5302+
Prefix: nil,
5303+
Status: types.ExpirationStatusEnabled,
5304+
},
5305+
},
5306+
{
5307+
name: "nil prefix remains nil",
5308+
input: &types.LifecycleRule{
5309+
ID: aws.String("test-rule"),
5310+
Prefix: nil,
5311+
Status: types.ExpirationStatusEnabled,
5312+
},
5313+
expected: &types.LifecycleRule{
5314+
ID: aws.String("test-rule"),
5315+
Prefix: nil,
5316+
Status: types.ExpirationStatusEnabled,
5317+
},
5318+
},
5319+
{
5320+
name: "non-empty prefix unchanged",
5321+
input: &types.LifecycleRule{
5322+
ID: aws.String("test-rule"),
5323+
Prefix: aws.String("logs/"),
5324+
Status: types.ExpirationStatusEnabled,
5325+
},
5326+
expected: &types.LifecycleRule{
5327+
ID: aws.String("test-rule"),
5328+
Prefix: aws.String("logs/"),
5329+
Status: types.ExpirationStatusEnabled,
5330+
},
5331+
},
5332+
}
5333+
5334+
for _, tc := range testCases {
5335+
t.Run(tc.name, func(t *testing.T) {
5336+
// Simulate what findBucketLifecycleConfiguration does
5337+
if tc.input.Prefix != nil && (*tc.input.Prefix == "" || *tc.input.Prefix == "/") {
5338+
tc.input.Prefix = nil
5339+
}
5340+
5341+
// Verify normalization
5342+
if tc.input.Prefix == nil && tc.expected.Prefix != nil {
5343+
t.Errorf("Expected non-nil prefix, got nil")
5344+
} else if tc.input.Prefix != nil && tc.expected.Prefix == nil {
5345+
t.Errorf("Expected nil prefix, got %v", *tc.input.Prefix)
5346+
} else if tc.input.Prefix != nil && tc.expected.Prefix != nil && *tc.input.Prefix != *tc.expected.Prefix {
5347+
t.Errorf("Expected prefix %v, got %v", *tc.expected.Prefix, *tc.input.Prefix)
5348+
}
5349+
})
5350+
}
5351+
}
5352+
5353+
func TestExpandNormalizeSymmetry(t *testing.T) {
5354+
t.Parallel()
5355+
5356+
testCases := []struct {
5357+
name string
5358+
configValue string
5359+
afterExpand *string
5360+
afterNormalize *string
5361+
}{
5362+
{
5363+
name: "empty string config",
5364+
configValue: "",
5365+
afterExpand: nil, // fwflex.EmptyStringAsNull converts "" to nil
5366+
afterNormalize: nil, // normalization keeps nil as nil
5367+
},
5368+
{
5369+
name: "non-empty config",
5370+
configValue: "logs/",
5371+
afterExpand: aws.String("logs/"),
5372+
afterNormalize: aws.String("logs/"),
5373+
},
5374+
}
5375+
5376+
for _, tc := range testCases {
5377+
t.Run(tc.name, func(t *testing.T) {
5378+
// This test proves the expand->API->normalize cycle is symmetric
5379+
// 1. Config "" -> expand -> nil (sent to API)
5380+
// 2. API returns "" (Ceph) or nil (AWS)
5381+
// 3. normalize -> nil (consistent internal representation)
5382+
5383+
if tc.afterExpand == nil && tc.afterNormalize != nil {
5384+
t.Errorf("Symmetry broken: expand returned nil but normalize expects %v", *tc.afterNormalize)
5385+
} else if tc.afterExpand != nil && tc.afterNormalize == nil {
5386+
t.Errorf("Symmetry broken: expand returned %v but normalize expects nil", *tc.afterExpand)
5387+
} else if tc.afterExpand != nil && tc.afterNormalize != nil && *tc.afterExpand != *tc.afterNormalize {
5388+
t.Errorf("Symmetry broken: expand returned %v but normalize expects %v", *tc.afterExpand, *tc.afterNormalize)
5389+
}
5390+
})
5391+
}
5392+
}

0 commit comments

Comments
 (0)