Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DHCP relay for IPv6 HLD #765

Merged
merged 1 commit into from
Jul 1, 2021
Merged

DHCP relay for IPv6 HLD #765

merged 1 commit into from
Jul 1, 2021

Conversation

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

DHCP Relay for IPv6 feature in SONiC should meet the following high-level functional requirements:

- Give the support for relaying DHCP packets from downstream networks to upstream networks using IPv6 addresses.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more requirement here is to support RFC:6936 option 79 as part of DHCPv6 relay functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is out of the scope of this HLD. this HLD is to cover only existing functionality on top of IPv6.
Any additional requirements should come with a new HLD to cover the new requirements.
@tahmed-dev do you agree?


## 2.3 DHCP Monitor

The existing DHCP monitor will be enhanced in order to support monitoring for DHCP IPv6 as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DHCP counters will be part of the new DHCPv6 relay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with the new approach the DHCP monitor application will be removed?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any support for DHCPv6 relay counters ? These will help a lot for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is, the DHCP monitor application is now supporting monitor for these counters as well.
Please check the PR of the implementation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any support for DHCPv6 relay counters ? These will help a lot for debugging.

Yes, there will be native support in the app to pull counters. Please have a look at PR:787

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is, the DHCP monitor application is now supporting monitor for these counters as well.
Please check the PR of the implementation.

the dhcpmon will not exist in DHCPv6 world, rather its functionality will be in the new SONiC DHCPv6 RA.

Copy link
Contributor Author

@shlomibitton shlomibitton Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tahmed-dev Can you please elaborate why the dhcpmon will not exist?
Same as the IPv4 implementation of this monitor, we can check inconsistency in received and transmitted packets.
for received SOLICIT/REQUEST packets we expect to see a RELAY-FORWARD packet transmitted.
for received RELAY-REPLY packets we expect to see ADVERTISE/REPLY packet transmitted.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shlomibitton The driving force to have a monitoring application (dhcpmon) was that ISC DHCP does not integrate well with SONIiC infra and its counters were not readily available via telemetry.. With the new DHCPv6 RA, it will integrate with the SONiC redis db and its counters will be available via telemetry and cli. So, we thought about eliminating it.

One might argue that the monitoring app would still capture anomalies in DHCPv6 behavior as it is external to it. I am not sure if this is a design pattern we would like to follow as if we end with every app having a shadow app that monitors it, one can image the number of app would grow...

Also, dhcpmon is launching per interface which is inefficient to have a process for every interface that has dhcp relay enabled.

Please comment on PR:787

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tahmed-dev thanks for the explanation.
On this case, what do you suggest to use as a monitor application with the isc-dhcp-relay package on this case?
This HLD is based on this implementation and we would like to have some kind of a monitor right?
On the other HLD on 'counters' section all I can see is the approach to keep the number of each message type but that's about it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shlomibitton counters will be available via telemetry. The cloud would alert on counters if incrementing. Also, we can alternatively add checks similar to dhcpmon monitoring check and print to syslog. I believe Telemetry is maturing now and so the former solution should be adopted.

For this HLD, it is fine to have dhcpmon changes for the time being. This HLD and the accompanying code will be reverted once the new solution is merged.

- Downstream network is the VLAN interface with the relay configuration. Global IPv6 address is required to be configured on that interface.
- Config DB schema should meet the following format:
```
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not finalized yer, however the idea is to have DHCP in a separate feature table instead of being property of Vlan. Something as follows:

    "DHCP": {
        "downlink-intf-i": {
            "dhcpv4_servers": ["dhcp-server-0", "dhcp-server-1", ...., "dhcp-server-n-1"],
            "dhcpv6_servers": ["dhcp-server-0", "dhcp-server-1", ...., "dhcp-server-n-1"]
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it is implemented in a way supervisord on dhcp_relay docker is taking the data by the 'dhcp_servers' on the Vlan interface.
To adapt this new approach all needed is to change the template to this schema.

The new process will listen to DHCP packets for IPv6 and forward them to the relevant interface according to the configuration.
For example, from the configuration described on the previous section, the following daemon will start:
```
admin@sonic:/# /usr/sbin/dhcrelay -6 -d --name-alias-map-file /tmp/port-name-alias-map.txt -l Vlan1000 -u 21da:d3:0:2f3b::7%Ethernet28 -u 21da:d3:0:2f3b::6%Ethernet28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new process will read config data from config db and will not rely on cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, this instance is taken from config DB data, by the template generated for supervisord on dhcp_realy docker.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it mandatory to specify the upstream interface along with the DHCPv6 server address? The server may not be directly connected to the L3 relay - for example, we could have an IPv6 route to the DHCPv6 server. Will that scenario work? What happens if the IPv6 addresses are removed/re-configured to different upstream interface? Will that require restart of DHCP docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the user this configuration is transparent, same as the IPv4 implementation, isc-dhcp-relay will try to relay DHCP packets to all configured servers, the kernel will determine if to send the packet or not depending on the route available in the kernel.
If the server is not directly connected to the L3 interface the DHCP_FORWARD message will be transmitted, if another relay is on the other side, it will forward the message again until it will reach the server and vise versa.
if the IPv6 addresses are removed/re-configured on the L3 up link interfaces than yes, a restart of the service is required.

@liat-grozovik
Copy link
Collaborator

@tahmed-dev and @lguohan
This HLD was presented in the community as planned and based on it the PRs will be soon available.
Beside of the fact that new DHCP relay for IPv6 will be available later, do you have any feedback to this HLD or we can move forward and approve it?

@liat-grozovik
Copy link
Collaborator

@tahmed-dev and @lguohan
This HLD was presented in the community as planned and based on it the PRs will be soon available.
Beside of the fact that new DHCP relay for IPv6 will be available later, do you have any feedback to this HLD or we can move forward and approve it?

@tahmed-dev can you please approve?

The new process will listen to DHCP packets for IPv6 and forward them to the relevant interface according to the configuration.
For example, from the configuration described on the previous section, the following daemon will start:
```
admin@sonic:/# /usr/sbin/dhcrelay -6 -d --name-alias-map-file /tmp/port-name-alias-map.txt -l Vlan1000 -u 21da:d3:0:2f3b::7%Ethernet28 -u 21da:d3:0:2f3b::6%Ethernet28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it mandatory to specify the upstream interface along with the DHCPv6 server address? The server may not be directly connected to the L3 relay - for example, we could have an IPv6 route to the DHCPv6 server. Will that scenario work? What happens if the IPv6 addresses are removed/re-configured to different upstream interface? Will that require restart of DHCP docker?

## 1.2 Configuration and Management Requirements

- DHCPv6 trap should be enabled through the COPP manager when the DHCP relay feature is enabled and vice versa.
- Downstream network is the VLAN interface with the relay configuration. Global IPv6 address is required to be configured on that interface.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the downstream interface be a physical L3 interfaces or PortChannel L3 interface (with out VLAN)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the scope of this HLD, the downsteam network designed to be a Vlan interface.


## 2.3 DHCP Monitor

The existing DHCP monitor will be enhanced in order to support monitoring for DHCP IPv6 as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any support for DHCPv6 relay counters ? These will help a lot for debugging.

Copy link

@tahmed-dev tahmed-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should close this PR in favor of PR:787

@liat-grozovik
Copy link
Collaborator

I think we should close this PR in favor of PR:787

I disagree.
This HLD was before any other HLD. this HLD was part of the early stage of 202106 and the provided changes/fixes are aligned with this HLD.
The other HLD was never discussed in the community and can be an enhancement on top of it.
Please proceed with this HLD approval and the code review of all the PRs tracked on this HLD.

@tahmed-dev
Copy link

I think we should close this PR in favor of PR:787

I disagree.
This HLD was before any other HLD. this HLD was part of the early stage of 202106 and the provided changes/fixes are aligned with this HLD.
The other HLD was never discussed in the community and can be an enhancement on top of it.
Please proceed with this HLD approval and the code review of all the PRs tracked on this HLD.

Thanks @liat-grozovik! Can we agree on the following two points?

  1. We are not going to have two DHCPv6 relay agents in SONiC build-image repository, and
  2. There should not be two HLDs describing the functionality of the same feature/binary.

Copy link

@tahmed-dev tahmed-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This HLD will serve as bridge gap solution and will be reverted once new HLD and new DHCPv6 relay agent are merged.

@tahmed-dev tahmed-dev merged commit cf61ace into sonic-net:master Jul 1, 2021
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 16, 2021
Why I did it
Currently SONiC use the 'isc-dhcp-relay' package to allow DHCP relay functionality on IPv4 networks only.
This will allow the IPv6 functionality along the IPv4 type.

How I did it
Edit supervisord template to start DHCPv6 instances when configured to do so on Config DB.
Align cfg unit test to the new change.
Add DHCPv6 relay minigraph parsing support and a suitable t0 topology xml file for UT.

How to verify it
Configure DHCPv6 agents as described on the feature HLD: sonic-net/SONiC#765
Test it with real client/server with IPv6 or use the dedicated automatic test: sonic-net/sonic-mgmt#3565
Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

* Split docker-dhcp-relay.supervisord.conf.j2 template into several files for easier code maintenance
@shlomibitton shlomibitton deleted the shlomi_dhcpv6 branch July 27, 2021 16:35
judyjoseph pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 4, 2021
Why I did it
Currently SONiC use the 'isc-dhcp-relay' package to allow DHCP relay functionality on IPv4 networks only.
This will allow the IPv6 functionality along the IPv4 type.

How I did it
Edit supervisord template to start DHCPv6 instances when configured to do so on Config DB.
Align cfg unit test to the new change.
Add DHCPv6 relay minigraph parsing support and a suitable t0 topology xml file for UT.

How to verify it
Configure DHCPv6 agents as described on the feature HLD: sonic-net/SONiC#765
Test it with real client/server with IPv6 or use the dedicated automatic test: sonic-net/sonic-mgmt#3565
Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

* Split docker-dhcp-relay.supervisord.conf.j2 template into several files for easier code maintenance
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Why I did it
Currently SONiC use the 'isc-dhcp-relay' package to allow DHCP relay functionality on IPv4 networks only.
This will allow the IPv6 functionality along the IPv4 type.

How I did it
Edit supervisord template to start DHCPv6 instances when configured to do so on Config DB.
Align cfg unit test to the new change.
Add DHCPv6 relay minigraph parsing support and a suitable t0 topology xml file for UT.

How to verify it
Configure DHCPv6 agents as described on the feature HLD: sonic-net/SONiC#765
Test it with real client/server with IPv6 or use the dedicated automatic test: sonic-net/sonic-mgmt#3565
Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

* Split docker-dhcp-relay.supervisord.conf.j2 template into several files for easier code maintenance
kellyyeh pushed a commit to kellyyeh/sonic-buildimage that referenced this pull request Sep 29, 2021
Why I did it
Currently SONiC use the 'isc-dhcp-relay' package to allow DHCP relay functionality on IPv4 networks only.
This will allow the IPv6 functionality along the IPv4 type.

How I did it
Edit supervisord template to start DHCPv6 instances when configured to do so on Config DB.
Align cfg unit test to the new change.
Add DHCPv6 relay minigraph parsing support and a suitable t0 topology xml file for UT.

How to verify it
Configure DHCPv6 agents as described on the feature HLD: sonic-net/SONiC#765
Test it with real client/server with IPv6 or use the dedicated automatic test: sonic-net/sonic-mgmt#3565
Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

* Split docker-dhcp-relay.supervisord.conf.j2 template into several files for easier code maintenance

(cherry picked from commit 604becd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants