BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #183

dkorzuno · 2020-11-17T03:04:20Z

What this PR does / why we need it:

The PR adds a parameter which enables encryption at host for virtual machines.

The corresponding upstream PR is kubernetes-sigs#1012

Special notes for your reviewer:

I am not sure how to go about unit testing the changes.

Release note:

1. Add SecurityProfile.EncryptionAtHost parameter to machine spec to enable host-based VM encryption.

JoelSpeed · 2020-11-17T10:12:27Z

@dkorzuno This PR seems to implement a feature similar to #158 which merged earlier during this release cycle.

Disk encryption and this feature seem to be at odds with each other, ie, you can't use them at the same time. Are you aware of how the two features differ? It would be good to understand the motivation for supporting both

jim-minter · 2020-11-18T22:30:39Z

@JoelSpeed yes, they're awfully similar, but not the same...

#158 is about device encryption of attached VM disks using user managed keys, which is largely an enterprise requirement.
This PR is about encryption of hypervisor-local resources including scratch volumes, which is largely a federal requirement.

The options are not mutually exclusive and I believe we have customer use cases for neither, either, and both. Aaaah!

Thanks,

Jim

JoelSpeed · 2020-11-19T10:50:11Z

Cannot be enabled if Azure Disk Encryption (guest-VM encryption using bitlocker/VM-Decrypt) is enabled on your VMs/virtual machine scale sets.

Azure Disk Encryption cannot be enabled on disks that have encryption at host enabled.

This is from the docs that are linked in the PR description, suggests to me that they are mutually exclusive and that you can't use both in tandem 🤔

Do we have an RFE for this feature? As far as I'm aware no one has brought this feature to the team's attention before raising this PR so we are a little unsure of what the drive is for adding this feature

jim-minter · 2020-11-19T17:28:12Z

Azure Disk Encryption is yet another technology, it's different again to disk encryption sets (#158) and encryption at host (this PR). I understand that OpenShift doesn't and won't support ADE.

Sorry there's no RFE :-/ Let's catch up about that tomorrow.

dkorzuno · 2020-11-23T05:06:35Z

Created a Bugzilla entry https://bugzilla.redhat.com/show_bug.cgi?id=1900454

JoelSpeed · 2020-11-23T10:13:53Z

/retitle BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption

openshift-ci-robot · 2020-11-23T10:15:16Z

@dkorzuno: This pull request references Bugzilla bug 1900454, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption

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.

JoelSpeed · 2020-11-23T10:19:37Z

/bugzilla refresh

openshift-ci-robot · 2020-11-23T10:19:44Z

@JoelSpeed: This pull request references Bugzilla bug 1900454, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

dkorzuno · 2020-11-26T22:15:51Z

@JoelSpeed (or someone) could you help me identify what caused the e2e test fail? The build log says

stderr:
+ curl -q -s --connect-timeout 1 http://localhost:10249/proxyMode
command terminated with exit code 7
error:
exit status 7

but it's hard to trace it back to the change I made.

JoelSpeed · 2020-11-30T14:56:52Z

could you help me identify what caused the e2e test fail?

I don't think this issue is related to your PR, let's give a restest and see if it fails in the same way again

/retest

JoelSpeed

The latest changes look good btw, I like the extraction of the virtualMachineParameter creation into its own function, very clean. Test cases looking good too

JoelSpeed · 2020-11-30T14:59:38Z

pkg/cloud/azure/services/virtualmachines/virtualmachines.go

+	return osProfile, nil
+}
+
+func DeriveVirtualMachineParameters(vmSpec *Spec, location string, subscription string, nic network.Interface) (*compute.VirtualMachine, error) {


Would be good to have a comment on top of this, also, probably doesn't need to be an exported method, deriveVirtualMachineParameters should be sufficient

Good point. Changes made.

JoelSpeed · 2020-11-30T15:01:34Z

pkg/cloud/azure/services/virtualmachines/virtualmachines_test.go

+		updateSpec := tc.updateSpec
+		validate := tc.validate


No need to declare these here is there? Can just use them inline?

dkorzuno · 2020-12-01T03:36:14Z

@JoelSpeed I made the changes you requested. The e2e test is still failing though :(
I'd appreciate any hints as to how can we move it further.

JoelSpeed · 2020-12-01T11:54:41Z

pkg/cloud/azure/services/virtualmachines/virtualmachines_test.go

+	}
+
+	for _, tc := range testCases {
+		tc := tc


I don't think we have a loop capturing issue here, this isn't normally needed since the range variable is a reference not a pointer. You can test this by breaking one of the test cases and removing this line, only the one should fail, not all of them

JoelSpeed · 2020-12-01T11:57:02Z

The E2E failures are unrelated, it is a known issue that the tests are rather flaky at the moment, don't worry about them.

FYI, I'm still waiting on PM direction as to whether we should be accepting this PR or not (CC @listey)

…rameter to enable host-based VM encryption

JoelSpeed · 2020-12-02T12:37:03Z

/approve

/hold until I get a response from PM about what to do here

openshift-ci-robot · 2020-12-02T12:37:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

JoelSpeed · 2021-01-14T10:02:49Z

Having spoken with PM, we should be able to merge this as soon as master opens for 4.8. Probably some time in February based on current schedules.

Will try and get someone else from my team to give a review as well so it's ready to go when master opens

Danil-Grigorev · 2021-01-14T13:22:16Z

This /lgtm Let's wait until the branch opens again.

JoelSpeed · 2021-02-09T11:33:39Z

/hold cancel

4.8 master is now open so this can be merged

Danil-Grigorev · 2021-02-09T11:34:40Z

/lgtm

openshift-ci-robot · 2021-02-09T14:38:35Z

@dkorzuno: All pull requests linked via external trackers have merged:

openshift/cluster-api-provider-azure#183

Bugzilla bug 1900454 has been moved to the MODIFIED state.

In response to this:

BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption

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.

mjudeikis · 2021-02-17T13:51:29Z

/cherry-pick release-4.7

openshift-cherrypick-robot · 2021-02-17T13:52:25Z

@mjudeikis: new pull request created: #201

In response to this:

/cherry-pick release-4.7

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.

mjudeikis · 2021-02-17T13:58:44Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-02-17T13:59:24Z

@mjudeikis: new pull request created: #202

In response to this:

/cherry-pick release-4.6

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 Danil-Grigorev and JoelSpeed November 17, 2020 03:04

openshift-ci-robot changed the title ~~Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption~~ BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption Nov 23, 2020

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Nov 23, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Nov 23, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 23, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Nov 23, 2020

dkorzuno force-pushed the encryption-at-host branch 2 times, most recently from e6deb30 to 363b0d5 Compare November 26, 2020 05:35

JoelSpeed reviewed Nov 30, 2020

View reviewed changes

dkorzuno force-pushed the encryption-at-host branch from 363b0d5 to 84e94d6 Compare December 1, 2020 01:04

JoelSpeed reviewed Dec 1, 2020

View reviewed changes

UPSTREAM: <carry>: openshift: add SecurityProfile.EncryptionAtHost pa…

6c9b028

…rameter to enable host-based VM encryption

dkorzuno force-pushed the encryption-at-host branch from 84e94d6 to 6c9b028 Compare December 2, 2020 04:36

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2021

openshift-ci-robot assigned Danil-Grigorev Feb 9, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2021

openshift-merge-robot merged commit 3442c7a into openshift:master Feb 9, 2021

openshift-cherrypick-robot mentioned this pull request Feb 17, 2021

[release-4.7] BUG 1929721: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #201

Merged

openshift-cherrypick-robot mentioned this pull request Feb 17, 2021

[release-4.6] BUG 1929725: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #183

BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #183

dkorzuno commented Nov 17, 2020

JoelSpeed commented Nov 17, 2020

jim-minter commented Nov 18, 2020

JoelSpeed commented Nov 19, 2020

jim-minter commented Nov 19, 2020

dkorzuno commented Nov 23, 2020

JoelSpeed commented Nov 23, 2020

openshift-ci-robot commented Nov 23, 2020

JoelSpeed commented Nov 23, 2020

openshift-ci-robot commented Nov 23, 2020

dkorzuno commented Nov 26, 2020 •

edited

Loading

JoelSpeed commented Nov 30, 2020

JoelSpeed left a comment

JoelSpeed Nov 30, 2020

dkorzuno Dec 1, 2020

JoelSpeed Nov 30, 2020

dkorzuno commented Dec 1, 2020

JoelSpeed Dec 1, 2020

JoelSpeed commented Dec 1, 2020

JoelSpeed commented Dec 2, 2020

openshift-ci-robot commented Dec 2, 2020

JoelSpeed commented Jan 14, 2021

Danil-Grigorev commented Jan 14, 2021

JoelSpeed commented Feb 9, 2021

Danil-Grigorev commented Feb 9, 2021

openshift-ci-robot commented Feb 9, 2021

mjudeikis commented Feb 17, 2021

openshift-cherrypick-robot commented Feb 17, 2021

mjudeikis commented Feb 17, 2021

openshift-cherrypick-robot commented Feb 17, 2021

BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #183

BUG 1900454: Add SecurityProfile.EncryptionAtHost parameter to enable host-based VM encryption #183

Conversation

dkorzuno commented Nov 17, 2020

JoelSpeed commented Nov 17, 2020

jim-minter commented Nov 18, 2020

JoelSpeed commented Nov 19, 2020

jim-minter commented Nov 19, 2020

dkorzuno commented Nov 23, 2020

JoelSpeed commented Nov 23, 2020

openshift-ci-robot commented Nov 23, 2020

JoelSpeed commented Nov 23, 2020

openshift-ci-robot commented Nov 23, 2020

dkorzuno commented Nov 26, 2020 • edited Loading

JoelSpeed commented Nov 30, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Nov 30, 2020

Choose a reason for hiding this comment

dkorzuno Dec 1, 2020

Choose a reason for hiding this comment

JoelSpeed Nov 30, 2020

Choose a reason for hiding this comment

dkorzuno commented Dec 1, 2020

JoelSpeed Dec 1, 2020

Choose a reason for hiding this comment

JoelSpeed commented Dec 1, 2020

JoelSpeed commented Dec 2, 2020

openshift-ci-robot commented Dec 2, 2020

JoelSpeed commented Jan 14, 2021

Danil-Grigorev commented Jan 14, 2021

JoelSpeed commented Feb 9, 2021

Danil-Grigorev commented Feb 9, 2021

openshift-ci-robot commented Feb 9, 2021

mjudeikis commented Feb 17, 2021

openshift-cherrypick-robot commented Feb 17, 2021

mjudeikis commented Feb 17, 2021

openshift-cherrypick-robot commented Feb 17, 2021

dkorzuno commented Nov 26, 2020 •

edited

Loading