DHCP Relay support for Physical and PO L3 interfaces #1311

AkhileshSamineni · 2020-12-16T02:42:08Z

Currently DHCP Relay support is for Vlan interfaces only.
This PR, extending the DHCP Relay support to Physical and Port channels L3 interfaces.

Config Commands :

**config interface ip dhcp-relay add <interface_name> <ip_addr1> <ip_addr2> <ip_addr3> <ip_addr4>**

This command enables user to configure IPv4 DHCP Server address on an interface. 
- One address is mandatory and a maximum of four addresses are allowed. 
- If the command is used multiple times with different addresses, the addresses are appended to the list of server addresses.

- Examples:

  admin@sonic:~$ sudo config interface ip dhcp-relay add Ethernet52 1.2.0.1 3.4.0.1 192.168.0.1
  admin@sonic:~$ sudo config interface ip dhcp-relay add Vlan206 1.2.0.1 3.4.0.1
  admin@sonic:~$ sudo config interface ip dhcp-relay add PortChannel007 192.168.0.1

**config interface ip dhcp-relay remove <interface_name> <ip_addr1> <ip_addr2> <ip_addr3> <ip_addr4>**

This command enables user to remove the configured DHCP Server address on an interface. 
- One address is mandatory and a maximum of four addresses are allowed. 
- A single server address can be deleted from the list of configured addresses by providing that address as argument.

- Examples:

  admin@sonic:~$ sudo config interface ip dhcp-relay remove Ethernet52 1.2.0.1 3.4.0.1 192.168.0.1 192.168.5.1
  admin@sonic:~$ sudo config interface ip dhcp-relay remove Vlan206 1.2.0.1 3.4.0.1 
  admin@sonic:~$ sudo config interface ip dhcp-relay remove PortChannel007 192.168.0.1 

These add/remove new commands are inline with the below existing command, which is used to configure the dhcp-relay on a VLAN.
  config vlan dhcp_relay add <vlan_id> <dhcp_relay_destination_ip>
  config vlan dhcp_relay del <vlan-id> <dhcp_relay_destination_ip>

Show Command :

**show ip dhcp-relay brief**

This command displays all the configured DHCP Server address configurations.

Sample Output:
admin@sonic:~$ sudo show ip dhcp-relay brief

+------------------+-------------------------+
| Interface Name   | DHCP Helper Address     |
+==========+=================================+
| Vlan10           | 20.20.1.2               |
|                  | 1.2.0.1                 |
+------------------+-------------------------+
| PortChannel10    | 40.20.1.2               |
+------------------+-------------------------+
| Ethernet52       | 10.10.1.2               |
+------------------+-------------------------+

Signed-off-by: Akhilesh Samineni [email protected]

lgtm-com · 2020-12-16T02:49:52Z

This pull request introduces 1 alert when merging b0c5cb7cd2117b3bd24dbfb408364838ce3d5f9d into 394b202 - view on LGTM.com

new alerts:

1 for Unused local variable

lguohan · 2020-12-16T22:04:08Z

can you add test?

ben-gale · 2021-01-12T17:13:22Z

Retest please

prsunny · 2021-01-29T01:40:52Z

tests/mock_tables/config_db.json

@@ -547,7 +547,7 @@
        "NULL": "NULL"
    },
    "INTERFACE|Ethernet0": {
-        "NULL": "NULL"
+        "dhcp_servers@": "12.0.0.1,12.0.0.2,12.0.0.3"


This is not according to the existing schema. INTERFACE tables does not have dhcp_servers as attribute . Are you proposing a schema change? Currently for vlans, it is part of VLAN table and not VLAN_INTERFACE

Yes, "dhcp_servers" is an L3 attribute, so added it to INTERFACE and PORTCHANNEL_INTERFACE tables.
For Vlan case, still using VLAN table only.

It will be inconsistent with VLAN_INTERFACE. Fine with me if @tahmed-dev, @jleveque and @lguohan sign off.

I agree with @prsunny. Lumping dhcp_servers with the interface is inconsistent with the current. I think since PR:6221 attempts to have DHCP relay preside on almost ever interface, it would be better to have separate table for DHCP_RELAY configuration. @lguohan, @jleveque what are your thoughts on that?

I think this is debatable. Previously we considered the list of DHCP server IPs to be an attribute of a VLAN table entry. This approach is consistent with the current approach, extending it to INTERFACE and PORTCHANNEL_INTERFACE tables. We could take your approach, but this would require refactoring to create a DHCP_RELAY table and I guess have the attributes be the interface names? Basically inverting this approach?

It is indeed. I am thinking of having just one binary to serve as DHCP relay down the road and so having those entries consolidated under DHCP_RELAY table would be self contained and eliminate the need to search multiple tables.

Yes, the attributes would be interface name serving as a key and a downlink at the same time and the map/table would contain uplink interfaces and DHCP helper IPs (helper IPs could be per DHCP feature.)

Is there a conclusion here please guys?

If we add new table like "DHCP_RELAY" table, then we need to change the supervisord.conf.j2 file also accordingly right, it be leads more changes for now.

Can we move the dhcp_servers from VLAN table to VLAN_INTERFACE table to be more consistent ??

jleveque · 2021-01-29T18:12:46Z

config/main.py

+# Use this method to validate unicast IPv4 address
+#
+def is_ip4_addr_valid(addr, display):
+    v4_invalid_list = [ipaddress.IPv4Address(unicode('0.0.0.0')), ipaddress.IPv4Address(unicode('255.255.255.255'))]


Conversion to unicode() should not be necessary, now that sonic-utilities is Python 3-only. This may also be causing the PR test failures.

Done. Removed unicode function.

jleveque · 2021-01-29T18:12:50Z

config/main.py

+def is_ip4_addr_valid(addr, display):
+    v4_invalid_list = [ipaddress.IPv4Address(unicode('0.0.0.0')), ipaddress.IPv4Address(unicode('255.255.255.255'))]
+    try:
+        ip = ipaddress.ip_address(unicode(addr))


Conversion to unicode() should not be necessary, now that sonic-utilities is Python 3-only. This may also be causing the PR test failures.

Done. Removed unicode function.

AkhileshSamineni · 2021-01-30T10:23:05Z

Retest this please.

AkhileshSamineni · 2021-01-31T12:31:50Z

Retest this please.

AkhileshSamineni · 2021-02-01T19:00:41Z

Retest this please.

AkhileshSamineni · 2021-02-04T14:24:36Z

Retest this please.

AkhileshSamineni · 2021-02-06T05:24:08Z

Retest this please.

…erface and Link-select support Signed-off-by: Akhilesh Samineni <[email protected]>

Signed-off-by: Akhilesh Samineni <[email protected]>

ben-gale added the brcm-upstream label Dec 16, 2020

AkhileshSamineni mentioned this pull request Jan 5, 2021

DHCP Relay support for Physical and PO L3 interfaces sonic-net/sonic-buildimage#6221

Closed

AkhileshSamineni changed the title ~~DHCP Relay support for Physical and PO L3 interfaces and also src-Interface and Link-select support~~ DHCP Relay support for Physical and PO L3 interfaces Jan 13, 2021

prsunny reviewed Jan 29, 2021

View reviewed changes

jleveque suggested changes Jan 29, 2021

View reviewed changes

prsunny requested review from tahmed-dev and lguohan January 29, 2021 19:12

AkhileshSamineni added 12 commits March 4, 2021 03:16

DHCP Relay support for Physical and PO L3 interfaces and also src-Int…

b3fc142

…erface and Link-select support Signed-off-by: Akhilesh Samineni <[email protected]>

Fixed LGTM error

9f3755b

Signed-off-by: Akhilesh Samineni <[email protected]>

Added dhcp_relay test cases

d9722ad

Signed-off-by: Akhilesh Samineni <[email protected]>

Fixed test cases.

d15cb97

Signed-off-by: Akhilesh Samineni <[email protected]>

Removed link-select and src-interface related changes.

abb28d3

Signed-off-by: Akhilesh Samineni <[email protected]>

Fixed test cases.

1193ad5

Signed-off-by: Akhilesh Samineni <[email protected]>

Added some unit test cases

7c157b4

Testcase fixes.

99d29c3

Removed unicode as it not supported by Python3.

9766696

Added mock_run_command changes for the test cases.

ad92eb3

Fixed failed test case exception.

72cfd89

Added Portchannel test case.

25f6d41

AkhileshSamineni force-pushed the dhcp_relay_support_ports branch from 8f445c9 to 25f6d41 Compare March 4, 2021 11:32

File correction after forced-push

70ca1ff

AkhileshSamineni closed this Nov 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DHCP Relay support for Physical and PO L3 interfaces #1311

DHCP Relay support for Physical and PO L3 interfaces #1311

AkhileshSamineni commented Dec 16, 2020 •

edited

Loading

lgtm-com bot commented Dec 16, 2020

lguohan commented Dec 16, 2020

ben-gale commented Jan 12, 2021

prsunny Jan 29, 2021

AkhileshSamineni Jan 29, 2021

prsunny Jan 29, 2021

tahmed-dev Jan 29, 2021

jleveque Jan 30, 2021

tahmed-dev Feb 2, 2021

ben-gale Feb 3, 2021

AkhileshSamineni Feb 4, 2021 •

edited

Loading

jleveque Jan 29, 2021

AkhileshSamineni Jan 29, 2021

jleveque Jan 29, 2021

AkhileshSamineni Jan 29, 2021

AkhileshSamineni commented Jan 30, 2021

AkhileshSamineni commented Jan 31, 2021

AkhileshSamineni commented Feb 1, 2021

AkhileshSamineni commented Feb 4, 2021

AkhileshSamineni commented Feb 6, 2021

DHCP Relay support for Physical and PO L3 interfaces #1311

DHCP Relay support for Physical and PO L3 interfaces #1311

Conversation

AkhileshSamineni commented Dec 16, 2020 • edited Loading

lgtm-com bot commented Dec 16, 2020

lguohan commented Dec 16, 2020

ben-gale commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkhileshSamineni Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkhileshSamineni commented Jan 30, 2021

AkhileshSamineni commented Jan 31, 2021

AkhileshSamineni commented Feb 1, 2021

AkhileshSamineni commented Feb 4, 2021

AkhileshSamineni commented Feb 6, 2021

AkhileshSamineni commented Dec 16, 2020 •

edited

Loading

AkhileshSamineni Feb 4, 2021 •

edited

Loading