Skip to content

gnrc_dhcpv6_client_6lbr: choose downstream if as !upstream#16530

Merged
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
benpicco:gnrc_dhcpv6_client_6lbr-generic
Jun 17, 2021
Merged

gnrc_dhcpv6_client_6lbr: choose downstream if as !upstream#16530
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
benpicco:gnrc_dhcpv6_client_6lbr-generic

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Jun 6, 2021

Contribution description

The gnrc_dhcpv6_client_6lbr code only considers 6LoWPAN interfaces possible downstream interface candidates.
This needlessly excludes the use of DHCPv6 for non-6lo downstream networks.

Chose all non-upstream interfaces as downstream interfaces for DHCPv6 to allow using it together with e.g. SLIP, DOSE or ethos.

Testing procedure

I tested this on same54-xpro which has an Ethernet interface that is used as uplink and which has a downlink device connected via SLIP.
For this to work with SLIP a second patch is needed to simulate a L2 address that is then used as an IPv6 IID.

I ran this with examples/gnrc_networking with the following modules added:

USEMODULE += ethos
USEMODULE += gnrc_dhcpv6_client_6lbr
CFLAGS += -DETHOS_UART=UART_DEV\(1\)
CFLAGS += -DETHOS_BAUDRATE=115200
CFLAGS += -DCONFIG_GNRC_DHCPV6_CLIENT_6LBR_UPSTREAM=6

master

No prefix for the non-6lo downstream interface

2021-06-06 23:56:51,753 # Iface  6  HWaddr: FC:C2:3D:0D:2D:1F 
2021-06-06 23:56:51,757 #           L2-PDU:1500  MTU:1492  HL:255  RTR  
2021-06-06 23:56:51,760 #           Source address length: 6
2021-06-06 23:56:51,763 #           Link type: wired
2021-06-06 23:56:51,769 #           inet6 addr: fe80::fec2:3dff:fe0d:2d1f  scope: link  VAL
2021-06-06 23:56:51,776 #           inet6 addr: 2001:16b8:45fb:1d00:fec2:3dff:fe0d:2d1f  scope: global  VAL
2021-06-06 23:56:51,779 #           inet6 group: ff02::2
2021-06-06 23:56:51,781 #           inet6 group: ff02::1
2021-06-06 23:56:51,785 #           inet6 group: ff02::1:ff0d:2d1f
2021-06-06 23:56:51,786 #           
2021-06-06 23:56:51,789 #           Statistics for Layer 2
2021-06-06 23:56:51,792 #             RX packets 43  bytes 10838
2021-06-06 23:56:51,796 #             TX packets 9 (Multicast: 9)  bytes 762
2021-06-06 23:56:51,800 #             TX succeeded 9 errors 0
2021-06-06 23:56:51,802 #           Statistics for IPv6
2021-06-06 23:56:51,806 #             RX packets 16  bytes 4722
2021-06-06 23:56:51,810 #             TX packets 9 (Multicast: 9)  bytes 636
2021-06-06 23:56:51,813 #             TX succeeded 9 errors 0
2021-06-06 23:56:51,813 # 
2021-06-06 23:56:51,817 # Iface  5  HWaddr: A6:ED:29:EC:C7:F3 
2021-06-06 23:56:51,821 #           L2-PDU:1500  MTU:1500  HL:64  RTR  
2021-06-06 23:56:51,823 #           RTR_ADV  
2021-06-06 23:56:51,826 #           Source address length: 6
2021-06-06 23:56:51,828 #           Link type: wired
2021-06-06 23:56:51,834 #           inet6 addr: fe80::a4ed:29ff:feec:c7f3  scope: link  VAL
2021-06-06 23:56:51,836 #           inet6 group: ff02::2
2021-06-06 23:56:51,839 #           inet6 group: ff02::1
2021-06-06 23:56:51,843 #           inet6 group: ff02::1:ffec:c7f3
2021-06-06 23:56:51,844 #           
2021-06-06 23:56:51,847 #           Statistics for Layer 2
2021-06-06 23:56:51,850 #             RX packets 0  bytes 0
2021-06-06 23:56:51,854 #             TX packets 1 (Multicast: 1)  bytes 78
2021-06-06 23:56:51,857 #             TX succeeded 0 errors 0
2021-06-06 23:56:51,860 #           Statistics for IPv6
2021-06-06 23:56:51,863 #             RX packets 0  bytes 0
2021-06-06 23:56:51,867 #             TX packets 1 (Multicast: 1)  bytes 64
2021-06-06 23:56:51,870 #             TX succeeded 1 errors 0

this PR

The downstream interface gets a prefix

2021-06-06 23:55:40,042 # Iface  6  HWaddr: FC:C2:3D:0D:2D:1F 
2021-06-06 23:55:40,046 #           L2-PDU:1500  MTU:1492  HL:255  RTR  
2021-06-06 23:55:40,050 #           Source address length: 6
2021-06-06 23:55:40,052 #           Link type: wired
2021-06-06 23:55:40,058 #           inet6 addr: fe80::fec2:3dff:fe0d:2d1f  scope: link  VAL
2021-06-06 23:55:40,065 #           inet6 addr: 2001:16b8:45fb:1d00:fec2:3dff:fe0d:2d1f  scope: global  VAL
2021-06-06 23:55:40,068 #           inet6 group: ff02::2
2021-06-06 23:55:40,070 #           inet6 group: ff02::1
2021-06-06 23:55:40,074 #           inet6 group: ff02::1:ff0d:2d1f
2021-06-06 23:55:40,075 #           
2021-06-06 23:55:40,078 #           Statistics for Layer 2
2021-06-06 23:55:40,081 #             RX packets 31  bytes 6684
2021-06-06 23:55:40,085 #             TX packets 8 (Multicast: 8)  bytes 734
2021-06-06 23:55:40,089 #             TX succeeded 8 errors 0
2021-06-06 23:55:40,091 #           Statistics for IPv6
2021-06-06 23:55:40,095 #             RX packets 16  bytes 3431
2021-06-06 23:55:40,099 #             TX packets 8 (Multicast: 8)  bytes 622
2021-06-06 23:55:40,102 #             TX succeeded 8 errors 0
2021-06-06 23:55:40,102 # 
2021-06-06 23:55:40,106 # Iface  5  HWaddr: A6:ED:29:EC:C7:F3 
2021-06-06 23:55:40,110 #           L2-PDU:1500  MTU:1500  HL:64  RTR  
2021-06-06 23:55:40,112 #           RTR_ADV  
2021-06-06 23:55:40,115 #           Source address length: 6
2021-06-06 23:55:40,117 #           Link type: wired
2021-06-06 23:55:40,123 #           inet6 addr: fe80::a4ed:29ff:feec:c7f3  scope: link  VAL
2021-06-06 23:55:40,130 #           inet6 addr: 2001:16b8:45fb:1df8:a4ed:29ff:feec:c7f3  scope: global  VAL
2021-06-06 23:55:40,133 #           inet6 group: ff02::2
2021-06-06 23:55:40,135 #           inet6 group: ff02::1
2021-06-06 23:55:40,139 #           inet6 group: ff02::1:ffec:c7f3
2021-06-06 23:55:40,142 #           inet6 group: ff02::1a
2021-06-06 23:55:40,143 #           
2021-06-06 23:55:40,146 #           Statistics for Layer 2
2021-06-06 23:55:40,149 #             RX packets 0  bytes 0
2021-06-06 23:55:40,153 #             TX packets 9 (Multicast: 9)  bytes 948
2021-06-06 23:55:40,156 #             TX succeeded 0 errors 0
2021-06-06 23:55:40,159 #           Statistics for IPv6
2021-06-06 23:55:40,162 #             RX packets 0  bytes 0
2021-06-06 23:55:40,166 #             TX packets 9 (Multicast: 9)  bytes 822
2021-06-06 23:55:40,169 #             TX succeeded 9 errors 0

Issues/PRs references

@benpicco benpicco added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jun 6, 2021
@github-actions github-actions bot added the Area: sys Area: System label Jun 6, 2021
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 6, 2021
@jeandudey jeandudey added this to the Release 2021.07 milestone Jun 7, 2021
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, trusting @benpicco testing.

Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Mhh, so the gnrc_dhcpv6_client_6lbr module now serves also non-6LoWPAN interfaces? Wouldn't it make much more sense to provide a new submodule for this?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

E.g. gnrc_dhcpv6_client_greedy.

@benpicco
Copy link
Copy Markdown
Contributor Author

I agree with renaming the module, but I wonder if this should not be the default behavior.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

I agree with renaming the module, but I wonder if this should not be the default behavior.

That's not what I mean. The gnrc_dhcpv6_client implementation is the de facto DHCPv6 client implementation for GNRC. gnrc_dhcpv6_client_6lbr is a convenience module that allows initialization of a DHCPv6 client tailored to a 6LoWPAN border router. So if you want to have a client that does something different (that is reusable), e.g. configure prefix delegation for all interfaces but the one the client is running on, it should go into its own module.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

See e.g. tests/gnrc_dhcpv6_client. It uses the DHCPv6 client without using gnrc_dhcpv6_client_6lbr, because the DHCPv6 client was designed to be able to work as such. Up until now there just wasn't another use case except 6LoWPAN border router. What you doing is a new use case, so there should be a new module.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

(maybe there is potential for cross polination between those new modules, e.g. the runner thread most likely will stay the same).

@benpicco
Copy link
Copy Markdown
Contributor Author

So how about

static void _configure_dhcpv6_client(void)
{
    gnrc_netif_t *netif = NULL;
    gnrc_netif_t *upstream = _find_upstream_netif();
    while ((netif = gnrc_netif_iter(netif))) {
        if (IS_USED(MODULE_GNRC_DHCPV6_CLIENT_6LBR)
            && !gnrc_netif_is_6lo(netif)) {
            continue;
        }
        if (netif != upstream) {
            dhcpv6_client_req_ia_pd(netif->pid, 64U);
        }
    }
}

and renaming gnrc_dhcpv6_client_6lbr -> gnrc_dhcpv6_client_greedy with gnrc_dhcpv6_client_6lbr remaining as a sub-module that pulls in gnrc_dhcpv6_client_greedy (or gnrc_dhcpv6_client_router)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

and renaming gnrc_dhcpv6_client_6lbr -> gnrc_dhcpv6_client_greedy with gnrc_dhcpv6_client_6lbr remaining as a sub-module that pulls in gnrc_dhcpv6_client_greedy (or gnrc_dhcpv6_client_router)

Mhhh from the naming this might be misleading. The 6LBR sub-module is all but greedy. How about renaming it gnrc_dhcpv6_client_simple64pd (for "simple 64-bit prefix delegation") and do:

static void _configure_dhcpv6_client(void)
{
    gnrc_netif_t *netif = NULL;
    gnrc_netif_t *upstream = (IS_USED(MODULE_GNRC_DHCPV6_CLIENT_GREEDY))
                                         ? _find_upstream_netif() : NULL;
    while ((netif = gnrc_netif_iter(netif))) {
        if (IS_USED(MODULE_GNRC_DHCPV6_CLIENT_6LBR)
            && !gnrc_netif_is_6lo(netif)) {
            continue;
        }
        if (IS_USED(MODULE_GNRC_DHCPV6_CLIENT_GREEDY) &&
            (netif == upstream)) {
            continue;
        }
        dhcpv6_client_req_ia_pd(netif->pid, 64U);
    }
}

and then make both sub-modules 6lbr and greedy dependent on simple64pd.

@benpicco
Copy link
Copy Markdown
Contributor Author

Just wondering, why '64-bit prefix delegation' - it will happily delegate delegate larger prefixes than can be further sub-divided by downstream nodes.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

Just wondering, why '64-bit prefix delegation' - it will happily delegate delegate larger prefixes than can be further sub-divided by downstream nodes.

Because of this line

        dhcpv6_client_req_ia_pd(netif->pid, 64U);

If we want something more configurable, we could do something like that (maybe with Kconfig) but then again, why not do interface-wise config then (and allow defining the interfaces instead of just taking all) ;-).

@benpicco
Copy link
Copy Markdown
Contributor Author

Huh interesting, looks like all the servers I tested with ignored that request and handed out a larger prefix.
So should we name it gnrc_dhcpv6_client_simple_pd so we don't need to rename it again when this becomes configurable 😉

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

So should we name it gnrc_dhcpv6_client_simple_pd so we don't need to rename it again when this becomes configurable 😉

Well the idea I had was that maybe having such config convenience modules won't be necessary when real config comes 😉

Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

You forgot to define the greedy sub-module ;-).

Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK, now that the architectural problems are out of the way, I trust your testing as well :-).

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 15, 2021

Please squash.

@benpicco benpicco force-pushed the gnrc_dhcpv6_client_6lbr-generic branch from a8d6864 to 056dcd8 Compare June 15, 2021 13:35
@fjmolinas fjmolinas merged commit a20790b into RIOT-OS:master Jun 17, 2021
@benpicco benpicco deleted the gnrc_dhcpv6_client_6lbr-generic branch June 17, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants