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

Adds support for creating KMS KeyRing resources #518

Merged
merged 34 commits into from
Oct 27, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ab18653
Instantiate the cloudkms client
mrparkers Sep 16, 2017
d18db25
Implement Create and Read for the kms key ring resource
mrparkers Sep 16, 2017
946eef1
Expose the kms key ring resource
mrparkers Sep 16, 2017
7afead9
Create acceptance test for creating a KeyRing, fix read to use KeyRin…
mrparkers Sep 29, 2017
b491ad0
Add cloudkms library to vendor
mrparkers Oct 3, 2017
0ce76d7
Merge branch 'master' into kms-keyring
mrparkers Oct 9, 2017
34e5b64
Address style comments
mrparkers Oct 15, 2017
7536f92
Use fully-qualified keyring name in read operation
mrparkers Oct 15, 2017
5b815d7
Remove call to SetId during read operation
mrparkers Oct 15, 2017
bc9527f
Set ID as entire resource string
mrparkers Oct 15, 2017
1248ab8
Spin up a new project for acceptance test
mrparkers Oct 15, 2017
e3de446
Merge branch 'master' into kms-keyring
mrparkers Oct 15, 2017
d6cb880
Use Getenv for billing and org environment variables
mrparkers Oct 16, 2017
9d7ff51
Merge branch 'master' into kms-keyring
mrparkers Oct 22, 2017
c524051
And test and logs around removal from state
mrparkers Oct 22, 2017
5a7b690
Add comments
mrparkers Oct 22, 2017
33b74c0
Fixes formatting
mrparkers Oct 23, 2017
5c49d08
Log warning instead of info
mrparkers Oct 23, 2017
775d43f
Use a single line for cloudkms client actions
mrparkers Oct 23, 2017
23500d0
Add resource import test
mrparkers Oct 24, 2017
b8d0dbe
Add ability to import resource, update helper functions to use keyRin…
mrparkers Oct 24, 2017
c4317ab
Use shorter terraform ID for easier import
mrparkers Oct 26, 2017
c2106a9
Update import test to use the same config as the basic test
mrparkers Oct 26, 2017
27fc744
Update KeyRing name regex to be consistent with API docs
mrparkers Oct 26, 2017
a2c3363
Add documentation page for resource
mrparkers Oct 26, 2017
82f4785
Add KeyRing documentation to sidebar
mrparkers Oct 26, 2017
6e2a6c0
Adds unit tests around parsing the KeyRing import id
mrparkers Oct 26, 2017
2120b47
Allow for project in id to be autopopulated from config
mrparkers Oct 26, 2017
65cbadc
Throw error in import if project provider is not provided for locatio…
mrparkers Oct 26, 2017
024b45b
Merge branch 'master' into kms-keyring
mrparkers Oct 26, 2017
2791c74
Consistent variable names
mrparkers Oct 26, 2017
248d518
Use tabs in resource config instead of spaces
mrparkers Oct 26, 2017
3fbf0bb
Remove "-x" suffix for docs
mrparkers Oct 26, 2017
6dc9035
Set project attribute on import if different from the project config
mrparkers Oct 26, 2017
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
9 changes: 9 additions & 0 deletions google/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"golang.org/x/oauth2/jwt"
"google.golang.org/api/bigquery/v2"
"google.golang.org/api/cloudbilling/v1"
"google.golang.org/api/cloudkms/v1"
"google.golang.org/api/cloudresourcemanager/v1"
resourceManagerV2Beta1 "google.golang.org/api/cloudresourcemanager/v2beta1"
computeBeta "google.golang.org/api/compute/v0.beta"
Expand Down Expand Up @@ -47,6 +48,7 @@ type Config struct {
clientComputeBeta *computeBeta.Service
clientContainer *container.Service
clientDns *dns.Service
clientKms *cloudkms.Service
clientLogging *cloudlogging.Service
clientPubsub *pubsub.Service
clientResourceManager *cloudresourcemanager.Service
Expand Down Expand Up @@ -155,6 +157,13 @@ func (c *Config) loadAndValidate() error {
}
c.clientDns.UserAgent = userAgent

log.Printf("[INFO] Instantiating Google Cloud KMS Client...")
c.clientKms, err = cloudkms.New(client)
if err != nil {
return err
}
c.clientKms.UserAgent = userAgent

log.Printf("[INFO] Instantiating Google Stackdriver Logging client...")
c.clientLogging, err = cloudlogging.New(client)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions google/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ func Provider() terraform.ResourceProvider {
"google_logging_billing_account_sink": resourceLoggingBillingAccountSink(),
"google_logging_folder_sink": resourceLoggingFolderSink(),
"google_logging_project_sink": resourceLoggingProjectSink(),
"google_kms_key_ring": resourceKmsKeyRing(),
"google_sourcerepo_repository": resourceSourceRepoRepository(),
"google_spanner_instance": resourceSpannerInstance(),
"google_spanner_database": resourceSpannerDatabase(),
Expand Down
93 changes: 93 additions & 0 deletions google/resource_kms_key_ring.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package google

import (
"fmt"
"github.com/hashicorp/terraform/helper/schema"
"google.golang.org/api/cloudkms/v1"
"log"
)

func resourceKmsKeyRing() *schema.Resource {
return &schema.Resource{
Create: resourceKmsKeyRingCreate,
Read: resourceKmsKeyRingRead,
Delete: resourceKmsKeyRingDelete,

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"location": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"project": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
},
}
}

func kmsResourceParentString(project, location string) string {
return fmt.Sprintf("projects/%s/locations/%s", project, location)
}

func kmsResourceParentKeyRingName(project, location, name string) string {
return fmt.Sprintf("%s/keyRings/%s", kmsResourceParentString(project, location), name)
}

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

project, err := getProject(d, config)
if err != nil {
return err
}

name := d.Get("name").(string)
location := d.Get("location").(string)

parent := kmsResourceParentString(project, location)

keyRing, err := config.clientKms.Projects.Locations.KeyRings.
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry to point this out again, but mind fixing the style here and elsewhere? https://github.com/golang/go/wiki/CodeReviewComments#line-length
I think one line would be fine, two is also fine (we usually break after the left paren)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I don't think I understood before but I see what you're saying now. I'll change it to a single line.

Create(parent, &cloudkms.KeyRing{}).
KeyRingId(name).
Do()

if err != nil {
return fmt.Errorf("Error creating KeyRing: %s", err)
}

log.Printf("[DEBUG] Created KeyRing %s", keyRing.Name)

d.SetId(keyRing.Name)

return resourceKmsKeyRingRead(d, meta)
}

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

keyRingName := d.Id()

log.Printf("[DEBUG] Executing read for KMS KeyRing %s", keyRingName)

_, err := config.clientKms.Projects.Locations.KeyRings.
Get(keyRingName).
Do()

if err != nil {
return fmt.Errorf("Error reading KeyRing: %s", err)
}

return nil
}

func resourceKmsKeyRingDelete(d *schema.ResourceData, meta interface{}) error {
return nil
}
105 changes: 105 additions & 0 deletions google/resource_kms_key_ring_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package google

import (
"fmt"
"testing"

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

func TestAccGoogleKmsKeyRing_basic(t *testing.T) {
skipIfEnvNotSet(t,
[]string{
"GOOGLE_ORG",
"GOOGLE_BILLING_ACCOUNT",
}...,
)

projectId := "terraform-" + acctest.RandString(10)
projectOrg := os.Getenv("GOOGLE_ORG")
projectBillingAccount := os.Getenv("GOOGLE_BILLING_ACCOUNT")
keyRingName := fmt.Sprintf("tf-test-%s", acctest.RandString(10))

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testGoogleKmsKeyRing_recreate(keyRingName),
Steps: []resource.TestStep{
resource.TestStep{
Config: testGoogleKmsKeyRing_basic(projectId, projectOrg, projectBillingAccount, keyRingName),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleKmsKeyRingExists("google_kms_key_ring.key_ring"),
),
},
},
})
}

func testAccCheckGoogleKmsKeyRingExists(resourceName string) resource.TestCheckFunc {
return func(s *terraform.State) error {
config := testAccProvider.Meta().(*Config)

rs, ok := s.RootModule().Resources[resourceName]
if !ok {
return fmt.Errorf("Resource not found: %s", resourceName)
}

name := rs.Primary.Attributes["name"]
location := rs.Primary.Attributes["location"]
project := rs.Primary.Attributes["project"]

parent := kmsResourceParentString(project, location)
keyRingName := kmsResourceParentKeyRingName(project, location, name)

listKeyRingsResponse, err := config.clientKms.Projects.Locations.KeyRings.List(parent).Do()
Copy link
Contributor

Choose a reason for hiding this comment

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

why list rather than get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used list because I don't like it when my tests look similar to the production code. I thought this would be a good way to still test the intended functionality without making it too similar to Read.

I am open to changing it if you would like me to.

if err != nil {
return fmt.Errorf("Error listing KeyRings: %s", err)
}

for _, keyRing := range listKeyRingsResponse.KeyRings {
log.Printf("[DEBUG] Found KeyRing: %s", keyRing.Name)

if keyRing.Name == keyRingName {
return nil
}
}

return fmt.Errorf("KeyRing not found: %s", keyRingName)
}
}

// TODO
// KMS KeyRings cannot be deleted. This will test if the resource can be added back to state after being removed
func testGoogleKmsKeyRing_recreate(name 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.

If the resource already exists server-side, we can add import functionality instead of trying to get it to essentially import on create.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the expected behavior of attempting to create a KeyRing that already exists? Should it fail because the resource already exists on the server? Or should it handle it similar to how an import would?

I think most resources would fail here because you'd expect to have to use terraform import for resources not managed by terraform. This seems like a special situation though, because for all other resources, you can do terraform destroy and terraform apply to re-create your infrastructure without having to modify the names of any of your resources. If calling Create for a KMS Keyring fails for KeyRings that already exist on the server, you wouldn't be able to do this.

In either case, my intention was for this test to capture that logic, where I either expect a failure when re-creating, or for it to be handled gracefully.

return func(s *terraform.State) error {
return nil
}
}

func testGoogleKmsKeyRing_basic(projectId, projectOrg, projectBillingAccount, keyRingName string) string {
return fmt.Sprintf(`
resource "google_project" "acceptance" {
name = "%s"
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 fix the formatting here? I think you just have a mix of tabs and spaces

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 fix the formatting here? I think you've got a spaces vs tabs issue (we usually use tabs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my editor automatically converts tabs to spaces for me. I can fix this.

project_id = "%s"
org_id = "%s"
billing_account = "%s"
}

resource "google_project_services" "acceptance" {
project = "${google_project.acceptance.project_id}"
services = [
"cloudkms.googleapis.com"
]
}

resource "google_kms_key_ring" "key_ring" {
project = "${google_project_services.acceptance.project}"
name = "%s"
location = "us-central1"
}
`, projectId, projectId, projectOrg, projectBillingAccount, keyRingName)
}
Loading