Slice module 206 requirement

Maxim Dounin mdounin at mdounin.ru
Fri Jul 15 04:07:11 UTC 2022


Hello!

On Wed, Jul 13, 2022 at 08:12:06PM +0400, Roman Arutyunyan wrote:

> Hi,
> 
> On Sun, Jul 10, 2022 at 11:35:48AM +0300, Maxim Dounin 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 at 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
> 
> Missing "t" in "Conten-Range".

Fixed, thnx.

> > 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
> 
> have -> has
> 417 -> 416

Fixed, thnx.

> > 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;
> 
> The patch looks ok to me
> 
> Tested with proxy_force_ranges.

Pushed to http://mdounin.ru/hg/nginx/.

-- 
Maxim Dounin
http://mdounin.ru/



More information about the nginx mailing list