Browse Source

Parse shorthand dockerhub images correctly

This fixes issue #804 by adding test cases that were missing, and
rewriting the parsing to pass.

In addition, I have changed the use of ImageID such that it
canonicalises elements (e.g., the domain) when asked, rather than when
parsing. This means that in general, images will appear as they are
supplied by users -- e.g., `alpine:3.5`.
Michael Bridgen 2 years ago
parent
commit
4639672fdf
9 changed files with 155 additions and 92 deletions
  1. 89
    47
      image.go
  2. 47
    19
      image_test.go
  3. 2
    2
      registry/cache/memcached.go
  4. 6
    6
      registry/client.go
  5. 2
    9
      registry/registry.go
  6. 3
    3
      registry/registry_test.go
  7. 4
    4
      registry/warming.go
  8. 1
    1
      update/filter.go
  9. 1
    1
      update/print.go

+ 89
- 47
image.go View File

@@ -3,6 +3,7 @@ package flux
3 3
 import (
4 4
 	"encoding/json"
5 5
 	"fmt"
6
+	"regexp"
6 7
 	"strings"
7 8
 	"time"
8 9
 
@@ -22,12 +23,17 @@ var (
22 23
 	ErrMalformedImageID = errors.Wrap(ErrInvalidImageID, `expected image name as either <image>:<tag> or just <image>`)
23 24
 )
24 25
 
25
-// ImageID is a fully qualified name that refers to a particular Image.
26
-// It is in the format: host[:port]/Image[:tag]
26
+// ImageID is a fully qualified name that refers to a particular
27
+// (tagged) image or image repository.  It is usually found
28
+// stringified in the format: `[host[:port]]/Image[:tag]`
27 29
 type ImageID struct {
28
-	Host, Image, Tag string
30
+	Domain, Image, Tag string
29 31
 }
30 32
 
33
+// ParseImageID parses a string representation of an image id into an
34
+// ImageID value. The grammar is shown here:
35
+// https://github.com/docker/distribution/blob/master/reference/reference.go
36
+// (but we do not care about all the productions.)
31 37
 func ParseImageID(s string) (ImageID, error) {
32 38
 	if s == "" {
33 39
 		return ImageID{}, ErrBlankImageID
@@ -35,51 +41,65 @@ func ParseImageID(s string) (ImageID, error) {
35 41
 	if strings.HasPrefix(s, "/") || strings.HasSuffix(s, "/") {
36 42
 		return ImageID{}, ErrMalformedImageID
37 43
 	}
38
-	var img ImageID
39
-	parts := strings.Split(s, ":")
40
-	switch len(parts) {
41
-	case 1:
42
-		img.Tag = "latest"
43
-	case 2:
44
-		img.Tag = parts[1]
45
-		s = parts[0]
46
-	case 3: // There might be three parts if there is a host with a custom port
47
-		img.Tag = parts[2]
48
-		s = s[:strings.LastIndex(s, ":")]
49
-	default:
50
-		return ImageID{}, ErrMalformedImageID
51
-	}
52
-	if s == "" {
53
-		return ImageID{}, ErrBlankImageID
44
+
45
+	var id ImageID
46
+
47
+	elements := strings.Split(s, "/")
48
+	switch len(elements) {
49
+	case 0: // NB strings.Split will never return []
50
+		return id, ErrBlankImageID
51
+	case 1: // no slashes, e.g., "alpine:1.5"; treat as library image
52
+		id.Image = s
53
+	case 2: // may have a domain e.g., "localhost/foo", or not e.g., "weaveworks/scope"
54
+		if domainRegexp.MatchString(elements[0]) {
55
+			id.Domain = elements[0]
56
+			id.Image = elements[1]
57
+		} else {
58
+			id.Image = s
59
+		}
60
+	default: // cannot be a library image, so the first element is assumed to be a domain
61
+		id.Domain = elements[0]
62
+		id.Image = strings.Join(elements[1:], "/")
54 63
 	}
55
-	parts = strings.SplitN(s, "/", 2)
56
-	switch len(parts) {
57
-	case 0:
58
-		return ImageID{}, ErrMalformedImageID
64
+
65
+	// Figure out if there's a tag
66
+	imageParts := strings.Split(id.Image, ":")
67
+	switch len(imageParts) {
59 68
 	case 1:
60
-		img.Host = dockerHubHost
61
-		img.Image = dockerHubLibrary + parts[0]
69
+		break
62 70
 	case 2:
63
-		// Replace docker.io with index.docker.io (#692)
64
-		if parts[0] == oldDockerHubHost {
65
-			parts[0] = dockerHubHost
71
+		if imageParts[0] == "" || imageParts[1] == "" {
72
+			return id, ErrMalformedImageID
66 73
 		}
67
-		img.Host = parts[0]
68
-		img.Image = parts[1]
74
+		id.Image = imageParts[0]
75
+		id.Tag = imageParts[1]
76
+	default:
77
+		return id, ErrMalformedImageID
69 78
 	}
70
-	return img, nil
79
+
80
+	return id, nil
71 81
 }
72 82
 
73
-// Fully qualified name
83
+var (
84
+	domainComponent = `([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])`
85
+	domain          = fmt.Sprintf(`localhost|(%s([.]%s)+)(:[0-9]+)?`, domainComponent, domainComponent)
86
+	domainRegexp    = regexp.MustCompile(domain)
87
+)
88
+
89
+// String returns the ImageID as a string (i.e., unparsed) without canonicalising it.
74 90
 func (i ImageID) String() string {
75 91
 	if i.Image == "" {
76 92
 		return "" // Doesn't make sense to return anything if it doesn't even have an image
77 93
 	}
78
-	var ta string
94
+	var tag string
79 95
 	if i.Tag != "" {
80
-		ta = fmt.Sprintf(":%s", i.Tag)
96
+		tag = ":" + i.Tag
81 97
 	}
82
-	return fmt.Sprintf("%s%s", i.Repository(), ta)
98
+	var host string
99
+	if i.Domain != "" {
100
+		host = i.Domain + "/"
101
+	}
102
+	return fmt.Sprintf("%s%s%s", host, i.Image, tag)
83 103
 }
84 104
 
85 105
 // ImageID is serialized/deserialized as a string
@@ -97,25 +117,47 @@ func (i *ImageID) UnmarshalJSON(data []byte) (err error) {
97 117
 	return err
98 118
 }
99 119
 
100
-// Repository returns the short version of an image's repository (trimming if dockerhub)
120
+// Repository returns the canonicalised path part of an ImageID.
101 121
 func (i ImageID) Repository() string {
102
-	r := i.HostImage()
103
-	r = strings.TrimPrefix(r, dockerHubHost+"/")
104
-	r = strings.TrimPrefix(r, dockerHubLibrary)
105
-	return r
122
+	switch i.Domain {
123
+	case "", oldDockerHubHost, dockerHubHost:
124
+		path := strings.Split(i.Image, "/")
125
+		if len(path) == 1 {
126
+			return "library/" + i.Image
127
+		}
128
+		return i.Image
129
+	default:
130
+		return i.Image
131
+	}
132
+}
133
+
134
+// Registry returns the domain name of the Docker image registry to
135
+// use to fetch the image or image metadata.
136
+func (i ImageID) Registry() string {
137
+	switch i.Domain {
138
+	case "", oldDockerHubHost:
139
+		return dockerHubHost
140
+	default:
141
+		return i.Domain
142
+	}
106 143
 }
107 144
 
108
-// HostImage includes all parts of the image, even if it is from dockerhub.
109
-func (i ImageID) HostImage() string {
110
-	return fmt.Sprintf("%s/%s", i.Host, i.Image)
145
+// CanonicalName returns the canonicalised registry host and image parts
146
+// of the ID.
147
+func (i ImageID) CanonicalName() string {
148
+	return fmt.Sprintf("%s/%s", i.Registry(), i.Repository())
111 149
 }
112 150
 
113
-func (i ImageID) FullID() string {
114
-	return fmt.Sprintf("%s/%s:%s", i.Host, i.Image, i.Tag)
151
+// CanonicalRef returns the full, canonicalised ID including the tag if present.
152
+func (i ImageID) CanonicalRef() string {
153
+	if i.Tag == "" {
154
+		return fmt.Sprintf("%s/%s", i.Registry(), i.Repository())
155
+	}
156
+	return fmt.Sprintf("%s/%s:%s", i.Registry(), i.Repository(), i.Tag)
115 157
 }
116 158
 
117
-func (i ImageID) Components() (host, repo, tag string) {
118
-	return i.Host, i.Image, i.Tag
159
+func (i ImageID) Components() (domain, repo, tag string) {
160
+	return i.Domain, i.Image, i.Tag
119 161
 }
120 162
 
121 163
 // WithNewTag makes a new copy of an ImageID with a new tag

+ 47
- 19
image_test.go View File

@@ -15,26 +15,54 @@ var (
15 15
 	testTime, _ = time.Parse(time.RFC3339Nano, constTime)
16 16
 )
17 17
 
18
+func TestDomainRegexp(t *testing.T) {
19
+	for _, d := range []string{
20
+		"localhost", "localhost:5000",
21
+		"example.com", "example.com:80",
22
+		"gcr.io",
23
+		"index.docker.com",
24
+	} {
25
+		if !domainRegexp.MatchString(d) {
26
+			t.Errorf("domain regexp did not match %q", d)
27
+		}
28
+	}
29
+}
30
+
18 31
 func TestImageID_ParseImageID(t *testing.T) {
19 32
 	for _, x := range []struct {
20 33
 		test     string
21
-		expected string
34
+		registry string
35
+		canon    string
22 36
 	}{
23
-		{"alpine", "alpine:latest"},
24
-		{"library/alpine", "alpine:latest"},
25
-		{"alpine:mytag", "alpine:mytag"},
26
-		{"quay.io/library/alpine", "quay.io/library/alpine:latest"},
27
-		{"quay.io/library/alpine:latest", "quay.io/library/alpine:latest"},
28
-		{"quay.io/library/alpine:mytag", "quay.io/library/alpine:mytag"},
29
-		{"localhost:5000/library/alpine:mytag", "localhost:5000/library/alpine:mytag"},
30
-		{"kube-registry.kube-system.svc.cluster.local:31000/secret/repo:latest", "kube-registry.kube-system.svc.cluster.local:31000/secret/repo:latest"},
37
+		// Library images can have the domain omitted; a
38
+		// single-element path is understood to be prefixed with "library".
39
+		{"alpine", dockerHubHost, "index.docker.io/library/alpine"},
40
+		{"library/alpine", dockerHubHost, "index.docker.io/library/alpine"},
41
+		{"alpine:mytag", dockerHubHost, "index.docker.io/library/alpine:mytag"},
42
+		// The old registry path should be replaced with the new one
43
+		{"docker.io/library/alpine", dockerHubHost, "index.docker.io/library/alpine"},
44
+		// It's possible to have a domain with a single-element path
45
+		{"localhost/hello:v1.1", "localhost", "localhost/hello:v1.1"},
46
+		{"localhost:5000/hello:v1.1", "localhost:5000", "localhost:5000/hello:v1.1"},
47
+		{"example.com/hello:v1.1", "example.com", "example.com/hello:v1.1"},
48
+		// The path can have an arbitrary number of elements
49
+		{"quay.io/library/alpine", "quay.io", "quay.io/library/alpine"},
50
+		{"quay.io/library/alpine:latest", "quay.io", "quay.io/library/alpine:latest"},
51
+		{"quay.io/library/alpine:mytag", "quay.io", "quay.io/library/alpine:mytag"},
52
+		{"localhost:5000/path/to/repo/alpine:mytag", "localhost:5000", "localhost:5000/path/to/repo/alpine:mytag"},
31 53
 	} {
32 54
 		i, err := ParseImageID(x.test)
33 55
 		if err != nil {
34
-			t.Fatalf("Failed parsing %q, expected %q", x.test, x.expected)
56
+			t.Errorf("Failed parsing %q: %s", x.test, err)
57
+		}
58
+		if i.String() != x.test {
59
+			t.Errorf("%q does not stringify as itself", x.test)
60
+		}
61
+		if i.Registry() != x.registry {
62
+			t.Errorf("%q registry: expected %q, got %q", x.test, x.registry, i.Registry())
35 63
 		}
36
-		if i.String() != x.expected {
37
-			t.Fatalf("%q does not match expected %q", i.String(), x.expected)
64
+		if i.CanonicalRef() != x.canon {
65
+			t.Errorf("%q full ID: expected %q, got %q", x.test, x.canon, i.CanonicalRef())
38 66
 		}
39 67
 	}
40 68
 }
@@ -68,7 +96,7 @@ func TestImageID_TestComponents(t *testing.T) {
68 96
 		test     string
69 97
 		expected string
70 98
 	}{
71
-		{i.Host, host},
99
+		{i.Domain, host},
72 100
 		{i.Image, image},
73 101
 		{i.Tag, tag},
74 102
 		{i.String(), fqn},
@@ -85,23 +113,23 @@ func TestImageID_Serialization(t *testing.T) {
85 113
 		test     ImageID
86 114
 		expected string
87 115
 	}{
88
-		{ImageID{Host: dockerHubHost, Image: dockerHubLibrary + "alpine", Tag: "a123"}, `"alpine:a123"`},
89
-		{ImageID{Host: "quay.io", Image: "weaveworks/foobar", Tag: "baz"}, `"quay.io/weaveworks/foobar:baz"`},
116
+		{ImageID{Image: "alpine", Tag: "a123"}, `"alpine:a123"`},
117
+		{ImageID{Domain: "quay.io", Image: "weaveworks/foobar", Tag: "baz"}, `"quay.io/weaveworks/foobar:baz"`},
90 118
 	} {
91 119
 		serialized, err := json.Marshal(x.test)
92 120
 		if err != nil {
93
-			t.Fatalf("Error encoding %v: %v", x.test, err)
121
+			t.Errorf("Error encoding %v: %v", x.test, err)
94 122
 		}
95 123
 		if string(serialized) != x.expected {
96
-			t.Fatalf("Encoded %v as %s, but expected %s", x.test, string(serialized), x.expected)
124
+			t.Errorf("Encoded %v as %s, but expected %s", x.test, string(serialized), x.expected)
97 125
 		}
98 126
 
99 127
 		var decoded ImageID
100 128
 		if err := json.Unmarshal([]byte(x.expected), &decoded); err != nil {
101
-			t.Fatalf("Error decoding %v: %v", x.expected, err)
129
+			t.Errorf("Error decoding %v: %v", x.expected, err)
102 130
 		}
103 131
 		if decoded != x.test {
104
-			t.Fatalf("Decoded %s as %v, but expected %v", x.expected, decoded, x.test)
132
+			t.Errorf("Decoded %s as %v, but expected %v", x.expected, decoded, x.test)
105 133
 		}
106 134
 	}
107 135
 }

+ 2
- 2
registry/cache/memcached.go View File

@@ -237,7 +237,7 @@ type manifestKey struct {
237 237
 }
238 238
 
239 239
 func NewManifestKey(username string, id flux.ImageID) (Keyer, error) {
240
-	return &manifestKey{username, id.HostImage(), id.Tag}, nil
240
+	return &manifestKey{username, id.CanonicalName(), id.Tag}, nil
241 241
 }
242 242
 
243 243
 func (k *manifestKey) Key() string {
@@ -257,7 +257,7 @@ type tagKey struct {
257 257
 }
258 258
 
259 259
 func NewTagKey(username string, id flux.ImageID) (Keyer, error) {
260
-	return &tagKey{username, id.HostImage()}, nil
260
+	return &tagKey{username, id.CanonicalName()}, nil
261 261
 }
262 262
 
263 263
 func (k *tagKey) Key() string {

+ 6
- 6
registry/client.go View File

@@ -35,13 +35,13 @@ type Remote struct {
35 35
 
36 36
 // Return the tags for this repository.
37 37
 func (a *Remote) Tags(id flux.ImageID) ([]string, error) {
38
-	return a.Registry.Tags(id.Image)
38
+	return a.Registry.Tags(id.Repository())
39 39
 }
40 40
 
41 41
 // We need to do some adapting here to convert from the return values
42 42
 // from dockerregistry to our domain types.
43 43
 func (a *Remote) Manifest(id flux.ImageID) (flux.Image, error) {
44
-	manifestV2, err := a.Registry.ManifestV2(id.Image, id.Tag)
44
+	manifestV2, err := a.Registry.ManifestV2(id.Repository(), id.Tag)
45 45
 	if err != nil {
46 46
 		if err, ok := err.(*url.Error); ok {
47 47
 			if err, ok := (err.Err).(*dockerregistry.HttpStatusError); ok {
@@ -62,7 +62,7 @@ func (a *Remote) Manifest(id flux.ImageID) (flux.Image, error) {
62 62
 	// image config. We have to fetch that in order to get the created
63 63
 	// datetime.
64 64
 	conf := manifestV2.Config
65
-	reader, err := a.Registry.DownloadLayer(id.Image, conf.Digest)
65
+	reader, err := a.Registry.DownloadLayer(id.Repository(), conf.Digest)
66 66
 	if err != nil {
67 67
 		return flux.Image{}, err
68 68
 	}
@@ -86,7 +86,7 @@ func (a *Remote) Manifest(id flux.ImageID) (flux.Image, error) {
86 86
 }
87 87
 
88 88
 func (a *Remote) ManifestFromV1(id flux.ImageID) (flux.Image, error) {
89
-	history, err := a.Registry.Manifest(id.Image, id.Tag)
89
+	history, err := a.Registry.Manifest(id.Repository(), id.Tag)
90 90
 	if err != nil || history == nil {
91 91
 		return flux.Image{}, errors.Wrap(err, "getting remote manifest")
92 92
 	}
@@ -142,7 +142,7 @@ func NewCache(creds Credentials, cr cache.Reader, expiry time.Duration, logger l
142 142
 }
143 143
 
144 144
 func (c *Cache) Manifest(id flux.ImageID) (flux.Image, error) {
145
-	creds := c.creds.credsFor(id.Host)
145
+	creds := c.creds.credsFor(id.Registry())
146 146
 	key, err := cache.NewManifestKey(creds.username, id)
147 147
 	if err != nil {
148 148
 		return flux.Image{}, err
@@ -161,7 +161,7 @@ func (c *Cache) Manifest(id flux.ImageID) (flux.Image, error) {
161 161
 }
162 162
 
163 163
 func (c *Cache) Tags(id flux.ImageID) ([]string, error) {
164
-	creds := c.creds.credsFor(id.Host)
164
+	creds := c.creds.credsFor(id.Registry())
165 165
 	key, err := cache.NewTagKey(creds.username, id)
166 166
 	if err != nil {
167 167
 		return []string{}, err

+ 2
- 9
registry/registry.go View File

@@ -50,15 +50,8 @@ func NewRegistry(c ClientFactory, l log.Logger, connections int) Registry {
50 50
 }
51 51
 
52 52
 // GetRepository yields a repository matching the given name, if any exists.
53
-// Repository may be of various forms, in which case omitted elements take
54
-// assumed defaults.
55
-//
56
-//   helloworld             -> index.docker.io/library/helloworld
57
-//   foo/helloworld         -> index.docker.io/foo/helloworld
58
-//   quay.io/foo/helloworld -> quay.io/foo/helloworld
59
-//
60 53
 func (reg *registry) GetRepository(id flux.ImageID) ([]flux.Image, error) {
61
-	client, err := reg.factory.ClientFor(id.Host, Credentials{})
54
+	client, err := reg.factory.ClientFor(id.Registry(), Credentials{})
62 55
 	if err != nil {
63 56
 		return nil, err
64 57
 	}
@@ -79,7 +72,7 @@ func (reg *registry) GetRepository(id flux.ImageID) ([]flux.Image, error) {
79 72
 
80 73
 // Get a single Image from the registry if it exists
81 74
 func (reg *registry) GetImage(id flux.ImageID) (flux.Image, error) {
82
-	client, err := reg.factory.ClientFor(id.Host, Credentials{})
75
+	client, err := reg.factory.ClientFor(id.Registry(), Credentials{})
83 76
 	if err != nil {
84 77
 		return flux.Image{}, err
85 78
 	}

+ 3
- 3
registry/registry_test.go View File

@@ -95,7 +95,7 @@ func TestRemoteFactory_RawClient(t *testing.T) {
95 95
 
96 96
 	// Refresh tags first
97 97
 	var tags []string
98
-	client, err := fact.ClientFor(id.Host, Credentials{})
98
+	client, err := fact.ClientFor(id.Registry(), Credentials{})
99 99
 	if err != nil {
100 100
 		t.Fatal(err)
101 101
 	}
@@ -109,7 +109,7 @@ func TestRemoteFactory_RawClient(t *testing.T) {
109 109
 		t.Fatal("Should have some tags")
110 110
 	}
111 111
 
112
-	client, err = fact.ClientFor(id.Host, Credentials{})
112
+	client, err = fact.ClientFor(id.Registry(), Credentials{})
113 113
 	if err != nil {
114 114
 		t.Fatal(err)
115 115
 	}
@@ -133,7 +133,7 @@ func TestRemoteFactory_InvalidHost(t *testing.T) {
133 133
 	if err != nil {
134 134
 		t.Fatal(err)
135 135
 	}
136
-	client, err := fact.ClientFor(invalidId.Host, Credentials{})
136
+	client, err := fact.ClientFor(invalidId.Registry(), Credentials{})
137 137
 	if err != nil {
138 138
 		return
139 139
 	}

+ 4
- 4
registry/warming.go View File

@@ -58,14 +58,14 @@ func (w *Warmer) Loop(stop <-chan struct{}, wg *sync.WaitGroup, imagesToFetchFun
58 58
 }
59 59
 
60 60
 func (w *Warmer) warm(id flux.ImageID, creds Credentials) {
61
-	client, err := w.ClientFactory.ClientFor(id.Host, creds)
61
+	client, err := w.ClientFactory.ClientFor(id.Registry(), creds)
62 62
 	if err != nil {
63 63
 		w.Logger.Log("err", err.Error())
64 64
 		return
65 65
 	}
66 66
 	defer client.Cancel()
67 67
 
68
-	username := w.Creds.credsFor(id.Host).username
68
+	username := w.Creds.credsFor(id.Registry()).username
69 69
 
70 70
 	// Refresh tags first
71 71
 	// Only, for example, "library/alpine" because we have the host information in the client above.
@@ -126,7 +126,7 @@ func (w *Warmer) warm(id flux.ImageID, creds Credentials) {
126 126
 	w.Logger.Log("fetching", id.String(), "to-update", len(toUpdate))
127 127
 
128 128
 	if expired {
129
-		w.Logger.Log("expiring", id.HostImage())
129
+		w.Logger.Log("expiring", id.String())
130 130
 	}
131 131
 
132 132
 	// The upper bound for concurrent fetches against a single host is
@@ -168,7 +168,7 @@ func (w *Warmer) warm(id flux.ImageID, creds Credentials) {
168 168
 		}(imID)
169 169
 	}
170 170
 	awaitFetchers.Wait()
171
-	w.Logger.Log("updated", id.HostImage())
171
+	w.Logger.Log("updated", id.String())
172 172
 }
173 173
 
174 174
 func withinExpiryBuffer(expiry time.Time, buffer time.Duration) bool {

+ 1
- 1
update/filter.go View File

@@ -30,7 +30,7 @@ func (f *SpecificImageFilter) Filter(u ControllerUpdate) ControllerResult {
30 30
 	for _, c := range u.Controller.Containers.Containers {
31 31
 		cID, _ := flux.ParseImageID(c.Image)
32 32
 		// If container image == image in update
33
-		if cID.HostImage() == f.Img.HostImage() {
33
+		if cID.CanonicalName() == f.Img.CanonicalName() {
34 34
 			// We want to update this
35 35
 			return ControllerResult{}
36 36
 		}

+ 1
- 1
update/print.go View File

@@ -25,7 +25,7 @@ func PrintResults(out io.Writer, results Result, verbose bool) {
25 25
 			extraLines = append(extraLines, result.Error)
26 26
 		}
27 27
 		for _, update := range result.PerContainer {
28
-			extraLines = append(extraLines, fmt.Sprintf("%s: %s -> %s", update.Container, update.Current.FullID(), update.Target.Tag))
28
+			extraLines = append(extraLines, fmt.Sprintf("%s: %s -> %s", update.Container, update.Current.String(), update.Target.Tag))
29 29
 		}
30 30
 
31 31
 		var inline string

Loading…
Cancel
Save