hystrix: emit ":no-chunks" pseudo-header only in HTTP/1.x streams.

[PiotrSikora]

It appears that we've added a custom "Envoy only" pseudo-header,
which is clearly forbidden by the HTTP/2 spec.

It's only used by the Hystrix Dashboard Event Stream, so presumably
it's not widely used, but it's been breaking some intermediaries.

Signed-off-by: Piotr Sikora piotrsikora@google.com

[PiotrSikora]

Reviving #9093.

[mattklein123]

@PiotrSikora I still don't understand how this is breaking anyone. This header is never written to the wire. Can you please explain how this is breaking anyone? Thank you.

[PiotrSikora]

@mattklein123 it's sent on the wire when using HTTP/2.

Add HystrixSink to your config in order to enable /hystrix_event_stream endpoint:

stats_sinks:
- name: envoy.stat_sinks.hystrix
  config:
    num_buckets: 10

$ nghttp -nv http://localhost:9901/hystrix_event_stream

[  0.000] Connected
[  0.000] send SETTINGS frame <length=12, flags=0x00, stream_id=0>
          (niv=2)
          [SETTINGS_MAX_CONCURRENT_STREAMS(0x03):100]
          [SETTINGS_INITIAL_WINDOW_SIZE(0x04):65535]
[  0.000] send PRIORITY frame <length=5, flags=0x00, stream_id=3>
          (dep_stream_id=0, weight=201, exclusive=0)
[  0.000] send PRIORITY frame <length=5, flags=0x00, stream_id=5>
          (dep_stream_id=0, weight=101, exclusive=0)
[  0.000] send PRIORITY frame <length=5, flags=0x00, stream_id=7>
          (dep_stream_id=0, weight=1, exclusive=0)
[  0.000] send PRIORITY frame <length=5, flags=0x00, stream_id=9>
          (dep_stream_id=7, weight=1, exclusive=0)
[  0.000] send PRIORITY frame <length=5, flags=0x00, stream_id=11>
          (dep_stream_id=3, weight=1, exclusive=0)
[  0.000] send HEADERS frame <length=54, flags=0x25, stream_id=13>
          ; END_STREAM | END_HEADERS | PRIORITY
          (padlen=0, dep_stream_id=11, weight=16, exclusive=0)
          ; Open new stream
          :method: GET
          :path: /hystrix_event_stream
          :scheme: http
          :authority: 127.0.0.1:9901
          accept: */*
          accept-encoding: gzip, deflate
          user-agent: nghttp2/1.18.1
[  0.021] recv SETTINGS frame <length=6, flags=0x00, stream_id=0>
          (niv=1)
          [SETTINGS_INITIAL_WINDOW_SIZE(0x04):268435456]
[  0.021] recv SETTINGS frame <length=0, flags=0x01, stream_id=0>
          ; ACK
          (niv=0)
[  0.021] recv WINDOW_UPDATE frame <length=4, flags=0x00, stream_id=0>
          (window_size_increment=268369921)
[  0.021] [ERROR] Invalid HTTP header field was received: frame type: 1, stream: 13, name: [:no-chunks], value: [0]
[  0.021] [INVALID; error=Invalid HTTP header field was received] recv HEADERS frame <length=155, flags=0x04, stream_id=13>
          ; END_HEADERS
          (padlen=0)
          ; First response header
[  0.021] send SETTINGS frame <length=0, flags=0x01, stream_id=0>
          ; ACK
          (niv=0)
[  0.021] send RST_STREAM frame <length=4, flags=0x00, stream_id=13>
          (error_code=PROTOCOL_ERROR(0x01))
[  0.021] send GOAWAY frame <length=8, flags=0x00, stream_id=0>
          (last_stream_id=0, error_code=NO_ERROR(0x00), opaque_data(0)=[])
Some requests were not processed. total=1, processed=0

[mattklein123]

@mattklein123 it's sent on the wire when using HTTP/2.

Ah OK, sorry, I forget that we now allow admin over HTTP/2.

TBH, I never should have allowed this hack in the first place. I think the correct way of handling this is to have some attribute API that is plumbed through all the way to the encoder, either in stream info or otherwise, that is then inspected by the HTTP/1 codec. So I think there are probably two options here:

1. If you want to do the "right" thing and actually fix this, please do.
2. I'm not crazy about setting a header that goes out onto the wire, so as an interim hack, can you just check if it's HTTP/2 in the hystrix handler and just not set the pseudo header, with a TODO to clean up this entire thing? We can open an issue on doing the real fix.

WDYT?

/wait

[PiotrSikora]

I'm going to do (2), since (1) is going to take way more time that I have right now.

Filled #9749 to track (1).

[PiotrSikora]

@mattklein123 Done. I removed the deprecation and runtime flags, since it sounds like this was never supposed to be sent on the wire, so it's purely a bugfix. PTAL.