New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image references should be parsed The Docker Way™ #857

Open
vrcsix opened this Issue May 27, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@vrcsix
Contributor

vrcsix commented May 27, 2016

Empire's logic for parsing image references is a bit on the simple side, leading to situations where emp deploy doesn't behave like docker pull would (#856). IMO it would be a good idea to use/rip the canonical implementation.

@ejholmes

This comment has been minimized.

Contributor

ejholmes commented Jun 8, 2016

Thanks for pointing out distribution/reference @protomouse. When we made image.Image type, Docker's parsing was all over the place. This looks much nicer!

@ejholmes

This comment has been minimized.

Contributor

ejholmes commented Jun 8, 2016

I have a branch that adds this, but ran into weird issues using reference.SplitHostname.

Given this test program:

package main

import (
    "fmt"

    "github.com/docker/distribution/reference"
)

func main() {
    n1, _ := reference.ParseNamed("quay.io/remind101/acme-inc")
    n2, _ := reference.ParseNamed("awsaccountid.dkr.ecr.us-west-2.amazonaws.com/myimage:tag")
    n3, _ := reference.ParseNamed("remind101/acme-inc")

    for i, n := range []reference.Named{n1, n2, n3} {
        fmt.Printf("n%d\n", i)
        fmt.Println(n.Name())
        fmt.Println(reference.SplitHostname(n))
        fmt.Println()
    }
}

When I run it, I get the following output:

n0
quay.io/remind101/acme-inc
quay.io remind101/acme-inc

n1
awsaccountid.dkr.ecr.us-west-2.amazonaws.com/myimage
awsaccountid.dkr.ecr.us-west-2.amazonaws.com myimage

n2
remind101/acme-inc
remind101 acme-inc

For some reason, it thinks the hostname of n3 is remind101.

@ejholmes

This comment has been minimized.

Contributor

ejholmes commented Jun 8, 2016

Looking at the test cases for reference.SplitHostname, it seems like it's missing some obvious cases.

For example, this passes:

{
        input:    "test_com/foo",
        hostname: "",
        name:     "test_com/foo",
},

However, this fails:

{
        input:    "remind101/foo",
        hostname: "",
        name:     "remind101/foo",
},

wat...

Maybe that's just not a stable function. Apparently, it's not used anywhere in docker/distribution:

$ git grep SplitHostname
reference/reference.go:130:// SplitHostname splits a named reference into a
reference/reference.go:134:func SplitHostname(named Named) (string, string) {
reference/reference_test.go:180:                        hostname, _ := SplitHostname(named)
reference/reference_test.go:218:// should succeed are covered by TestSplitHostname, below.
reference/reference_test.go:262:func TestSplitHostname(t *testing.T) {
reference/reference_test.go:314:                hostname, name := SplitHostname(named)

And not used in Docker itself.

@ejholmes

This comment has been minimized.

Contributor

ejholmes commented Jun 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment