From dcf39f0abec6d8925b628904c96fa9d075669e29 Mon Sep 17 00:00:00 2001
From: Valentin Rothberg <rothberg@redhat.com>
Date: Tue, 4 Aug 2020 15:55:53 +0200
Subject: image list: speed up

Listing images has shown increasing performance penalties with an
increasing number of images.  Unless `--all` is specified, Podman
will filter intermediate images.  Determining intermediate images
has been done by finding (and comparing!) parent images which is
expensive.  We had to query the storage many times which turned it
into a bottleneck.

Instead, create a layer tree and assign one or more images to nodes that
match the images' top layer.  Determining the children of an image is
now exponentially faster as we already know the child images from the
layer graph and the images using the same top layer, which may also be
considered child images based on their history.

On my system with 510 images, a rootful image list drops from 6 secs
down to 0.3 secs.

Also use the tree to compute parent nodes, and to filter intermediate
images for pruning.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
---
 libpod/image/filters.go    |  20 ++++
 libpod/image/image.go      | 133 ++-------------------------
 libpod/image/layer_tree.go | 222 +++++++++++++++++++++++++++++++++++++++++++++
 libpod/image/prune.go      |  11 ++-
 4 files changed, 261 insertions(+), 125 deletions(-)
 create mode 100644 libpod/image/layer_tree.go

(limited to 'libpod')

diff --git a/libpod/image/filters.go b/libpod/image/filters.go
index 11d081ec3..24e39a67b 100644
--- a/libpod/image/filters.go
+++ b/libpod/image/filters.go
@@ -29,6 +29,26 @@ func CreatedBeforeFilter(createTime time.Time) ResultFilter {
 	}
 }
 
+// IntermediateFilter returns filter for intermediate images (i.e., images
+// with children and no tags).
+func (ir *Runtime) IntermediateFilter(ctx context.Context, images []*Image) (ResultFilter, error) {
+	tree, err := ir.layerTree()
+	if err != nil {
+		return nil, err
+	}
+	return func(i *Image) bool {
+		if len(i.Names()) > 0 {
+			return true
+		}
+		children, err := tree.children(ctx, i, false)
+		if err != nil {
+			logrus.Error(err.Error())
+			return false
+		}
+		return len(children) == 0
+	}, nil
+}
+
 // CreatedAfterFilter allows you to filter on images created after
 // the given time.Time
 func CreatedAfterFilter(createTime time.Time) ResultFilter {
diff --git a/libpod/image/image.go b/libpod/image/image.go
index 048ec825d..6a87380c0 100644
--- a/libpod/image/image.go
+++ b/libpod/image/image.go
@@ -855,26 +855,6 @@ func (i *Image) Dangling() bool {
 	return len(i.Names()) == 0
 }
 
-// Intermediate returns true if the image is cache or intermediate image.
-// Cache image has parent and child.
-func (i *Image) Intermediate(ctx context.Context) (bool, error) {
-	parent, err := i.IsParent(ctx)
-	if err != nil {
-		return false, err
-	}
-	if !parent {
-		return false, nil
-	}
-	img, err := i.GetParent(ctx)
-	if err != nil {
-		return false, err
-	}
-	if img != nil {
-		return true, nil
-	}
-	return false, nil
-}
-
 // User returns the image's user
 func (i *Image) User(ctx context.Context) (string, error) {
 	imgInspect, err := i.inspect(ctx, false)
@@ -1213,7 +1193,7 @@ func splitString(input string) string {
 // the parent of any other layer in store. Double check that image with that
 // layer exists as well.
 func (i *Image) IsParent(ctx context.Context) (bool, error) {
-	children, err := i.getChildren(ctx, 1)
+	children, err := i.getChildren(ctx, false)
 	if err != nil {
 		if errors.Cause(err) == ErrImageIsBareList {
 			return false, nil
@@ -1288,63 +1268,16 @@ func areParentAndChild(parent, child *imgspecv1.Image) bool {
 
 // GetParent returns the image ID of the parent. Return nil if a parent is not found.
 func (i *Image) GetParent(ctx context.Context) (*Image, error) {
-	var childLayer *storage.Layer
-	images, err := i.imageruntime.GetImages()
+	tree, err := i.imageruntime.layerTree()
 	if err != nil {
 		return nil, err
 	}
-	if i.TopLayer() != "" {
-		if childLayer, err = i.imageruntime.store.Layer(i.TopLayer()); err != nil {
-			return nil, err
-		}
-	}
-	// fetch the configuration for the child image
-	child, err := i.ociv1Image(ctx)
-	if err != nil {
-		if errors.Cause(err) == ErrImageIsBareList {
-			return nil, nil
-		}
-		return nil, err
-	}
-	for _, img := range images {
-		if img.ID() == i.ID() {
-			continue
-		}
-		candidateLayer := img.TopLayer()
-		// as a child, our top layer, if we have one, is either the
-		// candidate parent's layer, or one that's derived from it, so
-		// skip over any candidate image where we know that isn't the
-		// case
-		if childLayer != nil {
-			// The child has at least one layer, so a parent would
-			// have a top layer that's either the same as the child's
-			// top layer or the top layer's recorded parent layer,
-			// which could be an empty value.
-			if candidateLayer != childLayer.Parent && candidateLayer != childLayer.ID {
-				continue
-			}
-		} else {
-			// The child has no layers, but the candidate does.
-			if candidateLayer != "" {
-				continue
-			}
-		}
-		// fetch the configuration for the candidate image
-		candidate, err := img.ociv1Image(ctx)
-		if err != nil {
-			return nil, err
-		}
-		// compare them
-		if areParentAndChild(candidate, child) {
-			return img, nil
-		}
-	}
-	return nil, nil
+	return tree.parent(ctx, i)
 }
 
 // GetChildren returns a list of the imageIDs that depend on the image
 func (i *Image) GetChildren(ctx context.Context) ([]string, error) {
-	children, err := i.getChildren(ctx, 0)
+	children, err := i.getChildren(ctx, true)
 	if err != nil {
 		if errors.Cause(err) == ErrImageIsBareList {
 			return nil, nil
@@ -1354,62 +1287,15 @@ func (i *Image) GetChildren(ctx context.Context) ([]string, error) {
 	return children, nil
 }
 
-// getChildren returns a list of at most "max" imageIDs that depend on the image
-func (i *Image) getChildren(ctx context.Context, max int) ([]string, error) {
-	var children []string
-
-	if _, err := i.toImageRef(ctx); err != nil {
-		return nil, nil
-	}
-
-	images, err := i.imageruntime.GetImages()
-	if err != nil {
-		return nil, err
-	}
-
-	// fetch the configuration for the parent image
-	parent, err := i.ociv1Image(ctx)
+// getChildren returns a list of imageIDs that depend on the image. If all is
+// false, only the first child image is returned.
+func (i *Image) getChildren(ctx context.Context, all bool) ([]string, error) {
+	tree, err := i.imageruntime.layerTree()
 	if err != nil {
 		return nil, err
 	}
-	parentLayer := i.TopLayer()
 
-	for _, img := range images {
-		if img.ID() == i.ID() {
-			continue
-		}
-		if img.TopLayer() == "" {
-			if parentLayer != "" {
-				// this image has no layers, but we do, so
-				// it can't be derived from this one
-				continue
-			}
-		} else {
-			candidateLayer, err := img.Layer()
-			if err != nil {
-				return nil, err
-			}
-			// if this image's top layer is not our top layer, and is not
-			// based on our top layer, we can skip it
-			if candidateLayer.Parent != parentLayer && candidateLayer.ID != parentLayer {
-				continue
-			}
-		}
-		// fetch the configuration for the candidate image
-		candidate, err := img.ociv1Image(ctx)
-		if err != nil {
-			return nil, err
-		}
-		// compare them
-		if areParentAndChild(parent, candidate) {
-			children = append(children, img.ID())
-		}
-		// if we're not building an exhaustive list, maybe we're done?
-		if max > 0 && len(children) >= max {
-			break
-		}
-	}
-	return children, nil
+	return tree.children(ctx, i, all)
 }
 
 // InputIsID returns a bool if the user input for an image
@@ -1608,6 +1494,7 @@ type LayerInfo struct {
 
 // GetLayersMapWithImageInfo returns map of image-layers, with associated information like RepoTags, parent and list of child layers.
 func GetLayersMapWithImageInfo(imageruntime *Runtime) (map[string]*LayerInfo, error) {
+	// TODO: evaluate if we can reuse `layerTree` here.
 
 	// Memory allocated to store map of layers with key LayerID.
 	// Map will build dependency chain with ParentID and ChildID(s)
diff --git a/libpod/image/layer_tree.go b/libpod/image/layer_tree.go
new file mode 100644
index 000000000..3699655fd
--- /dev/null
+++ b/libpod/image/layer_tree.go
@@ -0,0 +1,222 @@
+package image
+
+import (
+	"context"
+
+	ociv1 "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/pkg/errors"
+)
+
+// layerTree is an internal representation of local layers.
+type layerTree struct {
+	// nodes is the actual layer tree with layer IDs being keys.
+	nodes map[string]*layerNode
+	// ociCache is a cache for Image.ID -> OCI Image. Translations are done
+	// on-demand.
+	ociCache map[string]*ociv1.Image
+}
+
+// node returns a layerNode for the specified layerID.
+func (t *layerTree) node(layerID string) *layerNode {
+	node, exists := t.nodes[layerID]
+	if !exists {
+		node = &layerNode{}
+		t.nodes[layerID] = node
+	}
+	return node
+}
+
+// toOCI returns an OCI image for the specified image.
+func (t *layerTree) toOCI(ctx context.Context, i *Image) (*ociv1.Image, error) {
+	var err error
+	oci, exists := t.ociCache[i.ID()]
+	if !exists {
+		oci, err = i.ociv1Image(ctx)
+		t.ociCache[i.ID()] = oci
+	}
+	return oci, err
+}
+
+// layerNode is a node in a layerTree.  It's ID is the key in a layerTree.
+type layerNode struct {
+	children []*layerNode
+	images   []*Image
+	parent   *layerNode
+}
+
+// layerTree extracts a layerTree from the layers in the local storage and
+// relates them to the specified images.
+func (ir *Runtime) layerTree() (*layerTree, error) {
+	layers, err := ir.store.Layers()
+	if err != nil {
+		return nil, err
+	}
+
+	images, err := ir.GetImages()
+	if err != nil {
+		return nil, err
+	}
+
+	tree := layerTree{
+		nodes:    make(map[string]*layerNode),
+		ociCache: make(map[string]*ociv1.Image),
+	}
+
+	// First build a tree purely based on layer information.
+	for _, layer := range layers {
+		node := tree.node(layer.ID)
+		if layer.Parent == "" {
+			continue
+		}
+		parent := tree.node(layer.Parent)
+		node.parent = parent
+		parent.children = append(parent.children, node)
+	}
+
+	// Now assign the images to each (top) layer.
+	for i := range images {
+		img := images[i] // do not leak loop variable outside the scope
+		topLayer := img.TopLayer()
+		if topLayer == "" {
+			continue
+		}
+		node, exists := tree.nodes[topLayer]
+		if !exists {
+			return nil, errors.Errorf("top layer %s of image %s not found in layer tree", img.TopLayer(), img.ID())
+		}
+		node.images = append(node.images, img)
+	}
+
+	return &tree, nil
+}
+
+// children returns the image IDs of children . Child images are images
+// with either the same top layer as parent or parent being the true parent
+// layer.  Furthermore, the history of the parent and child images must match
+// with the parent having one history item less.
+// If all is true, all images are returned.  Otherwise, the first image is
+// returned.
+func (t *layerTree) children(ctx context.Context, parent *Image, all bool) ([]string, error) {
+	if parent.TopLayer() == "" {
+		return nil, nil
+	}
+
+	var children []string
+
+	parentNode, exists := t.nodes[parent.TopLayer()]
+	if !exists {
+		return nil, errors.Errorf("layer not found in layer tree: %q", parent.TopLayer())
+	}
+
+	parentID := parent.ID()
+	parentOCI, err := t.toOCI(ctx, parent)
+	if err != nil {
+		return nil, err
+	}
+
+	// checkParent returns true if child and parent are in such a relation.
+	checkParent := func(child *Image) (bool, error) {
+		if parentID == child.ID() {
+			return false, nil
+		}
+		childOCI, err := t.toOCI(ctx, child)
+		if err != nil {
+			return false, err
+		}
+		// History check.
+		return areParentAndChild(parentOCI, childOCI), nil
+	}
+
+	// addChildrenFrom adds child images of parent to children.  Returns
+	// true if any image is a child of parent.
+	addChildrenFromNode := func(node *layerNode) (bool, error) {
+		foundChildren := false
+		for _, childImage := range node.images {
+			isChild, err := checkParent(childImage)
+			if err != nil {
+				return foundChildren, err
+			}
+			if isChild {
+				foundChildren = true
+				children = append(children, childImage.ID())
+				if all {
+					return foundChildren, nil
+				}
+			}
+		}
+		return foundChildren, nil
+	}
+
+	// First check images where parent's top layer is also the parent
+	// layer.
+	for _, childNode := range parentNode.children {
+		found, err := addChildrenFromNode(childNode)
+		if err != nil {
+			return nil, err
+		}
+		if found && all {
+			return children, nil
+		}
+	}
+
+	// Now check images with the same top layer.
+	if _, err := addChildrenFromNode(parentNode); err != nil {
+		return nil, err
+	}
+
+	return children, nil
+}
+
+// parent returns the parent image or nil if no parent image could be found.
+func (t *layerTree) parent(ctx context.Context, child *Image) (*Image, error) {
+	if child.TopLayer() == "" {
+		return nil, nil
+	}
+
+	node, exists := t.nodes[child.TopLayer()]
+	if !exists {
+		return nil, errors.Errorf("layer not found in layer tree: %q", child.TopLayer())
+	}
+
+	childOCI, err := t.toOCI(ctx, child)
+	if err != nil {
+		return nil, err
+	}
+
+	// Check images from the parent node (i.e., parent layer) and images
+	// with the same layer (i.e., same top layer).
+	childID := child.ID()
+	images := node.images
+	if node.parent != nil {
+		images = append(images, node.parent.images...)
+	}
+	for _, parent := range images {
+		if parent.ID() == childID {
+			continue
+		}
+		parentOCI, err := t.toOCI(ctx, parent)
+		if err != nil {
+			return nil, err
+		}
+		// History check.
+		if areParentAndChild(parentOCI, childOCI) {
+			return parent, nil
+		}
+	}
+
+	return nil, nil
+}
+
+// hasChildrenAndParent returns true if the specified image has children and a
+// parent.
+func (t *layerTree) hasChildrenAndParent(ctx context.Context, i *Image) (bool, error) {
+	children, err := t.children(ctx, i, false)
+	if err != nil {
+		return false, err
+	}
+	if len(children) == 0 {
+		return false, nil
+	}
+	parent, err := t.parent(ctx, i)
+	return parent != nil, err
+}
diff --git a/libpod/image/prune.go b/libpod/image/prune.go
index 5ad7a9a5e..2448f7c87 100644
--- a/libpod/image/prune.go
+++ b/libpod/image/prune.go
@@ -66,6 +66,12 @@ func (ir *Runtime) GetPruneImages(ctx context.Context, all bool, filterFuncs []I
 	if err != nil {
 		return nil, err
 	}
+
+	tree, err := ir.layerTree()
+	if err != nil {
+		return nil, err
+	}
+
 	for _, i := range allImages {
 		// filter the images based on this.
 		for _, filterFunc := range filterFuncs {
@@ -85,8 +91,9 @@ func (ir *Runtime) GetPruneImages(ctx context.Context, all bool, filterFuncs []I
 			}
 		}
 
-		//skip the cache or intermediate images
-		intermediate, err := i.Intermediate(ctx)
+		// skip the cache (i.e., with parent) and intermediate (i.e.,
+		// with children) images
+		intermediate, err := tree.hasChildrenAndParent(ctx, i)
 		if err != nil {
 			return nil, err
 		}
-- 
cgit v1.2.3-54-g00ecf


From d9c86fd0f2ea2d2587d470327192bd3d33875b2d Mon Sep 17 00:00:00 2001
From: Paul Holzinger <paul.holzinger@web.de>
Date: Wed, 5 Aug 2020 14:12:55 +0200
Subject: fix podman logs --tail when log is bigger than pagesize

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
---
 libpod/logs/log.go                         | 12 ++++++------
 libpod/logs/reversereader/reversereader.go |  4 ++--
 test/e2e/logs_test.go                      |  8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

(limited to 'libpod')

diff --git a/libpod/logs/log.go b/libpod/logs/log.go
index 03acadb18..f66cbbadd 100644
--- a/libpod/logs/log.go
+++ b/libpod/logs/log.go
@@ -101,11 +101,14 @@ func getTailLog(path string, tail int) ([]*LogLine, error) {
 			if err != nil {
 				if errors.Cause(err) == io.EOF {
 					inputs <- []string{leftover}
-					close(inputs)
-					break
+				} else {
+					logrus.Error(err)
 				}
-				logrus.Error(err)
 				close(inputs)
+				if err := f.Close(); err != nil {
+					logrus.Error(err)
+				}
+				break
 			}
 			line := strings.Split(s+leftover, "\n")
 			if len(line) > 1 {
@@ -136,9 +139,6 @@ func getTailLog(path string, tail int) ([]*LogLine, error) {
 		}
 		// if we have enough loglines, we can hangup
 		if nllCounter >= tail {
-			if err := f.Close(); err != nil {
-				logrus.Error(err)
-			}
 			break
 		}
 	}
diff --git a/libpod/logs/reversereader/reversereader.go b/libpod/logs/reversereader/reversereader.go
index 72d9ad975..4fa1a3f88 100644
--- a/libpod/logs/reversereader/reversereader.go
+++ b/libpod/logs/reversereader/reversereader.go
@@ -60,7 +60,7 @@ func (r *ReverseReader) Read() (string, error) {
 	if int64(n) < r.readSize {
 		b = b[0:n]
 	}
-	// Set to the next page boundary
-	r.offset = -r.readSize
+	// Move the offset one pagesize up
+	r.offset -= r.readSize
 	return string(b), nil
 }
diff --git a/test/e2e/logs_test.go b/test/e2e/logs_test.go
index 298174a63..20af3daa3 100644
--- a/test/e2e/logs_test.go
+++ b/test/e2e/logs_test.go
@@ -72,16 +72,16 @@ var _ = Describe("Podman logs", func() {
 		Expect(len(results.OutputToStringArray())).To(Equal(0))
 	})
 
-	It("tail 99 lines", func() {
-		logc := podmanTest.Podman([]string{"run", "-dt", ALPINE, "sh", "-c", "echo podman; echo podman; echo podman"})
+	It("tail 800 lines", func() {
+		logc := podmanTest.Podman([]string{"run", "-dt", ALPINE, "sh", "-c", "i=1; while [ \"$i\" -ne 1000 ]; do echo \"line $i\"; i=$((i + 1)); done"})
 		logc.WaitWithDefaultTimeout()
 		Expect(logc).To(Exit(0))
 		cid := logc.OutputToString()
 
-		results := podmanTest.Podman([]string{"logs", "--tail", "99", cid})
+		results := podmanTest.Podman([]string{"logs", "--tail", "800", cid})
 		results.WaitWithDefaultTimeout()
 		Expect(results).To(Exit(0))
-		Expect(len(results.OutputToStringArray())).To(Equal(3))
+		Expect(len(results.OutputToStringArray())).To(Equal(800))
 	})
 
 	It("tail 2 lines with timestamps", func() {
-- 
cgit v1.2.3-54-g00ecf


From 729e0aa0d1fa00dc6452897dc57ccccaf04fc20c Mon Sep 17 00:00:00 2001
From: Matthew Heon <matthew.heon@pm.me>
Date: Wed, 5 Aug 2020 13:48:01 -0400
Subject: Ensure that exec errors write exit codes to the DB

In local Podman, the frontend interprets the error and exit code
given by the Exec API to determine the appropriate exit code to
set for Podman itself; special cases like a missing executable
receive special exit codes.

Exec for the remote API, however, has to do this inside Libpod
itself, as Libpod will be directly queried (via the Inspect API
for exec sessions) to get the exit code. This was done correctly
when the exec session started properly, but we did not properly
handle cases where the OCI runtime fails before the exec session
can properly start. Making two error returns that would otherwise
not set exit code actually do so should resolve the issue.

Fixes #6893

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
---
 libpod/container_exec.go  | 14 ++++++++++++++
 test/system/075-exec.bats |  9 +++++++++
 2 files changed, 23 insertions(+)

(limited to 'libpod')

diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index bd04ee9b9..23bf71d9e 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -415,6 +415,13 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, httpCon net.Conn, h
 
 	execOpts, err := prepareForExec(c, session)
 	if err != nil {
+		session.State = define.ExecStateStopped
+		session.ExitCode = define.ExecErrorCodeGeneric
+
+		if err := c.save(); err != nil {
+			logrus.Errorf("Error saving container %s exec session %s after failure to prepare: %v", err, c.ID(), session.ID())
+		}
+
 		return err
 	}
 
@@ -427,6 +434,13 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, httpCon net.Conn, h
 
 	pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, httpCon, httpBuf, streams, cancel)
 	if err != nil {
+		session.State = define.ExecStateStopped
+		session.ExitCode = define.TranslateExecErrorToExitCode(define.ExecErrorCodeGeneric, err)
+
+		if err := c.save(); err != nil {
+			logrus.Errorf("Error saving container %s exec session %s after failure to start: %v", err, c.ID(), session.ID())
+		}
+
 		return err
 	}
 
diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats
index f8c7f2766..019217d8f 100644
--- a/test/system/075-exec.bats
+++ b/test/system/075-exec.bats
@@ -21,6 +21,15 @@ load helpers
     run_podman exec $cid sh -c "cat /$rand_filename"
     is "$output" "$rand_content" "Can exec and see file in running container"
 
+
+    # Specially defined situations: exec a dir, or no such command.
+    # We don't check the full error message because runc & crun differ.
+    run_podman 126 exec $cid /etc
+    is "$output" ".*permission denied"  "podman exec /etc"
+    run_podman 127 exec $cid /no/such/command
+    is "$output" ".*such file or dir"   "podman exec /no/such/command"
+
+    # Done
     run_podman exec $cid rm -f /$rand_filename
 
     run_podman wait $cid
-- 
cgit v1.2.3-54-g00ecf


From 8931e99a18cc168ebee07bb72b62849815af7434 Mon Sep 17 00:00:00 2001
From: zhangguanzhang <zhangguanzhang@qq.com>
Date: Sun, 2 Aug 2020 21:08:13 +0800
Subject: API returns 500 in case network is not found instead of 404

Backported-by: Valentin Rothberg <rothberg@redhat.com>
Signed-off-by: zhangguanzhang <zhangguanzhang@qq.com>
---
 libpod/define/errors.go             | 3 +++
 pkg/api/handlers/compat/networks.go | 7 +++----
 pkg/api/handlers/libpod/networks.go | 6 +++---
 pkg/api/handlers/utils/errors.go    | 9 +++++++++
 pkg/network/config.go               | 5 -----
 pkg/network/files.go                | 3 ++-
 pkg/network/network.go              | 3 ++-
 test/apiv2/35-networks.at           | 8 ++++++++
 8 files changed, 30 insertions(+), 14 deletions(-)
 create mode 100644 test/apiv2/35-networks.at

(limited to 'libpod')

diff --git a/libpod/define/errors.go b/libpod/define/errors.go
index 1e9179353..5cadce396 100644
--- a/libpod/define/errors.go
+++ b/libpod/define/errors.go
@@ -23,6 +23,9 @@ var (
 	// ErrNoSuchVolume indicates the requested volume does not exist
 	ErrNoSuchVolume = errors.New("no such volume")
 
+	// ErrNoSuchNetwork indicates the requested network does not exist
+	ErrNoSuchNetwork = errors.New("network not found")
+
 	// ErrNoSuchExecSession indicates that the requested exec session does
 	// not exist.
 	ErrNoSuchExecSession = errors.New("no such exec session")
diff --git a/pkg/api/handlers/compat/networks.go b/pkg/api/handlers/compat/networks.go
index 2e11c0edb..ad15be270 100644
--- a/pkg/api/handlers/compat/networks.go
+++ b/pkg/api/handlers/compat/networks.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/containernetworking/cni/libcni"
 	"github.com/containers/libpod/v2/libpod"
+	"github.com/containers/libpod/v2/libpod/define"
 	"github.com/containers/libpod/v2/pkg/api/handlers/utils"
 	"github.com/containers/libpod/v2/pkg/domain/entities"
 	"github.com/containers/libpod/v2/pkg/domain/infra/abi"
@@ -44,9 +45,7 @@ func InspectNetwork(w http.ResponseWriter, r *http.Request) {
 	name := utils.GetName(r)
 	_, err = network.InspectNetwork(config, name)
 	if err != nil {
-		// TODO our network package does not distinguish between not finding a
-		// specific network vs not being able to read it
-		utils.InternalServerError(w, err)
+		utils.NetworkNotFound(w, name, err)
 		return
 	}
 	report, err := getNetworkResourceByName(name, runtime)
@@ -285,7 +284,7 @@ func RemoveNetwork(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if !exists {
-		utils.Error(w, "network not found", http.StatusNotFound, network.ErrNetworkNotFound)
+		utils.Error(w, "network not found", http.StatusNotFound, define.ErrNoSuchNetwork)
 		return
 	}
 	if err := network.RemoveNetwork(config, name); err != nil {
diff --git a/pkg/api/handlers/libpod/networks.go b/pkg/api/handlers/libpod/networks.go
index 12409bf50..0164d5d13 100644
--- a/pkg/api/handlers/libpod/networks.go
+++ b/pkg/api/handlers/libpod/networks.go
@@ -5,10 +5,10 @@ import (
 	"net/http"
 
 	"github.com/containers/libpod/v2/libpod"
+	"github.com/containers/libpod/v2/libpod/define"
 	"github.com/containers/libpod/v2/pkg/api/handlers/utils"
 	"github.com/containers/libpod/v2/pkg/domain/entities"
 	"github.com/containers/libpod/v2/pkg/domain/infra/abi"
-	"github.com/containers/libpod/v2/pkg/network"
 	"github.com/gorilla/schema"
 	"github.com/pkg/errors"
 )
@@ -78,7 +78,7 @@ func RemoveNetwork(w http.ResponseWriter, r *http.Request) {
 	}
 	if reports[0].Err != nil {
 		// If the network cannot be found, we return a 404.
-		if errors.Cause(err) == network.ErrNetworkNotFound {
+		if errors.Cause(err) == define.ErrNoSuchNetwork {
 			utils.Error(w, "Something went wrong", http.StatusNotFound, err)
 			return
 		}
@@ -104,7 +104,7 @@ func InspectNetwork(w http.ResponseWriter, r *http.Request) {
 	reports, err := ic.NetworkInspect(r.Context(), []string{name}, options)
 	if err != nil {
 		// If the network cannot be found, we return a 404.
-		if errors.Cause(err) == network.ErrNetworkNotFound {
+		if errors.Cause(err) == define.ErrNoSuchNetwork {
 			utils.Error(w, "Something went wrong", http.StatusNotFound, err)
 			return
 		}
diff --git a/pkg/api/handlers/utils/errors.go b/pkg/api/handlers/utils/errors.go
index 00d09ac11..5e77b4049 100644
--- a/pkg/api/handlers/utils/errors.go
+++ b/pkg/api/handlers/utils/errors.go
@@ -39,6 +39,7 @@ func VolumeNotFound(w http.ResponseWriter, name string, err error) {
 	msg := fmt.Sprintf("No such volume: %s", name)
 	Error(w, msg, http.StatusNotFound, err)
 }
+
 func ContainerNotFound(w http.ResponseWriter, name string, err error) {
 	if errors.Cause(err) != define.ErrNoSuchCtr {
 		InternalServerError(w, err)
@@ -55,6 +56,14 @@ func ImageNotFound(w http.ResponseWriter, name string, err error) {
 	Error(w, msg, http.StatusNotFound, err)
 }
 
+func NetworkNotFound(w http.ResponseWriter, name string, err error) {
+	if errors.Cause(err) != define.ErrNoSuchNetwork {
+		InternalServerError(w, err)
+	}
+	msg := fmt.Sprintf("No such network: %s", name)
+	Error(w, msg, http.StatusNotFound, err)
+}
+
 func PodNotFound(w http.ResponseWriter, name string, err error) {
 	if errors.Cause(err) != define.ErrNoSuchPod {
 		InternalServerError(w, err)
diff --git a/pkg/network/config.go b/pkg/network/config.go
index a504e0ad0..0115433e1 100644
--- a/pkg/network/config.go
+++ b/pkg/network/config.go
@@ -2,7 +2,6 @@ package network
 
 import (
 	"encoding/json"
-	"errors"
 	"net"
 )
 
@@ -20,10 +19,6 @@ const (
 	DefaultPodmanDomainName = "dns.podman"
 )
 
-var (
-	ErrNetworkNotFound = errors.New("network not found")
-)
-
 // GetDefaultPodmanNetwork outputs the default network for podman
 func GetDefaultPodmanNetwork() (*net.IPNet, error) {
 	_, n, err := net.ParseCIDR("10.88.1.0/24")
diff --git a/pkg/network/files.go b/pkg/network/files.go
index beb3289f3..f174a762c 100644
--- a/pkg/network/files.go
+++ b/pkg/network/files.go
@@ -10,6 +10,7 @@ import (
 	"github.com/containernetworking/cni/libcni"
 	"github.com/containernetworking/plugins/plugins/ipam/host-local/backend/allocator"
 	"github.com/containers/common/pkg/config"
+	"github.com/containers/libpod/v2/libpod/define"
 	"github.com/pkg/errors"
 )
 
@@ -55,7 +56,7 @@ func GetCNIConfigPathByName(config *config.Config, name string) (string, error)
 			return confFile, nil
 		}
 	}
-	return "", errors.Wrap(ErrNetworkNotFound, fmt.Sprintf("unable to find network configuration for %s", name))
+	return "", errors.Wrap(define.ErrNoSuchNetwork, fmt.Sprintf("unable to find network configuration for %s", name))
 }
 
 // ReadRawCNIConfByName reads the raw CNI configuration for a CNI
diff --git a/pkg/network/network.go b/pkg/network/network.go
index cbebb0be8..37f3f721a 100644
--- a/pkg/network/network.go
+++ b/pkg/network/network.go
@@ -8,6 +8,7 @@ import (
 	"github.com/containernetworking/cni/pkg/types"
 	"github.com/containernetworking/plugins/plugins/ipam/host-local/backend/allocator"
 	"github.com/containers/common/pkg/config"
+	"github.com/containers/libpod/v2/libpod/define"
 	"github.com/containers/libpod/v2/pkg/util"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -200,7 +201,7 @@ func InspectNetwork(config *config.Config, name string) (map[string]interface{},
 func Exists(config *config.Config, name string) (bool, error) {
 	_, err := ReadRawCNIConfByName(config, name)
 	if err != nil {
-		if errors.Cause(err) == ErrNetworkNotFound {
+		if errors.Cause(err) == define.ErrNoSuchNetwork {
 			return false, nil
 		}
 		return false, err
diff --git a/test/apiv2/35-networks.at b/test/apiv2/35-networks.at
new file mode 100644
index 000000000..fff3f3b1f
--- /dev/null
+++ b/test/apiv2/35-networks.at
@@ -0,0 +1,8 @@
+# -*- sh -*-
+#
+# network-related tests
+#
+
+t GET /networks/non-existing-network 404
+
+# vim: filetype=sh
-- 
cgit v1.2.3-54-g00ecf


From 92d01d76c63d0a286365f610151524fb24b74e16 Mon Sep 17 00:00:00 2001
From: Qi Wang <qiwan@redhat.com>
Date: Fri, 31 Jul 2020 08:26:50 -0400
Subject: Fix close fds of exec --preserve-fds

Fix the closing of fds from --preserve-fds to avoid the operation on unrelated fds.

Signed-off-by: Qi Wang <qiwan@redhat.com>
---
 libpod/oci_conmon_exec_linux.go | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

(limited to 'libpod')

diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index 1a6fe827b..9fe761638 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -449,9 +449,12 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 		return nil, nil, err
 	}
 
+	var filesToClose []*os.File
 	if options.PreserveFDs > 0 {
 		for fd := 3; fd < int(3+options.PreserveFDs); fd++ {
-			execCmd.ExtraFiles = append(execCmd.ExtraFiles, os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd)))
+			f := os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd))
+			filesToClose = append(filesToClose, f)
+			execCmd.ExtraFiles = append(execCmd.ExtraFiles, f)
 		}
 	}
 
@@ -483,14 +486,10 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 		return nil, nil, err
 	}
 
-	if options.PreserveFDs > 0 {
-		for fd := 3; fd < int(3+options.PreserveFDs); fd++ {
-			// These fds were passed down to the runtime.  Close them
-			// and not interfere
-			if err := os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd)).Close(); err != nil {
-				logrus.Debugf("unable to close file fd-%d", fd)
-			}
-		}
+	// These fds were passed down to the runtime.  Close them
+	// and not interfere
+	for _, f := range filesToClose {
+		errorhandling.CloseQuiet(f)
 	}
 
 	return execCmd, pipes, nil
-- 
cgit v1.2.3-54-g00ecf


From 3dfd8630a51a37734ad8c51162c4d004b8ffffb2 Mon Sep 17 00:00:00 2001
From: Daniel J Walsh <dwalsh@redhat.com>
Date: Tue, 30 Jun 2020 15:44:14 -0400
Subject: Add username to /etc/passwd inside of container if --userns keep-id

If I enter a continer with --userns keep-id, my UID will be present
inside of the container, but most likely my user will not be defined.

This patch will take information about the user and stick it into the
container.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
---
 cmd/podman/common/specgen.go            |  2 ++
 libpod/container.go                     |  8 ++++-
 libpod/container_internal_linux.go      | 58 ++++++++++++++++++++++++++++-----
 libpod/container_internal_linux_test.go | 42 ++++++++++++++++++++++++
 libpod/options.go                       | 14 ++++++++
 pkg/specgen/generate/namespaces.go      |  4 ++-
 test/e2e/run_userns_test.go             | 10 ++++++
 7 files changed, 128 insertions(+), 10 deletions(-)
 create mode 100644 libpod/container_internal_linux_test.go

(limited to 'libpod')

diff --git a/cmd/podman/common/specgen.go b/cmd/podman/common/specgen.go
index 7716fc150..2333f2f7e 100644
--- a/cmd/podman/common/specgen.go
+++ b/cmd/podman/common/specgen.go
@@ -260,6 +260,8 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *ContainerCLIOpts, args []string
 	// If some mappings are specified, assume a private user namespace
 	if userNS.IsDefaultValue() && (!s.IDMappings.HostUIDMapping || !s.IDMappings.HostGIDMapping) {
 		s.UserNS.NSMode = specgen.Private
+	} else {
+		s.UserNS.NSMode = specgen.NamespaceMode(userNS)
 	}
 
 	s.Terminal = c.TTY
diff --git a/libpod/container.go b/libpod/container.go
index 6a87ad3a9..9ad938a5c 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -277,6 +277,9 @@ type ContainerConfig struct {
 	User string `json:"user,omitempty"`
 	// Additional groups to add
 	Groups []string `json:"groups,omitempty"`
+	// AddCurrentUserPasswdEntry indicates that the current user passwd entry
+	// should be added to the /etc/passwd within the container
+	AddCurrentUserPasswdEntry bool `json:"addCurrentUserPasswdEntry,omitempty"`
 
 	// Namespace Config
 	// IDs of container to share namespaces with
@@ -774,7 +777,10 @@ func (c *Container) Hostname() string {
 
 // WorkingDir returns the containers working dir
 func (c *Container) WorkingDir() string {
-	return c.config.Spec.Process.Cwd
+	if c.config.Spec.Process != nil {
+		return c.config.Spec.Process.Cwd
+	}
+	return "/"
 }
 
 // State Accessors
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index cfcf9b823..f86903fd1 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"net"
 	"os"
+	"os/user"
 	"path"
 	"path/filepath"
 	"strconv"
@@ -33,7 +34,7 @@ import (
 	"github.com/containers/libpod/v2/pkg/util"
 	"github.com/containers/storage/pkg/archive"
 	securejoin "github.com/cyphar/filepath-securejoin"
-	"github.com/opencontainers/runc/libcontainer/user"
+	User "github.com/opencontainers/runc/libcontainer/user"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -1420,9 +1421,23 @@ func (c *Container) getHosts() string {
 	return hosts
 }
 
-// generatePasswd generates a container specific passwd file,
-// iff g.config.User is a number
-func (c *Container) generatePasswd() (string, error) {
+// generateCurrentUserPasswdEntry generates an /etc/passwd entry for the user
+// running the container engine
+func (c *Container) generateCurrentUserPasswdEntry() (string, error) {
+	uid := rootless.GetRootlessUID()
+	if uid == 0 {
+		return "", nil
+	}
+	u, err := user.LookupId(strconv.Itoa(rootless.GetRootlessUID()))
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to get current user")
+	}
+	return fmt.Sprintf("%s:x:%s:%s:%s:%s:/bin/sh\n", u.Username, u.Uid, u.Gid, u.Username, c.WorkingDir()), nil
+}
+
+// generateUserPasswdEntry generates an /etc/passwd entry for the container user
+// to run in the container.
+func (c *Container) generateUserPasswdEntry() (string, error) {
 	var (
 		groupspec string
 		gid       int
@@ -1440,14 +1455,16 @@ func (c *Container) generatePasswd() (string, error) {
 	if err != nil {
 		return "", nil
 	}
+
 	// Lookup the user to see if it exists in the container image
 	_, err = lookup.GetUser(c.state.Mountpoint, userspec)
-	if err != nil && err != user.ErrNoPasswdEntries {
+	if err != nil && err != User.ErrNoPasswdEntries {
 		return "", err
 	}
 	if err == nil {
 		return "", nil
 	}
+
 	if groupspec != "" {
 		ugid, err := strconv.ParseUint(groupspec, 10, 32)
 		if err == nil {
@@ -1460,14 +1477,39 @@ func (c *Container) generatePasswd() (string, error) {
 			gid = group.Gid
 		}
 	}
+	return fmt.Sprintf("%d:x:%d:%d:container user:%s:/bin/sh\n", uid, uid, gid, c.WorkingDir()), nil
+}
+
+// generatePasswd generates a container specific passwd file,
+// iff g.config.User is a number
+func (c *Container) generatePasswd() (string, error) {
+	if !c.config.AddCurrentUserPasswdEntry && c.config.User == "" {
+		return "", nil
+	}
+	pwd := ""
+	if c.config.User != "" {
+		entry, err := c.generateUserPasswdEntry()
+		if err != nil {
+			return "", err
+		}
+		pwd += entry
+	}
+	if c.config.AddCurrentUserPasswdEntry {
+		entry, err := c.generateCurrentUserPasswdEntry()
+		if err != nil {
+			return "", err
+		}
+		pwd += entry
+	}
+	if pwd == "" {
+		return "", nil
+	}
 	originPasswdFile := filepath.Join(c.state.Mountpoint, "/etc/passwd")
 	orig, err := ioutil.ReadFile(originPasswdFile)
 	if err != nil && !os.IsNotExist(err) {
 		return "", errors.Wrapf(err, "unable to read passwd file %s", originPasswdFile)
 	}
-
-	pwd := fmt.Sprintf("%s%d:x:%d:%d:container user:%s:/bin/sh\n", orig, uid, uid, gid, c.WorkingDir())
-	passwdFile, err := c.writeStringToRundir("passwd", pwd)
+	passwdFile, err := c.writeStringToRundir("passwd", string(orig)+pwd)
 	if err != nil {
 		return "", errors.Wrapf(err, "failed to create temporary passwd file")
 	}
diff --git a/libpod/container_internal_linux_test.go b/libpod/container_internal_linux_test.go
new file mode 100644
index 000000000..078cc53a7
--- /dev/null
+++ b/libpod/container_internal_linux_test.go
@@ -0,0 +1,42 @@
+// +build linux
+
+package libpod
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+
+	spec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGenerateUserPasswdEntry(t *testing.T) {
+	dir, err := ioutil.TempDir("", "libpod_test_")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+
+	c := Container{
+		config: &ContainerConfig{
+			User: "123:456",
+			Spec: &spec.Spec{},
+		},
+		state: &ContainerState{
+			Mountpoint: "/does/not/exist/tmp/",
+		},
+	}
+	user, err := c.generateUserPasswdEntry()
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, user, "123:x:123:456:container user:/:/bin/sh\n")
+
+	c.config.User = "567"
+	user, err = c.generateUserPasswdEntry()
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, user, "567:x:567:0:container user:/:/bin/sh\n")
+}
diff --git a/libpod/options.go b/libpod/options.go
index bff3f3c18..560b406e2 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -844,6 +844,20 @@ func WithPIDNSFrom(nsCtr *Container) CtrCreateOption {
 	}
 }
 
+// WithAddCurrentUserPasswdEntry indicates that container should add current
+// user entry to /etc/passwd, since the UID will be mapped into the container,
+// via user namespace
+func WithAddCurrentUserPasswdEntry() CtrCreateOption {
+	return func(ctr *Container) error {
+		if ctr.valid {
+			return define.ErrCtrFinalized
+		}
+
+		ctr.config.AddCurrentUserPasswdEntry = true
+		return nil
+	}
+}
+
 // WithUserNSFrom indicates the the container should join the user namespace of
 // the given container.
 // If the container has joined a pod, it can only join the namespaces of
diff --git a/pkg/specgen/generate/namespaces.go b/pkg/specgen/generate/namespaces.go
index 39a45398d..22670ca61 100644
--- a/pkg/specgen/generate/namespaces.go
+++ b/pkg/specgen/generate/namespaces.go
@@ -153,7 +153,9 @@ func namespaceOptions(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.
 	// User
 	switch s.UserNS.NSMode {
 	case specgen.KeepID:
-		if !rootless.IsRootless() {
+		if rootless.IsRootless() {
+			toReturn = append(toReturn, libpod.WithAddCurrentUserPasswdEntry())
+		} else {
 			// keep-id as root doesn't need a user namespace
 			s.UserNS.NSMode = specgen.Host
 		}
diff --git a/test/e2e/run_userns_test.go b/test/e2e/run_userns_test.go
index c0d98f7b1..24d3e42eb 100644
--- a/test/e2e/run_userns_test.go
+++ b/test/e2e/run_userns_test.go
@@ -89,6 +89,16 @@ var _ = Describe("Podman UserNS support", func() {
 		Expect(ok).To(BeTrue())
 	})
 
+	It("podman --userns=keep-id check passwd", func() {
+		session := podmanTest.Podman([]string{"run", "--userns=keep-id", "alpine", "id", "-un"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		u, err := user.Current()
+		Expect(err).To(BeNil())
+		ok, _ := session.GrepString(u.Name)
+		Expect(ok).To(BeTrue())
+	})
+
 	It("podman --userns=keep-id root owns /usr", func() {
 		session := podmanTest.Podman([]string{"run", "--userns=keep-id", "alpine", "stat", "-c%u", "/usr"})
 		session.WaitWithDefaultTimeout()
-- 
cgit v1.2.3-54-g00ecf


From ca00601b14f2253e5b9f89280d39c5d498efc9f3 Mon Sep 17 00:00:00 2001
From: Matthew Heon <matthew.heon@pm.me>
Date: Wed, 15 Jul 2020 15:25:12 -0400
Subject: Make changes to /etc/passwd on disk for non-read only

Bind-mounting /etc/passwd into the container is problematic
becuase of how system utilities like `useradd` work. They want
to make a copy and then rename to try to prevent breakage; this
is, unfortunately, impossible when the file they want to rename
is a bind mount. The current behavior is fine for read-only
containers, though, because we expect useradd to fail in those
cases.

Instead of bind-mounting, we can edit /etc/passwd in the
container's rootfs. This is kind of gross, because the change
will show up in `podman diff` and similar tools, and will be
included in images made by `podman commit`. However, it's a lot
better than breaking important system tools.

Fixes #6953

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
---
 libpod/container_internal_linux.go | 48 ++++++++++++++++++++++++++++++++++----
 test/e2e/run_passwd_test.go        |  8 +++----
 test/e2e/run_userns_test.go        | 25 ++++++++++++++++++++
 3 files changed, 72 insertions(+), 9 deletions(-)

(limited to 'libpod')

diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index f86903fd1..d72e3ad47 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -1428,11 +1428,26 @@ func (c *Container) generateCurrentUserPasswdEntry() (string, error) {
 	if uid == 0 {
 		return "", nil
 	}
+
 	u, err := user.LookupId(strconv.Itoa(rootless.GetRootlessUID()))
 	if err != nil {
 		return "", errors.Wrapf(err, "failed to get current user")
 	}
-	return fmt.Sprintf("%s:x:%s:%s:%s:%s:/bin/sh\n", u.Username, u.Uid, u.Gid, u.Username, c.WorkingDir()), nil
+
+	// Lookup the user to see if it exists in the container image.
+	_, err = lookup.GetUser(c.state.Mountpoint, u.Username)
+	if err != User.ErrNoPasswdEntries {
+		return "", err
+	}
+
+	// If the user's actual home directory exists, or was mounted in - use
+	// that.
+	homeDir := c.WorkingDir()
+	if MountExists(c.config.Spec.Mounts, u.HomeDir) {
+		homeDir = u.HomeDir
+	}
+
+	return fmt.Sprintf("%s:x:%s:%s:%s:%s:/bin/sh\n", u.Username, u.Uid, u.Gid, u.Username, homeDir), nil
 }
 
 // generateUserPasswdEntry generates an /etc/passwd entry for the container user
@@ -1458,12 +1473,9 @@ func (c *Container) generateUserPasswdEntry() (string, error) {
 
 	// Lookup the user to see if it exists in the container image
 	_, err = lookup.GetUser(c.state.Mountpoint, userspec)
-	if err != nil && err != User.ErrNoPasswdEntries {
+	if err != User.ErrNoPasswdEntries {
 		return "", err
 	}
-	if err == nil {
-		return "", nil
-	}
 
 	if groupspec != "" {
 		ugid, err := strconv.ParseUint(groupspec, 10, 32)
@@ -1504,6 +1516,32 @@ func (c *Container) generatePasswd() (string, error) {
 	if pwd == "" {
 		return "", nil
 	}
+
+	// If we are *not* read-only - edit /etc/passwd in the container.
+	// This is *gross* (shows up in changes to the container, will be
+	// committed to images based on the container) but it actually allows us
+	// to add users to the container (a bind mount breaks useradd).
+	// We should never get here twice, because generateUserPasswdEntry will
+	// not return anything if the user already exists in /etc/passwd.
+	if !c.IsReadOnly() {
+		containerPasswd, err := securejoin.SecureJoin(c.state.Mountpoint, "/etc/passwd")
+		if err != nil {
+			return "", errors.Wrapf(err, "error looking up location of container %s /etc/passwd", c.ID())
+		}
+
+		f, err := os.OpenFile(containerPasswd, os.O_APPEND|os.O_WRONLY, 0600)
+		if err != nil {
+			return "", errors.Wrapf(err, "error opening container %s /etc/passwd", c.ID())
+		}
+		defer f.Close()
+
+		if _, err := f.WriteString(pwd); err != nil {
+			return "", errors.Wrapf(err, "unable to append to container %s /etc/passwd", c.ID())
+		}
+
+		return "", nil
+	}
+
 	originPasswdFile := filepath.Join(c.state.Mountpoint, "/etc/passwd")
 	orig, err := ioutil.ReadFile(originPasswdFile)
 	if err != nil && !os.IsNotExist(err) {
diff --git a/test/e2e/run_passwd_test.go b/test/e2e/run_passwd_test.go
index d7f3233ef..6caf8a094 100644
--- a/test/e2e/run_passwd_test.go
+++ b/test/e2e/run_passwd_test.go
@@ -35,27 +35,27 @@ var _ = Describe("Podman run passwd", func() {
 	})
 
 	It("podman run no user specified ", func() {
-		session := podmanTest.Podman([]string{"run", BB, "mount"})
+		session := podmanTest.Podman([]string{"run", "--read-only", BB, "mount"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		Expect(session.LineInOutputContains("passwd")).To(BeFalse())
 	})
 	It("podman run user specified in container", func() {
-		session := podmanTest.Podman([]string{"run", "-u", "bin", BB, "mount"})
+		session := podmanTest.Podman([]string{"run", "--read-only", "-u", "bin", BB, "mount"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		Expect(session.LineInOutputContains("passwd")).To(BeFalse())
 	})
 
 	It("podman run UID specified in container", func() {
-		session := podmanTest.Podman([]string{"run", "-u", "2:1", BB, "mount"})
+		session := podmanTest.Podman([]string{"run", "--read-only", "-u", "2:1", BB, "mount"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		Expect(session.LineInOutputContains("passwd")).To(BeFalse())
 	})
 
 	It("podman run UID not specified in container", func() {
-		session := podmanTest.Podman([]string{"run", "-u", "20001:1", BB, "mount"})
+		session := podmanTest.Podman([]string{"run", "--read-only", "-u", "20001:1", BB, "mount"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		Expect(session.LineInOutputContains("passwd")).To(BeTrue())
diff --git a/test/e2e/run_userns_test.go b/test/e2e/run_userns_test.go
index 24d3e42eb..fef125e60 100644
--- a/test/e2e/run_userns_test.go
+++ b/test/e2e/run_userns_test.go
@@ -113,6 +113,31 @@ var _ = Describe("Podman UserNS support", func() {
 		Expect(session.OutputToString()).To(Equal("0"))
 	})
 
+	It("podman run --userns=keep-id can add users", func() {
+		if os.Geteuid() == 0 {
+			Skip("Test only runs without root")
+		}
+
+		userName := os.Getenv("USER")
+		if userName == "" {
+			Skip("Can't complete test if no username available")
+		}
+
+		ctrName := "ctr-name"
+		session := podmanTest.Podman([]string{"run", "--userns=keep-id", "--user", "root:root", "-d", "--stop-signal", "9", "--name", ctrName, fedoraMinimal, "sleep", "600"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		exec1 := podmanTest.Podman([]string{"exec", "-t", "-i", ctrName, "cat", "/etc/passwd"})
+		exec1.WaitWithDefaultTimeout()
+		Expect(exec1.ExitCode()).To(Equal(0))
+		Expect(exec1.OutputToString()).To(ContainSubstring(userName))
+
+		exec2 := podmanTest.Podman([]string{"exec", "-t", "-i", ctrName, "useradd", "testuser"})
+		exec2.WaitWithDefaultTimeout()
+		Expect(exec2.ExitCode()).To(Equal(0))
+	})
+
 	It("podman --userns=auto", func() {
 		u, err := user.Current()
 		Expect(err).To(BeNil())
-- 
cgit v1.2.3-54-g00ecf