Merge #96881 #96947 #96957 #96964 #96985

96881: servercontroller: serve http from default tenant on error r=aadityasondhi a=dhartunian

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

96947: kvserver: fix rebalance obj test on arm mac r=pavelkalinnikov a=kvoli

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

96957: sql: fix SHOW GRANTS FOR public r=ecwall a=rafiss

fixes #96948

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.

96964: roachtest: set range tombstones flag accordingly r=jbowens,msbutler a=nicktrav

Fix a bug in the `import-cancellation` roachtest whereby range tombstones are always disabled.

Release note: None.

Epic: CRDB-20465

96985: roachtest: disable scheduled backup in rust-postgres node restart r=rafiss a=msbutler

Fixes #96799

Release note: None

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: Austen McClernon <austen@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Nick Travers <travers@cockroachlabs.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>

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)

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)
 			},
 		})
 	}

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())
 

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",

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))

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)

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)

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
 

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

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

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)
 		}
 	}