discovery: correctly split DiscoverEndpoints and DiscoverPublicKeys

currently DiscoverEndpoints and DiscoverPublicKeys, despite their name,
returns both endpoints and keys.

Additionally their results are different.

For example: suppose in `example.com/images/app` we have this meta tags:

```
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}.{ext}">
```

while in a parent path like `example.com/images` we have this meta tag:

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

Calling DiscoverEndpoints will stop at the meta tags in
`example.com/images/app` and return 1 endpoint and 0 pubkeys

Calling DiscoverPublicKeys won't find any meta tag in
`example.com/images/app` so will search in `example.com/images` and
return 0 endpoints and 1 keys

To avoid these problems these functions should return just their
requested type. So the user is not tempted to use the other type just
calling one of the functions. This mean that endpoints and keys discovery
must do different http calls.

* fix actool that used only DiscoverEndpoints to also get discovery
keys.

* Remove appending of previously discovered endpoint and keys since I
can't find a logic in it (simplyfing various functions).

* Rework tests to check also this and other kind of problems and be more fine grained.

Author: sgotti

actool/discover.go

@@ -64,25 +64,40 @@ func runDiscover(args []string) (exit int) {
 		}
 		eps, attempts, err := discovery.DiscoverEndpoints(*app, nil, insecure)
 		if err != nil {
-			stderr("error fetching %s: %s", name, err)
+			stderr("error fetching endpoints for %s: %s", name, err)
 			return 1
 		}
 		for _, a := range attempts {
-			fmt.Printf("discover walk: prefix: %s error: %v\n", a.Prefix, a.Error)
+			fmt.Printf("discover endpoints walk: prefix: %s error: %v\n", a.Prefix, a.Error)
 		}
+		keys, attempts, err := discovery.DiscoverPublicKeys(*app, nil, insecure)
+		if err != nil {
+			stderr("error fetching keys for %s: %s", name, err)
+			return 1
+		}
+		for _, a := range attempts {
+			fmt.Printf("discover keys walk: prefix: %s error: %v\n", a.Prefix, a.Error)
+		}
+
+		type discoveryData struct {
+			ACIEndpoints []discovery.ACIEndpoint
+			Keys         []string
+		}
+
 		if outputJson {
-			jsonBytes, err := json.MarshalIndent(&eps, "", "    ")
+			dd := discoveryData{ACIEndpoints: eps, Keys: keys}
+			jsonBytes, err := json.MarshalIndent(dd, "", "    ")
 			if err != nil {
 				stderr("error generating JSON: %s", err)
 				return 1
 			}
 			fmt.Println(string(jsonBytes))
 		} else {
-			for _, aciEndpoint := range eps.ACIEndpoints {
+			for _, aciEndpoint := range eps {
 				fmt.Printf("ACI: %s, ASC: %s\n", aciEndpoint.ACI, aciEndpoint.ASC)
 			}
-			if len(eps.Keys) > 0 {
-				fmt.Println("Keys: " + strings.Join(eps.Keys, ","))
+			if len(keys) > 0 {
+				fmt.Println("Keys: " + strings.Join(keys, ","))
 			}
 		}
 	}

discovery/discovery.go

@@ -37,12 +37,19 @@ type ACIEndpoint struct {
 	ASC string
 }
 
-type Endpoints struct {
+// A struct containing both discovered endpoints and keys. Used to avoid
+// function duplication (one for endpoints and one for keys, so to avoid two
+// doDiscover, two DiscoverWalkFunc)
+type discoveryData struct {
 	ACIEndpoints []ACIEndpoint
 	Keys         []string
 }
 
-func (e *Endpoints) Append(ep Endpoints) {
+type Endpoints []ACIEndpoint
+
+type Keys []string
+
+func (e *discoveryData) Append(ep discoveryData) {
 	e.ACIEndpoints = append(e.ACIEndpoints, ep.ACIEndpoints...)
 	e.Keys = append(e.Keys, ep.Keys...)
 }
@@ -124,7 +131,7 @@ func createTemplateVars(app App) []string {
 	return tplVars
 }
 
-func doDiscover(pre string, hostHeaders map[string]http.Header, app App, insecure InsecureOption) (*Endpoints, error) {
+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
@@ -140,7 +147,7 @@ func doDiscover(pre string, hostHeaders map[string]http.Header, app App, insecur
 
 	tplVars := createTemplateVars(app)
 
-	de := &Endpoints{}
+	dd := &discoveryData{}
 
 	for _, m := range meta {
 		if !strings.HasPrefix(app.Name.String(), m.prefix) {
@@ -159,41 +166,37 @@ func doDiscover(pre string, hostHeaders map[string]http.Header, app App, insecur
 			if !ok {
 				continue
 			}
-			de.ACIEndpoints = append(de.ACIEndpoints, ACIEndpoint{ACI: aci, ASC: asc})
+			dd.ACIEndpoints = append(dd.ACIEndpoints, ACIEndpoint{ACI: aci, ASC: asc})
 
 		case "ac-discovery-pubkeys":
-			de.Keys = append(de.Keys, m.uri)
+			dd.Keys = append(dd.Keys, m.uri)
 		}
 	}
 
-	return de, nil
+	return dd, nil
 }
 
 // DiscoverWalk will make HTTPS requests to find discovery meta tags and
 // optionally will use HTTP if insecure is set. hostHeaders specifies the
 // header to apply depending on the host (e.g. authentication). Based on the
 // response of the discoverFn it will continue to recurse up the tree.
-func DiscoverWalk(app App, hostHeaders map[string]http.Header, insecure InsecureOption, discoverFn DiscoverWalkFunc) (err error) {
-	var (
-		eps *Endpoints
-	)
-
+func DiscoverWalk(app App, hostHeaders map[string]http.Header, insecure InsecureOption, discoverFn DiscoverWalkFunc) (dd *discoveryData, err error) {
 	parts := strings.Split(string(app.Name), "/")
 	for i := range parts {
 		end := len(parts) - i
 		pre := strings.Join(parts[:end], "/")
 
-		eps, err = doDiscover(pre, hostHeaders, app, insecure)
-		if derr := discoverFn(pre, eps, err); derr != nil {
-			return derr
+		dd, err = doDiscover(pre, hostHeaders, app, insecure)
+		if derr := discoverFn(pre, dd, err); derr != nil {
+			return dd, derr
 		}
 	}
 
-	return
+	return nil, fmt.Errorf("discovery failed")
 }
 
 // DiscoverWalkFunc can stop a DiscoverWalk by returning non-nil error.
-type DiscoverWalkFunc func(prefix string, eps *Endpoints, err error) error
+type DiscoverWalkFunc func(prefix string, dd *discoveryData, err error) error
 
 // FailedAttempt represents a failed discovery attempt. This is for debugging
 // and user feedback.
@@ -202,14 +205,13 @@ type FailedAttempt struct {
 	Error  error
 }
 
-func walker(out *Endpoints, attempts *[]FailedAttempt, testFn DiscoverWalkFunc) DiscoverWalkFunc {
-	return func(pre string, eps *Endpoints, err error) error {
+func walker(attempts *[]FailedAttempt, testFn DiscoverWalkFunc) DiscoverWalkFunc {
+	return func(pre string, dd *discoveryData, err error) error {
 		if err != nil {
 			*attempts = append(*attempts, FailedAttempt{pre, err})
 			return nil
 		}
-		out.Append(*eps)
-		if err := testFn(pre, eps, err); err != nil {
+		if err := testFn(pre, dd, err); err != nil {
 			return err
 		}
 		return nil
@@ -221,40 +223,40 @@ func walker(out *Endpoints, attempts *[]FailedAttempt, testFn DiscoverWalkFunc)
 // specifies the header to apply depending on the host (e.g. authentication).
 // It will not give up until it has exhausted the path or found an image
 // discovery.
-func DiscoverEndpoints(app App, hostHeaders map[string]http.Header, insecure InsecureOption) (out *Endpoints, attempts []FailedAttempt, err error) {
-	out = &Endpoints{}
-	testFn := func(pre string, eps *Endpoints, err error) error {
-		if len(out.ACIEndpoints) != 0 {
+func DiscoverEndpoints(app App, hostHeaders map[string]http.Header, insecure InsecureOption) (Endpoints, []FailedAttempt, error) {
+	testFn := func(pre string, dd *discoveryData, err error) error {
+		if len(dd.ACIEndpoints) != 0 {
 			return errEnough
 		}
 		return nil
 	}
 
-	err = DiscoverWalk(app, hostHeaders, insecure, walker(out, &attempts, testFn))
+	attempts := []FailedAttempt{}
+	dd, err := DiscoverWalk(app, hostHeaders, insecure, walker(&attempts, testFn))
 	if err != nil && err != errEnough {
 		return nil, attempts, err
 	}
 
-	return out, attempts, nil
+	return dd.ACIEndpoints, attempts, nil
 }
 
 // DiscoverPublicKey will make HTTPS requests to find the ac-public-keys meta
 // tags and optionally will use HTTP if insecure is set. hostHeaders
 // specifies the header to apply depending on the host (e.g. authentication).
 // It will not give up until it has exhausted the path or found an public key.
-func DiscoverPublicKeys(app App, hostHeaders map[string]http.Header, insecure InsecureOption) (out *Endpoints, attempts []FailedAttempt, err error) {
-	out = &Endpoints{}
-	testFn := func(pre string, eps *Endpoints, err error) error {
-		if len(out.Keys) != 0 {
+func DiscoverPublicKeys(app App, hostHeaders map[string]http.Header, insecure InsecureOption) (Keys, []FailedAttempt, error) {
+	testFn := func(pre string, dd *discoveryData, err error) error {
+		if len(dd.Keys) != 0 {
 			return errEnough
 		}
 		return nil
 	}
 
-	err = DiscoverWalk(app, hostHeaders, insecure, walker(out, &attempts, testFn))
+	attempts := []FailedAttempt{}
+	dd, err := DiscoverWalk(app, hostHeaders, insecure, walker(&attempts, testFn))
 	if err != nil && err != errEnough {
 		return nil, attempts, err
 	}
 
-	return out, attempts, nil
+	return dd.Keys, attempts, nil
 }