From c72bb4f8feb05a63c189fe6ce264ca652b399022 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 10 Aug 2020 15:19:02 +0200 Subject: [PATCH 01/25] generate systemd: fix error handling Fix a bug in the error handling which returned nil instead of an error and ultimately lead to nil dereferences in the client. To prevent future regressions, add a test and check for the error message. Fixes: #7271 Signed-off-by: Valentin Rothberg --- pkg/domain/infra/abi/generate.go | 5 +++-- test/e2e/generate_systemd_test.go | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/domain/infra/abi/generate.go b/pkg/domain/infra/abi/generate.go index 560be988b0..cff09bf2d7 100644 --- a/pkg/domain/infra/abi/generate.go +++ b/pkg/domain/infra/abi/generate.go @@ -20,9 +20,10 @@ func (ic *ContainerEngine) GenerateSystemd(ctx context.Context, nameOrID string, if ctrErr == nil { // Generate the unit for the container. s, err := generate.ContainerUnit(ctr, options) - if err == nil { - return &entities.GenerateSystemdReport{Output: s}, nil + if err != nil { + return nil, err } + return &entities.GenerateSystemdReport{Output: s}, nil } // If it's not a container, we either have a pod or garbage. diff --git a/test/e2e/generate_systemd_test.go b/test/e2e/generate_systemd_test.go index 654e995312..151d3f6fc4 100644 --- a/test/e2e/generate_systemd_test.go +++ b/test/e2e/generate_systemd_test.go @@ -53,6 +53,18 @@ var _ = Describe("Podman generate systemd", func() { Expect(session).To(ExitWithError()) }) + It("podman generate systemd bad restart-policy value", func() { + session := podmanTest.Podman([]string{"create", "--name", "foobar", "alpine", "top"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"generate", "systemd", "--restart-policy", "bogus", "foobar"}) + session.WaitWithDefaultTimeout() + Expect(session).To(ExitWithError()) + found, _ := session.ErrorGrepString("Error: bogus is not a valid restart policy") + Expect(found).Should(BeTrue()) + }) + It("podman generate systemd good timeout value", func() { session := podmanTest.Podman([]string{"create", "--name", "foobar", "alpine", "top"}) session.WaitWithDefaultTimeout() From dcf39f0abec6d8925b628904c96fa9d075669e29 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 4 Aug 2020 15:55:53 +0200 Subject: [PATCH 02/25] 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 --- libpod/image/filters.go | 20 +++ libpod/image/image.go | 133 ++--------------- libpod/image/layer_tree.go | 222 ++++++++++++++++++++++++++++ libpod/image/prune.go | 11 +- pkg/api/handlers/utils/images.go | 20 +-- pkg/domain/infra/abi/images_list.go | 17 +-- 6 files changed, 276 insertions(+), 147 deletions(-) create mode 100644 libpod/image/layer_tree.go diff --git a/libpod/image/filters.go b/libpod/image/filters.go index 11d081ec37..24e39a67ba 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 048ec825d4..6a87380c01 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 0000000000..3699655fde --- /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 5ad7a9a5e1..2448f7c877 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 } diff --git a/pkg/api/handlers/utils/images.go b/pkg/api/handlers/utils/images.go index 195e71b750..63b1b566b6 100644 --- a/pkg/api/handlers/utils/images.go +++ b/pkg/api/handlers/utils/images.go @@ -93,20 +93,14 @@ func GetImages(w http.ResponseWriter, r *http.Request) ([]*image.Image, error) { if query.All { return images, nil } - returnImages := []*image.Image{} - for _, img := range images { - if len(img.Names()) == 0 { - parent, err := img.IsParent(r.Context()) - if err != nil { - return nil, err - } - if parent { - continue - } - } - returnImages = append(returnImages, img) + + filter, err := runtime.ImageRuntime().IntermediateFilter(r.Context(), images) + if err != nil { + return nil, err } - return returnImages, nil + images = image.FilterImages(images, []image.ResultFilter{filter}) + + return images, nil } func GetImage(r *http.Request, name string) (*image.Image, error) { diff --git a/pkg/domain/infra/abi/images_list.go b/pkg/domain/infra/abi/images_list.go index 06ef673c7a..bb5775db56 100644 --- a/pkg/domain/infra/abi/images_list.go +++ b/pkg/domain/infra/abi/images_list.go @@ -13,6 +13,14 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions) return nil, err } + if !opts.All { + filter, err := ir.Libpod.ImageRuntime().IntermediateFilter(ctx, images) + if err != nil { + return nil, err + } + images = libpodImage.FilterImages(images, []libpodImage.ResultFilter{filter}) + } + summaries := []*entities.ImageSummary{} for _, img := range images { var repoTags []string @@ -32,15 +40,6 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions) if err != nil { return nil, err } - if len(img.Names()) == 0 { - parent, err := img.IsParent(ctx) - if err != nil { - return nil, err - } - if parent { - continue - } - } } digests := make([]string, len(img.Digests())) From d9c86fd0f2ea2d2587d470327192bd3d33875b2d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 5 Aug 2020 14:12:55 +0200 Subject: [PATCH 03/25] fix podman logs --tail when log is bigger than pagesize Signed-off-by: Paul Holzinger --- libpod/logs/log.go | 12 ++++++------ libpod/logs/reversereader/reversereader.go | 4 ++-- test/e2e/logs_test.go | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/libpod/logs/log.go b/libpod/logs/log.go index 03acadb187..f66cbbaddf 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 72d9ad9753..4fa1a3f88a 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 298174a63f..20af3daa37 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() { From 729e0aa0d1fa00dc6452897dc57ccccaf04fc20c Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 5 Aug 2020 13:48:01 -0400 Subject: [PATCH 04/25] 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 --- libpod/container_exec.go | 14 ++++++++++++++ test/system/075-exec.bats | 9 +++++++++ 2 files changed, 23 insertions(+) diff --git a/libpod/container_exec.go b/libpod/container_exec.go index bd04ee9b9a..23bf71d9e1 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 f8c7f27669..019217d8f2 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 From c5646acf4a06520f4fa65acdf8eebb4130c5e7d9 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Tue, 4 Aug 2020 12:52:45 -0500 Subject: [PATCH 05/25] podman-remote send name and tag when loading an image with podman-remote load, we need to send a name and a tag to the endpoint Fixes: #7124 Backported-by: Valentin Rothberg Signed-off-by: Brent Baude --- pkg/domain/infra/tunnel/images.go | 6 +++++- test/system/120-load.bats | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go index bfe5fbec3f..2e30621c5c 100644 --- a/pkg/domain/infra/tunnel/images.go +++ b/pkg/domain/infra/tunnel/images.go @@ -188,7 +188,11 @@ func (ir *ImageEngine) Load(ctx context.Context, opts entities.ImageLoadOptions) return nil, err } defer f.Close() - return images.Load(ir.ClientCxt, f, &opts.Name) + ref := opts.Name + if len(opts.Tag) > 0 { + ref += ":" + opts.Tag + } + return images.Load(ir.ClientCxt, f, &ref) } func (ir *ImageEngine) Import(ctx context.Context, opts entities.ImageImportOptions) (*entities.ImageImportReport, error) { diff --git a/test/system/120-load.bats b/test/system/120-load.bats index f290c18881..c0ddbf4d62 100644 --- a/test/system/120-load.bats +++ b/test/system/120-load.bats @@ -74,7 +74,7 @@ verify_iid_and_name() { verify_iid_and_name $img_name } -@test "podman load - NAME and NAME:TAG arguments work (requires: #2674)" { +@test "podman load - NAME and NAME:TAG arguments work" { get_iid_and_name run_podman save $iid -o $archive run_podman rmi $iid From a53b97beb3dfd3b95b205e35872344c5fadbb7c1 Mon Sep 17 00:00:00 2001 From: Qi Wang Date: Fri, 24 Jul 2020 14:09:12 -0400 Subject: [PATCH 06/25] fix bug podman sign storage path - fix the bud podman not using specified --directory as signature storage. - use manifest and image referce to set repo@digest. close #6994 close #6993 Backported-by: Valentin Rothberg Signed-off-by: Qi Wang --- go.mod | 5 +- go.sum | 8 +-- pkg/domain/infra/abi/images.go | 65 ++++++++----------- pkg/trust/trust.go | 4 +- test/e2e/image_sign_test.go | 62 ++++++++++++++++++ test/e2e/sign/secret-key.asc | 57 ++++++++++++++++ .../apimachinery/pkg/apis/meta/v1/types.go | 3 + .../k8s.io/apimachinery/pkg/util/net/http.go | 2 +- vendor/modules.txt | 4 +- 9 files changed, 161 insertions(+), 49 deletions(-) create mode 100644 test/e2e/image_sign_test.go create mode 100644 test/e2e/sign/secret-key.asc diff --git a/go.mod b/go.mod index 227979afbc..492b46032a 100644 --- a/go.mod +++ b/go.mod @@ -63,8 +63,7 @@ require ( golang.org/x/net v0.0.0-20200520004742-59133d7f0dd7 golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a golang.org/x/sys v0.0.0-20200519105757-fe76b779f299 - gopkg.in/yaml.v2 v2.3.0 - k8s.io/api v0.18.4 - k8s.io/apimachinery v0.18.4 + k8s.io/api v0.18.6 + k8s.io/apimachinery v0.18.6 k8s.io/client-go v0.0.0-20190620085101-78d2af792bab ) diff --git a/go.sum b/go.sum index 32601c7a6e..3bb51cab8d 100644 --- a/go.sum +++ b/go.sum @@ -630,11 +630,11 @@ gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= k8s.io/api v0.0.0-20190620084959-7cf5895f2711/go.mod h1:TBhBqb1AWbBQbW3XRusr7n7E4v2+5ZY8r8sAMnyFC5A= -k8s.io/api v0.18.4 h1:8x49nBRxuXGUlDlwlWd3RMY1SayZrzFfxea3UZSkFw4= -k8s.io/api v0.18.4/go.mod h1:lOIQAKYgai1+vz9J7YcDZwC26Z0zQewYOGWdyIPUUQ4= +k8s.io/api v0.18.6 h1:osqrAXbOQjkKIWDTjrqxWQ3w0GkKb1KA1XkUGHHYpeE= +k8s.io/api v0.18.6/go.mod h1:eeyxr+cwCjMdLAmr2W3RyDI0VvTawSg/3RFFBEnmZGI= k8s.io/apimachinery v0.0.0-20190612205821-1799e75a0719/go.mod h1:I4A+glKBHiTgiEjQiCCQfCAIcIMFGt291SmsvcrFzJA= -k8s.io/apimachinery v0.18.4 h1:ST2beySjhqwJoIFk6p7Hp5v5O0hYY6Gngq/gUYXTPIA= -k8s.io/apimachinery v0.18.4/go.mod h1:OaXp26zu/5J7p0f92ASynJa1pZo06YlV9fG7BoWbCko= +k8s.io/apimachinery v0.18.6 h1:RtFHnfGNfd1N0LeSrKCUznz5xtUP1elRGvHJbL3Ntag= +k8s.io/apimachinery v0.18.6/go.mod h1:OaXp26zu/5J7p0f92ASynJa1pZo06YlV9fG7BoWbCko= k8s.io/client-go v0.0.0-20190620085101-78d2af792bab h1:E8Fecph0qbNsAbijJJQryKu4Oi9QTp5cVpjTE+nqg6g= k8s.io/client-go v0.0.0-20190620085101-78d2af792bab/go.mod h1:E95RaSlHr79aHaX0aGSwcPNfygDiPKOVXdmivCIZT0k= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go index 9f594d7284..5f19f416ac 100644 --- a/pkg/domain/infra/abi/images.go +++ b/pkg/domain/infra/abi/images.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "net/url" "os" + "path" "path/filepath" "strconv" "strings" @@ -564,10 +565,6 @@ func (ir *ImageEngine) Shutdown(_ context.Context) { } func (ir *ImageEngine) Sign(ctx context.Context, names []string, options entities.SignOptions) (*entities.SignReport, error) { - dockerRegistryOptions := image.DockerRegistryOptions{ - DockerCertPath: options.CertDir, - } - mech, err := signature.NewGPGSigningMechanism() if err != nil { return nil, errors.Wrap(err, "error initializing GPG") @@ -586,7 +583,6 @@ func (ir *ImageEngine) Sign(ctx context.Context, names []string, options entitie } for _, signimage := range names { - var sigStoreDir string srcRef, err := alltransports.ParseImageName(signimage) if err != nil { return nil, errors.Wrapf(err, "error parsing image name") @@ -607,40 +603,38 @@ func (ir *ImageEngine) Sign(ctx context.Context, names []string, options entitie if dockerReference == nil { return nil, errors.Errorf("cannot determine canonical Docker reference for destination %s", transports.ImageName(rawSource.Reference())) } - - // create the signstore file - rtc, err := ir.Libpod.GetConfig() - if err != nil { - return nil, err - } - newImage, err := ir.Libpod.ImageRuntime().New(ctx, signimage, rtc.Engine.SignaturePolicyPath, "", os.Stderr, &dockerRegistryOptions, image.SigningOptions{SignBy: options.SignBy}, nil, util.PullImageMissing) - if err != nil { - return nil, errors.Wrapf(err, "error pulling image %s", signimage) + var sigStoreDir string + if options.Directory != "" { + sigStoreDir = options.Directory } if sigStoreDir == "" { if rootless.IsRootless() { sigStoreDir = filepath.Join(filepath.Dir(ir.Libpod.StorageConfig().GraphRoot), "sigstore") } else { + var sigStoreURI string registryInfo := trust.HaveMatchRegistry(rawSource.Reference().DockerReference().String(), registryConfigs) if registryInfo != nil { - if sigStoreDir = registryInfo.SigStoreStaging; sigStoreDir == "" { - sigStoreDir = registryInfo.SigStore - + if sigStoreURI = registryInfo.SigStoreStaging; sigStoreURI == "" { + sigStoreURI = registryInfo.SigStore } } + if sigStoreURI == "" { + return nil, errors.Errorf("no signature storage configuration found for %s", rawSource.Reference().DockerReference().String()) + + } + sigStoreDir, err = localPathFromURI(sigStoreURI) + if err != nil { + return nil, errors.Wrapf(err, "invalid signature storage %s", sigStoreURI) + } } } - sigStoreDir, err = isValidSigStoreDir(sigStoreDir) + manifestDigest, err := manifest.Digest(getManifest) if err != nil { - return nil, errors.Wrapf(err, "invalid signature storage %s", sigStoreDir) - } - repos, err := newImage.RepoDigests() - if err != nil { - return nil, errors.Wrapf(err, "error calculating repo digests for %s", signimage) + return nil, err } - if len(repos) == 0 { - logrus.Errorf("no repodigests associated with the image %s", signimage) - continue + repo := reference.Path(dockerReference) + if path.Clean(repo) != repo { // Coverage: This should not be reachable because /./ and /../ components are not valid in docker references + return nil, errors.Errorf("Unexpected path elements in Docker reference %s for signature storage", dockerReference.String()) } // create signature @@ -648,22 +642,21 @@ func (ir *ImageEngine) Sign(ctx context.Context, names []string, options entitie if err != nil { return nil, errors.Wrapf(err, "error creating new signature") } - - trimmedDigest := strings.TrimPrefix(repos[0], strings.Split(repos[0], "/")[0]) - sigStoreDir = filepath.Join(sigStoreDir, strings.Replace(trimmedDigest, ":", "=", 1)) - if err := os.MkdirAll(sigStoreDir, 0751); err != nil { + // create the signstore file + signatureDir := fmt.Sprintf("%s@%s=%s", filepath.Join(sigStoreDir, repo), manifestDigest.Algorithm(), manifestDigest.Hex()) + if err := os.MkdirAll(signatureDir, 0751); err != nil { // The directory is allowed to exist if !os.IsExist(err) { - logrus.Errorf("error creating directory %s: %s", sigStoreDir, err) + logrus.Errorf("error creating directory %s: %s", signatureDir, err) continue } } - sigFilename, err := getSigFilename(sigStoreDir) + sigFilename, err := getSigFilename(signatureDir) if err != nil { logrus.Errorf("error creating sigstore file: %v", err) continue } - err = ioutil.WriteFile(filepath.Join(sigStoreDir, sigFilename), newSig, 0644) + err = ioutil.WriteFile(filepath.Join(signatureDir, sigFilename), newSig, 0644) if err != nil { logrus.Errorf("error storing signature for %s", rawSource.Reference().DockerReference().String()) continue @@ -691,14 +684,12 @@ func getSigFilename(sigStoreDirPath string) (string, error) { } } -func isValidSigStoreDir(sigStoreDir string) (string, error) { - writeURIs := map[string]bool{"file": true} +func localPathFromURI(sigStoreDir string) (string, error) { url, err := url.Parse(sigStoreDir) if err != nil { return sigStoreDir, errors.Wrapf(err, "invalid directory %s", sigStoreDir) } - _, exists := writeURIs[url.Scheme] - if !exists { + if url.Scheme != "file" { return sigStoreDir, errors.Errorf("writing to %s is not supported. Use a supported scheme", sigStoreDir) } sigStoreDir = url.Path diff --git a/pkg/trust/trust.go b/pkg/trust/trust.go index 60de099fae..2348bc4101 100644 --- a/pkg/trust/trust.go +++ b/pkg/trust/trust.go @@ -12,9 +12,9 @@ import ( "strings" "github.com/containers/image/v5/types" + "github.com/ghodss/yaml" "github.com/pkg/errors" "github.com/sirupsen/logrus" - "gopkg.in/yaml.v2" ) // PolicyContent struct for policy.json file @@ -157,7 +157,7 @@ func HaveMatchRegistry(key string, registryConfigs *RegistryConfiguration) *Regi searchKey = searchKey[:strings.LastIndex(searchKey, "/")] } } - return nil + return registryConfigs.DefaultDocker } // CreateTmpFile creates a temp file under dir and writes the content into it diff --git a/test/e2e/image_sign_test.go b/test/e2e/image_sign_test.go new file mode 100644 index 0000000000..d5d460b387 --- /dev/null +++ b/test/e2e/image_sign_test.go @@ -0,0 +1,62 @@ +// +build !remote + +package integration + +import ( + "os" + "os/exec" + "path/filepath" + + . "github.com/containers/libpod/v2/test/utils" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("Podman image sign", func() { + var ( + origGNUPGHOME string + tempdir string + err error + podmanTest *PodmanTestIntegration + ) + + BeforeEach(func() { + tempdir, err = CreateTempDirInTempDir() + if err != nil { + os.Exit(1) + } + podmanTest = PodmanTestCreate(tempdir) + podmanTest.Setup() + podmanTest.SeedImages() + + tempGNUPGHOME := filepath.Join(podmanTest.TempDir, "tmpGPG") + err := os.Mkdir(tempGNUPGHOME, os.ModePerm) + Expect(err).To(BeNil()) + + origGNUPGHOME = os.Getenv("GNUPGHOME") + err = os.Setenv("GNUPGHOME", tempGNUPGHOME) + Expect(err).To(BeNil()) + + }) + + AfterEach(func() { + podmanTest.Cleanup() + f := CurrentGinkgoTestDescription() + processTestResult(f) + os.Setenv("GNUPGHOME", origGNUPGHOME) + }) + + It("podman sign image", func() { + cmd := exec.Command("gpg", "--import", "sign/secret-key.asc") + err := cmd.Run() + Expect(err).To(BeNil()) + sigDir := filepath.Join(podmanTest.TempDir, "test-sign") + err = os.MkdirAll(sigDir, os.ModePerm) + Expect(err).To(BeNil()) + session := podmanTest.Podman([]string{"image", "sign", "--directory", sigDir, "--sign-by", "foo@bar.com", "docker://library/alpine"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + _, err = os.Stat(filepath.Join(sigDir, "library")) + Expect(err).To(BeNil()) + }) +}) diff --git a/test/e2e/sign/secret-key.asc b/test/e2e/sign/secret-key.asc new file mode 100644 index 0000000000..23c0d05c32 --- /dev/null +++ b/test/e2e/sign/secret-key.asc @@ -0,0 +1,57 @@ +-----BEGIN PGP PRIVATE KEY BLOCK----- + +lQOYBF8kNqwBCAC0x3Kog+WlDNwcR6rWIP8Gj2T6LrQ2/3knSyAWzTgC/OBB6Oh0 +KAokXLjy8J3diG3EaSltE7erGG/bZCz8jYvMiwDJScON4zzidotqjoY80E+NeRDg +CC0gqvqmh0ftJIjYNBHzSxqrGRQwzwZU+u6ezlE8+0dvsHcHY+MRnxXJQrdM07EP +Prp85kKckChDlJ1tyGUB/YHieFQmOW5+TERA7ZqQOAQ12Vviv6V4kNfEJJq3MS2c +csZpO323tcHt3oebqsZCIElhX7uVw6GAeCw1tm4NZXs4g1yIC21Of/hzPeC18F72 +splCgKaAOiE9w/nMGLNEYy2NzgEclZLs2Y7jABEBAAEAB/9VOcwHvvrWN3xThsP2 +5CJmxNZtjfQfE4zZ5fRwW3pjCjVtTTC9hhzV7LKysZYzGPzqwksp5chKjKA7VXxR +6ic0nHmX68MaEr2i5BExAJUveWNvxloazC/+PS0ishdKKNWs28t0n/0oGZAnvIn3 +KT+ytYCeF7ajZJWQ8dncdlvuf86I8GdmqP2Og9A67LUpJfH2rtpBjzH25nLSZ3Qz +QbHoUIv318Wwb1sIidkmPsZufZG3pMsYjtFtJjkWt0lRsJQnSE9AQOpQTkLsVsh2 +FYQZ2ODhH8+NE86UNAAr2JiMZHoTrEUL2SLwpXEthFIR78N009dOS4nw8CLB61BL +pr6lBADH6yoF95rI0LR3jphfr7e8K3BViTPK97wke6EqwTlVo0TCdR785sa8T8O8 +HvlYx4a+h3e6D4D0vjDPzxtevjdTjYmxqI3cwE2N3NFALgGBwHs01qRRIw4qxZlC +1L1byJ8rUVi5z3YMO7X4RcXSAtg3fM2fx2x+sdpyH30jafuKPwQA533PVRo/Rlgt +pOak9Fs+3KOIb+oO8ypy7RRQcnsTKajts0sUE9scBhT4tSDeSa/HaDvzLiE8KKCK +3rhn8ZKLTW1fvKNZBj6oGlIRyfOFjtN/jMRjo0WVHSUDJ59Zr8C0khpP5J73yhTr +fDhcuTPWiCjlDYeSFHV/a4Z45GG2Kl0EAL1I31kxnSQR9bN5ZmvV+aOhTRKOuHDm +6nISF/XnVwuGHCvMbFRKsTxGkGrPO5VQZflFOqVab9umIQkOIcrzeKj+slYlm5VA +zKfCQ1vZ2f74QYCNP8oeRa1r3D46fszcElZJQxtZZewYRKX63bvU4F+hql8dJTqe +e3wVq8QD657yRwC0FGZvb2JhciA8Zm9vQGJhci5jb20+iQFUBBMBCAA+FiEERyT4 +ac7LLibByeabqaoHAy6P2bIFAl8kNqwCGwMFCQPCZwAFCwkIBwIGFQoJCAsCBBYC +AwECHgECF4AACgkQqaoHAy6P2bKtuggAgv54/F8wgi+uMrtFr8rqNtZMDyXRxfXa +XUy5uGNfqHD83yqxweEqxiA8lmFkRHixPWtgZ2MniFXMVc9kVmg8GNIIuzewXrPq +tXztvuURQo9phK68v8fXEqqT6K25wtq8TiQZ0J3mQIJPPTMe3pCCOyR6+W3iMtQp +2AmitxKbzLP3J3GG2i0rG5S147A2rPnzTeMYhds819+JE7jNMD7FkV+TcQlOVl4w +yOQhNEJcjb6rA6EUe5+s85pIFTBSyPMJpJ03Y0dLdcSGpKdncGTK2X9+hS96G1+F +P/t8hRIDblqUHtBRXe3Ozz6zSqpqu1DbAQSMbIrLYxXfnZEN+ro0dJ0DmARfJDas +AQgAncvLLZUHZkJWDPka3ocysJ7+/lmrXyAjT3D4r7UM4oaLBOMKjvaKSDw1uW5q +YmTxnnsqFDI0O5+XJxD1/0qEf6l2oUpnILdxVruf28FuvymbsyhDgs+MBoHz0jLW +WPHUW2oWLIqcvaF0BePQ1GS6UoZlmZejsLwwcSpbaAHJng7An/iLuqOBr5EdUA5X +MXqmdMFDrjh0uZezImJ2Eacu/hshBdu3IY49J5XP18GWrSdUnP27cv3tOii9j5Lf +l8QAvCN89vkALIU3eZtnMlWZqLgl5o6COVFmzpyx+iHOoCznQBt0aGoSNmE/dAqW +IQS/xCSFqMHI6kNd9N0oR0rEHwARAQABAAf+L3mAhBLJ2qDLrfSGenv3qr7zXggR +cLnFFeIZ2BdjLIYpLku2wgN34DrJOSR4umi/bxyEMPZX07Z0rgrC0E+VpKkSKX2u +oF/AqEUj1+SPEtGMaC8NfL4/1Tdk6ZFk/vanGufEixsbBEyekSUVD8nMawbHa5n9 +ZC+CbZG+VYDwLW6u0Pb26CIhqpFNQL3E88uLeVNhnE+nNJfgB2Nyo8gUQszovUxk +hv64UlXYA3wt49mpc9ORs9qKMZkuKdJfYmJkmvqLE35YpRRz6i1+hg71doj7Sjel +naBV3qIrIbcN6I2/9ZUwVwCzttpeHDfKOxQk5szWFop6H79TZGRsrV6boQQAwnFO +v5pjsZLhqHIZPty3zXz7Tv3LmaatwA260n6NcLBuQFiuEoh2QsMfVtLU8bBzyuC8 +Znx3kPlGCCognSjkEis+aEjsZgvCzR3aP+FWejkhnZnFiSJDvgEftODLF3gSPVp3 +dhc6q5GLysc0iN/gkBZN8Qm1lL/kEyeri4mbWT8EAM/AczrXL0tPEV3YzyeyM972 +HP9OnIYoyIkCa4M0PA0qhUPJ+vBHl/1+p5WZD/eokXqJ2M8IqNSlinuou3azbg+r +N3xTaB0a+Vx6O/RRI73+4UK2fyN9gYRH437eliNBRTkZeZCQ6Dd5eYcABaL2DbSs +1dyGXzRWfzdvGVu/r/0hBACER5u/uac+y9sXr79imoLVya25XkjvswGrDxmrlmNg +cfn/bix9Z93TXScYPiyxzLwlDDd7ovlpv1+mbHNgj6krSGG+R+uLQ2+nm+9glMmz +KupEYF59lzOgEYScJaHQWBULPRGUy/7HmZGpsDmz8zpj8lHaFNLlqDzrxw3MNKxO +F0NFiQE8BBgBCAAmFiEERyT4ac7LLibByeabqaoHAy6P2bIFAl8kNqwCGwwFCQPC +ZwAACgkQqaoHAy6P2bJfjQgAje6YR+p1QaNlTN9l4t2kGzy9RhkfYMrTgI2fEqbS +9bFJUy3Y3mH+vj/r2gN/kaN8LHH4K1d7fAohBsFqSI0flzHHIx2rfti9zAlbXcAE +rbnG+f0fk0AaqU7KelU35vjPfNe6Vn7ky6G9CC6jW04NkLZDNFA2GusdYf1aM0LW +ew5t4WZaquLVFhL36q9eHaogO/fcPR/quvQefHokk+b541ytwMN9l/g43rTbCvAj +rUDHwipbGbw91Wg2XjbecRiCXDKWds2M149BpxUzY5xHFtD5t5WSEE/SkkryGTMm +TxS3tuQZ9PdtCPGrNDO6Ts/amORF04Tf+YMJgfv3IWxMeQ== +=6kcB +-----END PGP PRIVATE KEY BLOCK----- diff --git a/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go index bf125b62a7..e7aaead8c3 100644 --- a/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go +++ b/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -873,6 +873,9 @@ const ( // FieldManagerConflict is used to report when another client claims to manage this field, // It should only be returned for a request using server-side apply. CauseTypeFieldManagerConflict CauseType = "FieldManagerConflict" + // CauseTypeResourceVersionTooLarge is used to report that the requested resource version + // is newer than the data observed by the API server, so the request cannot be served. + CauseTypeResourceVersionTooLarge CauseType = "ResourceVersionTooLarge" ) // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/vendor/k8s.io/apimachinery/pkg/util/net/http.go b/vendor/k8s.io/apimachinery/pkg/util/net/http.go index 7449cbb0a0..7b64e68157 100644 --- a/vendor/k8s.io/apimachinery/pkg/util/net/http.go +++ b/vendor/k8s.io/apimachinery/pkg/util/net/http.go @@ -446,7 +446,7 @@ redirectLoop: // Only follow redirects to the same host. Otherwise, propagate the redirect response back. if requireSameHostRedirects && location.Hostname() != originalLocation.Hostname() { - break redirectLoop + return nil, nil, fmt.Errorf("hostname mismatch: expected %s, found %s", originalLocation.Hostname(), location.Hostname()) } // Reset the connection. diff --git a/vendor/modules.txt b/vendor/modules.txt index bf2a14d4c6..4d11215536 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -689,10 +689,10 @@ gopkg.in/tomb.v1 gopkg.in/yaml.v2 # gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c gopkg.in/yaml.v3 -# k8s.io/api v0.18.4 +# k8s.io/api v0.18.6 k8s.io/api/apps/v1 k8s.io/api/core/v1 -# k8s.io/apimachinery v0.18.4 +# k8s.io/apimachinery v0.18.6 k8s.io/apimachinery/pkg/api/errors k8s.io/apimachinery/pkg/api/resource k8s.io/apimachinery/pkg/apis/meta/v1 From 760a9077db10f0837986ae6b7e2d4003c6ad9662 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 3 Aug 2020 09:38:10 +0200 Subject: [PATCH 07/25] rootless: system service joins immediately the namespaces when there is a pause process running, let the "system service" podman instance join immediately the existing namespaces. Closes: https://github.com/containers/podman/issues/7180 Closes: https://github.com/containers/podman/issues/6660 Signed-off-by: Giuseppe Scrivano --- pkg/rootless/rootless_linux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/rootless/rootless_linux.c b/pkg/rootless/rootless_linux.c index 716db81dce..2c6f7ae38a 100644 --- a/pkg/rootless/rootless_linux.c +++ b/pkg/rootless/rootless_linux.c @@ -205,7 +205,7 @@ can_use_shortcut () if (strcmp (argv[argc], "mount") == 0 || strcmp (argv[argc], "search") == 0 - || strcmp (argv[argc], "system") == 0) + || (strcmp (argv[argc], "system") == 0 && argv[argc+1] && strcmp (argv[argc+1], "service") != 0)) { ret = false; break; From 5301d40e81c5a0758a3c8302b037854629317d43 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Mon, 3 Aug 2020 14:19:21 -0500 Subject: [PATCH 08/25] docker-compose uses application/tar even though the official documentation suggests that application/x-tar should be used for tar files, it seems docker-compose uses application/tar. we now accept them and issue a warning. Fixes: #7185 Signed-off-by: Brent Baude --- pkg/api/handlers/compat/images_build.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/api/handlers/compat/images_build.go b/pkg/api/handlers/compat/images_build.go index 8ac5b80c1c..3d9efe61ee 100644 --- a/pkg/api/handlers/compat/images_build.go +++ b/pkg/api/handlers/compat/images_build.go @@ -20,6 +20,7 @@ import ( "github.com/containers/libpod/v2/pkg/api/handlers/utils" "github.com/containers/storage/pkg/archive" "github.com/gorilla/schema" + "github.com/sirupsen/logrus" ) func BuildImage(w http.ResponseWriter, r *http.Request) { @@ -33,7 +34,13 @@ func BuildImage(w http.ResponseWriter, r *http.Request) { } if hdr, found := r.Header["Content-Type"]; found && len(hdr) > 0 { - if hdr[0] != "application/x-tar" { + contentType := hdr[0] + switch contentType { + case "application/tar": + logrus.Warnf("tar file content type is %s, should use \"application/x-tar\" content type", contentType) + case "application/x-tar": + break + default: utils.BadRequest(w, "Content-Type", hdr[0], fmt.Errorf("Content-Type: %s is not supported. Should be \"application/x-tar\"", hdr[0])) return From 2ff8f485ea640b03a322df0f49ed79af92aa1f0b Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Mon, 3 Aug 2020 13:58:41 -0500 Subject: [PATCH 09/25] Missing return after early exit the exists code was plagued by a missing return statement meant to trigger an early exit. Fixes: #7197 Signed-off-by: Brent Baude --- pkg/api/handlers/libpod/containers.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/api/handlers/libpod/containers.go b/pkg/api/handlers/libpod/containers.go index 21904e21f3..2303ff17a8 100644 --- a/pkg/api/handlers/libpod/containers.go +++ b/pkg/api/handlers/libpod/containers.go @@ -23,6 +23,7 @@ func ContainerExists(w http.ResponseWriter, r *http.Request) { if err != nil { if errors.Cause(err) == define.ErrNoSuchCtr { utils.ContainerNotFound(w, name, err) + return } utils.InternalServerError(w, err) return From d6442f5f571112d66fd62309a2e8e15c163ff4f3 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 3 Aug 2020 13:33:08 -0400 Subject: [PATCH 10/25] Do not set host IP on ports when 0.0.0.0 requested Docker and CNI have very different ideas of what 0.0.0.0 means. Docker takes it to be 0.0.0.0/0 - that is, bind to every IPv4 address on the host. CNI (and, thus, root Podman) take it to mean the literal IP 0.0.0.0. Instead, CNI interprets the empty string ("") as "bind to all IPs". We could ask CNI to change, but given this is established behavior, that's unlikely. Instead, let's just catch 0.0.0.0 and turn it into "" when we parse ports. Fixes #7014 Signed-off-by: Matthew Heon --- cmd/podman/common/util.go | 13 ++++++++----- docs/source/markdown/podman-create.1.md | 3 ++- docs/source/markdown/podman-run.1.md | 2 ++ test/e2e/run_networking_test.go | 12 ++++++++++++ 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/cmd/podman/common/util.go b/cmd/podman/common/util.go index e21e349d98..52b637a789 100644 --- a/cmd/podman/common/util.go +++ b/cmd/podman/common/util.go @@ -175,12 +175,15 @@ func parseSplitPort(hostIP, hostPort *string, ctrPort string, protocol *string) if hostIP != nil { if *hostIP == "" { return newPort, errors.Errorf("must provide a non-empty container host IP to publish") + } else if *hostIP != "0.0.0.0" { + // If hostIP is 0.0.0.0, leave it unset - CNI treats + // 0.0.0.0 and empty differently, Docker does not. + testIP := net.ParseIP(*hostIP) + if testIP == nil { + return newPort, errors.Errorf("cannot parse %q as an IP address", *hostIP) + } + newPort.HostIP = testIP.String() } - testIP := net.ParseIP(*hostIP) - if testIP == nil { - return newPort, errors.Errorf("cannot parse %q as an IP address", *hostIP) - } - newPort.HostIP = testIP.String() } if hostPort != nil { if *hostPort == "" { diff --git a/docs/source/markdown/podman-create.1.md b/docs/source/markdown/podman-create.1.md index 2b383f51b0..715d6c1c96 100644 --- a/docs/source/markdown/podman-create.1.md +++ b/docs/source/markdown/podman-create.1.md @@ -628,7 +628,8 @@ Both hostPort and containerPort can be specified as a range of ports. When specifying ranges for both, the number of container ports in the range must match the number of host ports in the range. (e.g., `podman run -p 1234-1236:1222-1224 --name thisWorks -t busybox` but not `podman run -p 1230-1236:1230-1240 --name RangeContainerPortsBiggerThanRangeHostPorts -t busybox`) -With ip: `podman run -p 127.0.0.1:$HOSTPORT:$CONTAINERPORT --name CONTAINER -t someimage` +With host IP: `podman run -p 127.0.0.1:$HOSTPORT:$CONTAINERPORT --name CONTAINER -t someimage` +If host IP is set to 0.0.0.0 or not set at all, the port will be bound on all IPs on the host. Host port does not have to be specified (e.g. `podman run -p 127.0.0.1::80`). If it is not, the container port will be randomly assigned a port on the host. Use `podman port` to see the actual mapping: `podman port CONTAINER $CONTAINERPORT` diff --git a/docs/source/markdown/podman-run.1.md b/docs/source/markdown/podman-run.1.md index d20b695078..fab6485936 100644 --- a/docs/source/markdown/podman-run.1.md +++ b/docs/source/markdown/podman-run.1.md @@ -638,6 +638,8 @@ Both hostPort and containerPort can be specified as a range of ports. When specifying ranges for both, the number of container ports in the range must match the number of host ports in the range. +If host IP is set to 0.0.0.0 or not set at all, the port will be bound on all IPs on the host. + Host port does not have to be specified (e.g. `podman run -p 127.0.0.1::80`). If it is not, the container port will be randomly assigned a port on the host. diff --git a/test/e2e/run_networking_test.go b/test/e2e/run_networking_test.go index 40cc9e1e62..88e289f1f3 100644 --- a/test/e2e/run_networking_test.go +++ b/test/e2e/run_networking_test.go @@ -238,6 +238,18 @@ var _ = Describe("Podman run networking", func() { Expect((hp1 == "4000" && hp2 == "8000") || (hp1 == "8000" && hp2 == "4000")).To(BeTrue()) }) + It("podman run -p 0.0.0.0:8080:80", func() { + name := "testctr" + session := podmanTest.Podman([]string{"create", "-t", "-p", "0.0.0.0:8080:80", "--name", name, ALPINE, "/bin/sh"}) + session.WaitWithDefaultTimeout() + inspectOut := podmanTest.InspectContainer(name) + Expect(len(inspectOut)).To(Equal(1)) + Expect(len(inspectOut[0].NetworkSettings.Ports)).To(Equal(1)) + Expect(len(inspectOut[0].NetworkSettings.Ports["80/tcp"])).To(Equal(1)) + Expect(inspectOut[0].NetworkSettings.Ports["80/tcp"][0].HostPort).To(Equal("8080")) + Expect(inspectOut[0].NetworkSettings.Ports["80/tcp"][0].HostIP).To(Equal("")) + }) + It("podman run network expose host port 80 to container port 8000", func() { SkipIfRootless() session := podmanTest.Podman([]string{"run", "-dt", "-p", "80:8000", ALPINE, "/bin/sh"}) From 85d7cb5fd42642b26b0fe3c98f165c081b41c45e Mon Sep 17 00:00:00 2001 From: zhangguanzhang Date: Sat, 1 Aug 2020 23:37:41 +0800 Subject: [PATCH 11/25] implement the exitcode when start a container with attach Signed-off-by: zhangguanzhang --- cmd/podman/containers/start.go | 11 ++++++++--- test/e2e/start_test.go | 12 ++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/cmd/podman/containers/start.go b/cmd/podman/containers/start.go index 8f99844217..84a7aa4e26 100644 --- a/cmd/podman/containers/start.go +++ b/cmd/podman/containers/start.go @@ -99,12 +99,17 @@ func start(cmd *cobra.Command, args []string) error { } for _, r := range responses { - if r.Err == nil && !startOptions.Attach { - fmt.Println(r.RawInput) + if r.Err == nil { + if startOptions.Attach { + // Implement the exitcode when the only one container is enabled attach + registry.SetExitCode(r.ExitCode) + } else { + fmt.Println(r.RawInput) + } } else { errs = append(errs, r.Err) } } - // TODO need to understand an implement exitcodes + return errs.PrintErrors() } diff --git a/test/e2e/start_test.go b/test/e2e/start_test.go index 761b1c4ca7..3cc1668e74 100644 --- a/test/e2e/start_test.go +++ b/test/e2e/start_test.go @@ -86,6 +86,18 @@ var _ = Describe("Podman start", func() { Expect(session.OutputToString()).To(Equal(name)) }) + It("podman start single container with attach and test the signal", func() { + SkipIfRemote() + session := podmanTest.Podman([]string{"create", "--entrypoint", "sh", ALPINE, "-c", "exit 1"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + cid := session.OutputToString() + session = podmanTest.Podman([]string{"start", "--attach", cid}) + session.WaitWithDefaultTimeout() + // It should forward the signal + Expect(session.ExitCode()).To(Equal(1)) + }) + It("podman start multiple containers", func() { session := podmanTest.Podman([]string{"create", "-d", "--name", "foobar99", ALPINE, "ls"}) session.WaitWithDefaultTimeout() From 589627b0cae2fdfe2cea8426a5dfbe74983962c3 Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Mon, 13 Jul 2020 09:42:47 -0600 Subject: [PATCH 12/25] system tests: new tests for run, exec - Issue #6735 : problem with multiple namespaces; confirms combinations of --userns=keep-id, --privileged, --user=XX - Issue #6829 : --userns=keep-id will add a /etc/passwd entry - Issue #6593 : podman exec, with --userns=keep-id, errors (test is currently skipped because issue remains live) ...and, addendum: add new helper function, remove_same_dev_warning. Some CI systems issue a warning on podman run --privileged: WARNING: The same type, major and minor should not be used for multiple devices. We already had special-case code to ignore than in the SELinux test, but now we're seeing it in the new run tests I added, so I've refactored the "ignore this warning" code and written tests for the removal code. Signed-off-by: Ed Santiago --- test/system/030-run.bats | 42 ++++++++++++++++++++++ test/system/075-exec.bats | 22 ++++++++++-- test/system/410-selinux.bats | 9 +---- test/system/helpers.bash | 32 +++++++++++++++++ test/system/helpers.t | 70 +++++++++++++++++++++++++++++++++++- 5 files changed, 164 insertions(+), 11 deletions(-) diff --git a/test/system/030-run.bats b/test/system/030-run.bats index c7a9bf1915..2acc37579c 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -230,4 +230,46 @@ echo $rand | 0 | $rand run_podman rmi myi } +# #6735 : complex interactions with multiple user namespaces +# The initial report has to do with bind mounts, but that particular +# symptom only manifests on a fedora container image -- we have no +# reproducer on alpine. Checking directory ownership is good enough. +@test "podman run : user namespace preserved root ownership" { + for priv in "" "--privileged"; do + for user in "--user=0" "--user=100"; do + for keepid in "" "--userns=keep-id"; do + opts="$priv $user $keepid" + + for dir in /etc /usr;do + run_podman run --rm $opts $IMAGE stat -c '%u:%g:%n' $dir + remove_same_dev_warning # grumble + is "$output" "0:0:$dir" "run $opts ($dir)" + done + done + done + done +} + +# #6829 : add username to /etc/passwd inside container if --userns=keep-id +@test "podman run : add username to /etc/passwd if --userns=keep-id" { + # Default: always run as root + run_podman run --rm $IMAGE id -un + is "$output" "root" "id -un on regular container" + + # This would always work on root, but is new behavior on rootless: #6829 + # adds a user entry to /etc/passwd + run_podman run --rm --userns=keep-id $IMAGE id -un + is "$output" "$(id -un)" "username on container with keep-id" + + # --privileged should make no difference + run_podman run --rm --privileged --userns=keep-id $IMAGE id -un + remove_same_dev_warning # grumble + is "$output" "$(id -un)" "username on container with keep-id" + + # ...but explicitly setting --user should override keep-id + run_podman run --rm --privileged --userns=keep-id --user=0 $IMAGE id -un + remove_same_dev_warning # grumble + is "$output" "root" "--user=0 overrides keep-id" +} + # vim: filetype=sh diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats index 019217d8f2..f53a22a3f9 100644 --- a/test/system/075-exec.bats +++ b/test/system/075-exec.bats @@ -6,8 +6,6 @@ load helpers @test "podman exec - basic test" { - skip_if_remote - rand_filename=$(random_string 20) rand_content=$(random_string 50) @@ -89,4 +87,24 @@ load helpers run_podman rm $cid } +# #6829 : add username to /etc/passwd inside container if --userns=keep-id +# #6593 : doesn't actually work with podman exec +@test "podman exec - with keep-id" { + skip "Please enable once #6593 is fixed" + + run_podman run -d --userns=keep-id $IMAGE sh -c \ + "echo READY;while [ ! -f /stop ]; do sleep 1; done" + cid="$output" + wait_for_ready $cid + + run_podman exec $cid id -un + is "$output" "$(id -un)" "container is running as current user" + + # Until #6593 gets fixed, this just hangs. The server process barfs with: + # unable to find user : no matching entries in passwd file + run_podman exec --user=$(id -un) $cid touch /stop + run_podman wait $cid + run_podman rm $cid +} + # vim: filetype=sh diff --git a/test/system/410-selinux.bats b/test/system/410-selinux.bats index 8a0477eff3..3dca596418 100644 --- a/test/system/410-selinux.bats +++ b/test/system/410-selinux.bats @@ -19,15 +19,8 @@ function check_label() { # FIXME: on some CI systems, 'run --privileged' emits a spurious # warning line about dup devices. Ignore it. + remove_same_dev_warning local context="$output" - if [ ${#lines[@]} -gt 1 ]; then - if expr "${lines[0]}" : "WARNING: .* type, major" >/dev/null; then - echo "# ${lines[0]} [ignored]" >&3 - context="${lines[1]}" - else - die "FAILED: too much output, expected one single line" - fi - fi is "$context" ".*_u:system_r:.*" "SELinux role should always be system_r" diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 7ec2105d19..5afe147184 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -386,5 +386,37 @@ function find_exec_pid_files() { find $storage_path -type f -iname 'exec_pid_*' fi } + + +############################# +# remove_same_dev_warning # Filter out useless warning from output +############################# +# +# On some CI systems, 'podman run --privileged' emits a useless warning: +# +# WARNING: The same type, major and minor should not be used for multiple devices. +# +# This obviously screws us up when we look at output results. +# +# This function removes the warning from $output and $lines +# +function remove_same_dev_warning() { + # No input arguments. We operate in-place on $output and $lines + + local i=0 + local -a new_lines=() + while [[ $i -lt ${#lines[@]} ]]; do + if expr "${lines[$i]}" : 'WARNING: .* same type, major.* multiple' >/dev/null; then + : + else + new_lines+=("${lines[$i]}") + fi + i=$(( i + 1 )) + done + + lines=("${new_lines[@]}") + output=$(printf '%s\n' "${lines[@]}") +} + # END miscellaneous tools ############################################################################### diff --git a/test/system/helpers.t b/test/system/helpers.t index 7b4e48a841..a022f11c47 100755 --- a/test/system/helpers.t +++ b/test/system/helpers.t @@ -23,7 +23,8 @@ rc=0 function check_result { testnum=$(expr $testnum + 1) if [ "$1" = "$2" ]; then - echo "ok $testnum $3 = $1" + # Multi-level echo flattens newlines, makes success messages readable + echo $(echo "ok $testnum $3 = $1") else echo "not ok $testnum $3" echo "# expected: $2" @@ -141,5 +142,72 @@ done < <(parse_table "$table") # END dprint ############################################################################### +# BEGIN remove_same_dev_warning + +# Test-helper function: runs remove_same_dev_warning, compares resulting +# value of $lines and $output to expected values given on command line +function check_same_dev() { + local testname="$1"; shift + local -a expect_lines=("$@") + local nl=" +" + + remove_same_dev_warning + + # After processing, check the expected number of lines + check_result "${#lines[@]}" "${#@}" "$testname: expected # of lines" + + # ...and each expected line + local expect_output="" + local i=0 + while [ $i -lt ${#expect_lines[@]} ]; do + check_result "${lines[$i]}" "${expect_lines[$i]}" "$testname: line $i" + expect_output+="${expect_lines[$i]}$nl" + i=$(( i + 1 )) + done + + # ...and the possibly-multi-line $output + check_result "$output" "${expect_output%%$nl}" "$testname: output" +} + +# Simplest case: nothing removed. +declare -a lines=("a b c" "d" "e f") +check_same_dev "abc" "a b c" "d" "e f" + +# Confirm that the warning message is removed from the beginning +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." + "a" + "b" + "c" +) +check_same_dev "warning is removed" a b c + +# ...and from the middle (we do not expect to see this) +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." + "a" + "b" + "WARNING: The same type, major and minor should not be used for multiple devices." + "c" +) +check_same_dev "multiple warnings removed" a b c + +# Corner case: two lines of output, only one of which we care about +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." + "this is the only line we care about" +) +check_same_dev "one-line output" "this is the only line we care about" + +# Corner case: one line of output, but we expect zero. +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." +) +check_same_dev "zero-line output" + + +# END remove_same_dev_warning +############################################################################### exit $rc From 9b591e11d5d65e210871fa5a8d5083f6f6844434 Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Tue, 28 Jul 2020 08:36:52 -0600 Subject: [PATCH 13/25] cherry-pick: Reenable remote system tests NOTE: the remote tests are not reenabled but the changes are applied. Future commits depend on some of the changes and having the commit applied will likely facilitate future backports as well. podman-remote is in better shape now. Let's see what needs to be done to reenable remote system tests. - logs test: skip multilog, it doesn't work remote - diff test: use -l only when local, not with remote - many other tests: skip_if_remote, with 'FIXME: pending #xxxx' where xxxx is a filed issue. Unrelated: added new helper to skip_if_remote and _if_rootless, where we check if the source message includes "remote"/"rootless" and insert it if missing. This is a minor usability enhancement to make it easier to understand at-a-glance why a skip triggers. Backported-by: Valentin Rothberg Signed-off-by: Ed Santiago --- Makefile | 8 +- test/system/030-run.bats | 26 ++++ test/system/035-logs.bats | 2 + test/system/050-stop.bats | 7 +- test/system/055-rm.bats | 2 + test/system/070-build.bats | 154 +++++++++++++++++++++-- test/system/110-history.bats | 2 + test/system/120-load.bats | 2 + test/system/130-kill.bats | 2 + test/system/140-diff.bats | 13 +- test/system/160-volumes.bats | 6 + test/system/200-pod.bats | 17 ++- test/system/220-healthcheck.bats | 1 + test/system/300-cli-parsing.bats | 2 + test/system/400-unprivileged-access.bats | 70 +++++++++++ test/system/410-selinux.bats | 1 + test/system/helpers.bash | 22 +++- 17 files changed, 315 insertions(+), 22 deletions(-) diff --git a/Makefile b/Makefile index 104c50c4c2..3e4708beae 100644 --- a/Makefile +++ b/Makefile @@ -367,15 +367,15 @@ remotesystem: if timeout -v 1 true; then \ SOCK_FILE=$(shell mktemp --dry-run --tmpdir podman.XXXXXX);\ export PODMAN_SOCKET=unix:$$SOCK_FILE; \ - ./bin/podman system service --timeout=0 $$PODMAN_SOCKET &> $(if $(PODMAN_SERVER_LOG),$(PODMAN_SERVER_LOG),/dev/null) & \ + ./bin/podman system service --timeout=0 $$PODMAN_SOCKET > $(if $(PODMAN_SERVER_LOG),$(PODMAN_SERVER_LOG),/dev/null) 2>&1 & \ retry=5;\ - while [[ $$retry -ge 0 ]]; do\ + while [ $$retry -ge 0 ]; do\ echo Waiting for server...;\ sleep 1;\ - ./bin/podman-remote --url $$PODMAN_SOCKET info &>/dev/null && break;\ + ./bin/podman-remote --url $$PODMAN_SOCKET info >/dev/null 2>&1 && break;\ retry=$$(expr $$retry - 1);\ done;\ - if [[ $$retry -lt 0 ]]; then\ + if [ $$retry -lt 0 ]; then\ echo "Error: ./bin/podman system service did not come up on $$SOCK_FILE" >&2;\ exit 1;\ fi;\ diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 2acc37579c..4f707dda32 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -84,6 +84,8 @@ echo $rand | 0 | $rand # Believe it or not, 'sh -c' resulted in different behavior run_podman 0 run --rm $IMAGE sh -c /bin/true run_podman 1 run --rm $IMAGE sh -c /bin/false + + if is_remote; then sleep 2;fi # FIXME: pending #7119 } @test "podman run --name" { @@ -190,6 +192,8 @@ echo $rand | 0 | $rand } @test "podman run docker-archive" { + skip_if_remote "FIXME: pending #7116" + # Create an image that, when run, outputs a random magic string expect=$(random_string 20) run_podman run --name myc --entrypoint="[\"/bin/echo\",\"$expect\"]" $IMAGE @@ -235,6 +239,8 @@ echo $rand | 0 | $rand # symptom only manifests on a fedora container image -- we have no # reproducer on alpine. Checking directory ownership is good enough. @test "podman run : user namespace preserved root ownership" { + skip_if_remote "FIXME: pending #7195" + for priv in "" "--privileged"; do for user in "--user=0" "--user=100"; do for keepid in "" "--userns=keep-id"; do @@ -252,6 +258,8 @@ echo $rand | 0 | $rand # #6829 : add username to /etc/passwd inside container if --userns=keep-id @test "podman run : add username to /etc/passwd if --userns=keep-id" { + skip_if_remote "FIXME: pending #7195" + # Default: always run as root run_podman run --rm $IMAGE id -un is "$output" "root" "id -un on regular container" @@ -272,4 +280,22 @@ echo $rand | 0 | $rand is "$output" "root" "--user=0 overrides keep-id" } +# #6991 : /etc/passwd is modifiable +@test "podman run : --userns=keep-id: passwd file is modifiable" { + skip_if_remote "FIXME: pending #7195" + + run_podman run -d --userns=keep-id $IMAGE sh -c 'while ! test -e /stop; do sleep 0.1; done' + cid="$output" + + gecos="$(random_string 6) $(random_string 8)" + run_podman exec --user root $cid adduser -D -g "$gecos" -s /bin/sh newuser3 + is "$output" "" "output from adduser" + run_podman exec $cid tail -1 /etc/passwd + is "$output" "newuser3:x:1000:1000:$gecos:/home/newuser3:/bin/sh" \ + "newuser3 added to /etc/passwd in container" + + run_podman exec $cid touch /stop + run_podman wait $cid +} + # vim: filetype=sh diff --git a/test/system/035-logs.bats b/test/system/035-logs.bats index 055865c8d1..cbb2091e57 100644 --- a/test/system/035-logs.bats +++ b/test/system/035-logs.bats @@ -25,6 +25,8 @@ load helpers } @test "podman logs - multi" { + skip_if_remote "logs does not support multiple containers when run remotely" + # Simple helper to make the container starts, below, easier to read local -a cid doit() { diff --git a/test/system/050-stop.bats b/test/system/050-stop.bats index 093606ecea..f604ea2e28 100644 --- a/test/system/050-stop.bats +++ b/test/system/050-stop.bats @@ -12,9 +12,12 @@ load helpers run_podman stop $cid t1=$SECONDS - # Confirm that container is stopped + # Confirm that container is stopped. Podman-remote unfortunately + # cannot tell the difference between "stopped" and "exited", and + # spits them out interchangeably, so we need to recognize either. run_podman inspect --format '{{.State.Status}} {{.State.ExitCode}}' $cid - is "$output" "exited \+137" "Status and exit code of stopped container" + is "$output" "\\(stopped\|exited\\) \+137" \ + "Status and exit code of stopped container" # The initial SIGTERM is ignored, so this operation should take # exactly 10 seconds. Give it some leeway. diff --git a/test/system/055-rm.bats b/test/system/055-rm.bats index 8ef8a119eb..5ff19282e9 100644 --- a/test/system/055-rm.bats +++ b/test/system/055-rm.bats @@ -32,6 +32,8 @@ load helpers # # See https://github.com/containers/libpod/issues/3795 @test "podman rm -f" { + skip_if_remote "FIXME: pending #7117" + rand=$(random_string 30) ( sleep 3; run_podman rm -f $rand ) & run_podman 137 run --name $rand $IMAGE sleep 30 diff --git a/test/system/070-build.bats b/test/system/070-build.bats index 7d66602708..6ec6b09d95 100644 --- a/test/system/070-build.bats +++ b/test/system/070-build.bats @@ -6,9 +6,7 @@ load helpers @test "podman build - basic test" { - if is_remote && is_rootless; then - skip "unreliable with podman-remote and rootless; #2972" - fi + skip_if_remote "FIXME: pending #7136" rand_filename=$(random_string 20) rand_content=$(random_string 50) @@ -34,6 +32,7 @@ EOF # Regression from v1.5.0. This test passes fine in v1.5.0, fails in 1.6 @test "podman build - cache (#3920)" { + skip_if_remote "FIXME: pending #7136" if is_remote && is_rootless; then skip "unreliable with podman-remote and rootless; #2972" fi @@ -81,6 +80,8 @@ EOF } @test "podman build - URLs" { + skip_if_remote "FIXME: pending #7137" + tmpdir=$PODMAN_TMPDIR/build-test mkdir -p $tmpdir @@ -90,6 +91,7 @@ ADD https://github.com/containers/libpod/blob/master/README.md /tmp/ EOF run_podman build -t add_url $tmpdir run_podman run --rm add_url stat /tmp/README.md + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman rmi -f add_url # Now test COPY. That should fail. @@ -98,20 +100,156 @@ EOF is "$output" ".*error building at STEP .*: source can't be a URL for COPY" } -@test "podman build - stdin test" { - if is_remote && is_rootless; then - skip "unreliable with podman-remote and rootless; #2972" - fi +@test "podman build - workdir, cmd, env, label" { + skip_if_remote "FIXME: pending #7137" + + tmpdir=$PODMAN_TMPDIR/build-test + mkdir -p $tmpdir # Random workdir, and multiple random strings to verify command & env workdir=/$(random_string 10) + s_echo=$(random_string 15) + s_env1=$(random_string 20) + s_env2=$(random_string 25) + s_env3=$(random_string 30) + s_env4=$(random_string 40) + + # Label name: make sure it begins with a letter! jq barfs if you + # try to ask it for '.foo.xyz', i.e. any string beginning with digit + label_name=l$(random_string 8) + label_value=$(random_string 12) + + # Command to run on container startup with no args + cat >$tmpdir/mycmd <$PODMAN_TMPDIR/env-file <$tmpdir/Containerfile < expect=<$expect}>" + is "$actual" "$expect" "jq .Config.$field" + done + + # Bad symlink in volume. Prior to #7094, well, we wouldn't actually + # get here because any 'podman run' on a volume that had symlinks, + # be they dangling or valid, would barf with + # Error: chown /_data/symlink: ENOENT + run_podman run --rm build_test stat -c'%u:%g:%N' /a/b/c/badsymlink + is "$output" "1:2:'/a/b/c/badsymlink' -> '/no/such/nonesuch'" \ + "bad symlink to nonexistent file is chowned and preserved" + + run_podman run --rm build_test stat -c'%u:%g:%N' /a/b/c/goodsymlink + is "$output" "1:2:'/a/b/c/goodsymlink' -> '/bin/mydefaultcmd'" \ + "good symlink to existing file is chowned and preserved" + + run_podman run --rm build_test stat -c'%u:%g' /bin/mydefaultcmd + is "$output" "2:3" "target of symlink is not chowned" + + run_podman run --rm build_test stat -c'%u:%g:%N' /a/b/c/myfile + is "$output" "4:5:/a/b/c/myfile" "file in volume is chowned" + + # Clean up + run_podman rmi -f build_test +} + +@test "podman build - stdin test" { + skip_if_remote "FIXME: pending #7136" + + # Random workdir, and random string to verify build output + workdir=/$(random_string 10) + random_echo=$(random_string 15) PODMAN_TIMEOUT=240 run_podman build -t build_test - << EOF FROM $IMAGE RUN mkdir $workdir WORKDIR $workdir -RUN /bin/echo 'Test' +RUN /bin/echo $random_echo EOF is "$output" ".*STEP 5: COMMIT" "COMMIT seen in log" + is "$output" ".*STEP .: RUN /bin/echo $random_echo" run_podman run --rm build_test pwd is "$output" "$workdir" "pwd command in container" diff --git a/test/system/110-history.bats b/test/system/110-history.bats index 5dc221d610..b83e90fe4a 100644 --- a/test/system/110-history.bats +++ b/test/system/110-history.bats @@ -3,6 +3,8 @@ load helpers @test "podman history - basic tests" { + skip_if_remote "FIXME: pending #7122" + tests=" | .*[0-9a-f]\\\{12\\\} .* CMD .* LABEL --format '{{.ID}} {{.Created}}' | .*[0-9a-f]\\\{12\\\} .* ago diff --git a/test/system/120-load.bats b/test/system/120-load.bats index c0ddbf4d62..611799f8d0 100644 --- a/test/system/120-load.bats +++ b/test/system/120-load.bats @@ -28,6 +28,8 @@ verify_iid_and_name() { @test "podman load - by image ID" { + skip_if_remote "FIXME: pending #7123" + # FIXME: how to build a simple archive instead? get_iid_and_name diff --git a/test/system/130-kill.bats b/test/system/130-kill.bats index 7c2b9bed8e..1bab089feb 100644 --- a/test/system/130-kill.bats +++ b/test/system/130-kill.bats @@ -6,6 +6,8 @@ load helpers @test "podman kill - test signal handling in containers" { + skip_if_remote "FIXME: pending #7135" + # podman-remote and crun interact poorly in f31: crun seems to gobble up # some signals. # Workaround: run 'env --default-signal sh' instead of just 'sh' in diff --git a/test/system/140-diff.bats b/test/system/140-diff.bats index 9f4a2c0deb..01ec5430e6 100644 --- a/test/system/140-diff.bats +++ b/test/system/140-diff.bats @@ -6,9 +6,16 @@ load helpers @test "podman diff" { + n=$(random_string 10) # container name rand_file=$(random_string 10) - run_podman run $IMAGE sh -c "touch /$rand_file;rm /etc/services" - run_podman diff --format json -l + run_podman run --name $n $IMAGE sh -c "touch /$rand_file;rm /etc/services" + + # If running local, test `-l` (latest) option. This can't work with remote. + if ! is_remote; then + n=-l + fi + + run_podman diff --format json $n # Expected results for each type of diff declare -A expect=( @@ -22,7 +29,7 @@ load helpers is "$result" "${expect[$field]}" "$field" done - run_podman rm -l + run_podman rm $n } # vim: filetype=sh diff --git a/test/system/160-volumes.bats b/test/system/160-volumes.bats index 3233e6f04d..ef38b2a687 100644 --- a/test/system/160-volumes.bats +++ b/test/system/160-volumes.bats @@ -93,6 +93,7 @@ Labels.l | $mylabel is "$(<$mountpoint/myfile)" "$rand" "we see content created in container" # Clean up + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvolume } @@ -134,12 +135,14 @@ EOF is "$output" "got here -$rand-" "script in volume is runnable with default (exec)" # Clean up + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvolume } # Anonymous temporary volumes, and persistent autocreated named ones @test "podman volume, implicit creation with run" { + skip_if_remote "FIXME: pending #7128" # No hostdir arg: create anonymous container with random name rand=$(random_string) @@ -172,6 +175,7 @@ EOF run_podman run --rm -v $myvol:/myvol:z $IMAGE \ sh -c "cp /myvol/myfile /myvol/myfile2" + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvol # Autocreated volumes should also work with keep-id @@ -180,6 +184,7 @@ EOF run_podman run --rm -v $myvol:/myvol:z --userns=keep-id $IMAGE \ touch /myvol/myfile + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvol } @@ -187,6 +192,7 @@ EOF # Confirm that container sees the correct id @test "podman volume with --userns=keep-id" { is_rootless || skip "only meaningful when run rootless" + skip_if_remote "FIXME: pending #7195" myvoldir=${PODMAN_TMPDIR}/volume_$(random_string) mkdir $myvoldir diff --git a/test/system/200-pod.bats b/test/system/200-pod.bats index 478ff06bb6..6680a896d4 100644 --- a/test/system/200-pod.bats +++ b/test/system/200-pod.bats @@ -18,7 +18,9 @@ function teardown() { @test "podman pod top - containers in different PID namespaces" { - skip_if_remote "podman-pod does not work with podman-remote" + if is_remote && is_rootless; then + skip "FIXME: pending #7139" + fi # With infra=false, we don't get a /pause container (we also # don't pull k8s.gcr.io/pause ) @@ -53,7 +55,9 @@ function teardown() { @test "podman pod - communicating between pods" { - skip_if_remote "podman-pod does not work with podman-remote" + if is_remote && is_rootless; then + skip "FIXME: pending #7139" + fi podname=pod$(random_string) run_podman 1 pod exists $podname @@ -77,7 +81,7 @@ function teardown() { run_podman ps --format '{{.Pod}}' newline=" " - is "$output" "${podid:0:12}${newline}${podid:0:12}" "sdfdsf" + is "$output" "${podid:0:12}${newline}${podid:0:12}" "ps shows 2 pod IDs" # Talker: send the message via common port on localhost message=$(random_string 15) @@ -89,6 +93,7 @@ function teardown() { is "$output" "$message" "message sent from one container to another" # Clean up. First the nc -l container... + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman rm $cid1 # ...then, from pause container, find the image ID of the pause image... @@ -99,6 +104,7 @@ function teardown() { pause_iid="$output" # ...then rm the pod, then rmi the pause image so we don't leave strays. + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman pod rm $podname run_podman rmi $pause_iid @@ -135,6 +141,10 @@ function random_ip() { } @test "podman pod create - hashtag AllTheOptions" { + if is_remote && is_rootless; then + skip "FIXME: pending #7139" + fi + mac=$(random_mac) add_host_ip=$(random_ip) add_host_n=$(random_string | tr A-Z a-z).$(random_string | tr A-Z a-z).xyz @@ -192,6 +202,7 @@ function random_ip() { is "$output" ".*options $dns_opt" "--dns-opt was added" # pod inspect + if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman pod inspect --format '{{.Name}}: {{.ID}} : {{.NumContainers}} : {{.Labels}}' mypod is "$output" "mypod: $pod_id : 1 : map\[${labelname}:${labelvalue}]" \ "pod inspect --format ..." diff --git a/test/system/220-healthcheck.bats b/test/system/220-healthcheck.bats index e649ad3d24..3405029c1f 100644 --- a/test/system/220-healthcheck.bats +++ b/test/system/220-healthcheck.bats @@ -25,6 +25,7 @@ function _check_health { @test "podman healthcheck" { + skip_if_remote "FIXME: pending #7137" # Create an image with a healthcheck script; said script will # pass until the file /uh-oh gets created (by us, via exec) diff --git a/test/system/300-cli-parsing.bats b/test/system/300-cli-parsing.bats index 92c0731023..2abc01bb79 100644 --- a/test/system/300-cli-parsing.bats +++ b/test/system/300-cli-parsing.bats @@ -10,6 +10,8 @@ load helpers # Error: invalid argument "true=\"false\"" for "-l, --label" \ # flag: parse error on line 1, column 5: bare " in non-quoted-field run_podman run --rm --label 'true="false"' $IMAGE true + + if is_remote; then sleep 2;fi # FIXME: pending #7119 } # vim: filetype=sh diff --git a/test/system/400-unprivileged-access.bats b/test/system/400-unprivileged-access.bats index 98f8b82112..d020bf46ac 100644 --- a/test/system/400-unprivileged-access.bats +++ b/test/system/400-unprivileged-access.bats @@ -97,4 +97,74 @@ EOF run_podman rm c_uidmap c_uidmap_v } +# #6957 - mask out /proc/acpi, /sys/dev, and other sensitive system files +@test "sensitive mount points are masked without --privileged" { + # Weird error, maybe a flake? + # can only attach to created or running containers: container state improper + # https://github.com/containers/podman/pull/7111#issuecomment-666858715 + skip_if_remote "FIXME: Weird flake" + + # FIXME: this should match the list in pkg/specgen/generate/config_linux.go + local -a mps=( + /proc/acpi + /proc/kcore + /proc/keys + /proc/latency_stats + /proc/timer_list + /proc/timer_stats + /proc/sched_debug + /proc/scsi + /sys/firmware + /sys/fs/selinux + /sys/dev + ) + + # Some of the above may not exist on our host. Find only the ones that do. + local -a subset=() + for mp in ${mps[@]}; do + if [ -e $mp ]; then + subset+=($mp) + fi + done + + # Run 'stat' on all the files, plus /dev/null. Get path, file type, + # number of links, major, and minor (see below for why). Do it all + # in one go, to avoid multiple podman-runs + run_podman run --rm $IMAGE stat -c'%n:%F:%h:%T:%t' /dev/null ${subset[@]} + local devnull= + for result in "${lines[@]}"; do + # e.g. /proc/acpi:character special file:1:3:1 + local IFS=: + read path type nlinks major minor <<<"$result" + + if [[ $path = "/dev/null" ]]; then + # /dev/null is our reference point: masked *files* (not directories) + # will be created as /dev/null clones. + # This depends on 'stat' returning results in argv order, + # so /dev/null is first, so we have a reference for others. + # If that ever breaks, this test will have to be done in two passes. + devnull="$major:$minor" + elif [[ $type = "character special file" ]]; then + # Container file is a character device: it must match /dev/null + is "$major:$minor" "$devnull" "$path: major/minor matches /dev/null" + elif [[ $type = "directory" ]]; then + # Directories: must be empty (only two links). + # FIXME: this is a horrible almost-worthless test! It does not + # actually check for files in the directory (expect: zero), + # merely for the nonexistence of any subdirectories! It relies + # on the observed (by Ed) fact that all the masked directories + # contain further subdirectories on the host. If there's ever + # a new masked directory that contains only files, this test + # will silently pass without any indication of error. + # If you can think of a better way to do this check, + # please feel free to fix it. + is "$nlinks" "2" "$path: directory link count" + else + die "$path: Unknown file type '$type'" + fi + done + + if is_remote; then sleep 2;fi # FIXME: pending #7119 +} + # vim: filetype=sh diff --git a/test/system/410-selinux.bats b/test/system/410-selinux.bats index 3dca596418..1501f85546 100644 --- a/test/system/410-selinux.bats +++ b/test/system/410-selinux.bats @@ -16,6 +16,7 @@ function check_label() { # FIXME: it'd be nice to specify the command to run, e.g. 'ls -dZ /', # but alpine ls (from busybox) doesn't support -Z run_podman run --rm $args $IMAGE cat -v /proc/self/attr/current + if is_remote; then sleep 2;fi # FIXME: pending #7119 # FIXME: on some CI systems, 'run --privileged' emits a spurious # warning line about dup devices. Ignore it. diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 5afe147184..78326e6b70 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -234,12 +234,29 @@ function is_remote() { [[ "$PODMAN" =~ -remote ]] } +########################### +# _add_label_if_missing # make sure skip messages include rootless/remote +########################### +function _add_label_if_missing() { + local msg="$1" + local want="$2" + + if [ -z "$msg" ]; then + echo + elif expr "$msg" : ".*$want" &>/dev/null; then + echo "$msg" + else + echo "[$want] $msg" + fi +} + ###################### # skip_if_rootless # ...with an optional message ###################### function skip_if_rootless() { if is_rootless; then - skip "${1:-not applicable under rootless podman}" + local msg=$(_add_label_if_missing "$1" "rootless") + skip "${msg:-not applicable under rootless podman}" fi } @@ -248,7 +265,8 @@ function skip_if_rootless() { #################### function skip_if_remote() { if is_remote; then - skip "${1:-test does not work with podman-remote}" + local msg=$(_add_label_if_missing "$1" "remote") + skip "${msg:-test does not work with podman-remote}" fi } From 43527de53e40b737333502f6fbb2e1d73a3f3ec9 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Tue, 4 Aug 2020 13:41:44 -0500 Subject: [PATCH 14/25] correct go-binding key for volumes the go binding for remove container was using 'vols' for a key to remove volumes associated to the container. the correct key should be "v" and is documented as such. Fixes: #7128 Signed-off-by: Brent Baude --- pkg/bindings/containers/containers.go | 2 +- test/system/160-volumes.bats | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/bindings/containers/containers.go b/pkg/bindings/containers/containers.go index c690ea125c..c479e5dcb5 100644 --- a/pkg/bindings/containers/containers.go +++ b/pkg/bindings/containers/containers.go @@ -98,7 +98,7 @@ func Remove(ctx context.Context, nameOrID string, force, volumes *bool) error { params.Set("force", strconv.FormatBool(*force)) } if volumes != nil { - params.Set("vols", strconv.FormatBool(*volumes)) + params.Set("v", strconv.FormatBool(*volumes)) } response, err := conn.DoRequest(nil, http.MethodDelete, "/containers/%s", params, nil, nameOrID) if err != nil { diff --git a/test/system/160-volumes.bats b/test/system/160-volumes.bats index ef38b2a687..e2aefed431 100644 --- a/test/system/160-volumes.bats +++ b/test/system/160-volumes.bats @@ -142,8 +142,6 @@ EOF # Anonymous temporary volumes, and persistent autocreated named ones @test "podman volume, implicit creation with run" { - skip_if_remote "FIXME: pending #7128" - # No hostdir arg: create anonymous container with random name rand=$(random_string) run_podman run -v /myvol $IMAGE sh -c "echo $rand >/myvol/myfile" From 5b4952395bd7499f45f24a871009c235ba47ca0b Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Tue, 4 Aug 2020 16:24:34 -0400 Subject: [PATCH 15/25] Handle podman-remote run --rm We need to remove the container after it has exited for podman-remote run --rm commands. If we don't remove this container at this step, we open ourselves up to race conditions. Signed-off-by: Daniel J Walsh --- pkg/domain/infra/tunnel/containers.go | 14 +++++++++++--- test/system/030-run.bats | 2 -- test/system/070-build.bats | 1 - test/system/160-volumes.bats | 4 ---- test/system/200-pod.bats | 3 --- test/system/300-cli-parsing.bats | 2 -- test/system/400-unprivileged-access.bats | 2 -- test/system/410-selinux.bats | 1 - 8 files changed, 11 insertions(+), 18 deletions(-) diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 4ee709e376..8835248caa 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -500,9 +500,6 @@ func (ic *ContainerEngine) ContainerList(ctx context.Context, options entities.C } func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.ContainerRunOptions) (*entities.ContainerRunReport, error) { - if opts.Rm { - logrus.Info("the remote client does not support --rm yet") - } con, err := containers.CreateWithSpec(ic.ClientCxt, opts.Spec) if err != nil { return nil, err @@ -526,6 +523,17 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta if err != nil { report.ExitCode = define.ExitCode(err) } + if opts.Rm { + if err := containers.Remove(ic.ClientCxt, con.ID, bindings.PFalse, bindings.PTrue); err != nil { + if errors.Cause(err) == define.ErrNoSuchCtr || + errors.Cause(err) == define.ErrCtrRemoved { + logrus.Warnf("Container %s does not exist: %v", con.ID, err) + } else { + logrus.Errorf("Error removing container %s: %v", con.ID, err) + } + } + } + return &report, err } diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 4f707dda32..12c82bc4c2 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -84,8 +84,6 @@ echo $rand | 0 | $rand # Believe it or not, 'sh -c' resulted in different behavior run_podman 0 run --rm $IMAGE sh -c /bin/true run_podman 1 run --rm $IMAGE sh -c /bin/false - - if is_remote; then sleep 2;fi # FIXME: pending #7119 } @test "podman run --name" { diff --git a/test/system/070-build.bats b/test/system/070-build.bats index 6ec6b09d95..bdc05a172f 100644 --- a/test/system/070-build.bats +++ b/test/system/070-build.bats @@ -91,7 +91,6 @@ ADD https://github.com/containers/libpod/blob/master/README.md /tmp/ EOF run_podman build -t add_url $tmpdir run_podman run --rm add_url stat /tmp/README.md - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman rmi -f add_url # Now test COPY. That should fail. diff --git a/test/system/160-volumes.bats b/test/system/160-volumes.bats index e2aefed431..3f50bd3c4e 100644 --- a/test/system/160-volumes.bats +++ b/test/system/160-volumes.bats @@ -93,7 +93,6 @@ Labels.l | $mylabel is "$(<$mountpoint/myfile)" "$rand" "we see content created in container" # Clean up - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvolume } @@ -135,7 +134,6 @@ EOF is "$output" "got here -$rand-" "script in volume is runnable with default (exec)" # Clean up - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvolume } @@ -173,7 +171,6 @@ EOF run_podman run --rm -v $myvol:/myvol:z $IMAGE \ sh -c "cp /myvol/myfile /myvol/myfile2" - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvol # Autocreated volumes should also work with keep-id @@ -182,7 +179,6 @@ EOF run_podman run --rm -v $myvol:/myvol:z --userns=keep-id $IMAGE \ touch /myvol/myfile - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman volume rm $myvol } diff --git a/test/system/200-pod.bats b/test/system/200-pod.bats index 6680a896d4..93a7d7b5ee 100644 --- a/test/system/200-pod.bats +++ b/test/system/200-pod.bats @@ -93,7 +93,6 @@ function teardown() { is "$output" "$message" "message sent from one container to another" # Clean up. First the nc -l container... - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman rm $cid1 # ...then, from pause container, find the image ID of the pause image... @@ -104,7 +103,6 @@ function teardown() { pause_iid="$output" # ...then rm the pod, then rmi the pause image so we don't leave strays. - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman pod rm $podname run_podman rmi $pause_iid @@ -202,7 +200,6 @@ function random_ip() { is "$output" ".*options $dns_opt" "--dns-opt was added" # pod inspect - if is_remote; then sleep 2;fi # FIXME: pending #7119 run_podman pod inspect --format '{{.Name}}: {{.ID}} : {{.NumContainers}} : {{.Labels}}' mypod is "$output" "mypod: $pod_id : 1 : map\[${labelname}:${labelvalue}]" \ "pod inspect --format ..." diff --git a/test/system/300-cli-parsing.bats b/test/system/300-cli-parsing.bats index 2abc01bb79..92c0731023 100644 --- a/test/system/300-cli-parsing.bats +++ b/test/system/300-cli-parsing.bats @@ -10,8 +10,6 @@ load helpers # Error: invalid argument "true=\"false\"" for "-l, --label" \ # flag: parse error on line 1, column 5: bare " in non-quoted-field run_podman run --rm --label 'true="false"' $IMAGE true - - if is_remote; then sleep 2;fi # FIXME: pending #7119 } # vim: filetype=sh diff --git a/test/system/400-unprivileged-access.bats b/test/system/400-unprivileged-access.bats index d020bf46ac..acf0f0ba2c 100644 --- a/test/system/400-unprivileged-access.bats +++ b/test/system/400-unprivileged-access.bats @@ -163,8 +163,6 @@ EOF die "$path: Unknown file type '$type'" fi done - - if is_remote; then sleep 2;fi # FIXME: pending #7119 } # vim: filetype=sh diff --git a/test/system/410-selinux.bats b/test/system/410-selinux.bats index 1501f85546..3dca596418 100644 --- a/test/system/410-selinux.bats +++ b/test/system/410-selinux.bats @@ -16,7 +16,6 @@ function check_label() { # FIXME: it'd be nice to specify the command to run, e.g. 'ls -dZ /', # but alpine ls (from busybox) doesn't support -Z run_podman run --rm $args $IMAGE cat -v /proc/self/attr/current - if is_remote; then sleep 2;fi # FIXME: pending #7119 # FIXME: on some CI systems, 'run --privileged' emits a spurious # warning line about dup devices. Ignore it. From c24c64e89e8a32b4fa24385aa540ee00f5d0c991 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 3 Aug 2020 09:48:13 +0200 Subject: [PATCH 16/25] podman.service: drop install section podman.service is socket activated through podman.socket. It should not have its own [Install] section, it does not make sense to systemctl enable podman.service. This leads to podman.service always running on a Debian system, as Debian's policy is to enable/start running services by default. We don't want a daemon :^) Fixes: #7190 Reported-by: @martinpitt Signed-off-by: Valentin Rothberg --- contrib/systemd/system/podman.service | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contrib/systemd/system/podman.service b/contrib/systemd/system/podman.service index 4a63735a3b..c8751168db 100644 --- a/contrib/systemd/system/podman.service +++ b/contrib/systemd/system/podman.service @@ -8,7 +8,3 @@ StartLimitIntervalSec=0 [Service] Type=simple ExecStart=/usr/bin/podman system service - -[Install] -WantedBy=multi-user.target -Also=podman.socket From 8931e99a18cc168ebee07bb72b62849815af7434 Mon Sep 17 00:00:00 2001 From: zhangguanzhang Date: Sun, 2 Aug 2020 21:08:13 +0800 Subject: [PATCH 17/25] API returns 500 in case network is not found instead of 404 Backported-by: Valentin Rothberg Signed-off-by: zhangguanzhang --- 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 diff --git a/libpod/define/errors.go b/libpod/define/errors.go index 1e91793530..5cadce3960 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 2e11c0edb2..ad15be2700 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 12409bf502..0164d5d13c 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 00d09ac116..5e77b40492 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 a504e0ad0d..0115433e1a 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 beb3289f3c..f174a762ca 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 cbebb0be8b..37f3f721a7 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 0000000000..fff3f3b1f7 --- /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 From d0b6e258bfcd2c15d8954aeae38e630b49c8f803 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Sat, 1 Aug 2020 14:17:38 +0200 Subject: [PATCH 18/25] Change recommended systemd unit path for root. `/usr/lib/systemd/system` should only be used by the package manager administrators should use: `/etc/systemd/system` or `/usr/local/lib/systemd/system` see: man systemd.unit Signed-off-by: Paul Holzinger --- docs/source/markdown/podman-generate-systemd.1.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/source/markdown/podman-generate-systemd.1.md b/docs/source/markdown/podman-generate-systemd.1.md index 466c7e2bf8..d0b1b35889 100644 --- a/docs/source/markdown/podman-generate-systemd.1.md +++ b/docs/source/markdown/podman-generate-systemd.1.md @@ -149,9 +149,9 @@ WantedBy=multi-user.target default.target Podman-generated unit files include an `[Install]` section, which carries installation information for the unit. It is used by the enable and disable commands of systemctl(1) during installation. -Once you have generated the systemd unit file, you can copy the generated systemd file to ```/usr/lib/systemd/system``` for installing as a root user and to ```$HOME/.config/systemd/user ``` for installing it as a non-root user. Enable the copied unit file or files using `systemctl enable`. +Once you have generated the systemd unit file, you can copy the generated systemd file to ```/etc/systemd/system``` for installing as a root user and to ```$HOME/.config/systemd/user``` for installing it as a non-root user. Enable the copied unit file or files using `systemctl enable`. -Note: Coping unit files to ```/usr/lib/systemd/system``` and enabling it marks the unit file to be automatically started at boot. And smillarly, coping a unit file to ```$HOME/.config/systemd/user ``` and enabling it marks the unit file to be automatically started on user login. +Note: Coping unit files to ```/etc/systemd/system``` and enabling it marks the unit file to be automatically started at boot. And smillarly, coping a unit file to ```$HOME/.config/systemd/user``` and enabling it marks the unit file to be automatically started on user login. ``` @@ -162,14 +162,14 @@ $ podman generate systemd --files --name systemd-pod # Copy all the generated files. -$ sudo cp pod-systemd-pod.service container-great_payne.service /usr/lib/systemd/system +$ sudo cp pod-systemd-pod.service container-great_payne.service /etc/systemd/system $ systemctl enable pod-systemd-pod.service -Created symlink /etc/systemd/system/multi-user.target.wants/pod-systemd-pod.service → /usr/lib/systemd/system/pod-systemd-pod.service. -Created symlink /etc/systemd/system/default.target.wants/pod-systemd-pod.service → /usr/lib/systemd/system/pod-systemd-pod.service. +Created symlink /etc/systemd/system/multi-user.target.wants/pod-systemd-pod.service → /etc/systemd/system/pod-systemd-pod.service. +Created symlink /etc/systemd/system/default.target.wants/pod-systemd-pod.service → /etc/systemd/system/pod-systemd-pod.service. $ systemctl is-enabled pod-systemd-pod.service enabled ``` -To run the user services placed in `$HOME/.config/systemd/user/` on first login of that user, enable the service with --user flag. +To run the user services placed in `$HOME/.config/systemd/user` on first login of that user, enable the service with --user flag. ``` $ systemctl --user enable <.service> From 1752851958145f65e50cef5fab3e114487fcb63a Mon Sep 17 00:00:00 2001 From: Jhon Honce Date: Thu, 30 Jul 2020 08:16:14 -0700 Subject: [PATCH 19/25] Add versioned _ping endpoint Fixes #7008 Signed-off-by: Jhon Honce --- pkg/api/server/register_ping.go | 9 ++++----- test/apiv2/01-basic.at | 8 +++++++- test/apiv2/rest_api/test_rest_v1_0_0.py | 21 ++++++++++++++++++--- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/pkg/api/server/register_ping.go b/pkg/api/server/register_ping.go index 70e88ee00a..d4ae78e74b 100644 --- a/pkg/api/server/register_ping.go +++ b/pkg/api/server/register_ping.go @@ -9,9 +9,8 @@ import ( func (s *APIServer) registerPingHandlers(r *mux.Router) error { - r.Handle("/_ping", s.APIHandler(compat.Ping)).Methods(http.MethodGet) - r.Handle("/_ping", s.APIHandler(compat.Ping)).Methods(http.MethodHead) - + r.Handle("/_ping", s.APIHandler(compat.Ping)).Methods(http.MethodGet, http.MethodHead) + r.Handle(VersionedPath("/_ping"), s.APIHandler(compat.Ping)).Methods(http.MethodGet, http.MethodHead) // swagger:operation GET /libpod/_ping libpod libpodPingGet // --- // summary: Ping service @@ -62,7 +61,7 @@ func (s *APIServer) registerPingHandlers(r *mux.Router) error { // determine if talking to Podman engine or another engine // 500: // $ref: "#/responses/InternalError" - r.Handle("/libpod/_ping", s.APIHandler(compat.Ping)).Methods(http.MethodGet) - r.Handle("/libpod/_ping", s.APIHandler(compat.Ping)).Methods(http.MethodHead) + r.Handle("/libpod/_ping", s.APIHandler(compat.Ping)).Methods(http.MethodGet, http.MethodHead) + r.Handle(VersionedPath("/libpod/_ping"), s.APIHandler(compat.Ping)).Methods(http.MethodGet, http.MethodHead) return nil } diff --git a/test/apiv2/01-basic.at b/test/apiv2/01-basic.at index 79dac990a3..96b6aef7c6 100644 --- a/test/apiv2/01-basic.at +++ b/test/apiv2/01-basic.at @@ -5,9 +5,15 @@ # NOTE: paths with a leading slash will be interpreted as-is; # paths without will have '/v1.40/' prepended. -t GET /_ping 200 OK +t GET /_ping 200 OK t HEAD /_ping 200 t GET /libpod/_ping 200 OK +t HEAD /libpod/_ping 200 + +t GET _ping 200 OK +t HEAD _ping 200 +t GET libpod/_ping 200 OK +t HEAD libpod/_ping 200 for i in /version version; do t GET $i 200 \ diff --git a/test/apiv2/rest_api/test_rest_v1_0_0.py b/test/apiv2/rest_api/test_rest_v1_0_0.py index 7c53623cbd..2e574e0150 100644 --- a/test/apiv2/rest_api/test_rest_v1_0_0.py +++ b/test/apiv2/rest_api/test_rest_v1_0_0.py @@ -13,9 +13,11 @@ import requests from dateutil.parser import parse +PODMAN_URL = "http://localhost:8080" + def _url(path): - return "http://localhost:8080/v1.0.0/libpod" + path + return PODMAN_URL + "/v1.0.0/libpod" + path def podman(): @@ -205,7 +207,21 @@ def do_search(): search.join(timeout=10) self.assertFalse(search.is_alive(), "/images/search took too long") - def validateObjectFields(self, buffer): + def test_ping(self): + r = requests.get(PODMAN_URL + "/_ping") + self.assertEqual(r.status_code, 200, r.text) + + r = requests.head(PODMAN_URL + "/_ping") + self.assertEqual(r.status_code, 200, r.text) + + r = requests.get(_url("/_ping")) + self.assertEqual(r.status_code, 200, r.text) + + r = requests.get(_url("/_ping")) + self.assertEqual(r.status_code, 200, r.text) + + +def validateObjectFields(self, buffer): objs = json.loads(buffer) if not isinstance(objs, dict): for o in objs: @@ -214,6 +230,5 @@ def validateObjectFields(self, buffer): _ = objs["Id"] return objs - if __name__ == '__main__': unittest.main() From 3262f77406c597652e5cdbf6585a0c4f3bb30436 Mon Sep 17 00:00:00 2001 From: Jhon Honce Date: Fri, 31 Jul 2020 10:11:21 -0700 Subject: [PATCH 20/25] Fix podman service --valink timeout Documentation and unit files call for a millisecond timeout while the code was using a second resolution. Code change is smaller given varlink has been deprecated. Signed-off-by: Jhon Honce --- cmd/podman/system/service.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/podman/system/service.go b/cmd/podman/system/service.go index 312fcda19b..28080bc4dc 100644 --- a/cmd/podman/system/service.go +++ b/cmd/podman/system/service.go @@ -49,7 +49,7 @@ func init() { flags := srvCmd.Flags() flags.Int64VarP(&srvArgs.Timeout, "time", "t", 5, "Time until the service session expires in seconds. Use 0 to disable the timeout") - flags.BoolVar(&srvArgs.Varlink, "varlink", false, "Use legacy varlink service instead of REST") + flags.BoolVar(&srvArgs.Varlink, "varlink", false, "Use legacy varlink service instead of REST. Unit of --time changes from seconds to milliseconds.") _ = flags.MarkDeprecated("varlink", "valink API is deprecated.") flags.SetNormalizeFunc(aliasTimeoutFlag) @@ -88,14 +88,15 @@ func service(cmd *cobra.Command, args []string) error { opts := entities.ServiceOptions{ URI: apiURI, - Timeout: time.Duration(srvArgs.Timeout) * time.Second, Command: cmd, } if srvArgs.Varlink { + opts.Timeout = time.Duration(srvArgs.Timeout) * time.Millisecond return registry.ContainerEngine().VarlinkService(registry.GetContext(), opts) } + opts.Timeout = time.Duration(srvArgs.Timeout) * time.Second return restService(opts, cmd.Flags(), registry.PodmanConfig()) } From 895e0d0e2ea74cbba4a4a351e865c55f313bdf99 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 29 Jul 2020 21:58:46 +0200 Subject: [PATCH 21/25] fix pod creation with "new:" syntax When you execute podman create/run with the --pod new: syntax the pod was created but the namespaces where not shared and therefore containers could not communicate over localhost. Add the default namespaces and pass the network options to the pod create options. Signed-off-by: Paul Holzinger --- cmd/podman/containers/create.go | 13 ++++++------- cmd/podman/containers/run.go | 2 +- pkg/specgen/generate/namespaces.go | 4 ++++ test/e2e/run_test.go | 6 +++++- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go index 41e63da76b..0cd56f5408 100644 --- a/cmd/podman/containers/create.go +++ b/cmd/podman/containers/create.go @@ -124,7 +124,7 @@ func create(cmd *cobra.Command, args []string) error { return err } - if _, err := createPodIfNecessary(s); err != nil { + if _, err := createPodIfNecessary(s, cliVals.Net); err != nil { return err } @@ -279,7 +279,7 @@ func openCidFile(cidfile string) (*os.File, error) { // createPodIfNecessary automatically creates a pod when requested. if the pod name // has the form new:ID, the pod ID is created and the name in the spec generator is replaced // with ID. -func createPodIfNecessary(s *specgen.SpecGenerator) (*entities.PodCreateReport, error) { +func createPodIfNecessary(s *specgen.SpecGenerator, netOpts *entities.NetOptions) (*entities.PodCreateReport, error) { if !strings.HasPrefix(s.Pod, "new:") { return nil, nil } @@ -288,11 +288,10 @@ func createPodIfNecessary(s *specgen.SpecGenerator) (*entities.PodCreateReport, return nil, errors.Errorf("new pod name must be at least one character") } createOptions := entities.PodCreateOptions{ - Name: podName, - Infra: true, - Net: &entities.NetOptions{ - PublishPorts: s.PortMappings, - }, + Name: podName, + Infra: true, + Net: netOpts, + CreateCommand: os.Args, } s.Pod = podName return registry.ContainerEngine().PodCreate(context.Background(), createOptions) diff --git a/cmd/podman/containers/run.go b/cmd/podman/containers/run.go index 638b1c96e4..d8ffa57256 100644 --- a/cmd/podman/containers/run.go +++ b/cmd/podman/containers/run.go @@ -173,7 +173,7 @@ func run(cmd *cobra.Command, args []string) error { } runOpts.Spec = s - if _, err := createPodIfNecessary(s); err != nil { + if _, err := createPodIfNecessary(s, cliVals.Net); err != nil { return err } diff --git a/pkg/specgen/generate/namespaces.go b/pkg/specgen/generate/namespaces.go index 566830cd84..39a45398d1 100644 --- a/pkg/specgen/generate/namespaces.go +++ b/pkg/specgen/generate/namespaces.go @@ -452,6 +452,10 @@ func specConfigureNamespaces(s *specgen.SpecGenerator, g *generate.Generator, rt func GetNamespaceOptions(ns []string) ([]libpod.PodCreateOption, error) { var options []libpod.PodCreateOption var erroredOptions []libpod.PodCreateOption + if ns == nil { + //set the default namespaces + ns = strings.Split(specgen.DefaultKernelNamespaces, ",") + } for _, toShare := range ns { switch toShare { case "cgroup": diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go index d2950ed434..95eecd0425 100644 --- a/test/e2e/run_test.go +++ b/test/e2e/run_test.go @@ -797,7 +797,11 @@ USER mail` }) It("podman run --pod automatically", func() { - session := podmanTest.Podman([]string{"run", "--pod", "new:foobar", ALPINE, "ls"}) + session := podmanTest.Podman([]string{"run", "-d", "--pod", "new:foobar", ALPINE, "nc", "-l", "-p", "8080"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"run", "--pod", "foobar", ALPINE, "/bin/sh", "-c", "echo test | nc -w 1 127.0.0.1 8080"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) From 92d01d76c63d0a286365f610151524fb24b74e16 Mon Sep 17 00:00:00 2001 From: Qi Wang Date: Fri, 31 Jul 2020 08:26:50 -0400 Subject: [PATCH 22/25] 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 --- libpod/oci_conmon_exec_linux.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go index 1a6fe827b9..9fe7616389 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 From 3dfd8630a51a37734ad8c51162c4d004b8ffffb2 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Tue, 30 Jun 2020 15:44:14 -0400 Subject: [PATCH 23/25] 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 --- 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 diff --git a/cmd/podman/common/specgen.go b/cmd/podman/common/specgen.go index 7716fc1505..2333f2f7e4 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 6a87ad3a9b..9ad938a5c1 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 cfcf9b8233..f86903fd1b 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 0000000000..078cc53a73 --- /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 bff3f3c185..560b406e29 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 39a45398d1..22670ca615 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 c0d98f7b15..24d3e42eb2 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() From ca00601b14f2253e5b9f89280d39c5d498efc9f3 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 15 Jul 2020 15:25:12 -0400 Subject: [PATCH 24/25] 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 --- 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(-) diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index f86903fd1b..d72e3ad477 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 d7f3233ef5..6caf8a094a 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 24d3e42eb2..fef125e600 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()) From 3f2cab86433859a1facf1996ad68dac23c9899b9 Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Tue, 23 Jun 2020 06:06:48 -0600 Subject: [PATCH 25/25] system tests: invoke with abs path to podman Reversion of one part of #6679: my handling of 'realpath' would not work when $PODMAN is 'podman-remote --url etc'. Trying to handle that case got unmaintainable; so instead let's just force 'make {local,remote}system' to invoke with a full PODMAN path. This breaks down if someone runs the tests with a manual 'bats' invocation, but I think I'm the only one who ever does that. Since podman path will now be very long in the logs, add code to logformatter to abbreviate it like we do for the ginkgo logs. And, one thing that has bugged me for a long time: in the error logs, show a different prompt ('#' vs '$') to distinguish root vs rootless. This should make it much easier to see at-a-glance whether a log file is root or not. Add tests for it. Signed-off-by: Ed Santiago --- Makefile | 4 ++-- contrib/cirrus/logformatter | 4 ++++ contrib/cirrus/logformatter.t | 4 ++++ test/system/helpers.bash | 8 +++++++- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 3e4708beae..bef7fe2a2b 100644 --- a/Makefile +++ b/Makefile @@ -352,7 +352,7 @@ remoteintegration: varlink_generate test-binaries ginkgo-remote localsystem: # Wipe existing config, database, and cache: start with clean slate. $(RM) -rf ${HOME}/.local/share/containers ${HOME}/.config/containers - if timeout -v 1 true; then PODMAN=./bin/podman bats test/system/; else echo "Skipping $@: 'timeout -v' unavailable'"; fi + if timeout -v 1 true; then PODMAN=$(shell pwd)/bin/podman bats test/system/; else echo "Skipping $@: 'timeout -v' unavailable'"; fi .PHONY: remotesystem remotesystem: @@ -379,7 +379,7 @@ remotesystem: echo "Error: ./bin/podman system service did not come up on $$SOCK_FILE" >&2;\ exit 1;\ fi;\ - env PODMAN="./bin/podman-remote --url $$PODMAN_SOCKET" bats test/system/ ;\ + env PODMAN="$(shell pwd)/bin/podman-remote --url $$PODMAN_SOCKET" bats test/system/ ;\ rc=$$?;\ kill %1;\ rm -f $$SOCK_FILE;\ diff --git a/contrib/cirrus/logformatter b/contrib/cirrus/logformatter index 4bfe7b97f3..60c1e5985f 100755 --- a/contrib/cirrus/logformatter +++ b/contrib/cirrus/logformatter @@ -231,6 +231,10 @@ END_HTML if ($looks_like_bats) { my $css; + # Readability: /long/path/to/podman -> podman (hover for full path) + $line =~ s{^(#\s+(#|\$)\s+)(\S+/)(podman\S*)\s} + {$1$4 }; + if ($line =~ /^ok\s.*\s# skip/) { $css = 'skipped' } elsif ($line =~ /^ok\s/) { $css = 'passed' } elsif ($line =~ /^not\s+ok\s/) { $css = 'failed' } diff --git a/contrib/cirrus/logformatter.t b/contrib/cirrus/logformatter.t index 79c4563c2e..d2193cc6ca 100755 --- a/contrib/cirrus/logformatter.t +++ b/contrib/cirrus/logformatter.t @@ -88,12 +88,16 @@ __END__ ok 1 hi ok 2 bye # skip no reason not ok 3 fail +# $ /path/to/podman foo -bar +# #| FAIL: exit code is 123; expected 321 ok 4 blah >>> 1..4 ok 1 hi ok 2 bye # skip no reason not ok 3 fail +# $ podman foo -bar +# #| FAIL: exit code is 123; expected 321 ok 4 blah
Summary: 2 Passed, 1 Failed, 1 Skipped. Total tests: 4 diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 78326e6b70..0975effe74 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -16,6 +16,12 @@ IMAGE=$PODMAN_TEST_IMAGE_FQN # Default timeout for a podman command. PODMAN_TIMEOUT=${PODMAN_TIMEOUT:-60} +# Prompt to display when logging podman commands; distinguish root/rootless +_LOG_PROMPT='$' +if [ $(id -u) -eq 0 ]; then + _LOG_PROMPT='#' +fi + ############################################################################### # BEGIN setup/teardown tools @@ -132,7 +138,7 @@ function run_podman() { esac # stdout is only emitted upon error; this echo is to help a debugger - echo "\$ $PODMAN $*" + echo "$_LOG_PROMPT $PODMAN $*" # BATS hangs if a subprocess remains and keeps FD 3 open; this happens # if podman crashes unexpectedly without cleaning up subprocesses. run timeout --foreground -v --kill=10 $PODMAN_TIMEOUT $PODMAN "$@" 3>/dev/null