[dhcp_relay] Set Broadcast flag for all test packets

[bbinxie]

Description of PR

The expected offer& ack package is broadcast
But get the package is Unicast.
So，we need to set Broadcast flag for dhcp offer&ack,

Summary:
Fixes # (issue)
dhcp relay test failed because verified packet not correct.

Type of change

• [√] Bug fix
• [] Testbed and Framework(new/improvement)
• [] Test case(new/improvement)

Approach

How did you do it?

Modify file dhcp_relay_test.py
Add “set_broadcast_bit=True” in create_dhcp_offer_packet and create_dhcp_ack_packet
Set “flags=0x8000” in create_dhcp_offer_relayed_packet and create_dhcp_ack_relayed_packet

How did you verify/test it?

testbed dhcp_relay test

PLAY RECAP *********************************************************************
cel-e1031-01 : ok=58 changed=10 unreachable=0 failed=0

Wednesday 24 July 2019 09:41:45 +0000 (0:00:00.033) 0:00:30.786 ********

TASK: test : command ---------------------------------------------------- 4.53s
TASK: test : gather system version information -------------------------- 2.49s
TASK: setup ------------------------------------------------------------- 1.72s
TASK: test : Copy tests to the PTF container ---------------------------- 1.67s
TASK: test : Get interface facts ---------------------------------------- 1.47s
TASK: test : Get interface facts ---------------------------------------- 1.43s
TASK: test : set_fact --------------------------------------------------- 1.28s
TASK: test : Get process information in syncd docker -------------------- 1.24s
TASK: test : Get process information in syncd docker -------------------- 1.11s
TASK: test : Verify port channel interfaces are up correctly ------------ 0.98s
TASK: test : validate all interfaces are up after test ------------------ 0.82s
TASK: test : Fetch result files from switch to ansible machine ---------- 0.65s
TASK: test : Verify port channel interfaces are up correctly ------------ 0.64s
TASK: test : Gather minigraph facts about the device -------------------- 0.64s
TASK: test : debug ------------------------------------------------------ 0.60s
TASK: test : Gather minigraph facts about the device -------------------- 0.58s
TASK: test : Gathering minigraph facts about the device ----------------- 0.57s
TASK: test : Get orchagent process information -------------------------- 0.52s
TASK: test : validate all interfaces are up ----------------------------- 0.49s
TASK: test : Get orchagent process information -------------------------- 0.49s

Any platform specific information?

cel_e1031&cel_seastone devices

Supported testbed topology if it's a new test case?

Documentation

[jleveque]

We have not experienced failures with the current packet creation. Can you please paste output from a failed test run?

[bbinxie]

We have not experienced failures with the current packet creation. Can you please paste output from a failed test run?

As below is test failed log, BTW , I used sonic is 201811 branch, and testbed is master branch. What version do you test？

1. Test log from testbed ourput:

TASK [test : debug] ************************************************************
task path: /var/clsnet/git-sw-csa/sonic-mgmt/ansible/roles/test/tasks/ptf_runner.yml:52
Thursday 25 July 2019 02:20:24 +0000 (0:00:05.052) 0:00:22.093 *********
ok: [cel-e1031-01] => {
"out.stdout_lines": [
"WARNING: No route found for IPv6 destination :: (no default route?)",
"dhcp_relay_test.DHCPTest ... FAIL",
"",
"======================================================================",
"FAIL: dhcp_relay_test.DHCPTest",
"----------------------------------------------------------------------",
"Traceback (most recent call last):",
" File "ptftests/dhcp_relay_test.py", line 527, in runTest",
" self.verify_offer_received()",
" File "ptftests/dhcp_relay_test.py", line 445, in verify_offer_received",
" testutils.verify_packet(self, masked_offer, self.client_port_index)",
" File "/usr/lib/python2.7/dist-packages/ptf/testutils.py", line 2243, in verify_packet",
" % (device, port, result.format()))",
"AssertionError: Expected packet was not received on device 0, port 0.",
"========== EXPECTED ==========",
"Mask: ??????????????\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000????????\u0000\u0000??\u0000\u0000\u0000\u0000????????????????????????????????????????????\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000??????????????????????????",
"========== RECEIVED ==========",
"2 total packets. Displaying most recent 2 packets:",
"------------------------------",
"0000 00 E0 EC 4A 4B 00 00 E0 EC 7D 88 7A 08 00 45 10 ...JK....}.z..E.",
"0010 01 22 00 00 00 00 80 11 B8 67 C0 A8 00 01 C0 A8 .".......g......",
"0020 00 02 00 43 00 44 01 0E 7B 1B 02 01 06 00 00 00 ...C.D..{.......",
"0030 00 00 00 00 00 00 00 00 00 00 C0 A8 00 02 C0 00 ................",
"0040 00 01 C0 A8 00 01 00 E0 EC 4A 4B 00 00 00 00 00 .........JK.....",
"0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"0070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"0080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"0090 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"00a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"00b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"00c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"00d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"00e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"00f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"0100 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................",
"0110 00 00 00 00 00 00 63 82 53 63 35 01 02 36 04 C0 ......c.Sc5..6..",
"0120 00 00 01 33 04 00 01 51 80 01 04 FF FF F8 00 FF ...3...Q........",
"------------------------------",
"0000 01 80 C2 00 00 0E 00 E0 EC 7D 88 7A 88 CC 02 07 .........}.z....",
"0010 04 00 E0 EC 7D 88 7A 04 05 07 65 74 70 31 06 02 ....}.z...etp1..",
"0020 00 78 0A 0C 63 65 6C 2D 65 31 30 33 31 2D 30 31 .x..cel-e1031-01",
"0030 0C 62 44 65 62 69 61 6E 20 47 4E 55 2F 4C 69 6E .bDebian GNU/Lin",
"0040 75 78 20 38 20 28 6A 65 73 73 69 65 29 20 4C 69 ux 8 (jessie) Li",
"0050 6E 75 78 20 34 2E 39 2E 30 2D 38 2D 61 6D 64 36 nux 4.9.0-8-amd6",
"0060 34 20 23 31 20 53 4D 50 20 44 65 62 69 61 6E 20 4 #1 SMP Debian ",
"0070 34 2E 39 2E 31 31 30 2D 33 2B 64 65 62 39 75 36 4.9.110-3+deb9u6",
"0080 20 28 32 30 31 35 2D 31 32 2D 31 39 29 20 78 38 (2015-12-19) x8",
"0090 36 5F 36 34 0E 04 00 9C 00 14 10 0C 05 01 0A FA 6_64............",
"00a0 00 64 01 00 00 00 00 00 08 0D 53 65 72 76 65 72 .d........Server",
"00b0 73 30 3A 65 74 68 30 FE 09 00 12 0F 03 01 00 00 s0:eth0.........",
"00c0 00 00 FE 09 00 12 0F 01 00 00 00 00 00 00 00 ...............",
"==============================",
"",
"",
"----------------------------------------------------------------------",
"Ran 1 test in 3.221s",
"",
"FAILED (failures=1)"
]
}

2. Receive package is unicast from ptf.pacp file in ptf docker ， But verify package is broadcast in dhcp_relay_test.py Please check below information.
   
   

[jleveque]

For the future, I suggest synchronizing the branches of your repos (e.g., if your image is built from the 201811 branch of sonic-buildimage, you should test it using the 201811 branch of sonic-mgmt).

Nonetheless, I was able to reproduce this behavior using 201811 branches in both repos, but only when using an older image, not a newer image. Can you tell me what the commit ids of your sonic-buildimage and sonic-mgmt repos are?

[bbinxie]

Thanks for your suggestion.
my sonic-buildimage branch 201811 commit 9ae1103
I try to update the latest 201811, And I found the result is different. dhcp agent send broadcast offer package to client with flag 0. It is different with the old code.

According to my understanding about the work process of dhcp relay， If the “Broadcast Flag” value in the message is 0, the relay agent replaces the value with the IP address (Your IP field: 192.168.0.2) for unicasting. However, if the “Broadcast Flag” value is 1, the relay agent replaces it with the broadcast IP address (255.255.255.255) for broadcasting. right？
So I think in this case， flags=0， dhcp-relay agent should send unitcast， Or we can modify this flags = 1,let dhcp-relay forward broadcast package to Dhcp client. Am I right？

[jleveque]

Your understanding appears correct. If the client sets the "broadcast flag" to 1 in the DISCOVER packet, the relay agent should send the replies back as broadcasts, otherwise, the agent should send them back as unicast.

Is your claim that the latest DHCP realay agent code is always sending broadcast, even if the broadcast flag isn't set? That's what it looks like to me, just wanted to confirm.

[bbinxie]

Yes

[jleveque]

That sounds like a bug, but I'm not sure how any changes I made would have had this side effect. I'll have to look into it further. Can you provide me with the commit from the 201811 branch that you built the image that is failing the test?

[jleveque]

OK. So after some investigation, I found that with my recent changes to the relay agent, in order to support directed broadcasts, I switched the agent over to use linux sockets. The implementation of the agent when using sockets will never send replies back to the client unicast--even if the broadcast flag is not set--due to this function in socket.c:

int can_unicast_without_arp (ip)
    struct interface_info *ip;
{
    return 0;
}

Therefore, this test should now only test with the broadcast flag set. Thus, your PR is the correct fix. However, for the sake of completeness, can you also set the broadcast bit when forming the DISCOVER and REQUEST packets? The full diff should look like this:

--- a/ansible/roles/test/files/ptftests/dhcp_relay_test.py
+++ b/ansible/roles/test/files/ptftests/dhcp_relay_test.py
@@ -154,7 +154,7 @@ class DHCPTest(DataplaneBaseTest):
     """

     def create_dhcp_discover_packet(self):
-        return testutils.dhcp_discover_packet(eth_client=self.client_mac)
+        return testutils.dhcp_discover_packet(eth_client=self.client_mac, set_broadcast_bit=True)

     def create_dhcp_discover_relayed_packet(self):
         my_chaddr = ''.join([chr(int(octet, 16)) for octet in self.client_mac.split(':')])
@@ -183,7 +183,7 @@ class DHCPTest(DataplaneBaseTest):
                     hops=1,
                     xid=0,
                     secs=0,
-                    flags=0,
+                    flags=0x8000,
                     ciaddr=self.DEFAULT_ROUTE_IP,
                     yiaddr=self.DEFAULT_ROUTE_IP,
                     siaddr=self.DEFAULT_ROUTE_IP,
@@ -212,7 +212,8 @@ class DHCPTest(DataplaneBaseTest):
                     ip_gateway=self.relay_iface_ip,
                     netmask_client=self.client_subnet,
                     dhcp_lease=self.LEASE_TIME,
-                    padding_bytes=0)
+                    padding_bytes=0,
+                    set_broadcast_bit=True)

     def create_dhcp_offer_relayed_packet(self):
         my_chaddr = ''.join([chr(int(octet, 16)) for octet in self.client_mac.split(':')])
@@ -233,7 +234,7 @@ class DHCPTest(DataplaneBaseTest):
                     hops=0,
                     xid=0,
                     secs=0,
-                    flags=0,
+                    flags=0x8000,
                     ciaddr=self.DEFAULT_ROUTE_IP,
                     yiaddr=self.client_ip,
                     siaddr=self.server_ip,
@@ -257,7 +258,8 @@ class DHCPTest(DataplaneBaseTest):
     def create_dhcp_request_packet(self):
         return testutils.dhcp_request_packet(eth_client=self.client_mac,
                     ip_server=self.server_ip,
-                    ip_requested=self.client_ip)
+                    ip_requested=self.client_ip,
+                    set_broadcast_bit=True)

     def create_dhcp_request_relayed_packet(self):
         my_chaddr = ''.join([chr(int(octet, 16)) for octet in self.client_mac.split(':')])
@@ -279,7 +281,7 @@ class DHCPTest(DataplaneBaseTest):
                     hops=1,
                     xid=0,
                     secs=0,
-                    flags=0,
+                    flags=0x8000,
                     ciaddr=self.DEFAULT_ROUTE_IP,
                     yiaddr=self.DEFAULT_ROUTE_IP,
                     siaddr=self.DEFAULT_ROUTE_IP,
@@ -310,7 +312,8 @@ class DHCPTest(DataplaneBaseTest):
                     ip_gateway=self.relay_iface_ip,
                     netmask_client=self.client_subnet,
                     dhcp_lease=self.LEASE_TIME,
-                    padding_bytes=0)
+                    padding_bytes=0,
+                    set_broadcast_bit=True)

     def create_dhcp_ack_relayed_packet(self):
         my_chaddr = ''.join([chr(int(octet, 16)) for octet in self.client_mac.split(':')])
@@ -331,7 +334,7 @@ class DHCPTest(DataplaneBaseTest):
                     hops=0,
                     xid=0,
                     secs=0,
-                    flags=0,
+                    flags=0x8000,
                     ciaddr=self.DEFAULT_ROUTE_IP,
                     yiaddr=self.client_ip,
                     siaddr=self.server_ip,

[bbinxie]

OK,I have modify it,Please check. Thank you！

[jleveque]

Thanks!