-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 CryptoKeys resources #692
Changes from 18 commits
eb497da
b493bac
9de9eff
7ac65dc
0aa931e
1450129
58ada43
5da8276
4d5f9bf
42f6f24
e21d734
1ff44c3
7faf003
d9ebafa
cde0e24
0b49730
41eca4b
3ea4b8a
44ec6d6
d361401
adb4d54
56bb79d
f78800b
dfe7413
9f1857a
18b4791
90a04a4
345e299
d2e13b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package google | ||
|
||
import ( | ||
"testing" | ||
|
||
"fmt" | ||
"github.com/hashicorp/terraform/helper/acctest" | ||
"github.com/hashicorp/terraform/helper/resource" | ||
"os" | ||
) | ||
|
||
func TestAccGoogleKmsCryptoKey_importBasic(t *testing.T) { | ||
skipIfEnvNotSet(t, | ||
[]string{ | ||
"GOOGLE_ORG", | ||
"GOOGLE_BILLING_ACCOUNT", | ||
}..., | ||
) | ||
|
||
resourceName := "google_kms_crypto_key.crypto_key" | ||
|
||
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)) | ||
cryptoKeyName := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
Steps: []resource.TestStep{ | ||
resource.TestStep{ | ||
Config: testGoogleKmsCryptoKey_basic(projectId, projectOrg, projectBillingAccount, keyRingName, cryptoKeyName), | ||
}, | ||
|
||
resource.TestStep{ | ||
ResourceName: resourceName, | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
}, | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,262 @@ | ||
package google | ||
|
||
import ( | ||
"fmt" | ||
"log" | ||
"regexp" | ||
"strconv" | ||
"strings" | ||
"time" | ||
|
||
"github.com/hashicorp/terraform/helper/schema" | ||
"google.golang.org/api/cloudkms/v1" | ||
) | ||
|
||
func resourceKmsCryptoKey() *schema.Resource { | ||
return &schema.Resource{ | ||
Create: resourceKmsCryptoKeyCreate, | ||
Read: resourceKmsCryptoKeyRead, | ||
Delete: resourceKmsCryptoKeyDelete, | ||
Importer: &schema.ResourceImporter{ | ||
State: resourceKmsCryptoKeyImportState, | ||
}, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"name": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
}, | ||
"key_ring": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
}, | ||
"rotation_period": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ForceNew: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe a CryptoKey's rotation period can be changed, or at least there's an API for it. If this changed, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did bump into this, actually, but decided to defer it. On reflection, it might be worth doing given that you can't destroy the damn things properly... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with deferring it to a follow-up PR! |
||
}, | ||
}, | ||
} | ||
} | ||
|
||
type kmsCryptoKeyId struct { | ||
KeyRingId kmsKeyRingId | ||
Name string | ||
} | ||
|
||
// TODO: Add the info about rotation frequency and start time. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can you be a bit more specific here as to what you're adding it to? I don't think it belongs in the id, which is the section of code this comment is living in. |
||
|
||
func (s *kmsCryptoKeyId) cryptoKeyId() string { | ||
return fmt.Sprintf("%s/cryptoKeys/%s", s.KeyRingId.keyRingId(), s.Name) | ||
} | ||
|
||
func (s *kmsCryptoKeyId) parentId() string { | ||
return s.KeyRingId.keyRingId() | ||
} | ||
|
||
func (s *kmsCryptoKeyId) terraformId() string { | ||
return fmt.Sprintf("%s/%s", s.KeyRingId.terraformId(), s.Name) | ||
} | ||
|
||
func resourceKmsCryptoKeyCreate(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
keyRingId, err := parseKmsKeyRingId(d.Get("key_ring").(string), config) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
cryptoKeyId := &kmsCryptoKeyId{ | ||
KeyRingId: *keyRingId, | ||
Name: d.Get("name").(string), | ||
} | ||
|
||
key := cloudkms.CryptoKey{Purpose: "ENCRYPT_DECRYPT"} | ||
|
||
if d.Get("rotation_period") != "" { | ||
rotationPeriod := d.Get("rotation_period").(string) | ||
nextRotation, err := kmsCryptoKeyNextRotation(time.Now(), rotationPeriod) | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error setting CryptoKey rotation period: %s", err.Error()) | ||
} | ||
|
||
key.NextRotationTime = nextRotation | ||
key.RotationPeriod = rotationPeriod | ||
} | ||
|
||
cryptoKey, err := config.clientKms.Projects.Locations.KeyRings.CryptoKeys.Create(cryptoKeyId.KeyRingId.keyRingId(), &key).CryptoKeyId(cryptoKeyId.Name).Do() | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error creating CryptoKey: %s", err.Error()) | ||
} | ||
|
||
log.Printf("[DEBUG] Created CryptoKey %s", cryptoKey.Name) | ||
|
||
d.SetId(cryptoKeyId.terraformId()) | ||
|
||
return resourceKmsCryptoKeyRead(d, meta) | ||
} | ||
|
||
func resourceKmsCryptoKeyRead(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
cryptoKeyId, err := parseKmsCryptoKeyId(d.Id(), config) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
log.Printf("[DEBUG] Executing read for KMS CryptoKey %s", cryptoKeyId.cryptoKeyId()) | ||
|
||
_, err = config.clientKms.Projects.Locations.KeyRings.CryptoKeys.Get(cryptoKeyId.cryptoKeyId()).Do() | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error reading CryptoKey: %s", err) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we like to set as many fields as possible inside of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, are you just looking for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return nil | ||
} | ||
|
||
func clearCryptoKeyVersions(cryptoKeyId *kmsCryptoKeyId, config *Config) error { | ||
versionsClient := config.clientKms.Projects.Locations.KeyRings.CryptoKeys.CryptoKeyVersions | ||
|
||
versionsResponse, err := versionsClient.List(cryptoKeyId.cryptoKeyId()).Do() | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
for _, version := range versionsResponse.CryptoKeyVersions { | ||
request := &cloudkms.DestroyCryptoKeyVersionRequest{} | ||
_, err = versionsClient.Destroy(version.Name, request).Do() | ||
|
||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
/* | ||
Because KMS CryptoKey resources cannot be deleted on GCP, we are only going to remove it from state | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this comment has mismatched tabs/spaces for indentation, mind choosing one or the other? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
and destroy all its versions, rendering the key useless for encryption and decryption of data. | ||
Re-creation of this resource through Terraform will produce an error. | ||
*/ | ||
|
||
func resourceKmsCryptoKeyDelete(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
cryptoKeyId, err := parseKmsCryptoKeyId(d.Id(), config) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
log.Printf(` | ||
[WARNING] KMS CryptoKey resources cannot be deleted from GCP. The CryptoKey %s will be removed from Terraform state, | ||
and all its CryptoKeyVersions will be destroyed, but it will still be present on the server.`, cryptoKeyId.cryptoKeyId()) | ||
|
||
err = clearCryptoKeyVersions(cryptoKeyId, config) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
d.SetId("") | ||
return nil | ||
} | ||
|
||
func validateKmsCryptoKeyRotationPeriod(period string) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could actually probably become a ValidateFunc, which would allow doing these checks during |
||
pattern := regexp.MustCompile("^([0-9.]*\\d)s$") | ||
match := pattern.FindStringSubmatch(period) | ||
|
||
if len(match) == 0 { | ||
return fmt.Errorf("Invalid period format: %s", period) | ||
} | ||
|
||
number := match[1] | ||
seconds, err := strconv.ParseFloat(number, 64) | ||
|
||
if err == nil && seconds < 86400.0 { | ||
return fmt.Errorf("Rotation period must be greater than one day") | ||
} | ||
|
||
parts := strings.Split(number, ".") | ||
|
||
if err == nil && len(parts) > 1 && len(parts[1]) > 9 { | ||
return fmt.Errorf("Rotation period cannot have more than 9 fractional digits") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func kmsCryptoKeyNextRotation(now time.Time, period string) (string, error) { | ||
var result string | ||
var duration time.Duration | ||
|
||
err := validateKmsCryptoKeyRotationPeriod(period) | ||
|
||
if err == nil { | ||
duration, err = time.ParseDuration(period) | ||
} | ||
|
||
if err == nil { | ||
result = now.UTC().Add(duration).Format(time.RFC3339Nano) | ||
} | ||
|
||
return result, err | ||
} | ||
|
||
func parseKmsCryptoKeyId(id string, config *Config) (*kmsCryptoKeyId, error) { | ||
parts := strings.Split(id, "/") | ||
|
||
cryptoKeyIdRegex := regexp.MustCompile("^([a-z0-9-]+)/([a-z0-9-])+/([a-zA-Z0-9_-]{1,63})+/([a-zA-Z0-9_-]{1,63})$") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I didn't catch this earlier- shouldn't the third plus not be there since it already has the {1, 63}? I think that would end up essentially overriding the {1, 63} by allowing it to repeat some number of times |
||
cryptoKeyIdWithoutProjectRegex := regexp.MustCompile("^([a-z0-9-])+/([a-zA-Z0-9_-]{1,63})+/([a-zA-Z0-9_-]{1,63})$") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. likewise |
||
|
||
if cryptoKeyIdRegex.MatchString(id) { | ||
return &kmsCryptoKeyId{ | ||
KeyRingId: kmsKeyRingId{ | ||
Project: parts[0], | ||
Location: parts[1], | ||
Name: parts[2], | ||
}, | ||
Name: parts[3], | ||
}, nil | ||
} | ||
|
||
if cryptoKeyIdWithoutProjectRegex.MatchString(id) { | ||
if config.Project == "" { | ||
return nil, fmt.Errorf("The default project for the provider must be set when using the `{location}/{keyRingName}/{cryptoKeyName}` id format.") | ||
} | ||
|
||
return &kmsCryptoKeyId{ | ||
KeyRingId: kmsKeyRingId{ | ||
Project: config.Project, | ||
Location: parts[0], | ||
Name: parts[1], | ||
}, | ||
Name: parts[2], | ||
}, nil | ||
} | ||
|
||
return nil, fmt.Errorf("Invalid CryptoKey id format, expecting `{projectId}/{locationId}/{KeyringName}/{cryptoKeyName}` or `{locationId}/{keyRingName}/{cryptoKeyName}.`") | ||
} | ||
|
||
func resourceKmsCryptoKeyImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is actually similar enough to the Read fn that you don't need a separate Import fn, but instead could use the passthrough one:
Then inside the read function, you would do the same |
||
config := meta.(*Config) | ||
|
||
cryptoKeyId, err := parseKmsCryptoKeyId(d.Id(), config) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
d.Set("key_ring", cryptoKeyId.KeyRingId.terraformId()) | ||
d.Set("name", cryptoKeyId.Name) | ||
|
||
d.SetId(cryptoKeyId.terraformId()) | ||
|
||
return []*schema.ResourceData{d}, nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be a good idea to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done - further up so comment isn't hidden ... |
There was a problem hiding this comment.
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 newline) to the beginning of all tests?