Bug 1884632: Adding BYOK disk encryption through DES #158

kwoodson · 2020-08-13T21:22:42Z

This pull requests adds fields to the v1beta1 types to suppert disk encryption sets.

We have customers requesting that we provide OS disk encryption. AKS is using DES for OS level as well as PVC support. The following PR was an attempt to add the necessary fields for bring-your-own-key encryption using DiskEncryptionSets.

This is high priority for the ARO team and we would like to get the work going.

kwoodson · 2020-08-13T22:36:25Z

@enxebre @ingvagabund PTAL

kwoodson · 2020-08-13T23:40:55Z

@ingvagabund When running make generate the zz_generated.deepcopy.go was not generated. This might be due to my local setup. I hand patched it for the time being but it should be generated.

kwoodson · 2020-08-14T00:49:01Z

I had to grant Reader role on the DES for the service principal and then with this latest update I was able to get this to work:

oc get machines
...
kwtest-w6n4r-worker-eastus1-des-9xqdr   Running   Standard_D2s_v3   eastus   1      6m4s

Command line:

az vm show -n kwtest-w6n4r-worker-eastus1-des-9xqdr -g aro-w5o8qzfu
...
    "osDisk": {
      "caching": "ReadWrite",
      "createOption": "FromImage",
      "diffDiskSettings": null,
      "diskSizeGb": 128,
      "encryptionSettings": null,
      "image": null,
      "managedDisk": {
        "diskEncryptionSet": {
          "id": "/subscriptions/225e02bc-43d0-43d1-a01a-17e584a4ef69/resourceGroups/disk-encryption/providers/Microsoft.Compute/diskEncryptionSets/aro-de",
          "resourceGroup": "disk-encryption"
        },
        "id": "/subscriptions/225e02bc-43d0-43d1-a01a-17e584a4ef69/resourceGroups/ARO-W5O8QZFU/providers/Microsoft.Compute/disks/kwtest-w6n4r-worker-eastus1-des-9xqdr_OSDisk",

From the portal I was able to confirm the encryption:

kwtest-w6n4r-worker-eastus1-des-9xqdr_OSDisk
128 GiB
Premium SSD
SSE with CMK               <----- SSE (storage service encryption) with CMK (customer managed key)
Read/write

michaelgugino · 2020-08-14T16:23:42Z

Make generate scripts were broken. We'll need to re-run the make generate on this patch set after this merges: #159

Otherwise, this change LGTM

elmiko

this looks reasonable to me
/lgtm

ingvagabund · 2020-08-17T13:04:17Z

I had to grant Reader role on the DES for the service principal

Worth documenting the additional role

abhinavdahiya · 2020-08-28T19:02:28Z

pkg/apis/azureprovider/v1beta1/types.go

-	StorageAccountType string `json:"storageAccountType"`
+type ManagedDiskParameters struct {
+	StorageAccountType string                       `json:"storageAccountType"`
+	DiskEncryptionSet  *DiskEncryptionSetParameters `json:"diskEncryptionSet,omitempty"`


it would be nice if we can provide details on what kind of permissions are required to use this disk encryption set for this machine.

The service principal that is used with the cluster will require the reader role on the disk encryption set.
https://docs.microsoft.com/en-us/azure/aks/azure-disk-customer-managed-keys#grant-the-diskencryptionset-access-to-key-vault

abhinavdahiya · 2020-08-28T19:04:34Z

pkg/apis/azureprovider/v1beta1/types.go

+}
+
+type DiskEncryptionSetParameters struct {
+	ID string `json:"id,omitempty"`


Would be nice to include some documentation on the the expected form of the ID, Using Azure resource IDs implies that this resource can be from any subscription, any resource group.

Since this is for supporting customer managed encryption, it is fair to say we need to support this resource from resource group other than the cluster's but what about the subscription.

Here is an example of an a disk encryption set ID. Azure resource IDs include a path-like system so that it is clear where the object is located, what the object is, and its name.

/subscriptions/<subscriptionid>/resourceGroups/disk-encryption/providers/Microsoft.Compute/diskEncryptionSets/aro-de

According to the documentation here https://docs.microsoft.com/en-us/azure/virtual-machines/linux/disk-encryption-key-vault#create-a-key-vault it states:

Your key vault and VMs must be in the same subscription. Also, to ensure that encryption secrets don't cross regional boundaries, Azure Disk Encryption requires the Key Vault and the VMs to be co-located in the same region. Create and use a Key Vault that is in the same subscription and region as the VMs to be encrypted.

kwoodson · 2020-09-01T13:21:49Z

Rebased with #158, make generate, and pushed. Let's go CI bot!

kwoodson · 2020-09-01T13:42:16Z

I guess I need to add the UPSTREAM: <carry>: git commit message.

kwoodson · 2020-09-01T16:16:39Z

Looks like flakes under both e2e-azure and the operator:

11/42
- ingress
- dns
- image-registry
- monitoring
etc

3/2465
Failing tests:

[sig-cli] oc adm must-gather runs successfully [Suite:openshift/conformance/parallel]
[sig-cli] oc adm must-gather runs successfully with options [Suite:openshift/conformance/parallel]

kwoodson · 2020-09-01T16:42:48Z

/test e2e-azure-operator

kwoodson · 2020-09-02T16:52:27Z

/test e2e-azure-operator

kwoodson · 2020-09-02T16:52:31Z

/test e2e-azure

kwoodson · 2020-09-02T20:34:49Z

/test e2e-azure

kwoodson · 2020-09-02T21:55:29Z

@elmiko @michaelgugino Any ideas why this continues to fail? It looks like must-gather is failing. This PR only adds fields but isn't actually exercised. Is this inherently flaky?

What do we do to get this to pass?

If there is something wrong with the PR I'm happy to fix it. Can these tests be run locally? Just looking for some help.

michaelgugino · 2020-09-02T22:54:43Z

/test govet

openshift-bot · 2020-10-14T10:05:38Z

/retest

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

openshift-bot · 2020-10-14T12:21:40Z

/retest

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

openshift-bot · 2020-10-14T13:06:48Z

/retest

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

openshift-ci-robot · 2020-10-14T15:34:49Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure-operator	`1b3c02b`	link	`/test e2e-azure-operator`

Full PR test history. Your PR dashboard.

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.

openshift-bot · 2020-10-14T17:00:51Z

/retest

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

openshift-bot · 2020-10-14T17:26:57Z

/retest

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

openshift-bot · 2020-10-14T18:44:51Z

/retest

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

openshift-bot · 2020-10-14T22:51:49Z

/retest

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

openshift-bot · 2020-10-15T02:06:42Z

/retest

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

openshift-bot · 2020-10-15T02:19:44Z

/retest

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

openshift-bot · 2020-10-15T04:42:47Z

/retest

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

openshift-bot · 2020-10-15T04:55:47Z

/retest

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

openshift-bot · 2020-10-15T07:58:58Z

/retest

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

openshift-bot · 2020-10-15T11:29:33Z

/retest

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

openshift-bot · 2020-10-15T12:04:44Z

/retest

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

openshift-bot · 2020-10-15T14:40:49Z

/retest

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

openshift-bot · 2020-10-15T21:04:56Z

/retest

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

openshift-bot · 2020-10-16T00:43:08Z

/retest

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

openshift-bot · 2020-10-16T03:06:10Z

/retest

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

openshift-bot · 2020-10-16T03:19:07Z

/retest

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

openshift-bot · 2020-10-16T07:39:09Z

/retest

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

openshift-bot · 2020-10-16T07:52:08Z

/retest

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

openshift-bot · 2020-10-16T10:54:04Z

/retest

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

openshift-bot · 2020-10-16T11:46:14Z

/retest

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

openshift-bot · 2020-10-16T12:51:38Z

/retest

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

openshift-bot · 2020-10-16T13:04:10Z

/retest

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

openshift-ci-robot · 2020-10-16T15:58:58Z

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

openshift/cluster-api-provider-azure#158

Bugzilla bug 1884632 has been moved to the MODIFIED state.

In response to this:

Bug 1884632: Adding BYOK disk encryption through DES

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.

kwoodson requested review from ingvagabund and enxebre August 13, 2020 21:22

openshift-ci-robot requested review from alexander-demicev and michaelgugino August 13, 2020 21:23

kwoodson force-pushed the byok_des branch from a333f50 to e66a1d4 Compare August 14, 2020 00:36

elmiko approved these changes Aug 17, 2020

View reviewed changes

openshift-ci-robot assigned elmiko Aug 17, 2020

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

kwoodson mentioned this pull request Aug 17, 2020

Bug 1871858: update make generate #159

Merged

abhinavdahiya reviewed Aug 28, 2020

View reviewed changes

kwoodson force-pushed the byok_des branch from e66a1d4 to 2c4f1bd Compare September 1, 2020 13:19

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2020

kwoodson force-pushed the byok_des branch 2 times, most recently from 407534a to 9a07101 Compare September 1, 2020 13:53

michaelgugino closed this Sep 2, 2020

openshift-merge-robot merged commit 4090a69 into openshift:master Oct 16, 2020

JoelSpeed mentioned this pull request Oct 19, 2020

Bug 1884632: Bump Azure dependency to include BYOK encryption fields openshift/machine-api-operator#730

Merged

JoelSpeed mentioned this pull request Nov 17, 2020

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

Merged

Bug 1884632: Adding BYOK disk encryption through DES #158

Bug 1884632: Adding BYOK disk encryption through DES #158

Conversation

kwoodson commented Aug 13, 2020 • edited Loading

kwoodson commented Aug 13, 2020

kwoodson commented Aug 13, 2020

kwoodson commented Aug 14, 2020

michaelgugino commented Aug 14, 2020

elmiko left a comment

Choose a reason for hiding this comment

ingvagabund commented Aug 17, 2020

abhinavdahiya Aug 28, 2020

Choose a reason for hiding this comment

kwoodson Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

abhinavdahiya Aug 28, 2020

Choose a reason for hiding this comment

kwoodson Aug 28, 2020

Choose a reason for hiding this comment

kwoodson commented Sep 1, 2020

kwoodson commented Sep 1, 2020

kwoodson commented Sep 1, 2020

kwoodson commented Sep 1, 2020

kwoodson commented Sep 2, 2020

kwoodson commented Sep 2, 2020

kwoodson commented Sep 2, 2020

kwoodson commented Sep 2, 2020 • edited Loading

michaelgugino commented Sep 2, 2020

openshift-bot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 15, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-bot commented Oct 16, 2020

openshift-ci-robot commented Oct 16, 2020

kwoodson commented Aug 13, 2020 •

edited

Loading

kwoodson Aug 28, 2020 •

edited

Loading

kwoodson commented Sep 2, 2020 •

edited

Loading