scream icon indicating copy to clipboard operation
scream copied to clipboard

Published 20 hours ago • verified publisher icon EricssonResearch

bwtest NAT support

Open mirabilos opened this issue 2 years ago • 4 comments

On top of the changes in #46 this adds NAT support to the bandwidth tester:

  • the receiver now only gets its local port to listen to as command line argument and binds to INADDR_ANY on this port
    • the background thread is not yet started
    • the outgoing feedback sockaddr_in is initialised with AF_UNSPEC to signal it’s not fully initialised yet
  • the sender binds to a local ephemeral address and contacts the server by means of its “decoder” IP and port (unchanged)
  • the receiver gets the first packet and looks at the sender information from recvfrom(2) / recvmsg(2) msg_name and “records” that as outgoing feedback address
    • it also displays which IP:port the “connection” comes from
    • the background thread is then started
  • any further packets have their sender IP/port checked against the saved address and are verbosely discarded if they don’t match, to avoid mixing different senders in the same receiver program

This has been tested with the receiver on a public IPv4 and the sender behind NAT; it should also work with the sender behind multiple NATs as on Campus-IT PoC.

The totally inefficient and not-working NAT punching from the existing code has been removed as well.

mirabilos avatar Jun 28 '22 15:06 mirabilos

I’ve added -reverse mode, in which the receiver may be behind NAT and connects to the sender, which waits for a dummy packet (sent by the receiver in a new temporary preconnect thread) and then sends to the IP/port where the dummy packet came from.

This makes it possible to test asymmetric connections that are one-sided behind NAT.

mirabilos avatar Jun 29 '22 17:06 mirabilos

Hi Thanks for the hard work. I have a couple of questions :

  1. Have you verified that the code works as intended with all the proposed fixes ?
  2. Is it possible to put all white space changes as a separate commit?, it would make it easier to review the proposed changes.

Regards /Ingemar

From: mirabilos @.> Sent: Tuesday, 28 June 2022 17:59 To: EricssonResearch/scream @.> Cc: Subscribed @.***> Subject: [EricssonResearch/scream] bwtest NAT support (PR #47)

On top of the changes in #46https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-e8f50fcdfab826f6&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F46 this adds NAT support to the bandwidth tester:

  • the receiver now only gets its local port to listen to as command line argument and binds to INADDR_ANY on this port
 *   the background thread is not yet started
 *   the outgoing feedback sockaddr_in is initialised with AF_UNSPEC to signal it’s not fully initialised yet
  • the sender binds to a local ephemeral address and contacts the server by means of its “decoder” IP and port (unchanged)
  • the receiver gets the first packet and looks at the sender information from recvfrom(2) / recvmsg(2) msg_name and “records” that as outgoing feedback address
 *   it also displays which IP:port the “connection” comes from
 *   the background thread is then started
  • any further packets have their sender IP/port checked against the saved address and are verbosely discarded if they don’t match, to avoid mixing different senders in the same receiver program

This has been tested with the receiver on a public IPv4 and the sender behind NAT; it should also work with the sender behind multiple NATs as on Campus-IT PoC.

The totally inefficient and not-working NAT punching from the existing code has been removed as well.

You can view, comment on, or merge this pull request online at:

https://github.com/EricssonResearch/scream/pull/47https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-678eb03b5f281c81&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F47

Commit Summary

File Changes

(100 fileshttps://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-aaf5b3b8796822fd&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F47%2Ffiles)

Patch Links:

— Reply to this email directly, view it on GitHubhttps://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-678eb03b5f281c81&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F47, or unsubscribehttps://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-bc00f2435b1430eb&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACRZ2GDLL6XUQ2JB2SXVQ4TVRMONNANCNFSM52CWKCXQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>

IngJohEricsson avatar Jun 30 '22 05:06 IngJohEricsson

Hi Ingemar,

  1. Have you verified that the code works as intended with all the proposed fixes ?

yes, I’m easily reaching the same bandwidth as without these modifications on an 100 Mbit/s link and 200+ on Gbit links.

  1. Is it possible to put all white space changes as a separate commit?, it would make it easier to review the proposed changes.

They are separate commits, but you can also do:

git fetch origin pull/47/head:refs/pr/47 # to fetch the changes

git diff -w master..refs/pr/47 # to view ignoring WS changes

Since this PR is based on top of #46 maybe review+merge that first, then this PR will be reduced to the necessities.

Thanks, //mirabilos

Infrastrukturexperte • tarent solutions GmbH Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/ Telephon +49 228 54881-393 • Fax: +49 228 54881-235 HRB AG Bonn 5168 • USt-ID (VAT): DE122264941 Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

mirabilos avatar Jun 30 '22 11:06 mirabilos

Hi This one got stuck a little too long. Is it possible that you can do a new pull request based on the latest code ? I understand that the NAT handling stuff is very useful so it would definitely be goo to have it in /Ingemar

IngJohEricsson avatar Jan 09 '23 08:01 IngJohEricsson