Skip to content

feat: Implement dynamic MCP server integration with authentication#121

Merged
tisnik merged 2 commits into
lightspeed-core:mainfrom
eranco74:mcp_servers
Jun 25, 2025
Merged

feat: Implement dynamic MCP server integration with authentication#121
tisnik merged 2 commits into
lightspeed-core:mainfrom
eranco74:mcp_servers

Conversation

@eranco74
Copy link
Copy Markdown
Contributor

Description

  • Add ModelContextProtocolServer configuration model
  • Add MCP server registration during app startup lifecycle
  • Update query handler to use configured MCP tools with authentication
    • Integrate Agent with dynamic MCP tool registration from config
    • Implement access token forwarding to MCP servers via headers
  • Refactor auth dependency to extract and return access tokens
  • Migrate from startup events to lifespan context manager

This enables dynamic registration and authentication of multiple MCP servers through configuration without hardcoded endpoints.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Comment thread src/app/endpoints/query.py
Comment thread src/app/endpoints/query.py Outdated
Comment thread src/models/config.py
Comment thread src/utils/common.py Outdated
@manstis
Copy link
Copy Markdown
Contributor

manstis commented Jun 24, 2025

I welcome this PR as we're dynamically registering MCP Severs with a llama-stack server.

In order to move to lightspeed-stack we will also need this facility.

I wonder however whether the authentication should/could be moved to a separate PR.

The approach proposed in this PR does not fulfil our requirements (of providing different headers for different MCP servers).

@eranco74
Copy link
Copy Markdown
Contributor Author

we're dynamically registering MCP Severs with a llama-stack server.

In order to move to lightspeed-stack we will also need this facility.

I wonder however whether the authentication should/could be moved to a sep

Thanks for the review.
You are right that the authentication pice cover one case and won't work for mcp servers that require anything other than access token, yet I added it because without it we can register MCP servers all we want but as long they require any auth it won't work, so I figured it's better start with something basic that works e2e.

@eranco74 eranco74 force-pushed the mcp_servers branch 6 times, most recently from 4cbd2cd to 31ad6d8 Compare June 25, 2025 09:53
@tisnik
Copy link
Copy Markdown
Contributor

tisnik commented Jun 25, 2025

@eranco74 if you like, I will add you as maintainer for the core, so all the CI tests will run automatically. Agreed?

@eranco74 eranco74 force-pushed the mcp_servers branch 2 times, most recently from e404f65 to 3882c25 Compare June 25, 2025 11:02
Copy link
Copy Markdown
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet LGTM 👍

However, since I'm not a maintainer of lightspeed-stack defer approval to @tisnik

Copy link
Copy Markdown
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks ok!
Please:

  1. change from draft to ready for review state
  2. accept invitation, so your changes will trigger CI (TBH dunno if it works for existing PRs)
  3. try to fix linter errors, seems like pretty straightforward

LGTM instead

@eranco74 eranco74 marked this pull request as ready for review June 25, 2025 12:44
@eranco74 eranco74 requested a review from tisnik June 25, 2025 12:45
eranco74 added 2 commits June 25, 2025 15:49
- Add ModelContextProtocolServer configuration model
- Add MCP server registration during app startup lifecycle
- Update query handler to use configured MCP tools with authentication
  - Integrate Agent with dynamic MCP tool registration from config
  - Implement access token forwarding to MCP servers via headers
- Refactor auth dependency to extract and return access tokens
- Migrate from startup events to lifespan context manager

This enables dynamic registration and authentication of multiple MCP
servers through configuration without hardcoded endpoints.

Signed-off-by: Eran Cohen <eranco@redhat.com>
 * Configuration loading and access
 * Server registration logic
 * Integration with query processing
 * Authorization token propagation
 * Error handling scenarios

Signed-off-by: Eran Cohen <eranco@redhat.com>
Copy link
Copy Markdown
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@tisnik tisnik merged commit 6c9b887 into lightspeed-core:main Jun 25, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants