Skip to content
This repository has been archived by the owner on Jan 18, 2021. It is now read-only.

Expose TUS capabilities through capabilities API #177

Closed
PVince81 opened this issue May 6, 2020 · 24 comments · Fixed by #251
Closed

Expose TUS capabilities through capabilities API #177

PVince81 opened this issue May 6, 2020 · 24 comments · Fixed by #251
Assignees

Comments

@PVince81
Copy link
Contributor

PVince81 commented May 6, 2020

Additionally to the headers (Tus-Version, Tus-Resumable, Tus-Extension) that are specified in a response of a PROPFIND or HEAD on a folder, we should also expose the general support for TUS through the capabilities API, so that a client does not need to bother with TUS related logic if the server doesn't have TUS enabled at all.

@michaelstingl @butonic @felix-schwarz @TheOneRing

@PVince81
Copy link
Contributor Author

PVince81 commented May 6, 2020

Proposal for JSON structure, under

"capabilities": {
   "core": {
      ...
   },        
   "files": {
      "tusSupport": {
         "version": "1.0.0,0.4.2",
         "resumable": "1.0.0",
         "extension": "creation,creation-with-upload"           
      }
   },
   ...
}

The capability key name is the same as the header, stripped of "Tus-" and lowercased.

if we decide to also implement the "max chunk size" header as per tus/tus-resumable-upload-protocol#93, we should also include those in the capabilities.

@michaelstingl
Copy link

if we decide to also implement the "max chunk size" header as per tus/tus-resumable-upload-protocol#93, we should also include those in the capabilities.

Yeah, needed for the middleware/proxy scenarios, and clients could immediately go for creation-with-upload.

@PVince81
Copy link
Contributor Author

PVince81 commented May 6, 2020

note that the recommended chunk size could also be different depending on the storage.
so if we put it in the capabilities it would be the "default chunk size" in case a storage doesn't specify any.

@michaelstingl
Copy link

if we put it in the capabilities it would be the "default chunk size" in case a storage doesn't specify any.

yes, exactly

@felix-schwarz
Copy link

Talking about middleware and proxies… this would also be a good place to instruct clients to send X-HTTP-Method-Override headers (tus.io) - and which HTTP method to use for the actual HTTP request instead.

For example:

"capabilities": {
   "core": {
      ...
   },        
   "files": {
      "tusSupport": {
         "version": "1.0.0,0.4.2",
         "resumable": "1.0.0",
         "extension": "creation,creation-with-upload",
         "http-method-override": "POST"            
      }
   },
   ...
}

@michaelstingl
Copy link

Talking about middleware and proxies…

@felix-schwarz I mean real size limits, that clients can't detect. Independent which method. Nothing can be sent > 100 MB. See my comment in owncloud/product#19 (comment).

Nasty middleware example:
PUT > 100MB will fail:
https://demo.shniq.cloud/
Cloudflare limits upload size (HTTP POST request size) per plan type:
100MB Free and Pro
(Source: https://support.cloudflare.com/hc/en-us/articles/200172516#h_51422705-42d0-450d-8eb1-5321dcadb5bc)

Or check traefik example:
https://docs.traefik.io/middlewares/buffering/

Clients need to know max-size before 1st upload.

@felix-schwarz
Copy link

@michaelstingl I wasn't referring to size limits. Sorry if my comment lacked clarity and/or context here.

What I meant is that http-method-override would - independently of any other capabilities describing other connection constraints - be good to have in here, too, in case f.ex. PATCH or OPTIONS are blocked by a proxy in front of the ocis server. Tus allows to add a header in that case, so that

OPTIONS /endpoint/ HTTP/1.1
Tus-Version: …
…

can also be sent as

POST /endpoint/ HTTP/1.1
Tus-Version: …
X-HTTP-Method-Override: OPTIONS
…

in case OPTIONS requests wouldn't get through. And via http-method-override, it would be possible to tell the client to use another HTTP method and provide the actual method via X-HTTP-Method-Override header. Like so:

…
"http-method-override": "POST" `
…

@PVince81
Copy link
Contributor Author

PVince81 commented May 26, 2020

@PVince81
Copy link
Contributor Author

with the two above PRs, and using the go.mod approach, you can start the reva services after setting the env variables:

export REVA_FRONTEND_UPLOAD_MAX_CHUNK_SIZE=$(( 10 * 1024 * 1024 ))

it will make it advertise the given max chunk size in the capabilities

@michaelstingl
Copy link

yeah, builds nicely 👍

brew install hub
brew install go

git clone https://github.com/owncloud/ocis.git
git clone https://github.com/owncloud/ocis-reva.git
git clone https://github.com/cs3org/reva.git

cd ocis-reva
hub pr checkout 228
cd ..

cd reva
hub pr checkout 775
cd ..

cd ocis
echo 'replace github.com/owncloud/ocis-reva => ../ocis-reva' >> go.mod
echo 'replace github.com/cs3org/reva => ../reva' >> go.mod

make clean generate build

./bin/ocis server

Capabilities

% curl -u einstein:relativity -k 'https://localhost:9200/ocs/v1.php/cloud/capabilities?format=json' | jq '.ocs.data.capabilities.files , .ocs.data.capabilities.dav' 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1496  100  1496    0     0  42742      0 --:--:-- --:--:-- --:--:-- 42742
{
  "privateLinks": false,
  "bigfilechunking": false,
  "undelete": true,
  "versioning": true,
  "blacklisted_files": [],
  "tus_support": {
    "version": "1.0.0",
    "resumable": "1.0.0",
    "extension": "creation,creation-with-upload",
    "max_chunk_size": 0,
    "http_method_override": ""
  }
}
{
  "chunking": "",
  "trashbin": "1.0",
  "reports": [
    "search-files"
  ],
  "chunkingParallelUploadDisabled": false
}

@PVince81 is this correct? "max_chunk_size": 0, Default should be unlimited chunk size?

@michaelstingl
Copy link

michaelstingl commented May 27, 2020

export REVA_FRONTEND_UPLOAD_MAX_CHUNK_SIZE=$(( 10 * 1024 * 1024 ))
./bin/ocis server

With environment variable set, it looks good:

% curl -u einstein:relativity -k 'https://localhost:9200/ocs/v1.php/cloud/capabilities?format=json' | jq '.ocs.data.capabilities.files , .ocs.data.capabilities.dav'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1503  100  1503    0     0  26839      0 --:--:-- --:--:-- --:--:-- 26839
{
  "privateLinks": false,
  "bigfilechunking": false,
  "undelete": true,
  "versioning": true,
  "blacklisted_files": [],
  "tus_support": {
    "version": "1.0.0",
    "resumable": "1.0.0",
    "extension": "creation,creation-with-upload",
    "max_chunk_size": 10485760,
    "http_method_override": ""
  }
}
{
  "chunking": "",
  "trashbin": "1.0",
  "reports": [
    "search-files"
  ],
  "chunkingParallelUploadDisabled": false
}

Didn't test with remote setup, but don't expect trouble:
https://owncloud.github.io/ocis/basic-remote-setup/#use-the-binary

@felix-schwarz
Copy link

@PVince81 Nice! Since I see http_method_override in the response, too… is it also already supported?

@PVince81
Copy link
Contributor Author

PVince81 commented May 28, 2020

max_chunk_size = 0 means default unlimited.
this is something an admin could tweak.
we can discuss changing the default if needed

Regarding http_method_override I'm not sure if we need to change something on the server. If you like you can have a try. If this applies to the data provider where we use PATCH, I believe this one is already connected to tusd so it might already support it. If not then need to schedule time to add explicit support there.

@PVince81
Copy link
Contributor Author

@michaelstingl I'm fine removing the max-chunk-size as we didn't finish the discussion about it.
we could continue the discussion and implementation as a separate task

@michaelstingl
Copy link

No problem with max-chunk-size. It's needed for some use cases. Don't remove it. Default can be changed later…

@felix-schwarz
Copy link

Suggestion:

  • tus_support is missing -> no chunking supported
  • tus_support exists -> chunking supported (what'd be the point of TUS support otherwise?)
  • tus_support exists, no max_chunk_size key/value, or value of 0 -> client is free to pick a chunk size itself
  • tus_support exists, max_chunk_size key/value is set to non-zero value -> client is free to pick a chunk size itself, as long as it doesn't exceed max_chunk_size

FWIW the support for max_chunk_size built into the iOS SDK right now would need to be adjusted to be able to handle -1. It currently assumes 0 to be "any size" and the existence of tus_support to mean there's TUS - and therefore tus chunking - support.

@PVince81
Copy link
Contributor Author

PVince81 commented May 28, 2020

suggestion makes sense to me.
I'll adjust the PR to allow disabling TUS altogether.

also:

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 2, 2020

added option in reva to disable TUS support on OC layer: cs3org/reva#791

@felix-schwarz due to the way how the capabilities struct is created I cannot remove the "tus_support" attribute when disabled, so it will have the value "null" instead when TUS is disabled..

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 2, 2020

I just went ahead and added support for "X-HTTP-Method-Override" to reva:

to test, use:
export REVA_FRONTEND_UPLOAD_HTTP_METHOD_OVERRIDE=POST

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 2, 2020

Phoenix PR for acting on the chunk size and method override capabilities: owncloud/web#3568

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 3, 2020

reva PRs merged, will need a reva update:

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 5, 2020

implementation done, need to update docs for the new values

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 5, 2020

PR for updating the ocis-reva docs: #251

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 5, 2020

I'm not sure where to put this as it's more of a protocol thing, did we have any new location for protocol docs ? @butonic @micbar

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants