From 5b7134f126d3a216dbb278133a6c2fda61bde35a Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Tue, 22 Mar 2016 17:37:51 +0100 Subject: [PATCH 1/2] discovery: return only best matching templates. Actually the spec doesn't clarify when an endpoint template should be accepted or not. Now, the appc/spec implementation returns only endpoints that can be fully rendered. This means that it will also accept a template if it doesn't contain some of the required discovery labels. This looks good, but, trying to implement some meta discovery ideas it will bring to unwanted endpoints. Example 1: Suppose I want to implement the "latest" pattern behavior: ```html ``` If, on discovery, I ask for the _name_, _os_ and _arch_ labels only the second template will be rendered (since the first cannot be fully rendered due to missing _version_ label). So I achieved latest pattern. But if I'm asking for a specific _version_ both templates will be rendered. Example 2: As an extension of the first example, suppose I want to create a global meta discovery for all my images on example.com. So on the root of my server I'll put some meta tags using example.com as prefix: ```html ``` With this tags I want to implement global "latest" and "noarch" patterns and also return multiple mirrors. If, on discovery, I ask only for the _name_ and _version_ labels the template 3 and 4 will be rendered. So I achieved a versioned noarch pattern. But, unfortunately, also template 5, 6, 7, and 8 will be rendered. And, as the first example, if I'm asking for a specific _name_, _version_, _os_ and _arch_ ALL the templates will be rendered. Since the spec says: ``` Note that multiple `ac-discovery` tags MAY be returned for a given prefix-match [snip] In this case, the client implementation MAY choose which to use at its own discretion. ``` If an implementation chooses the second it can download the wrong image version. This patch makes template matching stricter choosing only best matching templates. Best matching templates are the one where all the templates labels can be substituted and with the highest number of substituted labels. With this we can obtain various patterns like latest and noarch and also keeping the ability to return multiple mirror urls. See also the tests added in this commit. It also documents this behavior. It should not BREAK many of the current meta tags implementation (it depends on how the client uses the returned endpoints, for example rkt, currently, only uses the first one) --- discovery/discovery.go | 29 +++++-- discovery/discovery_test.go | 141 ++++++++++++++++++++++++++++++++- discovery/testdata/meta06.html | 14 +++- discovery/testdata/meta07.html | 13 +++ spec/discovery.md | 12 +++ 5 files changed, 195 insertions(+), 14 deletions(-) create mode 100644 discovery/testdata/meta07.html diff --git a/discovery/discovery.go b/discovery/discovery.go index b2dc6595..17fe77ca 100644 --- a/discovery/discovery.go +++ b/discovery/discovery.go @@ -107,20 +107,26 @@ func extractACMeta(r io.Reader) []acMeta { } } -func renderTemplate(tpl string, kvs ...string) (string, bool) { +func renderTemplate(tpl string, kvs ...string) (string, int, bool) { + count := 0 for i := 0; i < len(kvs); i += 2 { k := kvs[i] v := kvs[i+1] + if k != "{name}" && strings.Contains(tpl, k) { + count++ + } tpl = strings.Replace(tpl, k, v, -1) } - return tpl, !templateExpression.MatchString(tpl) + return tpl, count, !templateExpression.MatchString(tpl) } func createTemplateVars(app App) []string { tplVars := []string{"{name}", app.Name.String()} - // If a label is called "name", it will be ignored as it appears after - // in the slice + // Ignore labels called "name" for n, v := range app.Labels { + if n == "name" { + continue + } tplVars = append(tplVars, fmt.Sprintf("{%s}", n), v) } return tplVars @@ -144,6 +150,7 @@ func doDiscover(pre string, hostHeaders map[string]http.Header, app App, insecur dd := &discoveryData{} + bestCount := 0 for _, m := range meta { if !strings.HasPrefix(app.Name.String(), m.prefix) { continue @@ -152,16 +159,22 @@ func doDiscover(pre string, hostHeaders map[string]http.Header, app App, insecur switch m.name { case "ac-discovery": // Ignore not handled variables as {ext} isn't already rendered. - uri, _ := renderTemplate(m.uri, tplVars...) - asc, ok := renderTemplate(uri, "{ext}", "aci.asc") + uri, count, _ := renderTemplate(m.uri, tplVars...) + asc, _, ok := renderTemplate(uri, "{ext}", "aci.asc") if !ok { continue } - aci, ok := renderTemplate(uri, "{ext}", "aci") + aci, _, ok := renderTemplate(uri, "{ext}", "aci") if !ok { continue } - dd.ACIEndpoints = append(dd.ACIEndpoints, ACIEndpoint{ACI: aci, ASC: asc}) + + if count > bestCount { + dd.ACIEndpoints = ACIEndpoints{ACIEndpoint{ACI: aci, ASC: asc}} + bestCount = count + } else if count == bestCount { + dd.ACIEndpoints = append(dd.ACIEndpoints, ACIEndpoint{ACI: aci, ASC: asc}) + } case "ac-discovery-pubkeys": dd.PublicKeys = append(dd.PublicKeys, m.uri) diff --git a/discovery/discovery_test.go b/discovery/discovery_test.go index bb2ca254..ef7db7a4 100644 --- a/discovery/discovery_test.go +++ b/discovery/discovery_test.go @@ -461,6 +461,7 @@ func TestDiscoverEndpoints(t *testing.T) { nil, }, // Test multiple ACIEndpoints. + // Should render the first two endpoint, since the others match less labels { &mockHTTPDoer{ doer: fakeHTTPGet( @@ -488,12 +489,144 @@ func TestDiscoverEndpoints(t *testing.T) { ASC: "https://storage.example.com/example.com/myapp-1.0.0-linux-amd64.aci.asc", }, ACIEndpoint{ - ACI: "https://storage.example.com/example.com/myapp-1.0.0.aci", - ASC: "https://storage.example.com/example.com/myapp-1.0.0.aci.asc", + ACI: "https://mirror.storage.example.com/example.com/myapp-1.0.0-linux-amd64.aci", + ASC: "https://mirror.storage.example.com/example.com/myapp-1.0.0-linux-amd64.aci.asc", + }, + }, + []string{"https://example.com/pubkeys.gpg"}, + nil, + }, + // Test multiple ACIEndpoints. + // Should render endpoint 3 and 4, since the 1 and 2 doesn't fully render and the others match less labels + // Example noarch versioned matching + { + &mockHTTPDoer{ + doer: fakeHTTPGet( + []meta{ + {"/myapp", + "meta06.html", + }, + }, + nil, + ), + }, + true, + true, + App{ + Name: "example.com/myapp", + Labels: map[types.ACIdentifier]string{ + "version": "1.0.0", + }, + }, + []ACIEndpoint{ + ACIEndpoint{ + ACI: "https://storage.example.com/example.com/myapp-1.0.0-noarch.aci", + ASC: "https://storage.example.com/example.com/myapp-1.0.0-noarch.aci.asc", + }, + ACIEndpoint{ + ACI: "https://mirror.storage.example.com/example.com/myapp-1.0.0-noarch.aci", + ASC: "https://mirror.storage.example.com/example.com/myapp-1.0.0-noarch.aci.asc", + }, + }, + []string{"https://example.com/pubkeys.gpg"}, + nil, + }, + // Test multiple ACIEndpoints. + // Should render endpoint 5 and 6, since the 1, 2, 3, 4 doesn't fully render and the others match less labels + // Example latest matching + { + &mockHTTPDoer{ + doer: fakeHTTPGet( + []meta{ + {"/myapp", + "meta06.html", + }, + }, + nil, + ), + }, + true, + true, + App{ + Name: "example.com/myapp", + Labels: map[types.ACIdentifier]string{ + "os": "linux", + "arch": "amd64", + }, + }, + []ACIEndpoint{ + ACIEndpoint{ + ACI: "https://storage.example.com/example.com/myapp-latest-linux-amd64.aci", + ASC: "https://storage.example.com/example.com/myapp-latest-linux-amd64.aci.asc", + }, + ACIEndpoint{ + ACI: "https://mirror.storage.example.com/example.com/myapp-latest-linux-amd64.aci", + ASC: "https://mirror.storage.example.com/example.com/myapp-latest-linux-amd64.aci.asc", + }, + }, + []string{"https://example.com/pubkeys.gpg"}, + nil, + }, + // Test multiple ACIEndpoints. + // Should render endpoint 7 and 8, since the others don't fully render. + // Example noarch latest matching + { + &mockHTTPDoer{ + doer: fakeHTTPGet( + []meta{ + {"/myapp", + "meta06.html", + }, + }, + nil, + ), + }, + true, + true, + App{ + Name: "example.com/myapp", + Labels: map[types.ACIdentifier]string{}, + }, + []ACIEndpoint{ + ACIEndpoint{ + ACI: "https://storage.example.com/example.com/myapp-latest-noarch.aci", + ASC: "https://storage.example.com/example.com/myapp-latest-noarch.aci.asc", }, ACIEndpoint{ - ACI: "hdfs://storage.example.com/example.com/myapp-1.0.0-linux-amd64.aci", - ASC: "hdfs://storage.example.com/example.com/myapp-1.0.0-linux-amd64.aci.asc", + ACI: "https://mirror.storage.example.com/example.com/myapp-latest-noarch.aci", + ASC: "https://mirror.storage.example.com/example.com/myapp-latest-noarch.aci.asc", + }, + }, + []string{"https://example.com/pubkeys.gpg"}, + nil, + }, + + // Test a discovery string that has an hardcoded app name instead of using the provided {name} + { + &mockHTTPDoer{ + doer: fakeHTTPGet( + []meta{ + {"/myapp", + "meta07.html", + }, + }, + nil, + ), + }, + true, + true, + App{ + Name: "example.com/myapp", + Labels: map[types.ACIdentifier]string{ + "version": "1.0.0", + "os": "linux", + "arch": "amd64", + }, + }, + []ACIEndpoint{ + ACIEndpoint{ + ACI: "https://storage.example.com/myapp-1.0.0-linux-amd64.aci", + ASC: "https://storage.example.com/myapp-1.0.0-linux-amd64.aci.asc", }, }, []string{"https://example.com/pubkeys.gpg"}, diff --git a/discovery/testdata/meta06.html b/discovery/testdata/meta06.html index 0814f58b..487cd280 100644 --- a/discovery/testdata/meta06.html +++ b/discovery/testdata/meta06.html @@ -3,9 +3,19 @@ My app + - - + + + + + + + + + + + diff --git a/discovery/testdata/meta07.html b/discovery/testdata/meta07.html new file mode 100644 index 00000000..a9b0b800 --- /dev/null +++ b/discovery/testdata/meta07.html @@ -0,0 +1,13 @@ + + + + + My app + + + + + +

My App

+ + diff --git a/spec/discovery.md b/spec/discovery.md index b5033ef3..a6f01a54 100644 --- a/spec/discovery.md +++ b/spec/discovery.md @@ -59,6 +59,18 @@ curl $(echo "$urltmpl" | sed -e "s/{name}/$name/" -e "s/{version}/$version/" -e where _name_, _version_, _os_, and _arch_ are set to their respective values for the image, and _ext_ is either `aci` or `aci.asc` for retrieving an App Container Image or signature respectively. +The client MUST accept only best matching templates. Best matching templates are the one where all the templates labels can be substituted and with the highest number of substituted labels. + +For example given these meta tags: +```html + + + +``` + +If the client requires the image name _name_ and labels _version_, _os_, _arch_ only the first two templates will be rendered since they match 3 labels (while the the last matches only 2 labels since it doesn't match the _version_ label). +If the client requires the image name _name_ and labels _os_, _arch_ only the last template will be rendered since in the first template '{version}' cannot be substituted. + Note that multiple `ac-discovery` tags MAY be returned for a given prefix-match (for example, with different scheme names representing different transport mechanisms). In this case, the client implementation MAY choose which to use at its own discretion. Public discovery implementations SHOULD always provide at least one HTTPS URL template. From 13ab4e09a6b5ddc34a5155502578b07f3bedcaa5 Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Wed, 23 Mar 2016 10:32:44 +0100 Subject: [PATCH 2/2] discovery: do not set version to latest if not provided Actually, if the required discovery labels doesn't contain a version label, it'll be set to a default of "latest". This behavior isn't documented in the spec and can create unwanted behaviors. This patch removes this and also adds to the spec an example on how to obtain the latest behavior without this "hidden" default. --- discovery/discovery.go | 7 ------- discovery/discovery_test.go | 12 +++--------- spec/discovery.md | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/discovery/discovery.go b/discovery/discovery.go index 17fe77ca..76ff7fae 100644 --- a/discovery/discovery.go +++ b/discovery/discovery.go @@ -49,10 +49,6 @@ type ACIEndpoints []ACIEndpoint type PublicKeys []string -const ( - defaultVersion = "latest" -) - var ( templateExpression = regexp.MustCompile(`{.*?}`) errEnough = errors.New("enough discovery information found") @@ -134,9 +130,6 @@ func createTemplateVars(app App) []string { func doDiscover(pre string, hostHeaders map[string]http.Header, app App, insecure InsecureOption) (*discoveryData, error) { app = *app.Copy() - if app.Labels["version"] == "" { - app.Labels["version"] = defaultVersion - } _, body, err := httpsOrHTTP(pre, hostHeaders, insecure) if err != nil { diff --git a/discovery/discovery_test.go b/discovery/discovery_test.go index ef7db7a4..fd897016 100644 --- a/discovery/discovery_test.go +++ b/discovery/discovery_test.go @@ -402,8 +402,7 @@ func TestDiscoverEndpoints(t *testing.T) { []string{"https://example.com/pubkeys.gpg"}, nil, }, - // Test missing labels. version label should default to - // "latest" and the first template should be rendered + // Test missing labels. No template will render { &mockHTTPDoer{ doer: fakeHTTPGet( @@ -415,18 +414,13 @@ func TestDiscoverEndpoints(t *testing.T) { nil, ), }, - true, + false, true, App{ Name: "example.com/myapp", Labels: map[types.ACIdentifier]string{}, }, - []ACIEndpoint{ - ACIEndpoint{ - ACI: "https://storage.example.com/example.com/myapp-latest.aci", - ASC: "https://storage.example.com/example.com/myapp-latest.aci.asc", - }, - }, + []ACIEndpoint{}, []string{"https://example.com/pubkeys.gpg"}, nil, }, diff --git a/spec/discovery.md b/spec/discovery.md index a6f01a54..fe6485ff 100644 --- a/spec/discovery.md +++ b/spec/discovery.md @@ -107,3 +107,19 @@ Authentication during the discovery process is optional. If an attempt at fetching any resource (the initial discovery URL, an App Container Image, or signature) returns a `401 Unauthorized`, implementations should enact the authentication policy set by the operator. For example, some implementations might only perform HTTP basic authentication over HTTPS connections. + +### Examples + +#### Latest pattern + +If a client wants to retrieve the latest available ACI (without knowing its version) it can provide these meta tags: + +```html + + +``` + +When requiring a specific version, the first template will be rendered, when not requiring a _version_ label the second template will match. + +On the http server, the "latest" url should point to the current latest ACI. +