sys/net/dhcpv6: Add IA_NA support to the DHCPv6 client by JKRhb · Pull Request #16228 · RIOT-OS/RIOT

JKRhb · 2021-03-24T19:28:09Z

Contribution description

This PR adds identity association for non-temporary addresses (IA_NA) to the DHCPv6 client and makes it possible to use the auto_init_dhcpv6_client module for auto configuring it.

While requesting a non-temporary address already works (see below), the implementation is probably not finished yet as I wasn't entirely sure how to correctly add the obtained address to the respective net interface and where to remove them if their lifetime passes. There also some other aspects which I wasn't quite sure about (the relevant lines on IA_NA in _nib-slaac.c for example) and code passages that probably need refactoring.

I would be very grateful to receive some feedback on this and hope that this PR is not too far away from a mergeable state.

Testing procedure

You can use the provided test under tests/gnrc_dhcpv6_client_auto_init to test the correct assignment of a non-temporary IPv6 address. You can run it by first using make all and then sudo make test-as-root.

For the test procedure, the existing test under tests/gnrc_dhcpv6_client_6lbr was adapted, which uses the Python library scapy.

miri64 · 2021-03-25T10:41:03Z

For a mesh you probably need a DHCPv6 Relay implementation first to have other nodes than the border router receive an address, right?

Other than that, I can have a look at the code soon™ (currently I have to do some other stuff first, so not much time sadly)

JKRhb · 2021-03-25T15:51:58Z

For a mesh you probably need a DHCPv6 Relay implementation first to have other nodes than the border router receive an address, right?

Oh, yes, very good point! I have to admit that so far I focused primarily on the use case of a "regular" device that requests an address via DHCPv6. But making it work for 6LoWPAN should of course be part of this PR eventually. The border router act as the relay agent in this scenario, right? Do you think the relay agent functionality should be a seperate module or could it be integrated into the existing DHCPv6 client?

Edit: Hmm, or should there be a seperate PR to implement the relay agent?

Other than that, I can have a look at the code soon™ (currently I have to do some other stuff first, so not much time sadly)

No worries, thank you already :) I will continue to work on this PR – hopefully the code will be a bit more refined once you have time to look at it.

miri64 · 2021-03-25T17:01:44Z

The border router act as the relay agent in this scenario, right?

The border router, yes, but also any router (6LR) between the border router and a leaf node when having a multihop network, as DHCPv6 is link-local only.

Do you think the relay agent functionality should be a seperate module or could it be integrated into the existing DHCPv6 client?

IMHO this should be a separate module (that should reuse client-like behavior from the client) and a separate PR for that matter, otherwise the focus of this PR can easily get out of hands ;-).

sys/include/net/dhcpv6/client.h

JKRhb · 2021-05-02T18:38:10Z

@miri64 Making some small additions and updating the tests, I resumed working on this PR again now :) Maybe you could have a quick look to assess how much there is missing for this PR to be merged?

Will do. However, I don't have much time this week. So for now, I dismiss my change request, as the comment was addressed.

JKRhb · 2021-06-07T12:19:00Z

I think I managed to make the implementation more stack-independent now. I also added the dhcpv6_client_req_ia_na function to the test under gnrc_dhcpv6_client to be able to test both the auto initialization and the "manual" case.

There might be still room for improvement, however. Should I maybe highlight the passages I am a bit uncertain about to make reviewing them a bit easier?

JKRhb · 2021-06-21T15:14:52Z

@miri64 Sorry to bother you, but can you estimate when you might have the time for a review? Should I maybe highlight the probably most critical portions as suggested in my last comment?

miri64 · 2021-06-22T12:12:21Z

Should I maybe highlight the probably most critical portions as suggested in my last comment?

That would be great. Probably next week at the latest I will finally have some time to have a look!

miri64

Some initial review.

sys/include/net/dhcpv6/client.h

sys/include/net/gnrc/netif/conf.h

sys/net/gnrc/network_layer/ipv6/nib/_nib-slaac.c

tests/gnrc_dhcpv6_client_6lbr/tests-as-root/01-run.py

tests/gnrc_dhcpv6_client_auto_init/README.md

tests/gnrc_dhcpv6_client_ia_na/tests-as-root/01-run.py

sys/include/net/gnrc/netif/flags.h

miri64

Murdock had a complaint. Please squash and ~~rebase (to resolve merge conflicts due to the merging of #16606)~~ [edit: to my surprise there were none] immediately.

sys/include/net/dhcpv6/client.h

miri64 · 2021-08-11T15:27:07Z

Please squash ;-).

miri64 · 2021-08-11T15:28:20Z

tests: Add test for DHCPv6 IA_NA and auto_init

While you are at it, I think the commit message for the test needs an update ;-)

JKRhb · 2021-08-11T15:28:25Z

Please squash ;-).

Ehem, done ;)

JKRhb · 2021-08-11T15:30:17Z

tests: Add test for DHCPv6 IA_NA and auto_init

While you are at it, I think the commit message for the test needs an update ;-)

Done :) I hope the new commit message is okay

miri64 · 2021-08-11T15:32:23Z

Done :) I hope the new commit message is okay

sys/include/net/dhcpv6/client.h

sys/net/application_layer/dhcpv6/client.c

JKRhb · 2021-08-11T16:05:16Z

Running the test for gnrc_dhcpv6_client_6lbr locally I am afraid there is another error incoming:

ISO C forbids zero-size array ‘addr_leases’

I think it might be the easiest to just add another definition like CONFIG_DHCPV6_CLIENT_ADDRS that is used for calculating CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF. I will make such a suggestion in another fixup commit. Sorry that this PR is causing so much hassle.

