TPT-4417: Resolve nodebalancer module ordering and sync issues; support in-place config updates (#778)

* Address nodebalancer module ordering and sync issues

* make format

* fix lint

* Fix remaining issues

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* various fixes

* oops

* Fix UDP issue

* Add support for in-place update logic

* Fix regression

* Address copilot feedback

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: lgarber-akamai

plugins/module_utils/linode_helper.py

@@ -660,6 +660,32 @@ def __attempt_delete() -> bool:
     )
 
 
+def poll_for_response_status(
+    timeout_ctx: TimeoutContext, func: Callable[[], None], *statuses: int
+):
+    """
+    Polls a given function until it raises an ApiError with one of the
+    specified response statuses.
+    """
+
+    def __attempt() -> bool:
+        try:
+            func()
+        except ApiError as err:
+            if err.status in statuses:
+                return True
+
+            raise err
+
+        return False
+
+    poll_condition(
+        __attempt,
+        step=4,
+        timeout=timeout_ctx.seconds_remaining,
+    )
+
+
 def api_filter_constructor_for_aclp_monitor_services(
     params: Dict[str, Any],
 ) -> Dict[str, Any]:

plugins/modules/nodebalancer.py

@@ -5,7 +5,6 @@
 
 from __future__ import absolute_import, division, print_function
 
-import copy
 from typing import Any, List, Optional, Set, Tuple, cast
 
 import ansible_collections.linode.cloud.plugins.module_utils.doc_fragments.nodebalancer as docs
@@ -20,6 +19,8 @@
     dict_select_matching,
     filter_null_values,
     handle_updates,
+    poll_condition,
+    poll_for_response_status,
 )
 from ansible_specdoc.objects import (
     FieldType,
@@ -28,11 +29,22 @@
     SpecReturnValue,
 )
 from linode_api4 import (
+    ApiError,
     Firewall,
     NodeBalancer,
     NodeBalancerConfig,
     NodeBalancerNode,
 )
+from linode_api4.polling import TimeoutContext
+
+# tcp/http/https share a port namespace; udp is independent. Used to detect
+# which existing config is blocking a create on a given port.
+_TCP_FAMILY = {"tcp", "http", "https"}
+
+
+def _protocol_family(protocol: Optional[str]) -> Optional[str]:
+    return "tcp" if protocol in _TCP_FAMILY else protocol
+
 
 linode_nodes_spec = {
     "label": SpecField(
@@ -329,6 +341,23 @@
     "client_udp_sess_throttle",
 }
 
+MUTABLE_CONFIG_FIELDS: set[str] = {
+    "algorithm",
+    "check",
+    "check_attempts",
+    "check_body",
+    "check_interval",
+    "check_passive",
+    "check_path",
+    "check_timeout",
+    "cipher_suite",
+    "port",
+    "protocol",
+    "proxy_protocol",
+    "stickiness",
+    "udp_check_port",
+}
+
 DOCUMENTATION = r"""
 """
 EXAMPLES = r"""
@@ -353,6 +382,7 @@ def __init__(self) -> None:
         }
 
         self._node_balancer: Optional[NodeBalancer] = None
+        self._deleted_config_ids: Set[int] = set()
 
         super().__init__(
             module_arg_spec=self.module_arg_spec,
@@ -413,17 +443,16 @@ def _create_nodebalancer(self) -> Optional[NodeBalancer]:
 
     def _create_config(
         self, node_balancer: NodeBalancer, config_params: dict
-    ) -> Optional[NodeBalancerConfig]:
-        """Creates a config with the given kwargs within the given NodeBalancer"""
+    ) -> tuple[Optional[NodeBalancerConfig], bool]:
+        """Creates a config with the given kwargs within the given NodeBalancer."""
 
         try:
-            return node_balancer.config_create(**config_params)
-        except Exception as exception:
-            return self.fail(
-                msg="failed to create nodebalancer config: {0}".format(
-                    exception
-                )
+            return node_balancer.config_create(**config_params), True
+        except ApiError as err:
+            self.fail(
+                msg="failed to create nodebalancer config: {0}".format(err)
             )
+            return None, False
 
     def _create_node(
         self, config: NodeBalancerConfig, node_params: dict
@@ -441,19 +470,21 @@ def _create_node(
 
     def _create_config_register(
         self, node_balancer: NodeBalancer, config_params: dict
-    ) -> NodeBalancerConfig:
+    ) -> Tuple[NodeBalancerConfig, bool]:
         """Registers a create action for the given config"""
 
-        config = self._create_config(node_balancer, config_params)
-        self.register_action("Created config: {0}".format(config.id))
+        config, created = self._create_config(node_balancer, config_params)
+        if created:
+            self.register_action("Created config: {0}".format(config.id))
 
-        return config
+        return config, created
 
     def _delete_config_register(self, config: NodeBalancerConfig) -> None:
         """Registers a delete action for the given config"""
 
         self.register_action("Deleted config: {0}".format(config.id))
         config.delete()
+        self._deleted_config_ids.add(config.id)
 
     def _create_node_register(
         self, config: NodeBalancerConfig, node_params: dict
@@ -502,79 +533,152 @@ def _handle_config_nodes(
         for node in node_map.values():
             self._delete_node_register(node)
 
-    @staticmethod
-    def _check_config_exists(
-        target: Set[NodeBalancerConfig], config: dict
-    ) -> Tuple[bool, Optional[NodeBalancerConfig]]:
-        """Returns whether a config exists in the target set"""
+        # Wait for nodes to propagate
+        expected_node_count = len(new_nodes)
 
-        tmp_config = copy.deepcopy(config)
+        def _nodes_synced() -> bool:
+            if hasattr(config, "_nodes"):
+                # In the future we should fix the invalidation behavior in the Python SDK,
+                # but this works as a workaround
+                object.__delattr__(config, "_nodes")
 
-        # These fields will return as <REDACTED> so we should not diff on them
-        tmp_config.pop("ssl_cert")
-        tmp_config.pop("ssl_key")
+            return len(config.nodes) == expected_node_count
 
-        for remote_config in target:
-            config_match, remote_config_match = dict_select_matching(
-                filter_null_values(tmp_config), remote_config._raw_json
-            )
+        poll_condition(_nodes_synced, step=2, timeout=120)
 
-            if config_match == remote_config_match:
-                return True, remote_config
+    def _find_port_conflict(
+        self,
+        target: Set[NodeBalancerConfig],
+        config: dict[str, Any],
+    ) -> Optional[NodeBalancerConfig]:
+        """Return a remote config matching on port+protocol-family."""
 
-        return False, None
+        port = config.get("port")
+        family = _protocol_family(config.get("protocol"))
 
-    def _handle_configs(self) -> None:
-        """Updates the configs defined in new_configs under this NodeBalancer"""
+        for remote_config in target:
+            if (
+                remote_config.id in self._deleted_config_ids
+                or remote_config.port != port
+                or _protocol_family(remote_config.protocol) != family
+            ):
+                continue
 
-        new_configs = self.module.params.get("configs") or []
-        remote_configs = set(self._node_balancer.configs)
+            return remote_config
 
-        to_create = []
-        to_update = []
-        to_delete = remote_configs
+        return None
 
-        for config in new_configs:
-            config_exists, remote_config = self._check_config_exists(
-                remote_configs, config
-            )
+    def _handle_configs(self) -> list:
+        """Updates the configs defined in new_configs under this NodeBalancer"""
 
-            if not config_exists:
-                to_create.append((config, remote_config))
-                continue
+        configs_param = self.module.params.get("configs")
+        if configs_param is None:
+            return []
 
-            if config.get("recreate"):
-                to_create.append((config, remote_config))
-                continue
+        new_configs = configs_param
 
-            to_update.append((config, remote_config))
-            to_delete.remove(remote_config)
+        to_create, to_update, to_delete = self._plan_configs(new_configs)
 
-        # Remove remaining configs
         for config in to_delete:
             self._delete_config_register(config)
 
+        # Wait for deletions to propagate before attempting creates; otherwise
+        # the API returns "already using port" 400s on overlapping ports.
+        for config in to_delete:
+            poll_for_response_status(
+                TimeoutContext(timeout_seconds=120), config._api_get, 404
+            )
+
         result_configs = []
 
-        for config, remote_config in to_create:
-            new_config = self._create_config_register(
+        for config in to_create:
+            new_config, _ = self._create_config_register(
                 self._node_balancer, config
             )
-            if config.get("nodes") is not None:
-                self._handle_config_nodes(new_config, config.get("nodes"))
-            new_config._api_get()
+            self._sync_config_nodes(new_config, config.get("nodes"))
             result_configs.append(new_config)
 
         for config, remote_config in to_update:
-            if config.get("nodes") is not None:
-                self._handle_config_nodes(remote_config, config.get("nodes"))
-            remote_config._api_get()
+            handle_updates(
+                remote_config,
+                filter_null_values(config),
+                MUTABLE_CONFIG_FIELDS,
+                self.register_action,
+            )
+            self._sync_config_nodes(remote_config, config.get("nodes"))
             result_configs.append(remote_config)
 
         cast(list, self.results["configs"]).extend(
             [c._raw_json for c in result_configs]
         )
 
+        return result_configs
+
+    def _stable_configs(
+        self, consecutive_threshold: int = 3
+    ) -> List[NodeBalancerConfig]:
+        """
+        Wait for the list endpoint to consistently show the expected config
+        count. Workaround for API propagation behavior.
+        """
+        successful_requests = 0
+        last_len = -1
+        last_configs: List[NodeBalancerConfig] = []
+
+        def _stable() -> bool:
+            nonlocal successful_requests, last_len, last_configs
+            self._node_balancer.invalidate()
+            last_configs = list(self._node_balancer.configs)
+
+            if len(last_configs) != last_len:
+                successful_requests = 0
+                last_len = len(last_configs)
+
+                return False
+
+            successful_requests += 1
+            return successful_requests >= consecutive_threshold
+
+        poll_condition(_stable, step=2, timeout=180)
+
+        return last_configs
+
+    def _plan_configs(self, new_configs: list[dict]) -> tuple[
+        list[dict],
+        list[tuple[dict, NodeBalancerConfig]],
+        set[NodeBalancerConfig],
+    ]:
+        """Partition desired configs into (create, update, delete) sets."""
+
+        remote_configs = set(self._stable_configs())
+        to_create: list[dict] = []
+        to_update: list[tuple[dict, NodeBalancerConfig]] = []
+        to_delete = remote_configs
+
+        for config in new_configs:
+            match = self._find_port_conflict(to_delete, config)
+
+            if match is not None and not config.get("recreate"):
+                to_update.append((config, match))
+                to_delete.remove(match)
+                continue
+
+            to_create.append(config)
+
+        return to_create, to_update, to_delete
+
+    def _sync_config_nodes(
+        self,
+        config: NodeBalancerConfig,
+        nodes: Optional[List[dict]],
+    ) -> None:
+        """Sync nodes against the given config if provided."""
+
+        if nodes is not None:
+            self._handle_config_nodes(config, nodes)
+
+        config._api_get()
+
     def _update_nodebalancer(self) -> None:
         """Handles updating the current NodeBalancer"""
 
@@ -640,10 +744,15 @@ def exec_module(self, **kwargs: Any) -> Optional[dict]:
             return self.results
 
         self._handle_nodebalancer()
-        self._handle_configs()
+        result_configs = self._handle_configs()
+
+        result_configs = (
+            result_configs
+            if self.module.params.get("configs") is not None
+            else self._node_balancer.configs
+        )
 
-        # Append all nodes to the result
-        for config in self._node_balancer.configs:
+        for config in result_configs:
             for node in config.nodes:
                 node._api_get()
                 cast(list, self.results["nodes"]).append(node._raw_json)