This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch main
in repository
https://gitbox.apache.org/repos/asf/superset-kubernetes-operator.git
The following commit(s) were added to refs/heads/main by this push:
new 35327d1 feat(lifecycle)!: run migrate on any image change, drop
downgrade block (#173)
35327d1 is described below
commit 35327d1ab6ca38fbe0d7d2d5bd1afb236d581253
Author: Ville Brofeldt <[email protected]>
AuthorDate: Mon Jul 6 14:18:01 2026 -0700
feat(lifecycle)!: run migrate on any image change, drop downgrade block
(#173)
* feat(lifecycle)!: run migrate on any image change, drop downgrade block
* remove stale assertion
---
AGENTS.md | 6 +-
Makefile | 1 -
api/v1alpha1/superset_types.go | 3 -
.../crds/superset.apache.org_supersets.yaml | 6 --
.../crd/bases/superset.apache.org_supersets.yaml | 6 --
docs/architecture/internals.md | 2 +-
docs/contributing/development-guidelines.md | 14 +++-
docs/reference/api-reference.md | 1 -
docs/reference/releases.md | 9 +++
docs/user-guide/lifecycle.md | 29 +++++---
go.mod | 2 +-
internal/controller/lifecycle.go | 70 +++++++------------
internal/controller/lifecycle_pipeline_test.go | 42 ++----------
internal/controller/lifecycle_settle_test.go | 7 +-
internal/controller/version.go | 40 -----------
internal/controller/version_fuzz_test.go | 78 ----------------------
internal/controller/version_test.go | 37 ----------
test/e2e/superset_journey_test.go | 1 -
18 files changed, 73 insertions(+), 281 deletions(-)
diff --git a/AGENTS.md b/AGENTS.md
index 8bd07c7..d3a26a0 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -79,7 +79,7 @@ The operator uses a **single public CRD architecture** where
the parent `Superse
- `component_descriptors.go` — table-driven component descriptors for parent
resource reconciliation
- `deployment_builder.go` — builds Deployment from FlatComponentSpec +
DeploymentConfig
- `initpod.go` — Task Job PodSpec helpers
- - `version.go` — Version comparison logic (upgrade/downgrade detection)
+ - `version.go` — `ImageRef` helper (canonical repository:tag string)
- `helpers.go` — componentLabels(), mergeLabels(), mergeAnnotations()
- `status.go` — condition helpers
- `scaling.go` — HPA (with custom metrics) + PDB reconciliation
@@ -115,11 +115,11 @@ The operator uses a **single public CRD architecture**
where the parent `Superse
- **HPA**: When `autoscaling` is set, Deployment replicas is nil (HPA
manages). Supports custom metrics via `autoscalingv2.MetricSpec`. Top-level
`autoscaling`/`podDisruptionBudget` provide defaults inherited by all scalable
components; per-component values override (not merge). CeleryBeat and lifecycle
tasks are excluded (singleton/one-off Jobs).
- **Beat singleton**: CeleryBeat always forces replicas=1 regardless of spec.
- **Gateway API**: Uses `sigs.k8s.io/gateway-api` types. Graceful handling of
missing CRDs via `meta.IsNoMatchError`.
-- **Lifecycle tasks**: `spec.lifecycle` on the parent CRD (type
`LifecycleSpec`) defines up to four sequential tasks: "seed" (database snapshot
from external source), "migrate" (`superset db upgrade`), "rotate" (`superset
re-encrypt-secrets` for secret key rotation), and "init" (`superset init`). The
parent controller sequences them (migrate waits for seed, rotate waits for
migrate, init waits for rotate), gates component deployment, and triggers
re-runs by deleting and recreating determ [...]
+- **Lifecycle tasks**: `spec.lifecycle` on the parent CRD (type
`LifecycleSpec`) defines up to four sequential tasks: "seed" (database snapshot
from external source), "migrate" (`superset db upgrade`), "rotate" (`superset
re-encrypt-secrets` for secret key rotation), and "init" (`superset init`). The
parent controller sequences them (migrate waits for seed, rotate waits for
migrate, init waits for rotate), gates component deployment, and triggers
re-runs by deleting and recreating determ [...]
- **CRD validation**: All validation uses CEL (`x-kubernetes-validations`) on
CRD types — no admission webhooks. Rules cover: environment mode restrictions,
secret mutual exclusivity, metastore/valkey validation, networking constraints,
monitoring constraints. Defaults (repository, pullPolicy, environment) use
kubebuilder default markers.
- **Metrics**: Operator exposes controller-runtime default metrics (reconcile
counts, durations, leader election) on HTTPS :8443 with Kubernetes auth/authz.
No custom metrics — controller-runtime defaults are sufficient. Superset
instance monitoring via optional `spec.monitoring.serviceMonitor` (creates a
Prometheus ServiceMonitor targeting the web-server component using unstructured
objects; gracefully skips if CRD is absent).
- **Config mount path**: `/app/pythonpath` for superset_config.py.
-- **Fuzzing**: Go-native `go test -fuzz` targets in `*_fuzz_test.go` on pure
string/codegen/merge functions (`CompareVersions`, `pyQuote`/`RenderConfig`,
`MergeMaps`). `make fuzz` runs them bounded; seeds replay under `make
test-unit`; scheduled `fuzz.yaml` runs them longer. Robustness, not a security
boundary (CR input is trusted). See
`docs/contributing/development-guidelines.md`.
+- **Fuzzing**: Go-native `go test -fuzz` targets in `*_fuzz_test.go` on pure
string/codegen/merge functions (`pyQuote`/`RenderConfig`, `MergeMaps`). `make
fuzz` runs them bounded; seeds replay under `make test-unit`; scheduled
`fuzz.yaml` runs them longer. Robustness, not a security boundary (CR input is
trusted). See `docs/contributing/development-guidelines.md`.
- **All Go files must have the Apache 2.0 copyright header** (see
`hack/boilerplate.go.txt`)
## Naming Conventions
diff --git a/Makefile b/Makefile
index 8eb4bfa..e7089d9 100644
--- a/Makefile
+++ b/Makefile
@@ -221,7 +221,6 @@ test-integration: manifests generate fmt vet setup-envtest
## Run integration te
fuzz: ## Run all fuzz targets for a bounded duration (FUZZTIME per target,
default 30s).
@FUZZTIME=$${FUZZTIME:-30s}; \
set -e; \
- go test ./internal/controller/ -run '^$$' -fuzz
'^FuzzCompareVersions$$' -fuzztime $$FUZZTIME; \
go test ./internal/config/ -run '^$$' -fuzz '^FuzzPyQuote$$'
-fuzztime $$FUZZTIME; \
go test ./internal/config/ -run '^$$' -fuzz '^FuzzRenderConfig$$'
-fuzztime $$FUZZTIME; \
go test ./internal/resolution/ -run '^$$' -fuzz '^FuzzMergeMaps$$'
-fuzztime $$FUZZTIME
diff --git a/api/v1alpha1/superset_types.go b/api/v1alpha1/superset_types.go
index 525bec4..af71f83 100644
--- a/api/v1alpha1/superset_types.go
+++ b/api/v1alpha1/superset_types.go
@@ -867,9 +867,6 @@ type UpgradeContext struct {
FromVersion string `json:"fromVersion,omitempty"`
// +optional
ToVersion string `json:"toVersion,omitempty"`
- // +optional
- // +kubebuilder:validation:Enum=Upgrade;Downgrade;Unknown
- Direction string `json:"direction,omitempty"`
// ApprovalToken is the annotation value required to approve this exact
upgrade transition.
// +optional
ApprovalToken string `json:"approvalToken,omitempty"`
diff --git a/charts/superset-operator/crds/superset.apache.org_supersets.yaml
b/charts/superset-operator/crds/superset.apache.org_supersets.yaml
index fa95763..78f67fc 100644
--- a/charts/superset-operator/crds/superset.apache.org_supersets.yaml
+++ b/charts/superset-operator/crds/superset.apache.org_supersets.yaml
@@ -40045,12 +40045,6 @@ spec:
properties:
approvalToken:
type: string
- direction:
- enum:
- - Upgrade
- - Downgrade
- - Unknown
- type: string
fromVersion:
type: string
startedAt:
diff --git a/config/crd/bases/superset.apache.org_supersets.yaml
b/config/crd/bases/superset.apache.org_supersets.yaml
index fa95763..78f67fc 100644
--- a/config/crd/bases/superset.apache.org_supersets.yaml
+++ b/config/crd/bases/superset.apache.org_supersets.yaml
@@ -40045,12 +40045,6 @@ spec:
properties:
approvalToken:
type: string
- direction:
- enum:
- - Upgrade
- - Downgrade
- - Unknown
- type: string
fromVersion:
type: string
startedAt:
diff --git a/docs/architecture/internals.md b/docs/architecture/internals.md
index 1a8ff53..7ca6715 100644
--- a/docs/architecture/internals.md
+++ b/docs/architecture/internals.md
@@ -446,7 +446,7 @@ The top-level `status.phase` reflects the overall instance
state:
| `Running` | All enabled components are ready and lifecycle is complete |
| `Degraded` | One or more components are not fully ready |
| `Suspended` | `spec.suspend: true` — all reconciliation paused |
-| `Blocked` | Downgrade detected — lifecycle tasks will not run (manual
intervention required) |
+| `Blocked` | Configuration error (e.g. an invalid `seed.cronSchedule`) —
lifecycle tasks will not run until corrected |
| `AwaitingApproval` | Supervised upgrade mode — waiting for the target-bound
approval annotation before proceeding |
Drain progress is reported on `status.lifecycle.phase=Draining`; it does not
diff --git a/docs/contributing/development-guidelines.md
b/docs/contributing/development-guidelines.md
index 75a0cb0..1dc2730 100644
--- a/docs/contributing/development-guidelines.md
+++ b/docs/contributing/development-guidelines.md
@@ -62,6 +62,14 @@ Across all tiers:
helper or restructuring a package breaks tests, those tests were coupled
to implementation, not behavior, and should be rewritten to assert on
observable outputs or removed entirely.
+- **Don't test non-actions.** A test should pin a behavior the code actively
+ produces, not assert the absence of behavior the code never had (or no longer
+ has). When you delete a feature, delete its tests — don't invert them into
+ "X no longer happens" assertions. The positive tests for the surviving
+ behavior already cover it: e.g. after removing downgrade blocking, a
+ "downgrade proceeds" test is redundant because the general "an image change
+ proceeds and re-runs migrate" test already exercises that path. (Mirrors the
+ documentation rule: don't describe what the default already precludes.)
### Security and the threat model
@@ -224,8 +232,8 @@ make fuzz FUZZTIME=2m # longer local run
```
Each target seeds a corpus with `f.Add(...)` cases and asserts an invariant
-beyond "does not panic" — e.g. `CompareVersions` is antisymmetric,
`RenderConfig`
-is deterministic, `pyQuote` round-trips, `MergeMaps` is a last-writer-wins
union.
+beyond "does not panic" — e.g. `RenderConfig` is deterministic, `pyQuote`
+round-trips, `MergeMaps` is a last-writer-wins union.
The seed corpus (plus any committed `testdata/fuzz/...` reproducers) replays as
ordinary subtests during `make test-unit`, so regressions are caught on every
PR;
the scheduled `fuzz.yaml` workflow runs the targets for longer to explore new
@@ -445,7 +453,7 @@ logr/zap). Verbosity is a single knob, `--zap-log-level`,
configured in
- **Info / V(0)** — `log.Info(msg, kv...)`. Low-frequency, meaningful state
*transitions* an operator wants at default verbosity (lifecycle task
- created/completed/failed, downgrade blocked, awaiting approval, maintenance
+ created/completed/failed, awaiting approval, maintenance
cleared, a component actually changed). **Must not fire on every reconcile.**
- **V(1) / debug** — `log.V(1).Info(msg, kv...)`. Per-reconcile progress and
decisions: reconcile entry, phase markers, checksum-skip decisions,
diff --git a/docs/reference/api-reference.md b/docs/reference/api-reference.md
index dd837fa..ed4379a 100644
--- a/docs/reference/api-reference.md
+++ b/docs/reference/api-reference.md
@@ -1159,7 +1159,6 @@ _Appears in:_
| --- | --- | --- | --- |
| `fromVersion` _string_ | | | Optional: \{\} <br /> |
| `toVersion` _string_ | | | Optional: \{\} <br /> |
-| `direction` _string_ | | | Enum: [Upgrade Downgrade Unknown] <br
/>Optional: \{\} <br /> |
| `approvalToken` _string_ | ApprovalToken is the annotation value required to
approve this exact upgrade transition. | | Optional: \{\} <br /> |
| `startedAt`
_[Time](https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Time)_ | | |
Optional: \{\} <br /> |
diff --git a/docs/reference/releases.md b/docs/reference/releases.md
index 7e4ab14..894bfe3 100644
--- a/docs/reference/releases.md
+++ b/docs/reference/releases.md
@@ -26,6 +26,15 @@ releases.
### Changed
+- **Breaking:** downgrade blocking is removed. Any change to the lifecycle
image
+ tag now re-runs the migrate task (`superset db upgrade`) regardless of
+ direction — the operator no longer performs semver comparison or sets
+ `status.phase: Blocked` on a version decrease, matching the official Superset
+ Helm chart. The migrate task only runs `superset db upgrade` (Superset's down
+ migrations are poorly tested and often break, so the operator never runs
+ them), so take a database backup before an upgrade if you may need to revert.
+ The `direction` field is removed from `status.lifecycle.upgrade`, and the
+ `VersionComparisonSkipped` warning event no longer fires.
- **Breaking:** the lifecycle `clone` task is renamed to `seed`. Rename
`spec.lifecycle.clone` to `spec.lifecycle.seed` (and its `postCloneSQL` field
to `postSeedSQL`) in your Superset resources. The task Job name suffix
changes
diff --git a/docs/user-guide/lifecycle.md b/docs/user-guide/lifecycle.md
index 76527ae..6c17187 100644
--- a/docs/user-guide/lifecycle.md
+++ b/docs/user-guide/lifecycle.md
@@ -174,16 +174,23 @@ You can monitor the upgrade status with:
kubectl get superset my-superset -o jsonpath='{.status.lifecycle}'
```
-The operator also performs semver comparison on image tags and blocks
downgrades
-to prevent accidental database corruption. A blocked downgrade sets
-`status.phase: Blocked` — revert the image tag to resolve.
-
-Downgrade protection requires semantically comparable (semver) image tags.
-Non-semver tags — such as `latest`, date stamps, or digest pins — cannot be
-ordered, so the operator cannot detect a downgrade and lifecycle tasks proceed
-normally. In that case it emits a `VersionComparisonSkipped` warning event
-rather than blocking. Use semver tags in Staging and Production if you rely on
-downgrade protection.
+### Version changes and downgrades
+
+Any change to the lifecycle image tag re-runs the migrate task
+(`superset db upgrade`), regardless of whether the new version is higher or
+lower than the previous one. The operator does not compare versions or block
+downgrades. This matches the behavior of the official Superset Helm chart,
which
+runs `superset db upgrade` on every upgrade.
+
+The migrate task only ever runs `superset db upgrade`. Superset does provide
+`superset db downgrade`, but its down migrations are poorly tested and often
+break, so the operator never runs them — pinning back to an older image re-runs
+the forward migration rather than reversing the schema. You are responsible for
+ensuring the database is compatible with the target version (for example, by
+restoring a backup taken before the upgrade). Allowing the change means a
+deployment that needs to revert after a failed upgrade is never stranded in a
+blocked state. Take a database backup before every migration so a known-good
+dump exists if you need to revert.
## Drain Behavior
@@ -767,7 +774,7 @@ Task Job names are deterministic: `{parentName}-{taskType}`
(e.g. `my-superset-m
|---|---|
| `Initializing` | First deployment — lifecycle tasks running for the first
time |
| `Upgrading` | Image change detected — lifecycle tasks running against new
version |
-| `Blocked` | Downgrade detected — lifecycle tasks will not run (manual
intervention required) |
+| `Blocked` | Configuration error (e.g. an invalid `seed.cronSchedule`) —
lifecycle tasks will not run until corrected |
| `AwaitingApproval` | Supervised upgrade mode — waiting for the target-bound
approval annotation before proceeding |
Drain progress appears in the `Lifecycle` column and
diff --git a/go.mod b/go.mod
index b715b00..8367e5f 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,6 @@ go 1.26.0
toolchain go1.26.4
require (
- github.com/Masterminds/semver/v3 v3.5.0
github.com/onsi/ginkgo/v2 v2.32.0
github.com/onsi/gomega v1.42.1
github.com/robfig/cron/v3 v3.0.1
@@ -21,6 +20,7 @@ require (
require (
cel.dev/expr v0.25.1 // indirect
+ github.com/Masterminds/semver/v3 v3.5.0 // indirect
github.com/antlr4-go/antlr/v4 v4.13.0 // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/blang/semver/v4 v4.0.0 // indirect
diff --git a/internal/controller/lifecycle.go b/internal/controller/lifecycle.go
index 1cbcfd6..64a5bc3 100644
--- a/internal/controller/lifecycle.go
+++ b/internal/controller/lifecycle.go
@@ -183,9 +183,10 @@ func (r *SupersetReconciler) reconcileLifecycle(
upgradeInProgress := lastImage != "" && currentImage != lastImage
parentLifecyclePhase := lifecycleParentPhase(upgradeInProgress)
- // Check upgrade gates (version comparison, downgrade blocking,
supervised approval).
- if gateResult, gated := r.checkUpgradeGates(ctx, superset,
imageChanged, lastImage, currentImage); gated {
- return gateResult, nil
+ // Enforce supervised upgrade approval. A gated change waits for the
approval
+ // annotation (which re-triggers reconcile), so no timed requeue is
needed.
+ if r.checkUpgradeGates(ctx, superset, imageChanged, lastImage,
currentImage) {
+ return lifecycleResult{}, nil
}
// Determine which tasks are enabled and prune orphans for disabled
ones.
@@ -411,64 +412,41 @@ func (r *SupersetReconciler) runLifecyclePipeline(
return lifecycleComplete(), nil
}
-// checkUpgradeGates handles version comparison, downgrade blocking, and
supervised approval.
-// Returns (result, gated) — if gated is true, the caller should return early
with result.
+// checkUpgradeGates records the in-flight image change and enforces supervised
+// approval. Version direction is not inspected — any image change proceeds and
+// re-runs migrate. Returns true if the change is gated (awaiting approval) and
+// the caller should stop reconciling until the approval annotation is set.
func (r *SupersetReconciler) checkUpgradeGates(
ctx context.Context,
superset *supersetv1alpha1.Superset,
imageChanged bool,
lastImage, currentImage string,
-) (lifecycleResult, bool) {
+) bool {
log := logf.FromContext(ctx)
if !imageChanged || lastImage == "" {
- return lifecycleResult{}, false
+ return false
}
oldTag := tagFromImageRef(lastImage)
newTag := tagFromImageRef(currentImage)
- direction := CompareVersions(oldTag, newTag)
approvalToken := upgradeApprovalToken(lastImage, currentImage)
- log.V(2).Info("Compared image versions", "from", oldTag, "to", newTag,
"direction", direction)
-
- if direction == DirectionDowngrade {
- log.Info("Downgrade detected, blocking lifecycle", "from",
oldTag, "to", newTag)
- setCondition(&superset.Status.Conditions,
supersetv1alpha1.ConditionTypeLifecycleComplete,
- metav1.ConditionFalse, "DowngradeBlocked",
- fmt.Sprintf("Downgrade from %s to %s is not supported.
Alembic migrations are forward-only.", oldTag, newTag),
- superset.Generation)
- superset.Status.Phase = phaseBlocked
- superset.Status.Lifecycle.Phase = lifecyclePhaseBlocked
- superset.Status.Lifecycle.Upgrade =
&supersetv1alpha1.UpgradeContext{
- FromVersion: oldTag,
- ToVersion: newTag,
- Direction: string(DirectionDowngrade),
- ApprovalToken: approvalToken,
- }
- r.Recorder.Eventf(superset, nil, corev1.EventTypeWarning,
"DowngradeBlocked", "Lifecycle",
- "Downgrade from %s to %s is not supported", oldTag,
newTag)
- return lifecycleTerminal(), true
- }
-
- contextMatches :=
upgradeContextMatches(superset.Status.Lifecycle.Upgrade, oldTag, newTag,
direction, approvalToken)
-
- // Non-semver tags (e.g. "latest", date stamps, digest pins) cannot be
- // ordered, so downgrade protection does not apply — a downgrade
expressed
- // with such tags would run forward-only migrations against an older
image
- // undetected. Surface this once per distinct image transition so
operators
- // can intervene; the !contextMatches guard prevents per-reconcile spam.
- if direction == DirectionUnknown && !contextMatches {
- log.Info("Image version change could not be compared
(non-semver tags); downgrade protection skipped",
- "from", oldTag, "to", newTag)
- r.Recorder.Eventf(superset, nil, corev1.EventTypeWarning,
"VersionComparisonSkipped", "Lifecycle",
- "Cannot compare image tags %q and %q (non-semver);
downgrade protection does not apply", oldTag, newTag)
- }
+ log.V(2).Info("Detected lifecycle image change", "from", oldTag, "to",
newTag)
+
+ // Any version change re-runs migrate (superset db upgrade); direction
is
+ // not inspected. Superset supports `superset db downgrade`, but down
+ // migrations are poorly tested and frequently break, so the operator
does
+ // not attempt to run them — pinning back to an older image just
re-runs the
+ // forward migration. Blocking the change instead strands deployments
that
+ // need to pin back after a failed upgrade. Matching the official
Superset
+ // Helm chart, the operator runs the migration on every change and
relies on
+ // the (optional) pre-upgrade backup as the safety net.
+ contextMatches :=
upgradeContextMatches(superset.Status.Lifecycle.Upgrade, oldTag, newTag,
approvalToken)
if !contextMatches {
superset.Status.Lifecycle.Upgrade =
&supersetv1alpha1.UpgradeContext{
FromVersion: oldTag,
ToVersion: newTag,
- Direction: string(direction),
ApprovalToken: approvalToken,
StartedAt: nowPtr(),
}
@@ -488,24 +466,22 @@ func (r *SupersetReconciler) checkUpgradeGates(
superset.Generation)
superset.Status.Phase = phaseAwaitingApproval
superset.Status.Lifecycle.Phase =
lifecyclePhaseAwaitingApproval
- return lifecycleResult{}, true
+ return true
}
}
- return lifecycleResult{}, false
+ return false
}
func upgradeContextMatches(
upgrade *supersetv1alpha1.UpgradeContext,
fromVersion string,
toVersion string,
- direction VersionDirection,
approvalToken string,
) bool {
return upgrade != nil &&
upgrade.FromVersion == fromVersion &&
upgrade.ToVersion == toVersion &&
- upgrade.Direction == string(direction) &&
upgrade.ApprovalToken == approvalToken
}
diff --git a/internal/controller/lifecycle_pipeline_test.go
b/internal/controller/lifecycle_pipeline_test.go
index e312c48..9cececa 100644
--- a/internal/controller/lifecycle_pipeline_test.go
+++ b/internal/controller/lifecycle_pipeline_test.go
@@ -299,28 +299,16 @@ func TestCheckUpgradeGates(t *testing.T) {
t.Run("no image change is not gated", func(t *testing.T) {
s := &supersetv1alpha1.Superset{Status:
supersetv1alpha1.SupersetStatus{Lifecycle: &supersetv1alpha1.LifecycleStatus{}}}
- _, gated := r.checkUpgradeGates(ctx, s, false,
"apache/superset:1", "apache/superset:1")
+ gated := r.checkUpgradeGates(ctx, s, false,
"apache/superset:1", "apache/superset:1")
assert.False(t, gated)
})
t.Run("first install (empty lastImage) is not gated", func(t
*testing.T) {
s := &supersetv1alpha1.Superset{Status:
supersetv1alpha1.SupersetStatus{Lifecycle: &supersetv1alpha1.LifecycleStatus{}}}
- _, gated := r.checkUpgradeGates(ctx, s, true, "",
"apache/superset:1")
+ gated := r.checkUpgradeGates(ctx, s, true, "",
"apache/superset:1")
assert.False(t, gated)
})
- t.Run("downgrade blocks with terminal result", func(t *testing.T) {
- s := &supersetv1alpha1.Superset{
- ObjectMeta: metav1.ObjectMeta{Name: "test"},
- Status: supersetv1alpha1.SupersetStatus{Lifecycle:
&supersetv1alpha1.LifecycleStatus{}},
- }
- res, gated := r.checkUpgradeGates(ctx, s, true,
"apache/superset:3.0.0", "apache/superset:2.0.0")
- assert.True(t, gated)
- assert.True(t, res.TerminalFailure)
- assert.Equal(t, lifecyclePhaseBlocked, s.Status.Lifecycle.Phase)
- assert.True(t, hasConditionReason(s.Status.Conditions,
supersetv1alpha1.ConditionTypeLifecycleComplete, "DowngradeBlocked"))
- })
-
t.Run("supervised upgrade awaits approval", func(t *testing.T) {
mode := upgradeModeSupervised
s := &supersetv1alpha1.Superset{
@@ -330,9 +318,8 @@ func TestCheckUpgradeGates(t *testing.T) {
},
Status: supersetv1alpha1.SupersetStatus{Lifecycle:
&supersetv1alpha1.LifecycleStatus{}},
}
- res, gated := r.checkUpgradeGates(ctx, s, true,
"apache/superset:2.0.0", "apache/superset:3.0.0")
+ gated := r.checkUpgradeGates(ctx, s, true,
"apache/superset:2.0.0", "apache/superset:3.0.0")
assert.True(t, gated)
- assert.False(t, res.TerminalFailure)
assert.Equal(t, lifecyclePhaseAwaitingApproval,
s.Status.Lifecycle.Phase)
assert.Equal(t, phaseAwaitingApproval, s.Status.Phase)
})
@@ -342,32 +329,11 @@ func TestCheckUpgradeGates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Status: supersetv1alpha1.SupersetStatus{Lifecycle:
&supersetv1alpha1.LifecycleStatus{}},
}
- _, gated := r.checkUpgradeGates(ctx, s, true,
"apache/superset:2.0.0", "apache/superset:3.0.0")
+ gated := r.checkUpgradeGates(ctx, s, true,
"apache/superset:2.0.0", "apache/superset:3.0.0")
assert.False(t, gated)
// Upgrade context recorded for the in-flight upgrade.
require.NotNil(t, s.Status.Lifecycle.Upgrade)
assert.Equal(t, "2.0.0", s.Status.Lifecycle.Upgrade.FromVersion)
assert.Equal(t, "3.0.0", s.Status.Lifecycle.Upgrade.ToVersion)
})
-
- t.Run("non-semver tags proceed but emit a warning", func(t *testing.T) {
- rec := events.NewFakeRecorder(10)
- nr := &SupersetReconciler{Recorder: rec}
- s := &supersetv1alpha1.Superset{
- ObjectMeta: metav1.ObjectMeta{Name: "test"},
- Status: supersetv1alpha1.SupersetStatus{Lifecycle:
&supersetv1alpha1.LifecycleStatus{}},
- }
- // "latest" → "main" cannot be ordered: not a downgrade, so not
gated.
- _, gated := nr.checkUpgradeGates(ctx, s, true,
"apache/superset:latest", "apache/superset:main")
- assert.False(t, gated)
- assert.NotEqual(t, lifecyclePhaseBlocked,
s.Status.Lifecycle.Phase)
- require.NotNil(t, s.Status.Lifecycle.Upgrade)
- assert.Equal(t, string(DirectionUnknown),
s.Status.Lifecycle.Upgrade.Direction)
- select {
- case ev := <-rec.Events:
- assert.Contains(t, ev, "VersionComparisonSkipped")
- default:
- t.Fatal("expected a VersionComparisonSkipped warning
event")
- }
- })
}
diff --git a/internal/controller/lifecycle_settle_test.go
b/internal/controller/lifecycle_settle_test.go
index a0c27db..90be6e0 100644
--- a/internal/controller/lifecycle_settle_test.go
+++ b/internal/controller/lifecycle_settle_test.go
@@ -118,7 +118,7 @@ func
TestCheckUpgradeGates_SupervisedApprovalRequiresRecordedToken(t *testing.T)
}
r := &SupersetReconciler{}
- _, gated := r.checkUpgradeGates(context.Background(), superset, true,
lastImage, currentImage)
+ gated := r.checkUpgradeGates(context.Background(), superset, true,
lastImage, currentImage)
if !gated {
t.Fatal("expected first reconcile to publish the approval token
before accepting it")
}
@@ -129,7 +129,7 @@ func
TestCheckUpgradeGates_SupervisedApprovalRequiresRecordedToken(t *testing.T)
t.Fatalf("expected approval token %q, got %q", token, got)
}
- _, gated = r.checkUpgradeGates(context.Background(), superset, true,
lastImage, currentImage)
+ gated = r.checkUpgradeGates(context.Background(), superset, true,
lastImage, currentImage)
if gated {
t.Fatal("expected matching recorded approval token to allow the
upgrade")
}
@@ -156,7 +156,6 @@ func
TestCheckUpgradeGates_StaleApprovalDoesNotApproveChangedTarget(t *testing.T
Upgrade: &supersetv1alpha1.UpgradeContext{
FromVersion: "1.0.0",
ToVersion: "1.1.0",
- Direction: string(DirectionUpgrade),
ApprovalToken: oldToken,
},
},
@@ -164,7 +163,7 @@ func
TestCheckUpgradeGates_StaleApprovalDoesNotApproveChangedTarget(t *testing.T
}
r := &SupersetReconciler{}
- _, gated := r.checkUpgradeGates(context.Background(), superset, true,
lastImage, newTarget)
+ gated := r.checkUpgradeGates(context.Background(), superset, true,
lastImage, newTarget)
if !gated {
t.Fatal("expected stale approval token to keep the changed
target gated")
}
diff --git a/internal/controller/version.go b/internal/controller/version.go
index 578506f..f78d4f2 100644
--- a/internal/controller/version.go
+++ b/internal/controller/version.go
@@ -19,46 +19,6 @@ under the License.
package controller
-import (
- "strings"
-
- "github.com/Masterminds/semver/v3"
-)
-
-// VersionDirection represents the relationship between two image versions.
-type VersionDirection string
-
-const (
- DirectionUpgrade VersionDirection = "Upgrade"
- DirectionDowngrade VersionDirection = "Downgrade"
- DirectionRebuild VersionDirection = "Rebuild"
- DirectionUnknown VersionDirection = "Unknown"
-)
-
-// CompareVersions determines the direction between two image tags.
-// Returns Upgrade if newTag > oldTag, Downgrade if newTag < oldTag,
-// Rebuild if equal, or Unknown if either tag is not valid semver.
-func CompareVersions(oldTag, newTag string) VersionDirection {
- if oldTag == newTag {
- return DirectionRebuild
- }
-
- oldVer, oldErr := semver.NewVersion(strings.TrimPrefix(oldTag, "v"))
- newVer, newErr := semver.NewVersion(strings.TrimPrefix(newTag, "v"))
-
- if oldErr != nil || newErr != nil {
- return DirectionUnknown
- }
-
- if newVer.GreaterThan(oldVer) {
- return DirectionUpgrade
- }
- if newVer.LessThan(oldVer) {
- return DirectionDowngrade
- }
- return DirectionRebuild
-}
-
// ImageRef returns the canonical "repository:tag" string for an image.
func ImageRef(repository, tag string) string {
return repository + ":" + tag
diff --git a/internal/controller/version_fuzz_test.go
b/internal/controller/version_fuzz_test.go
deleted file mode 100644
index e01ab5a..0000000
--- a/internal/controller/version_fuzz_test.go
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
-Licensed to the Apache Software Foundation (ASF) under one
-or more contributor license agreements. See the NOTICE file
-distributed with this work for additional information
-regarding copyright ownership. The ASF licenses this file
-to you under the Apache License, Version 2.0 (the
-"License"); you may not use this file except in compliance
-with the License. You may obtain a copy of the License at
-
- http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing,
-software distributed under the License is distributed on an
-"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-KIND, either express or implied. See the License for the
-specific language governing permissions and limitations
-under the License.
-*/
-
-package controller
-
-import "testing"
-
-// FuzzCompareVersions exercises CompareVersions with arbitrary image tags. The
-// tags come from the (trusted) Superset CR spec, so this is a robustness
check:
-// the semver parsing must never panic and the result must obey the ordering
-// contract. See docs/contributing/development-guidelines.md (Fuzzing).
-func FuzzCompareVersions(f *testing.F) {
- seeds := [][2]string{
- {"4.0.0", "4.0.1"},
- {"4.1.0", "4.0.0"},
- {"4.0.0", "4.0.0"},
- {"v4.0.0", "4.0.0"},
- {"4.0.0-rc1", "4.0.0"},
- {"latest", "4.0.0"},
- {"sha-abc123", "sha-def456"},
- {"", "x"},
- {"", ""},
- }
- for _, s := range seeds {
- f.Add(s[0], s[1])
- }
-
- isValid := func(d VersionDirection) bool {
- switch d {
- case DirectionUpgrade, DirectionDowngrade, DirectionRebuild,
DirectionUnknown:
- return true
- default:
- return false
- }
- }
-
- f.Fuzz(func(t *testing.T, oldTag, newTag string) {
- got := CompareVersions(oldTag, newTag)
- if !isValid(got) {
- t.Fatalf("CompareVersions(%q, %q) = %q, not a defined
VersionDirection", oldTag, newTag, got)
- }
-
- // Comparing a tag to itself is always a rebuild.
- if self := CompareVersions(oldTag, oldTag); self !=
DirectionRebuild {
- t.Fatalf("CompareVersions(%q, %q) = %q, want Rebuild",
oldTag, oldTag, self)
- }
-
- // Swapping the arguments must invert the direction: an upgrade
one way is
- // a downgrade the other, while Rebuild and Unknown are
symmetric.
- reverse := CompareVersions(newTag, oldTag)
- want := map[VersionDirection]VersionDirection{
- DirectionUpgrade: DirectionDowngrade,
- DirectionDowngrade: DirectionUpgrade,
- DirectionRebuild: DirectionRebuild,
- DirectionUnknown: DirectionUnknown,
- }[got]
- if reverse != want {
- t.Fatalf("CompareVersions(%q, %q) = %q but
CompareVersions(%q, %q) = %q, want %q",
- oldTag, newTag, got, newTag, oldTag, reverse,
want)
- }
- })
-}
diff --git a/internal/controller/version_test.go
b/internal/controller/version_test.go
index a75b232..bada47f 100644
--- a/internal/controller/version_test.go
+++ b/internal/controller/version_test.go
@@ -25,43 +25,6 @@ import (
"github.com/stretchr/testify/assert"
)
-func TestCompareVersions(t *testing.T) {
- tests := []struct {
- name string
- oldTag string
- newTag string
- expected VersionDirection
- }{
- {name: "upgrade patch", oldTag: "4.0.0", newTag: "4.0.1",
expected: DirectionUpgrade},
- {name: "upgrade minor", oldTag: "4.0.0", newTag: "4.1.0",
expected: DirectionUpgrade},
- {name: "upgrade major", oldTag: "4.0.0", newTag: "5.0.0",
expected: DirectionUpgrade},
- {name: "downgrade patch", oldTag: "4.0.1", newTag: "4.0.0",
expected: DirectionDowngrade},
- {name: "downgrade minor", oldTag: "4.1.0", newTag: "4.0.0",
expected: DirectionDowngrade},
- {name: "downgrade major", oldTag: "5.0.0", newTag: "4.0.0",
expected: DirectionDowngrade},
- {name: "same version", oldTag: "4.0.0", newTag: "4.0.0",
expected: DirectionRebuild},
- {name: "v-prefix upgrade", oldTag: "v4.0.0", newTag: "v4.1.0",
expected: DirectionUpgrade},
- {name: "v-prefix downgrade", oldTag: "v4.1.0", newTag:
"v4.0.0", expected: DirectionDowngrade},
- {name: "v-prefix same", oldTag: "v4.0.0", newTag: "v4.0.0",
expected: DirectionRebuild},
- {name: "mixed v-prefix upgrade", oldTag: "4.0.0", newTag:
"v4.1.0", expected: DirectionUpgrade},
- {name: "pre-release to release", oldTag: "4.0.0-rc1", newTag:
"4.0.0", expected: DirectionUpgrade},
- {name: "release to pre-release", oldTag: "4.0.0", newTag:
"4.1.0-rc1", expected: DirectionUpgrade},
- {name: "pre-release ordering", oldTag: "4.0.0-rc1", newTag:
"4.0.0-rc2", expected: DirectionUpgrade},
- {name: "non-semver old", oldTag: "latest", newTag: "4.0.0",
expected: DirectionUnknown},
- {name: "non-semver new", oldTag: "4.0.0", newTag: "latest",
expected: DirectionUnknown},
- {name: "both non-semver", oldTag: "latest", newTag:
"dev-abc123", expected: DirectionUnknown},
- {name: "sha tags", oldTag: "sha-abc123", newTag: "sha-def456",
expected: DirectionUnknown},
- {name: "branch tags", oldTag: "main", newTag: "feature-x",
expected: DirectionUnknown},
- {name: "different non-semver", oldTag: "latest", newTag:
"latest", expected: DirectionRebuild},
- }
-
- for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
- result := CompareVersions(tt.oldTag, tt.newTag)
- assert.Equal(t, tt.expected, result)
- })
- }
-}
-
func TestImageRef(t *testing.T) {
assert.Equal(t, "docker.io/superset:4.0.0",
ImageRef("docker.io/superset", "4.0.0"))
assert.Equal(t, "ghcr.io/apache/superset:latest",
ImageRef("ghcr.io/apache/superset", "latest"))
diff --git a/test/e2e/superset_journey_test.go
b/test/e2e/superset_journey_test.go
index 2eef8d7..045e142 100644
--- a/test/e2e/superset_journey_test.go
+++ b/test/e2e/superset_journey_test.go
@@ -270,7 +270,6 @@ spec:
expectJSONPath("superset", crName, "{.status.lifecycle.phase}",
"AwaitingApproval", time.Minute)
expectJSONPath("superset", crName,
"{.status.lifecycle.upgrade.fromVersion}", "6.0.0", time.Minute)
expectJSONPath("superset", crName,
"{.status.lifecycle.upgrade.toVersion}", "6.1.0", time.Minute)
- expectJSONPath("superset", crName,
"{.status.lifecycle.upgrade.direction}", "Upgrade", time.Minute)
By("approving the supervised upgrade with the recorded token")
token, err := jsonPath("superset", crName,
"{.status.lifecycle.upgrade.approvalToken}")
