listener: keep ListenerFactoryContext small by lambdai · Pull Request #7528 · envoyproxy/envoy

lambdai · 2019-07-10T22:07:06Z

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

Description:
ListenerFactoryContext is mainly used by NetworkFilter and ListenerFilterto access Listener.
Code base evolved and there is no use case to add listen socket options.
Instead, listener options should be added by Listener config proto

The existing usage are left in listener test to inject failure, probably fit the code in the past but should never seen in current code flow.

Risk Level: LOW
Testing: Modified tests

Fixes #7467

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

lambdai · 2019-07-10T22:11:19Z

@klarose @jrajahalme @mattklein123
Also CC @htuch who introduced addListenSocketOptions in #2922 to support IP_FREEBIND

mattklein123

Thanks, LGTM with some questions.

/wait

source/server/listener_manager_impl.h

test/mocks/server/mocks.h

mattklein123 · 2019-07-11T17:17:49Z

test/server/listener_manager_impl_test.cc

 };

 TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilter) {
-  // Static scope required for the io_handle to be in scope for the lambda below


I'm a little concerned that we are losing some test coverage that matters here. Are you positive that this is not being used by existing listener filters? I'm not convinced. Can you see who added these tests and ask them?

I remove the two functions from ListenerFactoryContext and the build is successful. That's the proof that no listener filter in the code tree is using it. Unfortunately I can never verify if private code is using ... and that is the linked issue for

source/server/listener_manager_impl.h

test/mocks/server/mocks.h

lambdai · 2019-07-11T19:06:56Z

test/server/listener_manager_impl_test.cc

 };

 TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilter) {
-  // Static scope required for the io_handle to be in scope for the lambda below


I remove the two functions from ListenerFactoryContext and the build is successful. That's the proof that no listener filter in the code tree is using it. Unfortunately I can never verify if private code is using ... and that is the linked issue for

lambdai · 2019-07-11T19:20:08Z

test/server/listener_manager_impl_test.cc

-                fd = socket.ioHandle().fd();
-                return true;
-              }));
-      context.addListenSocketOption(std::move(option));


@mattklein123 The tricky test is verifying if createFilterFactoryFromProto is called but delegating it to ListenerFactoryContext::addListenSocketOption. This might be reasonable in the past but is not appropriate at this moment.

BTW I think

envoy/source/server/listener_manager_impl.cc

Lines 208 to 217 in 1ca1d13

if (config.has_transparent()) {

addListenSocketOptions(Network::SocketOptionFactory::buildIpTransparentOptions());

}

if (config.has_freebind()) {

addListenSocketOptions(Network::SocketOptionFactory::buildIpFreebindOptions());

}

if (!config.socket_options().empty()) {

addListenSocketOptions(

Network::SocketOptionFactory::buildLiteralOptions(config.socket_options()));

}

is why OriginalDstListenerFilter(as well as other ListenerFilters) no longer need addListenSocketOption in ListenerFactoryContext

I'm still concerned that people may have private listener filters that are using this and won't be able to now. Can you see who added the tests that you changed and ask them?

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

lambdai · 2019-07-11T21:06:45Z

@jrajahalme The initial socket option changes in original dst listener filter was initiated by you in #2519

Could you confirm that it is not needed at the current code base?

As noted in #7528 the newly added sanity checking of possibly sensitive file paths prevents legitimate usage of passing bootstrap via a non-CLOEXEC file descriptor from a generator helper that execs Envoy. This PR relaxes the validation such that any path resolving to a canonical path with the prefix /dev/fd/ is considered valid. Risk Level: Low Testing: Unit test case is added that was failing before the change and passes afterwards. In addition I've manually verified that the old behaviour of allowing /dev/fd/ paths works with my dev binary. Fixes #7258 Signed-off-by: Paul Banks <banks@banksco.de>

lambdai · 2019-07-15T20:59:18Z

@jrajahalme Sorry to involve you in this PR. It's would be nice to have your confirm. Thanks!

jrajahalme · 2019-07-16T16:19:56Z

@silentdai Thanks for the heads-up! Unfortunately we currently depend on the functionality being removed here in the Cilium proxy (github.com/cilium/proxy) :-(

It will take some time for me to figure out how to replace it with the listener config proto. I think it should be doable, as the option and the value are derived from the listener filter configuration. To expedite this, maybe you could give a hint how to set the Linux SO_MARK (socket mark option) using the listener config proto?

jrajahalme · 2019-07-16T17:09:15Z

OK, I looked through the core.SocketOption docs, and this looks pretty straightforward. Will try this out.

jrajahalme · 2019-07-16T18:28:41Z

@silentdai Changed Cilium to set the option in listener config instead (cilium/cilium/pull/8581), so I'm fine with this PR being merged :-)

lambdai · 2019-07-17T16:50:11Z

@jrajahalme Thanks! Will take a look at https://github.com/cilium/proxy/tree/master/cilium as well when I make further changes.
@mattklein123 Do you have other concerns?

mattklein123

LGTM, thank you!

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

jrajahalme · 2023-08-11T12:29:36Z

@lambdai @mattklein123 This change is now coming to haunt me... I need to add a listen socket option that has a non-standard implementation, so that the current Listener config options are not enough. Would there be any appetite for reverting this change?

listener: keep ListenerFactoryContext small

b89beaa

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

lambdai marked this pull request as ready for review July 10, 2019 23:58

banks mentioned this pull request Jul 9, 2019

bootstrap: allow /dev/fd/<fd> paths for config files #7279

Merged

mattklein123 self-assigned this Jul 11, 2019

mattklein123 requested changes Jul 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 11, 2019

lambdai commented Jul 11, 2019

View reviewed changes

remove addListenSocketOption in mocks

ad7b62d

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

repokitteh-read-only bot removed the waiting label Jul 11, 2019

mattklein123 added the waiting label Jul 11, 2019

mattklein123 approved these changes Jul 18, 2019

View reviewed changes

mattklein123 merged commit 170c89e into envoyproxy:master Jul 18, 2019

TAOXUY pushed a commit to TAOXUY/envoy that referenced this pull request Jul 22, 2019

listener: keep ListenerFactoryContext small (envoyproxy#7528)

8d7fa04

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

	if (config.has_transparent()) {
	addListenSocketOptions(Network::SocketOptionFactory::buildIpTransparentOptions());
	}
	if (config.has_freebind()) {
	addListenSocketOptions(Network::SocketOptionFactory::buildIpFreebindOptions());
	}
	if (!config.socket_options().empty()) {
	addListenSocketOptions(
	Network::SocketOptionFactory::buildLiteralOptions(config.socket_options()));
	}

Conversation

lambdai commented Jul 10, 2019

Uh oh!

lambdai commented Jul 10, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lambdai Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai commented Jul 11, 2019

Uh oh!

lambdai commented Jul 15, 2019

Uh oh!

jrajahalme commented Jul 16, 2019

Uh oh!

jrajahalme commented Jul 16, 2019

Uh oh!

jrajahalme commented Jul 16, 2019

Uh oh!

lambdai commented Jul 17, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants