api: add envoy internal address by lambdai · Pull Request #12837 · envoyproxy/envoy

lambdai · 2020-08-27T10:44:51Z

Commit Message:

Add envoy internal address for envoy internal connection.

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

Additional Description:
Part of #11725

Risk Level: low (code unused)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a

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

repokitteh-read-only · 2020-08-27T10:44:56Z

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

see: more, trace.

api/envoy/config/core/v3/address.proto

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

api/envoy/config/core/v3/address.proto

lambdai · 2020-09-02T18:57:32Z

Chatted offline: I want to clarify that

This internal address is used by Envoy cluster to locate the virtual listener(or internal listener).
Dispatcher.registerPipeFactory(InternalAddress virtual_listener_address, ListenerCallbacks callbacks);
A natural requirement is that the virtual listener need to find the original address(as well as other socket options).
Conceptually, the virtual connection should have those attributes

    internal_address: listener_9000, typed InternalAddressInstance
    internal_peer_address:  no prototype yet but C++ type is InternalAddressInstance
    original_L4_address: 10.0.0.1:443, typed Ipv4AddressInstance
    original_peer_L4_address: 10.0.0.2:12345: typed Ipv4AddressInstance

These connection attributes will be gradually added in the further PRs so that virtual address could support various of network filters, listener filters

alyssawilk

Looks good - just a few minor nits on my end.

api/envoy/config/core/v3/address.proto

source/common/network/address_impl.h

alyssawilk · 2020-09-02T18:57:14Z

source/common/network/address_impl.h

+  const Ip* ip() const override { return nullptr; }
+  const Pipe* pipe() const override { return nullptr; }
+  const EnvoyInternalAddress* envoyInternalAddress() const override { return &internal_address_; }
+  const sockaddr* sockAddr() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }


I'm mildly concerned that a panic() isn't the right call here. Can you take a peek at call sites and see what we should do here to make sure these can never be called for the new address type?

We should probably also update the comments in the include directory to make it clear when it's safe to call and when it's not.

With NOT_IMPLEMENTED_GCOVR_EXCL_LINE it means
If the later version of control plane send the config containing EnvoyInternalAddress,
this particular version with this PR but no further of envoy might crash.

What I can do is to find the potential usage of
this new type of address.
Listener and Endpoint as a immediate candidate, making sure these are either
invalidate config per config update, mostly in master thread.
or fail the connection when this address is used.

What else can I do?

I think if we are making implementation of parts of the interface optional (panic if called), we either need to have TODOs to implement later or TODOs to check all call sites to make them internal-address compatible. Having bits of an interface which may work for some address and may not work for others, and no checks in the code base makes me uncomfortable. Again cc @mattklein123 in case he has other ideas of how to aim the decaped TCP stream back into a new envoy filter chain without a half-working address class.

Updated to return nullptr and I mentally go through all the sockAddr() usage in the existing code base: all of them accepts nullptr usage without crash.
Luckily these usages are inline followed by syscall (connect/bind/send/recv) so I could inject syscall error code as if the following syscalls fail.
Now I believe these errors are now non-fatal.

alyssawilk

Also can you make sure to fill out the rest of the template for your PRs?
for this one I'd say something like

Risk Level: low (code unused)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a

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

lambdai · 2020-09-03T10:17:03Z

For internal address as server address: I add the validation that all listener should reference internal address.
Such listener config will fails the addOrUpdateListener().

Internal address as client address is much harder.
Theoretically I can validate any internal address is not used in endpoint. However, we will allow it in the future.
And we need to fail the connections anyway.
I find that Envoy never fail fast at connection construction. I am not sure if this is good though.
I switch to provide a NullIoSocketIoHandler which always fails at sock related OS call. I assume the upper
connection should eventually close the connection given this NullIoSocket.
This client side efforts is WIP

I am more than happy to hear the alternative approaches from you @alyssawilk Thank you in advance!

alyssawilk

Please check CI, it looks like it is not passing.

alyssawilk · 2020-09-03T18:08:35Z

source/common/network/address_impl.h

+  const Ip* ip() const override { return nullptr; }
+  const Pipe* pipe() const override { return nullptr; }
+  const EnvoyInternalAddress* envoyInternalAddress() const override { return &internal_address_; }
+  const sockaddr* sockAddr() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }


I think if we are making implementation of parts of the interface optional (panic if called), we either need to have TODOs to implement later or TODOs to check all call sites to make them internal-address compatible. Having bits of an interface which may work for some address and may not work for others, and no checks in the code base makes me uncomfortable. Again cc @mattklein123 in case he has other ideas of how to aim the decaped TCP stream back into a new envoy filter chain without a half-working address class.

alyssawilk · 2020-09-03T18:10:40Z

