Operator improvements #8
Conversation
WalkthroughThis update introduces new RBAC ClusterRole and ClusterRoleBinding resources to grant read-only access for the MCP server's default service account, modifies controller logic to watch additional Kubernetes resources with label-based filtering, adjusts server deployment arguments and route configuration, adds a demo agent script with dependencies, and updates end-to-end tests for route handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent Script
participant LlamaStack Server
participant MCP Toolgroup
User->>Agent Script: Run script (auto or interactive)
Agent Script->>LlamaStack Server: Connect (local/remote)
Agent Script->>LlamaStack Server: List registered tools
Agent Script->>MCP Toolgroup: Register "mcp::openshift" (if needed)
Agent Script->>LlamaStack Server: Create Agent with model and toolgroup
alt Automated mode
Agent Script->>LlamaStack Server: Run prompt ("List all namespaces...")
LlamaStack Server-->>Agent Script: Stream responses
Agent Script->>User: Print responses
else Interactive mode
loop Until user types "/bye"
User->>Agent Script: Enter prompt
Agent Script->>LlamaStack Server: Send prompt
LlamaStack Server-->>Agent Script: Stream responses
Agent Script->>User: Print responses
end
end
opt Session info on exit
Agent Script->>LlamaStack Server: Get session info
Agent Script->>User: Print session info
end
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
config/rbac/mcp_server_get_role.yaml (1)
21-21: Add missing newline at end of file.- watch +config/rbac/mcp_server_get_role_binding.yaml (1)
12-12: Add missing newline at end of file.namespace: system +test/e2e/e2e_test.go (1)
184-196: LGTM! Consider extracting the SSE path as a constant.The simplified route handling correctly adapts to the changes in the MCPServer deployment. The test now fetches only the host and appends the SSE path, which aligns with the updated route configuration.
For better maintainability, consider extracting the hardcoded SSE path:
+const sseEndpointPath = "/sse" // Create the route URL using the host and the sse path -routeURL := fmt.Sprintf("http://%s/sse", routeHost) +routeURL := fmt.Sprintf("http://%s%s", routeHost, sseEndpointPath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
config/rbac/kustomization.yaml(1 hunks)config/rbac/mcp_server_get_role.yaml(1 hunks)config/rbac/mcp_server_get_role_binding.yaml(1 hunks)demo/llama_stack_mcp_agent.py(1 hunks)demo/requirements.txt(1 hunks)internal/controller/mcpserver.go(1 hunks)internal/controller/mcpserver_controller.go(2 hunks)test/e2e/e2e_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/e2e/e2e_test.go (1)
test/utils/utils.go (1)
Run(44-61)
internal/controller/mcpserver_controller.go (3)
api/v1/mcpserver_types.go (1)
MCPServer(44-50)pkg/cluster/gvk/gvk.go (1)
MCPServer(8-12)api/v1/groupversion_info.go (1)
GroupVersion(29-29)
🪛 YAMLlint (1.37.1)
config/rbac/mcp_server_get_role_binding.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
config/rbac/mcp_server_get_role.yaml
[error] 21-21: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.12.2)
demo/llama_stack_mcp_agent.py
5-5: termcolor.cprint imported but unused
Remove unused import: termcolor.cprint
(F401)
58-58: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🔇 Additional comments (5)
demo/requirements.txt (1)
1-37: Pinned package versions are up to date
I checked the key dependencies (requests, httpx, pydantic, PyYAML, numpy, urllib3) against PyPI and confirmed that each is pinned to the latest available release. No version mismatches were found.• For a full security audit, run a tool like pip-audit or Safety against demo/requirements.txt to catch any known advisories before shipping.
internal/controller/mcpserver.go (1)
71-71: Verify server binary supports the updated command arguments.We searched the Go code and found no definitions or registrations for the
--portor--log-levelflags in this repository. Please confirm that thekubernetes-mcp-serverbinary’s source actually defines and handles these new flags before merging.config/rbac/kustomization.yaml (1)
28-31: LGTM! Proper integration of new RBAC resources.The addition of the new RBAC configuration files is well-documented and properly integrated into the kustomization setup. The explanatory comment clearly states the purpose.
demo/llama_stack_mcp_agent.py (1)
36-42: Verify host.containers.internal compatibility.The local MCP URL uses
host.containers.internal, which is specific to Docker Desktop. This may not work in other container runtimes (e.g., Podman, containerd, or Linux Docker).Consider making this configurable via an environment variable with a fallback:
else: base_url="http://localhost:8321" - mcp_url="http://host.containers.internal:8000/sse" + mcp_url=os.getenv("LOCAL_MCP_URL", "http://host.containers.internal:8000/sse")internal/controller/mcpserver_controller.go (1)
132-146: Well-implemented label-based filtering.The predicate correctly filters events to only process resources labeled with
opendatahub.io/mcp-server. This efficient filtering prevents unnecessary reconciliations and improves controller performance.
CiaraM100104
force-pushed
the
operator-improvements
branch
from
9ef754c to
2402428
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
demo/llama_stack_mcp_agent.py (1)
82-82: Rename unused loop variable.The loop variable
iis not used within the loop body.Apply this diff to fix the issue:
- for i, prompt in enumerate(user_prompts): + for _, prompt in enumerate(user_prompts):config/rbac/mcp_server_get_role_binding.yaml (1)
10-12: Update the ServiceAccount name and add trailing newline.The ClusterRoleBinding should target the correct ServiceAccount used by the MCP server deployment, and the file needs a trailing newline.
subjects: - kind: ServiceAccount - name: default + name: controller-manager namespace: system +
🧹 Nitpick comments (4)
demo/llama_stack_mcp_agent.py (3)
10-10: Consider adding error handling for environment loading.The
load_dotenv()call should handle cases where the .env file doesn't exist or contains invalid data.-load_dotenv() +try: + load_dotenv() +except Exception as e: + logger.warning(f"Failed to load .env file: {e}")
32-32: Consider making the model configurable.The model is hardcoded. Consider making it configurable via command-line argument or environment variable for better flexibility.
-# Model -model="llama3.2:3b" +# Model +model = os.getenv("MODEL_NAME", "llama3.2:3b")
59-60: Consider graceful degradation instead of hard exit.Using
exit(1)terminates the entire program. Consider allowing the script to continue with reduced functionality or provide better error recovery.except Exception as e: logger.error(f"Error registering MCP tools: {e}") - exit(1) + logger.warning("Continuing without MCP tools registration") + # Or implement fallback behaviorconfig/rbac/mcp_server_get_role.yaml (1)
49-49: Add trailing newline.The file is missing a newline character at the end.
- get - list - watch +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/manager/kustomization.yaml(1 hunks)config/rbac/kustomization.yaml(1 hunks)config/rbac/mcp_server_get_role.yaml(1 hunks)config/rbac/mcp_server_get_role_binding.yaml(1 hunks)demo/llama_stack_mcp_agent.py(1 hunks)demo/requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/manager/kustomization.yaml
- demo/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- config/rbac/kustomization.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
config/rbac/mcp_server_get_role.yaml
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
config/rbac/mcp_server_get_role_binding.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.12.2)
demo/llama_stack_mcp_agent.py
82-82: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🔇 Additional comments (1)
config/rbac/mcp_server_get_role.yaml (1)
7-49: RBAC permissions are well-structured.The ClusterRole correctly organizes resources by their respective API groups and grants appropriate read-only permissions. The API group assignments are accurate for each resource type.
Description
This pull request implements the following:
Merge criteria:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores