Add deny-by-default visibility to vMCP aggregation - #6163
Conversation
The aggregation.tools list is sparse and fails open: a backend in the group with no entry has every tool advertised. Operators scoping a vMCP to a curated subset must remember to add excludeAll for each new group member, and forgetting silently widens what clients see. Add aggregation.defaultVisibility. Under "deny" a backend absent from tools contributes no tools, so only listed backends are advertised. A listed backend is opted in by its entry; its own excludeAll/filter then decide which of its tools show. Defaults to "allow", the pre-existing behavior, so existing configs and deployed CRs are unaffected and no migration is needed. Like the sibling visibility settings this controls advertising only, leaving all tools in the routing table for composite tools.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6163 +/- ##
==========================================
- Coverage 72.51% 72.47% -0.04%
==========================================
Files 739 739
Lines 76719 76768 +49
==========================================
+ Hits 55629 55641 +12
- Misses 17107 17159 +52
+ Partials 3983 3968 -15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new aggregation.defaultVisibility setting to vMCP tool aggregation so operators can choose between the current “fail open” behavior (allow, default) and a “deny-by-default” advertising mode (deny) where only backends explicitly listed under aggregation.tools contribute advertised tools. The change is explicitly scoped to advertising only: hidden tools remain routable for composite tools, and prompts/resources are unaffected.
Changes:
- Introduce
DefaultVisibility(allow/deny) onAggregationConfig, validate it on the CLI config path, and carry it through CRD → rendered config conversion. - Teach the default aggregator to withhold tools from unlisted backends when
defaultVisibility: deny. - Add focused unit tests covering compatibility (unset/allow) and deny-by-default behavior, plus conversion preservation.
Key Concerns
- suggestion:
convertAggregationcomments contradict themselves about “deep copy” vs “hand-copy”. This is minor but worth fixing to avoid misleading future maintainers about aliasing/mutation expectations (see stored comment incmd/thv-operator/pkg/vmcpconfig/converter.go).
Testing Assessment
The added tests cover the important input matrix (unset vs allow vs deny, deny + listed backend with filter/no filter/excludeAll, mixed listed+unlisted backends) and also reassert the routing-table invariant, which is the key safety property for “advertising-only” changes.
vMCP Anti-Pattern Check
No new vMCP anti-patterns stood out in the touched pkg/vmcp/** files (notably: no new interface churn/abstractions (anti-pattern #8), no new shared mutable state or concurrency hazards, and no “god object” expansion concerns from this change).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/config/config.go | Add DefaultVisibility type/field and document allow vs deny advertising semantics. |
| pkg/vmcp/config/validator.go | Validate defaultVisibility values for the CLI config path. |
| pkg/vmcp/config/validator_test.go | Add coverage for allow/deny/unset validity and invalid value rejection. |
| pkg/vmcp/aggregator/default_aggregator.go | Implement deny-by-default advertising for unlisted backends via a derived denyUnlisted flag. |
| pkg/vmcp/aggregator/advertised_backendid_test.go | Extend advertising tests for allow/deny/unset and add mixed-backend deny scenario. |
| cmd/thv-operator/pkg/vmcpconfig/converter.go | Preserve DefaultVisibility through CRD → rendered config conversion. |
| cmd/thv-operator/pkg/vmcpconfig/converter_test.go | Guard against converter regression dropping defaultVisibility. |
| deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml | Regenerated CRD schema/docs including defaultVisibility (chart template copy). |
| deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml | Regenerated CRD schema/docs including defaultVisibility (packaged CRD copy). |
| docs/operator/crd-api.md | Regenerated CRD API docs reflecting the new field and semantics. |
| docs/operator/virtualmcpserver-api.md | Add user-facing docs and deny-by-default example + advertising-only scope note. |
| docs/arch/10-virtual-mcp-architecture.md | Expand architecture docs around tool filtering/advertising semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Carried through explicitly: this converter hand-copies fields rather than | ||
| // deep-copying, so a visibility setting omitted here would be accepted by the | ||
| // CRD and then silently dropped before the vMCP process ever sees it — | ||
| // failing OPEN on a setting users rely on to withhold tools. | ||
| DefaultVisibility: srcAgg.DefaultVisibility, |
There was a problem hiding this comment.
Good catch — and the contradiction ran deeper than the two comments disagreeing: the pre-existing "Start with a deep copy" claim was simply wrong, and had been before this PR.
This is a field-by-field copy that aliases the source's slices (ConflictResolutionConfig.PriorityOrder, WorkloadToolConfig.Filter); only Overrides is genuinely deep-copied (via DeepCopy() in resolveToolConfigRefs). So "deep copy" was misleading in exactly the way you'd worry about — a maintainer trusting it might mutate a slice here and silently mutate the CR's in-memory spec.
Replaced both comments with one accurate statement covering the two things that actually matter: treat the source's slices as read-only, and every new AggregationConfig field must be added to this literal explicitly or it gets silently dropped (which for a visibility setting means failing open). Kept the fail-open rationale as you asked.
Fixed in 9bf1406.
The comment claimed a deep copy while the code does a field-by-field copy that aliases slices, and the note added alongside DefaultVisibility contradicted it. State the actual semantics once, including the aliasing caveat and why each new field must be listed explicitly.
github-actions
Bot
added
size/M
github-actions
Bot
added
size/M
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Read through this carefully plus traced the call paths. The feature works and the converter comment about copy semantics is the best thing in the diff — that's exactly the trap this would otherwise have fallen into.
One thing I think is a genuine regression: with the priority strategy, an unlisted backend can still win a name conflict and then get filtered out, so neither version of the tool ends up advertised. Details inline. Everything else is naming/docs, and one of them is a copy-pasteable example that doesn't work.
The bigger "hidden tools are still callable by name" issue is pre-existing (excludeAllTools and filter behave the same on main), so I don't think it belongs in this PR — but deny makes it the default posture for everything not enumerated, and CallTool's doc comment already claims it returns ErrNotFound for unadvertised names, which it doesn't. Worth a follow-up against main before this becomes the recommended way to curate a group.
| // tool; under "deny", withhold it so only backends named in the config | ||
| // contribute tools. A backend WITH a config is opted in either way — its | ||
| // own ExcludeAll/Filter decide below. | ||
| return !a.denyUnlisted |
There was a problem hiding this comment.
This filter runs after ResolveConflicts has already had every backend participate, which I think produces a surprising outcome with the priority strategy.
priorityOrder is independent of tools, so a backend can be in the priority order but have no tools entry. Then:
- Both backends expose
search. The unlisted one is higher priority, soselectWinnerpicks it (priority_resolver.go:147) and the listed backend'ssearchis dropped right there. - Here, the winner has no entry in
toolConfigMap, sodenyUnlistedwithholds it.
Net result: search disappears from tools/list entirely, even though a workload the operator explicitly listed was offering it. On main this didn't happen — the unlisted winner was advertised, so the tool was at least present.
Cheapest fix is probably validation rather than reordering the pipeline: reject (or warn on) a priorityOrder entry that has no tools entry when defaultVisibility: deny. Reordering means resolving conflicts twice, once over the visible candidate set and once over the complete set for composites, which feels like its own PR.
A mixed listed/unlisted collision test under priority would pin whichever behaviour you decide is right.
| case "", DefaultVisibilityAllow, DefaultVisibilityDeny: | ||
| default: | ||
| return fmt.Errorf("defaultVisibility must be one of: allow, deny") | ||
| } |
There was a problem hiding this comment.
Related to the priority-strategy comment on default_aggregator.go — this seems like the natural place for the guard, right after the enum check, since validateConflictStrategy above has already confirmed priorityOrder is well-formed.
Something like: when DefaultVisibility == DefaultVisibilityDeny, any PriorityOrder entry with no matching Tools[].Workload is a config error (or at minimum a warn), because that backend can win a conflict and then be withheld.
Separately, and much smaller: a typo in tools[].workload under deny silently withholds everything from the real backend, with no diagnostic anywhere in the path. Worth a slog.Warn at aggregator construction when a Tools entry matches no backend — under allow a typo just means "no filter applied", but under deny it means "this backend contributes nothing", which is a much worse thing to debug from an empty tool list.
| // +kubebuilder:validation:Enum=allow;deny | ||
| // +kubebuilder:default=allow | ||
| // +optional | ||
| DefaultVisibility DefaultVisibility `json:"defaultVisibility,omitempty" yaml:"defaultVisibility,omitempty"` |
There was a problem hiding this comment.
Two things about the name, while it's still free to change — after release this is a CRD field and we're stuck with it.
It only gates tools. mergeResources, mergeResourceTemplates and mergePrompts have no equivalent check, so under deny an unlisted backend's resources and prompts are still advertised. That's consistent with ExcludeAllTools — but that name says "Tools" and this one doesn't. The fact that the doc block has to disclaim the scope (and the operator guide does too) is the tell. defaultToolVisibility would carry its own scope.
"deny" is the most overloaded word in access control. The doc correctly says advertising-only and mentions the routing table, but stops short of the part that actually matters to whoever configures this: a hidden tool is still directly callable by name over the Modern path. CallTool routes off agg.RoutingTable, which keeps everything, and authorizeToolCall synthesizes a bare &vmcp.Tool{Name: name} rather than rejecting an unadvertised name — so with no Cedar policies configured (allow-all admission) a client that guesses jira_get_issue gets through. Legacy clients don't, because the SDK only has the advertised tools registered per session.
That asymmetry is pre-existing and not this PR's job to fix. But "Use this when the set of exposed tools must be enumerated deliberately" reads like an enforcement guarantee. One added sentence — hidden tools remain callable by name, use Cedar for enforcement — would set the right expectation.
| // DefaultVisibility names the advertising default applied to backends with no | ||
| // per-workload Tools entry. The zero value is "" (unset), which behaves as | ||
| // DefaultVisibilityAllow so existing configs keep today's behavior. | ||
| type DefaultVisibility string |
There was a problem hiding this comment.
Missing // +gendoc, so the generated reference emits a link to #vmcpconfigdefaultvisibility with no matching section — see the two bare blank lines in the crd-api.md hunk, that's where the type stanza should have rendered. Every other named type in this file has the marker.
| type DefaultVisibility string | |
| // DefaultVisibilityAllow so existing configs keep today's behavior. | |
| // +gendoc | |
| type DefaultVisibility string |
Needs a task crdref-gen re-run after.
| // every other visibility setting here, this controls advertising only — hidden | ||
| // tools remain in the routing table for composite tools (see the type doc). | ||
| // +kubebuilder:validation:Enum=allow;deny | ||
| // +kubebuilder:default=allow |
There was a problem hiding this comment.
Minor, but worth a thought: this default changes no behaviour — "" is already treated as allow both in NewDefaultAggregator and in the validator — while it does change bytes. apiextensions applies structural-schema defaults on decode, so after the CRD upgrade every existing VirtualMCPServer with spec.config.aggregation set reads back with defaultVisibility: "allow", which gets marshalled into config.yaml, which changes the ConfigMap content checksum, which is stamped on the pod template. So all vMCP deployments restart once on upgrade for a no-op field.
Dropping the marker avoids that with zero behaviour change. Keeping it is defensible (explicit in kubectl get -o yaml is nice) — just worth calling out in the user-facing-change section if so.
| spec: | ||
| groupRef: | ||
| name: my-services | ||
| aggregation: | ||
| conflictResolution: prefix | ||
| defaultVisibility: deny | ||
| tools: | ||
| # Only these two workloads are advertised. Any other workload in | ||
| # my-services contributes no tools, including ones added later. | ||
| - workload: github | ||
| filter: ["get_issue", "list_prs"] | ||
| - workload: jira | ||
| filter: ["get_issue"] |
There was a problem hiding this comment.
Wrong nesting — the CRD field is .spec.config.aggregation, not .spec.aggregation (VirtualMCPServerSpec.Config config.Config with json:"config,omitempty"). groupRef at the top level is right, which makes this easy to miss.
Depending on the cluster's field validation this is either rejected or silently pruned, and in the pruned case the operator ends up with allow-by-default while believing deny is on — the worst possible failure for this particular feature.
| spec: | |
| groupRef: | |
| name: my-services | |
| aggregation: | |
| conflictResolution: prefix | |
| defaultVisibility: deny | |
| tools: | |
| # Only these two workloads are advertised. Any other workload in | |
| # my-services contributes no tools, including ones added later. | |
| - workload: github | |
| filter: ["get_issue", "list_prs"] | |
| - workload: jira | |
| filter: ["get_issue"] | |
| spec: | |
| groupRef: | |
| name: my-services | |
| config: | |
| aggregation: | |
| conflictResolution: prefix | |
| defaultVisibility: deny | |
| tools: | |
| # Only these two workloads are advertised. Any other workload in | |
| # my-services contributes no tools, including ones added later. | |
| - workload: github | |
| filter: ["get_issue", "list_prs"] | |
| - workload: jira | |
| filter: ["get_issue"] |
Heads up that the adjacent prefix, priority and manual examples have the same mistake already — probably worth fixing all four in one go while you're in here.
| > **Note**: `defaultVisibility` controls **advertising** only, as do | ||
| > `excludeAllTools` and `excludeAll`. Tools hidden this way remain in the routing | ||
| > table so composite tools can still call them, and `defaultVisibility` does not | ||
| > affect resources or prompts, which are advertised regardless. For per-identity |
There was a problem hiding this comment.
This note is good and I'd keep it, but it stops one step short. "Tools hidden this way remain in the routing table so composite tools can still call them" describes why the tools are kept; it doesn't say that a direct tools/call naming one of them also succeeds (on the Modern path, with allow-all admission when no Cedar policies are configured).
Suggest making that explicit here, since this is the paragraph an operator will actually read before turning deny on.
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: this file's header says it exists to pin the BackendID-population contract, but this test's real subject is deny-visibility and routing-table retention. The test itself is good — asserting both "withheld from advertised" and "still in the routing table" is exactly right — it's just hard to find here. default_aggregator_test.go next to the existing ExcludeAllPreservesRoutingTableForCompositeTools cases would be a more natural home.
Rename defaultVisibility to defaultToolVisibility. The setting gates tools only — an unlisted backend's resources, resource templates, and prompts are still advertised under deny — so the name now carries its own scope instead of relying on a doc disclaimer. Free to change before the field ships in a release. Reject defaultToolVisibility deny combined with the priority strategy when a priorityOrder entry has no tools entry. Conflict resolution runs before the advertising filter, so an unlisted backend could win a name conflict and then be withheld, hiding the tool from every backend that offered it. Validating up front is cheaper than resolving conflicts twice, once over the visible set and once over the routable set. Warn when a tools entry names no backend in the group. Under allow a typo just means no filter is applied, but under deny the real backend contributes nothing and an empty tools/list is the only symptom. Drop the kubebuilder default. Empty already behaves as allow everywhere that reads the field, so defaulting changed only the serialized bytes — and apiextensions applies it on decode, which would rewrite config.yaml, change the ConfigMap checksum, and restart every vMCP deployment once for a no-op field. Add the missing gendoc marker so the generated reference renders the type section its field link points at, fix the aggregation examples to nest under spec.config, and move the deny test next to the ExcludeAll routing-table cases.
github-actions
Bot
added
size/L
|
Thanks — the priority interaction is a real find and I've fixed it. Pushed in Priority regression. Went with validation as you suggested: You left reject-vs-warn open and I went with reject: under Three validator cases pin it, including Naming. Renamed to Typo diagnostic. Added. A Default marker. Dropped — hadn't thought through the decode-time defaulting chain to the ConfigMap checksum. Confirmed
Docs. Fixed the On hidden tools being callable — agreed it's pre-existing and out of scope here, and you traced it to the same place I did. #6216 fixes it: it rejects |
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Re-checked everything from the last round. Six of eight fully addressed, and a couple went further than I asked — splitting the unmatched-workload selection out from the logging so it's testable without swapping the global logger is a nicer shape than the bare slog.Warn I suggested. Rename is clean too; grepping the old name across the branch turns up nothing, including both CRD copies and the generated reference.
The priority-strategy fix via validation rather than resolving conflicts twice is the right trade. Only nit: the ConflictResolutionConfig == nil early return in validateDenyVisibilityPriorityOrder can't be reached, since validateAggregation already errors on nil above it. Harmless.
Still open, and fine by me to leave: the docs say hidden tools stay in the routing table for composites, but not that a direct tools/call naming one still succeeds. That belongs with the follow-up that adds the guard in core.CallTool — worth doing before deny becomes the recommended way to curate a group, since CallTool's own doc comment already promises ErrNotFound for unadvertised names.
One cosmetic thing you may or may not care about: the kubebuilder-default rationale lives inside the field's doc comment, so it now renders verbatim in the public CRD reference — operators reading crd-api.md get the ConfigMap-checksum reasoning. Above the doc block, or in the commit message, would keep it out.
Also noting for later: the deny+priority rule is Go-side only, no CEL on the CRD, so on Kubernetes a bad combination is accepted at apply time and fails at vMCP startup instead. Consistent with how the rest of this config validates, just worth knowing that's what operators will see.
Approving.
Summary
aggregation.toolsis a sparse list that fails open: a backend in the group with no entry has every one of its tools advertised. An operator scoping a vMCP to a curated subset has to remember to addexcludeAll: truefor each new group member, and forgetting silently widens what clients see. Reported in Default deny filter for group exposed by vMCP #6073, where the group grows over time and each addition is a chance to accidentally expose a server.aggregation.defaultVisibility(allow|deny). Underdeny, a backend absent fromtoolscontributes no tools, so only listed backends are advertised. A backend that is listed is opted in by its entry — its ownexcludeAll/filterthen decide which of its tools show, so an overrides-only entry keeps working.allow, which is exactly today's behavior, so existing YAML configs and already-deployed CRs are unaffected and there is no migration or backfill.Closes #6073
Type of change
Test plan
task test)task test-e2e)task lint-fix)task testpasses with zero failures. New coverage:TestDefaultAggregator_AdvertisingFilterPreservesBackendID— added cases for unset /allow/deny, plusdenycombined with a listed backend'sfilter, with no filter, and withexcludeAll. The existing table already asserts the routing-table invariant, so each new case also proves tools stay routable.TestDefaultAggregator_DefaultVisibilityDenyMixedBackends— the motivating scenario: a group with one listed and one unlisted backend. Asserts only the listed backend is advertised and the unlisted backend's tools remain in the routing table.TestConvert_DefaultVisibilityPreserved— CRD → rendered-config carry-through fordeny/allow/ unset.validateAggregation— accepts unset/allow/deny, rejects an unknown value.Two pre-existing failures in this workspace, confirmed identical on a clean tree via
git stashand unrelated to this change: 4 lint findings (3 gosec, 1 staticcheck) in files this PR does not touch, and thecmd/thv-operator/test-integration/virtualmcpenvtest suite failing on a missing local/usr/local/kubebuilder/bin/etcd(that suite is excluded fromtask test).API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Purely additive: one new optional string field with
+kubebuilder:default=allowandenum: [allow, deny]. Omitting it yields the current behavior. Nozz_generateddeepcopy drift, since the field is a value type.Changes
pkg/vmcp/config/config.goDefaultVisibilityfield onAggregationConfig+DefaultVisibilitytype andallow/denyconstantspkg/vmcp/aggregator/default_aggregator.godenyUnlistedderived at construction; applied inshouldAdvertiseTool's no-config branchpkg/vmcp/config/validator.gocmd/thv-operator/pkg/vmcpconfig/converter.godeploy/charts/operator-crds/**(2 files)docs/operator/crd-api.mddocs/operator/virtualmcpserver-api.mddocs/arch/10-virtual-mcp-architecture.md*_test.goDoes this introduce a user-facing change?
Yes. vMCP operators can set
aggregation.defaultVisibility: denyso that only workloads explicitly listed inaggregation.toolshave their tools advertised. Adding a workload to the group then no longer exposes it by default. The default remainsallow, so nothing changes for existing deployments unless the field is set.Special notes for reviewers
Deliberate semantics — "listed means allowed". Under
deny, an entry with neitherfilternorexcludeAlladvertises all of that workload's tools;denygoverns only backends with no entry at all. This matches the request in #6073 ("only showing servers that are specifically listed") and avoids silently blanking out existing curated setups (e.g. overrides-only entries) the moment someone enables the flag. The stricter alternative — deny unlessfilterenumerates — is not what this implements. Locked in by test.The converter is the sharp edge.
convertAggregationhand-copies fields rather than deep-copying, so omittingDefaultVisibilitythere would let the CRD acceptdefaultVisibility: denywhile the rendered config falls back to advertise-everything — a security-relevant setting that looks applied and isn't.TestConvert_DefaultVisibilityPreservedguards this; I verified it genuinely catches the regression by deleting the copy line and confirming the failure (expected "deny", got "") before restoring it. Worth keeping in mind for any future field added here.Upgrade ordering matters. The
aggregationsubtree has nox-kubernetes-preserve-unknown-fields, so on a cluster where the operator is upgraded but the CRDs are not,defaultVisibility: denyis pruned at admission and the setting silently fails open. Standard ToolHive CRD-before-operator ordering, but the failure mode here is "tools you expected hidden are advertised", so it may deserve a release note.Scope — what this does not do. Documented in both docs files:
defaultVisibilityis advertising-only, so it does not affect resources or prompts, which have no filtering config at all. A vMCP scoped this way still exposes every backend resource and prompt.tools/callresolves straight against the routing table, so a hidden tool is still callable by name when no Cedar policy is configured. That is a pre-existing bug independent of this PR and is tracked as a separate follow-up; the Legacy path is unaffected because only advertised tools are registered per session. Reviewers evaluating this as a hardening measure should know that fix is a prerequisite for calling it a boundary.Related but intentionally out of scope: #3493 (tool-level deny-list with default-allow) is a different question — per-tool convenience rather than per-server scoping — and points the opposite way on fail-open. Kept separate so the two defaults don't land in one confusing change.
Generated with Claude Code