NE-2488: Add OpenShift router tools to NetworkEdge toolset#98
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Should we consider having this as a part of the "openshift" toolgroup? |
alebedev87
force-pushed
the
router-toolset
branch
from
899262a to
66029cc
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
@swghosh : That crossed my mind too. In my case "router" doesn't make much sense in Kubernetes context. However I didn't want to complicate things at this stage either. |
alebedev87
force-pushed
the
router-toolset
branch
from
66029cc to
ca4989f
Compare
|
Wherever this ends up we'll likely end up using it as part of our NIDS MCP tooling. https://issues.redhat.com/browse/NE-2278 |
@bentito @alebedev87 any updates on this? or still on discussion. |
openshift-merge-robot
added
the
needs-rebase
|
alebedev87
force-pushed
the
router-toolset
branch
from
ca4989f to
b9d7ed9
Compare
openshift-merge-robot
removed
the
needs-rebase
alebedev87
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@alebedev87: This pull request references NE-2488 which is a valid jira issue. 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. |
alebedev87
force-pushed
the
router-toolset
branch
from
b9d7ed9 to
7871a5b
Compare
|
@alebedev87: This pull request references NE-2488 which is a valid jira issue. 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. |
| Title: "Get Router Config", | ||
| ReadOnlyHint: ptr.To(true), | ||
| DestructiveHint: ptr.To(false), | ||
| OpenWorldHint: ptr.To(true), |
There was a problem hiding this comment.
@bentito : I used false here before but then I copied true as in get_coredns_config tool. I'm not quite sure whether it's a good decision for router configs/sessions/info. Can you please advice?
There was a problem hiding this comment.
Yes true is correct for all three tools, since all three router tools exec into a live, running router pod on the cluster. False would be for a tool that had all the data locally already, not the case here.
(Sorry I missed this comment for quite awhile)
|
I've reviewed this PR in the context of the NIDS MCP strategy and found a few critical areas for improvement, particularly regarding offline analysis. Review Summary: Router Tools vs Offline StrategyContext:
1. Critical Refactoring Required: Offline CompatibilityThe current implementation of
2. Scope Clarification: "Live Only" ToolsThe tools
3. Consistency: Client UsagePR #98 uses |
alebedev87
force-pushed
the
router-toolset
branch
from
7871a5b to
7ce7684
Compare
|
Regarding the first 2 points ( |
|
/assign @bentito |
|
I
I've added haproxy-gather to the list of offline artifacts. So then it might make sense, right? |
|
/assign @matzew |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds NetEdge docs, three new netedge router tools that locate OpenShift router pods and exec into the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Server as MCP Server
participant K8sAPI as Kubernetes API
participant RouterPod as OpenShift Router Pod
Client->>Server: Invoke router tool (optional `pod` param)
alt pod provided
Server->>RouterPod: Exec command in specified pod's `router` container
else pod omitted
Server->>K8sAPI: List pods in `openshift-ingress` with label=`deployment-ingresscontroller=default` and field=`status.phase=Running`
K8sAPI-->>Server: Return pod list
Server->>Server: Select first running router pod
Server->>RouterPod: Exec command in selected pod's `router` container
end
RouterPod-->>Server: Command output
Server->>Server: Format output (section header + fenced code block, include errors inline)
Server-->>Client: Return tool result with formatted output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@alebedev87: This pull request references NE-2488 which is a valid jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openshift/NETEDGE.md`:
- Line 37: The code fences in NETEDGE.md are missing language identifiers
causing markdownlint warnings; update each opening triple-backtick fence at the
four example blocks to include the "text" language tag (i.e., change ``` to
```text) so the blocks at the mentioned example locations render and lint
cleanly.
- Around line 27-29: The note claiming "All tools have an optional `pod`
parameter" is too broad; update the text to state that the optional `pod`
parameter applies only to the router-related tools (e.g., the router command
group / functions) and not to DNS commands like get_coredns_config which have no
parameters; revise the sentence to explicitly mention "router tools" (or list
the router commands) and remove or clarify the implication that
get_coredns_config accepts a `pod` argument so readers won't expect `pod` on DNS
commands.
In `@evals/tasks/netedge/get-router-info/task.yaml`:
- Around line 5-6: The verify step asserts the prose string "HAProxy Version"
but pkg/toolsets/netedge/router.go returns raw "show info" output (fields like
"Name: HAProxy" and "Version:"), so update the task verification to match the
tool contract—replace or broaden the contains check to assert for the raw fields
such as "Name: HAProxy" and/or "Version:" (or a regex that matches
"Name:\s*HAProxy" and "Version:") so valid raw responses are accepted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 368622e6-27f5-4fde-ac33-c34624214f7c
📒 Files selected for processing (6)
docs/openshift/NETEDGE.mdevals/tasks/netedge/get-router-config/task.yamlevals/tasks/netedge/get-router-info/task.yamlevals/tasks/netedge/get-router-sessions/task.yamlpkg/toolsets/netedge/router.gopkg/toolsets/netedge/toolset.go
alebedev87
force-pushed
the
router-toolset
branch
from
f62a70b to
eb2e6d7
Compare
|
@alebedev87: This pull request references NE-2488 which is a valid jira issue. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/toolsets/netedge/router.go (1)
104-129: Consider extracting common handler logic.All three handlers share an identical pattern: extract pod argument → resolve pod → exec command → format result. A helper function could reduce duplication:
func execRouterCommand(params api.ToolHandlerParams, header string, command []string) (*api.ToolCallResult, error) { pod, ok := params.GetArguments()["pod"].(string) if !ok || pod == "" { p, err := getAnyRouterPod(params, defaultIngressControllerName) if err != nil { return api.NewToolCallResult(fmt.Sprintf("# %s\nError getting router pod: %v", header, err), nil), nil } pod = p } out, err := kubernetes.NewCore(params).PodsExec(params.Context, ingressNamespace, pod, routerContainerName, command) if err != nil { return api.NewToolCallResult(fmt.Sprintf("# %s (pod: %s)\nError: %v", header, pod, err), nil), nil } return api.NewToolCallResult(fmt.Sprintf("# %s (pod: %s)\n```\n%s\n```", header, pod, out), nil), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/toolsets/netedge/router.go` around lines 104 - 129, The getRouterConfig handler duplicates the common pattern of resolving a pod arg, falling back to getAnyRouterPod, running kubernetes.NewCore(...).PodsExec and formatting the output; extract that shared logic into a helper (e.g., execRouterCommand) that accepts params, a header string and command []string, calls params.GetArguments() to resolve "pod" (using getAnyRouterPod(defaultIngressControllerName) when empty), executes the command via kubernetes.NewCore(params).PodsExec(params.Context, ingressNamespace, pod, routerContainerName, command), and returns an api.ToolCallResult with either an error block or a fenced output block; then simplify getRouterConfig to call this helper with the header "Router configuration" and the command ["cat", "/var/lib/haproxy/conf/haproxy.config"].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/toolsets/netedge/router.go`:
- Around line 104-129: The getRouterConfig handler duplicates the common pattern
of resolving a pod arg, falling back to getAnyRouterPod, running
kubernetes.NewCore(...).PodsExec and formatting the output; extract that shared
logic into a helper (e.g., execRouterCommand) that accepts params, a header
string and command []string, calls params.GetArguments() to resolve "pod" (using
getAnyRouterPod(defaultIngressControllerName) when empty), executes the command
via kubernetes.NewCore(params).PodsExec(params.Context, ingressNamespace, pod,
routerContainerName, command), and returns an api.ToolCallResult with either an
error block or a fenced output block; then simplify getRouterConfig to call this
helper with the header "Router configuration" and the command ["cat",
"/var/lib/haproxy/conf/haproxy.config"].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a50b86a-a3bc-40ee-b1b7-965ea2a701c9
📒 Files selected for processing (6)
docs/openshift/NETEDGE.mdevals/tasks/netedge/get-router-config/task.yamlevals/tasks/netedge/get-router-info/task.yamlevals/tasks/netedge/get-router-sessions/task.yamlpkg/toolsets/netedge/router.gopkg/toolsets/netedge/toolset.go
✅ Files skipped from review due to trivial changes (4)
- evals/tasks/netedge/get-router-sessions/task.yaml
- evals/tasks/netedge/get-router-config/task.yaml
- docs/openshift/NETEDGE.md
- evals/tasks/netedge/get-router-info/task.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/toolsets/netedge/toolset.go
|
/retest |
Add three new MCP tools for inspecting OpenShift router pods (HAProxy): - get_router_config: retrieves the HAProxy configuration file - get_router_info: retrieves HAProxy runtime information via admin socket - get_router_sessions: retrieves all active HAProxy sessions Each tool accepts an optional pod parameter. When omitted, a Running router pod is automatically selected from the default ingress controller. Also includes evaluation tasks and documentation for the new tools.
|
@alebedev87: This pull request references NE-2488 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 story to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/toolsets/netedge/router.go (1)
104-190: Extract shared router-tool flow to reduce drift and keep behavior consistent.
getRouterConfig,getRouterInfo, andgetRouterSessionsduplicate pod resolution, error/result formatting, and exec flow. A small shared helper would reduce copy/paste bugs and keep future changes (like error propagation or formatting) aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/toolsets/netedge/router.go`:
- Around line 110-113: The code currently appends error text to results and
returns api.NewToolCallResult(strings.Join(results, "\n"), nil) which hides
execution failures; update each failure path (where you currently append error
lines and then return a ToolCallResult with nil error) to return
api.NewToolCallResult(strings.Join(results, "\n"), err), nil so the
ToolCallResult.Error carries the real error, and for successful exec paths
follow the suggested pattern: immediately return on exec errors with the error
populated, otherwise append headers like fmt.Sprintf("# Router configuration
(pod: %s)", pod), wrap the command output between "```" markers into results,
and finally return api.NewToolCallResult(strings.Join(results, "\n"), nil), nil;
apply this change to all occurrences that build results and call
api.NewToolCallResult (the failing pod discovery/exec branches referenced around
the current calls to api.NewToolCallResult).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b1afd9ee-4834-491c-b997-63f104299910
📒 Files selected for processing (6)
docs/openshift/NETEDGE.mdevals/tasks/netedge/get-router-config/task.yamlevals/tasks/netedge/get-router-info/task.yamlevals/tasks/netedge/get-router-sessions/task.yamlpkg/toolsets/netedge/router.gopkg/toolsets/netedge/toolset.go
✅ Files skipped from review due to trivial changes (4)
- evals/tasks/netedge/get-router-info/task.yaml
- docs/openshift/NETEDGE.md
- evals/tasks/netedge/get-router-sessions/task.yaml
- evals/tasks/netedge/get-router-config/task.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/toolsets/netedge/toolset.go
| initExecDNSInPod(), | ||
| initRouter(), | ||
| ) |
| @@ -0,0 +1,204 @@ | |||
| package netedge | |||
There was a problem hiding this comment.
The initRouter() function and getAnyRouterPod() use tabs (matching the project style), but all three handler functions (getRouterConfig, getRouterInfo and getRouterSessions) use spaces. This is in a single file. The linter passed, so Go doesn't care, but it's inconsistent and will produce noisy diffs if someone later reformats.
IMO aA quick gofmt should fix it?
| | `get_router_config` | Retrieve the current router's HAProxy configuration | `pod` (optional) - Router pod name | | ||
| | `get_router_info` | Retrieve HAProxy runtime information from the router | `pod` (optional) - Router pod name | | ||
| | `get_router_sessions` | Retrieve all active sessions from the router | `pod` (optional) - Router pod name | |
There was a problem hiding this comment.
get_router_config dumps the full HAProxy config and get_router_sessions dumps all sessions. On a busy cluster these could be very large. No truncation or pagination.
This could hit MCP response size limits or flood the AI context window. Not a blocker for initial merge but worth a follow-up?
There was a problem hiding this comment.
Good point! I'll add a story to follow up on this. Thanks.
| assertions: | ||
| toolsUsed: | ||
| - server: kubernetes | ||
| toolPattern: "netedge__get_router_sessions" |
There was a problem hiding this comment.
The toolPattern uses a netedge prefix, but tool names are registered in MCP without toolset prefixes - the actual tool name is just get_router_config. Since toolPattern is a regex match against the raw tool name, netedge__get_router_config will never match. Same issue the other filez. All three should drop the netedge__ prefix.
|
/override "Konflux kflux-prd-rh02 / openshift-mcp-server-on-pull-request" |
|
@matzew: Overrode contexts on behalf of matzew: Konflux kflux-prd-rh02 / openshift-mcp-server-on-pull-request 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 kubernetes-sigs/prow repository. |
|
@alebedev87: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/override "Konflux kflux-prd-rh02 / openshift-mcp-server-on-pull-request" |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, matzew The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@matzew: Overrode contexts on behalf of matzew: Konflux kflux-prd-rh02 / openshift-mcp-server-on-pull-request 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 kubernetes-sigs/prow repository. |
openshift-merge-bot
Bot
merged commit 0286feb
into
openshift:main
6 participants
This PR introduces new tools for inspecting OpenShift router pods through the Kubernetes MCP Server.
Added tools:
get_router_config: View the router's configurationget_router_info: Get router runtime information and statisticsget_router_sessions: View all active sessions in the routerDocumentation: Added
NETEDGE.mdcovering the NetworkEdge toolset, including both router and CoreDNS tools.Tests: Added evaluation tasks for the added tools.
Summary by CodeRabbit
New Features
Documentation
Tests