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

Can't detect MethodOptions #404

Closed
majst01 opened this issue Nov 27, 2022 · 9 comments
Closed

Can't detect MethodOptions #404

majst01 opened this issue Nov 27, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@majst01
Copy link

majst01 commented Nov 27, 2022

I am currently in the process of migrating an existing grpc api server to connect-go. Went smoothly so far, great work !

I am actually stuck at one point, in the current implementation i detect the specified MethodOptions per service and construct ACL Rules from them.

Example Proto

service HealthService {
  rpc Get(HealthServiceGetRequest) returns (HealthServiceGetResponse) {
    option (visibility) = VISIBILITY_PUBLIC;
  }
}

Then i use @jhump "github.com/jhump/protoreflect/grpcreflect" to load the ServiceDescriptors and iterate through.
This requires a grpc.Server instance which is not available anymore.

Any hints howto get access to the MethodOptions with a connect-go implementation are welcome.

@majst01 majst01 added the enhancement New feature or request label Nov 27, 2022
@jhump
Copy link
Member

jhump commented Nov 27, 2022

Hi, @majst01. Unfortunately, the grpcreflect package is written to use the gRPC Go runtime API. So it will not be helpful when using Connect. But there is luckily a way to do what you are trying to do, now that reflection/descriptor support has been added to the core Protobuf Go runtime API.

A Connect server has no abstraction like a "server" or "service registrar". Instead it just generates HTTP handlers that you add to your own http.Server instance. So you'll have to track the registered services yourself, which for now (unfortunately) means putting hard-coded string literals for the fully-qualified service names into your program.

For a particular service, you can get the service's or methods' options via this code:

// import "google.golang.org/protobuf/reflect/protoreflect"
// import "google.golang.org/protobuf/reflect/protoregistry"
// import "google.golang.org/protobuf/types/descriptorpb"

descriptor, err := protoregistry.GlobalFiles.FindDescriptorByName("acme.user.v1.UserService")
if err != nil { return err }
userSvcDesc := descriptor.(protoreflect.ServiceDescriptor)

// inspect service options
userSvcOpts := userSvcDesc.Options().(*descriptorpb.ServiceOptions)

// inspect method options
for i := 0; i < userSvcDesc.Methods().Len(); i++ {
    methodDesc := userSvcDesc.Methods().Get(i)
    methodOpts := methodDesc.Options().(*descriptorpb.MethodOptions)
}

An improvement in the generated code for Connect would be to add exported constants with these service names. That way you don't have to hard-code the strings in your program; if the service name is changed, the program is not broken (though that is not a backwards-compatible change), and if the service is moved to a different package, there is a compile error instead of runtime error (though, again, that is not a backwards-compatible change). This would also improve the use of server reflection, which also currently requires hard-coding service names.

@majst01
Copy link
Author

majst01 commented Nov 27, 2022

Hi @jhump thanks for your explanation,

meanwhile i managed to implement it somehow with the help of github.com/jhump/protoreflect/desc/protoparse.
With that i was not able to get the MethodOptions in in type-safe way.

I will try your proposed way if i am able to get the types of the MethodOptions this way.

I tried this:

visibilities := proto.GetExtension(methodOpts, v1.E_Visibility).([]v1.Visibility)

But it always complained that methodOpts is expected to be of type Uninterpreted, but it was ?

@akshayjshah
Copy link
Member

An improvement in the generated code for Connect would be to add exported constants with these service names.

There are already generated constants for this! No need for hard-coded string literals anywhere.

@jhump
Copy link
Member

jhump commented Nov 28, 2022

There are already generated constants for this! No need for hard-coded string literals anywhere.

Oh, nice! Now that I re-read the README in connect-grpcreflect-go, I see there's a comment about it. So I just needed to read it more thoroughly. Mea culpa.

@akshayjshah
Copy link
Member

The approach that @jhump suggests works and is the best approach to this problem. I'm going to close this - @majst01, please ping this issue if you're still having trouble.

@majst01
Copy link
Author

majst01 commented Dec 8, 2022

Thanks for the support.

@solarhell
Copy link

solarhell commented Jun 5, 2024

in my case, i use the following code to get it

syntax = "proto3";

package permission.v1;

import "google/protobuf/descriptor.proto";

extend google.protobuf.MethodOptions {
  string permission = 50001;
}
package middleware

import (
	permissionv1 "permission/v1"
	"connectrpc.com/connect"
)

func NewUserAuthInterceptor() connect.UnaryInterceptorFunc {
	interceptor := func(next connect.UnaryFunc) connect.UnaryFunc {
		return func(
			ctx context.Context,
			req connect.AnyRequest,
		) (connect.AnyResponse, error) {
			methodDesc, ok := req.Spec().Schema.(protoreflect.MethodDescriptor)
			if !ok {
				return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("expected protoreflect.MethodDescriptor got %T", req.Spec().Schema))
			}
			var adminRequired bool

			ext, ok := proto.GetExtension(methodDesc.Options(), permissionv1.E_Permission).(string)
			if ok && ext == "admin" {
				adminRequired = true
			}

			fmt.Println(adminRequired)

...

			return next(ctx, req)
		}
	}
	return interceptor
}

@jhump
Copy link
Member

jhump commented Jun 6, 2024

@solarhell, thanks for posting that. That is indeed the best way to do this now.

The Schema field that your code is using was introduced in v1.13.0 (about a year after this issue was closed). When using protoc-gen-connect-go (v1.13.0 or later), the Schema will be populated with a protoreflect.MethodDescriptor that describes the method being invoked.

@solarhell
Copy link

solarhell commented Jun 7, 2024

@jhump I can help add the complete code to the examples repo, as this usage is very common.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants