From e496d934a83caff3653765194de99283016268e5 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Thu, 9 Feb 2023 11:11:33 -0500 Subject: [PATCH 1/5] servercontroller: serve http from default tenant on error Previously, when an HTTP request arrived with a tenant cookie attached, we would attempt to connect to that tenant and fail if the tenant couldn't be started or if it didn't exist. This causes a bad user-facing experience when someone's browser contains stale tenant cookies and they attempt to load DB Console. The error returns a 500 to the browser and user sees no UI, can't try and login again, and is basically stuck until they clear their cookies. This change falls back to serving HTTP from the default tenant when the requested one can't be reached. This ensures that *something* is served to ensure that unauthenticated endpoints can still be loaded in these scenarios and users can attempt to login again. Epic: CRDB-12100 Release note: None --- pkg/ccl/serverccl/server_controller_test.go | 42 +++++++++++++++++++++ pkg/server/server_controller_http.go | 16 +++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/pkg/ccl/serverccl/server_controller_test.go b/pkg/ccl/serverccl/server_controller_test.go index 92d1b204bc72..9599e7b5d650 100644 --- a/pkg/ccl/serverccl/server_controller_test.go +++ b/pkg/ccl/serverccl/server_controller_test.go @@ -181,6 +181,48 @@ VALUES($1, $2, $3, $4, $5)`, id, secret, username, created, expires) require.Equal(t, body.Sessions[0].ApplicationName, "hello system") } +// TestServerControllerBadHTTPCookies tests the controller's proxy +// layer for correct behavior under scenarios where the client has +// stale or invalid cookies. This helps ensure that we continue to +// serve static assets even when the browser is referencing an +// unknown tenant, or bad sessions. +func TestServerControllerBadHTTPCookies(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + + client, err := s.GetUnauthenticatedHTTPClient() + require.NoError(t, err) + + c := &http.Cookie{ + Name: server.TenantSelectCookieName, + Value: "some-nonexistent-tenant", + Path: "/", + HttpOnly: true, + Secure: true, + } + + req, err := http.NewRequest("GET", s.AdminURL()+"/", 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) + 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) +} + func TestServerStartStop(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/server/server_controller_http.go b/pkg/server/server_controller_http.go index 2a8b7be4ddd6..3eec8dcbe06e 100644 --- a/pkg/server/server_controller_http.go +++ b/pkg/server/server_controller_http.go @@ -64,7 +64,7 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) { s, err := c.getServer(ctx, tenantName) if err != nil { log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err) - // Clear session and tenant cookies after all logouts have completed. + // Clear session and tenant cookies since it appears they reference invalid state. http.SetCookie(w, &http.Cookie{ Name: MultitenantSessionCookieName, Value: "", @@ -79,7 +79,19 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) { HttpOnly: false, Expires: timeutil.Unix(0, 0), }) - w.WriteHeader(http.StatusInternalServerError) + // Fall back to serving requests from the default tenant. This helps us serve + // the root path along with static assets even when the browser contains invalid + // tenant names or sessions (common during development). Otherwise the user can + // get into a state where they cannot load DB Console assets at all due to invalid + // cookies. + defaultTenantName := roachpb.TenantName(defaultTenantSelect.Get(&c.st.SV)) + s, err = c.getServer(ctx, defaultTenantName) + if err != nil { + log.Warningf(ctx, "unable to find server for default tenant %q: %v", defaultTenantName, err) + w.WriteHeader(http.StatusInternalServerError) + return + } + s.getHTTPHandlerFn()(w, r) return } s.getHTTPHandlerFn()(w, r) From 8cc347bd7546cd7a9f32061e1d3f5bb3c256c02d Mon Sep 17 00:00:00 2001 From: Nick Travers Date: Fri, 10 Feb 2023 09:32:36 -0800 Subject: [PATCH 2/5] roachtest: set range tombstones flag accordingly Fix a bug in the `import-cancellation` roachtest whereby range tombstones are always disabled. Release note: None. --- pkg/cmd/roachtest/tests/import_cancellation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/roachtest/tests/import_cancellation.go b/pkg/cmd/roachtest/tests/import_cancellation.go index 5ac8f3596d3c..f854b144a83b 100644 --- a/pkg/cmd/roachtest/tests/import_cancellation.go +++ b/pkg/cmd/roachtest/tests/import_cancellation.go @@ -36,7 +36,7 @@ func registerImportCancellation(r registry.Registry) { Timeout: 4 * time.Hour, Cluster: r.MakeClusterSpec(6, spec.CPU(32)), Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { - runImportCancellation(ctx, t, c, false) + runImportCancellation(ctx, t, c, rangeTombstones) }, }) } From f5551963e750f8b8aae2353da67af84fa54626bf Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Fri, 10 Feb 2023 11:34:26 -0500 Subject: [PATCH 3/5] sql: fix SHOW GRANTS FOR public Release note (bug fix): Fixed the `SHOW GRANTS FOR public` command so it works correctly. Previously, this would return an error saying that the `public` role does not exist. --- pkg/sql/delegate/show_grants.go | 13 ++++++++++--- .../logic_test/grant_revoke_with_grant_option | 13 +++++++++++++ pkg/sql/logictest/testdata/logic_test/grant_role | 9 +++++++++ pkg/sql/logictest/testdata/logic_test/set_role | 6 +++--- pkg/sql/user.go | 7 ++----- 5 files changed, 37 insertions(+), 11 deletions(-) diff --git a/pkg/sql/delegate/show_grants.go b/pkg/sql/delegate/show_grants.go index 9d34bd9b4acf..a292f8d5944b 100644 --- a/pkg/sql/delegate/show_grants.go +++ b/pkg/sql/delegate/show_grants.go @@ -377,9 +377,16 @@ SELECT database_name, if err != nil { return nil, err } - userExists, err := d.catalog.RoleExists(d.ctx, user) - if err != nil { - return nil, err + // The `public` role is a pseudo-role, so we check it separately. RoleExists + // should not return true for `public` since other operations like GRANT and + // REVOKE should fail with a "role `public` does not exist" error if they + // are used with `public`. + userExists := user.IsPublicRole() + if !userExists { + userExists, err = d.catalog.RoleExists(d.ctx, user) + if err != nil { + return nil, err + } } if !userExists { return nil, sqlerrors.NewUndefinedUserError(user) diff --git a/pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option b/pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option index 78b4b7b35bde..3021c7945c85 100644 --- a/pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option +++ b/pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option @@ -7,6 +7,19 @@ CREATE USER testuser2 statement ok CREATE USER target +# Check privileges that `public` has by default, ignoring types and virtual tables. +query TTTTTB colnames +SELECT * FROM [SHOW GRANTS FOR public] WHERE relation_name IS NULL +---- +database_name schema_name relation_name grantee privilege_type is_grantable +test crdb_internal NULL public USAGE false +test information_schema NULL public USAGE false +test pg_catalog NULL public USAGE false +test pg_extension NULL public USAGE false +test public NULL public CREATE false +test public NULL public USAGE false +test NULL NULL public CONNECT false + statement error grant options cannot be granted to "public" role GRANT ALL PRIVILEGES ON TABLE t TO public WITH GRANT OPTION diff --git a/pkg/sql/logictest/testdata/logic_test/grant_role b/pkg/sql/logictest/testdata/logic_test/grant_role index a7516066c3f6..47b70bec8701 100644 --- a/pkg/sql/logictest/testdata/logic_test/grant_role +++ b/pkg/sql/logictest/testdata/logic_test/grant_role @@ -23,3 +23,12 @@ query B SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor)->'table'->>'version' = $role_members_version::STRING FROM system.descriptor WHERE id = 'system.public.role_members'::REGCLASS ---- true + +# GRANT or REVOKE on the public role should result in "not exists" +subtest grant_revoke_public + +statement error pgcode 42704 role/user \"public\" does not exist +GRANT testuser TO public + +statement error pgcode 42704 role/user \"public\" does not exist +REVOKE testuser FROM public diff --git a/pkg/sql/logictest/testdata/logic_test/set_role b/pkg/sql/logictest/testdata/logic_test/set_role index 28c60c2d859e..a72ac401a27f 100644 --- a/pkg/sql/logictest/testdata/logic_test/set_role +++ b/pkg/sql/logictest/testdata/logic_test/set_role @@ -15,13 +15,13 @@ GRANT ALL ON TABLE testuser.testuser_table TO testuser statement error role name "public" is reserved CREATE ROLE public -statement error pgcode 22023 role public does not exist +statement error pgcode 42704 role/user \"public\" does not exist SET ROLE public statement error role name "node" is reserved CREATE ROLE node -statement error pgcode 22023 role node does not exist +statement error pgcode 42704 role/user \"node\" does not exist SET ROLE node # Check root can reset and become itself. @@ -38,7 +38,7 @@ none statement ok RESET ROLE -statement error pgcode 22023 role non_existent_user does not exist +statement error pgcode 42704 role/user \"non_existent_user\" does not exist SET ROLE non_existent_user query TTTT diff --git a/pkg/sql/user.go b/pkg/sql/user.go index ca36a4b14d32..94ffe83c50cc 100644 --- a/pkg/sql/user.go +++ b/pkg/sql/user.go @@ -28,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessioninit" + "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util/contextutil" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -545,11 +546,7 @@ func (p *planner) setRole(ctx context.Context, local bool, s username.SQLUsernam return err } if !exists { - return pgerror.Newf( - pgcode.InvalidParameterValue, - "role %s does not exist", - becomeUser.Normalized(), - ) + return sqlerrors.NewUndefinedUserError(becomeUser) } } From 4c2d4c39b71157799e464778db9acac931e92d49 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Fri, 10 Feb 2023 14:42:04 +0000 Subject: [PATCH 4/5] kvserver: fix rebalance obj test on arm mac Previosly,`TestRebalanceObjectiveManager` and `TestLoadBasedRebalancingObjective` would assert assuming that it was possible for the test host to use `grunning`, however this is not true for ARM Mac. This patch ammends these tests to test a subset of behavior when `grunning` isn't supported and then exit. Fixes: #96934 Release note: None --- pkg/kv/kvserver/BUILD.bazel | 1 + pkg/kv/kvserver/rebalance_objective_test.go | 44 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index 52badce4e5ce..27bcb9e855b3 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/pkg/kv/kvserver/BUILD.bazel @@ -430,6 +430,7 @@ go_test( "//pkg/util/ctxgroup", "//pkg/util/encoding", "//pkg/util/errorutil", + "//pkg/util/grunning", "//pkg/util/hlc", "//pkg/util/humanizeutil", "//pkg/util/leaktest", diff --git a/pkg/kv/kvserver/rebalance_objective_test.go b/pkg/kv/kvserver/rebalance_objective_test.go index b16dc9c1480a..5b3fe8b01da7 100644 --- a/pkg/kv/kvserver/rebalance_objective_test.go +++ b/pkg/kv/kvserver/rebalance_objective_test.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/storepool" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/util/grunning" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/stretchr/testify/require" @@ -82,6 +83,29 @@ func TestLoadBasedRebalancingObjective(t *testing.T) { defer log.Scope(t).Close(t) ctx := context.Background() + // On ARM MacOS and other architectures, grunning isn't supported so + // changing the objective to CPU should never work. If this test is run on + // one of these unsupported aarch, test this behavior only. + if !grunning.Supported() { + st := cluster.MakeTestingClusterSettings() + + gossipStoreDescProvider := testMakeProviderNotifier(allPositiveCPUMap) + LoadBasedRebalancingObjective.Override(ctx, &st.SV, int64(LBRebalancingQueries)) + require.Equal(t, + LBRebalancingQueries, + ResolveLBRebalancingObjective(ctx, st, gossipStoreDescProvider.GetStores()), + ) + + // Despite setting to CPU, only QPS should be returned since this aarch + // doesn't support grunning. + LoadBasedRebalancingObjective.Override(ctx, &st.SV, int64(LBRebalancingCPU)) + require.Equal(t, + LBRebalancingQueries, + ResolveLBRebalancingObjective(ctx, st, gossipStoreDescProvider.GetStores()), + ) + return + } + t.Run("latest version supports all rebalance objectives", func(t *testing.T) { st := cluster.MakeTestingClusterSettings() gossipStoreDescProvider := testMakeProviderNotifier(allPositiveCPUMap) @@ -168,6 +192,26 @@ func TestRebalanceObjectiveManager(t *testing.T) { ), &callbacks } + // On ARM MacOS and other architectures, grunning isn't supported so + // changing the objective to CPU should never work. If this test is run on + // one of these unsupported aarch, test this behavior only. + if !grunning.Supported() { + st := cluster.MakeTestingClusterSettings() + LoadBasedRebalancingObjective.Override(ctx, &st.SV, int64(LBRebalancingQueries)) + providerNotifier := testMakeProviderNotifier(allPositiveCPUMap) + manager, callbacks := makeTestManager(st, providerNotifier) + + require.Equal(t, LBRebalancingQueries, manager.Objective()) + + // Changing the objective to CPU should not work since it isn't + // supported on this aarch. + LoadBasedRebalancingObjective.Override(ctx, &st.SV, int64(LBRebalancingCPU)) + require.Equal(t, LBRebalancingCPU, manager.Objective()) + require.Len(t, *callbacks, 0) + + return + } + t.Run("latest version", func(t *testing.T) { st := cluster.MakeTestingClusterSettings() LoadBasedRebalancingObjective.Override(ctx, &st.SV, int64(LBRebalancingCPU)) From f9cf8055677cffb9045a73a88196e6df944a4f05 Mon Sep 17 00:00:00 2001 From: Michael Butler Date: Fri, 10 Feb 2023 16:30:16 -0500 Subject: [PATCH 5/5] roachtest: disable scheduled backup in rust-postgres node restart Fixes #96799 Release note: None --- pkg/cmd/roachtest/tests/rust_postgres.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/roachtest/tests/rust_postgres.go b/pkg/cmd/roachtest/tests/rust_postgres.go index db23d83e8d06..41bf8f205990 100644 --- a/pkg/cmd/roachtest/tests/rust_postgres.go +++ b/pkg/cmd/roachtest/tests/rust_postgres.go @@ -126,7 +126,10 @@ func registerRustPostgres(r registry.Registry) { // We stop the cluster and restart with a port of 5433 since Rust postgres // has all of it's test hardcoded to use that port. c.Stop(ctx, t.L(), option.DefaultStopOpts(), c.All()) - startOpts := option.DefaultStartOpts() + + // Don't restart the cluster with automatic scheduled backups because roachprod's internal sql + // interface, through which the scheduled backup executes, is naive to the port change. + startOpts := option.DefaultStartOptsNoBackups() startOpts.RoachprodOpts.ExtraArgs = []string{"--port=5433"} c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.All())