Full picture of in memory listener and connection by lambdai · Pull Request #13361 · envoyproxy/envoy

lambdai · 2020-10-01T17:00:23Z

A almost rewrite of #12537

Prototype of #11725

Implementations

buffered_io_socket_handle_impl
Replaced the BufferSource transport socket. So existing transport socket could be supported. Special thank to @florincoras 's work dropping the file descriptor in transport sockets and other filters!
This is an socket handle which owns a buffer. Think the buffer as the socket buffer in the kernel. This handle should be always created with a peer so that this handle could write to peer's buffer, and notify readable or writable signal along with read/write/shutdown.
new api in dispatcher
Allow register internal listener and target listener when a client connection need to be created
new internal listener impl along with connection_handler
Register internal listener by name and "accept" connection. Currently internal listener could simulate tcp listener but not udp listener.

Past work

Address
Envoy Internal address type is introduced in api: add envoy internal address #12837~~A new type of address, or specialize the ip/pipe?~~
version 1 of the implementation
[WIP] Envoy Internal connection strawman #12537 Dropped since I force pushed...

Signed-off-by: Yuchen Dai <[email protected]>

…ernal and Tcp listener Signed-off-by: Yuchen Dai <[email protected]>

Signed-off-by: Yuchen Dai <[email protected]>

repokitteh-read-only · 2020-10-01T17:00:30Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13361 was opened by lambdai.

see: more, trace.

lambdai · 2020-10-01T17:02:37Z

@alyssawilk Please ignore the verbose log. I will clean up the log along with individual PRs

alyssawilk

Hey @lambdai, it's really exciting to see progress on this one!

I think the overall structure is fine. My WIP pass comments are to see where we can reduce duplicated code - the auto-close connection I think can be cleaned up and I'm wondering if we can also do some around the ActiveInternal classes.
To get this landed I also think we're going to need to split this up, rather than have one giant PR. I wonder if we can land the BufferedIoSocketHandleImpl with utilitiy classes but minimal changes to core code. WDYT?

alyssawilk · 2020-10-05T17:06:45Z

include/envoy/event/dispatcher.h

+   * Creates an client internal connection. Does NOT initiate the connection;
+   * the caller must then call connect() on the returned Network::ClientConnection.
+   * @param internal_address supplies the internal address to connect to.
+   * @param local_address supplies an address to bind to or nullptr if no bind is necessary.


Maybe explain what binding is in internal address context?

Right. Will add that listener side connection could obtain the bind local address for debug.

I also want to add here that I want to introduce a generic "socket options" param in the future to deliver more information including "original address" which is supposed to be "ip/port" address raising this internal address.

include/envoy/network/listener.h

alyssawilk · 2020-10-05T17:08:50Z

source/common/event/dispatcher_impl.cc

