Add ARP Responder module by GalSagie · Pull Request #494 · NetSys/bess

GalSagie · 2017-06-01T08:15:41Z

ARP responder is configured with known MAC's-IP's pairs
and know to answer ARP requests depending on its configured cache.

This module can be used to implement virtual routing in
virtual enviorments and also as a foundation for ARP
handling in general.

Still todo is the ability to learn new MAC-IP pairs from
the requests and replys and also removing of pairs
from cache (with maybe potential timeouts)

ARP responder is configured with known MAC's-IP's pairs and know to answer ARP requests depending on its configured cache. This module can be used to implement virtual routing in virtual enviorments and also as a foundation for ARP handling in general. Still todo is the ability to learn new MAC-IP pairs from the requests and replys and also removing of pairs from cache (with maybe potential timeouts)

add new lines at end of files

codecov · 2017-06-01T08:48:26Z

Codecov Report

Merging #494 into master will increase coverage by 28%.
The diff coverage is 80%.

@@           Coverage Diff            @@
##           master    #494     +/-   ##
========================================
+ Coverage      30%     58%    +28%     
========================================
  Files         168     178     +10     
  Lines        8181   10203   +2022     
  Branches     1556       0   -1556     
========================================
+ Hits         2489    5947   +3458     
+ Misses       5310    4256   -1054     
+ Partials      382       0    -382

Impacted Files	Coverage Δ
core/modules/arp_responder.h	`100% <100%> (ø)`
core/modules/arp_responder.cc	`79% <79%> (ø)`
core/modules/merge.cc	`25% <0%> (-8%)`	⬇️
core/metadata.h	`64% <0%> (-3%)`	⬇️
core/message.h	`18% <0%> (-2%)`	⬇️
core/modules/ip_checksum.cc	`11% <0%> (-1%)`	⬇️
core/modules/buffer.cc	`4% <0%> (-1%)`	⬇️
core/modules/timestamp.cc	`4% <0%> (-1%)`	⬇️
core/modules/generic_decap.cc	`7% <0%> (ø)`	⬇️
core/modules/port_out.cc	`3% <0%> (ø)`	⬇️
... and 140 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 024c1b9...103164b. Read the comment docs.

barath · 2017-06-01T12:49:11Z

+using bess::utils::Ethernet;
+using bess::utils::be32_t;
+
+struct arp_entry {


Comment for struct.

barath · 2017-06-01T12:49:29Z

+struct arp_entry {
+  Ethernet::Address mac_addr;
+  be32_t ip_addr;
+  uint64_t time;


Could you describe time, especially its units?

Done, added that its timestamp, its currently not used but will be in future patches

barath · 2017-06-01T13:04:53Z

+  CommandResponse CommandAdd(const bess::pb::ArpResponderArg &arg);
+
+ private:
+  std::map<be32_t, arp_entry> entries;


Member variable comment.

Also entries should be named entries_ and by general convention perhaps use struct arp_entry rather than arp_entry.

barath · 2017-06-01T13:17:57Z

+
+CommandResponse ArpResponder::CommandAdd(const bess::pb::ArpResponderArg &arg) {
+  be32_t ip_addr;
+  arp_entry entry;


No need to declare entry.

See comment below

barath · 2017-06-01T13:18:16Z

+
+  entry.mac_addr.FromString(arg.mac_addr());
+  entry.ip_addr = ip_addr;
+  entries[ip_addr] = entry;


You can brace initialize entry here with all of its fields.

the mac is now filled as part of a check (see fix above) and its a bit hard to do it now

Since you aren't brace initializing, could you explicitly initialize time? Otherwise I believe it is uninitialized.

barath · 2017-06-01T13:18:46Z

+    return CommandFailure(EINVAL, "Invalid IP Address: %s", arg.ip().c_str());
+  }
+
+  entry.mac_addr.FromString(arg.mac_addr());


Do you not want to check the MAC address just like you do the IP address?

barath · 2017-06-01T13:19:49Z

+  uint64_t time;
+};
+
+class ArpResponder final : public Module {


Could you add a class comment. Specifically, you may want to mention here (or somewhere) that this module drops non-ARP packets.

barath · 2017-06-01T13:21:34Z

+
+    Arp *arp = reinterpret_cast<Arp *>(eth + 1);
+    if (arp->opcode == be16_t(Arp::Opcode::kRequest)) {
+      // TODO When learn is added, learn SRC MAC here


Could you change TODO to have your name, e.g. TODO(galsagie), here and below?

barath · 2017-06-01T13:26:52Z

+
+struct arp_entry {
+  Ethernet::Address mac_addr;
+  be32_t ip_addr;


Is ip_addr redundant given that it is the key of the map below?

It is redundant, i prefer to leave it currently for clarity and optimize in later patch
If you think strongly about it, i will use the key

barath · 2017-06-01T13:30:28Z

+      // Try to find target IP in cache, if exists convert request to reply
+      auto it = entries.find(arp->target_ip_addr);
+      if (it != entries.end()) {
+        struct arp_entry *entry = &it->second;


Prefer a const reference, e.g.:

const struct arp_entry &entry = it->second

Address code review comments

barath · 2017-06-01T15:35:21Z

+      // Try to find target IP in cache, if exists convert request to reply
+      auto it = entries_.find(arp->target_ip_addr);
+      if (it != entries_.end()) {
+    	const struct arp_entry &entry = it->second;


Could you run clang-format to fix this and anything else it finds?

barath · 2017-06-01T15:36:26Z

+struct arp_entry {
+  Ethernet::Address mac_addr;
+  be32_t ip_addr;
+  // timestamp used to expire cache entries


Minor: since the other fields don't have comments, perhaps move the comment to the same line as time. Also, could you include the units?

Address code review comments

sangjinhan · 2017-06-01T23:09:21Z

+  for (int i = 0; i < cnt; i++) {
+    bess::Packet *pkt = batch->pkts()[i];
+
+    out_gates[i] = incoming_gate;


Given that the module has only one input/output gate, you can just assign 0. Unfortunately get_igate() is not free... :(

sangjinhan · 2017-06-01T23:11:02Z

+import scapy.all as scapy
+
+eth_header = scapy.Ether(src='02:1e:67:9f:4d:ae', dst='ff:ff:ff:ff:ff:ff')
+arp_header = scapy.ARP(op=1, pdst='1.1.1.1')


Maybe we can use another IP address that is not symmetric? Just as a safety measure for subtle endianness regression bugs later any time...

sangjinhan · 2017-06-01T23:14:23Z

+pkt = eth_header/arp_header
+packets = [str(pkt)]
+
+arp = ArpResponder(name="arp")


name="arp" is set but not used below. For conciseness, this line can be replaced with either:

arp::ArpResponder() # this is identical to the original statement

or

arp = ArpResponder() # the resulting (default) name will be arp_responder0

sangjinhan · 2017-06-01T23:16:03Z

+packets = [str(pkt)]
+
+arp = ArpResponder(name="arp")
+arp.add(ip='1.1.1.1', mac_addr='11:22:33:44:55:66')


Really a minor thing, but the MAC address is multicast :-)

sangjinhan · 2017-06-01T23:17:57Z

+        arp->sender_ip_addr = entry.ip_addr;
+      }
+    } else if (arp->opcode == be16_t(Arp::Opcode::kReply)) {
+      // TODO(galsagie) When learn is added, learn SRC MAC here


I believe these two else blocks should discard (after learning if necessary) the packet as well...? Otherwise, its downstream modules has to figure out what to do with the packets.

sangjinhan · 2017-06-01T23:20:42Z

 }
+
+/**
+ * The ARP Responder module is responding to ARP requests and can optionally learn new MAC's-IP's mapping


The comment "can optionally learn ..." can be updated so as to be clear that it is not supported yet...? For example, "TODO:"

Q: is the ARP learning feature for proxy ARP?

Done, yes it can be used for a proxy ARP and it can also be used later for "full" ARP layer implementation to the local host.

Address code review comments, 1) Drop ARP packets in case no cache entry is found and not an ARP request 2) Update TODO's 3) Use 0 as the default incoming gate

sangjinhan · 2017-06-06T23:04:30Z

Merged, thanks!

GalSagie added 2 commits June 1, 2017 11:12

Add ARP Responder module

374d956

add new lines at end of files

barath reviewed Jun 1, 2017

View reviewed changes

Add ARP Responder module

ca3bfa9

Address code review comments

barath reviewed Jun 1, 2017

View reviewed changes

Add ARP Responder module

b45491d

Address code review comments

sangjinhan reviewed Jun 1, 2017

View reviewed changes

Add ARP Responder module

103164b

Address code review comments, 1) Drop ARP packets in case no cache entry is found and not an ARP request 2) Update TODO's 3) Use 0 as the default incoming gate

sangjinhan merged commit 0129500 into NetSys:master Jun 6, 2017

Uh oh!

Conversation

GalSagie commented Jun 1, 2017

Uh oh!

codecov Bot commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalSagie Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalSagie Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barath Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 1, 2017 •

edited

Loading

GalSagie Jun 1, 2017 •

edited

Loading

GalSagie Jun 1, 2017 •

edited

Loading

barath Jun 1, 2017 •

edited

Loading