You’re truly awesome! I’ll give the patch a try tomorrow - and thanks for the other bits and pieces of information, especially regarding the expectations as well.

I wish you an awesome Sunday!

Best Regards,
Lucas Rolff

> On 10 Jul 2022, at 10:35, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Fri, Jul 08, 2022 at 07:13:33PM +0000, Lucas Rolff wrote:
>
>> I’m having an nginx instance where I utilise the nginx slice
>> module to slice upstream mp4 files when using proxy_cache.
>>
>> However, I have an interesting origin where if sending a range
>> request (which happens when the slice module is enabled), to a
>> file that’s less than the slice range, the origin returns a 200
>> OK, but with the range related headers such as content-range,
>> but obviously the full file is returned since it’s within the
>> requested range.
>>
>> When playing the MP4s through Google Chrome and Firefox it works
>> fine when going through the nginx proxy instance, however, it
>> somehow breaks Safari (both on MacOS, and iOS) - I guess Safari
>> is more strict.
>> When playing directly through the origin it works fine in all
>> browsers.
>>
>> The md5 of response from the origin remains the same, so it’s
>> not that the response itself is an invalid MP4 file, and even if
>> you compare the cache files on disk with a “working” origin and
>> the “broken” origin (one sends a 206 Partial Content, another
>> sends 200 OK) - the content of the cache files remain the same,
>> except obviously the header section of the cache file.
>>
>> The origin returns a 206 status code, only if the file exceeds
>> the slice size, so if I configure a slice size of 5 megabyte,
>> only files above 5 megabytes will give 206s. Anything under 5
>> megabytes will result in a 200 OK with content-range and the
>> correct content-length,
>>
>> Looking in the slice module itself I see:
>> https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_slice_filter_module.c#L116-L126
>>
>>
>> if (r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT) {
>> if (r == r->main) {
>> ngx_http_set_ctx(r, NULL, ngx_http_slice_filter_module);
>> return ngx_http_next_header_filter(r);
>> }
>>
>> ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
>> "unexpected status code %ui in slice response",
>> r->headers_out.status);
>> return NGX_ERROR;
>> }
>>
>> This seems like the slice module expects a 206 status code to be
>> returned,
>
> For the main request, the code accepts two basic valid variants:
>
> - 206, so the slice module will combine multiple responses to
> range requests as needed;
>
> - anything else, so the slice module will give up and simply
> return the response to the client.
>
> If the module sees a non-206 response to a subrequest, this is an
> error, as the slice module expects underlying resources to be
> immutable, and does not expect that some ranges can be requested,
> while some other aren't. This isn't something related to your
> case though.
>
>> however, later in the same function
>> https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_slice_filter_module.c#L200-L211
>>
>>
>> if (r->headers_out.status == NGX_HTTP_PARTIAL_CONTENT) {
>> if (ctx->start + (off_t) slcf->size <= r->headers_out.content_offset) {
>> ctx->start = slcf->size
>> * (r->headers_out.content_offset / slcf->size);
>> }
>>
>> ctx->end = r->headers_out.content_offset
>> + r->headers_out.content_length_n;
>>
>> } else {
>> ctx->end = cr.complete_length;
>> }
>>
>> There it will do an else statement if the status code isn’t 206.
>> So would this piece of code ever be reached, since there’s the initial error?
>
> Following the initial check, r->headers_out.status is explicitly
> changed to NGX_HTTP_OK. Later on the
> ngx_http_next_header_filter() call might again change
> r->headers_out.status as long as the client used a range request,
> and this is what checked here.
>
>> Additionally I don’t see in RFC7233 that 206 responses are an
>> absolute requirement, additionally I don’t see content-range
>> being prohibited/forbidden to be used for 200 OK responses.
>> Now, if one have a secondary proxy that modifies the response
>> headers in between the origin returning 200 OK with the
>> Content-Range header, and then strip out the Content-Range
>> header, the nginx slice module seems to handle it fine, so
>> somehow the combination of 200 OK and a Content-Range header
>> being present seems to break the slice module from functioning.
>>
>> I’m just curious why this happens within the slice module, and
>> if there’s any possible solution for it (like allowing the
>> combination of 200 OK and Content-Range, since those two would
>> still indicate that the origin/upstream supports range requests)
>> - obviously it would be nice to fix the upstream server but
>> sometimes that’s sadly not possible.
>
>> From the above explanation it is probably already clear that
> "disabling slice when an origin returns 200 OK" is what actually
> happens.
>
> The issue does not appear without the slice module in your testing
> because the Content-Range header seems to be only present in your
> backend 200 responses when there was a Range header in the
> request, and this is what happens only with the slice module.
>
> I've done some limited testing with Safari and manually added
> Content-Range header, and there seem to be at least two issues:
>
> - Range filter in nginx does not expect the Content-Range header
> to be already present in 200 responses and simply adds another
> one. This results in incorrect range responses with multiple
> Content-Range headers, and this breaks Safari.
>
> - Safari also breaks if its test request with "Range: bytes=0-1"
> results in 200 with the Content-Range header.
>
> My initial fix was to simply disable handling of 200 responses
> with Content-Range headers in the range filter, so such responses
> wouldn't be touched at all. This is perfectly correct and
> probably the most secure thing to do, but does not work with
> Safari due to the second issue outlined above.
>
> Another approach would be to clear pre-existing Content-Range
> headers in the range filter. This seems to work, at least in my
> testing. See below for the patch.
>
>> I know the parts of the slice module haven’t been touched for
>> years, so obviously it works for most people, just dipping my
>> toes here to see if there’s a possible solution other than
>> disabling slice when an origin returns 200 OK for files smaller
>> than the slice size.
>
> Note that that slice module is generally unsafe to use for
> arbitrary upstream servers: it relies on expectations which are
> beyond the HTTP standard requirements. In particular:
>
> - It requires resources to be immutable, so different range
> responses can be combined together.
>
> - It does not try to handle edge cases, such as 416 returned by
> the upstream on empty files (which is correct per RFC, but
> requires complicated additional handling to convert 416 to 200, so
> it is better to just return 200 OK).
>
> In general, the slice module is to be used only in your own
> infrastructure when you control the backend and can be sure that
> the slice module expectations are met. As such, disabling it for
> backends which do something unexpected might actually be a good
> idea. On the other hand, in this particular case the nginx
> behaviour can be adjusted to handle things gracefully.
>
> Below is a patch to clear pre-existing Content-Range headers
> in the range filter. Please test if it works for you.
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1657439390 -10800
> # Sun Jul 10 10:49:50 2022 +0300
> # Node ID 219217ea49a8d648f5cadd046f1b1294ef05693c
> # Parent 9d98d524bd02a562d9cd83f4e369c7e992c5753b
> Range filter: clearing of pre-existing Content-Range headers.
>
> Some servers might emit Conten-Range header on 200 responses, and this
> does not seem to contradict RFC 9110: as per RFC 9110, the Content-Range
> header have no meaning for status codes other than 206 and 417. Previously
> this resulted in duplicate Content-Range headers in nginx responses handled
> by the range filter. Fix is to clear pre-existing headers.
>
> diff --git a/src/http/modules/ngx_http_range_filter_module.c b/src/http/modules/ngx_http_range_filter_module.c
> --- a/src/http/modules/ngx_http_range_filter_module.c
> +++ b/src/http/modules/ngx_http_range_filter_module.c
> @@ -425,6 +425,10 @@ ngx_http_range_singlepart_header(ngx_htt
> return NGX_ERROR;
> }
>
> + if (r->headers_out.content_range) {
> + r->headers_out.content_range->hash = 0;
> + }
> +
> r->headers_out.content_range = content_range;
>
> content_range->hash = 1;
> @@ -582,6 +586,11 @@ ngx_http_range_multipart_header(ngx_http
> r->headers_out.content_length = NULL;
> }
>
> + if (r->headers_out.content_range) {
> + r->headers_out.content_range->hash = 0;
> + r->headers_out.content_range = NULL;
> + }
> +
> return ngx_http_next_header_filter(r);
> }
>
> @@ -598,6 +607,10 @@ ngx_http_range_not_satisfiable(ngx_http_
> return NGX_ERROR;
> }
>
> + if (r->headers_out.content_range) {
> + r->headers_out.content_range->hash = 0;
> + }
> +
> r->headers_out.content_range = content_range;
>
> content_range->hash = 1;
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx mailing list -- nginx@nginx.org
> To unsubscribe send an email to nginx-leave@nginx.org

_______________________________________________
nginx mailing list -- nginx@nginx.org
To unsubscribe send an email to nginx-leave@nginx.org