GCP: populate encryption values into tf and machines by michaelgugino · Pull Request #4397 · openshift/installer

michaelgugino · 2020-11-19T18:51:54Z

This commit ensures encryption values are properly
populated into terraform and machines/machinesets.

michaelgugino · 2020-11-19T18:52:57Z

pkg/asset/machines/gcp/machines.go


+	var encryptionKey *gcpprovider.GCPEncryptionKeyReference
+
+	if mpool.OSDisk.EncryptionKey != nil {


Is there a reason that OSDisk.EncryptionKey can't just be an imported type from the GCP provider? That should allow us to a do a straight assignment here.

Yeah. OSDisk.EncryptionKey is defined in the types package, which represents the installer API, so we try to keep it free of external dependencies considering other projects import the types package.

michaelgugino · 2020-11-19T18:53:31Z

pkg/tfvars/gcp/gcp.go

 	"github.com/openshift/installer/pkg/types"
 )

+const (


Maybe this belongs in GCP provider in a util package?

michaelgugino · 2020-11-19T18:53:41Z

pkg/tfvars/gcp/gcp.go

 	return json.MarshalIndent(cfg, "", "  ")
 }
+
+func generateDiskEncryptionKeyLink(keyRef *gcpprovider.GCPEncryptionKeyReference, projectID string) string {


Maybe this belongs in GCP provider in a util package?

To echo our conversation on slack, I would handle this in terraform:

create a type for the key similar to Auth in TFVars and pass the values in that way

conditionally create the self link variable (or null value therein) with logic similar to BYO networking: https://github.com/openshift/installer/blob/master/data/data/gcp/network/common.tf

I'm also fine with this approach and think it's OK to have this logic live here.

staebler

Looks good

staebler · 2020-11-19T20:26:07Z

pkg/asset/machines/gcp/machines.go

+	} else {
+		encryptionKey = nil
+	}


The else block is unnecessary.

staebler · 2020-11-19T20:37:00Z

data/data/gcp/main.tf

-  root_volume_type = var.gcp_master_root_volume_type
+  root_volume_size         = var.gcp_master_root_volume_size
+  root_volume_type         = var.gcp_master_root_volume_type
+  root_volume_kms_key_link = var.gcp_root_volume_kms_key_link


Need to add this to the "bootstrap" module, too.

https://github.com/openshift/installer/pull/4397/files#diff-c9542c57acc94c4d250fd8d806ac9cf57c9ccd52c3c8e282a34527198949ff57R35

Is that not done as the change in data/data/gcp/bootstrap/main.tf, or does that need to go into data/data/bootstrap somewhere?

In /data/data/gcp/main.tf as linked above. It is currently only passing the variable from main to master, but needs to do the same for bootstrap.

patrickdillon

Left a few comments, but this looks really good to me.

patrickdillon · 2020-11-19T20:30:22Z

pkg/asset/machines/gcp/machines.go

+			KMSKeyServiceAccount: mpool.OSDisk.EncryptionKey.KMSKeyServiceAccount,
+		}
+	} else {
+		encryptionKey = nil


nit: not sure this is necessary (should already be nil). if you want to leave it in, moving it out of else and before if would be more concise.

This shouldn't be needed as var encryptionKey *gcpprovider.GCPEncryptionKeyReference above the if makes it a nil anyway

patrickdillon · 2020-11-19T20:33:35Z

data/data/gcp/bootstrap/variables.tf


+variable "root_volume_kms_key_link" {
+  type       = string
+  description = "The GCP self link of KMS key to encrypt the volume"


nit: missing period.

patrickdillon · 2020-11-19T20:33:56Z

data/data/gcp/master/variables.tf


+variable "root_volume_kms_key_link" {
+  type       = string
+  description = "The GCP self link of KMS key to encrypt the volume"


nit: missing period

patrickdillon · 2020-11-19T20:34:05Z

data/data/gcp/variables-gcp.tf


+variable "gcp_root_volume_kms_key_link" {
+  type       = string
+  description = "The GCP self link of KMS key to encrypt the volume"


nit: missing period

patrickdillon · 2020-11-19T20:47:19Z

pkg/tfvars/gcp/gcp.go

 	return json.MarshalIndent(cfg, "", "  ")
 }
+
+func generateDiskEncryptionKeyLink(keyRef *gcpprovider.GCPEncryptionKeyReference, projectID string) string {


To echo our conversation on slack, I would handle this in terraform:

create a type for the key similar to Auth in TFVars and pass the values in that way

conditionally create the self link variable (or null value therein) with logic similar to BYO networking: https://github.com/openshift/installer/blob/master/data/data/gcp/network/common.tf

I'm also fine with this approach and think it's OK to have this logic live here.

patrickdillon · 2020-11-19T20:48:27Z

/test e2e-gcp

patrickdillon · 2020-11-19T20:50:58Z

data/data/gcp/master/variables.tf

+variable "root_volume_kms_key_link" {
+  type       = string
+  description = "The GCP self link of KMS key to encrypt the volume"
+  default = null


column alignment is off and causing tf-fmt to fail

patrickdillon · 2020-11-19T20:51:14Z

data/data/gcp/variables-gcp.tf

+variable "gcp_root_volume_kms_key_link" {
+  type       = string
+  description = "The GCP self link of KMS key to encrypt the volume"
+  default = null


column alignment is off and causing tf-fmt to fail

column alignment is off and causing tf-fmt to fail

weird, tf-fmt seems to be suggesting different alignment for this file than the other two:
https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/4397/pull-ci-openshift-installer-master-tf-fmt/1329497726143959040/build-log.txt

patrickdillon · 2020-11-19T20:52:03Z

data/data/gcp/bootstrap/variables.tf

+variable "root_volume_kms_key_link" {
+  type       = string
+  description = "The GCP self link of KMS key to encrypt the volume"
+  default = null


column alignment is off and causing tf-fmt to fail

patrickdillon · 2020-11-19T20:59:54Z

I would pull the missing dependencies into this PR in two separate commit: one commit for your changes and a separate one for the vendored files.

JoelSpeed · 2020-11-23T15:52:05Z

@patrickdillon I've fixed all of the comments that were left (I believe) on this branch master...JoelSpeed:gcp-encrypt

I've also added a commit for the missing dependencies as requested.

I didn't do anything for this one (https://github.com/openshift/installer/pull/4397/files#r527187850) as it seemed there wasn't a strong preference to change it.

Mike is out today and has suggested he will be out all week, I wondered if you could give my branch a check over and, if that's looking good, I could squash my fixups into his commit, open a new PR and we can merge that one while he is out, WDYT? Trying to give this PR as much time to soak in QE as possible

patrickdillon · 2020-11-23T18:15:14Z

@patrickdillon I've fixed all of the comments that were left (I believe) on this branch master...JoelSpeed:gcp-encrypt

I've also added a commit for the missing dependencies as requested.

I didn't do anything for this one (https://github.com/openshift/installer/pull/4397/files#r527187850) as it seemed there wasn't a strong preference to change it.

Mike is out today and has suggested he will be out all week, I wondered if you could give my branch a check over and, if that's looking good, I could squash my fixups into his commit, open a new PR and we can merge that one while he is out, WDYT? Trying to give this PR as much time to soak in QE as possible

The branch in general looks good. I don't have approval rights at this level, so I can't push this through. @staebler is out until Monday. I would recommend just opening a PR and getting everything to pass so it could be ready to go on Monday.

If you think it's really important to get it in ASAP we can try to coordinate an approve with @sdodson 's help.

michaelgugino · 2020-12-04T18:21:26Z

/retest

staebler · 2020-12-07T14:30:41Z

@michaelgugino The dependency mess has finally merged. This PR needs another rebase.

This commit ensures encryption values are properly populated into terraform and machines/machinesets.

staebler

/lgtm
/approve
/label tide/merge-method-squash

openshift-ci-robot · 2020-12-07T16:49:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~data/data/gcp/OWNERS~~ [staebler]
~~pkg/asset/machines/gcp/OWNERS~~ [staebler]
~~pkg/tfvars/gcp/OWNERS~~ [staebler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

staebler · 2020-12-07T16:49:11Z

/test e2e-gcp

openshift-bot · 2020-12-07T17:15:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2020-12-07T19:17:24Z

@michaelgugino: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-metal-ipi	`a903c53`	link	`/test e2e-metal-ipi`
ci/prow/e2e-aws-fips	`50d34a0`	link	`/test e2e-aws-fips`
ci/prow/e2e-crc	`50d34a0`	link	`/test e2e-crc`
ci/prow/e2e-libvirt	`50d34a0`	link	`/test e2e-libvirt`
ci/prow/e2e-aws-workers-rhel7	`50d34a0`	link	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-metal-ipi-ovn-ipv6	`50d34a0`	link	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-gcp	`50d34a0`	link	`/test e2e-gcp`
ci/prow/e2e-ovirt	`50d34a0`	link	`/test e2e-ovirt`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

staebler · 2020-12-07T19:25:38Z

The GCP install succeeded in the latest e2e-gcp test.

staebler · 2020-12-07T19:35:53Z

All the changes in this PR are in GCP-specific files.

/override ci/prow/e2e-aws

openshift-ci-robot · 2020-12-07T19:36:32Z

@staebler: Overrode contexts on behalf of staebler: ci/prow/e2e-aws

Details

In response to this:

All the changes in this PR are in GCP-specific files.

/override ci/prow/e2e-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from jstuever and patrickdillon November 19, 2020 18:52

michaelgugino commented Nov 19, 2020

View reviewed changes

staebler reviewed Nov 19, 2020

View reviewed changes

patrickdillon reviewed Nov 19, 2020

View reviewed changes

michaelgugino force-pushed the gcp-encrypt2 branch 2 times, most recently from 823aef4 to 11ae34a Compare December 4, 2020 17:29

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2020

michaelgugino and others added 3 commits December 7, 2020 11:08

GCP: populate encryption values into tf and machines

a40b496

This commit ensures encryption values are properly populated into terraform and machines/machinesets.

Address review feedback

523cd1b

fix tf-fmt

50d34a0

michaelgugino force-pushed the gcp-encrypt2 branch from f9a4267 to 50d34a0 Compare December 7, 2020 16:11

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2020

staebler reviewed Dec 7, 2020

View reviewed changes

openshift-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 7, 2020

openshift-ci-robot assigned staebler Dec 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2020

staebler mentioned this pull request Dec 7, 2020

WIP: Gcp encrypt #4435

Closed

openshift-merge-robot merged commit f7e061d into openshift:master Dec 7, 2020


		var encryptionKey *gcpprovider.GCPEncryptionKeyReference

		if mpool.OSDisk.EncryptionKey != nil {

Conversation

michaelgugino commented Nov 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Nov 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Nov 19, 2020

Uh oh!

JoelSpeed commented Nov 23, 2020

Uh oh!

patrickdillon commented Nov 23, 2020

Uh oh!

michaelgugino commented Dec 4, 2020

Uh oh!

staebler commented Dec 7, 2020

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Dec 7, 2020

Uh oh!

staebler commented Dec 7, 2020

Uh oh!

openshift-bot commented Dec 7, 2020

Uh oh!

openshift-merge-robot commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler commented Dec 7, 2020

Uh oh!

staebler commented Dec 7, 2020

Uh oh!

openshift-ci-robot commented Dec 7, 2020

Uh oh!

Reviewers

Assignees

Labels

openshift-merge-robot commented Dec 7, 2020 •

edited

Loading