From patchwork Mon Mar 26 21:18:28 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gurucharan Shetty X-Patchwork-Id: 891256 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4096VV4JQYz9s1b for ; Tue, 27 Mar 2018 08:20:02 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id D6B9F135B; Mon, 26 Mar 2018 21:19:58 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 3A87412E9 for ; Mon, 26 Mar 2018 21:19:57 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pl0-f52.google.com (mail-pl0-f52.google.com [209.85.160.52]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id F17234CC for ; Mon, 26 Mar 2018 21:19:55 +0000 (UTC) Received: by mail-pl0-f52.google.com with SMTP id b7-v6so12750259plr.8 for ; Mon, 26 Mar 2018 14:19:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=O18r1/YS/Gv7yHFJsxY7w2xDOxSdg+j1MVUkLqzNvG4=; b=Kd9hciqHW4jeSt0cVjKLgUXwxjHS5DX3B0KfhlOicryM7DL6NnPmNCoNVrC0SUyFc6 1cvGDXHswyzZb3BvkgRP1PfUsI4v+XFXRtHI8LoRF94pwfBmXrQnTWCfFEcgjJfdqoXC jsw4BCL25GYjVM3ajPs3nWPWd/Y/J0sUbAl+TQQLjL0n18PI8vztFbEyTupTUyPRK+3x q4VADjcgAjN8PJLYyzx48aquiEsPDjRypkaJOFuHWYw6d3HsNx2lvHiMcfm7nWDq55G6 JrkICYkFBvgBXaJ38xrxRUmjeAcU2gEoxE5VALHehdOSh1Grjp/6/OE3BFmNjpodBS47 1Z3w== X-Gm-Message-State: AElRT7HXz6KnBZbINUFYtK4vn9YosRlp3fZMZ2ptqJRnrEgOJUqGmISR xIayxviDn6dsax8TJON4BWhnig== X-Google-Smtp-Source: AG47ELvdN9l/VPAaorOlw98CddPf/LHlAAgyBcnt7A9DpF0Q6NSyY1GL1lxWgPnHIIBt25VxKASk5A== X-Received: by 2002:a17:902:7401:: with SMTP id g1-v6mr42712121pll.4.1522099195189; Mon, 26 Mar 2018 14:19:55 -0700 (PDT) Received: from ubuntu.eng.vmware.com ([66.170.99.1]) by smtp.gmail.com with ESMTPSA id y23sm25597055pgv.4.2018.03.26.14.19.53 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 26 Mar 2018 14:19:54 -0700 (PDT) From: Gurucharan Shetty To: dev@openvswitch.org Date: Mon, 26 Mar 2018 14:18:28 -0700 Message-Id: <1522099108-1323-1-git-send-email-guru@ovn.org> X-Mailer: git-send-email 2.7.4 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH] ovn: Recirculate packets after a unSNAT. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org commit f6fabcc6245 (ofproto-dpif: Mark packets as "untracked" after call to ct().) changed the behavior after a call to ct(). The +trk bit would automatically be unset if packet is sent to ct() and not forked. This caused a bug in the OVN gateway pipeline when there is SNAT rule as well as load-balancing rule. In the OVN gateway pipeline for the gateway router, we had an optimization where the packets sent to unSNAT need not go through a recirculation. But since doing this now means that the +trk bit gets unset, the DNAT rules for load-balancing a new packet in the next table won't get hit. This commit removes the optimization for unSNAT packets so that there is always a recirculation. Signed-off-by: Gurucharan Shetty Acked-by: Ben Pfaff --- include/ovn/actions.h | 3 --- ovn/controller/lflow.c | 10 ---------- ovn/lib/actions.c | 11 ----------- ovn/northd/ovn-northd.8.xml | 6 +++--- ovn/northd/ovn-northd.c | 17 ++++++----------- ovn/ovn-sb.xml | 21 +++------------------ tests/system-ovn.at | 5 +++++ 7 files changed, 17 insertions(+), 56 deletions(-) diff --git a/include/ovn/actions.h b/include/ovn/actions.h index b1988d6..7076f78 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -499,9 +499,6 @@ struct ovnact_encode_params { /* 'true' if the flow is for a switch. */ bool is_switch; - /* 'true' if the flow is for a gateway router. */ - bool is_gateway_router; - /* A struct to figure out the group_id for group actions. */ struct ovn_extend_table *group_table; diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index df125b1..fbd0fef 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -131,15 +131,6 @@ is_switch(const struct sbrec_datapath_binding *ldp) } -static bool -is_gateway_router(const struct sbrec_datapath_binding *ldp, - const struct hmap *local_datapaths) -{ - struct local_datapath *ld = - get_local_datapath(local_datapaths, ldp->tunnel_key); - return ld ? ld->has_local_l3gateway : false; -} - /* Adds the logical flows from the Logical_Flow table to flow tables. */ static void add_logical_flows(struct controller_ctx *ctx, @@ -303,7 +294,6 @@ consider_logical_flow(struct controller_ctx *ctx, .lookup_port = lookup_port_cb, .aux = &aux, .is_switch = is_switch(ldp), - .is_gateway_router = is_gateway_router(ldp, local_datapaths), .group_table = group_table, .meter_table = meter_table, diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index fc5ace1..6ed200c 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -832,17 +832,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, ct = ofpacts->header; if (cn->ip) { ct->flags |= NX_CT_F_COMMIT; - } else if (snat && ep->is_gateway_router) { - /* For performance reasons, we try to prevent additional - * recirculations. ct_snat which is used in a gateway router - * does not need a recirculation. ct_snat(IP) does need a - * recirculation. ct_snat in a distributed router needs - * recirculation regardless of whether an IP address is - * specified. - * XXX Should we consider a method to let the actions specify - * whether an action needs recirculation if there are more use - * cases?. */ - ct->recirc_table = NX_CT_RECIRC_NONE; } ofpact_finish(ofpacts, &ct->ofpact); ofpbuf_push_uninit(ofpacts, ct_offset); diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml index 444be72..d6d270b 100644 --- a/ovn/northd/ovn-northd.8.xml +++ b/ovn/northd/ovn-northd.8.xml @@ -1428,14 +1428,14 @@ icmp4 { If the Gateway router has been configured to force SNAT any previously DNATted packets to B, a priority-110 flow matches ip && ip4.dst == B with - an action ct_snat; next;. + an action ct_snat; .

If the Gateway router has been configured to force SNAT any previously load-balanced packets to B, a priority-100 flow matches ip && ip4.dst == B with - an action ct_snat; next;. + an action ct_snat; .

@@ -1443,7 +1443,7 @@ icmp4 { to change the source IP address of a packet from A to B, a priority-90 flow matches ip && ip4.dst == B with an action - ct_snat; next;. + ct_snat; .

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 3963810..8f9e803 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -5190,7 +5190,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_put_format(&match, "ip && ip4.dst == %s", nat->external_ip); ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 90, - ds_cstr(&match), "ct_snat; next;"); + ds_cstr(&match), "ct_snat;"); } else { /* Distributed router. */ @@ -5422,7 +5422,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_clear(&match); ds_put_format(&match, "ip && ip4.dst == %s", dnat_force_snat_ip); ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 110, - ds_cstr(&match), "ct_snat; next;"); + ds_cstr(&match), "ct_snat;"); /* Higher priority rules to force SNAT with the IP addresses * configured in the Gateway router. This only takes effect @@ -5441,7 +5441,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_clear(&match); ds_put_format(&match, "ip && ip4.dst == %s", lb_force_snat_ip); ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 100, - ds_cstr(&match), "ct_snat; next;"); + ds_cstr(&match), "ct_snat;"); /* Load balanced traffic will have flags.force_snat_for_lb set. * Force SNAT it. */ @@ -5455,19 +5455,14 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, if (!od->l3dgw_port) { /* For gateway router, re-circulate every packet through - * the DNAT zone. This helps with two things. + * the DNAT zone. This helps with the following. * - * 1. Any packet that needs to be unDNATed in the reverse + * Any packet that needs to be unDNATed in the reverse * direction gets unDNATed. Ideally this could be done in * the egress pipeline. But since the gateway router * does not have any feature that depends on the source * ip address being external IP address for IP routing, - * we can do it here, saving a future re-circulation. - * - * 2. Any packet that was sent through SNAT zone in the - * previous table automatically gets re-circulated to get - * back the new destination IP address that is needed for - * routing in the openflow pipeline. */ + * we can do it here, saving a future re-circulation. */ ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50, "ip", "flags.loopback = 1; ct_dnat;"); } else { diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index 6a8b818..7bcd3b9 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -1151,25 +1151,10 @@

ct_snat sends the packet through the SNAT zone to unSNAT any packet that was SNATed in the opposite direction. The - behavior on gateway routers differs from the behavior on a - distributed router: + packet is automatically sent to the next tables as if followed by + the next; action. The next tables will see the + changes in the packet caused by the connection tracker.

-
    -
  • - On a gateway router, if the packet needs to be sent to the next - tables, then it should be followed by a next; - action. The next tables will not see the changes in the packet - caused by the connection tracker. -
    • -
  • - On a distributed router, if the connection tracker finds a - connection that was SNATed in the opposite direction, then the - destination IP address of the packet is UNSNATed. The packet is - automatically sent to the next tables as if followed by the - next; action. The next tables will see the changes - in the packet caused by the connection tracker. -
    • -

ct_snat(IP) sends the packet through the SNAT zone to change the source IP address of the packet to diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 638c0b6..b7e2d77 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -855,6 +855,11 @@ ovn-nbctl set logical_router R2 load_balancer=$uuid # Config OVN load-balancer with another VIP (this time with ports). ovn-nbctl set load_balancer $uuid vips:'"30.0.0.2:8000"'='"192.168.1.2:80,192.168.2.2:80"' +# Add SNAT rule to make sure that Load-balancing still works with a SNAT rule. +ovn-nbctl -- --id=@nat create nat type="snat" logical_ip=192.168.2.2 \ + external_ip=30.0.0.2 -- add logical_router R2 nat @nat + + # Wait for ovn-controller to catch up. ovn-nbctl --wait=hv sync OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups br-int | \