Add option to skip Virtual MCP CRDs installation #2729

amirejaz · 2025-11-25T14:43:33Z

Add Feature Flags for Controller Groups and Optional Virtual MCP CRD Installation

Summary

This PR implements feature flags to enable/disable controller groups in the ToolHive operator and adds the ability to skip installing Virtual MCP CRDs. This allows users to deploy a minimal operator configuration when they only need core MCP server management features.

Related Issues

Closes Add optional installation flag for Virtual MCP CRDs in operator-crds Helm chart #2709 - Allow skipping Virtual MCP CRD installation
Closes Provide feature flags for enabling/disabling controllers #2564 - Add feature flags to enable/disable controllers

Changes

1. Feature Flags Implementation

Added three environment variable flags to control controller groups:

ENABLE_SERVER (default: true)
- Controls: MCPServer, MCPExternalAuthConfig, MCPRemoteProxy, and ToolConfig controllers
- When disabled: Skips all server-related controllers and field indexing
ENABLE_REGISTRY (default: true)
- Controls: MCPRegistry controller
- When disabled: Skips MCPRegistry controller
- Also affects MCPServer image validation mode (registry-enforcing vs always-allow)
ENABLE_AGGREGATION (default: true)
- Controls: VirtualMCPServer, MCPGroup controllers and their webhooks
- Dependency: Requires ENABLE_SERVER=true (validated with warning log)
- When disabled: Skips Virtual MCP aggregation features

2. Optional Virtual MCP CRD Installation

Added crds.install.virtualMCP option to the toolhive-operator-crds Helm chart:

When set to false: Skips installing VirtualMCPServer and VirtualMCPCompositeToolDefinition CRDs
Saves approximately 54KB of CRDs
Users should also set ENABLE_AGGREGATION=false in the operator to prevent controller errors

3. Code Refactoring

Refactored setupControllersAndWebhooks function into smaller, focused functions:

setupServerControllers() - Sets up server-related controllers
setupRegistryController() - Sets up registry controller
setupAggregationControllers() - Sets up aggregation controllers and webhooks

This improves maintainability and makes the code easier to test.

4. Image Validation Logic

Fixed image validation mode selection:

When ENABLE_REGISTRY=true: MCPServer uses ImageValidationRegistryEnforcing
When ENABLE_REGISTRY=false: MCPServer uses ImageValidationAlwaysAllow
Previously, this was set incorrectly after controller setup

Usage Examples

Minimal Deployment (Core Server Management Only)

# Skip Virtual MCP CRDs
helm upgrade -i toolhive-operator-crds oci://ghcr.io/stacklok/toolhive/toolhive-operator-crds \
  --set crds.install.virtualMCP=false

# Disable registry and aggregation features
helm upgrade -i toolhive-operator oci://ghcr.io/stacklok/toolhive/toolhive-operator \
  -n toolhive-system --create-namespace \
  --set operator.env.ENABLE_REGISTRY=false \
  --set operator.env.ENABLE_AGGREGATION=false

Registry-Only Deployment

# Enable only server and registry features
helm upgrade -i toolhive-operator oci://ghcr.io/stacklok/toolhive/toolhive-operator \
  -n toolhive-system --create-namespace \
  --set operator.env.ENABLE_AGGREGATION=false

Backward Compatibility

All feature flags default to true when not set
Existing deployments continue to work without any changes
CRD installation defaults to true (installs all CRDs)

Documentation

Updated operator-crds/README.md with feature flags documentation
Updated operator/values.yaml with feature flag comments
Added usage examples and dependency information

Testing

✅ Code compiles successfully
✅ All linting checks pass
✅ Functionality verified with feature flags enabled/disabled

JAORMX

Let's also add an environment variable to disable the controllers if this is enabled.

codecov · 2025-11-25T17:21:03Z

Codecov Report

❌ Patch coverage is 0% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.38%. Comparing base (c73c5ef) to head (07a8538).

Files with missing lines	Patch %	Lines
cmd/thv-operator/main.go	0.00%	53 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2729      +/-   ##
==========================================
- Coverage   56.48%   56.38%   -0.11%     
==========================================
  Files         319      319              
  Lines       30943    30992      +49     
==========================================
- Hits        17479    17475       -4     
- Misses      11960    12012      +52     
- Partials     1504     1505       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JAORMX

@amirejaz what about the virtual MCP controllers? shouldn't those be handled via a boolean as well? I was thinking the helm boolean would set the ENABLE_VMCP (or similar) environment variable in the main operator deployment and thus allow us to toggle easily. I'm not sure we should expose environment variables like this, instead, we should use helm flags with proper documentation.

jhrozek · 2025-11-26T14:46:20Z

@dmartinol FYI you were interested in reviewing this on our call

dmartinol · 2025-11-26T15:37:48Z

deploy/charts/operator-crds/values.yaml

+    # -- Install Virtual MCP CRDs (VirtualMCPServer and VirtualMCPCompositeToolDefinition).
+    # Users who only need core MCP server management can set this to false to skip
+    # installing Virtual MCP aggregation features (saves ~54KB of CRDs).
+    virtualMCP: true


pls add the same implementation to disable the registry and server CRDs according to the specs at #2564

Good point, will update the PR.

dmartinol

my concern is that there are mixed CRDs and controllers that seem not to be managed, at least not as defined in the #2564
and I fear we should also anticipate the removal of unneeded controllers from the platform, as requested in #2662 so that we can reduce the number of controllers to handle by these flags

dmartinol · 2025-11-26T15:38:00Z

...charts/operator-crds/crd-files/toolhive.stacklok.dev_virtualmcpcompositetooldefinitions.yaml

I think these files are generated, it's not enough renaming them, you should update the operator-manifests task.
anyway, these

updated operator-manifest

dmartinol · 2025-11-26T15:44:42Z

deploy/charts/operator-crds/README.md.gotmpl

not a file to be edited IMO, ask @ChrisJBurns

@ChrisJBurns ?

You shouldn't need to modify this file unless you're providing information that you want generated onto the README.md. In this case it may be ok. Otherwise if it's only on the readme.md it will get overridden by the gotmpl

dmartinol · 2025-11-26T15:45:36Z

deploy/charts/operator-crds/README.md

 helm uninstall <release_name>
 ```

+### Skipping Virtual MCP CRDs


the scope should be extended to disable registry CRD

dmartinol

I assume @jhrozek will also share his thoughts here.
Once we agree on the design, pls also update the ITs to re-run the existing tests with the other features disabled

dmartinol · 2025-11-27T07:11:05Z

.github/workflows/operator-ci.yml

        id: git-check
        run: |
-          git diff --exit-code deploy/charts/operator-crds/crds || echo "crd-changes=true" >> $GITHUB_OUTPUT
+          git diff --exit-code deploy/charts/operator-crds/crds deploy/charts/operator-crds/crds-server deploy/charts/operator-crds/crds-registry deploy/charts/operator-crds/crds-virtualmcp || echo "crd-changes=true" >> $GITHUB_OUTPUT


what about using regex like:
... deploy/charts/operator-crds/crds* ...?

dmartinol · 2025-11-27T07:17:11Z

deploy/charts/operator-crds/templates/mcpexternalauthconfigs-crd.yaml

@@ -0,0 +1,5 @@
+{{- if .Values.crds.install.server }}


IIRC, external auth can also be used with virtual MCPs, so this should be in OR with .Values.crds.install.virtualMCP

dmartinol · 2025-11-27T07:22:16Z

.github/workflows/operator-ci.yml

        id: git-check
        run: |
-          git diff --exit-code deploy/charts/operator-crds/crds || echo "crd-changes=true" >> $GITHUB_OUTPUT
+          git diff --exit-code deploy/charts/operator-crds/crds deploy/charts/operator-crds/crds-server deploy/charts/operator-crds/crds-registry deploy/charts/operator-crds/crds-virtualmcp || echo "crd-changes=true" >> $GITHUB_OUTPUT


BTW: why aren't we keeping all the original CRDs in the same folder instead? This would simplify all the involved steps (of course, ITs should list all the individual files)

if we keep all of them under crds/, it automatically installs all the crds. I moved all of them in one folder crd-files.

this is what I meant: the same folder but different from the one used as the chart template

dmartinol · 2025-11-27T07:24:38Z

cmd/thv-operator/test-integration/mcp-server/suite_test.go

-		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "..", "..", "deploy", "charts", "operator-crds", "crds")},
+		CRDDirectoryPaths: []string{
+			filepath.Join("..", "..", "..", "..", "deploy", "charts", "operator-crds", "crds-server"),
+			filepath.Join("..", "..", "..", "..", "deploy", "charts", "operator-crds", "crds-virtualmcp"),


I see MCPGroup is needed here because the association is bottom-up, from server to group: the server controller should take this into account and avoid, e.g., using validateGroupRef

dmartinol · 2025-11-27T07:31:42Z

cmd/thv-operator/test-integration/virtualmcp/suite_test.go

 	testEnv = &envtest.Environment{
-		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "..", "..", "deploy", "charts", "operator-crds", "crds")},
+		CRDDirectoryPaths: []string{
+			filepath.Join("..", "..", "..", "..", "deploy", "charts", "operator-crds", "crds-server"),


Even if this is logically correct, I imagine the tests would work even w/o the server CRD.
Anyway, I imagine that the vMCP controllers should fail to start if the server functionality is disabled

dmartinol · 2025-11-27T07:33:20Z

cmd/thv-operator/main.go

+	// Check feature flags
+	enableServer := isFeatureEnabled("ENABLE_SERVER", true)
+	enableRegistry := isFeatureEnabled("ENABLE_REGISTRY", true)
+	enableAggregation := isFeatureEnabled("ENABLE_AGGREGATION", true)


I think enableAggregation also depends on enableServer

I think this is going to become quite brittle quite quickly. Could we have a simple dependency Go map where we express the dependencies?

Another possible simplification would be to not implement ENABLE_SERVER for now but only ENABLE_REGISTRY and ENABLE_AGGREGATION and treat ENABLE_SERVER as a dependency.

@dmartinol did you have a use case for ENABLE_REGISTRY only? I know it was in the original issue but I wasn't sure if it was just for completeness

@dmartinol did you have a use case for ENABLE_REGISTRY only? I know it was in the original issue but I wasn't sure if it was just for completeness

I imagine it would be for those wanting to deploy an "official" registry in k8s without caring about the toolhive tools. The same could be probably achieved via an ad-hoc deployment that someone tracked in the design document as:

In Kubernetes via a Helm chart (Optional, if we have enough time)

(I'm not sure this is tracked for the MVP, anyway)

I think this is going to become quite brittle quite quickly. Could we have a simple dependency Go map where we express the dependencies?

Move the dependencies to the go map.

dmartinol · 2025-11-27T07:39:46Z

deploy/charts/operator-crds/README.md.gotmpl

@ChrisJBurns ?

jhrozek

I need to digest the changes, better, submitting a first pass

jhrozek · 2025-11-27T10:11:55Z

deploy/charts/operator-crds/README.md

+  --set crds.install.virtualMCP=false
+```
+
+This saves approximately 54KB of CRDs and is useful for deployments that don't require Virtual MCP aggregation features.


Let's not include the size, it screams "AI generated".

jhrozek · 2025-11-27T10:48:32Z

cmd/thv-operator/main.go

+
+// setupServerControllers sets up server-related controllers (MCPServer, MCPExternalAuthConfig, MCPRemoteProxy, ToolConfig)
+func setupServerControllers(mgr ctrl.Manager, enableRegistry bool) error {
 	// Create a shared platform detector for all controllers


At least the comment is a bit misleading. I wonder if there is a reason not to pass the detector down from the caller, IIRC it already uses sync.Once?

passed platform detector from caller

jhrozek · 2025-11-27T10:49:31Z

cmd/thv-operator/main.go

+	if !found {
+		return defaultValue
+	}
+	return strings.EqualFold(value, "true")


strconv.ParseBool ?

jhrozek · 2025-11-27T11:20:26Z

cmd/thv-operator/main.go

+	// Check feature flags
+	enableServer := isFeatureEnabled("ENABLE_SERVER", true)
+	enableRegistry := isFeatureEnabled("ENABLE_REGISTRY", true)
+	enableAggregation := isFeatureEnabled("ENABLE_AGGREGATION", true)


I think this is going to become quite brittle quite quickly. Could we have a simple dependency Go map where we express the dependencies?

Another possible simplification would be to not implement ENABLE_SERVER for now but only ENABLE_REGISTRY and ENABLE_AGGREGATION and treat ENABLE_SERVER as a dependency.

@dmartinol did you have a use case for ENABLE_REGISTRY only? I know it was in the original issue but I wasn't sure if it was just for completeness

jhrozek · 2025-11-27T11:21:04Z

deploy/charts/operator/templates/deployment.yaml

+            value: {{ .Values.operator.features.server | quote }}
+          - name: ENABLE_REGISTRY
+            value: {{ .Values.operator.features.registry | quote }}
+          - name: ENABLE_AGGREGATION


Could we just call this ENABLE_VMCP ?

jhrozek · 2025-11-27T11:25:03Z

deploy/charts/operator-crds/crd-files/toolhive.stacklok.dev_mcpgroups.yaml

I'm not sure if this move is correct, MCPGroup is usable even without vMCP, moving it here would break the use-case.

what use case is it?

jhrozek · 2025-11-27T11:32:15Z

cmd/thv-operator/Taskfile.yml

+      # Move server CRDs
+      - cmd: |
+          mv deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpservers.yaml deploy/charts/operator-crds/crds-server/ 2>/dev/null || true
+          mv deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml deploy/charts/operator-crds/crds-server/ 2>/dev/null || true


Why are we ignoring errors?

dmartinol · 2025-11-28T16:13:31Z

deploy/charts/operator-crds/README.md

 helm uninstall <release_name>
 ```

+### Skipping CRDs


This is great but maybe a summary table could be more effective.
Can be fixed in another PR if we think it's needed.

dmartinol · 2025-11-28T16:14:51Z

deploy/charts/operator-crds/values.yaml

+    # Users who only need server management without registry features can set this to false
+    # to skip installing the registry CRD.
+    registry: true
+    # -- Install Virtual MCP CRDs (VirtualMCPServer and VirtualMCPCompositeToolDefinition).


and MCPGroup

dmartinol · 2025-11-28T16:18:22Z

deploy/charts/operator/templates/clusterrole/role.yaml

-  - get
-  - list
-  - watch
+{{- if not .Values.operator.testMode }}


could you explain where this operator.testMode come from now?

operator.testMode comes from our chart-testing setup and is used in CI to disable operator RBAC/resources to avoid Helm ownership conflicts during ct install.

dmartinol

/lgtm leaving final approval to @jhrozek
Thanks!

github-actions bot added the size/XS Extra small PR: < 100 lines changed label Nov 25, 2025

JAORMX reviewed Nov 25, 2025

View reviewed changes

github-actions bot added size/XS Extra small PR: < 100 lines changed size/S Small PR: 100-299 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Nov 25, 2025

github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Nov 25, 2025

amirejaz requested a review from JAORMX November 25, 2025 17:44

github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Nov 25, 2025

JAORMX reviewed Nov 26, 2025

View reviewed changes

github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/S Small PR: 100-299 lines changed labels Nov 26, 2025

dmartinol reviewed Nov 26, 2025

View reviewed changes

github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Nov 26, 2025

dmartinol reviewed Nov 27, 2025

View reviewed changes

jhrozek reviewed Nov 27, 2025

View reviewed changes

github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Nov 27, 2025

amirejaz force-pushed the operator-crds-optional-virtualmcp branch from 2ceca6b to 215e33a Compare November 28, 2025 01:52

add option to skip Virtual MCP CRDs and controllers installation

d2c3613

amirejaz force-pushed the operator-crds-optional-virtualmcp branch from 215e33a to d2c3613 Compare November 28, 2025 02:00

github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Nov 28, 2025

amirejaz added 2 commits November 28, 2025 14:31

set skip clean up to true and skips cluser roles in test mode

788abbf

merged remote main into local

22a0459

github-actions bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed labels Nov 28, 2025

remove unnecessary config

178efca

github-actions bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Nov 28, 2025

amirejaz marked this pull request as ready for review November 28, 2025 15:24

dmartinol reviewed Nov 28, 2025

View reviewed changes

added default false in the wrapper condition

07a8538

github-actions bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Nov 28, 2025

Add option to skip Virtual MCP CRDs installation #2729

Are you sure you want to change the base?

Add option to skip Virtual MCP CRDs installation #2729

Uh oh!

Conversation

amirejaz commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add Feature Flags for Controller Groups and Optional Virtual MCP CRD Installation

Summary

Related Issues

Changes

1. Feature Flags Implementation

2. Optional Virtual MCP CRD Installation

3. Code Refactoring

4. Image Validation Logic

Usage Examples

Minimal Deployment (Core Server Management Only)

Registry-Only Deployment

Backward Compatibility

Documentation

Testing

Uh oh!

JAORMX left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JAORMX left a comment

Choose a reason for hiding this comment

Uh oh!

jhrozek commented Nov 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmartinol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmartinol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirejaz commented Nov 25, 2025 •

edited

Loading

codecov bot commented Nov 25, 2025 •

edited

Loading