Add support for google_service_account_key #472

sebglon · 2017-09-27T06:48:44Z

to correct PR #204

danawillow

Just to confirm, this change adds about 70 new vendored files. Is that intentional?

Also, before this is checked in it'll need documentation in the website/ folder.

danawillow · 2017-10-03T04:18:31Z

google/resource_google_service_account_key.go

+	}
+
+	d.SetId(sak.Name)
+	d.Set("name", sak.Name)


Instead of setting these values on create, we usually call the read function at the end of the create function. Is it possible for you to do that here? Alternatively, if your goal was to avoid an extra API call you could add a helper function that could take the ServiceAccountKey object and do the d.Set calls

danawillow · 2017-10-03T04:19:42Z

google/resource_google_service_account_key.go

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


We're trying to get to a more clear way of ordering fields in the schema, by doing Requireds, then Optionals, then Computeds. Mind doing that here too?

danawillow · 2017-10-03T04:26:47Z

google/resource_google_service_account_key_test.go

+}
+
+func testAccGoogleServiceAccountKey(account, name string) string {
+	t := `resource "google_service_account" "acceptance" {


for consistency with other resources, can you just start this with the fmt.Sprintf instead of making it a variable?

danawillow · 2017-10-03T04:27:34Z

google/resource_google_service_account_key_test.go

+}
+
+resource "google_service_account_key" "acceptance" {
+	service_account_id = "${google_service_account.acceptance.id}"


do you also want to test some of the other fields? There's some logic regarding pgp keys in your create function so at a minimum I'd like to see that tested, but ideally the other fields as well

danawillow · 2017-10-03T04:29:14Z

google/resource_google_service_account_key_test.go

+			resource.TestStep{
+				Config: testAccGoogleServiceAccountKey(accountId, displayName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckGoogleServiceAccountKeyExists("google_service_account_key.acceptance"),


You can check values with fields with resource.TestCheckResourceAttr to ensure they have the values you want instead of just that the resource exists

danawillow · 2017-10-03T04:34:14Z

google/resource_google_service_account_key.go

+		return fmt.Errorf("Error creating service account key: %s", err)
+	}
+
+	if v, ok := d.GetOk("pgp_key"); ok {


I'm a bit confused about this part, since it's just reading a value from the config, encrypting it, and then storing it back in state. I'm not totally against it, but it does make me cautious since it's not necessarily matching the API. What's the use case?

Use case is to get GPG_key on output for using it with another tools. If it is possible to have them on outpup without storing it, i'm interesting.

sebglon · 2017-10-03T07:51:37Z

@danawillow I have modified all comment except Adding test on all fields.
Is it possible to have field output without storing them? If it's possible, i can remove all added vendor.

sebglon · 2017-10-13T06:23:02Z

I have open on issue on Google googleapis/google-api-go-client#234
Because create finish in 200 but if you make get just after we have 404 with the generated key name.
If i wait 10 sec or more under create and get, i have get 200.
And if i refresh google console i can see the key but i can't download public key on it to validate.

@danawillow can you see help us on google to have the good people?

…lient#234

sebglon · 2017-10-16T07:00:18Z

I have added a workaroud for waiting public Key.
Is ti possible to Validate doc?

danawillow · 2017-10-17T20:23:46Z

google/resource_google_service_account_key.go

+
+	r := &iam.CreateServiceAccountKeyRequest{}
+
+	if v, ok := d.GetOk("key_algorithm"); ok {


Since this has a default value, ok will always be true, and this and the above can be simplified to:

r := &iam.CreateServiceAccountKeyRequest { KeyAlgorithm: d.Get("key_algorithm").(string), }

danawillow · 2017-10-17T20:24:25Z

google/resource_google_service_account_key.go

+	}
+
+	if pubKey == "" {
+


nit: extra newline

danawillow · 2017-10-17T20:28:37Z

google/resource_google_service_account_key.go

+		pubKey = pubkeyInterface.(string)
+	}
+
+	if pubKey == "" {


Since this variable never gets used, it might make sense to just not set it at all and instead do an if statement like:

if _, ok := d.GetOk("public_key"); !ok { // stuff }

danawillow · 2017-10-17T21:06:18Z

google/resource_google_service_account_key.go

+
+		sak, err := config.clientIAM.Projects.ServiceAccounts.Keys.Create(serviceAccount, r).Do()
+		if err != nil || sak == nil {
+			return fmt.Errorf("Error creating service account key: %s", err)


it might be nice to differentiate in the message the case where it returned an error vs returning nothing

danawillow · 2017-10-17T21:13:08Z

google/resource_google_service_account_key.go

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


Looks like this value has a reasonable default: https://cloud.google.com/iam/reference/rest/v1/projects.serviceAccounts.keys/get. Any reason why it's Required vs private_key_type and key_algorithm being Optional?

danawillow · 2017-10-17T21:36:47Z

google/resource_google_service_account_key.go

+				Optional: true,
+				ForceNew: true,
+			},
+			"private_key": &schema.Schema{


I think this should be down in the Computed section

danawillow · 2017-10-17T21:37:39Z

website/docs/r/google_service_account_key.html.markdown

+
+## Import
+
+Lightsail Key Pairs cannot be imported, because the private and public key are


what's lightsail?

danawillow · 2017-10-17T21:39:52Z

website/docs/r/google_service_account_key.html.markdown

+}
+```
+
+## Argument Reference


missing docs for public_key_type

danawillow · 2017-10-17T21:41:37Z

website/docs/r/google_service_account_key.html.markdown

+
+## Attributes Reference
+
+The following attributes are exported in addition to the arguments listed above:


also valid_after and valid_before

danawillow · 2017-10-17T21:42:19Z

website/docs/r/google_service_account_key.html.markdown

+* `public_key` - the public key, base64 encoded
+* `private_key` - the private key, base64 encoded. This is only populated
+when creating a new key, and when no `pgp_key` is provided
+* `encrypted_private_key` – the private key material, base 64 encoded and


In the schema these are called private_key_encrypted and private_key_fingerprint

danawillow · 2017-10-20T22:32:35Z

google/resource_google_service_account_key.go

+
+		r.PrivateKeyType = d.Get("private_key_type").(string)
+
+		sak, err := config.clientIAM.Projects.ServiceAccounts.Keys.Create(serviceAccount, r).Do()


I'm confused, why do we only call the create function if public_key is unset?

yes of course.

aeneasr · 2017-10-22T12:26:05Z

Anything I can do to help speed up this PR? :) Creating service account keys is indispensable to configuring Google Cloud SQL on GKE containers as the Cloud SQL Proxy requires those credentials - otherwise it's not possible to connect from containers within GKE to Google Cloud SQL which in turn makes it impossible to automate the process with terraform at the moment.

danawillow

Almost there!

danawillow · 2017-10-23T21:39:33Z

google/resource_google_service_account_key.go

+		KeyAlgorithm: d.Get("key_algorithm").(string),
+	}
+
+	var err error


You don't have to declare this here since you initialize it just a few lines down with the :=

danawillow · 2017-10-23T21:40:21Z

google/resource_google_service_account_key.go

+
+	var err error
+
+	r.PrivateKeyType = d.Get("private_key_type").(string)


You can also add this into the struct on line 94ish like you did with key_algorithm

danawillow · 2017-10-23T21:42:35Z

google/resource_google_service_account_key.go

+	}
+
+	err = serviceAccountKeyWaitTime(config.clientIAM.Projects.ServiceAccounts.Keys, d.Id(), d.Get("public_key_type").(string), "Creating Service account key", 4)
+	if err != nil {


If there's an error creating it, we probably want to d.SetId("") so it doesn't end up in state.

This is actualiy a bug on GCE IAM, the key is is well created but not downloadable just after creation. it is necessary to wait a few second or minut to download it.

The setId is good because key well exist

danawillow · 2017-10-23T21:48:01Z

website/docs/r/google_service_account_key.html.markdown

+## Argument Reference
+
+The following arguments are supported:
+* `name` - The name used for this key pair (not used on create)


Since name is computed, it probably belongs in the Attributes Reference section (and then you can get rid of the parenthetical)

danawillow · 2017-10-23T21:49:29Z

website/docs/r/google_service_account_key.html.markdown

+
+The following attributes are exported in addition to the arguments listed above:
+
+* `fingerprint` - The MD5 public key fingerprint as specified in section 4 of RFC 4716.


nit: can you make sure there's a new line between each bullet point?

danawillow · 2017-10-23T21:53:13Z

google/resource_google_service_account_key_test.go

+)
+
+// Test that a service account key can be created and destroyed
+func TestAccGoogleServiceAccountKey_basic(t *testing.T) {


Can you add t.Parallel() (and then a new line) to the beginning of each of these tests?

danawillow · 2017-10-23T22:03:48Z

google/resource_google_service_account_key_test.go

+	public_key_type = "TYPE_X509_PEM_FILE"
+	pgp_key = <<EOF
+%s
+EOF 


you have an extra space after the EOF which is causing the test to fail. If you remove the extra space it passes.

danawillow · 2017-10-23T22:10:19Z

Oh also you'll want to add an entry into website/google.erb so the documentation shows up on the sidebar

sebglon · 2017-10-24T06:47:50Z

All change are made.

danawillow · 2017-10-25T18:12:13Z

google/resource_google_service_account_key.go

+	if err != nil {
+		return err
+	}
+	resourceGoogleServiceAccountKeyRead(d, meta)


this can just be return resourceGoogleServiceAccountKeyRead(d, meta) and then you can get rid of the rest of the function

danawillow · 2017-10-25T18:14:49Z

google/resource_google_service_account_key.go

+			"public_key": {
+				Type:     schema.TypeString,
+				Computed: true,
+				Optional: true,


I think this Optional line should be removed, and this whole field should move into the Computed section

danawillow · 2017-10-25T18:16:28Z

website/docs/r/google_service_account_key.html.markdown

+* `public_key` - The public key, base64 encoded
+
+* `private_key` - The private key, base64 encoded. This is only populated
+


nit: the newline should be after the next line, not before it.

danawillow · 2017-10-25T18:17:58Z

google/resource_google_service_account_key_test.go

+
+// Test that a service account key can be created and destroyed
+func TestAccGoogleServiceAccountKey_basic(t *testing.T) {
+	t.Parallel()


nit: empty line after all t.Parallel()s

danawillow · 2017-10-25T18:19:12Z

website/docs/r/google_service_account_key.html.markdown

@@ -0,0 +1,85 @@
+---
+layout: "google"
+page_title: "Google: google_service_accout_key"


typo: account

danawillow · 2017-10-25T18:19:31Z

website/google.erb

@@ -92,6 +92,10 @@
      <li<%= sidebar_current("docs-google-service-account") %>>
        <a href="/docs/providers/google/r/google_service_account.html">google_service_account</a>
      </li>
+      <li<%= sidebar_current("docs-google-service-account") %>>


this should be docs-google-service-account-key

sebglon · 2017-10-25T19:07:54Z

@danawillow All are good

danawillow · 2017-10-25T19:43:12Z

Thanks @sebglon!

$ make testacc TEST=./google TESTARGS='-run=TestAccGoogleServiceAccountKey'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleServiceAccountKey -timeout 120m
=== RUN   TestAccGoogleServiceAccountKey_basic
=== RUN   TestAccGoogleServiceAccountKey_pgp
--- PASS: TestAccGoogleServiceAccountKey_pgp (59.03s)
--- PASS: TestAccGoogleServiceAccountKey_basic (60.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	60.324s

aeneasr · 2017-10-25T19:59:10Z

Awesome, thank you both for your work on this! Is there a way to check the ETA of unreleased versions (specifically 1.1.2 :) )?

danawillow · 2017-10-25T20:08:37Z

Our goal is to release every two weeks or so, but we don't give exact dates because sometimes we need the flexibility to release sooner or later.

aeneasr · 2017-10-25T20:09:30Z

Thank you for the swift response, I'm looking forward to having this released!

aeneasr · 2017-11-01T15:54:20Z

Hope this doesn't come by as annoying, but this issue has actually blocked my team and me since it came up. It would be extremely awesome if it was possible to release some time this week, especially because i have no idea how to add custom built plugins to terraform (installed from binaries).

* Initial support for google service account keys * Add vendor for vault and encryption * Add change for PR comment * Add doc and improvement fo public key management * adding waiter for compatibility with issue googleapis/google-api-go-client#234 * improvement * Add test with pgp_key * Perform doc anf format * remove test if public_key exists * Add link on doc * correct pr

@rileykarson

…corp#472)  /cc @rileykarson

ghost · 2020-03-30T14:22:48Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

michaelahern and others added 2 commits September 26, 2017 19:17

Initial support for google service account keys

f330886

Add vendor for vault and encryption

2b33016

sebglon mentioned this pull request Sep 27, 2017

[WIP] Add support for google_service_account_key #204

Closed

danawillow suggested changes Oct 3, 2017

View reviewed changes

Add change for PR comment

0168ffa

sebglon force-pushed the PR-204 branch from 2c033e5 to 0168ffa Compare October 3, 2017 07:23

radeksimko added the new-resource label Oct 8, 2017

selmanj mentioned this pull request Oct 12, 2017

Feature Request: Google Service Account Key #61

Closed

Sébastien GLON added 2 commits October 13, 2017 08:33

Add doc and improvement fo public key management

cff02b2

adding waiter for compatibility with issue googleapis/google-api-go-c…

4c08aa3

…lient#234

danawillow suggested changes Oct 17, 2017

View reviewed changes

Sébastien GLON added 3 commits October 18, 2017 21:34

improvement

4e4140d

Add test with pgp_key

631e88a

Perform doc anf format

f30f857

danawillow reviewed Oct 20, 2017

View reviewed changes

remove test if public_key exists

0a7181f

danawillow suggested changes Oct 23, 2017

View reviewed changes

sebglon force-pushed the PR-204 branch from ff08fe3 to 52e5ad6 Compare October 24, 2017 06:42

Add link on doc

0da0fbc

sebglon force-pushed the PR-204 branch from 52e5ad6 to 0da0fbc Compare October 24, 2017 06:46

danawillow suggested changes Oct 25, 2017

View reviewed changes

correct pr

9f2f87c

danawillow approved these changes Oct 25, 2017

View reviewed changes

danawillow merged commit 94e0b74 into hashicorp:master Oct 25, 2017

sebglon deleted the PR-204 branch October 25, 2017 20:27

AndreaGiardini mentioned this pull request Nov 23, 2017

Feature: google_service_account_keys #116

Closed

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

Fix the 3rd occurence of old ig instances interpolation syntax (hashi…

3533a74

…corp#472)  /cc @rileykarson

ghost locked and limited conversation to collaborators Mar 30, 2020


		r := &iam.CreateServiceAccountKeyRequest{}

		if v, ok := d.GetOk("key_algorithm"); ok {


		## Import

		Lightsail Key Pairs cannot be imported, because the private and public key are


		## Attributes Reference

		The following attributes are exported in addition to the arguments listed above:


		r.PrivateKeyType = d.Get("private_key_type").(string)

		sak, err := config.clientIAM.Projects.ServiceAccounts.Keys.Create(serviceAccount, r).Do()


		var err error

		r.PrivateKeyType = d.Get("private_key_type").(string)


		The following attributes are exported in addition to the arguments listed above:

		* `fingerprint` - The MD5 public key fingerprint as specified in section 4 of RFC 4716.

		* `public_key` - The public key, base64 encoded

		* `private_key` - The private key, base64 encoded. This is only populated

Add support for google_service_account_key #472

Add support for google_service_account_key #472

Conversation

sebglon commented Sep 27, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebglon commented Oct 3, 2017

sebglon commented Oct 13, 2017 • edited

sebglon commented Oct 16, 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

aeneasr commented Oct 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow commented Oct 23, 2017

sebglon commented Oct 24, 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

sebglon commented Oct 25, 2017

danawillow commented Oct 25, 2017

aeneasr commented Oct 25, 2017

danawillow commented Oct 25, 2017

aeneasr commented Oct 25, 2017

aeneasr commented Nov 1, 2017

ghost commented Mar 30, 2020

sebglon commented Oct 13, 2017 •

edited