http/2: remove upstream max concurrent streams check

[jwfang]

try to implement #963 , for preview

current only upstream/client side of max stream config,

other thing to be done:

1. listener side config and nghttp2 setting;
2. docs

[mattklein123]

@jwfang in thinking about this a bit more, I think we should just delete the check on the client side connection pool: https://github.com/lyft/envoy/blob/master/source/common/http/http2/conn_pool.cc#L85

Basically, delete that line and remove all occurrences of maxConcurrentStreams() function. The reason for this is that we already have protection via circuit breakers. There is no point in having additional protection (other than potentially for push, which we don't support currently, and can deal with later). If we overflow the other side, the code will correctly handle the REFUSED_STREAM reset that happens.

This should still be configurable on the listener side of things, but that could be done in a different change if you want if that is not a problem that you have.

[jwfang]

great, that's what i am confused too.

in theory, for upstream connection, i think we need to check against MAX_STREAM setting from the server, not envoy's limit.
i guess envoy's upstream limit is another net previously. since we don't need it any more, this cleared my confusion.

i will do it as you suggested when at the office. for listener side MAX_STREAM, can you give me some guide ?

[jwfang]

BTW, do you know what's the behaviour for nghttp2 when we overflow MAX_STREAM?
for both sending and receiving.

[mattklein123]

This is a little confusing so I will discuss upstream and listener side independently.

Both sides: it comes down to actually setting max concurrent streams in the http/2 settings frame here: https://github.com/lyft/envoy/blob/master/source/common/http/http2/codec_impl.cc#L509. Currently we do this the same for both client and server connections. The client side connection actually doesn't do anything, since this only affects the number of allowed incoming streams. It would effect push streams, but we don't support that currently, so it's effectively NOP.

Upstream: So for upstream, the settings frame basically does nothing, which is why the check should be removed. I think I originally added this since we run Envoy everywhere in an http/2 mesh and at some point it made sense to do an early check since I know what the other side is doing. However in general, this makes no sense, so just kill the check entirely. IIRC nghttp2 will auto fail a new stream locally if it knows the remote side can't take it. Envoy will correctly 503 the request. Optimally, we would listen to the incoming settings frame from the other side, and know what the limit is, and then fail it, with a good stat. We could file a different issue on doing that.

Downstream/listener: If you don't care about this for your use case, I would just ignore in this PR and we can deal with it some other time and just note in the issue. If you do care, you basically have to plumb through the limit from the config all the way to the codec. Basically, figure out how uint64_t codec_options gets to the codec from the listener. Switch that to a struct CodecOptions which contains both some flags (current options) as well as the ability to set max concurrent streams. Then just plumb that all the way through and use it to fill in the outbound settings.

[jwfang]

thanks for your detailed explanation.

then are we going to remove the hard-coded max stream check ( this pull request)?
and then fire two separate issues for:

1. upstream using peer max stream setting
2. listeners advertise envoy's max stream setting

the later two issues maybe a little hard for me at the moment, but i will try to see if i can help.

[jwfang]

added #988 and #987

[mattklein123]

@jwfang thanks, I think this is the right fix. Please sign the CLA: https://oss.lyft.com/cla/#/signature

[jwfang]

@mattklein123 signed

[mattklein123]

Thanks!