AzExt - Adding support for the v2021-09-01-preview API #1739

Makdaam · 2021-09-14T12:44:35Z

disk encryption sets, sdn selection, encryption at host options

Which issue this PR addresses:

Fixes https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/7403671/

What this PR does / why we need it:

This PR switches the Azure CLI extension to the 2021-09-01-preview API, address the additional create parameters. We need it for customers who want to use the disk encryption features and have the feature flag enabled on their subscription to access the preview API. The plan is to give those customers an extension while the API is in preview, and keep it in parallel to the officiall AzureCLI command.

I didn't find any documentation on supporting multiple API versions inside an AzureCLI extension, so I went ahead with updating it to latest. This means that the extension can't be used anymore as a step in AzureCLI command development directly.

mostly changed:
create command, parameters, added validation

unchanged:
delete, list, list-credentials, show commands, update command

Rationale for not changing the update command:

Migration between SDN types can't be done reliably and isn't managed by any OCP operator - option shouldn't be changable
Swapping a disk from under a VM to one with a different Disk Encryption Set is a non-trivial operation, also this setting in aro could be misused instead of the key refresh functionality built into Disk Encryption Sets - not allowing the customer to change DES
Encryption At Host requires VM reprovisioning, changing the setting doesn't enable/disable it for live VMs - this should be managed via Machine objects

Test plan for issue:

Install the azure extension with make az
Follow the "how to create ARO cluster" official documentation until right before the az aro create command.
Run the following create commands - they should all fail validation

az aro create  --resource-group $RESOURCEGROUP  --name $CLUSTER  --vnet aro-vnet  --master-subnet master-subnet   --worker-subnet worker-subnet --disk-encryption-set 'notaresourceid'

az aro create  --resource-group $RESOURCEGROUP  --name $CLUSTER  --vnet aro-vnet  --master-subnet master-subnet   --worker-subnet worker-subnet --disk-encryption-set '/subscriptions/123/resourceGroups/v4-eastus/providers/Microsoft.Compute/diskEncryptionSets/nonexistentdiskencryptionset'

az aro create  --resource-group $RESOURCEGROUP  --name $CLUSTER  --vnet aro-vnet  --master-subnet master-subnet   --worker-subnet worker-subnet --master-encryption-at-host='notvalid'

az aro create  --resource-group $RESOURCEGROUP  --name $CLUSTER  --vnet aro-vnet  --master-subnet master-subnet   --worker-subnet worker-subnet --worker-encryption-at-host='notvalid'

az aro create  --resource-group $RESOURCEGROUP  --name $CLUSTER  --vnet aro-vnet  --master-subnet master-subnet   --worker-subnet worker-subnet --software-defined-network='notanetwork'

Create and run the aro create command

set additional env variables

export KEYVAULT_NAME=$USER-enckv
export KEYVAULT_KEY_NAME=$USER-key
export DISK_ENCRYPTION_SET_NAME=$USER-des

create the KeyVault and Key


az keyvault create -n $KEYVAULT_NAME -g $RESOURCEGROUP -l $LOCATION --enable-purge-protection true --enable-soft-delete true

az keyvault key create --vault-name $KEYVAULT_NAME -n $KEYVAULT_KEY_NAME --protection software

KEYVAULT_ID=$(az keyvault show --name $KEYVAULT_NAME --query "[id]" -o tsv)

KEYVAULT_KEY_URL=$(az keyvault key show --vault-name $KEYVAULT_NAME --name $KEYVAULT_KEY_NAME --query "[key.kid]" -o tsv)

create the DES and add permissions to use the KeyVault

az disk-encryption-set create -n $DISK_ENCRYPTION_SET_NAME -l $LOCATION -g $RESOURCEGROUP --source-vault $KEYVAULT_ID --key-url $KEYVAULT_KEY_URL

DES_IDENTITY=$(az disk-encryption-set show -n $DISK_ENCRYPTION_SET_NAME -g $RESOURCEGROUP --query "[identity.principalId]" -o tsv)

az keyvault set-policy -n $KEYVAULT_NAME -g $RESOURCEGROUP --object-id $DES_IDENTITY --key-permissions wrapkey unwrapkey get

run the az aro create command

az aro create  --resource-group $RESOURCEGROUP  --name $CLUSTER  --vnet aro-vnet  --master-subnet master-subnet   --worker-subnet worker-subnet --disk-encryption-set $DES_ID

The create should complete with Success. You can check the encryption parameters on the vm disks, they should show that the encryption is customer controlled and should point to the right disk-encryption-set.

Run az aro list -o table and az aro list, also the az aro show command should show exactly the same cluster document.
Run az aro list-credentials -g $RESOURCEGROUP --name $CLUSTER and see if the credentials work in the web console
Run az aro delete --resource-group $RESOURCEGROUP --name $CLUSTER --yes it should cleanly delete the cluster
Remove the DES and Keyvault

az disk-encryption-set delete -n $DISK_ENCRYPTION_SET_NAME -g $RESOURCEGROUP
az keyvault delete -n $KEYVAULT_NAME -g $RESOURCEGROUP

clean up any Vnets and Subnets created to for cluster provisioning.

Is there any documentation that needs to be updated for this PR?

docs/disk-encryption-set.md was added to describe how to provision a disk-encryption-set and create a cluster

Edit after some dicussions:

we also need a clear documentation in docs.microsoft.com both for the How-To create and use the Disk Encryption Set, how it interacts with the "encryption at host" settings and what are the customer's responsibilities (they customer should know from day 1, that if they fail to maintain the keys their whole cluster will be broken beyond repair)

…, sdn selection, encryption at host options

Makdaam · 2021-09-17T11:01:19Z

Just to keep all notes in once place:

tested on local dev RP
tested on prod RP in eastus (the preview API is live, it's just a matter of using it)

petrkotas

Very Good job. LGTM

petrkotas · 2021-09-17T13:29:38Z

python/az/aro/azext_aro/_params.py


+        c.argument('disk_encryption_set',


We discussed with @Makdaam that encryption parameters need a clear documentation on docs.microsoft.com.
disk_encryption_set is providing customer keys and makes customer responsible for managing them. Without this option set, disks will get encrypted with Azure provided and rotated keys.

This will be documented later on.

petrkotas · 2021-09-17T13:31:14Z

python/az/aro/azext_aro/custom.py

@@ -279,6 +289,13 @@ def get_network_resources(cli_ctx, subnets, vnet):
    return resources


+def get_disk_encryption_resources(oc):
+    disk_encryption_set = oc.master_profile.disk_encryption_set_id
+    resources = set()


After a discussion, the set() is used to keep code consistency even when there will be one object. This makes code more streamlined.

25region · 2021-09-21T19:11:25Z

Tidied up documentation a bit.

mjudeikis

Provisional approval for this. Few nit questions

python/az/aro/azext_aro/_params.py

mjudeikis · 2021-09-22T11:51:05Z

python/az/aro/azext_aro/_params.py

+                   help='ResourceID of the DiskEncryptionSet to be used for master and worker VMs.',
+                   validator=validate_disk_encryption_set)
+        c.argument('master_encryption_at_host', arg_type=get_enum_type(['Enabled', 'Disabled']),
+                   options_list=['--master-enc-at-host'],


is there reason we use --master-enc-at-host and not --master-encryption-at-host? Matches original flag name

+1 on using full name.

Also I see that az vm create uses --encryption-at-host argument as a boolean flag (either present or not, no value). If I read this code correctly our command is going to be --master-enc-at-host Enabled.

Maybe we also need to use it as boolean for simplicity? If present - send Enabled to the API, if not - send Disabled.

I had master-encryption-at-host, but that doesn't pass validation since the parameter name is too long.

Ok, found the linter rule, and at least one version of the parameter has to be <22 characters (not counting -- or - in front). I'm wondering if I should shorten the short versions even more.

To sum up:

added long names and shortened the short names a bit

changed the param type to tri-state (so that you don't have to do "Enabled")

removed param validator because it's not needed.

mjudeikis · 2021-09-22T11:53:48Z

python/az/aro/azext_aro/custom.py

@@ -42,7 +43,11 @@ def aro_create(cmd,  # pylint: disable=too-many-locals
               client_secret=None,
               pod_cidr=None,
               service_cidr=None,
+               software_defined_network=None,
+               disk_encryption_set=None,


API Accepts separate disk encryption sets for master and workers, should we not add 2 options here too?

@mjudeikis API does accept two valeus, but we decided to force worker encrpyion set id match master one for now (see code below).

ARO-RP/pkg/api/v20210901preview/openshiftcluster_validatestatic.go

Lines 292 to 294 in 1af1f2b

if !strings.EqualFold(mp.DiskEncryptionSetID, wp.DiskEncryptionSetID) {

return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, path+".subnetId", "The provided worker disk encryption set '%s' is invalid: must be the same as master disk encryption set '%s'.", wp.DiskEncryptionSetID, mp.DiskEncryptionSetID)

}

I don't worry much about implementation: I'm ok with aro_create accepting two separate params.

But if we are talking about exposing separate CLI params to the customers - I would be in favour of keeping 1 param for now to avoid confusion. If we decide to relax this validation - we will add a second param (--disk-encryption-set and new --master-disk-encryption-set) or rename existing and add new one (so we have, for example, --worker-disk-encryption-set and --master-disk-encryption-set).

If we decide to relax I'd leave the old --disk-encryption-set, and add 2 new (one for master, one for worker). Then throw an error if the old one is specified at the same time as one of the new ones. This will keep backwards compatibility if someone uses az aro in a script.

m1kola

We need to finish this PR as soon as possible to allow documentation work (need extension to be publisged to allow testing).

I think we need to address encryption at host params (shortened vs full name and boolean vs string balue) before we merge it.

The PR looks good overall.

python/az/aro/azext_aro/_params.py

m1kola · 2021-10-04T10:46:01Z

docs/disk-encryption-set.md

+az aro create --resource-group $RESOURCEGROUP \
+              --name $CLUSTER  \
+              --vnet aro-vnet  \
+              --master-subnet master-subnet \
+              --worker-subnet worker-subnet \
+              --disk-encryption-set $DES_ID


Are we covering disk encrpytion sets (server side encryption) only in this doc or do we also want to cover encryption at host?

To be honest I didn't add the --sdn-type documentation since the differences are described on docs.openshift.com.
I think the same applies to the encryption at host - we're just passing on a parameter to the compute RP, so the AzureDocs are the correct source here (and it's a bool flag, no setup steps or prerequisites required).

m1kola · 2021-10-04T10:50:32Z

python/az/aro/azext_aro/_params.py

+                   help='ResourceID of the DiskEncryptionSet to be used for master and worker VMs.',
+                   validator=validate_disk_encryption_set)
+        c.argument('master_encryption_at_host', arg_type=get_enum_type(['Enabled', 'Disabled']),
+                   options_list=['--master-enc-at-host'],


+1 on using full name.

Also I see that az vm create uses --encryption-at-host argument as a boolean flag (either present or not, no value). If I read this code correctly our command is going to be --master-enc-at-host Enabled.

Maybe we also need to use it as boolean for simplicity? If present - send Enabled to the API, if not - send Disabled.

python/az/aro/azext_aro/_validators.py

python/az/aro/azext_aro/custom.py

Makdaam · 2021-10-04T12:52:33Z

@m1kola @mjudeikis Changes applied, ready for review/more discussion.

python/az/aro/azext_aro/_params.py

python/az/aro/azext_aro/_validators.py

Adding support for the v2021-09-01-preview API - disk encryption sets…

0463556

…, sdn selection, encryption at host options

Makdaam added enhancement New feature or request ready-for-review size-medium Size medium labels Sep 15, 2021

Adding doc about how to use the disk-encryption-set

6d6c922

Makdaam marked this pull request as ready for review September 15, 2021 11:09

Makdaam requested review from jewzaam, m1kola and mjudeikis as code owners September 15, 2021 11:09

Shortening az parameter names, switching default SDN to OpenShiftSDN

417dad3

petrkotas previously approved these changes Sep 17, 2021

View reviewed changes

petrkotas reviewed Sep 17, 2021

View reviewed changes

Documentation cleanup

fd22ba2

25region dismissed petrkotas’s stale review via fd22ba2 September 21, 2021 19:10

mjudeikis previously approved these changes Sep 22, 2021

View reviewed changes

m1kola suggested changes Oct 4, 2021

View reviewed changes

m1kola added the next-up label Oct 4, 2021

Makdaam dismissed mjudeikis’s stale review via ee8cc1a October 4, 2021 12:21

Makdaam requested review from bennerv, hawkowl, mwoodson and rogbas as code owners October 4, 2021 12:21

fixing params and validation

b6451f3

m1kola reviewed Oct 4, 2021

View reviewed changes

python/az/aro/azext_aro/_params.py Outdated Show resolved Hide resolved

python/az/aro/azext_aro/_validators.py Outdated Show resolved Hide resolved

changed Exception messages and fixed a --help typo

d8d13bd

m1kola approved these changes Oct 4, 2021

View reviewed changes

m1kola merged commit aa13757 into Azure:master Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AzExt - Adding support for the v2021-09-01-preview API #1739

AzExt - Adding support for the v2021-09-01-preview API #1739

Makdaam commented Sep 14, 2021 •

edited

Loading

Makdaam commented Sep 17, 2021

petrkotas left a comment

petrkotas Sep 17, 2021

petrkotas Sep 17, 2021

25region commented Sep 21, 2021

mjudeikis left a comment

mjudeikis Sep 22, 2021

m1kola Oct 4, 2021

Makdaam Oct 4, 2021

Makdaam Oct 4, 2021

Makdaam Oct 4, 2021

mjudeikis Sep 22, 2021

m1kola Oct 4, 2021

Makdaam Oct 4, 2021

m1kola left a comment

m1kola Oct 4, 2021

Makdaam Oct 4, 2021

m1kola Oct 4, 2021

Makdaam commented Oct 4, 2021

	if !strings.EqualFold(mp.DiskEncryptionSetID, wp.DiskEncryptionSetID) {
	return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, path+".subnetId", "The provided worker disk encryption set '%s' is invalid: must be the same as master disk encryption set '%s'.", wp.DiskEncryptionSetID, mp.DiskEncryptionSetID)
	}

AzExt - Adding support for the v2021-09-01-preview API #1739

AzExt - Adding support for the v2021-09-01-preview API #1739

Conversation

Makdaam commented Sep 14, 2021 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

Makdaam commented Sep 17, 2021

petrkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

25region commented Sep 21, 2021

mjudeikis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makdaam commented Oct 4, 2021

Makdaam commented Sep 14, 2021 •

edited

Loading