Adds WithPlatform constructor option for initialization/manifest list querying values

[micahyoung]

The changes in this PR are to introduce a WithPlatform() option for NewImage() to make it easier to override the default Architecture/OS/OSVersion with alternate values in new images and querying manifest lists with FromBaseImage or WithPreviousImage. Imgutil NewImage() currently creates new images with a default of linux/amd64 for remote and daemon OS/amd64 for local, and when using remote.FromBaseImage() or remote.WithPreviousImage(), it queries manifest lists preferring linux/amd64 as inherited from go-containerregistry.

The new implementation attempts to be additive and optional, preserving existing behavior (with an exception on Windows¹) but enabling some new scenarios:

• Initialize a new empty image with Architecture/OS/OSVersion:
  remote

  img, err := remote.NewImage(
      "gcr.io/my-new-linux-arm-image:latest",
      authn.DefaultKeychain,
      remote.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "linux",
      }),
  )
  ...
  img.Architecture() // => "arm"
  img.OS()           // => "linux"

  local with a windows/amd64 daemon

  img, err := local.NewImage(
      "gcr.io/my-new-windows-arm-image:latest",
      dockerClient,
      local.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "windows",
  	    OSVersion:    "10.0.999",
      }),
  )
  ...
  img.Architecture() // => "arm"
  img.OS()           // => "windows"
  img.OSVersion()    // => "10.0.999"

• Initialize a new remote image from a base image in multi-arch/os manifest list:

  img, err := remote.NewImage(
      "gcr.io/my-new-linux-arm-image:latest",
      authn.DefaultKeychain,
      remote.FromBaseImage("golang:1.15"), // golang manifest list (contains matching linux/arm image)
      remote.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "linux",
      }),
  )
  ...
  img.Architecture() // => "arm"
  img.OS()           // => "linux"

• Initialize a new empty image when manifest list doesn't contain image manifest for requested platform:

  img, err := remote.NewImage(
      "gcr.io/my-new-windows-arm-image:latest",
      authn.DefaultKeychain,
      remote.FromBaseImage("golang:1.15"),  // golang manifest list (has no windows/arm image)
      remote.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "windows",
      }),
  )
  ...
  img.Architecture() => "arm"
  img.OS() => "windows"

There is additional behavior that is intended to stay as close as possible to existing behavior for NewImage() and FromBaseImage(), but may nevertheless be surprising:

• A single image manifest result for FromBaseImage() is always used, even when it doesn't match the WithPlatform() values

  img, err := remote.NewImage(
      "gcr.io/my-supposedly-windows-arm-image:latest",
      authn.DefaultKeychain,
      remote.FromBaseImage("amd64/golang:1.15"),  // linux/amd64 image manifest
      remote.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "windows",
      }),
  )
  ...
  img.Architecture() => "amd64"
  img.OS() => "linux"

  // with a windows/amd64 daemon
  img, err := local.NewImage(
      "gcr.io/my-supposedly-windows-arm-image:latest",
      authn.DefaultKeychain,
      remote.FromBaseImage("winamd64/golang:1.15"),  // locally pulled windows/amd64 image
      remote.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "windows",
      }),
  )
  ...
  img.Architecture() => "amd64"
  img.OS() => "windows"

  The reasoning for this behavior is that WithPlatform() should primarily be used to assign values to a new image when nothing is inherited - when either no FromBaseImage() is given or when the FromBaseImage() image is not found. It's secondary purpose is for remote's special edge-case when FromBaseImage() needs a manifest list tie-breaker. It shouldn't be treated as a constraint or validator. This approach also follows ggcr/remote.Image(), which currently only uses it's own ggcr/remote.WithPlatform() option when the ref is a manifest list, and disregards it for single image manifests even when the platform and image conflict.

  In addition to deviating from ggcr, the potential alternative approaches I looked into all had downsides:

  • If it returned the base image with the platform values clobbered, it would be difficult for a consumer to figure out what happened and message a user.
  • If it were to ignore the base image result and return an empty image based on the platform values, it would also be difficult to for consumers to deduce what happened since the image ref was otherwise valid.
  • If it were to return an error, we'd need to add custom filtering and error conditions. Not returning an image could potentially prevent consumers from implementing their own filtering/validation after NewImage is called. For consumers currently using NewImage() and accepting all image platforms, new errors could make backward compatibility challenging, if they only want WithPlatform for manifest list tie-breaking and new image defaults.

  With the PR approach, consumers can hopefully compare the returned image with their desired platform, and throw an appropriate error if needed. I'm sure there are other considerations though so I'm open for feedback on this.

• ¹ New empty Windows images are always initialized with minimal Windows base layers (Adds support for creating Windows scratch-equivalent baselayers #64)

  // with a windows/amd64 daemon
  img, err := local.NewImage(
      "gcr.io/my-new-windows-arm-image:latest",
      dockerClient,
      local.WithPlatform(imgutil.Platform{
  	    Architecture: "arm",
  	    OS:           "windows",
  	    OSVersion:    "10.0.999",
      }),
  )
  ...
  img.Architecture() // => "arm"
  img.OS()           // => "windows"
  img.OSVersion()    // => "10.0.999"
  len(image.Layers()) // => 1

  A Windows image cannot be pulled or stored on a daemon without a valid base layer - either from a Microsoft-authored base image or one from layer.WindowsBaseLayer(). To simplify new image creation, NewImage() now adds one based on layer.WindowsBaseLayer(), when FromBaseImage() is not used or returns an empty image. This will allow Linux and Windows to behave much more consistently and hopefully hides away this implementation detail for most consumers. It could also allow us to deprecate the shim base layer if it becomes unnecessary.

  It is a breaking change for all Windows consumers that already externally add their own base layer with layer.WindowsBaseLayer() and AddLayer() since that will effectively add it twice. This should be acceptable since it's only known to be used in pack which has it behind an experimental flag. Additionally, this new functionality should likely have been included in the original implementation but was blocked on the missing WithPlatform support. It is included in the PR since it's the driver for this WithPlatform implementation (Fixes cache image with incorrect OS on windows lifecycle#532), though can be separated out if needed.

[natalieparellano]

Is this to avoid having the assertion switch on the daemon OS?

[micahyoung]

Yes, and I realize it's repeated in a couple other places - I'll toss in a helper to clarify it.

The reason it needed to be changed here is that NewImage() can no longer create Windows images with no layers at all (which technically could be saved to a daemon, but have no value that I could see).

If layerless Windows images are important, I can certainly take a shot at getting them back

[micahyoung]

I added a new helper that works as follows:

from:

prevLayer1SHA = inspect.RootFS.Layers[len(inspect.RootFS.Layers)-2]

to:

// negative index (offset from len(inspect.RootFS.Layers))
prevLayer1SHA = h.StringElementAt(inspect.RootFS.Layers), -2)

// also normal index
prevLayer1SHA = h.StringElementAt(inspect.RootFS.Layers), 1)

[ekcasey]

The reason it needed to be changed here is that NewImage() can no longer create Windows images with no layers at all (which technically could be saved to a daemon, but have no value that I could see).

This statement confuses me. I thought we were always adding a base layer on windows images because it is impossible to pull a windows image without one. Can you successfully docker load an empty image, even though you can't pull one?

[natalieparellano]

@micahyoung I think the flow you proposed makes sense. I know @ekcasey will want to review this one, but this looks good to me!

[ekcasey]

Should we just fail in this case? Given that we can't sensibly reconcile the two options?

[micahyoung]

I think this gets tricky because we have WithPlatform doing double-duty for remote (1. as a new image value setter, 2. as a tie-breaker for manifest lists). If an imgutil consumer wants to call NewImage in a way that ignores platform for image manifest refs, but has some tie-breaking for manifest list refs, then I think the only way to do this with a single call is to ignore the platform for image manifest results entirely (much like ggcr does). And because we're stuck with that behavior for remote, I think we need to stay consistent in local.

Maybe a different name for WithPlatform would clarify this behavior though, maybe WithPreferredPlatform or WithFallbackPlatform or something. Or we could split out the double-duty of remote.WithPlatform and add a platform param to remote.FromBaseImage and remote.WithPreviousImage and those could deal with manifest list platform disambiguation only.

I'd love to hear other thoughts though.

[micahyoung]

Thanks for the reviews! Here's my TODO list so far:

• Add doc comments for each ImageOption
• Rename to WithDefaultPlatform
• Remove Variants, Features, OSFeatures from imgutil.Platform
• Refactor ImageOption to not alter the image directly, restore ImageOption instead of interface{} in constructor
• Sanity check state after WithPlatform + SetOS are used.
• Add test helper for assertions on top-2-image layers
• Add test case for WithPlatform() + WithPreviousImage()

[micahyoung]

After the above changes, plus some manual and newly enabled acceptance lifecycle testing on Windows, I'm feeling like this is in a good state and ready for another look, if you all wouldn't mind.

[ekcasey]

Looking good! Just a couple suggestions for cleanup.

[micahyoung]

I made changes based on the feedback, which did clarify the logic. I feel like I'll want to revisit this at some point after living with it a bit and maybe looking into some different option pattern examples - I feel there must be some tidier ways to move the logic back into the ImageOptions, but still enforce the order of execution that we need.