Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

discovery: return only best matching templates. #596

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

discovery: return only best matching templates. #596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 21 additions & 8 deletions discovery/discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really we should probably error if this happens, but I guess this is OK

continue
}
tplVars = append(tplVars, fmt.Sprintf("{%s}", n), v)
}
return tplVars
Expand All @@ -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
Expand All @@ -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)
Expand Down
157 changes: 149 additions & 8 deletions discovery/discovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,8 @@ func TestDiscoverEndpoints(t *testing.T) {
&mockHTTPDoer{
doer: fakeHTTPGet(
[]meta{
{"/myapp",
{
"/myapp",
"meta05.html",
},
},
Expand Down Expand Up @@ -408,7 +409,8 @@ func TestDiscoverEndpoints(t *testing.T) {
&mockHTTPDoer{
doer: fakeHTTPGet(
[]meta{
{"/myapp",
{
"/myapp",
"meta05.html",
},
},
Expand Down Expand Up @@ -461,11 +463,13 @@ func TestDiscoverEndpoints(t *testing.T) {
nil,
},
// Test multiple ACIEndpoints.
// Should render the first two endpoint, since the others match fewer labels
{
&mockHTTPDoer{
doer: fakeHTTPGet(
[]meta{
{"/myapp",
{
"/myapp",
"meta06.html",
},
},
Expand All @@ -488,12 +492,148 @@ 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 endpoints 3 and 4, since 1 and 2 don't fully render and the others match fewer 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 endpoints 5 and 6, since 1, 2, 3, 4 don't fully render and the others match fewer 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 endpoints 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"},
Expand All @@ -505,7 +645,8 @@ func TestDiscoverEndpoints(t *testing.T) {
&mockHTTPDoer{
doer: fakeHTTPGet(
[]meta{
{"/myapp",
{
"/myapp",
"meta01.html",
},
},
Expand Down
14 changes: 12 additions & 2 deletions discovery/testdata/meta06.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,19 @@
<html>
<head>
<title>My app</title>
<!-- ACIs with specific version -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}.{ext}">
<meta name="ac-discovery" content="example.com hdfs://storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<!-- noarch ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-noarch.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-{version}-noarch.{ext}">
<!-- Latest ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-latest-{os}-{arch}.{ext}">
<!-- Latest noarch ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-noarch.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-latest-noarch.{ext}">

<meta name="ac-discovery-pubkeys" content="example.com https://example.com/pubkeys.gpg">
</head>

Expand Down
13 changes: 13 additions & 0 deletions discovery/testdata/meta07.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!DOCTYPE html>

<html>
<head>
<title>My app</title>
<meta name="ac-discovery" content="example.com/myapp https://storage.example.com/myapp-{version}-{os}-{arch}.{ext}">
<meta name="ac-discovery-pubkeys" content="example.com https://example.com/pubkeys.gpg">
</head>

<body>
<h1>My App</h1>
</body>
</html>
12 changes: 12 additions & 0 deletions spec/discovery.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a little underspecified here which labels clients should/may require. We should probably call that out specifically

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the templates labels can be substituted and with the highest number of substituted labels

I don't really get the rationale here. If all client-supplied labels must be substituted, then won't be "highest number of substituted labels" always be equal to the number of client-supplied labels?

Or are you talking about templates where the same variable might exist multiple times? e.g. given client-supplied label {name}, then {name}/{name}.{ext} being somehow preferable to {name}.{ext} - if so, why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all client-supplied labels must be substituted

That was the original idea in #580, in this patch there isn't the requirement to substitute all the client supplied labels but to return only the templates that match the highest number of client-supplied labels.

Perhaps, If you agree on this new idea, the spec description needs your help for better wording 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this patch there isn't the requirement to substitute all the client supplied labels

but in the text you have

all the templates labels can be substituted and with the highest number of substituted labels

Perhaps it should just be this instead?

with the highest number of substituted labels

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in the text you have

all the templates labels can be substituted and with the highest number of substituted labels

Sorry, I'll try to reword it:

all the templates expressions can be substituted by the client supplied labels and have the highest number of substituted client supplied labels.

Where all the templates expressions can be substituted by the client supplied labels is already in the code but not defined, so it's not related to this patch, just added in the spec.

have the highest number of substituted client supplied labels is what this patch does.


For example given these meta tags:
```html
<meta name="ac-discovery" content="example.com https://storage.example.com/{os}/{arch}/{name}-{version}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{os}/{arch}/{name}-{version}.{ext}">
<meta name="ac-discovery" content="example.com https://storage.example.com/{os}/{arch}/{name}-latest.{ext}">
```

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.
Expand Down