Re: [ovs-dev] [PATCH v4 3/3] ovn-northd: Add logical flows to support native DNS

2017-04-17 Thread Numan Siddique 
On Sat, Apr 15, 2017 at 8:51 AM, Ben Pfaff  wrote:

> On Mon, Apr 03, 2017 at 04:10:27PM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > OVN implements native DNS resolution which can be used to resolve the
> > internal DNS names belonging to a logical datapath.
> >
> > To support this, a new table 'DNS' is added in the NB DB. A new column
> > 'dns_records' is added in 'Logical_Switch' table which references to the
> > 'DNS' table.
> >
> > Following flows are added for each logical switch if configured with
> > DNS records in the 'dns_records' column
> >  - A logical flow in DNS_LOOKUP stage which uses the action 'dns_lookup'
> >to transform the DNS query to DNS reply packet and advances
> >to the next stage - DNS_RESPONSE.
> >
> >  - A logical flow in DNS_RESPONSE stage which implements the DNS
> responder
> >by sending the DNS reply from previous stage back to the inport.
> >
> > Signed-off-by: Numan Siddique 
>
> Thank you very much!
>
> I see a few uses of  in ovn-northd.8.xml.  I think that you can
> just write - instead.
>
> ovn-northd.c uses smap_count() in a few places to determine whether an
> smap is nonempty.  I would prefer !smap_is_empty(), instead.
>
> It looks to me like at least two places have code that check for at
> least one DNS record in an ovn_datapath.  I think that a helper function
> could clarify and reduce the amount of code.
>
> For iteration with HMAP_FOR_EACH and similar constructs, please write a
> space after HMAP_FOR_EACH, so that it looks a little more like the
> formatting we use for a "for" statement in OVS.
>
> sync_dns_entries() has two different HMAP_FOR_EACH_WITH_HASH loops that
> search for a dns_info struct with a particular uuid.  I think that it
> would be better to write a helper function, to save code and make the
> intent clearer.
>
> sync_dns_entries() calls sbrec_dns_set_datapaths() and
> sbrec_dns_set_records() in two places.  I think that this could be done
> in a single place, in the final loop (although it might require an extra
> 'delete' member in struct dns_info) or it could be a helper function.
>
> Thanks again!
>


Thanks Ben for the review and the comments. I have addressed all your
comments from  P2 and this patch and will submit the v5 shortly.
(This patch anyway needs a re base because of conflict).

Thanks again
Numan


> Ben
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 3/3] ovn-northd: Add logical flows to support native DNS

2017-04-14 Thread Ben Pfaff 
On Mon, Apr 03, 2017 at 04:10:27PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> OVN implements native DNS resolution which can be used to resolve the
> internal DNS names belonging to a logical datapath.
> 
> To support this, a new table 'DNS' is added in the NB DB. A new column
> 'dns_records' is added in 'Logical_Switch' table which references to the
> 'DNS' table.
> 
> Following flows are added for each logical switch if configured with
> DNS records in the 'dns_records' column
>  - A logical flow in DNS_LOOKUP stage which uses the action 'dns_lookup'
>to transform the DNS query to DNS reply packet and advances
>to the next stage - DNS_RESPONSE.
> 
>  - A logical flow in DNS_RESPONSE stage which implements the DNS responder
>by sending the DNS reply from previous stage back to the inport.
> 
> Signed-off-by: Numan Siddique 

Thank you very much!

I see a few uses of  in ovn-northd.8.xml.  I think that you can
just write - instead.

ovn-northd.c uses smap_count() in a few places to determine whether an
smap is nonempty.  I would prefer !smap_is_empty(), instead.

It looks to me like at least two places have code that check for at
least one DNS record in an ovn_datapath.  I think that a helper function
could clarify and reduce the amount of code.

For iteration with HMAP_FOR_EACH and similar constructs, please write a
space after HMAP_FOR_EACH, so that it looks a little more like the
formatting we use for a "for" statement in OVS.

sync_dns_entries() has two different HMAP_FOR_EACH_WITH_HASH loops that
search for a dns_info struct with a particular uuid.  I think that it
would be better to write a helper function, to save code and make the
intent clearer.

sync_dns_entries() calls sbrec_dns_set_datapaths() and
sbrec_dns_set_records() in two places.  I think that this could be done
in a single place, in the final loop (although it might require an extra
'delete' member in struct dns_info) or it could be a helper function.

Thanks again!

Ben
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 3/3] ovn-northd: Add logical flows to support native DNS

2017-04-03 Thread nusiddiq 
From: Numan Siddique 

OVN implements native DNS resolution which can be used to resolve the
internal DNS names belonging to a logical datapath.

To support this, a new table 'DNS' is added in the NB DB. A new column
'dns_records' is added in 'Logical_Switch' table which references to the
'DNS' table.

Following flows are added for each logical switch if configured with
DNS records in the 'dns_records' column
 - A logical flow in DNS_LOOKUP stage which uses the action 'dns_lookup'
   to transform the DNS query to DNS reply packet and advances
   to the next stage - DNS_RESPONSE.

 - A logical flow in DNS_RESPONSE stage which implements the DNS responder
   by sending the DNS reply from previous stage back to the inport.

Signed-off-by: Numan Siddique 
---
 ovn/northd/ovn-northd.8.xml |  85 +-
 ovn/northd/ovn-northd.c | 180 -
 ovn/ovn-nb.ovsschema|  20 ++-
 ovn/ovn-nb.xml  |  27 +++-
 ovn/utilities/ovn-nbctl.c   |   3 +
 tests/ovn.at| 377 
 6 files changed, 679 insertions(+), 13 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index ab8fd88..cb7473c 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -724,7 +724,71 @@ output;
   
 
 
-Ingress Table 13 Destination Lookup
+Ingress Table 13 DNS Lookup
+
+
+  This table looks up and resolves the DNS names of the logical ports
+  if configured with the host names.
+
+
+
+  
+
+  A priority-100 logical flow for each logical switch datapath
+  if it is configured with DNS records, which matches the IPv4 and IPv6
+  packets with udp.dst = 53 and applies the action
+  dns_lookup and advances the packet to the next table.
+
+
+
+reg0[4] = dns_lookup(); next;
+
+
+
+  For valid DNS packets, this transforms the packet into a DNS
+  reply if the DNS name can be resolved, and stores 1 into reg0[4].
+  For failed DNS resolution or other kinds of packets, it just stores
+  0 into reg0[4]. Either way, it continues to the next table.
+
+  
+
+
+Ingress Table 14 DNS Responses
+
+
+  This table implements DNS responder for the DNS replies generated by
+  the previous table.
+
+
+
+  
+
+  A priority-100 logical flow for each logical switch datapath
+  if it is configured with DNS records, which matches the IPv4 and IPv6
+  packets with udp.dst = 53  reg0[4] == 1
+  and responds back to the inport after applying these
+  actions.  If reg0[4] is set to 1, it means that the
+  action dns_lookup was successful.
+
+
+
+eth.dst  eth.src;
+ip4.src  ip4.dst;
+udp.dst = udp.src;
+udp.src = 53;
+outport = P;
+flags.loopback = 1;
+output;
+
+
+
+  (This terminates ingress packet processing; the packet does not go
+   to the next ingress table.)
+
+  
+
+
+Ingress Table 15 Destination Lookup
 
 
   This table implements switching behavior.  It contains these logical
@@ -834,11 +898,22 @@ output;
 
 
 
-  Also a priority 34000 logical flow is added for each logical port which
-  has DHCPv4 options defined to allow the DHCPv4 reply packet and which has
-  DHCPv6 options defined to allow the DHCPv6 reply packet from the
-  Ingress Table 12: DHCP responses.
+  Also the following flows are added.
 
+
+  
+A priority 34000 logical flow is added for each logical port which
+has DHCPv4 options defined to allow the DHCPv4 reply packet and which 
has
+DHCPv6 options defined to allow the DHCPv6 reply packet from the
+Ingress Table 12: DHCP responses.
+  
+
+  
+A priority 34000 logical flow is added for each logical switch datapath
+if it is configured with DNS records, which allows the DNS reply packet
+from the Ingress Table 14:DNS responses.
+  
+
 
 Egress Table 7: Egress Port Security - IP
 
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 5a2e5ab..5753777 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -112,7 +112,9 @@ enum ovn_stage {
 PIPELINE_STAGE(SWITCH, IN,  ARP_ND_RSP,10, "ls_in_arp_rsp")   \
 PIPELINE_STAGE(SWITCH, IN,  DHCP_OPTIONS,  11, "ls_in_dhcp_options")  \
 PIPELINE_STAGE(SWITCH, IN,  DHCP_RESPONSE, 12, "ls_in_dhcp_response") \
-PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,   13, "ls_in_l2_lkup")   \
+PIPELINE_STAGE(SWITCH, IN,  DNS_LOOKUP,  13, "ls_in_dns_lookup") \
+PIPELINE_STAGE(SWITCH, IN,  DNS_RESPONSE,  14, "ls_in_dns_response") \
+PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,   15, "ls_in_l2_lkup")   \