Merge cockroachdb#80707 cockroachdb#81580

80707: server: correctly redact sql constants in ListSessions api r=matthewtodd a=xinhaoz

Previously, there was a bug in the redaction of SQL constants
for `VIEWACTIVITYREDACTED` users in the `ListSessions` api.
Firstly, the range loop used to set the sql query field
was manipulating the copy of the query object provided
by the range. Secondly, the previous logic when redacting
set the `Sql` and `LastActiveQuery` fields to empty strings.
Since these fields are read in  other parts of the codebase
(e.g. to fill out columns of virtual tables), we should just
set these fields to equal `SqlNoConstants` and
`LastActiveQueryNoConstants` respectively.

Release note (bug fix): constants in sql query fields are
correctly removed for VIEWACTIVITYREDACTED users

81580: ci: update how the `tc_release_branch` check is performed r=rail a=rickystewart

How TeamCity sets the `TC_BUILD_BRANCH` environment variable is a bit
unpredictable. While we expect it to be a string like `master`, at times
(see cockroachdb#81546) this variable can be set to a ref like `refs/heads/master`.
This inconsistency means that we are sometimes not reporting issues to
GitHub as we should be. To be resilient to this I make sure we strip
off the `refs/heads/` prefix when performing the `tc_release_branch`
check.

Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

build/teamcity-bazel-support.sh

@@ -34,9 +34,15 @@ run_bazel() {
         $BAZEL_IMAGE "$@"
 }
 
+# local copy of _tc_build_branch from teamcity-support.sh to avoid imports.
+_tc_build_branch() {
+    echo "${TC_BUILD_BRANCH#refs/heads/}"
+}
+
 # local copy of tc_release_branch from teamcity-support.sh to avoid imports.
 _tc_release_branch() {
-  [[ "$TC_BUILD_BRANCH" == master || "$TC_BUILD_BRANCH" == release-* || "$TC_BUILD_BRANCH" == provisional_* ]]
+  branch=$(_tc_build_branch)
+  [[ "$branch" == master || "$branch" == release-* || "$branch" == provisional_* ]]
 }
 
 # process_test_json processes logs and submits failures to GitHub

build/teamcity-support.sh

@@ -282,12 +282,22 @@ changed_go_pkgs() {
     | { while read path; do if ls "$path"/*.go &>/dev/null; then echo -n "./$path "; fi; done; }
 }
 
+# tc_build_branch returns $TC_BUILD_BRANCH but with the optional refs/heads/
+# prefix stripped.
+tc_build_branch() {
+    echo "${TC_BUILD_BRANCH#refs/heads/}"
+}
+
+# NB: Update _tc_release_branch in teamcity-bazel-support.sh if you update this
+# function.
 tc_release_branch() {
-  [[ "$TC_BUILD_BRANCH" == master || "$TC_BUILD_BRANCH" == release-* || "$TC_BUILD_BRANCH" == provisional_* ]]
+  branch=$(tc_build_branch)
+  [[ "$branch" == master || "$branch" == release-* || "$branch" == provisional_* ]]
 }
 
 tc_bors_branch() {
-  [[ "$TC_BUILD_BRANCH" == staging ]]
+  branch=$(tc_build_branch)
+  [[ "$branch" == staging ]]
 }
 
 if_tc() {

build/teamcity/util/roachtest_util.sh

@@ -25,8 +25,9 @@ function upload_stats {
           bucket="cockroach-nightly"
       fi
 
-      remote_artifacts_dir="artifacts-${TC_BUILD_BRANCH}"
-      if [[ "${TC_BUILD_BRANCH}" == "master" ]]; then
+      branch=$(tc_build_branch)
+      remote_artifacts_dir="artifacts-${branch}"
+      if [[ "${branch}" == "master" ]]; then
         # The master branch is special, as roachperf hard-codes
         # the location.
         remote_artifacts_dir="artifacts"

pkg/server/status.go

@@ -219,19 +219,20 @@ func (b *baseStatusServer) getLocalSessions(
 	userSessions := make([]serverpb.Session, 0, len(sessions)+len(closedSessions))
 	sessions = append(sessions, closedSessions...)
 
+	reqUserNameNormalized := reqUsername.Normalized()
 	for _, session := range sessions {
-		if reqUsername.Normalized() != session.Username && !showAll {
+		if reqUserNameNormalized != session.Username && !showAll {
 			continue
 		}
 
-		if !isAdmin && hasViewActivityRedacted && (sessionUser != reqUsername) {
+		if !isAdmin && hasViewActivityRedacted && (reqUserNameNormalized != session.Username) {
 			// Remove queries with constants if user doesn't have correct privileges.
 			// Note that users can have both VIEWACTIVITYREDACTED and VIEWACTIVITY,
 			// with the former taking precedence.
-			for _, query := range session.ActiveQueries {
-				query.Sql = ""
+			for idx := range session.ActiveQueries {
+				session.ActiveQueries[idx].Sql = session.ActiveQueries[idx].SqlNoConstants
 			}
-			session.LastActiveQuery = ""
+			session.LastActiveQuery = session.LastActiveQueryNoConstants
 		}
 
 		userSessions = append(userSessions, session)

pkg/server/status_test.go

@@ -3330,7 +3330,7 @@ func TestStatusAPIListSessions(t *testing.T) {
 	err = getStatusJSONProtoWithAdminOption(serverProto, "sessions", &resp, false)
 	require.NoError(t, err)
 	session := getSessionWithTestAppName(&resp)
-	require.Empty(t, session.LastActiveQuery)
+	require.Equal(t, session.LastActiveQuery, session.LastActiveQueryNoConstants)
 	require.Equal(t, "SELECT _", session.LastActiveQueryNoConstants)
 
 	// Grant VIEWACTIVITY, VIEWACTIVITYREDACTED should take precedence.
@@ -3340,7 +3340,7 @@ func TestStatusAPIListSessions(t *testing.T) {
 	require.NoError(t, err)
 	session = getSessionWithTestAppName(&resp)
 	require.Equal(t, appName, session.ApplicationName)
-	require.Empty(t, session.LastActiveQuery)
+	require.Equal(t, session.LastActiveQuery, session.LastActiveQueryNoConstants)
 	require.Equal(t, "SELECT _, _", session.LastActiveQueryNoConstants)
 
 	// Remove VIEWACTIVITYREDCATED. User should now see full query.