Skip to content

Commit

Permalink
simplify resource ID parsing with regex; create resource ID with only…
Browse files Browse the repository at this point in the history 
… comma separators; add id test coverage
  • Loading branch information
@anGie44
anGie44 committed Feb 5, 2022
1 parent c913409 commit 0a1c397
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 42 deletions.
71 changes: 43 additions & 28 deletions internal/service/s3/bucket_acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"log"
"regexp"
"strings"

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

const (
BucketAclSeparator = "/"
BucketAndExpectedBucketOwnerSeparator = ","
)
const BucketAclSeparator = ","

func ResourceBucketAcl() *schema.Resource {
return &schema.Resource{
Expand Down Expand Up @@ -451,42 +449,59 @@ func BucketACLCreateResourceID(bucket, expectedBucketOwner, acl string) string {
}

if acl == "" {
return strings.Join([]string{bucket, expectedBucketOwner}, BucketAndExpectedBucketOwnerSeparator)
return strings.Join([]string{bucket, expectedBucketOwner}, BucketAclSeparator)
}

parts := []string{bucket, expectedBucketOwner}
id := strings.Join([]string{strings.Join(parts, BucketAndExpectedBucketOwnerSeparator), acl}, BucketAclSeparator)

return id
return strings.Join([]string{bucket, expectedBucketOwner, acl}, BucketAclSeparator)
}

// BucketACLParseResourceID is a method for parsing the ID string
// for the bucket name, accountID, and ACL if provided.
func BucketACLParseResourceID(id string) (string, string, string, error) {
idParts := strings.Split(id, BucketAndExpectedBucketOwnerSeparator)

// Bucket name and optional ACL
if len(idParts) == 1 && idParts[0] != "" {
parts := strings.Split(idParts[0], BucketAclSeparator)

if len(parts) == 1 { // no ACL in ID
return parts[0], "", "", nil
} else if len(parts) == 2 && parts[0] != "" && parts[1] != "" { // ACL in ID
return parts[0], "", parts[1], nil
// For only bucket name in the ID e.g. bucket
// ~> Bucket names can consist of only lowercase letters, numbers, dots, and hyphens; Max 63 characters
bucketRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63}$`)
// For bucket and accountID in the ID e.g. bucket,123456789101
// ~> Account IDs must consist of 12 digits
bucketAndOwnerRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63},\d{12}$`)
// For bucket and ACL in the ID e.g. bucket,public-read
// ~> (Canned) ACL values include: private, public-read, public-read-write, authenticated-read, aws-exec-read, and log-delivery-write
bucketAndAclRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63},[a-z-]+$`)
// For bucket, accountID, and ACL in the ID e.g. bucket,123456789101,public-read
bucketOwnerAclRegex := regexp.MustCompile(`^[a-z0-9.-]{1,63},\d{12},[a-z-]+$`)

// Bucket name ONLY
if bucketRegex.MatchString(id) {
return id, "", "", nil
}

// Bucket and Account ID ONLY
if bucketAndOwnerRegex.MatchString(id) {
parts := strings.Split(id, BucketAclSeparator)
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET%sEXPECTED_BUCKET_OWNER", id, BucketAclSeparator)
}
return parts[0], parts[1], "", nil
}

// Bucket name, expected bucket owner (i.e. account ID) and optional ACL
if len(idParts) == 2 && idParts[0] != "" && idParts[1] != "" {
parts := strings.Split(idParts[1], BucketAclSeparator)
// Bucket and ACL ONLY
if bucketAndAclRegex.MatchString(id) {
parts := strings.Split(id, BucketAclSeparator)
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET%sACL", id, BucketAclSeparator)
}
return parts[0], "", parts[1], nil
}

if len(parts) == 1 { // no ACL in ID
return idParts[0], parts[0], "", nil
} else if len(parts) == 2 && parts[0] != "" && parts[1] != "" { // ACL in ID
return idParts[0], parts[0], parts[1], nil
// Bucket, Account ID, and ACL
if bucketOwnerAclRegex.MatchString(id) {
parts := strings.Split(id, BucketAclSeparator)
if len(parts) != 3 || parts[0] == "" || parts[1] == "" || parts[2] == "" {
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET%[2]sEXPECTED_BUCKET_OWNER%[2]sACL", id, BucketAclSeparator)
}
return parts[0], parts[1], parts[2], nil
}

return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET or BUCKET%[2]sEXPECTED_BUCKET_OWNER or BUCKET%[3]sACL "+
"or BUCKET%[2]sEXPECTED_BUCKET_OWNER%[3]sACL", id, BucketAndExpectedBucketOwnerSeparator, BucketAclSeparator)
return "", "", "", fmt.Errorf("unexpected format for ID (%s), expected BUCKET or BUCKET%[2]sEXPECTED_BUCKET_OWNER or BUCKET%[2]sACL "+
"or BUCKET%[2]sEXPECTED_BUCKET_OWNER%[2]sACL", id, BucketAclSeparator)
}
77 changes: 68 additions & 9 deletions internal/service/s3/bucket_acl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,23 @@ func TestBucketACLParseResourceID(t *testing.T) {
ExpectError: true,
},
{
TestName: "incorrect format with comma separators",
InputID: "test,example,123456789012",
TestName: "incorrect bucket and account ID format with slash separator",
InputID: "test/123456789012",
ExpectError: true,
},
{
TestName: "incorrect format with slash separators",
InputID: "test/example/123456789012",
TestName: "incorrect bucket, account ID, and ACL format with slash separators",
InputID: "test/123456789012/private",
ExpectError: true,
},
{
TestName: "incorrect bucket, account ID, and ACL format with mixed separators",
InputID: "test/123456789012,private",
ExpectError: true,
},
{
TestName: "incorrect bucket, ACL, and account ID format",
InputID: "test,private,123456789012",
ExpectError: true,
},
{
Expand All @@ -46,13 +56,48 @@ func TestBucketACLParseResourceID(t *testing.T) {
ExpectedBucket: "example",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket that has hyphens",
InputID: tfs3.BucketACLCreateResourceID("my-example-bucket", "", ""),
ExpectedACL: "",
ExpectedBucket: "my-example-bucket",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket that has dot and hyphens",
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket", "", ""),
ExpectedACL: "",
ExpectedBucket: "my-example.bucket",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket that has dots, hyphen, and numbers",
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "", ""),
ExpectedACL: "",
ExpectedBucket: "my-example.bucket.4000",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket and acl",
InputID: tfs3.BucketACLCreateResourceID("example", "", s3.BucketCannedACLPrivate),
ExpectedACL: s3.BucketCannedACLPrivate,
ExpectedBucket: "example",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket and acl that has hyphens",
InputID: tfs3.BucketACLCreateResourceID("example", "", s3.BucketCannedACLPublicReadWrite),
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
ExpectedBucket: "example",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket that has dot, hyphen, and number and acl that has hyphens",
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "", s3.BucketCannedACLPublicReadWrite),
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
ExpectedBucket: "my-example.bucket.4000",
ExpectedBucketOwner: "",
},
{
TestName: "valid ID with bucket and bucket owner",
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", ""),
Expand All @@ -61,19 +106,33 @@ func TestBucketACLParseResourceID(t *testing.T) {
ExpectedBucketOwner: "123456789012",
},
{
TestName: "valid ID with bucket, bucket owner, and 'private' acl",
TestName: "valid ID with bucket that has dot, hyphen, and number and bucket owner",
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "123456789012", ""),
ExpectedACL: "",
ExpectedBucket: "my-example.bucket.4000",
ExpectedBucketOwner: "123456789012",
},
{
TestName: "valid ID with bucket, bucket owner, and acl",
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", s3.BucketCannedACLPrivate),
ExpectedACL: s3.BucketCannedACLPrivate,
ExpectedBucket: "example",
ExpectedBucketOwner: "123456789012",
},
{
TestName: "valid ID with bucket, bucket owner, and 'public-read' acl",
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", s3.BucketCannedACLPublicRead),
ExpectedACL: s3.BucketCannedACLPublicRead,
TestName: "valid ID with bucket, bucket owner, and acl that has hyphens",
InputID: tfs3.BucketACLCreateResourceID("example", "123456789012", s3.BucketCannedACLPublicReadWrite),
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
ExpectedBucket: "example",
ExpectedBucketOwner: "123456789012",
},
{
TestName: "valid ID with bucket that has dot, hyphen, and numbers, bucket owner, and acl that has hyphens",
InputID: tfs3.BucketACLCreateResourceID("my-example.bucket.4000", "123456789012", s3.BucketCannedACLPublicReadWrite),
ExpectedACL: s3.BucketCannedACLPublicReadWrite,
ExpectedBucket: "my-example.bucket.4000",
ExpectedBucketOwner: "123456789012",
},
}

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

if gotAcl != testCase.ExpectedACL {
t.Errorf("got bucket %s, expected %s", gotAcl, testCase.ExpectedACL)
t.Errorf("got ACL %s, expected %s", gotAcl, testCase.ExpectedACL)
}

if gotBucket != testCase.ExpectedBucket {
Expand Down
10 changes: 5 additions & 5 deletions website/docs/r/s3_bucket_acl.html.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ The `grantee` configuration block supports the following arguments:

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

* `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.
* `id` - The `bucket`, `expected_bucket_owner` (if configured), and `acl` (if configured) separated by commas (`,`).

## Import

Expand All @@ -115,10 +115,10 @@ S3 bucket ACL can be imported using the `bucket` e.g.,
$ terraform import aws_s3_bucket_acl.example bucket-name
```

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

```
$ terraform import aws_s3_bucket_acl.example bucket-name/private
$ terraform import aws_s3_bucket_acl.example bucket-name,private
```

S3 bucket ACL can also be imported using the `bucket` and `expected_bucket_owner` separated by a comma (`,`) e.g.,
Expand All @@ -127,8 +127,8 @@ S3 bucket ACL can also be imported using the `bucket` and `expected_bucket_owner
$ terraform import aws_s3_bucket_acl.example bucket-name,123456789012
```

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.,
S3 bucket ACL can also be imported using the `bucket`, `expected_bucket_owner`, and `acl` separated by commas (`,`) e.g.,

```
$ terraform import aws_s3_bucket_acl.example bucket-name,123456789012/private
$ terraform import aws_s3_bucket_acl.example bucket-name,123456789012,private
```

0 comments on commit 0a1c397

Please sign in to comment.