Merge pull request #2634 from blublinsky/mcp-headers

changed mcp headers definitions

Author: openshift-merge-bot[bot]

ols/src/query_helpers/docs_summarizer.py

@@ -106,7 +106,11 @@ def __init__(
         mcp_config_builder = MCPConfigBuilder(
             self.user_token, config.mcp_servers.servers
         )
-        self.mcp_servers = mcp_config_builder.dump_client_config()
+        try:
+            self.mcp_servers = mcp_config_builder.dump_client_config()
+        except Exception as e:
+            logger.error("Failed to resolve MCP server(s): %s", e)
+            self.mcp_servers = {}
         if self.mcp_servers:
             logger.info("MCP servers provided: %s", list(self.mcp_servers.keys()))
             self._tool_calling_enabled = True

ols/src/tools/mcp_config_builder.py

@@ -6,13 +6,12 @@
 from typing import Any
 
 from ols.app.models.config import MCPServerConfig
+from ols.utils import checks
 
 logger = logging.getLogger(__name__)
 
-
-# Additional header containing user token for k8s/ocp authentication
-# for SSE MCP servers.
-K8S_AUTH_HEADER = "kubernetes-authorization"
+# Constant, defining usage of kubernetes token
+KUBERNETES_PLACEHOLDER = "kubernetes"
 
 
 class MCPConfigBuilder:
@@ -25,20 +24,6 @@ def __init__(
         self.user_token = user_token
         self.mcp_server_configs = mcp_server_configs
 
-    @staticmethod
-    def include_auth_header(user_token: str, config: dict[str, Any]) -> dict[str, Any]:
-        """Include user token in the config headers."""
-        # Only add Authorization header if we have a valid token
-        if user_token and user_token.strip():
-            if "headers" not in config:
-                config["headers"] = {}
-            if K8S_AUTH_HEADER in config["headers"]:
-                logger.warning(
-                    "Kubernetes auth header is already set, overriding with actual user token."
-                )
-            config["headers"][K8S_AUTH_HEADER] = f"Bearer {user_token}"
-        return config
-
     def include_auth_to_stdio(self, server_envs: dict[str, str]) -> dict[str, str]:
         """Resolve OpenShift stdio env config."""
         logger.debug("Updating env configuration of openshift stdio mcp server")
@@ -79,18 +64,20 @@ def dump_client_config(self) -> dict[str, Any]:
                         server_conf.stdio.env
                     )
                 servers_conf[server_conf.name].update(stdio_conf)
+                continue
 
             if server_conf.sse:
                 sse_conf = server_conf.sse.model_dump()
-                self.include_auth_header(self.user_token, sse_conf)
+                sse_conf["headers"] = self._resolve_tokens_to_value(sse_conf["headers"])
                 servers_conf[server_conf.name].update(sse_conf)
+                continue
 
             if server_conf.streamable_http:
-                servers_conf[server_conf.name].update(
-                    self.include_auth_header(
-                        self.user_token, server_conf.streamable_http.model_dump()
-                    )
+                http_conf = server_conf.streamable_http.model_dump()
+                http_conf["headers"] = self._resolve_tokens_to_value(
+                    http_conf["headers"]
                 )
+                servers_conf[server_conf.name].update(http_conf)
                 # Note: Streamable HTTP transport expects timedelta instead of
                 # int as for the sse - blame langchain-mcp-adapters for
                 # inconsistency
@@ -100,3 +87,21 @@ def dump_client_config(self) -> dict[str, Any]:
                     )
 
         return servers_conf
+
+    def _resolve_tokens_to_value(self, headers: dict[str, str]) -> dict[str, Any]:
+        """Convert header definitions to values."""
+        updated = {}
+        for name, value in headers.items():
+            if value == KUBERNETES_PLACEHOLDER:
+                updated[name] = f"Bearer {self.user_token}"
+            else:
+                try:
+                    # load token value
+                    with open(value, "r", encoding="utf-8") as token_store:
+                        token = token_store.read()
+                    updated[name] = token
+                except Exception as e:
+                    raise checks.InvalidConfigurationError(
+                        f"token value refers to non existent file  '{value}', error {e}"
+                    )
+        return updated

tests/unit/tools/test_mcp_config_builder.py

@@ -1,6 +1,7 @@
 """Tests for MCPConfigBuilder."""
 
 import os
+import tempfile
 from datetime import timedelta
 from unittest.mock import patch
 
@@ -10,7 +11,7 @@
     StdioTransportConfig,
     StreamableHttpTransportConfig,
 )
-from ols.src.tools.mcp_config_builder import K8S_AUTH_HEADER, MCPConfigBuilder
+from ols.src.tools.mcp_config_builder import KUBERNETES_PLACEHOLDER, MCPConfigBuilder
 
 
 def test_mcp_config_builder_dump_client_config():
@@ -145,66 +146,42 @@ def test_missing_kubeconfig_and_kubernetes_service(caplog):
         assert mcp_config == expected
         assert "Missing necessary KUBECONFIG/KUBERNETES_SERVICE_* envs" in caplog.text
 
-    @staticmethod
-    def test_include_auth_header():
-        """Test include_auth_header method."""
-        user_token = "fake-token"  # noqa: S105
-        config = {}
-
-        result = MCPConfigBuilder.include_auth_header(user_token, config)
-
-        assert "headers" in result
-        assert result["headers"][K8S_AUTH_HEADER] == f"Bearer {user_token}"
-
-    @staticmethod
-    def test_include_auth_header_existing_headers():
-        """Test include_auth_header with existing headers."""
-        user_token = "fake-token"  # noqa: S105
-        config = {"headers": {"Content-Type": "application/json"}}
-
-        result = MCPConfigBuilder.include_auth_header(user_token, config)
-
-        assert result["headers"]["Content-Type"] == "application/json"
-        assert result["headers"][K8S_AUTH_HEADER] == f"Bearer {user_token}"
-
-    @staticmethod
-    def test_include_auth_header_existing_auth(caplog):
-        """Test include_auth_header with existing auth header."""
-        user_token = "fake-token"  # noqa: S105
-        config = {"headers": {K8S_AUTH_HEADER: "old-token"}}
-
-        result = MCPConfigBuilder.include_auth_header(user_token, config)
-
-        assert result["headers"][K8S_AUTH_HEADER] == f"Bearer {user_token}"
-        assert (
-            "Kubernetes auth header is already set, overriding with actual user token"
-            in caplog.text
-        )
-
     @staticmethod
     def test_dump_client_config_with_sse():
         """Test dump_client_config with SSE configuration."""
-        mcp_server_configs = [
-            MCPServerConfig(
-                name="sse-server",
-                transport="sse",
-                sse=SseTransportConfig(
-                    url="https://example.com/events",
-                    headers={"X-Custom-Header": "value"},
+        file_descriptor, file_path = tempfile.mkstemp(suffix=".tmp")
+        try:
+            with os.fdopen(file_descriptor, "w") as open_file:
+                open_file.write("value")
+            mcp_server_configs = [
+                MCPServerConfig(
+                    name="sse-server",
+                    transport="sse",
+                    sse=SseTransportConfig(
+                        url="https://example.com/events",
+                        headers={
+                            "X-Custom-Header": file_path,
+                            "kubernetes": KUBERNETES_PLACEHOLDER,
+                        },
+                    ),
                 ),
-            ),
-        ]
-        user_token = "fake-token"  # noqa: S105
-
-        builder = MCPConfigBuilder(user_token, mcp_server_configs)
-        result = builder.dump_client_config()
+            ]
+            user_token = "fake-token"  # noqa: S105
 
-        assert result["sse-server"]["transport"] == "sse"
-        assert result["sse-server"]["url"] == "https://example.com/events"
-        assert result["sse-server"]["headers"]["X-Custom-Header"] == "value"
-        assert (
-            result["sse-server"]["headers"][K8S_AUTH_HEADER] == f"Bearer {user_token}"
-        )
+            builder = MCPConfigBuilder(user_token, mcp_server_configs)
+            try:
+                result = builder.dump_client_config()
+            except Exception as e:
+                print(f"failed  creating config {e}")
+                assert False
+            assert result["sse-server"]["transport"] == "sse"
+            assert result["sse-server"]["url"] == "https://example.com/events"
+            assert result["sse-server"]["headers"]["X-Custom-Header"] == "value"
+            assert (
+                result["sse-server"]["headers"]["kubernetes"] == f"Bearer {user_token}"
+            )
+        finally:
+            os.unlink(file_path)
 
     @staticmethod
     def test_dump_client_config_with_mixed_transports():
@@ -237,40 +214,52 @@ def test_dump_client_config_with_mixed_transports():
         assert result["openshift"]["transport"] == "stdio"
         assert result["sse-server"]["transport"] == "sse"
         assert result["openshift"]["env"]["OC_USER_TOKEN"] == user_token
-        assert (
-            result["sse-server"]["headers"][K8S_AUTH_HEADER] == f"Bearer {user_token}"
-        )
 
     @staticmethod
     def test_dump_client_config_with_streamable_http():
         """Test dump_client_config with streamable HTTP configuration."""
-        mcp_server_configs = [
-            MCPServerConfig(
-                name="streamable-server",
-                transport="streamable_http",
-                streamable_http=StreamableHttpTransportConfig(
-                    url="https://example.com/stream",
-                    headers={"X-Custom-Header": "value"},
-                    timeout=30,
-                    sse_read_timeout=60,
+        file_descriptor, file_path = tempfile.mkstemp(suffix=".tmp")
+        try:
+            with os.fdopen(file_descriptor, "w") as open_file:
+                open_file.write("value")
+            mcp_server_configs = [
+                MCPServerConfig(
+                    name="streamable-server",
+                    transport="streamable_http",
+                    streamable_http=StreamableHttpTransportConfig(
+                        url="https://example.com/stream",
+                        headers={
+                            "X-Custom-Header": file_path,
+                            "kubernetes": KUBERNETES_PLACEHOLDER,
+                        },
+                        timeout=30,
+                        sse_read_timeout=60,
+                    ),
                 ),
-            ),
-        ]
-        user_token = "fake-token"  # noqa: S105
-
-        builder = MCPConfigBuilder(user_token, mcp_server_configs)
-        result = builder.dump_client_config()
+            ]
+            user_token = "fake-token"  # noqa: S105
 
-        assert result["streamable-server"]["transport"] == "streamable_http"
-        assert result["streamable-server"]["url"] == "https://example.com/stream"
-        assert result["streamable-server"]["headers"]["X-Custom-Header"] == "value"
-        assert (
-            result["streamable-server"]["headers"][K8S_AUTH_HEADER]
-            == f"Bearer {user_token}"
-        )
-        # Verify that timeout values are converted to timedelta objects
-        assert result["streamable-server"]["timeout"] == timedelta(seconds=30)
-        assert result["streamable-server"]["sse_read_timeout"] == timedelta(seconds=60)
+            builder = MCPConfigBuilder(user_token, mcp_server_configs)
+            try:
+                result = builder.dump_client_config()
+            except Exception as e:
+                print(f"failed  creating config {e}")
+                assert False
+
+            assert result["streamable-server"]["transport"] == "streamable_http"
+            assert result["streamable-server"]["url"] == "https://example.com/stream"
+            assert result["streamable-server"]["headers"]["X-Custom-Header"] == "value"
+            assert (
+                result["streamable-server"]["headers"]["kubernetes"]
+                == f"Bearer {user_token}"
+            )
+            # Verify that timeout values are converted to timedelta objects
+            assert result["streamable-server"]["timeout"] == timedelta(seconds=30)
+            assert result["streamable-server"]["sse_read_timeout"] == timedelta(
+                seconds=60
+            )
+        finally:
+            os.unlink(file_path)
 
     @staticmethod
     def test_dump_client_config_with_all_transports():
@@ -312,10 +301,3 @@ def test_dump_client_config_with_all_transports():
         assert result["sse-server"]["transport"] == "sse"
         assert result["streamable-server"]["transport"] == "streamable_http"
         assert result["openshift"]["env"]["OC_USER_TOKEN"] == user_token
-        assert (
-            result["sse-server"]["headers"][K8S_AUTH_HEADER] == f"Bearer {user_token}"
-        )
-        assert (
-            result["streamable-server"]["headers"][K8S_AUTH_HEADER]
-            == f"Bearer {user_token}"
-        )