Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Feb 17, 2021

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 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.

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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
Copy link
Member

@tbg tbg left a 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: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)

Copy link
Contributor

@knz knz left a 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: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)

Copy link
Collaborator Author

@stevendanna stevendanna left a 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: :shipit: 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.

@stevendanna
Copy link
Collaborator Author

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.

@irfansharif irfansharif removed their request for review March 2, 2021 17:43
@stevendanna stevendanna deleted the bump/grpc branch April 13, 2021 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cdc Change Data Capture T-cdc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vendor: can't upgrade to grpc-go v1.34
5 participants