merge #640 into opencontainers/umoci:main

Aleksa Sarai (7):
  umoci new: include host ARM variant by default
  config: add support for platform variants
  oci: config: fix annotation precedence
  test: config: add umoci-stat verification checks
  test: config: add tests for config.json annotation conversions
  test: check annotations for config --{os,architecture}
  config: add "platform" prefix to "os" and "architecture" setting names

LGTMs: cyphar

Author: cyphar

CHANGELOG.md

@@ -26,9 +26,13 @@ explicitly use any of the newer features, this is mostly a quality-of-life
 update to move away from our ancient pinned version of the runtime-spec.
 
 ### Breaking ###
-* The existing `ConfigExposedPorts` and `ConfigVolumes` methods of
-  `github.com/opencontainers/umoci/oci/config/generate.Generator` now return a
-  sorted `[]string` instead of a map.
+* `github.com/opencontainers/umoci/oci/config/generate.Generator` has had the
+  following breaking API changes made to it:
+  - The existing `ConfigExposedPorts` and `ConfigVolumes` methods now return a
+    sorted `[]string` instead of a `map`.
+  - The `(Set)OS` and `(Set)Architecture` methods have been renamed to have a
+    `Platform` prefix (to match image-spec v1.1's organisational changes). They
+    now read as `(Set)PlatformOS` and `(Set)PlatformArchitecture` respectively.
 
 ### Added ###
 * `umoci stat` now includes information about the manifest and configuration of
@@ -59,6 +63,11 @@ update to move away from our ancient pinned version of the runtime-spec.
   better off just using `gomtree`.
 * `umoci --version` now provides more information about the specification
   versions supported by the `umoci` binary as well as the Go version used.
+* `umoci config` now supports specifying the architecture variant of the image
+  with `--platform.variant`. In addition, `--os` and `--architecture` can now
+  be set using `--platform.os` and `--platform.arch` respectively.
+* `umoci new` will not automatically fill the architecture variant on ARM
+  systems to match the host CPU.
 
 ### Changed ###
 * The output format of `umoci stat` has had some minor changes made to how
@@ -71,6 +80,10 @@ update to move away from our ancient pinned version of the runtime-spec.
   archives. Previously, we would defer to the Go stdlib's `archive/tar` which
   rounds to the nearest second (which is incompatible with `gomtree` and so in
   theory could lead to inconsistent results).
+* Previously, when generating the runtime-spec `config.json`, `umoci unpack`
+  would incorrectly prioritise the automatically generated annotations over
+  explicitly configured labels. This precdence was the opposite of what the
+  image-spec requires, and has now been resolved.
 
 [source-date-epoch]: https://reproducible-builds.org/docs/source-date-epoch/
 [obs-gomtree]: https://build.opensuse.org/package/show/Virtualization:containers/go-mtree

cmd/umoci/config.go

@@ -91,8 +91,10 @@ image.`,
 		cli.StringFlag{Name: "config.stopsignal"},
 		cli.StringFlag{Name: "created"}, // FIXME: Implement TimeFlag.
 		cli.StringFlag{Name: "author"},
-		cli.StringFlag{Name: "architecture"},
-		cli.StringFlag{Name: "os"},
+		cli.StringFlag{Name: "platform.os,os"},
+		cli.StringFlag{Name: "platform.arch,architecture"},
+		cli.StringFlag{Name: "platform.variant"},
+		// TODO: platform.os.{version,features}
 		cli.StringSliceFlag{Name: "manifest.annotation"},
 		cli.StringSliceFlag{Name: "clear"},
 	},
@@ -107,8 +109,9 @@ func toImage(config ispec.ImageConfig, meta mutate.Meta) ispec.Image {
 		Created: &created,
 		Author:  meta.Author,
 		Platform: ispec.Platform{
-			Architecture: meta.Architecture,
 			OS:           meta.OS,
+			Architecture: meta.Architecture,
+			Variant:      meta.Variant,
 		},
 	}
 }
@@ -121,8 +124,9 @@ func fromImage(image ispec.Image) (ispec.ImageConfig, mutate.Meta) {
 	return image.Config, mutate.Meta{
 		Created:      created,
 		Author:       image.Author,
-		Architecture: image.Architecture,
 		OS:           image.OS,
+		Architecture: image.Architecture,
+		Variant:      image.Variant,
 	}
 }
 
@@ -234,11 +238,14 @@ func config(ctx *cli.Context) (Err error) {
 	if ctx.IsSet("author") {
 		g.SetAuthor(ctx.String("author"))
 	}
-	if ctx.IsSet("architecture") {
-		g.SetArchitecture(ctx.String("architecture"))
+	if ctx.IsSet("platform.os") {
+		g.SetPlatformOS(ctx.String("platform.os"))
+	}
+	if ctx.IsSet("platform.arch") {
+		g.SetPlatformArchitecture(ctx.String("platform.arch"))
 	}
-	if ctx.IsSet("os") {
-		g.SetOS(ctx.String("os"))
+	if ctx.IsSet("platform.variant") {
+		g.SetPlatformVariant(ctx.String("platform.variant"))
 	}
 	if ctx.IsSet("config.user") {
 		g.SetConfigUser(ctx.String("config.user"))

doc/man/umoci-config.1.md

@@ -24,8 +24,8 @@ umoci config - Modifies the configuration of an OCI image
 [**--config.workingdir**=*value*]
 [**--created**=*value*]
 [**--author**=*value*]
-[**--architecture**=*value*]
-[**--os**=*value*]
+[**--platform.os**=*value*]
+[**--platform.arch**=*value*]
 [**--manifest.annotation**=*value*]
 
 # DESCRIPTION
@@ -100,8 +100,8 @@ or image manifest. For more information see [the OCI image specification][1].
 * **--config.workingdir**=*value*
 * **--created**=*value*
 * **--author**=*value*
-* **--architecture**=*value*
-* **--os**=*value*
+* **--platform.os**=*value* (can also be set with **--os**)
+* **--platform.arch**=*value* (can also be set with **--architecture**)
 * **--manifest.annotation**=*value*
 
 # EXAMPLE
@@ -113,7 +113,7 @@ overwrites the original tag with the new image.
 % umoci config --image image:tag --clear=config.env --config.env="VARIABLE=true" \
 	--config.user="user:group" --config.entrypoint=cat --config.cmd=/proc/self/stat \
 	--config.label="com.cyphar.umoci=true" --author="Aleksa Sarai <[email protected]>" \
-	--os="gnu/hurd" --architecture="lisp" --created="$(date --iso-8601=seconds)"
+	--platform.os="linux" --platform.arch="amd64" --created="$(date --iso-8601=seconds)"
 ```
 
 # SEE ALSO

go.mod

@@ -23,6 +23,7 @@ require (
 	github.com/AdaLogics/go-fuzz-headers v0.0.0-20230106234847-43070de90fa1
 	github.com/apex/log v1.9.0
 	github.com/blang/semver/v4 v4.0.0
+	github.com/containerd/platforms v0.2.1
 	github.com/cyphar/filepath-securejoin v0.5.0
 	github.com/docker/go-units v0.5.0
 	github.com/klauspost/compress v1.11.3
@@ -42,6 +43,7 @@ require (
 )
 
 require (
+	github.com/containerd/log v0.1.0 // indirect
 	github.com/cpuguy83/go-md2man/v2 v2.0.7 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/fatih/color v1.18.0 // indirect

go.sum

@@ -10,6 +10,10 @@ github.com/aws/aws-sdk-go v1.20.6/go.mod h1:KmX6BPdI08NWTb3/sm4ZGu5ShLoqVDhKgpiN
 github.com/aybabtme/rgbterm v0.0.0-20170906152045-cc83f3b3ce59/go.mod h1:q/89r3U2H7sSsE2t6Kca0lfwTK8JdoNGS/yzM/4iH5I=
 github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM=
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
+github.com/containerd/log v0.1.0 h1:TCJt7ioM2cr/tfR8GPbGf9/VRAX8D2B4PjzCpfX540I=
+github.com/containerd/log v0.1.0/go.mod h1:VRRf09a7mHDIRezVKTRCrOq78v577GXq3bSa3EhrzVo=
+github.com/containerd/platforms v0.2.1 h1:zvwtM3rz2YHPQsF2CHYM8+KtB5dvhISiXh5ZpSBQv6A=
+github.com/containerd/platforms v0.2.1/go.mod h1:XHCb+2/hzowdiut9rkudds9bE5yJ7npe7dG/wG+uFPw=
 github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
 github.com/cpuguy83/go-md2man/v2 v2.0.7 h1:zbFlGlXEAKlwXpmvle3d8Oe3YnkKIK4xSRTd3sHPnBo=
 github.com/cpuguy83/go-md2man/v2 v2.0.7/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=

mutate/mutate.go

@@ -95,9 +95,15 @@ type Meta struct {
 	// are built to run on.
 	Architecture string `json:"architecture"`
 
+	// Variant is the variant of the CPU architecture which the binaries in
+	// this image are built to run on.
+	Variant string `json:"variant"`
+
 	// OS is the name of the operating system which the image is built to run
 	// on.
 	OS string `json:"os"`
+
+	// TODO: Should we embed ispec.Platform?
 }
 
 // cache ensures that the cached versions of the related configurations have
@@ -232,6 +238,7 @@ func (m *Mutator) Set(ctx context.Context, config ispec.ImageConfig, meta Meta,
 	m.config.Created = timePtr(meta.Created)
 	m.config.Author = meta.Author
 	m.config.Architecture = meta.Architecture
+	m.config.Variant = meta.Variant
 	m.config.OS = meta.OS
 
 	// Append history.

new.go

@@ -21,10 +21,10 @@ package umoci
 import (
 	"context"
 	"fmt"
-	"runtime"
 	"time"
 
 	"github.com/apex/log"
+	"github.com/containerd/platforms"
 	imeta "github.com/opencontainers/image-spec/specs-go"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 
@@ -46,12 +46,14 @@ func NewImage(engineExt casext.Engine, tagName string, sourceDateEpoch *time.Tim
 		createTime = *sourceDateEpoch
 	}
 
-	// Set all of the defaults we need.
 	g.SetCreated(createTime)
-	g.SetOS(runtime.GOOS)
-	g.SetArchitecture(runtime.GOARCH)
 	g.ClearHistory()
 
+	hostPlatform := platforms.DefaultSpec()
+	g.SetPlatformOS(hostPlatform.OS)
+	g.SetPlatformArchitecture(hostPlatform.Architecture)
+	g.SetPlatformVariant(hostPlatform.Variant)
+
 	// Make sure we have no diffids.
 	g.SetRootfsType("layers")
 	g.ClearRootfsDiffIDs()

oci/config/convert/runtime.go

@@ -37,12 +37,13 @@ import (
 // in an image configuration that do not have a native representation in the
 // runtime-spec).
 const (
-	osAnnotation           = "org.opencontainers.image.os"
-	archAnnotation         = "org.opencontainers.image.architecture"
-	authorAnnotation       = "org.opencontainers.image.author"
-	createdAnnotation      = "org.opencontainers.image.created"
-	stopSignalAnnotation   = "org.opencontainers.image.stopSignal"
-	exposedPortsAnnotation = "org.opencontainers.image.exposedPorts"
+	platformOsAnnotation      = "org.opencontainers.image.os"
+	platformArchAnnotation    = "org.opencontainers.image.architecture"
+	platformVariantAnnotation = "org.opencontainers.image.variant"
+	authorAnnotation          = "org.opencontainers.image.author"
+	createdAnnotation         = "org.opencontainers.image.created"
+	stopSignalAnnotation      = "org.opencontainers.image.stopSignal"
+	exposedPortsAnnotation    = "org.opencontainers.image.exposedPorts"
 )
 
 // ToRuntimeSpec converts the given OCI image configuration to a runtime
@@ -107,14 +108,16 @@ func allocateNilStruct(spec *rspec.Spec) {
 }
 
 // MutateRuntimeSpec mutates a given runtime configuration with the image
-// configuration provided.
+// configuration provided in accordance with the image specification's
+// conversion mechanism (for more information, see
+// <https://github.com/opencontainers/image-spec/blob/main/conversion.md>).
 func MutateRuntimeSpec(spec *rspec.Spec, rootfs string, image ispec.Image) error {
 	ig, err := igen.NewFromImage(image)
 	if err != nil {
 		return fmt.Errorf("creating image generator: %w", err)
 	}
 
-	if ig.OS() != "linux" {
+	if ig.PlatformOS() != "linux" {
 		return fmt.Errorf("unsupported OS: %s", image.OS)
 	}
 
@@ -164,13 +167,23 @@ func MutateRuntimeSpec(spec *rspec.Spec, rootfs string, image ispec.Image) error
 		spec.Process.Args = args
 	}
 
-	// Set annotations fields
+	// Set the "annotation fields".
+	setAnnotation := func(name, value string) {
+		if value != "" {
+			spec.Annotations[name] = value
+		} else {
+			delete(spec.Annotations, name)
+		}
+	}
+	setAnnotation(platformOsAnnotation, ig.PlatformOS())
+	setAnnotation(platformArchAnnotation, ig.PlatformArchitecture())
+	setAnnotation(platformVariantAnnotation, ig.PlatformVariant())
+	setAnnotation(authorAnnotation, ig.Author())
+	setAnnotation(createdAnnotation, ig.Created().Format(igen.ISO8601))
+	setAnnotation(stopSignalAnnotation, image.Config.StopSignal)
+	setAnnotation(exposedPortsAnnotation, strings.Join(ig.ConfigExposedPorts(), ","))
+	// Config.Labels need to be applied after the auto-applied labels.
 	maps.Copy(spec.Annotations, ig.ConfigLabels())
-	spec.Annotations[osAnnotation] = ig.OS()
-	spec.Annotations[archAnnotation] = ig.Architecture()
-	spec.Annotations[authorAnnotation] = ig.Author()
-	spec.Annotations[createdAnnotation] = ig.Created().Format(igen.ISO8601)
-	spec.Annotations[stopSignalAnnotation] = image.Config.StopSignal
 
 	// Set parsed fields
 	// Get the *actual* uid and gid of the user. If the image doesn't contain
@@ -204,10 +217,6 @@ func MutateRuntimeSpec(spec *rspec.Spec, rootfs string, image ispec.Image) error
 		appendEnv(&spec.Process.Env, "HOME", execUser.Home)
 	}
 
-	// Set optional fields
-	ports := ig.ConfigExposedPorts()
-	spec.Annotations[exposedPortsAnnotation] = strings.Join(ports, ",")
-
 	for _, vol := range ig.ConfigVolumes() {
 		// XXX: This is _fine_ but might cause some issues in the future.
 		spec.Mounts = append(spec.Mounts, rspec.Mount{

oci/config/generate/spec.go

@@ -319,22 +319,32 @@ func (g *Generator) Author() string {
 	return g.image.Author
 }
 
-// SetArchitecture is the CPU architecture which the binaries in this image are built to run on.
-func (g *Generator) SetArchitecture(arch string) {
+// SetPlatformOS sets the name of the operating system which the image is built to run on.
+func (g *Generator) SetPlatformOS(os string) {
+	g.image.OS = os
+}
+
+// PlatformOS returns the name of the operating system which the image is built to run on.
+func (g *Generator) PlatformOS() string {
+	return g.image.OS
+}
+
+// SetPlatformArchitecture is the CPU architecture which the binaries in this image are built to run on.
+func (g *Generator) SetPlatformArchitecture(arch string) {
 	g.image.Architecture = arch
 }
 
-// Architecture returns the CPU architecture which the binaries in this image are built to run on.
-func (g *Generator) Architecture() string {
+// PlatformArchitecture returns the CPU architecture which the binaries in this image are built to run on.
+func (g *Generator) PlatformArchitecture() string {
 	return g.image.Architecture
 }
 
-// SetOS sets the name of the operating system which the image is built to run on.
-func (g *Generator) SetOS(os string) {
-	g.image.OS = os
+// SetPlatformVariant is the CPU architecture variant which the binaries in this image are built to run on.
+func (g *Generator) SetPlatformVariant(variant string) {
+	g.image.Variant = variant
 }
 
-// OS returns the name of the operating system which the image is built to run on.
-func (g *Generator) OS() string {
-	return g.image.OS
+// PlatformVariant returns the CPU architecture variant which the binaries in this image are built to run on.
+func (g *Generator) PlatformVariant() string {
+	return g.image.Variant
 }

oci/config/generate/spec_test.go

@@ -68,24 +68,34 @@ func TestConfigWorkingDir(t *testing.T) {
 	assert.Equal(t, expected, got, "ConfigWorkingDir get/set should match")
 }
 
-func TestArchitecture(t *testing.T) {
+func TestPlatformOS(t *testing.T) {
 	g := New()
 	expected := "some_value"
 
-	g.SetArchitecture(expected)
-	got := g.Architecture()
+	g.SetPlatformOS(expected)
+	got := g.PlatformOS()
 
-	assert.Equal(t, expected, got, "Architecture get/set should match")
+	assert.Equal(t, expected, got, "PlatformOS get/set should match")
 }
 
-func TestOS(t *testing.T) {
+func TestPlatformArchitecture(t *testing.T) {
 	g := New()
 	expected := "some_value"
 
-	g.SetOS(expected)
-	got := g.OS()
+	g.SetPlatformArchitecture(expected)
+	got := g.PlatformArchitecture()
 
-	assert.Equal(t, expected, got, "OS get/set should match")
+	assert.Equal(t, expected, got, "PlatformArchitecture get/set should match")
+}
+
+func TestPlatformVariant(t *testing.T) {
+	g := New()
+	expected := "some_value"
+
+	g.SetPlatformVariant(expected)
+	got := g.PlatformVariant()
+
+	assert.Equal(t, expected, got, "PlatformVariant get/set should match")
 }
 
 func TestAuthor(t *testing.T) {