Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Wed, Jul 20, 2016 at 06:33:11PM -0700, Alexey Ivanov wrote:

> 
> > On Jul 20, 2016, at 6:23 PM, Maxim Dounin  wrote:
> > 
> > Hello!
> > 
> > On Wed, Jul 20, 2016 at 03:34:46PM -0700, Alexey Ivanov wrote:
> > 
> >> Speaking of trailers: we had couple of use cases for HTTP
> >> trailers, most of them were around streaming data to user.
> >> 
> >> For example, when webpage is generated we send headers and part
> >> of the body(usually up to ``) almost immediately, but
> >> then we start querying all the micro services for the content
> >> (SOA, yay!).
> >> The problem is that upstreams will inevitably fail/timeout, and
> >> when that happens there is no way to pass any metadata about the
> >> error to nginx, since headers are already sent. Using trailers
> >> here may improve MTTR since backend metadata is available on the
> >> frontend.
> >> 
> >> Another example may be computing checksums for data while you
> >> stream it and putting it in the trailer. This should reduce TTFB
> >> by quite a lot on some workloads we have.
> > 
> > Do you actually use something like this, or know places where
> > something like this is actually used?
> These are examples from our production. Currently we are using 
> workarounds for both of these problems. Though I'm not sure that 
> we would use trailers if they were supported, since it's one of 
> very obscure HTTP/1.1 features that people do not usually know 
> about.
> 
> That is starting to change bit by bit though, since people try 
> using gRPC more and more.

Could you please elaborate what exactly you do use now and if you 
know any examples of trailers being used instead?

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Alexey Ivanov]

> On Jul 20, 2016, at 6:23 PM, Maxim Dounin  wrote:
> 
> Hello!
> 
> On Wed, Jul 20, 2016 at 03:34:46PM -0700, Alexey Ivanov wrote:
> 
>> Speaking of trailers: we had couple of use cases for HTTP
>> trailers, most of them were around streaming data to user.
>> 
>> For example, when webpage is generated we send headers and part
>> of the body(usually up to ``) almost immediately, but
>> then we start querying all the micro services for the content
>> (SOA, yay!).
>> The problem is that upstreams will inevitably fail/timeout, and
>> when that happens there is no way to pass any metadata about the
>> error to nginx, since headers are already sent. Using trailers
>> here may improve MTTR since backend metadata is available on the
>> frontend.
>> 
>> Another example may be computing checksums for data while you
>> stream it and putting it in the trailer. This should reduce TTFB
>> by quite a lot on some workloads we have.
> 
> Do you actually use something like this, or know places where
> something like this is actually used?
These are examples from our production. Currently we are using workarounds for 
both of these problems. Though I'm not sure that we would use trailers if they 
were supported, since it's one of very obscure HTTP/1.1 features that people do 
not usually know about.

That is starting to change bit by bit though, since people try using gRPC more 
and more.
> 
> Obviously enough, trailers have lots of theoretical use cases, and
> that's why there were introduced in HTTP/1.1 in the first place.
> The problem is that it doesn't seem to be used in the real world
> though.
> 
> --
> Maxim Dounin
> http://nginx.org/
> 
> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



signature.asc
Description: Message signed with OpenPGP using GPGMail
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Wed, Jul 20, 2016 at 03:34:46PM -0700, Alexey Ivanov wrote:

> Speaking of trailers: we had couple of use cases for HTTP 
> trailers, most of them were around streaming data to user.
> 
> For example, when webpage is generated we send headers and part 
> of the body(usually up to ``) almost immediately, but 
> then we start querying all the micro services for the content 
> (SOA, yay!).
> The problem is that upstreams will inevitably fail/timeout, and 
> when that happens there is no way to pass any metadata about the 
> error to nginx, since headers are already sent. Using trailers 
> here may improve MTTR since backend metadata is available on the 
> frontend.
> 
> Another example may be computing checksums for data while you 
> stream it and putting it in the trailer. This should reduce TTFB 
> by quite a lot on some workloads we have.

Do you actually use something like this, or know places where 
something like this is actually used?

Obviously enough, trailers have lots of theoretical use cases, and 
that's why there were introduced in HTTP/1.1 in the first place.  
The problem is that it doesn't seem to be used in the real world 
though.

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Alexey Ivanov]

Speaking of trailers: we had couple of use cases for HTTP trailers, most of 
them were around streaming data to user.

For example, when webpage is generated we send headers and part of the 
body(usually up to ``) almost immediately, but then we start querying 
all the micro services for the content (SOA, yay!).
The problem is that upstreams will inevitably fail/timeout, and when that 
happens there is no way to pass any metadata about the error to nginx, since 
headers are already sent. Using trailers here may improve MTTR since backend 
metadata is available on the frontend.

Another example may be computing checksums for data while you stream it and 
putting it in the trailer. This should reduce TTFB by quite a lot on some 
workloads we have.

> On Jul 13, 2016, at 5:34 PM, Piotr Sikora  wrote:
> 
> Hey Maxim,
> 
>> I'm talking about trailers in general (though it's more about
>> requests than responses).  Normal request (and response)
>> processing in nginx assumes that headers are processed before the
>> body, and adding trailers (which are headers "to be added later")
>> to the picture are likely to have various security implications.
> 
> Let's step back a bit... I have no plans to change the processing
> logic nor merge trailers with headers. Trailers are going to be
> ignored (passed, but not processed) by NGINX, not discarded.
> 
> AFAIK, Apache does the same thing.
> 
> Basically, at this point, trailers act as metadata for the application
> (browser, gRPC, 3rd-party NGINX module, etc.), with no HTTP semantics,
> so there are no security implications for NGINX itself.
> 
> Best regards,
> Piotr Sikora
> 
> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



signature.asc
Description: Message signed with OpenPGP using GPGMail
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> I'm talking about trailers in general (though it's more about
> requests than responses).  Normal request (and response)
> processing in nginx assumes that headers are processed before the
> body, and adding trailers (which are headers "to be added later")
> to the picture are likely to have various security implications.

Let's step back a bit... I have no plans to change the processing
logic nor merge trailers with headers. Trailers are going to be
ignored (passed, but not processed) by NGINX, not discarded.

AFAIK, Apache does the same thing.

Basically, at this point, trailers act as metadata for the application
(browser, gRPC, 3rd-party NGINX module, etc.), with no HTTP semantics,
so there are no security implications for NGINX itself.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Wed, Jul 13, 2016 at 10:25:57AM -0700, Piotr Sikora wrote:

> > So the question remains: are there any real-world use cases?  May
> > be someone will be able to provide some.
> 
> It's a chicken-and-egg problem. It's hard to use a feature that's not
> supported by one of the most popular web servers.

The feature was there for years before nginx became popular.  And 
no one prevents it to be used with other servers, including more 
popular ones.  The fact that there are no real-world use cases 
suggests that there is something wrong with the feature.

> > Without real-world use cases I don't think this should be added,
> > as in general trailers is quite external concept to how nginx
> > works and also may have various security implications.
> 
> Just to be clear, are you talking about HTTP/1.1 trailers or trailers
> in general? The patch also includes HTTP/2 trailers and it's not clear
> which one you don't like.

I'm talking about trailers in general (though it's more about 
requests than responses).  Normal request (and response) 
processing in nginx assumes that headers are processed before the 
body, and adding trailers (which are headers "to be added later") 
to the picture are likely to have various security implications.

> > Silently dropping trailers is what anyway happens if the client
> > doesn't support chunked encoding at all (e.g., uses HTTP/1.0).
> > And this is also what happens in your patch if there is no "TE:
> > trailers".
> 
> Yes, but then it happens for a reason (client doesn't support
> trailers) and not "just because".

"Transfer encoding used doesn't allow trailers" looks like a 
reason for me.

> > I think that whether to drop Content-Length and switch to chunked
> > encoding is highly use-case specific question.  In some cases it
> > may be appropriate, in some cases not, and in some cases one may
> > want to add trailers even without "TE: trailers".  So forcing
> > chunked encoding probably should be configured separately.  On the
> > other hand, it's very hard to decide something given there are no
> > real use cases known.
> 
> Why? I don't see anyone making a big deal out of forced chunked
> encoding with "gzip on".

Gzip drops Content-Length because it's not possible to know it in 
advice.  And does so only for some responses, with various 
configuration options available to prevent this happening for 
clients who can't handle it and/or for responses where it is not 
beneficial.

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> So the question remains: are there any real-world use cases?  May
> be someone will be able to provide some.

It's a chicken-and-egg problem. It's hard to use a feature that's not
supported by one of the most popular web servers.

> Without real-world use cases I don't think this should be added,
> as in general trailers is quite external concept to how nginx
> works and also may have various security implications.

Just to be clear, are you talking about HTTP/1.1 trailers or trailers
in general? The patch also includes HTTP/2 trailers and it's not clear
which one you don't like.

> Silently dropping trailers is what anyway happens if the client
> doesn't support chunked encoding at all (e.g., uses HTTP/1.0).
> And this is also what happens in your patch if there is no "TE:
> trailers".

Yes, but then it happens for a reason (client doesn't support
trailers) and not "just because".

> I think that whether to drop Content-Length and switch to chunked
> encoding is highly use-case specific question.  In some cases it
> may be appropriate, in some cases not, and in some cases one may
> want to add trailers even without "TE: trailers".  So forcing
> chunked encoding probably should be configured separately.  On the
> other hand, it's very hard to decide something given there are no
> real use cases known.

Why? I don't see anyone making a big deal out of forced chunked
encoding with "gzip on".

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Thu, Jul 07, 2016 at 01:08:46PM -0700, Piotr Sikora wrote:

[...]

> Furthermore, like you previously noted, official gRPC clients & server
> are using HTTP/2, so there is absolutely no Google-agenda in pushing
> HTTP/1.1 trailers, other than me wanting to add proper support for
> trailers.

So the question remains: are there any real-world use cases?  May 
be someone will be able to provide some.

Without real-world use cases I don't think this should be added, 
as in general trailers is quite external concept to how nginx 
works and also may have various security implications.

> > Your patch forces use of chunked transfer encoding, and that's the
> > point where I disagree.
> 
> OK, that wasn't clear before.
> 
> > The "TE: trailers" request header means
> > that client is able to understand trailers, but it doesn't mean
> > that chunked encoding must be used even if content length is
> > known.  Using chunked transfer encoding instead of Content-Length
> > may or may not be justified by trailers, and this depends on a
> > particular use case.
> 
> Well, I think that if someone decides to add trailers in nginx.conf
> (either via "add_trailer" or "proxy_pass_trailers" in the future),
> then forcing chunked encoding and emitting those trailers is much
> closer to the intent of the person that configured NGINX than silently
> dropping them.
> 
> If that's a blocker for you, then I can change this behavior, but I
> think that most people would be quite surprised by explicitly
> configured trailers that are silently dropped from the response.

Silently dropping trailers is what anyway happens if the client 
doesn't support chunked encoding at all (e.g., uses HTTP/1.0).  
And this is also what happens in your patch if there is no "TE: 
trailers".

I think that whether to drop Content-Length and switch to chunked 
encoding is highly use-case specific question.  In some cases it 
may be appropriate, in some cases not, and in some cases one may 
want to add trailers even without "TE: trailers".  So forcing 
chunked encoding probably should be configured separately.  On the 
other hand, it's very hard to decide something given there are no 
real use cases known.

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> And these 3 people are the only people asking/commenting about
> trailers during the whole nginx history, AFAIR.  And none of them
> provided any real-world use for trailers.

That's true... Wesley, Shuxin, would you mind sharing your use cases?

> On the other hand, your
> motivation is quite clear, it's about Google pushing it's own
> library, nothing more.

I disagree. There is nothing gRPC-specific about trailers, the
protocol is just layered on top of HTTP standard.

Furthermore, like you previously noted, official gRPC clients & server
are using HTTP/2, so there is absolutely no Google-agenda in pushing
HTTP/1.1 trailers, other than me wanting to add proper support for
trailers.

> Your patch forces use of chunked transfer encoding, and that's the
> point where I disagree.

OK, that wasn't clear before.

> The "TE: trailers" request header means
> that client is able to understand trailers, but it doesn't mean
> that chunked encoding must be used even if content length is
> known.  Using chunked transfer encoding instead of Content-Length
> may or may not be justified by trailers, and this depends on a
> particular use case.

Well, I think that if someone decides to add trailers in nginx.conf
(either via "add_trailer" or "proxy_pass_trailers" in the future),
then forcing chunked encoding and emitting those trailers is much
closer to the intent of the person that configured NGINX than silently
dropping them.

If that's a blocker for you, then I can change this behavior, but I
think that most people would be quite surprised by explicitly
configured trailers that are silently dropped from the response.

Best regard,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Wed, Jul 06, 2016 at 02:02:19PM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > I'm not convinced this feature is needed at all.  It looks more
> > like a Google-specific experiment, and this is not something I
> > would like to see in nginx code.
> 
> Wait, what? Are you talking about trailers as defined in RFC7230 (and
> previously in RFC2616 from 1999, when Google barely existed)? There is
> nothing Google-specific about them.
> 
> Also, you had people from 3 different companies asking and/or
> commenting about trailers on this very mailing list in the last 6
> weeks, I'm not sure why you chose to ignore that.

And these 3 people are the only people asking/commenting about 
trailers during the whole nginx history, AFAIR.  And none of them 
provided any real-world use for trailers.  On the other hand, your 
motivation is quite clear, it's about Google pushing it's own 
library, nothing more.

> > If at all implemented, I would rather prefer adding trailers if a
> > transfer encoding used allows this, and discrading them otherwise.
> > This is something that anyway will happen if a protocol used does
> > not allow trailers at all (e.g., HTTP/1.0).
> 
> I'm confused, this is exactly what my patch is doing... unless you
> want to completely ignore "TE" request header?

Your patch forces use of chunked transfer encoding, and that's the 
point where I disagree.  The "TE: trailers" request header means 
that client is able to understand trailers, but it doesn't mean 
that chunked encoding must be used even if content length is 
known.  Using chunked transfer encoding instead of Content-Length 
may or may not be justified by trailers, and this depends on a 
particular use case.

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> I'm not convinced this feature is needed at all.  It looks more
> like a Google-specific experiment, and this is not something I
> would like to see in nginx code.

Wait, what? Are you talking about trailers as defined in RFC7230 (and
previously in RFC2616 from 1999, when Google barely existed)? There is
nothing Google-specific about them.

Also, you had people from 3 different companies asking and/or
commenting about trailers on this very mailing list in the last 6
weeks, I'm not sure why you chose to ignore that.

> If at all implemented, I would rather prefer adding trailers if a
> transfer encoding used allows this, and discrading them otherwise.
> This is something that anyway will happen if a protocol used does
> not allow trailers at all (e.g., HTTP/1.0).

I'm confused, this is exactly what my patch is doing... unless you
want to completely ignore "TE" request header?

Just keep in mind that per RFC7230, sec. 4.1.2:

   Unless the request includes a TE header field indicating "trailers"
   is acceptable, as described in Section 4.3, a server SHOULD NOT
   generate trailer fields that it believes are necessary for the user
   agent to receive.  Without a TE containing "trailers", the server
   ought to assume that the trailer fields might be silently discarded
   along the path to the user agent.  This requirement allows
   intermediaries to forward a de-chunked message to an HTTP/1.0
   recipient without buffering the entire response.

and sec. 4.3:

   The presence of the keyword "trailers" indicates that the client is
   willing to accept trailer fields in a chunked transfer coding, as
   defined in Section 4.1.2, on behalf of itself and any downstream
   clients.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Thu, Jun 30, 2016 at 12:56:50PM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > Would adding r->trailers_emit (or r->expect_trailers, whichever you
> > prefer) make you happy?
> 
> Ping.

I'm not convinced this feature is needed at all.  It looks more 
like a Google-specific experiment, and this is not something I 
would like to see in nginx code.

If at all implemented, I would rather prefer adding trailers if a 
transfer encoding used allows this, and discrading them otherwise.  
This is something that anyway will happen if a protocol used does 
not allow trailers at all (e.g., HTTP/1.0).

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> Would adding r->trailers_emit (or r->expect_trailers, whichever you
> prefer) make you happy?

Ping.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Shuxin,

> How do you manage to tackle this scenario without changing your existing
> code:
>
> backend server responses with trailer "lol = 123", and you define the
> same trailer,
> "lol = $haha" in *.conf. You have two options as to how to combine them:
> either override the
> one from origin, and override the one from your *.conf.

...or the third option - emit both trailers ;)

This is consistent with how duplicate headers are currently handled in
NGINX, that is: headers emitted by upstream and those added with
"add_header" are both transmitted to the client.

> It seems to me that the
> chunk-module is again a best place for such change.

Again, chunked module is HTTP/1.1 specific and nothing other than
"encode trailers from r->headers_out.trailers to HTTP/1.1" should be
handled there.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[shuxinyang]

It's "trailer support" for the web serving part of NGINX, just not proxying.


  - the "get" in "ngx_http_chunked_get_trailers" is sort of misleading.
The "get" here actually
means "generate". I thought you are looking for a existing buffer.

That function name got changed a few times, since I cannot find one
that I would be 100% happy with... Any suggestions?
I hope you don't call me "make fuss about nothing":-). Being an 
impatient code reader,
I usually take a glimpse the code by reading comment and/or function, 
and imagine

what it tries to do, and stitch them together into a big picture.

the verb "get" did confuse me yesterday and this morning when I took a 
quick look.

I would say "create" or "generate" (or something else) would be better.


Anyway, naming is not big deal, maybe I (and other impatient code 
reader) need change:-)






   Now go back to the long list of trailer support,  I believe it needs to
cover at least:
 1) generate trailer headers and sent to downstream/visitor as you just
did
 2) reverse proxy propagate trailer headers from upstream, which needs
 2.0 parse the incoming trailer header, both considering the body is
buffered and not buffered,
 2.1. make the incoming trailer accessible via variable after content
phase,
 2.2. if response body is is not buffered, combine incoming trailer
header with the generated headers.
 2.3. convert incoming trailer to regular header if response body is
buffered

3|4|..|n) other features.

I think 2.x is much harder than 1). I only implement 1) and 2.0) and
2.1). While implement 2.1), I left some
room for 2.2 and 2.3 for the future. Now that you have plan for 2.x as your
email suggests, wouldn't it
be nice to submit these code first, and then go ahead with the code of 1). I
believe to support 2.2 or 2.3,
your existing code in chunk module needs lots of change.

I don't see how anything you listed in 2.x would require modifications
to the code I submitted... Actually, everything you listed should be
handled in upstream and/or proxy modules, not chunking module, and it
could easily use the API (r->headers_out.trailers) I want to add in
this commit.


How do you manage to tackle this scenario without changing your existing 
code:


backend server responses with trailer "lol = 123", and you define 
the same trailer,
"lol = $haha" in *.conf. You have two options as to how to combine them: 
either override the
one from origin, and override the one from your *.conf. It seems to me 
that the

chunk-module is again a best place for such change.

Best Regards
Shuxin



Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel


___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Shuxin,

>  I took some time to read your code, I believe I fully understand your
> code at this moment.
> My implementation is slightly differently from yours, and we have different
> use-cases.

Thanks!

>  Here is my $0.02 value of comment to your code (cosmetic wise):
>  - the verb "support" in "trailer support" in too general. The
> trailer-support in my mind include
>a long list of features (see bellow)

It's "trailer support" for the web serving part of NGINX, just not proxying.

>  - the "get" in "ngx_http_chunked_get_trailers" is sort of misleading.
> The "get" here actually
>means "generate". I thought you are looking for a existing buffer.

That function name got changed a few times, since I cannot find one
that I would be 100% happy with... Any suggestions?

>   Now go back to the long list of trailer support,  I believe it needs to
> cover at least:
> 1) generate trailer headers and sent to downstream/visitor as you just
> did
> 2) reverse proxy propagate trailer headers from upstream, which needs
> 2.0 parse the incoming trailer header, both considering the body is
> buffered and not buffered,
> 2.1. make the incoming trailer accessible via variable after content
> phase,
> 2.2. if response body is is not buffered, combine incoming trailer
> header with the generated headers.
> 2.3. convert incoming trailer to regular header if response body is
> buffered
>
>3|4|..|n) other features.
>
>I think 2.x is much harder than 1). I only implement 1) and 2.0) and
> 2.1). While implement 2.1), I left some
> room for 2.2 and 2.3 for the future. Now that you have plan for 2.x as your
> email suggests, wouldn't it
> be nice to submit these code first, and then go ahead with the code of 1). I
> believe to support 2.2 or 2.3,
> your existing code in chunk module needs lots of change.

I don't see how anything you listed in 2.x would require modifications
to the code I submitted... Actually, everything you listed should be
handled in upstream and/or proxy modules, not chunking module, and it
could easily use the API (r->headers_out.trailers) I want to add in
this commit.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[shuxinyang]

Hi, Piotr:

 I took some time to read your code, I believe I fully understand 
your code at this moment.
My implementation is slightly differently from yours, and we have 
different use-cases.


 Here is my $0.02 value of comment to your code (cosmetic wise):
 - the verb "support" in "trailer support" in too general. The 
trailer-support in my mind include

   a long list of features (see bellow)

 - the "get" in "ngx_http_chunked_get_trailers" is sort of 
misleading. The "get" here actually

   means "generate". I thought you are looking for a existing buffer.

  Now go back to the long list of trailer support,  I believe it needs 
to cover at least:
1) generate trailer headers and sent to downstream/visitor as you 
just did

2) reverse proxy propagate trailer headers from upstream, which needs
2.0 parse the incoming trailer header, both considering the 
body is buffered and not buffered,
2.1. make the incoming trailer accessible via variable after 
content phase,
2.2. if response body is is not buffered, combine incoming 
trailer header with the generated headers.
2.3. convert incoming trailer to regular header if response 
body is buffered


   3|4|..|n) other features.

   I think 2.x is much harder than 1). I only implement 1) and 2.0) and 
2.1). While implement 2.1), I left some
room for 2.2 and 2.3 for the future. Now that you have plan for 2.x as 
your email suggests, wouldn't it
be nice to submit these code first, and then go ahead with the code of 
1). I believe to support 2.2 or 2.3,

your existing code in chunk module needs lots of change.

Thanks
Shuxin


On 06/27/2016 12:14 PM, Piotr Sikora wrote:

Hey Shuxin,


   As far as I can understand, your change is just to add trailer headers
(not including the part that paring incoming
trailer header from upstream, or merge the incoming trailer and generated
trailer). If that is correct, you just need
to add "trailer: hdr1,hdr2...  hdrn" to the out-headers.

Did you look at both patches I've sent?
http://mailman.nginx.org/pipermail/nginx-devel/2016-June/008429.html
http://mailman.nginx.org/pipermail/nginx-devel/2016-June/008430.html

Because they cover much more than just adding "Trailer" header.


TE is for something
else as Maxim pointed out,
and adding this header can be done in chunked-filter-module as well.

"TE" is a _request_ header, so I don't see how adding it to response
is relevant here.

And yes, "TE" header could be parsed in chunked filter, but it's IMHO
wrong place to do it, since you need to parse this header in HTTP/2
requests as well.


   My previous implementation of generating trailer header is completely done
in chunk-module.  Later on, I change
my mind, and add a standalone module along with minor change to configure
script.

Does you implementation support trailers in HTTP/2 as well?

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel


___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Shuxin,

>   As far as I can understand, your change is just to add trailer headers
> (not including the part that paring incoming
> trailer header from upstream, or merge the incoming trailer and generated
> trailer). If that is correct, you just need
> to add "trailer: hdr1,hdr2...  hdrn" to the out-headers.

Did you look at both patches I've sent?
http://mailman.nginx.org/pipermail/nginx-devel/2016-June/008429.html
http://mailman.nginx.org/pipermail/nginx-devel/2016-June/008430.html

Because they cover much more than just adding "Trailer" header.

> TE is for something
> else as Maxim pointed out,
> and adding this header can be done in chunked-filter-module as well.

"TE" is a _request_ header, so I don't see how adding it to response
is relevant here.

And yes, "TE" header could be parsed in chunked filter, but it's IMHO
wrong place to do it, since you need to parse this header in HTTP/2
requests as well.

>   My previous implementation of generating trailer header is completely done
> in chunk-module.  Later on, I change
> my mind, and add a standalone module along with minor change to configure
> script.

Does you implementation support trailers in HTTP/2 as well?

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> Last time I checked gRPC wasn't able to talk HTTP/1.1, and
> therefore wasn't able to work with nginx at all.  So no real-world
> use for now, right?

Official clients & servers support only HTTP/2, but gRPC protocol can
be layered on top of HTTP/1.1 with trailers, so it's up to the
implementation to offer gRPC-over-HTTP/1.1 support.

As for HTTP/2 trailers, the code I submitted is already used to send
gRPC-over-HTTP/2, so it's quite real-world use case.

> I disagree here.  "TE: trailers" doesn't mean that client asked
> for trailers, it instead indicates that client supports trailers.
> Quoting RFC 7230:
>
>The "TE" header field in a request indicates what transfer codings,
>besides chunked, the client is willing to accept in response, and
>whether or not the client is willing to accept trailer fields in a
>chunked transfer coding.
>
> Using chunked for all clients who send "TE: trailers" looks like a
> bad idea for me, it will hurt at least some real-world clients.

In theory, you're right...

In practice, I'm not aware of any generic-purpose client sending "TE:
trailers" header (i.e. browsers don't send it, even though at least
some support trailers), and it's used only by clients that want to
receive trailers.

> At least in case of proxy, trailers are expected to appear if and
> only if protocol used to talk to the upstream server supports
> them.  That is, it's only expected to happen when talking to a
> backend via HTTP/1.1, and the response uses chunked encoding.

NGINX can add them with "add_trailer", which works even with HTTP/1.0
backends... But point taken.

Would adding r->trailers_emit (or r->expect_trailers, whichever you
prefer) make you happy?

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[shuxinyang]

Hi, Piotr and Maxim:

> to Maxim's question about "does trailer has real-world use" first.

At least we do, some info is available only after the entire body is 
sent. Trailer is a viable and convenient
solution for that matter, albeit it is not the only solution for it.  I 
cannot "leak" more info as I don't know if
I'm allowed say more about it. The bottom line is we did use trailer 
header in some situations.


To Piotr:

  > parsing of "TE" header is done in 
  As far as I can understand, your change is just to add trailer 
headers (not including the part that paring incoming
trailer header from upstream, or merge the incoming trailer and 
generated trailer). If that is correct, you just need
to add "trailer: hdr1,hdr2...  hdrn" to the out-headers. TE is for 
something else as Maxim pointed out,

