providers/google: Create and delete Service Accounts #9946

evandbrown · 2016-11-08T07:28:49Z

This change adds support for the google_service_account resource. The google_service_account allows a Terraform user to create, modify, and delete Service Accounts on Google Cloud Platform.

evandbrown · 2016-11-08T07:29:39Z

Ping @danawillow for super-early initial review. Tomorrow I'll add docs and support for IAM policy attachment.

cblecker · 2016-11-08T20:37:33Z

This is awesome! I actually went looking to see if Terraform could do this just the other day.

danawillow

Looks fine overall, just a few small things.

danawillow · 2016-11-08T21:28:31Z

builtin/providers/google/resource_google_service_account.go

+	sa, err := config.clientIAM.Projects.ServiceAccounts.Get(d.Id()).Do()
+	if err != nil {
+		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 {
+			log.Printf("[WARN] Removing service account %q because it no longer exists", d.Id())


I feel like if I saw this error message when running a command I would have no idea what it meant. Maybe something like "resetting reference to" instead of "removing"? Not sure I love that either honestly. What exactly is happening here?

danawillow · 2016-11-08T21:29:34Z

builtin/providers/google/resource_google_service_account.go

+			return fmt.Errorf("Error getting service account with name %q: %s", d.Id(), err)
+		}
+
+		return fmt.Errorf("Error reading service account: %q", err)


Why does the above error message reference the name and not this one?

danawillow · 2016-11-08T21:32:27Z

builtin/providers/google/resource_google_service_account_test.go

+var testAccGoogleServiceAccount_basic = `
+resource "google_service_account" "acceptance" {
+    account_id = "%v"
+	display_name = "%v"


nit: alignment

cblecker · 2016-11-10T08:08:21Z

builtin/providers/google/resource_google_service_account_test.go

+
+var testAccGoogleServiceAccount_basic = `
+resource "google_service_account" "acceptance" {
+    account_id = "%v"


Instead of passing static variables, would it not be preferable to have a test that passes a random character string, similar to the other google provider tests (example:

terraform/builtin/providers/google/resource_compute_route_test.go

Lines 104 to 110 in 45f441f

resource "google_compute_route" "foobar" {

name = "route-test-%s"

dest_range = "15.0.0.0/24"

network = "${google_compute_network.foobar.name}"

next_hop_ip = "10.0.1.5"

priority = 100

}`, acctest.RandString(10), acctest.RandString(10))

)

This would ensure a test wouldn't falsely pass because a resource wasn't cleaned up matching the static name. Or is there some reason I'm missing that wouldn't work for this resource type?

I initially had some concerns about a circular reference between service accounts and IAM policies. Still working through that, but I'll use the random char where possible. Thanks!

evandbrown · 2016-11-14T18:19:59Z

@cblecker You want to make the call on merging this? Makes sense to me as you're interested in the feature :)

cblecker

As far as I can see, the code looks good. Do the acceptance tests all pass? I'm still in the learning stages of Go, though. A few alignment and spacing nits.

I'll let someone else make the call on merging though, both because I'm inexperienced, and I don't have merge rights :P

cblecker · 2016-11-14T18:53:10Z

website/source/layouts/google.erb

@@ -24,6 +24,9 @@
 		<ul class="nav nav-visible">
 			<li<%= sidebar_current("docs-google-project") %>>
 			<a href="/docs/providers/google/r/google_project.html">google_project</a>
+            </li>
+            <li<%= sidebar_current("docs-google-service-account") %>>
+			<a href="/docs/providers/google/r/google_service_account.html">google_service_account</a>


nit: alignment

Gah tabs v spaces in vim. Thanks!

cblecker · 2016-11-14T18:53:37Z

builtin/providers/google/resource_google_service_account_test.go

+var testAccGoogleServiceAccount_policy = `
+resource "google_service_account" "acceptance" {
+    account_id = "%v"
+	display_name = "%v"


nit: alignment

cblecker · 2016-11-14T19:01:08Z

website/source/docs/providers/google/r/google_service_account.html.markdown

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


nit: double space after original

cblecker · 2016-11-14T19:01:45Z

website/source/docs/providers/google/r/google_service_account.html.markdown

+```js
+resource "google_service_account" "object_viewer" {
+    account_id = "object-viewer"
+	display_name = "Object viewer"


nit: alignment

evandbrown · 2016-11-14T19:21:01Z

Thanks, @cblecker! Acceptance tests are good:

$ make testacc TEST=./builtin/providers/google GOOGLE_PROJECT=evandbrown17 GOOGLE_REGION=us-central1 TESTARGS='-run=TestAccGoogleServiceAccount_...'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/14 11:18:48 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccGoogleServiceAccount_... -timeout 120m
=== RUN   TestAccGoogleServiceAccount_basic
--- PASS: TestAccGoogleServiceAccount_basic (5.84s)
=== RUN   TestAccGoogleServiceAccount_createPolicy
--- PASS: TestAccGoogleServiceAccount_createPolicy (7.74s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/google 13.645s

$ git rev-parse HEAD
4bdf65c36f7dd2092692d6e311845050b08add99

I know Mr @stack72 had need for service accounts recently. Perhaps he'd like to comment on this change and merge when ready?

paddycarver · 2016-11-14T20:48:53Z

If @stack72 doesn't get a chance this week to make the call, I will. :)

paddycarver · 2016-11-15T00:52:25Z

@stack72 passed this to me, so I'll have a response for it in the next few days. :)

paddycarver · 2016-11-16T04:13:43Z

builtin/providers/google/resource_google_service_account.go

+		}
+
+		oldPStringf, _ := json.MarshalIndent(oldPString, "", "   ")
+		newPStringf, _ := json.MarshalIndent(newPString, "", "   ")


Why use json.MarshalIndent here? It's being called on a string, so from my experiments, it would seem to have no effect that fmt.Printf("%q") wouldn't. Also, ignoring the errors is making me nervous. I understand that it'll only effect debug outputs, but even just logging them and carrying on would make me feel better; at least we would have accurate debug output. :)

Ooh, %q. That works!

paddycarver · 2016-11-16T04:25:26Z

builtin/providers/google/resource_google_service_account_test.go

+		Steps: []resource.TestStep{
+			// The first step creates a basic service account
+			resource.TestStep{
+				Config: fmt.Sprintf(testAccGoogleServiceAccount_basic, accountId, displayName),


The pattern I've seen is that these get implemented as functions, with accountId and displayName passed in as arguments and the Config string returned, instead of as fmt.Sprintf strings. This seems like a better option to me, as it documents the requirements and should they change after this is reused, it won't build unless you update them everywhere. With the Sprintf string, it'll just have weird output.

Great catch. Fixed.

paddycarver · 2016-11-16T04:29:53Z

builtin/providers/google/resource_google_service_account_test.go

+			return fmt.Errorf("Not found: %s", r)
+		}
+
+		if rs.Primary.Attributes["display_name"] != displayName2 {


I'm probably missing something basic here, but why make displayName2 global mutable state instead of passing it in as an argument, as you did for testAccCheckGoogleServiceAccountPolicyCount?

You're not missing anything; I missed that in a refactor.

paddycarver · 2016-11-16T04:31:50Z

website/source/docs/providers/google/r/google_service_account.html.markdown

+
+# google\_service\_account
+
+Allows management of a Google Cloud Platform service account.


Maybe link to service account docs?

paddycarver · 2016-11-16T04:34:33Z

website/source/docs/providers/google/r/google_service_account.html.markdown

+    Changing this updates the policy.
+
+    Deleting this removes the policy, but leaves the original policy
+    intact. If there are overlapping `binding` entries between the original


What is the "original policy" in this case? I'm somewhat familiar with service accounts, but I'm confused about what happens when deleting the policy.

paddycarver · 2016-11-16T04:37:23Z

Sorry for the delay. Got around to reviewing this, and I had a few questions. They may not require code changes at all, it's entirely possible I'm missing things. :) Let me know if you have any questions or disagree with any of my feedback. And thanks for taking the time to submit the new resource!

evandbrown · 2016-11-17T17:51:50Z

Thanks, @paddyforan. Great feedback, and prompt. I really appreciate it. Just pushed a new change that addresses them all.

paddycarver

Thanks for the speedy update on the code. Glad the review was helpful.

This looks good to me, so I'll go ahead and merge it. Super glad we got this in, and thanks again for contributing it!

paddycarver · 2016-11-17T18:51:31Z

website/source/docs/providers/google/r/google_service_account.html.markdown

-    intact. If there are overlapping `binding` entries between the original
-    policy and the data source policy, they will be removed.
+    Deleting this removes the policy declared in Terraform. Any policy bindings
+    associated with the project before Terraform was used are not deleted.


That makes so much more sense to me now. :D

ghost · 2020-04-20T01:50:54Z

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.

danawillow suggested changes Nov 8, 2016

View reviewed changes

cblecker reviewed Nov 10, 2016

View reviewed changes

providers/google: Create and delete Service Accounts

5423349

evandbrown force-pushed the google-service-account-resource branch from 3f260aa to 5423349 Compare November 14, 2016 04:49

evandbrown added 2 commits November 14, 2016 09:42

providers/google: random resource names in SA test

9a18804

docs: Add google_service_account to sidebar

08dba7e

evandbrown changed the title ~~WIP: providers/google: Create and delete Service Accounts~~ providers/google: Create and delete Service Accounts Nov 14, 2016

evandbrown self-assigned this Nov 14, 2016

evandbrown added the provider/google-cloud label Nov 14, 2016

Fix go vet issue

1612d6a

cblecker approved these changes Nov 14, 2016

View reviewed changes

Fix spacing inconsistencies

4bdf65c

paddycarver self-assigned this Nov 15, 2016

paddycarver suggested changes Nov 16, 2016

View reviewed changes

Resolve review feedback

a32fe2d

paddycarver approved these changes Nov 17, 2016

View reviewed changes

paddycarver merged commit 642dd87 into hashicorp:master Nov 17, 2016

ghost locked and limited conversation to collaborators Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

providers/google: Create and delete Service Accounts #9946

providers/google: Create and delete Service Accounts #9946

evandbrown commented Nov 8, 2016 •

edited

Loading

evandbrown commented Nov 8, 2016 •

edited

Loading

cblecker commented Nov 8, 2016

danawillow left a comment

danawillow Nov 8, 2016

danawillow Nov 8, 2016

danawillow Nov 8, 2016

cblecker Nov 10, 2016

evandbrown Nov 14, 2016

evandbrown commented Nov 14, 2016

cblecker left a comment

cblecker Nov 14, 2016

evandbrown Nov 14, 2016

cblecker Nov 14, 2016

cblecker Nov 14, 2016

cblecker Nov 14, 2016

evandbrown commented Nov 14, 2016

paddycarver commented Nov 14, 2016

paddycarver commented Nov 15, 2016

paddycarver Nov 16, 2016

evandbrown Nov 17, 2016

paddycarver Nov 16, 2016

evandbrown Nov 17, 2016

paddycarver Nov 16, 2016

evandbrown Nov 17, 2016

paddycarver Nov 16, 2016

evandbrown Nov 17, 2016

paddycarver Nov 16, 2016

evandbrown Nov 17, 2016

paddycarver commented Nov 16, 2016

evandbrown commented Nov 17, 2016

paddycarver left a comment

paddycarver Nov 17, 2016

ghost commented Apr 20, 2020

	resource "google_compute_route" "foobar" {
	name = "route-test-%s"
	dest_range = "15.0.0.0/24"
	network = "${google_compute_network.foobar.name}"
	next_hop_ip = "10.0.1.5"
	priority = 100
	}`, acctest.RandString(10), acctest.RandString(10))


		Changing this updates the policy.

		Deleting this removes the policy, but leaves the original policy


		# google\_service\_account

		Allows management of a Google Cloud Platform service account.

providers/google: Create and delete Service Accounts #9946

providers/google: Create and delete Service Accounts #9946

Conversation

evandbrown commented Nov 8, 2016 • edited Loading

evandbrown commented Nov 8, 2016 • edited Loading

cblecker commented Nov 8, 2016

danawillow 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

evandbrown commented Nov 14, 2016

cblecker 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

evandbrown commented Nov 14, 2016

paddycarver commented Nov 14, 2016

paddycarver commented Nov 15, 2016

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 16, 2016

evandbrown commented Nov 17, 2016

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 20, 2020

evandbrown commented Nov 8, 2016 •

edited

Loading

evandbrown commented Nov 8, 2016 •

edited

Loading