Skip to content

[XrdHttp] Allow static headers to be specified for browsers #2389

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.

Sign up for GitHub

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

Jump to bottom
Merged
amadio merged 2 commits into from
Feb 14, 2025
Merged

[XrdHttp] Allow static headers to be specified for browsers #2389

amadio merged 2 commits into from
Feb 14, 2025

Conversation

@bbockelm
Copy link
Collaborator

@bbockelm bbockelm commented Dec 12, 2024

(This patch is more of a RFI -- I think all the infrastructure is in place that I can add a unit test before marking it as not a draft)

In Pelican, we want to allow users from browsers to be able to access XRootD servers (including through redirections). However, to do this, we need to add a few extra headers to all GET and OPTIONS requests to permit redirections across sites (we hit various CORS safeguards that are reasonable for a generic webapp -- but needed for a data federation).

The existing plugin mechanism isn't sufficient because we don't want to replace the XRootD handling of the requests (such as a GET) but rather augment it with a few static headers.

This PR adds a new configuration option, http.staticheader, that permits such a header to be set. The config option has the following form:

http.staticheader [-verb=[GET|HEAD|...]]* header [value]

Here's a few examples:

http.staticheader -verb=OPTIONS Access-Control-Allow-Origin *
http.staticheader -verb=GET Foo Bar
http.staticheader -verb=GET Foo Baz
http.staticheader Test 1
  • If the -verb is omitted then the header is set for all requests
  • If the value is omitted, then the prior static header settings are unset. Otherwise, they are additive (same header specified multiple times will cause multiple values to be sent; this is perfectly fine in HTTP-land).

So, for the example configuration above, here's the response for a simple GET request:

< HTTP/1.1 403 Forbidden
< Connection: Close
< Server: XrootD/v5.7.1-85-g42eab06b3
< Test: 1
< Foo: Bar
< Foo: Baz
< Content-Length: 36

@bbockelm bbockelm marked this pull request as draft December 12, 2024 22:19
@bbockelm
Copy link
Collaborator Author

bbockelm commented Dec 13, 2024

@amadio - do you recognize the test failures? I'd be a bit surprised if it was due to the PR itself (as the tests appear to be over the xrootd protocol and the changes here are localized to HTTP).

@amadio
Copy link
Member

amadio commented Dec 13, 2024

@bbockelm Yes, those are from Andy's last couple of commits. I think it's actually better to send pull requests for master, since I can merge either there or retarget to devel more easily than the other way around. And you will not see these test failures.

@abh3
Copy link
Member

abh3 commented Dec 13, 2024

No doubt, but other than the test ending with a non-zero RC there are no other error messages. Another transient error?

@bbockelm bbockelm force-pushed the xrdhttp_static_options branch from c6bccb7 to e11031a Compare January 10, 2025 20:32
@bbockelm
Copy link
Collaborator Author

bbockelm commented Jan 10, 2025

Ok, hearing no fundamental objections, I added an integration test and have marked this as ready for review.

@bbockelm bbockelm marked this pull request as ready for review January 10, 2025 20:57
@bbockelm bbockelm requested a review from ccaffy January 10, 2025 20:58
ccaffy
ccaffy requested changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@ccaffy ccaffy left a comment

Choose a reason for hiding this comment

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

Thanks again, I found some issues under certain circumstances (see different comments)

@bbockelm bbockelm mentioned this pull request Jan 15, 2025
Closed
@bbockelm bbockelm force-pushed the xrdhttp_static_options branch from 8b4490e to 5f73727 Compare January 15, 2025 19:41
@bbockelm bbockelm requested a review from ccaffy January 15, 2025 19:43
@bbockelm
Copy link
Collaborator Author

bbockelm commented Jan 15, 2025

@ccaffy - I touched up the pieces requested and adjusted the integration test accordingly. Please re-review.

ccaffy
ccaffy requested changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@ccaffy ccaffy left a comment

Choose a reason for hiding this comment

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

Just one typo and then it's ready to go. I tried it on my box and it works as expected! Thanks

@bbockelm bbockelm force-pushed the xrdhttp_static_options branch from 5f73727 to 1ecc6f9 Compare January 16, 2025 19:51
@bbockelm bbockelm requested a review from ccaffy January 16, 2025 19:51
ccaffy
ccaffy approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@ccaffy ccaffy left a comment

Choose a reason for hiding this comment

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

Thanks Brian for the fixes!
I'll create an issue for @abh3 to update the xrd http documentation to document these flags behavior.

@ccaffy ccaffy mentioned this pull request Jan 17, 2025
Closed
@bbockelm
Copy link
Collaborator Author

bbockelm commented Jan 27, 2025

@amadio - would you like me to retarget this to master? I think that'll (potentially) lose the review comments from @ccaffy (though those are potentially less important now he's approved the PR) but will clean up the commit history and fix up the unit tests.

@amadio
Copy link
Member

amadio commented Jan 27, 2025

If this doesn't depend on anything from devel, yes, go ahead. However, due to the nature of the changes, this will only get merged into a feature release.

@bbockelm bbockelm force-pushed the xrdhttp_static_options branch from 1ecc6f9 to 772b027 Compare January 27, 2025 15:39
@bbockelm bbockelm changed the base branch from devel to master January 27, 2025 15:40
@bbockelm
Copy link
Collaborator Author

bbockelm commented Jan 27, 2025

@amadio - sounds good! While there are some nearby code changes, nothing from devel is needed for this. I updated the PR to target master (which should help with the unit tests; I really like the "green checkmark") and feel free to merge into devel once you're ready.

@amadio amadio added this to the 5.8.0 milestone Jan 27, 2025
@amadio
Copy link
Member

amadio commented Jan 27, 2025

Ok, I'm marking this for 5.8.0. If there are no problems with ABI (which I will test later), I will merge this after 5.7.3 is released.

@bbockelm @amadio
[XrdHttp] Add http.staticheader command
d28d63a
@bbockelm @amadio
[XrdHttp] Add integration test for static headers
aaa558c 
This adds some simple, statically-defined headers to the
integration test to ensure they are appropriately included in the
responses.
@amadio amadio force-pushed the xrdhttp_static_options branch from 772b027 to aaa558c Compare February 13, 2025 14:32
@amadio
Copy link
Member

amadio commented Feb 13, 2025

Rebased to resolve conflicts with other merged changes that touched http.sh test script.

@amadio amadio merged commit dd98910 into xrootd:master Feb 14, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ccaffy ccaffy ccaffy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.8.0

Development

Successfully merging this pull request may close these issues.

4 participants

@bbockelm @amadio @abh3 @ccaffy