Skip to content

Commit f40b991

Browse files
Mia-CrossMia-Cross
authored
feat(object): acl: revert to private on resource deletion (#2513)
1 parent 604759f commit f40b991

19 files changed

+17864
-8404
lines changed

‎go.mod

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ module github.com/scaleway/terraform-provider-scaleway/v2
22

33
require (
44
github.com/aws/aws-sdk-go v1.51.19
5+
github.com/aws/aws-sdk-go-v2/service/s3 v1.53.1
56
github.com/docker/docker v26.0.0+incompatible
67
github.com/dustin/go-humanize v1.0.1
78
github.com/google/go-cmp v0.6.0
@@ -28,6 +29,7 @@ require (
2829
github.com/ProtonMail/go-crypto v1.1.0-alpha.0 // indirect
2930
github.com/agext/levenshtein v1.2.3 // indirect
3031
github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect
32+
github.com/aws/smithy-go v1.20.2 // indirect
3133
github.com/cloudflare/circl v1.3.7 // indirect
3234
github.com/containerd/log v0.1.0 // indirect
3335
github.com/davecgh/go-spew v1.1.1 // indirect

‎go.sum

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ github.com/apparentlymart/go-textseg/v15 v15.0.0/go.mod h1:K8XmNZdhEBkdlyDdvbmms
1414
github.com/aws/aws-sdk-go v1.31.9/go.mod h1:5zCpMtNQVjRREroY7sYe8lOMRSxkhG6MZveU8YkpAk0=
1515
github.com/aws/aws-sdk-go v1.51.19 h1:jp/Vx/mUpXttthvvo/4/Nn/3+zumirIlAFkp1Irf1kM=
1616
github.com/aws/aws-sdk-go v1.51.19/go.mod h1:LF8svs817+Nz+DmiMQKTO3ubZ/6IaTpq3TjupRn3Eqk=
17+
github.com/aws/aws-sdk-go-v2/service/s3 v1.53.1 h1:6cnno47Me9bRykw9AEv9zkXE+5or7jz8TsskTTccbgc=
18+
github.com/aws/aws-sdk-go-v2/service/s3 v1.53.1/go.mod h1:qmdkIIAC+GCLASF7R2whgNrJADz0QZPX+Seiw/i4S3o=
19+
github.com/aws/smithy-go v1.20.2 h1:tbp628ireGtzcHDDmLT/6ADHidqnwgF57XOXZe6tp4Q=
20+
github.com/aws/smithy-go v1.20.2/go.mod h1:krry+ya/rV9RDcV/Q16kpu6ypI4K2czasz0NC3qS14E=
1721
github.com/bufbuild/protocompile v0.4.0 h1:LbFKd2XowZvQ/kajzguUp2DC9UEIQhIq77fZZlaQsNA=
1822
github.com/bufbuild/protocompile v0.4.0/go.mod h1:3v93+mbWn/v3xzN+31nwkJfrEpAUwp+BagBSZWx+TP8=
1923
github.com/cenkalti/backoff/v4 v4.2.1 h1:y4OZtCnogmCPw98Zjyt5a6+QwPLGkiQsYW5oUqylYbM=

‎internal/services/object/bucket.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func ResourceBucket() *schema.Resource {
4848
Type: schema.TypeString,
4949
Optional: true,
5050
Default: "private",
51-
Description: "ACL of the bucket: either 'public-read' or 'private'.",
51+
Description: "ACL of the bucket: either 'private', 'public-read', 'public-read-write' or 'authenticated-read'.",
5252
ValidateFunc: validation.StringInSlice([]string{
5353
s3.ObjectCannedACLPrivate,
5454
s3.ObjectCannedACLPublicRead,
@@ -470,7 +470,7 @@ func resourceObjectBucketRead(ctx context.Context, d *schema.ResourceData, m int
470470
return diags
471471
}
472472
} else if acl != nil && acl.Owner != nil {
473-
_ = d.Set("project_id", normalizeOwnerID(acl.Owner.ID))
473+
_ = d.Set("project_id", NormalizeOwnerID(acl.Owner.ID))
474474
}
475475

476476
// Get object_lock_enabled

‎internal/services/object/bucket_acl.go

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func ResourceBucketACL() *schema.Resource {
112112
"acl": {
113113
Type: schema.TypeString,
114114
Optional: true,
115-
Description: "ACL of the bucket: either 'public-read' or 'private'.",
115+
Description: "ACL of the bucket: either 'private', 'public-read', 'public-read-write' or 'authenticated-read'.",
116116
ValidateFunc: validation.StringInSlice([]string{
117117
s3.ObjectCannedACLPrivate,
118118
s3.ObjectCannedACLPublicRead,
@@ -333,11 +333,11 @@ func flattenBucketACLAccessControlPolicyGrantsGrantee(grantee *s3.Grantee) []int
333333
m := make(map[string]interface{})
334334

335335
if grantee.DisplayName != nil {
336-
m["display_name"] = aws.StringValue(normalizeOwnerID(grantee.DisplayName))
336+
m["display_name"] = aws.StringValue(NormalizeOwnerID(grantee.DisplayName))
337337
}
338338

339339
if grantee.ID != nil {
340-
m["id"] = aws.StringValue(normalizeOwnerID(grantee.ID))
340+
m["id"] = aws.StringValue(NormalizeOwnerID(grantee.ID))
341341
}
342342

343343
if grantee.Type != nil {
@@ -355,11 +355,11 @@ func flattenBucketACLAccessControlPolicyOwner(owner *s3.Owner) []interface{} {
355355
m := make(map[string]interface{})
356356

357357
if owner.DisplayName != nil {
358-
m["display_name"] = aws.StringValue(normalizeOwnerID(owner.DisplayName))
358+
m["display_name"] = aws.StringValue(NormalizeOwnerID(owner.DisplayName))
359359
}
360360

361361
if owner.ID != nil {
362-
m["id"] = aws.StringValue(normalizeOwnerID(owner.ID))
362+
m["id"] = aws.StringValue(NormalizeOwnerID(owner.ID))
363363
}
364364

365365
return []interface{}{m}
@@ -402,7 +402,7 @@ func resourceBucketACLRead(ctx context.Context, d *schema.ResourceData, m interf
402402
return diag.FromErr(fmt.Errorf("error setting access_control_policy: %w", err))
403403
}
404404
_ = d.Set("region", region)
405-
_ = d.Set("project_id", normalizeOwnerID(output.Owner.ID))
405+
_ = d.Set("project_id", NormalizeOwnerID(output.Owner.ID))
406406
_ = d.Set("bucket", locality.ExpandID(bucket))
407407

408408
return nil
@@ -453,7 +453,25 @@ func resourceBucketACLUpdate(ctx context.Context, d *schema.ResourceData, m inte
453453
return resourceBucketACLRead(ctx, d, m)
454454
}
455455

456-
func resourceBucketACLDelete(ctx context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics {
457-
tflog.Warn(ctx, "[WARN] Cannot destroy Object Bucket ACL. Terraform will remove this resource from the state file, however resources may remain.")
458-
return nil
456+
func resourceBucketACLDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
457+
conn, _, bucket, _, err := s3ClientWithRegionWithNameACL(d, m, d.Id())
458+
if err != nil {
459+
return diag.FromErr(err)
460+
}
461+
462+
_, err = conn.PutBucketAclWithContext(ctx, &s3.PutBucketAclInput{
463+
Bucket: &bucket,
464+
ACL: scw.StringPtr(s3.ObjectCannedACLPrivate),
465+
})
466+
if err != nil {
467+
return diag.FromErr(fmt.Errorf("error putting bucket ACL: %w", err))
468+
}
469+
470+
return diag.Diagnostics{
471+
diag.Diagnostic{
472+
Severity: diag.Warning,
473+
Summary: "Deleting Object Bucket ACL resource resets ACL to private",
474+
Detail: "Deleting Object Bucket ACL resource resets the bucket's ACL to its default value: private.\nIf you wish to set it to something else, you should recreate a Bucket ACL resource with the `acl` field filled accordingly.",
475+
},
476+
}
459477
}

‎internal/services/object/bucket_acl_test.go

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,25 @@
11
package object_test
22

33
import (
4+
"errors"
45
"fmt"
56
"regexp"
7+
"strings"
68
"testing"
79

10+
"github.com/aws/aws-sdk-go/service/s3"
811
sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
912
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
13+
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
1014
"github.com/scaleway/terraform-provider-scaleway/v2/internal/acctest"
15+
"github.com/scaleway/terraform-provider-scaleway/v2/internal/services/object"
1116
objectchecks "github.com/scaleway/terraform-provider-scaleway/v2/internal/services/object/testfuncs"
17+
"github.com/scaleway/terraform-provider-scaleway/v2/internal/types"
18+
)
19+
20+
const (
21+
s3ACLGranteeAllUsers = "AllUsers"
22+
s3ACLGranteeAuthenticatedUsers = "AuthenticatedUsers"
1223
)
1324

1425
func TestAccObjectBucketACL_Basic(t *testing.T) {
@@ -263,3 +274,178 @@ func TestAccObjectBucketACL_WithBucketName(t *testing.T) {
263274
},
264275
})
265276
}
277+
278+
func TestAccObjectBucketACL_Remove(t *testing.T) {
279+
tt := acctest.NewTestTools(t)
280+
defer tt.Cleanup()
281+
testBucketName := sdkacctest.RandomWithPrefix("test-acc-scw-object-acl-remove")
282+
283+
resource.Test(t, resource.TestCase{
284+
PreCheck: func() { acctest.PreCheck(t) },
285+
ProviderFactories: tt.ProviderFactories,
286+
CheckDestroy: objectchecks.IsBucketDestroyed(tt),
287+
Steps: []resource.TestStep{
288+
{
289+
Config: fmt.Sprintf(`
290+
resource "scaleway_object_bucket" "main" {
291+
name = "%s"
292+
region = "%s"
293+
acl = "authenticated-read"
294+
}
295+
`, testBucketName, objectTestsMainRegion),
296+
Check: resource.ComposeTestCheckFunc(
297+
objectchecks.CheckBucketExists(tt, "scaleway_object_bucket.main", true),
298+
resource.TestCheckResourceAttr("scaleway_object_bucket.main", "acl", "authenticated-read"),
299+
testAccObjectBucketACLCheck(tt, "scaleway_object_bucket.main", "authenticated-read"),
300+
),
301+
},
302+
{
303+
Config: fmt.Sprintf(`
304+
resource "scaleway_object_bucket" "main" {
305+
name = "%s"
306+
region = "%s"
307+
acl = "authenticated-read"
308+
}
309+
310+
resource "scaleway_object_bucket_acl" "main" {
311+
bucket = scaleway_object_bucket.main.id
312+
acl = "public-read"
313+
}
314+
`, testBucketName, objectTestsMainRegion),
315+
Check: resource.ComposeTestCheckFunc(
316+
objectchecks.CheckBucketExists(tt, "scaleway_object_bucket.main", true),
317+
resource.TestCheckResourceAttr("scaleway_object_bucket_acl.main", "bucket", testBucketName),
318+
resource.TestCheckResourceAttr("scaleway_object_bucket.main", "acl", "authenticated-read"),
319+
resource.TestCheckResourceAttr("scaleway_object_bucket_acl.main", "acl", "public-read"),
320+
testAccObjectBucketACLCheck(tt, "scaleway_object_bucket.main", "public-read"),
321+
),
322+
},
323+
{
324+
Config: fmt.Sprintf(`
325+
resource "scaleway_object_bucket" "main" {
326+
name = "%s"
327+
region = "%s"
328+
acl = "authenticated-read"
329+
}
330+
`, testBucketName, objectTestsMainRegion),
331+
Check: resource.ComposeTestCheckFunc(
332+
objectchecks.CheckBucketExists(tt, "scaleway_object_bucket.main", true),
333+
resource.TestCheckResourceAttr("scaleway_object_bucket.main", "acl", "authenticated-read"),
334+
testAccObjectBucketACLCheck(tt, "scaleway_object_bucket.main", "private"),
335+
),
336+
},
337+
{
338+
Config: fmt.Sprintf(`
339+
resource "scaleway_object_bucket" "main" {
340+
name = "%s"
341+
region = "%s"
342+
}
343+
`, testBucketName, objectTestsMainRegion),
344+
Check: resource.ComposeTestCheckFunc(
345+
objectchecks.CheckBucketExists(tt, "scaleway_object_bucket.main", true),
346+
resource.TestCheckResourceAttr("scaleway_object_bucket.main", "acl", "private"),
347+
testAccObjectBucketACLCheck(tt, "scaleway_object_bucket.main", "private"),
348+
),
349+
},
350+
},
351+
})
352+
}
353+
354+
func testAccObjectBucketACLCheck(tt *acctest.TestTools, name string, expectedACL string) resource.TestCheckFunc {
355+
return func(s *terraform.State) error {
356+
rs, ok := s.RootModule().Resources[name]
357+
if !ok {
358+
return fmt.Errorf("resource not found: %s", name)
359+
}
360+
361+
bucketRegion := rs.Primary.Attributes["region"]
362+
s3Client, err := object.NewS3ClientFromMeta(tt.Meta, bucketRegion)
363+
if err != nil {
364+
return err
365+
}
366+
if rs.Primary.ID == "" {
367+
return errors.New("no ID is set")
368+
}
369+
370+
bucketName := rs.Primary.Attributes["name"]
371+
actualACL, err := s3Client.GetBucketAcl(&s3.GetBucketAclInput{
372+
Bucket: types.ExpandStringPtr(bucketName),
373+
})
374+
if err != nil {
375+
return fmt.Errorf("could not get ACL for bucket %s: %v", bucketName, err)
376+
}
377+
378+
errs := s3ACLAreEqual(expectedACL, actualACL)
379+
if len(errs) > 0 {
380+
return fmt.Errorf("unexpected result: %w", errors.Join(errs...))
381+
}
382+
return nil
383+
}
384+
}
385+
386+
func s3ACLAreEqual(expected string, actual *s3.GetBucketAclOutput) (errs []error) {
387+
ownerID := *object.NormalizeOwnerID(actual.Owner.ID)
388+
grantsMap := make(map[string]string)
389+
for _, actualACL := range actual.Grants {
390+
if actualACL.Permission == nil {
391+
return append(errs, errors.New("grant has no permission"))
392+
}
393+
if actualACL.Grantee.ID != nil {
394+
grantsMap[*actualACL.Permission] = *object.NormalizeOwnerID(actualACL.Grantee.ID)
395+
} else {
396+
groupURI := strings.Split(*actualACL.Grantee.URI, "/")
397+
grantsMap[*actualACL.Permission] = groupURI[len(groupURI)-1]
398+
}
399+
}
400+
401+
switch expected {
402+
case "private":
403+
if len(grantsMap) != 1 {
404+
errs = append(errs, fmt.Errorf("expected 1 grant, but got %d", len(grantsMap)))
405+
return errs
406+
}
407+
if grantsMap["FULL_CONTROL"] != ownerID {
408+
errs = append(errs, fmt.Errorf("expected FULL_CONTROL to be granted to owner (%s), instead got %q", ownerID, grantsMap["FULL_CONTROL"]))
409+
}
410+
411+
case "public-read":
412+
if len(grantsMap) != 2 {
413+
errs = append(errs, fmt.Errorf("expected 2 grants, but got %d", len(grantsMap)))
414+
return errs
415+
}
416+
if grantsMap["FULL_CONTROL"] != ownerID {
417+
errs = append(errs, fmt.Errorf("expected FULL_CONTROL to be granted to owner (%s), instead got %q", ownerID, grantsMap["FULL_CONTROL"]))
418+
}
419+
if grantsMap["READ"] != s3ACLGranteeAllUsers {
420+
errs = append(errs, fmt.Errorf("expected READ to be granted to %q, instead got %q", s3ACLGranteeAllUsers, grantsMap["READ"]))
421+
}
422+
423+
case "public-read-write":
424+
if len(grantsMap) != 3 {
425+
errs = append(errs, fmt.Errorf("expected 3 grants, but got %d", len(grantsMap)))
426+
return errs
427+
}
428+
if grantsMap["FULL_CONTROL"] != ownerID {
429+
errs = append(errs, fmt.Errorf("expected FULL_CONTROL to be granted to owner (%s), instead got %q", ownerID, grantsMap["FULL_CONTROL"]))
430+
}
431+
if grantsMap["READ"] != s3ACLGranteeAllUsers {
432+
errs = append(errs, fmt.Errorf("expected READ to be granted to %q, instead got %q", s3ACLGranteeAllUsers, grantsMap["READ"]))
433+
}
434+
if grantsMap["WRITE"] != s3ACLGranteeAllUsers {
435+
errs = append(errs, fmt.Errorf("expected WRITE to be granted to %q, instead got %q", s3ACLGranteeAllUsers, grantsMap["WRITE"]))
436+
}
437+
438+
case "authenticated-read":
439+
if len(grantsMap) != 2 {
440+
errs = append(errs, fmt.Errorf("expected 2 grants, but got %d", len(grantsMap)))
441+
return errs
442+
}
443+
if grantsMap["FULL_CONTROL"] != ownerID {
444+
errs = append(errs, fmt.Errorf("expected FULL_CONTROL to be granted to owner (%s), instead got %q", ownerID, grantsMap["FULL_CONTROL"]))
445+
}
446+
if grantsMap["READ"] != s3ACLGranteeAuthenticatedUsers {
447+
errs = append(errs, fmt.Errorf("expected READ to be granted to %q, instead got %q", s3ACLGranteeAuthenticatedUsers, grantsMap["READ"]))
448+
}
449+
}
450+
return errs
451+
}

‎internal/services/object/bucket_lock_configuration.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func resourceObjectLockConfigurationRead(ctx context.Context, d *schema.Resource
148148
if err != nil {
149149
return diag.FromErr(fmt.Errorf("couldn't read bucket acl: %s", err))
150150
}
151-
_ = d.Set("project_id", normalizeOwnerID(acl.Owner.ID))
151+
_ = d.Set("project_id", NormalizeOwnerID(acl.Owner.ID))
152152

153153
_ = d.Set("bucket", bucket)
154154
if output.ObjectLockConfiguration != nil {

‎internal/services/object/bucket_policy.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ func resourceObjectBucketPolicyRead(ctx context.Context, d *schema.ResourceData,
159159
return diags
160160
}
161161
} else if acl != nil && acl.Owner != nil {
162-
_ = d.Set("project_id", normalizeOwnerID(acl.Owner.ID))
162+
_ = d.Set("project_id", NormalizeOwnerID(acl.Owner.ID))
163163
}
164164

165165
return diags

‎internal/services/object/bucket_website_configuration.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ func resourceBucketWebsiteConfigurationRead(ctx context.Context, d *schema.Resou
189189
if err != nil {
190190
return diag.FromErr(fmt.Errorf("couldn't read bucket acl: %s", err))
191191
}
192-
_ = d.Set("project_id", normalizeOwnerID(acl.Owner.ID))
192+
_ = d.Set("project_id", NormalizeOwnerID(acl.Owner.ID))
193193

194194
return nil
195195
}

‎internal/services/object/data_source_object_bucket.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func DataSourceObjectStorageRead(ctx context.Context, d *schema.ResourceData, m
6060
if err != nil {
6161
return diag.FromErr(fmt.Errorf("couldn't read bucket acl: %s", err))
6262
}
63-
_ = d.Set("project_id", normalizeOwnerID(acl.Owner.ID))
63+
_ = d.Set("project_id", NormalizeOwnerID(acl.Owner.ID))
6464

6565
bucketRegionalID := regional.NewIDString(region, bucket)
6666
d.SetId(bucketRegionalID)

‎internal/services/object/data_source_object_bucket_policy.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func DataSourceObjectBucketPolicyRead(ctx context.Context, d *schema.ResourceDat
7878
if err != nil {
7979
return diag.FromErr(fmt.Errorf("couldn't read bucket acl: %s", err))
8080
}
81-
_ = d.Set("project_id", normalizeOwnerID(acl.Owner.ID))
81+
_ = d.Set("project_id", NormalizeOwnerID(acl.Owner.ID))
8282

8383
d.SetId(regional.NewIDString(region, bucket))
8484
return nil

‎internal/services/object/helpers_object.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,7 @@ func buildBucketOwnerID(id *string) *string {
561561
return &s
562562
}
563563

564-
func normalizeOwnerID(id *string) *string {
564+
func NormalizeOwnerID(id *string) *string {
565565
tab := strings.Split(*id, ":")
566566
if len(tab) != 2 {
567567
return id

0 commit comments

Comments
 (0)