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

Channel level interceptors #1148

Open
jongarate opened this issue Mar 15, 2021 · 12 comments
Open

Channel level interceptors #1148

jongarate opened this issue Mar 15, 2021 · 12 comments

Comments

@jongarate
Copy link

Description

The current interceptor implementation sits on top of each service client. This forces the implementation on the client side of the gRPC service to arrange a factory with an interceptor instance per available service method.

When the backend supports multiple services (which is usually the case), forces the development on the client side to define interceptors per-service/per-method, resulting in a lot of boilerplate code and cumbersomeness.

Suggested solution

Just like in Android, it would be helpful to have an option to define interceptors at the Channel level, so that at least basic exchange data can be logged/observed (e.g. Headers, etc...) by hooking to a single point in the whole architecture.

@glbrntt
Copy link
Collaborator

glbrntt commented Mar 15, 2021

This is quite cumbersome at the moment and worth improving.

Adding it at the channel-level has a few difficulties, mainly that it's not easy to resolve which interceptors to use when they exist at the channel level and in the interceptor factory. Should the interceptors from the factory override or be added to those defined on the channel?

Another approach might be to change the generated interceptor factory: it could provide a generic factory method for 'default' interceptors and default the implementation of each factory method to call the default. That way you'd only have to implement one method per service and you retain full control over configuring interceptors on a per-method basis.

@jongarate
Copy link
Author

To my view, channel-level interceptors would sit higher architecturally speaking than the method specific ones, which could be further extended in more detail in the method-level ones if found necessary. They should essentially coexist as their scope and purpose is quite different.

Providing a generic factory for default interceptors would certainly be an improvement, and I guess more feasible in the short run. However, it would still require attaching them to each client explicitly, which hurts maintainability in the long run.

@VorobyevS
Copy link

For base factory, temp solution. Resolving unnecessary recreations for same requests. U may improve it for your self.

typealias InterceptorEntity = (SwiftProtobuf.Message
                               & SwiftProtobuf._MessageImplementationBase
                               & SwiftProtobuf._ProtoNameProviding)

class GPRCBaseInterceptorFactory {
    private final class InterceptorsLocator {
        private var interceptors = [String: [Any]]()
        
        func register<T: InterceptorEntity, D: InterceptorEntity>(_ interceptors: [ClientInterceptor<T, D>]) {
            let description = String(describing: T.self) + String(describing: D.self)
            self.interceptors[description] = interceptors
        }
        
        func resolve<T: InterceptorEntity, D: InterceptorEntity>() -> [ClientInterceptor<T, D>]? {
            let description = String(describing: T.self) + String(describing: D.self)
            return interceptors[description] as? [ClientInterceptor<T, D>]
        }
    }

    
    //MARK: Internal properties
    private static let locator = InterceptorsLocator()
    private let creators: [InterceptorCreator]
    
    //MARK: Initialization
    required init(creators: [InterceptorCreator]) {
        self.creators = creators
    }
    
    func getInterceptors<T: InterceptorEntity, D: InterceptorEntity>() -> [ClientInterceptor<T, D>] {
        guard let interceptors: [ClientInterceptor<T, D>]  = Self.locator.resolve() else {
            let newInterceptors: [ClientInterceptor<T, D>] = creators.map { $0.create() }
            Self.locator.register(newInterceptors)
            return getInterceptors()
        }
        return interceptors
    }
}

@jongarate
Copy link
Author

Still, as I mentioned, this requires feeding the interceptor manually for each service's client method, which is what the whole point of discussion is about.

@lacasaprivata2
Copy link

lacasaprivata2 commented Jul 5, 2021

thankfully, there are plenty of pre-existing examples from java-grpc to tonic (rust-grpc) on how to architect this. This would be huge currently have a section at the bottom of each file I copy & past authentication interceptors into. It's actually quite shocking this hasn't been a more popular issue given the universality of authentication.

@XabierGoros
Copy link

Indeed it would be very helpful for everyone.

@bartekpacia
Copy link

I'm so surprised that this isn't implemented.

@Jerry-Carter
Copy link

Just ran across this issue on the server side. In several situations, registering an interceptors at the Channel level is strongly preferred to adding interceptors for every single method. A single registration point will reduce both developer errors and reduces the testing burden.

Three use cases that come to mind:

  • Enhanced error logging. Append the origin address to the request headers for function level error logging. This information is not in the HPACKHeaders.
  • Blacklist. Compare incoming request addresses against a blacklist to block known malicious requests. [There are alternative mechanisms for implementing this, but they add architectural complexity which may not be desirable.]
  • Authentication / Authorization. Though not my preferred mechanism, a channel interceptor would allow authorization / authentication checks to be centralized. In my hypothetical implementation, I'd use a channel interceptor to verify that a malicious request didn't attempt to set authorization requirements. Then I'd use a function interceptor to specify the access requirements. Then I'd use a channel interceptor to perform the authentication / authorization check.

@cassianomonteiro
Copy link

+1

@talksik
Copy link

talksik commented Dec 10, 2023

For a newbie in swift, how do I get this working? I was able to do this simply in Dart, but can't find a way to do it with swift.

I read through various related issues in this repo regarding this. In current state, do I have to create the boilerplate for every single method?

Thank you 🙏🏽

Edit: found the partial solution here: #1567 (comment). Reduces boilerplate at least a little bit.

@glbrntt
Copy link
Collaborator

glbrntt commented Dec 11, 2023

For a newbie in swift, how do I get this working? I was able to do this simply in Dart, but can't find a way to do it with swift.

I read through various related issues in this repo regarding this. In current state, do I have to create the boilerplate for every single method?

You do, yes. This will be addressed in the next major version.

@talksik
Copy link

talksik commented Dec 11, 2023

Thank you @glbrntt !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants