http: HCM changes to support rate limited / metered data

[mattklein123]

In thinking about actually implementing the rate rate limiting portion of #5942, I realized that we don't really have any good way to do what is required for either rate limiting body data or generally building a filter that emits data on a periodic basis, while potentially still buffering/watermarking incoming data.

The most well-formed thought I have right now (which isn't very formed) is something like: add explicit decodeData(...) / encodeData(...) calls which allow a decoding / encoding filter to directly inject data to further filters. This is different from commonContinue() in the sense that we would not be using HCM buffering. This would allow buffering/watermarking of incoming data to be disjoint from continued data. The downside to this is I don't think it would correctly work if a further filter buffers.

I've also considered some capability to continue only a portion of the buffered data using some kind of buffer slice, but that also seems very complicated.

I'm going to think about this more tomorrow, but I figured I would open an issue to see if anyone that has been working in this space recently has thought about this already or has any good ideas. If not I will report back! cc @alyssawilk @snowp @soya3129 @lizan

[mattklein123]

I've been thinking about this, and I cannot come up with any solution that would have the HCM do all the buffering without a large amount of changes at very considerable regression risk.

Therefore, I'm thinking about implementing any required buffering inside the fault filter and just manually doing the watermarking given the configured buffer limit (@alyssawilk conveniently provided all of the interface functions to do this). the only downside that I see to this is that if a further filter buffers, we might end up buffering too much if we don't somehow account for all of the buffering. IMO this is an edge case that we can likely avoid in v1.

I think there is still going to be some minor HCM work to do to allow a filter to "continue" data to further filters when it had not been paused (e.g., when trickling out data on a timer), however I think these changes will be minor and I will develop them in parallel and introduce a PR for those changes when they are ready.

Let me know if anyone has any thoughts on this approach.

[alyssawilk]

If the desire is to simulate slow connections, I think you need to do this on a per network::connection basis not on a per-stream basis. The bookkeeping trying to emulate a slow network with multiple streams is a total pain.

On ingress we already limit reads, and could delay the next-read alarm based on the configured throughput. On the egress path, I believe we can use the existing kernel option SO_MAX_PACING_RATE for linux, or we could be cross-platform and do that in user-space as well.

[mattklein123]

I think you need to do this on a per network::connection basis not on a per-stream basis

Per the linked issue, for usability reasons it's not practical to do what we want to do at the L4 level so it needs to be done at the L7 level. In any case, I think I have a plan to do this that should not require any invasive changes to HCM.

[alyssawilk]

I totally get that for turning throttles up it makes sense to do the expedient thing and use HTTP headers.
I don't understand why that should affect the implementation. Can you clue me in?

[alyssawilk]

A bit more specifically, if you tweak your app to say "use LTE level bandwidth" via HTTP headers but you happen to have 50 streams on a connection, I think you're going to want to have the underlying TCP state have LTE-compatible bandwidth, not have each stream rate limited separately, because that won't actually mimic the behavior you're trying to elicit, will it?

[mattklein123]

A bit more specifically, if you tweak your app to say "use LTE level bandwidth" via HTTP headers but you happen to have 50 streams on a connection, I think you're going to want to have the underlying TCP state have LTE-compatible bandwidth, not have each stream rate limited separately, because that won't actually mimic the behavior you're trying to elicit, will it?

I agree that would be technically more correct, but IMO the implementation will be more difficult for a couple of different reasons:

1. IIRC SO_MAX_PACING_RATE is Linux specific as you point out, but I think also depends on a specific qdisc which I'm not sure is the default and also needs to be configured, so I'm not sure we can rely on it being available. So this would mean we either need to build rate limiting into the connection object itself (and give access to that capability from HTTP filters), or build an L4 rate limit filter and have that interact with the L7 filter, keeping track of which headers were set when. Honestly it sounds very complicated (I know I said trying to do this in the HCM is very complicated, but that was only because I was trying to have it work with buffering in the HCM itself, when you take that out, it's not that complicated).
2. If we are setting headers on a per-stream basis, IMO it makes more sense to do enforcement on a per-stream basis, and this also fits much better with its implementation in the fault filter itself, in which we might want to only inject faults on particular routes/streams/etc.

I agree that for the app/slow-network use case, it's imperfect if we are trying to simulate the entire connection, but IMO it's an OK tradeoff since at the app layer we can guarantee that the headers are set for all streams, and then turned off if we want, even with an open connection (on the client the app level code doesn't always cleanly interact with the underlying connection pools, so it's easier to just stop setting headers when we want the behavior to go away).

[alyssawilk]

So still in brainstorming mode here, if we don't like SO_MAX_PACING_RATE due to platform constraints, what about making it a transport socket thing? I think it'd be useful for testing backup on both L7 and L4 as well as slow H1 upstreams (once we got around to configuration options for the others), it'd keep the implementation in one place, it should be fairly simple to just fake EAGAINs and set a timer to resume, and it'd have the benefits of more accurate simulation for multiplexed connections as mentioned above.

[mattklein123]

I guess here is how I'm thinking about it: I do think that we should support L4 rate limiting, either at the network filter or transport socket layer.

At the same time, I think adding L7 rate limiting, both configurable via normal config and headers in the fault filter is a reasonable feature addition on its own (due to standard fault filter per-route/stream/etc. settings), and this feature is enough to do what Lyft needs (and in fact is simpler for us).

What that said, I'm inclined to continue forward and build it into the fault filter and we can track the L4 rate limit support separately. WDYT?

[alyssawilk]

I think having it done at the L7 layer is going to be more trouble than it's worth for folks who don't grok H2, but given that it'll work for you and isn't going to add complexity to the HCM, feel free to go with it (and please just comment up the muxing issues for those who don't understand H2 and the implications of doing this at L7 as well as you do! =P)