Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Listener ECDS: reject the connection directly when missing config #21544

Merged
merged 2 commits into from
Jun 13, 2022

Conversation

soulxu
Copy link
Member

@soulxu soulxu commented Jun 2, 2022

Signed-off-by: He Jie Xu [email protected]

Commit Message: Listener ECDS: reject the connection directly when missing config
Additional Description:
Risk Level: high
Testing: the existing integration test covered
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes #21427

@soulxu
Copy link
Member Author

soulxu commented Jun 2, 2022

filter_manager.addAcceptFilter(nullptr,
std::make_unique<MissingConfigTcpListenerFilter>(stats_scope));
added_missing_config_filter = true;
if (!config.has_value()) {
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this!

Should we keep this logic consistent in buildUdpFilterChain() and its caller there, i.e, directly return (void->bool) in this function if the dynamic config is missing?

void FilterChainUtility::buildUdpFilterChain(

config_->filterChainFactory().createUdpListenerFilterChain(*this, *this);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the code, the UDP is different from TCP. The TCP will create new filter factories for the new connection, the UDP is different.

config_->filterChainFactory().createUdpListenerFilterChain(*this, *this);

I'm curious how do you implement ecds for udp, since the filter factories is created in the beginning of listener created, if the filter factories begin with a invalid config, then you have no chance update the filter factories.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw @yanjunxiang-google submitted a draft for UDP ecds #21606

If we enable to trigger the udp listener filter factory to recreate when config updated, then we should be able to do the same thing as the TCP doing, just reject the connection, when an empty config was updated.

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this does simplify the code.
Left a high-level question.

} else {
// If create listener filter chain failed, it means the listener is missing
// config due to the ECDS. Then close the connection directly.
active_socket->socket_->close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test that validates what happens when the connection is closed, but active_socket->iter_ isn't at the end of the filters?
Will those filters work as expected if the actual socket is closed?

Copy link
Member Author

@soulxu soulxu Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

This is going to line 101, which is the same code path when the listener filter rejected the connection. So it should be tested well.

Here is ActiveTcpSocket knew that the listener filter rejected the connection

if (!socket().ioHandle().isOpen()) {
// Break the loop but should not create new connection.
no_error = false;
break;

Then it sets the active_socket->iter_ to the active_socket->accept_filters_.end()

if (no_error) {
newConnection();
} else {
// Signal the caller that no extra filter chain iteration is needed.
iter_ = accept_filters_.end();
}

The initial value of active_socket->iter_ is active_socket->accept_filters_.end()

iter_(accept_filters_.end()),

So we close the socket and have active_socket->iter_ as the end, then it is going to the same code path.

We have integration test cover this

TEST_P(ListenerFilterIntegrationTest, InspectDataFiltersCloseConnectionAfterGetData) {

and unittest

TEST_F(ActiveTcpListenerTest, ListenerFilterCloseSockets) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that essentially after the call for close() one can add an ASSERT(active_socket->iter_ == active_socket->accept_filters_.end().
What confused me was the structure of this function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, that is good idea, thanks!

adisuissa
adisuissa previously approved these changes Jun 9, 2022
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@lambdai can you PTAL.

} else {
// If create listener filter chain failed, it means the listener is missing
// config due to the ECDS. Then close the connection directly.
active_socket->socket_->close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that essentially after the call for close() one can add an ASSERT(active_socket->iter_ == active_socket->accept_filters_.end().
What confused me was the structure of this function.

} else {
// If create listener filter chain failed, it means the listener is missing
// config due to the ECDS. Then close the connection directly.
active_socket->socket_->close();
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the socket is already closed, should we directly return here, and skip the code from line 97 onwards?

Other than this, LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! that is good question. If we return here, then we will lose the access log at line 103. I guess it would be good to keep the access log, WDYT?

Signed-off-by: He Jie Xu <[email protected]>
@soulxu
Copy link
Member Author

soulxu commented Jun 12, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #21544 (comment) was created by @soulxu.

see: more, trace.

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@mattklein123 can you PTAL?

@mattklein123 mattklein123 self-assigned this Jun 13, 2022
@mattklein123 mattklein123 merged commit f6450f2 into envoyproxy:main Jun 13, 2022
tyxia pushed a commit to tyxia/envoy that referenced this pull request Jun 14, 2022
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

listener ecds: reject the connection directly instead of using a fake listener filter
4 participants