Skip to content

udp proxy: complete MVP#9046

Merged
mattklein123 merged 8 commits intomasterfrom
final_udp_change
Dec 5, 2019
Merged

udp proxy: complete MVP#9046
mattklein123 merged 8 commits intomasterfrom
final_udp_change

Conversation

@mattklein123
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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 <mklein@lyft.com>
@mattklein123 mattklein123 changed the base branch from next_udp_change to master November 25, 2019 18:27
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member Author

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

Signed-off-by: Matt Klein <mklein@lyft.com>
@repokitteh-read-only
Copy link
Copy Markdown

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 <mklein@lyft.com>
@mattklein123
Copy link
Copy Markdown
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
Copy Markdown
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.


// 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
Copy Markdown
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
Copy Markdown
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

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member Author

@danzh2010 updated PTAL

@mattklein123
Copy link
Copy Markdown
Member Author

/azp run envoy-linux

@azure-pipelines
Copy link
Copy Markdown

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