From 4eb8db31f00f5bcb25fe55341731df33f3067768 Mon Sep 17 00:00:00 2001 From: umohnani8 Date: Mon, 31 Jul 2017 14:46:07 -0400 Subject: [PATCH] Fix oci to save the full image name Fix oci to save the full image name "image:tag" instead of just the tag in index.json. Add function to retrieve the image name from index.json when loading or pulling image from oci directory. Signed-off-by: umohnani8 --- oci/layout/oci_dest.go | 9 +- oci/layout/oci_transport.go | 113 +++++++++++------- oci/layout/oci_transport_test.go | 67 ++++++----- .../alltransports/alltransports_test.go | 2 +- 4 files changed, 114 insertions(+), 77 deletions(-) diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index 0536730985..2b789fc88f 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -23,13 +23,16 @@ type ociImageDestination struct { } // newImageDestination returns an ImageDestination for writing to an existing directory. -func newImageDestination(ref ociReference) types.ImageDestination { +func newImageDestination(ref ociReference) (types.ImageDestination, error) { + if ref.image == "" { + return nil, errors.Errorf("cannot save image with empty image.ref.name") + } index := imgspecv1.Index{ Versioned: imgspec.Versioned{ SchemaVersion: 2, }, } - return &ociImageDestination{ref: ref, index: index} + return &ociImageDestination{ref: ref, index: index}, nil } // Reference returns the reference used to set up this destination. Note that this should directly correspond to user's intent, @@ -178,7 +181,7 @@ func (d *ociImageDestination) PutManifest(m []byte) error { } annotations := make(map[string]string) - annotations["org.opencontainers.image.ref.name"] = d.ref.tag + annotations["org.opencontainers.image.ref.name"] = d.ref.image desc.Annotations = annotations desc.Platform = &imgspecv1.Platform{ Architecture: runtime.GOARCH, diff --git a/oci/layout/oci_transport.go b/oci/layout/oci_transport.go index f1fc107139..1406dd18ca 100644 --- a/oci/layout/oci_transport.go +++ b/oci/layout/oci_transport.go @@ -36,7 +36,14 @@ func (t ociTransport) ParseReference(reference string) (types.ImageReference, er return ParseReference(reference) } -var refRegexp = regexp.MustCompile(`^([A-Za-z0-9._-]+)+$`) +// annotation spex from https://github.com/opencontainers/image-spec/blob/master/annotations.md#pre-defined-annotation-keys +const ( + separator = `(?:[-._:@+]|--)` + alphanum = `(?:[A-Za-z0-9]+)` + component = `(?:` + alphanum + `(?:` + separator + alphanum + `)*)` +) + +var refRegexp = regexp.MustCompile(`^` + component + `(?:/` + component + `)*$`) // ValidatePolicyConfigurationScope checks that scope is a valid name for a signature.PolicyTransportScopes keys // (i.e. a valid PolicyConfigurationIdentity() or PolicyConfigurationNamespaces() return value). @@ -44,19 +51,14 @@ var refRegexp = regexp.MustCompile(`^([A-Za-z0-9._-]+)+$`) // scope passed to this function will not be "", that value is always allowed. func (t ociTransport) ValidatePolicyConfigurationScope(scope string) error { var dir string - sep := strings.LastIndex(scope, ":") - if sep == -1 { - dir = scope - } else { - dir = scope[:sep] - tag := scope[sep+1:] - if !refRegexp.MatchString(tag) { - return errors.Errorf("Invalid tag %s", tag) - } - } + sep := strings.SplitN(scope, ":", 2) + dir = sep[0] - if strings.Contains(dir, ":") { - return errors.Errorf("Invalid OCI reference %s: path contains a colon", scope) + if len(sep) == 2 { + image := sep[1] + if !refRegexp.MatchString(image) { + return errors.Errorf("Invalid image %s", image) + } } if !strings.HasPrefix(dir, "/") { @@ -64,7 +66,7 @@ func (t ociTransport) ValidatePolicyConfigurationScope(scope string) error { } // Refuse also "/", otherwise "/" and "" would have the same semantics, // and "" could be unexpectedly shadowed by the "/" entry. - // (Note: we do allow "/:sometag", a bit ridiculous but why refuse it?) + // (Note: we do allow "/:someimage", a bit ridiculous but why refuse it?) if scope == "/" { return errors.New(`Invalid scope "/": Use the generic default scope ""`) } @@ -85,28 +87,26 @@ type ociReference struct { // (But in general, we make no attempt to be completely safe against concurrent hostile filesystem modifications.) dir string // As specified by the user. May be relative, contain symlinks, etc. resolvedDir string // Absolute path with no symlinks, at least at the time of its creation. Primarily used for policy namespaces. - tag string + image string // If image=="", it means the only image in the index.json is used } // ParseReference converts a string, which should not start with the ImageTransport.Name prefix, into an OCI ImageReference. func ParseReference(reference string) (types.ImageReference, error) { - var dir, tag string - sep := strings.LastIndex(reference, ":") - if sep == -1 { - dir = reference - tag = "latest" - } else { - dir = reference[:sep] - tag = reference[sep+1:] + var dir, image string + sep := strings.SplitN(reference, ":", 2) + dir = sep[0] + + if len(sep) == 2 { + image = sep[1] } - return NewReference(dir, tag) + return NewReference(dir, image) } -// NewReference returns an OCI reference for a directory and a tag. +// NewReference returns an OCI reference for a directory and a image. // // We do not expose an API supplying the resolvedDir; we could, but recomputing it // is generally cheap enough that we prefer being confident about the properties of resolvedDir. -func NewReference(dir, tag string) (types.ImageReference, error) { +func NewReference(dir, image string) (types.ImageReference, error) { resolved, err := explicitfilepath.ResolvePathToFullyExplicit(dir) if err != nil { return nil, err @@ -114,12 +114,12 @@ func NewReference(dir, tag string) (types.ImageReference, error) { // This is necessary to prevent directory paths returned by PolicyConfigurationNamespaces // from being ambiguous with values of PolicyConfigurationIdentity. if strings.Contains(resolved, ":") { - return nil, errors.Errorf("Invalid OCI reference %s:%s: path %s contains a colon", dir, tag, resolved) + return nil, errors.Errorf("Invalid OCI reference %s:%s: path %s contains a colon", dir, image, resolved) } - if !refRegexp.MatchString(tag) { - return nil, errors.Errorf("Invalid tag %s", tag) + if len(image) > 0 && !refRegexp.MatchString(image) { + return nil, errors.Errorf("Invalid image %s", image) } - return ociReference{dir: dir, resolvedDir: resolved, tag: tag}, nil + return ociReference{dir: dir, resolvedDir: resolved, image: image}, nil } func (ref ociReference) Transport() types.ImageTransport { @@ -132,7 +132,7 @@ func (ref ociReference) Transport() types.ImageTransport { // e.g. default attribute values omitted by the user may be filled in in the return value, or vice versa. // WARNING: Do not use the return value in the UI to describe an image, it does not contain the Transport().Name() prefix. func (ref ociReference) StringWithinTransport() string { - return fmt.Sprintf("%s:%s", ref.dir, ref.tag) + return fmt.Sprintf("%s:%s", ref.dir, ref.image) } // DockerReference returns a Docker reference associated with this reference @@ -150,7 +150,10 @@ func (ref ociReference) 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 ociReference) PolicyConfigurationIdentity() string { - return fmt.Sprintf("%s:%s", ref.resolvedDir, ref.tag) + // NOTE: ref.image is not a part of the image identity, because "$dir:$someimage" and "$dir:" may mean the + // same image and the two can’t be statically disambiguated. Using at least the repository directory is + // less granular but hopefully still useful. + return fmt.Sprintf("%s", ref.resolvedDir) } // PolicyConfigurationNamespaces returns a list of other policy configuration namespaces to search @@ -196,26 +199,48 @@ func (ref ociReference) getManifestDescriptor() (imgspecv1.Descriptor, error) { if err := json.NewDecoder(indexJSON).Decode(&index); err != nil { return imgspecv1.Descriptor{}, err } + var d *imgspecv1.Descriptor - for _, md := range index.Manifests { - if md.MediaType != imgspecv1.MediaTypeImageManifest { - continue - } - refName, ok := md.Annotations["org.opencontainers.image.ref.name"] - if !ok { - continue + if ref.image == "" { + // return manifest if only one image is in the oci directory + if len(index.Manifests) == 1 { + d = &index.Manifests[0] + } else { + // ask user to choose image when more than one image in the oci directory + return imgspecv1.Descriptor{}, errors.Wrapf(err, "more than one image in oci, choose an image") } - if refName == ref.tag { - d = &md - break + } else { + // if image specified, look through all manifests for a match + for _, md := range index.Manifests { + if md.MediaType != imgspecv1.MediaTypeImageManifest { + continue + } + refName, ok := md.Annotations["org.opencontainers.image.ref.name"] + if !ok { + continue + } + if refName == ref.image { + d = &md + break + } } } if d == nil { - return imgspecv1.Descriptor{}, fmt.Errorf("no descriptor found for reference %q", ref.tag) + return imgspecv1.Descriptor{}, fmt.Errorf("no descriptor found for reference %q", ref.image) } return *d, nil } +// LoadManifestDescriptor loads the manifest descriptor to be used to retrieve the image name +// when pulling an image +func LoadManifestDescriptor(imgRef types.ImageReference) (imgspecv1.Descriptor, error) { + ociRef, ok := imgRef.(ociReference) + if !ok { + return imgspecv1.Descriptor{}, errors.Errorf("error typecasting, need type ociRef") + } + return ociRef.getManifestDescriptor() +} + // NewImageSource returns a types.ImageSource for this reference, // asking the backend to use a manifest from requestedManifestMIMETypes if possible. // nil requestedManifestMIMETypes means manifest.DefaultRequestedManifestMIMETypes. @@ -227,7 +252,7 @@ func (ref ociReference) NewImageSource(ctx *types.SystemContext, requestedManife // NewImageDestination returns a types.ImageDestination for this reference. // The caller must call .Close() on the returned ImageDestination. func (ref ociReference) NewImageDestination(ctx *types.SystemContext) (types.ImageDestination, error) { - return newImageDestination(ref), nil + return newImageDestination(ref) } // DeleteImage deletes the named image from the registry, if supported. diff --git a/oci/layout/oci_transport_test.go b/oci/layout/oci_transport_test.go index 9c63addd5c..10b091086d 100644 --- a/oci/layout/oci_transport_test.go +++ b/oci/layout/oci_transport_test.go @@ -38,9 +38,9 @@ func TestTransportValidatePolicyConfigurationScope(t *testing.T) { "/has/./dot", "/has/dot/../dot", "/trailing/slash/", - "/etc:invalid'tag!value@", - "/path:with/colons", - "/path:with/colons/and:tag", + "/etc:invalid'image!value@", + "/etc_no_image:", + "/etc:multiple_:separators", } { err := Transport.ValidatePolicyConfigurationScope(scope) assert.Error(t, err, scope) @@ -64,48 +64,57 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err "relativepath", tmpDir + "/thisdoesnotexist", } { - for _, tag := range []struct{ suffix, tag string }{ - {":notlatest", "notlatest"}, - {"", "latest"}, + for _, image := range []struct{ suffix, image string }{ + {":notlatest:image", "notlatest:image"}, + {":latestimage", "latestimage"}, + {":", ""}, + {"", ""}, } { - input := path + tag.suffix + input := path + image.suffix ref, err := fn(input) require.NoError(t, err, input) ociRef, ok := ref.(ociReference) require.True(t, ok) assert.Equal(t, path, ociRef.dir, input) - assert.Equal(t, tag.tag, ociRef.tag, input) + assert.Equal(t, image.image, ociRef.image, input) } } - _, err = fn(tmpDir + "/with:multiple:colons:and:tag") - assert.Error(t, err) - - _, err = fn(tmpDir + ":invalid'tag!value@") + _, err = fn(tmpDir + ":invalid'image!value@") assert.Error(t, err) } func TestNewReference(t *testing.T) { - const tagValue = "tagValue" + const ( + imageValue = "imageValue" + noImageValue = "" + ) tmpDir, err := ioutil.TempDir("", "oci-transport-test") require.NoError(t, err) defer os.RemoveAll(tmpDir) - ref, err := NewReference(tmpDir, tagValue) + ref, err := NewReference(tmpDir, imageValue) require.NoError(t, err) ociRef, ok := ref.(ociReference) require.True(t, ok) assert.Equal(t, tmpDir, ociRef.dir) - assert.Equal(t, tagValue, ociRef.tag) + assert.Equal(t, imageValue, ociRef.image) + + ref, err = NewReference(tmpDir, noImageValue) + require.NoError(t, err) + ociRef, ok = ref.(ociReference) + require.True(t, ok) + assert.Equal(t, tmpDir, ociRef.dir) + assert.Equal(t, noImageValue, ociRef.image) - _, err = NewReference(tmpDir+"/thisparentdoesnotexist/something", tagValue) + _, err = NewReference(tmpDir+"/thisparentdoesnotexist/something", imageValue) assert.Error(t, err) - _, err = NewReference(tmpDir+"/has:colon", tagValue) + _, err = NewReference(tmpDir, "invalid'image!value@") assert.Error(t, err) - _, err = NewReference(tmpDir, "invalid'tag!value@") + _, err = NewReference(tmpDir+"/has:colon", imageValue) assert.Error(t, err) } @@ -127,14 +136,14 @@ func refToTempOCI(t *testing.T) (ref types.ImageReference, tmpDir string) { "os": "linux" }, "annotations": { - "org.opencontainers.image.ref.name": "tagValue" + "org.opencontainers.image.ref.name": "imageValue" } } ] } ` ioutil.WriteFile(filepath.Join(tmpDir, "index.json"), []byte(m), 0644) - ref, err = NewReference(tmpDir, "tagValue") + ref, err = NewReference(tmpDir, "imageValue") require.NoError(t, err) return ref, tmpDir } @@ -151,8 +160,8 @@ func TestReferenceStringWithinTransport(t *testing.T) { defer os.RemoveAll(tmpDir) for _, c := range []struct{ input, result string }{ - {"/dir1:notlatest", "/dir1:notlatest"}, // Explicit tag - {"/dir2", "/dir2:latest"}, // Default tag + {"/dir1:notlatest:notlatest", "/dir1:notlatest:notlatest"}, // Explicit image + {"/dir3:", "/dir3:"}, // No image } { ref, err := ParseReference(tmpDir + c.input) require.NoError(t, err, c.input) @@ -176,17 +185,17 @@ func TestReferencePolicyConfigurationIdentity(t *testing.T) { ref, tmpDir := refToTempOCI(t) defer os.RemoveAll(tmpDir) - assert.Equal(t, tmpDir+":tagValue", ref.PolicyConfigurationIdentity()) + assert.Equal(t, tmpDir, ref.PolicyConfigurationIdentity()) // A non-canonical path. Test just one, the various other cases are // tested in explicitfilepath.ResolvePathToFullyExplicit. - ref, err := NewReference(tmpDir+"/.", "tag2") + ref, err := NewReference(tmpDir+"/.", "image2") require.NoError(t, err) - assert.Equal(t, tmpDir+":tag2", ref.PolicyConfigurationIdentity()) + assert.Equal(t, tmpDir, ref.PolicyConfigurationIdentity()) // "/" as a corner case. - ref, err = NewReference("/", "tag3") + ref, err = NewReference("/", "image3") require.NoError(t, err) - assert.Equal(t, "/:tag3", ref.PolicyConfigurationIdentity()) + assert.Equal(t, "/", ref.PolicyConfigurationIdentity()) } func TestReferencePolicyConfigurationNamespaces(t *testing.T) { @@ -208,7 +217,7 @@ func TestReferencePolicyConfigurationNamespaces(t *testing.T) { for _, path := range []string{"/etc/skel", "/etc/skel/./."} { _, err := os.Lstat(path) require.NoError(t, err) - ref, err := NewReference(path, "sometag") + ref, err := NewReference(path, "someimage") require.NoError(t, err) ns := ref.PolicyConfigurationNamespaces() require.NotNil(t, ns) @@ -216,7 +225,7 @@ func TestReferencePolicyConfigurationNamespaces(t *testing.T) { } // "/" as a corner case. - ref, err := NewReference("/", "tag3") + ref, err := NewReference("/", "image3") require.NoError(t, err) assert.Equal(t, []string{}, ref.PolicyConfigurationNamespaces()) } diff --git a/transports/alltransports/alltransports_test.go b/transports/alltransports/alltransports_test.go index 2cff97852f..cfd56816f0 100644 --- a/transports/alltransports/alltransports_test.go +++ b/transports/alltransports/alltransports_test.go @@ -33,7 +33,7 @@ func TestImageNameHandling(t *testing.T) { {"docker-daemon", "busybox:latest", "busybox:latest"}, {"docker-archive", "/var/lib/oci/busybox.tar:busybox:latest", "/var/lib/oci/busybox.tar:docker.io/library/busybox:latest"}, {"docker-archive", "busybox.tar:busybox:latest", "busybox.tar:docker.io/library/busybox:latest"}, - {"oci", "/etc:sometag", "/etc:sometag"}, + {"oci", "/etc:someimage:latest", "/etc:someimage:latest"}, // "atomic" not tested here because it depends on per-user configuration for the default cluster. // "containers-storage" not tested here because it needs to initialize various directories on the fs. } {