HLD: DHCPv4 - Specify dhcp relay's Gateway explicitly with Primary address. #1470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pushpraj
force-pushed
the
dhcpv4_gateway_explicit
branch
from
4679933 to
a419786
Compare
venkatmahalingam
suggested changes
Sep 15, 2023
prsunny
reviewed
Oct 3, 2023
Can you please explain more about this problem? DHCP relay is enabled on a given VLAN/Interface and the 'giaddr' is populated using the IP address assigned to that VLAN/interface. So where are the "multiple interfaces" coming from? Are you referring to a scenario whether a given VLAN/interface can have multiple IP addresses (one primary and several secondary)? |
Yes. this is the scenario where one VLAN can have multiple IP addresses. We have updated the design to mark such additional addresses as 'secondary' now.
Yes. |
doc/DHCPv4_Gateway/DHCPv4_gateway.md
Outdated
| 1. Support a new member 'secondary' of VLAN_INTERFACE in config_db. | ||
| 2. Support parsing and assignment of subnets from minigraph/json to config_db. | ||
| 3. Support specifying non-secondary interfaces' gateway address to command line arguments to /usr/sbin/dhcrelay as -g. | ||
| 4. isc-dhcp/dhcrelay to support '-g gateway' argument, by porting an existing patch. |
There was a problem hiding this comment.
Hope we handle any on-the-fly primary/secondary flag changes in CONFIG_DB for VLAN interface? i.e restarting the DHCP relay to pickup right primary IP address.
There was a problem hiding this comment.
Add relay address by cli would cause dhcp_relay container to restart https://github.com/sonic-net/sonic-buildimage/blob/c71fb3a30f3d384438f2e4861c80c28aea398fef/dockers/docker-dhcp-relay/cli/config/plugins/dhcp_relay.py#L77
|
Community review recording https://zoom.us/rec/share/LiTYydJg8GJCntz_HL1DZk-sFQxMdcAA89Mj35l3zBAUYtLgxNR4XZyq4p9e_xqC.eAJyKMK62ORcaaFZ |
pushpraj
force-pushed
the
dhcpv4_gateway_explicit
branch
from
53f77b7 to
fbb584e
Compare
pushpraj
changed the title
|
@venkatmahalingam can you please review this hld, and respective PRs too? thank you. |
This was referenced Nov 22, 2023
lguohan
pushed a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Nov 22, 2023
This is change taken as part of the HLD: sonic-net/SONiC#1470 and this is a follow up on the PR #16827 where in the docker-dhcp we pick the value of primary gateway of the interface from the VLAN_Interface table which has "secondary" flag set in the config_db Microsoft ADO (number only): 16784946 How did I do it - Changes in the j2 file to add a new "-pg" parameter in the dhcpv4-relay.agents.j2, the ip would be retrieved from the config db's vlan_interface table such that the interface which are picked will have secondary field set. - Changes in isc-dhcp to re-order the addresses of the discovered interface and which has the ip which has the passed parameter.
lguohan
pushed a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Nov 22, 2023
This is change taken as part of the HLD: sonic-net/SONiC#1470. In this PR we add the logic to parse the SecondarySubnets field in the minigraph and add a flag in "secondary" in the vlan_interface table of the config db. Microsoft ADO (number only): 16784946 How I did it Made changes in the minigraph.py to parse the xml entry and add the parsed value to the config db How to verify it Added python tests in the sonic-config-engine folder to test the config db entries.
yxieca
pushed a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Dec 4, 2023
This is change taken as part of the HLD: sonic-net/SONiC#1470 and this is a follow up on the PR #16827 where in the docker-dhcp we pick the value of primary gateway of the interface from the VLAN_Interface table which has "secondary" flag set in the config_db Microsoft ADO (number only): 16784946 How did I do it - Changes in the j2 file to add a new "-pg" parameter in the dhcpv4-relay.agents.j2, the ip would be retrieved from the config db's vlan_interface table such that the interface which are picked will have secondary field set. - Changes in isc-dhcp to re-order the addresses of the discovered interface and which has the ip which has the passed parameter.
yxieca
pushed a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Dec 4, 2023
This is change taken as part of the HLD: sonic-net/SONiC#1470. In this PR we add the logic to parse the SecondarySubnets field in the minigraph and add a flag in "secondary" in the vlan_interface table of the config db. Microsoft ADO (number only): 16784946 How I did it Made changes in the minigraph.py to parse the xml entry and add the parsed value to the config db How to verify it Added python tests in the sonic-config-engine folder to test the config db entries.
|
@pushpraj I tried back-porting this feature on 202111. I see that it is working. But I observed one weird behaviour. Issue: During the normal scenario it works fine as expected. But after reboot of the device, dhcp-relay honours the primary address and forwards the dhcp request via the correct subnet / gateway (172.19.5.x). But when it receives the dhcp reply from the server, dhcp-relay forwards the IP back to the client via 1.1.1.x gateway (secondary) in place of 172.19.5.x. LOGS: Are you also seeing similar behaviour? |
|
@prsunny @venkatmahalingam can you please help to approve this PR if you are ok? Thanks. |
|
@pushpraj I am wondering if it's possible to specify an IP address as the primary address for the DHCP relay service. In the current design, the 'secondary' parameter's meaning is applied under the interface, encompassing the entire interface view. However, this feature is intended only for the DHCP relay, not for the entire interface |
|
@prsunny @venkatmahalingam are you ok to merge this PR? Thanks. |
|
yes @zhangyanzhao , we can merge this PR |
venkatmahalingam
approved these changes
May 13, 2024
prsunny
approved these changes
May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This document describes the High Level Design of 'Secondary' interfaces of vlan. These secondary interfaces are also excluded in use in dhcpv4 relay.
A vlan can have more than one subnet assigned to it. Such additional interfaces should be tagged as 'secondary'. Sonic OS should support dhcpv4 over non-secondary interfaces.
Use case: We want to increase the IP addresses range supported by a live switch. These additional IPs are to be used for new scenarios on existing physical server by additional IP addresses.
Challenge: DHCPv4 is affected when sonic switch is configured with multiple interfaces in a vlan. When a dhcpv4 packet is forwarded via dhcprelayagent, it embeds its own gateway address as 'giaddr' for return communication.
In case of multiple interfaces, dhcprelayagent randomizes giaddr among all interfaces.
This behavior is fixed by marking additional subnets as 'secondary' and explicitly specifying the primary interface as gateway for dhcpv4.