Merge #154265 #154267

154265: kvserver: more logging for TestReplicaCircuitBreaker_Partial_Retry r=tbg a=miraradeva

This test verifies the value of the `InLeaseTransferBackoffs` metric, but without more verbose `DistSender` logging, it's hard to investigate failures where the metric has unexpected values.

Fixes: #154179

Release note: None

154267: sql,unsafesql,sessiondata: do not log unsafe access logs for internal app names r=kyle-a-wong a=kyle-a-wong

If the app name of a session is an internal app name, we won't do any checks
on internals access.  An app name is considered to be an internal app name
if it begins with "$ internal"

Resolves: #153803
Epic: None
Release note: None

Co-authored-by: Mira Radeva <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>

Author: 3 people

pkg/kv/kvserver/client_replica_circuit_breaker_test.go

@@ -793,6 +793,11 @@ func TestReplicaCircuitBreaker_Partial_Retry(t *testing.T) {
 	skip.UnderRace(t)
 	skip.UnderDeadlock(t)
 
+	// To help debug issues like #154179.
+	prevVModule := log.GetVModule()
+	defer func() { _ = log.SetVModule(prevVModule) }()
+	require.NoError(t, log.SetVModule("dist_sender=3"))
+
 	testutils.RunValues(t, "lease-type", roachpb.ExpirationAndLeaderLeaseType(),
 		func(t *testing.T, leaseType roachpb.LeaseType) {
 			// Use a context timeout, to prevent test hangs on failures.

pkg/sql/conn_executor.go

@@ -920,7 +920,7 @@ func (s *Server) SetupConn(
 	// to use the InternalMetrics. However, some external connections use the prefix as well, for example
 	// the debug zip cli tool.
 	metrics := &s.Metrics
-	if strings.HasPrefix(sd.ApplicationName, catconstants.InternalAppNamePrefix) {
+	if sd.IsInternalAppName() {
 		metrics = &s.InternalMetrics
 	}
 
@@ -1255,7 +1255,6 @@ func (s *Server) newConnExecutor(
 	}
 
 	ex.applicationName.Store(ex.sessionData().ApplicationName)
-
 	ex.applicationStats = applicationStats
 	// We ignore statements and transactions run by the internal executor by
 	// passing a nil writer.

pkg/sql/sessiondata/session_data.go

@@ -9,9 +9,11 @@ import (
 	"net"
 	"reflect"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -154,6 +156,10 @@ func (s *SessionData) SessionUser() username.SQLUsername {
 	return s.SessionUserProto.Decode()
 }
 
+func (s *SessionData) IsInternalAppName() bool {
+	return strings.HasPrefix(s.ApplicationName, catconstants.InternalAppNamePrefix)
+}
+
 // LocalUnmigratableSessionData contains session parameters that cannot
 // be propagated to remote nodes and cannot be migrated to another
 // session.

pkg/sql/unsafesql/BUILD.bazel

@@ -29,6 +29,7 @@ go_test(
         "//pkg/sql/isql",
         "//pkg/sql/sqlerrors",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/sqlutils",
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/log/eventpb",

pkg/sql/unsafesql/unsafesql.go

@@ -49,7 +49,7 @@ func CheckInternalsAccess(
 	sv *settings.Values,
 ) error {
 	// If the querier is internal, we should allow it.
-	if sd.Internal {
+	if sd.Internal || sd.IsInternalAppName() {
 		return nil
 	}
 

pkg/sql/unsafesql/unsafesql_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/unsafesql"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
@@ -67,12 +68,12 @@ func TestAccessCheckServer(t *testing.T) {
 	pool := s.SQLConn(t)
 	defer pool.Close()
 
+	runner := sqlutils.MakeSQLRunner(pool)
 	// override the log limiter so that the tests can run without pauses.
 	defer unsafesql.TestRemoveLimiters()()
 
 	// create a user table to test user table access.
-	_, err := pool.Exec("CREATE TABLE foo (id INT PRIMARY KEY)")
-	require.NoError(t, err)
+	runner.Exec(t, "CREATE TABLE foo (id INT PRIMARY KEY)")
 
 	// create and register a log accessedSpy to see the unsafe access logs
 	accessedSpy := logtestutils.NewStructuredLogSpy[eventpb.UnsafeInternalsAccessed](
@@ -119,6 +120,7 @@ func TestAccessCheckServer(t *testing.T) {
 
 	for _, test := range []struct {
 		Query                string
+		AppName              string
 		Internal             bool
 		AllowUnsafeInternals bool
 		Passes               bool
@@ -157,6 +159,14 @@ func TestAccessCheckServer(t *testing.T) {
 			Passes:               true,
 			LogsAccessed:         true,
 		},
+		{
+			Query:                "SELECT * FROM system.namespace",
+			AppName:              "$ internal app",
+			Internal:             false,
+			AllowUnsafeInternals: false,
+			Passes:               true,
+			LogsAccessed:         false,
+		},
 		// Tests on unsupported crdb_internal objects.
 		{
 			Query:                "SELECT * FROM crdb_internal.gossip_alerts",
@@ -223,6 +233,7 @@ func TestAccessCheckServer(t *testing.T) {
 		t.Run(fmt.Sprintf("query=%s,internal=%t,allowUnsafe=%t", test.Query, test.Internal, test.AllowUnsafeInternals), func(t *testing.T) {
 			accessedSpy.Reset()
 			deniedSpy.Reset()
+			runner.Exec(t, "SET application_name = $1", test.AppName)
 			err := sendQuery(test.AllowUnsafeInternals, test.Internal, test.Query)
 			if test.Passes {
 				require.NoError(t, err)