include/envoy/network/socket_interface.h

                                          const Address::InstanceConstSharedPtr addr) {
+  // Cannot create IoHandle for internal address yet.
+  if (addr->envoyInternalAddress()) {
+    return nullptr;


if this stands, there should be a unit test please.

You are so right! My brain stop working last night so I pushed before I save this change.

The latest update has an integration test connecting to a internal address

alyssawilk · 2020-09-03T18:10:46Z

include/envoy/network/socket_interface.h

 static inline IoHandlePtr ioHandleForAddr(Socket::Type type,
                                          const Address::InstanceConstSharedPtr addr) {
+  // Cannot create IoHandle for internal address yet.
+  if (addr->envoyInternalAddress()) {


Unfortunately this looks like it's solid error handling but most callers of ioHandleForAddr will crash if it returns nullptr.

@mattklein123 @lambdai 's plan is basically creating this new listener type to be able to point a "cluster" at this new virtual address. Any thoughts on how that might best be done without causing this cascading chain of not-fully-implemented things?

I fixed this by providing the NullIoSocketHandleImpl to give it a socket handle allow upper layer to create a connection but will fail to send/recv.
https://github.com/envoyproxy/envoy/pull/12837/files#r483259684

The ideal case is control plane never send unsupported addresses before envoy could support.

Mark the cluster config invalid if it contains internal address. It's always safer to reject cluster configs at master.
I did this check in LDS config as well. That is easy since LDS is the only entrance to receive new listener config.

In case of escaped internal address which we fail to invalid, the NullIoSocketHandle could fail the per client connect.
This NullSocketHandle could also support mingled addresses such as

endpoints: - internal address: "not_yet_supported_listener_address" - socket address: "127.0.0.1:80"

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

lambdai · 2020-09-03T21:18:48Z

source/common/network/io_socket_handle_impl.h

+/**
+ * IoHandle for invalid socket. This is rquired because Envoy expects IoHandle creation never fail.
+ * This NullIoSocketHandleImpl allows Envoy surives on unsupported socket options.
+ */
+class NullIoSocketHandleImpl : public IoHandle, protected Logger::Loggable<Logger::Id::io> {
+public:
+  NullIoSocketHandleImpl() : closed_{false} {


NullIoSocketHandle

lambdai · 2020-09-03T21:42:02Z

test/integration/socket_interface_integration_test.cc

+// Test that connecting to internal address should always followed by disconnection with no crash.
+TEST_P(SocketInterfaceIntegrationTest, InternalAddressWithSocketInterface) {
+  BaseIntegrationTest::initialize();
+
+  ConnectionStatusCallbacks connect_callbacks_;
+  Network::ClientConnectionPtr client_;
+  const Network::SocketInterface* sock_interface = Network::socketInterface(
+      "envoy.extensions.network.socket_interface.default_socket_interface");
+  Network::Address::InstanceConstSharedPtr address =
+      std::make_shared<Network::Address::EnvoyInternalInstance>("listener_0", sock_interface);
+
+  client_ = dispatcher_->createClientConnection(address, Network::Address::InstanceConstSharedPtr(),
+                                                Network::Test::createRawBufferSocket(), nullptr);
+
+  client_->addConnectionCallbacks(connect_callbacks_);
+  client_->connect();
+
+  while (!connect_callbacks_.closed()) {
+    dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+  }
+
+  client_->close(Network::ConnectionCloseType::FlushWrite);
+}


Testing Dispatcher::createClientConnection(). This should cover the client tcp connection IMO.
I have the concern of unknown unknown...

Now I am adding UDP socket test

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

alyssawilk · 2020-09-08T18:12:35Z

clang tidy is still failing. Let me know when this is ready for another pass?

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

lambdai · 2020-09-09T19:05:38Z

@alyssawilk @mattklein123

The recent update is that socket manipulation on internal address are non-fatal.
Either with socket() syscall series, or send()/recv() syscall series.

The goal is to enable internal address usage everywhere.

Let me know if this is good enough to move on. Thanks!

alyssawilk

OK, as discussed on slack let's hide the config and remove some of the temporary classes with TODOs (it's Ok to crash if you're configuring Envoy with hidden incomplete-implementation code :-P )
Thanks so much for bearing with me on this!

htuch · 2020-09-10T17:49:44Z

Yes, fine to leave docs until we un-hide.

htuch · 2020-09-10T17:49:52Z

/lgtm api

lambdai

Thanks! Updating PR

include/envoy/network/address.h

source/common/network/io_socket_handle_impl.cc

test/common/network/address_impl_test.cc

source/common/network/io_socket_error_impl.cc

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

lambdai · 2020-09-10T19:12:41Z

I find a machine running sphinx correct and fixed the docs. It turns out to be EnvoyInternalAddress field(located in one_of) must be marked hidden as well. Now the doc should pass on CI...

lambdai · 2020-09-10T19:14:35Z

I find a machine running sphinx correct and fixed the docs. It turns out to be EnvoyInternalAddress field(located in one_of) must be marked hidden as well. Now the doc should pass on CI...

@htuch Sorry... another api approval is needed b/c my doc fix :(

alyssawilk

LGTM! over to @mattklein123

mattklein123

LGTM at a high level, though, I wonder whether it's really worth landing this PR vs. the real implementation. Some small comments. Thank you!

/wait

source/common/network/resolver_impl.cc

mattklein123 · 2020-09-10T23:17:26Z

source/server/listener_manager_impl.cc

+    ENVOY_LOG(debug,
+              "listener {} has envoy internal address {}. Internal address cannot be used by "
+              "listener yet",
+              config.name(), config.address().envoy_internal_address().DebugString());
+    return false;


Just use NOT_IMPLEMENTED? Or just ASSERT not has_envoy_internal_address? Seems not worth it to handle this case right now.

NOT_IMPLEMENTED will crash the Envoy. That is not expected.

ASSERT could replace the the ENVOY_LOG, but this if branch should be kept in case Envoy does receive a listener config with internal address. Schema check is not helping here.

What is the point of this PR? It's not that many lines of code and it doesn't actually do anything? I would rather you either a) just hold this PR and implement the full thing or b) put in ASSERT or RELEASE_ASSERT for stuff like this since you have a not implemented and not documented feature. It doesn't make any sense to add this type of logic.

I'd like to push api first as the conversion.

I don't get your plan b RELEASE_ASSERT is easier but it will crash Envoy. Why do you prefer an unsupported listener config crashing Envoy? With these lines above envoy would NACK the listener config without crash.

Because you are adding error handling/logging logic for something that is not implemented. It bloats the code base for no good reason. If you want to make sure it crashes, do RELEASE_ASSERT(..., "this is not implemented yet."). Thanks.

/wait

Oh, to NACK I should throw exception instead of return false.

@mattklein123 Is throwing EnvoyException beating return false and RELEASE_ASSERT?

No. Crash, or please just close this PR and reopen when there is an implementation. It does not make any sense to add actual error handling for an unimplemented partial feature.

Happy to crash and move to next listener config api.
And then the internal connection driven by scheduler.

Running tests...

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

htuch · 2020-09-11T20:38:23Z

/lgtm api

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

mattklein123

Thanks!

lambdai · 2020-09-11T23:20:03Z

Thank you @mattklein123
This crash on unimplemented strategy make my future PRs much easier.
I am expecting more combinations are allowed in the follow up

core: add envoy internal address

fb1db47

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

repokitteh-read-only bot added the api label Aug 27, 2020

lambdai mentioned this pull request Aug 27, 2020

listener: add envoy internal listener #12838

Closed

htuch reviewed Aug 27, 2020

View reviewed changes

api/envoy/config/core/v3/address.proto Outdated Show resolved Hide resolved

mattklein123 assigned alyssawilk Aug 27, 2020

lambdai added 3 commits August 27, 2020 22:39

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]>

alyssawilk reviewed Aug 31, 2020

View reviewed changes

api/envoy/config/core/v3/address.proto Show resolved Hide resolved

alyssawilk reviewed Sep 2, 2020

View reviewed changes

lambdai added 2 commits September 3, 2020 09:59

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]>

alyssawilk reviewed Sep 3, 2020

View reviewed changes

add integration test for NullIoSocketHandleImpl

a337bde

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

lambdai commented Sep 3, 2020

View reviewed changes

lambdai added 4 commits September 3, 2020 23:40

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]>

rename and clang-tidy

b3754c1

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

mattklein123 self-assigned this Sep 9, 2020

add sendTo test

a512711

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

alyssawilk reviewed Sep 9, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 10, 2020

lambdai commented Sep 10, 2020

View reviewed changes

address comment

f193c2d

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

repokitteh-read-only bot added the api label Sep 10, 2020

alyssawilk previously approved these changes Sep 10, 2020

View reviewed changes

mattklein123 requested changes Sep 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 10, 2020

remove specifier default

d9ba5be

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

lambdai dismissed alyssawilk’s stale review via d9ba5be September 11, 2020 17:30

repokitteh-read-only bot removed the waiting label Sep 11, 2020

mattklein123 added the waiting label Sep 11, 2020

massage gcc b/c about NOT_REACH

248e6e1

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

repokitteh-read-only bot added waiting and removed waiting labels Sep 11, 2020

repokitteh-read-only bot removed the api label Sep 11, 2020

lambdai added 2 commits September 11, 2020 21:43

crash on internal address

10d6dab

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

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

ed0e0aa

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

repokitteh-read-only bot removed the waiting label Sep 11, 2020

mattklein123 approved these changes Sep 11, 2020

View reviewed changes

mattklein123 merged commit bef044a into envoyproxy:master Sep 11, 2020

asraa mentioned this pull request Sep 14, 2020

[api] fix address configuration bugs #13090

Merged

This was referenced Oct 1, 2020

[WIP] Envoy Internal connection strawman #12537

Closed

Full picture of in memory listener and connection #13361

Closed

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 27, 2020

Uh oh!

Uh oh!

Uh oh!

lambdai commented Sep 2, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

lambdai commented Sep 3, 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!

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!

alyssawilk commented Sep 8, 2020

Uh oh!

lambdai commented Sep 9, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Sep 10, 2020

Uh oh!

htuch commented Sep 10, 2020

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Sep 10, 2020

Uh oh!

lambdai commented Sep 10, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

lambdai commented Aug 27, 2020 •

edited

Loading

mattklein123 Sep 11, 2020 •

edited

Loading