Implementation proposal: honor admintools version in schema setup job

[Azerothian]

Source

Upstream issue: alexandrevilain#933 — "admin tools version not honored in schema setup job"

In the latest version v0.21.0, Schema setup job seems to be pulling the default admin tools image tag 1.24.2-tctl-1.18.1-cli-0.13.2. It might be regression with PR alexandrevilain#825 … Example Spec: spec.admintools.version: 1.24.2.

Why it fits this operator

The operator runs the schema-setup job to bootstrap and upgrade the Temporal databases. That job's admin-tools image must honor the user-configured spec.admintools.version (and image), otherwise schema setup runs with a mismatched admin-tools version — a correctness concern where the operator owns persistence initialization.

Proposed implementation

1. Fix the image tag source — in the schema-setup job builder (internal/resource/persistence/schema_setup_job_builder.go), resolve the admin-tools image/version from spec.admintools.{image,version}, falling back to defaults only when unset. Restores behavior that PR fix(admintools): add default image tag if left empty alexandrevilain/temporal-operator#825 regressed.
2. Reuse helper — centralize admin-tools image resolution into one shared helper used by both the job builder and the admin-tools deployment so they can't drift.
3. Tests — add/restore a unit test asserting the job's container image reflects spec.admintools.version; confirm e2e coverage.

Affected areas

• internal/resource/persistence/schema_setup_job_builder.go
• Admin-tools image/version resolution helper
• Schema-setup e2e tests

Risks / migration concerns

• Users relying on the buggy auto-default may see the job image change to match their admintools.version — that's the intended, documented contract being restored.
• Ensure a user-specified admintools.image (private registry) is honored by the job too, not just version.
• Minimal blast radius: image selection only; job semantics and TTL unchanged.

[Azerothian]

Implementation proposal (code-verified)

Verified against main @ 424555b. Confirmed exactly as described, down to the offending commit.

The regression

internal/resource/persistence/schema_setup_job_builder.go:125:

Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, version.DefaultAdminToolTag(b.instance.Spec.Version)),

spec.admintools.version is never read. git log -L125,125 points at 4f19690 "fix(admintools): add default image tag if left empty" (upstream PR alexandrevilain#825, 2024-10-02), which deleted the read:

-	tag := b.instance.Spec.AdminTools.Version
-	if tag == "" {
-		tag = version.PersistenceJobTag(b.instance.Spec.Version)
-	}
...
-							Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, tag),
+							Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, version.DefaultAdminToolTag(b.instance.Spec.Version)),

That commit added the spec.admintools.version defaulting to TemporalCluster.Default() (api/v1beta1/temporalcluster_defaults.go:216-218) precisely so the builders could stop computing the fallback themselves — the admintools deployment builder was updated to match (internal/resource/admintools/deployment_builder.go:137 reads b.instance.Spec.AdminTools.Version), but the schema-setup job builder was left calling the default helper directly. The fallback logic is now duplicated in two places and only one of them respects the user's field.

Reproduced

Built the job from a TemporalCluster with spec.version: 1.24.2 and spec.admintools.version: 1.24.2, after Default():

spec.admintools.image="temporalio/admin-tools" spec.admintools.version="1.24.2"
schema-setup job image = "temporalio/admin-tools:1.24.2-tctl-1.18.1-cli-1.0.0"
want                     "temporalio/admin-tools:1.24.2"

Same shape as the upstream report (the cli- suffix differs only because this fork carries a newer DefaultAdminToolTag table — pkg/version/admintools.go:14-17).

Note spec.admintools.image is already honoured by the job; only the tag is wrong. So a private-registry user gets myregistry/admin-tools:1.24.2-tctl-1.18.1-cli-1.0.0 — a tag that almost certainly does not exist in their registry, and the job fails to pull. That makes this more than cosmetic.

Refined implementation

1. Fix the builder (internal/resource/persistence/schema_setup_job_builder.go:125)

Since Default() already fills Spec.AdminTools.Version, the correct line is just:

Image: b.instance.Spec.AdminTools.ImageWithTag(b.instance.Spec.Version),

Drop the now-unused "github.com/alexandrevilain/temporal-operator/pkg/version" import from that file if nothing else there uses it.

2. Shared helper (item 2 of the proposal — keep it, it is what prevents the next drift)

Put it on the API type next to the spec it reads, api/v1beta1/temporalcluster_types.go:

// ImageWithTag returns the admin tools image reference, falling back to the operator's
// default tag for the cluster version when the user left spec.admintools.version empty.
// The fallback exists for callers that build resources from a CR that has not been
// through Default(), such as unit tests.
func (s *TemporalAdminToolsSpec) ImageWithTag(clusterVersion *version.Version) string {
	image := s.Image
	if image == "" {
		image = defaultTemporalAdmintoolsImage
	}
	tag := s.Version
	if tag == "" {
		tag = version.DefaultAdminToolTag(clusterVersion)
	}
	return fmt.Sprintf("%s:%s", image, tag)
}

Then use it at both call sites — schema_setup_job_builder.go:125 and admintools/deployment_builder.go:137.

Gotchas:

• Nil receiver. Spec.AdminTools is *TemporalAdminToolsSpec and +optional. Default() allocates it (temporalcluster_defaults.go:208-210), so the production path is safe — but both current call sites already dereference it unguarded, and the helper is the natural place to make that explicit. Either add a if s == nil branch returning the full default, or keep the deref and let it panic loudly; do not leave it ambiguous. I'd take the nil branch: the schema-setup job runs regardless of spec.admintools.enabled, so the job builder must work for a CR that never configured admintools at all.
• api/v1beta1 already imports pkg/version (temporalcluster_defaults.go), so no import cycle.
• defaultTemporalAdmintoolsImage lives in api/v1beta1, which is where the helper is going — no new export needed.

3. Do not change the defaulting. Default() already does the right thing; the bug is purely that one builder bypassed it.

Tests

internal/resource/persistence/ has no schema_setup_job_builder_test.go today (only schema_scripts_configmap_builder_test.go, template_test.go, utils_test.go) — which is why this regressed silently. Add one:

• spec.admintools.version: "1.24.2" set → job image is temporalio/admin-tools:1.24.2. This is the direct regression test; it fails on main with the output above.
• spec.admintools.image set to a private registry + version set → both honoured.
• neither set, after Default() → falls back to DefaultAdminToolTag(spec.version), i.e. today's behaviour is preserved for users who never set the field.
• spec.admintools nil, Default() not run → helper returns the full default rather than panicking.

Also add a case to internal/resource/admintools (if it has a builder test) asserting the deployment and the job resolve to the same image for the same CR — that is the invariant the shared helper exists to hold.

e2e: tests/e2e/persistence_test.go exercises the schema jobs but not the image; asserting the container image there would catch a future divergence at the cluster level. Optional — the unit test is the load-bearing one.

Risk

As the issue says: users who never set spec.admintools.version see no change (the fallback is identical), and users who did set it see the job image change to what they asked for. The blast radius is the image string only. The one thing to watch is that temporalio/admin-tools:<plain version> must actually exist for the versions users configure — DefaultAdminToolTag exists precisely because some do not (1.23.1, the 1.24.x naming, the >= 1.25 major.minor scheme). Restoring the contract means a user who sets a nonexistent tag now gets an ImagePullBackOff on the setup job instead of a silently-substituted working tag. That is the documented behaviour and matches the admintools deployment, but it is worth a line in the release notes.