Skip to content

CA-406403: Do not return HTTP 500 when Accept header can't be parsed #6298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

psafont
Copy link
Member

@psafont psafont commented Feb 12, 2025

/update_rrds returned a 500 HTTP code in some cases where the accept header was
invalid. Now these cases are treated in the same way as a lack of Accept
header.

Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking whether an existing library could do this, all I could find was:
https://ocaml.org/p/httpaf_caged/latest/doc/Httpaf_caged/Accept/index.html
https://ocaml.org/p/dream-accept/latest

But they each depend on their own http libraries (httpaf and dream-message), and not the common http library.

@psafont psafont force-pushed the private/paus/accept-rrd branch from 5453d23 to 277c79a Compare February 12, 2025 17:22
@gangj
Copy link
Contributor

gangj commented Feb 13, 2025

I was checking whether an existing library could do this, all I could find was: https://ocaml.org/p/httpaf_caged/latest/doc/Httpaf_caged/Accept/index.html https://ocaml.org/p/dream-accept/latest

But they each depend on their own http libraries (httpaf and dream-message), and not the common http library.

Just a question, why didn't we use one of these http libraries, is it because there was no available library then? Thank you!

@psafont
Copy link
Member Author

psafont commented Feb 13, 2025

Just a question, why didn't we use one of these http libraries, is it because there was no available library then?

before 2012 OCaml didn't have a common repository of libraries (opam). So some may have existed, but the normal way of working was for everybody to build its own stack of libraries.

@psafont psafont force-pushed the private/paus/accept-rrd branch from 277c79a to edcffb6 Compare February 13, 2025 10:41
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this implementing the spec or deals with the cases we support? The spec has a lot of details that would require a parser.

@psafont psafont force-pushed the private/paus/accept-rrd branch 2 times, most recently from 90d42da to 1b49eba Compare February 13, 2025 11:20
@psafont
Copy link
Member Author

psafont commented Feb 13, 2025

The changes in the parser have brought a lot of uneeded attention, as the parsing wasn't being changed, only how the errors were transmitted. I've changed the PR to only document the existing parser and change how it's handled by the /update_rrds HTTP endpoint

@psafont psafont force-pushed the private/paus/accept-rrd branch from 1b49eba to a73fe46 Compare February 13, 2025 11:26
/update_rrds returned a 500 HTTP code in some cases where the accept header was
invalid. Now these cases are treated in the same way as a lack of Accept
header.

Signed-off-by: Pau Ruiz Safont <[email protected]>
@psafont psafont force-pushed the private/paus/accept-rrd branch from a73fe46 to 7e64a26 Compare February 13, 2025 11:35
@psafont
Copy link
Member Author

psafont commented Feb 13, 2025

Without the patch:

$ curl -k -sS -H "Accept: Garbage" -D - "https://xs-box/rrd_updates?session_id=OpaqueRef:634ef2ae-9599-f26d-1709-ef5ace1ce6fb&cf=AVERAGE&start=1738774514&host=true" -o /dev/null
HTTP/1.1 500 Internal Error
content-length: 277
content-type: text/html
connection: close
cache-control: no-cache, no-store

with the patch:

$ curl -k -sS -H "Accept: Garbage" -D - "https://xs-box/rrd_updates?session_id=OpaqueRef:067f4032-67a5-b6ec-ed73-450206054271&cf=AVERAGE&start=1738774514&host=true" -o /dev/null
HTTP/1.1 200 OK
Connection: keep-alive
Content-Length: 10816
Cache-Control: no-cache, no-store
content-type: text/xml
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: X-Requested-With

@psafont psafont added this pull request to the merge queue Feb 13, 2025
Merged via the queue into xapi-project:master with commit 5b99bed Feb 13, 2025
15 checks passed
@psafont psafont deleted the private/paus/accept-rrd branch February 14, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants