Bugfix: rest path (URI) was always "/" even if a path is provided

[f-froehlich]

No description provided.

[ThomasWaldmann]

The purpose of get_rest_backend(base_url) is to return a REST backend (== a REST client).

The path isn't used for that, so it could be "/" (this is what the code expects) or empty (that would be another option).

Later, when using that backend (== calling the backend api methods), URLs will be constructed by joining the base_url with the storage item path (argument of api calls).

[f-froehlich]

Then there must be something wrong elsewhere. If I call:

borg -r http://USER:PWD@localhost:8001/repos/borg/MyRepo/ repo-create --encryption=repokey-aes-ocb

There is a request made "POST /?cmd=create HTTP/1.1" 404 7032 witch will fail because the server is listening on the path /repos/borg/MyRepo/ so the repo-create failed. The correct request in this case must be /repos/borg/MyRepo/?cmd=create.

The provided change fixed this issue.

[ThomasWaldmann]

First setup the REST server, you give the path to the store there:

python3 -m borgstore.server.rest --host 127.0.0.1 --port 8001 \
        --username USER --password PWD \
        --backend file:///tmp/teststore

Then use:

borg -r http://USER:PWD@localhost:8001/ repo-create --encryption=repokey-aes-ocb

[f-froehlich]

Yes I know that but the route / could be in use by another application or not proxied by a reverse proxy to the borgstore server application. Therefore it is not the right way to enforce the repo-create (and other operations) to use the uri /. Doing so is neither best practice nor the expected behavior.

Another example would be use different backends on one webserver so the repository could be at localhost:8001/FirstHost/RepoN and localhost:8001/SecondHost/RepoN where FirstHost could use FS backend and SecondHost S3 Backend and so on.

[ThomasWaldmann]

Well, the REST server as it is now only deals with forwarding the requests to 1 backend and it gives the PATH it gets in a request directly to the backend, so there must not be any path prefix.

[f-froehlich]

Yes but you can spin up 200 instances of the borgstore server on one host on a different port and use a reverse proxy to use the right one. However this wont work if a repo-create (and other requests) always uses path /. So I need 200 different vhost configs with tls setup (and in my case also 200 IPv6 ips) in order to get it running. This is overkill for a simple regex change. The current behavior doesn't chang so if I already use a borgstore instance on / it will be running on / even with this change

[ThomasWaldmann]

OK, so, would your reverse proxy setup remove the base path from the request it does to the borgstore REST server, so that it only sees the correct path for the store, without the base path?

[ThomasWaldmann]

Guess we need better docs for this. The REST server setup could be split off in a separate SERVER.rst that explains a typical (e.g. nginx) reverse proxy setup with the borgstore REST server.

[f-froehlich]

This would be one option to do so. However I'm trying to get it working with a custom REST backend implementation witch is capable of handling multiple repositories while only using a single django rest application

[ThomasWaldmann]

Ah, ok, so you are not using the borgstore rest server.

How about the trailing slash? In your PR, there now must be a trailing slash when using the root path, but it is not required when using a path below that. Guess it should be either always be optional (and normalized by the code) or it should be always required?

[f-froehlich]

In my tests if I don't add the / at the end of the url it uses the PosixFS backend instead. This is also the case without my changes

[ThomasWaldmann]

Yes, that is because borg requires the trailing slash for the rest backend. Also, the posixfs regex catches a lot of stuff because filenames on posix can be almost anything.

OK, so for consistency: always require the trailing slash here also?

OR: not require it (and also not require it in borg)

[f-froehlich]

You only need a / after the domain/port so http://localhost:8001/, http://localhost:8001/foo/bar/ and http://localhost:8001/foo/bar works

[ThomasWaldmann]

my question refers to the desired "how it should be" state, not necessarily to the current state.

[ThomasWaldmann]

Note: making the path group optional will result in path == None if no path is given.

Is it required to be optional?

[f-froehlich]

If you're having a decision I can change the regex if needed.

[ThomasWaldmann]

How about this?

-    # note: path component must be "/" (no sub-path allowed, as it would silently prepend to all item names)
+    # note: borgstore.server.rest does not support sub-paths, but sub-paths are
+    # supported in the rest client for use with reverse-proxied or custom REST servers.
     http_regex = r"""
         (?P<scheme>http|https)://
         ((?P<username>[^:]+):(?P<password>[^@]+)@)?
         (?P<host>[^:/]+)(:(?P<port>\d+))?
-        (?P<path>/)
+        (?P<path>/[^?#]*)?
     """
     m = re.match(http_regex, base_url, re.VERBOSE)
     if m:
         scheme = m.group("scheme")
         host = m.group("host")
         port = m.group("port")
-        path = m.group("path")
+        path = m.group("path") or ""

Also, some tests whether the URLs are built correctly would be nice (no path, /, /sub, /sub/).

[ThomasWaldmann]

Superceded by #156 - I needed it there to get the tests for the CI nginx reverse proxy setup with sub-path dispatching working.

@f-froehlich Thanks for bringing this up, this was an important change.

It looks like nginx can also do that sub-path -> / transformation, so that one borgstore server process still only has to deal with 1 store and item paths directly below the root path.

I found that the code can happily live without any trailing slash, because it strips and re-adds it anyway when constructing URLs. So making the path optional was a good idea, so users stumble less over "bad" URLs. It just needed that or "" so that the repr does have None.