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

Support for grpc.callback APIs #171

Merged
merged 8 commits into from
Apr 13, 2020
Merged

Support for grpc.callback APIs #171

merged 8 commits into from
Apr 13, 2020

Conversation

haiodo
Copy link
Contributor

@haiodo haiodo commented Mar 30, 2020

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.

image

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

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>
@haiodo haiodo requested review from edwarnicke and fkautz March 30, 2020 14:14
Signed-off-by: Andrey Sobolev <haiodo@gmail.com>
@edwarnicke
Copy link
Member

@denis-tingaikin denis-tingaikin added the enhancement New feature or request label Mar 31, 2020
haiodo added 2 commits April 2, 2020 23:46
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>
haiodo added 4 commits April 7, 2020 19:21
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())
Copy link
Member

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()?

Copy link
Member

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 :)

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

@edwarnicke edwarnicke merged commit ba2b816 into networkservicemesh:master Apr 13, 2020
nsmbot pushed a commit that referenced this pull request Apr 24, 2024
…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>
denis-tingaikin pushed a commit that referenced this pull request Apr 25, 2024
…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>
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

Successfully merging this pull request may close these issues.

3 participants