removed additional mcp transport, streamlined MCP Auth resolution #2705

blublinsky · 2026-01-20T17:09:44Z

Description

Type of change

Related Tickets & Documents

Related Issue #
OLS-2275
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.

blublinsky · 2026-01-22T08:49:15Z

/retest

onmete · 2026-01-22T09:40:24Z

ols/app/models/config.py

-    timeout: int = constants.SSE_TRANSPORT_DEFAULT_TIMEOUT
-    sse_read_timeout: int = constants.SSE_TRANSPORT_DEFAULT_READ_TIMEOUT
-    headers: dict[str, str] = Field(default_factory=dict)
+    authorization_headers: dict[str, str] = Field(


Can we rename this to just headers? I understand we use it for auth purposes now, but it can be any other arbitrary headers.

We can, but this was original name. To do this, we also need to change operator. So unless you absolutely want this, I would prefer to leave it

Where? I see you changed it in one of the lcore func in operator, but this service accepts only headers in the current head.

in the operator internal/controller/appserver/assets.go, line 783

I'm not sure if that is the right reference: https://github.com/openshift/lightspeed-operator/blob/main/internal/controller/appserver/assets.go#L783C7-L783C122
Currently it produces this config:

mcp_servers: - name: "openshift" transport: "streamable_http" streamable_http: url: "http://localhost:8080/mcp" timeout: 60 sse_read_timeout: 30 headers: Authorization: "kubernetes"

The authorization_headers is used for lcore config.

onmete · 2026-01-22T09:46:26Z

ols/app/models/config.py

+    """

+    name: str = Field(
+        ...,


The Field should work without the Ellipsis, eg. just

name: str = Field( title="MCP name", description="MCP server name that must be unique", )

It is required field. Thats Pydantic convention to use Elipsees

It is required for pydantic < 2

onmete · 2026-01-22T09:47:28Z

ols/app/models/config.py

+        """Resolved authorization headers (computed from authorization_headers)."""
+        return self._resolved_authorization_headers
+
+    timeout: Optional[int] = Field(


Please move up to the rest of the class attributes - before the private attr and property

onmete · 2026-01-22T09:49:44Z

ols/app/models/config.py

-    sse: Optional[SseTransportConfig] = None
-    streamable_http: Optional[StreamableHttpTransportConfig] = None
+    embed_model: str = Field(
+        default="sentence-transformers/all-mpnet-base-v2",


Have you tested other (smaller) models? Or we just use the same thing we already have available?

onmete · 2026-01-22T09:50:00Z

ols/app/models/config.py

-    sse_read_timeout: int = constants.STREAMABLE_HTTP_TRANSPORT_DEFAULT_READ_TIMEOUT
-    headers: dict[str, str] = Field(default_factory=dict)

+class ToolFilteringConfig(BaseModel):


This doesn't need to be in this PR, right?

I am synching operator with service. Its question of 4 PRs vs 2

Are you confident that this config won't change with the actual implementation of the tool filtering? It would make sense to me to have this as part of the actual implementation, but if you tell me this is stable lets proceed.

onmete · 2026-01-22T09:51:14Z

ols/app/models/config.py


    proxy_config: Optional[ProxyConfig] = None

+    tool_filtering: Optional[ToolFilteringConfig] = None


Is it a feature of mcp servers or ols? Answer implies position in the config.

ols - tools filtering is an OLS property

Right, the lines were truncated in a way that it seems like it is under MCPConfig, my bad.

onmete · 2026-01-22T09:58:04Z

ols/utils/checks.py

+    resolved: dict[str, str] = {}
+
+    for header_name, header_value in authorization_headers.items():
+        match header_value.strip():


Should it also be case-insensitive for special values "kubernetes" and "client"?

Its using constants now

Would both of these work the same?

mcp_servers: - name: openshift url: "http://localhost:8989/mcp" headers: bla: "kubernetes"

and

mcp_servers: - name: openshift url: "http://localhost:8989/mcp" headers: bla: "KUBERNETES"

asamal4 · 2026-01-23T07:27:40Z

ols/app/models/config.py

-                    "config should not be provided"
-                )
-        return self
+    embed_model: str = Field(


Probably this should be a comment for operator changes.
is this going to be user facing ? We will probably end up using same embedding model what we package as part of RAG, considering on-prem scenario.

As far service is concerned, It is not aligned with the RAG embedding model property. This is in-consistent

This is a completely separate RAG. So technically here we can use a completely different model. In general I do not foresee users ever changing it, but just in case it is there. And of course it is aligned with the operator

I am specifically talking about on-prem scenario where we can not download the model from huggingface. Model has to be picked from file-system/volume. For RAG use-case we are handling this. So similar processing we need to do for tool rag also. In this case we will provide a path to the embedding model..

Regarding parameter name inconsistency in Service/API: Agree that this embedding model is for different purpose (and can be a different model), but IMO we should keep same parameter name in both places. For RAG the name is embeddings_model_path and here we are using embed_model. But this is not very critical. It's up to you.

blublinsky · 2026-01-23T21:59:15Z

/retest

…ined MCP Auth resolution

blublinsky · 2026-01-25T10:38:50Z

ci/prow/e2e-ols-cluster failure is expected for the following tests:
azure_openai_tool_calling
openai_tool_calling
watsonx_tool_calling
These tests add MCP configuration, but the operator is still creating old configuration. As a result app server fails to start properly

bparees · 2026-01-26T18:26:53Z

README.md

+    url: http://api-service:8080
+    authorization_headers:
+      Authorization: /var/secrets/api-token    # Path to file containing token
+      X-API-Key: /var/secrets/api-key          # Multiple headers supported


nit, but maybe make this look more like a traditional header name?

bparees · 2026-01-26T18:27:27Z

README.md

+
+#### 2. Kubernetes Token (User Context)
+
+Use the special `kubernetes` placeholder to automatically inject the authenticated user's Kubernetes token. This requires the `k8s` authentication module:


describe what header(name, format) is passed to the MCP server when this is used.

It can apply to any header name. If you are asking that it has to be Authorization, the answer is no

So in the sample:

authorization_headers: Authorization: kubernetes

Authorization is the name of the header that will be passed to the mcp server? So can i write:

authorization_headers: my-custom-header: kubernetes

?

If so, there is a lot of implicit behavior here that I can't say i love.

Explicit behavior would be something like:

authorization_headers: k8s-token: headername: Authorization custom-header: headername: my-custom-header headervalue: /path/to/secret

(And if either of those subsections aren't populated, that behavior isn't enabled)

but at a minimum if you're going to keep it this way, more doc is needed please

header_name: kubernetes token

bparees · 2026-01-26T18:38:31Z

README.md

+
+#### 2. Kubernetes Token (User Context)
+
+Use the special `kubernetes` placeholder to automatically inject the authenticated user's Kubernetes token. This requires the `k8s` authentication module:


So in the sample:

authorization_headers: Authorization: kubernetes

Authorization is the name of the header that will be passed to the mcp server? So can i write:

authorization_headers: my-custom-header: kubernetes

?

If so, there is a lot of implicit behavior here that I can't say i love.

Explicit behavior would be something like:

authorization_headers: k8s-token: headername: Authorization custom-header: headername: my-custom-header headervalue: /path/to/secret

(And if either of those subsections aren't populated, that behavior isn't enabled)

but at a minimum if you're going to keep it this way, more doc is needed please

bparees · 2026-01-26T18:39:35Z

README.md

+    url: http://api-service:8080
+    authorization_headers:
+      Authorization: /var/secrets/api-token    # Path to file containing token
+      X-API-Key: /var/secrets/api-key          # Multiple headers supported


also how do multiple headers work? How do you specify more than one, and does each one have a different path to a file containing its token? how are they linked?

you can specify as many as you want, each with its own path. they are all added as headers to the MCP server calls

Yeah but how do you specify more than one? This example seems to imply it's not a yaml array, so what's the actual schema to do that?

ah. you are looking at the service, that is pure python executable driven by configuration. Yaml is used by the operator openshift/lightspeed-operator#1222

ah. you are looking at the service, that knows nothing about yaml. jaml is in the operator.

openshift-ci · 2026-01-26T20:51:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joshuawilson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

onmete

Lets resolve pending comments and proceed with this.

onmete · 2026-01-27T17:55:58Z

ols/app/models/config.py

+        ),
+    )

+    headers: dict[str, str] = Field(


So you kept it as headers, right? Can you please update the title and rest of the refs (docs) that are using authorization_headers?

onmete · 2026-01-27T18:02:58Z

README.md

+  - name: github
+    url: http://github-mcp-server:8080
+    headers:
+      Authorization: client  # Client provides token via MCP-HEADERS header


Can you pretty please explain why we need to bring this to the OLS before we migrate to LCORE? Do you see a usecase/need prior to that?

because

We are making a change in the operator, which is not compatible with the old configuration. Its easier to make a change here to keep an operator clean

This makes the code smaller and more streamlined

It makes it simpler to wire tools RAG, next PRs

There is an important corner case in toolsRag implementation related to the client's headers. I would like to make sure I do it correctly here before moving it to LCore

I have my reservations here, but for the sake of progressing, let's move on.

onmete · 2026-01-27T18:08:34Z

So in the sample:

authorization_headers:
Authorization: kubernetes
Authorization is the name of the header that will be passed to the mcp server? So can i write:

authorization_headers:
my-custom-header: kubernetes
?

If so, there is a lot of implicit behavior here that I can't say i love.

Explicit behavior would be something like:

authorization_headers:
k8s-token:
headername: Authorization
custom-header:
headername: my-custom-header
headervalue: /path/to/secret
(And if either of those subsections aren't populated, that behavior isn't enabled)

but at a minimum if you're going to keep it this way, more doc is needed please

Why I can't just reply and continue the conversation as a thread here - github ux is driving me nuts...

Anyway, @bparees I agree that this would be more explicit for users. We can consider using this format in the user-facing OLSConfig CR. But changing this on the config level would require to also change it in LCORE side, which might (or might not) break someone else on LCORE that already uses it.
WDYT @blublinsky?

blublinsky · 2026-01-27T19:02:35Z

the user facing definition openshift/lightspeed-operator#1222 describes it clearer (I hope) and even contains examples

openshift-ci · 2026-01-29T11:06:35Z

@blublinsky: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-ols-cluster	`38ab1d6`	link	true	`/test e2e-ols-cluster`
ci/prow/ols-evaluation	`38ab1d6`	link	true	`/test ols-evaluation`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2026

openshift-ci bot requested review from raptorsun and tisnik January 20, 2026 17:10

blublinsky force-pushed the mcp-simplification branch from c00804a to d92f62a Compare January 20, 2026 17:23

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2026

blublinsky force-pushed the mcp-simplification branch 3 times, most recently from 19fcc14 to 65d866f Compare January 21, 2026 14:59

blublinsky changed the title ~~removed additional mcp transport, added MCP headers to query, streaml…~~ removed additional mcp transport, streamlined MCP Auth resolution Jan 21, 2026

blublinsky force-pushed the mcp-simplification branch from 65d866f to 525d1be Compare January 21, 2026 15:25

onmete reviewed Jan 22, 2026

View reviewed changes

blublinsky force-pushed the mcp-simplification branch 2 times, most recently from 0209b07 to 6ec8fa5 Compare January 22, 2026 13:19

xrajesh requested a review from asamal4 January 22, 2026 15:53

asamal4 reviewed Jan 23, 2026

View reviewed changes

blublinsky force-pushed the mcp-simplification branch 2 times, most recently from d670d7d to 012bebe Compare January 23, 2026 15:29

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2026

blublinsky added 2 commits January 23, 2026 22:04

removed additional mcp transport, added MCP headers to query, streaml…

aa272f5

…ined MCP Auth resolution

addressed review comments

df28668

blublinsky force-pushed the mcp-simplification branch from cec1910 to df28668 Compare January 23, 2026 22:06

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2026

bparees reviewed Jan 26, 2026

View reviewed changes

bparees mentioned this pull request Jan 26, 2026

LCORE passes the k8s token as a standard Authorization header rhobs/obs-mcp#14

Merged

onmete reviewed Jan 27, 2026

View reviewed changes

readme update

38ab1d6

blublinsky force-pushed the mcp-simplification branch from 5b48908 to 38ab1d6 Compare January 29, 2026 09:17


		proxy_config: Optional[ProxyConfig] = None

		tool_filtering: Optional[ToolFilteringConfig] = None


		#### 2. Kubernetes Token (User Context)

		Use the special `kubernetes` placeholder to automatically inject the authenticated user's Kubernetes token. This requires the `k8s` authentication module:

removed additional mcp transport, streamlined MCP Auth resolution #2705

Are you sure you want to change the base?

removed additional mcp transport, streamlined MCP Auth resolution #2705

Uh oh!

Conversation

blublinsky commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

blublinsky commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asamal4 Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asamal4 Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asamal4 Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blublinsky commented Jan 23, 2026

Uh oh!

blublinsky commented Jan 25, 2026

blublinsky commented Jan 20, 2026 •

edited

Loading

asamal4 Jan 23, 2026 •

edited

Loading

asamal4 Jan 23, 2026 •

edited

Loading

asamal4 Jan 23, 2026 •

edited

Loading