Project

General

Profile

Actions
Copy link

Bug #20647

closed

container_request/.../logs endpoint needs to handle CORS preflight (unauthenticated OPTIONS) requests

Added by Tom Clegg over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tom Clegg
Category:
Crunch
Target version:
Development 2023-06-21 sprint
Story points:
1.0
Release:
Arvados 2.7.0
Release relationship:
Auto

Description

The controller endpoint currently handles authenticated OPTIONS requests, which webdav clients use to discover which webdav methods are supported.

It also needs to handle CORS preflight requests (unauthenticated OPTIONS requests). Responses do not need to include the DAV headers. This is how keep-web does it:

        if method := r.Header.Get("Access-Control-Request-Method"); method != "" && r.Method == "OPTIONS" {
                if !browserMethod[method] && !webdavMethod[method] {
                        w.WriteHeader(http.StatusMethodNotAllowed)
                        return
                }
                w.Header().Set("Access-Control-Allow-Headers", corsAllowHeadersHeader)
                w.Header().Set("Access-Control-Allow-Methods", "COPY, DELETE, GET, LOCK, MKCOL, MOVE, OPTIONS, POST, PROPFIND, PROPPATCH, PUT, RMCOL, UNLOCK")
                w.Header().Set("Access-Control-Allow-Origin", "*")
                w.Header().Set("Access-Control-Max-Age", "86400")
                return
        }

Subtasks 1 (0 open — 1 closed)

Task #20648: Review 20647-cr-logs-preflightResolvedTom Clegg06/16/2023Actions

Related issues 1 (0 open — 1 closed)

Blocks Arvados - Feature #20219: Log panel on container view fetches live logs using periodic range request on new container log endpointResolvedStephen Smith07/28/2023Actions
Actions
Copy link
 #1

Updated by Tom Clegg over 1 year ago

  • Blocks Feature #20219: Log panel on container view fetches live logs using periodic range request on new container log endpoint added
Actions
Copy link
 #2

Updated by Tom Clegg over 1 year ago

  • Status changed from New to In Progress
Actions
Copy link
 #4

Updated by Peter Amstutz over 1 year ago

Tom Clegg wrote in #note-3:

20647-cr-logs-preflight @ c18d038b87786aee25da9aac751193219e316c3a -- developer-run-tests: #3705

This LGTM.

Actions
Copy link
 #5

Updated by Tom Clegg over 1 year ago

  • Status changed from In Progress to Resolved
Actions
Copy link
 #6

Updated by Tom Clegg over 1 year ago

  • Status changed from Resolved to In Progress

These duplicate headers cause problems. Here is a successful non-CORS webdav request that gets passed through to keep-web because the container is finished:

$ curl -v -X OPTIONS -H "Origin: *" -H "Authorization: Bearer $ARVADOS_API_TOKEN" https://tordo.arvadosapi.com/arvados/v1/container_requests/tordo-xvhdp-zzy8gx32za3f5n1/log/
...
< HTTP/2 200 
< server: nginx/1.14.2
< date: Fri, 16 Jun 2023 20:22:11 GMT
< content-length: 0
< access-control-allow-headers: Authorization, Content-Type, Range, X-Http-Method-Override
< access-control-allow-methods: GET, HEAD, OPTIONS, PROPFIND, PUT, POST, PATCH, DELETE
< access-control-allow-origin: *
< access-control-allow-origin: *
< access-control-expose-headers: Content-Range
< access-control-expose-headers: Content-Range
< access-control-max-age: 86486400
< allow: OPTIONS, LOCK, DELETE, PROPPATCH, COPY, MOVE, UNLOCK, PROPFIND
< dav: 1, 2
< ms-author-via: DAV
< vary: X-Webdav-Prefix,
< x-request-id: req-1rpqvkfqcwf4d10hwprr
< x-request-id: req-1rpqvkfqcwf4d10hwprr
< strict-transport-security: max-age=63072000
<
Actions
Copy link
 #8

Updated by Peter Amstutz over 1 year ago

Tom Clegg wrote in #note-7:

20647-cr-logs-preflight @ 00c93619f7691c0828f5273bc457e2840dbdc084 -- developer-run-tests: #3710

So if I'm following along, what's happening is that queries are proxied to either keep-web or crunch-run, and those respond with CORS headers and request-id, but then controller also adds those headers by default, so the solution is to delete all headers propagated from the keep-web or crunch-run response and only send whatever headers controller adds. Is that right?

Will this remove headers from the response that we actually need? For example, content-range?

Actions
Copy link
 #9

Updated by Tom Clegg over 1 year ago

Not quite, it's
  1. controller sets some headers ABCDE on w
  2. reverseproxy does the upstream request, which returns some response headers CDEFG in resp
  3. reverseproxy calls ModifyResponse func, which deletes CDEFG from w
  4. reverseproxy adds CDEFG to w
  5. now w has ABCDEFG
Before, we had
  1. controller sets some headers ABCDE on w
  2. reverseproxy does the upstream request, which returns some response headers CDEFG in resp
  3. reverseproxy adds CDEFG to w
  4. now w has ABCCDDEEFG
Actions
Copy link
 #11

Updated by Peter Amstutz over 1 year ago

Tom Clegg wrote in #note-10:

20647-cr-logs-preflight @ 1fb22ef7709eb9b07b05b863d1bdadc39e35c995 -- developer-run-tests: #3711

LGTM please merge.

Actions
Copy link
 #12

Updated by Tom Clegg over 1 year ago

  • Status changed from In Progress to Resolved
Actions
Copy link
 #13

Updated by Peter Amstutz over 1 year ago

  • Release set to 66
Actions
Copy link

Also available in: Atom PDF