Merge #61617

61617: lint: forbid gRPC Status.WithDetails() due to gogoproto issues r=tbg a=erikgrinaker

gRPC's `Status.WithDetails()` allows callers to attach
Protobuf-structured details to gRPC errors. Unfortunately, this does not
work with gogoproto types, since they're stored in an `Any` field
internally and gogo types are not registered in the standard Protobuf
type registry. Calling `Details()` with such a type will return an
unmarshalling error in place of the detail.

To avoid this problem, this patch adds a linter that disallows any use of
`Status.WithDetails()`.

Note that there is a separate package `github.com/gogo/status` that
emulates the standard gRPC `status` package using gogoproto Protobufs
instead. However, due to the uncertain future of the gogoproto project
we are hesitant to introduce additional gogo dependencies.

Related to #56208 and cockroachdb/errors#63.

Release justification: non-production code changes
Release note: None

Co-authored-by: Erik Grinaker <[email protected]>

pkg/testutils/lint/passes/forbiddenmethod/analyzers.go

@@ -34,6 +34,15 @@ var grpcClientConnCloseOptions = Options{
 		"Do `grpcConn.Close() // nolint:grpcconnclose` when the conn does not come from an rpc.Context.",
 }
 
+var grpcStatusWithDetailsOptions = Options{
+	PassName: "grpcstatuswithdetails",
+	Doc:      `prevent calls to (status.Status).WithDetails`,
+	Package:  "google.golang.org/grpc/internal/status",
+	Type:     "Status",
+	Method:   "WithDetails",
+	Hint:     "does not work with gogoproto-generated Protobufs.",
+}
+
 // DescriptorMarshalAnalyzer checks for correct unmarshaling of descpb
 // descriptors by disallowing calls to (descpb.Descriptor).GetTable().
 var DescriptorMarshalAnalyzer = Analyzer(descriptorMarshalOptions)
@@ -43,8 +52,21 @@ var DescriptorMarshalAnalyzer = Analyzer(descriptorMarshalOptions)
 // Errant calls to Close() disrupt the connection for all users.
 var GRPCClientConnCloseAnalyzer = Analyzer(grpcClientConnCloseOptions)
 
+// GRPCStatusWithDetailsAnalyzer checks that we don't use Status.WithDetails(),
+// since it does not support Protobufs generated by gogoproto. This is
+// because it uses an Any field to store details, with a reference to the
+// type, but gogoproto types are not registered with the standard Protobuf
+// type registry and are therefore unknown to the unmarshaller.
+//
+// One could instead use a GoGo-compatible version of Status from
+// github.com/gogo/status, but we probably want to move away from gogoproto.
+//
+// See also: https://github.com/cockroachdb/errors/issues/63
+var GRPCStatusWithDetailsAnalyzer = Analyzer(grpcStatusWithDetailsOptions)
+
 // Analyzers are all of the Analyzers defined in this package.
 var Analyzers = []*analysis.Analyzer{
 	DescriptorMarshalAnalyzer,
 	GRPCClientConnCloseAnalyzer,
+	GRPCStatusWithDetailsAnalyzer,
 }

pkg/testutils/lint/passes/forbiddenmethod/descriptormarshal_test.go

@@ -23,4 +23,5 @@ func Test(t *testing.T) {
 	testdata := analysistest.TestData()
 	analysistest.Run(t, testdata, forbiddenmethod.DescriptorMarshalAnalyzer, "descmarshaltest")
 	analysistest.Run(t, testdata, forbiddenmethod.GRPCClientConnCloseAnalyzer, "grpcconnclosetest")
+	analysistest.Run(t, testdata, forbiddenmethod.GRPCStatusWithDetailsAnalyzer, "grpcstatuswithdetailstest")
 }

pkg/testutils/lint/passes/forbiddenmethod/testdata/src/google.golang.org/grpc/internal/status/BUILD.bazel

@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "status",
+    srcs = ["status.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod/testdata/src/google.golang.org/grpc/internal/status",
+    visibility = ["//visibility:public"],
+)

pkg/testutils/lint/passes/forbiddenmethod/testdata/src/google.golang.org/grpc/internal/status/status.go

@@ -0,0 +1,17 @@
+// Copyright 2021 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 status
+
+type Status struct{}
+
+func (s *Status) WithDetails() (*Status, error) {
+	return s, nil
+}

pkg/testutils/lint/passes/forbiddenmethod/testdata/src/google.golang.org/grpc/status/BUILD.bazel

@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "status",
+    srcs = ["status.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod/testdata/src/google.golang.org/grpc/status",
+    visibility = ["//visibility:public"],
+)

pkg/testutils/lint/passes/forbiddenmethod/testdata/src/google.golang.org/grpc/status/status.go

@@ -0,0 +1,23 @@
+// Copyright 2021 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 status
+
+import (
+	istatus "google.golang.org/grpc/internal/status"
+)
+
+// This mirrors the gRPC structure, where status.Status is an alias for an
+// internal type. The lint needs to check on the internal type.
+type Status = istatus.Status
+
+func New(_ int, _ string) *Status {
+	return &Status{}
+}

pkg/testutils/lint/passes/forbiddenmethod/testdata/src/grpcstatuswithdetailstest/BUILD.bazel

@@ -0,0 +1,9 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "grpstatuswithdetailstest",
+    srcs = ["foo.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod/testdata/src/grpcstatuswithdetailstest",
+    visibility = ["//visibility:public"],
+    deps = ["@org_golang_google_grpc//:go_default_library"],
+)

pkg/testutils/lint/passes/forbiddenmethod/testdata/src/grpcstatuswithdetailstest/status.go

@@ -0,0 +1,21 @@
+// Copyright 2021 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 grpcstatuswithdetailstest
+
+import "google.golang.org/grpc/status"
+
+func F() {
+	s := status.New(1, "message")
+	_, _ = s.WithDetails() // want `Illegal call to Status.WithDetails\(\)`
+
+	//nolint:grpcstatuswithdetails
+	_, _ = s.WithDetails()
+}