Skip to content

Commit 8932f93

Browse files
ChrisJBurnsclaudeamirejazJAORMX
authored
Add StorageVersionMigrator controller (opt-in, default off) (#5362)
* Add StorageVersionMigrator controller (opt-in, default off) Adds a StorageVersionMigrator controller to the operator behind a default-off feature flag (ENABLE_STORAGE_VERSION_MIGRATOR). The controller reconciles status.storedVersions on opted-in toolhive.stacklok.dev CRDs by issuing a Get+Update against each CR, which re-encodes etcd objects to the current storage version. This is required before a future release can drop a deprecated apiVersion (e.g. v1alpha1) from spec.versions without orphaning rows. The flag defaults to false, so this PR has no functional impact for any user until they explicitly opt in via operator.env. Helm chart exposure (values.yaml flag entry and deployment env-var wiring) is deferred to a follow-up PR to reduce accidental enablement during the deprecation window. RBAC for the controller's verbs ships in this PR so the flag can be flipped on at any time without permission-denied errors. Opt-in label markers on the 12 v1beta1 root types and the marker-coverage CI test are deferred to a follow-up PR; the envtest suite creates its own CRDs at runtime and is independent of those changes. Part of #4969. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review feedback - F1: fix doc comment claiming "Enabled by default" — the controller is default-off in this release, opt-in via env var only; chart surface lands in a follow-up PR - F2: fail loudly on unparsable env-var value instead of silently defaulting to disabled (per go-style "Fail Loudly") - F6: document why patchStoredVersions cannot use the operator-wide MutateAndPatchStatus helper (CRD storedVersions is co-owned with kube-apiserver, optimistic lock is load-bearing) - F7: demote disabled-state log from INFO to V(1) (silent success) - F8: prefix env var with TOOLHIVE_ to match sibling operator env vars and avoid cross-operator collisions - Codespell: unparseable→unparsable, overrideable→overridable - Chainsaw RBAC fixture: include the migrator's apiextensions verbs and the wildcard toolhive.stacklok.dev rules so the multi-tenancy setup assertion matches the regenerated ClusterRole Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Sync single-tenancy chainsaw RBAC fixture with regenerated ClusterRole Same update as the multi-tenancy fixture in the previous commit — single-tenancy/setup also snapshots the operator ClusterRole and the migrator's RBAC rules need to appear in its expected YAML too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add unit tests for StorageVersionMigrator controller helpers Covers the parts of storageversionmigrator_controller.go that the envtest suite can't reach or only exercises transitively: migrationCache: TTL expiry with lazy eviction, RV mismatch as a miss, gc() evicting only expired entries, concurrent has/add/gc under the race detector, key uniqueness across CRDs and across UIDs, add() refreshing the expiry on existing entries. Pure predicate functions (isManagedCRD, isToolhiveCRDName, findStorageVersion, isMigrationNeeded): table-driven coverage of every input shape including edge cases the envtest suite would have to spin up an apiserver to exercise (nil labels, no storage version, empty storedVersions, multi-element storedVersions, etc.). Uses the migrationCache's existing `now func() time.Time` seam for frozen-clock TTL tests. No production code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Extend SVM unit tests to ensureInitialized and Reconcile early returns Adds unit tests for the cheaper of the previously-uncovered controller paths: ensureInitialized: defaults applied on zero values, explicit values preserved, idempotent across repeated calls. Reconcile early returns via the controller-runtime fake client: CRD IsNotFound, foreign group skip, missing opt-in label skip, no-storage-version skip, and already-clean storedVersions short-circuit. Brings file coverage from 25.4% to 41.3% (52/126 statements). The remaining uncovered functions (restoreCRs, restoreOne, patchStoredVersions, SetupWithManager) depend on apiserver-side behaviour that the fake client doesn't simulate — storage-encoder elision, optimistic-lock 409s, manager-driven watch wiring — so they stay in envtest territory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add fake-client unit tests for restoreOne/restoreCRs/patchStoredVersions Covers the orchestration code paths in storageversionmigrator_controller.go that the envtest suite also tests, but at code-path level rather than apiserver-semantic level. The fake client can't simulate storage-encoder elision or optimistic-lock 409s, but it can verify: restoreOne: Get + Update propagation, NotFound on Get, Conflict on Update, generic error on Update. restoreCRs: happy path with cache population, empty list, per-CR NotFound silent skip, Conflict counted + sentinel returned, generic error aggregated, mixed conflicts+errors returns aggregate (not sentinel), first List failure wraps and returns immediately, cache hits skip Update, paginating reader confirms Continue token threads through ListOptions across three synthesized pages. patchStoredVersions: storedVersions trimmed in fake store on success, goes through /status subresource (not main resource), patch body carries resourceVersion (proof MergeFromWithOptimisticLock is in effect, not plain MergeFrom), patch errors propagate. File coverage: 41.3% → 77.8% (98/126 statements). - restoreOne, patchStoredVersions: 100% - restoreCRs: 92.3% - Reconcile: 56% (deep paths still envtest-only) - SetupWithManager: 0% (manager wiring not testable without a manager) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Refactor SVM unit tests for conciseness Consolidates several test groups into table-driven form and trims redundant table cases without dropping any meaningful assertion or coverage. File shrinks 1261 → 1028 lines (~18%); coverage stays at 77.8% (98/126 statements on storageversionmigrator_controller.go). Notable consolidations: - 5 TestReconcile_* early-return tests → 1 table-driven TestReconcile_EarlyReturns - 3 TestRestoreOne_Propagates* tests → 1 table-driven TestRestoreOne_PropagatesErrors - 4 TestRestoreCRs_* error-classification tests → 1 table-driven TestRestoreCRs_ErrorClassification - 3 TestPatchStoredVersions_* success tests → 1 TestPatchStoredVersions_SuccessAssertsAllProperties - 2 cache key-collision tests → 1 TestMigrationCache_KeyIsolation - Dropped low-signal TestMigrationCache_GCNoopOnEmpty - Dropped duplicate helpers (newReconcileTestScheme, reconcileWithFake) - Trimmed redundant table cases in pure-function tests Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address second-round PR review feedback (F1, F2, F3, F4, F5, F6) - F1: patchStoredVersions no longer mutates the caller's CRD. Writes the desired state to a deep-copy and uses the original as the merge-patch base. Honors go-style "Copy Before Mutating Caller Input". - F2: AutoMigrateLabel doc comment now reads "will be applied via a kubebuilder marker… in the follow-up PR" instead of falsely claiming the marker is already in place. - F3: Added a chart-consumer note to the RBAC-rationale block explaining that role.yaml's wildcard widens the operator SA's permissions on every install regardless of feature flag. Templating the rule behind a helm conditional is deferred to PR-C. (The reviewer's suggested template comment isn't viable — role.yaml is generated from kubebuilder markers and would lose hand-edits on regen.) - F4: Migrated from legacy k8s.io/client-go/tools/record + GetEventRecorderFor to k8s.io/client-go/tools/events + GetEventRecorder, matching every other reconciler in the package. Reconciler struct field, app.go wiring, three Reconcile call sites (new 7-arg signature), and the test doubles in both the unit-test and envtest files all updated. - F5: ensureInitialized is now wrapped in sync.Once. Provably safe under any number of callers (Setup, Reconcile, or future entrypoints) without dropping the Reconcile-side call that envtest helpers rely on. Addresses the future-race concern more comprehensively than the reviewer's "drop the call" suggestion would have. - F6: Reconcile no longer emits a Warning event when restoreCRs returns the conflict-retry sentinel. Conflicts are normal steady-state under concurrent writes and self-heal on the next reconcile, so the previous Warning event misrepresented routine retries as failures. Logs at V(1) and still returns the error so controller-runtime requeues. F7 (zero-means-default footgun) deliberately not actioned: the existing field comments already document the contract, and the Fail Loudly rule targets user-input constructors, not internal test seams. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address test-review feedback (T2, T3, T6, T7 + coverage gaps) T3: delete TestNewMigrationCache_InitializesUsableInstance — the constructor's struct literal is compiler-enforced; the add/has roundtrip is already covered by TestMigrationCache_AddThenHasReturnsTrue. T6: envtest pagination assertion now uses Equal(3) instead of BeNumerically(">=", 3). With 7 CRs at PageSize=3 the loop count is deterministically 3 — Equal pins the contract so a runaway over-fetch would fail the test. T7: cross-version envtest now asserts spec.marker survives the v1alpha1→v1beta1 re-encode. createCRs already writes the field; reading it back post-reconcile proves the apiserver's encode-decode round-trip preserves spec content, not just bumps RV. T2: renamed "migrates storedVersions from [v1alpha1,v1beta1] to [v1beta1]" → "succeeds end-to-end with elided updates when all CRs are already at storage version". Added countingAPIReader assertion (listCalls == 1) so the test self-documents that the list loop ran. Honest naming + non-trivial assertion. Coverage gap — context cancellation: new envtest spec cancels ctx before reconcile, asserts storedVersions stays intact. Guards against a future refactor that swallows ctx.Err() and trims anyway, which would orphan un-migrated CRs on operator shutdown. Coverage gap — event recording: cross-version test now uses events.NewFakeRecorder and asserts MigrationSucceeded fires; partial-failure test does the same for MigrationFailed. The two exported EventReason constants are public contract for operators consuming the CRD's Events stream. Skipped: T4 (TestRestoreCRs_EmptyListIsNoop) — asserts a real invariant (empty list ⇒ empty cache), not just range semantics. T5 (TestEnsureInitialized_AppliesDefaultsOnZeroValues) — exercises specific default values that indirect tests wouldn't catch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix lint findings on SVM unit tests - thelper: add t.Helper() to per-case check callbacks in TestRestoreOne_PropagatesErrors and TestRestoreCRs_ErrorClassification. Linter (correctly) flags any *testing.T-taking closure as a helper. - unparam: drop newCache(t, ttl) and makeTestCR(namespace, name) params. Every caller passed time.Hour / "default" respectively; hardcode and shrink the call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix codespell findings in SVM unit tests - "re-use" → "reuse" in a comment. - Rename optIn local var → optInLabels. The shorter name was being matched case-insensitively to "optin" → "option" by codespell; optInLabels is more descriptive anyway and dodges the false positive without adding a custom ignore wordlist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update cmd/thv-operator/controllers/storageversionmigrator_controller.go Co-authored-by: Juan Antonio Osorio <ozz@stacklok.com> * Address JAORMX's PR review findings Six controller-side changes + two new tests, addressing valid findings from the multi-agent review. Bundled as one commit because findings #5 and #7 compose. RequeueAfter on the sentinel path. restoreOne now retries up to restoreOneMaxRetries=3 attempts when Update returns IsConflict, re-Getting the live object between attempts to refresh the resourceVersion. Reconcile returns ctrl.Result{RequeueAfter: 30s} with nil error when restoreCRs returns errMigrationRetriedDueToConflicts — this avoids controller-runtime's exponential backoff (5ms→16min) for what is normal steady-state behaviour under concurrent .status churn. Matches upstream kube-storage-version-migrator semantics. restoreOne. The list-page object is now passed directly to Update on the first attempt; retries (after IsConflict) still re-Get to refresh the RV. Halves apiserver read load. After sentinelConflictLogThreshold=5 consecutive sentinel passes the controller emits an INFO log so SREs have operator-visible signal that migration is not converging. Reset to zero on the next successful patchStoredVersions. toolhive.stacklok.dev/*/status. The migrator never writes to a toolhive CR's status subresource — patchStoredVersions targets the CRD's apiextensions.k8s.io status. Regenerated role.yaml; updated both chainsaw RBAC fixtures. comment. Admission webhooks fire on every Update, before the apiserver's bytes-equality check — only etcd writes + watch fanout are elided. The previous wording was materially wrong for any cluster running Kyverno/Gatekeeper. peak list-page memory from ~25MB to ~5MB inside the default 128Mi operator memory limit. never trims and no success event is emitted". Verifies the post-#5 contract: across 6 reconciles with one CR returning IsConflict every pass, err is nil, RequeueAfter=30s, storedVersions stays untrimmed, and no MigrationSucceeded event ever fires. table covering unset, explicit true/false, ParseBool truthy/falsy (1/0), unparseable typo, and empty string. Pins the F2 fail-loudly behaviour from the previous review round. Not actioned (rationale in code-review reply, not commit): - #8 (cache hooks untested) — review missed the existing unit tests for TTL expiry, GC eviction, and concurrent access in the unit-test file. No code change. - #2 (RBAC widening of previously-RO CRDs) — already documented in the PR body's "user-facing change" section. Will refine the PR body wording. - #4 full (metrics pipeline) — significant scope; deferred to follow-up issue. Light visibility piece landed in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Recover lost controller changes from the previous rebase The previous "Address JAORMX's PR review findings" commit (b7c4116) shipped the *tests* for findings #4, #5, #6, #7 but accidentally dropped the *production code* that implements them. The rebase conflict resolution used `git checkout --ours` — but during a rebase "ours" means the branch being rebased onto, not the commits being applied, so the agent's controller-file changes were silently discarded. This commit restores them from the pre-rebase tree (7d063f28e): - #5: restoreOneMaxRetries=3 per-CR IsConflict retry in restoreOne; Reconcile returns ctrl.Result{RequeueAfter: 30s} with nil error on errMigrationRetriedDueToConflicts. - #7: list-page object passed directly to Update on attempt 0; fresh Get only on retry attempts. - #6: webhook-side-effects comment corrected. - #4 (light): per-CRD conflictPasses counter + INFO log after sentinelConflictLogThreshold=5 consecutive sentinel passes. The two existing envtest specs that already shipped against the NEW contract (`leaves storedVersions untouched when a CR re-store hits a Conflict, then trims on retry` and `under sustained conflict pressure...`) now pass because the production code matches what they assert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix CI failures from review-feedback commit - Spellcheck: rename "unparseable" → "unparsable" and replace the "ture" test fixture with "garbage" so codespell doesn't false-flag the deliberate-typo test case. - Chainsaw RBAC fixtures: re-sync to the regenerated role.yaml. Same rules but rule ordering differs; chainsaw asserts byte-exact match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Muhammad Amir Ejaz <amir@stacklok.com> Co-authored-by: Juan Antonio Osorio <ozz@stacklok.com>
1 parent 164c251 commit 8932f93

9 files changed

Lines changed: 2860 additions & 0 deletions

File tree

cmd/thv-operator/app/app.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@ import (
1212
"fmt"
1313
"log/slog"
1414
"os"
15+
"strconv"
1516
"strings"
1617

1718
"github.com/go-logr/logr"
19+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1820
"k8s.io/apimachinery/pkg/runtime"
1921
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
2022
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
@@ -42,10 +44,18 @@ var (
4244
setupLog = log.Log.WithName("setup")
4345
)
4446

47+
// envEnableStorageVersionMigrator is the opt-in for the StorageVersionMigrator
48+
// controller. The controller defaults to OFF in this release so the change can
49+
// ship safely without functional impact. Set to "true" (or "1", "t") to enable.
50+
// A follow-up release will flip the default to true alongside the helm chart
51+
// surface and user docs.
52+
const envEnableStorageVersionMigrator = "TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR"
53+
4554
func init() {
4655
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
4756
utilruntime.Must(mcpv1alpha1.AddToScheme(scheme))
4857
utilruntime.Must(mcpv1beta1.AddToScheme(scheme))
58+
utilruntime.Must(apiextensionsv1.AddToScheme(scheme))
4959
//+kubebuilder:scaffold:scheme
5060
}
5161

@@ -150,10 +160,56 @@ func setupControllersAndWebhooks(mgr ctrl.Manager, imagePullSecretsDefaults imag
150160
if err := setupAggregationControllers(mgr, imagePullSecretsDefaults); err != nil {
151161
return err
152162
}
163+
enabled, err := isStorageVersionMigratorEnabled()
164+
if err != nil {
165+
return err
166+
}
167+
if enabled {
168+
if err := setupStorageVersionMigrator(mgr); err != nil {
169+
return err
170+
}
171+
} else {
172+
setupLog.V(1).Info("StorageVersionMigrator disabled", "envVar", envEnableStorageVersionMigrator)
173+
}
153174
//+kubebuilder:scaffold:builder
154175
return nil
155176
}
156177

178+
// setupStorageVersionMigrator wires the StorageVersionMigrator controller into
179+
// the manager. The controller reconciles status.storedVersions on opted-in
180+
// toolhive.stacklok.dev CRDs so a future operator release can drop deprecated
181+
// versions from spec.versions without orphaning etcd objects.
182+
func setupStorageVersionMigrator(mgr ctrl.Manager) error {
183+
if err := (&controllers.StorageVersionMigratorReconciler{
184+
Client: mgr.GetClient(),
185+
APIReader: mgr.GetAPIReader(),
186+
Scheme: mgr.GetScheme(),
187+
Recorder: mgr.GetEventRecorder("storageversionmigrator-controller"),
188+
}).SetupWithManager(mgr); err != nil {
189+
return fmt.Errorf("unable to create controller StorageVersionMigrator: %w", err)
190+
}
191+
return nil
192+
}
193+
194+
// isStorageVersionMigratorEnabled reports whether the StorageVersionMigrator
195+
// controller should be registered. Defaults to false in this release — admins
196+
// must explicitly opt in via TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR=true.
197+
// An unparsable value returns an error so startup fails loudly rather than
198+
// silently disabling the feature an admin asked to turn on.
199+
func isStorageVersionMigratorEnabled() (bool, error) {
200+
value, found := os.LookupEnv(envEnableStorageVersionMigrator)
201+
if !found {
202+
return false, nil
203+
}
204+
enabled, err := strconv.ParseBool(value)
205+
if err != nil {
206+
return false, fmt.Errorf(
207+
"invalid value for %s: %q (expected true/false): %w",
208+
envEnableStorageVersionMigrator, value, err)
209+
}
210+
return enabled, nil
211+
}
212+
157213
// setupGroupRefFieldIndexes sets up field indexing for spec.groupRef on all resource types
158214
// that can reference an MCPGroup. This enables efficient lookups by groupRef in controllers.
159215
func setupGroupRefFieldIndexes(mgr ctrl.Manager) error {

cmd/thv-operator/app/app_test.go

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package app
5+
6+
import (
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// TestIsStorageVersionMigratorEnabled exercises the env-var contract for the
14+
// StorageVersionMigrator feature flag. The function must:
15+
// - default to (false, nil) when the env var is unset (the controller is
16+
// opt-in for this release),
17+
// - accept strconv.ParseBool's full truth-table for explicit values,
18+
// - fail loudly on unparsable values so a misconfigured admin sees a
19+
// startup error instead of silently disabled migration.
20+
//
21+
// The error-case rows assert on both the env-var name AND the offending
22+
// value being present in the error message, so a future refactor that
23+
// drops either fragment fails this test.
24+
func TestIsStorageVersionMigratorEnabled(t *testing.T) {
25+
// Intentionally NOT t.Parallel(): subtests use t.Setenv, which
26+
// panics if the test (or any ancestor) is parallel. Subtests are
27+
// also serial for the same reason. The trade-off is negligible —
28+
// the test is sub-millisecond — and matches Go 1.20+ guidance for
29+
// env-var-driven tests.
30+
31+
tests := []struct {
32+
name string
33+
setEnv bool
34+
envValue string
35+
wantEnabled bool
36+
wantErr bool
37+
}{
38+
{
39+
name: "unset defaults to disabled",
40+
setEnv: false,
41+
wantEnabled: false,
42+
wantErr: false,
43+
},
44+
{
45+
name: "explicit true enables",
46+
setEnv: true,
47+
envValue: "true",
48+
wantEnabled: true,
49+
wantErr: false,
50+
},
51+
{
52+
name: "explicit false disables",
53+
setEnv: true,
54+
envValue: "false",
55+
wantEnabled: false,
56+
wantErr: false,
57+
},
58+
{
59+
name: "explicit 1 enables (ParseBool truthy)",
60+
setEnv: true,
61+
envValue: "1",
62+
wantEnabled: true,
63+
wantErr: false,
64+
},
65+
{
66+
name: "explicit 0 disables (ParseBool falsy)",
67+
setEnv: true,
68+
envValue: "0",
69+
wantEnabled: false,
70+
wantErr: false,
71+
},
72+
{
73+
name: "unparsable value errors with env-var name and bad value",
74+
setEnv: true,
75+
envValue: "garbage",
76+
wantEnabled: false,
77+
wantErr: true,
78+
},
79+
{
80+
name: "empty string errors (ParseBool rejects empty)",
81+
setEnv: true,
82+
envValue: "",
83+
wantEnabled: false,
84+
wantErr: true,
85+
},
86+
}
87+
88+
for _, tc := range tests {
89+
t.Run(tc.name, func(t *testing.T) {
90+
// Subtests intentionally do NOT call t.Parallel(): t.Setenv
91+
// is incompatible with a parallel *testing.T (it panics with
92+
// "test using t.Setenv ... can not use t.Parallel"). The
93+
// outer test still runs in parallel with other tests in the
94+
// package — only sibling subtests of this test serialize,
95+
// which is fine because t.Setenv restores the prior value
96+
// at the subtest's end.
97+
if tc.setEnv {
98+
t.Setenv(envEnableStorageVersionMigrator, tc.envValue)
99+
}
100+
101+
got, err := isStorageVersionMigratorEnabled()
102+
103+
if tc.wantErr {
104+
require.Error(t, err)
105+
assert.Contains(t, err.Error(), envEnableStorageVersionMigrator,
106+
"error message must name the env var so admins can find the misconfiguration")
107+
assert.Contains(t, err.Error(), `"`+tc.envValue+`"`,
108+
"error message must quote the offending value so admins can spot typos")
109+
} else {
110+
require.NoError(t, err)
111+
}
112+
assert.Equal(t, tc.wantEnabled, got)
113+
})
114+
}
115+
}

0 commit comments

Comments
 (0)