Thanks for the review and comments.
On Thu, Mar 9, 2017 at 10:00 AM, Darrell Ball <[email protected]> wrote: > Daniele and I discussed > > 1) Seems ok in that there is security at the VM LP so weakening the > Check at the router port for ICMP seems ok. > 2) The same applies to V6 ? > I need to test this before confirming. > > Thanks > > > On 3/8/17, 1:32 PM, "[email protected] on behalf of Ben > Pfaff" <[email protected] on behalf of [email protected]> 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, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 <[email protected]> > > --- > > 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. > > This above comment is not needed as there is only the below flow added. > "build_pre_acls" adds the above flow (though this patch doesn't add this flow and the comment is somewhat unrelated to this patch), but the documentation is missing. I can spin another patch if it's not fine to have this change in this patch. https://github.com/openvswitch/ovs/blob/master/ovn/northd/ovn-northd.c#L2460 > > > + </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 > > [email protected] > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail. > openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c= > uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m= > 1bF0sSIIafODfWGrMWfrLDZp6Gan2-gIhuE5AqyVSS8&s=Gt9_RJ5SYYMdi7iiM_ > NPyk3NXkB4wLYbxyx_pQjlRrA&e= > _______________________________________________ > dev mailing list > [email protected] > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail. > openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c= > uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m= > 1bF0sSIIafODfWGrMWfrLDZp6Gan2-gIhuE5AqyVSS8&s=Gt9_RJ5SYYMdi7iiM_ > NPyk3NXkB4wLYbxyx_pQjlRrA&e= > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
