gRPC: Enable per-service interceptor registration

[michalszynkiewicz]

Description

ATM we can only register server interceptors globally.
We do that by creating a CDI bean that implements ServerInterceptor.

I can imagine that someone may e.g. want to have one of the service secured and another one publicly available.
And interceptors are a good fit for security in gRPC.

Implementation ideas

I think this would mean that we have to introduce two annotations.
One for global interceptors, so it would be sth like:

@GrpcGlobalInterceptor
public class MyInterceptor implements ServerInterceptor {
}

And another one for per-service interceptors:

@GrpcRegisterInterceptor(MyOtherInterceptor.class) 
@GrpcService
public class HelloService implements Greeter {
...
}

[quarkus-bot]

/cc @cescoffier

[michalszynkiewicz]

@mkouba WDYT?

[mkouba]

Sounds good. But introducing the @GlobalInterceptor annotation would break compatibility, right? Because currently all beans implementing io.grpc.ServerInterceptor are "global interceptors". We could introduce @RegistrableInterceptor instead (or some other name along these lines ;-), to mark an interceptor that must be registered via @RegisterInterceptor. Also it might make sense to be able to register a per-service-method interceptor...

[mkouba]

I don't like the names though... how about @WithInterceptor to align with io.grpc.stub.AbstractStub.withInterceptors(ClientInterceptor...)?

[michalszynkiewicz]

AFAIR, gRPC doesn't allow per-service-method.
Yes it would be a breaking change.

[mkouba]

AFAIR, gRPC doesn't allow per-service-method.

You can create a new stub via withInterceptors() just for the specific method, or?

Yes it would be a breaking change.

-1 We already broke some stuff in the recent versions of the grpc extension..

[michalszynkiewicz]

JAX-RS has @RegisterProvider that works for filters, etc. I thought @RegisterInterceptor or @RegisterGrpcInterceptor (or @GrpcRegisterInterceptor) would give similar feel

[michalszynkiewicz]

AFAIR, gRPC doesn't allow per-service-method.

You can create a new stub via withInterceptors() just for the specific method, or?

Yes it would be a breaking change.

-1 We already broke some stuff in the recent versions of the grpc extension..

and look how much better UX of Quarkus gRPC is now :) We could have a switch to keep auto-registration. But annotation to disable global registration doesn't sound nice to me.

[mkouba]

Do you mean org.eclipse.microprofile.rest.client.annotation.RegisterProvider? That's a client-side annotation, right?

[michalszynkiewicz]

oh, you are right.

However, there registering global filters is opt-in, you need to add @Provider on them.

[mkouba]

and look how much better UX of Quarkus gRPC is now :)

+100 :-)

We could have a switch to keep auto-registration. But annotation to disable global registration doesn't sound nice to me.

It's definitely not nice...

However, there registering global filters is opt-in, you need to add @Provider on them.

Maybe we could detect interceptors that are NOT used on any service and are NOT annotated with @GlobalInterceptor and fail the build/log a big warning?

[mkouba]

BTW we could reuse the @javax.annotation.Priority approach from CDI interceptors, i.e. @Priority enables the interceptor globally... Hm, but it's probably not a good idea ;-).

[michalszynkiewicz]

and look how much better UX of Quarkus gRPC is now :)

+100 :-)

We could have a switch to keep auto-registration. But annotation to disable global registration doesn't sound nice to me.

It's definitely not nice...

However, there registering global filters is opt-in, you need to add @Provider on them.

Maybe we could detect interceptors that are NOT used on any service and are NOT annotated with @GlobalInterceptor and fail the build/log a big warning?

I like this idea. I think I'd leave it a warning though so that people who add dependencies with grpc interceptors (that are not used) don't end up with failing builds.

Re @Priority - with this we wouldn't be able to order per-service interceptors, right?

[mkouba]

I like this idea. I think I'd leave it a warning though so that people who add dependencies with grpc interceptors (that are not used) don't end up with failing builds.

+1

Re @Priority - with this we wouldn't be able to order per-service interceptors, right?

It could still implement Prioritized, or? But I don't think it was a good idea ;-)