diff --git a/plugins/lookup/deployment.py b/plugins/lookup/deployment.py index a10931a144..643eb48f98 100644 --- a/plugins/lookup/deployment.py +++ b/plugins/lookup/deployment.py @@ -59,7 +59,8 @@ class LookupModule(LookupBase): return [cached] running = running_apps(vars_) - deployed = list(running) if runtime in _EPHEMERAL_RUNTIMES else list(groups) + ephemeral = runtime in _EPHEMERAL_RUNTIMES + deployed = list(running) if ephemeral else list(groups) result = { "whitelist": whitelist, "running": list(running), @@ -67,6 +68,7 @@ class LookupModule(LookupBase): "inventory": inventory, "deployed": deployed, "runtime": runtime, + "ephemeral": ephemeral, "all": list(list_invokable_app_ids()), } _STATUS_CACHE[key] = result diff --git a/plugins/lookup/mcp_servers.py b/plugins/lookup/mcp_servers.py index 7e93ff2e46..6bb92b58f2 100644 --- a/plugins/lookup/mcp_servers.py +++ b/plugins/lookup/mcp_servers.py @@ -28,11 +28,12 @@ per-tool filter can express, and a surface that withholds every tool it serves as mutating has nothing left to offer. Advertising either as a configured server would leave an agent wired to a provider it can never call. -Providers are narrowed to ``application_closure(deployment.whitelist)``, the same -set ``sys-service-loader`` preloads from. Inventory membership, ``group_names`` -and the raw whitelist all admit roles the run never deploys, and a client then -carries an endpoint nothing is serving. In compose the closure covers everything -present, so only swarm makes the difference visible. +On an ephemeral runtime (``deployment.ephemeral``) providers are narrowed to +``application_closure(deployment.whitelist)``, the same set ``sys-service-loader`` +preloads from: there inventory membership, ``group_names`` and the raw whitelist +all admit roles the run never deploys, and a client would carry an endpoint +nothing is serving. A persistent host keeps serving every provider an earlier run +deployed, so a partial run there narrows nothing and the client keeps them. ``credential_missing`` is the one authorized rejection that does not abort, because a provider deploying later in the same play has not written its secret @@ -291,15 +292,17 @@ def resolve_consumer_id(vars_: Mapping[str, Any], templar: Any) -> str: class LookupModule(LookupBase): def _deploy_closure(self, templar: Any, vars_: dict[str, Any]) -> set[str]: - """Roles this run deploys, or an empty set when that cannot be resolved.""" + """Roles an ephemeral run deploys; empty on a persistent host or when unresolvable.""" try: - whitelist = lookup_loader.get( + deployment = lookup_loader.get( "deployment", loader=self._loader, templar=templar - ).run([], variables=vars_)[0]["whitelist"] + ).run([], variables=vars_)[0] + if not deployment["ephemeral"]: + return set() return set( lookup_loader.get( "application_closure", loader=self._loader, templar=templar - ).run([whitelist], variables=vars_)[0] + ).run([deployment["whitelist"]], variables=vars_)[0] ) except (AnsibleError, KeyError, IndexError, TypeError): return set() diff --git a/roles/web-app-flowise/files/python/mint_mcp_api_key.py b/roles/web-app-flowise/files/python/mint_mcp_api_key.py index e8b8e751f9..a81d8d82fb 100644 --- a/roles/web-app-flowise/files/python/mint_mcp_api_key.py +++ b/roles/web-app-flowise/files/python/mint_mcp_api_key.py @@ -1,6 +1,3 @@ -# nocheck: mirrored-unit-test - the contract is the register/login/mint order Flowise -# 3.1.3 enforces over a session cookie; every step is a live HTTP call and a stubbed -# opener would only replay the order this file already states """Mint the scoped Flowise API identity the MCP registry reconciler uses. Flowise 3.1.3 guards ``/api/v1/apikey`` with ``apikeys:create``, so a key @@ -53,6 +50,7 @@ KEY_PERMISSIONS = [ "chatflows:view", "chatflows:create", "chatflows:update", + "chatflows:delete", ] @@ -129,15 +127,28 @@ def named_key(keys): return matches[0] if matches else None +def grants_key_permissions(key): + """Return whether a listed key carries exactly KEY_PERMISSIONS. + + Args: + key: one entry of ``GET /api/v1/apikey``. + """ + return set(key.get("permissions") or []) == set(KEY_PERMISSIONS) + + def api_key(): - """Return the managed key, reusing it when a previous run already made it.""" + """Return the managed key, reused while it grants KEY_PERMISSIONS, else minted anew.""" status, body = call("/api/v1/apikey") if status != 200: sys.exit(f"FAILED listing api keys: {status} {body}") existing = named_key(body) - if existing: + if existing and grants_key_permissions(existing): return str(existing["apiKey"]), False + if existing: + status, detail = call(f"/api/v1/apikey/{existing['id']}", method="DELETE") + if status not in (200, 204): + sys.exit(f"FAILED deleting api key {KEY_NAME}: {status} {detail}") status, body = call( "/api/v1/apikey", diff --git a/roles/web-app-flowise/files/python/reconcile/mcp_servers.py b/roles/web-app-flowise/files/python/reconcile/mcp_servers.py index ec2197e928..4c94a27238 100644 --- a/roles/web-app-flowise/files/python/reconcile/mcp_servers.py +++ b/roles/web-app-flowise/files/python/reconcile/mcp_servers.py @@ -62,6 +62,33 @@ START_NODE = "startAgentflow_0" TOOL_NODE = "toolAgentflow_0" TOOL_COMPONENT = "customMcpServerTool" +REFUSED_RC = 3 +REFUSED_STATUSES = (401, 403) + + +class Refused(SystemExit): + """Exit with REFUSED_RC for a refusal that no retry can change.""" + + def __init__(self, message): + super().__init__(REFUSED_RC) + self.message = message + + def __str__(self): + return self.message + + +def fail(message, status): + """Stop the run, with REFUSED_RC when Flowise refused the call itself. + + Args: + message: the reason, printed to stderr. + status: the HTTP status of the call that failed. + """ + if status in REFUSED_STATUSES: + print(message, file=sys.stderr) + raise Refused(message) + sys.exit(message) + def call(path, method="GET", payload=None): """Return ``(status, body)`` of one Flowise API call. @@ -96,7 +123,7 @@ def registry_entries(): """ status, body = call(REGISTRY) if status != 200: - sys.exit(f"FAILED listing custom MCP servers: {status} {body}") + fail(f"FAILED listing custom MCP servers: {status} {body}", status) if isinstance(body, dict): return body.get("data") or [] return body if isinstance(body, list) else [] @@ -128,24 +155,24 @@ def upsert(entries, server): if not matches: status, body = call(REGISTRY, method="POST", payload=payload) if status not in (200, 201): - sys.exit(f"FAILED creating {name}: {status} {body}") + fail(f"FAILED creating {name}: {status} {body}", status) return body["id"], True entry_id = matches[0]["id"] status, body = call(f"{REGISTRY}/{entry_id}", method="PUT", payload=payload) if status != 200: - sys.exit(f"FAILED updating {name}: {status} {body}") + fail(f"FAILED updating {name}: {status} {body}", status) return entry_id, matches[0].get("serverUrl") != server["url"] def authorize(entry_id, server): status, body = call(f"{REGISTRY}/{entry_id}/authorize", method="POST") if status != 200: - sys.exit(f"FAILED authorizing {server['id']}: {status} {body}") + fail(f"FAILED authorizing {server['id']}: {status} {body}", status) status, tools = call(f"{REGISTRY}/{entry_id}/tools") if status != 200: - sys.exit(f"FAILED listing tools of {server['id']}: {status} {tools}") + fail(f"FAILED listing tools of {server['id']}: {status} {tools}", status) found = sorted(str(tool.get("name")) for tool in tools or [] if tool.get("name")) expected = sorted(server.get("tools") or []) @@ -174,7 +201,7 @@ def prune(entries, keep_ids): continue status, body = call(f"{REGISTRY}/{entry['id']}", method="DELETE") if status not in (200, 204): - sys.exit(f"FAILED deleting stale {name}: {status} {body}") + fail(f"FAILED deleting stale {name}: {status} {body}", status) removed += 1 return removed @@ -293,7 +320,7 @@ def upsert_fixture(server_id, entry_id, tool, args): name = fixture_name(server_id) status, body = call(CHATFLOWS) if status != 200: - sys.exit(f"FAILED listing chatflows: {status} {body}") + fail(f"FAILED listing chatflows: {status} {body}", status) matches = [flow for flow in body or [] if flow.get("name") == name] if len(matches) > 1: @@ -310,14 +337,14 @@ def upsert_fixture(server_id, entry_id, tool, args): if not matches: status, body = call(CHATFLOWS, method="POST", payload=payload) if status not in (200, 201): - sys.exit(f"FAILED creating fixture {name}: {status} {body}") + fail(f"FAILED creating fixture {name}: {status} {body}", status) return str((body or {}).get("id")), True status, body = call( f"{CHATFLOWS}/{matches[0]['id']}", method="PUT", payload=payload ) if status != 200: - sys.exit(f"FAILED updating fixture {name}: {status} {body}") + fail(f"FAILED updating fixture {name}: {status} {body}", status) return str(matches[0]["id"]), matches[0].get("flowData") != payload["flowData"] @@ -332,7 +359,7 @@ def prune_fixtures(keep_id): """ status, body = call(CHATFLOWS) if status != 200: - sys.exit(f"FAILED listing chatflows: {status} {body}") + fail(f"FAILED listing chatflows: {status} {body}", status) prefix = f"{OWNERSHIP_PREFIX}fixture:" removed = 0 @@ -342,7 +369,7 @@ def prune_fixtures(keep_id): continue status, detail = call(f"{CHATFLOWS}/{flow['id']}", method="DELETE") if status not in (200, 204): - sys.exit(f"FAILED deleting stale {name}: {status} {detail}") + fail(f"FAILED deleting stale {name}: {status} {detail}", status) removed += 1 return removed @@ -370,7 +397,7 @@ def assert_denied(url): if status == 400 and "not allowed by policy" in str(body): return if status not in (200, 201): - sys.exit(f"FAILED creating the deny-list probe: {status} {body}") + fail(f"FAILED creating the deny-list probe: {status} {body}", status) entry_id = body["id"] status, body = call(f"{REGISTRY}/{entry_id}/authorize", method="POST") @@ -400,7 +427,7 @@ def execute_fixture(chatflow_id, server_id, tool): payload={"question": f"call {tool}", "streaming": False}, ) if status != 200: - sys.exit(f"FAILED calling {tool} on {server_id}: {status} {body}") + fail(f"FAILED calling {tool} on {server_id}: {status} {body}", status) if isinstance(body, dict) and body.get("error"): sys.exit(f"FAILED calling {tool} on {server_id}: {body['error']}") diff --git a/roles/web-app-flowise/tasks/utils/mcp.yml b/roles/web-app-flowise/tasks/utils/mcp.yml index 34cd597abf..2101f378eb 100644 --- a/roles/web-app-flowise/tasks/utils/mcp.yml +++ b/roles/web-app-flowise/tasks/utils/mcp.yml @@ -65,7 +65,7 @@ register: flowise_mcp_registry retries: "{{ 30 if MCP_RECONCILE_STRICT | bool else 0 }}" delay: 10 - until: flowise_mcp_registry.rc == 0 + until: flowise_mcp_registry.rc in [0, 3] failed_when: flowise_mcp_registry.rc != 0 and (MCP_RECONCILE_STRICT | bool) changed_when: flowise_mcp_registry.stdout.startswith('CHANGED') no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" diff --git a/tests/unit/python/plugins/lookup/test_deployment.py b/tests/unit/python/plugins/lookup/test_deployment.py index 8a206ae003..199b15e949 100644 --- a/tests/unit/python/plugins/lookup/test_deployment.py +++ b/tests/unit/python/plugins/lookup/test_deployment.py @@ -29,6 +29,7 @@ class TestDeploymentLookup(unittest.TestCase): "inventory", "deployed", "runtime", + "ephemeral", "all", }, ) @@ -225,6 +226,7 @@ class TestDeploymentLookup(unittest.TestCase): ) self.assertEqual(result[0]["deployed"], ["only-this"]) self.assertEqual(result[0]["runtime"], runtime) + self.assertTrue(result[0]["ephemeral"]) @patch("plugins.lookup.deployment.list_invokable_app_ids") def test_deployed_uses_groups_in_persistent_runtime(self, mock_list): @@ -243,6 +245,7 @@ class TestDeploymentLookup(unittest.TestCase): result[0]["deployed"], ["partial", "still-running", "also-running"], ) + self.assertFalse(result[0]["ephemeral"]) if __name__ == "__main__": diff --git a/tests/unit/python/plugins/lookup/test_mcp_servers.py b/tests/unit/python/plugins/lookup/test_mcp_servers.py index 5a553f64cd..b79ac855ea 100644 --- a/tests/unit/python/plugins/lookup/test_mcp_servers.py +++ b/tests/unit/python/plugins/lookup/test_mcp_servers.py @@ -1,13 +1,14 @@ """Unit tests for ``plugins/lookup/mcp_servers.py``. -Only the pure builder and the credential resolver are exercised; the lookup -wrapper forwards ``roles_with_service``, ``applications`` and ``users`` into -them. +The pure builder, the credential resolver and the deploy closure are exercised; +the lookup wrapper forwards ``roles_with_service``, ``applications`` and ``users`` +into them. """ import importlib import unittest from typing import ClassVar +from unittest.mock import MagicMock, patch from ansible.errors import AnsibleError @@ -89,6 +90,34 @@ class TestSelectDeployed(unittest.TestCase): self.assertEqual([], plugin_module.select_deployed(None, {"web-app-hermes"})) +class TestDeployClosure(unittest.TestCase): + def closure(self, deployment): + def get(name, **kwargs): + plugin = MagicMock() + if name == "deployment": + plugin.run.return_value = [deployment] + else: + plugin.run.side_effect = lambda terms, variables=None: [ + {*terms[0], "svc-db-postgres"} + ] + return plugin + + with patch.object(plugin_module.lookup_loader, "get", side_effect=get): + return plugin_module.LookupModule()._deploy_closure(None, {}) + + def test_a_partial_run_on_a_persistent_host_narrows_nothing(self): + self.assertEqual( + set(), + self.closure({"ephemeral": False, "whitelist": ["web-opt-rdr-domains"]}), + ) + + def test_an_ephemeral_run_narrows_to_the_whitelist_closure(self): + self.assertEqual( + {"web-app-hermes", "svc-db-postgres"}, + self.closure({"ephemeral": True, "whitelist": ["web-app-hermes"]}), + ) + + class TestBuildMcpDiscovery(unittest.TestCase): def test_no_servers_yields_nothing(self): for servers in (None, []): diff --git a/tests/unit/python/roles/web-app-flowise/files/__init__.py b/tests/unit/python/roles/web-app-flowise/files/__init__.py index e69de29bb2..087c384b57 100644 --- a/tests/unit/python/roles/web-app-flowise/files/__init__.py +++ b/tests/unit/python/roles/web-app-flowise/files/__init__.py @@ -0,0 +1,5 @@ +from pathlib import Path + +PROJECT_ROOT: Path = Path(__file__).resolve().parents[6] + +__all__ = ["PROJECT_ROOT"] diff --git a/tests/unit/python/roles/web-app-flowise/files/reconcile/test_mcp_servers.py b/tests/unit/python/roles/web-app-flowise/files/reconcile/test_mcp_servers.py index 35e1bd78dd..f0d72aceea 100644 --- a/tests/unit/python/roles/web-app-flowise/files/reconcile/test_mcp_servers.py +++ b/tests/unit/python/roles/web-app-flowise/files/reconcile/test_mcp_servers.py @@ -67,7 +67,9 @@ class FakeApi: permissive=False, refuse_on_create=False, create_error=None, + flow_delete_status=200, ): + self.flow_delete_status = flow_delete_status self.permissive = permissive self.refuse_on_create = refuse_on_create self.create_error = create_error @@ -104,6 +106,8 @@ class FakeApi: self.flows_updated.append(payload["name"]) return 200, payload if path.startswith("/api/v1/chatflows/") and method == "DELETE": + if self.flow_delete_status != 200: + return self.flow_delete_status, '{"message":"Forbidden"}' dead = path.rsplit("/", 1)[-1] self.flows_deleted.append(dead) self.flows = [f for f in self.flows if f["id"] != dead] @@ -352,6 +356,28 @@ class TestReconcileMcpServers(unittest.TestCase): module.main() self.assertEqual(["old"], api.flows_deleted) + def test_a_refused_delete_exits_with_the_no_retry_code(self) -> None: + module = load_script([SUPPORTED_SERVER]) + api = FakeApi( + flows=[{"id": "old", "name": "infinito:fixture:web-app-prometheus"}], + flow_delete_status=403, + ) + with patch.object(module, "call", api), self.assertRaises(SystemExit) as exit_: + module.main() + self.assertEqual(module.REFUSED_RC, exit_.exception.code) + self.assertIn("FAILED deleting stale", str(exit_.exception)) + + def test_a_server_error_keeps_the_retryable_exit(self) -> None: + module = load_script([SUPPORTED_SERVER]) + api = FakeApi( + flows=[{"id": "old", "name": "infinito:fixture:web-app-prometheus"}], + flow_delete_status=500, + ) + with patch.object(module, "call", api), self.assertRaises(SystemExit) as exit_: + module.main() + self.assertNotEqual(module.REFUSED_RC, exit_.exception.code) + self.assertIn("FAILED deleting stale", str(exit_.exception)) + def test_loopback_and_metadata_are_probed_and_refused(self) -> None: module = load_script([SUPPORTED_SERVER]) api = FakeApi() diff --git a/tests/unit/python/roles/web-app-flowise/files/test_mint_mcp_api_key.py b/tests/unit/python/roles/web-app-flowise/files/test_mint_mcp_api_key.py new file mode 100644 index 0000000000..2e24fe5051 --- /dev/null +++ b/tests/unit/python/roles/web-app-flowise/files/test_mint_mcp_api_key.py @@ -0,0 +1,107 @@ +"""Unit tests for ``roles/web-app-flowise/files/python/mint_mcp_api_key.py``.""" + +from __future__ import annotations + +import importlib.util +import unittest +from unittest.mock import patch + +from . import PROJECT_ROOT + +SCRIPT_PATH = PROJECT_ROOT / "roles/web-app-flowise/files/python/mint_mcp_api_key.py" +KEY_NAME = "infinito:mcp-registry" + + +def load_script() -> object: + spec = importlib.util.spec_from_file_location("mint_mcp_api_key", SCRIPT_PATH) + module = importlib.util.module_from_spec(spec) + with patch.dict( + "os.environ", + {"FLOWISE_BASE": "http://localhost:3000", "FLOWISE_API_KEY_NAME": KEY_NAME}, + ): + spec.loader.exec_module(module) + return module + + +class FakeApi: + """Stand-in for the Flowise ``/api/v1/apikey`` routes.""" + + def __init__(self, keys, delete_status=200): + self.keys = list(keys) + self.delete_status = delete_status + self.calls = [] + + def __call__(self, path, method="GET", payload=None): + self.calls.append((method, path)) + if method == "GET": + return 200, list(self.keys) + if method == "DELETE": + if self.delete_status == 200: + self.keys = [ + k for k in self.keys if path != f"/api/v1/apikey/{k['id']}" + ] + return self.delete_status, "denied" if self.delete_status != 200 else {} + created = { + "id": "new", + "keyName": payload["keyName"], + "apiKey": "sk-new", + "permissions": payload["permissions"], + } + self.keys.append(created) + return 200, list(self.keys) + + +class TestApiKey(unittest.TestCase): + def setUp(self): + self.module = load_script() + + def key(self, permissions): + return { + "id": "old", + "keyName": KEY_NAME, + "apiKey": "sk-old", + "permissions": permissions, + } + + def test_a_key_with_the_wanted_permissions_is_reused_in_any_order(self): + api = FakeApi([self.key(list(reversed(self.module.KEY_PERMISSIONS)))]) + with patch.object(self.module, "call", api): + self.assertEqual(self.module.api_key(), ("sk-old", False)) + self.assertEqual(api.calls, [("GET", "/api/v1/apikey")]) + + def test_a_key_missing_a_permission_is_replaced(self): + stale = [p for p in self.module.KEY_PERMISSIONS if p != "chatflows:delete"] + api = FakeApi([self.key(stale)]) + with patch.object(self.module, "call", api): + self.assertEqual(self.module.api_key(), ("sk-new", True)) + self.assertEqual( + api.calls, + [ + ("GET", "/api/v1/apikey"), + ("DELETE", "/api/v1/apikey/old"), + ("POST", "/api/v1/apikey"), + ], + ) + self.assertEqual( + set(api.keys[0]["permissions"]), set(self.module.KEY_PERMISSIONS) + ) + + def test_a_missing_key_is_minted_without_a_delete(self): + api = FakeApi([]) + with patch.object(self.module, "call", api): + self.assertEqual(self.module.api_key(), ("sk-new", True)) + self.assertEqual( + api.calls, [("GET", "/api/v1/apikey"), ("POST", "/api/v1/apikey")] + ) + + def test_a_refused_delete_stops_the_run(self): + api = FakeApi([self.key([])], delete_status=403) + with patch.object(self.module, "call", api), self.assertRaises(SystemExit): + self.module.api_key() + + def test_the_fixture_prune_is_allowed_to_delete_chatflows(self): + self.assertIn("chatflows:delete", self.module.KEY_PERMISSIONS) + + +if __name__ == "__main__": + unittest.main()