Skip to content

Commit 5a1fab2

Browse files
anGie44anGie44
committed
simplify resource ID parsing with regex; create resource ID with only comma separators; add id test coverage
1 parent c913409 commit 5a1fab2

File tree

3 files changed

+117
-43
lines changed

3 files changed

+117
-43
lines changed

‎internal/service/s3/bucket_acl.go

+43-28
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"log"
7+
"regexp"
78
"strings"
89

910
"github.com/aws/aws-sdk-go/aws"
@@ -16,10 +17,7 @@ import (
1617
"github.com/hashicorp/terraform-provider-aws/internal/verify"
1718
)
1819

19-
const (
20-
BucketAclSeparator = "/"
21-
BucketAndExpectedBucketOwnerSeparator = ","
22-
)
20+
const BucketAclSeparator = ","
2321

2422
func ResourceBucketAcl() *schema.Resource {
2523
return &schema.Resource{
@@ -451,42 +449,59 @@ func BucketACLCreateResourceID(bucket, expectedBucketOwner, acl string) string {
451449
}
452450

453451
if acl == "" {
454-
return strings.Join([]string{bucket, expectedBucketOwner}, BucketAndExpectedBucketOwnerSeparator)
452+
return strings.Join([]string{bucket, expectedBucketOwner}, BucketAclSeparator)
455453
}
456454

457-
parts := []string{bucket, expectedBucketOwner}
458-
id := strings.Join([]string{strings.Join(parts, BucketAndExpectedBucketOwnerSeparator), acl}, BucketAclSeparator)
459-
460-
return id
455+
return strings.Join([]string{bucket, expectedBucketOwner, acl}, BucketAclSeparator)
461456
}
462457

463458
// BucketACLParseResourceID is a method for parsing the ID string
464459
// for the bucket name, accountID, and ACL if provided.
465460
func BucketACLParseResourceID(id string) (string, string, string, error) {
466-
idParts := strings.Split(id, BucketAndExpectedBucketOwnerSeparator)
467-
468-
// Bucket name and optional ACL
469-
if len(idParts) == 1 && idParts[0] != "" {
470-
parts := strings.Split(idParts[0], BucketAclSeparator)
471-
472-
if len(parts) == 1 { // no ACL in ID
473-
return parts[0], "", "", nil
474-
} else if len(parts) == 2 && parts[0] != "" && parts[1] != "" { // ACL in ID
475-
return parts[0], "", parts[1], nil
461+
// For only bucket name in the ID e.g. bucket
462+
// ~> Bucket names can consist of only lowercase letters, numbers, dots, and hyphens; Max 63 characters
463+
bucketRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63}$`)
464+
// For bucket and accountID in the ID e.g. bucket,123456789101
465+
// ~> Account IDs must consist of 12 digits
466+
bucketAndOwnerRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63},\d{12}$`)
467+
// For bucket and ACL in the ID e.g. bucket,public-read
468+
// ~> (Canned) ACL values include: private, public-read, public-read-write, authenticated-read, aws-exec-read, and log-delivery-write
469+
bucketAndAclRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63},[a-z-]+$`)
470+
// For bucket, accountID, and ACL in the ID e.g. bucket,123456789101,public-read
471+
bucketOwnerAclRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63},\d{12},[a-z-]+$`)
472+
473+
// Bucket name ONLY
474+
if bucketRegex.MatchString(id) {
475+
return id, "", "", nil
476+
}
477+
478+
// Bucket and Account ID ONLY
479+
if bucketAndOwnerRegex.MatchString(id) {
480+
parts := strings.Split(id, BucketAclSeparator)
481+
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
482+
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET%sEXPECTED_BUCKET_OWNER", id, BucketAclSeparator)
476483
}
484+
return parts[0], parts[1], "", nil
477485
}
478486

479-
// Bucket name, expected bucket owner (i.e. account ID) and optional ACL
480-
if len(idParts) == 2 && idParts[0] != "" && idParts[1] != "" {
481-
parts := strings.Split(idParts[1], BucketAclSeparator)
487+
// Bucket and ACL ONLY
488+
if bucketAndAclRegex.MatchString(id) {
489+
parts := strings.Split(id, BucketAclSeparator)
490+
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
491+
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET%sACL", id, BucketAclSeparator)
492+
}
493+
return parts[0], "", parts[1], nil
494+
}
482495

483-
if len(parts) == 1 { // no ACL in ID
484-
return idParts[0], parts[0], "", nil
485-
} else if len(parts) == 2 && parts[0] != "" && parts[1] != "" { // ACL in ID
486-
return idParts[0], parts[0], parts[1], nil
496+
// Bucket, Account ID, and ACL
497+
if bucketOwnerAclRegex.MatchString(id) {
498+
parts := strings.Split(id, BucketAclSeparator)
499+
if len(parts) != 3 || parts[0] == "" || parts[1] == "" || parts[2] == "" {
500+
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET%[2]sEXPECTED_BUCKET_OWNER%[2]sACL", id, BucketAclSeparator)
487501
}
502+
return parts[0], parts[1], parts[2], nil
488503
}
489504

490-
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET or BUCKET%[2]sEXPECTED_BUCKET_OWNER or BUCKET%[3]sACL "+
491-
"or BUCKET%[2]sEXPECTED_BUCKET_OWNER%[3]sACL", id, BucketAndExpectedBucketOwnerSeparator, BucketAclSeparator)
505+
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET or BUCKET%[2]sEXPECTED_BUCKET_OWNER or BUCKET%[2]sACL "+
506+
"or BUCKET%[2]sEXPECTED_BUCKET_OWNER%[2]sACL", id, BucketAclSeparator)
492507
}

‎internal/service/s3/bucket_acl_test.go

+68-9
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,23 @@ func TestBucketACLParseResourceID(t *testing.T) {
3030
ExpectError: true,
3131
},
3232
{
33-
TestName: "incorrect format with comma separators",
34-
InputID: "test,example,123456789012",
33+
TestName: "incorrect bucket and account ID format with slash separator",
34+
InputID: "test/123456789012",
3535
ExpectError: true,
3636
},
3737
{
38-
TestName: "incorrect format with slash separators",
39-
InputID: "test/example/123456789012",
38+
TestName: "incorrect bucket, account ID, and ACL format with slash separators",
39+
InputID: "test/123456789012/private",
40+
ExpectError: true,
41+
},
42+
{
43+
TestName: "incorrect bucket, account ID, and ACL format with mixed separators",
44+
InputID: "test/123456789012,private",
45+
ExpectError: true,
46+
},
47+
{
48+
TestName: "incorrect bucket, ACL, and account ID format",
49+
InputID: "test,private,123456789012",
4050
ExpectError: true,
4151
},
4252
{
@@ -46,13 +56,48 @@ func TestBucketACLParseResourceID(t *testing.T) {
4656
ExpectedBucket: "example",
4757
ExpectedBucketOwner: "",
4858
},
59+
{
60+
TestName: "valid ID with bucket that has hyphens",
61+
InputID: tfs3.BucketACLCreateResourceID("my-example-bucket", "", ""),
62+
ExpectedACL: "",
63+
ExpectedBucket: "my-example-bucket",
64+
ExpectedBucketOwner: "",
65+
},
66+
{
67+
TestName: "valid ID with bucket that has dot and hyphens",
68+
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket", "", ""),
69+
ExpectedACL: "",
70+
ExpectedBucket: "my-example.bucket",
71+
ExpectedBucketOwner: "",
72+
},
73+
{
74+
TestName: "valid ID with bucket that has dots, hyphen, and numbers",
75+
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "", ""),
76+
ExpectedACL: "",
77+
ExpectedBucket: "my-example.bucket.4000",
78+
ExpectedBucketOwner: "",
79+
},
4980
{
5081
TestName: "valid ID with bucket and acl",
5182
InputID: tfs3.BucketACLCreateResourceID("example", "", s3.BucketCannedACLPrivate),
5283
ExpectedACL: s3.BucketCannedACLPrivate,
5384
ExpectedBucket: "example",
5485
ExpectedBucketOwner: "",
5586
},
87+
{
88+
TestName: "valid ID with bucket and acl that has hyphens",
89+
InputID: tfs3.BucketACLCreateResourceID("example", "", s3.BucketCannedACLPublicReadWrite),
90+
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
91+
ExpectedBucket: "example",
92+
ExpectedBucketOwner: "",
93+
},
94+
{
95+
TestName: "valid ID with bucket that has dot, hyphen, and number and acl that has hyphens",
96+
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "", s3.BucketCannedACLPublicReadWrite),
97+
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
98+
ExpectedBucket: "my-example.bucket.4000",
99+
ExpectedBucketOwner: "",
100+
},
56101
{
57102
TestName: "valid ID with bucket and bucket owner",
58103
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", ""),
@@ -61,19 +106,33 @@ func TestBucketACLParseResourceID(t *testing.T) {
61106
ExpectedBucketOwner: "123456789012",
62107
},
63108
{
64-
TestName: "valid ID with bucket, bucket owner, and 'private' acl",
109+
TestName: "valid ID with bucket that has dot, hyphen, and number and bucket owner",
110+
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "123456789012", ""),
111+
ExpectedACL: "",
112+
ExpectedBucket: "my-example.bucket.4000",
113+
ExpectedBucketOwner: "123456789012",
114+
},
115+
{
116+
TestName: "valid ID with bucket, bucket owner, and acl",
65117
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", s3.BucketCannedACLPrivate),
66118
ExpectedACL: s3.BucketCannedACLPrivate,
67119
ExpectedBucket: "example",
68120
ExpectedBucketOwner: "123456789012",
69121
},
70122
{
71-
TestName: "valid ID with bucket, bucket owner, and 'public-read' acl",
72-
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", s3.BucketCannedACLPublicRead),
73-
ExpectedACL: s3.BucketCannedACLPublicRead,
123+
TestName: "valid ID with bucket, bucket owner, and acl that has hyphens",
124+
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", s3.BucketCannedACLPublicReadWrite),
125+
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
74126
ExpectedBucket: "example",
75127
ExpectedBucketOwner: "123456789012",
76128
},
129+
{
130+
TestName: "valid ID with bucket that has dot, hyphen, and numbers, bucket owner, and acl that has hyphens",
131+
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "123456789012", s3.BucketCannedACLPublicReadWrite),
132+
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
133+
ExpectedBucket: "my-example.bucket.4000",
134+
ExpectedBucketOwner: "123456789012",
135+
},
77136
}
78137

79138
for _, testCase := range testCases {
@@ -89,7 +148,7 @@ func TestBucketACLParseResourceID(t *testing.T) {
89148
}
90149

91150
if gotAcl != testCase.ExpectedACL {
92-
t.Errorf("got bucket %s, expected %s", gotAcl, testCase.ExpectedACL)
151+
t.Errorf("got ACL %s, expected %s", gotAcl, testCase.ExpectedACL)
93152
}
94153

95154
if gotBucket != testCase.ExpectedBucket {

‎website/docs/r/s3_bucket_acl.html.markdown

+6-6
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ The `grantee` configuration block supports the following arguments:
105105

106106
In addition to all arguments above, the following attributes are exported:
107107

108-
* `id` - The `bucket` and `expected_bucket_owner` separated by a comma (`,`) if the latter is provided, and the `acl` prefixed with a slash (`/`) if configured.
108+
* `id` - The `bucket`, `expected_bucket_owner` (if configured), and `acl` (if configured) separated by commas (`,`).
109109

110110
## Import
111111

@@ -115,20 +115,20 @@ S3 bucket ACL can be imported using the `bucket` e.g.,
115115
$ terraform import aws_s3_bucket_acl.example bucket-name
116116
```
117117

118-
S3 bucket ACL can also be imported using the `bucket` and `acl` separated by a slash (`/`) e.g.,
118+
S3 bucket ACL can also be imported using the `bucket` and `acl` separated by a comma (`,`), e.g.
119119

120120
```
121-
$ terraform import aws_s3_bucket_acl.example bucket-name/private
121+
$ terraform import aws_s3_bucket_acl.example bucket-name,private
122122
```
123123

124-
S3 bucket ACL can also be imported using the `bucket` and `expected_bucket_owner` separated by a comma (`,`) e.g.,
124+
S3 bucket ACL can also be imported using the `bucket` and `expected_bucket_owner` separated by a comma (`,`), e.g.
125125

126126
```
127127
$ terraform import aws_s3_bucket_acl.example bucket-name,123456789012
128128
```
129129

130-
S3 bucket ACL can also be imported using the `bucket` and `expected_bucket_owner` separated by a comma (`,`) and `acl` prefixed with a slash (`/`) e.g.,
130+
S3 bucket ACL can also be imported using the `bucket`, `expected_bucket_owner`, and `acl` separated by commas (`,`), e.g.
131131

132132
```
133-
$ terraform import aws_s3_bucket_acl.example bucket-name,123456789012/private
133+
$ terraform import aws_s3_bucket_acl.example bucket-name,123456789012,private
134134
```

0 commit comments

Comments
 (0)