dispatcher: add the client connection factory to support connecting to internal listener

[lambdai]

Commit Message:
Implements internal listener. This PR duplicates some section about internal listener config in #18104
The major functionality of this PR is to create client connection to an internal listener.

Additional Description:
Risk Level: LOW. A default client connection factory should follow the previous flow of creating TCP/Pipe connection.

Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18105 was opened by lambdai.

see: more, trace.

[lambdai]

/assign @yanavlasov

[yanavlasov]

I think the approach is sound.

/wait

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[lambdai]

ack

[lambdai]

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #18105 (comment) was created by @lambdai.

see: more, trace.

[antoniovicente]

Could we avoid use of the dispatcher interface as a registration mechanism for factories like this one?

[lambdai]

It seems there was slight agreement on this so I stopped investing on bootstrap extension.

My opinion is that adding this interface to dispatcher is more straightforward, well, at the cost of adding getter/setter

CC @yanavlasov

[antoniovicente]

I think we need a client connection factory extension point that connection pools and other things can use to create connections. An eventual goal may be to completely remove the client connection creation methods from the dispatcher. But this is something that @envoyproxy/senior-maintainers may want to chime in on.

[lambdai]

I see two arguments here,
the first is to store the internal listener registry, here I choose dispatcher over bootstrap extension(where we can add a threadlocal registery), because the life time of dispatcher and listener manager is cleaner. The dispatcher is guaranteed existing when an internal listener is ready to add and use.

I think we can add the complexity in booting the server, and offer a hook for internal listener registry if we choose to store the internal listeners there. I feel we can do that after.

the second is to remove the thin dispatcher::createClientConnection. Again we can do that along with this PR but I prefer to do it later when signed off.

[antoniovicente]

The current approach of adding the internal listener registry to the dispatcher does not generalize to the next extension that needs to hook into the code that creates client connections. I recommend introducing a factory mechanism for client connections and transition to it, eventually deprecating the current Dispatcher::createClientConnection

[yanavlasov]

I agree with @antoniovicente that the extension behavior has leaked into core interfaces. II think it would be cleaner if the mechanism for finding the listener was completely encapsulated in the factory. For example when internal listener is created it is registering itself in the factory's thread local storage, where it can later be looked up for establishing client connections. In this way you can avoid needing to change the dispatcher interface and ConnectionHandlerImpl

[lambdai]

Chatted offline and I will introduce a thread local storage slot for the internal listener registry

It seems we can do that either in ListenerManager or create another bootstrap extension.

This effort will be addressed in PR(back fill here) and revisit this PR after

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!