upstream: Allow users of HttpPoolData to access the underlying pool.

[mum4k]

A recent change (#16544) moved the Http::ConnectionPool::Instance* returned from ThreadLocalCluster::httpConnPool() behind a new class HttpPoolData which no longer exposes the pool instance.

Nighthawk depends on the pool when terminating a benchmark here.

This PR proposes a HttpPoolData::pool() method which exposes the pool to Nighthawk in order to retain the original functionality. Happy to discuss alternatives if this isn't acceptable. Note - this breakage in Nighthawk will prevent an Envoy import into Google until / unless we find a workaround.

Risk Level: low, existing functionality isn't affected.
Testing: n/a, only accessor method added.
Docs Changes: n/a.
Release Notes: n/a.

Signed-off-by: Jakub Sobon mumak@google.com

[mum4k]

@alyssawilk as fyi and to propose alternatives if needed.

[mum4k]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16829 (comment) was created by @mum4k.

see: more, trace.

[alyssawilk]

Ooh sorry about that breakage. I wonder if we should have an at least advisory nighthawk build in Envoy CI?

Anyway, addressing today's problem, the goal here was to remove pool access because then folks can create streams directly, so adding an accessor would undo that. How about adding a wrapper function for adding a callback which nighthawk can call?

[mum4k]

@alyssawilk thank you for responding and don't worry about the breakage. As you pointed out, you really had no way of knowing.

Turns out @yanavlasov was also working on this problem and came up with a solution via a pre-existing friend class that is purely local to Nighthawk (so for now preferred):
https://github.com/envoyproxy/nighthawk/blob/3d8e9a91e4740b3635c67a617d93c94eeaf1e67b/source/client/benchmark_client_impl.h#L28-L41

My plan is to close this PR and discuss with @yanavlasov as to how we want to take this forward.

I am really glad that you mentioned an advisory Nighthawk build in the Envoy CI. That is something we would very much like. Do we need to start a wider discussion before we send a PR adding one?

[alyssawilk]

glad you're sorted via the other PR. :-)

For CI, I'd say if nighthawk breaks once a quarter it may not justify the CI cost. If it's more regular you could open an issue just to get other opinions (see if anyone objects really) and then do something like we do for Envoy filter examples, where we have that build as part of Envoy CI.