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.

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

provider/aws: Add aws_s3_bucket_policy resource #8615

Merged
merged 2 commits into from Sep 2, 2016

Conversation

Projects
None yet
3 participants
@jen20
Copy link
Contributor

jen20 commented Sep 1, 2016

This commit adds a new "attachment" style resource for setting the policy of an AWS S3 bucket. This is desirable such that the ARN of the bucket can be referenced in an IAM Policy Document.

In addition, we now suppress diffs on the (now-computed) policy in the S3 bucket for structurally equivalent policies, which prevents flapping because of whitespace and map ordering changes made by the S3 endpoint.

@jen20 jen20 force-pushed the f-aws-s3-bucket-policy branch 2 times, most recently from 3abd21c to c251c5f Sep 1, 2016

@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Sep 1, 2016

Just realised I forgot the docs, will add.

package aws

import (
//"encoding/json"

This comment has been minimized.

@mitchellh

mitchellh Sep 2, 2016

Member

Worth removing?

} else if err := d.Set("policy", v); err != nil {
return err
}
}

This comment has been minimized.

@mitchellh

mitchellh Sep 2, 2016

Member

First, is it okay to be throwing away the first error? (if err != nil)

Second, I think this logic can be simplified quite a bit:

v := ""
if err == nil && pol.Policy != nil {
  v = *pol.Policy
}
if err := d.Set("policy", v); err != nil {
  return err
}

This comment has been minimized.

@jen20

jen20 Sep 2, 2016

Author Contributor

Yup, good catch, this was a hangover from extracting the same code out of aws_s3_bucket, we should probably clean it up there too.

@mitchellh

This comment has been minimized.

Copy link
Member

mitchellh commented Sep 2, 2016

A few reviews, and a meta question: I have no problem with that lib being under your account, but I also wouldn't be against it just being a sub-library within the Terraform repo if it is fairly specialized to us. Either way.

jen20 added some commits Sep 1, 2016

provider/aws: Add aws_s3_bucket_policy resource
This commit adds a new "attachment" style resource for setting the
policy of an AWS S3 bucket. This is desirable such that the ARN of the
bucket can be referenced in an IAM Policy Document.

In addition, we now suppress diffs on the (now-computed) policy in the
S3 bucket for structurally equivalent policies, which prevents flapping
because of whitespace and map ordering changes made by the S3 endpoint.

@jen20 jen20 force-pushed the f-aws-s3-bucket-policy branch from ffeb930 to 93f31fc Sep 2, 2016

@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Sep 2, 2016

I think over time (once we have more confidence in effectiveness across a wider range of policy types) we should bring it to be a helper library in the AWS provider. For now I think it's better to keep it vendored so it can be worked on separately without risking regression in Terraform?

@mitchellh

This comment has been minimized.

Copy link
Member

mitchellh commented Sep 2, 2016

LGTM!

@jen20 jen20 merged commit bb3a896 into master Sep 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jen20 jen20 deleted the f-aws-s3-bucket-policy branch Sep 2, 2016

@kwilczynski

This comment has been minimized.

Copy link
Contributor

kwilczynski commented Sep 14, 2016

@mitchellh the change here that introduces Computed: true to the policy in S3 bucket seem to be causing this (on master):

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Policy'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/14 14:47:14 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Policy -timeout 120m
=== RUN   TestAccAWSS3Bucket_Policy
--- FAIL: TestAccAWSS3Bucket_Policy (52.19s)
        testing.go:265: Step 1 error: Check failed: Check 2/2 error: unexpected end of JSON input
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    52.206s
make: *** [testacc] Error 1

Either the test or the ReadFunc needs to be updated, I haven't looked into this yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment