Merge #56878

56878: server: remove KeepRedactable logs rpc option r=andreimatei a=andreimatei

This patch removes the KeepRedactable option from the Logs and LogFile
RPCs. The option existed just for backwards compatibility with pre-20.2
clients. Even for such old clients, I don't think there's real
consequences to removing this option. This patch retains the behvior of
the option being set.
The option, when coupled with the Redact=false option, made the RPCs
keep the log entries exactly as they are in the log file - i.e.
redactable, with sensistive data markers. With this patch, the logs
returned by the RPCs are either redacted, or redactable. "Flattening"
them (i.e. not redacting, but stripping the markers) is no longer an
option.

The Logs RPC is used by the AdminUI, which doesn't use either Redact or
KeepRedactable - so, with this patch, the UI will show logs with markers
in them. I think that's OK.
The LogFile RPC is used by the cli getting a debug zip. Since 20.2, the
client is setting KeepRedactable, and so nothing changes. Older clients
will get log entries with markers in them.

This change is motivated by the fact that I'd like to replace the
LogFile RPC with a more efficient version that returns a raw file, not a
proto-encoded list of log entries. In fact we already have that RPC in
the form of GetFiles, which we may be able to use. This KeepRedactable
option was in my way.

Release note: None

Co-authored-by: Andrei Matei <[email protected]>

docs/generated/http/full.md

@@ -1300,7 +1300,6 @@ information about the resources on a node used by that table.
 | node_id | [string](#cockroach.server.serverpb.LogFileRequest-string) |  | node_id is a string so that "local" can be used to specify that no forwarding is necessary. |
 | file | [string](#cockroach.server.serverpb.LogFileRequest-string) |  |  |
 | redact | [bool](#cockroach.server.serverpb.LogFileRequest-bool) |  | redact, if true, requests redaction of sensitive data away from the retrieved log entries. Only admin users can send a request with redact = false. |
-| keep_redactable | [bool](#cockroach.server.serverpb.LogFileRequest-bool) |  | keep_redactable, if true, requests that retrieved entries preserve the redaction markers if any were present in the log files. If false, redaction markers are stripped away. Note that redact = false && redactable = false implies "flat" entries with all sensitive information enclosed and no markers; this is suitable for backward-compatibility with RPC clients from prior the introduction of redactable logs. |
 
 
 
@@ -1343,7 +1342,6 @@ information about the resources on a node used by that table.
 | max | [string](#cockroach.server.serverpb.LogsRequest-string) |  |  |
 | pattern | [string](#cockroach.server.serverpb.LogsRequest-string) |  |  |
 | redact | [bool](#cockroach.server.serverpb.LogsRequest-bool) |  | redact, if true, requests redaction of sensitive data away from the retrieved log entries. Only admin users can send a request with redact = false. |
-| keep_redactable | [bool](#cockroach.server.serverpb.LogsRequest-bool) |  | keep_redactable, if true, requests that retrieved entries preserve the redaction markers if any were present in the log files. If false, redaction markers are stripped away. Note that redact = false && redactable = false implies "flat" entries with all sensitive information enclosed and no markers; this is suitable for backward-compatibility with RPC clients from prior the introduction of redactable logs. |
 
 
 

pkg/cli/zip.go

@@ -645,7 +645,7 @@ func runDebugZip(cmd *cobra.Command, args []string) (retErr error) {
 						func(ctx context.Context) error {
 							entries, err = status.LogFile(
 								ctx, &serverpb.LogFileRequest{
-									NodeId: id, File: file.Name, Redact: zipCtx.redactLogs, KeepRedactable: true,
+									NodeId: id, File: file.Name, Redact: zipCtx.redactLogs,
 								})
 							return err
 						}); err != nil {

pkg/server/serverpb/status.proto

@@ -310,14 +310,7 @@ message LogsRequest {
   // from the retrieved log entries.
   // Only admin users can send a request with redact = false.
   bool redact = 7;
-  // keep_redactable, if true, requests that retrieved entries preserve
-  // the redaction markers if any were present in the log files.
-  // If false, redaction markers are stripped away.
-  // Note that redact = false && redactable = false implies
-  // "flat" entries with all sensitive information enclosed and
-  // no markers; this is suitable for backward-compatibility with
-  // RPC clients from prior the introduction of redactable logs.
-  bool keep_redactable = 8;
+  reserved 8;
 }
 
 message LogEntriesResponse {
@@ -345,14 +338,7 @@ message LogFileRequest {
   // from the retrieved log entries.
   // Only admin users can send a request with redact = false.
   bool redact = 3;
-  // keep_redactable, if true, requests that retrieved entries preserve
-  // the redaction markers if any were present in the log files.
-  // If false, redaction markers are stripped away.
-  // Note that redact = false && redactable = false implies
-  // "flat" entries with all sensitive information enclosed and
-  // no markers; this is suitable for backward-compatibility with
-  // RPC clients from prior the introduction of redactable logs.
-  bool keep_redactable = 4;
+  reserved 4;
 }
 
 enum StacksType {

pkg/server/status.go

@@ -982,7 +982,7 @@ func (s *statusServer) LogFile(
 	}
 
 	// Determine how to redact.
-	inputEditMode := log.SelectEditMode(req.Redact, req.KeepRedactable)
+	inputEditMode := log.SelectEditMode(req.Redact, log.KeepRedactable)
 
 	// Ensure that the latest log entries are available in files.
 	log.Flush()
@@ -1007,13 +1007,6 @@ func (s *statusServer) LogFile(
 		resp.Entries = append(resp.Entries, entry)
 	}
 
-	// Erase the redactable bit if requested by client.
-	if !req.KeepRedactable {
-		for i := range resp.Entries {
-			resp.Entries[i].Redactable = false
-		}
-	}
-
 	return &resp, nil
 }
 
@@ -1075,7 +1068,7 @@ func (s *statusServer) Logs(
 	}
 
 	// Determine how to redact.
-	inputEditMode := log.SelectEditMode(req.Redact, req.KeepRedactable)
+	inputEditMode := log.SelectEditMode(req.Redact, log.KeepRedactable)
 
 	// Select the time interval.
 	startTimestamp, err := parseInt64WithDefault(
@@ -1119,13 +1112,6 @@ func (s *statusServer) Logs(
 		return nil, err
 	}
 
-	// Erase the redactable bit if requested by client.
-	if !req.KeepRedactable {
-		for i := range entries {
-			entries[i].Redactable = false
-		}
-	}
-
 	return &serverpb.LogEntriesResponse{Entries: entries}, nil
 }
 

pkg/server/status_test.go

@@ -561,11 +561,11 @@ func TestStatusLocalLogs(t *testing.T) {
 		}
 		for _, entry := range wrapper.Entries {
 			switch entry.Message {
-			case "TestStatusLocalLogFile test message-Error":
+			case "‹TestStatusLocalLogFile test message-Error›":
 				foundError = true
-			case "TestStatusLocalLogFile test message-Warning":
+			case "‹TestStatusLocalLogFile test message-Warning›":
 				foundWarning = true
-			case "TestStatusLocalLogFile test message-Info":
+			case "‹TestStatusLocalLogFile test message-Info›":
 				foundInfo = true
 			}
 		}
@@ -640,11 +640,11 @@ func TestStatusLocalLogs(t *testing.T) {
 				fmt.Fprintln(&logsBuf, entry.Message)
 
 				switch entry.Message {
-				case "TestStatusLocalLogFile test message-Error":
+				case "‹TestStatusLocalLogFile test message-Error›":
 					actual.Error = true
-				case "TestStatusLocalLogFile test message-Warning":
+				case "‹TestStatusLocalLogFile test message-Warning›":
 					actual.Warning = true
-				case "TestStatusLocalLogFile test message-Info":
+				case "‹TestStatusLocalLogFile test message-Info›":
 					actual.Info = true
 				}
 			}
@@ -664,39 +664,24 @@ func TestStatusLogRedaction(t *testing.T) {
 	testData := []struct {
 		redactableLogs     bool // logging flag
 		redact             bool // RPC request flag
-		keepRedactable     bool // RPC request flag
 		expectedMessage    string
 		expectedRedactable bool // redactable bit in result entries
 	}{
-		// Note: all 2^3 combinations of (redactableLogs, redact,
-		// keepRedactable) must be tested below.
-
-		// redact=false, keepredactable=false results in an unsafe "flat"
-		// format regardless of whether there were markers in the log
-		// file.
-		{false, false, false, `THISISSAFE THISISUNSAFE`, false},
-		// keepredactable=true, if there were no markers to start with
-		// (redactableLogs=false), introduces markers around the entire
-		// message to indicate it's not known to be safe.
-		{false, false, true, `‹THISISSAFE THISISUNSAFE›`, true},
+		// Note: all combinations of (redactableLogs, redact) must be tested below.
+
+		// If there were no markers to start with (redactableLogs=false), we
+		// introduce markers around the entire message to indicate it's not known to
+		// be safe.
+		{false, false, `‹THISISSAFE THISISUNSAFE›`, true},
 		// redact=true must be conservative and redact everything out if
 		// there were no markers to start with (redactableLogs=false).
-		{false, true, false, `‹×›`, false},
-		{false, true, true, `‹×›`, false},
-		// redact=false, keepredactable=false results in an unsafe "flat"
-		// format regardless of whether there were markers in the log
-		// file.
-		{true, false, false, `THISISSAFE THISISUNSAFE`, false},
-		// keepredactable=true, redact=false, keeps whatever was in the
-		// log file.
-		{true, false, true, `THISISSAFE ‹THISISUNSAFE›`, true},
-		// if there were markers in the log to start with, redact=true
-		// removes only the unsafe information.
-		{true, true, false, `THISISSAFE ‹×›`, false},
+		{false, true, `‹×›`, false},
+		// redact=false keeps whatever was in the log file.
+		{true, false, `THISISSAFE ‹THISISUNSAFE›`, true},
 		// Whether or not to keep the redactable markers has no influence
 		// on the output of redaction, just on the presence of the
 		// "redactable" marker. In any case no information is leaked.
-		{true, true, true, `THISISSAFE ‹×›`, true},
+		{true, true, `THISISSAFE ‹×›`, true},
 	}
 
 	testutils.RunTrueAndFalse(t, "redactableLogs",
@@ -732,7 +717,7 @@ func TestStatusLogRedaction(t *testing.T) {
 				if tc.redactableLogs != redactableLogs {
 					continue
 				}
-				t.Run(fmt.Sprintf("redact=%v,keepredactable=%v", tc.redact, tc.keepRedactable),
+				t.Run(fmt.Sprintf("redact=%v", tc.redact),
 					func(t *testing.T) {
 						// checkEntries asserts that the redaction results are
 						// those expected in tc.
@@ -753,8 +738,7 @@ func TestStatusLogRedaction(t *testing.T) {
 
 						// Retrieve the log entries with the configured flags using
 						// the LogFiles() RPC.
-						logFilesURL := fmt.Sprintf("logfiles/local/%s?redact=%v&keep_redactable=%v",
-							file.Name, tc.redact, tc.keepRedactable)
+						logFilesURL := fmt.Sprintf("logfiles/local/%s?redact=%v", file.Name, tc.redact)
 						var wrapper serverpb.LogEntriesResponse
 						if err := getStatusJSONProto(ts, logFilesURL, &wrapper); err != nil {
 							t.Fatal(err)
@@ -771,8 +755,7 @@ func TestStatusLogRedaction(t *testing.T) {
 						}
 
 						// Retrieve the log entries using the Logs() RPC.
-						logsURL := fmt.Sprintf("logs/local?redact=%v&keep_redactable=%v",
-							tc.redact, tc.keepRedactable)
+						logsURL := fmt.Sprintf("logs/local?redact=%v", tc.redact)
 						var wrapper2 serverpb.LogEntriesResponse
 						if err := getStatusJSONProto(ts, logsURL, &wrapper2); err != nil {
 							t.Fatal(err)

pkg/util/log/redact.go

@@ -38,6 +38,11 @@ const (
 	WithoutSensitiveData
 )
 
+// KeepRedactable can be used as an argument to SelectEditMode to indicate that
+// the logs should retain their sensitive data markers so that they can be
+// redacted later.
+const KeepRedactable = true
+
 // SelectEditMode returns an EditSensitiveData value that's suitable
 // for use with NewDecoder depending on client-side desired
 // "redact" and "keep redactable" flags.