Add StorageVersionMigrator controller (opt-in, default off)

cmd/thv-operator/app/app.go

@@ -12,9 +12,11 @@ import (
 	"fmt"
 	"log/slog"
 	"os"
+	"strconv"
 	"strings"
 
 	"github.com/go-logr/logr"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
@@ -42,10 +44,18 @@ var (
 	setupLog = log.Log.WithName("setup")
 )
 
+// envEnableStorageVersionMigrator is the opt-in for the StorageVersionMigrator
+// controller. The controller defaults to OFF in this release so the change can
+// ship safely without functional impact. Set to "true" (or "1", "t") to enable.
+// A follow-up release will flip the default to true alongside the helm chart
+// surface and user docs.
+const envEnableStorageVersionMigrator = "TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR"
+
 func init() {
 	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
 	utilruntime.Must(mcpv1alpha1.AddToScheme(scheme))
 	utilruntime.Must(mcpv1beta1.AddToScheme(scheme))
+	utilruntime.Must(apiextensionsv1.AddToScheme(scheme))
 	//+kubebuilder:scaffold:scheme
 }
 
@@ -150,10 +160,56 @@ func setupControllersAndWebhooks(mgr ctrl.Manager, imagePullSecretsDefaults imag
 	if err := setupAggregationControllers(mgr, imagePullSecretsDefaults); err != nil {
 		return err
 	}
+	enabled, err := isStorageVersionMigratorEnabled()
+	if err != nil {
+		return err
+	}
+	if enabled {
+		if err := setupStorageVersionMigrator(mgr); err != nil {
+			return err
+		}
+	} else {
+		setupLog.V(1).Info("StorageVersionMigrator disabled", "envVar", envEnableStorageVersionMigrator)
+	}
 	//+kubebuilder:scaffold:builder
 	return nil
 }
 
+// setupStorageVersionMigrator wires the StorageVersionMigrator controller into
+// the manager. The controller reconciles status.storedVersions on opted-in
+// toolhive.stacklok.dev CRDs so a future operator release can drop deprecated
+// versions from spec.versions without orphaning etcd objects.
+func setupStorageVersionMigrator(mgr ctrl.Manager) error {
+	if err := (&controllers.StorageVersionMigratorReconciler{
+		Client:    mgr.GetClient(),
+		APIReader: mgr.GetAPIReader(),
+		Scheme:    mgr.GetScheme(),
+		Recorder:  mgr.GetEventRecorder("storageversionmigrator-controller"),
+	}).SetupWithManager(mgr); err != nil {
+		return fmt.Errorf("unable to create controller StorageVersionMigrator: %w", err)
+	}
+	return nil
+}
+
+// isStorageVersionMigratorEnabled reports whether the StorageVersionMigrator
+// controller should be registered. Defaults to false in this release — admins
+// must explicitly opt in via TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR=true.
+// An unparsable value returns an error so startup fails loudly rather than
+// silently disabling the feature an admin asked to turn on.
+func isStorageVersionMigratorEnabled() (bool, error) {
+	value, found := os.LookupEnv(envEnableStorageVersionMigrator)
+	if !found {
+		return false, nil
+	}
+	enabled, err := strconv.ParseBool(value)
+	if err != nil {
+		return false, fmt.Errorf(
+			"invalid value for %s: %q (expected true/false): %w",
+			envEnableStorageVersionMigrator, value, err)
+	}
+	return enabled, nil
+}
+
 // setupGroupRefFieldIndexes sets up field indexing for spec.groupRef on all resource types
 // that can reference an MCPGroup. This enables efficient lookups by groupRef in controllers.
 func setupGroupRefFieldIndexes(mgr ctrl.Manager) error {

cmd/thv-operator/app/app_test.go

@@ -0,0 +1,115 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestIsStorageVersionMigratorEnabled exercises the env-var contract for the
+// StorageVersionMigrator feature flag. The function must:
+//   - default to (false, nil) when the env var is unset (the controller is
+//     opt-in for this release),
+//   - accept strconv.ParseBool's full truth-table for explicit values,
+//   - fail loudly on unparsable values so a misconfigured admin sees a
+//     startup error instead of silently disabled migration.
+//
+// The error-case rows assert on both the env-var name AND the offending
+// value being present in the error message, so a future refactor that
+// drops either fragment fails this test.
+func TestIsStorageVersionMigratorEnabled(t *testing.T) {
+	// Intentionally NOT t.Parallel(): subtests use t.Setenv, which
+	// panics if the test (or any ancestor) is parallel. Subtests are
+	// also serial for the same reason. The trade-off is negligible —
+	// the test is sub-millisecond — and matches Go 1.20+ guidance for
+	// env-var-driven tests.
+
+	tests := []struct {
+		name        string
+		setEnv      bool
+		envValue    string
+		wantEnabled bool
+		wantErr     bool
+	}{
+		{
+			name:        "unset defaults to disabled",
+			setEnv:      false,
+			wantEnabled: false,
+			wantErr:     false,
+		},
+		{
+			name:        "explicit true enables",
+			setEnv:      true,
+			envValue:    "true",
+			wantEnabled: true,
+			wantErr:     false,
+		},
+		{
+			name:        "explicit false disables",
+			setEnv:      true,
+			envValue:    "false",
+			wantEnabled: false,
+			wantErr:     false,
+		},
+		{
+			name:        "explicit 1 enables (ParseBool truthy)",
+			setEnv:      true,
+			envValue:    "1",
+			wantEnabled: true,
+			wantErr:     false,
+		},
+		{
+			name:        "explicit 0 disables (ParseBool falsy)",
+			setEnv:      true,
+			envValue:    "0",
+			wantEnabled: false,
+			wantErr:     false,
+		},
+		{
+			name:        "unparsable value errors with env-var name and bad value",
+			setEnv:      true,
+			envValue:    "garbage",
+			wantEnabled: false,
+			wantErr:     true,
+		},
+		{
+			name:        "empty string errors (ParseBool rejects empty)",
+			setEnv:      true,
+			envValue:    "",
+			wantEnabled: false,
+			wantErr:     true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Subtests intentionally do NOT call t.Parallel(): t.Setenv
+			// is incompatible with a parallel *testing.T (it panics with
+			// "test using t.Setenv ... can not use t.Parallel"). The
+			// outer test still runs in parallel with other tests in the
+			// package — only sibling subtests of this test serialize,
+			// which is fine because t.Setenv restores the prior value
+			// at the subtest's end.
+			if tc.setEnv {
+				t.Setenv(envEnableStorageVersionMigrator, tc.envValue)
+			}
+
+			got, err := isStorageVersionMigratorEnabled()
+
+			if tc.wantErr {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), envEnableStorageVersionMigrator,
+					"error message must name the env var so admins can find the misconfiguration")
+				assert.Contains(t, err.Error(), `"`+tc.envValue+`"`,
+					"error message must quote the offending value so admins can spot typos")
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tc.wantEnabled, got)
+		})
+	}
+}