From 41feb9fda7c2442186a33932597f0bd6536115ad Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Tue, 7 May 2024 15:22:55 -0400 Subject: [PATCH 1/3] Fix aws.s3.Bucket tag drift detection Make sure that `pulumi refresh` detects changes to bucket tagging. The implementation restores aws_s3_legacy_bucket resource participation in the generic tags interceptor. --- patches/0038-Restore-legacy-bucket.patch | 132 ++++++++++++++++++ provider/provider_yaml_test.go | 36 +++++ .../test-programs/regress-3674/Pulumi.yaml | 16 +++ 3 files changed, 184 insertions(+) create mode 100644 provider/test-programs/regress-3674/Pulumi.yaml diff --git a/patches/0038-Restore-legacy-bucket.patch b/patches/0038-Restore-legacy-bucket.patch index 86bcf65c52c..e67dddc621e 100644 --- a/patches/0038-Restore-legacy-bucket.patch +++ b/patches/0038-Restore-legacy-bucket.patch @@ -76,6 +76,54 @@ index deadff7071..6310023478 100644 func (client *AWSClient) S3ConnURICleaningDisabled(ctx context.Context) *s3_sdkv1.S3 { config := client.S3Conn(ctx).Config config.DisableRestProtocolURICleaning = aws_sdkv2.Bool(true) +diff --git a/internal/provider/provider.go b/internal/provider/provider.go +index 9b4ebd5a46..0e7a94e470 100644 +--- a/internal/provider/provider.go ++++ b/internal/provider/provider.go +@@ -16,7 +16,6 @@ import ( + + "github.com/hashicorp/terraform-provider-aws/internal/service/ecr" + "github.com/hashicorp/terraform-provider-aws/internal/service/gamelift" +- "github.com/hashicorp/terraform-provider-aws/internal/service/s3legacy" + + "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" + awsbase "github.com/hashicorp/aws-sdk-go-base/v2" +@@ -264,8 +263,6 @@ func New(ctx context.Context) (*schema.Provider, error) { + }, + + ResourcesMap: map[string]*schema.Resource{ +- "aws_s3_bucket_legacy": s3legacy.ResourceBucketLegacy(), +- + "aws_gamelift_matchmaking_configuration": gamelift.ResourceMatchMakingConfiguration(), + "aws_gamelift_matchmaking_rule_set": gamelift.ResourceMatchmakingRuleSet(), + }, +@@ -278,7 +275,7 @@ func New(ctx context.Context) (*schema.Provider, error) { + var errs []error + servicePackageMap := make(map[string]conns.ServicePackage) + +- for _, sp := range servicePackages(ctx) { ++ for _, sp := range servicePackagesAll(ctx) { + servicePackageName := sp.ServicePackageName() + servicePackageMap[servicePackageName] = sp + +diff --git a/internal/provider/service_packages_all.go b/internal/provider/service_packages_all.go +new file mode 100644 +index 0000000000..51ca53f883 +--- /dev/null ++++ b/internal/provider/service_packages_all.go +@@ -0,0 +1,12 @@ ++package provider ++ ++import ( ++ "context" ++ ++ "github.com/hashicorp/terraform-provider-aws/internal/conns" ++ "github.com/hashicorp/terraform-provider-aws/internal/service/s3legacy" ++) ++ ++func servicePackagesAll(ctx context.Context) []conns.ServicePackage { ++ return append(servicePackages(ctx), s3legacy.ServicePackage(ctx)) ++} diff --git a/internal/service/s3/service_package_bwcompat.go b/internal/service/s3/service_package_bwcompat.go new file mode 100644 index 0000000000..4278d1e70a @@ -161,3 +209,87 @@ index 007c2f2dc1..5f64a814be 100644 } arn := arn.ARN{ +diff --git a/internal/service/s3legacy/service_package.go b/internal/service/s3legacy/service_package.go +new file mode 100644 +index 0000000000..5d2ea27364 +--- /dev/null ++++ b/internal/service/s3legacy/service_package.go +@@ -0,0 +1,78 @@ ++package s3legacy ++ ++import ( ++ "context" ++ ++ "github.com/hashicorp/terraform-provider-aws/internal/conns" ++ "github.com/hashicorp/terraform-provider-aws/internal/types" ++) ++ ++type servicePackage struct{} ++ ++func (p *servicePackage) FrameworkDataSources(ctx context.Context) []*types.ServicePackageFrameworkDataSource { ++ return []*types.ServicePackageFrameworkDataSource{} ++} ++ ++func (p *servicePackage) FrameworkResources(ctx context.Context) []*types.ServicePackageFrameworkResource { ++ return []*types.ServicePackageFrameworkResource{} ++} ++ ++func (p *servicePackage) SDKDataSources(ctx context.Context) []*types.ServicePackageSDKDataSource { ++ return []*types.ServicePackageSDKDataSource{} ++} ++ ++func (p *servicePackage) SDKResources(ctx context.Context) []*types.ServicePackageSDKResource { ++ return []*types.ServicePackageSDKResource{ ++ { ++ Factory: ResourceBucketLegacy, ++ TypeName: "aws_s3_bucket_legacy", ++ Name: "BucketLegacy", ++ Tags: &types.ServicePackageResourceTags{ ++ IdentifierAttribute: "bucket", ++ ResourceType: "Bucket", ++ }, ++ }, ++ } ++} ++ ++func (p *servicePackage) ServicePackageName() string { ++ return "s3legacy" ++} ++ ++func ServicePackage(ctx context.Context) conns.ServicePackage { ++ return &servicePackage{} ++} ++ ++// import ( ++// "context" ++ ++// "github.com/aws/aws-sdk-go-v2/aws" ++// "github.com/aws/aws-sdk-go-v2/aws/retry" ++// "github.com/aws/aws-sdk-go-v2/service/s3" ++// "github.com/hashicorp/aws-sdk-go-base/v2/tfawserr" ++// "github.com/hashicorp/terraform-provider-aws/internal/conns" ++// "github.com/hashicorp/terraform-provider-aws/names" ++// ) ++ ++// NewClient returns a new AWS SDK for Go v2 client for this service package's AWS API. ++// func (p *servicePackage) NewClient(ctx context.Context, config map[string]any) (*s3.Client, error) { ++// cfg := *(config["aws_sdkv2_config"].(*aws.Config)) ++ ++// return s3.NewFromConfig(cfg, func(o *s3.Options) { ++// if endpoint := config["endpoint"].(string); endpoint != "" { ++// o.BaseEndpoint = aws.String(endpoint) ++// } else if o.Region == names.USEast1RegionID && config["s3_us_east_1_regional_endpoint"].(string) != "regional" { ++// // Maintain the AWS SDK for Go v1 default of using the global endpoint in us-east-1. ++// // See https://github.com/hashicorp/terraform-provider-aws/issues/33028. ++// o.Region = names.GlobalRegionID ++// } ++// o.UsePathStyle = config["s3_use_path_style"].(bool) ++ ++// o.Retryer = conns.AddIsErrorRetryables(cfg.Retryer().(aws.RetryerV2), retry.IsErrorRetryableFunc(func(err error) aws.Ternary { ++// if tfawserr.ErrMessageContains(err, errCodeOperationAborted, "A conflicting conditional operation is currently in progress against this resource. Please try again.") { ++// return aws.TrueTernary ++// } ++// return aws.UnknownTernary // Delegate to configured Retryer. ++// })) ++// }), nil ++// } diff --git a/provider/provider_yaml_test.go b/provider/provider_yaml_test.go index 39fa6a9ca64..952a587856b 100644 --- a/provider/provider_yaml_test.go +++ b/provider/provider_yaml_test.go @@ -6,16 +6,20 @@ package provider import ( + "context" "fmt" "math/rand" "os" "path/filepath" "testing" + "github.com/aws/aws-sdk-go-v2/config" + s3sdk "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/pulumi/providertest/pulumitest" "github.com/pulumi/providertest/pulumitest/assertpreview" "github.com/pulumi/providertest/pulumitest/opttest" "github.com/pulumi/pulumi/sdk/v3/go/common/apitype" + "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -296,3 +300,35 @@ func TestNonIdempotentSnsTopic(t *testing.T) { _, err := ptest.CurrentStack().Up(ptest.Context()) require.ErrorContains(t, err, "already exists") } + +// Make sure that legacy Bucket supports deleting tags out of band and detecting drift. +func TestRegress3674(t *testing.T) { + ptest := pulumiTest(t, filepath.Join("test-programs", "regress-3674")) + upResult := ptest.Up() + bucketName := upResult.Outputs["bucketName"].Value.(string) + deleteBucketTagging(ptest.Context(), bucketName) + result := ptest.Refresh() + t.Logf("%s", result.StdOut) + require.Equal(t, 1, (*result.Summary.ResourceChanges)["update"]) +} + +func configureS3() *s3sdk.Client { + loadOpts := []func(*config.LoadOptions) error{} + if p, ok := os.LookupEnv("AWS_PROFILE"); ok { + loadOpts = append(loadOpts, config.WithSharedConfigProfile(p)) + } + if r, ok := os.LookupEnv("AWS_REGION"); ok { + loadOpts = append(loadOpts, config.WithRegion(r)) + } + cfg, err := config.LoadDefaultConfig(context.TODO(), loadOpts...) + contract.AssertNoErrorf(err, "failed to load AWS config") + return s3sdk.NewFromConfig(cfg) +} + +func deleteBucketTagging(ctx context.Context, awsBucket string) { + s3 := configureS3() + _, err := s3.DeleteBucketTagging(ctx, &s3sdk.DeleteBucketTaggingInput{ + Bucket: &awsBucket, + }) + contract.AssertNoErrorf(err, "failed to delete bucket tagging") +} diff --git a/provider/test-programs/regress-3674/Pulumi.yaml b/provider/test-programs/regress-3674/Pulumi.yaml new file mode 100644 index 00000000000..4c19a9c92e4 --- /dev/null +++ b/provider/test-programs/regress-3674/Pulumi.yaml @@ -0,0 +1,16 @@ +name: regress-3674 +runtime: yaml +config: + pulumi:tags: + value: + pulumi:template: aws-yaml +outputs: + # Export the name of the bucket + bucketName: ${my-bucket.id} +resources: + # Create an AWS resource (S3 Bucket) + my-bucket: + type: aws:s3:BucketV2 + properties: + tags: + Name: my-bucket From 9d17357530fb1d247c56bb3d52d574852f8f8258 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 11:24:35 -0400 Subject: [PATCH 2/3] Fix tests --- provider/provider_test.go | 8 +++----- provider/provider_yaml_test.go | 5 ++++- provider/test-programs/regress-3674/Pulumi.yaml | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/provider/provider_test.go b/provider/provider_test.go index ff93c188b6d..1589aa4d10a 100644 --- a/provider/provider_test.go +++ b/provider/provider_test.go @@ -73,7 +73,7 @@ func testProviderUpgrade(t *testing.T, dir string, opts *testProviderUpgradeOpti assertpreview.HasNoReplacements(t, result) } -func pulumiTest(t *testing.T, dir string) *pulumitest.PulumiTest { +func pulumiTest(t *testing.T, dir string, opts ...opttest.Option) *pulumitest.PulumiTest { if testing.Short() { t.Skipf("Skipping in testing.Short() mode, assuming this is a CI run without AWS creds") return nil @@ -82,9 +82,7 @@ func pulumiTest(t *testing.T, dir string) *pulumitest.PulumiTest { if err != nil { t.Error(err) } - ptest := pulumitest.NewPulumiTest(t, dir, - opttest.LocalProviderPath("aws", filepath.Join(cwd, "..", "bin")), - ) - + opts = append(opts, opttest.LocalProviderPath("aws", filepath.Join(cwd, "..", "bin"))) + ptest := pulumitest.NewPulumiTest(t, dir, opts...) return ptest } diff --git a/provider/provider_yaml_test.go b/provider/provider_yaml_test.go index 952a587856b..dff9a4d5e49 100644 --- a/provider/provider_yaml_test.go +++ b/provider/provider_yaml_test.go @@ -303,13 +303,16 @@ func TestNonIdempotentSnsTopic(t *testing.T) { // Make sure that legacy Bucket supports deleting tags out of band and detecting drift. func TestRegress3674(t *testing.T) { - ptest := pulumiTest(t, filepath.Join("test-programs", "regress-3674")) + ptest := pulumiTest(t, filepath.Join("test-programs", "regress-3674"), opttest.SkipInstall()) upResult := ptest.Up() bucketName := upResult.Outputs["bucketName"].Value.(string) deleteBucketTagging(ptest.Context(), bucketName) result := ptest.Refresh() t.Logf("%s", result.StdOut) require.Equal(t, 1, (*result.Summary.ResourceChanges)["update"]) + state, err := ptest.ExportStack().Deployment.MarshalJSON() + require.NoError(t, err) + require.NotContainsf(t, string(state), "MyTestTag", "Expected MyTestTag to be removed") } func configureS3() *s3sdk.Client { diff --git a/provider/test-programs/regress-3674/Pulumi.yaml b/provider/test-programs/regress-3674/Pulumi.yaml index 4c19a9c92e4..4ae8a0b88e4 100644 --- a/provider/test-programs/regress-3674/Pulumi.yaml +++ b/provider/test-programs/regress-3674/Pulumi.yaml @@ -13,4 +13,4 @@ resources: type: aws:s3:BucketV2 properties: tags: - Name: my-bucket + MyTestTag: MyTestTag From be1cecac9f681dd3c435fd99f4bb4feb4a6d3cd9 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Thu, 9 May 2024 16:39:37 -0400 Subject: [PATCH 3/3] Regress the random-test-found combination --- examples/examples_go_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/examples/examples_go_test.go b/examples/examples_go_test.go index 8f4cddeed14..2960e9609a0 100644 --- a/examples/examples_go_test.go +++ b/examples/examples_go_test.go @@ -94,6 +94,11 @@ func TestTagsCombinationsGo(t *testing.T) { tagsState{DefaultTags: map[string]string{}, ResourceTags: map[string]string{"x": "s", "y": ""}}, tagsState{DefaultTags: map[string]string{"x": ""}, ResourceTags: map[string]string{}}, }, + { + "regress 3", + tagsState{DefaultTags: map[string]string{"x": "", "y": "s"}, ResourceTags: map[string]string{"x": "s", "y": "s"}}, + tagsState{DefaultTags: map[string]string{}, ResourceTags: map[string]string{"x": "", "y": "s"}}, + }, } for i, tc := range testCases {