-
Notifications
You must be signed in to change notification settings - Fork 36
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
Support for grpc.callback APIs #171
Conversation
Support for grpc.callback APIs using bi-directionsl grpc streams. Serve GRPC via one client to server connection. Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Add test with TLS and identity passed with TLS using peer.Peer Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Review fixes Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
Fix race data concurrency Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
target := "callback:{client-authority}" | ||
// If target is not in a list of callback server targets, | ||
// it will perform grpc.DialContext to connect to passed target | ||
nsmClientGRPC, err := grpc.DialContext(context.Background(), target, callback.WithCallbackDialer(server.callbackServer, target), grpc.WithInsecure()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require grpc.WithInsecure()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to put it differently... can I use grpc.Peer from within the callback client? It would be quite handy for the client using the callback to be able to get the correct AuthInfo for its peer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without insecure, it will be grpc error showing it require authentication, SSL could be used for this connection as well, but since it is passed using already authenticated connection I suppose better to not use it. It require individual grpc server on client to handle callbacks from particular server, so not sure if additional authentication or identification is required. Potentially we could enhance solution and have just one grpc for callbacks on client and connections to different servers, but current implementation doesn't support so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to be able to get peer information for clients to properly compute tokens... is there any way to have this properly set the peer information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only found a way to retrieve client certificate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK... looking at the code more closely... this will in fact correctly handle the case of using the TLS in TLS if need be... or some simpler workarounds for Token Generation.
…i@main PR link: networkservicemesh/api#171 Commit: d0df988 Author: Nikita Skrynnik Date: 2024-04-25 04:04:52 +0700 Message: - Create releases with Github CLI (#171) * Create releases with Github CLI Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * use reusable github actions for release and release-dependent-repositories jobs Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * fix yaml linter issues Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * remove get-tag job Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> --------- Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…i@main (#1614) PR link: networkservicemesh/api#171 Commit: d0df988 Author: Nikita Skrynnik Date: 2024-04-25 04:04:52 +0700 Message: - Create releases with Github CLI (#171) * Create releases with Github CLI * use reusable github actions for release and release-dependent-repositories jobs * fix yaml linter issues * remove get-tag job --------- Signed-off-by: NSMBot <nsmbot@networkservicmesh.io> Co-authored-by: NSMBot <nsmbot@networkservicmesh.io>
Support for grpc.callback APIs using bi-directionsl grpc streams.
Serve GRPC via one client to server connection.
So it could be used to perform a server callback to client using one client connection GRPC server.
SPEC why it is importsnt is here: https://docs.google.com/document/d/1Lmlcg_qEOQDLdNMCKUL_K-rM2c2lK2lxwKCbPbscCRg/edit?usp=sharing
Signed-off-by: Andrey Sobolev haiodo@gmail.com