From patchwork Thu Jul 15 12:14:46 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Gray X-Patchwork-Id: 1505660 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=fCXTQGN1; dkim-atps=neutral Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4GQYGW52pDz9sWc for ; Thu, 15 Jul 2021 22:14:59 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id DE59260B88; Thu, 15 Jul 2021 12:14:57 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id YgOdlau7Zizd; Thu, 15 Jul 2021 12:14:56 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp3.osuosl.org (Postfix) with ESMTPS id A6D4A6003C; Thu, 15 Jul 2021 12:14:55 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7A035C0010; Thu, 15 Jul 2021 12:14:55 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 94791C000E for ; Thu, 15 Jul 2021 12:14:54 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 7529583D75 for ; Thu, 15 Jul 2021 12:14:54 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp1.osuosl.org (amavisd-new); dkim=pass (1024-bit key) header.d=redhat.com Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xRb64gfQA5A4 for ; Thu, 15 Jul 2021 12:14:53 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by smtp1.osuosl.org (Postfix) with ESMTPS id 0FADC83D5A for ; Thu, 15 Jul 2021 12:14:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1626351291; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Io5SJveP5uXnPnxILbS40/wl1ztDa76QCnBjPNHo90w=; b=fCXTQGN1an55JEN5kLXSmdaPwVu3HQt+WnakwHlso1f6gYeCzICGMRJUr/FC9vw5HPYCpj OoPOvD/1BF9ZR89eV62mIuFma4r8UzEe6A/xsp11o+Vo4oqoft6fn6DHGJjV/tuHLEKI3J racUMY2wjw7F1FJNLyJEKfC/gmZ5x1k= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-175-dx6nzNpePbyNRZNd-LB7-w-1; Thu, 15 Jul 2021 08:14:50 -0400 X-MC-Unique: dx6nzNpePbyNRZNd-LB7-w-1 Received: by mail-wr1-f70.google.com with SMTP id t8-20020a05600001c8b029013e2027cf9aso3196684wrx.9 for ; Thu, 15 Jul 2021 05:14:50 -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:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=Io5SJveP5uXnPnxILbS40/wl1ztDa76QCnBjPNHo90w=; b=bglo1jG69go8+b4/M/NiN6nsKGMN89RFCcx0wcEtorewZWODUeVcEGF7YCvuRKCSgm jbU+kvhdb7wle852RqCVGBhwlG9KhVdKRrLwvRpQPSakEn6MXtbbLNDZmMSQZ76QE52H m5MAuutBp/6jQIfo2Ubdisv/n/TN9ytGxjj/gROi2VS0OJMQ+WZ9wzCrYm69o2PC5REZ nXy5jTEHqAzucoFa5DxpjeyJGzA05a3yTOlR3JNwV1eqalEmGZoklgAYOVAsHjMVRskJ 4AWRW0M0H3GDLPXZROrd3+E06vzpRaH7BRzAJTzz9oID1+Wc2AmN0+XJDEXEBBpeX2Le Yk2A== X-Gm-Message-State: AOAM532pgrp43nJkTpS8wwnM4ef3DJEc8Qpf7LBnAqc3Q2tjjoSASJmd lknNVrBCYf3QJCuiIxfzjmjGSQ/OAg+Mhv1iJZRuFNrhqzDc8N+jzOjXWW4ZNNUxZcKpzPdkxtr 9lYsO/I35u656w2RVjBf3/7qg3GH+svhm26yDAIqVPEdNEgtKTR5KiLGyCxagJTs36F/5 X-Received: by 2002:a7b:c452:: with SMTP id l18mr10198323wmi.164.1626351289438; Thu, 15 Jul 2021 05:14:49 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwknXJVGF7VtfpIXA1fMKsZC+7P7JyFf4os/UKoYDNNL6yPcdKDm6F3lYAUG50G+iuMBZdX6w== X-Received: by 2002:a7b:c452:: with SMTP id l18mr10198281wmi.164.1626351288967; Thu, 15 Jul 2021 05:14:48 -0700 (PDT) Received: from wsfd-netdev91.ntdv.lab.eng.bos.redhat.com (nat-pool-bos-t.redhat.com. [66.187.233.206]) by smtp.gmail.com with ESMTPSA id p5sm4745335wme.2.2021.07.15.05.14.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Jul 2021 05:14:48 -0700 (PDT) From: Mark Gray To: dev@openvswitch.org Date: Thu, 15 Jul 2021 08:14:46 -0400 Message-Id: <20210715121446.2614139-1-mark.d.gray@redhat.com> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mark.d.gray@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn] northd: Fix defrag flows for duplicate vips X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When adding two SB flows with the same vip but different protocols, only the most recent flow will be added due to the `if` statement: if (!sset_contains(&all_ips, lb_vip->vip_str)) { sset_add(&all_ips, lb_vip->vip_str); This can cause unexpected behaviour when two load balancers with the same VIP (and different protocols) are added to a logical router. This is due to the addition of "protocol" to the match in defrag table flows in a previous commit. Revert that change. This bug was discovered through the OVN CI (ovn-kubernetes.yml). Fixes: 384a7c6237da ("northd: Refactor Logical Flows for routers with DNAT/Load Balancers") Signed-off-by: Mark Gray --- northd/ovn-northd.c | 8 -------- northd/ovn_northd.dl | 9 +-------- tests/ovn-northd.at | 46 ++++++++++++++++++++++---------------------- 3 files changed, 24 insertions(+), 39 deletions(-) diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 999c3f482c29..5fab62c0fcf7 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -9219,11 +9219,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, for (size_t j = 0; j < lb->n_vips; j++) { struct ovn_lb_vip *lb_vip = &lb->vips[j]; - bool is_udp = nullable_string_is_equal(nb_lb->protocol, "udp"); - bool is_sctp = nullable_string_is_equal(nb_lb->protocol, - "sctp"); - const char *proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp"; - struct ds defrag_actions = DS_EMPTY_INITIALIZER; if (!sset_contains(&all_ips, lb_vip->vip_str)) { sset_add(&all_ips, lb_vip->vip_str); @@ -9249,9 +9244,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, lb_vip->vip_str); } - if (lb_vip->vip_port) { - ds_put_format(match, " && %s", proto); - } ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG, 100, ds_cstr(match), ds_cstr(&defrag_actions), diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index ceeabe6f384e..b37da86f76aa 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -6167,14 +6167,7 @@ for (RouterLBVIP( * pick a DNAT ip address from a group. * 2. If there are L4 ports in load balancing rules, we * need the defragmentation to match on L4 ports. */ - var match1 = "ip && ${ipX}.dst == ${ip_address}" in - var match2 = - if (port != 0) { - " && ${proto}" - } else { - "" - } in - var __match = match1 ++ match2 in + var __match = "ip && ${ipX}.dst == ${ip_address}" in var xx = ip_address.xxreg() in var __actions = "${xx}${rEG_NEXT_HOP()} = ${ip_address}; ct_dnat;" in /* One of these flows must be created for each unique LB VIP address. diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 11461d3f4c2a..072616898d63 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -3167,7 +3167,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -3200,7 +3200,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -3243,7 +3243,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -3300,7 +3300,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -3344,7 +3344,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.20 && tcp), action=(reg0 = 10.0.0.20; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.20), action=(reg0 = 10.0.0.20; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | grep skip_snat_for_lb | sort], [0], [dnl @@ -4361,10 +4361,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;) table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -4421,10 +4421,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;) table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -4476,10 +4476,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;) table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -4534,11 +4534,11 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.10), action=(reg0 = 172.168.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;) table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -4605,12 +4605,12 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.10), action=(reg0 = 172.168.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;) table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip6.dst == def0::2 && tcp), action=(xxreg0 = def0::2; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip6.dst == def0::2), action=(xxreg0 = def0::2; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl