Skip to content

fix: record recovered local address #13581

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.

Sign up for GitHub

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

Merged
antoniovicente merged 5 commits into from
Oct 19, 2020
Merged

fix: record recovered local address #13581

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7bad421
fix: record recovered local address
kyessenov Oct 14, 2020
9a3fe80
fix test
kyessenov Oct 14, 2020
7fe3fcf
review
kyessenov Oct 19, 2020
f368f32
Merge remote-tracking branch 'upstream/master' into update_socket_add…
kyessenov Oct 19, 2020
ca9e25a
fix format
kyessenov Oct 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions source/server/connection_handler_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,9 @@ void ConnectionHandlerImpl::ActiveTcpListener::resumeListening() {

void ConnectionHandlerImpl::ActiveTcpListener::newConnection(
Network::ConnectionSocketPtr&& socket, std::unique_ptr<StreamInfo::StreamInfo> stream_info) {
// Update local address in case it is restored by a listener filter

@lambdai lambdai Oct 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add the TODO? This is good since we known original_dst could change setDownstreamLocalAddress but we don't know if other crazy listener filter could do (proxy protocol?)

@kyessenov kyessenov Oct 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Without exposing stream info to listener filters, we just have to fix this case by case. Is there something else that is changed by listener filters?

@lambdai lambdai Oct 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TransportProtocol applicationProtocol. It depends on the overlap of stream info and socket info.
Also newConnection() is not the only termination of the socket. For a closed socket the small discrepancy is not ideal but should be fine for waiting for the further solution.

@antoniovicente antoniovicente Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm having some trouble understanding the issue being fixed in this PR. Should original_dst be calling setDownstreamLocalAddress? I'm guessing that the answer is no.

I'm concerned that this change breaks other intended behavior, but I can see the counter argument involving no existing tests breaking. That said, a lot of functionality is covered by small tests, which wouldn't detect breakages due to changes like this one which involve interaction between multiple extensions.

@kyessenov kyessenov Oct 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The history is that this is a regression. Prior to #13554, stream info was instantiated after listener filters, so the value of downstream local address is the modified value. After that PR, stream info is instantiated before listener filters, so the value has changed. This is a fix to restore the original behavior.

The real issue is lack of any integration testing for original_dst. I think we would have caught the regression sooner if we had any test that exercised original_dst and logging. I don't know why it's like that, maybe it was difficult to set up iptables in a test.

@antoniovicente antoniovicente Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is socket->localAdress() the actual address coming from the socket or a modified value set by orig dst filter?

Basically, I'm having trouble understanding new behavior based on the comment added here.

@kyessenov kyessenov Oct 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's the modified value AFAICT.

@antoniovicente antoniovicente Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you update the comment to something like:

// Refresh local address in case it was restored by a listener filter like the original dst filter.

stream_info->setDownstreamLocalAddress(socket->localAddress());

// Find matching filter chain.
const auto filter_chain = config_->filterChainManager().findFilterChain(*socket);
if (filter_chain == nullptr) {
Expand Down
7 changes: 6 additions & 1 deletion test/server/connection_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,12 @@ TEST_F(ConnectionHandlerTest, NormalRedirect) {
EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "test.downstream_cx_total")->value());
EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "test.downstream_cx_active")->value());

EXPECT_CALL(*access_log_, log(_, _, _, _)).Times(1);
EXPECT_CALL(*access_log_, log(_, _, _, _))
.WillOnce(
Invoke([&](const Http::RequestHeaderMap*, const Http::ResponseHeaderMap*,
const Http::ResponseTrailerMap*, const StreamInfo::StreamInfo& stream_info) {
EXPECT_EQ(alt_address, stream_info.downstreamLocalAddress());
}));

@antoniovicente antoniovicente Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be good to exercise this fix in an integration test to fully capture the full implications.

@kyessenov kyessenov Oct 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, but there is no integration test for original_dst, unfortunately.

@antoniovicente antoniovicente Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you work with the owners of original_dst to improve test coverage?

@kyessenov kyessenov Oct 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Who's the owner of original_dst? It's not in CODEOWNERS.

@mattklein123 mattklein123 Oct 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know who originally wrote the code (someone in Istio I'm sure). The problem is I'm not sure we can have an integration test that is sandboxed. It requires iptables. If this is possible we should do it.

connection->close(Network::ConnectionCloseType::NoFlush);
dispatcher_.clearDeferredDeleteList();
EXPECT_EQ(0UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value());
Expand Down