and adding this header can be done in chunked-filter-module as well.

  My previous implementation of generating trailer header is completely 
done in chunk-module.  Later on, I change
my mind, and add a standalone module along with minor change to 
configure script.


Thanks
Shuxin



On 06/27/2016 10:13 AM, Piotr Sikora wrote:

Hey Shuxin,


I'm wondering why not just change the ngx_http_chunked_filter_module.c?
or add a module inserted right after the chunked-filter-module?

Hmm...? I'm confused by your comment.

This patch modifies 3 files:
- parsing of "TE" header is done in ngx_http_request.c,
- HTTP/1.1 trailers are handled in ngx_http_chunked_filter_module.c,
- HTTP/2 trailers are handled in ngx_http_v2_filter_module.c.

Are you suggesting that HTTP/2 trailers should be handled in
ngx_http_chunked_filter_module.c?

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel


___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Mon, Jun 27, 2016 at 07:37:31AM -0700, Piotr Sikora wrote:

> > What's the goal?  Any real-world use for this?
> 
> For us? gRPC, which uses HTTP/2 and/or HTTP/1.1 with trailers as a
> transport protocol.

Last time I checked gRPC wasn't able to talk HTTP/1.1, and 
therefore wasn't able to work with nginx at all.  So no real-world 
use for now, right?

> > As far as I understand the patch, this will cause chunked encoding
> > to be used for all responses to a client which supports trailers.
> > This certainly looks like a bad idea.
> 
> It will force chunked encoding in responses to HTTP/1.1 requests with
> "TE: trailers", i.e. only when HTTP/1.1 client explicitly asked for
> trailers.

I disagree here.  "TE: trailers" doesn't mean that client asked 
for trailers, it instead indicates that client supports trailers.  
Quoting RFC 7230:

   The "TE" header field in a request indicates what transfer codings,
   besides chunked, the client is willing to accept in response, and
   whether or not the client is willing to accept trailer fields in a
   chunked transfer coding.

Using chunked for all clients who send "TE: trailers" looks like a 
bad idea for me, it will hurt at least some real-world clients.

> Since, at this point (i.e. while processing headers), we don't know
> whether there will be any trailers after response body (because proxy*
> and/or 3rd-party modules might add them later), forcing chunked
> encoding for all clients that asked for trailers is the most
> reasonable thing we can do.
> 
> Alternatively, we could add an indicator (i.e. r->trailers_emit) that
> proxy* and/or 3rd-party modules could set in case they expect to emit
> trailers, but to be honest, I feel that it would be just set to 1
> (unless we want to whitelist which trailers are passed down from
> upstream).

At least in case of proxy, trailers are expected to appear if and 
only if protocol used to talk to the upstream server supports 
them.  That is, it's only expected to happen when talking to a 
backend via HTTP/1.1, and the response uses chunked encoding.

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Shuxin,

>I'm wondering why not just change the ngx_http_chunked_filter_module.c?
> or add a module inserted right after the chunked-filter-module?

Hmm...? I'm confused by your comment.

This patch modifies 3 files:
- parsing of "TE" header is done in ngx_http_request.c,
- HTTP/1.1 trailers are handled in ngx_http_chunked_filter_module.c,
- HTTP/2 trailers are handled in ngx_http_v2_filter_module.c.

Are you suggesting that HTTP/2 trailers should be handled in
ngx_http_chunked_filter_module.c?

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[shuxinyang]

Hi, Piotr:

   I'm wondering why not just change the ngx_http_chunked_filter_module.c?
or add a module inserted right after the chunked-filter-module?

Shuxin

On 06/27/2016 07:37 AM, Piotr Sikora wrote:

Hey Maxim,


What's the goal?  Any real-world use for this?




On 06/27/2016 06:45 AM, Maxim Dounin wrote:

Hello!

On Sun, Jun 26, 2016 at 04:13:15PM -0700, Piotr Sikora wrote:


# HG changeset patch
# User Piotr Sikora 
# Date 1466735480 25200
#  Thu Jun 23 19:31:20 2016 -0700
# Node ID a96187a9806536ab2a8bf3fa7f7b659cf5d3ff13
# Parent  6f69e3c0f780e29bca752fc1f938f4a459a1ec59
HTTP: add support for trailers in HTTP responses.

Example:

ngx_table_elt_t  *h;

h = ngx_list_push(>headers_out.trailers);
if (h == NULL) {
return NGX_ERROR;
}

ngx_str_set(>key, "Fun");
ngx_str_set(>value, "with trailers");

For us? gRPC, which uses HTTP/2 and/or HTTP/1.1 with trailers as a
transport protocol.


As far as I understand the patch, this will cause chunked encoding
to be used for all responses to a client which supports trailers.
This certainly looks like a bad idea.

It will force chunked encoding in responses to HTTP/1.1 requests with
"TE: trailers", i.e. only when HTTP/1.1 client explicitly asked for
trailers.

Since, at this point (i.e. while processing headers), we don't know
whether there will be any trailers after response body (because proxy*
and/or 3rd-party modules might add them later), forcing chunked
encoding for all clients that asked for trailers is the most
reasonable thing we can do.

Alternatively, we could add an indicator (i.e. r->trailers_emit) that
proxy* and/or 3rd-party modules could set in case they expect to emit
trailers, but to be honest, I feel that it would be just set to 1
(unless we want to whitelist which trailers are passed down from
upstream).

* I'm going to work on proxy support for trailers at some point in
future, if this gets merged.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel


___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> What's the goal?  Any real-world use for this?

For us? gRPC, which uses HTTP/2 and/or HTTP/1.1 with trailers as a
transport protocol.

> As far as I understand the patch, this will cause chunked encoding
> to be used for all responses to a client which supports trailers.
> This certainly looks like a bad idea.

It will force chunked encoding in responses to HTTP/1.1 requests with
"TE: trailers", i.e. only when HTTP/1.1 client explicitly asked for
trailers.

Since, at this point (i.e. while processing headers), we don't know
whether there will be any trailers after response body (because proxy*
and/or 3rd-party modules might add them later), forcing chunked
encoding for all clients that asked for trailers is the most
reasonable thing we can do.

Alternatively, we could add an indicator (i.e. r->trailers_emit) that
proxy* and/or 3rd-party modules could set in case they expect to emit
trailers, but to be honest, I feel that it would be just set to 1
(unless we want to whitelist which trailers are passed down from
upstream).

* I'm going to work on proxy support for trailers at some point in
future, if this gets merged.

Best regards,
Piotr Sikora

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Sun, Jun 26, 2016 at 04:13:15PM -0700, Piotr Sikora wrote:

> # HG changeset patch
> # User Piotr Sikora 
> # Date 1466735480 25200
> #  Thu Jun 23 19:31:20 2016 -0700
> # Node ID a96187a9806536ab2a8bf3fa7f7b659cf5d3ff13
> # Parent  6f69e3c0f780e29bca752fc1f938f4a459a1ec59
> HTTP: add support for trailers in HTTP responses.
> 
> Example:
> 
>ngx_table_elt_t  *h;
> 
>h = ngx_list_push(>headers_out.trailers);
>if (h == NULL) {
>return NGX_ERROR;
>}
> 
>ngx_str_set(>key, "Fun");
>ngx_str_set(>value, "with trailers");
>h->hash = ngx_hash_key_lc(h->key.data, h->key.len);
> 
> The code above adds "Fun: with trailers" trailer to the response to
> the request with "TE: trailers" header (which indicates support for
> trailers).
> 
> Note that trailers MUST be added before last buffer of the response
> (last_buf = 1) reaches chunked or v2 output filters, otherwise they
> are going to be ignored.
> 
> Signed-off-by: Piotr Sikora 

What's the goal?  Any real-world use for this?

> 
> diff -r 6f69e3c0f780 -r a96187a98065 
> src/http/modules/ngx_http_chunked_filter_module.c
> --- a/src/http/modules/ngx_http_chunked_filter_module.c
> +++ b/src/http/modules/ngx_http_chunked_filter_module.c
> @@ -17,6 +17,7 @@ typedef struct {
>  
>  
>  static ngx_int_t ngx_http_chunked_filter_init(ngx_conf_t *cf);
> +static ngx_chain_t *ngx_http_chunked_get_trailers(ngx_http_request_t *r);
>  
>  
>  static ngx_http_module_t  ngx_http_chunked_filter_module_ctx = {
> @@ -69,28 +70,31 @@ ngx_http_chunked_header_filter(ngx_http_
>  return ngx_http_next_header_filter(r);
>  }
>  
> -if (r->headers_out.content_length_n == -1) {
> +clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
> +
> +if (clcf->chunked_transfer_encoding && r->trailers_ok) {
> +ngx_http_clear_content_length(r);
> +r->chunked = 1;

As far as I understand the patch, this will cause chunked encoding 
to be used for all responses to a client which supports trailers.  
This certainly looks like a bad idea.

[...]

-- 
Maxim Dounin
http://nginx.org/

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel