#843: Add policy support to storage buckets #1190

ReneKroon · 2018-03-14T12:50:39Z

#843 is fixed by this line, after the consensus that legacy roles should not be implicitly supported.

Since this is my first commit to the project, i'd like to have some pointers on testing. For example this change only wraps existing policy functionality on a bucket. Does this require a separate testcase anyway?

Since using this feature could remove legacy rows, how would a documentation update for this change work?

Thanks in advance,

Rene Kroon

nat-henderson · 2018-03-14T19:29:16Z

You'll need to add documentation for this under website/docs/r/storage_bucket_iam.html.markdown: https://github.com/terraform-providers/terraform-provider-google/blob/master/website/docs/r/storage_bucket_iam.html.markdown

Please be sure to explicitly call out the legacy roles that might be auto-removed!

As for testing, let me look into that and get back to you.

nat-henderson · 2018-03-14T19:29:58Z

You'll just need to add a simple test to https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_storage_bucket_iam_test.go - similar to what's already there.

ReneKroon · 2018-03-15T13:17:10Z

@ndmckinley I adressed the documentation and test. It also explains what happens if the legacy roles are removed and how that can be restored. The test is alike to IAM Binding. As far as i can tell this policy is now on par with the other two methods of assigning permissions.

nat-henderson · 2018-03-15T17:22:44Z

website/docs/r/storage_bucket_iam.html.markdown


 * `google_storage_bucket_iam_binding`: Authoritative for a given role. Updates the IAM policy to grant a role to a list of members. Other roles within the IAM policy for the storage bucket are preserved.
 * `google_storage_bucket_iam_member`: Non-authoritative. Updates the IAM policy to grant a role to a new member. Other members for the role for the storage bucket are preserved.
+* `google_storage_bucket_iam_policy`: Authoritative for a given policy. Resets any other permission on the bucket, including default Google permissions that you inherit upon bucket creation. See the usage example on how to handle this.


Could you make this a little bit more cautious? "Setting a policy removes all other permissions on the bucket, and if done incorrectly, there's a real chance you will lock yourself out of the bucket. If possible for your use case, using multiple google_storage_bucket_iam_binding resources will be much safer.", or something like that.

nat-henderson

Looks good, running the tests now. One minor doc tweak I'd like to ask for.

nat-henderson · 2018-03-15T18:09:43Z

Bummer:


------- Stdout: -------
=== RUN   TestAccStorageBucketIamPolicy
--- FAIL: TestAccStorageBucketIamPolicy (0.01s)
    testing.go:513: Step 0 error: Error loading configuration: Error loading /opt/teamcity-agent/temp/buildTmp/tf-test827642825/main.tf: Error reading config for google_storage_bucket_iam_policy[foo]: google_storage_bucket.tf-test-3276351694613004242: resource variables must be three parts: TYPE.NAME.ATTR in:
        
        ${google_storage_bucket.tf-test-3276351694613004242}
FAIL

ReneKroon · 2018-03-16T14:18:11Z

I updated the readme, and i think i found the issue with the test. When running the PR against real google cloud it works, but i have not yet replicated the acceptance test suite environment.

nat-henderson · 2018-03-16T17:49:18Z

I'll give it another run. :)

nat-henderson · 2018-03-16T17:58:22Z

google/resource_storage_bucket_iam_test.go