JKRhb · 2021-08-11T16:30:32Z

I guess with the latest addition the tests should pass now. Let me know if there is anything that should be changed.

sys/net/gnrc/netif/Kconfig

sys/include/net/dhcpv6/client.h

miri64 · 2021-08-11T17:05:26Z

I think it might be the easiest to just add another definition like CONFIG_DHCPV6_CLIENT_ADDRS that is used for calculating CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF. I will make such a suggestion in another fixup commit. Sorry that this PR is causing so much hassle.

With my latest proposals, the situation should be less confusing for a user.

miri64 · 2021-08-11T17:13:38Z

Don't forget about #16228 (comment) ;-)

sys/include/net/gnrc/netif/conf.h

JKRhb · 2021-08-11T17:14:49Z

Don't forget about #16228 (comment) ;-)

Ahem, should be fixed now :)

miri64 · 2021-08-11T18:49:54Z

Here. We. Go. 🎉

JKRhb · 2021-08-11T19:09:38Z

Here. We. Go. 🎉

Yay! \o/ Thank you so much for help and feedback along the way :)

JKRhb requested review from PeterKietzmann, cgundogan, haukepetersen, jia200x, leandrolanzieri and miri64 as code owners March 24, 2021 19:28

miri64 added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Mar 25, 2021

miri64 reviewed Mar 30, 2021

View reviewed changes

sys/include/net/dhcpv6/client.h Show resolved Hide resolved

miri64 previously requested changes Mar 30, 2021

View reviewed changes

sys/include/net/dhcpv6/client.h Outdated Show resolved Hide resolved

JKRhb force-pushed the dhcp-ia-na branch from eb4e3f4 to 549f268 Compare May 1, 2021 09:05

github-actions bot added Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework labels Jun 7, 2021

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

miri64 requested changes Jun 28, 2021

View reviewed changes

JKRhb mentioned this pull request Jun 28, 2021

tests/gnrc_dhcpv6_client_6lbr: Update test for DHCPv6 MUD option #16593

Merged

JKRhb force-pushed the dhcp-ia-na branch from da68fa0 to 3f27ae5 Compare June 28, 2021 15:59

miri64 reviewed Jun 29, 2021

View reviewed changes

tests/gnrc_dhcpv6_client_ia_na/tests-as-root/01-run.py Outdated Show resolved Hide resolved

miri64 reviewed Jun 29, 2021

View reviewed changes

sys/include/net/gnrc/netif/flags.h Outdated Show resolved Hide resolved

miri64 requested changes Aug 11, 2021

View reviewed changes

sys/include/net/dhcpv6/client.h Outdated Show resolved Hide resolved

JKRhb force-pushed the dhcp-ia-na branch from d9abbbd to c14b877 Compare August 11, 2021 15:25

JKRhb force-pushed the dhcp-ia-na branch from c14b877 to 9977de6 Compare August 11, 2021 15:28

miri64 approved these changes Aug 11, 2021

View reviewed changes

JKRhb force-pushed the dhcp-ia-na branch from 9977de6 to f202b80 Compare August 11, 2021 15:29

miri64 reviewed Aug 11, 2021

View reviewed changes

JKRhb force-pushed the dhcp-ia-na branch from f202b80 to 6f098b7 Compare August 11, 2021 15:44

miri64 reviewed Aug 11, 2021

View reviewed changes

sys/net/gnrc/netif/Kconfig Outdated Show resolved Hide resolved

miri64 reviewed Aug 11, 2021

View reviewed changes

sys/include/net/dhcpv6/client.h Outdated Show resolved Hide resolved

JKRhb force-pushed the dhcp-ia-na branch from 1f5b132 to b4cbe39 Compare August 11, 2021 17:09

JKRhb force-pushed the dhcp-ia-na branch from b4cbe39 to e3bb354 Compare August 11, 2021 17:13

miri64 reviewed Aug 11, 2021

View reviewed changes

sys/include/net/gnrc/netif/conf.h Outdated Show resolved Hide resolved

JKRhb added 2 commits August 11, 2021 19:15

sys/net/dhcpv6: Add IA_NA implementation

234a718

tests/gnrc_dhcpv6_client: Add test for IA_NA

85eae9d

JKRhb force-pushed the dhcp-ia-na branch from e3bb354 to 85eae9d Compare August 11, 2021 17:15

miri64 merged commit f2f6700 into RIOT-OS:master Aug 11, 2021

JKRhb deleted the dhcp-ia-na branch August 11, 2021 19:08

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

Conversation

JKRhb commented Mar 24, 2021

Contribution description

Testing procedure

Uh oh!

miri64 commented Mar 25, 2021

Uh oh!

JKRhb commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

JKRhb commented May 2, 2021

Uh oh!

JKRhb commented Jun 7, 2021

Uh oh!

JKRhb commented Jun 21, 2021

Uh oh!

miri64 commented Jun 22, 2021

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miri64 commented Aug 11, 2021

Uh oh!

miri64 commented Aug 11, 2021

Uh oh!

JKRhb commented Aug 11, 2021

Uh oh!

JKRhb commented Aug 11, 2021

Uh oh!

miri64 commented Aug 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JKRhb commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JKRhb commented Aug 11, 2021

Uh oh!

Uh oh!

Uh oh!

miri64 commented Aug 11, 2021

Uh oh!

miri64 commented Aug 11, 2021

Uh oh!

Uh oh!

JKRhb commented Aug 11, 2021

Uh oh!

miri64 commented Aug 11, 2021

Uh oh!

JKRhb commented Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

JKRhb commented Mar 25, 2021 •

edited

Loading

miri64 left a comment •

edited

Loading

JKRhb commented Aug 11, 2021 •

edited

Loading