+namespace {
+Network::Address::InstanceConstSharedPtr
+nextClientAddress(const Network::Address::InstanceConstSharedPtr& server_address) {
+  uint64_t id = 0;


did you mean this to be static? If so fix and test?

Ah, yes. It should be static. The local address is well used, only need to guarantee such address is not nullptr.
Look like you are fine with such a debug purpose address :)

source/common/event/dispatcher_impl.cc

alyssawilk · 2020-10-05T17:11:12Z

source/common/event/dispatcher_impl.cc

+  for (const auto& [name, _] : internal_listeners_) {
+    ENVOY_LOG_MISC(debug, "lambdai: p listener {}", name);
+  }
+  if (iter == internal_listeners_.end()) {


hm, I would have expected this in connect(). When we do normal IP style create and connect do we validate before connect?
no strong opinion either way, but if you prefer this maybe comment about where validation is done in include/ ?

Actually I think if you move this work to CONNECT you can skip the entire closingClientConnectionImpl which would be nice clean up

The reason I want to split ClosingClientConnectionImpl and ClientConnectionImpl:

The underlying BufferedIoSocketHandleImpl is aware of single transition: from "connected" to "disconnected".
If we set up the server connection in connect(), the client connection need to track "uninitialized" to "connected", "uninitialized" to "fail to connect".

BTW, The model internal connection I am borrowing from "socketpair()"

alyssawilk · 2020-10-05T17:12:10Z

source/common/event/dispatcher_impl.cc

+  // Find the internal listener callback. The listener will setup the server connection.
+  auto iter = internal_listeners_.find(internal_address->asString());
+  for (const auto& [name, _] : internal_listeners_) {
+    ENVOY_LOG_MISC(debug, "lambdai: p listener {}", name);


I'm inclined to do my next pass once you've had a chance to clean these up, and do a sweep to make sure the code is commented.

This PR is adding 3000 lines. Do you think it is realistic to merge this PR without splitting?

If not I prefer to keep the debug purpose log: I will backport the split PRs and see if the sub PRs breaks this overall PR: This PR has quite a few real world integration tests. These integration tests must pass but cannot be put in early sub PRs.

I think given the complexity it'd be better to split out what we can. It could be that it's just 2-3? As said I think the buffer code would split out pretty easily so let's give that a try and see what's left.
Also w.r.t. logging I think it's fine to leave many of the logs in (all the error corner case logs for example) and just move them from lambdai/LOG_MISC to real logs. It's fine to tackle that as the last step when the big PR is ready for review if you prefer.

alyssawilk · 2020-10-05T17:12:30Z

source/common/event/dispatcher_impl.cc

+  if (iter == internal_listeners_.end()) {
+    ENVOY_LOG_MISC(debug, "lambdai: no valid listener registered for envoy internal address {}",
+                   internal_address->asString());
+    return std::make_unique<Network::ClosingClientConnectionImpl>(*this, internal_address,


Also some of these are good to keep as debug logs, though not MISC_LOG

Absolutely. Will assign reasonable logger and level in the split PRs

lambdai

Thank you for the review! Let me add comments first

lambdai · 2020-10-05T18:42:22Z

include/envoy/event/dispatcher.h

+   * Creates an client internal connection. Does NOT initiate the connection;
+   * the caller must then call connect() on the returned Network::ClientConnection.
+   * @param internal_address supplies the internal address to connect to.
+   * @param local_address supplies an address to bind to or nullptr if no bind is necessary.


Right. Will add that listener side connection could obtain the bind local address for debug.

I also want to add here that I want to introduce a generic "socket options" param in the future to deliver more information including "original address" which is supposed to be "ip/port" address raising this internal address.

include/envoy/network/listener.h

lambdai · 2020-10-05T18:46:20Z

source/common/event/dispatcher_impl.cc

+namespace {
+Network::Address::InstanceConstSharedPtr
+nextClientAddress(const Network::Address::InstanceConstSharedPtr& server_address) {
+  uint64_t id = 0;


Ah, yes. It should be static. The local address is well used, only need to guarantee such address is not nullptr.
Look like you are fine with such a debug purpose address :)

source/common/event/dispatcher_impl.cc

lambdai · 2020-10-05T18:55:29Z

source/common/event/dispatcher_impl.cc

+  // Find the internal listener callback. The listener will setup the server connection.
+  auto iter = internal_listeners_.find(internal_address->asString());
+  for (const auto& [name, _] : internal_listeners_) {
+    ENVOY_LOG_MISC(debug, "lambdai: p listener {}", name);


This PR is adding 3000 lines. Do you think it is realistic to merge this PR without splitting?

If not I prefer to keep the debug purpose log: I will backport the split PRs and see if the sub PRs breaks this overall PR: This PR has quite a few real world integration tests. These integration tests must pass but cannot be put in early sub PRs.

lambdai · 2020-10-05T19:12:13Z

source/common/event/dispatcher_impl.cc

+  for (const auto& [name, _] : internal_listeners_) {
+    ENVOY_LOG_MISC(debug, "lambdai: p listener {}", name);
+  }
+  if (iter == internal_listeners_.end()) {


The reason I want to split ClosingClientConnectionImpl and ClientConnectionImpl:

The underlying BufferedIoSocketHandleImpl is aware of single transition: from "connected" to "disconnected".
If we set up the server connection in connect(), the client connection need to track "uninitialized" to "connected", "uninitialized" to "fail to connect".

BTW, The model internal connection I am borrowing from "socketpair()"

lambdai · 2020-10-05T19:12:56Z

source/common/event/dispatcher_impl.cc

+  if (iter == internal_listeners_.end()) {
+    ENVOY_LOG_MISC(debug, "lambdai: no valid listener registered for envoy internal address {}",
+                   internal_address->asString());
+    return std::make_unique<Network::ClosingClientConnectionImpl>(*this, internal_address,


Absolutely. Will assign reasonable logger and level in the split PRs

lambdai · 2020-10-28T20:34:10Z

source/common/event/dispatcher_impl.cc

+  auto client_io_handle_ = std::make_unique<Network::BufferedIoSocketHandleImpl>();
+  auto server_io_handle_ = std::make_unique<Network::BufferedIoSocketHandleImpl>();


BufferedIoSocketHandleImpl as extension here?

github-actions · 2020-12-09T03:10:52Z

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 · 2020-12-16T04:04:13Z

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!

lambdai added 30 commits August 27, 2020 05:17

core: add envoy internal address

fb1db47

Signed-off-by: Yuchen Dai <[email protected]>

listener: add internal listener api

6fceaf2

Signed-off-by: Yuchen Dai <[email protected]>

rename Network::ListenerImpl and Network::ListenerCallbacks

df05bba

Signed-off-by: Yuchen Dai <[email protected]>

rename file

d7cac74

Signed-off-by: Yuchen Dai <[email protected]>

scaffold of internal listener impl

be6f09d

Signed-off-by: Yuchen Dai <[email protected]>

add missing file

d305855

Signed-off-by: Yuchen Dai <[email protected]>

remove client address and add tests

b6d9da5

Signed-off-by: Yuchen Dai <[email protected]>

Merge remote-tracking branch 'origin/master' into eiaddr

c413df9

Signed-off-by: Yuchen Dai <[email protected]>

remove unimplement to fix docs

48e0640

Signed-off-by: Yuchen Dai <[email protected]>

guard internal address

e81a41e

Signed-off-by: Yuchen Dai <[email protected]>

Merge remote-tracking branch 'me/eiaddr' into eiaddr

72cd41d

Signed-off-by: Yuchen Dai <[email protected]>

add integration test for NullIoSocketHandleImpl

a337bde

Signed-off-by: Yuchen Dai <[email protected]>

add integration test for NullIoSocketHandleImpl

dc87455

Signed-off-by: Yuchen Dai <[email protected]>

Merge branch 'eiaddr' of github.com:lambdai/envoy-dai into eiaddr

3be784d

Signed-off-by: Yuchen Dai <[email protected]>

fix spelling

51914a9

Signed-off-by: Yuchen Dai <[email protected]>

Merge remote-tracking branch 'origin/master' into eiaddr

f12c3d2

Signed-off-by: Yuchen Dai <[email protected]>

Merge branch 'eiaddr' into eilistener

219e0da

Signed-off-by: Yuchen Dai <[email protected]>

incomplete impl of internal listener

f8816b0

Signed-off-by: Yuchen Dai <[email protected]>

adding active internal listener

bc57e78

Signed-off-by: Yuchen Dai <[email protected]>

add listener manager test

4c2d693

Signed-off-by: Yuchen Dai <[email protected]>

refactor ConnectionSocketImpl: has SocketImpl instead of inherit

cfca895

Signed-off-by: Yuchen Dai <[email protected]>

rename listener to stream_listener in tcpsocket

07c1ecb

Signed-off-by: Yuchen Dai <[email protected]>

extract generic listener filter

171b1d9

Signed-off-by: Yuchen Dai <[email protected]>

active internal socket

c99aa54

Signed-off-by: Yuchen Dai <[email protected]>

adding BufferedIoSocketHandleImpl, start with recv

b25f28d

Signed-off-by: Yuchen Dai <[email protected]>

format

73c5074

Signed-off-by: Yuchen Dai <[email protected]>

add buffered_io_socket_handle_test

8439a67

Signed-off-by: Yuchen Dai <[email protected]>

add flow control test

d7cff5c

Signed-off-by: Yuchen Dai <[email protected]>

add EventScheduleBasic test

808ea05

Signed-off-by: Yuchen Dai <[email protected]>

add more event test

50479ca

Signed-off-by: Yuchen Dai <[email protected]>

lambdai added 15 commits September 24, 2020 09:27

add test close

5fdec41

Signed-off-by: Yuchen Dai <[email protected]>

add shutdown test

b55e03e

Signed-off-by: Yuchen Dai <[email protected]>

fixing test: working on TestReadFlowAfterShutdownWrite

4a7970c

Signed-off-by: Yuchen Dai <[email protected]>

close buffered_io_socket_handle_impl_test

5ffedb4

Signed-off-by: Yuchen Dai <[email protected]>

Merge branch 'master' into eilistener

7a972a5

Signed-off-by: Yuchen Dai <[email protected]>

add basic closing client connection test

81d4c2e

Signed-off-by: Yuchen Dai <[email protected]>

add connection callback and active close test in CLosingClientImpl

32eca3f

Signed-off-by: Yuchen Dai <[email protected]>

dispatcher: create internal connection to nowhere

80a9d2c

Signed-off-by: Yuchen Dai <[email protected]>

fixing create connection

a5572cb

Signed-off-by: Yuchen Dai <[email protected]>

basic internal client connection test

254daa4

Signed-off-by: Yuchen Dai <[email protected]>

fix connect probe and add client write test

568e42b

Signed-off-by: Yuchen Dai <[email protected]>

various fixes: client id on dispatcher, StreamListener for active Int…

953326b

…ernal and Tcp listener Signed-off-by: Yuchen Dai <[email protected]>

adding back chain tcp proxy integration test

cd30f84

Signed-off-by: Yuchen Dai <[email protected]>

fix all internal listener integration test

95b5a54

Signed-off-by: Yuchen Dai <[email protected]>

fix test

abbffa5

Signed-off-by: Yuchen Dai <[email protected]>

repokitteh-read-only bot added the api label Oct 1, 2020

lambdai assigned alyssawilk Oct 1, 2020

alyssawilk reviewed Oct 5, 2020

View reviewed changes

lambdai commented Oct 5, 2020

View reviewed changes

lambdai mentioned this pull request Oct 7, 2020

Io socket handle for internal socket #13418

Closed

lambdai commented Oct 28, 2020

View reviewed changes

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020

github-actions bot closed this Dec 16, 2020

		auto client_io_handle_ = std::make_unique<Network::BufferedIoSocketHandleImpl>();
		auto server_io_handle_ = std::make_unique<Network::BufferedIoSocketHandleImpl>();

Conversation

lambdai commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 1, 2020

Uh oh!

lambdai commented Oct 1, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 9, 2020

Uh oh!

github-actions bot commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lambdai commented Oct 1, 2020 •

edited

Loading