Use JSON-RPC over HTTP for communication between plugins #297
Use JSON-RPC over HTTP for communication between plugins #297
Conversation
Signed-off-by: Bill Farner <bill@docker.com>
Signed-off-by: Bill Farner <bill@docker.com>
acdb3e7
to
99b03c7
Compare
Please sign your commits following these rules: $ git clone -b "gorilla-rpc-with-logging" git@github.com:wfarner/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353945600
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
Current coverage is 70.84% (diff: 84.25%)@@ master #297 diff @@
==========================================
Files 25 25
Lines 1303 1228 -75
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 905 870 -35
+ Misses 321 285 -36
+ Partials 77 73 -4
|
if err != nil { | ||
return err | ||
} | ||
flavorPlugin = flavor_plugin.NewClient(endpoint.Address) |
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.
Why is the protocol being removed? We need to support named pipes per #285 and this change here will have to be undone.
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'm uneasy about half-supported features. Assuming that #285 is O(months) out before we break ground, i find the added clarity of support and simplicity welcome in the meantime.
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.
That said, i don't feel strongly. So if you feel strongly about keeping the protocol plumbing on this end, i can revert.
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.
Let's just get this in. Some of this may have to be reworked for Windows support but for now it's fine.
Server: &http.Server{Addr: fmt.Sprintf("unix://%s", socketPath), Handler: handler}, | ||
} | ||
|
||
listener, err := net.Listen("unix", socketPath) |
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.
How is this going to work for #285? Now that we are moving this code around, we should make accommodations for the protocol string.
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'm of the opinion that #285 will require a concerted effort that would not be mitigated by accepting a protocol string piecemeal. I don't see much benefit to involving support for alternate protocols in this patch.
edit: to rephrase, this patch doesn't really set us back from support for alternate protocols, and i don't think we should block this patch for also not moving us forward on support for alternate protocols.
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.
It won't take O(months). I'm planning on using https://github.com/natefinch/npipe and it has the same convention as net.Listen()
. However, I will need the protocol to be passed in so that we can implement a switch.
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.
LGTM - this should make logging much better now.
…#297) Signed-off-by: Ken Cochrane <KenCochrane@gmail.com>
While looking into adding request/response logging in #269, i realized that plugin communications were using raw socket I/O rather than HTTP. This patch uses JSON-RPC over HTTP, which i suspect will be easier for others to implement and test with conventional tools. Specifically, this uses
github.com/gorilla/rpc
. I chose this overnet/rpc
andnet/rpc/jsonrpc
to favor traditional HTTP request-oriented behavior.net/rpc
can operate over HTTP, but only as a starting point after which it hijacks connections and no longer uses HTTP requests (similar to websocket behavior).I've elected to use
gopkg.in/tylerb/graceful.v1
for handling graceful shutdown of the HTTP server, which simplified lifecycle management.In addition, this patch adds HTTP request logging at the debug log level. I will follow up to refine the log output, as it currently does not log the request body.
The patch comes in 2 commits - one with the code changes, and one with vendor updates in the course of adding dependencies.
Signed-off-by: Bill Farner bill@docker.com