Enhancement: option to send HTTP over unix socket#4655
Enhancement: option to send HTTP over unix socket#4655MichaelEischer merged 3 commits intomasterfrom unknown repository
Conversation
There was a problem hiding this comment.
I strongly dislike adding a CLI option that then goes on to override the path passed to the backend. That just feels wrong. (It would also introduce multiple weird failure cases for other backends).
My suggestion is to use "github.com/peterbourgon/unixtransport" instead. With a call to unixtransport.Register(tr) it becomes possible to use rest:http+unix:///path/to/socket without requiring a messy CLI option. That adds a new (very small) dependency, but that's better than the alternative.
Thanks, that library looks good and that's a much cleaner outcome. Have made change as suggested. On my system I also needed to make a fix to that library (so that it will ignore HTTP proxies when invoked). We might want to wait to verify that patch is accepted (and version bumped) before merging this one: I've tested the above against the and it seems to work. |
MichaelEischer
left a comment
There was a problem hiding this comment.
The code is ok now, but I'd really like to see a small test, that connecting to a http+unix server does work. Otherwise, there's a too large chance that we accidentally break this feature.
Waiting for a fix to the proxy support is probably a good idea. And restic/rest-server#272 also has to be completed first.
|
Happy to add tests once the server-part lands. |
|
Rebased to fix conflicts, and have bumped the |
|
Integration test added and cleaned up regular HTTP based |
|
Rebased to fix |
add tests for unix socket connection switch HTTP rest-server test to use any free port allow rest-server test graceful shutdown opportunity
MichaelEischer
left a comment
There was a problem hiding this comment.
LGTM. I've fixed the test failures (we'll have to switch rest-server back to latest once the next version is released) and there's now a changelog. Please take a look at it, afterwards we should be ready to merge this PR.
|
Looks good. Thanks for fixing up and following this through. |
What does this PR change? What problem does it solve?
Add one method to send HTTP traffic (for any backend) over a unix socket.
I'm not sure this is the best way, but it certainly seemed an easy way without a large refactor.
Was the change previously discussed in an issue or on the forum?
Closes #4287
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.Happy to add tests but wanted feedback on approach first.
While adding syntax like
-r rest:unix:/tmp/socketlooked simple in the original issue, the way the code is structured it's difficult to get access to thehttp.Transportobject from within therestbackend. Hence I added a global option, which means this would also work for tunnelling other HTTP based backends over a socket if desired.Our intended use-case is in conjunction with the
rest-server(see restic/rest-server#272) where a central backup server essentially makes the following SSH calls out to hosts to back them up:rest-server --listen unix:/tmp/socket --data /path/to/data & scp restic host:/tmp ssh -R /tmp/socket:/tmp/socket host /tmp/restic -use-unix-socket-for-http /tmp/socket -r rest:http://ignored/repo(the
ignoredbit is ugly)We can do the above with ephermal ports today, but unix sockets feels like it might be cleaner for this-use case.