listener: create internal listener by lambdai · Pull Request #18104 · envoyproxy/envoy

lambdai · 2021-09-13T22:08:18Z

Commit Message:
Implements internal listener.
This PR allows creating server connection but the abilitity to connect to such a listener is located in #18105.

Additional Description:
Risk Level: LOW. Not impacting existing TCP/UDP listener.

Testing: Mostly unit test since the dispatcher is not yet ready to create pair of internal connections.

Docs Changes:
Release Notes:
Platform Specific Features:
Runtime guard: envoy.reloadable_features.internal_address to allow/reject internal listener config.

[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only · 2021-09-13T22:08:21Z

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: #18104 was opened by lambdai.

see: more, trace.

lambdai · 2021-09-13T22:15:47Z

Talked to @yanavlasov about this PR.
/assign yanavlasov

yanavlasov

/wait

source/common/network/connection_impl.cc

source/common/upstream/upstream_impl.cc

test/integration/socket_interface_integration_test.cc

test/server/BUILD

test/server/active_internal_listener_test.cc

test/server/connection_handler_test.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only · 2021-11-05T07:20:15Z

@envoyproxy/senior-maintainers assignee is @ggreenway

🐱

Caused by: a #18104 (comment) was created by @rojkov.

see: more, trace.

ggreenway

Looks good overall

/wait

envoy/network/listener.h

source/server/active_internal_listener.cc

test/integration/internal_listener_integration_test.cc

ggreenway · 2021-11-08T20:37:13Z

envoy/network/listener.h

+/**
+ * Configuration for an internal listener.
+ */
+class InternalListenerConfig {


It looks like this isn't used yet for anything. Delete until it's needed?

This is used by the ConnectionHandler to determine if the listener type is Internal.
I feel this is cleaner than find the listen address and query the address type then dispatch by the address type.

In fact I almost make the ConnectionHandler not relying on the address type.

source/common/network/listen_socket_impl.h

source/common/tcp_proxy/tcp_proxy.cc

test/server/active_internal_listener_test.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-11-09T23:04:13Z

@ggreenway I resolve most of them except introducing the empty InternalListenerConfig interface. PTAL

ggreenway

I think you need to validate listener config to make sure all the tcp-socket specific options aren't set. Such as tcp-fast-open, socket options, keepalive, mptcp, reuse_port, etc.

envoy/network/listener.h

source/server/connection_handler_impl.cc

source/common/network/listen_socket_impl.h

yanavlasov · 2021-11-12T17:58:28Z

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-11-15T18:21:26Z

/retest

repokitteh-read-only · 2021-11-15T18:21:29Z

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

🐱

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

see: more, trace.

lambdai · 2021-11-15T18:52:27Z

/retest

repokitteh-read-only · 2021-11-15T18:52:31Z

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

🐱

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

see: more, trace.

lambdai · 2021-11-15T22:02:48Z

/retest

repokitteh-read-only · 2021-11-15T22:02:52Z

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

🐱

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

see: more, trace.

ggreenway

/wait-any

ggreenway · 2021-11-15T22:26:17Z

source/server/listener_impl.cc

 void ListenerImpl::buildInternalListener() {
  if (config_.address().has_envoy_internal_address()) {
    internal_listener_config_ = std::make_unique<Network::InternalListenerConfig>();
+    if (config_.has_connection_balance_config() || config_.enable_mptcp() ||


Does anything go wrong if this is thrown after buildFilterChains has already run? I can't remember if that has any interaction with the previous config on the listener. If there's any risk, you could move this into validateConfig() to do it earlier.

As long as the exception was thrown prior to ListenerImpl::initialize(), there is no risk to wrongly deem the exception as a signal of "listener is wamed up".

We don't cross the line here

api listener is handled early: Api listener config doesn't instantiate ListenerImpl but HttpApiListener
I add a test in listenerimpl pretending that some how a listenerImpl is constructed with Apilistener. Though in production it will never hit :)

A separate question to @junr03: should we reject api listener with internal address?
https://github.com/envoyproxy/envoy/blob/main/source/server/api_listener_impl.h#L39

ggreenway · 2021-11-15T22:30:47Z

source/server/listener_impl.cc

+    if (config_.has_connection_balance_config() || config_.enable_mptcp() ||
+        config_.has_enable_reuse_port() || config_.has_freebind() ||
+        config_.has_tcp_backlog_size() || config_.has_tcp_fast_open_queue_length() ||
+        config_.has_transparent()) {


|| config_.enable_mptcp(). And maybe has_api_listener()?

For all of the BoolValue's, I think it's still valid if it is present, but set to false.

This validation is going to be a big mess. Maybe it's not worth doing. Any guidance from @envoyproxy/api-shepherds on what validation to do for mutually-incompatible options?

re: it's still valid if it is present. Right, Let me fix immediately.

Regarding the high level validation, I remember we opened an issue to move the tcp only options to a dedicated tcp listener config, though there was no solid move. That would be the long term fix.
Another fact is that Udp listener doesn't validate tcp options, just ignore.

I will add the quick fix on the validation here and leave api shephereds to the final call.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-11-16T18:44:28Z

/retest

repokitteh-read-only · 2021-11-16T18:44:32Z

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

🐱

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

see: more, trace.

ggreenway

I think this is pretty good. It's possible that we'll want to change some of the details as you complete the other parts of this (large) feature, but this is good enough for moving on to the next steps.

lambdai · 2021-11-17T18:26:21Z

Thank you @ggreenway @yanavlasov

lambdai added 8 commits August 13, 2021 21:42

socket: add get sockopt for connect

d712a58

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add inteneral listener

1217008

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add inteneral listener

b673747

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge branch 'main' into ilisteneronconnect

bc6e8a3

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

merge master

0361f4a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

clean up

c5e1131

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge branch 'main' into ilisteneronconnect

713efa5

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

revert stub for advanced integration test

da47f12

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Sep 13, 2021

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

Closed

repokitteh-read-only bot assigned yanavlasov Sep 13, 2021

yanavlasov requested changes Sep 22, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2021

lambdai added 3 commits September 29, 2021 04:47

cleanup

427efc1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge branch 'main' into ilisteneronconnect

d17804a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

more

a6aca89

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Sep 29, 2021

lambdai added 9 commits October 5, 2021 05:55

add new listen socket type for internal listener

6361da0

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

WIP: listener address mutated to ip

6d50f2a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add integration test

254a12f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

listener: fix duplicate and isOpen of the listener socket

131eaa9

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add integration test

cb8f53d

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

stash

dc4116e

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add new test that destroy listener when connections are alive

f5692cf

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add test coverage

09d023c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

clear duplicated runtime value

84300f3

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai changed the title ~~[WIP] listener: create internal listener~~ listener: create internal listener Oct 6, 2021

lambdai marked this pull request as ready for review October 6, 2021 21:44

lambdai requested a review from alyssawilk as a code owner October 6, 2021 21:44

ggreenway requested changes Nov 8, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 8, 2021

address comment

3f6407b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed rojkov’s stale review via 3f6407b November 9, 2021 06:19

repokitteh-read-only bot removed the waiting label Nov 9, 2021

ggreenway requested changes Nov 11, 2021

View reviewed changes

envoy/network/listener.h Outdated Show resolved Hide resolved

envoy/network/listener.h Outdated Show resolved Hide resolved

source/server/connection_handler_impl.cc Show resolved Hide resolved

source/common/network/listen_socket_impl.h Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 12, 2021

add test case and address naming

18a4d07

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 12, 2021

ggreenway reviewed Nov 15, 2021

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Nov 15, 2021

more validation

f5efb17

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

yanavlasov approved these changes Nov 16, 2021

View reviewed changes

ggreenway approved these changes Nov 17, 2021

View reviewed changes

ggreenway merged commit 8598c3f into envoyproxy:main Nov 17, 2021

adisuissa mentioned this pull request Jul 19, 2022

envoy_reloadable_features_internal_address deprecation #22290

Closed

Conversation

lambdai commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 13, 2021

Uh oh!

lambdai commented Sep 13, 2021

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 5, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Nov 9, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanavlasov commented Nov 12, 2021

Uh oh!

lambdai commented Nov 15, 2021

Uh oh!

repokitteh-read-only bot commented Nov 15, 2021

Uh oh!

lambdai commented Nov 15, 2021

Uh oh!

repokitteh-read-only bot commented Nov 15, 2021

Uh oh!

lambdai commented Nov 15, 2021

Uh oh!

repokitteh-read-only bot commented Nov 15, 2021

Uh oh!

ggreenway 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!

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 commented Nov 16, 2021

Uh oh!

repokitteh-read-only bot commented Nov 16, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

lambdai commented Sep 13, 2021 •

edited

Loading