Merge #153868

153868: rpc, server: add interceptor to catch panics in DRPC for DB console requests r=cthumuluru-crdb,shubhamdhama a=Nukitt

Previously if a DRPC handler experienced an uncaught panic, the entire node would crash. When serving a DB console request, uncaught panics due to its functionality should not cause the CRDB node to crash.

To address this, this patch introduces metadata into the context for all requests originating from the gateway and this is used by a panic recovery interceptor to detect such requests. This allows the server to decide whether to recover from the panic instead of letting it propagate and crash the node.

Epic: none
Fixes: #153452
Release note: None

Co-authored-by: Nukitt <[email protected]>

Author: craig[bot]

pkg/rpc/drpc.go

@@ -219,6 +219,9 @@ func NewDRPCServer(_ context.Context, rpcCtx *Context, opts ...ServerOption) (DR
 	unaryInterceptors = append(unaryInterceptors, stopUnary)
 	streamInterceptors = append(streamInterceptors, stopStream)
 
+	// Recover from any uncaught panics caused by DB Console requests.
+	unaryInterceptors = append(unaryInterceptors, DRPCGatewayRequestRecoveryInterceptor)
+
 	if !rpcCtx.ContextOptions.Insecure {
 		a := kvAuth{
 			sv: &rpcCtx.Settings.SV,

pkg/rpc/drpc_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 	"storj.io/drpc"
 )
@@ -77,3 +78,54 @@ func TestMakeStopperInterceptors(t *testing.T) {
 	require.ErrorIs(t, err, stop.ErrUnavailable)
 	require.False(t, called)
 }
+
+func TestGatewayRequestDRPCRecoveryInterceptor(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// With gateway metadata - should recover from panic
+	t.Run("with gateway metadata", func(t *testing.T) {
+		ctx := MarkDRPCGatewayRequest(context.Background())
+
+		handler := func(ctx context.Context, req interface{}) (interface{}, error) {
+			panic("test panic")
+		}
+
+		resp, err := DRPCGatewayRequestRecoveryInterceptor(ctx, nil, "test", handler)
+
+		require.Nil(t, resp)
+		require.ErrorContains(t, err, "unexpected error occurred")
+	})
+
+	// Without gateway metadata - should not recover from panic
+	t.Run("without gateway metadata", func(t *testing.T) {
+		ctx := context.Background()
+
+		handler := func(ctx context.Context, req interface{}) (interface{}, error) {
+			panic("test panic")
+		}
+
+		defer func() {
+			if r := recover(); r == nil {
+				t.Fatal("expected panic to propagate, got none")
+			}
+		}()
+
+		_, _ = DRPCGatewayRequestRecoveryInterceptor(ctx, nil, "test", handler)
+	})
+
+	// With gateway metadata but no panic - should pass through normally
+	t.Run("with gateway metadata no panic", func(t *testing.T) {
+		ctx := MarkDRPCGatewayRequest(context.Background())
+
+		expectedResp := "success"
+		expectedErr := errors.New("expected error")
+		handler := func(ctx context.Context, req interface{}) (interface{}, error) {
+			return expectedResp, expectedErr
+		}
+
+		resp, err := DRPCGatewayRequestRecoveryInterceptor(ctx, nil, "test", handler)
+
+		require.Equal(t, expectedResp, resp)
+		require.ErrorIs(t, err, expectedErr)
+	})
+}

pkg/rpc/metrics.go

@@ -23,10 +23,12 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
+	"storj.io/drpc/drpcmetadata"
+	"storj.io/drpc/drpcmux"
 )
 
 // gwRequestKey is a field set on the context to indicate a request
-// is coming from gRPC gateway.
+// is coming from RPC gateway.
 const gwRequestKey = "gw-request"
 
 var (
@@ -512,3 +514,27 @@ func gatewayRequestRecoveryInterceptor(
 	resp, err = handler(ctx, req)
 	return resp, err
 }
+
+// MarkDRPCGatewayRequest annotates ctx so that downstream DRPC calls can
+// be recognized as originating from the DB Console HTTP gateway.
+func MarkDRPCGatewayRequest(ctx context.Context) context.Context {
+	return drpcmetadata.Add(ctx, gwRequestKey, "true")
+}
+
+// DRPCGatewayRequestRecoveryInterceptor recovers from panics in DRPC handlers
+// that are invoked due to DB console requests. For these requests, we do not
+// want an uncaught panic to crash the node.
+func DRPCGatewayRequestRecoveryInterceptor(
+	ctx context.Context, req interface{}, rpc string, handler drpcmux.UnaryHandler,
+) (resp interface{}, err error) {
+	if val, ok := drpcmetadata.GetValue(ctx, gwRequestKey); ok && val != "" {
+		defer func() {
+			if p := recover(); p != nil {
+				logcrash.ReportPanic(ctx, nil, p, 1 /* depth */)
+				err = errors.New("an unexpected error occurred")
+			}
+		}()
+	}
+	resp, err = handler(ctx, req)
+	return resp, err
+}

pkg/server/apiinternal/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/roachpb",
+        "//pkg/rpc",
         "//pkg/rpc/rpcbase",
         "//pkg/server/authserver",
         "//pkg/server/serverpb",

pkg/server/apiinternal/api_internal.go

@@ -13,6 +13,7 @@ import (
 	"reflect"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/rpc/rpcbase"
 	"github.com/cockroachdb/cockroach/pkg/server/authserver"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
@@ -120,6 +121,7 @@ func executeRPC[TReq, TResp protoutil.Message](
 ) error {
 	ctx := req.Context()
 	ctx = authserver.ForwardHTTPAuthInfoToRPCCalls(ctx, req)
+	ctx = rpc.MarkDRPCGatewayRequest(ctx)
 
 	if err := decoder.Decode(rpcReq, req.URL.Query()); err != nil {
 		return err