RHOBS-1621: feat: add korrel8r MCP server - #181
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alanconway The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds Korrel8r as an MCP toolset with eight documented tools, configurable URL resolution, Kubernetes-aware authentication transports, MCP server registration, grouped documentation generation, dependency updates, and HTTP-backed integration tests. The token context and certificate-pool helpers are exported. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@iNecas can you review this? It's the MCP server/HTTP client that you suggested. |
The korrel8r MCP server is an HTTP REST client to a korrel8r back-end server. Bearer token is passed in HTTP header to delegate authorization.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/korrel8r/config.go`:
- Around line 93-99: Prevent delegated bearer tokens from reaching redirect
targets in contextAuthTransport.RoundTrip. Configure the associated http.Client
with CheckRedirect to reject cross-origin redirects, or restrict Authorization
injection to requests matching the configured Korrel8r origin; preserve token
injection for same-origin requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 425ce8f8-6b47-45dd-89a8-ffbca3b04b15
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
TOOLS.mdcmd/obs-mcp/main.gogo.modpkg/auth/auth.gopkg/auth/token.gopkg/auth/token_test.gopkg/korrel8r/config.gopkg/korrel8r/config_test.gopkg/mcp/server.gopkg/mcp/tools.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/auth/token.go
- pkg/korrel8r/config_test.go
- cmd/obs-mcp/main.go
- go.mod
- pkg/mcp/tools.go
- TOOLS.md
- pkg/mcp/server.go
| func (t *contextAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| token := auth.ReadTokenFromContext(req.Context()) | ||
| if token != "" { | ||
| req = req.Clone(req.Context()) | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| } | ||
| return t.base.RoundTrip(req) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent bearer-token forwarding to redirect targets.
http.Client follows redirects, and redirected requests retain this context. This transport re-adds Authorization after Go strips cross-host headers, leaking the delegated token to any cross-origin Location. Reject cross-origin redirects via CheckRedirect, or inject the token only when the request origin matches the configured Korrel8r URL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/korrel8r/config.go` around lines 93 - 99, Prevent delegated bearer tokens
from reaching redirect targets in contextAuthTransport.RoundTrip. Configure the
associated http.Client with CheckRedirect to reject cross-origin redirects, or
restrict Authorization injection to requests matching the configured Korrel8r
origin; preserve token injection for same-origin requests.
|
PR needs rebase. DetailsInstructions 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. |
|
Step towards right direction (at least if you want to leverage obs-mcp / openshift-mcp as the delivery vehicle), but I would still recommend going a bit further. We're standardizing on Given we're already acting as part of the delivery mechanism of openshift-mcp server, I'm not much in favor of yet another indirection: we get to the point where openshift-mcp imports from obs-mcp and obs-mcp further imports from korrel8r: coordination is already becoming a challenge (e.g. during the releases or when something changes in the upstream). There are also no e2e tests and evals anywhere for the korrel8r tools. Therefore I would suggest to define the mcp tools here directly, while importing the client code from korrel8r (which at this point would not be mcp-specific). |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return auth.BuildRoundTripper(context.Background(), restConfig, cfg.AuthMode, useTLS, cfg.Insecure) |
There was a problem hiding this comment.
why not call this directly? That should already handle all AuthMode cases and it's not clear to me why korrl8r needs something more?
alanconway
changed the title
|
@alanconway: This pull request references RHOBS-1621 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
This approach seems fine for wrapping non-MCP services as MCP tools, but I don't think it is a sane or scalable approach to dealing with already-existing MCP tools. It is crazy to duplicate all the metadata and logic here. Korrel8r won't be the last case of an already-existing MCP tool that we want to use. I feel like we should go back to the dynamic proxy approach if we can't import the existing Go code. Duplicating it makes no sense to me. Korrel8r does not need an MCP server, it already is an MCP server. That should be something obs-mcp can handle. Note that delegating to an existing MCP server is no more indirection than delegating to an existing HTTP/REST server. Given that obs-mcp is an MCP native tool, surely delegating to MCP should be as easy, if not easier, than delegating to HTTP/REST. |
|
Alternatively: am I missing the purpose of obs-mcp? Is it intended only to wrap non-MCP services, in which case tools like lightspeed should just use Korrel8r directly? |
|
I'm thinking about obs-mcp in this context as one phase to get integrated via openshift-mcp, which by now is the main official delivery mechanism to get the mcp tools into the openshift ecosystem. What you're describing here is more a need for an MCP gateway. While there are efforts going on, I'm still not sure they are the best fit for core product-related tools and would be probably more useful for customers bring-your-own MCP cases. An example of benefit of having the tools collocated more together: Just looking at the list of tools of one component in the context of others make some bad things very obvious, such as the generic-sounding |
No, acting as an MCP gateway is purposeful out of scope for obs-mcp.
Yes and no. Yes: mcp is just another http service so sure, you can treat it as one. But this is not just about the technical mechanism of plumbing things together. It's much more about making sure the things work well with it each other, follow the standards, have evals and e2e test. Every time we introduce part of the mcp definition to be somewhere else, we make it more and more complicated to do any changes and loose flexibility of changing things. Is is the best mechanism that could be? No. But it's the most pragmatic one. You can definitely try getting through the productization check-list on your own and integrate higher on the stack. I'm just not sure how ready the ecosystem is for that kind of integration. You can also try integrating with openshift-mcp directly the very same way obs-mcp is doing. With that obs-mcp at least would not have to act as a man-in-the-middle. Given you would still need to provide the kuberenetes-mcp interface, we could import it in here when needed in standalone obs-mcp cases. |
|
I understand the need to have some uniformity - and that the MCP protocol itself allows some variation. |
3 participants
The korrel8r MCP server is an HTTP REST client to a korrel8r back-end server.
Bearer token is passed in HTTP header to delegate authorization.