Issue sharing a gRPC service with DI.

[Euan-McVie]

Related to #120 (I've found why we were mapping the interface)
The reason was to allow inter service communication to not have to worry about it being a local or remote service and so we would be free to move services between hosts as required.

In order to do this we register the services with DI in the startup.cs, rather than have the gRPC service being a wrapper around another DI registered service. This then means that for the gRPC mapping to use the service we found that we had to use endpoints.MapGrpcService<ICalculator>(); so that the instance of ICalculator came from DI rather than it's own instance.

This works, but then means that the [Authorize] issue is back as although the instance is pulled from DI, the metadata is only pulled from the type that is mapped.

As long as the services are scoped/transient it isn't a big issue that they use a different instance, it is only if the service is a singleton that the problem becomes really apparent.

I have uploaded an example to my fork by extending your *_CS example.

• I extended the MyCalculator to store something within the instance.
• I updated MyTimeService to use it for the delay as an example of direct inter service use.
• In Startup.cs the MyCalculator has been registered as a singleton.
• By switching between the endpoint mappings you can see the behaviour switch:
  • Mapping to ICalculator works and both the client and server report the delay from the MyCalculator singleton.
  • Mapping to MyCalculator doesn't work as the client now gets a new MyCalculator instance per request.
• If you uncomment the [Authorize] on MyCalculator:
  • Mapping to ICalculator is NOT rejected when the client calls it.
  • Mapping to MyCalculator is rejected when the client calls it.

One of the great things about your lib was the ability for us to have the interfaces for remote and local services registered in DI so that the consumer did not need to worry about if it was local or remote, and how it removed a lot of boiler plate code. While we could create a "gRPC controller" wrapper that shares the interface, and just passes through to the real DI service it is a bit of boilerplate that would be good to avoid if we can.

Any feedback is appreciated.
Thanks
Euan

[mgravell]

The difficulty I see here is that if you are using <ICalculator>, then the library has no knowledge of MyCalculator, so indeed: it isn't going to look at MyCalculator to find attributes. It does look at the interface, but [Authorize] has AttributeUsage that limits it to class and method - it doesn't allow interface. Does moving the attribute to the individual interface methods have utility for you?

The metadata discovery is done at service bind time, not at per-instance execution time, so it never has the opportunity to look at the actual service object (you can see this in ServiceBindContext.GetMetadata(MethodInfo)).

[Euan-McVie]

Adding [Authorize] on the interface is a little messy for us as the interfaces are shared with net461 client/server code using so the Microsoft.AspNetCore.Authorization dependency would be great to avoid. (We have other lib/redirect issues that mean we multitarget rather than use netstandard where possible)

I've tried injecting a mapper to the netcore version that allows the bind time discovery of interface implementations.
Euan-McVie@8b9ca9c
Note the reason I didn't add to the binder was that I saw that netcore doesn't appear to have a binder override as part of AddCodeFirstGrpc and I just wanted to get it working as a POC.

