Skip to content
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

Set Accept-Ranges: none header when compressing responses #229

Closed
palant opened this issue May 6, 2024 · 3 comments
Closed

Set Accept-Ranges: none header when compressing responses #229

palant opened this issue May 6, 2024 · 3 comments
Assignees
@andrewhavck
Labels
Accepted This change is accepted by us and merged to our internal repo bug Something isn't working

Comments

@palant
Copy link
Contributor

palant commented May 6, 2024

What is the problem your feature solves, or the need it fulfills?

When adjusting response headers, ResponseCompressionCtx will currently remove the Content-Length header as the old header does not apply. A potentially present Accept-Ranges: bytes header is left untouched however, even though ranged requests no longer make sense.

Describe the solution you'd like

Call resp.insert_header(&ACCEPT_RANGES, HeaderValue::from_static("none")) when compressing. This will both overwrite any existing Accept-Ranges header and explicitly indicate to the client that ranged requests are unsupported.

Describe alternatives you've considered

N/A

Additional context

Accept-Ranges header spec
@palant palant changed the title Set Accept-Ranges: none when compressing responses Set Accept-Ranges: none header when compressing responses May 7, 2024
@andrewhavck andrewhavck added the enhancement New feature or request label May 9, 2024
@eaufavor eaufavor added bug Something isn't working and removed enhancement New feature or request labels May 10, 2024
@andrewhavck andrewhavck self-assigned this Jun 8, 2024
@andrewhavck andrewhavck added WIP We are working on this feature internally Accepted This change is accepted by us and merged to our internal repo and removed WIP We are working on this feature internally labels Jun 8, 2024
palant added a commit to palant/pingora that referenced this issue Jun 15, 2024
@palant
Fixes cloudflare#229, cloudflare#233 – Set proper response headers wh…
78f09b2 
…en compression is enabled
@palant palant mentioned this issue Jun 15, 2024
Closed
@andrewhavck
Copy link
Contributor

The Accept-Ranges header is removed when compressing a response.

This change was synced internally before 286 was created.

@palant
Copy link
Contributor Author

palant commented Jun 21, 2024 •
edited
Loading

@andrewhavck Yes, I got that. I updated #286 to account for the recent changes.

Note that b8ec7aa (correctly) applies to the decompression scenario as well, but it only tests the compression one. My pull request tests both.

@palant
Copy link
Contributor Author

palant commented Jul 4, 2024

Closing as this is largely resolved.

@palant palant closed this as completed Jul 4, 2024
@palant palant mentioned this issue Jul 4, 2024
Closed
johnhurt pushed a commit that referenced this issue Aug 23, 2024
@palant @andrewhavck
Fixes #229, #233 – Set proper response headers when compression is en…
a155c50 
…abled

---
Merge branch 'main' into compression-headers
---
Updated “compression enabled” check for recent changes
---
Fixed clippy warning
---
Reverted changes related to Accept-Ranges header
---
Handle multiple Vary headers
---
Merged main branch

Includes-commit: 4ea0c3f
Includes-commit: 78f09b2
Includes-commit: 973d596
Includes-commit: 9f82578
Includes-commit: d6e94c9
Includes-commit: e88fdb8
Includes-commit: f647f04
Replicated-from: #286
johnhurt pushed a commit that referenced this issue Aug 23, 2024
@palant @johnhurt
Fixes #229, #233 – Set proper response headers when compression is en…
55049c4 
…abled

---
Merge branch 'main' into compression-headers
---
Updated “compression enabled” check for recent changes
---
Fixed clippy warning
---
Reverted changes related to Accept-Ranges header
---
Handle multiple Vary headers
---
Merged main branch

Includes-commit: 4ea0c3f
Includes-commit: 78f09b2
Includes-commit: 973d596
Includes-commit: 9f82578
Includes-commit: d6e94c9
Includes-commit: e88fdb8
Includes-commit: f647f04
Replicated-from: #286
escoffier pushed a commit to escoffier/pingora that referenced this issue Sep 6, 2024
@palant @escoffier
Fixes cloudflare#229, cloudflare#233 – Set proper response headers wh…
ca5a262 
…en compression is enabled

---
Merge branch 'main' into compression-headers
---
Updated “compression enabled” check for recent changes
---
Fixed clippy warning
---
Reverted changes related to Accept-Ranges header
---
Handle multiple Vary headers
---
Merged main branch

Includes-commit: 4ea0c3f
Includes-commit: 78f09b2
Includes-commit: 973d596
Includes-commit: 9f82578
Includes-commit: d6e94c9
Includes-commit: e88fdb8
Includes-commit: f647f04
Replicated-from: cloudflare#286
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andrewhavck andrewhavck

Labels
Accepted This change is accepted by us and merged to our internal repo bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@andrewhavck @palant @eaufavor