I'm also looking at this one. I was trying to review today, but have been slowed down by getting an OpenStack test environment working for testing this and looking closer.
On Wed, Mar 8, 2017 at 4:32 PM, Ben Pfaff <b...@ovn.org> wrote: > Hi Darrell and Daniele, do one of you have an opinion on whether this is > the right approach? > > Thanks, > > Ben. > > On Mon, Feb 27, 2017 at 11:29:14AM +0530, nusid...@redhat.com wrote: >> From: Numan Siddique <nusid...@redhat.com> >> >> Presently, the icmp4 requests to the router gateway ip are sent to the >> connectiont tracker, but the icmp4 reply packets responded by >> 'lr_in_ip_input' stage are not sent to the connection tracker. >> Also no zone ids are assigned for the router ports. Because of which >> the icmp4 request packets in the connection tracker will be in the >> UNREPLIED state. If the CMS has added ACLs to drop packets which >> are not in ESTABLISHED state, the icmp4 replies will be dropped. >> >> To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl' >> stage which doesn't set reg0[0] = 1 for icmp4 request packets destined >> to the router gateway ips. >> >> Alternate solution would be to assign zone ids for the router ports >> and send the packets from the router ports to the connection tracker. >> >> The approach used in this patch seems to be simpler. >> >> Signed-off-by: Numan Siddique <nusid...@redhat.com> >> --- >> ovn/northd/ovn-northd.8.xml | 29 +++++++++++--- >> ovn/northd/ovn-northd.c | 92 >> +++++++++++++++++++++++++++------------------ >> 2 files changed, 79 insertions(+), 42 deletions(-) >> >> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml >> index ab8fd88..06465ff 100644 >> --- a/ovn/northd/ovn-northd.8.xml >> +++ b/ovn/northd/ovn-northd.8.xml >> @@ -245,11 +245,30 @@ >> <p> >> This table prepares flows for possible stateful ACL processing in >> ingress table <code>ACLs</code>. It contains a priority-0 flow that >> - simply moves traffic to the next table. If stateful ACLs are used in >> the >> - logical datapath, a priority-100 flow is added that sets a hint >> - (with <code>reg0[0] = 1; next;</code>) for table >> - <code>Pre-stateful</code> to send IP packets to the connection tracker >> - before eventually advancing to ingress table <code>ACLs</code>. >> + simply moves traffic to the next table. It adds the following flows if >> + stateful ACLs are used in the logical datapath. >> + >> + <ul> >> + <li> >> + A priority-100 flow is added that sets a hint >> + (with <code>reg0[0] = 1; next;</code>) for table >> + <code>Pre-stateful</code> to send IP packets to the connection >> tracker >> + before eventually advancing to ingress table <code>ACLs</code>. >> + </li> >> + >> + <li> >> + A priority-110 flow which doesn't set the connection tracking >> hint for >> + the packets from the router ports. >> + </li> >> + >> + <li> >> + A priority-110 flow which doesn't set the connection tracking hint >> + for the packets with the match >> + <code>ip4 && icmp4 && ip4.dst = >> {<var>R</var>}</code> >> + where <var>R</var> is the IPv4 address(es) of the router ports >> + connected to the logical datapath. >> + </li> >> + </ul> >> </p> >> >> <h3>Ingress Table 4: Pre-LB</h3> >> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> index 03dc850..4fc86f4 100644 >> --- a/ovn/northd/ovn-northd.c >> +++ b/ovn/northd/ovn-northd.c >> @@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od) >> } >> >> static void >> +op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast) >> +{ >> + if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) { >> + ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s); >> + return; >> + } >> + >> + ds_put_cstr(ds, "{"); >> + for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >> + ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s); >> + if (add_bcast) { >> + ds_put_format(ds, "%s, ", >> op->lrp_networks.ipv4_addrs[i].bcast_s); >> + } >> + } >> + ds_chomp(ds, ' '); >> + ds_chomp(ds, ','); >> + ds_put_cstr(ds, "}"); >> +} >> + >> +static void >> +op_put_v6_networks(struct ds *ds, const struct ovn_port *op) >> +{ >> + if (op->lrp_networks.n_ipv6_addrs == 1) { >> + ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s); >> + return; >> + } >> + >> + ds_put_cstr(ds, "{"); >> + for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >> + ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s); >> + } >> + ds_chomp(ds, ' '); >> + ds_chomp(ds, ','); >> + ds_put_cstr(ds, "}"); >> +} >> + >> +static void >> build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) >> { >> bool has_stateful = has_stateful_acl(od); >> @@ -2378,6 +2415,24 @@ build_pre_acls(struct ovn_datapath *od, struct hmap >> *lflows) >> >> ds_destroy(&match_in); >> ds_destroy(&match_out); >> + >> + /* The icmp4 request to the router ip (eg. ip4.dst = 10.0.0.1) >> will >> + * be sent to the CT, but the reply from the router ip is not >> sent >> + * to the CT. Also the zone id's are not assigned for the router >> + * ports. Because of which the icmp request packet in the CT >> will >> + * be in the UNREPLIED state. >> + * The icmp4 reply packets could be dropped if the CMS has added >> + * ACLs to drop packets which are not in ct.est state. >> + * So, don't send the icpmp4 request packet for the router ips >> + * to the CT. >> + */ >> + if (op->peer && op->peer->lrp_networks.n_ipv4_addrs) { >> + struct ds match = DS_EMPTY_INITIALIZER; >> + ds_put_format(&match, "ip4 && icmp4 && ip4.dst == "); >> + op_put_v4_networks(&match, op->peer, false); >> + ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, >> + ds_cstr(&match), "next;"); >> + } >> } >> /* Ingress and Egress Pre-ACL Table (Priority 110). >> * >> @@ -3644,43 +3699,6 @@ free_prefix_s: >> free(prefix_s); >> } >> >> -static void >> -op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast) >> -{ >> - if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) { >> - ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s); >> - return; >> - } >> - >> - ds_put_cstr(ds, "{"); >> - for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >> - ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s); >> - if (add_bcast) { >> - ds_put_format(ds, "%s, ", >> op->lrp_networks.ipv4_addrs[i].bcast_s); >> - } >> - } >> - ds_chomp(ds, ' '); >> - ds_chomp(ds, ','); >> - ds_put_cstr(ds, "}"); >> -} >> - >> -static void >> -op_put_v6_networks(struct ds *ds, const struct ovn_port *op) >> -{ >> - if (op->lrp_networks.n_ipv6_addrs == 1) { >> - ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s); >> - return; >> - } >> - >> - ds_put_cstr(ds, "{"); >> - for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >> - ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s); >> - } >> - ds_chomp(ds, ' '); >> - ds_chomp(ds, ','); >> - ds_put_cstr(ds, "}"); >> -} >> - >> static const char * >> get_force_snat_ip(struct ovn_datapath *od, const char *key_type, ovs_be32 >> *ip) >> { >> -- >> 2.9.3 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- Russell Bryant _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev