Skip registry ingestion in k8s mode and add Kubernetes deployment examples #98

therealnb · 2025-12-03T18:41:26Z

Summary

This PR addresses issue #89 by skipping registry ingestion when running in k8s mode, and adds comprehensive Kubernetes deployment examples.

Changes

Issue #89: Skip registry ingestion in k8s mode

Added check in ingest_registry() to skip registry ingestion when runtime_mode == "k8s"
In k8s mode, workloads come from Kubernetes CRDs, not ToolHive's registry API
Registry is not available via ToolHive HTTP API in k8s mode

Additional Changes

URL normalization improvements for streamable-http transport
- Strips URL fragments (#github → removed)
- Converts /sse paths to /mcp
- Ensures path ends with /mcp if missing
- For SSE: preserves original URL (fragments needed for container identification)
Enhanced MCP client with better error handling
Added comprehensive Kubernetes deployment examples:
- MCPServer manifests for fetch, github, toolhive-doc-mcp, and mcp-optimizer
- Helper scripts for managing MCP servers (apply, delete, status)
- Shared ServiceAccount configuration
- Ingress configuration
- Comprehensive README with installation instructions

Related Issues

Addresses StacklokLabs/research#89: Call k8s registry API instead of thv registry API when running in k8s

Signed-off-by: nigel brown <nigel@stacklok.com>

For streamable-http: Strips URL fragments (#github → removed) Converts /sse paths to /mcp Ensures path ends with /mcp if missing For SSE: preserves original URL (fragments needed for container identification) Signed-off-by: nigel brown <nigel@stacklok.com>

Automated update of ToolHive API models from OpenAPI specification. Co-authored-by: aponcedeleonch <7890853+aponcedeleonch@users.noreply.github.com>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…81) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

claude · 2025-12-03T18:42:38Z

PR Review - Skip registry ingestion in k8s mode and enable OpenTelemetry

Summary

Overall solid PR addressing two distinct issues. The code quality is good, with clear separation of concerns. A few minor improvements recommended.

Key Changes Reviewed

K8s mode registry skip (ingestion.py:1716-1724) ✅
URL normalization (mcp_client.py:42-95) ✅
Error handling improvements (mcp_client.py:97-130) ✅
K8s deployment examples ✅

Issues Found

1. URL Normalization Logic - Redundancy (mcp_client.py:68-73)

elif not path.endswith("/mcp"):
    if path.endswith("/"):
        path = path + "mcp"
    else:
        path = path + "/mcp"

Can be simplified to:

elif not path.endswith("/mcp"):
    path = path.rstrip("/") + "/mcp"

2. Shell Script - Missing Quotes (create-github-secrets.sh:49)
The read -r GITHUB_USERNAME should validate input more robustly. Also line 67 should quote "${GITHUB_USERNAME}" consistently.

3. Inconsistent f-string Usage (mcp_client.py:145, 154, 178, 185)
Mixed use of f-strings and structured logging. Since you're using structlog with kwargs, avoid f-strings in log messages:

# Current
logger.debug(f"Determining proxy mode from proxy_mode field: {proxy_mode_lower}", ...)
# Better
logger.debug("Determining proxy mode from proxy_mode field", proxy_mode=proxy_mode_lower, ...)

4. Potential Breaking Change - URL Modification
The URL normalization changing /sse to /mcp (line 66-67) could break existing deployments if any workloads use /sse endpoints. Consider:

Adding migration notes
Or checking if endpoint responds before assuming it needs normalization

Positive Observations

Clear comments explaining k8s mode behavior
Good error message extraction from ExceptionGroups
Comprehensive K8s examples with RBAC
Shell scripts have proper error handling with set -e

Performance & Security

✅ No performance regressions expected
✅ No security issues found
⚠️ Shell scripts handle secrets properly but could use readonly variables

Recommendations

Simplify path normalization logic
Standardize logging (avoid f-strings with structlog)
Add migration notes for URL normalization behavior change
Consider making shell script variables readonly where applicable

Estimated Fix Time: 10-15 minutes

- Updated normalization logic to check if /mcp already exists in path before adding it - Updated test expectations to account for URL normalization behavior - Fixes failing tests where URLs like /mcp/test-server were getting /mcp appended

aponcedeleonch

the only major comment I have is the _normalize_url logic. I believe we should not mangle with the URL coming from ToolHive

aponcedeleonch · 2025-12-04T09:28:50Z

.github/workflows/offline-tests.yml

    steps:
      - name: Checkout code
-        uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
+        uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1


are these updates coming from a rebase? not sure why they're showing in the diff..

aponcedeleonch · 2025-12-04T09:37:07Z

src/mcp_optimizer/mcp_client.py

+        if proxy_mode == ToolHiveProxyMode.STREAMABLE:
+            # Strip fragments for streamable-http
+            # (fragments not supported by streamable-http client)
+            parsed = urlparse(url)
+
+            # Fix path: streamable-http uses /mcp endpoint, not /sse
+            path = parsed.path
+            if path.endswith("/sse"):
+                path = path.replace("/sse", "/mcp")
+            elif not path.endswith("/mcp"):
+                # Only add /mcp if the path doesn't already contain /mcp
+                # This prevents double-adding /mcp to URLs like /mcp/test-server
+                if "/mcp" not in path:
+                    # If path doesn't end with /mcp or /sse, and doesn't contain /mcp,
+                    # ensure it ends with /mcp
+                    if path.endswith("/"):
+                        path = path + "mcp"
+                    else:
+                        path = path + "/mcp"
+
+            # Reconstruct URL without fragment and with corrected path
+            normalized_tuple = (
+                parsed.scheme,
+                parsed.netloc,
+                path,
+                parsed.params,
+                parsed.query,
+                "",  # Empty fragment
+            )
+            normalized = str(urlunparse(normalized_tuple))
+            if normalized != url:
+                logger.debug(
+                    "Normalized URL for streamable-http",
+                    original_url=url,
+                    normalized_url=normalized,
+                    workload=self.workload.name,
+                )
+            return normalized


are you sure we need this logic? I remember we had problems in the past when trying to enforce the URL to follow a pattern. I did a quick search on the specification and there's no mention that the URL of streamable-http transport need to finish in /mcp. I think we need to trust ToolHive and that it gives us the correct URL. If it doesn't then probably it's a bug in ToolHive. Maybe instead of trying to assume and enforce the URL we should log a warning that it doesn't follow the convention of ending in /mcp

If toolhive gives us the right URLs, then this is a NOP. I'm inclined to leave it in there for now. It was needed to get it working with github. I'll raise a toolhive issue to consider it there.

Created stacklok/toolhive#2890

aponcedeleonch · 2025-12-04T09:39:11Z

src/mcp_optimizer/toolhive/api_models/audit.py

@@ -1,6 +1,6 @@
 # generated by datamodel-codegen:
 #   filename:  http://127.0.0.1:8080/api/openapi.json
-#   timestamp: 2025-11-02T00:37:46+00:00


are these changes in toolhive/api_models also rebase leftovers?

I was going to ask you about that. There's no actual change, just timestamps.

They come from #92

therealnb and others added 9 commits December 2, 2025 15:01

thv group investigation

82c8cd9

Signed-off-by: nigel brown <nigel@stacklok.com>

Example tweaks

3c87fb4

Signed-off-by: nigel brown <nigel@stacklok.com>

Skip registry if in k8s mode

d6ef090

Signed-off-by: nigel brown <nigel@stacklok.com>

Update ToolHive API models (#92)

66aaebb

Automated update of ToolHive API models from OpenAPI specification. Co-authored-by: aponcedeleonch <7890853+aponcedeleonch@users.noreply.github.com>

chore(deps): update actions/checkout action to v6 (#77)

87cbf7f

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

chore(deps): update peter-evans/create-pull-request action to v7.0.9 (#…

533f5bc

…81) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

chore(deps): update docker/metadata-action action to v5.10.0 (#87)

e47667f

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Update renovate.json to bump pyproject.toml dependencies (#93)

a6c87eb

therealnb force-pushed the thv-group-investigation branch from ec3eacd to a6c87eb Compare December 3, 2025 18:43

therealnb changed the title ~~Skip registry ingestion in k8s mode and enable OpenTelemetry for toolhive-doc-mcp~~ Skip registry ingestion in k8s mode and add Kubernetes deployment examples Dec 3, 2025

therealnb added 2 commits December 3, 2025 18:48

Fix formatting issues

45c2261

therealnb requested review from aponcedeleonch, ptelang and tgrunnagle December 3, 2025 19:02

aponcedeleonch approved these changes Dec 4, 2025

View reviewed changes

therealnb merged commit dcf4923 into main Dec 4, 2025
14 checks passed

therealnb deleted the thv-group-investigation branch December 4, 2025 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Skip registry ingestion in k8s mode and add Kubernetes deployment examples #98

Skip registry ingestion in k8s mode and add Kubernetes deployment examples #98

Uh oh!

therealnb commented Dec 3, 2025 •

edited

Loading

Uh oh!

claude bot commented Dec 3, 2025

Uh oh!

aponcedeleonch left a comment

Uh oh!

aponcedeleonch Dec 4, 2025

Uh oh!

aponcedeleonch Dec 4, 2025

Uh oh!

therealnb Dec 4, 2025

Uh oh!

therealnb Dec 4, 2025

Uh oh!

aponcedeleonch Dec 4, 2025

Uh oh!

therealnb Dec 4, 2025 •

edited

Loading

Uh oh!

therealnb Dec 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Skip registry ingestion in k8s mode and add Kubernetes deployment examples #98

Skip registry ingestion in k8s mode and add Kubernetes deployment examples #98

Uh oh!

Conversation

therealnb commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Issue #89: Skip registry ingestion in k8s mode

Additional Changes

Related Issues

Uh oh!

claude bot commented Dec 3, 2025

PR Review - Skip registry ingestion in k8s mode and enable OpenTelemetry

Summary

Key Changes Reviewed

Issues Found

Positive Observations

Performance & Security

Recommendations

Uh oh!

aponcedeleonch left a comment

Choose a reason for hiding this comment

Uh oh!

aponcedeleonch Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

aponcedeleonch Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

therealnb Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

therealnb Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

aponcedeleonch Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

therealnb Dec 4, 2025 • 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!

therealnb Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

therealnb commented Dec 3, 2025 •

edited

Loading

therealnb Dec 4, 2025 •

edited

Loading