operator: resolve component images from a full registry prefix (#574) - #576
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes operator component image resolution so UNBOUNDED_IMAGE_REGISTRY is treated as a complete image-repository prefix (registry host + optional org/namespace path) rather than implicitly appending /azure/, preventing forks/mirrors/air-gapped registries from silently pulling upstream azure/* images.
Changes:
- Update component image construction to
TrimRight(ImageRegistry)+"/"+component+":"+tag(no implicit/azure/) and align defaults toghcr.io/azure. - Eliminate Makefile drift by deriving
UNBOUNDED_OPERATOR_IMAGE_REGISTRYfromCONTAINER_REGISTRY. - Add migration + tests so reinstall rewrites preserved bare-host legacy values (e.g.
ghcr.io) toghcr.io/azure, keeping behavior stable across upgrades.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Derives UNBOUNDED_OPERATOR_IMAGE_REGISTRY from CONTAINER_REGISTRY to prevent operator/component registry drift. |
| internal/operator/component/env.go | Implements full-prefix semantics for component image resolution (drops hardcoded /azure/). |
| internal/operator/component/env_test.go | Rewrites Config.Image test to a table covering bare host, org prefix, multi-segment paths, and trailing slashes. |
| internal/operator/components/net/net_test.go | Updates expected stamped component image references to match new prefix semantics. |
| internal/operator/components/machina/machina_test.go | Updates expected machina image reference to remove the extra /azure/ segment. |
| internal/operator/components/metalman/metalman_test.go | Updates expected metalman image reference to remove the extra /azure/ segment. |
| internal/operator/components/storage/storage_test.go | Updates expected storage supervisor image reference to remove the extra /azure/ segment. |
| cmd/unbounded-operator/main.go | Changes --image-registry default/help to the new full-prefix default ghcr.io/azure. |
| cmd/unbounded-operator/main_test.go | Updates default registry expectation to ghcr.io/azure. |
| cmd/kubectl-unbounded/app/install.go | Updates install-time default and adds migration for preserved legacy bare-host registry values. |
| cmd/kubectl-unbounded/app/install_test.go | Updates expectations and adds migration-focused tests (unit + install flow). |
| deploy/unbounded-operator/03-configmap.yaml.tmpl | Updates ConfigMap default and clarifies full-prefix semantics in template comments. |
| deploy/unbounded-operator/04-deployment.yaml.tmpl | Updates config-hash default for UNBOUNDED_IMAGE_REGISTRY to ghcr.io/azure to match templates/CLI. |
| deploy/unbounded-operator/render_test.go | Adds a guard test ensuring rendered operator image prefix matches rendered component registry prefix (prevents regression). |
| docs/content/reference/cli.md | Updates CLI docs for new --image-registry default/meaning and documents install-time migration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cmd/kubectl-unbounded/app/install.go:446
- migrateLegacyImageRegistry currently appends "/azure" to any legacy (unmarked) UNBOUNDED_IMAGE_REGISTRY value, including path-based mirrors (e.g. "registry.corp/unbounded" -> "registry.corp/unbounded/azure"). The PR description mentions migrating only a preserved bare host (no path segment). Please confirm which behavior is intended and align either the migration logic or the PR description/release notes so they match actual upgrade semantics.
return trimmed + "/azure"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cmd/kubectl-unbounded/app/install.go:384
- embeddedImageRegistry() silently falls back to "ghcr.io/azure" when the embedded manifests cannot be walked/parsed. In the (admittedly unexpected) case of embed/read failures or an injected manifest set missing the ConfigMap, this can repoint an install to upstream images without the caller explicitly choosing that registry (the exact failure mode #574 is trying to avoid). Consider returning an error (and failing install unless --image-registry is provided) rather than silently defaulting.
files, err := yamlFiles(h.manifests())
if err != nil {
return fallbackImageRegistry
}
cmd/kubectl-unbounded/app/install.go:350
- The comment implies only a legacy bare "ghcr.io" value is rewritten, but the current logic overwrites any previously stored UNBOUNDED_IMAGE_REGISTRY with the embedded/flag value and logs the change. Update the comment to match the actual behavior so future readers don’t assume the rewrite is limited to bare-host migration.
// Surface a registry change so repointing components is not silent (the main
// concern in #574): an upgrade of an existing cluster rewrites a pre-#574
// bare "ghcr.io" to the resolved full prefix.
The operator resolved component images by appending a hardcoded /azure/ segment to UNBOUNDED_IMAGE_REGISTRY, while release images are published flat at <registry>/<component> where the org (azure) is the only path segment. The two only agreed because the GitHub org is literally named azure, so no value of the setting produced a correct reference for any fork, mirror, or air-gapped registry. Make Config.Image treat ImageRegistry as a full image-repository prefix (registry host plus org/namespace) and drop the literal. Defaults move to ghcr.io/azure across the operator flag, the operator ConfigMap and Deployment hash templates, and kubectl unbounded install. The Makefile now derives UNBOUNDED_OPERATOR_IMAGE_REGISTRY from CONTAINER_REGISTRY so it cannot drift from the operator's own image, and forks work because the release workflow already overrides CONTAINER_REGISTRY. Existing clusters store a bare ghcr.io that would resolve to ghcr.io/machina under the new semantics, so install migrates a preserved bare host (no path segment) to <host>/azure on reinstall. The rewrite is idempotent and leaves full-prefix values untouched. Add a Config.Image table test, a migration test, and a render guard that asserts the operator image and component registry share a prefix.
Follow-up to the full-prefix change addressing review findings. Finding 1 (install discarded the build registry): install hardcoded ghcr.io/azure as the fresh-install component registry and overwrote the embedded ConfigMap, so a fork build (operator image ghcr.io/myorg) still configured components from ghcr.io/azure. prepareOperatorConfig now reads the UNBOUNDED_IMAGE_REGISTRY baked into the embedded operator ConfigMap, tying the default to the same CONTAINER_REGISTRY that produced the operator image. The manifest FS is injectable so tests exercise a fork build. Finding 2 (fork release binaries embedded Azure manifests): the GoReleaser before-hook renders the operator manifests embedded in kubectl-unbounded but the binaries job passed no CONTAINER_REGISTRY. Add the REGISTRY lowercasing step (ghcr rejects uppercase, owner Azure -> azure) and pass CONTAINER_REGISTRY to GoReleaser so a fork's binary embeds its own registry. Findings 3 and 4 (incomplete/ambiguous migration): replace the bare-host heuristic with a schema-marker annotation on the operator ConfigMap. An unmarked (pre-#574) value is migrated by appending /azure, which reproduces the old images for bare hosts and path-based mirrors alike; a marked value is preserved verbatim so an intentional prefix is never rewritten and migration runs exactly once. The template carries the marker for direct-apply/GitOps installs. Tests: fork-build install preserves the embedded registry and operator image; table migration covers bare, path, and marked values; render guard now asserts the registry is lowercase and the marker is present. Docs note the full-prefix breaking change and the auto-migration.
Replace the schema-marker migration with a simpler model: install always re-derives UNBOUNDED_IMAGE_REGISTRY from the binary's embedded operator ConfigMap (overridable by --image-registry), rather than preserving and migrating a stored cluster value. The component registry must match the operator image for version lockstep, and install already re-derives the operator image from the binary on every run (an empty --operator-image keeps the embedded image). Preserving the registry independently was what let the two drift. Deriving both from the same embedded source removes the drift by construction, so the marker annotation, the migration function, and the preserve-across-reinstall logic are all unnecessary. An older cluster storing the pre-change bare "ghcr.io" is overwritten with the embedded ghcr.io/azure on the next install; the change is logged so it is not silent. Endpoint and reaper flag remain preserved (they are cluster policy, not build artifacts). Tests: reinstall overwrites a stale stored registry with the embedded value; --image-registry overrides; fork build derives its own registry and operator image. Drops the marker/migration tests and the ConfigMap schema annotation. Docs updated for the derive-not-preserve behavior.
Address two review findings. Fail closed (was: silent fallback to ghcr.io/azure). embeddedImageRegistry now returns an error when the embedded operator manifests are unreadable, missing the unbounded-operator-config ConfigMap, or carry an empty UNBOUNDED_IMAGE_REGISTRY, instead of quietly defaulting to the upstream registry. prepareOperatorConfig uses --image-registry when set, otherwise requires the build-derived value and errors with an actionable message. A malformed fork/private build can no longer install while pulling Azure components. Tests cover missing/empty/malformed embedded values and the flag override. Release repo now derives from the checked-out repository. GoReleaser's release.github owner/name were hardcoded to Azure/unbounded, so a fork's release job failed before producing the fork-correct binaries. The Actions GITHUB_TOKEN is scoped to the running repository, so omitting owner/name lets GoReleaser target that repository without adding any cross-repo capability. The separate concern that `site init` implicitly (re)installs and can repoint the operator is tracked in #578.
7c91e52 to
655b766
Compare
- install: update the --image-registry flag help to the full-prefix wording, matching the operator flag and cli.md. - tests: make the build-derived-registry assertions hermetic. Inject a known embedded manifest set (new operatorManifestsFS helper, shared with the fork test) in TestInstallReinstallDerivesRegistryFromBuild, and read the embedded value in TestInstallMergesLiveReaperConfig instead of hardcoding ghcr.io/azure, so a fork build (non-default CONTAINER_REGISTRY) no longer fails these. - embeddedImageRegistry: note why it walks the manifests directly rather than reusing component.DefaultConfigMap.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/release.yaml:207
CONTAINER_REGISTRYis set from the${{ env.REGISTRY }}expression, butREGISTRYis being normalized via$GITHUB_ENVin a prior step. Values written to$GITHUB_ENVare available as runtime environment variables (e.g.$REGISTRY) but are not reliably reflected in theenvexpression context, so this can still pass an un-normalized (mixed-case) registry into GoReleaser and break GHCR pushes for owners likeAzure.
Derive the lowercase value in the expression itself (or set CONTAINER_REGISTRY via $GITHUB_ENV and omit the override).
CONTAINER_REGISTRY: ${{ env.REGISTRY }}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cmd/kubectl-unbounded/app/install.go:379
- PR description mentions the component registry is "overridable by --image-registry", but this implementation has no such flag (it derives from --operator-image or from the embedded manifests). Please update the PR description to match the actual interface (override via --operator-image only, or reintroduce a dedicated flag if that was intended).
// resolveComponentRegistry determines the registry prefix components are pulled
// from. When --operator-image is set it derives the registry from that image, so
// the operator and the components it deploys always come from the same registry
// (there is no separate flag that can drift). Otherwise it uses the registry
// baked into the embedded manifests at build time. Both paths fail closed rather
// than defaulting to the upstream registry.
func (h *installHandler) resolveComponentRegistry() (string, error) {
994593c to
719de5b
Compare
Keep both install flags but make them unambiguous about the registry so the operator and the components it deploys can never point at different registries (the #574 drift, reachable before via a registry-qualified --operator-image that disagreed with --image-registry). - --image-registry is the single source for where all unbounded images come from; it is written verbatim to UNBOUNDED_IMAGE_REGISTRY. - --operator-image is only the operator's name:tag; a registry-qualified value is rejected. install stamps the operator Deployment image as <registry>/<name:tag>. The registry comes from --image-registry or, unset, the registry baked into the embedded manifests; the operator name:tag comes from --operator-image or the embedded operator image. Both fail closed rather than defaulting to the upstream registry. When --image-registry is set without --operator-image, install rewrites the embedded operator image's registry to match, preventing the reverse drift where the operator would run from the embedded registry while components come from --image-registry. The operator's runtime mechanism is unchanged (it reads UNBOUNDED_IMAGE_REGISTRY; the component tag is its compiled version). No new dependency: extracting the registry is a known-prefix strip, not image-reference parsing. BREAKING: --image-registry is a full image-repository prefix (registry host plus org/namespace), not a bare host the operator appends /azure/ to; --operator-image is a name:tag with no registry. Automation passing a bare ghcr.io to --image-registry, or a fully-qualified --operator-image, must update. Tests: --image-registry drives both images; a bare --image-registry reconstructs the operator image's registry; a registry-qualified --operator-image is rejected; fail-closed when neither flag nor embedded value yields a registry. Docs updated for the two-flag model.
719de5b to
2716273
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cmd/kubectl-unbounded/app/install.go:481
- embeddedOperatorImage() can return a misleading error if the embedded unbounded-operator Deployment exists but lacks a container named "controller": the loop falls through and ultimately reports that no Deployment exists. This makes diagnosing a malformed embedded manifest set harder.
for _, c := range containers {
container, ok := c.(map[string]any)
if !ok || container["name"] != "controller" {
continue
}
cmd/kubectl-unbounded/app/install_test.go:563
- The doc comment says supplying flags lets install proceed "without reading the embedded manifests", but resolveImages() still calls embeddedImageRegistry() unconditionally (it just doesn't fail when flags are set). This comment should be adjusted to avoid implying the embedded manifests are never read.
// TestInstallFailsClosedOnUnresolvableRegistry asserts install errors (rather
// than defaulting to ghcr.io/azure) when the embedded registry cannot be resolved
// and no --image-registry is given, and that supplying the flags lets install
// proceed without reading the embedded manifests.
func TestInstallFailsClosedOnUnresolvableRegistry(t *testing.T) {
- execute: reset resolvedOperatorImage alongside the other per-run derived fields so a repeated call cannot carry a stale value. - Deduplicate the embedded-manifest walkers behind a shared embeddedManifestValue(kind, name, desc, extract) helper; embeddedImageRegistry and embeddedOperatorImage become a match plus an extract closure. - Guard the operator name:tag derived from the embedded image against a "/", so the invariant (embedded image is a direct child of its registry) is symmetric with the flag path; add a unit test. Unreachable via the render pipeline, catches a hand-edited manifest. - Reword the fail-closed error when the operator name:tag cannot be recovered from the embedded manifests, and collapse the duplicate branches with errors.Join.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cmd/kubectl-unbounded/app/install.go:410
--operator-imageis documented as aname:tagoverride, butresolveImagesonly rejects values containing "/". If a user passes--operator-image unbounded-operator(no tag/digest), Kubernetes will pull:latestwhile components still use<operator-version>, breaking the operator/component version-lockstep invariant and potentially installing mismatched binaries.
nameTag := h.operatorImage
if nameTag != "" {
if strings.Contains(nameTag, "/") {
return "", "", fmt.Errorf("--operator-image %q must be an image name and tag with no registry (for example unbounded-operator:v1); set the registry with --image-registry", nameTag)
}
Problem
Fixes #574.
The operator resolved every component image by appending a hardcoded
/azure/segment to a registry setting:
But release images are published flat at
<registry>/<component>, where<registry> = ghcr.io/${{ github.repository_owner }}and theazuresegment isjust the org. The literal and the org only agreed because the GitHub org is
literally named
azure, so:ghcr.io, andthe operator's own image came from an unrelated variable (
CONTAINER_REGISTRY,default
ghcr.io/azure). Setting the registry toghcr.io/myorgproduced aphantom
ghcr.io/myorg/azure/machina; leaving it default produced upstreamghcr.io/azure/machina.was correct) and then deployed components from
ghcr.io/azure, violating theversion-lockstep invariant. If the tag existed upstream the pull succeeded on
the wrong binaries.
/azure/directory, blockingoffline install (support offline installation and per-site image configuration for Unbounded components #461).
The root cause was two independent paths for the operator image and the component
registry that were only kept in sync by the org name.
Fix
1. Full-prefix semantics.
Config.Imagenow treatsImageRegistryas acomplete image-repository prefix (registry host plus org/namespace) and appends
the flat component name directly, with no implicit segment. Defaults move to
ghcr.io/azureacross the operator flag, the operator ConfigMap and Deploymenttemplates.
2. Remove the drift at its source.
UNBOUNDED_OPERATOR_IMAGE_REGISTRYnowderives from
CONTAINER_REGISTRYin the Makefile, so the component registrycannot diverge from the operator image, and a fork that overrides
CONTAINER_REGISTRYgets both pointed at its own org.3.
kubectl unbounded installsingle-sources the registry.--image-registryis the one knob for where all unbounded images come from (written verbatim to
UNBOUNDED_IMAGE_REGISTRY);--operator-imageis only the operator'sname:tag(a registry-qualified value is rejected). install stamps the operator Deployment
image as
<registry>/<name:tag>, so the operator and the components it deploysalways share the registry and cannot diverge. Unset, the registry and the operator
name:tag both come from the manifests embedded at build time (rendered from
CONTAINER_REGISTRY), so fork/mirror builds work with no flag. When--image-registryis set without--operator-image, install rewrites the embeddedoperator image's registry to match (preventing the reverse drift). Both defaults
fail closed rather than defaulting to the upstream registry; a stale stored value
is overwritten and the change is logged. The operator's runtime mechanism is
unchanged (it reads
UNBOUNDED_IMAGE_REGISTRY; the component tag is always theoperator's compiled version). No new dependency is needed - extracting the registry
is a known-prefix strip, not image-reference parsing.
4. Release pipeline renders fork-correct binaries. The GoReleaser before-hook
renders the operator manifests embedded in
kubectl-unbounded. Thebinariesjob now normalizes
REGISTRYto lowercase (ghcr rejects uppercase repositorynames, so owner
Azure->azure) and passesCONTAINER_REGISTRYto GoReleaser,so a fork's released binary embeds its own registry instead of
ghcr.io/azure.Behavior changes
UNBOUNDED_IMAGE_REGISTRY/ install--image-registryis now a full image-repository prefix (registry host plus org/namespace), not a
bare host the operator appends
/azure/to. Automation passing a bareghcr.iomust pass
ghcr.io/azure(or its own equivalent).--operator-imageis now the operator'sname:tagwith noregistry (the registry comes from
--image-registry); a registry-qualified valueis rejected. Automation passing a fully-qualified
--operator-imagemust move theregistry to
--image-registry.reinstalls (it tracks the operator image). A private/air-gapped install sets
--image-registry(e.g.registry.corp/unbounded), or uses a build whoseCONTAINER_REGISTRYbakes it in; the operator image follows.Upgrade / migration
ghcr.iois rewritten to the full prefix(
ghcr.io/azureon a stock binary) on the nextkubectl unbounded install;install logs
updating component image registry from ... to ...so the repointis not silent.
kubectl apply/ GitOps upgrades migrate via the new rendered ConfigMapdefault (
ghcr.io/azure).Tests
Config.Imagetable test (bare host, host+org, fork org, multi-segment path,trailing slash) plus updated component image assertions (net, machina, storage,
metalman).
--image-registrydrives both the component registry and the operatorDeployment image (
<registry>/<name:tag>); a bare--image-registry(no--operator-image) reconstructs the operator image's registry; aregistry-qualified
--operator-imageis rejected; fail-closed when neither theflags nor the embedded value yield a registry; fork build (injected embedded
manifests) derives its own registry and keeps its own operator image;
endpoint/reaper flag remain preserved.
prefix (
<registry>/unbounded-operator:<tag>) and that the registry islowercase, so the two paths and the ghcr casing requirement cannot regress.
Out of scope
kubectl applyUX gap (kubectl unbounded install --dry-run -o yaml),noted in operator: image registry override cannot express non-azure registries #574, is filed separately.
<prefix>/azure/layout arenot auto-rewritten; re-specify the registry (or re-mirror without the phantom
directory) on upgrade.