client/tailscale: fix Client.BuildURL and Client.BuildTailnetURL (#15064)

This method uses `path.Join` to build the URL. Turns out with 1.24 this
started stripping consecutive "/" characters, so "http://..." in baseURL
becomes "http:/...".

Also, `c.Tailnet` is a function that returns `c.tailnet`. Using it as a
path element would encode as a pointer instead of the tailnet name.

Finally, provide a way to prevent escaping of path elements e.g. for `?`
in `acl?details=1`.

Updates #15015

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

client/tailscale/acl.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/netip"
+	"net/url"
 )
 
 // ACLRow defines a rule that grants access by a set of users or groups to a set
@@ -126,7 +127,7 @@ func (c *Client) ACLHuJSON(ctx context.Context) (acl *ACLHuJSON, err error) {
 		}
 	}()
 
-	path := c.BuildTailnetURL("acl?details=1")
+	path := c.BuildTailnetURL("acl", url.Values{"details": {"1"}})
 	req, err := http.NewRequestWithContext(ctx, "GET", path, nil)
 	if err != nil {
 		return nil, err
@@ -146,7 +147,7 @@ func (c *Client) ACLHuJSON(ctx context.Context) (acl *ACLHuJSON, err error) {
 		Warnings []string `json:"warnings"`
 	}{}
 	if err := json.Unmarshal(b, &data); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("json.Unmarshal %q: %w", b, err)
 	}
 
 	acl = &ACLHuJSON{
@@ -328,7 +329,7 @@ type ACLPreview struct {
 }
 
 func (c *Client) previewACLPostRequest(ctx context.Context, body []byte, previewType string, previewFor string) (res *ACLPreviewResponse, err error) {
-	path := c.BuildTailnetURL("acl/preview")
+	path := c.BuildTailnetURL("acl", "preview")
 	req, err := http.NewRequestWithContext(ctx, "POST", path, bytes.NewBuffer(body))
 	if err != nil {
 		return nil, err
@@ -488,7 +489,7 @@ func (c *Client) ValidateACLJSON(ctx context.Context, source, dest string) (test
 		return nil, err
 	}
 
-	path := c.BuildTailnetURL("acl/validate")
+	path := c.BuildTailnetURL("acl", "validate")
 	req, err := http.NewRequestWithContext(ctx, "POST", path, bytes.NewBuffer(postData))
 	if err != nil {
 		return nil, err

client/tailscale/tailscale.go

@@ -66,31 +66,41 @@ func (c *Client) httpClient() *http.Client {
 }
 
 // BuildURL builds a url to http(s)://<apiserver>/api/v2/<slash-separated-pathElements>
-// using the given pathElements. It url escapes each path element, so the caller
-// doesn't need to worry about that.
+// using the given pathElements. It url escapes each path element, so the
+// caller doesn't need to worry about that. The last item of pathElements can
+// be of type url.Values to add a query string to the URL.
 //
 // For example, BuildURL(devices, 5) with the default server URL would result in
 // https://api.tailscale.com/api/v2/devices/5.
 func (c *Client) BuildURL(pathElements ...any) string {
-	elem := make([]string, 2, len(pathElements)+1)
-	elem[0] = c.baseURL()
-	elem[1] = "/api/v2"
-	for _, pathElement := range pathElements {
-		elem = append(elem, url.PathEscape(fmt.Sprint(pathElement)))
+	elem := make([]string, 1, len(pathElements)+1)
+	elem[0] = "/api/v2"
+	var query string
+	for i, pathElement := range pathElements {
+		if uv, ok := pathElement.(url.Values); ok && i == len(pathElements)-1 {
+			query = uv.Encode()
+		} else {
+			elem = append(elem, url.PathEscape(fmt.Sprint(pathElement)))
+		}
 	}
-	return path.Join(elem...)
+	url := c.baseURL() + path.Join(elem...)
+	if query != "" {
+		url += "?" + query
+	}
+	return url
 }
 
 // BuildTailnetURL builds a url to http(s)://<apiserver>/api/v2/tailnet/<tailnet>/<slash-separated-pathElements>
-// using the given pathElements.  It url escapes each path element, so the
-// caller doesn't need to worry about that.
+// using the given pathElements. It url escapes each path element, so the
+// caller doesn't need to worry about that. The last item of pathElements can
+// be of type url.Values to add a query string to the URL.
 //
 // For example, BuildTailnetURL(policy, validate) with the default server URL and a tailnet of "example.com"
 // would result in https://api.tailscale.com/api/v2/tailnet/example.com/policy/validate.
 func (c *Client) BuildTailnetURL(pathElements ...any) string {
-	allElements := make([]any, 3, len(pathElements)+2)
+	allElements := make([]any, 2, len(pathElements)+2)
 	allElements[0] = "tailnet"
-	allElements[1] = c.Tailnet
+	allElements[1] = c.tailnet
 	allElements = append(allElements, pathElements...)
 	return c.BuildURL(allElements...)
 }
@@ -189,7 +199,7 @@ func (e ErrResponse) Error() string {
 func HandleErrorResponse(b []byte, resp *http.Response) error {
 	var errResp ErrResponse
 	if err := json.Unmarshal(b, &errResp); err != nil {
-		return err
+		return fmt.Errorf("json.Unmarshal %q: %w", b, err)
 	}
 	errResp.Status = resp.StatusCode
 	return errResp

client/tailscale/tailscale_test.go

@@ -0,0 +1,86 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package tailscale
+
+import (
+	"net/url"
+	"testing"
+)
+
+func TestClientBuildURL(t *testing.T) {
+	c := Client{BaseURL: "http://127.0.0.1:1234"}
+	for _, tt := range []struct {
+		desc     string
+		elements []any
+		want     string
+	}{
+		{
+			desc:     "single-element",
+			elements: []any{"devices"},
+			want:     "http://127.0.0.1:1234/api/v2/devices",
+		},
+		{
+			desc:     "multiple-elements",
+			elements: []any{"tailnet", "example.com"},
+			want:     "http://127.0.0.1:1234/api/v2/tailnet/example.com",
+		},
+		{
+			desc:     "escape-element",
+			elements: []any{"tailnet", "example dot com?foo=bar"},
+			want:     `http://127.0.0.1:1234/api/v2/tailnet/example%20dot%20com%3Ffoo=bar`,
+		},
+		{
+			desc:     "url.Values",
+			elements: []any{"tailnet", "example.com", "acl", url.Values{"details": {"1"}}},
+			want:     `http://127.0.0.1:1234/api/v2/tailnet/example.com/acl?details=1`,
+		},
+	} {
+		t.Run(tt.desc, func(t *testing.T) {
+			got := c.BuildURL(tt.elements...)
+			if got != tt.want {
+				t.Errorf("got %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestClientBuildTailnetURL(t *testing.T) {
+	c := Client{
+		BaseURL: "http://127.0.0.1:1234",
+		tailnet: "example.com",
+	}
+	for _, tt := range []struct {
+		desc     string
+		elements []any
+		want     string
+	}{
+		{
+			desc:     "single-element",
+			elements: []any{"devices"},
+			want:     "http://127.0.0.1:1234/api/v2/tailnet/example.com/devices",
+		},
+		{
+			desc:     "multiple-elements",
+			elements: []any{"devices", 123},
+			want:     "http://127.0.0.1:1234/api/v2/tailnet/example.com/devices/123",
+		},
+		{
+			desc:     "escape-element",
+			elements: []any{"foo bar?baz=qux"},
+			want:     `http://127.0.0.1:1234/api/v2/tailnet/example.com/foo%20bar%3Fbaz=qux`,
+		},
+		{
+			desc:     "url.Values",
+			elements: []any{"acl", url.Values{"details": {"1"}}},
+			want:     `http://127.0.0.1:1234/api/v2/tailnet/example.com/acl?details=1`,
+		},
+	} {
+		t.Run(tt.desc, func(t *testing.T) {
+			got := c.BuildTailnetURL(tt.elements...)
+			if got != tt.want {
+				t.Errorf("got %q, want %q", got, tt.want)
+			}
+		})
+	}
+}