From patchwork Mon Nov 16 15:49:56 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Flaviof X-Patchwork-Id: 1401000 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=silver.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=flaviof.com Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4CZYSM1407z9sRK for ; Tue, 17 Nov 2020 02:50:26 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id DF5582051F; Mon, 16 Nov 2020 15:50:23 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id lzUO189ZYe4v; Mon, 16 Nov 2020 15:50:14 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id E39AA20117; Mon, 16 Nov 2020 15:50:13 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id D6D81C0891; Mon, 16 Nov 2020 15:50:13 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id F139BC07FF for ; Mon, 16 Nov 2020 15:50:12 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id DEB5287166 for ; Mon, 16 Nov 2020 15:50:12 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pv-nKY9zqDdy for ; Mon, 16 Nov 2020 15:50:11 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-qv1-f48.google.com (mail-qv1-f48.google.com [209.85.219.48]) by hemlock.osuosl.org (Postfix) with ESMTPS id CE124870A4 for ; Mon, 16 Nov 2020 15:50:10 +0000 (UTC) Received: by mail-qv1-f48.google.com with SMTP id 63so8961908qva.7 for ; Mon, 16 Nov 2020 07:50:10 -0800 (PST) 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:in-reply-to :references; bh=xFcBaWkG8dsoB6bUsMeC3fNX/zFtya8FdsdU7IHkXIo=; b=Dgh1Hc1TnUoOV5yFZjcyVlontnYcJH0FRzJOF4FQyymtGAKRCfIQNDdtjcSSUmF+Nb BHmu/GXx5nMLWHj3WeqHXEE+HepaMbAj1xpqjVv1O48iDDjy+evwVvW6eSQX/y2ctjoo pN3R/E6cxtKR3q29q4a3I8goSY92E5NvIin82tL0DNzpIF1Fj+VeiajUjhxsSEvA70VG 3Qsv4PM3P7NY6EgQMM1s7FrPccr+NkV1VF8rP13Qf85lBYyprnqRjTECtjWw9ZVCGqpb bRuQHnjnNTK8tRyQr7XvbQXJeoEKxamxvt0ZbHLthKDQnJOiXzbeJXSFQqqc57p7BCPO iwLg== X-Gm-Message-State: AOAM532QOKmzF71v4hscpHP7eFJdqoqFiq6k/xHRemnlxStmbrN2K0kd aW5AgpD3RsygU0XJrhUpkUmggpLVFjk/mw== X-Google-Smtp-Source: ABdhPJy8Ee6gjYBKMdoy7RzyEzbKFp9x38uJ/G9DoxhFx6bfmbPapZ2MHqKFqwt+GCx5J1ICifbo4g== X-Received: by 2002:a0c:fb4a:: with SMTP id b10mr16183077qvq.1.1605541808977; Mon, 16 Nov 2020 07:50:08 -0800 (PST) Received: from c8vm.redhat.com (pool-173-76-170-96.bstnma.fios.verizon.net. [173.76.170.96]) by smtp.gmail.com with ESMTPSA id y4sm11960196qtd.24.2020.11.16.07.50.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Nov 2020 07:50:07 -0800 (PST) From: Flavio Fernandes To: dev@openvswitch.org Date: Mon, 16 Nov 2020 15:49:56 +0000 Message-Id: <20201116154957.2443-2-flavio@flaviof.com> X-Mailer: git-send-email 2.18.4 In-Reply-To: <20201116154957.2443-1-flavio@flaviof.com> References: <20201116154957.2443-1-flavio@flaviof.com> Subject: [ovs-dev] [PATCH v5 ovn 1/2] northd: Fair ACL log meters. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When multiple ACL rows use the same Meter for log rate-limiting, the 'noisiest' ACL matches may completely consume the Meter and shadow other ACL logs. This patch introduces a column in northbound's Meter table called fair that allows for a Meter to rate-limit each ACL log separately. The change is backward compatible. Based on the fair column of a Meter row, northd will turn a single Meter in the NB into multiple Meters in the SB by appending the ACL uuid to its name. It will also make action in logical flow use the unique Meter name for each affected ACL log. In order to make the Meter behave as described, add Meter with this option: ovn-nbctl --fair meter-add ${meter_name} drop ${rate} ${unit} Example: ovn-nbctl --fair meter-add meter_me drop 1 pktps Reported-by: Flavio Fernandes Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html Suggested-by: Dumitru Ceara Suggested-by: Ben Pfaff Signed-off-by: Flavio Fernandes --- This change can be tracked in the following github clone/branch: https://github.com/flavio-fernandes/ovn/commits/acl-meters.v5.ddlog v4 -> v5: * Rebase https://github.com/blp/ovs-reviews/ddlog10 (f56dafa83). v3 -> v4: * Rename 'shared meters' to 'fair meters' to keep it less confusing. * NB schema change: Add column in Meter to track which meters are 'fair'. * Add optional '--fair' flag to ovn-nbctl meter-add command. v2 -> v3: * Use recently introduced testing helpers (4afe409e9). v1 -> v2: * Rebase master b38e10f4b. NEWS | 2 + northd/ovn-northd.c | 184 ++++++++++++++++++++++++++++++------------ ovn-nb.ovsschema | 5 +- ovn-nb.xml | 16 +++- tests/ovn-nbctl.at | 6 +- tests/ovn-northd.at | 97 ++++++++++++++++++++++ utilities/ovn-nbctl.c | 16 +++- 7 files changed, 268 insertions(+), 58 deletions(-) diff --git a/NEWS b/NEWS index 04b75e68c..0965dff4f 100644 --- a/NEWS +++ b/NEWS @@ -14,6 +14,8 @@ Post-v20.09.0 server. - Support other_config:vlan-passthru=true to allow VLAN tagged incoming traffic. + - Add "fair" column in Meter table to allow multiple ACL logs to use the + same Meter while being rate-limited independently. OVN v20.09.0 - 28 Sep 2020 -------------------------- diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 4d4190cb9..cee8abce9 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5353,8 +5353,45 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows) } } +static bool +is_a_fair_meter(const struct shash *meter_groups, const char *meter_name) +{ + const struct nbrec_meter *nb_meter = + shash_find_data(meter_groups, meter_name); + if (nb_meter && nb_meter->fair) { + return *nb_meter->fair; + } + return false; +} + +static char* +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) +{ + return xasprintf("%s__" UUID_FMT, + acl->meter, UUID_ARGS(&acl->header_.uuid)); +} + +static void +build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, + const struct shash *meter_groups) +{ + if (!acl->meter) { + return; + } + + /* If ACL log meter uses a fair meter, use unique Meter name. */ + if (is_a_fair_meter(meter_groups, acl->meter)) { + char *meter_name = alloc_acl_log_unique_meter_name(acl); + ds_put_format(actions, "meter=\"%s\", ", meter_name); + free(meter_name); + } else { + ds_put_format(actions, "meter=\"%s\", ", acl->meter); + } +} + static void -build_acl_log(struct ds *actions, const struct nbrec_acl *acl) +build_acl_log(struct ds *actions, const struct nbrec_acl *acl, + const struct shash *meter_groups) { if (!acl->log) { return; @@ -5382,9 +5419,7 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl) ds_put_cstr(actions, "verdict=allow, "); } - if (acl->meter) { - ds_put_format(actions, "meter=\"%s\", ", acl->meter); - } + build_acl_log_meter(actions, acl, meter_groups); ds_chomp(actions, ' '); ds_chomp(actions, ','); @@ -5395,7 +5430,8 @@ static void build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, enum ovn_stage stage, struct nbrec_acl *acl, struct ds *extra_match, struct ds *extra_actions, - const struct ovsdb_idl_row *stage_hint) + const struct ovsdb_idl_row *stage_hint, + const struct shash *meter_groups) { struct ds match = DS_EMPTY_INITIALIZER; struct ds actions = DS_EMPTY_INITIALIZER; @@ -5407,7 +5443,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK) : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); if (extra_match->length > 0) { ds_put_format(&match, "(%s) && ", extra_match->string); } @@ -5432,7 +5468,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, static void consider_acl(struct hmap *lflows, struct ovn_datapath *od, - struct nbrec_acl *acl, bool has_stateful) + struct nbrec_acl *acl, bool has_stateful, + const struct shash *meter_groups) { bool ingress = !strcmp(acl->direction, "from-lport") ? true :false; enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL; @@ -5446,7 +5483,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, * associated conntrack entry and would return "+invalid". */ if (!has_stateful) { struct ds actions = DS_EMPTY_INITIALIZER; - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5472,7 +5509,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(&match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)", acl->match); ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; "); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5491,7 +5528,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(&match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)", acl->match); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5516,10 +5553,10 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_cstr(&match, REGBIT_ACL_HINT_DROP " == 1"); if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, &match, - &actions, &acl->header_); + &actions, &acl->header_, meter_groups); } else { ds_put_format(&match, " && (%s)", acl->match); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5543,10 +5580,10 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_cstr(&actions, "ct_commit { ct_label.blocked = 1; }; "); if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, &match, - &actions, &acl->header_); + &actions, &acl->header_, meter_groups); } else { ds_put_format(&match, " && (%s)", acl->match); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5559,9 +5596,9 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, * logical flow action in all cases. */ if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, &match, - &actions, &acl->header_); + &actions, &acl->header_, meter_groups); } else { - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5641,7 +5678,7 @@ build_port_group_lswitches(struct northd_context *ctx, struct hmap *pgs, static void build_acls(struct ovn_datapath *od, struct hmap *lflows, - struct hmap *port_groups) + struct hmap *port_groups, const struct shash *meter_groups) { bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od)); @@ -5744,13 +5781,14 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, /* Ingress or Egress ACL Table (Various priorities). */ for (size_t i = 0; i < od->nbs->n_acls; i++) { struct nbrec_acl *acl = od->nbs->acls[i]; - consider_acl(lflows, od, acl, has_stateful); + consider_acl(lflows, od, acl, has_stateful, meter_groups); } struct ovn_port_group *pg; HMAP_FOR_EACH (pg, key_node, port_groups) { if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { - consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful); + consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful, + meter_groups); } } } @@ -7448,7 +7486,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, build_pre_lb(od, lflows, meter_groups, lbs); build_pre_stateful(od, lflows); build_acl_hints(od, lflows); - build_acls(od, lflows, port_groups); + build_acls(od, lflows, port_groups, meter_groups); build_qos(od, lflows); build_lb(od, lflows); build_stateful(od, lflows, lbs); @@ -11633,12 +11671,77 @@ done: return need_update; } +static void +sync_meters_iterate_nb_meter(struct northd_context *ctx, + const char *meter_name, + const struct nbrec_meter *nb_meter, + struct shash *sb_meters) +{ + bool new_sb_meter = false; + + const struct sbrec_meter *sb_meter = shash_find_and_delete(sb_meters, + meter_name); + if (!sb_meter) { + sb_meter = sbrec_meter_insert(ctx->ovnsb_txn); + sbrec_meter_set_name(sb_meter, meter_name); + new_sb_meter = true; + } + + if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { + struct sbrec_meter_band **sb_bands; + sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); + for (size_t i = 0; i < nb_meter->n_bands; i++) { + const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; + + sb_bands[i] = sbrec_meter_band_insert(ctx->ovnsb_txn); + + sbrec_meter_band_set_action(sb_bands[i], nb_band->action); + sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); + sbrec_meter_band_set_burst_size(sb_bands[i], + nb_band->burst_size); + } + sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); + free(sb_bands); + } + + sbrec_meter_set_unit(sb_meter, nb_meter->unit); +} + +static void +sync_acl_fair_meters(struct northd_context *ctx, + struct hmap *datapaths, + const struct nbrec_meter *nb_meter, + struct shash *sb_meters) +{ + if (!nb_meter->fair || *nb_meter->fair == false) { + return; + } + + struct ovn_datapath *od; + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbs) { + continue; + } + for (size_t i = 0; i < od->nbs->n_acls; i++) { + struct nbrec_acl *acl = od->nbs->acls[i]; + if (!acl->meter || strcmp(nb_meter->name, acl->meter)) { + continue; + } + + char *meter_name = alloc_acl_log_unique_meter_name(acl); + sync_meters_iterate_nb_meter(ctx, meter_name, nb_meter, sb_meters); + free(meter_name); + } + } +} + /* Each entry in the Meter and Meter_Band tables in OVN_Northbound have * a corresponding entries in the Meter and Meter_Band tables in - * OVN_Southbound. + * OVN_Southbound. Additionally, ACL logs that use fair meters have + * a private copy of its meter in the SB table. */ static void -sync_meters(struct northd_context *ctx) +sync_meters(struct northd_context *ctx, struct hmap *datapaths) { struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); @@ -11649,33 +11752,14 @@ sync_meters(struct northd_context *ctx) const struct nbrec_meter *nb_meter; NBREC_METER_FOR_EACH (nb_meter, ctx->ovnnb_idl) { - bool new_sb_meter = false; - - sb_meter = shash_find_and_delete(&sb_meters, nb_meter->name); - if (!sb_meter) { - sb_meter = sbrec_meter_insert(ctx->ovnsb_txn); - sbrec_meter_set_name(sb_meter, nb_meter->name); - new_sb_meter = true; - } - - if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { - struct sbrec_meter_band **sb_bands; - sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); - for (size_t i = 0; i < nb_meter->n_bands; i++) { - const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; - - sb_bands[i] = sbrec_meter_band_insert(ctx->ovnsb_txn); - - sbrec_meter_band_set_action(sb_bands[i], nb_band->action); - sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); - sbrec_meter_band_set_burst_size(sb_bands[i], - nb_band->burst_size); - } - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); - free(sb_bands); - } - - sbrec_meter_set_unit(sb_meter, nb_meter->unit); + sync_meters_iterate_nb_meter(ctx, nb_meter->name, nb_meter, + &sb_meters); + /* + * In addition to creating a 'non-fair' Meter in the SB from + * the line above, check and see if additional rows are needed + * to get ACLs logs individually rate-limited. + */ + sync_acl_fair_meters(ctx, datapaths, nb_meter, &sb_meters); } struct shash_node *node, *next; @@ -12168,7 +12252,7 @@ ovnnb_db_run(struct northd_context *ctx, sync_address_sets(ctx); sync_port_groups(ctx, &port_groups); - sync_meters(ctx); + sync_meters(ctx, datapaths); sync_dns_entries(ctx, datapaths); destroy_ovn_lbs(&lbs); hmap_destroy(&lbs); diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index 092322ab2..269e3a888 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "5.27.0", - "cksum": "3507518247 26773", + "version": "5.28.0", + "cksum": "610359755 26847", "tables": { "NB_Global": { "columns": { @@ -264,6 +264,7 @@ "refType": "strong"}, "min": 1, "max": "unlimited"}}, + "fair": {"type": {"key": "boolean", "min": 0, "max": 1}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, diff --git a/ovn-nb.xml b/ovn-nb.xml index 5e6c6c7a3..c49bb1332 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -1785,7 +1785,9 @@ The name of a meter to rate-limit log messages for the ACL. The string must match the column of a row in the table. By - default, log messages are not rate-limited. + default, log messages are not rate-limited. In order to ensure + that the same rate limits multiple ACL logs + separately, set the column.

