From d742d1caa48830e32367791e5196ce50539a2bc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 19 Nov 2016 03:03:40 +0100 Subject: [PATCH] Finish daemonReference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow either a !NameOnly named reference, or a sha256:hex digest. Both forms can be used for an ImageSource; ImageDestination accepts only a name:tag value. Because the sha256:hex reference values make it impossible to create a reasonable policy hierarchy, only support a trivial namespace with a single per-transport policy. Signed-off-by: Miloslav Trmač --- docker/daemon/daemon_dest.go | 11 +- docker/daemon/daemon_src.go | 4 +- docker/daemon/daemon_transport.go | 87 +++++++-- docker/daemon/daemon_transport_test.go | 236 +++++++++++++++++++++++++ transports/transports_test.go | 3 +- 5 files changed, 326 insertions(+), 15 deletions(-) create mode 100644 docker/daemon/daemon_transport_test.go diff --git a/docker/daemon/daemon_dest.go b/docker/daemon/daemon_dest.go index a5b62781..ee0ffda9 100644 --- a/docker/daemon/daemon_dest.go +++ b/docker/daemon/daemon_dest.go @@ -14,6 +14,7 @@ import ( "time" "github.com/Sirupsen/logrus" + "github.com/containers/image/docker/reference" "github.com/containers/image/manifest" "github.com/containers/image/types" "github.com/docker/engine-api/client" @@ -33,7 +34,13 @@ type daemonImageDestination struct { // newImageDestination returns a types.ImageDestination for the specified image reference. func newImageDestination(systemCtx *types.SystemContext, ref daemonReference) (types.ImageDestination, error) { - // FIXME: Do something with ref + if ref.ref == nil { + return nil, fmt.Errorf("Invalid destination docker-daemon:%s: a destination must be a name:tag", ref.StringWithinTransport()) + } + if _, ok := ref.ref.(reference.NamedTagged); !ok { + return nil, fmt.Errorf("Invalid destination docker-daemon:%s: a destination must be a name:tag", ref.StringWithinTransport()) + } + c, err := client.NewClient(client.DefaultDockerHost, "1.22", nil, nil) // FIXME: overridable host if err != nil { return nil, fmt.Errorf("Error initializing docker engine client: %v", err) @@ -176,7 +183,7 @@ func (d *daemonImageDestination) PutManifest(m []byte) error { } items := []manifestItem{{ Config: man.Config.Digest, - RepoTags: []string{string(d.ref)}, // FIXME: Only if ref is a NamedTagged + RepoTags: []string{d.ref.ref.String()}, // newImageDestination ensures that d.ref.ref is a reference.NamedTagged Layers: layerPaths, Parent: "", LayerSources: nil, diff --git a/docker/daemon/daemon_src.go b/docker/daemon/daemon_src.go index bb234dd3..25a50967 100644 --- a/docker/daemon/daemon_src.go +++ b/docker/daemon/daemon_src.go @@ -52,7 +52,9 @@ func newImageSource(ctx *types.SystemContext, ref daemonReference) (types.ImageS if err != nil { return nil, fmt.Errorf("Error initializing docker engine client: %v", err) } - inputStream, err := c.ImageSave(context.TODO(), []string{string(ref)}) // FIXME: ref should be per docker/reference.ParseIDOrReference, and we don't want NameOnly + // Per NewReference(), ref.StringWithinTransport() is either an image ID (config digest), or a !reference.NameOnly() reference. + // Either way ImageSave should create a tarball with exactly one image. + inputStream, err := c.ImageSave(context.TODO(), []string{ref.StringWithinTransport()}) if err != nil { return nil, fmt.Errorf("Error loading image from docker engine: %v", err) } diff --git a/docker/daemon/daemon_transport.go b/docker/daemon/daemon_transport.go index 06430580..1492e2fe 100644 --- a/docker/daemon/daemon_transport.go +++ b/docker/daemon/daemon_transport.go @@ -1,11 +1,13 @@ package daemon import ( + "errors" "fmt" "github.com/containers/image/docker/reference" "github.com/containers/image/image" "github.com/containers/image/types" + "github.com/docker/distribution/digest" ) // Transport is an ImageTransport for images managed by a local Docker daemon. @@ -28,19 +30,69 @@ func (t daemonTransport) ParseReference(reference string) (types.ImageReference, // It is acceptable to allow an invalid value which will never be matched, it can "only" cause user confusion. // scope passed to this function will not be "", that value is always allowed. func (t daemonTransport) ValidatePolicyConfigurationScope(scope string) error { - // FIXME FIXME - return nil + // See the explanation in daemonReference.PolicyConfigurationIdentity. + return errors.New(`docker-daemon: does not support any scopes except the default "" one`) } -// daemonReference is an ImageReference for images managed by a local Docker daemon. -type daemonReference string // FIXME FIXME +// daemonReference is an ImageReference for images managed by a local Docker daemon +// Exactly one of id and ref can be set. +// For daemonImageSource, both id and ref are acceptable, ref must not be a NameOnly (interpreted as all tags in that repository by the daemon) +// For daemonImageDestination, it must be a ref, which is NamedTagged. +// (We could, in principle, also allow storing images without tagging them, and the user would have to refer to them using the docker image ID = config digest. +// Using the config digest requires the caller to parse the manifest themselves, which is very cumbersome; so, for now, we don’t bother.) +type daemonReference struct { + id digest.Digest + ref reference.Named // !reference.IsNameOnly +} // ParseReference converts a string, which should not start with the ImageTransport.Name prefix, into an ImageReference. -func ParseReference(reference string) (types.ImageReference, error) { - return daemonReference(reference), nil // FIXME FIXME +func ParseReference(refString string) (types.ImageReference, error) { + // This is intended to be compatible with reference.ParseIDOrReference, but more strict about refusing some of the ambiguous cases. + // In particular, this rejects unprefixed digest values (64 hex chars), and sha256 digest prefixes (sha256:fewer-than-64-hex-chars). + + // digest:hexstring is structurally the same as a reponame:tag (meaning docker.io/library/reponame:tag). + // reference.ParseIDOrReference interprets such strings as digests. + if dgst, err := digest.ParseDigest(refString); err == nil { + // The daemon explicitly refuses to tag images with a reponame equal to digest.Canonical - but _only_ this digest name. + // Other digest references are ambiguous, so refuse them. + if dgst.Algorithm() != digest.Canonical { + return nil, fmt.Errorf("Invalid docker-daemon: reference %s: only digest algorithm %s accepted", refString, digest.Canonical) + } + return NewReference(dgst, nil) + } + + ref, err := reference.ParseNamed(refString) // This also rejects unprefixed digest values + if err != nil { + return nil, err + } + if ref.Name() == digest.Canonical.String() { + return nil, fmt.Errorf("Invalid docker-daemon: reference %s: The %s repository name is reserved for (non-shortened) digest references", refString, digest.Canonical) + } + return NewReference("", ref) } -// FIXME FIXME: NewReference? +// NewReference returns a docker-daemon reference for either the supplied image ID (config digest) or the supplied reference (which must satisfy !reference.IsNameOnly) +func NewReference(id digest.Digest, ref reference.Named) (types.ImageReference, error) { + if id != "" && ref != nil { + return nil, errors.New("docker-daemon: reference must not have an image ID and a reference string specified at the same time") + } + if ref != nil { + if reference.IsNameOnly(ref) { + return nil, fmt.Errorf("docker-daemon: reference %s has neither a tag nor a digest", ref.String()) + } + // A github.com/distribution/reference value can have a tag and a digest at the same time! + // docker/reference does not handle that, so fail. + _, isTagged := ref.(reference.NamedTagged) + _, isDigested := ref.(reference.Canonical) + if isTagged && isDigested { + return nil, fmt.Errorf("docker-daemon: references with both a tag and digest are currently not supported") + } + } + return daemonReference{ + id: id, + ref: ref, + }, nil +} func (ref daemonReference) Transport() types.ImageTransport { return Transport @@ -53,14 +105,21 @@ func (ref daemonReference) Transport() types.ImageTransport { // WARNING: Do not use the return value in the UI to describe an image, it does not contain the Transport().Name() prefix; // instead, see transports.ImageName(). func (ref daemonReference) StringWithinTransport() string { - return string(ref) // FIXME FIXME + switch { + case ref.id != "": + return ref.id.String() + case ref.ref != nil: + return ref.ref.String() + default: // Coverage: Should never happen, NewReference above should refuse such values. + panic("Internal inconsistency: daemonReference has empty id and nil ref") + } } // DockerReference returns a Docker reference associated with this reference // (fully explicit, i.e. !reference.IsNameOnly, but reflecting user intent, // not e.g. after redirect or alias processing), or nil if unknown/not applicable. func (ref daemonReference) DockerReference() reference.Named { - return nil // FIXME FIXME + return ref.ref // May be nil } // PolicyConfigurationIdentity returns a string representation of the reference, suitable for policy lookup. @@ -71,7 +130,10 @@ func (ref daemonReference) DockerReference() reference.Named { // not required/guaranteed that it will be a valid input to Transport().ParseReference(). // Returns "" if configuration identities for these references are not supported. func (ref daemonReference) PolicyConfigurationIdentity() string { - return string(ref) // FIXME FIXME + // We must allow referring to images in the daemon by image ID, otherwise untagged images would not be accessible. + // But the existence of image IDs means that we can’t truly well namespace the input; the untagged images would have to fall into the default policy, + // which can be unexpected. So, punt. + return "" // This still allows using the default "" scope to define a policy for this transport. } // PolicyConfigurationNamespaces returns a list of other policy configuration namespaces to search @@ -110,5 +172,8 @@ func (ref daemonReference) NewImageDestination(ctx *types.SystemContext) (types. // DeleteImage deletes the named image from the registry, if supported. func (ref daemonReference) DeleteImage(ctx *types.SystemContext) error { - return fmt.Errorf("Deleting images not implemented for docker-daemon: images") // FIXME FIXME? + // Should this just untag the image? Should this stop running containers? + // The semantics is not quite as clear as for remote repositories. + // The user can run (docker rmi) directly anyway, so, for now(?), punt instead of trying to guess what the user meant. + return fmt.Errorf("Deleting images not implemented for docker-daemon: images") } diff --git a/docker/daemon/daemon_transport_test.go b/docker/daemon/daemon_transport_test.go new file mode 100644 index 00000000..d2c874a5 --- /dev/null +++ b/docker/daemon/daemon_transport_test.go @@ -0,0 +1,236 @@ +package daemon + +import ( + "testing" + + "github.com/containers/image/docker/reference" + "github.com/containers/image/types" + "github.com/docker/distribution/digest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + sha256digestHex = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + sha256digest = "sha256:" + sha256digestHex +) + +func TestTransportName(t *testing.T) { + assert.Equal(t, "docker-daemon", Transport.Name()) +} + +func TestTransportParseReference(t *testing.T) { + testParseReference(t, Transport.ParseReference) +} + +func TestTransportValidatePolicyConfigurationScope(t *testing.T) { + for _, scope := range []string{ // A semi-representative assortment of values; everything is rejected. + sha256digestHex, + sha256digest, + "docker.io/library/busybox:latest", + "docker.io", + "", + } { + err := Transport.ValidatePolicyConfigurationScope(scope) + assert.Error(t, err, scope) + } +} + +func TestParseReference(t *testing.T) { + testParseReference(t, ParseReference) +} + +// testParseReference is a test shared for Transport.ParseReference and ParseReference. +func testParseReference(t *testing.T, fn func(string) (types.ImageReference, error)) { + for _, c := range []struct{ input, expectedID, expectedRef string }{ + {sha256digest, sha256digest, ""}, // Valid digest format + {"sha512:" + sha256digestHex + sha256digestHex, "", ""}, // Non-digest.Canonical digest + {"sha256:ab", "", ""}, // Invalid digest value (too short) + {sha256digest + "ab", "", ""}, // Invalid digest value (too long) + {"sha256:XX23456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", "", ""}, // Invalid digest value + {"UPPERCASEISINVALID", "", ""}, // Invalid reference input + {"busybox", "", ""}, // Missing tag or digest + {"busybox:latest", "", "busybox:latest"}, // Explicit tag + {"busybox@" + sha256digest, "", "busybox@" + sha256digest}, // Explicit digest + // A github.com/distribution/reference value can have a tag and a digest at the same time! + // github.com/docker/reference handles that by dropping the tag. That is not obviously the + // right thing to do, but it is at least reasonable, so test that we keep behaving reasonably. + // This test case should not be construed to make this an API promise. + // FIXME? Instead work extra hard to reject such input? + {"busybox:latest@" + sha256digest, "", "busybox@" + sha256digest}, // Both tag and digest + {"docker.io/library/busybox:latest", "", "busybox:latest"}, // All implied values explicitly specified + } { + ref, err := fn(c.input) + if c.expectedID == "" && c.expectedRef == "" { + assert.Error(t, err, c.input) + } else { + require.NoError(t, err, c.input) + daemonRef, ok := ref.(daemonReference) + require.True(t, ok, c.input) + // If we don't reject the input, the interpretation must be consistent for reference.ParseIDOrReference + dockerID, dockerRef, err := reference.ParseIDOrReference(c.input) + require.NoError(t, err, c.input) + + if c.expectedRef == "" { + assert.Equal(t, c.expectedID, daemonRef.id.String(), c.input) + assert.Nil(t, daemonRef.ref, c.input) + + assert.Equal(t, c.expectedID, dockerID.String(), c.input) + assert.Nil(t, dockerRef, c.input) + } else { + assert.Equal(t, "", daemonRef.id.String(), c.input) + require.NotNil(t, daemonRef.ref, c.input) + assert.Equal(t, c.expectedRef, daemonRef.ref.String(), c.input) + + assert.Equal(t, "", dockerID.String(), c.input) + require.NotNil(t, dockerRef, c.input) + assert.Equal(t, c.expectedRef, dockerRef.String(), c.input) + } + } + } +} + +// refWithTagAndDigest is a reference.NamedTagged and reference.Canonical at the same time. +type refWithTagAndDigest struct{ reference.Canonical } + +func (ref refWithTagAndDigest) Tag() string { + return "notLatest" +} + +// A common list of reference formats to test for the various ImageReference methods. +// (For IDs it is much simpler, we simply use them unmodified) +var validNamedReferenceTestCases = []struct{ input, dockerRef, stringWithinTransport string }{ + {"busybox:notlatest", "busybox:notlatest", "busybox:notlatest"}, // Explicit tag + {"busybox" + sha256digest, "busybox" + sha256digest, "busybox" + sha256digest}, // Explicit digest + {"docker.io/library/busybox:latest", "busybox:latest", "busybox:latest"}, // All implied values explicitly specified + {"example.com/ns/foo:bar", "example.com/ns/foo:bar", "example.com/ns/foo:bar"}, // All values explicitly specified +} + +func TestNewReference(t *testing.T) { + // An ID reference. + id, err := digest.ParseDigest(sha256digest) + require.NoError(t, err) + ref, err := NewReference(id, nil) + require.NoError(t, err) + daemonRef, ok := ref.(daemonReference) + require.True(t, ok) + assert.Equal(t, id, daemonRef.id) + assert.Nil(t, daemonRef.ref) + + // Named references + for _, c := range validNamedReferenceTestCases { + parsed, err := reference.ParseNamed(c.input) + require.NoError(t, err) + ref, err := NewReference("", parsed) + require.NoError(t, err, c.input) + daemonRef, ok := ref.(daemonReference) + require.True(t, ok, c.input) + assert.Equal(t, "", daemonRef.id.String()) + require.NotNil(t, daemonRef.ref) + assert.Equal(t, c.dockerRef, daemonRef.ref.String(), c.input) + } + + // Both an ID and a named reference provided + parsed, err := reference.ParseNamed("busybox:latest") + require.NoError(t, err) + _, err = NewReference(id, parsed) + assert.Error(t, err) + + // A reference with neither a tag nor digest + parsed, err = reference.ParseNamed("busybox") + require.NoError(t, err) + _, err = NewReference("", parsed) + assert.Error(t, err) + + // A github.com/distribution/reference value can have a tag and a digest at the same time! + parsed, err = reference.ParseNamed("busybox@" + sha256digest) + require.NoError(t, err) + refDigested, ok := parsed.(reference.Canonical) + require.True(t, ok) + tagDigestRef := refWithTagAndDigest{refDigested} + _, err = NewReference("", tagDigestRef) + assert.Error(t, err) +} + +func TestReferenceTransport(t *testing.T) { + ref, err := ParseReference(sha256digest) + require.NoError(t, err) + assert.Equal(t, Transport, ref.Transport()) + + ref, err = ParseReference("busybox:latest") + require.NoError(t, err) + assert.Equal(t, Transport, ref.Transport()) +} + +func TestReferenceStringWithinTransport(t *testing.T) { + ref, err := ParseReference(sha256digest) + require.NoError(t, err) + assert.Equal(t, sha256digest, ref.StringWithinTransport()) + + for _, c := range validNamedReferenceTestCases { + ref, err := ParseReference(c.input) + require.NoError(t, err, c.input) + stringRef := ref.StringWithinTransport() + assert.Equal(t, c.stringWithinTransport, stringRef, c.input) + // Do one more round to verify that the output can be parsed, to an equal value. + ref2, err := Transport.ParseReference(stringRef) + require.NoError(t, err, c.input) + stringRef2 := ref2.StringWithinTransport() + assert.Equal(t, stringRef, stringRef2, c.input) + } +} + +func TestReferenceDockerReference(t *testing.T) { + ref, err := ParseReference(sha256digest) + require.NoError(t, err) + assert.Nil(t, ref.DockerReference()) + + for _, c := range validNamedReferenceTestCases { + ref, err := ParseReference(c.input) + require.NoError(t, err, c.input) + dockerRef := ref.DockerReference() + require.NotNil(t, dockerRef, c.input) + assert.Equal(t, c.dockerRef, dockerRef.String(), c.input) + } +} + +func TestReferencePolicyConfigurationIdentity(t *testing.T) { + ref, err := ParseReference(sha256digest) + require.NoError(t, err) + assert.Equal(t, "", ref.PolicyConfigurationIdentity()) + + for _, c := range validNamedReferenceTestCases { + ref, err := ParseReference(c.input) + require.NoError(t, err, c.input) + assert.Equal(t, "", ref.PolicyConfigurationIdentity(), c.input) + } +} + +func TestReferencePolicyConfigurationNamespaces(t *testing.T) { + ref, err := ParseReference(sha256digest) + require.NoError(t, err) + assert.Empty(t, ref.PolicyConfigurationNamespaces()) + + for _, c := range validNamedReferenceTestCases { + ref, err := ParseReference(c.input) + require.NoError(t, err, c.input) + assert.Empty(t, ref.PolicyConfigurationNamespaces(), c.input) + } +} + +// daemonReference.NewImage, daemonReference.NewImageSource, openshiftReference.NewImageDestination +// untested because just creating the objects immediately connects to the daemon. + +func TestReferenceDeleteImage(t *testing.T) { + ref, err := ParseReference(sha256digest) + require.NoError(t, err) + err = ref.DeleteImage(nil) + assert.Error(t, err) + + for _, c := range validNamedReferenceTestCases { + ref, err := ParseReference(c.input) + require.NoError(t, err, c.input) + err = ref.DeleteImage(nil) + assert.Error(t, err, c.input) + } +} diff --git a/transports/transports_test.go b/transports/transports_test.go index 60e694bd..2daec0dd 100644 --- a/transports/transports_test.go +++ b/transports/transports_test.go @@ -32,7 +32,8 @@ func TestImageNameHandling(t *testing.T) { {"dir", "/etc", "/etc"}, {"docker", "//busybox", "//busybox:latest"}, {"docker", "//busybox:notlatest", "//busybox:notlatest"}, // This also tests handling of multiple ":" characters - {"docker-daemon", "FIXME FIXME", "FIXME FIXME"}, + {"docker-daemon", "sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", "sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}, + {"docker-daemon", "busybox:latest", "busybox:latest"}, {"oci", "/etc:sometag", "/etc:sometag"}, // "atomic" not tested here because it depends on per-user configuration for the default cluster. } {