Merge branch 'pr-167'

  CHANGELOG: mention changes to --[ug]id-map
  repack: change --[ug]id-map= UX to match user_namespaces(7)

LGTMs: @cyphar
Closes #167

CHANGELOG.md

@@ -18,7 +18,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   fixed, and we've added tests to our CI to ensure that something like this
   won't go unnoticed in the future. openSUSE/umoci#157
 
+### Changed
+- `umoci unpack`'s mapping options (`--uid-map` and `--gid-map`) have had an
+  interface change, to better match the [`user_namespaces(7)`][user_namespaces]
+  interfaces. Note that this is a **breaking change**, but the workaround is to
+  switch to the trivially different (but now more consistent) format.
+  openSUSE/umoci#XXX
+
 [cii]: https://bestpractices.coreinfrastructure.org/projects/1084
+[user_namespaces]: http://man7.org/linux/man-pages/man7/user_namespaces.7.html
 
 ## [0.3.0] - 2017-07-20
 ### Added

cmd/umoci/unpack.go

@@ -55,11 +55,11 @@ creation with umoci-repack(1).`,
 	Flags: []cli.Flag{
 		cli.StringSliceFlag{
 			Name:  "uid-map",
-			Usage: "specifies a uid mapping to use when repacking",
+			Usage: "specifies a uid mapping to use when repacking (container:host:size)",
 		},
 		cli.StringSliceFlag{
 			Name:  "gid-map",
-			Usage: "specifies a gid mapping to use when repacking",
+			Usage: "specifies a gid mapping to use when repacking (container:host:size)",
 		},
 		cli.BoolFlag{
 			Name:  "rootless",
@@ -94,10 +94,10 @@ func unpack(ctx *cli.Context) error {
 	meta.MapOptions.Rootless = ctx.Bool("rootless")
 	if meta.MapOptions.Rootless {
 		if !ctx.IsSet("uid-map") {
-			ctx.Set("uid-map", fmt.Sprintf("%d:0:1", os.Geteuid()))
+			ctx.Set("uid-map", fmt.Sprintf("0:%d:1", os.Geteuid()))
 		}
 		if !ctx.IsSet("gid-map") {
-			ctx.Set("gid-map", fmt.Sprintf("%d:0:1", os.Getegid()))
+			ctx.Set("gid-map", fmt.Sprintf("0:%d:1", os.Getegid()))
 		}
 	}
 	// Parse and set up the mapping options.

doc/man/umoci-unpack.1.md

@@ -27,16 +27,18 @@ The global options are defined in **umoci**(1).
 
 **--uid-map**=[*value*]
   Specifies a UID mapping to use while unpacking layers. This is used in a
-  similar fashion to **user_namespaces**(7).
+  similar fashion to **user_namespaces**(7), and is of the form
+  **container:host[:size]**.
 
 **--gid-map**=[*value*]
   Specifies a GID mapping to use while unpacking layers. This is used in a
-  similar fashion to **user_namespaces**(7).
+  similar fashion to **user_namespaces**(7), and is of the form
+  **container:host[:size]**.
 
 **--rootless**
   Enable rootless unpacking support. This allows for **umoci-unpack**(1) and
   **umoci-repack**(1) to be used as an unprivileged user. Use of this flag
-  implies **--uid-map=$(id -u):0:1** and **--gid-map=$(id -g):0:1**, as well as
+  implies **--uid-map=0:$(id -u):1** and **--gid-map=0:$(id -g):1**, as well as
   enabling several features to fake parts of the unpacking in the attempt to
   generate an as-close-as-possible extraction of the filesystem. Note that it
   is almost always not possible to perfectly extract an OCI image with

pkg/idtools/idtools.go

@@ -60,7 +60,7 @@ func ToContainer(hostID int, idMap []rspec.LinuxIDMapping) (int, error) {
 	return -1, errors.Errorf("host id %d cannot be mapped to a container id", hostID)
 }
 
-// ParseMapping takes a mapping string of the form "host:container[:size]" and
+// ParseMapping takes a mapping string of the form "container:host[:size]" and
 // returns the corresponding rspec.LinuxIDMapping. An error is returned if not
 // enough fields are provided or are otherwise invalid. The default size is 1.
 func ParseMapping(spec string) (rspec.LinuxIDMapping, error) {
@@ -80,14 +80,14 @@ func ParseMapping(spec string) (rspec.LinuxIDMapping, error) {
 		return rspec.LinuxIDMapping{}, errors.Errorf("invalid number of fields in mapping '%s': %d", spec, len(parts))
 	}
 
-	hostID, err = strconv.Atoi(parts[0])
+	contID, err = strconv.Atoi(parts[0])
 	if err != nil {
-		return rspec.LinuxIDMapping{}, errors.Wrap(err, "invalid hostID in mapping")
+		return rspec.LinuxIDMapping{}, errors.Wrap(err, "invalid containerID in mapping")
 	}
 
-	contID, err = strconv.Atoi(parts[1])
+	hostID, err = strconv.Atoi(parts[1])
 	if err != nil {
-		return rspec.LinuxIDMapping{}, errors.Wrap(err, "invalid containerID in mapping")
+		return rspec.LinuxIDMapping{}, errors.Wrap(err, "invalid hostID in mapping")
 	}
 
 	return rspec.LinuxIDMapping{

pkg/idtools/idtools_test.go

@@ -297,10 +297,10 @@ func TestParseIDMapping(t *testing.T) {
 		failure               bool
 	}{
 		{spec: "0:0:1", host: 0, container: 0, size: 1, failure: false},
-		{spec: "100:32:2421", host: 100, container: 32, size: 2421, failure: false},
-		{spec: "1337:0:1924", host: 1337, container: 0, size: 1924, failure: false},
-		{spec: "1:2", host: 1, container: 2, size: 1, failure: false},
-		{spec: "123:422", host: 123, container: 422, size: 1, failure: false},
+		{spec: "32:100:2421", host: 100, container: 32, size: 2421, failure: false},
+		{spec: "0:1337:1924", host: 1337, container: 0, size: 1924, failure: false},
+		{spec: "2:1", host: 1, container: 2, size: 1, failure: false},
+		{spec: "422:123", host: 123, container: 422, size: 1, failure: false},
 		{spec: "", host: 0, container: 0, size: 0, failure: true},
 		{spec: "::", host: 0, container: 0, size: 0, failure: true},
 		{spec: "1:2:", host: 0, container: 0, size: 0, failure: true},
@@ -318,13 +318,13 @@ func TestParseIDMapping(t *testing.T) {
 				t.Errorf("unexpected error: %+v", err)
 			} else {
 				if idMap.HostID != test.host {
-					t.Errorf("expected to get host %d, got %d", test.host, idMap.HostID)
+					t.Errorf("%q: expected to get host %d, got %d", test.spec, test.host, idMap.HostID)
 				}
 				if idMap.ContainerID != test.container {
-					t.Errorf("expected to get container %d, got %d", test.container, idMap.HostID)
+					t.Errorf("%q: expected to get container %d, got %d", test.spec, test.container, idMap.HostID)
 				}
 				if idMap.Size != test.size {
-					t.Errorf("expected to get size %d, got %d", test.size, idMap.HostID)
+					t.Errorf("%q: expected to get size %d, got %d", test.spec, test.size, idMap.HostID)
 				}
 			}
 		}

test/pack_mapping.bats

@@ -35,7 +35,7 @@ function teardown() {
 	BUNDLE_B="$(setup_tmpdir)"
 
 	# Unpack the image.
-	umoci unpack --image "${IMAGE}:${TAG}" --uid-map "1337:0:65535" --gid-map "8888:0:65535" "$BUNDLE_A"
+	umoci unpack --image "${IMAGE}:${TAG}" --uid-map "0:1337:65535" --gid-map "0:8888:65535" "$BUNDLE_A"
 	[ "$status" -eq 0 ]
 	bundle-verify "$BUNDLE_A"
 
@@ -53,7 +53,7 @@ function teardown() {
 	}'
 
 	# Unpack the image with a differen uid and gid mapping.
-	umoci unpack --image "${IMAGE}:${TAG}" --uid-map "8080:0:65535" --gid-map "7777:0:65535" "$BUNDLE_B"
+	umoci unpack --image "${IMAGE}:${TAG}" --uid-map "0:8080:65535" --gid-map "0:7777:65535" "$BUNDLE_B"
 	[ "$status" -eq 0 ]
 	bundle-verify "$BUNDLE_B"
 
@@ -73,7 +73,7 @@ function teardown() {
 	image-verify "${IMAGE}"
 }
 
-@test "umoci repack [with unpack --uid-map --gid-map]" {
+@test "umoci repack [--uid-map --gid-map]" {
 	# We do a bunch of remapping tricks, which we can't really do if we're not root.
 	requires root
 
@@ -84,7 +84,7 @@ function teardown() {
 	BUNDLE_C="$(setup_tmpdir)"
 
 	# Unpack the image.
-	umoci unpack --image "${IMAGE}:${TAG}" --uid-map "1337:0:65535" --gid-map "7331:0:65535" "$BUNDLE_A"
+	umoci unpack --image "${IMAGE}:${TAG}" --uid-map "0:1337:65535" --gid-map "0:7331:65535" "$BUNDLE_A"
 	[ "$status" -eq 0 ]
 	bundle-verify "$BUNDLE_A"
 
@@ -101,7 +101,7 @@ function teardown() {
 	image-verify "${IMAGE}"
 
 	# Unpack it again with a different mapping.
-	umoci unpack --image "${IMAGE}:${TAG}-new" --uid-map "4000:0:65535" --gid-map "4000:0:65535" "$BUNDLE_B"
+	umoci unpack --image "${IMAGE}:${TAG}-new" --uid-map "0:4000:65535" --gid-map "0:4000:65535" "$BUNDLE_B"
 	[ "$status" -eq 0 ]
 	bundle-verify "$BUNDLE_B"