| Message ID | cover.1573116139.git.lorenzo.bianconi@redhat.com |
|---|---|
| Headers | show |
| Series | Add IPv6 Prefix delegation (RFC3633) | expand |
On Thu, Nov 7, 2019 at 2:18 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > Introduce IPv6 Prefix delegation state machine according to RFC 3633 > https://tools.ietf.org/html/rfc3633. > Add dhcp6_server_pkt controller action to parse advertise/reply from > IPv6 delegation server. > Introduce logical flows in ovn router pipeline in order to parse dhcpv6 > advertise/reply from IPv6 prefix delegation router. > This series relies on the following OVS commit: > https://github.com/openvswitch/ovs/commit/cec89046f72cb044b068ba6a4e30dbcc4292c4c1 > > Lorenzo Bianconi (2): > controller: add ipv6 prefix delegation state machine > northd: add logical flows for dhcpv6 pfd parsing Hi Lorenzo, I did a higher level review. I have few comments (1) - I woukd suggest to change the name of new action from dhcp6_server_pkt to may be "handle_dhcpv6_reply". (2) Let's say you have the below logical network topology ********** switch 41afb0dc-fea4-4464-831a-879dfcde7999 (sw1) port sw1-lr0 type: router router-port: lr0-sw1 port sw1-port1 addresses: ["40:54:00:00:00:03"] switch f826550f-4340-43b0-ac82-4baac6a0b668 (public) port public-lr0 type: router router-port: lr0-public switch 346d731f-1d28-4c94-a96c-d456eb9a489e (sw0) port sw0-lr0 type: router router-port: lr0-sw0 port sw0-port1 addresses: ["50:54:00:00:00:03"] router 40525d05-09d7-4d9c-9194-31d0f2afcfd6 (lr0) port lr0-public mac: "00:00:20:20:12:13" networks: ["172.168.0.100/24"] port lr0-sw0 mac: "00:00:00:00:ff:01" networks: ["10.0.0.1/24"] port lr0-sw1 mac: "00:00:00:00:ff:02" networks: ["20.0.0.1/24"] ***** If I understand correctly, this patch expects that "prefix_delegation=true" is set in options columns of lr0-public right ? So how would sw0 and sw1 logical switches get their prefixes ? The main goal of prefix delegation is that the private networks connected to a logical router should get their prefixes from an upstream prefix delegation server. I don't see that addressed in this patch series. I think we should allow CMS to set "prefix_delegation=true" in sw0-lr0 and sw1-lr0's options so that ovn-controller will send IPv6 prefix delegation request for each of these logical router ports. ovn-controller where cr-lr0-public is resident should send out these packets via the lr0-public port and it can allocate unique IAID for each logical router port - i.e lr0-sw0 and lr0-sw1. This way we can distinguish to which logical port the prefix delegation packet belongs to. (3) Prefix delegation feature is used if CMS is not sure what IPv6 prefix to use for each of the logical switches. When adding a logical router port to a router, we won't be knowing the network prefix. So we should allow something like - ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 <dynamic> and then once we receive the prefix from the PD server we can use that in router flows. This patch series should handle this use case as well. (4) Patch 1 needs to add documentation for the new action (5) Test cases should be added - both unit and system tests. (6) In my sandbox when I ran "ovn-nbctl set logical_router_port lr0-public options:prefix_delegation=true" I see the below logical flows for dhcp6 handling *** ovn-sbctl dump-flows lr0 | grep dhcp table=3 (lr_in_ip_input ), priority=100 , match=(inport == "cr-lr0-public" && ip6.dst == fe80::200:20ff:fe20:1213 && udp.src == 547 && udp.dst == 546), action=(reg0 = 0; dhcp6_server_pkt { eth.dst <-> eth.src; ip6.dst <-> ip6.src; outport <-> inport; output; };) table=3 (lr_in_ip_input ), priority=100 , match=(inport == "lr0-public" && ip6.dst == fe80::200:20ff:fe20:1213 && udp.src == 547 && udp.dst == 546), action=(reg0 = 0; dhcp6_server_pkt { eth.dst <-> eth.src; ip6.dst <-> ip6.src; outport <-> inport; output; };) *** I think it should not add flows for cr-lr0-public. Thanks Numan > > controller/pinctrl.c | 553 ++++++++++++++++++++++++++++++++++++++++++ > include/ovn/actions.h | 9 +- > lib/actions.c | 22 ++ > lib/ovn-l7.h | 19 ++ > northd/ovn-northd.c | 66 ++++- > ovn-nb.xml | 10 + > tests/ovn.at | 6 + > utilities/ovn-trace.c | 2 + > 8 files changed, 685 insertions(+), 2 deletions(-) > > -- > 2.21.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> On Thu, Nov 7, 2019 at 2:18 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > Introduce IPv6 Prefix delegation state machine according to RFC 3633 > > https://tools.ietf.org/html/rfc3633. > > Add dhcp6_server_pkt controller action to parse advertise/reply from > > IPv6 delegation server. > > Introduce logical flows in ovn router pipeline in order to parse dhcpv6 > > advertise/reply from IPv6 prefix delegation router. > > This series relies on the following OVS commit: > > https://github.com/openvswitch/ovs/commit/cec89046f72cb044b068ba6a4e30dbcc4292c4c1 > > > > Lorenzo Bianconi (2): > > controller: add ipv6 prefix delegation state machine > > northd: add logical flows for dhcpv6 pfd parsing > > Hi Lorenzo, > > I did a higher level review. I have few comments Hi Numan, thx for the review > > (1) - I woukd suggest to change the name of new action from > dhcp6_server_pkt to may be "handle_dhcpv6_reply". ack, will do in v2 > > (2) Let's say you have the below logical network topology > > ********** > switch 41afb0dc-fea4-4464-831a-879dfcde7999 (sw1) > port sw1-lr0 > type: router > router-port: lr0-sw1 > port sw1-port1 > addresses: ["40:54:00:00:00:03"] > switch f826550f-4340-43b0-ac82-4baac6a0b668 (public) > port public-lr0 > type: router > router-port: lr0-public > switch 346d731f-1d28-4c94-a96c-d456eb9a489e (sw0) > port sw0-lr0 > type: router > router-port: lr0-sw0 > port sw0-port1 > addresses: ["50:54:00:00:00:03"] > router 40525d05-09d7-4d9c-9194-31d0f2afcfd6 (lr0) > port lr0-public > mac: "00:00:20:20:12:13" > networks: ["172.168.0.100/24"] > port lr0-sw0 > mac: "00:00:00:00:ff:01" > networks: ["10.0.0.1/24"] > port lr0-sw1 > mac: "00:00:00:00:ff:02" > networks: ["20.0.0.1/24"] > > ***** > > If I understand correctly, this patch expects that > "prefix_delegation=true" is set in options columns of lr0-public right > ? > So how would sw0 and sw1 logical switches get their prefixes ? > > The main goal of prefix delegation is that the private networks > connected to a logical router should get their prefixes > from an upstream prefix delegation server. I don't see that addressed > in this patch series. According to the RFC3633: "The requesting router is then responsible for the delegated prefix(es). For example, the requesting router might assign a subnet from a delegated prefix to one of its interfaces, and begin sending router advertisements for the prefix on that link" In the current implementation the requesting router (OVN) will get a single IPv6 prefix and let the CMS divide the prefix among the downstream interfaces in order to send this info in Router Advertisement. I though this approach is the most configurable and moreover it is more scalable since reduces the number of states/network traffic. (e.g what if the logical router has 10000 logical router port?) > > I think we should allow CMS to set "prefix_delegation=true" in > sw0-lr0 and sw1-lr0's options so > that ovn-controller will send IPv6 prefix delegation request for each > of these logical router ports. > > ovn-controller where cr-lr0-public is resident should send out these > packets via the lr0-public port and it can > allocate unique IAID for each logical router port - i.e lr0-sw0 and > lr0-sw1. This way we can distinguish > to which logical port the prefix delegation packet belongs to. > > (3) Prefix delegation feature is used if CMS is not sure what IPv6 > prefix to use for each of the logical switches. > When adding a logical router port to a router, we won't be knowing the > network prefix. So we should allow something > like - ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 <dynamic> and > then once we receive the prefix from the PD server > we can use that in router flows. This patch series should handle this > use case as well. > > > (4) Patch 1 needs to add documentation for the new action ack will do in v2 > > (5) Test cases should be added - both unit and system tests. ack will do in v2 > > (6) In my sandbox when I ran "ovn-nbctl set logical_router_port > lr0-public options:prefix_delegation=true" I see the below logical > flows for dhcp6 handling > > *** > ovn-sbctl dump-flows lr0 | grep dhcp > table=3 (lr_in_ip_input ), priority=100 , match=(inport == > "cr-lr0-public" && ip6.dst == fe80::200:20ff:fe20:1213 && udp.src == > 547 && udp.dst == 546), action=(reg0 = 0; dhcp6_server_pkt { eth.dst > <-> eth.src; ip6.dst <-> ip6.src; outport <-> inport; output; };) > table=3 (lr_in_ip_input ), priority=100 , match=(inport == > "lr0-public" && ip6.dst == fe80::200:20ff:fe20:1213 && udp.src == 547 > && udp.dst == 546), action=(reg0 = 0; dhcp6_server_pkt { eth.dst <-> > eth.src; ip6.dst <-> ip6.src; outport <-> inport; output; };) > *** > > I think it should not add flows for cr-lr0-public. ack will fix in v2 Regards, Lorenzo > > > Thanks > Numan > > > > > controller/pinctrl.c | 553 ++++++++++++++++++++++++++++++++++++++++++ > > include/ovn/actions.h | 9 +- > > lib/actions.c | 22 ++ > > lib/ovn-l7.h | 19 ++ > > northd/ovn-northd.c | 66 ++++- > > ovn-nb.xml | 10 + > > tests/ovn.at | 6 + > > utilities/ovn-trace.c | 2 + > > 8 files changed, 685 insertions(+), 2 deletions(-) > > > > -- > > 2.21.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >