[ovs-dev] [PATCH 1/2] ovn-controller: Add 'dns_lkup' action
From: Numan Siddique
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
---
include/ovn/actions.h | 17 +++-
ovn/controller/pinctrl.c | 243 +-
ovn/lib/actions.c | 51 ++
ovn/lib/ovn-util.h| 14 +++
ovn/ovn-sb.ovsschema | 22 -
ovn/ovn-sb.xml| 34 +++
ovn/utilities/ovn-sbctl.c | 3 +
ovn/utilities/ovn-trace.c | 3 +
tests/ovn.at | 6 ++
9 files changed, 385 insertions(+), 8 deletions(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index d2510fd..789bed1 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -70,7 +70,8 @@ struct simap;
OVNACT(PUT_ND,ovnact_put_mac_bind) \
OVNACT(PUT_DHCPV4_OPTS, ovnact_put_dhcp_opts) \
OVNACT(PUT_DHCPV6_OPTS, ovnact_put_dhcp_opts) \
-OVNACT(SET_QUEUE, ovnact_set_queue)
+OVNACT(SET_QUEUE, ovnact_set_queue) \
+OVNACT(DNS_LKUP,ovnact_dns_lkup)
/* enum ovnact_type, with a member OVNACT_ for each action. */
enum OVS_PACKED_ENUM ovnact_type {
@@ -258,6 +259,12 @@ struct ovnact_set_queue {
uint16_t queue_id;
};
+/* OVNACT_DNS_LKUP. */
+struct ovnact_dns_lkup {
+struct ovnact ovnact;
+struct expr_field dst; /* 1-bit destination field. */
+};
+
/* Internal use by the helpers below. */
void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
@@ -385,6 +392,14 @@ enum action_opcode {
* - Any number of DHCPv6 options.
*/
ACTION_OPCODE_PUT_DHCPV6_OPTS,
+
+/* "result = dns_lkup()".
+ * Arguments follow the action_header, in this format:
+ * - A 32-bit or 64-bit OXM header designating the result field.
+ * - A 32-bit integer specifying a bit offset within the result field.
+ *
+ */
+ACTION_OPCODE_DNS_LKUP,
};
/* Header. */
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 0cdbf87..e11a374 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -659,7 +659,237 @@ exit:
}
static void
-process_packet_in(const struct ofp_header *msg)
+pinctrl_handle_dns_lkup(
+struct dp_packet *pkt_in, struct ofputil_packet_in *pin,
+struct ofpbuf *userdata, struct ofpbuf *continuation,
+struct controller_ctx *ctx)
+{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+enum ofp_version version = rconn_get_version(swconn);
+enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
+struct dp_packet *pkt_out_ptr = NULL;
+uint32_t success = 0;
+
+/* Parse result field. */
+const struct mf_field *f;
+enum ofperr ofperr = nx_pull_header(userdata, NULL, &f, NULL);
+if (ofperr) {
+ VLOG_WARN_RL(&rl, "bad result OXM (%s)", ofperr_to_string(ofperr));
+ goto exit;
+}
+
+/* Parse result offset. */
+ovs_be32 *ofsp = ofpbuf_try_pull(userdata, sizeof *ofsp);
+if (!ofsp) {
+VLOG_WARN_RL(&rl, "offset not present in the userdata");
+goto exit;
+}
+
+/* Check that the result is valid and writable. */
+struct mf_subfield dst = { .field = f, .ofs = ntohl(*ofsp), .n_bits = 1 };
+ofperr = mf_check_dst(&dst, NULL);
+if (ofperr) {
+VLOG_WARN_RL(&rl, "bad result bit (%s)", ofperr_to_string(ofperr));
+goto exit;
+}
+
+/* Extract the DNS header */
+struct dns_header const *in_dns_header = dp_packet_get_udp_payload(pkt_in);
+
+/* Check if it is DNS request or not */
+if (in_dns_header->lo_flag & 0x80) {
+/* It's a DNS response packet which we are not interested in */
+goto exit;
+}
+
+/* Check if at least one query request is present */
+if (!ntohs(in_dns_header->qdcount)) {
+goto exit;
+}
+
+struct udp_header *in_udp = dp_packet_l4(pkt_in);
+size_t udp_len = ntohs(in_udp->udp_len);
+size_t l4_len = dp_packet_l4_size(pkt_in);
+uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len);
+uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1);
+uint8_t *in_hostname = in_dns_data;
+uint8_t idx = 0;
+struct ds hostname;
+ds_init(&hostname);
+/* Extract the hostname. If the hostname is - 'www.ovn.org' it would be
+ * encoded as (in hex) - 03 77 77 77
Re: [ovs-dev] [PATCH 1/2] ovn-controller: Add 'dns_lkup' action
On Fri, Feb 10, 2017 at 08:02:15PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique
>
> 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
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, , and ANY query types.
Thanks,
Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] ovn-controller: Add 'dns_lkup' action
Thanks for the review Ben.
Please see inline for few comments.
Numan
On Wed, Mar 8, 2017 at 10:22 PM, Ben Pfaff wrote:
> On Fri, Feb 10, 2017 at 08:02:15PM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique
> >
> > 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
>
> 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.
>
Done.
>
> 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.
>
>
In the v2 patch (which I will send shortly), I have put a message "as no
implemented". I think it should be possible for ovn-trace to support
"dns_lookup". I will look into it as a follow patch. Please let me know if
you have any concerns which you want me to handle in this patch.
ovn-sb.xml should document the new action.
>
>
Done.
> 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.
>
>
Corrected it.
> 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.
>
> Done.
> 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.
>
> Done. Just one column in v2.
> 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).
>
>
Agree. I have dropped "fqdn" column in v2.
> 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.:
>
Done.
>
> @@ -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.
>
>
Done.
> In pinctrl_handle_dns_lkup() here, I would drop the ntohs() for checking
> that a value is nonzero:
>
> if (!ntohs(in_dns_header->qdcount)) {
>
> Done
> 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) {
>
>
Done
> 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++]);
> }
>
>
Done.
> I think it would be good to declare macros or enums for query types, or
> at least for the supported A, , and ANY query types.
>
>
Done.
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