@@ -2082,6 +2084,18 @@

+ +

+ This column is used to further describe the desired behavior + of the meter when there are multiple references to it. If this + column is empty or is set to false, the rate will + be shared across all rows that refer to the same Meter + . Conversely, when this column + is set to true, each user of the same Meter will be + rate-limited on its own. +

+ + See External IDs at the beginning of this document. diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 79d580d3f..11091d8d0 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -346,7 +346,7 @@ OVN_NBCTL_TEST([ovn_nbctl_meters], [meters], [ AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps]) AT_CHECK([ovn-nbctl meter-add meter2 drop 3 kbps 2]) AT_CHECK([ovn-nbctl meter-add meter3 drop 100 kbps 200]) -AT_CHECK([ovn-nbctl meter-add meter4 drop 10 pktps 30]) +AT_CHECK([ovn-nbctl --fair meter-add meter4 drop 10 pktps 30]) dnl Add duplicate meter name AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps], [1], [], [stderr]) @@ -382,7 +382,7 @@ meter2: bands: drop: 3 kbps, 2 kb burst meter3: bands: drop: 100 kbps, 200 kb burst -meter4: bands: +meter4: (fair) bands: drop: 10 pktps, 30 packet burst ]) @@ -393,7 +393,7 @@ meter1: bands: drop: 10 kbps meter3: bands: drop: 100 kbps, 200 kb burst -meter4: bands: +meter4: (fair) bands: drop: 10 pktps, 30 packet burst ]) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 7d73b0b83..21ce20c87 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1920,6 +1920,103 @@ sw1flows3: table=5 (ls_out_acl ), priority=2003 , match=((reg0[[9]] == AT_CLEANUP ]) +AT_SETUP([ovn-northd -- ACL fair Meters]) +AT_KEYWORDS([acl log meter fair]) +ovn_start + +ovn-nbctl ls-add sw0 +ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 "50:54:00:00:00:01 10.0.0.11" +ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2 "50:54:00:00:00:02 10.0.0.12" +ovn-nbctl lsp-add sw0 sw0-p3 -- lsp-set-addresses sw0-p3 "50:54:00:00:00:03 10.0.0.13" + +ovn-nbctl meter-add meter_me drop 1 pktps +nb_meter_uuid=$(fetch_column nb:Meter _uuid name=meter_me) + +ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.12' allow +ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.13' allow + +acl1=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.12' | head -1) +acl2=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.13' | head -1) +ovn-nbctl set acl $acl1 log=true severity=alert meter=meter_me name=acl_one +ovn-nbctl set acl $acl2 log=true severity=info meter=meter_me name=acl_two +ovn-nbctl --wait=sb sync + +check_row_count nb:meter 1 +check_column meter_me nb:meter name + +check_acl_lflow() { + acl_log_name=$1 + meter_name=$2 + # echo checking that logical flow for acl log $acl_log_name has $meter_name + AT_CHECK([ovn-sbctl lflow-list | grep ls_out_acl | \ + grep "\"${acl_log_name}\"" | \ + grep -c "meter=\"${meter_name}\""], [0], [1 +]) +} + +check_meter_by_name() { + [test "$1" = "NOT"] && { expected_count=0; shift; } || expected_count=1 + for meter_name in $* ; do + # echo checking for $expected_count $meter_name in sb meter table + check_row_count meter $expected_count name=$meter_name + done +} + +# Make sure 'fair' value properly affects the Meters in SB +check_meter_by_name meter_me +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} + +ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} + +ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=false +check_meter_by_name meter_me +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} + +ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} + +# Change template meter and make sure that is reflected on acl meters as well +template_band=$(fetch_column nb:meter bands name=meter_me) +ovn-nbctl --wait=sb set meter_band $template_band rate=123 +# Make sure that every Meter_Band has the right rate. (ovn-northd +# creates 3 identical Meter_Band rows, all identical; ovn-northd-ddlog +# creates just 1. It doesn't matter, they work just as well.) +n_meter_bands=$(count_rows meter_band) +AT_FAIL_IF([test "$n_meter_bands" != 1 && test "$n_meter_bands" != 3]) +check_row_count meter_band $n_meter_bands rate=123 + +# Check meter in logical flows for acl logs +check_acl_lflow acl_one meter_me__${acl1} +check_acl_lflow acl_two meter_me__${acl2} + +# Stop using meter for acl1 +ovn-nbctl --wait=sb clear acl $acl1 meter +check_meter_by_name meter_me meter_me__${acl2} +check_meter_by_name NOT meter_me__${acl1} +check_acl_lflow acl_two meter_me__${acl2} + +# Remove template Meter should remove all others as well +ovn-nbctl --wait=sb meter-del meter_me +check_row_count meter 0 +# Check that logical flow remains but uses non-unique meter since fair +# attribute is lost by the removal of the Meter row. +check_acl_lflow acl_two meter_me + +# Re-add template meter and make sure acl2's meter is back in sb +ovn-nbctl --wait=sb --fair meter-add meter_me drop 1 pktps +check_meter_by_name meter_me meter_me__${acl2} +check_meter_by_name NOT meter_me__${acl1} +check_acl_lflow acl_two meter_me__${acl2} + +# Remove acl2 +sw0=$(fetch_column nb:logical_switch _uuid name=sw0) +ovn-nbctl --wait=sb remove logical_switch $sw0 acls $acl2 +check_meter_by_name meter_me +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} + +AT_CLEANUP + OVN_FOR_EACH_NORTHD([ AT_SETUP([datapath requested-tnl-key]) AT_KEYWORDS([requested tnl tunnel key keys]) diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index b5633c725..11c6b5576 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -617,6 +617,7 @@ QoS commands:\n\ qos-list SWITCH print QoS rules for SWITCH\n\ \n\ Meter commands:\n\ + [--fair]\n\ meter-add NAME ACTION RATE UNIT [BURST]\n\ add a meter\n\ meter-del [NAME] remove meters\n\ @@ -2693,7 +2694,12 @@ nbctl_meter_list(struct ctl_context *ctx) for (size_t i = 0; i < n_meters; i++) { meter = meters[i]; - ds_put_format(&ctx->output, "%s: bands:\n", meter->name); + ds_put_format(&ctx->output, "%s:", meter->name); + if (meter->fair) { + ds_put_format(&ctx->output, " (%s)", + *meter->fair ? "fair" : "shared"); + } + ds_put_format(&ctx->output, " bands:\n"); for (size_t j = 0; j < meter->n_bands; j++) { const struct nbrec_meter_band *band = meter->bands[j]; @@ -2770,6 +2776,12 @@ nbctl_meter_add(struct ctl_context *ctx) nbrec_meter_set_name(meter, name); nbrec_meter_set_unit(meter, unit); nbrec_meter_set_bands(meter, &band, 1); + + /* Fair option */ + bool fair = shash_find(&ctx->options, "--fair") != NULL; + if (fair) { + nbrec_meter_set_fair(meter, &fair, 1); + } } static void @@ -6464,7 +6476,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { /* meter commands. */ { "meter-add", 4, 5, "NAME ACTION RATE UNIT [BURST]", NULL, - nbctl_meter_add, NULL, "", RW }, + nbctl_meter_add, NULL, "--fair", RW }, { "meter-del", 0, 1, "[NAME]", NULL, nbctl_meter_del, NULL, "", RW }, { "meter-list", 0, 0, "", NULL, nbctl_meter_list, NULL, "", RO }, From patchwork Mon Nov 16 15:49:57 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Flaviof X-Patchwork-Id: 1401001 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.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=flaviof.com Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4CZYSV6WJ3z9sRK for ; Tue, 17 Nov 2020 02:50:34 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id B776D86794; Mon, 16 Nov 2020 15:50:32 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kBn-2ApZQmIu; Mon, 16 Nov 2020 15:50:17 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 2DBD28672B; Mon, 16 Nov 2020 15:50:17 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id F3984C1833; Mon, 16 Nov 2020 15:50:16 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 85417C0891 for ; Mon, 16 Nov 2020 15:50:15 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 72220867F0 for ; Mon, 16 Nov 2020 15:50:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1geSKm9nYm8h for ; Mon, 16 Nov 2020 15:50:11 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by whitealder.osuosl.org (Postfix) with ESMTPS id A28E2866AE for ; Mon, 16 Nov 2020 15:50:11 +0000 (UTC) Received: by mail-qt1-f173.google.com with SMTP id b16so12925141qtb.6 for ; Mon, 16 Nov 2020 07:50:11 -0800 (PST) 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:in-reply-to :references; bh=ONtdo3ybWOmVSEpD7UW/A8DIEsC6n4E1WUY2nTszMqI=; b=AIZcFRv1SRTTsdcFGTSA9sRkYoqy3Oi3qdTkRZJaYKUGqd2C+u9xMa1qzkgeqD6SJa sJ0lJ2TBJ3SLUuSlDJufjasMVl49eSpxNMG3efryAHO1JxPlcisyxAlbktKga1xw6D9J qc/WJ2ooj8PgtgXGTyCIgKZo80F/MkywsZ/NIRPL2/FRqzOwY1kvewO4fGc9N7kWBpTu CnMWoArcu8gKG32TO5HKGsx93RYFl9xPxO7GqXIAs21h8/5b0HvzIcdefXQb1BGL8S/x CAoDq8HCh/3TbjgW2+/rTsjy3lMTi8MpUbtQimc3u3Fnm9KBenxMLBbkyZ8C9gk+B3mo /QVQ== X-Gm-Message-State: AOAM532fRJtPxPqtw1gQtDcQZQ83uhNx0hlgnqYOT2YcPr4mHxx1BrFN VyX3NJ9P2DSjGlkkoRBRvC/N+7TX26xIyw== X-Google-Smtp-Source: ABdhPJwLYeg7NObbC9o8pFlCIxsOiTWDYd5BLMVppmc7p0E+YZY74Q1NuNemSHSn8aW5C58rbUzorA== X-Received: by 2002:ac8:65d3:: with SMTP id t19mr5046682qto.28.1605541809956; Mon, 16 Nov 2020 07:50:09 -0800 (PST) Received: from c8vm.redhat.com (pool-173-76-170-96.bstnma.fios.verizon.net. [173.76.170.96]) by smtp.gmail.com with ESMTPSA id y4sm11960196qtd.24.2020.11.16.07.50.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Nov 2020 07:50:09 -0800 (PST) From: Flavio Fernandes To: dev@openvswitch.org Date: Mon, 16 Nov 2020 15:49:57 +0000 Message-Id: <20201116154957.2443-3-flavio@flaviof.com> X-Mailer: git-send-email 2.18.4 In-Reply-To: <20201116154957.2443-1-flavio@flaviof.com> References: <20201116154957.2443-1-flavio@flaviof.com> Subject: [ovs-dev] [PATCH v5 ovn 2/2] northd: Fair ACL log meters. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" ddlog for ACL log meters. Implement fair meters for acl logs in ovn_northd.dl. Enable fair Meter test to also exercise ovn-northd-ddlog. This is a small variation of blp's implementation of ddlog for ACL log meters, now using the northbound schema changes [1]. The changes address overall design issues mentioned in that link. [1]: https://patchwork.ozlabs.org/project/ovn/patch/20201107213913.GC2753733@ovn.org/ Co-authored-by: Ben Pfaff Signed-off-by: Flavio Fernandes --- northd/ovn_northd.dl | 281 ++++++++++++++++++++++++------------------- tests/ovn-northd.at | 2 + 2 files changed, 161 insertions(+), 122 deletions(-) diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index 3fbe67b31..46489e4cb 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -27,6 +27,23 @@ output relation Warning[string] index Logical_Flow_Index() on sb::Out_Logical_Flow() +/* Single-row relation that contains the set of meters marked as fair. */ +relation AclFairLogMeters[Set] +AclFairLogMeters[meters] :- AclFairLogMeters0[meters]. +AclFairLogMeters[set_empty()] :- + Unit(), + not AclFairLogMeters0[_]. + +relation AclFairLogMeters0[Set] +AclFairLogMeters0[meters] :- + meter in nb::Meter(.fair = Some{fair}), + fair, + var meters = { + var meters = set_empty(); + set_insert(meters, meter.name); + meters + }. + /* Meter_Band table */ for (mb in nb::Meter_Band) { sb::Out_Meter_Band(._uuid = mb._uuid, @@ -42,6 +59,14 @@ for (meter in nb::Meter) { .unit = meter.unit, .bands = meter.bands) } +sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid), + .name = "${name}__${uuid2str(acl_uuid)}", + .unit = unit, + .bands = bands) :- + AclFairLogMeters[names], + var name = FlatMap(names), + nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands), + nb::ACL(._uuid = acl_uuid, .meter = Some{name}). /* Proxy table for Out_Datapath_Binding: contains all Datapath_Binding fields, * except tunnel id, which is allocated separately (see TunKeyAllocation). */ @@ -2017,7 +2042,7 @@ for (&Switch(.ls = ls)) { .external_ids = map_empty()) } -function build_acl_log(acl: nb::ACL): string = +function build_acl_log(acl: nb::ACL, fair_meters: Set): string = { if (not acl.log) { "" @@ -2049,7 +2074,14 @@ function build_acl_log(acl: nb::ACL): string = }; match (acl.meter) { None -> (), - Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}") + Some{meter} -> { + var s = if (fair_meters.contains(meter)) { + "${meter}__${uuid2str(acl._uuid)}" + } else { + meter + }; + vec_push(strs, "meter=${json_string_escape(s)}") + } }; "log(${string_join(strs, \", \")}); " } @@ -2067,31 +2099,33 @@ function oVN_ACL_PRI_OFFSET(): integer = 1000 relation Reject(lsuuid: uuid, pipeline: string, stage: Stage, acl: nb::ACL, extra_match: string, extra_actions: string) /* build_reject_acl_rules() */ -for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) { - var extra_match = match (extra_match_) { - "" -> "", - s -> "(${s}) && " - } in - var extra_actions = match (extra_actions_) { - "" -> "", - s -> "${s} " - } in - var next = match (pipeline == "ingress") { - true -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, QOS_MARK)).0})", - false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, L2_LKUP)).0})" - } in - var acl_log = build_acl_log(acl) in { - var __match = extra_match ++ acl.__match in - var actions = acl_log ++ extra_actions ++ "reg0 = 0; " - "reject { " - "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ " - "outport <-> inport; ${next}; };" in - Flow(.logical_datapath = lsuuid, - .stage = stage, - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = __match, - .actions = actions, - .external_ids = stage_hint(acl._uuid)) +for (AclFairLogMeters[fair_meters]) { + for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) { + var extra_match = match (extra_match_) { + "" -> "", + s -> "(${s}) && " + } in + var extra_actions = match (extra_actions_) { + "" -> "", + s -> "${s} " + } in + var next = match (pipeline == "ingress") { + true -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, QOS_MARK)).0})", + false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, L2_LKUP)).0})" + } in + var acl_log = build_acl_log(acl, fair_meters) in { + var __match = extra_match ++ acl.__match in + var actions = acl_log ++ extra_actions ++ "reg0 = 0; " + "reject { " + "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ " + "outport <-> inport; ${next}; };" in + Flow(.logical_datapath = lsuuid, + .stage = stage, + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = __match, + .actions = actions, + .external_ids = stage_hint(acl._uuid)) + } } } @@ -2347,114 +2381,117 @@ for (&Switch(.ls = ls)) { } /* Ingress or Egress ACL Table (Various priorities). */ -for (&SwitchACL(.sw = &Switch{.ls = ls, .has_stateful_acl = has_stateful}, .acl = &acl)) { - /* consider_acl */ - var ingress = acl.direction == "from-lport" in - var stage = if (ingress) { switch_stage(IN, ACL) } else { switch_stage(OUT, ACL) } in - var pipeline = if ingress "ingress" else "egress" in - var stage_hint = stage_hint(acl._uuid) in - if (acl.action == "allow" or acl.action == "allow-related") { - /* If there are any stateful flows, we must even commit "allow" - * actions. This is because, while the initiater's - * direction may not have any stateful rules, the server's - * may and then its return traffic would not have an - * associated conntrack entry and would return "+invalid". */ - if (not has_stateful) { - Flow(.logical_datapath = ls._uuid, - .stage = stage, - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = acl.__match, - .actions = "${build_acl_log(acl)}next;", - .external_ids = stage_hint) - } else { - /* Commit the connection tracking entry if it's a new - * connection that matches this ACL. After this commit, - * the reply traffic is allowed by a flow we create at - * priority 65535, defined earlier. - * - * It's also possible that a known connection was marked for - * deletion after a policy was deleted, but the policy was - * re-added while that connection is still known. We catch - * that case here and un-set ct_label.blocked (which will be done - * by ct_commit in the "stateful" stage) to indicate that the - * connection should be allowed to resume. - */ - Flow(.logical_datapath = ls._uuid, - .stage = stage, - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", - .actions = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${build_acl_log(acl)}next;", - .external_ids = stage_hint); - - /* Match on traffic in the request direction for an established - * connection tracking entry that has not been marked for - * deletion. There is no need to commit here, so we can just - * proceed to the next table. We use this to ensure that this - * connection is still allowed by the currently defined - * policy. Match untracked packets too. */ - Flow(.logical_datapath = ls._uuid, - .stage = stage, - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", - .actions = "${build_acl_log(acl)}next;", - .external_ids = stage_hint) - } - } else if (acl.action == "drop" or acl.action == "reject") { - /* The implementation of "drop" differs if stateful ACLs are in - * use for this datapath. In that case, the actions differ - * depending on whether the connection was previously committed - * to the connection tracker with ct_commit. */ - if (has_stateful) { - /* If the packet is not tracked or not part of an established - * connection, then we can simply reject/drop it. */ - var __match = "${rEGBIT_ACL_HINT_DROP()} == 1" in - if (acl.action == "reject") { - Reject(ls._uuid, pipeline, stage, acl, __match, "") - } else { +for (AclFairLogMeters[fair_meters]) { + for (&SwitchACL(.sw = &Switch{.ls = ls, .has_stateful_acl = has_stateful}, .acl = &acl)) { + /* consider_acl */ + var ingress = acl.direction == "from-lport" in + var stage = if (ingress) { switch_stage(IN, ACL) } else { switch_stage(OUT, ACL) } in + var pipeline = if ingress "ingress" else "egress" in + var stage_hint = stage_hint(acl._uuid) in + var log_acl = build_acl_log(acl, fair_meters) in + if (acl.action == "allow" or acl.action == "allow-related") { + /* If there are any stateful flows, we must even commit "allow" + * actions. This is because, while the initiater's + * direction may not have any stateful rules, the server's + * may and then its return traffic would not have an + * associated conntrack entry and would return "+invalid". */ + if (not has_stateful) { Flow(.logical_datapath = ls._uuid, .stage = stage, .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = __match ++ " && (${acl.__match})", - .actions = "${build_acl_log(acl)}/* drop */", + .__match = acl.__match, + .actions = "${log_acl}next;", .external_ids = stage_hint) - }; - /* For an existing connection without ct_label set, we've - * encountered a policy change. ACLs previously allowed - * this connection and we committed the connection tracking - * entry. Current policy says that we should drop this - * connection. First, we set bit 0 of ct_label to indicate - * that this connection is set for deletion. By not - * specifying "next;", we implicitly drop the packet after - * updating conntrack state. We would normally defer - * ct_commit() to the "stateful" stage, but since we're - * rejecting/dropping the packet, we go ahead and do it here. - */ - var __match = "${rEGBIT_ACL_HINT_BLOCK()} == 1" in - var actions = "ct_commit { ct_label.blocked = 1; }; " in - if (acl.action == "reject") { - Reject(ls._uuid, pipeline, stage, acl, __match, actions) } else { + /* Commit the connection tracking entry if it's a new + * connection that matches this ACL. After this commit, + * the reply traffic is allowed by a flow we create at + * priority 65535, defined earlier. + * + * It's also possible that a known connection was marked for + * deletion after a policy was deleted, but the policy was + * re-added while that connection is still known. We catch + * that case here and un-set ct_label.blocked (which will be done + * by ct_commit in the "stateful" stage) to indicate that the + * connection should be allowed to resume. + */ Flow(.logical_datapath = ls._uuid, .stage = stage, .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = __match ++ " && (${acl.__match})", - .actions = "${actions}${build_acl_log(acl)}/* drop */", - .external_ids = stage_hint) - } - } else { - /* There are no stateful ACLs in use on this datapath, - * so a "reject/drop" ACL is simply the "reject/drop" - * logical flow action in all cases. */ - if (acl.action == "reject") { - Reject(ls._uuid, pipeline, stage, acl, "", "") - } else { + .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", + .actions = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${log_acl}next;", + .external_ids = stage_hint); + + /* Match on traffic in the request direction for an established + * connection tracking entry that has not been marked for + * deletion. There is no need to commit here, so we can just + * proceed to the next table. We use this to ensure that this + * connection is still allowed by the currently defined + * policy. Match untracked packets too. */ Flow(.logical_datapath = ls._uuid, .stage = stage, .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = acl.__match, - .actions = "${build_acl_log(acl)}/* drop */", + .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", + .actions = "${log_acl}next;", .external_ids = stage_hint) } + } else if (acl.action == "drop" or acl.action == "reject") { + /* The implementation of "drop" differs if stateful ACLs are in + * use for this datapath. In that case, the actions differ + * depending on whether the connection was previously committed + * to the connection tracker with ct_commit. */ + if (has_stateful) { + /* If the packet is not tracked or not part of an established + * connection, then we can simply reject/drop it. */ + var __match = "${rEGBIT_ACL_HINT_DROP()} == 1" in + if (acl.action == "reject") { + Reject(ls._uuid, pipeline, stage, acl, __match, "") + } else { + Flow(.logical_datapath = ls._uuid, + .stage = stage, + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = __match ++ " && (${acl.__match})", + .actions = "${log_acl}/* drop */", + .external_ids = stage_hint) + }; + /* For an existing connection without ct_label set, we've + * encountered a policy change. ACLs previously allowed + * this connection and we committed the connection tracking + * entry. Current policy says that we should drop this + * connection. First, we set bit 0 of ct_label to indicate + * that this connection is set for deletion. By not + * specifying "next;", we implicitly drop the packet after + * updating conntrack state. We would normally defer + * ct_commit() to the "stateful" stage, but since we're + * rejecting/dropping the packet, we go ahead and do it here. + */ + var __match = "${rEGBIT_ACL_HINT_BLOCK()} == 1" in + var actions = "ct_commit { ct_label.blocked = 1; }; " in + if (acl.action == "reject") { + Reject(ls._uuid, pipeline, stage, acl, __match, actions) + } else { + Flow(.logical_datapath = ls._uuid, + .stage = stage, + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = __match ++ " && (${acl.__match})", + .actions = "${actions}${log_acl}/* drop */", + .external_ids = stage_hint) + } + } else { + /* There are no stateful ACLs in use on this datapath, + * so a "reject/drop" ACL is simply the "reject/drop" + * logical flow action in all cases. */ + if (acl.action == "reject") { + Reject(ls._uuid, pipeline, stage, acl, "", "") + } else { + Flow(.logical_datapath = ls._uuid, + .stage = stage, + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = acl.__match, + .actions = "${log_acl}/* drop */", + .external_ids = stage_hint) + } + } } } } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 21ce20c87..ee17b13e2 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1920,6 +1920,7 @@ sw1flows3: table=5 (ls_out_acl ), priority=2003 , match=((reg0[[9]] == AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn-northd -- ACL fair Meters]) AT_KEYWORDS([acl log meter fair]) ovn_start @@ -2016,6 +2017,7 @@ check_meter_by_name meter_me check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} AT_CLEANUP +]) OVN_FOR_EACH_NORTHD([ AT_SETUP([datapath requested-tnl-key])