From e7b7be5734c8f97475d50f696946a8a5c99104f1 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 6 Nov 2021 09:15:16 -0400 Subject: [PATCH 01/10] proxy: Add an API to fetch the config upconverted to OCI While the caller could fetch this today as a blob, it'd be in either docker or oci schema. In keeping with the model of having this proxy only expose OCI, add an API which uses the c/image logic to do the conversion. This is necessary for callers to get the diffIDs, and in general to implement something like an external `skopeo copy`. Signed-off-by: Colin Walters --- cmd/skopeo/proxy.go | 57 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 74d11686..70dc1115 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -83,9 +83,11 @@ import ( // protocolVersion is semantic version of the protocol used by this proxy. // The first version of the protocol has major version 0.2 to signify a -// departure from the original code which used HTTP. The minor version is 1 -// instead of 0 to help exercise semver parsers. -const protocolVersion = "0.2.1" +// departure from the original code which used HTTP. +// +// 0.2.1: Initial version +// 0.2.2: Added support for fetching image configuration as OCI +const protocolVersion = "0.2.2" // maxMsgSize is the current limit on a packet size. // Note that all non-metadata (i.e. payload data) is sent over a pipe. @@ -380,6 +382,53 @@ func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { return ret, nil } +// GetConfig returns a copy of the image configuration, converted to OCI format. +func (h *proxyHandler) GetConfig(args []interface{}) (replyBuf, error) { + h.lock.Lock() + defer h.lock.Unlock() + + var ret replyBuf + + if h.sysctx == nil { + return ret, fmt.Errorf("Must invoke Initialize") + } + if len(args) != 1 { + return ret, fmt.Errorf("invalid request, expecting: [imgid]") + } + imgref, err := h.parseImageFromID(args[0]) + if err != nil { + return ret, err + } + + ctx := context.TODO() + config, err := imgref.img.OCIConfig(ctx) + if err != nil { + return ret, err + } + serialized, err := json.Marshal(&config.Config) + if err != nil { + return ret, err + } + + piper, f, err := h.allocPipe() + if err != nil { + return ret, err + } + + go func() { + // Signal completion when we return + defer f.wg.Done() + _, err = io.Copy(f.w, bytes.NewReader(serialized)) + if err != nil { + f.err = err + } + }() + + ret.fd = piper + ret.pipeid = uint32(f.w.Fd()) + return ret, nil +} + // GetBlob fetches a blob, performing digest verification. func (h *proxyHandler) GetBlob(args []interface{}) (replyBuf, error) { h.lock.Lock() @@ -551,6 +600,8 @@ func (h *proxyHandler) processRequest(req request) (rb replyBuf, terminate bool, rb, err = h.CloseImage(req.Args) case "GetManifest": rb, err = h.GetManifest(req.Args) + case "GetConfig": + rb, err = h.GetConfig(req.Args) case "GetBlob": rb, err = h.GetBlob(req.Args) case "FinishPipe": From 6510f1011ba833c98c177f290cc137fe971dd4cb Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 6 Nov 2021 09:21:05 -0400 Subject: [PATCH 02/10] proxy: Add a helper to return a byte array Since this is shared between the manifest and config paths. Signed-off-by: Colin Walters --- cmd/skopeo/proxy.go | 61 +++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 70dc1115..961dfd5c 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -302,6 +302,30 @@ func (h *proxyHandler) allocPipe() (*os.File, *activePipe, error) { return piper, &f, nil } +// returnBytes generates a return pipe() from a byte array +// In the future it might be nicer to return this via memfd_create() +func (h *proxyHandler) returnBytes(retval interface{}, buf []byte) (replyBuf, error) { + var ret replyBuf + piper, f, err := h.allocPipe() + if err != nil { + return ret, err + } + + go func() { + // Signal completion when we return + defer f.wg.Done() + _, err = io.Copy(f.w, bytes.NewReader(buf)) + if err != nil { + f.err = err + } + }() + + ret.value = retval + ret.fd = piper + ret.pipeid = uint32(f.w.Fd()) + return ret, nil +} + // GetManifest returns a copy of the manifest, converted to OCI format, along with the original digest. func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { h.lock.Lock() @@ -362,24 +386,7 @@ func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { } else { serialized = rawManifest } - piper, f, err := h.allocPipe() - if err != nil { - return ret, err - } - - go func() { - // Signal completion when we return - defer f.wg.Done() - _, err = io.Copy(f.w, bytes.NewReader(serialized)) - if err != nil { - f.err = err - } - }() - - ret.value = digest.String() - ret.fd = piper - ret.pipeid = uint32(f.w.Fd()) - return ret, nil + return h.returnBytes(digest, serialized) } // GetConfig returns a copy of the image configuration, converted to OCI format. @@ -409,24 +416,8 @@ func (h *proxyHandler) GetConfig(args []interface{}) (replyBuf, error) { if err != nil { return ret, err } + return h.returnBytes(nil, serialized) - piper, f, err := h.allocPipe() - if err != nil { - return ret, err - } - - go func() { - // Signal completion when we return - defer f.wg.Done() - _, err = io.Copy(f.w, bytes.NewReader(serialized)) - if err != nil { - f.err = err - } - }() - - ret.fd = piper - ret.pipeid = uint32(f.w.Fd()) - return ret, nil } // GetBlob fetches a blob, performing digest verification. From a3adf36db6a79c7550ab3cddd7c322e0c3ed1dc5 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 6 Nov 2021 09:25:00 -0400 Subject: [PATCH 03/10] =?UTF-8?q?proxy:=20Use=20float=20=E2=86=92=20int=20?= =?UTF-8?q?helper=20for=20pipeid?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just noticed while scrolling past the code. Signed-off-by: Colin Walters --- cmd/skopeo/proxy.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 961dfd5c..a0565f70 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -491,11 +491,11 @@ func (h *proxyHandler) FinishPipe(args []interface{}) (replyBuf, error) { var ret replyBuf - pipeidf, ok := args[0].(float64) - if !ok { - return ret, fmt.Errorf("finishpipe: expecting pipeid, not %T", args[0]) + pipeidv, err := parseUint64(args[0]) + if err != nil { + return ret, err } - pipeid := uint32(pipeidf) + pipeid := uint32(pipeidv) f, ok := h.activePipes[pipeid] if !ok { @@ -507,7 +507,7 @@ func (h *proxyHandler) FinishPipe(args []interface{}) (replyBuf, error) { // And only now do we close the write half; this forces the client to call this API f.w.Close() // Propagate any errors from the goroutine worker - err := f.err + err = f.err delete(h.activePipes, pipeid) return ret, err } From 83416068d36c6dd0ad3421eef5980e34d410009d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 6 Nov 2021 14:21:37 -0400 Subject: [PATCH 04/10] tests/integration/proxy_test: New test that exercises `proxy.go` I debated adding "reverse dependency testing" using https://crates.io/crates/containers-image-proxy but I think it's easier to reuse the test infrastructure here. This also starts fleshing out a Go client for the proxy (not that this is going to be something most Go projects would want versus vendoring c/image...but hey, maybe it'll be useful). Now what I hit in trying to use the main test images is currently the proxy fails on manifest lists, so I'll need to fix that. Signed-off-by: Colin Walters --- integration/proxy_test.go | 231 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 integration/proxy_test.go diff --git a/integration/proxy_test.go b/integration/proxy_test.go new file mode 100644 index 00000000..cbf6672f --- /dev/null +++ b/integration/proxy_test.go @@ -0,0 +1,231 @@ +package main + +import ( + "encoding/json" + "fmt" + "io/ioutil" + "net" + "os" + "os/exec" + "strings" + "syscall" + "time" + + "gopkg.in/check.v1" + + "github.com/containers/image/v5/manifest" +) + +// This image is known to be x86_64 only right now +const knownNotManifestListedImage_x8664 = "docker://quay.io/coreos/11bot" + +const expectedProxySemverMajor = "0.2" + +// request is copied from proxy.go +// We intentionally copy to ensure that we catch any unexpected "API" changes +// in the JSON. +type request struct { + // Method is the name of the function + Method string `json:"method"` + // Args is the arguments (parsed inside the fuction) + Args []interface{} `json:"args"` +} + +// reply is copied from proxy.go +type reply struct { + // Success is true if and only if the call succeeded. + Success bool `json:"success"` + // Value is an arbitrary value (or values, as array/map) returned from the call. + Value interface{} `json:"value"` + // PipeID is an index into open pipes, and should be passed to FinishPipe + PipeID uint32 `json:"pipeid"` + // Error should be non-empty if Success == false + Error string `json:"error"` +} + +// maxMsgSize is also copied from proxy.go +const maxMsgSize = 32 * 1024 + +type proxy struct { + c *net.UnixConn +} + +type pipefd struct { + // id is the remote identifier "pipeid" + id uint + fd *os.File +} + +func (self *proxy) call(method string, args []interface{}) (rval interface{}, fd *pipefd, err error) { + req := request{ + Method: method, + Args: args, + } + reqbuf, err := json.Marshal(&req) + if err != nil { + return + } + n, err := self.c.Write(reqbuf) + if err != nil { + return + } + if n != len(reqbuf) { + err = fmt.Errorf("short write during call of %d bytes", n) + return + } + oob := make([]byte, syscall.CmsgSpace(1)) + replybuf := make([]byte, maxMsgSize) + n, oobn, _, _, err := self.c.ReadMsgUnix(replybuf, oob) + if err != nil { + err = fmt.Errorf("reading reply: %v", err) + return + } + var reply reply + err = json.Unmarshal(replybuf[0:n], &reply) + if err != nil { + err = fmt.Errorf("Failed to parse reply: %w", err) + return + } + if !reply.Success { + err = fmt.Errorf("remote error: %s", reply.Error) + return + } + + if reply.PipeID > 0 { + var scms []syscall.SocketControlMessage + scms, err = syscall.ParseSocketControlMessage(oob[:oobn]) + if err != nil { + err = fmt.Errorf("failed to parse control message: %v", err) + return + } + if len(scms) != 1 { + err = fmt.Errorf("Expected 1 received fd, found %d", len(scms)) + return + } + var fds []int + fds, err = syscall.ParseUnixRights(&scms[0]) + if err != nil { + err = fmt.Errorf("failed to parse unix rights: %v", err) + return + } + fd = &pipefd{ + fd: os.NewFile(uintptr(fds[0]), "replyfd"), + id: uint(reply.PipeID), + } + } + + rval = reply.Value + return +} + +func newProxy() (*proxy, error) { + fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_SEQPACKET, 0) + if err != nil { + return nil, err + } + myfd := os.NewFile(uintptr(fds[0]), "myfd") + defer myfd.Close() + theirfd := os.NewFile(uintptr(fds[1]), "theirfd") + defer theirfd.Close() + + mysock, err := net.FileConn(myfd) + if err != nil { + return nil, err + } + + // Note ExtraFiles starts at 3 + proc := exec.Command("skopeo", "experimental-image-proxy", "--sockfd", "3") + proc.Stderr = os.Stderr + proc.SysProcAttr = &syscall.SysProcAttr{ + Pdeathsig: syscall.SIGTERM, + } + proc.ExtraFiles = append(proc.ExtraFiles, theirfd) + + if err = proc.Start(); err != nil { + return nil, err + } + + return &proxy{ + c: mysock.(*net.UnixConn), + }, nil +} + +func init() { + check.Suite(&ProxySuite{}) +} + +type ProxySuite struct { +} + +func (s *ProxySuite) SetUpSuite(c *check.C) { +} + +func (s *ProxySuite) TearDownSuite(c *check.C) { +} + +func initOci(p string) error { + err := ioutil.WriteFile(filepath.Join(p, "oci-layout"), []byte("{\"imageLayoutVersion\":\"1.0.0\"}"), 0644) + if err != nil { + return err + } + + blobdir := filepath.Join(p, "blobs/sha256") + err = os.MkdirAll(blobdir, 0755) + if err != nil { + return err + } + return nil +} + +type byteFetch struct { + content []byte + err error +} + +func (s *ProxySuite) TestProxy(c *check.C) { + p, err := newProxy() + c.Assert(err, check.IsNil) + + v, fd, err := p.call("Initialize", nil) + c.Assert(err, check.IsNil) + semver, ok := v.(string) + if !ok { + c.Fatalf("Unexpected value %T", v) + } + if !strings.HasPrefix(semver, expectedProxySemverMajor) { + c.Fatalf("Unexpected semver %s", semver) + } + c.Assert(fd, check.IsNil) + + v, fd, err = p.call("OpenImage", []interface{}{knownNotManifestListedImage_x8664}) + c.Assert(err, check.IsNil) + c.Assert(fd, check.IsNil) + + imgidv, ok := v.(float64) + c.Assert(ok, check.Equals, true) + imgid := uint32(imgidv) + + v, fd, err = p.call("GetManifest", []interface{}{imgid}) + c.Assert(err, check.IsNil) + c.Assert(fd, check.NotNil) + fetchchan := make(chan byteFetch) + go func() { + manifestBytes, err := ioutil.ReadAll(fd.fd) + fetchchan <- byteFetch{ + content: manifestBytes, + err: err, + } + }() + _, _, err = p.call("FinishPipe", []interface{}{fd.id}) + c.Assert(err, check.IsNil) + fetchRes := <-fetchchan + c.Assert(fetchRes.err, check.IsNil) + + _, err = manifest.OCI1FromManifest(fetchRes.content) + c.Assert(err, check.IsNil) + + td, err := ioutil.TempDir("", "skopeo-proxy") + defer os.RemoveAll(td) + + c.Assert(initOci(td), check.IsNil) +} From 644074cbb43e1733963b2e8bb7a20ae9e8cb739a Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 8 Nov 2021 09:24:43 -0500 Subject: [PATCH 05/10] proxy: Add support for manifest lists We need to support manifest lists. I'm not sure how I missed this originally. At least now we have integration tests that cover this. The issue here is fairly subtle - the way c/image works right now, `image.FromUnparsedImage` does pick a matching image from a list by default. But it also overrides `GetManifest()` to return the original manifest list, which defeats our goal here. Handle this by adding explicit manifest list support code. We'll want this anyways for future support for `GetRawManifest` or so which exposes OCI manifest lists to the client. Signed-off-by: Colin Walters --- cmd/skopeo/proxy.go | 70 +++++++++++++++++++++++++------ integration/proxy_test.go | 87 +++++++++++++++++++++++++++------------ 2 files changed, 118 insertions(+), 39 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index a0565f70..34d2509f 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -143,9 +143,9 @@ type activePipe struct { // openImage is an opened image reference type openImage struct { // id is an opaque integer handle - id uint32 - src types.ImageSource - img types.Image + id uint32 + src types.ImageSource + cachedimg types.Image } // proxyHandler is the state associated with our socket. @@ -219,16 +219,11 @@ func (h *proxyHandler) OpenImage(args []interface{}) (replyBuf, error) { if err != nil { return ret, err } - img, err := image.FromUnparsedImage(context.Background(), h.sysctx, image.UnparsedInstance(imgsrc, nil)) - if err != nil { - return ret, fmt.Errorf("failed to load image: %w", err) - } h.imageSerial++ openimg := &openImage{ id: h.imageSerial, src: imgsrc, - img: img, } h.images[openimg.id] = openimg ret.value = openimg.id @@ -326,7 +321,46 @@ func (h *proxyHandler) returnBytes(retval interface{}, buf []byte) (replyBuf, er return ret, nil } +// cacheTargetManifest is invoked when GetManifest or GetConfig is invoked +// the first time for a given image. If the requested image is a manifest +// list, this function resolves it to the image matching the calling process' +// operating system and architecture. +// +// TODO: Add GetRawManifest or so that exposes manifest lists +func (h *proxyHandler) cacheTargetManifest(img *openImage) error { + ctx := context.Background() + if img.cachedimg != nil { + return nil + } + unparsedToplevel := image.UnparsedInstance(img.src, nil) + mfest, manifestType, err := unparsedToplevel.Manifest(ctx) + if err != nil { + return err + } + var target *image.UnparsedImage + if manifest.MIMETypeIsMultiImage(manifestType) { + manifestList, err := manifest.ListFromBlob(mfest, manifestType) + if err != nil { + return err + } + instanceDigest, err := manifestList.ChooseInstance(h.sysctx) + if err != nil { + return err + } + target = image.UnparsedInstance(img.src, &instanceDigest) + } else { + target = unparsedToplevel + } + cachedimg, err := image.FromUnparsedImage(ctx, h.sysctx, target) + if err != nil { + return err + } + img.cachedimg = cachedimg + return nil +} + // GetManifest returns a copy of the manifest, converted to OCI format, along with the original digest. +// Manifest lists are resolved to the current operating system and architecture. func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { h.lock.Lock() defer h.lock.Unlock() @@ -344,11 +378,18 @@ func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { return ret, err } - ctx := context.TODO() - rawManifest, manifestType, err := imgref.img.Manifest(ctx) + err = h.cacheTargetManifest(imgref) if err != nil { return ret, err } + img := imgref.cachedimg + + ctx := context.Background() + rawManifest, manifestType, err := img.Manifest(ctx) + if err != nil { + return ret, err + } + // We only support OCI and docker2schema2. We know docker2schema2 can be easily+cheaply // converted into OCI, so consumers only need to see OCI. switch manifestType { @@ -373,7 +414,7 @@ func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { // docker schema and MIME types. if manifestType != imgspecv1.MediaTypeImageManifest { manifestUpdates := types.ManifestUpdateOptions{ManifestMIMEType: imgspecv1.MediaTypeImageManifest} - ociImage, err := imgref.img.UpdatedImage(ctx, manifestUpdates) + ociImage, err := img.UpdatedImage(ctx, manifestUpdates) if err != nil { return ret, err } @@ -406,9 +447,14 @@ func (h *proxyHandler) GetConfig(args []interface{}) (replyBuf, error) { if err != nil { return ret, err } + err = h.cacheTargetManifest(imgref) + if err != nil { + return ret, err + } + img := imgref.cachedimg ctx := context.TODO() - config, err := imgref.img.OCIConfig(ctx) + config, err := img.OCIConfig(ctx) if err != nil { return ret, err } diff --git a/integration/proxy_test.go b/integration/proxy_test.go index cbf6672f..10ad276a 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -145,9 +145,25 @@ func newProxy() (*proxy, error) { return nil, err } - return &proxy{ + p := &proxy{ c: mysock.(*net.UnixConn), - }, nil + } + + v, fd, err := p.call("Initialize", nil) + if err != nil { + return nil, err + } + if fd != nil { + return nil, fmt.Errorf("proxy Initialize: Unexpected fd") + } + semver, ok := v.(string) + if !ok { + return nil, fmt.Errorf("proxy Initialize: Unexpected value %T", v) + } + if !strings.HasPrefix(semver, expectedProxySemverMajor) { + return nil, fmt.Errorf("Unexpected semver %s", semver) + } + return p, nil } func init() { @@ -182,32 +198,28 @@ type byteFetch struct { err error } -func (s *ProxySuite) TestProxy(c *check.C) { - p, err := newProxy() - c.Assert(err, check.IsNil) - - v, fd, err := p.call("Initialize", nil) - c.Assert(err, check.IsNil) - semver, ok := v.(string) - if !ok { - c.Fatalf("Unexpected value %T", v) +func runTestGetManifest(p *proxy, img string) error { + v, fd, err := p.call("OpenImage", []interface{}{knownNotManifestListedImage_x8664}) + if err != nil { + return err } - if !strings.HasPrefix(semver, expectedProxySemverMajor) { - c.Fatalf("Unexpected semver %s", semver) + if fd != nil { + return fmt.Errorf("Unexpected fd") } - c.Assert(fd, check.IsNil) - - v, fd, err = p.call("OpenImage", []interface{}{knownNotManifestListedImage_x8664}) - c.Assert(err, check.IsNil) - c.Assert(fd, check.IsNil) imgidv, ok := v.(float64) - c.Assert(ok, check.Equals, true) + if !ok { + return fmt.Errorf("OpenImage return value is %T", v) + } imgid := uint32(imgidv) v, fd, err = p.call("GetManifest", []interface{}{imgid}) - c.Assert(err, check.IsNil) - c.Assert(fd, check.NotNil) + if err != nil { + return err + } + if fd == nil { + return fmt.Errorf("expected GetManifest fd") + } fetchchan := make(chan byteFetch) go func() { manifestBytes, err := ioutil.ReadAll(fd.fd) @@ -217,15 +229,36 @@ func (s *ProxySuite) TestProxy(c *check.C) { } }() _, _, err = p.call("FinishPipe", []interface{}{fd.id}) - c.Assert(err, check.IsNil) + if err != nil { + return err + } fetchRes := <-fetchchan - c.Assert(fetchRes.err, check.IsNil) - + if fetchRes.err != nil { + return err + } _, err = manifest.OCI1FromManifest(fetchRes.content) + if err != nil { + return err + } + + _, _, err = p.call("CloseImage", []interface{}{imgid}) + + return nil +} + +func (s *ProxySuite) TestProxy(c *check.C) { + p, err := newProxy() c.Assert(err, check.IsNil) - td, err := ioutil.TempDir("", "skopeo-proxy") - defer os.RemoveAll(td) + err = runTestGetManifest(p, knownNotManifestListedImage_x8664) + if err != nil { + err = fmt.Errorf("Testing image %s: %v", knownNotManifestListedImage_x8664, err) + } + c.Assert(err, check.IsNil) - c.Assert(initOci(td), check.IsNil) + err = runTestGetManifest(p, knownListImage) + if err != nil { + err = fmt.Errorf("Testing image %s: %v", knownListImage, err) + } + c.Assert(err, check.IsNil) } From f90725d80c29ddf11212e7a1942e3d7a5215c14e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 8 Nov 2021 09:34:49 -0500 Subject: [PATCH 06/10] proxy_test: Add a helper method to call without fd To verify in one place. Signed-off-by: Colin Walters --- integration/proxy_test.go | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/integration/proxy_test.go b/integration/proxy_test.go index 10ad276a..ae667f27 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -118,6 +118,19 @@ func (self *proxy) call(method string, args []interface{}) (rval interface{}, fd return } +func (self *proxy) callNoFd(method string, args []interface{}) (rval interface{}, err error) { + var fd *pipefd + rval, fd, err = self.call(method, args) + if err != nil { + return + } + if fd != nil { + err = fmt.Errorf("Unexpected fd from method %s", method) + return + } + return rval, nil +} + func newProxy() (*proxy, error) { fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_SEQPACKET, 0) if err != nil { @@ -149,13 +162,10 @@ func newProxy() (*proxy, error) { c: mysock.(*net.UnixConn), } - v, fd, err := p.call("Initialize", nil) + v, err := p.callNoFd("Initialize", nil) if err != nil { return nil, err } - if fd != nil { - return nil, fmt.Errorf("proxy Initialize: Unexpected fd") - } semver, ok := v.(string) if !ok { return nil, fmt.Errorf("proxy Initialize: Unexpected value %T", v) @@ -179,33 +189,16 @@ func (s *ProxySuite) SetUpSuite(c *check.C) { func (s *ProxySuite) TearDownSuite(c *check.C) { } -func initOci(p string) error { - err := ioutil.WriteFile(filepath.Join(p, "oci-layout"), []byte("{\"imageLayoutVersion\":\"1.0.0\"}"), 0644) - if err != nil { - return err - } - - blobdir := filepath.Join(p, "blobs/sha256") - err = os.MkdirAll(blobdir, 0755) - if err != nil { - return err - } - return nil -} - type byteFetch struct { content []byte err error } func runTestGetManifest(p *proxy, img string) error { - v, fd, err := p.call("OpenImage", []interface{}{knownNotManifestListedImage_x8664}) + v, err := p.callNoFd("OpenImage", []interface{}{knownNotManifestListedImage_x8664}) if err != nil { return err } - if fd != nil { - return fmt.Errorf("Unexpected fd") - } imgidv, ok := v.(float64) if !ok { @@ -213,7 +206,7 @@ func runTestGetManifest(p *proxy, img string) error { } imgid := uint32(imgidv) - v, fd, err = p.call("GetManifest", []interface{}{imgid}) + v, fd, err := p.call("GetManifest", []interface{}{imgid}) if err != nil { return err } From 2bb6f27d13dc27e5e5b9f94bcaee398f8304fb71 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 8 Nov 2021 09:38:46 -0500 Subject: [PATCH 07/10] proxy_test: Add helper to read all from a reply Prep for testing `GetConfig`. Signed-off-by: Colin Walters --- integration/proxy_test.go | 61 +++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/integration/proxy_test.go b/integration/proxy_test.go index ae667f27..7a7d8946 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -131,6 +131,42 @@ func (self *proxy) callNoFd(method string, args []interface{}) (rval interface{} return rval, nil } +func (self *proxy) callReadAllBytes(method string, args []interface{}) (rval interface{}, buf []byte, err error) { + var fd *pipefd + rval, fd, err = self.call(method, args) + if err != nil { + return + } + if fd == nil { + err = fmt.Errorf("Expected fd from method %s", method) + return + } + fetchchan := make(chan byteFetch) + go func() { + manifestBytes, err := ioutil.ReadAll(fd.fd) + fetchchan <- byteFetch{ + content: manifestBytes, + err: err, + } + }() + _, err = self.callNoFd("FinishPipe", []interface{}{fd.id}) + if err != nil { + return + } + select { + case fetchRes := <-fetchchan: + err = fetchRes.err + if err != nil { + return + } + + buf = fetchRes.content + case <-time.After(5 * time.Minute): + err = fmt.Errorf("timed out during proxy fetch") + } + return +} + func newProxy() (*proxy, error) { fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_SEQPACKET, 0) if err != nil { @@ -206,35 +242,16 @@ func runTestGetManifest(p *proxy, img string) error { } imgid := uint32(imgidv) - v, fd, err := p.call("GetManifest", []interface{}{imgid}) + v, manifestBytes, err := p.callReadAllBytes("GetManifest", []interface{}{imgid}) if err != nil { return err } - if fd == nil { - return fmt.Errorf("expected GetManifest fd") - } - fetchchan := make(chan byteFetch) - go func() { - manifestBytes, err := ioutil.ReadAll(fd.fd) - fetchchan <- byteFetch{ - content: manifestBytes, - err: err, - } - }() - _, _, err = p.call("FinishPipe", []interface{}{fd.id}) - if err != nil { - return err - } - fetchRes := <-fetchchan - if fetchRes.err != nil { - return err - } - _, err = manifest.OCI1FromManifest(fetchRes.content) + _, err = manifest.OCI1FromManifest(manifestBytes) if err != nil { return err } - _, _, err = p.call("CloseImage", []interface{}{imgid}) + _, err = p.callNoFd("CloseImage", []interface{}{imgid}) return nil } From fa86297c36286e21309952599cca20a30a2932d6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 8 Nov 2021 09:45:24 -0500 Subject: [PATCH 08/10] proxy_test: Test `GetConfig` Now that we have a test suite, let's use it more. Signed-off-by: Colin Walters --- integration/proxy_test.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/integration/proxy_test.go b/integration/proxy_test.go index 7a7d8946..30b33556 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -14,6 +14,7 @@ import ( "gopkg.in/check.v1" "github.com/containers/image/v5/manifest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" ) // This image is known to be x86_64 only right now @@ -230,7 +231,7 @@ type byteFetch struct { err error } -func runTestGetManifest(p *proxy, img string) error { +func runTestGetManifestAndConfig(p *proxy, img string) error { v, err := p.callNoFd("OpenImage", []interface{}{knownNotManifestListedImage_x8664}) if err != nil { return err @@ -251,6 +252,21 @@ func runTestGetManifest(p *proxy, img string) error { return err } + v, configBytes, err := p.callReadAllBytes("GetConfig", []interface{}{imgid}) + if err != nil { + return err + } + var config imgspecv1.ImageConfig + err = json.Unmarshal(configBytes, &config) + if err != nil { + return err + } + + // Validate that the config seems sane + if len(config.Cmd) == 0 && len(config.Entrypoint) == 0 { + return fmt.Errorf("No CMD or ENTRYPOINT set") + } + _, err = p.callNoFd("CloseImage", []interface{}{imgid}) return nil @@ -260,13 +276,13 @@ func (s *ProxySuite) TestProxy(c *check.C) { p, err := newProxy() c.Assert(err, check.IsNil) - err = runTestGetManifest(p, knownNotManifestListedImage_x8664) + err = runTestGetManifestAndConfig(p, knownNotManifestListedImage_x8664) if err != nil { err = fmt.Errorf("Testing image %s: %v", knownNotManifestListedImage_x8664, err) } c.Assert(err, check.IsNil) - err = runTestGetManifest(p, knownListImage) + err = runTestGetManifestAndConfig(p, knownListImage) if err != nil { err = fmt.Errorf("Testing image %s: %v", knownListImage, err) } From e9535f868b751f36534fed060170424cc19099db Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 8 Nov 2021 11:23:14 -0500 Subject: [PATCH 09/10] tests: Add new "procutils" that exposes PDEATHSIG To fix compilation on MacOS. I think actually we want to use this pervasively in our tests on Linux; it doesn't really matter when run inside a transient container, but `PDEATHSIG` is useful for persistent containers (e.g.) toolbox and when running outside of a pid namespace, e.g. on a host system shell directly or in systemd. Signed-off-by: Colin Walters --- integration/procutils.go | 12 ++++++++++++ integration/procutils_linux.go | 14 ++++++++++++++ integration/proxy_test.go | 4 +--- 3 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 integration/procutils.go create mode 100644 integration/procutils_linux.go diff --git a/integration/procutils.go b/integration/procutils.go new file mode 100644 index 00000000..c6a59f18 --- /dev/null +++ b/integration/procutils.go @@ -0,0 +1,12 @@ +//go:build !linux +// +build !linux + +package main + +import ( + "os/exec" +) + +// cmdLifecycleToParentIfPossible tries to exit if the parent process exits (only works on Linux) +func cmdLifecycleToParentIfPossible(c *exec.Cmd) { +} diff --git a/integration/procutils_linux.go b/integration/procutils_linux.go new file mode 100644 index 00000000..59a78cd9 --- /dev/null +++ b/integration/procutils_linux.go @@ -0,0 +1,14 @@ +package main + +import ( + "os/exec" + "syscall" +) + +// cmdLifecyleToParentIfPossible is a thin wrapper around prctl(PR_SET_PDEATHSIG) +// on Linux. +func cmdLifecycleToParentIfPossible(c *exec.Cmd) { + c.SysProcAttr = &syscall.SysProcAttr{ + Pdeathsig: syscall.SIGTERM, + } +} diff --git a/integration/proxy_test.go b/integration/proxy_test.go index 30b33556..b456289d 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -186,9 +186,7 @@ func newProxy() (*proxy, error) { // Note ExtraFiles starts at 3 proc := exec.Command("skopeo", "experimental-image-proxy", "--sockfd", "3") proc.Stderr = os.Stderr - proc.SysProcAttr = &syscall.SysProcAttr{ - Pdeathsig: syscall.SIGTERM, - } + cmdLifecycleToParentIfPossible(proc) proc.ExtraFiles = append(proc.ExtraFiles, theirfd) if err = proc.Start(); err != nil { From 05a2ed49216f7311db08dfa0c05ec6b6670b3052 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 10 Nov 2021 08:49:59 -0500 Subject: [PATCH 10/10] proxy: Uncapitalize all errors By Go convention. Signed-off-by: Colin Walters --- cmd/skopeo/proxy.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 34d2509f..dad201dd 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -201,14 +201,14 @@ func (h *proxyHandler) OpenImage(args []interface{}) (replyBuf, error) { var ret replyBuf if h.sysctx == nil { - return ret, fmt.Errorf("Must invoke Initialize") + return ret, fmt.Errorf("client error: must invoke Initialize") } if len(args) != 1 { return ret, fmt.Errorf("invalid request, expecting one argument") } imageref, ok := args[0].(string) if !ok { - return ret, fmt.Errorf("Expecting string imageref, not %T", args[0]) + return ret, fmt.Errorf("expecting string imageref, not %T", args[0]) } imgRef, err := alltransports.ParseImageName(imageref) @@ -237,7 +237,7 @@ func (h *proxyHandler) CloseImage(args []interface{}) (replyBuf, error) { var ret replyBuf if h.sysctx == nil { - return ret, fmt.Errorf("Must invoke Initialize") + return ret, fmt.Errorf("client error: must invoke Initialize") } if len(args) != 1 { return ret, fmt.Errorf("invalid request, expecting one argument") @@ -255,7 +255,7 @@ func (h *proxyHandler) CloseImage(args []interface{}) (replyBuf, error) { func parseImageID(v interface{}) (uint32, error) { imgidf, ok := v.(float64) if !ok { - return 0, fmt.Errorf("Expecting integer imageid, not %T", v) + return 0, fmt.Errorf("expecting integer imageid, not %T", v) } return uint32(imgidf), nil } @@ -264,10 +264,10 @@ func parseImageID(v interface{}) (uint32, error) { func parseUint64(v interface{}) (uint64, error) { f, ok := v.(float64) if !ok { - return 0, fmt.Errorf("Expecting numeric, not %T", v) + return 0, fmt.Errorf("expecting numeric, not %T", v) } if f > maxJSONFloat { - return 0, fmt.Errorf("Out of range integer for numeric %f", f) + return 0, fmt.Errorf("out of range integer for numeric %f", f) } return uint64(f), nil } @@ -279,7 +279,7 @@ func (h *proxyHandler) parseImageFromID(v interface{}) (*openImage, error) { } imgref, ok := h.images[imgid] if !ok { - return nil, fmt.Errorf("No image %v", imgid) + return nil, fmt.Errorf("no image %v", imgid) } return imgref, nil } @@ -368,7 +368,7 @@ func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { var ret replyBuf if h.sysctx == nil { - return ret, fmt.Errorf("Must invoke Initialize") + return ret, fmt.Errorf("client error: must invoke Initialize") } if len(args) != 1 { return ret, fmt.Errorf("invalid request, expecting one argument") @@ -397,9 +397,9 @@ func (h *proxyHandler) GetManifest(args []interface{}) (replyBuf, error) { break // Explicitly reject e.g. docker schema 1 type with a "legacy" note case manifest.DockerV2Schema1MediaType, manifest.DockerV2Schema1SignedMediaType: - return ret, fmt.Errorf("Unsupported legacy manifest MIME type: %s", manifestType) + return ret, fmt.Errorf("unsupported legacy manifest MIME type: %s", manifestType) default: - return ret, fmt.Errorf("Unsupported manifest MIME type: %s", manifestType) + return ret, fmt.Errorf("unsupported manifest MIME type: %s", manifestType) } // We always return the original digest, as that's what clients need to do pull-by-digest @@ -438,7 +438,7 @@ func (h *proxyHandler) GetConfig(args []interface{}) (replyBuf, error) { var ret replyBuf if h.sysctx == nil { - return ret, fmt.Errorf("Must invoke Initialize") + return ret, fmt.Errorf("client error: must invoke Initialize") } if len(args) != 1 { return ret, fmt.Errorf("invalid request, expecting: [imgid]") @@ -474,7 +474,7 @@ func (h *proxyHandler) GetBlob(args []interface{}) (replyBuf, error) { var ret replyBuf if h.sysctx == nil { - return ret, fmt.Errorf("Must invoke Initialize") + return ret, fmt.Errorf("client error: must invoke Initialize") } if len(args) != 3 { return ret, fmt.Errorf("found %d args, expecting (imgid, digest, size)", len(args)) @@ -517,7 +517,7 @@ func (h *proxyHandler) GetBlob(args []interface{}) (replyBuf, error) { return } if n != int64(size) { - f.err = fmt.Errorf("Expected %d bytes in blob, got %d", size, n) + f.err = fmt.Errorf("expected %d bytes in blob, got %d", size, n) } if !verifier.Verified() { f.err = fmt.Errorf("corrupted blob, expecting %s", d.String())