[ovs-dev] [PATCH 1/2] ovn-controller: Add 'dns_lkup' action

[Ben Pfaff]

On Fri, Feb 10, 2017 at 08:02:15PM +0530, nusiddiq at redhat.com wrote:
> From: Numan Siddique <nusiddiq at redhat.com>
> 
> This patch adds a new OVN action 'dns_lkup' to support native DNS.
> ovn-controller parses this action and adds a NXT_PACKET_IN2
> OF flow with 'pause' flag set.
> 
> A new table 'DNS' is added in the SB DB to look up and resolve
> the DNS queries. When a valid DNS packet is received by
> ovn-controller, it looks up the DNS name in the 'DNS' table
> and if successful, it frames a DNS reply, resumes the packet
> and stores 1 in the 1-bit subfield. If the packet is invalid
> or cannot be resolved, it resumes the packet without any
> modifications and stores 0 in the 1-bit subfield.
> 
> reg0[4] = dns_lkup(); next;
> 
> An upcoming patch will use this action and adds logical flows.
> 
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>

Thank you for working on this!

Would you mind spelling out "lookup"?  I don't know of a reason to
abbreviate to "lkup" here.  It's always a little easier to read a full
word.

I understand why ovn-trace can't do a proper lookup.  I think that it
should at least set the destination bit to 0 and put a message into the
trace about it.

ovn-sb.xml should document the new action.

The documentation for the DNS table in ovn-sb.xml talks about VIFs.  I
don't think that name resolution is limited to names for VIFs or even
specialized for VIFs.

I don't think that struct dns_header needs to be marked "packed",
because I don't see anything in the struct that the compiler would pad.

In the DNS table, I wonder whether there is an advantage to the proposed
approach of having separate columns for IPv4 and IPv6 addresses.  It
might be more user friendly to just have a single column that can
contain both kinds of addresses.

In the DNS table, I wonder about the "hostname" and "fqdn" columns.  I
thought that DNS servers always worked in terms of FQDN, and that it was
the DNS clients that were responsible for transforming a hostname to an
FQDN (by appending their own domain name).

I guess that, if "hostname" and "fqdn" both make sense, then there
should be a database index on each of them, like this, in
ovn-sb.ovsschema:

            "indexes": [["hostname", "datapath"], ["fqdn", "datapath"]],

I don't see anything that validates that dns_lkup() is called on a UDP
packet.  One way to do that would be to make udp a prereq for
dns_lkup(), e.g.:

@@ -1727,6 +1727,7 @@ parse_dns_lkup(struct action_context *ctx, const struct expr_field *dst,
         return;
     }
     dl->dst = *dst;
+    add_prerequisite(ctx, "udp");
 }
 
 static void

This patch causes some warnings from Clang:

    ../ovn/controller/pinctrl.c:741:35: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'ovs_be16 *' (aka 'unsigned short *') increases required alignment from 1 to 2 [-Werror,-Wcast-align]
    /usr/include/netinet/in.h:403:33: note: expanded from macro 'ntohs'
    /usr/include/i386-linux-gnu/bits/byteswap-16.h:27:62: note: expanded from macro '__bswap_16'

I think that the cast in question is OK, so I would change it to an
ALIGNED_CAST to suppress the warning.

In pinctrl_handle_dns_lkup() here, I would drop the ntohs() for checking
that a value is nonzero:

    if (!ntohs(in_dns_header->qdcount)) {

Here, I think that for safety the operands of && should be in the
opposite order:

    while (in_dns_data[idx] && (in_dns_data + idx) < end) {

I don't see anything here that checks for reading beyond the end of the
data:

        uint8_t label_len = in_dns_data[idx++];
        for (uint8_t i = 0; i < label_len; i++) {
            ds_put_char(&hostname, in_dns_data[idx++]);
        }

I think it would be good to declare macros or enums for query types, or
at least for the supported A, AAAA, and ANY query types.

Thanks,

Ben.