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
