{AKS} Update validator for cluster auto-scaler profile (#20845)

* update validator for cluster_autoscaler_profile

* remove old validator for cluster autoscaler profile

* fix lint & sort import

Author: FumingZhang

src/azure-cli/azure/cli/command_modules/acs/_params.py

@@ -18,7 +18,7 @@
 from ._completers import (
     get_vm_size_completion_list, get_k8s_versions_completion_list, get_k8s_upgrades_completion_list, get_ossku_completion_list)
 from ._validators import (
-    validate_cluster_autoscaler_profile, validate_create_parameters, validate_kubectl_version, validate_kubelogin_version, validate_k8s_version, validate_linux_host_name,
+    validate_create_parameters, validate_kubectl_version, validate_kubelogin_version, validate_k8s_version, validate_linux_host_name,
     validate_list_of_integers, validate_ssh_key, validate_nodes_count,
     validate_nodepool_name, validate_vm_set_type, validate_load_balancer_sku, validate_load_balancer_outbound_ips,
     validate_priority, validate_eviction_policy, validate_spot_max_price,
@@ -234,8 +234,7 @@ def load_arguments(self, _):
         c.argument('outbound_type', arg_type=get_enum_type(outbound_types))
         c.argument('auto_upgrade_channel', arg_type=get_enum_type(auto_upgrade_channels))
         c.argument('enable_cluster_autoscaler', action='store_true')
-        # TODO: replace "validate_cluster_autoscaler_profile" with "_extract_cluster_autoscaler_params"
-        c.argument('cluster_autoscaler_profile', nargs='+', options_list=["--cluster-autoscaler-profile", "--ca-profile"], validator=validate_cluster_autoscaler_profile,
+        c.argument('cluster_autoscaler_profile', nargs='+', options_list=["--cluster-autoscaler-profile", "--ca-profile"],
                    help="Space-separated list of key=value pairs for configuring cluster autoscaler. Pass an empty string to clear the profile.")
         c.argument('min_count', type=int, validator=validate_nodes_count)
         c.argument('max_count', type=int, validator=validate_nodes_count)
@@ -316,8 +315,7 @@ def load_arguments(self, _):
                    "--disable-cluster-autoscaler", "-d"], action='store_true')
         c.argument('update_cluster_autoscaler', options_list=[
                    "--update-cluster-autoscaler", "-u"], action='store_true')
-        # TODO: replace "validate_cluster_autoscaler_profile" with "_extract_cluster_autoscaler_params
-        c.argument('cluster_autoscaler_profile', nargs='+', options_list=["--cluster-autoscaler-profile", "--ca-profile"], validator=validate_cluster_autoscaler_profile,
+        c.argument('cluster_autoscaler_profile', nargs='+', options_list=["--cluster-autoscaler-profile", "--ca-profile"],
                    help="Space-separated list of key=value pairs for configuring cluster autoscaler. Pass an empty string to clear the profile.")
         c.argument('min_count', type=int, validator=validate_nodes_count)
         c.argument('max_count', type=int, validator=validate_nodes_count)

src/azure-cli/azure/cli/command_modules/acs/_validators.py

@@ -4,22 +4,19 @@
 # --------------------------------------------------------------------------------------------
 
 from __future__ import unicode_literals
+
 import os
 import os.path
 import re
-from math import isnan, isclose
 from ipaddress import ip_network
+from math import isclose, isnan
 
-# pylint: disable=no-name-in-module,import-error
-from knack.log import get_logger
-
-from azure.cli.core.profiles import ResourceType
-
+from azure.cli.core import keys
+from azure.cli.core.azclierror import InvalidArgumentValueError
 from azure.cli.core.commands.validators import validate_tag
 from azure.cli.core.util import CLIError
-from azure.cli.core.azclierror import InvalidArgumentValueError
-from azure.cli.core import keys
-
+# pylint: disable=no-name-in-module,import-error
+from knack.log import get_logger
 
 logger = get_logger(__name__)
 
@@ -108,39 +105,6 @@ def validate_k8s_version(namespace):
                            'such as "1.11.8" or "1.12.6"')
 
 
-def validate_cluster_autoscaler_profile(cmd, namespace):
-    """ Validates that cluster autoscaler profile is acceptable by:
-        1. Extracting the key[=value] format to map
-        2. Validating that the key isn't empty and that the key is valid
-        Empty strings pass validation
-    """
-    _extract_cluster_autoscaler_params(namespace)
-    if namespace.cluster_autoscaler_profile is not None:
-        for key in namespace.cluster_autoscaler_profile.keys():
-            _validate_cluster_autoscaler_key(cmd, key)
-
-
-def _validate_cluster_autoscaler_key(cmd, key):
-    if not key:
-        raise CLIError('Empty key specified for cluster-autoscaler-profile')
-    ManagedClusterPropertiesAutoScalerProfile = cmd.get_models('ManagedClusterPropertiesAutoScalerProfile',
-                                                               resource_type=ResourceType.MGMT_CONTAINERSERVICE,
-                                                               operation_group='managed_clusters')
-    valid_keys = list(k.replace("_", "-") for k, v in ManagedClusterPropertiesAutoScalerProfile._attribute_map.items())  # pylint: disable=protected-access
-    if key not in valid_keys:
-        raise CLIError("'{0}' is an invalid key for cluster-autoscaler-profile. "
-                       "Valid keys are {1}.".format(key, ', '.join(valid_keys)))
-
-
-def _extract_cluster_autoscaler_params(namespace):
-    """ Extracts multiple space-separated cluster autoscaler parameters in key[=value] format """
-    if isinstance(namespace.cluster_autoscaler_profile, list):
-        params_dict = {}
-        for item in namespace.cluster_autoscaler_profile:
-            params_dict.update(validate_tag(item))
-        namespace.cluster_autoscaler_profile = params_dict
-
-
 def validate_nodepool_name(namespace):
     """Validates a nodepool name to be at most 12 characters, alphanumeric only."""
     if namespace.nodepool_name != "":
@@ -436,16 +400,18 @@ def extract_comma_separated_string(
     raw_string,
     enable_strip=False,
     extract_kv=False,
-    key_only=False,
+    allow_empty_value=False,
     keep_none=False,
     default_value=None,
 ):
     """Extract comma-separated string.
 
     If enable_strip is specified, will remove leading and trailing whitespace before each operation on the string.
-    If extract_kv is specified, will extract key value pair from the string, otherwise keep the entire string.
-    Option key_only is valid since extract_kv is specified, when the number of string segments split by "=" is not 2,
-    only the first segment is retained as the key.
+    If extract_kv is specified, will extract key value pairs from the string with "=" as the delimiter and this would
+    return a dictionary, otherwise keep the entire string.
+    Option allow_empty_value is valid since extract_kv is specified. When the number of string segments split by "="
+    is 1, the first segment is retained as the key and empty string would be set as its corresponding value without
+    raising an exception.
     If keep_none is specified, will return None when input is None. Otherwise will return default_value if input is
     None or empty string.
     """
@@ -463,25 +429,29 @@ def extract_comma_separated_string(
         if enable_strip:
             item = item.strip()
         if extract_kv:
-            kv = item.split("=")
-            if key_only:
-                if enable_strip:
-                    result[kv[0].strip()] = ""
-                else:
-                    result[kv[0]] = ""
-            else:
-                if len(kv) == 2:
-                    if enable_strip:
-                        result[kv[0].strip()] = kv[1].strip()
-                    else:
-                        result[kv[0]] = kv[1]
-                else:
+            kv_list = item.split("=")
+            if len(kv_list) in [1, 2]:
+                key = kv_list[0]
+                value = ""
+                if len(kv_list) == 2:
+                    value = kv_list[1]
+                if not allow_empty_value and (value == "" or value.isspace()):
                     raise InvalidArgumentValueError(
-                        "The format of '{}' in '{}' is incorrect, correct format should be "
-                        "'Key1=Value1,Key2=Value2'.".format(
-                            item, raw_string
+                        "Empty value not allowed. The value '{}' of key '{}' in '{}' is empty. Raw input '{}'.".format(
+                            value, key, item, raw_string
                         )
                     )
+                if enable_strip:
+                    key = key.strip()
+                    value = value.strip()
+                result[key] = value
+            else:
+                raise InvalidArgumentValueError(
+                    "The format of '{}' in '{}' is incorrect, correct format should be "
+                    "'Key1=Value1,Key2=Value2'.".format(
+                        item, raw_string
+                    )
+                )
         else:
             result.append(item)
     return result

src/azure-cli/azure/cli/command_modules/acs/decorator.py

@@ -529,6 +529,56 @@ def __validate_counts_in_autoscaler(
                     )
                 )
 
+    def __validate_cluster_autoscaler_profile(
+        self, cluster_autoscaler_profile: Union[List, Dict, None]
+    ) -> Union[Dict, None]:
+        """Helper function to parse and verify cluster_autoscaler_profile.
+
+        If the user input is a list, parse it with function "extract_comma_separated_string". If the type of user input
+        or parsed value is not a dictionary, raise an InvalidArgumentValueError. Otherwise, take the keys from the
+        attribute map of ManagedClusterPropertiesAutoScalerProfile to verify whether the keys in the key-value pairs
+        provided by the user are valid. If not, raise an InvalidArgumentValueError.
+
+        :return: dictionary or None
+        """
+        if cluster_autoscaler_profile is not None:
+            # convert list to dict
+            if isinstance(cluster_autoscaler_profile, list):
+                params_dict = {}
+                for item in cluster_autoscaler_profile:
+                    params_dict.update(
+                        extract_comma_separated_string(
+                            item,
+                            extract_kv=True,
+                            allow_empty_value=True,
+                            default_value={},
+                        )
+                    )
+                cluster_autoscaler_profile = params_dict
+            # check if the type is dict
+            if not isinstance(cluster_autoscaler_profile, dict):
+                raise InvalidArgumentValueError(
+                    "Unexpected input cluster-autoscaler-profile, value: '{}', type '{}'.".format(
+                        cluster_autoscaler_profile,
+                        type(cluster_autoscaler_profile),
+                    )
+                )
+            # verify keys
+            # pylint: disable=protected-access
+            valid_keys = list(
+                k.replace("_", "-") for k in self.models.ManagedClusterPropertiesAutoScalerProfile._attribute_map.keys()
+            )
+            for key in cluster_autoscaler_profile.keys():
+                if not key:
+                    raise InvalidArgumentValueError("Empty key specified for cluster-autoscaler-profile")
+                if key not in valid_keys:
+                    raise InvalidArgumentValueError(
+                        "'{}' is an invalid key for cluster-autoscaler-profile. Valid keys are {}.".format(
+                            key, ", ".join(valid_keys)
+                        )
+                    )
+        return cluster_autoscaler_profile
+
     def get_subscription_id(self):
         """Helper function to obtain the value of subscription_id.
 
@@ -4077,42 +4127,20 @@ def get_node_osdisk_diskencryptionset_id(self) -> Union[str, None]:
     def _get_cluster_autoscaler_profile(self, read_only: bool = False) -> Union[Dict[str, str], None]:
         """Internal function to dynamically obtain the value of cluster_autoscaler_profile according to the context.
 
+        This function will call function "__validate_cluster_autoscaler_profile" to parse and verify the parameter
+        by default.
+
         In update mode, when cluster_autoscaler_profile is assigned and auto_scaler_profile in the `mc` object has also
         been set, dynamic completion will be triggerd. We will first make a copy of the original configuration
         (extract the dictionary from the ManagedClusterPropertiesAutoScalerProfile object), and then update the copied
         dictionary with the dictionary of new options.
 
-        This function will verify the parameter by default. If the user input is not empty, take the keys from the
-        attribute map of ManagedClusterPropertiesAutoScalerProfile to verify whether the keys in the key-value pairs
-        provided by the user are valid. If not, raise an InvalidArgumentValueError. The value of the raw parameter
-        should be a dictionary after being processed by the validator. If not, raise a CLIInternalError.
-
         :return: dictionary or None
         """
         # read the original value passed by the command
         cluster_autoscaler_profile = self.raw_param.get("cluster_autoscaler_profile")
-        # validate user input (replace function "_validate_cluster_autoscaler_key" in file "_validators.py")
-        if cluster_autoscaler_profile is not None:
-            if not isinstance(cluster_autoscaler_profile, dict):
-                raise CLIInternalError(
-                    "Unexpected input cluster-autoscaler-profile, value: '{}', type '{}'.".format(
-                        cluster_autoscaler_profile,
-                        type(cluster_autoscaler_profile),
-                    )
-                )
-            # pylint: disable=protected-access
-            valid_keys = list(
-                k.replace("_", "-") for k in self.models.ManagedClusterPropertiesAutoScalerProfile._attribute_map.keys()
-            )
-            for key in cluster_autoscaler_profile.keys():
-                if not key:
-                    raise InvalidArgumentValueError("Empty key specified for cluster-autoscaler-profile")
-                if key not in valid_keys:
-                    raise InvalidArgumentValueError(
-                        "'{}' is an invalid key for cluster-autoscaler-profile. Valid keys are {}.".format(
-                            key, ", ".join(valid_keys)
-                        )
-                    )
+        # parse and validate user input
+        cluster_autoscaler_profile = self.__validate_cluster_autoscaler_profile(cluster_autoscaler_profile)
 
         # In create mode, try to read the property value corresponding to the parameter from the `mc` object.
         if self.decorator_mode == DecoratorMode.CREATE:
@@ -4141,16 +4169,14 @@ def _get_cluster_autoscaler_profile(self, read_only: bool = False) -> Union[Dict
     def get_cluster_autoscaler_profile(self) -> Union[Dict[str, str], None]:
         """Dynamically obtain the value of cluster_autoscaler_profile according to the context.
 
+        This function will call function "__validate_cluster_autoscaler_profile" to parse and verify the parameter
+        by default.
+
         In update mode, when cluster_autoscaler_profile is assigned and auto_scaler_profile in the `mc` object has also
         been set, dynamic completion will be triggerd. We will first make a copy of the original configuration
         (extract the dictionary from the ManagedClusterPropertiesAutoScalerProfile object), and then update the copied
         dictionary with the dictionary of new options.
 
-        This function will verify the parameter by default. If the user input is not empty, take the keys from the
-        attribute map of ManagedClusterPropertiesAutoScalerProfile to verify whether the keys in the key-value pairs
-        provided by the user are valid. If not, raise an InvalidArgumentValueError. The value of the raw parameter
-        should be a dictionary after being processed by the validator. If not, raise a CLIInternalError.
-
         :return: dictionary or None
         """
         return self._get_cluster_autoscaler_profile()

src/azure-cli/azure/cli/command_modules/acs/tests/hybrid_2020_09_01/test_validators.py

@@ -96,59 +96,6 @@ def test_IPv6(self):
         self.assertEqual(str(cm.exception), err)
 
 
-class TestClusterAutoscalerParamsValidators(unittest.TestCase):
-    def setUp(self):
-        self.cli = MockCLI()
-
-    def test_empty_key_empty_value(self):
-        cluster_autoscaler_profile = ["="]
-        namespace = Namespace(cluster_autoscaler_profile=cluster_autoscaler_profile)
-        err = "Empty key specified for cluster-autoscaler-profile"
-
-        with self.assertRaises(CLIError) as cm:
-            validators.validate_cluster_autoscaler_profile(MockCmd(self.cli), namespace)
-        self.assertEqual(str(cm.exception), err)
-
-    def test_non_empty_key_empty_value(self):
-        cluster_autoscaler_profile = ["scan-interval="]
-        namespace = Namespace(cluster_autoscaler_profile=cluster_autoscaler_profile)
-
-        validators.validate_cluster_autoscaler_profile(MockCmd(self.cli), namespace)
-
-    def test_two_empty_keys_empty_value(self):
-        cluster_autoscaler_profile = ["=", "="]
-        namespace = Namespace(cluster_autoscaler_profile=cluster_autoscaler_profile)
-        err = "Empty key specified for cluster-autoscaler-profile"
-
-        with self.assertRaises(CLIError) as cm:
-            validators.validate_cluster_autoscaler_profile(MockCmd(self.cli), namespace)
-        self.assertEqual(str(cm.exception), err)
-
-    def test_one_empty_key_in_pair_one_non_empty(self):
-        cluster_autoscaler_profile = ["scan-interval=20s", "="]
-        namespace = Namespace(cluster_autoscaler_profile=cluster_autoscaler_profile)
-        err = "Empty key specified for cluster-autoscaler-profile"
-
-        with self.assertRaises(CLIError) as cm:
-            validators.validate_cluster_autoscaler_profile(MockCmd(self.cli), namespace)
-        self.assertEqual(str(cm.exception), err)
-
-    def test_invalid_key(self):
-        cluster_autoscaler_profile = ["bad-key=val"]
-        namespace = Namespace(cluster_autoscaler_profile=cluster_autoscaler_profile)
-        err = "'bad-key' is an invalid key for cluster-autoscaler-profile"
-
-        with self.assertRaises(CLIError) as cm:
-            validators.validate_cluster_autoscaler_profile(MockCmd(self.cli), namespace)
-        self.assertIn(err, str(cm.exception),)
-
-    def test_valid_parameters(self):
-        cluster_autoscaler_profile = ["scan-interval=20s", "scale-down-delay-after-add=15m"]
-        namespace = Namespace(cluster_autoscaler_profile=cluster_autoscaler_profile)
-
-        validators.validate_cluster_autoscaler_profile(MockCmd(self.cli), namespace)
-
-
 class Namespace:
     def __init__(self, api_server_authorized_ip_ranges=None, cluster_autoscaler_profile=None):
         self.api_server_authorized_ip_ranges = api_server_authorized_ip_ranges