diff --git a/salt/_modules/metalk8s_checks.py b/salt/_modules/metalk8s_checks.py index ef46f33aec..7364a7690c 100644 --- a/salt/_modules/metalk8s_checks.py +++ b/salt/_modules/metalk8s_checks.py @@ -4,6 +4,7 @@ """ import ipaddress +import re from salt.exceptions import CheckError @@ -38,6 +39,11 @@ def node(raises=True, **kwargs): if svc_ret is not True: errors.append(svc_ret) + # Run `ports` check + ports_ret = __salt__["metalk8s_checks.ports"](raises=False, **kwargs) + if ports_ret is not True: + errors.append(ports_ret) + # Run `route_exists` check for the Service Cluster IPs service_cidr = kwargs.pop( "service_cidr", __pillar__.get("networks", {}).get("service", None) @@ -169,6 +175,193 @@ def services(conflicting_services=None, raises=True, **kwargs): return error_msg or True +def ports( + listening_process=None, + raises=True, + listening_process_per_role=None, + roles=None, + **kwargs +): + """Check if an unexpected process already listening on a port on the machine, + return a string (or raise if `raises` is set to `True`) with the list of + unexpected process and port. + + Arguments: + listening_process (dict): override the list of ports expected to be + unused (or bind to a MetalK8s process). + raises (bool): the method will raise if there is any unexpected process + bind on a MetalK8s port. + listening_process_per_role (dict): If `listening_process` not provided + compute it from this dict. Dict matching between listening process + and roles (default: retrieve using map.jinja) + roles (list): List of local role for the node (default: retrieve from + pillar) + + Note: `listening_process` is a dict like + ``` + { + '
': { + 'expected': '', + 'descritpion': '', + 'mandatory': 'True/False' + } + } + ``` + Where: + - `
` could be a full address `:` or just a `` + (if `` is equal to `control_plane_ip` or `workload_plane_ip` we replace + it with the local expected IP + - `` could be a single process regexp matching or a list of regexp + (if one of the processes in this list match then result is ok) + - `` is optional, and is just a description of the expected usage + of this address + - `` is a boolean to force this expected process, if set to False + we expected either, the process in `expected` either nothing (default: False) + """ + if listening_process is None: + if listening_process_per_role is None: + listening_process_per_role = __salt__["metalk8s.get_from_map"]( + "networks", saltenv=kwargs.get("saltenv") + )["listening_process_per_role"] + if roles is None: + roles = ( + __pillar__.get("metalk8s", {}) + .get("nodes", {}) + .get(__grains__["id"], {}) + .get("roles") + ) + + # If role not yet set consider we have all roles + roles = listening_process_per_role.keys() + + # Compute full dict of listening process according to local `roles` + # Note: We consider all minions as "node" + listening_process = listening_process_per_role.get("node") or {} + for role in roles: + listening_process.update(listening_process_per_role.get(role) or {}) + + if not isinstance(listening_process, dict): + raise ValueError( + "Invalid listening process, expected dict but got {}.".format( + type(listening_process).__name__ + ) + ) + + errors = [] + + all_listen_connections = __salt__["metalk8s_network.get_listening_processes"]() + + for address, matching in listening_process.items(): + if isinstance(address, int): + address = str(address) + ip = None + port = address + if len(address.rsplit(":")) == 2: + ip, port = address.rsplit(":") + + if ip and ip in ["control_plane_ip", "workload_plane_ip"]: + ip = __grains__["metalk8s"][ip] + + # We also update the `address` for error message + address = "{}:{}".format(ip, port) + + processes = matching.get("expected") + if not isinstance(processes, list): + processes = [processes] + + if not matching.get("mandatory") and None not in processes: + processes.append(None) + + process_on_port = all_listen_connections.get( + str(port), all_listen_connections.get(int(port)) + ) + + success = False + error_process = {} + # If one of this process is expected one, succeed + for process in processes: + match = True + + # We expect nothing to be listening on this port + if process is None: + if process_on_port: + # Failure: + # - we expect nothing listening on this port + # - a process listen on every IPs + # - something already listening on the expected IP + # NOTE: Normaly if a process listen on `0.0.0.0` we do not + # have any other process on this port + if not ip: + error_process = process_on_port + match = False + if "0.0.0.0" in process_on_port: + error_process["0.0.0.0"] = process_on_port["0.0.0.0"] + match = False + if ip in process_on_port: + error_process[ip] = process_on_port[ip] + match = False + + # We expect "" to be listening on this port + # NOTE: if nothing listening it's a failure + else: + # Failure: + # - nothing listening on this ip:port + # - nothing listening on the expected IP + # - something not expected already listening + if ( + not process_on_port + or ip + and "0.0.0.0" not in process_on_port + and ip not in process_on_port + ): + match = False + elif "0.0.0.0" in process_on_port and not re.match( + process, process_on_port["0.0.0.0"]["name"] + ): + error_process["0.0.0.0"] = process_on_port["0.0.0.0"] + match = False + elif ( + ip + and ip in process_on_port + and not re.match(process, process_on_port[ip]["name"]) + ): + error_process[ip] = process_on_port[ip] + match = False + elif not ip: + for proc_ip, proc in process_on_port.items(): + if not re.match(process, proc["name"]): + error_process[proc_ip] = proc + match = False + + # This "process" match what we expect + if match: + success = True + break + + if not success: + fail_msg = "{} should be listening on {} but {}.".format( + matching.get( + "description", + " or ".join(process or "nothing" for process in processes), + ), + address, + "nothing listening" + if not error_process + else " and ".join( + "{}({}) listen on {}".format(proc["name"], proc["pid"], proc_ip) + for proc_ip, proc in error_process.items() + ), + ) + + errors.append(fail_msg) + + error_msg = "\n".join(errors) + if error_msg and raises: + raise CheckError(error_msg) + + return error_msg or True + + def sysctl(params, raises=True): """Check if the given sysctl key-values match the ones in memory and return a string (or raise if `raises` is set to `True`) with the list diff --git a/salt/metalk8s/orchestrate/deploy_node.sls b/salt/metalk8s/orchestrate/deploy_node.sls index 1ed7a04172..767fa11146 100644 --- a/salt/metalk8s/orchestrate/deploy_node.sls +++ b/salt/metalk8s/orchestrate/deploy_node.sls @@ -1,4 +1,5 @@ {%- from "metalk8s/map.jinja" import defaults with context %} +{%- from "metalk8s/map.jinja" import networks with context %} {%- from "metalk8s/map.jinja" import repo with context %} {%- set node_name = pillar.orchestrate.node_name %} @@ -46,6 +47,8 @@ Check node: {{ repo.conflicting_packages | tojson }} conflicting_services: >- {{ defaults.conflicting_services | tojson }} + listening_process_per_role: >- + {{ networks.listening_process_per_role | tojson }} - failhard: true - require: - metalk8s: Set grains ssh diff --git a/salt/tests/unit/modules/files/test_metalk8s_checks.yaml b/salt/tests/unit/modules/files/test_metalk8s_checks.yaml index 7ee190e1ea..6fc14188bb 100644 --- a/salt/tests/unit/modules/files/test_metalk8s_checks.yaml +++ b/salt/tests/unit/modules/files/test_metalk8s_checks.yaml @@ -3,6 +3,7 @@ node: - &node_nominal packages_ret: True services_ret: True + ports_ret: True route_exists_ret: True pillar: networks: @@ -30,7 +31,13 @@ node: expect_raise: True result: "Node my_node_1: Service abcd got an error because of penguin" - # 5. Error: Route for Service CIDR + # 5. Error: Ports + - <<: *node_nominal + ports_ret: "banana should be listening on 1.2.3.4:12345" + expect_raise: True + result: "Node my_node_1: banana should be listening on 1.2.3.4:12345" + + # 6. Error: Route for Service CIDR - <<: *node_nominal route_exists_ret: No route exists for banana expect_raise: True @@ -47,14 +54,16 @@ node: interface and route for this range (for details, see https://github.com/kubernetes/kubernetes/issues/57534#issuecomment-527653412). - # 6. Error: Combined errors + # 7. Error: Combined errors - <<: *node_nominal packages_ret: "Package abcd got an error because of banana" services_ret: "Service abcd got an error because of penguin" + ports_ret: "Monkey should be listening on Tree" expect_raise: True result: |- Node my_node_1: Package abcd got an error because of banana Service abcd got an error because of penguin + Monkey should be listening on Tree - <<: *node_nominal packages_ret: "Package abcd got an error because of banana" route_exists_ret: "No route exists for penguin" @@ -286,6 +295,257 @@ services: result: |- Service my-enabled-service conflicts with MetalK8s installation, please stop and disable it. +ports: + # 1. Success: Nominal + - listening_process: + 6443: + expected: apiserver + 127.0.0.1:9099: + expected: + - calico-node + control_plane_ip:4505: + expected: salt-master + get_listening_ret: + 4505: + 1.1.1.1: + pid: 147 + name: salt-master + result: True + # 1. bis (Nothing listening) + - listening_process: + 6443: {} + 127.0.0.1:9099: + expected: null + control_plane_ip:4505: + expected: salt-master + result: True + + # 2. Success: Nothing expected + - listening_process: {} + result: True + + # 3. Success: Process already running + - listening_process: + 6443: + expected: apiserver + 127.0.0.1:9099: + expected: calico-node + get_listening_ret: + 6443: + 0.0.0.0: + pid: 123 + name: apiserver + 9099: + 127.0.0.1: + pid: 345 + name: calico-node + result: True + # 3. bis (per role) (and `node` role added by default) + - listening_process_per_role: + master: + 6443: + expected: apiserver + node: + 127.0.0.1:9099: + expected: calico-node + mandatory: true + bootstrap: + 1.2.3.4:4505: + expected: + - null + - salt-master + roles: + - master + get_listening_ret: + 6443: + 1.2.3.4: + pid: 123 + name: apiserver + 9099: + 127.0.0.1: + pid: 345 + name: calico-node + 4505: + 1.2.3.4: + pid: 678 + name: salt-master + result: True + # 3. ter (all roles per default) + - get_map_ret: + listening_process_per_role: + master: + 6443: + expected: apiserver + node: + 127.0.0.1:9099: + expected: + - calico-node + bootstrap: + 1.2.3.4:4505: + expected: salt-master + get_listening_ret: + 6443: + 0.0.0.0: + pid: 123 + name: apiserver + 9099: + 127.0.0.1: + pid: 345 + name: calico-node + 4505: + 1.2.3.4: + pid: 678 + name: salt-master + result: True + # 3. quater (with role from pillar) + - listening_process_per_role: + master: + 6443: + expected: apiserver + node: + 127.0.0.1:9099: + expected: calico-.* + bootstrap: + 1.2.3.4:4505: + expected: salt-master + pillar: + metalk8s: + nodes: + my_node_1: + roles: + - master + - node + get_listening_ret: + 6443: + 0.0.0.0: + pid: 123 + name: apiserver + 9099: + 127.0.0.1: + pid: 345 + name: calico-node + 4505: + 1.2.3.4: + pid: 678 + name: salt-master + result: True + + # 4. Success: exact matching + - listening_process: + 6443: + expected: apiserver + mandatory: True + 127.0.0.1:9099: + expected: calico-.* + mandatory: True + get_listening_ret: + 6443: + 0.0.0.0: + pid: 123 + name: apiserver + 9099: + 127.0.0.1: + pid: 345 + name: calico-node + 22: + 0.0.0.0: + pid: 111 + name: sshd + result: True + + # 5. Error: expected process is not listening + - listening_process: + 6443: + expected: apiserver + description: Kubernetes apiserver + mandatory: True + expect_raise: True + result: |- + Kubernetes apiserver should be listening on 6443 but nothing listening. + # 4. bis + - listening_process: + 127.0.0.1:9099: + expected: calico-node + mandatory: True + get_listening_process: + 9099: + 1.2.3.4: + pid: 111 + name: another-proc + expect_raise: True + result: |- + calico-node should be listening on 127.0.0.1:9099 but nothing listening. + + # 5. Error: expect nothing listening + - listening_process: + 6443: {} + get_listening_ret: + 6443: + 0.0.0.0: + pid: 123 + name: my-invalid-process + expect_raise: True + result: |- + nothing should be listening on 6443 but my-invalid-process\(123\) listen on 0.0.0.0. + # 5. bis (a process listen on :: when we want 127.0.0.1) + - listening_process: + control_plane_ip:9099: + expected: null + get_listening_ret: + 9099: + 0.0.0.0: + pid: 123 + name: my-invalid-process + raises: False + result: |- + nothing should be listening on 1.1.1.1:9099 but my-invalid-process(123) listen on 0.0.0.0. + + # 6. Error: Another unexpected process already listening + - listening_process: + 127.0.0.1:9099: + expected: calico-.* + 6443: + expected: apiserver + description: Kubernetes apiserver + get_listening_ret: + 6443: + 127.0.0.1: + pid: 111 + name: not-apiserver-at-all + 1.2.3.4: + pid: 123 + name: banana + 9099: + 127.0.0.1: + pid: 222 + name: not-calico-node + expect_raise: True + result: |- + calico-.* or nothing should be listening on 127.0.0.1\:9099 but not-calico-node\(222\) listen on 127.0.0.1. + Kubernetes apiserver should be listening on 6443 but not-apiserver-at-all\(111\) listen on 127.0.0.1 and banana\(123\) listen on 1.2.3.4. + # 6. bis (listening on another IP) + - listening_process: + 127.0.0.1:9099: + expected: calico-node + 6443: + expected: apiserver + mandatory: True + get_listening_ret: + 9099: + 0.0.0.0: + pid: 222 + name: not-calico-node + expect_raise: True + result: |- + calico-node or nothing should be listening on 127.0.0.1:9099 but not-calico-node\(222\) listen on 0.0.0.0. + apiserver should be listening on 6443 but nothing listening. + + # 7. Error: Invalid listening process object + - listening_process: abcd + expect_raise: True + result: |- + Invalid listening process, expected dict but got str. + sysctl: - params: net.ipv4.ip_forward: 1 diff --git a/salt/tests/unit/modules/test_metalk8s_checks.py b/salt/tests/unit/modules/test_metalk8s_checks.py index 99c1a68b02..caf485e7e0 100644 --- a/salt/tests/unit/modules/test_metalk8s_checks.py +++ b/salt/tests/unit/modules/test_metalk8s_checks.py @@ -39,6 +39,7 @@ def test_node( expect_raise=False, packages_ret=True, services_ret=True, + ports_ret=True, route_exists_ret=True, pillar=None, **kwargs @@ -48,11 +49,13 @@ def test_node( """ packages_mock = MagicMock(return_value=packages_ret) services_mock = MagicMock(return_value=services_ret) + ports_mock = MagicMock(return_value=ports_ret) route_exists_mock = MagicMock(return_value=route_exists_ret) salt_dict = { "metalk8s_checks.packages": packages_mock, "metalk8s_checks.services": services_mock, + "metalk8s_checks.ports": ports_mock, } with patch.dict(metalk8s_checks.__grains__, {"id": "my_node_1"}), patch.dict( @@ -121,6 +124,45 @@ def test_services( else: self.assertEqual(metalk8s_checks.services(**kwargs), result) + @utils.parameterized_from_cases(YAML_TESTS_CASES["ports"]) + def test_ports( + self, + result, + get_map_ret=None, + get_listening_ret=None, + expect_raise=False, + pillar=None, + **kwargs + ): + """ + Tests the return of `ports` function + """ + salt_dict = { + "metalk8s.get_from_map": MagicMock(return_value=get_map_ret), + "metalk8s_network.get_listening_processes": MagicMock( + return_value=get_listening_ret or {} + ), + } + + with patch.dict(metalk8s_checks.__salt__, salt_dict), patch.dict( + metalk8s_checks.__pillar__, pillar or {} + ), patch.dict( + metalk8s_checks.__grains__, + { + "id": "my_node_1", + "metalk8s": { + "control_plane_ip": "1.1.1.1", + "workload_plane_ip": "1.1.1.2", + }, + }, + ): + if expect_raise: + self.assertRaisesRegex( + Exception, result, metalk8s_checks.ports, **kwargs + ) + else: + self.assertEqual(metalk8s_checks.ports(**kwargs), result) + @utils.parameterized_from_cases(YAML_TESTS_CASES["sysctl"]) def test_sysctl(self, params, data, result, raises=False): """