Not sure if it's something you'd want to introduce (and I'm sure could be made tidier/more robust) but it does "work" for my scenario.
I suspect that barring some DI lookup solution, we'll probably have to switch to having a gRPC service proxy wrapping the real implementation, which is a little ugly/extra maintenance.

If you do want to introduce it somehow (either by default or an injection point to allow us to provide a custom implementation) then happy to raise an official PR with reworked code matching an approach you prefer.

Thanks

[mgravell]

The other option we could look at here is some mechanism for you to opt in to controlling metadata; consider, for example:

services.AddCodeFirstGrpc().WithServiceMetadata<IMyService, MyService>();

(this API doesn't exist; I'm just throwing it out there as a discussion piece)

The above (imaginary) API would tell it to also consider MyService when thinking about IMyService

[mgravell]

hmm; actually it looks like something like this already exists!

Can you try the following please?

endpoints.MapGrpcService<IMyCalculator>().WithMetadata(new AuthorizeAttribute());

if this works, we could perhaps add an extension method like:

endpoints.MapGrpcService<IMyCalculator>().WithMetadataFrom<MyCalculator>();

?

[Euan-McVie]

That works.

Adding it to the endpoints if possible would be great as we already have our endpoints.MapGrpcServices() extension that does the dynamic/reflection lookup that called the MapGrpcService previously with the interface, so it already has both the interface and service type pair, and can be hidden in our infrastructure so not forgettable (thus leaving endpoints unsecured accidentally).

While the example at the moment just has [Authorize] on the service, in future their is likely to be more complex policies applied to per class or per method so having a WithMetadataFrom that can merge in other metadata at all levels would be great, assuming it is possible for it to pick up the per method metadata?

For further background:
We have devs at various levels of .net (and .net core) experience and so have been trying to abstract away most of the boilerplate code so that it is harder for mistakes to be made.
i.e. until now all the devs needed to do to add a service to an existing host was to add a single
services.AddScoped<ICalculator, MyCalculator>(); to the startup.cs and that is it.

Currently the endpoints.MapGrpcServices() extension hides the other service specific mapping, so it would be great if we could hide in there didn't need to explicitly map them as part of an AddCodeFirstGrpc extension in addition to the normal services.AddScoped<ICalculator, MyCalculator>();. But if not then if the AddCodeFirstGrpc().WithServiceMetadata() (or GrpcServiceOptions property?) could take in a lazy evaluated Func<IEnumerable<(contract, service)>> then we can put in the

var serviceImplementationType = serviceCollection.SingleOrDefault(x => x.ServiceType == serviceType);
return (serviceImplementationType is null) ? serviceType : serviceImplementationType.ImplementationType;

as we know it is currently safe for our usage.

Thanks

[mgravell]

Given a concrete service type and an interface method, it should be possible to revolve the actual implementation to lookup the attributes. I'll see what I can find. As for where this happens in the call chain: I'll need to investigate the timing - sometimes these calls are deferred, sometimes they are immediate. This to timing impacts the API, as obviously the config needs to come before the actual service discovery.

…

On Fri, 7 Aug 2020, 16:55 Euan McVie, ***@***.***> wrote: That works. Adding it to the endpoints if possible would be great as we already have our endpoints.MapGrpcServices() extension that does the dynamic/reflection lookup that called the MapGrpcService previously with the interface, so it already has both the interface and service type pair, and can be hidden in our infrastructure so not forgettable (thus leaving endpoints unsecured accidentally). While the example at the moment just has [Authorize] on the service, in future their is likely to be more complex policies applied to per class or per method so having a WithMetadataFrom that can merge in other metadata at all levels would be great, assuming it is possible for it to pick up the per method metadata? For further background: We have devs at various levels of .net (and .net core) experience and so have been trying to abstract away most of the boilerplate code so that it is harder for mistakes to be made. i.e. until now all the devs needed to do to add a service to an existing host was to add a single services.AddScoped<ICalculator, MyCalculator>(); to the startup.cs and that is it. Currently the endpoints.MapGrpcServices() extension hides the other service specific mapping, so it would be great if we could hide in there didn't need to explicitly map them as part of an AddCodeFirstGrpc extension in addition to the normal services.AddScoped<ICalculator, MyCalculator>();. But if not then if the AddCodeFirstGrpc().WithServiceMetadata() could take in a lazy evaluated Func<IEnumerable<(contract, service)>> then we can put in the var serviceImplementationType = serviceCollection.SingleOrDefault(x => x.ServiceType == serviceType); return (serviceImplementationType is null) ? serviceType : serviceImplementationType.ImplementationType; Thanks — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHME3QZYDKDLTREMOTUTR7QPXPANCNFSM4PXP3OWQ> .

[Euan-McVie]

Thanks, greatly appreciated.

[Euan-McVie]

Hi,
Any update on approaching a fix to this issue?
I've poked around a bit further, but can't see how you would be easily be able to extend the endpoints mapping and get the code where you need it.
Going down the route of explicitly registering implementations like .WithServiceMetadata<IMyService, MyService>(); has a similar issue that at the moment you have avoided having much extra stuff on top of the Grpc.AspNetCore.

My immediate thinking would be to try something like

1. Adding to the ServiceBinder a protected virtual Type GetConcreteServiceType(Type serviceType) => serviceType;.
2. Updating to new ServiceBindContext(serviceContract, binderConfiguration.Binder.GetConcreteServiceType(serviceType), state) in the ServerBinder
3. We can then create our own binder that handles the IServiceCollection lookup that we are interested in.

public class ServiceCollectionServiceBinder : ServiceBinder
{
    private readonly IServiceCollection _serviceCollection;

    public ServiceCollectionServiceBinder(IServiceCollection serviceCollection)
    {
        _serviceCollection = serviceCollection;
    }

    public override Type GetConcreteServiceType(Type serviceType)
    {
        if (!serviceType.IsInterface)
            return serviceType;

        var serviceDescriptor = _serviceCollection.SingleOrDefault(x => x.ServiceType == serviceType);
        return (serviceDescriptor is null) ? serviceType : serviceDescriptor.ImplementationType;
    }
}

First 2 bits preserve the existing behaviour while enabling extensions.
Would this be an acceptable approach?

Note we are planning to release next month, and while it will be acceptable for us to use the workaround to enable Authorization for all service calls via the endpoint extension for this release, it would be great to have a more complete solution ready before we start on the next set of services where when we will need to start covering them with different policies.

[mgravell]

I haven't had time to look into it, indeed. As for the method: yes, something along those lines is probably fine, but I'd shy away from making it about the "concrete service type", because that isn't what we actually care about. What we're really doing here is qualifying the metadata. Perhaps what any such API should offer is:

protected virtual object[] GetMetadata(MethodInfo method) => method.GetCustomAttributes(inherit: true);
protected virtual object[] GetMetadata(Type type) => type.GetCustomAttributes(inherit: true);

and we change ServiceBindContext to call those methods rather than dealing with attributes locally. Then your own ServiceBinder can still do whatever it wants.

[Euan-McVie]

Hi @mgravell
Understand you likely have other priorities, but do you have a timeline for when you may be able to review the PR #130 .
Will help with our sprint/release planning if you can give an idea of turnaround.
Much appreciated
Thanks
Euan

[mgravell]

@Euan-McVie some feedback added; sorry for delay - life is ... weird (for everyone) right now