Skip to content

Commit

Permalink
rpc: install custom proto codec
Browse files Browse the repository at this point in the history
This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes cockroachdb#60531

Release note: None
  • Loading branch information
stevendanna committed Feb 17, 2021
1 parent 1a43d11 commit 3e3d61c
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 4 deletions.
4 changes: 3 additions & 1 deletion pkg/rpc/BUILD.bazel
Expand Up @@ -9,6 +9,7 @@ go_library(
"auth_tenant.go",
"breaker.go",
"clock_offset.go",
"codec.go",
"connection_class.go",
"context.go",
"context_testutils.go",
Expand Down Expand Up @@ -39,6 +40,7 @@ go_library(
"//pkg/util/log/severity",
"//pkg/util/metric",
"//pkg/util/netutil",
"//pkg/util/protoutil",
"//pkg/util/stop",
"//pkg/util/syncutil",
"//pkg/util/timeutil",
Expand All @@ -49,6 +51,7 @@ go_library(
"@com_github_cockroachdb_errors//:errors",
"@com_github_cockroachdb_logtags//:logtags",
"@com_github_facebookgo_clock//:clock",
"@com_github_gogo_protobuf//proto",
"@com_github_golang_snappy//:snappy",
"@com_github_montanaflynn_stats//:stats",
"@com_github_vividcortex_ewma//:ewma",
Expand All @@ -57,7 +60,6 @@ go_library(
"@org_golang_google_grpc//codes",
"@org_golang_google_grpc//credentials",
"@org_golang_google_grpc//encoding",
"@org_golang_google_grpc//encoding/proto",
"@org_golang_google_grpc//keepalive",
"@org_golang_google_grpc//metadata",
"@org_golang_google_grpc//peer",
Expand Down
41 changes: 41 additions & 0 deletions pkg/rpc/codec.go
@@ -0,0 +1,41 @@
// Copyright 2020 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 rpc

import (
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/gogo/protobuf/proto"
"google.golang.org/grpc/encoding"
)

const name = "proto"

type codec struct{}

var _ encoding.Codec = codec{}

func (codec) Marshal(v interface{}) ([]byte, error) {
if pm, ok := v.(proto.Marshaler); ok {
return pm.Marshal()
}
return protoutil.Marshal(v.(protoutil.Message))
}

func (codec) Unmarshal(data []byte, v interface{}) error {
if pm, ok := v.(proto.Unmarshaler); ok {
return pm.Unmarshal(data)
}
return protoutil.Unmarshal(data, v.(protoutil.Message))
}

func (codec) Name() string {
return name
}
4 changes: 1 addition & 3 deletions pkg/rpc/context.go
Expand Up @@ -46,7 +46,6 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/encoding"
encodingproto "google.golang.org/grpc/encoding/proto"
"google.golang.org/grpc/metadata"
grpcstatus "google.golang.org/grpc/status"
)
Expand Down Expand Up @@ -794,8 +793,7 @@ func (c growStackCodec) Unmarshal(data []byte, v interface{}) error {
// Install the growStackCodec over the default proto codec in order to grow the
// stack for BatchRequest RPCs prior to unmarshaling.
func init() {
protoCodec := encoding.GetCodec(encodingproto.Name)
encoding.RegisterCodec(growStackCodec{Codec: protoCodec})
encoding.RegisterCodec(growStackCodec{Codec: codec{}})
}

// onlyOnceDialer implements the grpc.WithDialer interface but only
Expand Down
2 changes: 2 additions & 0 deletions pkg/testutils/lint/lint_test.go
Expand Up @@ -964,6 +964,7 @@ func TestLint(t *testing.T) {
":!sql/*.pb.go",
":!util/protoutil/marshal.go",
":!util/protoutil/marshaler.go",
":!rpc/codec.go",
":!settings/settings_test.go",
":!sql/types/types_jsonpb.go",
":!sql/schemachanger/scbuild/builder_test.go",
Expand Down Expand Up @@ -1007,6 +1008,7 @@ func TestLint(t *testing.T) {
":!util/protoutil/marshal.go",
":!util/protoutil/marshaler.go",
":!util/encoding/encoding.go",
":!rpc/codec.go",
":!util/hlc/timestamp.go",
":!sql/types/types_jsonpb.go",
)
Expand Down

0 comments on commit 3e3d61c

Please sign in to comment.