From 125379f9cf8614b308edb558d05a6dfecc85cf9c Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Thu, 15 Jun 2023 17:27:07 -0400 Subject: [PATCH 1/2] test: refactor AdminURL test helper to return a struct 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 --- pkg/ccl/oidcccl/authentication_oidc_test.go | 6 ++-- pkg/ccl/serverccl/server_controller_test.go | 28 ++++++++---------- pkg/ccl/serverccl/server_sql_test.go | 8 +++-- .../serverccl/statusccl/tenant_grpc_test.go | 8 ++--- pkg/ccl/serverccl/tenant_test_utils.go | 2 +- pkg/ccl/serverccl/tenant_vars_test.go | 2 +- pkg/cli/democluster/demo_cluster.go | 25 +++++++--------- pkg/kv/kvserver/client_tenant_test.go | 2 +- pkg/security/certs_rotation_test.go | 2 +- pkg/security/certs_test.go | 12 ++++---- pkg/server/admin_cluster_test.go | 2 +- pkg/server/admin_test.go | 6 ++-- pkg/server/api_v2_ranges_test.go | 6 ++-- pkg/server/api_v2_sql_schema_test.go | 16 +++++----- pkg/server/api_v2_sql_test.go | 2 +- pkg/server/api_v2_test.go | 10 +++---- pkg/server/authentication_test.go | 16 ++++------ pkg/server/node_http_router_test.go | 9 ++---- pkg/server/server_http_test.go | 2 +- pkg/server/server_test.go | 20 ++++++------- pkg/server/status_test.go | 12 ++++---- pkg/server/testserver_http.go | 4 +-- pkg/testutils/serverutils/test_server_shim.go | 5 ++-- pkg/testutils/serverutils/test_tenant_shim.go | 29 ++++++++++++++++++- pkg/testutils/testcluster/testcluster_test.go | 4 +-- 25 files changed, 127 insertions(+), 111 deletions(-) diff --git a/pkg/ccl/oidcccl/authentication_oidc_test.go b/pkg/ccl/oidcccl/authentication_oidc_test.go index ad07c8e3bbce..2d7c06170da5 100644 --- a/pkg/ccl/oidcccl/authentication_oidc_test.go +++ b/pkg/ccl/oidcccl/authentication_oidc_test.go @@ -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() } @@ -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) } @@ -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) } diff --git a/pkg/ccl/serverccl/server_controller_test.go b/pkg/ccl/serverccl/server_controller_test.go index 9edaef5030fa..2a193ec080ca 100644 --- a/pkg/ccl/serverccl/server_controller_test.go +++ b/pkg/ccl/serverccl/server_controller_test.go @@ -16,7 +16,6 @@ import ( "io" "net/http" "net/http/cookiejar" - "net/url" "testing" "time" @@ -167,7 +166,6 @@ func TestServerControllerHTTP(t *testing.T) { // Retrieve a privileged HTTP client. NB: this also populates // system.web_sessions. - aurl := s.AdminURL() client, err := s.GetAdminHTTPClient() require.NoError(t, err) @@ -252,8 +250,10 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3 return &ls, err } + var aurl *serverutils.TestURL newreq := func() *http.Request { - req, err := http.NewRequest("GET", aurl+"/_status/sessions", nil) + aurl = s.AdminURL() + req, err := http.NewRequest("GET", aurl.WithPath("/_status/sessions").String(), nil) require.NoError(t, err) return req } @@ -289,9 +289,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) @@ -303,7 +301,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) @@ -351,7 +349,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\"})")), ) @@ -392,7 +390,7 @@ 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) @@ -400,7 +398,7 @@ func TestServerControllerBadHTTPCookies(t *testing.T) { 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) @@ -569,7 +567,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() @@ -590,7 +588,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() @@ -605,11 +603,9 @@ 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", @@ -617,7 +613,7 @@ func TestServerControllerLoginLogout(t *testing.T) { }) 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() diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index d9dbe83d5f6b..ffa7aebd0246 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -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) @@ -180,7 +180,11 @@ 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") + u := tenant.AdminURL().WithPath("/debug/pprof/goroutine") + q := u.Query() + q.Set("debug", "2") + u.RawQuery = q.Encode() + resp, err := httpClient.Get(u.String()) require.NoError(t, err) defer resp.Body.Close() body, err := io.ReadAll(resp.Body) diff --git a/pkg/ccl/serverccl/statusccl/tenant_grpc_test.go b/pkg/ccl/serverccl/statusccl/tenant_grpc_test.go index 57cb9f6d0f7f..aee73a266e2d 100644 --- a/pkg/ccl/serverccl/statusccl/tenant_grpc_test.go +++ b/pkg/ccl/serverccl/statusccl/tenant_grpc_test.go @@ -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) @@ -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) @@ -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) @@ -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) diff --git a/pkg/ccl/serverccl/tenant_test_utils.go b/pkg/ccl/serverccl/tenant_test_utils.go index c32a8b5d32b2..41aa5ecd296a 100644 --- a/pkg/ccl/serverccl/tenant_test_utils.go +++ b/pkg/ccl/serverccl/tenant_test_utils.go @@ -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 { diff --git a/pkg/ccl/serverccl/tenant_vars_test.go b/pkg/ccl/serverccl/tenant_vars_test.go index d7d6db4134a0..26087ae225a3 100644 --- a/pkg/ccl/serverccl/tenant_vars_test.go +++ b/pkg/ccl/serverccl/tenant_vars_test.go @@ -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}, diff --git a/pkg/cli/democluster/demo_cluster.go b/pkg/cli/democluster/demo_cluster.go index e7801589153b..4b833dcfa309 100644 --- a/pkg/cli/democluster/demo_cluster.go +++ b/pkg/cli/democluster/demo_cluster.go @@ -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 { diff --git a/pkg/kv/kvserver/client_tenant_test.go b/pkg/kv/kvserver/client_tenant_test.go index 973ed252c38f..3ced0e2f285f 100644 --- a/pkg/kv/kvserver/client_tenant_test.go +++ b/pkg/kv/kvserver/client_tenant_test.go @@ -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) diff --git a/pkg/security/certs_rotation_test.go b/pkg/security/certs_rotation_test.go index 8979464c055a..a1871b6125e3 100644 --- a/pkg/security/certs_rotation_test.go +++ b/pkg/security/certs_rotation_test.go @@ -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") } diff --git a/pkg/security/certs_test.go b/pkg/security/certs_test.go index 9d79d180643a..0dd3a25f8d43 100644 --- a/pkg/security/certs_test.go +++ b/pkg/security/certs_test.go @@ -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) } @@ -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) } @@ -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) } @@ -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) } @@ -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) } @@ -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) } diff --git a/pkg/server/admin_cluster_test.go b/pkg/server/admin_cluster_test.go index dea4cd2addcc..4fce51af9edc 100644 --- a/pkg/server/admin_cluster_test.go +++ b/pkg/server/admin_cluster_test.go @@ -128,7 +128,7 @@ func TestAdminAPITableStats(t *testing.T) { } } - url := server0.AdminURL() + "/_admin/v1/databases/test test/tables/foo foo/stats" + url := server0.AdminURL().String() + "/_admin/v1/databases/test test/tables/foo foo/stats" var tsResponse serverpb.TableStatsResponse // The new SQL table may not yet have split into its own range. Wait for diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go index 53b70fd59070..e2f40bddb141 100644 --- a/pkg/server/admin_test.go +++ b/pkg/server/admin_test.go @@ -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 @@ -382,14 +382,14 @@ func TestAdminAPIStatementDiagnosticsBundle(t *testing.T) { client, err := ts.GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession) require.NoError(t, err) - resp, err := client.Get(ts.AdminURL() + "/_admin/v1/stmtbundle/" + diagnosticRow) + resp, err := client.Get(ts.AdminURL().WithPath("/_admin/v1/stmtbundle/" + diagnosticRow).String()) require.NoError(t, err) defer resp.Body.Close() require.Equal(t, 500, resp.StatusCode) adminClient, err := ts.GetAuthenticatedHTTPClient(true, serverutils.SingleTenantSession) require.NoError(t, err) - adminResp, err := adminClient.Get(ts.AdminURL() + "/_admin/v1/stmtbundle/" + diagnosticRow) + adminResp, err := adminClient.Get(ts.AdminURL().WithPath("/_admin/v1/stmtbundle/" + diagnosticRow).String()) require.NoError(t, err) defer adminResp.Body.Close() require.Equal(t, 200, adminResp.StatusCode) diff --git a/pkg/server/api_v2_ranges_test.go b/pkg/server/api_v2_ranges_test.go index 3b89130fdea2..b8fde8bed994 100644 --- a/pkg/server/api_v2_ranges_test.go +++ b/pkg/server/api_v2_ranges_test.go @@ -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) @@ -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) @@ -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) diff --git a/pkg/server/api_v2_sql_schema_test.go b/pkg/server/api_v2_sql_schema_test.go index a9c4cebf9ef3..cb9bdc1978fe 100644 --- a/pkg/server/api_v2_sql_schema_test.go +++ b/pkg/server/api_v2_sql_schema_test.go @@ -44,7 +44,7 @@ func TestUsersV2(t *testing.T) { client, err := ts1.GetAdminHTTPClient() require.NoError(t, err) - req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"users/", nil) + req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"users/").String(), nil) require.NoError(t, err) resp, err := client.Do(req) require.NoError(t, err) @@ -86,7 +86,7 @@ func TestDatabasesTablesV2(t *testing.T) { require.NoError(t, err) defer client.CloseIdleConnections() - req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"databases/", nil) + req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"databases/").String(), nil) require.NoError(t, err) resp, err := client.Do(req) require.NoError(t, err) @@ -99,7 +99,7 @@ func TestDatabasesTablesV2(t *testing.T) { require.Contains(t, dr.Databases, "testdb") - req, err = http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"databases/testdb/", nil) + req, err = http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"databases/testdb/").String(), nil) require.NoError(t, err) resp, err = client.Do(req) require.NoError(t, err) @@ -110,7 +110,7 @@ func TestDatabasesTablesV2(t *testing.T) { require.NoError(t, json.NewDecoder(resp.Body).Decode(&ddr)) require.NoError(t, resp.Body.Close()) - req, err = http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"databases/testdb/grants/", nil) + req, err = http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"databases/testdb/grants/").String(), nil) require.NoError(t, err) resp, err = client.Do(req) require.NoError(t, err) @@ -122,7 +122,7 @@ func TestDatabasesTablesV2(t *testing.T) { require.NoError(t, resp.Body.Close()) require.NotEmpty(t, dgr.Grants) - req, err = http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"databases/testdb/tables/", nil) + req, err = http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"databases/testdb/tables/").String(), nil) require.NoError(t, err) resp, err = client.Do(req) require.NoError(t, err) @@ -135,7 +135,7 @@ func TestDatabasesTablesV2(t *testing.T) { require.Contains(t, dtr.TableNames, "public.testtable") // Test that querying the wrong db name returns 404. - req, err = http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"databases/testdb2/tables/", nil) + req, err = http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"databases/testdb2/tables/").String(), nil) require.NoError(t, err) resp, err = client.Do(req) require.NoError(t, err) @@ -143,7 +143,7 @@ func TestDatabasesTablesV2(t *testing.T) { require.Equal(t, 404, resp.StatusCode) require.NoError(t, resp.Body.Close()) - req, err = http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"databases/testdb/tables/public.testtable/", nil) + req, err = http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"databases/testdb/tables/public.testtable/").String(), nil) require.NoError(t, err) resp, err = client.Do(req) require.NoError(t, err) @@ -181,7 +181,7 @@ func TestEventsV2(t *testing.T) { client, err := ts1.GetAdminHTTPClient() require.NoError(t, err) - req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"events/", nil) + req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"events/").String(), nil) require.NoError(t, err) resp, err := client.Do(req) require.NoError(t, err) diff --git a/pkg/server/api_v2_sql_test.go b/pkg/server/api_v2_sql_test.go index 230a0be0b761..70d032364be3 100644 --- a/pkg/server/api_v2_sql_test.go +++ b/pkg/server/api_v2_sql_test.go @@ -62,7 +62,7 @@ func TestExecSQL(t *testing.T) { } resp, err := client.Post( - server.AdminURL()+"/api/v2/sql/", "application/json", + server.AdminURL().WithPath("/api/v2/sql/").String(), "application/json", bytes.NewReader([]byte(d.Input)), ) require.NoError(t, err) diff --git a/pkg/server/api_v2_test.go b/pkg/server/api_v2_test.go index ac65bb86ae98..7dd3747bf37f 100644 --- a/pkg/server/api_v2_test.go +++ b/pkg/server/api_v2_test.go @@ -59,7 +59,7 @@ func TestListSessionsV2(t *testing.T) { }() doSessionsRequest := func(client http.Client, limit int, start string) listSessionsResponse { - req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"sessions/", nil) + req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"sessions/").String(), nil) require.NoError(t, err) query := req.URL.Query() query.Add("exclude_closed_sessions", "true") @@ -120,7 +120,7 @@ func TestListSessionsV2(t *testing.T) { // A non-admin user cannot see sessions at all. nonAdminClient, err := ts1.GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession) require.NoError(t, err) - req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"sessions/", nil) + req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"sessions/").String(), nil) require.NoError(t, err) resp, err := nonAdminClient.Do(req) require.NoError(t, err) @@ -145,7 +145,7 @@ func TestHealthV2(t *testing.T) { client, err := ts1.GetAdminHTTPClient() require.NoError(t, err) - req, err := http.NewRequest("GET", ts1.AdminURL()+apiV2Path+"health/", nil) + req, err := http.NewRequest("GET", ts1.AdminURL().WithPath(apiV2Path+"health/").String(), nil) require.NoError(t, err) resp, err := client.Do(req) require.NoError(t, err) @@ -173,7 +173,7 @@ func TestRulesV2(t *testing.T) { client, err := ts.GetUnauthenticatedHTTPClient() require.NoError(t, err) - req, err := http.NewRequest("GET", ts.AdminURL()+apiV2Path+"rules/", nil) + req, err := http.NewRequest("GET", ts.AdminURL().WithPath(apiV2Path+"rules/").String(), nil) require.NoError(t, err) resp, err := client.Do(req) require.NoError(t, err) @@ -246,7 +246,7 @@ func TestAuthV2(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - req, err := http.NewRequest("GET", ts.AdminURL()+apiV2Path+"sessions/", nil) + req, err := http.NewRequest("GET", ts.AdminURL().WithPath(apiV2Path+"sessions/").String(), nil) require.NoError(t, err) if tc.header != "" { req.Header.Set(apiV2AuthHeader, tc.header) diff --git a/pkg/server/authentication_test.go b/pkg/server/authentication_test.go index 4b9781c887b2..912b8bb55c77 100644 --- a/pkg/server/authentication_test.go +++ b/pkg/server/authentication_test.go @@ -520,7 +520,7 @@ func TestAuthenticationAPIUserLogin(t *testing.T) { } var resp serverpb.UserLoginResponse return httputil.PostJSONWithRequest( - httpClient, ts.AdminURL()+loginPath, &req, &resp, + httpClient, ts.AdminURL().WithPath(loginPath).String(), &req, &resp, ) } @@ -591,7 +591,7 @@ func TestLogoutClearsCookies(t *testing.T) { require.NoError(t, err) // Log out. - resp, err := authHTTPClient.Get(ts.AdminURL() + logoutPath) + resp, err := authHTTPClient.Get(ts.AdminURL().WithPath(logoutPath).String()) require.NoError(t, err) defer resp.Body.Close() @@ -621,7 +621,7 @@ func TestLogout(t *testing.T) { // Log out. var resp serverpb.UserLogoutResponse - if err := httputil.GetJSON(authHTTPClient, ts.AdminURL()+logoutPath, &resp); err != nil { + if err := httputil.GetJSON(authHTTPClient, ts.AdminURL().WithPath(logoutPath).String(), &resp); err != nil { t.Fatal("logout request failed:", err) } @@ -637,7 +637,7 @@ func TestLogout(t *testing.T) { t.Fatal("expected revoked at to not be empty; was empty") } - databasesURL := ts.AdminURL() + "/_admin/v1/databases" + databasesURL := ts.AdminURL().WithPath("/_admin/v1/databases").String() // Verify that we're unauthorized after logout. response, err := authHTTPClient.Get(databasesURL) @@ -651,10 +651,6 @@ func TestLogout(t *testing.T) { } // Try to use the revoked cookie; verify that it doesn't work. - parsedURL, err := url.Parse(s.AdminURL()) - if err != nil { - t.Fatal(err) - } encodedCookie, err := EncodeSessionCookie(cookie, false /* forHTTPSOnly */) if err != nil { t.Fatal(err) @@ -669,7 +665,7 @@ func TestLogout(t *testing.T) { t.Fatal(err) } invalidAuthClient.Jar = jar - invalidAuthClient.Jar.SetCookies(parsedURL, []*http.Cookie{encodedCookie}) + invalidAuthClient.Jar.SetCookies(s.AdminURL().URL, []*http.Cookie{encodedCookie}) invalidAuthResp, err := invalidAuthClient.Get(databasesURL) if err != nil { @@ -708,7 +704,7 @@ func TestAuthenticationMux(t *testing.T) { runRequest := func( client http.Client, method string, path string, body []byte, cookieHeader string, expected int, ) error { - req, err := http.NewRequest(method, tsrv.AdminURL()+path, bytes.NewBuffer(body)) + req, err := http.NewRequest(method, tsrv.AdminURL().WithPath(path).String(), bytes.NewBuffer(body)) if cookieHeader != "" { // The client still attaches its own cookies to this one. req.Header.Set("cookie", cookieHeader) diff --git a/pkg/server/node_http_router_test.go b/pkg/server/node_http_router_test.go index d45d681c51ce..c56ebdc642b9 100644 --- a/pkg/server/node_http_router_test.go +++ b/pkg/server/node_http_router_test.go @@ -16,7 +16,6 @@ import ( "io" "net/http" "net/http/cookiejar" - "net/url" "regexp" "testing" @@ -154,11 +153,9 @@ func TestRouteToNode(t *testing.T) { client.Jar, err = cookiejar.New(&cookiejar.Options{}) require.NoError(t, err) } - adminURL, err := url.Parse(s.AdminURL()) - require.NoError(t, err) - client.Jar.SetCookies(adminURL, []*http.Cookie{{Name: RemoteNodeID, Value: rt.nodeIDRequestedInCookie}}) + client.Jar.SetCookies(s.AdminURL().URL, []*http.Cookie{{Name: RemoteNodeID, Value: rt.nodeIDRequestedInCookie}}) - requestPath := s.AdminURL() + rt.path + requestPath := s.AdminURL().WithPath(rt.path).String() if rt.nodeIDRequestedInQueryParam != "" { requestPath += fmt.Sprintf("?%s=%s", RemoteNodeID, rt.nodeIDRequestedInQueryParam) } @@ -191,7 +188,7 @@ func TestRouteToNode(t *testing.T) { client, err := s.GetUnauthenticatedHTTPClient() require.NoError(t, err) - resp, err := client.Get(s.AdminURL() + fmt.Sprintf("/_status/vars?%s=%s", RemoteNodeID, "2")) + resp, err := client.Get(s.AdminURL().WithPath(fmt.Sprintf("/_status/vars?%s=%s", RemoteNodeID, "2")).String()) require.NoError(t, err) defer resp.Body.Close() // We expect some error here. It's difficult to know what diff --git a/pkg/server/server_http_test.go b/pkg/server/server_http_test.go index 03112d87894f..55f5f6eb76e9 100644 --- a/pkg/server/server_http_test.go +++ b/pkg/server/server_http_test.go @@ -40,7 +40,7 @@ func TestHSTS(t *testing.T) { urlsToTest := []string{"/", "/_status/vars", "/index.html"} - adminURLHTTPS := s.AdminURL() + adminURLHTTPS := s.AdminURL().String() adminURLHTTP := strings.Replace(adminURLHTTPS, "https", "http", 1) for _, u := range urlsToTest { diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 97ba5a690b8a..d009095ad26e 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -99,7 +99,7 @@ func TestPanicRecovery(t *testing.T) { })) // Create a request. - req, err := http.NewRequest(http.MethodGet, ts.AdminURL()+"/panic", nil /* body */) + req, err := http.NewRequest(http.MethodGet, ts.AdminURL().WithPath("/panic").String(), nil /* body */) require.NoError(t, err) // Create a ResponseRecorder to record the response. @@ -261,7 +261,7 @@ func TestPlainHTTPServer(t *testing.T) { // They won't succeed because we're not jumping through // authentication hoops, but they verify that the server is using // the correct protocol. - url := s.AdminURL() + url := s.AdminURL().String() if !strings.HasPrefix(url, "http://") { t.Fatalf("expected insecure admin url to start with http://, but got %s", url) } @@ -372,7 +372,7 @@ func TestAcceptEncoding(t *testing.T) { } for _, d := range testData { func() { - req, err := http.NewRequest("GET", s.AdminURL()+statusPrefix+"metrics/local", nil) + req, err := http.NewRequest("GET", s.AdminURL().WithPath(statusPrefix+"metrics/local").String(), nil) if err != nil { t.Fatalf("could not create request: %s", err) } @@ -764,7 +764,7 @@ func TestServeIndexHTML(t *testing.T) { require.NoError(t, err) t.Run("short build", func(t *testing.T) { - resp, err := client.Get(s.AdminURL()) + resp, err := client.Get(s.AdminURL().String()) require.NoError(t, err) defer resp.Body.Close() require.Equal(t, 200, resp.StatusCode) @@ -785,7 +785,7 @@ Binary built without web UI. t.Run("non-short build", func(t *testing.T) { linkInFakeUI() defer unlinkFakeUI() - resp, err := client.Get(s.AdminURL()) + resp, err := client.Get(s.AdminURL().String()) require.NoError(t, err) defer resp.Body.Close() require.Equal(t, 200, resp.StatusCode) @@ -796,7 +796,7 @@ Binary built without web UI. respString := string(respBytes) require.Equal(t, htmlTemplate, respString) - resp, err = client.Get(s.AdminURL() + "/uiconfig") + resp, err = client.Get(s.AdminURL().WithPath("/uiconfig").String()) require.NoError(t, err) defer resp.Body.Close() require.Equal(t, 200, resp.StatusCode) @@ -851,7 +851,7 @@ Binary built without web UI. for i, testCase := range cases { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - req, err := http.NewRequestWithContext(ctx, "GET", s.AdminURL(), nil) + req, err := http.NewRequestWithContext(ctx, "GET", s.AdminURL().String(), nil) require.NoError(t, err) resp, err := testCase.client.Do(req) @@ -865,7 +865,7 @@ Binary built without web UI. respString := string(respBytes) require.Equal(t, htmlTemplate, respString) - req, err = http.NewRequestWithContext(ctx, "GET", s.AdminURL()+"/uiconfig", nil) + req, err = http.NewRequestWithContext(ctx, "GET", s.AdminURL().WithPath("/uiconfig").String(), nil) require.NoError(t, err) resp, err = testCase.client.Do(req) @@ -925,7 +925,7 @@ Binary built without web UI. for _, testCase := range cases { t.Run(fmt.Sprintf("bundle caching for %s", testCase.desc), func(t *testing.T) { // Request bundle.js without an If-None-Match header first, to simulate the initial load - uncachedReq, err := http.NewRequestWithContext(ctx, "GET", s.AdminURL()+"/bundle.js", nil) + uncachedReq, err := http.NewRequestWithContext(ctx, "GET", s.AdminURL().WithPath("/bundle.js").String(), nil) require.NoError(t, err) uncachedResp, err := testCase.client.Do(uncachedReq) @@ -937,7 +937,7 @@ Binary built without web UI. require.NotEmpty(t, etag, "Server must provide ETag response header with asset responses") // Use that ETag header on the next request to simulate a client reload - cachedReq, err := http.NewRequestWithContext(ctx, "GET", s.AdminURL()+"/bundle.js", nil) + cachedReq, err := http.NewRequestWithContext(ctx, "GET", s.AdminURL().WithPath("/bundle.js").String(), nil) require.NoError(t, err) cachedReq.Header.Add("If-None-Match", etag) diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index f8a46711ebaa..1a3e23508612 100644 --- a/pkg/server/status_test.go +++ b/pkg/server/status_test.go @@ -193,7 +193,7 @@ func TestHealthTelemetry(t *testing.T) { if err := serverutils.GetJSONProto(s, "/health", &details); err != nil { t.Fatal(err) } - if _, err := getText(s, s.AdminURL()+statusPrefix+"vars"); err != nil { + if _, err := getText(s, s.AdminURL().WithPath(statusPrefix+"vars").String()); err != nil { t.Fatal(err) } @@ -999,7 +999,7 @@ func TestMetricsEndpoint(t *testing.T) { s := startServer(t) defer s.Stopper().Stop(context.Background()) - if _, err := getText(s, s.AdminURL()+statusPrefix+"metrics/"+s.Gossip().NodeID.String()); err != nil { + if _, err := getText(s, s.AdminURL().WithPath(statusPrefix+"metrics/"+s.Gossip().NodeID.String()).String()); err != nil { t.Fatal(err) } } @@ -1358,12 +1358,12 @@ func TestStatusVars(t *testing.T) { s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) defer s.Stopper().Stop(context.Background()) - if body, err := getText(s, s.AdminURL()+statusPrefix+"vars"); err != nil { + if body, err := getText(s, s.AdminURL().WithPath(statusPrefix+"vars").String()); err != nil { t.Fatal(err) } else if !bytes.Contains(body, []byte("# TYPE sql_bytesout counter\nsql_bytesout")) { t.Errorf("expected sql_bytesout, got: %s", body) } - if body, err := getText(s, s.AdminURL()+statusPrefix+"load"); err != nil { + if body, err := getText(s, s.AdminURL().WithPath(statusPrefix+"load").String()); err != nil { t.Fatal(err) } else if !bytes.Contains(body, []byte("# TYPE sys_cpu_user_ns gauge\nsys_cpu_user_ns")) { t.Errorf("expected sys_cpu_user_ns, got: %s", body) @@ -1387,7 +1387,7 @@ func TestStatusVarsTxnMetrics(t *testing.T) { t.Fatal(err) } - body, err := getText(s, s.AdminURL()+statusPrefix+"vars") + body, err := getText(s, s.AdminURL().WithPath(statusPrefix+"vars").String()) if err != nil { t.Fatal(err) } @@ -1429,7 +1429,7 @@ func TestSpanStatsResponse(t *testing.T) { Spans: []roachpb.Span{span}, } - url := ts.AdminURL() + statusPrefix + "span" + url := ts.AdminURL().WithPath(statusPrefix + "span").String() if err := httputil.PostJSON(httpClient, url, &request, &response); err != nil { t.Fatal(err) } diff --git a/pkg/server/testserver_http.go b/pkg/server/testserver_http.go index d560de7318a7..0510ffe64554 100644 --- a/pkg/server/testserver_http.go +++ b/pkg/server/testserver_http.go @@ -64,14 +64,14 @@ func (t tenantHeaderDecorator) RoundTrip(req *http.Request) (*http.Response, err var _ http.RoundTripper = &tenantHeaderDecorator{} // AdminURL implements TestServerInterface. -func (ts *httpTestServer) AdminURL() string { +func (ts *httpTestServer) AdminURL() *serverutils.TestURL { u := ts.t.sqlServer.execCfg.RPCContext.Config.AdminURL() if ts.t.tenantName != "" { q := u.Query() q.Add(TenantNameParamInQueryURL, string(ts.t.tenantName)) u.RawQuery = q.Encode() } - return u.String() + return &serverutils.TestURL{URL: u} } // GetUnauthenticatedHTTPClient implements TestServerInterface. diff --git a/pkg/testutils/serverutils/test_server_shim.go b/pkg/testutils/serverutils/test_server_shim.go index 4617d453ba04..6cf38f984ef7 100644 --- a/pkg/testutils/serverutils/test_server_shim.go +++ b/pkg/testutils/serverutils/test_server_shim.go @@ -523,7 +523,8 @@ func GetJSONProtoWithAdminOption( if err != nil { return err } - fullURL := ts.AdminURL() + path + u := ts.AdminURL().String() + fullURL := u + path log.Infof(context.Background(), "test retrieving protobuf over HTTP: %s", fullURL) return httputil.GetJSON(httpClient, fullURL, response) } @@ -544,7 +545,7 @@ func PostJSONProtoWithAdminOption( if err != nil { return err } - fullURL := ts.AdminURL() + path + fullURL := ts.AdminURL().WithPath(path).String() log.Infof(context.Background(), "test retrieving protobuf over HTTP: %s", fullURL) return httputil.PostJSON(httpClient, fullURL, request, response) } diff --git a/pkg/testutils/serverutils/test_tenant_shim.go b/pkg/testutils/serverutils/test_tenant_shim.go index 7d367f7ff2de..d52dea126ac4 100644 --- a/pkg/testutils/serverutils/test_tenant_shim.go +++ b/pkg/testutils/serverutils/test_tenant_shim.go @@ -16,6 +16,7 @@ package serverutils import ( "context" "net/http" + "net/url" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/config" @@ -38,6 +39,32 @@ const ( MultiTenantSession ) +type TestURL struct { + *url.URL +} + +func NewTestURL(path string) TestURL { + u, err := url.Parse(path) + if err != nil { + panic(err) + } + return TestURL{u} +} + +// WithPath is a helper that allows the user of the `TestURL` to easily +// append paths to use for testing. Please be aware that your path will +// automatically be escaped for you, but if it includes any invalid hex +// escapes (eg: `%s`) it will fail silently and you'll get back a blank +// URL. +func (t *TestURL) WithPath(path string) *TestURL { + newPath, err := url.JoinPath(t.Path, path) + if err != nil { + panic(err) + } + t.Path = newPath + return t +} + // TestTenantInterface defines SQL-only tenant functionality that tests need; it // is implemented by server.Test{Tenant,Server}. Tests written against this // interface are effectively agnostic to the type of tenant (host or secondary) @@ -147,7 +174,7 @@ type TestTenantInterface interface { AmbientCtx() log.AmbientContext // AdminURL returns the URL for the admin UI. - AdminURL() string + AdminURL() *TestURL // GetUnauthenticatedHTTPClient returns an http client configured with the client TLS // config required by the TestServer's configuration. // Discourages implementer from using unauthenticated http connections diff --git a/pkg/testutils/testcluster/testcluster_test.go b/pkg/testutils/testcluster/testcluster_test.go index 3c303d894dda..44af28ef8634 100644 --- a/pkg/testutils/testcluster/testcluster_test.go +++ b/pkg/testutils/testcluster/testcluster_test.go @@ -202,7 +202,7 @@ func TestStopServer(t *testing.T) { if err != nil { t.Fatal(err) } - url := server1.AdminURL() + "/_status/metrics/local" + url := server1.AdminURL().WithPath("/_status/metrics/local").String() if err := httputil.GetJSON(httpClient1, url, &response); err != nil { t.Fatal(err) } @@ -264,7 +264,7 @@ func TestStopServer(t *testing.T) { if err != nil { t.Fatal(err) } - url = tc.Server(0).AdminURL() + "/_status/metrics/local" + url = tc.Server(0).AdminURL().WithPath("/_status/metrics/local").String() if err := httputil.GetJSON(httpClient1, url, &response); err != nil { t.Fatal(err) } From cc16c03c373bde2bee4757b39a1987e5ca189658 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Sun, 19 Mar 2023 13:06:48 -0400 Subject: [PATCH 2/2] server, tenant: gate process debugging behind capability Previously, all tenant servers were started with a debug server that granted process-manipulating power via pprof and vmodule HTTP endpoints. This implementation is fine when servers serve just 1 tenant in a given process; that tenant then legitimately has access to all process-level control. However, it becomes a problem in shared-process multitenancy, when the same process is shared by multiple tenants. In that case, it is undesirable for 1 tenant to have access to & control process properties, as it could influence the well-functioning of other tenants or potentially leak data across tenant boundaries. This commit gates access to the debug server behind a capability **only with shared process multitenancy**. Tenant servers running within their own process will contain a debug server with no capability gating since they own their process. The gating is implemented via a tenant authorizer function backed by the capability store that is injected into the debug server upon startup. Shared process tenants are provided this authorizer, while separate process tenants and the system tenant use the no-op authorizer. Epic: CRDB-12100 Resolves: #97946 Release note: None Co-authored-by: Raphael 'kena' Poss --- .../testdata/logic_test/tenant_capability | 15 ++ pkg/ccl/serverccl/server_sql_test.go | 133 ++++++++++++++++++ pkg/configprofiles/testdata/multitenant-app | 3 + .../testdata/multitenant-app-repl | 3 + pkg/configprofiles/testdata/multitenant-noapp | 2 + .../testdata/multitenant-noapp-repl | 2 + pkg/kv/kvserver/tenantrate/limiter_test.go | 16 +++ .../tenantcapabilities/capabilities.go | 6 + .../tenantcapabilities/id_string.go | 9 +- .../tenantcapabilities/interfaces.go | 4 + .../allow_everything.go | 7 + .../allow_nothing.go | 7 + .../authorizer.go | 29 ++++ .../testdata/authorizer_enabled | 4 +- .../tenantcapabilitiespb/capabilities.proto | 4 + pkg/multitenant/tenantcapabilities/values.go | 2 + pkg/rpc/auth_test.go | 6 + pkg/server/config.go | 5 + pkg/server/debug/BUILD.bazel | 1 + pkg/server/debug/server.go | 120 ++++++++++------ pkg/server/server.go | 2 + pkg/server/server_controller_new_server.go | 5 +- pkg/server/tenant.go | 7 + pkg/server/testserver.go | 2 + 24 files changed, 345 insertions(+), 49 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability index 790382695505..b4f9559a17bb 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability +++ b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability @@ -36,6 +36,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -61,6 +62,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -79,6 +81,7 @@ can_admin_scatter true can_admin_split false can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -104,6 +107,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -129,6 +133,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -154,6 +159,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info true can_view_tsdb_metrics false @@ -172,6 +178,7 @@ can_admin_scatter true can_admin_split false can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -190,6 +197,7 @@ can_admin_scatter true can_admin_split false can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -208,6 +216,7 @@ can_admin_scatter true can_admin_split false can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -227,6 +236,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit true can_check_consistency true +can_debug_process true can_use_nodelocal_storage true can_view_node_info true can_view_tsdb_metrics true @@ -268,6 +278,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -297,6 +308,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -331,6 +343,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -349,6 +362,7 @@ can_admin_scatter false can_admin_split false can_admin_unsplit false can_check_consistency false +can_debug_process false can_use_nodelocal_storage false can_view_node_info false can_view_tsdb_metrics false @@ -367,6 +381,7 @@ can_admin_scatter true can_admin_split true can_admin_unsplit true can_check_consistency true +can_debug_process true can_use_nodelocal_storage true can_view_node_info true can_view_tsdb_metrics true diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index ffa7aebd0246..3e9ef4a785df 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -13,6 +13,7 @@ import ( "errors" "fmt" "io" + "net/http" "strings" "testing" "time" @@ -20,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/ccl" "github.com/cockroachdb/cockroach/pkg/ccl/utilccl/licenseccl" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher/systemconfigwatchertest" @@ -194,6 +196,137 @@ func TestTenantHTTP(t *testing.T) { } +// TestTenantProcessDebugging verifies that in-process SQL tenant servers gate +// process debugging behind capabilities. +func TestTenantProcessDebugging(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + ctx := context.Background() + + tc := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{ServerArgs: base.TestServerArgs{ + DefaultTestTenant: base.TestTenantDisabled, + }}) + defer tc.Stopper().Stop(ctx) + db := tc.ServerConn(0) + defer db.Close() + + s := tc.Server(0) + + tenant, _, err := s.StartSharedProcessTenant(ctx, + base.TestSharedProcessTenantArgs{ + TenantID: serverutils.TestTenantID(), + TenantName: "processdebug", + }) + require.NoError(t, err) + defer tenant.Stopper().Stop(ctx) + + t.Run("system tenant pprof", func(t *testing.T) { + httpClient, err := s.GetAdminHTTPClient() + require.NoError(t, err) + defer httpClient.CloseIdleConnections() + + url := s.AdminURL().URL + url.Path = url.Path + "/debug/pprof/goroutine" + q := url.Query() + q.Add("debug", "2") + url.RawQuery = q.Encode() + + resp, err := httpClient.Get(url.String()) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Contains(t, string(body), "goroutine") + }) + + t.Run("pprof", func(t *testing.T) { + httpClient, err := tenant.GetAdminHTTPClient() + require.NoError(t, err) + defer httpClient.CloseIdleConnections() + + url := tenant.AdminURL().URL + url.Path = url.Path + "/debug/pprof/" + q := url.Query() + q.Add("debug", "2") + url.RawQuery = q.Encode() + + resp, err := httpClient.Get(url.String()) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Contains(t, string(body), "tenant does not have capability to debug the running process") + + _, err = db.Exec(`ALTER TENANT processdebug GRANT CAPABILITY can_debug_process=true`) + require.NoError(t, err) + + tc.WaitForTenantCapabilities(t, serverutils.TestTenantID(), map[tenantcapabilities.ID]string{ + tenantcapabilities.CanDebugProcess: "true", + }) + + resp, err = httpClient.Get(url.String()) + require.NoError(t, err) + defer resp.Body.Close() + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Contains(t, string(body), "goroutine") + + _, err = db.Exec(`ALTER TENANT processdebug REVOKE CAPABILITY can_debug_process`) + require.NoError(t, err) + + tc.WaitForTenantCapabilities(t, serverutils.TestTenantID(), map[tenantcapabilities.ID]string{ + tenantcapabilities.CanDebugProcess: "false", + }) + }) + + t.Run("vmodule", func(t *testing.T) { + httpClient, err := tenant.GetAdminHTTPClient() + require.NoError(t, err) + defer httpClient.CloseIdleConnections() + + url := tenant.AdminURL().URL + url.Path = url.Path + "/debug/vmodule" + q := url.Query() + q.Add("duration", "-1s") + q.Add("vmodule", "exec_log=3") + url.RawQuery = q.Encode() + + resp, err := httpClient.Get(url.String()) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Contains(t, string(body), "tenant does not have capability to debug the running process") + + _, err = db.Exec(`ALTER TENANT processdebug GRANT CAPABILITY can_debug_process=true`) + require.NoError(t, err) + + tc.WaitForTenantCapabilities(t, serverutils.TestTenantID(), map[tenantcapabilities.ID]string{ + tenantcapabilities.CanDebugProcess: "true", + }) + + resp, err = httpClient.Get(url.String()) + require.NoError(t, err) + defer resp.Body.Close() + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Contains(t, string(body), "previous vmodule configuration: \nnew vmodule configuration: exec_log=3\n") + + _, err = db.Exec(`ALTER TENANT processdebug REVOKE CAPABILITY can_debug_process`) + require.NoError(t, err) + + tc.WaitForTenantCapabilities(t, serverutils.TestTenantID(), map[tenantcapabilities.ID]string{ + tenantcapabilities.CanDebugProcess: "false", + }) + }) +} + func TestNonExistentTenant(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/configprofiles/testdata/multitenant-app b/pkg/configprofiles/testdata/multitenant-app index a4e71ecbb9fe..d2425716bcd5 100644 --- a/pkg/configprofiles/testdata/multitenant-app +++ b/pkg/configprofiles/testdata/multitenant-app @@ -58,6 +58,7 @@ SHOW TENANTS WITH CAPABILITIES 1 system ready shared can_admin_split true 1 system ready shared can_admin_unsplit true 1 system ready shared can_check_consistency true +1 system ready shared can_debug_process true 1 system ready shared can_use_nodelocal_storage true 1 system ready shared can_view_node_info true 1 system ready shared can_view_tsdb_metrics true @@ -68,6 +69,7 @@ SHOW TENANTS WITH CAPABILITIES 2 template ready none can_admin_split true 2 template ready none can_admin_unsplit true 2 template ready none can_check_consistency true +2 template ready none can_debug_process true 2 template ready none can_use_nodelocal_storage true 2 template ready none can_view_node_info true 2 template ready none can_view_tsdb_metrics true @@ -78,6 +80,7 @@ SHOW TENANTS WITH CAPABILITIES 3 application ready shared can_admin_split true 3 application ready shared can_admin_unsplit true 3 application ready shared can_check_consistency true +3 application ready shared can_debug_process true 3 application ready shared can_use_nodelocal_storage true 3 application ready shared can_view_node_info true 3 application ready shared can_view_tsdb_metrics true diff --git a/pkg/configprofiles/testdata/multitenant-app-repl b/pkg/configprofiles/testdata/multitenant-app-repl index ae5e43c6f68c..bbfef13442e1 100644 --- a/pkg/configprofiles/testdata/multitenant-app-repl +++ b/pkg/configprofiles/testdata/multitenant-app-repl @@ -58,6 +58,7 @@ SHOW TENANTS WITH CAPABILITIES 1 system ready shared can_admin_split true 1 system ready shared can_admin_unsplit true 1 system ready shared can_check_consistency true +1 system ready shared can_debug_process true 1 system ready shared can_use_nodelocal_storage true 1 system ready shared can_view_node_info true 1 system ready shared can_view_tsdb_metrics true @@ -68,6 +69,7 @@ SHOW TENANTS WITH CAPABILITIES 2 template ready none can_admin_split true 2 template ready none can_admin_unsplit true 2 template ready none can_check_consistency true +2 template ready none can_debug_process true 2 template ready none can_use_nodelocal_storage true 2 template ready none can_view_node_info true 2 template ready none can_view_tsdb_metrics true @@ -78,6 +80,7 @@ SHOW TENANTS WITH CAPABILITIES 3 application ready shared can_admin_split true 3 application ready shared can_admin_unsplit true 3 application ready shared can_check_consistency true +3 application ready shared can_debug_process true 3 application ready shared can_use_nodelocal_storage true 3 application ready shared can_view_node_info true 3 application ready shared can_view_tsdb_metrics true diff --git a/pkg/configprofiles/testdata/multitenant-noapp b/pkg/configprofiles/testdata/multitenant-noapp index dc207e1a0b3d..1785b6138a68 100644 --- a/pkg/configprofiles/testdata/multitenant-noapp +++ b/pkg/configprofiles/testdata/multitenant-noapp @@ -51,6 +51,7 @@ SHOW TENANTS WITH CAPABILITIES 1 system ready shared can_admin_split true 1 system ready shared can_admin_unsplit true 1 system ready shared can_check_consistency true +1 system ready shared can_debug_process true 1 system ready shared can_use_nodelocal_storage true 1 system ready shared can_view_node_info true 1 system ready shared can_view_tsdb_metrics true @@ -61,6 +62,7 @@ SHOW TENANTS WITH CAPABILITIES 2 template ready none can_admin_split true 2 template ready none can_admin_unsplit true 2 template ready none can_check_consistency true +2 template ready none can_debug_process true 2 template ready none can_use_nodelocal_storage true 2 template ready none can_view_node_info true 2 template ready none can_view_tsdb_metrics true diff --git a/pkg/configprofiles/testdata/multitenant-noapp-repl b/pkg/configprofiles/testdata/multitenant-noapp-repl index e5af19772c99..52456f3f6f57 100644 --- a/pkg/configprofiles/testdata/multitenant-noapp-repl +++ b/pkg/configprofiles/testdata/multitenant-noapp-repl @@ -51,6 +51,7 @@ SHOW TENANTS WITH CAPABILITIES 1 system ready shared can_admin_split true 1 system ready shared can_admin_unsplit true 1 system ready shared can_check_consistency true +1 system ready shared can_debug_process true 1 system ready shared can_use_nodelocal_storage true 1 system ready shared can_view_node_info true 1 system ready shared can_view_tsdb_metrics true @@ -61,6 +62,7 @@ SHOW TENANTS WITH CAPABILITIES 2 template ready none can_admin_split true 2 template ready none can_admin_unsplit true 2 template ready none can_check_consistency true +2 template ready none can_debug_process true 2 template ready none can_use_nodelocal_storage true 2 template ready none can_view_node_info true 2 template ready none can_view_tsdb_metrics true diff --git a/pkg/kv/kvserver/tenantrate/limiter_test.go b/pkg/kv/kvserver/tenantrate/limiter_test.go index 5efc33f62409..446fb1c17f36 100644 --- a/pkg/kv/kvserver/tenantrate/limiter_test.go +++ b/pkg/kv/kvserver/tenantrate/limiter_test.go @@ -667,6 +667,16 @@ func (ts *testState) HasCapabilityForBatch( func (ts *testState) BindReader(tenantcapabilities.Reader) {} +var _ tenantcapabilities.Authorizer = &testState{} + +func (ts *testState) HasProcessDebugCapability(ctx context.Context, tenID roachpb.TenantID) error { + if ts.capabilities[tenID].CanDebugProcess { + return nil + } else { + return errors.New("unauthorized") + } +} + func (ts *testState) HasNodeStatusCapability(_ context.Context, tenID roachpb.TenantID) error { if ts.capabilities[tenID].CanViewNodeInfo { return nil @@ -767,6 +777,8 @@ func parseStrings(t *testing.T, d *datadriven.TestData) []string { // is subject to rate limit checks. (For testing in this package.) type fakeAuthorizer struct{} +var _ tenantcapabilities.Authorizer = &fakeAuthorizer{} + func (fakeAuthorizer) HasNodeStatusCapability(_ context.Context, tenID roachpb.TenantID) error { return nil } @@ -787,3 +799,7 @@ func (fakeAuthorizer) HasCapabilityForBatch( return nil } func (fakeAuthorizer) BindReader(tenantcapabilities.Reader) {} + +func (fakeAuthorizer) HasProcessDebugCapability(ctx context.Context, tenID roachpb.TenantID) error { + return nil +} diff --git a/pkg/multitenant/tenantcapabilities/capabilities.go b/pkg/multitenant/tenantcapabilities/capabilities.go index e9869a79bb81..c6b59e442dd4 100644 --- a/pkg/multitenant/tenantcapabilities/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/capabilities.go @@ -84,6 +84,11 @@ const ( // span configs. TenantSpanConfigBounds // span_config_bounds + // CanDebugProcess describes the ability of a tenant to set vmodule on the + // process and run pprof profiles and tools. This can reveal information + // across tenant boundaries. + CanDebugProcess // can_debug_process + MaxCapabilityID ID = iota - 1 ) @@ -114,6 +119,7 @@ var capabilities = [MaxCapabilityID + 1]Capability{ CanViewTSDBMetrics: boolCapability(CanViewTSDBMetrics), ExemptFromRateLimiting: boolCapability(ExemptFromRateLimiting), TenantSpanConfigBounds: spanConfigBoundsCapability(TenantSpanConfigBounds), + CanDebugProcess: boolCapability(CanDebugProcess), } // EnableAll enables maximum access to services. diff --git a/pkg/multitenant/tenantcapabilities/id_string.go b/pkg/multitenant/tenantcapabilities/id_string.go index 4ba2401e8697..da79f2376809 100644 --- a/pkg/multitenant/tenantcapabilities/id_string.go +++ b/pkg/multitenant/tenantcapabilities/id_string.go @@ -18,7 +18,8 @@ func _() { _ = x[CanViewTSDBMetrics-8] _ = x[ExemptFromRateLimiting-9] _ = x[TenantSpanConfigBounds-10] - _ = x[MaxCapabilityID-10] + _ = x[CanDebugProcess-11] + _ = x[MaxCapabilityID-11] } func (i ID) String() string { @@ -43,6 +44,8 @@ func (i ID) String() string { return "exempt_from_rate_limiting" case TenantSpanConfigBounds: return "span_config_bounds" + case CanDebugProcess: + return "can_debug_process" default: return "ID(" + strconv.FormatInt(int64(i), 10) + ")" } @@ -59,7 +62,8 @@ var stringToCapabilityIDMap = map[string]ID{ "can_view_tsdb_metrics": 8, "exempt_from_rate_limiting": 9, "span_config_bounds": 10, - "MaxCapabilityID": 10, + "can_debug_process": 11, + "MaxCapabilityID": 11, } var IDs = []ID{ @@ -68,6 +72,7 @@ var IDs = []ID{ CanAdminSplit, CanAdminUnsplit, CanCheckConsistency, + CanDebugProcess, CanUseNodelocalStorage, CanViewNodeInfo, CanViewTSDBMetrics, diff --git a/pkg/multitenant/tenantcapabilities/interfaces.go b/pkg/multitenant/tenantcapabilities/interfaces.go index 1dd4dfb911a4..498c9f1aaa0d 100644 --- a/pkg/multitenant/tenantcapabilities/interfaces.go +++ b/pkg/multitenant/tenantcapabilities/interfaces.go @@ -70,6 +70,10 @@ type Authorizer interface { // IsExemptFromRateLimiting returns true of the tenant should // not be subject to rate limiting. IsExemptFromRateLimiting(ctx context.Context, tenID roachpb.TenantID) bool + + // HasProcessDebugCapability returns an error if a tenant, referenced by its ID, + // is not allowed to debug the running process. + HasProcessDebugCapability(ctx context.Context, tenID roachpb.TenantID) error } // Entry ties together a tenantID with its capabilities. diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_everything.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_everything.go index 1426b7c58516..e6bc27658a3a 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_everything.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_everything.go @@ -66,3 +66,10 @@ func (n *AllowEverythingAuthorizer) IsExemptFromRateLimiting( ) bool { return true } + +// HasProcessDebugCapability implements the tenantcapabilities.Authorizer interface. +func (n *AllowEverythingAuthorizer) HasProcessDebugCapability( + ctx context.Context, tenID roachpb.TenantID, +) error { + return nil +} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_nothing.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_nothing.go index f759f6ac0e76..02eb5a0597aa 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_nothing.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/allow_nothing.go @@ -65,3 +65,10 @@ func (n *AllowNothingAuthorizer) HasNodelocalStorageCapability( func (n *AllowNothingAuthorizer) IsExemptFromRateLimiting(context.Context, roachpb.TenantID) bool { return false } + +// HasProcessDebugCapability implements the tenantcapabilities.Authorizer interface. +func (n *AllowNothingAuthorizer) HasProcessDebugCapability( + ctx context.Context, tenID roachpb.TenantID, +) error { + return errors.New("operation blocked") +} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go index 2c4ae8f1b034..9443b77fc2ea 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go @@ -390,3 +390,32 @@ func (a *Authorizer) getMode( } return cp, selectedMode } + +func (a *Authorizer) HasProcessDebugCapability(ctx context.Context, tenID roachpb.TenantID) error { + if tenID.IsSystem() { + return nil + } + errFn := func() error { + return errors.New("client tenant does not have capability to debug the process") + } + cp, mode := a.getMode(ctx, tenID) + switch mode { + case authorizerModeOn: + break // fallthrough to the next check. + case authorizerModeAllowAll: + return nil + case authorizerModeV222: + return errFn() + default: + err := errors.AssertionFailedf("unknown authorizer mode: %d", mode) + logcrash.ReportOrPanic(ctx, &a.settings.SV, "%v", err) + return err + } + + if !tenantcapabilities.MustGetBoolByID( + cp, tenantcapabilities.CanDebugProcess, + ) { + return errFn() + } + return nil +} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled index b4e244d89491..0f345632857c 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled @@ -8,7 +8,7 @@ client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRe has-capability-for-batch ten=10 cmds=(Merge) ---- -client tenant does not have capability "ID(12)" (*kvpb.MergeRequest) +client tenant does not have capability "ID(13)" (*kvpb.MergeRequest) has-node-status-capability ten=10 ---- @@ -34,7 +34,7 @@ ok has-capability-for-batch ten=10 cmds=(Merge) ---- -client tenant does not have capability "ID(12)" (*kvpb.MergeRequest) +client tenant does not have capability "ID(13)" (*kvpb.MergeRequest) has-node-status-capability ten=10 ---- diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto index 398c2c08458b..173d0dfda0e2 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto @@ -92,6 +92,10 @@ message TenantCapabilities { // CanCheckConsistency if set to true, grants the tenant the ability to run // range consistency checks. bool can_check_consistency = 10; + + // CanDebugProcess, if set to true, grants the tenant the ability to + // set vmodule on the process and run pprof profiles and tools. + bool can_debug_process = 11; }; // SpanConfigBound is used to constrain the possible values a SpanConfig may diff --git a/pkg/multitenant/tenantcapabilities/values.go b/pkg/multitenant/tenantcapabilities/values.go index 9f9d93f3af70..50dc8daab735 100644 --- a/pkg/multitenant/tenantcapabilities/values.go +++ b/pkg/multitenant/tenantcapabilities/values.go @@ -56,6 +56,8 @@ func GetValueByID(t *tenantcapabilitiespb.TenantCapabilities, id ID) (Value, err return (*boolValue)(&t.ExemptFromRateLimiting), nil case TenantSpanConfigBounds: return &spanConfigBoundsValue{b: &t.SpanConfigBounds}, nil + case CanDebugProcess: + return (*boolValue)(&t.CanDebugProcess), nil default: return nil, errors.AssertionFailedf("unknown capability: %q", id.String()) } diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index 7e7219bc81a5..b0a5d8f6cfbc 100644 --- a/pkg/rpc/auth_test.go +++ b/pkg/rpc/auth_test.go @@ -954,6 +954,12 @@ type mockAuthorizer struct { hasExemptFromRateLimiterCapability bool } +func (m mockAuthorizer) HasProcessDebugCapability( + ctx context.Context, tenID roachpb.TenantID, +) error { + return errors.New("tenant does not have capability") +} + var _ tenantcapabilities.Authorizer = &mockAuthorizer{} // HasCapabilityForBatch implements the tenantcapabilities.Authorizer interface. diff --git a/pkg/server/config.go b/pkg/server/config.go index 096369fac8e2..0a2993eed8ed 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -30,6 +30,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/server/autoconfig/acprovider" @@ -536,6 +537,10 @@ type LocalKVServerInfo struct { InternalServer kvpb.InternalServer ServerInterceptors rpc.ServerInterceptorInfo Tracer *tracing.Tracer + + // SameProcessCapabilityAuthorizer is the tenant capability authorizer to + // use for servers running in the same process as the KV node. + SameProcessCapabilityAuthorizer tenantcapabilities.Authorizer } // MakeSQLConfig returns a SQLConfig with default values. diff --git a/pkg/server/debug/BUILD.bazel b/pkg/server/debug/BUILD.bazel index 90bc9c8c4609..3c40fcc2a6ea 100644 --- a/pkg/server/debug/BUILD.bazel +++ b/pkg/server/debug/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//pkg/kv/kvserver", "//pkg/kv/kvserver/closedts/sidetransport", "//pkg/kv/kvserver/kvstorage", + "//pkg/multitenant/tenantcapabilities", "//pkg/roachpb", "//pkg/server/debug/goroutineui", "//pkg/server/debug/pprofui", diff --git a/pkg/server/debug/server.go b/pkg/server/debug/server.go index 284b6cf3f8b5..f4caee78e89c 100644 --- a/pkg/server/debug/server.go +++ b/pkg/server/debug/server.go @@ -24,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/closedts/sidetransport" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/debug/goroutineui" "github.com/cockroachdb/cockroach/pkg/server/debug/pprofui" @@ -72,41 +73,98 @@ type Server struct { type serverTickleFn = func(ctx context.Context, name roachpb.TenantName) error -// NewServer sets up a debug server. -func NewServer( - ambientContext log.AmbientContext, +func setupProcessWideRoutes( + mux *http.ServeMux, st *cluster.Settings, - hbaConfDebugFn http.HandlerFunc, + tenantID roachpb.TenantID, + authorizer tenantcapabilities.Authorizer, + vsrv *vmoduleServer, profiler pprofui.Profiler, - serverTickleFn serverTickleFn, -) *Server { - mux := http.NewServeMux() +) { + authzCheck := func(w http.ResponseWriter, r *http.Request) bool { + if err := authorizer.HasProcessDebugCapability(r.Context(), tenantID); err != nil { + http.Error(w, "tenant does not have capability to debug the running process", http.StatusForbidden) + return false + } + return true + } - // Install a redirect to the UI's collection of debug tools. - mux.HandleFunc(Endpoint, handleLanding) + authzFunc := func(origHandler http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if authzCheck(w, r) { + origHandler(w, r) + } + } + } // Cribbed straight from pprof's `init()` method. See: // https://golang.org/src/net/http/pprof/pprof.go - mux.HandleFunc("/debug/pprof/", pprof.Index) - mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) - mux.HandleFunc("/debug/pprof/profile", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/debug/pprof/", authzFunc(pprof.Index)) + mux.HandleFunc("/debug/pprof/cmdline", authzFunc(pprof.Cmdline)) + mux.HandleFunc("/debug/pprof/profile", authzFunc(func(w http.ResponseWriter, r *http.Request) { CPUProfileHandler(st, w, r) - }) - mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) - mux.HandleFunc("/debug/pprof/trace", pprof.Trace) + })) + mux.HandleFunc("/debug/pprof/symbol", authzFunc(pprof.Symbol)) + mux.HandleFunc("/debug/pprof/trace", authzFunc(pprof.Trace)) // Cribbed straight from trace's `init()` method. See: // https://github.com/golang/net/blob/master/trace/trace.go - mux.HandleFunc("/debug/requests", trace.Traces) - mux.HandleFunc("/debug/events", trace.Events) + mux.HandleFunc("/debug/requests", authzFunc(trace.Traces)) + mux.HandleFunc("/debug/events", authzFunc(trace.Events)) // This registers a superset of the variables exposed through the // /debug/vars endpoint onto the /debug/metrics endpoint. It includes all // expvars registered globally and all metrics registered on the // DefaultRegistry. - mux.Handle("/debug/metrics", exp.ExpHandler(metrics.DefaultRegistry)) + mux.HandleFunc("/debug/metrics", authzFunc(exp.ExpHandler(metrics.DefaultRegistry).ServeHTTP)) // Also register /debug/vars (even though /debug/metrics is better). - mux.Handle("/debug/vars", expvar.Handler()) + mux.HandleFunc("/debug/vars", authzFunc(expvar.Handler().ServeHTTP)) + + // Register the stopper endpoint, which lists all active tasks. + mux.HandleFunc("/debug/stopper", authzFunc(stop.HandleDebug)) + + // Set up the vmodule endpoint. + mux.HandleFunc("/debug/vmodule", authzFunc(vsrv.vmoduleHandleDebug)) + + ps := pprofui.NewServer(pprofui.NewMemStorage(pprofui.ProfileConcurrency, pprofui.ProfileExpiry), profiler) + mux.Handle("/debug/pprof/ui/", authzFunc(func(w http.ResponseWriter, r *http.Request) { + http.StripPrefix("/debug/pprof/ui", ps).ServeHTTP(w, r) + })) + + mux.HandleFunc("/debug/pprof/goroutineui/", authzFunc(func(w http.ResponseWriter, req *http.Request) { + dump := goroutineui.NewDump() + + _ = req.ParseForm() + switch req.Form.Get("sort") { + case "count": + dump.SortCountDesc() + case "wait": + dump.SortWaitDesc() + default: + } + _ = dump.HTML(w) + })) + +} + +// NewServer sets up a debug server. +func NewServer( + ambientContext log.AmbientContext, + st *cluster.Settings, + hbaConfDebugFn http.HandlerFunc, + profiler pprofui.Profiler, + serverTickleFn serverTickleFn, + tenantID roachpb.TenantID, + authorizer tenantcapabilities.Authorizer, +) *Server { + mux := http.NewServeMux() + + // Install a redirect to the UI's collection of debug tools. + mux.HandleFunc(Endpoint, handleLanding) + + // Debug routes that retrieve process-wide state. + vsrv := &vmoduleServer{} + setupProcessWideRoutes(mux, st, tenantID, authorizer, vsrv, profiler) if hbaConfDebugFn != nil { // Expose the processed HBA configuration through the debug @@ -114,9 +172,6 @@ func NewServer( mux.HandleFunc("/debug/hba_conf", hbaConfDebugFn) } - // Register the stopper endpoint, which lists all active tasks. - mux.HandleFunc("/debug/stopper", stop.HandleDebug) - if serverTickleFn != nil { // Register the server tickling function. // @@ -126,10 +181,6 @@ func NewServer( mux.Handle("/debug/tickle", handleTickle(serverTickleFn)) } - // Set up the vmodule endpoint. - vsrv := &vmoduleServer{} - mux.HandleFunc("/debug/vmodule", vsrv.vmoduleHandleDebug) - // Set up the log spy, a tool that allows inspecting filtered logs at high // verbosity. We require the tenant ID from the ambientCtx to set the logSpy // tenant filter. @@ -149,23 +200,6 @@ func NewServer( mux.HandleFunc("/debug/logspy", spy.handleDebugLogSpy) - ps := pprofui.NewServer(pprofui.NewMemStorage(pprofui.ProfileConcurrency, pprofui.ProfileExpiry), profiler) - mux.Handle("/debug/pprof/ui/", http.StripPrefix("/debug/pprof/ui", ps)) - - mux.HandleFunc("/debug/pprof/goroutineui/", func(w http.ResponseWriter, req *http.Request) { - dump := goroutineui.NewDump() - - _ = req.ParseForm() - switch req.Form.Get("sort") { - case "count": - dump.SortCountDesc() - case "wait": - dump.SortWaitDesc() - default: - } - _ = dump.HTML(w) - }) - return &Server{ ambientCtx: ambientContext, st: st, diff --git a/pkg/server/server.go b/pkg/server/server.go index 4d939a804d06..2e230eacb4e2 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -1242,6 +1242,8 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) { } return errors.Newf("server found with type %T", d) }, + roachpb.SystemTenantID, + authorizer, ) recoveryServer := loqrecovery.NewServer( diff --git a/pkg/server/server_controller_new_server.go b/pkg/server/server_controller_new_server.go index e00c64cbae89..7c712a6cddfd 100644 --- a/pkg/server/server_controller_new_server.go +++ b/pkg/server/server_controller_new_server.go @@ -155,8 +155,9 @@ func (s *Server) makeSharedProcessTenantConfig( // Create a configuration for the new tenant. parentCfg := s.cfg localServerInfo := LocalKVServerInfo{ - InternalServer: s.node, - ServerInterceptors: s.grpc.serverInterceptorsInfo, + InternalServer: s.node, + ServerInterceptors: s.grpc.serverInterceptorsInfo, + SameProcessCapabilityAuthorizer: s.rpcContext.TenantRPCAuthorizer, } baseCfg, sqlCfg, err := makeSharedProcessTenantServerConfig(ctx, tenantID, index, parentCfg, localServerInfo, stopper, s.recorder) if err != nil { diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go index 2eff7dd1a8da..a42a1dca5a7e 100644 --- a/pkg/server/tenant.go +++ b/pkg/server/tenant.go @@ -411,6 +411,11 @@ func newTenantServer( sStatus.baseStatusServer.sqlServer = sqlServer sAdmin.sqlServer = sqlServer + var processCapAuthz tenantcapabilities.Authorizer = &tenantcapabilitiesauthorizer.AllowEverythingAuthorizer{} + if lsi := sqlCfg.LocalKVServerInfo; lsi != nil { + processCapAuthz = lsi.SameProcessCapabilityAuthorizer + } + // Create the debug API server. debugServer := debug.NewServer( baseCfg.AmbientCtx, @@ -418,6 +423,8 @@ func newTenantServer( sqlServer.pgServer.HBADebugFn(), sqlServer.execCfg.SQLStatusServer, nil, /* serverTickleFn */ + sqlCfg.TenantID, + processCapAuthz, ) return &SQLServerWrapper{ diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index 80ec0922cafd..8c22328fbb75 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -921,6 +921,8 @@ func (ts *TestServer) StartSharedProcessTenant( hts := &httpTestServer{} hts.t.authentication = sqlServerWrapper.authentication hts.t.sqlServer = sqlServer + hts.t.tenantName = args.TenantName + testTenant := &TestTenant{ SQLServer: sqlServer, Cfg: sqlServer.cfg,