Merge #103856

103856: cloud,grpcutil: avoid pretty-printing nil gRPC status r=knz a=stevendanna

In some cases, errors from the GCP library would be printed as

    grpc:  [code 0/OK]

This was the result of a few interacting features and changes:

- The GCP library added an Unwrap() method to the error type they returned.

- Our errors library has code to manually unwrap errors.

- The underlying GCP error attempts to abstract over gRPC and HTTP errors. It implements a GRPCStatus() method that returns nil if the underlying issue was an HTTP issue.

- Our custom error printer in grpcutil would specially print anything that implements GRPCStatus(), regardless of the return value.

Here, I've updated the custom printer to ignore cases where GRPCStatus() returns nil.

Epic: CRDB-27642

Release note (bug fix): Fix a bug in which some GCP-related errors would be returned with an uninformative error.

Co-authored-by: Steven Danna <[email protected]>

go.mod

@@ -151,6 +151,7 @@ require (
 	github.com/google/go-github/v42 v42.0.0
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
 	github.com/google/skylark v0.0.0-20181101142754-a5f7082aabed
+	github.com/googleapis/gax-go/v2 v2.7.0
 	github.com/gorilla/mux v1.8.0
 	github.com/goware/modvendor v0.5.0
 	github.com/grpc-ecosystem/grpc-gateway v1.16.0
@@ -303,7 +304,6 @@ require (
 	github.com/google/go-querystring v1.1.0 // indirect
 	github.com/google/safehtml v0.0.2 // indirect
 	github.com/googleapis/enterprise-certificate-proxy v0.2.0 // indirect
-	github.com/googleapis/gax-go/v2 v2.7.0 // indirect
 	github.com/gorilla/handlers v1.5.1 // indirect
 	github.com/gorilla/websocket v1.4.2 // indirect
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect

pkg/cloud/gcp/BUILD.bazel

@@ -46,6 +46,7 @@ go_library(
 go_test(
     name = "gcp_test",
     srcs = [
+        "error_test.go",
         "gcp_kms_test.go",
         "gcs_storage_test.go",
     ],
@@ -62,9 +63,12 @@ go_test(
         "//pkg/util/ioctx",
         "//pkg/util/leaktest",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_googleapis_gax_go_v2//apierror",
+        "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
         "@com_google_cloud_go_kms//apiv1",
         "@com_google_cloud_go_storage//:storage",
+        "@org_golang_google_api//googleapi",
         "@org_golang_google_api//impersonate",
         "@org_golang_x_oauth2//google",
     ],

pkg/cloud/gcp/error_test.go

@@ -0,0 +1,35 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package gcp
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/errors"
+	"github.com/googleapis/gax-go/v2/apierror"
+	"github.com/stretchr/testify/assert"
+	"google.golang.org/api/googleapi"
+)
+
+func TestErrorBehaviour(t *testing.T) {
+	orig := &googleapi.Error{
+		Code:    403,
+		Message: "ACCESS DENIED. ALL YOUR BASE ARE BELONG TO US",
+	}
+	apiError, ok := apierror.ParseError(orig, false)
+	if ok {
+		orig.Wrap(apiError)
+	}
+	wrap1 := errors.Wrap(orig, "wrap1")
+	wrap2 := errors.Wrap(wrap1, "wrap2")
+	assert.Equal(t, "wrap1: googleapi: Error 403: ACCESS DENIED. ALL YOUR BASE ARE BELONG TO US", wrap1.Error())
+	assert.Equal(t, "wrap2: wrap1: googleapi: Error 403: ACCESS DENIED. ALL YOUR BASE ARE BELONG TO US", wrap2.Error())
+}

pkg/util/grpcutil/BUILD.bazel

@@ -56,6 +56,7 @@ go_test(
         "@org_golang_google_grpc//codes",
         "@org_golang_google_grpc//health/grpc_health_v1",
         "@org_golang_google_grpc//metadata",
+        "@org_golang_google_grpc//status",
     ],
 )
 

pkg/util/grpcutil/grpc_err_redaction.go

@@ -16,13 +16,25 @@ import (
 	"github.com/cockroachdb/errors/errbase"
 	"github.com/cockroachdb/redact"
 	"github.com/gogo/status"
+	grpcStatus "google.golang.org/grpc/status"
 )
 
 func grpcSpecialCasePrintFn(err error, p errors.Printer, isLeaf bool) (handled bool, next error) {
+	// If GRPCStatus() returns nil, status.FromError will still
+	// return true, but the retuned error will have no message and
+	// a code of OK.
+	// nolint:errcmp
+	if se, ok := err.(interface{ GRPCStatus() *grpcStatus.Status }); ok {
+		if se.GRPCStatus() == nil {
+			return false, nil
+		}
+	}
+
 	s, ok := status.FromError(err)
 	if !ok {
 		return false, nil
 	}
+
 	p.Printf("grpc: %s [code %[2]d/%[2]s]", s.Message(), redact.Safe(s.Code()))
 	for _, d := range s.Details() {
 		p.Printf(", %s", d)

pkg/util/grpcutil/grpc_err_redaction_test.go

@@ -20,13 +20,24 @@ import (
 	"github.com/gogo/status"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
+	grpcStatus "google.golang.org/grpc/status"
 )
 
 func TestGRPCErrRedaction(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	t.Run("prints gogo status errors", func(t *testing.T) {
+		s := status.Newf(codes.Unauthenticated, "%d %s %s", 1, "two", redact.Safe("three"))
+		err := errors.Wrap(s.Err(), "boom")
+		require.EqualValues(t, `boom: grpc: ‹1 two three› [code 16/Unauthenticated]`, redact.Sprint(err))
+	})
+	t.Run("does not handle nil status.Status", func(t *testing.T) {
+		e := &testingErrWithGRPCStatus{}
+		err := errors.Wrap(e, "boom")
+		require.EqualValues(t, `boom: ‹test error›`, redact.Sprint(err))
+	})
+}
 
-	s := status.Newf(codes.Unauthenticated, "%d %s %s", 1, "two", redact.Safe("three"))
+type testingErrWithGRPCStatus struct{}
 
-	err := errors.Wrap(s.Err(), "boom")
-	require.EqualValues(t, `boom: grpc: ‹1 two three› [code 16/Unauthenticated]`, redact.Sprint(err))
-}
+func (e *testingErrWithGRPCStatus) GRPCStatus() *grpcStatus.Status { return nil }
+func (e *testingErrWithGRPCStatus) Error() string                  { return "test error" }