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

added boot_disk_kms_key to node_config #3044

Merged

Conversation

megan07
Copy link
Contributor

@megan07 megan07 commented Jan 30, 2020

Fixes hashicorp/terraform-provider-google#5478

Added boot_disk_kms_key to node_config block.

This will require us to update the vendor package google.golang.org/api/container/v1beta1, but the attribute we need hasn't been released yet, and the hash I updated it to for testing, broke another resource. Do I wait for it to be released? Or manually revert the broken package? Or try to find a different hash to update to?

Release Note Template for Downstream PRs (will be copied)

container: added `boot_disk_kms_key` to `node_config` block.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

@emilymye
Copy link
Contributor

I think it's fine to wait until the attribute has been released in library for the handwritten resources, that's typically what we've done

@megan07 megan07 force-pushed the megan_node_config_cmek branch from 0a01673 to 6429c00 Compare February 4, 2020 22:28
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 15 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 6 files changed, 163 insertions(+), 9 deletions(-))

@kirillmakhonin
Copy link

Hi @megan07,
as I understand KMS for node_pool should be specified directly in node pool's, not in cluster's config.

https://cloud.google.com/kubernetes-engine/docs/how-to/using-cmek

To create a node pool with customer-managed encryption for node boot disks, specify a value for the --boot-disk-kms-key parameter in your creation command.

gcloud beta container node-pools create [NODE_POOL_NAME] \
--zone [ZONE] \
--disk-type [DISK_TYPE] \
--boot-disk-kms-key projects/[KEY_PROJECT_ID]/locations/[LOCATION]/keyRings/[RING_NAME]/cryptoKeys/[KEY_NAME] \
--project [CLUSTER_PROJECT_ID] \
--cluster [CLUSTER_NAME]

@emilymye
Copy link
Contributor

emilymye commented Feb 6, 2020

@kirillmakhonin we share the schema for node_config (https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1beta1/NodeConfig) whereever it is used in the GKE API resources, so it covers adding it as part of both google_container_cluster and google_container_node_pool. If you've encountered a situation where GKE returns an error if it's specified in google_container_cluster, we can address that, but gcloud in general masks the format of the API call so you wouldn't be able to tell that it's part of the node_config when using gcloud

Copy link
Contributor

@emilymye emilymye left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of things!

@@ -244,6 +244,16 @@ var schemaNodeConfig = &schema.Schema{
},
},
},

"boot_disk_kms_key": {
<% if version.nil? || version == 'ga' -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's a new field that didn't exist in GA before, we can just add <% unless version == 'ga' -%> gates around the field

@@ -627,7 +627,7 @@ func resourceSqlDatabaseInstanceCreate(d *schema.ResourceData, meta interface{})
for _, u := range users.Items {
if u.Name == "root" && u.Host == "%" {
err = retry(func() error {
op, err = config.clientSqlAdmin.Users.Delete(project, instance.Name, u.Host, u.Name).Do()
op, err = config.clientSqlAdmin.Users.Delete(project, instance.Name).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'd update the description to explain why we're doing this here, or I'd move it to the vendor

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 7 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 6 files changed, 164 insertions(+), 9 deletions(-))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GKE CMEK on Boot DIsk
5 participants