remind101 / empire

A PaaS built on top of Amazon EC2 Container Service (ECS)
BSD 2-Clause "Simplified" License
2.69k stars 158 forks source link

Image references should be parsed The Docker Way™ #857

Open Flygsand opened 8 years ago

Flygsand commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

So, turns out it's a legitimate bug in distribution/reference: https://github.com/docker/distribution/pull/963/files/b07d759241defb2f345e95ed04bfdeb8ac010ab2#r66186285