Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for google_service_account_key #472

Merged
merged 11 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions google/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ func Provider() terraform.ResourceProvider {
"google_runtimeconfig_config": resourceRuntimeconfigConfig(),
"google_runtimeconfig_variable": resourceRuntimeconfigVariable(),
"google_service_account": resourceGoogleServiceAccount(),
"google_service_account_key": resourceGoogleServiceAccountKey(),
"google_storage_bucket": resourceStorageBucket(),
"google_storage_bucket_acl": resourceStorageBucketAcl(),
"google_storage_bucket_object": resourceStorageBucketObject(),
Expand Down
175 changes: 175 additions & 0 deletions google/resource_google_service_account_key.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
package google

import (
"fmt"

"github.com/hashicorp/terraform/helper/encryption"
"github.com/hashicorp/terraform/helper/schema"
"google.golang.org/api/iam/v1"
)

func resourceGoogleServiceAccountKey() *schema.Resource {
return &schema.Resource{
Create: resourceGoogleServiceAccountKeyCreate,
Read: resourceGoogleServiceAccountKeyRead,
Delete: resourceGoogleServiceAccountKeyDelete,
Schema: map[string]*schema.Schema{
// Required
"service_account_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"public_key_type": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For enum types, would you mind adding a ValidateFunc that checks the values?

ValidateFunc: validation.StringInSlice([]string{"TYPE_NONE", "TYPE_X509_PEM_FILE", "TYPE_RAW_PUBLIC_KEY"}, false),

},
// Optional
"private_key_type": &schema.Schema{
Type: schema.TypeString,
Default: "TYPE_GOOGLE_CREDENTIALS_FILE",
Optional: true,
ForceNew: true,
},

"key_algorithm": &schema.Schema{
Type: schema.TypeString,
Default: "KEY_ALG_RSA_2048",
Optional: true,
ForceNew: true,
},

"pgp_key": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
"private_key": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be down in the Computed section

Type: schema.TypeString,
Computed: true,
Sensitive: true,
},
"public_key": {
Type: schema.TypeString,
Computed: true,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ForceNew: true,
},
"name": &schema.Schema{
Type: schema.TypeString,
Computed: true,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is marked Optional, but it doesn't look like setting it will have any effect.

ForceNew: true,
},
// Computed
"valid_after": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"valid_before": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"private_key_encrypted": {
Type: schema.TypeString,
Computed: true,
},
"private_key_fingerprint": {
Type: schema.TypeString,
Computed: true,
},
},
}
}

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

serviceAccount := d.Get("service_account_id").(string)

r := &iam.CreateServiceAccountKeyRequest{}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
}

r.KeyAlgorithm = v.(string)
}

var pubKey string
var err error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if pubkeyInterface, ok := d.GetOk("public_key"); ok {
pubKey = pubkeyInterface.(string)
}

if pubKey == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra newline

if v, ok := d.GetOk("private_key_type"); ok {
r.PrivateKeyType = v.(string)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes of course.

if err != nil || sak == nil {
return fmt.Errorf("Error creating service account key: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

d.SetId(sak.Name)
// Data only available on create.
d.Set("valid_after", sak.ValidAfterTime)
d.Set("valid_before", sak.ValidBeforeTime)
if v, ok := d.GetOk("pgp_key"); ok {
encryptionKey, err := encryption.RetrieveGPGKey(v.(string))
if err != nil {
return err
}

fingerprint, encrypted, err := encryption.EncryptValue(encryptionKey, sak.PrivateKeyData, "Google Service Account Key")
if err != nil {
return err
}

d.Set("private_key_encrypted", encrypted)
d.Set("private_key_fingerprint", fingerprint)
} else {
d.Set("private_key", sak.PrivateKeyData)
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return err
}
resourceGoogleServiceAccountKeyRead(d, meta)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return err
}

return nil
}

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

publicKeyType := d.Get("public_key_type").(string)

// Confirm the service account key exists
sak, err := config.clientIAM.Projects.ServiceAccounts.Keys.Get(d.Id()).PublicKeyType(publicKeyType).Do()
if err != nil {
return handleNotFoundError(err, d, fmt.Sprintf("Service Account Key %q", d.Id()))
}

d.Set("name", sak.Name)
d.Set("key_algorithm", sak.KeyAlgorithm)
d.Set("public_key", sak.PublicKeyData)
return nil
}

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

