-
Notifications
You must be signed in to change notification settings - Fork 28
[LEADS-25] Support new MCP header #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,11 +41,21 @@ api: | |
| # API input configuration | ||
| provider: "openai" # LLM provider for queries | ||
| model: "gpt-4o-mini" # Model to use for queries | ||
| no_tools: null # Whether to bypass tools and MCP servers (optional) | ||
| no_tools: false # Whether to bypass tools and MCP servers (optional) | ||
| system_prompt: null # System prompt (default None) | ||
|
|
||
| cache_dir: ".caches/api_cache" # Directory with lightspeed-stack cache | ||
| cache_enabled: true # Is lightspeed-stack cache enabled? | ||
|
|
||
| # MCP Server Authentication Configuration | ||
| mcp_headers: | ||
| enabled: true # Enable MCP headers functionality | ||
| servers: # MCP server configurations | ||
| filesystem-tools: | ||
| auth_type: bearer # Authentication type: only bearer is supported | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a plan to support anything else in future ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. afaik only bearer is supported currently. probably we can add more in the future. I can add a placeholder
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am primarily trying to understand is it ever going to be implemented in LCORE, if not why do we need to keep auth_type..
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. im not really sure what if you want information on how our MCP authentication is done the documentation is up to date :). I am also happy to answer any questions about the status of auth in LS stack. I will look more into what this PR entails on Monday so that I can get a better grasp of the context of any questions y'all might have :). |
||
| env_var: API_KEY # Environment variable containing the token/key | ||
|
|
||
| # Legacy authentication (fallback when mcp_headers.enabled is false) | ||
| # Authentication via API_KEY environment variable only for MCP server | ||
|
Comment on lines
+58
to
59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fallback comment is narrower than runtime behavior. The comment says fallback happens only when 🤖 Prompt for AI Agents |
||
|
|
||
| # Retry configuration for 429 Too Many Requests API errors | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,8 +107,6 @@ embedding: | |
| This section configures the inference API for generating the responses. It can be any Lightspeed-Core compatible API. | ||
| Note that it can be easily integrated with other APIs with a minimal change. | ||
|
|
||
| Authentication via `API_KEY` environment variable only for MCP server. | ||
|
|
||
| | Setting (api.) | Default | Description | | ||
| |----------------|---------|-------------| | ||
| | enabled | `"true"` | Enable/disable API calls | | ||
|
|
@@ -121,6 +119,30 @@ Authentication via `API_KEY` environment variable only for MCP server. | |
| | system_prompt | `null` | Custom system prompt (optional) | | ||
| | cache_dir | `".caches/api_cache"` | Directory with cached API responses | | ||
| | cache_enabled | `true` | Is API cache enabled? | | ||
| | mcp_headers | `null` | MCP headers configuration for authentication (see below) | | ||
| | num_retries | `3` | Maximum number of retry attempts for API calls on 429 errors | | ||
|
|
||
| ### MCP Server Authentication | ||
|
|
||
| The framework supports two methods for MCP server authentication: | ||
|
|
||
| #### 1. New MCP Headers Configuration (Recommended) | ||
| The `mcp_headers` configuration provides a flexible way to configure authentication for individual MCP servers: | ||
|
|
||
| | Setting (api.mcp_headers.) | Default | Description | | ||
| |----------------------------|---------|-------------| | ||
| | enabled | `true` | Enable/disable MCP headers functionality | | ||
| | servers | `{}` | Dictionary of MCP server configurations | | ||
|
|
||
| For each server in `servers`, you can configure: | ||
|
|
||
| | Setting (api.mcp_headers.servers.<server_name>.) | Default | Description | | ||
| |---------------------------------------------------|---------|-------------| | ||
| | auth_type | `"bearer"` | Authentication type (currently only "bearer" is supported) | | ||
| | env_var | required | Environment variable containing the authentication token | | ||
|
|
||
| #### 2. Legacy Authentication (Fallback) | ||
| When `mcp_headers.enabled` is `false`, the system falls back to using the `API_KEY` environment variable for all MCP server authentication. | ||
|
|
||
| ### API Modes | ||
|
|
||
|
|
@@ -137,6 +159,8 @@ Authentication via `API_KEY` environment variable only for MCP server. | |
| - **Reproducible results**: Same response data used across runs | ||
| ### Example | ||
|
|
||
| #### Example Configuration | ||
|
|
||
| ```yaml | ||
| api: | ||
| enabled: true | ||
|
|
@@ -150,8 +174,37 @@ api: | |
| system_prompt: null | ||
| cache_dir: ".caches/api_cache" | ||
| cache_enabled: true | ||
| num_retries: 3 | ||
|
|
||
| # MCP Server Authentication Configuration | ||
| mcp_headers: | ||
| enabled: true # Enable MCP headers functionality | ||
| servers: # MCP server configurations | ||
| filesystem-tools: | ||
| auth_type: bearer # Authentication type: only bearer is supported | ||
| env_var: API_KEY # Environment variable containing the token/key | ||
| another-mcp-server: | ||
| auth_type: bearer | ||
| env_var: ANOTHER_API_KEY # Use a different environment variable | ||
| ``` | ||
|
|
||
| #### Lightspeed Stack API Compatibility | ||
|
|
||
| **Important Note for lightspeed-stack API users**: To use the MCP headers functionality with the lightspeed-stack API, you need to modify the `llama_stack_api/openai_responses.py` file in your lightspeed-stack installation: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any idea, when will this be fixed in lls ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this was the outcome of llama-stack api implementation change.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the fix has been merged in lightspeed-stack. However this an existing bug that has yet to be solved where passing authorization headers will only work in server mode and will not work in library mode. Hopefully a fix will be coming soon but in the meantime if you could just add that limitation to the documentation that would be great! |
||
|
|
||
| In the `OpenAIResponsesToolMCP` class, change the `authorization` parameter's `exclude` field from `True` to `False`: | ||
|
|
||
| ```python | ||
| # In llama_stack_api/openai_responses.py | ||
| class OpenAIResponsesToolMCP: | ||
| authorization: Optional[str] = Field( | ||
| default=None, | ||
| exclude=False # Change this from True to False | ||
| ) | ||
| ``` | ||
|
|
||
| This change allows the authorization headers to be properly passed through to MCP servers. | ||
|
|
||
| ## Metrics | ||
| Metrics are enabled globally (as described below) or within the input data for each individual conversation or individual turn (question/answer pair). To enable a metrics globally you need to set `default` meta data attribute to `true` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -91,14 +91,62 @@ def _setup_client(self) -> None: | |||||||||||||||
| ) | ||||||||||||||||
| self.client.headers.update({"Content-Type": "application/json"}) | ||||||||||||||||
|
|
||||||||||||||||
| # Use API_KEY environment variable for authentication | ||||||||||||||||
| api_key = os.getenv("API_KEY") | ||||||||||||||||
| if api_key and self.client: | ||||||||||||||||
| self.client.headers.update({"Authorization": f"Bearer {api_key}"}) | ||||||||||||||||
|
Comment on lines
-94
to
-97
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would suggest retain this but when mcp header is disabled or not present. for backward compatibility
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am retaining below in the fallback code
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fallback is different from backward compatibility. This got removed by this PR. So it will fail for the teams who are using this older method
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see. good catch! |
||||||||||||||||
| # Set up MCP headers based on configuration | ||||||||||||||||
| mcp_headers_dict = self._build_mcp_headers() | ||||||||||||||||
| if mcp_headers_dict: | ||||||||||||||||
| self.client.headers.update( | ||||||||||||||||
| {"MCP-HEADERS": json.dumps(mcp_headers_dict)} | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| except Exception as e: | ||||||||||||||||
| raise APIError(f"Failed to setup API client: {e}") from e | ||||||||||||||||
|
|
||||||||||||||||
| def _build_mcp_headers(self) -> dict[str, dict[str, str]]: | ||||||||||||||||
| """Build MCP headers based on configuration. | ||||||||||||||||
|
|
||||||||||||||||
| Returns: | ||||||||||||||||
| Dictionary of MCP server headers, or empty dict if none configured. | ||||||||||||||||
| """ | ||||||||||||||||
| # Use new MCP headers configuration if available and enabled | ||||||||||||||||
| if ( | ||||||||||||||||
| self.config.mcp_headers | ||||||||||||||||
| and self.config.mcp_headers.enabled | ||||||||||||||||
| and self.config.mcp_headers.servers | ||||||||||||||||
| ): | ||||||||||||||||
|
|
||||||||||||||||
| mcp_headers = {} | ||||||||||||||||
| for server_name, server_config in self.config.mcp_headers.servers.items(): | ||||||||||||||||
| # Get token from environment variable | ||||||||||||||||
| token = os.getenv(server_config.env_var) | ||||||||||||||||
| if not token: | ||||||||||||||||
| logger.warning( | ||||||||||||||||
| "Environment variable '%s' not found " | ||||||||||||||||
| "for MCP server '%s'. Skipping authentication.", | ||||||||||||||||
| server_config.env_var, | ||||||||||||||||
| server_name, | ||||||||||||||||
| ) | ||||||||||||||||
| continue | ||||||||||||||||
|
|
||||||||||||||||
| # Set up bearer auth headers | ||||||||||||||||
| header_name = server_config.header_name or "Authorization" | ||||||||||||||||
| header_value = f"Bearer {token}" | ||||||||||||||||
|
|
||||||||||||||||
| mcp_headers[server_name] = {header_name: header_value} | ||||||||||||||||
|
|
||||||||||||||||
| if not mcp_headers: | ||||||||||||||||
| raise APIError( | ||||||||||||||||
| "MCP headers are enabled, but no valid server credentials were resolved. " | ||||||||||||||||
| "Check mcp_headers.servers and required environment variables." | ||||||||||||||||
| ) | ||||||||||||||||
| return mcp_headers | ||||||||||||||||
|
|
||||||||||||||||
| # Fallback to legacy API_KEY behavior for backward compatibility | ||||||||||||||||
| api_key = os.getenv("API_KEY") | ||||||||||||||||
| if api_key: | ||||||||||||||||
| return {"filesystem-tools": {"Authorization": f"Bearer {api_key}"}} | ||||||||||||||||
|
|
||||||||||||||||
| return {} | ||||||||||||||||
|
Comment on lines
+143
to
+148
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are assuming that the server is filesystem-tools when mcp header is not given.. This is not true. Currently we don't have to set the server name.. I would suggest to simply set empty {} and handle it separately the way it was done earlier (previous comment)
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| def query( | ||||||||||||||||
| self, | ||||||||||||||||
| query: str, | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,49 @@ def _validate_provider(cls, v: str) -> str: | |
| return v | ||
|
|
||
|
|
||
| class MCPServerConfig(BaseModel): | ||
| """Configuration for a single MCP server authentication.""" | ||
|
|
||
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| auth_type: str = Field( | ||
| default="bearer", | ||
| description="Authentication type: only bearer is supported", | ||
| ) | ||
| env_var: str = Field( | ||
| ..., | ||
| min_length=1, | ||
| description="Environment variable containing the token/key", | ||
| ) | ||
|
Comment on lines
+172
to
+176
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add validation that env variable is set when mcp header is enabled.. |
||
| header_name: Optional[str] = Field( | ||
| default=None, | ||
| description="Custom header name (optional, defaults based on auth_type)", | ||
| ) | ||
|
|
||
| @field_validator("auth_type") | ||
| @classmethod | ||
| def validate_auth_type(cls, v: str) -> str: | ||
| """Validate auth_type is supported.""" | ||
| if v != "bearer": | ||
| raise ValueError("auth_type must be 'bearer'") | ||
| return v | ||
|
|
||
|
|
||
| class MCPHeadersConfig(BaseModel): | ||
| """Configuration for MCP headers functionality.""" | ||
|
|
||
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| enabled: bool = Field( | ||
| default=True, | ||
| description="Enable MCP headers functionality", | ||
| ) | ||
| servers: dict[str, MCPServerConfig] = Field( | ||
| default_factory=dict, | ||
| description="MCP server configurations", | ||
| ) | ||
|
Comment on lines
+196
to
+203
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Require non-empty Line 198 currently allows 🔧 Proposed fix class MCPHeadersConfig(BaseModel):
"""Configuration for MCP headers functionality."""
@@
servers: dict[str, MCPServerConfig] = Field(
default_factory=dict,
description="MCP server configurations",
)
+
+ `@model_validator`(mode="after")
+ def validate_enabled_servers(self) -> "MCPHeadersConfig":
+ """Ensure enabled MCP headers have at least one configured server."""
+ if self.enabled and not self.servers:
+ raise ConfigurationError(
+ "mcp_headers.enabled is true, but no MCP servers are configured."
+ )
+ return self🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class APIConfig(BaseModel): | ||
| """API configuration for dynamic data generation.""" | ||
|
|
||
|
|
@@ -196,6 +239,9 @@ class APIConfig(BaseModel): | |
| cache_enabled: bool = Field( | ||
| default=True, description="Is caching of lightspeed-stack queries enabled?" | ||
| ) | ||
| mcp_headers: Optional[MCPHeadersConfig] = Field( | ||
| default=None, description="MCP headers configuration for authentication" | ||
| ) | ||
| num_retries: int = Field( | ||
| default=DEFAULT_API_NUM_RETRIES, | ||
| ge=0, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep it is as null..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.