cmd/protoc-gen-go-grpc: export consts for full method names

[KenxinKun]

Original issue: #5876

Changes:

• The principal change is an update to the cmd/protoc-gen-go-grpc/grpc.go command to export additional symbols for all full method paths. Existing behaviour is untouched beyond the additional symbols.
• Updated generated protos to include new symbols throughout the repo for all *.pb.go

Remaining questions

• Assuming the proposed change is acceptable, should I put it behind a command option or leave it on by default?

All feedback welcome! :)

RELEASE NOTES: N/A

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: KenxinKun / name: Borja Lazaro Toralles (5d4246d, c08b3f5, 8971c5e, 8ddfba1, 344b4a7)

[easwars]

We looked at the generated code in other languages, and none of them seem to be export constants for full method names. So, we are inclined towards not making this change in Go alone.

I remember you mentioned that you are trying to implement an authz plugin and hence need constants for full method names. gRPC does have an authz implementation. Did you take a look at it? Does it not meet your needs? If so, why and is that something that we can discuss. Thanks.

[KenxinKun]

Hi @easwars thanks for taking the time to review! I understand the wish to try and keep exported symbols consistent across plugins/languages, although I'd like to first see if we agree on the utility of the proposed symbols.

I'm happy to look into the existing authz package and discuss, I was not aware of it. Are you referring to this experimental package (https://pkg.go.dev/google.golang.org/grpc@v1.51.0/authz)? Or someting else?

If it is the one above, I've just looked into the code/tests, and focusing on the StaticInterceptor I see it's based on an authPolicy string to set it up. First thoughts are:

• I'm happy that the input policy matches the v3rbacpb.RBAC types, so it can be constructed with type safety (as shown in the tests) 😃
• I note however, that we still need to specify the rule.request.paths (similar to my example). In this case, to create a policy I would still need to hardcode the string "/helloworld.Greeter/SayHello" instead of just using the proposed new symbol Greeter_SayHello_FullMethod (maybe better renamed to Greeter_SayHello_Path instead).
  • This is the problem I'm trying to solve with this PR!
  • My example can be replaced to use the existing authz package, I wish I had noticed it earlier, I didn't mean to distract from the main point.
  • The main utility for the new proposed symbols is to avoid bugs arising from mistyping the paths in the policies, specially if the proto rpc definitions change in the future.

What do you think?

PS: I've gone ahead and replaced the example to just use the authz package

[easwars]

The main utility for the new proposed symbols is to avoid bugs arising from mistyping the paths in the policies, specially if the proto rpc definitions change in the future.

Changing method names in the proto is a breaking API change for the service being exposed and I feel that we shouldn't be trying to solve that problem.

As far as typos are concerned, can't these be handled with some unit tests? If the policy is maintained as a file (and version controlled), one could add some pre-submit and post-submit tests to ensure that the policy file is sane. What do you think about this approach?

I will bring this topic up with the greater gRPC team in the new year (once we have a quorum) and will provide an update here.

[KenxinKun]

Hi @easwars happy new year :) thanks for bringing this up with the team.
In terms of your suggestion, wouldn't that unit test be really be testing that the grpc-go generation didn't change the way its internals work? :) If the symbol is available then there's a contractual consistency between the plugin and the consumer, whilst if hardcoding the string the user is having to rely on no breaking changes happening in the plugin, plus having to examine how it works internally, right? That's on top of the typos issue I mentioned of course.

[easwars]

@KenxinKun : I have just initiated a discussion with the greater gRPC team about this. Will update when I have something to share.

In the meantime, we do have an open issue to add an example for the authz pacakge. See: #5900. Do you mind splitting the example you have here as a separate PR and sending that our for review. We will be happy to review that in the mean time. Thanks.

[easwars]

LGTM.

[KenxinKun]

@dfawley @easwars just applied the last 2 suggestions :)

[dfawley]

Thank you for the changes!

[easwars]

Thank you very much for your contribution !!

[KenxinKun]

Thanks for the feedback and the sweet merge!