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.

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

We set Accept-Range on pass, is that OK ? #3251

Closed
bsdphk opened this issue Mar 15, 2020 · 5 comments
Closed

We set Accept-Range on pass, is that OK ? #3251

bsdphk opened this issue Mar 15, 2020 · 5 comments
Assignees

Comments

@bsdphk
Copy link
Contributor

bsdphk commented Mar 15, 2020

Shouldn't we leave it to the backend to set A-R on pass ?

@nigoroll
Copy link
Member

nigoroll commented Mar 16, 2020

bugwash:

Right now, we add Accept-Range to any 200 response when http_range_support is on. This happens after vcl_deliver, so there is no VCL control over it.

We agree that we want more VCL control and three alternative approaches have been discussed:

  • add a switch to vcl if Accept-Range should be added if not present
    • concern: switch-ism
  • set Accept-Range before calling vcl_deliver, so it can be removed
    • concern: how would vcl determine where the A-R came from?
  • do not set Accept-Range in core code, but rather in builtin.vcl
    • concern: Will this still DTRT for 99.9% of the users?

We agree to handle this as part of #3246

Also we agree that the documentation of http_range_support could be improved.

dridi added a commit that referenced this issue Mar 17, 2020
nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Apr 14, 2020
This is an alternative approach to the ideas recorded in varnishcache#3251: By
centralizing control over the Range-Request related headers, range
handling can be disabled by removing the range vdp from the response
filters.

By default, we now always push the range VDP as long as
http_range_support is on. If that is the case, behaviour is just the
same as before, but the Accept-Ranges response header is now set in the
range VDP's init callback.

The difference is that, if the range vdp is removed from the filters, no
Accept-Ranges header will be sent.

Note that if the range will not be active during delivery if it only
sets Accept-Ranges (by returning 1 from the init callback).

Fixes varnishcache#3251
@nigoroll
Copy link
Member

I realized that the actual cause of the problem is that we mixed handling of ranges in core code and the range vdp. By moving all relevant parts to that one, whether or not we send Accept-Ranges can be controlled by having the range vdp active or not.

See #3289

@bsdphk
Copy link
Contributor Author

bsdphk commented Apr 20, 2020

How this should work:

  1. vcl_backend_deliver{} should be able to set/unset Accept-Ranges as desired.
  2. On pass'ed responses, the backend controls if A-R should be in resp.http going into vcl_deliver{}

@nigoroll
Copy link
Member

nigoroll commented Apr 20, 2020

@bsdphk that option was discussed before, see above:

  • set Accept-Range before calling vcl_deliver, so it can be removed
    • concern: how would vcl determine where the A-R came from?

How do you address the concern?

@bsdphk
Copy link
Contributor Author

bsdphk commented May 17, 2021

You can test obj.uncachable if it is true, the header came from the backend.

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 a pull request may close this issue.

2 participants