Init Storage IAM policy #493

sebglon · 2017-09-29T10:18:40Z

refer to issue #481

sebglon · 2017-09-30T20:59:09Z

Tested OK for Bucket Iam Policy

sebglon · 2017-10-10T08:20:08Z

make testacc TEST=./google TESTARGS='-run=TestAccGoogleStorageObjectIAMPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleStorageObjectIAMPolicy -timeout 120m
=== RUN   TestAccGoogleStorageObjectIAMPolicy_basic
--- SKIP: TestAccGoogleStorageObjectIAMPolicy_basic (0.00s)
	resource_google_project_test.go:432: Environment variable GOOGLE_PROJECT_NUMBER is not set
=== RUN   TestAccGoogleStorageObjectIAMPolicy_upgrade
--- SKIP: TestAccGoogleStorageObjectIAMPolicy_upgrade (0.00s)
	resource_google_project_test.go:432: Environment variable GOOGLE_PROJECT_NUMBER is not set
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	0.009s

paddycarver

Hey @sebglon, this looks fantastic, thanks for the PR! I have some minor, nitpicky comments, and it looks like we're missing documentation for object policy, but other than that, this looks amazing. Thanks so much for the hard work. Do let me know if you have any questions.

paddycarver · 2017-10-26T21:24:57Z

google/resource_storage_bucket_iampolicy.go

@@ -0,0 +1,122 @@
+package google


This is definitely a nitpick, but I'd love to see a _ between iam and policy in the filename here.

paddycarver · 2017-10-26T21:26:30Z

google/resource_storage_bucket_iampolicy.go

+func resourceStorageBucketIAMPolicyCreate(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+
+	if err := setBucketIamPolicy(d, config); err != nil {


We usually like to do a log.Printf("[DEBUG] Setting IAM policy for bucket %q", bucket) before we make an API call, and a log.Printf("[DEBUG] Set IAM policy for bucket %q", bucket) after the request completes, to help debug.

paddycarver · 2017-10-26T21:30:17Z

google/resource_storage_bucket_iampolicy.go

+}
+
+func marshalStorageIamPolicy(policy *storagev1.Policy) string {
+	pdBytes, _ := json.Marshal(&storagev1.Policy{


I know it seems impossible, but I'd really be much more comfortable if we checked this error here.

paddycarver · 2017-10-26T21:31:32Z

google/resource_storage_bucket_imapolicy_test.go

+)
+
+func testBucketIamName() string {
+	return fmt.Sprintf("%s-%d", "tf-test-iam-bucket", acctest.RandInt())


You can just use acctest.RandomWithPrefix("tf-test-iam-bucket") which is a little less convoluted.

paddycarver · 2017-10-26T21:34:17Z

google/resource_storage_object_iampolicy.go

+
+func resourceStorageObjectIAMPolicyCreate(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+


Again, debug logs here would be awesome.

Actually, just before and after every API request would be 💯

paddycarver · 2017-10-26T21:35:30Z

google/resource_storage_object_iampolicy.go

+		return err
+	}
+
+	d.SetId(d.Get("object").(string))


Shouldn't this be a combination of bucket and object, to ensure uniqueness? Also, for future import, we'd need at least project, bucket, and object, I think. So I think setting it to a combination of the three would be really useful.

paddycarver · 2017-10-26T21:35:58Z

google/resource_storage_bucket_iampolicy.go

+		return err
+	}
+
+	d.SetId(d.Get("bucket").(string))


If we set this to a combination of project and bucket, we won't need to change IDs to support import.

I don't understand exactly what you want. Actualy i do not have project var.
As seen in resource_storage_bucket_acl, ican set bucket+ "-iam_policy"?

So what I'm suggesting here and why:

When we make resources importable, we need to be able to identify the resources based on their ID. This means the ID needs to not only be unique, but contain all the information necessary to retrieve the resource.

For this resource, it looks like we need both the project and the bucket ID to be able to retrieve the bucket.

So ideally, our ID would be something like {project}/{bucket}, which we could then parse into the info we need to be able to retrieve the resource given just its ID.

On google Policy, i have etag parameter that is an entity ID (return by API). but this does not contain project.
I don't see how to make it.

paddycarver · 2017-10-26T21:38:27Z

vendor/vendor.json

@@ -852,10 +852,10 @@
 			"revisionTime": "2016-12-14T09:25:55Z"
 		},
 		{
-			"checksumSHA1": "FEzQdhqmb6aqGL1lKnjOcUHIGSY=",
+			"checksumSHA1": "F/N7JzfuHZAk1UXi2zw/62hFPpM=",
 			"path": "google.golang.org/api/bigquery/v2",


Seems like we're bringing in a lot of unnecessary vendor changes for this PR. Could we clean this up to just update storage, or is there a reason it needs to bring bigquery along?

(Also, is a storage library update necessary to support this functionality?)

Yes an update is necessary and this update bring bigquery lib with small change type

sebglon · 2017-11-02T20:22:50Z

@paddycarver I have cleaned dependencies with master change

sebglon · 2017-11-03T11:11:28Z

@danawillow and @paddycarver is itpossible to have status on this PR?

paddycarver

Sorry for the delay on this. Some new comments, but we're pretty close on this one. Thanks for all the great work!

paddycarver · 2017-11-03T20:30:14Z

google/resource_storage_bucket_iam_policy_test.go

+		for _, member := range binding.Members {
+			bindingBuffer.WriteString(fmt.Sprintf("\"%s\",\n", member))
+		}
+		bindingBuffer.WriteString("]}\n")


This part of the code scares me, to be honest.

I don't understand Why?

this is the same than resource_google_folder_iam_policy_test.go;

I have added Comment on source code

Apologies, missed this response. It largely scares me because we're dynamically building the config string in a buffer, which is more complicated than tests usually require. I think this may be more generalized than is strictly necessary, but it's really not worth holding a merge over.

paddycarver · 2017-11-03T20:33:04Z

google/resource_storage_object_iam_policy.go

+		return err
+	}
+
+	d.SetId(d.Get("bucket").(string) + "-" + d.Get("object").(string) + "-iam-policy")


If we could use a separator here that is an invalid character for a bucket name, that'd be awesome. (/ is a good choice, I believe.)

The idea is that we can then parse the ID to get the bucket and object back, which helps in a few places. As it stands, there's no real way to reliably turn this ID back into its constituent parts. In my-bucket-object-id-iam-policy, for example, we'd have no idea what was the bucket and what was the object.

paddycarver · 2017-11-03T20:40:40Z

google/resource_storage_object_iam_policy_test.go

+  %s
+}
+resource "google_storage_Object_iam_policy" "test" {
+  Object = "${google_storage_Object.Object.name}"


Looks like a find and replace is doing some weird capitalising here.

paddycarver · 2017-11-03T20:41:04Z

google/resource_storage_object_iam_policy_test.go

+)
+
+func testObjectIamName() string {
+	return fmt.Sprintf("%s-%d", "tf-test-iam-Object", acctest.RandInt())


Looks like a find and replace is doing some weird capitalising here.

paddycarver · 2017-11-03T20:41:58Z

google/resource_storage_object_iam_policy_test.go

+	config := testAccProvider.Meta().(*Config)
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type != "google_storage_Object_iam_policy" {


Looks like a find and replace is doing some weird capitalising here.

paddycarver · 2017-11-03T20:43:38Z

website/docs/r/storage_bucket_iam_policy.html.markdown

@@ -0,0 +1,58 @@
+---


Looks like you've duplicated the bucket docs here?

Looks like you have both a google_storage_bucket_iam_policy.html.markdown and a nearly identical storage_bucket_iam_policy.html.markdown file (without the "google" prefix). I assume that's what @paddycarver is referring to.

Yup, sorry. I'm seeing two different files here with almost the same name and can't tell what happened.

FYI, @sebglon deleted the duplicate file. I think the only outstanding issue left is the "this part of the code scares me" section, if you can expand on that?

paddycarver · 2017-11-03T20:44:07Z

website/docs/r/storage_object_iam_policy.html.markdown

+ Allows management of an IAM policy for a Google Storage Bucket Object.
+---
+
+# google\_storage\-_object\_iam\_policy


Looks like you have an extra - in here that's messing things up.

paddycarver · 2017-11-03T20:44:46Z

website/docs/r/storage_object_iam_policy.html.markdown

+
+resource "google_storage_object" "object" {
+  display_name = "%s"
+	bucket = "${google_storage_bucket.bucket.name}"


Tabs/spaces problem. Generally we ask that documentation examples be run through terraform fmt.

paddycarver · 2017-11-03T20:46:30Z

website/docs/r/storage_object_iam_policy.html.markdown

+
+    Changing this updates the policy.
+
+    Deleting this removes the policy, but leaves the original object policy


Is this true? Looking up at the code, it looks like the policy is just set to an empty policy on delete.

paddycarver · 2017-11-08T19:28:18Z

It looks like after 580b153, the code no longer builds.

sebglon · 2017-11-13T07:56:21Z

@paddycarver Compile is corrected.

sebglon · 2017-12-05T14:21:58Z

@danawillow , @paddycarver Can i have status on validation; this lock my project?

tragiclifestories · 2017-12-06T11:19:32Z

Cc @rosbo who has been working on the IAM system

rosbo · 2017-12-06T17:51:10Z

In #744 we refactored the IAM code to make it easier to write new IAM resources. #822 adds IAM support for storage bucket using the new architecture.

Closing this one in favor #822.

@rileykarson

/cc @rileykarson

ghost · 2020-03-30T14:07:40Z

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!

danawillow requested a review from paddycarver October 3, 2017 17:46

radeksimko added the new-resource label Oct 8, 2017

sebglon force-pushed the storage-iam-policy branch 6 times, most recently from d9413e2 to 7e436a5 Compare October 16, 2017 08:00

paddycarver suggested changes Oct 26, 2017

View reviewed changes

sebglon force-pushed the storage-iam-policy branch from 2eb1c30 to 597e537 Compare October 27, 2017 07:30

Sébastien GLON added 7 commits November 2, 2017 20:48

Init Storage IAM policy

4ed5ee3

remove unused func

a7a8730

Add object iam policy

f991696

allow disabled KubernetesDashboard

37dc607

Add website doc for resource storage_bucket_iam_policy

78b5839

Correct PR

538df50

Add docs

e3c7096

sebglon force-pushed the storage-iam-policy branch from 3844afc to e3c7096 Compare November 2, 2017 19:59

Sébastien GLON added 2 commits November 2, 2017 21:18

clean

3e577f0

clean

616dbda

paddycarver suggested changes Nov 3, 2017

View reviewed changes

Sébastien GLON added 3 commits November 5, 2017 21:38

correct PR

fa1d298

correct pr

44b4eb9

remove duplicate

1487793

paddycarver added the waiting-response label Nov 7, 2017

Add comment on test

580b153

correct

09007e7

This was referenced Dec 6, 2017

Add IAM support for storage bucket #822

Merged

Add IAM support for storage bucket #481

Closed

rosbo closed this Dec 6, 2017

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Add a datasource for retrieving a list of projects (hashicorp#493)

065adad

 /cc @rileykarson

ghost locked and limited conversation to collaborators Mar 30, 2020


		func resourceStorageObjectIAMPolicyCreate(d *schema.ResourceData, meta interface{}) error {
		config := meta.(*Config)


		Changing this updates the policy.

		Deleting this removes the policy, but leaves the original object policy

Init Storage IAM policy #493

Init Storage IAM policy #493

Conversation

sebglon commented Sep 29, 2017 • edited Loading

sebglon commented Sep 30, 2017

sebglon commented Oct 10, 2017

paddycarver left a 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

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

sebglon Oct 28, 2017 • edited Loading

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

sebglon commented Nov 2, 2017

sebglon commented Nov 3, 2017

paddycarver left a 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

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

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

paddycarver commented Nov 8, 2017

sebglon commented Nov 13, 2017

sebglon commented Dec 5, 2017

tragiclifestories commented Dec 6, 2017

rosbo commented Dec 6, 2017

ghost commented Mar 30, 2020

sebglon commented Sep 29, 2017 •

edited

Loading

sebglon Oct 28, 2017 •

edited

Loading