Skip to content

Commit b3a57f0

Browse files
ChrisJBurnsclaude
andcommitted
Reject storage version migrator in namespace scope
The StorageVersionMigrator watches cluster-scoped CustomResourceDefinitions and re-stores custom resources across all namespaces, so it requires cluster-scoped RBAC and a cluster-scoped manager cache. A namespace-scoped operator gets only per-namespace RoleBindings and a namespace-restricted cache, so the controller cannot sync its CRD informer and wedges the manager — which is what broke the multi-tenancy chainsaw E2E once the feature was defaulted on. Add a Helm validation that fails the render when storageVersionMigrator is true under operator.rbac.scope=namespace, with an actionable message. Fail loudly rather than silently dropping the feature: a silent drop would let a namespace-scoped admin believe storedVersions are being kept clean when they are not, leading to the skip-a-version upgrade trap later. - Add validateStorageVersionMigrator helper; invoke it from deployment.yaml - Opt the namespace-scoped multi-tenancy chainsaw setup out of the migrator - Pin the rejection and the supported namespace opt-out in helm unittests; disable the migrator in the namespace-scope suite's shared values - Document the cluster-scope requirement and the standalone-migrator alternative for namespace-scoped clusters Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 01bffcf commit b3a57f0

8 files changed

Lines changed: 75 additions & 2 deletions

File tree

deploy/charts/operator/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ The command removes all the Kubernetes components associated with the chart and
6161
| operator.defaultRedis.existingSecretKey | string | `""` | existingSecretKey is the key within existingSecret that holds the password. Empty means use global.redis.existingSecretKey or fall back to "redis-password". |
6262
| operator.env | list | `[]` | Environment variables to set in the operator container. Supported toolhive-specific variables include: - TOOLHIVE_SKIP_UPDATE_CHECK: set to "true" to disable the operator's periodic update check against the ToolHive update API. Also disables the usage-metrics collection that is gated on the same check. |
6363
| operator.features.experimental | bool | `false` | Enable experimental features |
64-
| operator.features.storageVersionMigrator | bool | `true` | Enable the StorageVersionMigrator controller, which auto-cleans status.storedVersions on opted-in toolhive.stacklok.dev CRDs so a future release can drop deprecated versions (e.g. v1alpha1) without orphaning etcd objects in the cluster. Enabled by default; set to false to opt out and handle storage-version cleanup yourself. Sets TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR in the operator deployment. |
64+
| operator.features.storageVersionMigrator | bool | `true` | Enable the StorageVersionMigrator controller, which auto-cleans status.storedVersions on opted-in toolhive.stacklok.dev CRDs so a future release can drop deprecated versions (e.g. v1alpha1) without orphaning etcd objects in the cluster. Enabled by default; set to false to opt out and handle storage-version cleanup yourself. Sets TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR in the operator deployment. Requires `operator.rbac.scope=cluster` — the controller watches cluster-scoped CRDs and re-stores resources across all namespaces, so the chart rejects this being true when scope is namespace. |
6565
| operator.gc | object | `{"gogc":75,"gomemlimit":"110MiB"}` | Go memory limits and garbage collection percentage for the operator container |
6666
| operator.gc.gogc | int | `75` | Go garbage collection percentage for the operator container |
6767
| operator.gc.gomemlimit | string | `"110MiB"` | Go memory limits for the operator container |

deploy/charts/operator/templates/_helpers.tpl

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,24 @@ Common labels for the toolhive resources
6868
{{- define "toolhive.labels" -}}
6969
app: toolhive
7070
app.kubernetes.io/name: toolhive
71-
{{- end }}
71+
{{- end }}
72+
73+
{{/*
74+
Validate feature-flag / RBAC-scope combinations and fail the render early with
75+
an actionable message rather than deploying a wedged operator.
76+
77+
The StorageVersionMigrator controller watches cluster-scoped
78+
CustomResourceDefinition objects and re-stores custom resources across every
79+
namespace, so it requires cluster-scoped RBAC and a cluster-scoped manager
80+
cache. A namespace-scoped operator (operator.rbac.scope=namespace) gets only
81+
per-namespace RoleBindings and a namespace-restricted cache, so the controller
82+
cannot sync its CRD informer and would prevent the manager from starting. We
83+
fail loudly here instead of silently dropping the feature, because a silent
84+
drop would let a namespace-scoped admin believe storedVersions are being kept
85+
clean when they are not.
86+
*/}}
87+
{{- define "toolhive-operator.validateStorageVersionMigrator" -}}
88+
{{- if and .Values.operator.features.storageVersionMigrator (ne .Values.operator.rbac.scope "cluster") -}}
89+
{{- fail "operator.features.storageVersionMigrator requires operator.rbac.scope=cluster: the StorageVersionMigrator controller watches cluster-scoped CustomResourceDefinitions and re-stores resources across all namespaces, which a namespace-scoped operator cannot do. Set operator.features.storageVersionMigrator=false for namespace-scoped installs." -}}
90+
{{- end -}}
91+
{{- end -}}

deploy/charts/operator/templates/deployment.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
{{- include "toolhive-operator.validateStorageVersionMigrator" . }}
12
apiVersion: apps/v1
23
kind: Deployment
34
metadata:

deploy/charts/operator/tests/feature_flags_test.yaml

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ suite: opt-in feature flags
33
# vars. A dropped quote or a renamed var silently flips the feature, so pin both
44
# the enabled and disabled states. storageVersionMigrator defaults to true, so
55
# the disabled state is the one the baseline suite can't cover — pin it here.
6+
# The migrator also requires cluster-scoped RBAC, so the chart rejects it in
7+
# namespace scope; pin that validation too.
68
templates:
79
- deployment.yaml
810
tests:
@@ -27,3 +29,30 @@ tests:
2729
- contains:
2830
path: 'spec.template.spec.containers[0].env'
2931
content: { name: TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR, value: "false" }
32+
33+
# The migrator watches cluster-scoped CRDs, so it must never be enabled in a
34+
# namespace-scoped install. The chart fails the render rather than deploying a
35+
# wedged operator. Pin both the rejection and the supported opt-out.
36+
- it: rejects the storage version migrator in namespace scope
37+
template: deployment.yaml
38+
set:
39+
operator.rbac.scope: namespace
40+
operator.rbac.allowedNamespaces: [toolhive-system]
41+
operator.features.storageVersionMigrator: true
42+
asserts:
43+
- failedTemplate:
44+
errorPattern: "operator.features.storageVersionMigrator requires operator.rbac.scope=cluster"
45+
46+
- it: allows namespace scope when the storage version migrator is disabled
47+
template: deployment.yaml
48+
set:
49+
operator.rbac.scope: namespace
50+
operator.rbac.allowedNamespaces: [toolhive-system]
51+
operator.features.storageVersionMigrator: false
52+
asserts:
53+
- contains:
54+
path: 'spec.template.spec.containers[0].env'
55+
content: { name: TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR, value: "false" }
56+
- contains:
57+
path: 'spec.template.spec.containers[0].env'
58+
content: { name: WATCH_NAMESPACE, value: "toolhive-system" }

deploy/charts/operator/tests/namespace_scope_test.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ suite: namespace-scoped install
66
set:
77
operator.rbac.scope: namespace
88
operator.rbac.allowedNamespaces: [team-a, team-b]
9+
# storageVersionMigrator requires cluster scope; the chart rejects it under
10+
# namespace scope (see feature_flags_test.yaml), so disable it here.
11+
operator.features.storageVersionMigrator: false
912
templates:
1013
- deployment.yaml
1114
- clusterrole/rolebinding.yaml

deploy/charts/operator/values.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ operator:
1414
# orphaning etcd objects in the cluster. Enabled by default; set to
1515
# false to opt out and handle storage-version cleanup yourself. Sets
1616
# TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR in the operator deployment.
17+
# Requires `operator.rbac.scope=cluster` — the controller watches
18+
# cluster-scoped CRDs and re-stores resources across all namespaces, so
19+
# the chart rejects this being true when scope is namespace.
1720
storageVersionMigrator: true
1821
# -- Number of replicas for the operator deployment
1922
replicaCount: 1

docs/operator/storage-version-migration.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,19 @@ operator:
8383
8484
Once enabled, the controller is dormant on CRDs whose `storedVersions` already equals `[<currentStorageVersion>]` — most of the time, most CRDs. It only does meaningful work when a CRD's stored-versions list is dirty (typically right after a graduation release).
8585

86+
### Requires cluster-scoped RBAC
87+
88+
The migrator only works when the operator runs cluster-scoped (`operator.rbac.scope=cluster`, the default). It watches cluster-scoped `CustomResourceDefinition` objects and re-stores custom resources across **all** namespaces — neither is possible for a namespace-scoped operator, which gets only per-namespace `RoleBindings` and a namespace-restricted manager cache. To prevent a silently wedged operator, the chart **fails the render** if `storageVersionMigrator: true` is combined with `operator.rbac.scope=namespace`:
89+
90+
```
91+
operator.features.storageVersionMigrator requires operator.rbac.scope=cluster: the
92+
StorageVersionMigrator controller watches cluster-scoped CustomResourceDefinitions and
93+
re-stores resources across all namespaces, which a namespace-scoped operator cannot do.
94+
Set operator.features.storageVersionMigrator=false for namespace-scoped installs.
95+
```
96+
97+
Namespace-scoped installs must set `storageVersionMigrator: false` and handle storage-version cleanup by other means — e.g. running the standalone [`kube-storage-version-migrator`](https://github.com/kubernetes-sigs/kube-storage-version-migrator), which is a cluster-scoped component with its own identity, before any version-removal release.
98+
8699
## Per-CRD emergency escape hatch
87100

88101
Removing the label on a live cluster excludes that single CRD from migration immediately:

test/e2e/chainsaw/operator/multi-tenancy/setup/chainsaw-test.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,10 @@ spec:
4242
- operator.rbac.scope=namespace
4343
- --set
4444
- operator.rbac.allowedNamespaces={toolhive-system,test-namespace,toolhive-test-ns-1,toolhive-test-ns-2}
45+
# The StorageVersionMigrator watches cluster-scoped CRDs, so it is
46+
# unsupported (and chart-rejected) in namespace scope. Opt out here.
47+
- --set
48+
- operator.features.storageVersionMigrator=false
4549
- assert:
4650
file: assert-operator-ready.yaml
4751
- assert:

0 commit comments

Comments
 (0)