Skip to content

Commit

Permalink
test: refactor AdminURL test helper to return a struct
Browse files Browse the repository at this point in the history 
We were previously passing around our AdminURL() return values as
`string` types which made manipulating them in tests a bit brittle.
Recent addition of tenant selection query params rendered some URL
strings challenging to manipulate without parsing them into a url.URL
first anyway.

This commit migrates to a new struct `serverutils.TestURL` which
wraps `url.URL` and adds a helper `WithPath` that handles the
common use case of concatenating and hides the error in a panic.

All call sites have been migrated.

Epic: None

Release note: None
  • Loading branch information
@dhartunian
dhartunian committed Jun 26, 2023
1 parent f49b836 commit 763b105
Show file tree
Hide file tree
Showing 25 changed files with 113 additions and 108 deletions.
6 changes: 3 additions & 3 deletions pkg/ccl/oidcccl/authentication_oidc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import (
"github.com/stretchr/testify/require"
)

func tenantOrSystemAdminURL(s serverutils.TestServerInterface) string {
func tenantOrSystemAdminURL(s serverutils.TestServerInterface) *serverutils.TestURL {
if len(s.TestTenants()) > 0 {
return s.TestTenants()[0].AdminURL()
}
Expand Down Expand Up @@ -71,7 +71,7 @@ func TestOIDCBadRequestIfDisabled(t *testing.T) {
client, err := testCertsContext.GetHTTPClient()
require.NoError(t, err)

resp, err := client.Get(tenantOrSystemAdminURL(s) + "/oidc/v1/login")
resp, err := client.Get(tenantOrSystemAdminURL(s).WithPath("/oidc/v1/login").String())
if err != nil {
t.Fatalf("could not issue GET request to admin server: %s", err)
}
Expand Down Expand Up @@ -196,7 +196,7 @@ func TestOIDCEnabled(t *testing.T) {
}

t.Run("login redirect", func(t *testing.T) {
resp, err := client.Get(tenantOrSystemAdminURL(s) + "/oidc/v1/login")
resp, err := client.Get(tenantOrSystemAdminURL(s).WithPath("/oidc/v1/login").String())
if err != nil {
t.Fatalf("could not issue GET request to admin server: %s", err)
}
Expand Down
25 changes: 10 additions & 15 deletions pkg/ccl/serverccl/server_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"io"
"net/http"
"net/http/cookiejar"
"net/url"
"testing"
"time"

Expand Down Expand Up @@ -253,7 +252,7 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3
}

newreq := func() *http.Request {
req, err := http.NewRequest("GET", aurl+"/_status/sessions", nil)
req, err := http.NewRequest("GET", aurl.WithPath("/_status/sessions").String(), nil)
require.NoError(t, err)
return req
}
Expand Down Expand Up @@ -289,9 +288,7 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3
HttpOnly: true,
Secure: true,
}
purl, err := url.Parse(aurl)
require.NoError(t, err)
client.Jar.SetCookies(purl, []*http.Cookie{c})
client.Jar.SetCookies(aurl.URL, []*http.Cookie{c})

req = newreq()
body, err = get(req)
Expand All @@ -303,7 +300,7 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3
t.Logf("retrieving session list from test tenant via cookie")

c.Value = "hello"
client.Jar.SetCookies(purl, []*http.Cookie{c})
client.Jar.SetCookies(aurl.URL, []*http.Cookie{c})
req = newreq()
body, err = get(req)
require.NoError(t, err)
Expand Down Expand Up @@ -351,7 +348,7 @@ func TestServerControllerDefaultHTTPTenant(t *testing.T) {
client, err := s.GetUnauthenticatedHTTPClient()
require.NoError(t, err)

resp, err := client.Post(s.AdminURL()+"/login",
resp, err := client.Post(s.AdminURL().WithPath("/login").String(),
"application/json",
bytes.NewBuffer([]byte("{\"username\":\"foo\",\"password\":\"cockroach\"})")),
)
Expand Down Expand Up @@ -392,15 +389,15 @@ func TestServerControllerBadHTTPCookies(t *testing.T) {
Secure: true,
}

req, err := http.NewRequest("GET", s.AdminURL()+"/", nil)
req, err := http.NewRequest("GET", s.AdminURL().WithPath("/").String(), nil)
require.NoError(t, err)
req.AddCookie(c)
resp, err := client.Do(req)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode)

req, err = http.NewRequest("GET", s.AdminURL()+"/bundle.js", nil)
req, err = http.NewRequest("GET", s.AdminURL().WithPath("/bundle.js").String(), nil)
require.NoError(t, err)
req.AddCookie(c)
resp, err = client.Do(req)
Expand Down Expand Up @@ -569,7 +566,7 @@ func TestServerControllerLoginLogout(t *testing.T) {
client, err := s.GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession)
require.NoError(t, err)

resp, err := client.Post(s.AdminURL()+"/logout", "", nil)
resp, err := client.Post(s.AdminURL().WithPath("/logout").String(), "", nil)
require.NoError(t, err)
defer resp.Body.Close()

Expand All @@ -590,7 +587,7 @@ func TestServerControllerLoginLogout(t *testing.T) {
clientMT, err := s2.GetAuthenticatedHTTPClient(false, serverutils.MultiTenantSession)
require.NoError(t, err)

respMT, err := clientMT.Get(s.AdminURL() + "/logout")
respMT, err := clientMT.Get(s.AdminURL().WithPath("/logout").String())
require.NoError(t, err)
defer respMT.Body.Close()

Expand All @@ -605,19 +602,17 @@ func TestServerControllerLoginLogout(t *testing.T) {
require.ElementsMatch(t, []string{"", ""}, cookieValues)

// Now using manual clients to simulate states that might be invalid
url, err := url.Parse(s2.AdminURL())
require.NoError(t, err)
cookieJar, err := cookiejar.New(nil)
require.NoError(t, err)
cookieJar.SetCookies(url, []*http.Cookie{
cookieJar.SetCookies(s2.AdminURL().URL, []*http.Cookie{
{
Name: "multitenant-session",
Value: "abc-123",
},
})
clientMT.Jar = cookieJar

respBadCookie, err := clientMT.Get(s.AdminURL() + "/logout")
respBadCookie, err := clientMT.Get(s.AdminURL().WithPath("/logout").String())
require.NoError(t, err)
defer respBadCookie.Body.Close()

Expand Down
4 changes: 2 additions & 2 deletions pkg/ccl/serverccl/server_sql_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func TestTenantHTTP(t *testing.T) {
httpClient, err := tenant.GetUnauthenticatedHTTPClient()
require.NoError(t, err)
defer httpClient.CloseIdleConnections()
resp, err := httpClient.Get(tenant.AdminURL() + "/_status/vars")
resp, err := httpClient.Get(tenant.AdminURL().WithPath("/_status/vars").String())
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
Expand All @@ -180,7 +180,7 @@ func TestTenantHTTP(t *testing.T) {
httpClient, err := tenant.GetAdminHTTPClient()
require.NoError(t, err)
defer httpClient.CloseIdleConnections()
resp, err := httpClient.Get(tenant.AdminURL() + "/debug/pprof/goroutine?debug=2")
resp, err := httpClient.Get(tenant.AdminURL().WithPath("/debug/pprof/goroutine?debug=2").String())
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
Expand Down
8 changes: 4 additions & 4 deletions pkg/ccl/serverccl/statusccl/tenant_grpc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func TestTenantGRPCServices(t *testing.T) {
defer httpClient.CloseIdleConnections()

t.Run("gRPC Gateway is running", func(t *testing.T) {
resp, err := httpClient.Get(tenant.AdminURL() + "/_status/statements")
resp, err := httpClient.Get(tenant.AdminURL().WithPath("/_status/statements").String())
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
Expand All @@ -97,7 +97,7 @@ func TestTenantGRPCServices(t *testing.T) {
defer connTenant2.Close()

t.Run("statements endpoint fans out request to multiple pods", func(t *testing.T) {
resp, err := httpClient.Get(tenant2.AdminURL() + "/_status/statements")
resp, err := httpClient.Get(tenant2.AdminURL().WithPath("/_status/statements").String())
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
Expand All @@ -117,7 +117,7 @@ func TestTenantGRPCServices(t *testing.T) {
require.NoError(t, err)
defer httpClient3.CloseIdleConnections()

resp, err := httpClient3.Get(tenant3.AdminURL() + "/_status/statements")
resp, err := httpClient3.Get(tenant3.AdminURL().WithPath("/_status/statements").String())
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
Expand Down Expand Up @@ -155,7 +155,7 @@ func TestTenantGRPCServices(t *testing.T) {
})

t.Run("sessions endpoint is available", func(t *testing.T) {
resp, err := httpClient.Get(tenant.AdminURL() + "/_status/sessions")
resp, err := httpClient.Get(tenant.AdminURL().WithPath("/_status/sessions").String())
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode)
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/serverccl/tenant_test_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func (c tenantCluster) TenantHTTPClient(t *testing.T, idx serverIdx, isAdmin boo
client, err = c.Tenant(idx).GetTenant().GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession)
}
require.NoError(t, err)
return &httpClient{t: t, client: client, baseURL: c[idx].GetTenant().AdminURL()}
return &httpClient{t: t, client: client, baseURL: c[idx].GetTenant().AdminURL().String()}
}

func (c tenantCluster) TenantAdminHTTPClient(t *testing.T, idx serverIdx) *httpClient {
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/serverccl/tenant_vars_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestTenantVars(t *testing.T) {
}

startNowNanos := timeutil.Now().UnixNano()
url := tenant.AdminURL() + "/_status/load"
url := tenant.AdminURL().WithPath("/_status/load").String()
client := http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
Expand Down
25 changes: 10 additions & 15 deletions pkg/cli/democluster/demo_cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1899,26 +1899,21 @@ func (c *transientCluster) ListDemoNodes(w, ew io.Writer, justOne, verbose bool)
}

rpcAddr := c.tenantServers[i].RPCAddr()
tenantUiURLstr := c.tenantServers[i].AdminURL()
tenantUiURL, err := url.Parse(tenantUiURLstr)
tenantUiURL := c.tenantServers[i].AdminURL()
tenantSqlURL, err := c.getNetworkURLForServer(context.Background(), i,
false /* includeAppName */, forSecondaryTenant)
if err != nil {
fmt.Fprintln(ew, errors.Wrap(err, "retrieving network URL for tenant server"))
} else {
tenantSqlURL, err := c.getNetworkURLForServer(context.Background(), i,
false /* includeAppName */, forSecondaryTenant)
if err != nil {
fmt.Fprintln(ew, errors.Wrap(err, "retrieving network URL for tenant server"))
} else {
// Only include a separate HTTP URL if there's no server
// controller.
includeHTTP := !c.demoCtx.Multitenant || c.demoCtx.DisableServerController
// Only include a separate HTTP URL if there's no server
// controller.
includeHTTP := !c.demoCtx.Multitenant || c.demoCtx.DisableServerController

socketDetails, err := c.sockForServer(i, forSecondaryTenant)
if err != nil {
fmt.Fprintln(ew, errors.Wrap(err, "retrieving socket URL for tenant server"))
}
c.printURLs(w, ew, tenantSqlURL, tenantUiURL, socketDetails, rpcAddr, verbose, includeHTTP)
socketDetails, err := c.sockForServer(i, forSecondaryTenant)
if err != nil {
fmt.Fprintln(ew, errors.Wrap(err, "retrieving socket URL for tenant server"))
}
c.printURLs(w, ew, tenantSqlURL, tenantUiURL.URL, socketDetails, rpcAddr, verbose, includeHTTP)
}
fmt.Fprintln(w)
if verbose {
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/client_tenant_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func TestTenantRateLimiter(t *testing.T) {
httpClient, err := s.GetUnauthenticatedHTTPClient()
require.NoError(t, err)
getMetrics := func() string {
resp, err := httpClient.Get(s.AdminURL() + "/_status/vars")
resp, err := httpClient.Get(s.AdminURL().WithPath("/_status/vars").String())
require.NoError(t, err)
read, err := io.ReadAll(resp.Body)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/security/certs_rotation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func TestRotateCerts(t *testing.T) {

// Client test function.
clientTest := func(httpClient http.Client) error {
req, err := http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err := http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
return errors.Wrap(err, "could not create request")
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/security/certs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ func TestUseCerts(t *testing.T) {
if err != nil {
t.Fatal(err)
}
req, err := http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err := http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
t.Fatalf("could not create request: %v", err)
}
Expand All @@ -436,7 +436,7 @@ func TestUseCerts(t *testing.T) {
if err != nil {
t.Fatalf("Expected success, got %v", err)
}
req, err = http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err = http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
t.Fatalf("could not create request: %v", err)
}
Expand Down Expand Up @@ -502,7 +502,7 @@ func TestUseSplitCACerts(t *testing.T) {
if err != nil {
t.Fatal(err)
}
req, err := http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err := http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
t.Fatalf("could not create request: %v", err)
}
Expand All @@ -528,7 +528,7 @@ func TestUseSplitCACerts(t *testing.T) {
if err != nil {
t.Fatalf("Expected success, got %v", err)
}
req, err = http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err = http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
t.Fatalf("could not create request: %v", err)
}
Expand Down Expand Up @@ -630,7 +630,7 @@ func TestUseWrongSplitCACerts(t *testing.T) {
if err != nil {
t.Fatal(err)
}
req, err := http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err := http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
t.Fatalf("could not create request: %v", err)
}
Expand All @@ -656,7 +656,7 @@ func TestUseWrongSplitCACerts(t *testing.T) {
if err != nil {
t.Fatalf("Expected success, got %v", err)
}
req, err = http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
req, err = http.NewRequest("GET", s.AdminURL().WithPath("/_status/metrics/local").String(), nil)
if err != nil {
t.Fatalf("could not create request: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/admin_cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func TestAdminAPITableStats(t *testing.T) {
}
}

url := server0.AdminURL() + "/_admin/v1/databases/test test/tables/foo foo/stats"
url := server0.AdminURL().WithPath("/_admin/v1/databases/test test/tables/foo foo/stats").String()
var tsResponse serverpb.TableStatsResponse

// The new SQL table may not yet have split into its own range. Wait for
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/admin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func getJSON(ts serverutils.TestServerInterface, url string) (interface{}, error

// debugURL returns the root debug URL.
func debugURL(s serverutils.TestServerInterface) string {
return s.AdminURL() + debug.Endpoint
return s.AdminURL().WithPath(debug.Endpoint).String()
}

// TestAdminDebugExpVar verifies that cmdline and memstats variables are
Expand Down
6 changes: 3 additions & 3 deletions pkg/server/api_v2_ranges_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestHotRangesV2(t *testing.T) {
client, err := ts.GetAdminHTTPClient()
require.NoError(t, err)

req, err := http.NewRequest("GET", ts.AdminURL()+apiV2Path+"ranges/hot/", nil)
req, err := http.NewRequest("GET", ts.AdminURL().WithPath(apiV2Path+"ranges/hot/").String(), nil)
require.NoError(t, err)
resp, err := client.Do(req)
require.NoError(t, err)
Expand Down Expand Up @@ -76,7 +76,7 @@ func TestNodeRangesV2(t *testing.T) {
client, err := ts.GetAdminHTTPClient()
require.NoError(t, err)

req, err := http.NewRequest("GET", ts.AdminURL()+apiV2Path+"nodes/local/ranges/", nil)
req, err := http.NewRequest("GET", ts.AdminURL().WithPath(apiV2Path+"nodes/local/ranges/").String(), nil)
require.NoError(t, err)
resp, err := client.Do(req)
require.NoError(t, err)
Expand Down Expand Up @@ -141,7 +141,7 @@ func TestNodesV2(t *testing.T) {
client, err := ts1.GetAdminHTTPClient()
require.NoError(t, err)

req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"nodes/", nil)
req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"nodes/").String(), nil)
require.NoError(t, err)
resp, err := client.Do(req)
require.NoError(t, err)
Expand Down
Loading

0 comments on commit 763b105

Please sign in to comment.