From 531b4187a575652d44ba9e8ea2c10b372d0d97b5 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 11:48:13 -0700 Subject: [PATCH 01/11] Uses region filtering for S3 bucket sweeper --- internal/service/s3/sweep.go | 96 +++++++++++++++--------------------- 1 file changed, 39 insertions(+), 57 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index a4b191db20b..098859a3299 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -15,12 +15,15 @@ import ( "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/id" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-provider-aws/internal/conns" tfslices "github.com/hashicorp/terraform-provider-aws/internal/slices" "github.com/hashicorp/terraform-provider-aws/internal/sweep" "github.com/hashicorp/terraform-provider-aws/internal/sweep/awsv2" "github.com/hashicorp/terraform-provider-aws/internal/sweep/framework" + "github.com/hashicorp/terraform-provider-aws/internal/sweep/sdk" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/names" ) @@ -34,16 +37,12 @@ func RegisterSweepers() { }, }) - resource.AddTestSweepers("aws_s3_bucket", &resource.Sweeper{ - Name: "aws_s3_bucket", - F: sweepBuckets, - Dependencies: []string{ - "aws_s3_access_point", - "aws_s3_object", - "aws_s3control_access_grants_instance", - "aws_s3control_multi_region_access_point", - }, - }) + awsv2.Register("aws_s3_bucket", sweepBuckets, + "aws_s3_access_point", + "aws_s3_object", + "aws_s3control_access_grants_instance", + "aws_s3control_multi_region_access_point", + ) resource.AddTestSweepers("aws_s3_directory_bucket", &resource.Sweeper{ Name: "aws_s3_directory_bucket", @@ -75,7 +74,9 @@ func sweepObjects(region string) error { } buckets := tfslices.Filter(output.Buckets, bucketRegionFilter(ctx, conn, region, client.S3UsePathStyle(ctx))) - buckets = tfslices.Filter(buckets, bucketNameFilter) + buckets = tfslices.Filter(buckets, func(v types.Bucket) bool { + return bucketNameFilter(ctx, v) + }) sweepables := make([]sweep.Sweepable, 0) for _, bucket := range buckets { @@ -115,7 +116,7 @@ func sweepObjects(region string) error { } for _, v := range page.Buckets { - if !bucketNameFilter(v) { + if !bucketNameFilter(ctx, v) { continue } @@ -167,52 +168,34 @@ func (os directoryBucketObjectSweeper) Delete(ctx context.Context, timeout time. return nil } -func sweepBuckets(region string) error { - ctx := sweep.Context(region) - client, err := sweep.SharedRegionalSweepClient(ctx, region) - if err != nil { - return fmt.Errorf("getting client: %s", err) - } +func sweepBuckets(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { conn := client.S3Client(ctx) - input := &s3.ListBucketsInput{} - output, err := conn.ListBuckets(ctx, input) + var sweepResources []sweep.Sweepable + r := resourceBucket() - if awsv2.SkipSweepError(err) { - log.Printf("[WARN] Skipping S3 Buckets sweep for %s: %s", region, err) - return nil - } - - if err != nil { - return fmt.Errorf("listing S3 Buckets: %w", err) - } - - if len(output.Buckets) == 0 { - log.Print("[DEBUG] No S3 Buckets to sweep") - return nil - } - - buckets := tfslices.Filter(output.Buckets, bucketRegionFilter(ctx, conn, region, client.S3UsePathStyle(ctx))) - buckets = tfslices.Filter(buckets, bucketNameFilter) - sweepables := make([]sweep.Sweepable, 0) - - for _, bucket := range buckets { - name := aws.ToString(bucket.Name) - - r := resourceBucket() - d := r.Data(nil) - d.SetId(name) - - sweepables = append(sweepables, sweep.NewSweepResource(r, d, client)) + input := s3.ListBucketsInput{ + BucketRegion: aws.String(client.Region), } + pages := s3.NewListBucketsPaginator(conn, &input) + for pages.HasMorePages() { + page, err := pages.NextPage(ctx) + if err != nil { + return nil, err + } - err = sweep.SweepOrchestrator(ctx, sweepables) + for _, bucket := range page.Buckets { + tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) + if bucketNameFilter(ctx, bucket) { + d := r.Data(nil) + d.SetId(aws.ToString(bucket.Name)) - if err != nil { - return fmt.Errorf("error sweeping S3 Buckets (%s): %w", region, err) + sweepResources = append(sweepResources, sdk.NewSweepResource(r, d, client)) + } + } } - return nil + return sweepResources, nil } func bucketRegion(ctx context.Context, conn *s3.Client, bucket string, s3UsePathStyle bool) (string, error) { @@ -231,7 +214,7 @@ func bucketRegion(ctx context.Context, conn *s3.Client, bucket string, s3UsePath return region, nil } -func bucketNameFilter(bucket types.Bucket) bool { +func bucketNameFilter(ctx context.Context, bucket types.Bucket) bool { name := aws.ToString(bucket.Name) prefixes := []string{ @@ -249,18 +232,17 @@ func bucketNameFilter(bucket types.Bucket) bool { } } + defaultNameRegexp := regexache.MustCompile(fmt.Sprintf(`^%s\d+$`, id.UniqueIdPrefix)) if defaultNameRegexp.MatchString(name) { return true } - log.Printf("[INFO] Skipping S3 Bucket (%s): not in prefix list", name) + tflog.Info(ctx, "Skipping resource", map[string]any{ + "skip_reason": "no match on prefix list", + }) return false } -var ( - defaultNameRegexp = regexache.MustCompile(fmt.Sprintf(`^%s\d+$`, id.UniqueIdPrefix)) -) - func bucketRegionFilter(ctx context.Context, conn *s3.Client, region string, s3UsePathStyle bool) tfslices.Predicate[types.Bucket] { return func(bucket types.Bucket) bool { name := aws.ToString(bucket.Name) @@ -305,7 +287,7 @@ func sweepDirectoryBuckets(region string) error { } for _, v := range page.Buckets { - if !bucketNameFilter(v) { + if !bucketNameFilter(ctx, v) { continue } From cfc3ee71380e31113dffc3b0edcfad237006f18a Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:14:45 -0700 Subject: [PATCH 02/11] Uses region filtering for S3 general purpose bucket object sweeper --- internal/service/s3/sweep.go | 120 ++++++++++++++--------------------- 1 file changed, 48 insertions(+), 72 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 098859a3299..0bd87265f15 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -12,14 +12,12 @@ import ( "github.com/YakDriver/regexache" "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/id" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-provider-aws/internal/conns" - tfslices "github.com/hashicorp/terraform-provider-aws/internal/slices" "github.com/hashicorp/terraform-provider-aws/internal/sweep" "github.com/hashicorp/terraform-provider-aws/internal/sweep/awsv2" "github.com/hashicorp/terraform-provider-aws/internal/sweep/framework" @@ -59,45 +57,16 @@ func sweepObjects(region string) error { if err != nil { return fmt.Errorf("getting client: %s", err) } - conn := client.S3Client(ctx) - - // General purpose buckets. - output, err := conn.ListBuckets(ctx, &s3.ListBucketsInput{}) + sweepables, err := sweepGeneralPurposeBucketObjects(ctx, client) if awsv2.SkipSweepError(err) { - log.Printf("[WARN] Skipping S3 Objects sweep for %s: %s", region, err) + tflog.Warn(ctx, "Skipping sweeper", map[string]any{ + "error": err.Error(), + }) return nil } - if err != nil { - return fmt.Errorf("error listing S3 Buckets: %w", err) - } - - buckets := tfslices.Filter(output.Buckets, bucketRegionFilter(ctx, conn, region, client.S3UsePathStyle(ctx))) - buckets = tfslices.Filter(buckets, func(v types.Bucket) bool { - return bucketNameFilter(ctx, v) - }) - sweepables := make([]sweep.Sweepable, 0) - - for _, bucket := range buckets { - bucket := aws.ToString(bucket.Name) - objLockConfig, err := findObjectLockConfiguration(ctx, conn, bucket, "") - - var objectLockEnabled bool - - if !tfresource.NotFound(err) { - if err != nil { - log.Printf("[WARN] Reading S3 Bucket Object Lock Configuration (%s): %s", bucket, err) - continue - } - objectLockEnabled = objLockConfig.ObjectLockEnabled == types.ObjectLockEnabledEnabled - } - - sweepables = append(sweepables, objectSweeper{ - conn: conn, - bucket: bucket, - locked: objectLockEnabled, - }) + return fmt.Errorf("error listing General Purpose S3 Buckets (%s): %w", region, err) } // Directory buckets. @@ -136,6 +105,49 @@ func sweepObjects(region string) error { return nil } +func sweepGeneralPurposeBucketObjects(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { + conn := client.S3Client(ctx) + + var sweepables []sweep.Sweepable + + input := s3.ListBucketsInput{ + BucketRegion: aws.String(client.Region), + } + pages := s3.NewListBucketsPaginator(conn, &input) + for pages.HasMorePages() { + page, err := pages.NextPage(ctx) + if err != nil { + return nil, err + } + + for _, bucket := range page.Buckets { + bucketName := aws.ToString(bucket.Name) + tflog.SetField(ctx, "bucket_name", bucketName) + if bucketNameFilter(ctx, bucket) { + var objectLockEnabled bool + objLockConfig, err := findObjectLockConfiguration(ctx, conn, bucketName, "") + if !tfresource.NotFound(err) { + if err != nil { + tflog.Warn(ctx, "Reading S3 Bucket Object Lock Configuration", map[string]any{ + "error": err.Error(), + }) + continue + } + objectLockEnabled = objLockConfig.ObjectLockEnabled == types.ObjectLockEnabledEnabled + } + + sweepables = append(sweepables, objectSweeper{ + conn: conn, + bucket: bucketName, + locked: objectLockEnabled, + }) + } + } + } + + return sweepables, nil +} + type objectSweeper struct { conn *s3.Client bucket string @@ -198,22 +210,6 @@ func sweepBuckets(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepab return sweepResources, nil } -func bucketRegion(ctx context.Context, conn *s3.Client, bucket string, s3UsePathStyle bool) (string, error) { - region, err := manager.GetBucketRegion(ctx, conn, bucket, func(o *s3.Options) { - // By default, GetBucketRegion forces virtual host addressing, which - // is not compatible with many non-AWS implementations. Instead, pass - // the provider s3_force_path_style configuration, which defaults to - // false, but allows override. - o.UsePathStyle = s3UsePathStyle - }) - - if err != nil { - return "", err - } - - return region, nil -} - func bucketNameFilter(ctx context.Context, bucket types.Bucket) bool { name := aws.ToString(bucket.Name) @@ -243,26 +239,6 @@ func bucketNameFilter(ctx context.Context, bucket types.Bucket) bool { return false } -func bucketRegionFilter(ctx context.Context, conn *s3.Client, region string, s3UsePathStyle bool) tfslices.Predicate[types.Bucket] { - return func(bucket types.Bucket) bool { - name := aws.ToString(bucket.Name) - - bucketRegion, err := bucketRegion(ctx, conn, name, s3UsePathStyle) - - if err != nil { - log.Printf("[WARN] Getting S3 Bucket (%s) region: %s", name, err) - return false - } - - if bucketRegion != region { - log.Printf("[INFO] Skipping S3 Bucket (%s): not in %s", name, region) - return false - } - - return true - } -} - func sweepDirectoryBuckets(region string) error { ctx := sweep.Context(region) client, err := sweep.SharedRegionalSweepClient(ctx, region) From 827bc78e5925236627e4e04d611076dabea313cb Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:32:31 -0700 Subject: [PATCH 03/11] Create separate function for directory bucket object sweeping --- internal/service/s3/sweep.go | 61 ++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 0bd87265f15..74f6b3c9504 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -70,31 +70,17 @@ func sweepObjects(region string) error { } // Directory buckets. - s3ExpressConn := client.S3ExpressClient(ctx) - pages := s3.NewListDirectoryBucketsPaginator(s3ExpressConn, &s3.ListDirectoryBucketsInput{}) - for pages.HasMorePages() { - page, err := pages.NextPage(ctx) - - if awsv2.SkipSweepError(err) { - log.Printf("[WARN] Skipping S3 Objects sweep for %s: %s", region, err) - break // Allow objects in general purpose buckets to be deleted. - } - - if err != nil { - return fmt.Errorf("error listing S3 Directory Buckets (%s): %w", region, err) - } - - for _, v := range page.Buckets { - if !bucketNameFilter(ctx, v) { - continue - } - - sweepables = append(sweepables, directoryBucketObjectSweeper{ - conn: s3ExpressConn, - bucket: aws.ToString(v.Name), - }) - } + dbSweepables, err := sweepDirectoryBucketObjects(ctx, client) + if awsv2.SkipSweepError(err) { + tflog.Warn(ctx, "Skipping sweeper", map[string]any{ + "error": err.Error(), + }) + // Allow objects in general purpose buckets to be deleted. } + if err != nil { + return fmt.Errorf("error listing S3 Directory Buckets (%s): %w", region, err) + } + sweepables = append(sweepables, dbSweepables...) err = sweep.SweepOrchestrator(ctx, sweepables) @@ -148,6 +134,33 @@ func sweepGeneralPurposeBucketObjects(ctx context.Context, client *conns.AWSClie return sweepables, nil } +func sweepDirectoryBucketObjects(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { + conn := client.S3ExpressClient(ctx) + + var sweepables []sweep.Sweepable + + pages := s3.NewListDirectoryBucketsPaginator(conn, &s3.ListDirectoryBucketsInput{}) + for pages.HasMorePages() { + page, err := pages.NextPage(ctx) + if err != nil { + return nil, err + } + + for _, bucket := range page.Buckets { + bucketName := aws.ToString(bucket.Name) + tflog.SetField(ctx, "bucket_name", bucketName) + if bucketNameFilter(ctx, bucket) { + sweepables = append(sweepables, directoryBucketObjectSweeper{ + conn: conn, + bucket: bucketName, + }) + } + } + } + + return sweepables, nil +} + type objectSweeper struct { conn *s3.Client bucket string From 3b887de2d0a676c25fbdda5caef0c44369c9db71 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:39:29 -0700 Subject: [PATCH 04/11] Separates object sweepers by general purpose and directory buckets --- internal/service/s3/sweep.go | 53 ++++++------------------------------ 1 file changed, 9 insertions(+), 44 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 74f6b3c9504..68704d07e01 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -27,14 +27,6 @@ import ( ) func RegisterSweepers() { - resource.AddTestSweepers("aws_s3_object", &resource.Sweeper{ - Name: "aws_s3_object", - F: sweepObjects, - Dependencies: []string{ - "aws_m2_application", - }, - }) - awsv2.Register("aws_s3_bucket", sweepBuckets, "aws_s3_access_point", "aws_s3_object", @@ -49,46 +41,19 @@ func RegisterSweepers() { "aws_s3_object", }, }) -} -func sweepObjects(region string) error { - ctx := sweep.Context(region) - client, err := sweep.SharedRegionalSweepClient(ctx, region) - if err != nil { - return fmt.Errorf("getting client: %s", err) - } + awsv2.Register("aws_s3_object", sweepObjects) - sweepables, err := sweepGeneralPurposeBucketObjects(ctx, client) - if awsv2.SkipSweepError(err) { - tflog.Warn(ctx, "Skipping sweeper", map[string]any{ - "error": err.Error(), - }) - return nil - } - if err != nil { - return fmt.Errorf("error listing General Purpose S3 Buckets (%s): %w", region, err) - } - - // Directory buckets. - dbSweepables, err := sweepDirectoryBucketObjects(ctx, client) - if awsv2.SkipSweepError(err) { - tflog.Warn(ctx, "Skipping sweeper", map[string]any{ - "error": err.Error(), - }) - // Allow objects in general purpose buckets to be deleted. - } - if err != nil { - return fmt.Errorf("error listing S3 Directory Buckets (%s): %w", region, err) - } - sweepables = append(sweepables, dbSweepables...) - - err = sweep.SweepOrchestrator(ctx, sweepables) + awsv2.Register("aws_s3_object_directory_bucket", sweepDirectoryBucketObjects) - if err != nil { - return fmt.Errorf("error sweeping S3 Objects (%s): %w", region, err) - } + awsv2.Register("aws_s3_object_gp_bucket", sweepGeneralPurposeBucketObjects, + "aws_m2_application", + ) +} - return nil +func sweepObjects(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { + tflog.Info(ctx, "Noop sweeper") + return nil, nil } func sweepGeneralPurposeBucketObjects(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { From be60655f979f1cc845c4be5421fd03c810b2d788 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:44:05 -0700 Subject: [PATCH 05/11] Uses `tflog` for object sweepers --- internal/service/s3/sweep.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 68704d07e01..ef3c68a1f77 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -134,12 +134,14 @@ type objectSweeper struct { func (os objectSweeper) Delete(ctx context.Context, timeout time.Duration, optFns ...tfresource.OptionsFunc) error { // Delete everything including locked objects. - log.Printf("[INFO] Emptying S3 Bucket (%s)", os.bucket) + tflog.Info(ctx, "Emptying S3 General Purpose Bucket") n, err := emptyBucket(ctx, os.conn, os.bucket, os.locked) if err != nil { return fmt.Errorf("deleting S3 Bucket (%s) objects: %w", os.bucket, err) } - log.Printf("[INFO] Deleted %d S3 Objects from S3 Bucket (%s)", n, os.bucket) + tflog.Info(ctx, "Deleted Objects from S3 General Purpose Bucket", map[string]any{ + "object_count": n, + }) return nil } @@ -149,12 +151,14 @@ type directoryBucketObjectSweeper struct { } func (os directoryBucketObjectSweeper) Delete(ctx context.Context, timeout time.Duration, optFns ...tfresource.OptionsFunc) error { - log.Printf("[INFO] Emptying S3 Directory Bucket (%s)", os.bucket) + tflog.Info(ctx, "Emptying S3 Directory Bucket") n, err := emptyDirectoryBucket(ctx, os.conn, os.bucket) if err != nil { return fmt.Errorf("deleting S3 Directory Bucket (%s) objects: %w", os.bucket, err) } - log.Printf("[INFO] Deleted %d S3 Objects from S3 Directory Bucket (%s)", n, os.bucket) + tflog.Info(ctx, "Deleted Objects from S3 Directory Bucket", map[string]any{ + "object_count": n, + }) return nil } From d151fffec4e8b2ca1e04fd2eb1ef104efb4d56f1 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:45:14 -0700 Subject: [PATCH 06/11] Sweeps specific object types --- internal/service/s3/sweep.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index ef3c68a1f77..8db9990f557 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -29,7 +29,7 @@ import ( func RegisterSweepers() { awsv2.Register("aws_s3_bucket", sweepBuckets, "aws_s3_access_point", - "aws_s3_object", + "aws_s3_object_gp_bucket", "aws_s3control_access_grants_instance", "aws_s3control_multi_region_access_point", ) @@ -38,7 +38,7 @@ func RegisterSweepers() { Name: "aws_s3_directory_bucket", F: sweepDirectoryBuckets, Dependencies: []string{ - "aws_s3_object", + "aws_s3_object_directory_bucket", }, }) From 1ff288abc89fbfa1ab8370079da8ead2cfa7d037 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:49:31 -0700 Subject: [PATCH 07/11] Uses `awsv2.Register` to register `aws_s3_directory_bucket` sweeper --- internal/service/s3/sweep.go | 46 +++++++++--------------------------- 1 file changed, 11 insertions(+), 35 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 8db9990f557..23fa18ee633 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -6,7 +6,6 @@ package s3 import ( "context" "fmt" - "log" "strings" "time" @@ -16,7 +15,6 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/id" - "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/sweep" "github.com/hashicorp/terraform-provider-aws/internal/sweep/awsv2" @@ -34,13 +32,7 @@ func RegisterSweepers() { "aws_s3control_multi_region_access_point", ) - resource.AddTestSweepers("aws_s3_directory_bucket", &resource.Sweeper{ - Name: "aws_s3_directory_bucket", - F: sweepDirectoryBuckets, - Dependencies: []string{ - "aws_s3_object_directory_bucket", - }, - }) + awsv2.Register("aws_s3_directory_bucket", sweepDirectoryBuckets) awsv2.Register("aws_s3_object", sweepObjects) @@ -221,45 +213,29 @@ func bucketNameFilter(ctx context.Context, bucket types.Bucket) bool { return false } -func sweepDirectoryBuckets(region string) error { - ctx := sweep.Context(region) - client, err := sweep.SharedRegionalSweepClient(ctx, region) - if err != nil { - return fmt.Errorf("getting client: %s", err) - } +func sweepDirectoryBuckets(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { conn := client.S3ExpressClient(ctx) - input := &s3.ListDirectoryBucketsInput{} - sweepResources := make([]sweep.Sweepable, 0) - pages := s3.NewListDirectoryBucketsPaginator(conn, input) + var sweepResources []sweep.Sweepable + + pages := s3.NewListDirectoryBucketsPaginator(conn, &s3.ListDirectoryBucketsInput{}) for pages.HasMorePages() { page, err := pages.NextPage(ctx) - - if awsv2.SkipSweepError(err) { - log.Printf("[WARN] Skipping S3 Directory Bucket sweep for %s: %s", region, err) - return nil - } - if err != nil { - return fmt.Errorf("error listing S3 Directory Buckets (%s): %w", region, err) + return nil, err } - for _, v := range page.Buckets { - if !bucketNameFilter(ctx, v) { + for _, bucket := range page.Buckets { + tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) + if !bucketNameFilter(ctx, bucket) { continue } sweepResources = append(sweepResources, framework.NewSweepResource(newDirectoryBucketResource, client, - framework.NewAttribute(names.AttrID, aws.ToString(v.Name)), + framework.NewAttribute(names.AttrID, aws.ToString(bucket.Name)), )) } } - err = sweep.SweepOrchestrator(ctx, sweepResources) - - if err != nil { - return fmt.Errorf("error sweeping S3 Directory Buckets (%s): %w", region, err) - } - - return nil + return sweepResources, nil } From 1861d5a4cf191fc47de7bfe5b0706c0039c30c46 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 12:52:21 -0700 Subject: [PATCH 08/11] Makes skipping when `bucketNameFilter` does not pass --- internal/service/s3/sweep.go | 60 ++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 23fa18ee633..901e4b82d90 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -66,25 +66,27 @@ func sweepGeneralPurposeBucketObjects(ctx context.Context, client *conns.AWSClie for _, bucket := range page.Buckets { bucketName := aws.ToString(bucket.Name) tflog.SetField(ctx, "bucket_name", bucketName) - if bucketNameFilter(ctx, bucket) { - var objectLockEnabled bool - objLockConfig, err := findObjectLockConfiguration(ctx, conn, bucketName, "") - if !tfresource.NotFound(err) { - if err != nil { - tflog.Warn(ctx, "Reading S3 Bucket Object Lock Configuration", map[string]any{ - "error": err.Error(), - }) - continue - } - objectLockEnabled = objLockConfig.ObjectLockEnabled == types.ObjectLockEnabledEnabled - } + if !bucketNameFilter(ctx, bucket) { + continue + } - sweepables = append(sweepables, objectSweeper{ - conn: conn, - bucket: bucketName, - locked: objectLockEnabled, - }) + var objectLockEnabled bool + objLockConfig, err := findObjectLockConfiguration(ctx, conn, bucketName, "") + if !tfresource.NotFound(err) { + if err != nil { + tflog.Warn(ctx, "Reading S3 Bucket Object Lock Configuration", map[string]any{ + "error": err.Error(), + }) + continue + } + objectLockEnabled = objLockConfig.ObjectLockEnabled == types.ObjectLockEnabledEnabled } + + sweepables = append(sweepables, objectSweeper{ + conn: conn, + bucket: bucketName, + locked: objectLockEnabled, + }) } } @@ -106,12 +108,14 @@ func sweepDirectoryBucketObjects(ctx context.Context, client *conns.AWSClient) ( for _, bucket := range page.Buckets { bucketName := aws.ToString(bucket.Name) tflog.SetField(ctx, "bucket_name", bucketName) - if bucketNameFilter(ctx, bucket) { - sweepables = append(sweepables, directoryBucketObjectSweeper{ - conn: conn, - bucket: bucketName, - }) + if !bucketNameFilter(ctx, bucket) { + continue } + + sweepables = append(sweepables, directoryBucketObjectSweeper{ + conn: conn, + bucket: bucketName, + }) } } @@ -172,12 +176,14 @@ func sweepBuckets(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepab for _, bucket := range page.Buckets { tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) - if bucketNameFilter(ctx, bucket) { - d := r.Data(nil) - d.SetId(aws.ToString(bucket.Name)) - - sweepResources = append(sweepResources, sdk.NewSweepResource(r, d, client)) + if !bucketNameFilter(ctx, bucket) { + continue } + + d := r.Data(nil) + d.SetId(aws.ToString(bucket.Name)) + + sweepResources = append(sweepResources, sdk.NewSweepResource(r, d, client)) } } From d404d883b2cc839bb9696b93d0fb65f247f676fd Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 13:05:34 -0700 Subject: [PATCH 09/11] Fixes `SetField` --- internal/service/s3/sweep.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 901e4b82d90..96dd6ca91dc 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -65,7 +65,7 @@ func sweepGeneralPurposeBucketObjects(ctx context.Context, client *conns.AWSClie for _, bucket := range page.Buckets { bucketName := aws.ToString(bucket.Name) - tflog.SetField(ctx, "bucket_name", bucketName) + ctx = tflog.SetField(ctx, "bucket_name", bucketName) if !bucketNameFilter(ctx, bucket) { continue } @@ -107,7 +107,7 @@ func sweepDirectoryBucketObjects(ctx context.Context, client *conns.AWSClient) ( for _, bucket := range page.Buckets { bucketName := aws.ToString(bucket.Name) - tflog.SetField(ctx, "bucket_name", bucketName) + ctx = tflog.SetField(ctx, "bucket_name", bucketName) if !bucketNameFilter(ctx, bucket) { continue } @@ -175,7 +175,7 @@ func sweepBuckets(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepab } for _, bucket := range page.Buckets { - tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) + ctx = tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) if !bucketNameFilter(ctx, bucket) { continue } @@ -232,7 +232,7 @@ func sweepDirectoryBuckets(ctx context.Context, client *conns.AWSClient) ([]swee } for _, bucket := range page.Buckets { - tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) + ctx = tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) if !bucketNameFilter(ctx, bucket) { continue } From 7f4f36932a9db158c7d421b0c5378251a432cce0 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 28 Oct 2024 13:06:58 -0700 Subject: [PATCH 10/11] Adds Semgrep check for using returned context from `tflog.SetField` --- .ci/semgrep/tflog/tflog.yml | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .ci/semgrep/tflog/tflog.yml diff --git a/.ci/semgrep/tflog/tflog.yml b/.ci/semgrep/tflog/tflog.yml new file mode 100644 index 00000000000..431c83c360a --- /dev/null +++ b/.ci/semgrep/tflog/tflog.yml @@ -0,0 +1,9 @@ +rules: + - id: setfield-without-assign + languages: [go] + message: The return value of "tflog.SetField" must be used + patterns: + - pattern: tflog.SetField(...) + - pattern-not-inside: $CTX = tflog.SetField($CTX, ...) + - pattern-not-inside: return tflog.SetField($CTX, ...) + severity: ERROR From 0f2039674e784d4a9eef9aaa8bd8a63632a0c9a4 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 29 Oct 2024 15:55:57 -0700 Subject: [PATCH 11/11] Const --- internal/service/s3/sweep.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/service/s3/sweep.go b/internal/service/s3/sweep.go index 96dd6ca91dc..3c0d4579276 100644 --- a/internal/service/s3/sweep.go +++ b/internal/service/s3/sweep.go @@ -43,6 +43,8 @@ func RegisterSweepers() { ) } +const logKeyBucketName = "bucket_name" + func sweepObjects(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepable, error) { tflog.Info(ctx, "Noop sweeper") return nil, nil @@ -65,7 +67,7 @@ func sweepGeneralPurposeBucketObjects(ctx context.Context, client *conns.AWSClie for _, bucket := range page.Buckets { bucketName := aws.ToString(bucket.Name) - ctx = tflog.SetField(ctx, "bucket_name", bucketName) + ctx = tflog.SetField(ctx, logKeyBucketName, bucketName) if !bucketNameFilter(ctx, bucket) { continue } @@ -107,7 +109,7 @@ func sweepDirectoryBucketObjects(ctx context.Context, client *conns.AWSClient) ( for _, bucket := range page.Buckets { bucketName := aws.ToString(bucket.Name) - ctx = tflog.SetField(ctx, "bucket_name", bucketName) + ctx = tflog.SetField(ctx, logKeyBucketName, bucketName) if !bucketNameFilter(ctx, bucket) { continue } @@ -175,7 +177,7 @@ func sweepBuckets(ctx context.Context, client *conns.AWSClient) ([]sweep.Sweepab } for _, bucket := range page.Buckets { - ctx = tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) + ctx = tflog.SetField(ctx, logKeyBucketName, aws.ToString(bucket.Name)) if !bucketNameFilter(ctx, bucket) { continue } @@ -232,7 +234,7 @@ func sweepDirectoryBuckets(ctx context.Context, client *conns.AWSClient) ([]swee } for _, bucket := range page.Buckets { - ctx = tflog.SetField(ctx, "bucket_name", aws.ToString(bucket.Name)) + ctx = tflog.SetField(ctx, logKeyBucketName, aws.ToString(bucket.Name)) if !bucketNameFilter(ctx, bucket) { continue }