_, err := config.clientIAM.Projects.ServiceAccounts.Keys.Delete(d.Id()).Do()
if err != nil {
return err
}

d.SetId("")
return nil
}
60 changes: 60 additions & 0 deletions google/resource_google_service_account_key_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package google

import (
"fmt"
"testing"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

resourceName := "google_service_account_key.acceptance"
accountID := "a" + acctest.RandString(10)
displayName := "Terraform Test"
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccGoogleServiceAccountKey(accountID, displayName),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleServiceAccountKeyExists(resourceName),
resource.TestCheckResourceAttrSet(resourceName, "public_key"),
resource.TestCheckResourceAttrSet(resourceName, "valid_after"),
resource.TestCheckResourceAttrSet(resourceName, "valid_before"),
),
},
},
})
}

func testAccCheckGoogleServiceAccountKeyExists(r string) resource.TestCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually our exists tests also read from the API to confirm the resource exists remotely. If there's a good reason why that shouldn't happen in this test, mind writing a comment as to why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[r]
if !ok {
return fmt.Errorf("Not found: %s", r)
}

if rs.Primary.ID == "" {
return fmt.Errorf("No ID is set")
}

return nil
}
}

func testAccGoogleServiceAccountKey(account, name string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a fair number of the fields in the schema are untested. Mind making sure there's coverage of the rest of the fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

return fmt.Sprintf(`resource "google_service_account" "acceptance" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency with the other resources' tests, can you move everything after fmt.Sprintf( to the next line? And then at the end, everything starting at the ` can also go on the next line.
(also be careful with formatting)

account_id = "%v"
display_name = "%v"

}

resource "google_service_account_key" "acceptance" {
service_account_id = "${google_service_account.acceptance.id}"
public_key_type = "TYPE_X509_PEM_FILE"
}`, account, name)
}
61 changes: 61 additions & 0 deletions google/service_account_waiter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package google

import (
"fmt"
"time"

"github.com/hashicorp/terraform/helper/resource"
"google.golang.org/api/googleapi"
iam "google.golang.org/api/iam/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this import need to be named?

)

type ServiceAccountKeyWaiter struct {
Service *iam.ProjectsServiceAccountsKeysService
PublicKeyType string
KeyName string
}

func (w *ServiceAccountKeyWaiter) RefreshFunc() resource.StateRefreshFunc {
return func() (interface{}, string, error) {
var err error
var sak *iam.ServiceAccountKey
sak, err = w.Service.Get(w.KeyName).PublicKeyType(w.PublicKeyType).Do()

if err != nil {
if err.(*googleapi.Error).Code == 404 {
return nil, "PENDING", nil
} else {
return nil, "", err
}
} else {
return sak, "DONE", nil
}
}
}

func (w *ServiceAccountKeyWaiter) Conf() *resource.StateChangeConf {
return &resource.StateChangeConf{
Pending: []string{"PENDING"},
Target: []string{"DONE"},
Refresh: w.RefreshFunc(),
}
}

func serviceAccountKeyWaitTime(client *iam.ProjectsServiceAccountsKeysService, keyName, publicKeyType, activity string, timeoutMin int) error {
w := &ServiceAccountKeyWaiter{
Service: client,
PublicKeyType: publicKeyType,
KeyName: keyName,
}

state := w.Conf()
state.Delay = 10 * time.Second
state.Timeout = time.Duration(timeoutMin) * time.Minute
state.MinTimeout = 2 * time.Second
_, err := state.WaitForState()
if err != nil {
return fmt.Errorf("Error waiting for %s: %s", activity, err)
}

return nil
}
15 changes: 15 additions & 0 deletions vendor/github.com/golang/snappy/AUTHORS

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 37 additions & 0 deletions vendor/github.com/golang/snappy/CONTRIBUTORS

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions vendor/github.com/golang/snappy/LICENSE

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading