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.

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

providers/aws: Add support for policy on S3 bucket #1992

Merged
merged 3 commits into from
May 18, 2015

Conversation

justincampbell
Copy link
Contributor

Adds support for policies on S3 buckets: http://docs.aws.amazon.com/AmazonS3/latest/dev/example-bucket-policies.html

resource "aws_s3_bucket" "default" {
  bucket = "hc-jc-tf-test"
  policy = "${file("policy.json")}"
}


if v := out.Policy; v == nil {
if policy != "" {
return fmt.Errorf("bad policy, found nil, expected: %s", policy)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently fails due to aws/aws-sdk-go#236

I believe @m-s-austin also ran into this in iJoinSolutions@cd45e45

Copy link
Contributor

Choose a reason for hiding this comment

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

That does indeed appear to be bugged in the sdk, I had not had a chance to test it directly yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been resolved in aws/aws-sdk-go@c37b3cb

@justincampbell
Copy link
Contributor Author

--- PASS: TestAccAWSS3Bucket_basic (3.80s)
--- PASS: TestAccAWSS3Bucket_Policy (4.22s)
--- PASS: TestAccAWSS3Bucket_Website (7.14s)
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (4.78s)

@m-s-austin
Copy link
Contributor

@justincampbell I have been testing your code today, and one thing I noticed, in the version I was working on with a separate resource for the policy, I could delete the policy resource from the tf file, and then terraform apply would also delete the policy from the bucket. It seems that in this version I can not achieve the same functionality? Deleting the argument has no action and if I try to explicitly set policy = "" I get an error policy: "" => "Error parsing JSON: unexpected end of JSON input"

leaving me with no way to remove a policy from tf.

@catsby
Copy link
Contributor

catsby commented May 18, 2015

TestAccAWSS3Bucket_Policy is not passing for me:

provider.aws
--- FAIL: TestAccAWSS3Bucket_Policy (3.11s)
    testing.go:131: Step 0 error: Check failed: bad policy, found nil, expected: { "Version": "2008-10-17", "Statement": [ { "Sid": "", "Effect": "Allow", "Principal": { "AWS": "*" }, "Action": "s3:GetObject", "Resource": "arn:aws:s3:::tf-test-bucket-7742106432384881169/*" } ] }
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    3.126s

@justincampbell
Copy link
Contributor Author

@m-s-austin Are you using the latest code from this PR? It seems to remove properly for me.

The "" => "Error parsing JSON: unexpected end of JSON input" behavior is unfortunate with invalid JSON. We're using normalizeJson as a SchemaStateFunc, which only returns a string: https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go#L174

I'm not sure what to do here. We could change SchemaStateFunc to also return an error. Or, just return an empty string, or possibly the original string passed in, on invalid JSON.

@justincampbell
Copy link
Contributor Author

@catsby You may need to update aws-sdk-go

@catsby
Copy link
Contributor

catsby commented May 18, 2015

@justincampbell that did help, thanks 😄

It seems though that I have invalid JSON:

+ aws_s3_bucket.bucket
    acl:              "" => "public-read"
    bucket:           "" => "tf-test-bucket-14"
    hosted_zone_id:   "" => "<computed>"
    policy:           "" => "Error parsing JSON: invalid character '\\'' looking for beginning of object key string"
    region:           "" => "<computed>"
    website_endpoint: "" => "<computed>"

I do not want that policy applied 😸

@catsby
Copy link
Contributor

catsby commented May 18, 2015

That issue aside, I used a valid policy JSON doc (from a file) and it seemed to apply / remove as expected.

@justincampbell did you test this with in-line policies? That's where I got the parsing error, I wonder if Go or HCL did something there. If you anticipate most people using from a file, we should provide show the "${file("./policy.json")}" example in the docs

Other than that, 👍

@m-s-austin
Copy link
Contributor

@justincampbell sorry that was my bad I forgot to update sdk.

Seems to be working now

@justincampbell
Copy link
Contributor Author

@catsby Does a heredoc work? Otherwise you would have to escape all of the quotes

@m-s-austin
Copy link
Contributor

I have tested with this format it works fine:

    policy = <<EOF
{
  "Id": "Policy1431447664360",
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "Stmt1431447659874",
      "Action": [
        "s3:GetObject"
      ],
      "Effect": "Allow",
      "Resource": "arn:aws:s3:::my_tf_test_bucket/*",
      "Principal": "*"
    }
  ]
}
EOF

@justincampbell
Copy link
Contributor Author

@m-s-austin Thanks for testing!

AWS always returns a compressed JSON body, without spaces or newlines, so we round-trip the JSON before storing in the state.
@justincampbell
Copy link
Contributor Author

@catsby Added policy = "#{file("policy.json")}" to the website example

@catsby
Copy link
Contributor

catsby commented May 18, 2015

LGTM 👍

justincampbell added a commit that referenced this pull request May 18, 2015
providers/aws: Add support for policy on S3 bucket
@justincampbell justincampbell merged commit c585aae into hashicorp:master May 18, 2015
@justincampbell justincampbell deleted the s3-policy branch May 18, 2015 17:35
@justincampbell justincampbell restored the s3-policy branch April 5, 2016 15:10
@ghost
Copy link

ghost commented Apr 26, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants