Fix scratch multi-platform images

[jedevc]

Fixes docker/build-push-action#1024.

The main fix is a bug in ConvertResult that accidentally discarded zero-values. This essentially meant that images that had nil entries in the refs map (i.e. scratch images) would lose those - this caused an error in the exporter: cannot export multiple platforms without multi-platform enabled.

Everything else is a fix needed to not get nil panics everywhere - there are certain parts of the code that never saw nil in the Refs map, so these now need to be handled (and mostly skipped over).

Skipping over attestations

@tonistiigi raised this in 3c1deef#r135018757.

Note from the commit message:

Instead, we just skip over nil refs. This isn't ideal - we should be able generate provenance for a nil ref. However, 1. this isn't handled for llb.Scratch as a single-platform result, and 2. this is tricky, since the ResultProxy is nil at this point (so there's no ID to match on). This should be ok for a quick fix, but we can come back and fix this later.

If the ResultProxy is nil there's no ID that we can match for findByResult - to allow provenance for nil reference (which we should do), then we'd need to have a ResultProxy for this empty result.

Here's an example of where this matters: we do two builds on a provenanceBridge, and both of them have multi-platform scratch results (so nil in the refs entry). If we pass nil to findByResult, which one of these should match? We have no way of working backwards to it.

A potential fix to this problem (but one I think is out-of-scope for here) is we need to make sure the ResultProxy is never nil so that we can track it for provenance - essentially make sure that even when req.Definition.Def below is nil, that we still have a proxy:

buildkit/solver/llbsolver/provenance.go

Lines 175 to 177 in 3c0fe5e

	} else {
	return &frontend.Result{}, nil
	}

This feels a bit more invasive, and I'm less confident about that change than the rest of this, so figured it would make sense to submit this as a patch, and we could discuss about the rest of it as a follow-up.

[tonistiigi]

I wonder if it makes sense to extend the test to return image config for the scratch result (or move to Dockerfile test where this is easier). So that we can test that everything else around tracking the results by the platform key and results having separate metadata still works even if there are no layers.

Regarding provenance, I still think the nil result should have provenance. Ok to leave it as a follow up but would be good to at least have a test that checks that asking for a provenance for such build does not cause any panics. Otherwise these extra continue and zero value changes are hard to validate.

Otherwise LGTM

[jedevc]

I wonder if it makes sense to extend the test to return image config for the scratch result

Added this.

Out of curiosity (potential follow-up) - if we don't return a config key as part of the metadata, each platforms config defaults to the current platform:

buildkit/exporter/containerimage/writer.go

Lines 620 to 634 in 15b7b54

	func defaultImageConfig() ([]byte, error) {
	pl := platforms.Normalize(platforms.DefaultSpec())
	img := ocispecs.Image{}
	img.Architecture = pl.Architecture
	img.OS = pl.OS
	img.OSVersion = pl.OSVersion
	img.OSFeatures = pl.OSFeatures
	img.Variant = pl.Variant
	img.RootFS.Type = "layers"
	img.Config.WorkingDir = "/"
	img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(pl.OS)}
	dt, err := json.Marshal(img)
	return dt, errors.Wrap(err, "failed to create empty image config")
	}

Instead, shouldn't we prefer to use the detected platform from ParsePlatforms which uses ExporterPlatformsKey if available?

Ok to leave it as a follow up but would be good to at least have a test that checks that asking for a provenance for such build does not cause any panics

Added attest:provenance flag in here. This shouldn't have an impact, since the provenance is always generated (so it can be added to the history API). However, the codepaths are slightly difference, so worth having the check anyways.

[tonistiigi]

Instead, shouldn't we prefer to use the detected platform from ParsePlatforms which uses ExporterPlatformsKey if available?

SGTM.

[tonistiigi]

Could we add like a label in here that is unique per platform so that we can verify it. Otherwise there still isn't a strong guarantee that this config ended up in build result.

[jedevc]

Done.