From 17ddb44b8accf3ed7c990516e8679c4684f5b889 Mon Sep 17 00:00:00 2001 From: Felix Matouschek Date: Thu, 3 Apr 2025 15:24:34 +0200 Subject: [PATCH] feat(inventory): Lookup winrm services for Windows hosts This feature adds looking up winrm services and tries to populate the ansible_host and ansible_port variables with the values from a found service for the host. It looks up both winrm HTTP and HTTPS services and prefers HTTPS if it found both for a given host. Signed-off-by: Felix Matouschek --- plugins/inventory/kubevirt.py | 73 ++++++++++++--- .../test_kubevirt_ansible_connection_winrm.py | 54 +++++++++-- tests/unit/plugins/inventory/test_kubevirt.py | 50 ++++++++++ .../inventory/test_kubevirt_fetch_objects.py | 12 +-- ...st_kubevirt_get_services_for_namespace.py} | 91 +++++++++++++++++-- .../test_kubevirt_set_vars_from_vmi.py | 86 +++++++++++++++++- 6 files changed, 326 insertions(+), 40 deletions(-) rename tests/unit/plugins/inventory/{test_kubevirt_get_ssh_services_for_namespace.py => test_kubevirt_get_services_for_namespace.py} (58%) diff --git a/plugins/inventory/kubevirt.py b/plugins/inventory/kubevirt.py index b83a1bb..213f3fb 100644 --- a/plugins/inventory/kubevirt.py +++ b/plugins/inventory/kubevirt.py @@ -163,6 +163,9 @@ LABEL_KUBEVIRT_IO_DOMAIN = "kubevirt.io/domain" TYPE_LOADBALANCER = "LoadBalancer" TYPE_NODEPORT = "NodePort" ID_MSWINDOWS = "mswindows" +SERVICE_TARGET_PORT_SSH = 22 +SERVICE_TARGET_PORT_WINRM_HTTP = 5985 +SERVICE_TARGET_PORT_WINRM_HTTPS = 5986 class KubeVirtInventoryException(Exception): @@ -308,6 +311,24 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): and obj["metadata"].get("uid") ) + @staticmethod + def _find_service_with_target_port( + services: List[Dict], target_port: int + ) -> Optional[Dict]: + """ + _find_service_with_target_port returns the first found service with a given + target port in the passed in list of services or otherwise None. + """ + for service in services: + if ( + (ports := service.get("spec", {}).get("ports")) is not None + and len(ports) == 1 + and ports[0].get("targetPort", 0) == target_port + ): + return service + + return None + @staticmethod def _get_host_from_service( service: Dict, node_name: Optional[str] @@ -483,7 +504,7 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): namespaces[namespace] = { "vms": vms, "vmis": vmis, - "services": self._get_ssh_services_for_namespace(client, namespace), + "services": self._get_services_for_namespace(client, namespace), } return { @@ -568,11 +589,11 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): label_selector=opts.label_selector, ) - def _get_ssh_services_for_namespace( + def _get_services_for_namespace( self, client: K8SClient, namespace: str - ) -> Dict: + ) -> Dict[str, List[Dict]]: """ - _get_ssh_services_for_namespace retrieves all services of a namespace exposing port 22/ssh. + _get_services_for_namespace retrieves all services of a namespace exposing ssh or winrm. The services are mapped to the name of the corresponding domain. """ items = self._get_resources( @@ -595,17 +616,24 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): continue # Continue if ports are not defined, there are more than one port mapping - # or the target port is not port 22/ssh + # or the target port is not port 22 (ssh) or port 5985 or 5986 (winrm). if ( (ports := spec.get("ports")) is None or len(ports) != 1 - or ports[0].get("targetPort") != 22 + or ports[0].get("targetPort") + not in [ + SERVICE_TARGET_PORT_SSH, + SERVICE_TARGET_PORT_WINRM_HTTP, + SERVICE_TARGET_PORT_WINRM_HTTPS, + ] ): continue - # Only add the service to the dict if the domain selector is present + # Only add the service to the list if the domain selector is present if domain := spec.get("selector", {}).get(LABEL_KUBEVIRT_IO_DOMAIN): - services[domain] = service + if domain not in services: + services[domain] = [] + services[domain].append(service) return services @@ -642,9 +670,8 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): return services = { - domain: service - for domain, service in data["services"].items() - if self._obj_is_valid(service) + domain: [service for service in services if self._obj_is_valid(service)] + for domain, services in data["services"].items() } name = self._sanitize_group_name(opts.name) @@ -695,7 +722,11 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): self._set_common_vars(hostname, "vm", vm, opts) def _set_vars_from_vmi( - self, hostname: str, vmi: Dict, services: Dict, opts: InventoryOptions + self, + hostname: str, + vmi: Dict, + services: Dict[str, List[Dict]], + opts: InventoryOptions, ) -> None: """ _set_vars_from_vmi sets inventory variables from a VMI prefixed with vmi_ and @@ -720,6 +751,10 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): if not interface or not interface.get("ipAddress"): return + _services = services.get( + vmi["metadata"].get("labels", {}).get(LABEL_KUBEVIRT_IO_DOMAIN), [] + ) + # Set up the connection service = None if self._is_windows( @@ -727,10 +762,18 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): vmi["metadata"].get("annotations", {}), ): self.inventory.set_variable(hostname, "ansible_connection", "winrm") - else: - service = services.get( - vmi["metadata"].get("labels", {}).get(LABEL_KUBEVIRT_IO_DOMAIN) + service = self._find_service_with_target_port( + _services, SERVICE_TARGET_PORT_WINRM_HTTPS ) + if service is None: + service = self._find_service_with_target_port( + _services, SERVICE_TARGET_PORT_WINRM_HTTP + ) + else: + service = self._find_service_with_target_port( + _services, SERVICE_TARGET_PORT_SSH + ) + self._set_ansible_host_and_port( vmi, hostname, diff --git a/tests/unit/plugins/inventory/blackbox/test_kubevirt_ansible_connection_winrm.py b/tests/unit/plugins/inventory/blackbox/test_kubevirt_ansible_connection_winrm.py index c652bac..8b4dfaa 100644 --- a/tests/unit/plugins/inventory/blackbox/test_kubevirt_ansible_connection_winrm.py +++ b/tests/unit/plugins/inventory/blackbox/test_kubevirt_ansible_connection_winrm.py @@ -22,6 +22,7 @@ BASE_VMI = { "name": "testvmi", "namespace": "default", "uid": "e86c603c-fb13-4933-bf67-de100bdba0c3", + "labels": {"kubevirt.io/domain": "testdomain"}, }, "spec": {}, "status": { @@ -56,28 +57,57 @@ WINDOWS_VMI_4 = merge_dicts( "metadata": {"annotations": {"vm.kubevirt.io/os": "windows2k22"}}, }, ) +SVC_LB_WINRM_HTTPS = { + "apiVersion": "v1", + "kind": "Service", + "metadata": { + "name": "test-lb-winrm-https", + "namespace": "default", + "uid": "22f20931-e47b-4074-a2a8-21fa59073dfd", + }, + "spec": { + "ports": [ + { + "protocol": "TCP", + "port": 12345, + "targetPort": 5986, + }, + ], + "type": "LoadBalancer", + "selector": {"kubevirt.io/domain": "testdomain"}, + }, + "status": {"loadBalancer": {"ingress": [{"ip": "192.168.1.100"}]}}, +} @pytest.mark.parametrize( - "vmi,expected", + "vmi,expected,use_service", [ - (BASE_VMI, False), - (WINDOWS_VMI_1, True), - (WINDOWS_VMI_2, True), - (WINDOWS_VMI_3, True), - (WINDOWS_VMI_4, True), + (BASE_VMI, False, False), + (WINDOWS_VMI_1, True, True), + (WINDOWS_VMI_2, True, True), + (WINDOWS_VMI_3, True, True), + (WINDOWS_VMI_4, True, True), + (WINDOWS_VMI_1, True, False), + (WINDOWS_VMI_2, True, False), + (WINDOWS_VMI_3, True, False), + (WINDOWS_VMI_4, True, False), ], ) -def test_ansible_connection_winrm(inventory, hosts, vmi, expected): +def test_ansible_connection_winrm(inventory, hosts, vmi, expected, use_service): inventory._populate_inventory( { "default_hostname": "test", "cluster_domain": "test.com", "namespaces": { - "default": {"vms": [], "vmis": [vmi], "services": {}}, + "default": { + "vms": [], + "vmis": [vmi], + "services": {"testdomain": [SVC_LB_WINRM_HTTPS]}, + } }, }, - InventoryOptions(), + InventoryOptions(use_service=use_service), ) host = f"{DEFAULT_NAMESPACE}-{vmi['metadata']['name']}" @@ -85,3 +115,9 @@ def test_ansible_connection_winrm(inventory, hosts, vmi, expected): assert hosts[host]["ansible_connection"] == "winrm" else: assert "ansible_connection" not in hosts[host] + if use_service: + assert hosts[host]["ansible_host"] == "192.168.1.100" + assert hosts[host]["ansible_port"] == 12345 + else: + assert hosts[host]["ansible_host"] == "10.10.10.10" + assert hosts[host]["ansible_port"] is None diff --git a/tests/unit/plugins/inventory/test_kubevirt.py b/tests/unit/plugins/inventory/test_kubevirt.py index 1f41eff..593fe29 100644 --- a/tests/unit/plugins/inventory/test_kubevirt.py +++ b/tests/unit/plugins/inventory/test_kubevirt.py @@ -96,6 +96,56 @@ def test_obj_is_valid(obj, expected): assert InventoryModule._obj_is_valid(obj) == expected +@pytest.mark.parametrize( + "services,target_port,expected", + [ + ([], 1234, None), + ([{"spec": {"something": "something"}}], 1234, None), + ([{"spec": {"ports": []}}], 1234, None), + ([{"spec": {"ports": [{"port": 1234}]}}], 1234, None), + ([{"spec": {"ports": [{"targetPort": 2222}]}}], 1234, None), + ( + [{"spec": {"ports": [{"targetPort": 1234}]}}], + 1234, + {"spec": {"ports": [{"targetPort": 1234}]}}, + ), + ( + [ + { + "metadata": {"name": "first"}, + "spec": {"ports": [{"targetPort": 1234}]}, + }, + { + "metadata": {"name": "second"}, + "spec": {"ports": [{"targetPort": 1234}]}, + }, + ], + 1234, + {"metadata": {"name": "first"}, "spec": {"ports": [{"targetPort": 1234}]}}, + ), + ( + [ + { + "metadata": {"name": "first"}, + "spec": {"ports": [{"targetPort": 2222}]}, + }, + { + "metadata": {"name": "second"}, + "spec": {"ports": [{"targetPort": 1234}]}, + }, + ], + 1234, + {"metadata": {"name": "second"}, "spec": {"ports": [{"targetPort": 1234}]}}, + ), + ], +) +def test_find_service_with_target_port(services, target_port, expected): + assert ( + InventoryModule._find_service_with_target_port(services, target_port) + == expected + ) + + @pytest.mark.parametrize( "service,node_name,expected", [ diff --git a/tests/unit/plugins/inventory/test_kubevirt_fetch_objects.py b/tests/unit/plugins/inventory/test_kubevirt_fetch_objects.py index 2edb3a7..5915ad5 100644 --- a/tests/unit/plugins/inventory/test_kubevirt_fetch_objects.py +++ b/tests/unit/plugins/inventory/test_kubevirt_fetch_objects.py @@ -45,8 +45,8 @@ def test_fetch_objects(mocker, inventory, opts, namespaces): get_vmis_for_namespace = mocker.patch.object( inventory, "_get_vmis_for_namespace", return_value=[{}] ) - get_ssh_services_for_namespace = mocker.patch.object( - inventory, "_get_ssh_services_for_namespace", return_value=[] + get_services_for_namespace = mocker.patch.object( + inventory, "_get_services_for_namespace", return_value=[] ) get_default_hostname = mocker.patch.object( inventory, "_get_default_hostname", return_value="default-hostname" @@ -68,7 +68,7 @@ def test_fetch_objects(mocker, inventory, opts, namespaces): get_vmis_for_namespace.assert_has_calls( [mocker.call(mocker.ANY, namespace, opts) for namespace in namespaces] ) - get_ssh_services_for_namespace.assert_has_calls( + get_services_for_namespace.assert_has_calls( [mocker.call(mocker.ANY, namespace) for namespace in namespaces] ) get_default_hostname.assert_called_once() @@ -85,8 +85,8 @@ def test_fetch_objects_early_return(mocker, inventory): get_vmis_for_namespace = mocker.patch.object( inventory, "_get_vmis_for_namespace", return_value=[] ) - get_ssh_services_for_namespace = mocker.patch.object( - inventory, "_get_ssh_services_for_namespace" + get_services_for_namespace = mocker.patch.object( + inventory, "_get_services_for_namespace" ) get_default_hostname = mocker.patch.object( inventory, "_get_default_hostname", return_value="default-hostname" @@ -104,6 +104,6 @@ def test_fetch_objects_early_return(mocker, inventory): get_vmis_for_namespace.assert_called_once_with( mocker.ANY, DEFAULT_NAMESPACE, InventoryOptions() ) - get_ssh_services_for_namespace.assert_not_called() + get_services_for_namespace.assert_not_called() get_default_hostname.assert_called_once() get_cluster_domain.assert_called_once() diff --git a/tests/unit/plugins/inventory/test_kubevirt_get_ssh_services_for_namespace.py b/tests/unit/plugins/inventory/test_kubevirt_get_services_for_namespace.py similarity index 58% rename from tests/unit/plugins/inventory/test_kubevirt_get_ssh_services_for_namespace.py rename to tests/unit/plugins/inventory/test_kubevirt_get_services_for_namespace.py index a010efd..2cdd002 100644 --- a/tests/unit/plugins/inventory/test_kubevirt_get_ssh_services_for_namespace.py +++ b/tests/unit/plugins/inventory/test_kubevirt_get_services_for_namespace.py @@ -46,20 +46,99 @@ SVC_NP_SSH = { }, } +SVC_LB_WINRM_HTTP = { + "apiVersion": "v1", + "kind": "Service", + "metadata": {"name": "test-lb-winrm-http"}, + "spec": { + "ports": [ + { + "protocol": "TCP", + "port": 5985, + "targetPort": 5985, + }, + ], + "type": "LoadBalancer", + "selector": {"kubevirt.io/domain": "test-lb-winrm-http"}, + }, +} + +SVC_NP_WINRM_HTTP = { + "apiVersion": "v1", + "kind": "Service", + "metadata": {"name": "test-np-winrm-http"}, + "spec": { + "ports": [ + { + "protocol": "TCP", + "port": 5985, + "targetPort": 5985, + }, + ], + "type": "NodePort", + "selector": {"kubevirt.io/domain": "test-np-winrm-http"}, + }, +} + +SVC_LB_WINRM_HTTPS = { + "apiVersion": "v1", + "kind": "Service", + "metadata": {"name": "test-lb-winrm-https"}, + "spec": { + "ports": [ + { + "protocol": "TCP", + "port": 5986, + "targetPort": 5986, + }, + ], + "type": "LoadBalancer", + "selector": {"kubevirt.io/domain": "test-lb-winrm-https"}, + }, +} + +SVC_NP_WINRM_HTTPS = { + "apiVersion": "v1", + "kind": "Service", + "metadata": {"name": "test-np-winrm-https"}, + "spec": { + "ports": [ + { + "protocol": "TCP", + "port": 5986, + "targetPort": 5986, + }, + ], + "type": "NodePort", + "selector": {"kubevirt.io/domain": "test-np-winrm-https"}, + }, +} + @pytest.mark.parametrize( "client", [ { - "services": [SVC_LB_SSH, SVC_NP_SSH], + "services": [ + SVC_LB_SSH, + SVC_NP_SSH, + SVC_LB_WINRM_HTTP, + SVC_NP_WINRM_HTTP, + SVC_LB_WINRM_HTTPS, + SVC_NP_WINRM_HTTPS, + ], }, ], indirect=["client"], ) -def test_get_ssh_services_for_namespace(inventory, client): - assert inventory._get_ssh_services_for_namespace(client, DEFAULT_NAMESPACE) == { - "test-lb-ssh": SVC_LB_SSH, - "test-np-ssh": SVC_NP_SSH, +def test_get_services_for_namespace(inventory, client): + assert inventory._get_services_for_namespace(client, DEFAULT_NAMESPACE) == { + "test-lb-ssh": [SVC_LB_SSH], + "test-np-ssh": [SVC_NP_SSH], + "test-lb-winrm-http": [SVC_LB_WINRM_HTTP], + "test-np-winrm-http": [SVC_NP_WINRM_HTTP], + "test-lb-winrm-https": [SVC_LB_WINRM_HTTPS], + "test-np-winrm-https": [SVC_NP_WINRM_HTTPS], } @@ -165,4 +244,4 @@ SVC_NO_SELECTOR = { indirect=["client"], ) def test_ignore_unwanted_services(inventory, client): - assert not inventory._get_ssh_services_for_namespace(client, DEFAULT_NAMESPACE) + assert not inventory._get_services_for_namespace(client, DEFAULT_NAMESPACE) diff --git a/tests/unit/plugins/inventory/test_kubevirt_set_vars_from_vmi.py b/tests/unit/plugins/inventory/test_kubevirt_set_vars_from_vmi.py index 429e51a..dbb5325 100644 --- a/tests/unit/plugins/inventory/test_kubevirt_set_vars_from_vmi.py +++ b/tests/unit/plugins/inventory/test_kubevirt_set_vars_from_vmi.py @@ -6,6 +6,8 @@ from __future__ import absolute_import, division, print_function __metaclass__ = type +import pytest + from ansible_collections.kubevirt.core.plugins.inventory.kubevirt import ( InventoryOptions, LABEL_KUBEVIRT_IO_DOMAIN, @@ -97,8 +99,17 @@ def test_set_winrm_if_windows(mocker, inventory): set_variable.assert_called_once_with(hostname, "ansible_connection", "winrm") -def test_service_lookup(mocker, inventory): +@pytest.mark.parametrize( + "is_windows,target_port", + [ + (False, 22), + (True, 5985), + (True, 5986), + ], +) +def test_service_lookup(mocker, inventory, is_windows, target_port): mocker.patch.object(inventory, "_set_common_vars") + mocker.patch.object(inventory, "_is_windows", return_value=is_windows) set_ansible_host_and_port = mocker.patch.object( inventory, "_set_ansible_host_and_port" ) @@ -106,12 +117,79 @@ def test_service_lookup(mocker, inventory): hostname = "default-testvm" vmi = { "metadata": {"labels": {LABEL_KUBEVIRT_IO_DOMAIN: "testdomain"}}, - "status": {"interfaces": [{"name": "somename", "ipAddress": "1.1.1.1"}]}, + "status": {"interfaces": [{"ipAddress": "1.1.1.1"}]}, } opts = InventoryOptions() - service = {"metadata": {"name": "testsvc"}} - inventory._set_vars_from_vmi(hostname, vmi, {"testdomain": service}, opts) + service = { + "metadata": {"name": "testsvc"}, + "spec": {"ports": [{"targetPort": target_port}]}, + } + inventory._set_vars_from_vmi(hostname, vmi, {"testdomain": [service]}, opts) set_ansible_host_and_port.assert_called_once_with( vmi, hostname, "1.1.1.1", service, opts ) + + +@pytest.mark.parametrize( + "is_windows,target_port", + [ + (True, 22), + (False, 5985), + (False, 5986), + ], +) +def test_service_ignore_not_matching_connection( + mocker, inventory, is_windows, target_port +): + mocker.patch.object(inventory, "_set_common_vars") + mocker.patch.object(inventory, "_is_windows", return_value=is_windows) + set_ansible_host_and_port = mocker.patch.object( + inventory, "_set_ansible_host_and_port" + ) + + hostname = "default-testvm" + vmi = { + "metadata": {"labels": {LABEL_KUBEVIRT_IO_DOMAIN: "testdomain"}}, + "status": {"interfaces": [{"ipAddress": "1.1.1.1"}]}, + } + opts = InventoryOptions() + service = { + "metadata": {"name": "testsvc"}, + "spec": {"ports": [{"targetPort": target_port}]}, + } + inventory._set_vars_from_vmi(hostname, vmi, {"testdomain": [service]}, opts) + + set_ansible_host_and_port.assert_called_once_with( + vmi, hostname, "1.1.1.1", None, opts + ) + + +def test_service_prefer_winrm_https(mocker, inventory): + mocker.patch.object(inventory, "_set_common_vars") + mocker.patch.object(inventory, "_is_windows", return_value=True) + set_ansible_host_and_port = mocker.patch.object( + inventory, "_set_ansible_host_and_port" + ) + + hostname = "default-testvm" + vmi = { + "metadata": {"labels": {LABEL_KUBEVIRT_IO_DOMAIN: "testdomain"}}, + "status": {"interfaces": [{"ipAddress": "1.1.1.1"}]}, + } + opts = InventoryOptions() + service_winrm_http = { + "metadata": {"name": "svc_winrm_http"}, + "spec": {"ports": [{"targetPort": 5985}]}, + } + service_winrm_https = { + "metadata": {"name": "svc_winrm_https"}, + "spec": {"ports": [{"targetPort": 5986}]}, + } + inventory._set_vars_from_vmi( + hostname, vmi, {"testdomain": [service_winrm_http, service_winrm_https]}, opts + ) + + set_ansible_host_and_port.assert_called_once_with( + vmi, hostname, "1.1.1.1", service_winrm_https, opts + )