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

udp proxy: complete MVP #9046

Merged
merged 8 commits into from
Dec 5, 2019
Merged

udp proxy: complete MVP #9046

merged 8 commits into from
Dec 5, 2019

Conversation

mattklein123
Copy link
Member

@mattklein123 mattklein123 commented Nov 16, 2019

Fixes #492
Fixes #9018

Risk Level: Low, new feature
Testing: Integration/unit
Docs Changes: Added
Release Notes: Added

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9046 was opened by mattklein123.

see: more, trace.

@mattklein123
Copy link
Member Author

Note: This is stacked on top of #8999. I wanted to get this out before I take off for conference week in case anyone wants to kick the tires. cc @danzh2010

@mattklein123
Copy link
Member Author

Quick note here for anyone trying this out that I forgot to mention. Until we merge the SO_REUSEPORT change for UDP listeners, this will only work with concurrency == 1.

Fixes #492

Signed-off-by: Matt Klein <[email protected]>
@mattklein123 mattklein123 changed the base branch from next_udp_change to master November 25, 2019 18:27
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
@mattklein123
Copy link
Member Author

@danzh2010 @moderation ready for proper review. Thank you!

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9046 was synchronize by mattklein123.

see: more, trace.

Signed-off-by: Matt Klein <[email protected]>
@mattklein123
Copy link
Member Author

Note that I just pushed a fix for the test flake reported in #9018 to this PR. It's low frequency enough that I think we can land here.

Copy link
Contributor

@danzh2010 danzh2010 left a comment

Choose a reason for hiding this comment

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

Great work! It looks good to me overall, other than some personal opinions regarding session creation and tearing down.

@@ -569,7 +569,8 @@ class FakeUpstream : Logger::Loggable<Logger::Id::testing>,
std::chrono::milliseconds timeout = TestUtility::DefaultTimeout);

// Send a UDP datagram on the fake upstream thread.
void sendUdpDatagram(const std::string& buffer, const Network::Address::Instance& peer);
void sendUdpDatagram(const std::string& buffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why making this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix for #9018

@mattklein123
Copy link
Member Author

@danzh2010 updated PTAL

@mattklein123
Copy link
Member Author

/azp run envoy-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mattklein123 mattklein123 merged commit aa3cb57 into master Dec 5, 2019
@mattklein123 mattklein123 deleted the final_udp_change branch December 10, 2019 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

udp_proxy_integration_test tsan flake UDP proxying support
4 participants