From be79dc5704c507f4d4081e1619bd93ea91fe59e4 Mon Sep 17 00:00:00 2001 From: Daniel Pivonka Date: Thu, 1 Apr 2021 13:56:48 -0400 Subject: [PATCH] mgr/orchestrator: validate lists in spec jsons Signed-off-by: Daniel Pivonka (cherry picked from commit 7844ce0785595c951f5822d2c38d1381dc13c8c1) --- .../mgr/cephadm/tests/test_scheduling.py | 7 +- src/pybind/mgr/orchestrator/_interface.py | 6 +- src/pybind/mgr/orchestrator/module.py | 6 +- .../ceph/deployment/drive_group.py | 5 +- src/python-common/ceph/deployment/hostspec.py | 26 ++++++ .../ceph/deployment/service_spec.py | 87 ++++++++++--------- .../ceph/tests/test_drive_group.py | 6 +- .../ceph/tests/test_service_spec.py | 7 +- 8 files changed, 90 insertions(+), 60 deletions(-) diff --git a/src/pybind/mgr/cephadm/tests/test_scheduling.py b/src/pybind/mgr/cephadm/tests/test_scheduling.py index f23bf56c14608..82085957ffc47 100644 --- a/src/pybind/mgr/cephadm/tests/test_scheduling.py +++ b/src/pybind/mgr/cephadm/tests/test_scheduling.py @@ -6,7 +6,8 @@ from typing import NamedTuple, List, Dict import pytest from ceph.deployment.hostspec import HostSpec -from ceph.deployment.service_spec import ServiceSpec, PlacementSpec, ServiceSpecValidationError, IngressSpec +from ceph.deployment.service_spec import ServiceSpec, PlacementSpec, IngressSpec +from ceph.deployment.hostspec import SpecValidationError from cephadm.module import HostAssignment from cephadm.schedule import DaemonPlacement @@ -164,7 +165,7 @@ def run_scheduler_test(results, mk_spec, hosts, daemons, key_elems): # | | | | + expected result # | | | | | test_explicit_scheduler_results = [ - (k("* * 0 *"), error(ServiceSpecValidationError, 'num/count must be > 1')), + (k("* * 0 *"), error(SpecValidationError, 'num/count must be > 1')), (k("* e N l"), error(OrchestratorValidationError, 'Cannot place : No matching hosts for label mylabel')), (k("* e N p"), error(OrchestratorValidationError, 'Cannot place : No matching hosts')), (k("* e N h"), error(OrchestratorValidationError, 'placement spec is empty: no hosts, no label, no pattern, no count')), @@ -908,7 +909,7 @@ def test_bad_placements(placement): try: PlacementSpec.from_string(placement.split(' ')) assert False - except ServiceSpecValidationError: + except SpecValidationError: pass diff --git a/src/pybind/mgr/orchestrator/_interface.py b/src/pybind/mgr/orchestrator/_interface.py index 9b2c1e1fa88d9..550d0b623ac0e 100644 --- a/src/pybind/mgr/orchestrator/_interface.py +++ b/src/pybind/mgr/orchestrator/_interface.py @@ -31,9 +31,9 @@ import yaml from ceph.deployment import inventory from ceph.deployment.service_spec import ServiceSpec, NFSServiceSpec, RGWSpec, \ - ServiceSpecValidationError, IscsiServiceSpec, IngressSpec + IscsiServiceSpec, IngressSpec from ceph.deployment.drive_group import DriveGroupSpec -from ceph.deployment.hostspec import HostSpec +from ceph.deployment.hostspec import HostSpec, SpecValidationError from ceph.utils import datetime_to_str, str_to_datetime from mgr_module import MgrModule, CLICommand, HandleCommandResult @@ -94,7 +94,7 @@ def handle_exception(prefix: str, perm: str, func: FuncT) -> FuncT: def wrapper(*args: Any, **kwargs: Any) -> Any: try: return func(*args, **kwargs) - except (OrchestratorError, ServiceSpecValidationError) as e: + except (OrchestratorError, SpecValidationError) as e: # Do not print Traceback for expected errors. return HandleCommandResult(e.errno, stderr=str(e)) except ImportError as e: diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py index 3d4bdefdab734..d6b5bef0fbf62 100644 --- a/src/pybind/mgr/orchestrator/module.py +++ b/src/pybind/mgr/orchestrator/module.py @@ -10,8 +10,8 @@ from prettytable import PrettyTable from ceph.deployment.inventory import Device from ceph.deployment.drive_group import DriveGroupSpec, DeviceSelection -from ceph.deployment.service_spec import PlacementSpec, ServiceSpec, \ - ServiceSpecValidationError +from ceph.deployment.service_spec import PlacementSpec, ServiceSpec +from ceph.deployment.hostspec import SpecValidationError from ceph.utils import datetime_now from mgr_util import to_pretty_timedelta, format_dimless @@ -1020,7 +1020,7 @@ Usage: try: self.get_foreign_ceph_option('mon', k) except KeyError: - raise ServiceSpecValidationError(f'Invalid config option {k} in spec') + raise SpecValidationError(f'Invalid config option {k} in spec') if dry_run and not isinstance(spec, HostSpec): spec.preview_only = dry_run diff --git a/src/python-common/ceph/deployment/drive_group.py b/src/python-common/ceph/deployment/drive_group.py index 58123ea053838..93d7aaef1779f 100644 --- a/src/python-common/ceph/deployment/drive_group.py +++ b/src/python-common/ceph/deployment/drive_group.py @@ -1,7 +1,8 @@ import yaml from ceph.deployment.inventory import Device -from ceph.deployment.service_spec import ServiceSpecValidationError, ServiceSpec, PlacementSpec +from ceph.deployment.service_spec import ServiceSpec, PlacementSpec +from ceph.deployment.hostspec import SpecValidationError try: from typing import Optional, List, Dict, Any, Union @@ -121,7 +122,7 @@ class DeviceSelection(object): return repr(self) == repr(other) -class DriveGroupValidationError(ServiceSpecValidationError): +class DriveGroupValidationError(SpecValidationError): """ Defining an exception here is a bit problematic, cause you cannot properly catch it, if it was raised in a different mgr module. diff --git a/src/python-common/ceph/deployment/hostspec.py b/src/python-common/ceph/deployment/hostspec.py index f15272d5f37e6..df9aa42ad782d 100644 --- a/src/python-common/ceph/deployment/hostspec.py +++ b/src/python-common/ceph/deployment/hostspec.py @@ -1,9 +1,22 @@ +import errno try: from typing import Optional, List, Any except ImportError: pass # just for type checking +class SpecValidationError(Exception): + """ + Defining an exception here is a bit problematic, cause you cannot properly catch it, + if it was raised in a different mgr module. + """ + def __init__(self, + msg: str, + errno: int = -errno.EINVAL): + super(SpecValidationError, self).__init__(msg) + self.errno = errno + + class HostSpec(object): """ Information about hosts. Like e.g. ``kubectl get nodes`` @@ -38,12 +51,25 @@ class HostSpec(object): @classmethod def from_json(cls, host_spec: dict) -> 'HostSpec': + host_spec = cls.normalize_json(host_spec) _cls = cls(host_spec['hostname'], host_spec['addr'] if 'addr' in host_spec else None, list(set(host_spec['labels'])) if 'labels' in host_spec else None, host_spec['status'] if 'status' in host_spec else None) return _cls + @staticmethod + def normalize_json(host_spec: dict) -> dict: + labels = host_spec.get('labels') + if labels is None: + return host_spec + if isinstance(labels, list): + return host_spec + if not isinstance(labels, str): + raise SpecValidationError(f'Labels ({labels}) must be a string or list of strings') + host_spec['labels'] = [labels] + return host_spec + def __repr__(self) -> str: args = [self.hostname] # type: List[Any] if self.addr is not None: diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py index 6e30e47aa7c28..5a21071649c3d 100644 --- a/src/python-common/ceph/deployment/service_spec.py +++ b/src/python-common/ceph/deployment/service_spec.py @@ -1,4 +1,3 @@ -import errno import fnmatch import re from collections import OrderedDict @@ -9,25 +8,13 @@ from typing import Optional, Dict, Any, List, Union, Callable, Iterable, Type, T import yaml -from ceph.deployment.hostspec import HostSpec +from ceph.deployment.hostspec import HostSpec, SpecValidationError from ceph.deployment.utils import unwrap_ipv6 ServiceSpecT = TypeVar('ServiceSpecT', bound='ServiceSpec') FuncT = TypeVar('FuncT', bound=Callable) -class ServiceSpecValidationError(Exception): - """ - Defining an exception here is a bit problematic, cause you cannot properly catch it, - if it was raised in a different mgr module. - """ - def __init__(self, - msg: str, - errno: int = -errno.EINVAL): - super(ServiceSpecValidationError, self).__init__(msg) - self.errno = errno - - def assert_valid_host(name: str) -> None: p = re.compile('^[a-zA-Z0-9-]+$') try: @@ -37,7 +24,7 @@ def assert_valid_host(name: str) -> None: assert len(part) <= 63, '.-delimited name component must not be more than 63 chars' assert p.match(part), 'name component must include only a-z, 0-9, and -' except AssertionError as e: - raise ServiceSpecValidationError(str(e)) + raise SpecValidationError(str(e)) def handle_type_error(method: FuncT) -> FuncT: @@ -47,7 +34,7 @@ def handle_type_error(method: FuncT) -> FuncT: return method(cls, *args, **kwargs) except (TypeError, AttributeError) as e: error_msg = '{}: {}'.format(cls.__name__, e) - raise ServiceSpecValidationError(error_msg) + raise SpecValidationError(error_msg) return cast(FuncT, inner) @@ -285,31 +272,31 @@ class PlacementSpec(object): def validate(self) -> None: if self.hosts and self.label: # TODO: a less generic Exception - raise ServiceSpecValidationError('Host and label are mutually exclusive') + raise SpecValidationError('Host and label are mutually exclusive') if self.count is not None and self.count <= 0: - raise ServiceSpecValidationError("num/count must be > 1") + raise SpecValidationError("num/count must be > 1") if self.count_per_host is not None and self.count_per_host < 1: - raise ServiceSpecValidationError("count-per-host must be >= 1") + raise SpecValidationError("count-per-host must be >= 1") if self.count_per_host is not None and not ( self.label or self.hosts or self.host_pattern ): - raise ServiceSpecValidationError( + raise SpecValidationError( "count-per-host must be combined with label or hosts or host_pattern" ) if self.count is not None and self.count_per_host is not None: - raise ServiceSpecValidationError("cannot combine count and count-per-host") + raise SpecValidationError("cannot combine count and count-per-host") if ( self.count_per_host is not None and self.hosts and any([hs.network or hs.name for hs in self.hosts]) ): - raise ServiceSpecValidationError( + raise SpecValidationError( "count-per-host cannot be combined explicit placement with names or networks" ) if self.host_pattern and self.hosts: - raise ServiceSpecValidationError('cannot combine host patterns and hosts') + raise SpecValidationError('cannot combine host patterns and hosts') for h in self.hosts: h.validate() @@ -362,7 +349,7 @@ tPlacementSpec(hostname='host2', network='', name='')]) else: strings = [arg] else: - raise ServiceSpecValidationError('invalid placement %s' % arg) + raise SpecValidationError('invalid placement %s' % arg) count = None count_per_host = None @@ -397,14 +384,14 @@ tPlacementSpec(hostname='host2', network='', name='')]) labels = [x for x in strings if 'label:' in x] if len(labels) > 1: - raise ServiceSpecValidationError('more than one label provided: {}'.format(labels)) + raise SpecValidationError('more than one label provided: {}'.format(labels)) for l in labels: strings.remove(l) label = labels[0][6:] if labels else None host_patterns = strings if len(host_patterns) > 1: - raise ServiceSpecValidationError( + raise SpecValidationError( 'more than one host pattern provided: {}'.format(host_patterns)) ps = PlacementSpec(count=count, @@ -448,7 +435,7 @@ class ServiceSpec(object): 'container': CustomContainerSpec, }.get(service_type, cls) if ret == ServiceSpec and not service_type: - raise ServiceSpecValidationError('Spec needs a "service_type" key.') + raise SpecValidationError('Spec needs a "service_type" key.') return ret def __new__(cls: Type[ServiceSpecT], *args: Any, **kwargs: Any) -> ServiceSpecT: @@ -532,9 +519,11 @@ class ServiceSpec(object): """ if not isinstance(json_spec, dict): - raise ServiceSpecValidationError( + raise SpecValidationError( f'Service Spec is not an (JSON or YAML) object. got "{str(json_spec)}"') + json_spec = cls.normalize_json(json_spec) + c = json_spec.copy() # kludge to make `from_json` compatible to `Orchestrator.describe_service` @@ -556,6 +545,18 @@ class ServiceSpec(object): return _cls._from_json_impl(c) # type: ignore + @staticmethod + def normalize_json(json_spec: dict) -> dict: + networks = json_spec.get('networks') + if networks is None: + return json_spec + if isinstance(networks, list): + return json_spec + if not isinstance(networks, str): + raise SpecValidationError(f'Networks ({networks}) must be a string or list of strings') + json_spec['networks'] = [networks] + return json_spec + @classmethod def _from_json_impl(cls: Type[ServiceSpecT], json_spec: dict) -> ServiceSpecT: args = {} # type: Dict[str, Any] @@ -611,16 +612,16 @@ class ServiceSpec(object): def validate(self) -> None: if not self.service_type: - raise ServiceSpecValidationError('Cannot add Service: type required') + raise SpecValidationError('Cannot add Service: type required') if self.service_type in self.REQUIRES_SERVICE_ID: if not self.service_id: - raise ServiceSpecValidationError('Cannot add Service: id required') + raise SpecValidationError('Cannot add Service: id required') if not re.match('^[a-zA-Z0-9_.-]+$', self.service_id): - raise ServiceSpecValidationError('Service id contains invalid characters, ' - 'only [a-zA-Z0-9_.-] allowed') + raise SpecValidationError('Service id contains invalid characters, ' + 'only [a-zA-Z0-9_.-] allowed') elif self.service_id: - raise ServiceSpecValidationError( + raise SpecValidationError( f'Service of type \'{self.service_type}\' should not contain a service id') if self.placement is not None: @@ -628,14 +629,14 @@ class ServiceSpec(object): if self.config: for k, v in self.config.items(): if k in self.MANAGED_CONFIG_OPTIONS: - raise ServiceSpecValidationError( + raise SpecValidationError( f'Cannot set config option {k} in spec: it is managed by cephadm' ) for network in self.networks or []: try: ip_network(network) except ValueError as e: - raise ServiceSpecValidationError( + raise SpecValidationError( f'Cannot parse network {network}: {e}' ) @@ -686,7 +687,7 @@ class NFSServiceSpec(ServiceSpec): super(NFSServiceSpec, self).validate() if not self.pool: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add NFS: No Pool specified') def rados_config_name(self): @@ -767,10 +768,10 @@ class RGWSpec(ServiceSpec): super(RGWSpec, self).validate() if self.rgw_realm and not self.rgw_zone: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add RGW: Realm specified but no zone specified') if self.rgw_zone and not self.rgw_realm: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add RGW: Zone specified but no realm specified') @@ -818,7 +819,7 @@ class IscsiServiceSpec(ServiceSpec): super(IscsiServiceSpec, self).validate() if not self.pool: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add ISCSI: No Pool specified') # Do not need to check for api_user and api_password as they @@ -923,16 +924,16 @@ class IngressSpec(ServiceSpec): super(IngressSpec, self).validate() if not self.backend_service: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add ingress: No backend_service specified') if not self.frontend_port: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add ingress: No frontend_port specified') if not self.monitor_port: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add ingress: No monitor_port specified') if not self.virtual_ip: - raise ServiceSpecValidationError( + raise SpecValidationError( 'Cannot add ingress: No virtual_ip provided') diff --git a/src/python-common/ceph/tests/test_drive_group.py b/src/python-common/ceph/tests/test_drive_group.py index 0fd162a992792..76d343edada1b 100644 --- a/src/python-common/ceph/tests/test_drive_group.py +++ b/src/python-common/ceph/tests/test_drive_group.py @@ -5,9 +5,9 @@ import pytest import yaml from ceph.deployment import drive_selection, translate -from ceph.deployment.hostspec import HostSpec +from ceph.deployment.hostspec import HostSpec, SpecValidationError from ceph.deployment.inventory import Device -from ceph.deployment.service_spec import PlacementSpec, ServiceSpecValidationError +from ceph.deployment.service_spec import PlacementSpec from ceph.tests.utils import _mk_inventory, _mk_device from ceph.deployment.drive_group import DriveGroupSpec, DeviceSelection, \ DriveGroupValidationError @@ -76,7 +76,7 @@ spec: ), ]) def test_DriveGroup_fail(match, test_input): - with pytest.raises(ServiceSpecValidationError, match=match): + with pytest.raises(SpecValidationError, match=match): osd_spec = DriveGroupSpec.from_json(yaml.safe_load(test_input)) osd_spec.validate() diff --git a/src/python-common/ceph/tests/test_service_spec.py b/src/python-common/ceph/tests/test_service_spec.py index 3642a9182fb0c..a7591e6c04a4b 100644 --- a/src/python-common/ceph/tests/test_service_spec.py +++ b/src/python-common/ceph/tests/test_service_spec.py @@ -5,8 +5,9 @@ import yaml import pytest from ceph.deployment.service_spec import HostPlacementSpec, PlacementSpec, \ - ServiceSpec, ServiceSpecValidationError, RGWSpec, NFSServiceSpec, IscsiServiceSpec + ServiceSpec, RGWSpec, NFSServiceSpec, IscsiServiceSpec from ceph.deployment.drive_group import DriveGroupSpec +from ceph.deployment.hostspec import SpecValidationError @pytest.mark.parametrize("test_input,expected, require_network", @@ -84,7 +85,7 @@ def test_parse_placement_specs(test_input, expected): ] ) def test_parse_placement_specs_raises(test_input): - with pytest.raises(ServiceSpecValidationError): + with pytest.raises(SpecValidationError): PlacementSpec.from_string(test_input) @pytest.mark.parametrize("test_input", @@ -331,6 +332,6 @@ def test_service_name(s_type, s_id, s_name): ]) def test_service_id_raises_invalid_char(s_type, s_id): - with pytest.raises(ServiceSpecValidationError): + with pytest.raises(SpecValidationError): spec = ServiceSpec.from_json(_get_dict_spec(s_type, s_id)) spec.validate() -- 2.39.5