New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade google.golang.org/grpc, fixing gogoproto incompatibility #60709
Conversation
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
This includes a large number of changes. Updating google.golang.org/grpc appears to require an upgrade to google.golang.org/api which then pushes cloud.google.com/go, leading to many of the other updates. cloud.google.com/go v0.34.0 -> v0.74.0 googleapis/google-cloud-go@v0.34.0...v0.77.0 github.com/golang/protobuf v1.4.2 -> v1.4.3 golang/protobuf@v1.4.2...v1.4.3 github.com/google/go-cmp v0.5.2 -> v0.5.4 google/go-cmp@v0.5.2...v0.5.4 github.com/google/pprof v0.0.0-20190109223431-e84dfd68c163 -> v0.0.0-20201203190320-1bf35d6f28c2 google/pprof@e84dfd6...1bf35d6 github.com/googleapis/gax-go v2.0.2+incompatible -> v2.0.5 google/pprof@v.2.0.2...v2.0.5 github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6 -> v0.0.0-20200824232613-28f6c0f3b639 ianlancetaylor/demangle@5e5cf60...28f6c0f go.opencensus.io v0.18.0 -> v0.22.5 census-instrumentation/opencensus-go@v0.18.0...v0.22.5 golang.org/x/lint v0.0.0-20200130185559-910be7a94367 -> v0.0.0-20201208152925-83fdc39ff7b5 golang/lint@910be7a...83fdc39 golang.org/x/mod v0.3.1-0.20200828183125-ce943fd02449 -> v0.4.0 golang/mod@ce943fd...v0.4.0 golang.org/x/oauth2 v0.0.0-20190115181402-5dab4167f31c -> v0.0.0-20201208152858-08078c50e5b5 golang/oauth2@5dab416...08078c5 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9 -> v0.0.0-20201207232520-09787c993a3a golang/sync@67f06af...09787c9 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4 -> v0.0.0-20191024005414-555d28b269f0 golang/time@9d24e82...555d28b google.golang.org/api v0.1.0 -> v0.40.0 googleapis/google-api-go-client@v0.1.0...v0.40. google.golang.org/appengine v1.4.0 -> v1.6.7 golang/appengine@v1.4.0...v1.6.7 google.golang.org/genproto v0.0.0-20200218151345-dad8c97a84f5 -> v0.0.0-20201214200347-8c77b98c765d googleapis/go-genproto@dad8c97...8c77b98 google.golang.org/grpc v1.29.1 -> v1.35.0 grpc/grpc-go@v1.29.1...v1.35.0 google.golang.org/protobuf v1.23.0 -> v1.25.0 protocolbuffers/protobuf-go@v1.23.0...v1.25.0 Release note: None
51429c0
to
eec9984
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @stevendanna and sorry for letting this sit! Due to the stability period, I would postpone the merge to after the release branch has been cut. I have held off on scrutinizing the dep bump so far, so we'd have to do that before the merge.
Also cc @knz for server/networking.
Reviewed 4 of 4 files at r1, 8 of 8 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wowzer that's a lot of dep changes.
The changes inside crdb LGTM, but as usual we will want to scrutinize the upstream changes as well. Agreed with Tobias that may not be suitable for the 21.1 release but we should get this started soon afterwards.
Reviewed 3 of 4 files at r1, 7 of 8 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knz @tbg Completely understandable and likely very prudent.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)
pkg/rpc/codec.go, line 29 at r2 (raw file):
return pm.Marshal() } return protoutil.Marshal(v.(protoutil.Message))
I updated this line to use protoutil in accordance with the linter. But, I think we should probably exclude this file from the linter and move this back to calling one of the proto libraries directly. My concern is that the conversion to protoutil.Message requires that the MarshalTo method be implemented which I don't think is going to be the case for various third party libraries.
I've pulled out the first commit and added some tests for it here: #61333 I still think both the new codec and this upgrade can wait until we cut a new branch though. |
In a different line of work, attempting to upgrade a client library for
google cloud APIs brought in an upgrade to google.golang.org/grpc and revealed
that a recent changes to google.golang.org/grpc have modified 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 itappears 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.
The upgrade of google.golang.org/grpc is larger than I might like, but I
was unable to convince Go to upgrade fewer dependencies as the grpc update
forces an update of google.golang.org/api, which then forces an upgrade
of a number of other dependencies. If we don't want to take the dependency
upgrade yet, we can merge the first commit to unblock future upgrades.
Fixes #60531