-
Notifications
You must be signed in to change notification settings - Fork 2
plugin v2 preview #10
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
ec70e6e
to
e6fc00f
Compare
@@ -0,0 +1,33 @@ | |||
syntax = "proto3"; |
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 haven't done much with grpc, what benefits do we have in comparison to using json for the message payload? Probably reduced payload size, but I don't think this has much impact here.
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 am still debating whether to use their streaming RPC ..
There are three architectures I am trying to decide between:
- The Server opens an "infra" server which accepts messages and storage and plugin opens a server each for server callbacks: totally transport agnostic, we can use HTTP or anything. I am currently trying this out but I might change as it requires two connections per plugin.
- Only the plugin starts a RPC Server, and the Gotify server "pulls" messages and configuarion changes in using client streaming. gRPC helps great with IO infrastructure work here.
- Same as above but Gotify server acts as the RPC server, but I am gearing towards letting plugin be the server because the plugin process may die and we have to handle that. It's easier to handle both process crash and client connection crash as one route (restart the process and try connecting again until giveup).
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.
Hashicorp plugin system uses the third option, and then we'd only need one unix socket/tcp connection for all plugins. I think I'd prefer this, as this seems simpler, but this is only a gut feeling (:.
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.
Hmm, I am not sure I read it the same, to me Hashicorp uses Option 2 to me:
There is this "plugin.Serve" which every plugin use in the main process, and it does spawn an gRPC server.
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.
Ahh okay, have misread the docs.
} | ||
|
||
func NewEphemeralTLSClient() (*EphemeralTLSClient, error) { | ||
caPub, caPriv, err := ed25519.GenerateKey(rand.Reader) |
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.
You previously linked the hashicorp plugin docs, they using mutual tls too and provide this plugin library https://github.com/hashicorp/go-plugin which has support for mtls and grpc. Have you considered using this? (just a question, not request to use this lib now)
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 took a quick peek and there are a couple things:
- They use the Go RPC not gRPC.. I think since our call interface is simple I don't see a reason to use an interface exclusive to Go.
- I think a key difficulty would be try to get HTTP reverse proxies working over the Server-to-Plugin connection and we might need abstraction breakouts here (raw gRPC has support for multiplexing over HTTP/2 listeners: https://pkg.go.dev/google.golang.org/grpc#Server.ServeHTTP which allows us to route
Webhooker
request in the same socket). Vault has their own abstractions over HTTP and they don't have this problem. - I don't see a process spawn hook: being a security oriented company they don't expose any loading panics, we might have to show stderr to the user for it to be usable for the general user.
I have an authentication plugin you can checkout the code pattern for: https://github.com/eternal-flame-AD/vault-auth-emerg-yubiotp
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.
👍, thanks for the explanation.
) | ||
|
||
func newTCPListener() (net.Listener, error) { | ||
listener, err := net.Listen("tcp", "[::1]:0") |
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.
Do we need this? Aren't unix sockets enough?
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 guess since we are going with RPC I want to make it work on Windows as well (and maybe as a fallback if for some reason we can't establish a unix socket on the tempdir)... Might not survive the final release, I will keep this codepath for now and see if it is too much work/security risk.
I am totally okay with removing this, we never promised plugins will work on windows anyways.
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.
Ahh yeah, windows, forgot about this (:. I'm okay with supporting this, tho we maybe wait until people want this feature, I can't imaging that many host their gotify server on windows. Even tho we have many windows exe downloads https://somsubhra.github.io/github-release-stats/?username=gotify&repository=server&page=1&per_page=5
very much WIP, expect big changes as I go.
Just sharing to get some early opinions.
TODO: