Azure / Azure/unbounded

operator: derive component images from rendered workloads instead of a hand-maintained table

Open
#618 0 comments 0 reactions 0 assignees View on GitHub
go releng
Dominant language
Go
Stars
28
Forks
11
Avg merge
1d 8h
Merged PRs (30d)
55

Description

Follow-up from #617.

## Context

#617 added `internal/operator/imagecoverage_test.go`, which holds `nightly.yaml`, `release.yaml`, and the release BOM to the set of images the operator applies to its workloads. That set lives in a hand-maintained table:

```go
var componentImages = map[string][]string{
"net": {"unbounded-net-controller", "unbounded-net-node"},
"machina": {"machina"},
"gantry": {"gantry"},
"metalman": {"metalman"},
"storage": {"unbounded-storage-supervisor"},
}
```

`TestComponentImagesCoversRegistry` cross-checks the table's **keys** against `DefaultRegistry()` in both directions and rejects an empty list, so a new component cannot be registered without a matching edit.

## The gap

The cross-check compares component names, never the image lists under them. Nothing ties those values back to the `cfg.Image(...)` call sites they mirror. Two classes escape:

1. **An existing component grows an additional image.** `net` already applies two, gated at `internal/operator/components/net/net.go:166-168` on the workload's own name. Add a third workload, extend that gate, and `componentImages["net"]` stays a stale two-element list while every test in the file stays green.
2. **A repository name that is merely wrong.** `cfg.Image("machina")` becoming `cfg.Image("machina-controller")` is invisible.

The compensating control is weaker than #617's original description claimed. `hack/release/wait-rollouts.sh` is scoped to the workloads named in its argument list, which at `nightly.yaml:541-546` is:

```
deploy/unbounded-operator deploy/unbounded-net-controller ds/unbounded-net-node
deploy/machina-controller ds/gantry
```

`metalman` and `unbounded-storage-supervisor` are absent, so two of the five components have no deploy-time image coverage at all. The script also documents itself as fail-open, and treats `ImagePullBackOff` as retryable with a 90s grace rather than terminal.

## Proposal

Replace the hand-maintained values with images scraped from what the components actually render, keeping the table (or dropping it entirely) as a cross-check:

1. Build each component from `DefaultRegistry()` with a `component.Env` backed by a fake client and a sentinel `ImageRegistry`/`ImageTag`.
2. Run `Plan()` against a synthetic `Site`.
3. Walk the resulting objects' `initContainers` and `containers`, collecting images carrying the sentinel prefix.
4. Assert that set against the workflows and the release BOM.

This is the actual source of truth, so it closes both classes above, for all five components including `net`.

The pattern already exists per-component and would mostly be assembly:

- `internal/operator/components/gantry/gantry_test.go:41` and `internal/operator/components/storage/storage_test.go:25` build `component.Env` with `fake.NewClientBuilder()`.
- `internal/operator/components/gantry/gantry_test.go:183` already asserts on rendered container images.

## Things to work out

- **Images pinned to a public reference must be excluded.** Gantry's `chown-hostpaths` init container is `mcr.microsoft.com/cbl-mariner/busybox:2.0` (`deploy/gantry/daemonset.yaml.tmpl:121`) and is deliberately left alone via `SetNamedContainerImage` (`gantry.go:238`). Filtering on the sentinel registry prefix should handle this, but it needs to be a deliberate assertion rather than an accident.
- **`Plan()` reads cluster state**, so the fake client needs enough seeded objects for each component to reach the workload-rendering path. Site components additionally need `Enabled()` to return true.
- **Whether the table survives.** If the scrape is reliable, `componentImages` could be deleted and the workflow and BOM checks driven straight off the rendered set. Worth deciding whether losing the explicit, reviewable list is a good trade.
- Keep the anti-vacuity guards from #617; a scrape that silently returns nothing would make all three tests pass forever.

## Related, out of scope here

`release.yaml`'s multi-arch `:TAG` index for the two net images is created by `docker buildx imagetools create` in the `net-images-manifest` job (`release.yaml:479-490`), not by a `docker/build-push-action` step. #617's parser only sees the per-arch push steps, so it confirms the repository is built without confirming the tag the operator actually pulls exists. Worth a separate look.

Contributor guide

Open the contributing guide

Research direction

Start with internal/operator/imagecoverage_test.go and DefaultRegistry(), then compare the component Env setup in internal/operator/components/gantry/gantry_test.go:41 and storage_test.go:25. Run Plan() for a synthetic Site and collect sentinel-prefixed images from rendered initContainers and containers, including the existing assertion pattern at gantry_test.go:183. Done means the rendered image sets cover the workflows and release BOM, exclude public images deliberately, and retain anti-vacuity checks.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
devops, infrastructure, testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.