Repository: Azure/azure-cli

Issue:
az storage account update encryption - syntax mismatch?
### Description

`az storage account update \
	--name ${storageacct} --resource-group ${res_group} \
	--encryption queue table blob file`

based on this in usage:

`[--encryption {queue,table,blob,file} [{queue,table,blob,file} ...]]`

results in 

`az storage account update: error: __init__() got an unexpected keyword argument 'queue'`

issueing them one at a time appears to work.

---

### Environment summary

**Install Method:** nightly
**CLI Version:** azure-cli (2.0.12+1.dev20170808)
**OS Version:** Ubuntu 16.04 x64
**Shell Type:** bash


Discussion:

Patch:
diff --git a/src/command_modules/azure-cli-storage/HISTORY.rst b/src/command_modules/azure-cli-storage/HISTORY.rst
index 3a69e8aa3..51989b349 100644
--- a/src/command_modules/azure-cli-storage/HISTORY.rst
+++ b/src/command_modules/azure-cli-storage/HISTORY.rst
@@ -3,6 +3,12 @@
 Release History
 ===============
 
+unreleased
+++++++++++
+* Enable service encryption by customer managed key
+* Breaking change: rename --encryption option to --encryption-services for az storage account create and az storage account update command.
+* Fix #4220: az storage account update encryption - syntax mismatch
+
 2.0.12 (2017-08-11)
 +++++++++++++++++++
 * Enable create storage account with system assigned identity
diff --git a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_params.py b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_params.py
index 0e159c576..157a5d77f 100644
--- a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_params.py
+++ b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_params.py
@@ -19,7 +19,7 @@ from azure.cli.core.commands import (register_cli_argument, register_extra_cli_a
 
 from azure.common import AzureMissingResourceHttpError
 
-from azure.cli.core.profiles import get_sdk, ResourceType
+from azure.cli.core.profiles import get_sdk, ResourceType, supported_api_version
 
 from ._factory import get_storage_data_service_client
 from ._validators import \
@@ -30,8 +30,8 @@ from ._validators import \
      validate_custom_domain, validate_public_access,
      process_blob_upload_batch_parameters, process_blob_download_batch_parameters,
      process_file_upload_batch_parameters, process_file_download_batch_parameters,
-     get_content_setting_validator, validate_encryption, validate_accept,
-     validate_key, storage_account_key_options,
+     get_content_setting_validator, validate_encryption_services, validate_accept,
+     validate_key, storage_account_key_options, validate_encryption_source,
      process_file_download_namespace,
      process_metric_update_namespace, process_blob_copy_batch_namespace,
      get_source_file_or_blob_service_client, process_blob_source_uri,
@@ -112,7 +112,6 @@ class CommandContext(object):
         max_api = kwargs.pop('max_api', None)
         min_api = kwargs.pop('min_api', None)
         if resource_type and (max_api or min_api):
-            from azure.cli.core.profiles import supported_api_version
             return supported_api_version(resource_type, min_api=min_api, max_api=max_api)
         return True
 
@@ -286,7 +285,6 @@ register_cli_argument('storage', 'if_match', arg_group='Pre-condition')
 register_cli_argument('storage', 'if_none_match', arg_group='Pre-condition')
 
 register_cli_argument('storage', 'container_name', container_name_type)
-
 for item in ['check-name', 'delete', 'list', 'show', 'show-usage', 'update', 'keys']:
     register_cli_argument('storage account {}'.format(item), 'account_name', account_name_type, options_list=('--name', '-n'))
 
@@ -300,21 +298,35 @@ for item in ['blob', 'file', 'queue', 'table']:
 def register_common_storage_account_options(context):
     context.reg_arg('https_only', help='Allows https traffic only to storage service.', **three_state_flag())
     context.reg_arg('sku', help='The storage account SKU.', **model_choice_list(ResourceType.MGMT_STORAGE, 'SkuName'))
-    context.reg_arg('assign_identity', help='Generate and assign a new Storage Account Identity for this storage '
-                                            'account for use with key management services like Azure KeyVault.',
-                    action='store_true', resource_type=ResourceType.MGMT_STORAGE, min_api='2017-06-01')
     context.reg_arg('access_tier', help='The access tier used for billing StandardBlob accounts. Cannot be set for '
                                         'StandardLRS, StandardGRS, StandardRAGRS, or PremiumLRS account types. It is '
                                         'required for StandardBlob accounts during creation',
                     **model_choice_list(ResourceType.MGMT_STORAGE, 'AccessTier'))
 
-    encryption_services_model = get_sdk(ResourceType.MGMT_STORAGE, 'models#EncryptionServices')
-    if encryption_services_model:
-        encryption_choices = list(encryption_services_model._attribute_map.keys())  # pylint: disable=protected-access
-        context.reg_arg('encryption', nargs='+', help='Specifies which service(s) to encrypt.',
-                        validator=validate_encryption,
-                        resource_type=ResourceType.MGMT_STORAGE, min_api='2016-12-01',
-                        **enum_choice_list(encryption_choices))
+    # after API 2016-12-01
+    if supported_api_version(resource_type=ResourceType.MGMT_STORAGE, min_api='2016-12-01'):
+        encryption_services_model = get_sdk(ResourceType.MGMT_STORAGE, 'models#EncryptionServices')
+        if encryption_services_model:
+
+            encryption_choices = []
+            for attribute in encryption_services_model._attribute_map.keys():  # pylint: disable=protected-access
+                if not encryption_services_model._validation.get(attribute, {}).get('readonly'):  # pylint: disable=protected-access
+                    # skip readonly attributes, which are not for input
+                    encryption_choices.append(attribute)
+
+            context.reg_arg('encryption_services', nargs='+', help='Specifies which service(s) to encrypt.',
+                            validator=validate_encryption_services, **enum_choice_list(encryption_choices))
+
+    # after API 2017-06-01
+    if supported_api_version(resource_type=ResourceType.MGMT_STORAGE, min_api='2017-06-01'):
+        context.reg_arg('assign_identity', action='store_true',
+                        help='Generate and assign a new Storage Account Identity for this storage account for use with '
+                             'key management services like Azure KeyVault.')
+
+        # the options of encryption key sources are hardcoded since there isn't a enum represents them in the SDK.
+        context.reg_arg('encryption_key_source', help='The encryption keySource (provider). Default: Microsoft.Storage',
+                        validator=validate_encryption_source,
+                        **enum_choice_list(['Microsoft.Storage', 'Microsoft.Keyvault']))
 
 
 with CommandContext('storage account create') as c:
@@ -338,6 +350,15 @@ with CommandContext('storage account update') as c:
               **enum_choice_list(['true', 'false']))
     c.reg_arg('tags', tags_type, default=None)
 
+    # after API 2017-06-01
+    if supported_api_version(resource_type=ResourceType.MGMT_STORAGE, min_api='2017-06-01'):
+        c.reg_arg('encryption_key_vault_properties', ignore_type)
+
+        with c.arg_group('Customer managed key') as g:
+            g.reg_extra_arg('encryption_key_name', help='The name of the KeyVault key.')
+            g.reg_extra_arg('encryption_key_vault', help='The Uri of the KeyVault')
+            g.reg_extra_arg('encryption_key_version', help='The version of the KeyVault key')
+
 
 register_cli_argument('storage account keys renew', 'key_name', options_list=('--key',), help='The key to regenerate.', validator=validate_key, **enum_choice_list(list(storage_account_key_options.keys())))
 register_cli_argument('storage account keys renew', 'account_name', account_name_type, id_part=None)
diff --git a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_validators.py b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_validators.py
index ffaddd11d..eb8363060 100644
--- a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_validators.py
+++ b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_validators.py
@@ -371,17 +371,43 @@ def validate_custom_domain(namespace):
         raise ValueError('usage error: --custom-domain DOMAIN [--use-subdomain]')
 
 
-def validate_encryption(namespace):
-    ''' Builds up the encryption object for storage account operations based on the
-    list of services passed in. '''
-    if namespace.encryption:
-        Encryption, EncryptionServices, EncryptionService = get_sdk(ResourceType.MGMT_STORAGE,
-                                                                    'Encryption',
-                                                                    'EncryptionServices',
-                                                                    'EncryptionService',
-                                                                    mod='models')
-        services = {service: EncryptionService(True) for service in namespace.encryption}
-        namespace.encryption = Encryption(EncryptionServices(**services))
+def validate_encryption_services(namespace):
+    """
+    Builds up the encryption services object for storage account operations based on the list of services passed in.
+    """
+    if namespace.encryption_services:
+        EncryptionServices, EncryptionService = get_sdk(
+            ResourceType.MGMT_STORAGE, 'EncryptionServices', 'EncryptionService', mod='models')
+        services = {service: EncryptionService(True) for service in namespace.encryption_services}
+
+        namespace.encryption_services = EncryptionServices(**services)
+
+
+def validate_encryption_source(namespace):
+    ns = vars(namespace)
+    if namespace.encryption_key_source:
+        allowed_options = ['Microsoft.Storage', 'Microsoft.Keyvault']
+        if namespace.encryption_key_source not in allowed_options:
+            raise ValueError('--encryption-key-source allows to values: {}'.format(', '.join(allowed_options)))
+
+    key_name = ns.pop('encryption_key_name', None)
+    key_version = ns.pop('encryption_key_version', None)
+    key_vault_uri = ns.pop('encryption_key_vault', None)
+
+    if namespace.encryption_key_source == 'Microsoft.Keyvault' and not (key_name and key_version and key_vault_uri):
+        raise ValueError('--encryption-key-name, --encryption-key-vault, and --encryption-key-version are required '
+                         'when --encryption-key-source=Microsoft.Keyvault is specified.')
+
+    if key_name or key_version or key_vault_uri:
+        if namespace.encryption_key_source != 'Microsoft.Keyvault':
+            raise ValueError('--encryption-key-name, --encryption-key-vault, and --encryption-key-version are not '
+                             'applicable when --encryption-key-source=Microsoft.Keyvault is not specified.')
+        KeyVaultProperties = get_sdk(ResourceType.MGMT_STORAGE, 'KeyVaultProperties', mod='models')
+        if not KeyVaultProperties:
+            return
+
+        kv_prop = KeyVaultProperties(key_name, key_version, key_vault_uri)
+        namespace.encryption_key_vault_properties = kv_prop
 
 
 def validate_entity(namespace):
diff --git a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/custom.py b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/custom.py
index 8ce90a73a..273fabe5c 100644
--- a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/custom.py
+++ b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/custom.py
@@ -40,8 +40,8 @@ def _update_progress(current, total):
 # CUSTOM METHODS
 
 def create_storage_account(resource_group_name, account_name, sku, assign_identity=False, location=None, kind=None,
-                           tags=None, custom_domain=None, encryption=None, access_tier=None, https_only=None):
-    StorageAccountCreateParameters, Kind, Sku, CustomDomain, AccessTier, Identity = get_sdk(
+                           encryption_services=None, tags=None, custom_domain=None, access_tier=None, https_only=None):
+    StorageAccountCreateParameters, Kind, Sku, CustomDomain, AccessTier, Identity, Encryption = get_sdk(
         ResourceType.MGMT_STORAGE,
         'StorageAccountCreateParameters',
         'Kind',
@@ -49,13 +49,14 @@ def create_storage_account(resource_group_name, account_name, sku, assign_identi
         'CustomDomain',
         'AccessTier',
         'Identity',
+        'Encryption',
         mod='models')
     scf = storage_client_factory()
     params = StorageAccountCreateParameters(sku=Sku(sku), kind=Kind(kind), location=location, tags=tags)
     if custom_domain:
         params.custom_domain = CustomDomain(custom_domain, None)
-    if encryption:
-        params.encryption = encryption
+    if encryption_services:
+        params.encryption = Encryption(services=encryption_services)
     if access_tier:
         params.access_tier = AccessTier(access_tier)
     if assign_identity:
@@ -77,15 +78,17 @@ def create_storage_account_with_account_type(resource_group_name, account_name,
     return scf.storage_accounts.create(resource_group_name, account_name, params)
 
 
-def update_storage_account(instance, sku=None, tags=None, custom_domain=None, use_subdomain=None, encryption=None,
+def update_storage_account(instance, sku=None, tags=None, custom_domain=None, use_subdomain=None,
+                           encryption_services=None, encryption_key_source=None, encryption_key_vault_properties=None,
                            access_tier=None, https_only=None, assign_identity=False):
-    StorageAccountUpdateParameters, Sku, CustomDomain, AccessTier, Identity = get_sdk(
+    StorageAccountUpdateParameters, Sku, CustomDomain, AccessTier, Identity, Encryption = get_sdk(
         ResourceType.MGMT_STORAGE,
         'StorageAccountUpdateParameters',
         'Sku',
         'CustomDomain',
         'AccessTier',
         'Identity',
+        'Encryption',
         mod='models')
     domain = instance.custom_domain
     if custom_domain is not None:
@@ -93,11 +96,19 @@ def update_storage_account(instance, sku=None, tags=None, custom_domain=None, us
         if use_subdomain is not None:
             domain.name = use_subdomain == 'true'
 
+    encryption = instance.encryption or Encryption()
+    if encryption_services:
+        encryption.services = encryption_services
+    if encryption_key_source:
+        encryption.key_source = encryption_key_source
+    if encryption_key_vault_properties:
+        encryption.key_vault_properties = encryption_key_vault_properties
+
     params = StorageAccountUpdateParameters(
         sku=Sku(sku) if sku is not None else instance.sku,
         tags=tags if tags is not None else instance.tags,
         custom_domain=domain,
-        encryption=encryption if encryption is not None else instance.encryption,
+        encryption=encryption,
         access_tier=AccessTier(access_tier) if access_tier is not None else instance.access_tier,
         enable_https_traffic_only=https_only if https_only is not None else instance.enable_https_traffic_only
     )


Test patch:
diff --git a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/tests/test_storage_validators.py b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/tests/test_storage_validators.py
index cfd57ca27..92da19ef3 100644
--- a/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/tests/test_storage_validators.py
+++ b/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/tests/test_storage_validators.py
@@ -6,14 +6,14 @@
 import unittest
 from argparse import Namespace
 from six import StringIO
-from azure.cli.command_modules.storage._validators import (
-    get_permission_validator, get_datetime_type, datetime, ipv4_range_type, resource_type_type, services_type,
-    process_blob_source_uri, get_char_options_validator)
+
+from azure.cli.command_modules.storage._validators import (get_permission_validator, get_datetime_type, datetime,
+                                                           ipv4_range_type, resource_type_type, services_type,
+                                                           process_blob_source_uri, get_char_options_validator)
 from azure.cli.core.profiles import get_sdk, ResourceType
 from azure.cli.testsdk import api_version_constraint
 
 
-@api_version_constraint(ResourceType.MGMT_STORAGE, min_api='2016-12-01')
 class TestStorageValidators(unittest.TestCase):
     def setUp(self):
         self.io = StringIO()
@@ -116,5 +116,59 @@ class TestStorageValidators(unittest.TestCase):
         self.assertEqual(result, set('ab'))
 
 
+@api_version_constraint(resource_type=ResourceType.MGMT_STORAGE, min_api='2016-12-01')
+class TestEncryptionValidators(unittest.TestCase):
+    def test_validate_encryption_services(self):
+        from azure.cli.command_modules.storage._validators import validate_encryption_services
+
+        ns = Namespace(encryption_services=['blob'])
+        validate_encryption_services(ns)
+        self.assertIsNotNone(ns.encryption_services.blob)
+        self.assertTrue(ns.encryption_services.blob.enabled)
+        self.assertIsNone(ns.encryption_services.file)
+
+        ns = Namespace(encryption_services=['file'])
+        validate_encryption_services(ns)
+        self.assertIsNotNone(ns.encryption_services.file)
+        self.assertTrue(ns.encryption_services.file.enabled)
+        self.assertIsNone(ns.encryption_services.blob)
+
+        ns = Namespace(encryption_services=['blob', 'file'])
+        validate_encryption_services(ns)
+        self.assertIsNotNone(ns.encryption_services.blob)
+        self.assertTrue(ns.encryption_services.blob.enabled)
+        self.assertIsNotNone(ns.encryption_services.file)
+        self.assertTrue(ns.encryption_services.file.enabled)
+
+    def test_validate_encryption_source(self):
+        from azure.cli.command_modules.storage._validators import validate_encryption_source
+
+        with self.assertRaises(ValueError):
+            validate_encryption_source(Namespace(encryption_key_source='Notanoption'))
+
+        with self.assertRaises(ValueError):
+            validate_encryption_source(Namespace(encryption_key_source='Microsoft.Keyvault'))
+
+        with self.assertRaises(ValueError):
+            validate_encryption_source(Namespace(encryption_key_source='Microsoft.Storage',
+                                                 encryption_key_name='key_name',
+                                                 encryption_key_version='key_version',
+                                                 encryption_key_vault='https://example.com/key_uri'))
+
+        ns = Namespace(encryption_key_source='Microsoft.Keyvault',
+                       encryption_key_name='key_name',
+                       encryption_key_version='key_version',
+                       encryption_key_vault='https://example.com/key_uri')
+        validate_encryption_source(ns)
+        self.assertFalse(hasattr(ns, 'encryption_key_name'))
+        self.assertFalse(hasattr(ns, 'encryption_key_version'))
+        self.assertFalse(hasattr(ns, 'encryption_key_uri'))
+
+        properties = ns.encryption_key_vault_properties
+        self.assertEqual(properties.key_name, 'key_name')
+        self.assertEqual(properties.key_version, 'key_version')
+        self.assertEqual(properties.key_vault_uri, 'https://example.com/key_uri')
+
+
 if __name__ == '__main__':
     unittest.main()