Add DHCPv6 Relay Agent by kellyyeh · Pull Request #8251 · sonic-net/sonic-buildimage

kellyyeh · 2021-07-22T20:59:05Z

DHCPv6 is a network protocol used to assign IPv6 addresses and provide configuration for devices to communicate on a network. DHCPv6 relay agent helps forwards DHCP packets between clients and servers that do not reside on a shared physical subnet, so that all subnets can share DHCPv6 server, and DHCPv6 server is not required on every LAN. This DHCPv6 relay agent supports option 79 which incorporates the client link-layer address in Relay-Forward messages that is sent to the server, which is needed by the server to allow IPv6 reservation to clients based on their physical MAC addresses.

This PR is part of: sonic-net/SONiC#787
Signed-off-by: Kelly Yeh [email protected]

Why I did it

To support option 79 in DHCPv6.

How I did it

Implemented DHCPv6 operations along with option 79.
Read from config_db to process dhcpv6 server addresses and options.

How to verify it

Build the binary with Makefile and run it on switch with IPv6 configured and DHCPv6 server support

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

tahmed-dev

Thanks!

tahmed-dev · 2021-07-23T23:07:38Z

src/dhcp6relay/src/configInterface.cpp

+        std::string vlan = kfvKey(entry);
+        std::vector<swss::FieldValueTuple> fieldValues = kfvFieldsValues(entry);
+
+        for (auto &fieldValue: fieldValues) {


we need to check the operation here if it is ADD/DEL

The dhcpv6 server address are type string value. e.g.: "fc02:2000::1,fc02:2000::2"
HDEL would delete the entire string of addresses. HSET updates the list of server addresses.
When config_db updates the list of server addresses, all addresses are properly fetched and processed regardless new addresses are added/deleted. Therefore, I don't think there is a need to check for add/del

You can get the operation using std::string operation = kfvOp(entry);. This way we can tell the type of the record updates. I think they are SET/DEL operation.

src/dhcp6relay/src/main.cpp

src/dhcp6relay/src/relay.cpp

tahmed-dev · 2021-07-23T23:23:32Z

src/dhcp6relay/src/relay.cpp

+    addr[0] = 0;
+
+    if (getifaddrs(&ifa) == -1) {
+		    syslog(LOG_WARNING, "getifaddrs: Unable to get network interfaces\n");


nit: use spaces only for indentation

src/dhcp6relay/src/relay.cpp

tahmed-dev · 2021-07-23T23:37:33Z

src/dhcp6relay/src/relay.cpp

+    auto dhcp_header = parse_dhcpv6_hdr(current_position);
+
+    if ((ntohs(udp_header->dest)) == 547 ) {
+        if(dhcp_header->msg_type == SOLICIT || REQUEST || CONFIRM || RENEW || REBIND || RELEASE || DECLINE) {


what is meant by this condition? this is true all the time

Thanks for catching. Fixed.

tahmed-dev · 2021-07-23T23:38:36Z

src/dhcp6relay/src/relay.cpp

+ * @return              none
+ */
+void listen_server(relay_config *config) {
+    while (serverListener) {


why the server listener is not part of the even loop?

Added server listener to libevent

tahmed-dev · 2021-07-30T20:10:05Z

src/dhcp6relay/src/relay.cpp

-        if(dhcp_header->msg_type == SOLICIT || REQUEST || CONFIRM || RENEW || REBIND || RELEASE || DECLINE) {
-            relay_client(config->local_sock, current_position, ntohs(udp_header->len) - sizeof(udphdr), ip_header, ether_header, config);
-        }    
+    if (((ntohs(udp_header->dest)) == RELAY_PORT) && (dhcp_header->msg_type == SOLICIT || REQUEST || CONFIRM || RENEW || REBIND || RELEASE || DECLINE)) {


This condition will always be true:
(dhcp_header->msg_type == SOLICIT || REQUEST || CONFIRM || RENEW || REBIND || RELEASE || DECLINE)

tahmed-dev · 2021-08-03T20:56:20Z

src/dhcp6relay/debian/rules

@@ -0,0 +1,7 @@
+#!/usr/bin/make -f
+
+DEB_CFLAGS_APPEND=-std=gnu11


you can add this flag to CFLAGS in Makefile above.

tahmed-dev · 2021-08-03T20:57:51Z

src/dhcp6relay/src/configInterface.cpp

+
+constexpr auto DEFAULT_TIMEOUT_MSEC = 1000;
+
+bool mPollSwssNotifcation = true;


you can drop the letter m as it is normally used to indicate class member.

tahmed-dev · 2021-08-03T21:00:49Z

src/dhcp6relay/src/configInterface.cpp

+void handleSwssNotification(arg_config *context)
+{
+    std::shared_ptr<swss::DBConnector> configDbPtr = std::make_shared<swss::DBConnector> ("CONFIG_DB", 0);
+    swss::SubscriberStateTable ipHelpersTable(configDbPtr.get(), "DHCP");


We need to add DHCP as a table name in schema.h file.

tahmed-dev · 2021-08-03T21:02:44Z

src/dhcp6relay/src/configInterface.cpp

+                while (ss.good()) {
+                    std::string substr;
+                    getline(ss, substr, ',');
+                    if(substr == "79")


is the option 79 name up-to-date?

tahmed-dev · 2021-08-03T21:03:51Z

src/dhcp6relay/src/configInterface.h

+#include "select.h"
+#include "relay.h"
+
+void initialize_swss(arg_config *context);


please add doxygen comments to functions in this file.

tahmed-dev · 2021-08-03T21:05:53Z

src/dhcp6relay/src/relay.cpp

+
+    ifa_tmp = ifa;
+	while (ifa_tmp) {
+		if ((ifa_tmp->ifa_addr) && (ifa_tmp->ifa_addr->sa_family == AF_INET6)) {


indentation

please remove tabs and use 4 spaces indentation.

tahmed-dev · 2021-08-03T21:10:52Z

src/dhcp6relay/src/main.cpp

+    }
+    catch (std::exception &e)
+    {
+        printf("Usage: -i <interface> -o <option>\n");


nit: we are not using cli args anymore.

tahmed-dev · 2021-08-03T21:15:03Z

src/dhcp6relay/src/relay.h

+/* DHCPv6 filter */
+/* sudo tcpdump -dd "ip6 dst ff02::1:2 && udp dst port 547" */
+
+static struct sock_filter ether_relay_filter[] = {


please move this static variable to .c file

src/dhcp6relay/src/relay.cpp

src/dhcp6relay/src/main.cpp

src/dhcp6relay/src/relay.cpp

tahmed-dev · 2021-08-13T23:13:08Z

src/dhcp6relay/src/relay.cpp

+
+    ifa_tmp = ifa;
+	while (ifa_tmp) {
+		if ((ifa_tmp->ifa_addr) && (ifa_tmp->ifa_addr->sa_family == AF_INET6)) {


please remove tabs and use 4 spaces indentation.

tahmed-dev · 2021-08-13T23:14:49Z

src/dhcp6relay/src/relay.cpp

+ * @return              none
+ */
+ void relay_relay_reply(int sock, const uint8_t *msg, int32_t len, relay_config *configs) {
+    uint8_t buffer[4096];


can you please avoid having large local vars?

src/dhcp6relay/src/main.cpp

src/dhcp6relay/src/relay.cpp

* Added DHCPv6 Relay * Added DHCPv6 Counter (cherry picked from commit f0e640f)

* Added DHCPv6 Relay * Added DHCPv6 Counter

kellyyeh added 2 commits July 22, 2021 20:26

Added DHCPv6 Relay

f1db17f

Added DHCPv6 Relay

18a598b

kellyyeh requested a review from tahmed-dev July 22, 2021 20:59

kellyyeh requested a review from lguohan as a code owner July 22, 2021 20:59

tahmed-dev reviewed Jul 23, 2021

View reviewed changes

Deleted argument parser and fixed port naming and indentation

eccb093

tahmed-dev reviewed Jul 30, 2021

View reviewed changes

Added server listening to libevent loop

4ba13b9

tahmed-dev reviewed Aug 3, 2021

View reviewed changes

kellyyeh added 3 commits August 3, 2021 22:14

Added operation key to configInterface

429e752

Support multi vlans

6c4ed48

Added DHCPv6 Counter and addressed PR comments

3e9093d

tahmed-dev reviewed Aug 13, 2021

View reviewed changes

tahmed-dev reviewed Aug 16, 2021

View reviewed changes

src/dhcp6relay/src/main.cpp Outdated Show resolved Hide resolved

src/dhcp6relay/src/relay.cpp Show resolved Hide resolved

kellyyeh added 3 commits August 17, 2021 16:17

Addressed PR comments, removed arg_config

56dcedc

Updated counter to support multi-vlan

c339272

Updated update_counter

96664c5

tahmed-dev previously approved these changes Aug 17, 2021

View reviewed changes

Changed counter table name

308658f

kellyyeh dismissed tahmed-dev’s stale review via 308658f August 17, 2021 18:51

kellyyeh added 4 commits August 18, 2021 21:36

Use specified intf address instead of ipv6 auto selection for now

f61c7db

Updated counter table name

99f91c0

Updated option parsing

e345890

Updated option parsing

f7b7d4a

tahmed-dev previously approved these changes Aug 18, 2021

View reviewed changes

SuvarnaMeenakshi dismissed tahmed-dev’s stale review via f7b7d4a August 19, 2021 18:22

Updated relay.cpp

6a42ef2

tahmed-dev approved these changes Aug 20, 2021

View reviewed changes

kellyyeh merged commit f0e640f into sonic-net:master Aug 20, 2021

kellyyeh deleted the kellyyeh-dhcp6relay branch August 20, 2021 16:12

kellyyeh restored the kellyyeh-dhcp6relay branch August 20, 2021 17:28

kellyyeh added a commit to kellyyeh/sonic-buildimage that referenced this pull request Sep 30, 2021

Add DHCPv6 Relay Agent (sonic-net#8251)

0b2746a

* Added DHCPv6 Relay * Added DHCPv6 Counter (cherry picked from commit f0e640f)

kellyyeh mentioned this pull request Sep 30, 2021

Replace isc-dhcp with DHCPv6 Relay in dhcp_relay docker #8884

Merged

5 tasks

kellyyeh added the Request for 202106 Branch label Oct 12, 2021

kellyyeh deleted the kellyyeh-dhcp6relay branch October 13, 2021 23:46

yxieca pushed a commit to yxieca/sonic-buildimage that referenced this pull request Oct 25, 2021

Add DHCPv6 Relay Agent (sonic-net#8251)

802ce9b

* Added DHCPv6 Relay * Added DHCPv6 Counter

kellyyeh added a commit to kellyyeh/sonic-buildimage that referenced this pull request Oct 25, 2021

Add DHCPv6 Relay Agent (sonic-net#8251)

37ca8c5

* Added DHCPv6 Relay * Added DHCPv6 Counter

yxieca pushed a commit that referenced this pull request Oct 26, 2021

Add DHCPv6 Relay Agent (#8251)

aa8eecb

* Added DHCPv6 Relay * Added DHCPv6 Counter

		@@ -0,0 +1,7 @@
		#!/usr/bin/make -f

		DEB_CFLAGS_APPEND=-std=gnu11


		constexpr auto DEFAULT_TIMEOUT_MSEC = 1000;

		bool mPollSwssNotifcation = true;

Conversation

kellyyeh commented Jul 22, 2021

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

tahmed-dev left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants