Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add acl validator to aws s3 bucket resource #15327

Merged
merged 2 commits into from
Nov 10, 2020
Merged

add acl validator to aws s3 bucket resource #15327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca6c281
add in currently documented s3 bucket canned acls to validator
NikolaeVarius Sep 24, 2020
0544ae1
add validator for s3 bucket acl
NikolaeVarius Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions aws/resource_aws_s3_bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ func resourceAwsS3Bucket() *schema.Resource {
Default: "private",
Optional: true,
ConflictsWith: []string{"grant"},
ValidateFunc: validation.StringInSlice([]string{
Copy link
Contributor

@reedloden reedloden Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding them, how about just using s3.BucketCannedACL_Values()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont really have a preference. But, looking at other validation functions, they seem to list out all the values. I dont think I want to go against the style?

https://github.com/terraform-providers/terraform-provider-aws/blob/0544ae1fe0f788616cc483df8d0e68d3dd36a0ed/aws/resource_aws_s3_bucket.go#L120
https://github.com/terraform-providers/terraform-provider-aws/blob/0544ae1fe0f788616cc483df8d0e68d3dd36a0ed/aws/resource_aws_s3_bucket.go#L406
https://github.com/terraform-providers/terraform-provider-aws/blob/0544ae1fe0f788616cc483df8d0e68d3dd36a0ed/aws/resource_aws_s3_bucket.go#L453

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @NikolaeVarius. We've actually been changing over to the _Values() functions so that new values are automatically picked up when we update the AWS SDK. We're tracking this in #14601

The _Values() functions were actually added by one of our maintainers in aws/aws-sdk-go#3447 🙂

s3.BucketCannedACLPrivate,
s3.BucketCannedACLPublicRead,
s3.BucketCannedACLPublicReadWrite,
s3.BucketCannedACLAWSExecRead,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this exists for buckets... For objects, there's ObjectCannedACLAwsExecRead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl

image

It seems to say it works on both

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK doesn't seem to support it. See https://docs.aws.amazon.com/sdk-for-go/api/service/s3/#pkg-constants. Or if you look at the code directly (https://raw.githubusercontent.com/aws/aws-sdk-go/master/service/s3/api.go), you can take a look at the BucketCannedACL_Values() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you're correct. Those values are exist at the ObjectCannedACL_Values() function. I dont know the behavior just yet, and would need to test, but in this case, would it be wise to add this validator in if the upstream SDK doesn't support all possible values for a bucket? Wouldn't want to return a validation error when the ACL is perfectly valid.

// ObjectCannedACL_Values returns all elements of the ObjectCannedACL enum
func ObjectCannedACL_Values() []string {
	return []string{
		ObjectCannedACLPrivate,
		ObjectCannedACLPublicRead,
		ObjectCannedACLPublicReadWrite,
		ObjectCannedACLAuthenticatedRead,
		ObjectCannedACLAwsExecRead,
		ObjectCannedACLBucketOwnerRead,
		ObjectCannedACLBucketOwnerFullControl,
	}
}

s3.BucketCannedACLAuthenticatedRead,
s3.BucketCannedACLLogDeliveryWrite,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in the other comment, yeah you're correct

}, false),
},

"grant": {
Expand Down
8 changes: 8 additions & 0 deletions vendor/github.com/aws/aws-sdk-go/service/s3/api.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.