[ovs-dev] [PATCH v5 ovn 2/2] northd: Fair ACL log meters.

Flavio Fernandes flavio at flaviof.com
Mon Nov 16 15:49:57 UTC 2020


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 <blp at ovn.org>
Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
---
 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<string>]
+AclFairLogMeters[meters] :- AclFairLogMeters0[meters].
+AclFairLogMeters[set_empty()] :-
+    Unit(),
+    not AclFairLogMeters0[_].
+
+relation AclFairLogMeters0[Set<string>]
+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>): 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])
-- 
2.18.4



More information about the dev mailing list