Merge pull request #171 from 0xFelix/winrm-service

feat(inventory): Lookup winrm services for Windows hosts

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
+            domain: [service for service in services if self._obj_is_valid(service)]
-            for domain, service in data["services"].items()
+            for domain, services in data["services"].items()
-            if self._obj_is_valid(service)
         }
 
         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 = self._find_service_with_target_port(
-            service = services.get(
+                _services, SERVICE_TARGET_PORT_WINRM_HTTPS
-                vmi["metadata"].get("labels", {}).get(LABEL_KUBEVIRT_IO_DOMAIN)
             )
+            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,

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),
+        (BASE_VMI, False, False),
-        (WINDOWS_VMI_1, True),
+        (WINDOWS_VMI_1, True, True),
-        (WINDOWS_VMI_2, True),
+        (WINDOWS_VMI_2, True, True),
-        (WINDOWS_VMI_3, True),
+        (WINDOWS_VMI_3, True, True),
-        (WINDOWS_VMI_4, 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

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",
     [

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(
+    get_services_for_namespace = mocker.patch.object(
-        inventory, "_get_ssh_services_for_namespace", return_value=[]
+        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(
+    get_services_for_namespace = mocker.patch.object(
-        inventory, "_get_ssh_services_for_namespace"
+        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()

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):
+def test_get_services_for_namespace(inventory, client):
-    assert inventory._get_ssh_services_for_namespace(client, DEFAULT_NAMESPACE) == {
+    assert inventory._get_services_for_namespace(client, DEFAULT_NAMESPACE) == {
-        "test-lb-ssh": SVC_LB_SSH,
+        "test-lb-ssh": [SVC_LB_SSH],
-        "test-np-ssh": SVC_NP_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)

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"}}
+    service = {
-    inventory._set_vars_from_vmi(hostname, vmi, {"testdomain": service}, opts)
+        "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
+    )