Api: Add uptream_address to proxy config #534
Conversation
Signed-off-by: Kateryna Nezdolii <[email protected]>
nezdolik
requested review from
markmandel,
XAMPPRocky,
luna-duclos,
rezvaneh and
bombsimon
as code owners
| @@ -134,8 +136,11 @@ impl SessionArgs { | |||
| impl Session { | |||
| /// internal constructor for a Session from SessionArgs | |||
| async fn new(args: SessionArgs) -> Result<Self> { | |||
| let addr = (std::net::Ipv4Addr::UNSPECIFIED, 0); | |||
| let socket = Arc::new(UdpSocket::bind(addr).await.map_err(Error::BindUdpSocket)?); | |||
| let socket = Arc::new( | |||
There was a problem hiding this comment.
Won't this fail if you end up with more than one session (or endpoint)? Since each session will attempt to bind to the same port, and will then fail, and no session will be bound.
I'm not quite sure this will work 🤔 Maybe each endpoint you could be able to specify the port?
The proxy is built to be non-transparent, so Sessions do use unique sender and receiver information as it's storage for individual Sessions:
quilkin/src/proxy/sessions/session_manager.rs
Lines 26 to 27 in 786d0d3
Breaking that contract could have some unintended consequences as well.
I would also advocate that there is some integration tests for this to ensure it works as expected.
There was a problem hiding this comment.
Just reviewing @XAMPPRocky 's original comment
Rather than bind a new port on each session, we probably need to do something more like having an Option<Arc<UdpSocker>> that gets passed into the Session, and if it's None then we bind to the ephemeral port.
Still not quite sure how/if that solves the unique key issue within SessionManager though 🤔 I'll write some thoughts on the original PR for some design discussion.
There was a problem hiding this comment.
Thanks for review @markmandel, good catch. Tested on live Quilkin process but with one session. That should de definitely covered in integration test. Looking forward to design discussions, meanwhile will pause dev work on this.
|
If you rebase against |
Signed-off-by: Kateryna Nezdolii <[email protected]>
|
Build Succeeded 🥳 Build Id: 0362826b-7124-4a54-880a-10797624162d To build this version: |
|
Shall we close this PR, until the details of this issue are worked out in #345? (which is also a gentle bump to take another look at the suggestions in that ticket 😄 ) |
|
@markmandel makes sense, closing. |
Signed-off-by: Kateryna Nezdolii [email protected]
What type of PR is this?
feature
What this PR does / Why we need it:
Adds possibility to configure Quilkin with upstream address to bind to when proxying packets upstream.
Which issue(s) this PR fixes:
Closes #345