-
Notifications
You must be signed in to change notification settings - Fork 191
AzExt - Adding support for the v2021-09-01-preview API #1739
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
Changes from all commits
0463556
6d6c922
417dad3
fd22ba2
b6451f3
d8d13bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # Using a custom Disk Encryption Set | ||
|
|
||
| ## What is the Disk Encryption Set used for? | ||
|
|
||
| In summary, it allows a customer to control the keys that are used to encrypt/decrypt VM disks. | ||
| See [deploy-a-vm-with-customer-managed-keys](https://docs.microsoft.com/en-us/azure/virtual-machines/disks-enable-host-based-encryption-portal#deploy-a-vm-with-customer-managed-keys) for more information. | ||
|
|
||
| ## How to deploy? | ||
| First, install and use the AzureCLI extension with | ||
| ```bash | ||
| make az | ||
| ``` | ||
|
|
||
| >You can check if the extension is in use by running: | ||
| ```bash | ||
| az extension list | ||
| [ | ||
| { | ||
| "experimental": false, | ||
| "extensionType": "dev", | ||
| "name": "aro", | ||
| "path": "<path to go SRC>/github.com/Azure/ARO-RP/python/az/aro", | ||
| "preview": true, | ||
| "version": "1.0.1" | ||
| } | ||
| ] | ||
| ``` | ||
|
|
||
| Follow [tutorial-create-cluster](https://docs.microsoft.com/en-us/azure/openshift/tutorial-create-cluster) but don't run the `az aro create` command, instead proceed as follows: | ||
|
|
||
| - set additional env variables | ||
| ```bash | ||
| export KEYVAULT_NAME=$USER-enckv | ||
| export KEYVAULT_KEY_NAME=$USER-key | ||
| export DISK_ENCRYPTION_SET_NAME=$USER-des | ||
| ``` | ||
| - create the KeyVault and Key | ||
| ```bash | ||
| 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 | ||
| ```bash | ||
| 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 | ||
| ```bash | ||
| az aro create --resource-group $RESOURCEGROUP \ | ||
| --name $CLUSTER \ | ||
| --vnet aro-vnet \ | ||
| --master-subnet master-subnet \ | ||
| --worker-subnet worker-subnet \ | ||
| --disk-encryption-set $DES_ID | ||
| ``` | ||
|
|
||
| After creating the cluster all VMs should have the customer controlled Disk Encryption Set. | ||
| >Remember to delete the disk-encryption-set and keyvault when done. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,3 +25,7 @@ Release History | |
| 1.0.0 | ||
| ++++++ | ||
| * Remove preview flag. | ||
|
|
||
| 1.0.1 | ||
| ++++++ | ||
| * Switch to new preview API | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,10 @@ | |
| from azext_aro._validators import validate_cidr | ||
| from azext_aro._validators import validate_client_id | ||
| from azext_aro._validators import validate_cluster_resource_group | ||
| from azext_aro._validators import validate_disk_encryption_set | ||
| from azext_aro._validators import validate_domain | ||
| from azext_aro._validators import validate_pull_secret | ||
| from azext_aro._validators import validate_sdn | ||
| from azext_aro._validators import validate_subnet | ||
| from azext_aro._validators import validate_client_secret | ||
| from azext_aro._validators import validate_visibility | ||
|
|
@@ -54,10 +56,23 @@ def load_arguments(self, _): | |
| c.argument('service_cidr', | ||
| help='CIDR of service network. Must be a minimum of /18 or larger.', | ||
| validator=validate_cidr('service_cidr')) | ||
| c.argument('software_defined_network', arg_type=get_enum_type(['OVNKubernetes', 'OpenShiftSDN']), | ||
| options_list=['--software-defined-network-type', '--sdn-type'], | ||
| help='SDN type either "OpenShiftSDN" (default) or "OVNKubernetes"', | ||
| validator=validate_sdn) | ||
|
|
||
| c.argument('disk_encryption_set', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed with @Makdaam that encryption parameters need a clear documentation on docs.microsoft.com. This will be documented later on. |
||
| 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_three_state_flag(), | ||
| options_list=['--master-encryption-at-host', '--master-enc-host'], | ||
| help='Encryption at host flag for master VMs.') | ||
| c.argument('master_vm_size', | ||
| help='Size of master VMs.') | ||
|
|
||
| c.argument('worker_encryption_at_host', arg_type=get_three_state_flag(), | ||
| options_list=['--worker-encryption-at-host', '--worker-enc-host'], | ||
| help='Encryption at host flag for worker VMs.') | ||
| c.argument('worker_vm_size', | ||
| help='Size of worker VMs.') | ||
| c.argument('worker_vm_disk_size_gb', | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,10 +15,11 @@ | |||||||
| from msrest.exceptions import HttpOperationError | ||||||||
| from knack.log import get_logger | ||||||||
|
|
||||||||
| import azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2020_04_30.models as openshiftcluster | ||||||||
| import azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2021_09_01_preview.models as openshiftcluster | ||||||||
|
|
||||||||
| from azext_aro._aad import AADManager | ||||||||
| from azext_aro._rbac import assign_network_contributor_to_resource, has_network_contributor_on_resource | ||||||||
| from azext_aro._rbac import assign_role_to_resource, has_role_assignment_on_resource | ||||||||
| from azext_aro._rbac import ROLE_NETWORK_CONTRIBUTOR, ROLE_READER | ||||||||
| from azext_aro._validators import validate_subnets | ||||||||
|
|
||||||||
| logger = get_logger(__name__) | ||||||||
|
|
@@ -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, | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API Accepts separate disk encryption sets for master and workers, should we not add 2 options here too?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
I don't worry much about implementation: I'm ok with 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 (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
| master_encryption_at_host=False, | ||||||||
| master_vm_size=None, | ||||||||
| worker_encryption_at_host=False, | ||||||||
| worker_vm_size=None, | ||||||||
| worker_vm_disk_size_gb=None, | ||||||||
| worker_count=None, | ||||||||
|
|
@@ -104,10 +109,13 @@ def aro_create(cmd, # pylint: disable=too-many-locals | |||||||
| network_profile=openshiftcluster.NetworkProfile( | ||||||||
| pod_cidr=pod_cidr or '10.128.0.0/14', | ||||||||
| service_cidr=service_cidr or '172.30.0.0/16', | ||||||||
| software_defined_network=software_defined_network or 'OpenShiftSDN' | ||||||||
| ), | ||||||||
| master_profile=openshiftcluster.MasterProfile( | ||||||||
| vm_size=master_vm_size or 'Standard_D8s_v3', | ||||||||
| subnet_id=master_subnet, | ||||||||
| encryption_at_host='Enabled' if master_encryption_at_host else 'Disabled', | ||||||||
| disk_encryption_set_id=disk_encryption_set, | ||||||||
| ), | ||||||||
| worker_profiles=[ | ||||||||
| openshiftcluster.WorkerProfile( | ||||||||
|
|
@@ -116,6 +124,8 @@ def aro_create(cmd, # pylint: disable=too-many-locals | |||||||
| disk_size_gb=worker_vm_disk_size_gb or 128, | ||||||||
| subnet_id=worker_subnet, | ||||||||
| count=worker_count or 3, | ||||||||
| encryption_at_host='Enabled' if worker_encryption_at_host else 'Disabled', | ||||||||
| disk_encryption_set_id=disk_encryption_set, | ||||||||
| ) | ||||||||
| ], | ||||||||
| apiserver_profile=openshiftcluster.APIServerProfile( | ||||||||
|
|
@@ -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() | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After a discussion, the set() is used to keep code consistency even when there will be one object. This makes code more streamlined. |
||||||||
| resources.add(disk_encryption_set) | ||||||||
| return resources | ||||||||
|
|
||||||||
|
|
||||||||
| # cluster_application_update manages cluster application & service principal update | ||||||||
| # If called without parameters it should be best-effort | ||||||||
| # If called with parameters it fails if something is not possible | ||||||||
|
|
@@ -363,8 +380,9 @@ def resolve_rp_client_id(): | |||||||
|
|
||||||||
| def ensure_resource_permissions(cli_ctx, oc, fail, sp_obj_ids): | ||||||||
| try: | ||||||||
| # Get cluster resources we need to assign network contributor on | ||||||||
| resources = get_cluster_network_resources(cli_ctx, oc) | ||||||||
| # Get cluster resources we need to assign permissions on, sort to ensure the same order of operations | ||||||||
| resources = {ROLE_NETWORK_CONTRIBUTOR: sorted(get_cluster_network_resources(cli_ctx, oc)), | ||||||||
| ROLE_READER: sorted(get_disk_encryption_resources(oc))} | ||||||||
Makdaam marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| except (CloudError, HttpOperationError) as e: | ||||||||
| if fail: | ||||||||
| logger.error(e.message) | ||||||||
|
|
@@ -373,18 +391,19 @@ def ensure_resource_permissions(cli_ctx, oc, fail, sp_obj_ids): | |||||||
| return | ||||||||
|
|
||||||||
| for sp_id in sp_obj_ids: | ||||||||
| for resource in sorted(resources): | ||||||||
| # Create the role assignment if it doesn't exist | ||||||||
| # Assume that the role assignment exists if we fail to look it up | ||||||||
| resource_contributor_exists = True | ||||||||
|
|
||||||||
| try: | ||||||||
| resource_contributor_exists = has_network_contributor_on_resource(cli_ctx, resource, sp_id) | ||||||||
| except CloudError as e: | ||||||||
| if fail: | ||||||||
| logger.error(e.message) | ||||||||
| raise | ||||||||
| logger.info(e.message) | ||||||||
|
|
||||||||
| if not resource_contributor_exists: | ||||||||
| assign_network_contributor_to_resource(cli_ctx, resource, sp_id) | ||||||||
| for role in sorted(resources): | ||||||||
| for resource in resources[role]: | ||||||||
| # Create the role assignment if it doesn't exist | ||||||||
| # Assume that the role assignment exists if we fail to look it up | ||||||||
| resource_contributor_exists = True | ||||||||
|
|
||||||||
| try: | ||||||||
| resource_contributor_exists = has_role_assignment_on_resource(cli_ctx, resource, sp_id, role) | ||||||||
| except CloudError as e: | ||||||||
| if fail: | ||||||||
| logger.error(e.message) | ||||||||
| raise | ||||||||
| logger.info(e.message) | ||||||||
|
|
||||||||
| if not resource_contributor_exists: | ||||||||
| assign_role_to_resource(cli_ctx, resource, sp_id, role) | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we covering disk encrpytion sets (server side encryption) only in this doc or do we also want to cover encryption at host?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).