Add google_project_iam_binding and google_project_iam_member resources. #171

paddycarver · 2017-07-04T02:16:09Z

Add a resource that manages just a single binding within a Google
project's IAM Policy.

Add a resource that manages just a single member of a single binding
within a Google project's IAM Policy.

Note that these resources should not be used when
google_project_iam_policy is used, or they will fight over which is
correct. They should also not be assigned the same role within the
same project, or they will fight with each other.

This also required wrapping the error returned from setProjectIamPolicy,
as we need to test to see if it's a 409 error and retry, which can't be
done if we just use fmt.Errorf.

The code that is here works, and the tests pass. I still need to add a google_project_iam_member resource that controls IAM at the member level, which is the full solution to this problem. That will be pushed to this PR shortly. This is now complete and ready for review.

This should resolve #72 and #58.

Add a resource that manages just a single binding within a Google project's IAM Policy. Note that this resource should not be used when google_project_iam_policy is used, or they will fight over which is correct. This also required wrapping the error returned from setProjectIamPolicy, as we need to test to see if it's a 409 error and retry, which can't be done if we just use fmt.Errorf.

Add docs to the website for google_project_iam_binding.

Changing the role is ForceNew, because the role is part of the ID. Make reads go through to the Binding functions, not the Policy functions. That's embarrassing.

It shouldn't be an error, we should just remove that binding from state.

Add the missing parameter to Errorf statements.

Adds the google_project_iam_member resource, which just ensures that a single member has a single role. google_project_iam_member should not be used to grant permissions to a role controlled by google_project_iam_binding or to a policy controlled by google_project_iam_policy, as they'll fight for control.

Fix a sentence in the binding docs, and add docs for members.

danawillow · 2017-07-05T18:02:58Z

google/resource_google_project_iam_binding.go

+		} else if err != nil {
+			return fmt.Errorf("Error applying IAM policy to project: %v", err)
+		}
+		break


Totally up to you, but I think this structure would be a bit more clear:

for { // stuff if err == nil { // yay, successful update break } else if isConflictError(err) { // stuff continue } else { return fmt.Errorf(stuff) }

danawillow · 2017-07-05T18:08:40Z

google/resource_google_project_iam_binding.go

+					Type: schema.TypeString,
+				},
+			},
+			"etag": &schema.Schema{


Since we're doing a read-modify-write, do we actually need to store the etag?

The code doesn't use it for anything, but I couldn't think of a good reason not to expose it.

danawillow · 2017-07-05T18:28:59Z

google/resource_google_project_iam_binding.go

+			break
+		}
+		if toRemove < 0 {
+			return resourceGoogleProjectIamBindingRead(d, meta)


Is there a reason we need to do another read here, or can we just set the id to "" and be done?

danawillow · 2017-07-05T18:32:57Z

google/resource_google_project_iam_binding.go

+
+		p.Bindings = append(p.Bindings[:toRemove], p.Bindings[toRemove+1:]...)
+
+		log.Printf("[DEBUG]: Setting policy for project %q to %+v\n", pid, p)


Setting the policy seems to be done exactly the same in create, update, and delete- any way we could have a separate function that gets called by all 3?

danawillow · 2017-07-05T19:44:37Z

google/resource_google_project_iam_binding.go

+// Get a cloudresourcemanager.Binding from a schema.ResourceData
+func getResourceIamBinding(d *schema.ResourceData) *cloudresourcemanager.Binding {
+	members := d.Get("members").(*schema.Set).List()
+	m := make([]string, 0, len(members))


if you want you can do m := convertStringArr(members) (the fn is in a weird place right now but it's getting moved to provider.go to make it easier to find in #152

danawillow · 2017-07-05T19:51:27Z

google/resource_google_project_iam_binding_test.go

+func testAccGoogleProjectAssociateBindingBasic(pid, name, org string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+    project_id = "%s"


Mind lining up your =s?

danawillow · 2017-07-05T19:51:49Z

google/resource_google_project_iam_binding_test.go

+    org_id = "%s"
+}
+resource "google_project_iam_binding" "acceptance" {
+    project = "${google_project.acceptance.id}"


why id and not project_id?

Force of habit? I dunno. I'll change to project_id to be more explicit. :)

danawillow · 2017-07-05T19:56:04Z

google/resource_google_project_iam_member.go

+		}
+	}
+	d.Set("etag", p.Etag)
+	d.Set("member", member)


What if there is a binding server-side but not with the member we expect?

danawillow · 2017-07-05T19:57:46Z

google/resource_google_project_iam_member.go

+				Required: true,
+				ForceNew: true,
+			},
+			"etag": &schema.Schema{


Same comments from iam binding apply here as well

kkallday · 2017-07-06T01:33:06Z

cc @davewalter @rainmaker @rizwanreza @acrmp @joshzarrabi @madamkiwi @ryanmoran

Rewrite the structure of our if statement to make the conditions under which each portion executes a bit more clear.

We can just set the ID of bindings that are scheduled to be deleted but don't exist in the API, we don't need an entire separate read request.

We were repeating that logic a lot, so this helper just reads a policy, calls the passed modify function on the policy, then writes the policy back and takes care of the optimistic concurrency logic for the caller. So now all the caller has to do is the unique part, which is the modify function.

Helps keep things explicit.

Use the new projectIamPolicyReadModifyWrite helper to manage the RMW loop for our policy member resource. Handle the case of having a binding server-side that doesn't have the member we expect more elegantly.

For clarity and explicitness(? is that a word?) use the project_id instead of Terraform's internal id in our test configs.

paddycarver · 2017-07-25T19:19:41Z

Think I addressed all your comments @danawillow, thanks for the thorough review. Let me know how the updated code looks. :)

grubernaut · 2017-07-26T14:52:56Z

google/resource_google_project_iam_binding.go

+		Delete: resourceGoogleProjectIamBindingDelete,
+
+		Schema: map[string]*schema.Schema{
+			"project": &schema.Schema{


super minor nit: can remove the duplicate type declarations here, and the rest of the schema struct:

"project": {

grubernaut · 2017-07-26T14:53:23Z

google/resource_google_project_iam_binding_test.go

+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			// Create a new project
+			resource.TestStep{


Same with the duplicate type declarations here

danawillow · 2017-07-26T20:01:11Z

google/resource_google_project_iam_binding_test.go

+					}, pid),
+				),
+			},
+			// Apply an updated IAM binding


Just for completeness, I wouldn't mind another step in here that removes a single member from the binding

danawillow · 2017-07-26T20:02:43Z

google/resource_google_project_iam_binding_test.go

+				),
+			},
+			// Apply an IAM binding
+			resource.TestStep{


do you need this test step in here? I wonder if this test would actually give us more signal if it jumped from one binding to two (plus, the previous test essentially does the first two teststeps of this one)

I'm confused as to what you're suggesting, I'm sorry. The first step applies Binding A. The second step then adds Binding B. So this is testing that, once a binding already exists, you can add another binding. Which seems to be what the second sentence of your comment is asking for, so I think I'm misunderstanding somewhere.

Oh whoops my second sentence was meant to say from zero to two. I'm fine with leaving the test as it is though.

I'll just add another test, no big deal. We should totally test that. :)

danawillow · 2017-07-26T20:07:31Z

google/resource_google_project_iam_binding.go

+	if err != nil {
+		return err
+	}
+	d.SetId(pid + ":" + p.Role)


A lot of the resources we've been adding import to lately have settled on "/" as a separator for ids. Maybe do that for consistency?

danawillow · 2017-07-26T20:08:20Z

google/resource_google_project_iam_member.go

+		break
+	}
+	if binding == nil {
+		d.SetId("")


A log message here would probably be useful.

danawillow · 2017-07-26T20:08:33Z

google/resource_google_project_iam_member.go

+		}
+	}
+	if member == "" {
+		d.SetId("")


danawillow · 2017-07-26T20:12:47Z

google/resource_google_project_iam_binding.go

+		return err
+	}
+
+	return resourceGoogleProjectIamBindingRead(d, meta)


What's the purpose of the final read?

Updates the state file with the state according to the API. In this case, confirms the API no longer has the binding in it, and removes it. If for whatever reason the delete function silently failed and didn't actually delete the binding, Terraform wouldn't lose it.

danawillow · 2017-07-26T20:12:55Z

google/resource_google_project_iam_binding.go

+		}
+
+		p.Bindings = append(p.Bindings[:toRemove], p.Bindings[toRemove+1:]...)
+		return nil