+}
+
+resource "google_storage_bucket_iam_policy" "fooPolicy" {
+  bucket      = "${google_storage_bucket.%s}"


This is the problem - you'll need to append .name. You can't use the entire resource here, you need to access the right attribute on the resource, which is .name.

nat-henderson · 2018-03-16T17:58:44Z

Sorry, no dice:

------- Stdout: -------
=== RUN   TestAccStorageBucketIamPolicy
--- FAIL: TestAccStorageBucketIamPolicy (0.01s)
    testing.go:513: Step 0 error: Error loading configuration: Error loading /opt/teamcity-agent/temp/buildTmp/tf-test827642825/main.tf: Error reading config for google_storage_bucket_iam_policy[foo]: google_storage_bucket.tf-test-3276351694613004242: resource variables must be three parts: TYPE.NAME.ATTR in:
        
        ${google_storage_bucket.tf-test-3276351694613004242}
FAIL

I commented the problem in a review.

ReneKroon · 2018-03-16T19:34:44Z

i got and fixed it, next time i'll use proper search/replace to go from actual files to templated test versions.

nat-henderson · 2018-03-16T19:57:40Z

google/resource_storage_bucket_iam_test.go

@@ -144,7 +144,7 @@ data "google_iam_policy" "fooPolicy" {
 }

 resource "google_storage_bucket_iam_policy" "foo" {
-  bucket      = "${google_storage_bucket.%s}"
+  bucket      = "${google_storage_bucket.%s.name}"


This still isn't quite right - you just want "${google_storage_bucket.bucket.name}", right? You just want the name of that bucket?

%s here is the name of the bucket (the value of google_storage_bucket.bucket.name), right?

resource "google_storage_bucket" "bolcom-sbx-kroonapp-010-kroon" { name = "bolcom-sbx-kroonapp-010-kroon" project = "bolcom-sbx-kroonapp-010" location = "EUROPE-WEST1" storage_class = "REGIONAL" force_destroy = false labels = { opex = "teamprovisioning" stores_pii_data = "false" } versioning = { enabled = false } } data "google_iam_policy" "kroontest" { binding { role = "roles/storage.objectAdmin" members = [ "user:[email protected]", ] } } resource "google_storage_bucket_iam_policy" "kroontest" { bucket = "${google_storage_bucket.bolcom-sbx-kroonapp-010-kroon.name}" policy_data = "${data.google_iam_policy.kroontest.policy_data}" }

This is the generated terraform file that works for me.

Right, because the bucket resource is called "bolcom-sbx-kroonapp-010-kroon", but that's also its name attribute. In the test, the bucket resource is called "bucket", but its name attribute is a random string. You want to access that random string here. I'm rerunning the tests now.

nat-henderson · 2018-03-16T20:31:17Z

------- Stdout: -------
=== RUN   TestAccStorageBucketIamPolicy
--- FAIL: TestAccStorageBucketIamPolicy (0.00s)
    testing.go:513: Step 0 error: config is invalid: resource 'google_storage_bucket_iam_policy.fooPolicy' config: unknown resource 'google_storage_bucket.tf-test-8494430595842574891' referenced in variable google_storage_bucket.tf-test-8494430595842574891.name
FAIL

The bucket's resource name isn't the same as the bucket name, which is the problem. Sorry!

ReneKroon · 2018-03-29T14:18:29Z

@ndmckinley I have run the integration tests myself. It turns out that removing all policies (i.e. setting an empty iam policy) is non trivial. I found that gsutil is able to do this by just setting the Etag value and i copied this behaviour.

nat-henderson · 2018-04-04T18:26:37Z

I'll run the tests now.

nat-henderson · 2018-04-04T20:13:16Z

Sorry:


------- Stdout: -------
=== RUN   TestAccStorageBucketIamPolicy
--- FAIL: TestAccStorageBucketIamPolicy (2.67s)
    testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
        
        * google_storage_bucket_iam_policy.bucketBinding: 1 error(s) occurred:
        
        * google_storage_bucket_iam_policy.bucketBinding: Error retrieving IAM policy for Storage Bucket "tf-test-5427762820494880885": googleapi: Error 403: terraform-acceptance-tests@hc-terraform-testing.iam.gserviceaccount.com does not have storage.buckets.getIamPolicy access to tf-test-5427762820494880885., forbidden
    testing.go:573: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Error refreshing: 1 error(s) occurred:
        
        * google_storage_bucket.bucket: 1 error(s) occurred:
        
        * google_storage_bucket.bucket: google_storage_bucket.bucket: Error reading Storage Bucket "tf-test-5427762820494880885": googleapi: Error 403: terraform-acceptance-tests@hc-terraform-testing.iam.gserviceaccount.com does not have storage.buckets.get access to tf-test-5427762820494880885., forbidden
        
        State: <nil>
FAIL

If I'm reading that right, I think you can configure it when you create the bucket.

ReneKroon · 2018-04-05T12:03:30Z

@ndmckinley I ran the test in a separate cloud, so i ran under 'Owner' permissions. What user can i expect for these integration tests? It probably means i have to set up roles and permissions for the service account user first which makes the test config a dependant on other parts of the application as well.

nat-henderson · 2018-04-17T00:17:29Z

Ah! Sorry, this completely fell off my radar and I didn't find it again until I did a sweep of open PRs. Sorry about that! @paddycarver might be able to comment on the permission setup in our CI system and how you can make sure that the user has permission to read from the buckets. I'll look into it myself as well and try to get back to you with that answer.

nat-henderson · 2018-04-17T00:22:04Z

Yeah, if I'm reading this right, you can probably just add that service account terraform-acceptance-tests@hc-terraform-testing.iam.gserviceaccount.com into the policy as an owner, and solve this issue in a quick hacky way ... let me look into whether there's something less process-dependent you could do.

paddycarver · 2018-04-17T02:51:53Z

I'm not following here, sorry--why should a contributor have to add our service account to anything? Our service account should have all the permissions it needs when running tests against our CI environment. If it doesn't, I would say that's a problem with the CI setup, not with the PR, and if we think it's a permissions problem, I can investigate the CI setup and make sure things are right.

nat-henderson · 2018-04-17T18:22:04Z

I think I'm a little confused myself, but let's see if I understand: Since the new test applies an entire IAM policy to the bucket, it removes the service account's access to that bucket (since the service account is not in the policy). To work around that so that the test can pass, we need to add the service account to the policy in the test? That's how I understand it, or my theory at least.

ReneKroon · 2018-04-17T19:13:03Z

i run with the acceptance with the following environment variables set:

GOOGLE_PROJECT=lustrous-fabric-165106
GOOGLE_REGION=us-central1
GOOGLE_USE_DEFAULT_CREDENTIALS=1
TF_ACC=1
TF_LOG=TRACE
TF_LOG_PATH=/tmp/tf.log
TF_LOG_PATH_MASK=/tmp/%s

You are right that there it's non standard to delete the bucket which has it's policy removed. However it's not impossible, but i can imagine that my use of GOOGLE_USE_DEFAULT_CREDENTIALS which makes me owner on the project has some effect.

paddycarver · 2018-04-17T21:19:56Z

Could we just use an environment variable to pass in the service account/email address running the tests, and then inject that into the policy with the appropriate rights? That would sidestep the issue, I believe?

nat-henderson · 2018-04-17T21:28:05Z

That's a good idea, thanks. @ReneKroon - sorry to keep asking for more here. I'm happy to add that if you'd prefer not to, you've definitely spent enough time on this already, just let me know.

ReneKroon · 2018-04-19T10:15:44Z

@ndmckinley Well i've learned at least to manage the full development cycle with this change :-) I would like to ask you to make this CI related adjustment. It will be a lot of roundtrips again if something would not work right away, since i have no insight into your CI infrastructure.

nat-henderson · 2018-05-07T23:30:43Z

Okay, there we go. @ReneKroon please be aware! To continue with this PR I had to push to your master branch because that's the branch you used to open the PR. Because of some merge conflicts, I had to merge master into the PR ... so your master branch just jumped by a couple versions. If you need your old master branch back, you should be able to do this:

git checkout d96138b
git branch -f master
git push origin master -f

Of course that'll screw up this PR, but I still have all this stuff on my fork, so you don't need to worry about that - I'll just open a new one if you need that.

nat-henderson · 2018-05-08T00:29:25Z

Test passed, but since my code's in here, I probably ought to get another review on that stuff. Hang on, I'll get this merged yet. :)

danawillow · 2018-05-08T17:41:54Z

website/docs/r/storage_bucket_iam.html.markdown

+```hcl
+resource "google_storage_bucket_iam_policy" "member" {
+  bucket = "your-bucket-name"
+  policy_data = "policy_data"


Looks like a string snuck in here- I think you meant to have this refer to a policy data source.

danawillow · 2018-05-08T17:42:37Z

google/resource_storage_bucket_iam_test.go

+    ]
+  }
+
+	binding {


nit: tabs/spaces consistency

danawillow · 2018-05-08T17:43:24Z

google/resource_storage_bucket_iam_test.go

+}
+
+
+data "google_iam_policy" "fooPolicy" {


nit: let's aim for consistency between the naming case for the resources, so this should be foo-policy and then below would be bucket-binding

nat-henderson · 2018-05-08T21:00:40Z

Storage bucket tests all pass.

ghost · 2018-11-18T14:09:51Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

nat-henderson self-requested a review March 14, 2018 19:29

nat-henderson mentioned this pull request Mar 14, 2018

add a google_organization_iam_policy resource #1128

Closed

nat-henderson reviewed Mar 15, 2018

View reviewed changes

nat-henderson approved these changes Mar 15, 2018

View reviewed changes

nat-henderson reviewed Mar 16, 2018

View reviewed changes

ReneKroon force-pushed the master branch from 4b0179e to dad108b Compare March 29, 2018 13:49

vancluever added enhancement new-resource labels Mar 29, 2018

hashicorp#843: Add policy support to storage buckets

d96138b

ReneKroon force-pushed the master branch from e34326f to d96138b Compare April 3, 2018 08:35

nat-henderson added 2 commits May 7, 2018 23:25

Modify test to pass on our CI system.

11e9933

Merge branch 'master' into policy-support

ed84f0f

Fix bad merge.

314c393

nat-henderson requested a review from danawillow May 8, 2018 00:29

danawillow reviewed May 8, 2018

View reviewed changes

nat-henderson added 3 commits May 8, 2018 18:29

Whitespace fixes.

1f3e970

Doc update.

194c7f4

Another consistency fix.

a9d12f3

danawillow approved these changes May 8, 2018

View reviewed changes

nat-henderson merged commit dda2677 into hashicorp:master May 8, 2018

paddycarver mentioned this pull request Jun 22, 2018

Add Bucket IAM policy resource #843

Closed

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

hashicorp#843: Add policy support to storage buckets (hashicorp#1190)

3beda2c

ghost locked and limited conversation to collaborators Nov 18, 2018

#843: Add policy support to storage buckets #1190

#843: Add policy support to storage buckets #1190

Conversation

ReneKroon commented Mar 14, 2018

nat-henderson commented Mar 14, 2018

nat-henderson commented Mar 14, 2018

ReneKroon commented Mar 15, 2018

nat-henderson Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

nat-henderson left a comment

Choose a reason for hiding this comment

nat-henderson commented Mar 15, 2018

ReneKroon commented Mar 16, 2018

nat-henderson commented Mar 16, 2018

Choose a reason for hiding this comment

nat-henderson commented Mar 16, 2018

ReneKroon commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nat-henderson commented Mar 16, 2018

ReneKroon commented Mar 29, 2018

nat-henderson commented Apr 4, 2018

nat-henderson commented Apr 4, 2018

ReneKroon commented Apr 5, 2018

nat-henderson commented Apr 17, 2018

nat-henderson commented Apr 17, 2018

paddycarver commented Apr 17, 2018

nat-henderson commented Apr 17, 2018

ReneKroon commented Apr 17, 2018

paddycarver commented Apr 17, 2018

nat-henderson commented Apr 17, 2018

ReneKroon commented Apr 19, 2018

nat-henderson commented May 7, 2018

nat-henderson commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nat-henderson commented May 8, 2018

ghost commented Nov 18, 2018

nat-henderson Mar 15, 2018 •

edited

Loading