[ovs-dev] [PATCH v2] ovn-northd: Add logical flows to support native DHCPv4

[Zong Kai LI]

>
> +        <pre>
> +eth.dst = eth.src;
> +eth.src = <var>E</var>;
> +ip4.dst = <var>O</var>;
> +ip4.src = <var>S</var>;
> +udp.src = 67;
> +udp.dst = 68;
> +outport = <var>P</var>;
> +inport = ""; /* Allow sending out inport. */
> +output;
> +        </pre>
> +
> +        <p>
> +          where <var>E</var> is the server MAC address and <var>S</var>
> is the
> +          server IPv4 address defined in the DHCPv4 options and
> <var>O</var> is
> +          the IPv4 address defined in the logical port's addresses column.
> +        </p>
> +
> +        <p>
> +          (This terminates packet processing; the packet does not go on
> the
> +          next ingress table.)
> +        </p>
>

Do you mean action "output;"? Yes, instead of going to the next ingress
table, packets will be sent to table OFTABLE_REMOTE_OUTPUT, but "terminates
packet processing" seems unclear or precise.

+      </li>
> +
> +      <li>
> +        A priority-0 flow that matches all packets to advances to table 8.
> +      </li>
> +    </ul>
> +
> +    <h3>Ingress Table 8: Destination Lookup</h3>
>
>      <p>
>        This table implements switching behavior.  It contains these logical
> @@ -387,6 +470,12 @@ output;
>        This is similar to ingress table 4 except for <code>to-lport</code>
> ACLs.
>      </p>
>
> +    <p>
> +      Also a priority 34000 logical flow is added for each logical port
> which
> +      has DHCPv4 options defined to allow the DHCPv4 reply packet from the
> +      <code>Ingress Table 7: DHCP response</code>.
> +    </p>
>
>
34000, a magic number ?


> +static bool
>  has_stateful_acl(struct ovn_datapath *od)
>  {
>      for (size_t i = 0; i < od->nbs->n_acls; i++) {
> @@ -1478,6 +1551,35 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows, struct hmap *ports)
>                            acl->match, "drop;");
>          }
>      }
> +
> +    /* Add 34000 priority flow to allow DHCP reply from ovn-controller to
> all
> +     * logical ports of the datapath if the CMS has configured DHCPv4
> options*/
> +    if (od->nbs && od->nbs->n_ports) {
> +        for (size_t i = 0; i < od->nbs->n_ports; i++) {
> +            if (od->nbs->ports[i]->dhcpv4_options) {
> +                const char *server_id = smap_get(
> +                    &od->nbs->ports[i]->dhcpv4_options->options,
> "server_id");
> +                const char *server_mac = smap_get(
> +                    &od->nbs->ports[i]->dhcpv4_options->options,
> "server_mac");
> +                const char *lease_time = smap_get(
> +                    &od->nbs->ports[i]->dhcpv4_options->options,
> "lease_time");
> +                const char *router = smap_get(
> +                    &od->nbs->ports[i]->dhcpv4_options->options,
> "router");
> +                if (server_id && server_mac && lease_time && router) {
>

Well, how about extract above code and write a new function to check if
dhcpv4_options are valid ?

 static void
> @@ -1635,7 +1737,71 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ARP_RSP, 0, "1", "next;");
>      }
>
> -    /* Ingress table 6: Destination lookup, broadcast and multicast
> handling
> +    /* Logical switch ingress table 6 and 7: DHCP options and response
> +         * priority 100 flows. */
> +    HMAP_FOR_EACH (op, key_node, ports) {
> +        if (!op->nbs) {
> +           continue;
> +        }
> +
> +        if (!lsp_is_enabled(op->nbs) || !strcmp(op->nbs->type, "router"))
> {
>

Why not put op->nbs->dhcpv4_options checking here?
And update to "... if the port is a router port or if dhcp is not enabled."

+            /* Don't add the DHCP flows if the port is not enabled or if
> the
> +             * port is a router port. */
> +            continue;
> +        }
> +
> +        for (size_t i = 0; i < op->nbs->n_addresses; i++) {
> +            struct lport_addresses laddrs;
> +            if (!extract_lsp_addresses(op->nbs->addresses[i], &laddrs,
> +                                       false)) {
> +                continue;
> +            }
> +
> +            for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
> +                struct ds options_action = DS_EMPTY_INITIALIZER;
> +                struct ds response_action = DS_EMPTY_INITIALIZER;
> +                if (build_dhcpv4_action(op, laddrs.ipv4_addrs[j].addr,
> +                                        &options_action,
> &response_action)) {
>

There are some dhcp_options relevant but not lsp address relevant checking
in build_dhcpv4_action.
So think about it, it's unnecessary to do duplicated check when lsp has
multiple addresses.

@@ -43,6 +43,11 @@
>                                             "max": "unlimited"}},
>                  "up": {"type": {"key": "boolean", "min": 0, "max": 1}},
>                  "enabled": {"type": {"key": "boolean", "min": 0, "max":
> 1}},
> +                "dhcpv4_options": {"type": {"key": {"type": "uuid",
> +                                            "refTable": "DHCP_Options",
> +                                            "refType": "strong"},
>

Great for the refTable.

That's all what I can see now.

Thanks.
Zong Kai, LI