feat: AgentToolProvider reconciler for cross-cluster MCP tool federation (grid#41) - #56
Open
jordigilh wants to merge 7 commits into
Open
Conversation
This was referenced Aug 13, 2026
Contributor
Author
|
Filed #57 for the cross-site SWIM/CRDT propagation of |
…y (grid#41)
Adds the live-probe half of the AgentToolProvider reconciler: an rmcp-based
Streamable HTTP client that calls tools/list against spec.endpoint, with
SSRF protection (blocks loopback/link-local/cloud-metadata targets), TLS
material resolution, and bearer-token auth from a Secret.
Also wires grid_mcp_probe_total{outcome} and grid_mcp_probe_duration_seconds
metrics, following grid#9's bounded-cardinality convention: TlsConfigInvalid's
carried reason string is deliberately collapsed to a single label so metric
cardinality stays fixed regardless of cluster misconfiguration variety.
Signed-off-by: Jordi Gil <jgil@redhat.com>
Wires the AgentToolProvider CRD to the live MCP probe client added in the previous commit: resolves siteSelector matches against GridNetwork/GridSite, runs the probe, and maps its outcome to status.phase/status.reason/ status.discoveredTools. Mirrors InferenceProvider's reconciliation shape, including its documented cross-resource watch limitation as a follow-up. Data-plane MCP tool-catalog aggregation and cross-cluster tools/call routing (praxis-ai#155, #205, #173) are out of scope here — this is the Grid-side control plane only, per grid#41's stated scope. Signed-off-by: Jordi Gil <jgil@redhat.com>
…k MCP server, and E2E check (grid#41) - charts/grid-operator: install the AgentToolProvider CRD and grant its RBAC verbs, with helm-unittest coverage for the new ClusterRole rule; wires charts/grid-operator into the CI helm-unittest job (only charts/grid-site ran before). - mock-providers: new --mcp-server mode built on the real rmcp server SDK (Streamable HTTP), so the E2E check below probes actual MCP wire protocol behavior, not a hand-rolled JSON-RPC approximation. Disables rmcp's default DNS-rebinding Host-header allowlist (localhost/127.0.0.1/::1 only), since this mock is reached over its in-cluster Service DNS name or NodePort address, never loopback. - xtask: cargo xtask env verify-agenttoolprovider-convergence deploys the mock as a real in-cluster NodePort service (the operator under test runs out-of-cluster, so it can't resolve in-cluster .svc DNS names) and proves Pending -> Available with discoveredTools populated end-to-end, plus the unreachable-endpoint failure path landing on Unavailable with a reason. Verified twice against a real kind cluster. Signed-off-by: Jordi Gil <jgil@redhat.com>
jordigilh
force-pushed
the
feat/grid-41-agenttoolprovider-reconciler
branch
from
August 13, 2026 22:33
d46aa45 to
02a0b14
Compare
…st gaps (grid#41) Backfills the coverage gaps found during grid#56's GA readiness audit: - attach_tls_ca/attach_tls_client_identity/read_tls_material now have real unit-test coverage against a mocked kube::Client (tower::service_fn), which the module's own doc comment had falsely claimed already existed. - Along the way, found and fixed a real bug: reqwest::Certificate::from_pem and reqwest::Identity::from_pem don't reject malformed/empty PEM input, unlike the rustls::pki_types-based validation InferenceProvider's build_tls_client_config already uses. attach_tls_ca and attach_tls_client_identity now eagerly validate PEM material the same strict way before handing it to reqwest, so EndpointTlsMaterialInvalid/ EndpointTlsIdentityMismatch are reachable in practice, not just in name. - Also found (but did not fix here, to keep this change scoped) a pre-existing bug in the shared endpoint_tls.rs/secret.rs Secret-read path that also affects InferenceProvider: a key entirely absent from an existing Secret's data is misreported as SecretMissing instead of KeyMissing. Filed as grid#58, with a test that locks in current behavior and points at the issue. - static_config_failure_reason (GridNetworkNotFound/ProviderConfigInvalid) now has direct unit-test coverage via the same kube::Client mocking pattern, proving wrong an existing code comment that claimed this "cannot be unit-tested without a live cluster or a mock Kubernetes server." - Fixed doc drift: overview.md's claim that AgentToolProvider "does not currently run full controllers" (it does, as of grid#56), crds.md's Degraded phase (unreachable by design for this CRD, per phase_and_reason_from_probe's own doc comment) and missing reason/ observedGeneration status fields, and two stale "once PR 2 lands" code comments left over from the original 3-PR plan that landed as one. Signed-off-by: Jordi Gil <jgil@redhat.com>
praxis-bot
reviewed
Aug 14, 2026
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Adds AgentToolProvider CRD reconciler with an rmcp-based MCP tools/list live probe (SSRF-hardened, DNS-pinned), site-selector matching mirroring InferenceProvider, Helm/RBAC scaffolding, a real rmcp-server-backed mock MCP server, and an end-to-end convergence xtask.
Overall: High-quality, well-structured PR. The three-layer test architecture (pure decision logic, mocked Kubernetes I/O, real network integration) is exemplary. SSRF protection is thorough -- loopback, link-local, unspecified, cloud metadata, IPv4-mapped-IPv6 bypass, embedded credentials, DNS-rebinding address pinning, redirect suppression, and proxy bypass are all covered. The reconciler mirrors InferenceProvider's proven pattern, and conventions are followed consistently. The pre-existing items[2] duplication bug in generate-deployment-crds.sh is quietly fixed as well.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 1 |
| Medium | 2 |
Findings without inline placement
(none -- all findings are inline)
`router`'s doc comment linked to `FixedToolsServer`, a private struct -- the link only resolved locally because of --document-private-items and failed CI's -D rustdoc::private-intra-doc-links check. Drop the link, keep the type name as plain text with a note that it's module-private. Signed-off-by: Jordi Gil <jgil@redhat.com>
Addresses grid#56 review feedback (pull/56#pullrequestreview, praxis-bot): - PROBE_TIMEOUT was documented as a single combined budget for the live MCP probe, but resolve_endpoint_for_probe (DNS), the connect/handshake, and tools/list each independently got up to the full timeout, and TLS Secret material reads via the Kubernetes API had no timeout at all. Worst-case wall-clock time could exceed 3x the documented 10s budget, plus unbounded Kubernetes API latency. Fixed by wrapping the whole probe sequence in a single outer tokio::time::timeout in probe_agent_tool_provider, so all phases now share one real budget. - auth_header_map silently dropped the Authorization header when a bearer token contained characters invalid in an HTTP header value, with no log signal -- the resulting probe failure would look like an auth/response problem with no trace back to the real cause. Added a tracing::warn! on that branch. - validate_probe_url already blocks IPv6 link-local (fe80::/10) and unique-local (fd00::/8) addresses via is_ssrf_sensitive, but had no test coverage proving it (only the IPv4 link-local and IPv6 loopback cases were tested). Added the two missing regression tests. Signed-off-by: Jordi Gil <jgil@redhat.com>
…lient All existing agent_tool_provider tests exercise resolve_phase_and_sites's constituent resolve_*/pure-logic functions in isolation; none drove the public reconcile() entrypoint itself, so nothing proved the resolved (phase, reason, matchingSites, discoveredTools) tuple actually reaches the Kubernetes API as the status PATCH body a real controller sends. Add a PATCH-capturing mock kube::Client and two reconcile()-level tests covering the two code paths that need no live MCP probe: a config-invalid provider (fully short-circuited, no Kubernetes calls at all) and a provider whose gridNetworkRef doesn't resolve (exercises the live GridNetwork GET). Both mutation-tested by hand against the reason strings they assert on to confirm they fail on regression, not just pass by construction. Signed-off-by: Jordi Gil <jgil@redhat.com>
jordigilh
commented
Aug 18, 2026
jordigilh
left a comment
jordigilh
left a comment
Contributor
Author
There was a problem hiding this comment.
Really solid feature overall — went deep on this one given the scope (SSRF handling, RBAC, live Kind verification all checked out clean). One inline comment below; two more notes here since they land outside this PR's actual diff:
AgentToolProviderSpec.access_policy(agent_tool_provider.rs:37-39, predates this PR) is accepted by the API but I couldn't find it referenced anywhere ingrid_network.rs— a repo-wide grep comes up empty. Contrast with the identical field onInferenceProviderSpec, which is consumed byaccess_policy_to_crdtand published into CRDT/SWIM state. An operator setting this reasonably believes they've restricted which sites can consume these tools, and nothing enforces it yet. Might be worth wiring it into the same publish path, or documenting today's status as not-yet-enforced until cross-cluster propagation (#57) lands.charts/grid-operator/templates/NOTES.txtstill doesn't mentionagenttoolproviders.grid.praxis-proxy.ioin its CRD-check/apply/removal sections.
Also, the cross-cluster propagation gap implied by the PR title is real but already tracked in #57, so that's a non-issue, just flagging that I checked.
| | `McpAuthRejected` | The MCP server rejected the configured `spec.auth` credentials. | | ||
| | `EndpointTlsSecretMissing` | `spec.tls`'s referenced Secret does not exist in the cluster. | | ||
| | `EndpointTlsKeyMissing` | `spec.tls`'s referenced Secret exists but is missing the expected key. | | ||
| | `EndpointTlsMaterialInvalid` | `spec.tls`'s certificate or key material could not be parsed. | |
Contributor
Author
There was a problem hiding this comment.
This table (and the identical copy in charts/grid-operator/crds/agenttoolprovider.yaml) is missing the ProviderConfigInvalid/GridNetworkNotFound rows that are already in the source doc comment (agent_tool_provider.rs:95-96). No functional impact since this is an untyped string field, but the "CRD synchronization" check in verify-helm-chart.sh only diffs these two checked-in copies against each other, never against a fresh generator run, so this class of drift won't get caught going forward either. Might be worth regenerating both files here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #41.
Adds the Grid-side control plane for
AgentToolProvider: a live MCPtools/listprobe client and a reconciler that mirrors the already-provenInferenceProviderpipeline, plus the Helm/E2E scaffolding to validate it end-to-end.operator/src/resources/mcp_probe.rs):rmcp-based Streamable HTTP client with SSRF protection (blocks loopback/link-local/cloud-metadata targets), TLS material resolution, and bearer-token auth resolved from a Secret. Bounded telemetry (grid_mcp_probe_total{outcome},grid_mcp_probe_duration_seconds) follows feat(ingress): publish bounded edge availability for global traffic managers #9's cardinality convention.operator/src/controller/agent_tool_provider.rs): resolvessiteSelectormatches againstGridNetwork/GridSite, runs the probe, and maps outcomes tostatus.phase/status.reason/status.discoveredTools.AgentToolProviderCRD and RBAC in thegrid-operatorchart (withhelm-unittestcoverage, now running in CI), a realrmcp-server-backed mock MCP server (mock-providers --mcp-server), and a newcargo xtask env verify-agenttoolprovider-convergencecommand provingPending -> AvailablewithdiscoveredToolspopulated against a real in-cluster mock, plus the unreachable-endpoint failure path landing onUnavailablewith a populated reason.Data-plane MCP tool-catalog aggregation and cross-cluster
tools/callrouting (praxis-ai#155,#205,#173) are out of scope here, per #41's stated scope — this PR is the Grid-side control plane only.Test plan
cargo test --workspace— all unit/integration tests pass (1121+ operator tests, 473+ xtask tests)cargo clippy --workspace --all-targets -- -D warnings— cleancargo +nightly fmt --all -- --check— cleanhelm unittest charts/grid-operator charts/grid-site— 7/7 passingcargo xtask env verify-agenttoolprovider-convergencerun twice against a real kind cluster (healthy convergence + unreachable-endpoint path both pass reliably)