Do you need to set the id to "" here too?

The read call does that. :)

danawillow · 2017-07-26T20:13:35Z

google/resource_google_project_iam_member.go

+			break
+		}
+		if bindingToRemove < 0 {
+			return nil


Same comments from the other file around setting the id and doing the final read

Because this happens in the read-modify-write helper function, we don't need to call SetId, because when it returns nil, it just returns back up to the Delete function. The Delete calls read, which will verify the member does not actually exist anymore, and then removes it from state.

danawillow · 2017-07-26T20:19:02Z

website/docs/r/google_project_iam_binding.html.markdown

+    `google_project_iam_binding` can be used per role.
+
+* `project` - (Optional) The project ID. If not specified, uses the
+    ID of the project configured with the provider.## Attributes Reference


Missing newlines

danawillow · 2017-07-26T20:19:17Z

website/docs/r/google_project_iam_member.html.markdown

+* `role` - (Required) The role that should be applied.
+
+* `project` - (Optional) The project ID. If not specified, uses the
+    ID of the project configured with the provider.## Attributes Reference


Some struct types can be inferred instead of being repeated, so let's take advantage of that.

Add a test case that updates a binding to remove a member.

Switch to using / as a separator for IDs, instead of :.

Update member IDs to use / instead of :. Make sure we're logging any time we remove something from state.

Tests need to have unique names. Whoooops. Also, the Elem property accepts an interface I guess, which means we actually need the struct type repetition there.

We don't need to set the ID to "" in read-modify-write helpers, because once they're done, we read anyways to update state based on the changes. And that read checks if the binding/member still exists, and does the SetId("") if it doesn't. This way, we stick with state only getting set based on the API state, not by what we think the state will be.

That's embarrassing. How did I miss that?

paddycarver · 2017-07-27T21:44:57Z

Latest push should address all comments. :)

danawillow

I'm super excited about these changes, thanks @paddycarver! Merge at will.

Add google_project_iam_binding and google_project_iam_member resources.

ghost · 2018-11-16T13:54:00Z

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!

paddycarver added 2 commits July 3, 2017 19:01

Update website docs for google_project_iam_binding.

eed8487

Add docs to the website for google_project_iam_binding.

paddycarver added the new-resource label Jul 4, 2017

paddycarver requested a review from danawillow July 4, 2017 02:16

paddycarver added 5 commits July 3, 2017 19:36

Fix reads, make ForceNew.

11abe24

Changing the role is ForceNew, because the role is part of the ID. Make reads go through to the Binding functions, not the Policy functions. That's embarrassing.

Remove ID when binding isn't found.

65a4a36

It shouldn't be an error, we should just remove that binding from state.

Fix vet errors.

a84b22d

Add the missing parameter to Errorf statements.

Documentation updates.

8a880fd

Fix a sentence in the binding docs, and add docs for members.

paddycarver changed the title ~~Add google_project_iam_binding resource.~~ Add google_project_iam_binding and google_project_iam_member resources. Jul 4, 2017

danawillow suggested changes Jul 5, 2017

View reviewed changes

danawillow mentioned this pull request Jul 20, 2017

IAM policy for container-engine-robot.iam.gserviceaccount.com appears out of the void #214

Closed

paddycarver added 9 commits July 25, 2017 11:08

Refactor binding update loop for clarity.

d3f901b

Rewrite the structure of our if statement to make the conditions under which each portion executes a bit more clear.

Just remove deleted bindings not present in the API.

9c1c0bb

We can just set the ID of bindings that are scheduled to be deleted but don't exist in the API, we don't need an entire separate read request.

Use string slice conversion helper.

5282ad7

terraform fmt test configs.

f9eeb36

id => project_id in test configs.

52d552d

Helps keep things explicit.

Use the policy r/m/w helper and handle edge case.

b0e3790

Use the new projectIamPolicyReadModifyWrite helper to manage the RMW loop for our policy member resource. Handle the case of having a binding server-side that doesn't have the member we expect more elegantly.

id => project_id

91d227c

For clarity and explicitness(? is that a word?) use the project_id instead of Terraform's internal id in our test configs.

Terraform fmt on our test configs.

655435f

Terraform fmt website examples.

fa2d54f

paddycarver mentioned this pull request Jul 25, 2017

WIP: providers/google: restore_policy supports update hashicorp/terraform#11737

Closed

grubernaut reviewed Jul 26, 2017

View reviewed changes

danawillow suggested changes Jul 26, 2017

View reviewed changes

paddycarver added 2 commits July 27, 2017 13:39

Excise unnecessary type declarations.

199ff5d

Some struct types can be inferred instead of being repeated, so let's take advantage of that.

Add test case for updating to remove member.

adc206a

Add a test case that updates a binding to remove a member.

paddycarver added 8 commits July 27, 2017 13:50

Switch to / as separator.

f94c387

Switch to using / as a separator for IDs, instead of :.

Add logging statements, update : to / in IDs.

7854535

Update member IDs to use / instead of :. Make sure we're logging any time we remove something from state.

Test adding multiple bindings at once.

f88e042

Return a type that was needed, rename a test function.

ac5df40

Tests need to have unique names. Whoooops. Also, the Elem property accepts an interface I guess, which means we actually need the struct type repetition there.

Fix embarrassing typo in log message.

6f98217

Newlines on website source.

4b9432d

Newlines, but really this time.

c89429c

That's embarrassing. How did I miss that?

danawillow approved these changes Jul 28, 2017

View reviewed changes

paddycarver merged commit 37df582 into master Jul 28, 2017

paddycarver mentioned this pull request Jul 28, 2017

Potential Race Condition When Apply Google Project IAM Policy #58

Closed

danawillow mentioned this pull request Aug 8, 2017

Improve container engine / node pool handling #285

Closed

paddycarver mentioned this pull request Aug 10, 2017

fixes issue where IAM policies get deleted due to race condition hashicorp/terraform#13439

Closed

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Merge pull request hashicorp#171 from terraform-providers/paddy_iam

2cebbef

Add google_project_iam_binding and google_project_iam_member resources.

paultyng deleted the paddy_iam branch October 29, 2018 19:29

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

Merge pull request hashicorp#171 from terraform-providers/paddy_iam

7b463c1

Add google_project_iam_binding and google_project_iam_member resources.

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add google_project_iam_binding and google_project_iam_member resources. #171

Add google_project_iam_binding and google_project_iam_member resources. #171

paddycarver commented Jul 4, 2017 •

edited

Loading

danawillow Jul 5, 2017

danawillow Jul 5, 2017

paddycarver Jul 25, 2017

danawillow Jul 5, 2017

danawillow Jul 5, 2017

danawillow Jul 5, 2017

danawillow Jul 5, 2017

danawillow Jul 5, 2017

paddycarver Jul 25, 2017

danawillow Jul 5, 2017

danawillow Jul 5, 2017

kkallday commented Jul 6, 2017

paddycarver commented Jul 25, 2017

grubernaut Jul 26, 2017

grubernaut Jul 26, 2017

danawillow Jul 26, 2017

danawillow Jul 26, 2017

paddycarver Jul 27, 2017

danawillow Jul 27, 2017

paddycarver Jul 27, 2017

danawillow Jul 26, 2017

danawillow Jul 26, 2017

danawillow Jul 26, 2017

danawillow Jul 26, 2017

paddycarver Jul 27, 2017

danawillow Jul 26, 2017

paddycarver Jul 27, 2017

danawillow Jul 26, 2017

paddycarver Jul 27, 2017

danawillow Jul 26, 2017

danawillow Jul 26, 2017

paddycarver commented Jul 27, 2017

danawillow left a comment

ghost commented Nov 16, 2018


		p.Bindings = append(p.Bindings[:toRemove], p.Bindings[toRemove+1:]...)

		log.Printf("[DEBUG]: Setting policy for project %q to %+v\n", pid, p)

Add google_project_iam_binding and google_project_iam_member resources. #171

Add google_project_iam_binding and google_project_iam_member resources. #171

Conversation

paddycarver commented Jul 4, 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

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

kkallday commented Jul 6, 2017

paddycarver commented Jul 25, 2017

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

Choose a reason for hiding this comment

paddycarver commented Jul 27, 2017

danawillow left a comment

Choose a reason for hiding this comment

ghost commented Nov 16, 2018

paddycarver commented Jul 4, 2017 •

edited

Loading