support "v1" of the reflection protocol, not just "v1alpha"

[jhump]

The server reflection service has a v1:
https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto

But this repo still only uses the v1alpha version.

Ideally, the implementation in the reflection sub-package would register implementations for both (or at least accept an option to do so). That would support a transition to the v1 API while still supporting clients that continue to use the v1alpha version.

[dfawley]

@ejona86 @markdroth What is happening in grpc-java/c++ here? Do you support both versions? v1 was added to the grpc-proto repo 4 years ago, but we never picked it up in grpc-go.

[ejona86]

To my knowledge, no language is using v1. Carl added the v1 version, but nothing else happened with it.

It's one of those "how is nobody complaining about this" things, while simultaneously being "things work fine." I do expect this will be painful in many languages, as the API used to register the service commonly only supports a single service.

[ejona86]

See also: grpc/grpc-java#6724

[buzzsurfr]

Requesting to take this one as a first-time contributor (in code) to grpc-go. (Still pumped from KubeCon NA 2022)

[dfawley]

@buzzsurfr SGTM, I think we can go ahead and support both v1 and v1alpha.

[easwars]

As part of this change, it would also be good to fix our script which compiles the proto files. Currently we have a copy of this proto file in our repo. We are trying to get rid of this approach where we have a copy of the proto file in our repo, but instead want the regenerate.sh to pull protos from their canonical sources and only store the generated pb.gos in our repo.

[buzzsurfr]

@easwars Looking at the script, what's the difference between legacy sources (Generates sources without the embed requirement) and sources (Generates only the new gRPC Service symbols). What's the "embed requirement"?

grpc-go/regenerate.sh

Lines 53 to 74 in 7f23df0

	# Generates sources without the embed requirement
	LEGACY_SOURCES=(
	${WORKDIR}/grpc-proto/grpc/binlog/v1/binarylog.proto
	${WORKDIR}/grpc-proto/grpc/channelz/v1/channelz.proto
	${WORKDIR}/grpc-proto/grpc/health/v1/health.proto
	${WORKDIR}/grpc-proto/grpc/lb/v1/load_balancer.proto
	profiling/proto/service.proto
	reflection/grpc_reflection_v1alpha/reflection.proto
	)
	# Generates only the new gRPC Service symbols
	SOURCES=(
	$(git ls-files --exclude-standard --cached --others "*.proto" | grep -v '^\(profiling/proto/service.proto\|reflection/grpc_reflection_v1alpha/reflection.proto\)$')
	${WORKDIR}/grpc-proto/grpc/gcp/altscontext.proto
	${WORKDIR}/grpc-proto/grpc/gcp/handshaker.proto
	${WORKDIR}/grpc-proto/grpc/gcp/transport_security_common.proto
	${WORKDIR}/grpc-proto/grpc/lookup/v1/rls.proto
	${WORKDIR}/grpc-proto/grpc/lookup/v1/rls_config.proto
	${WORKDIR}/grpc-proto/grpc/testing/*.proto
	${WORKDIR}/grpc-proto/grpc/core/*.proto
	)

[buzzsurfr]

I did try moving v1alpha to use the protobuf from grpc-proto/grpc/reflection/v1alpha but it's missing a go_package directive.

WARNING: Missing 'go_package' option in "grpc/reflection/v1alpha/reflection.proto"

I see that v1 has it--should I open a separate PR to add a similar line to v1alpha?

[easwars]

I see that v1 has it--should I open a separate PR to add a similar line to v1alpha?

That would be cool if you could do it. Thank you so much.

[buzzsurfr]

From #5773:

the implementation must import both packages (and import alias them appropriately, something like reflectionv1pb and reflectionv1alphapb), and export both services

Should there be two separate implementations--one for v1alpha and one for v1? I'm not sure how we can implement both without either changing the function names or swapping them out at some point.

[dfawley]

I think we will need one primary implementation and one wrapper implementation that adapts v1alpha requests & responses into v1 requests & responses. I think we can use proto.Marshal + proto.Unmarshal to convert; it's not very efficient but reflection shouldn't be performance sensitive, either.

Register should register both.

[dfawley]

Sorry, one more thing: NewServer should continue returning a v1alpha server, and NewServerV1 can return the v1 implementation.

[buzzsurfr]

@dfawley Since we'd eventually want to move v1alpha to v1, should we instead wrap this into an interface and you can pass in either a v1alpha or v1 proto?

[dfawley]

IMO that's a fine implementation detail, but the important part is: we should make the Register behavior register both v1 and v1alpha for now (ultimately only v1 in the distant future), and we should provide another API that returns the v1 rpb.ServerReflectionServer.

[easwars]

@buzzsurfr : Just checking in to see if there is any update here. Thanks.

[yurishkuro]

I am trying to upgrade to latest grpc-go version and staticcheck linter is breaking because #5799 started using a different proto file and all v1alpha types are now labeled deprecated. However, upgrading to v1 is not possible because reflection package itself is using v1alpha.

[dfawley]

@yurishkuro you will need to work around this on your end. We will need to compile in v1alpha protos for a very long time (possibly even forever?) for compatibility with other versions of gRPC. Generally speaking, "deprecated" does not mean "should never be used", so these warnings from staticcheck should be advisory, not blocking.

[yurishkuro]

@dfawley yes, I've excluded the code path from the linter. But I do think the "deprecated" labeling is invalid, given that not only this "should be used" per your comment, but it's the only thing that can be used based on the code and comments above regarding usage in other languages.

[jhump]

@dfawley, what's the status of this? Is it being worked on? Would you accept a patch?

I've got a library that supports v1 from the client, and a corresponding "patch" for the server-side, for testing. I wonder if an approach like this would be acceptable: https://github.com/jhump/protoreflect/blob/v2/grpcreflect/client_test.go#L548
(Though maybe the other way around, where it uses the generated v1 types and adapts v1alpha to that, instead of vice versa.)