binocarlos
commented
9 years ago Hey,
I think we can separate the routing issue from the format of the body issue. IMHO - the URL of the request should be the URL that docker was hit with. Doing HTTP routing (i.e. is this a /containers/create or a /containers/:id/start) on the actual request will vastly improve the experience of writing adapters.
Presently - I'm writing a bunch of RegExp's where I could be using the vast array of HTTP routers from npm (insert preferred package manager here). Also - the URL the adapter is hit with is currently arbitrary and so feels like an unexploited resource.
The url could be appended to something like '/adapter' providing it contains the docker route in there somewhere - this enables an adapter to have other endpoints like '/status' etc (as pointed out by @progrium)
I think 'Version' and 'Type' belong in the headers (imho) - regardless of the structure of the request body - these things feel like meta data alongside Content-type and similar values.
In terms of the request body - this is a hard one - I'm not liking the bodies as strings thing at the moment - although it seems trivial it has already caused me pain a few times and adds a layer of thinking (do I need to stringify this or parse this etc). Multi-part feels harder - although there is lots of library support for it. However, if multi-part enables us to stream the body through the adapter (see footnote) - I'm all for it.
Perhaps the field names could become just 'request' and 'response'? These are the 2 core properties of the JSON body - 'ClientRequest' and 'ModifiedClientRequest' seem superfluous because sometimes I am writing 'ModifiedClientRequest' even when it's not been modified - again, somewhat trivial but it all adds to the mental load.
Summary:
- use the docker URL as the URL that hits the adapter (optionally appended to '/adapter')
- move meta (Version, Type) into headers
- simplify JSON field names 'ClientRequest' & 'ModifiedClientRequest' become just 'request'
footnote
It would be 100% great to be able to do this:
$ cat myfile.txt | docker run -ti --rm transformer --uppercaseAnd for 'somehow' an adapter (either pre or post) to be able to transform the body but AS A STREAM.
For example - I could override the 'uppercase' in the post-hook and make all data on stdin become lowercase and print it to stdout.
Another example - stream the output of a database backup container and send it off to S3 as well as pipe it to stdout on the terminal (like a unix tee adapter).
This opens up a world where powerstrip adapters are working on large data sets that are streamed via the proxy rather than gathered into one massive JSON packet. I cannot see how this would work in the context of having the request body as a JSON packet above and so might need considering.
progrium
robhaswell
@progrium @binocarlos @robhaswell
We discussed via email the pros and cons of wrapping the adapter protocol bodies etc in JSON versus having unwrapped request/response bodies and using HTTP headers to communicate metadata about the request/response being proxied/modified.
I suggest we use use this issue to openly discuss the pros and cons of these approaches and try to drive to a conclusion for e.g. whether we should work on building an unwrapped v2 protocol.
The main argument in favour of keeping the current wrapped approach is flexibility in terms of encapsulating multiple bodies within one response. For example the post-hook can currently receive both the original client request and the Docker response, so that it can see both what the client asked for as well as what Docker said. (Knowing what the client asked for isn't information that's available via a sub-request to the Docker API, probably.)
The main argument against seems to be that clients end up having to write their own routing logic for handling e.g. request URIs. Since I appear to be biased towards our original design, please add other arguments against on this thread. :)
One idea that may or may not carry any weight is that we might not always want to communicate with an adapter via HTTP. Having a pure JSON protocol means that we're not tied to HTTP-level constructs if, say, we wanted to have an option for communication with an adapter to occur via, e.g. stdin/out of a process exec or in the future something like libchan.
Another idea in favour of keeping what we've got now is that we want to encourage people to build powerstrip adapters. Incompatibly changing the protocol will add a burden to anyone who builds a powerstrip adapter now and then has to change it later. I don't think this argument alone is strong enough to mean that we shouldn't change the protocol (it's v0.0.1 for a reason) but I do think it raises the bar for having good reasons to change it if we're going to.
Finally, encoding this data in JSON means we can nest data structures, which is harder with an HTTP header approach (unless we put JSON in the headers, which feels weird to me). So for example we can have one data type for "requests" and one for "responses", then we can put as many of these objects in a JSON structure. For example, in future we may wish to extend the protocol so that an adapter hook in a chain can see all the previous request/responses that happened to the request before it got called in the chain as a JSON list of request/response objects.