Skip to content

Conversation

@ldjebran
Copy link
Contributor

@ldjebran ldjebran commented Jul 8, 2025

Description

issue: https://issues.redhat.com/browse/LCORE-312
this supports the client providing the headers to supply to mcp servers via llama-stack in the form of a dictionary like:
{
"mcp-server-url-1": {"HEADER-1": "HEADER-1-VALUE-1", "HEADER-1": "HEADER-1-VALUE-2"},
"mcp-server-url-2": {"HEADER-2": "HEADER-2-VALUE"}
}

Suppose each different mcp server support it's own authentication type, or needs some headers to be supplied, this PR will allow to fine tune which headers may be passed to which mcp server.

For the moment did not moved the extra_headers to create_turn function as the AsyncAgent is not supporting that.

moved the toolgroups to create_turn function as when declared at the agent, the headers are not sent to mcp servers the first time llama-stack is listing the tools from mcp servers, that may lead to authentication failure.

The optimal is to move also the headers to be declared in create_turn function and not at the agent level, but this should be done in it's own task ticket. as we weed to implement the same for AsyncAgent. This will allow us to have one agent instance for all sessions and remove the agent caching.

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

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

Tested with 2 mcp servers that support authentication, the mcp servers does not allow tools listing without authentication too, put in configuration file lightspeed-stack.yaml

...
mcp_servers:
- name: mcp::aap-controller
  provider_id: model-context-protocol
  url: http://localhost:8004/sse
- name: mcp::aap-gateway
  provider_id: model-context-protocol
  url: http://localhost:8003/sse
...

The llama-stack was running as an external service by default using model "granite-3.3-8b-instruct" via vllm container runing in openshift

Created 2 scripts one for streaming and one for non streaming endpoints:

lightspeed-stack-mco-headers-2025-07-08.at.11.17.03.mp4

@ldjebran ldjebran added the enhancement New feature or request label Jul 8, 2025
@ldjebran ldjebran force-pushed the add-mcp-headers branch 2 times, most recently from b874043 to 31b367c Compare July 8, 2025 09:33
@ldjebran ldjebran requested review from manstis and tisnik July 8, 2025 09:46
Copy link
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.

LGTM 👍

I think there's a small flaw in part of your logic.

Have a look and let me know where I've misunderstood 🤣

Copy link
Contributor

@umago umago left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, left a question inline

Copy link
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.

LGTM 👍

issue: https://issues.redhat.com/browse/LCORE-312
this supports the client providing the headers to supply to mcp servers via llama-stack in the form of a dictionary like
mcp_headers = {
    "mcp-server-url-1": {"HEADER-1": "HEADER-1-VALUE-1", "HEADER-1": "HEADER-1-VALUE-2"},
    "mcp-server-url-2": {"HEADER-2": "HEADER-2-VALUE"}
}

Signed-off-by: Djebran Lezzoum <ldjebran@gmail.com>
@manstis
Copy link
Contributor

manstis commented Jul 8, 2025

@umago Did you want another look to approve before this is merged?

Copy link
Contributor

@umago umago left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@umago
Copy link
Contributor

umago commented Jul 8, 2025

@umago Did you want another look to approve before this is merged?

Sorry, just took another look at the patch. It LGTM! Thank you

@ldjebran ldjebran merged commit 1e28b2d into lightspeed-core:main Jul 8, 2025
16 checks passed
Copy link
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 good, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants