[ovs-dev] ddlog for ACL log meters (was: [PATCH v3 ovn 1/1] northd: Enhance the implementation of ACL log meters.)
Flavio Fernandes
flavio at flaviof.com
Mon Nov 9 14:53:16 UTC 2020
[inline]
> On Nov 7, 2020, at 4:39 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> On Tue, Nov 03, 2020 at 10:18:34PM +0000, Flavio Fernandes wrote:
>> 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 an option in NB_Global that allows for a
>> Meter to rate-limit each ACL log separately.
>>
>> The change is backward compatible. Based on a well known option, 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 this option:
>>
>> ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"
>>
>> If more than one Meter is wanted, simply use a comma to separate each name.
>>
>> Reported-by: Flavio Fernandes <flavio at flaviof.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
>> Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
>
> I'm not sure I like the overall design here. This option isn't going to
> scale very well to many of these meters, since they'd all need to be
> shoved into a single comma-separated list. Another way might be to add
> a column to the ACL table to mark a meter as shared or private. Or
> there could be a name convention, e.g. prefix with "shared_" to make it
> shared.
Understood. I think I tried "too hard" to avoid making changes to the northbound schema, but
you bring a valid concern about scalability. Adding a "bool" to the ACL row, or to the Meter row
would definitely make this more scalable. I would like to ask for opinion from you on the following
choices (not listed in any order). You == Ben and the rest of the ovn core developers and users.
A) add a bool column to ACL row called "fair_log_meter" (please help me with the name);
B) add a bool column to Meter row called ^same as above^;
C) change parsing of the value of the global to allow for up to one Meter name
D) change parsing of the value of the global to allow for up to a constant Meter names
E) have an implict behavior based on the name of the Meter "shared_", so that multiple meters are created in the SB as needed.
F) same as 'E', but use a different prefix str
G) any other good approach?
>
> I don't really understand the vocabulary here, either. I would have
> guessed that "shared" would mean that all the ACLs with a given meter
> name share a single southbound Meter, but it's the opposite: a "shared"
> meter has a different southbound Meter for each ACL.
Yes, good point. I was seeing this change mostly from the Northbound perspective,
but you are right that this is a confusing name looking at what happens at the SB.
How about we use the word "fair" instead? So, something like "acl_fair_log_meters".
Any suggestions on a better name are very welcome.
>
> OK, all that is quibbling. You asked me to help with the ddlog part. I
> did that, and let me explain it.
I am super grateful for the help below. I will defer going over that in this
message to focus on the topics above. Regardless, this is super useful for
folks [like me] to ramp up on ddlog-northd.
Best,
-- flaviof
>
> I'll start with a way that the ddlog implementation I built differs from
> the C implementation. That's in the southbound Meter_Band table. The C
> implementation creates Meter_Band rows per meter instance; if a "shared"
> meter has 3 instances, it'll create 3 copies of the Meter_Band rows.
> There's nothing valuable about the extra instances, since they are all
> the same, and ovn-controller only looks at the values in the rows and
> doesn't care that they're different rows. It's slightly easier not to
> create the extra instances in the ddlog implementation, so I just made
> it create one. So this existing code in ovn_northd.dl can stay:
>
> /* Meter_Band table */
> for (mb in nb::Meter_Band) {
> sb::Out_Meter_Band(._uuid = mb._uuid,
> .action = mb.action,
> .rate = mb.rate,
> .burst_size = mb.burst_size)
> }
>
> The new test does check that there are three (identical) Meter_Band
> rows. I updated the test to be more flexible, so that it expects 1 or 3
> rows all with the correct rate:
>
> # 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
>
> OK, now for what does need to change in the DDlog code. First, we need
> to extract the set of shared meters from NB_Global. There's a few ways
> we could do that. One way would be to put them into a relation, one
> shared meter name per row. Then later on we could do joins against that
> relation to figure out whether a meter was shared. That would be the
> best way, probably, if there might be hundreds or thousands of these
> things. I guess there won't be, though, since they're all going to be
> glommed into a single comma-separated value. Instead, I decided that it
> would be easier to have a relation with just a single row with a single
> column, typed as "set of string". It's easy to do that:
>
> relation AclSharedLogMeters0[Set<string>]
> AclSharedLogMeters0[meters] :-
> nb in nb::NB_Global(),
> Some{var s} = nb.options.get("acl_shared_log_meters"),
> var meters = s.split(",").to_set().
>
> Note the use of [] instead of () for this relation. () would be if I
> wanted the relation to contain rows whose values are structs with named
> columns, which is kind of the most common case, but I just want it to
> have a single unnamed value so this is slightly easier (I don't have to
> invent a name for the column).
>
> We're almost there for the set of shared meters. There is, however, the
> possibility that NB_Global.options doesn't have an
> "acl_shared_log_meters" key in it. In that case, the relation I just
> invented would be empty, so the joins I want to do on it later would
> fail to work the way I want. What I want in that case is for the
> relation to have a single row whose value is the empty set. The easiest
> way to get that is to bootstrap another relation off of the one I just
> made, like this:
>
> relation AclSharedLogMeters[Set<string>]
> AclSharedLogMeters[meters] :- AclSharedLogMeters0[meters].
> AclSharedLogMeters[set_empty()] :-
> Unit(),
> not AclSharedLogMeters0[_].
>
> This says that, if AclSharedLogMeters0 has a value, then so does
> AclSharedLogMeters. And if AclSharedLogMeters0 has no values, then
> AclSharedLogMeters has row with the empty set in it. (Unit() is here
> because DDlog doesn't allow a negative ("not") clause as the first
> clause in a production. We're talking about getting rid of the need for
> that sometime, since this kind of construction is a common idiom in
> DDlog.)
>
> Now we can use the set of shared meters. We'll need to pass it into the
> DDlog build_acl_log() function. We'll add it as a parameter to that
> function:
>
> -function build_acl_log(acl: nb::ACL): string =
> +function build_acl_log(acl: nb::ACL, shared_meters: Set<string>): string =
>
> and then use it inside the function:
>
> match (acl.meter) {
> None -> (),
> - Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}")
> + Some{meter} -> {
> + var s = if (shared_meters.contains(meter)) {
> + "${meter}__${uuid2str(acl._uuid)}"
> + } else {
> + meter
> + };
> + vec_push(strs, "meter=${json_string_escape(s)}")
> + }
> };
>
> and then pass in the value from each caller by adding a join against
> AclSharedLogMeters. Here's one example (ignoring indentation changes):
>
> +for (AclSharedLogMeters[shared_meters]) {
> for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
> ...
> - var acl_log = build_acl_log(acl) in {
> + var acl_log = build_acl_log(acl, shared_meters) in {
> ...
> }
> }
> +}
>
> Finally, we need to create new southbound Meter records for the "shared"
> meters. I'm actually a bit confused about this. I would have thought
> that "shared" meters wouldn't need the Meter records whose names are
> un-suffixed by __<uuid>, but the C implementation appears to always
> create them. So I left in place the existing DDlog code that just
> copies from nb::Meter to sb::Out_Meter:
>
> /* Meter table */
> for (meter in nb::Meter) {
> sb::Out_Meter(._uuid = meter._uuid,
> .name = meter.name,
> .unit = meter.unit,
> .bands = meter.bands)
> }
>
> and just added another production that generates the suffixed ones:
>
> sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
> .name = "${name}__${uuid2str(acl_uuid)}",
> .unit = unit,
> .bands = bands) :-
> AclSharedLogMeters[names],
> var name = FlatMap(names),
> nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
> nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
>
> The above might need some explanation for those relatively new to
> Datalog or DDlog. First, the style. This uses Datalog-style syntax
> (output :- input) instead of the FLWOR-style syntax (for (input) {
> output }). One reason for that is because the FLWOR-style syntax
> currently doesn't support FlatMap, which we need in that case. Maybe
> DDlog will add support for it later (we've talked about it). Another
> reason is that, after working with DDlog for some months, I've come to
> find the Datalog-style syntax better for most purposes than the FLWOR
> syntax (it doesn't result in cascading indentations, for instance).
>
> Anyway, how do we read this? I usually start by skipping to the clauses
> after the :-. Each of these is sort of equivalent to a for loop. You
> can read them like this:
>
> AclSharedLogMeters[names],
>
> "For every row in AclSharedLogMeters, and call its value 'names'".
> By design, there is exactly one row in AclSharedLogMeters, so this
> just grabs its value as 'names'. Its type is "set of string".
>
> var name = FlatMap(names),
>
> "For every value in 'names', and call the value 'name'". This means
> that everything after this is repeated for every string in 'names'.
> (It also unbinds 'names'; you can't use that again after this
> point.) If 'names' was empty (i.e. if there were no shared ACL
> meters), then it's an iteration over 0 elements, and we're done.
>
> nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
>
> "For every row in the northbound Meter table whose 'name' column
> equals 'name'; also, call the meter's _uuid column 'meter_uuid' (and
> so on)". This is a join against the northbound Meter table on
> column 'name'.
>
> This syntax can be confusing at first because the .<x> = <y> syntax
> has two different meanings. For _uuid, unit, and bands, it names
> the value in the column. For name, it performs a join against the
> existing value of name. If that's confusing, consider a different
> situation where we wanted to join against, not 'name' itself, but
> "shared_<name>". We could write this as:
>
> nb::Meter(._uuid = meter_uuid,
> .name = "shared_" ++ name,
> .unit = unit,
> .bands = bands),
>
> On the other hand, we could not write expressions here in terms of
> meter_uuid or unit or bands, because none of them has been bound to
> a value yet; this join is what binds them.
>
> (Yes, this is confusing. It's inherited from Datalog. Maybe we
> will invent better syntax someday.)
>
> Here's another way we could have written this, using an expression
> to make the join really explicit:
>
> nb::Meter(._uuid = meter_uuid,
> .name = meter_name,
> .unit = unit,
> .bands = bands),
> meter_name == name,
>
> nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
>
> "For every row in the northbound ACL table, where the 'meter' column
> has 'name' in it, call the _uuid column's value 'acl_uuid'". This
> is a join against the northbound ACL table on the 'meter' column.
> The 'meter' column has type Option<string>, where Option is defined
> in differential-datalog/lib/ddlog_std.dl as:
>
> typedef Option<'A> = None
> | Some{x: 'A}
>
> OK, so the above describes the join we're doing, which amounts to
> finding every ACL that names a shared meter. Then we skip back up to
> the part of the production before the :-. It says:
>
> sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
> .name = "${name}__${uuid2str(acl_uuid)}",
> .unit = unit,
> .bands = bands) :-
>
> This produces a row in the southbound output ("Out_") Meter table. We
> have to construct the _uuid ourselves in some unique way; a hash of the
> meter and the ACL seems reasonable. We also construct the name
> according to the same algorithm used in the C version. (My first
> attempt at this omitted the call to uuid2str(), which was wrong because,
> in the DDlog version, UUIDs are just 128-bit integers, which format as
> decimal integers by default. There's nothing really wrong with that
> except that the test expected them to be formatted in the canonical way
> for UUIDs.)
>
> Here's the incremental patch to add ddlog support. It is bigger than
> one might expect because of the increased indentation in a couple of
> blocks in ovn_northd.dl. After that, the revised patch overall.
>
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 23e140148c35..135ff559520b 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -27,6 +27,19 @@ output relation Warning[string]
>
> index Logical_Flow_Index() on sb::Out_Logical_Flow()
>
> +/* Single-row relation that contains the set of shared meters. */
> +relation AclSharedLogMeters[Set<string>]
> +AclSharedLogMeters[meters] :- AclSharedLogMeters0[meters].
> +AclSharedLogMeters[set_empty()] :-
> + Unit(),
> + not AclSharedLogMeters0[_].
> +
> +relation AclSharedLogMeters0[Set<string>]
> +AclSharedLogMeters0[meters] :-
> + nb in nb::NB_Global(),
> + Some{var s} = nb.options.get("acl_shared_log_meters"),
> + var meters = s.split(",").to_set().
> +
> /* Meter_Band table */
> for (mb in nb::Meter_Band) {
> sb::Out_Meter_Band(._uuid = mb._uuid,
> @@ -42,6 +55,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) :-
> + AclSharedLogMeters[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). */
> @@ -2008,7 +2029,7 @@ for (&Switch(.ls = ls)) {
> .external_ids = map_empty())
> }
>
> -function build_acl_log(acl: nb::ACL): string =
> +function build_acl_log(acl: nb::ACL, shared_meters: Set<string>): string =
> {
> if (not acl.log) {
> ""
> @@ -2040,7 +2061,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 (shared_meters.contains(meter)) {
> + "${meter}__${uuid2str(acl._uuid)}"
> + } else {
> + meter
> + };
> + vec_push(strs, "meter=${json_string_escape(s)}")
> + }
> };
> "log(${string_join(strs, \", \")}); "
> }
> @@ -2058,31 +2086,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 (AclSharedLogMeters[shared_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, shared_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))
> + }
> }
> }
>
> @@ -2338,114 +2368,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 (AclSharedLogMeters[shared_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, shared_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 590d00c105e9..778482741b10 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1981,7 +1981,12 @@ done
> # Change template meter and make sure that is reflected on acl meters as well
> template_band=$(ovn-nbctl --bare --column=bands find meter name=meter_me)
> ovn-nbctl --wait=sb set meter_band $template_band rate=123
> -check_row_count meter_band 3 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}
>
> -8<--------------------------cut here-------------------------->8--
>
> From: Flavio Fernandes <flavio at flaviof.com>
> Date: Tue, 3 Nov 2020 22:18:34 +0000
> Subject: [PATCH ovn] northd: Enhance the implementation of ACL log meters.
>
> 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 an option in NB_Global that allows for a
> Meter to rate-limit each ACL log separately.
>
> The change is backward compatible. Based on a well known option, 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 this option:
>
> ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"
>
> If more than one Meter is wanted, simply use a comma to separate each name.
>
> Reported-by: Flavio Fernandes <flavio at flaviof.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
> Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> northd/ovn-northd.c | 201 +++++++++++++++++++++++--------
> northd/ovn_northd.dl | 277 ++++++++++++++++++++++++-------------------
> ovn-nb.xml | 14 +++
> tests/ovn-northd.at | 99 ++++++++++++++++
> 4 files changed, 417 insertions(+), 174 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 684c2bd478e5..2886d2f2d292 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5356,8 +5356,53 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
> }
> }
>
> +static bool
> +is_a_shared_meter(const struct smap *nb_options, const char *meter_name)
> +{
> + bool is_shared_meter = false;
> + const char *meters = smap_get(nb_options, "acl_shared_log_meters");
> + if (meters && meters[0]) {
> + char *cur, *next, *start;
> + next = start = xstrdup(meters);
> + while ((cur = strsep(&next, ",")) && *cur) {
> + if (!strcmp(meter_name, cur)) {
> + is_shared_meter = true;
> + break;
> + }
> + }
> + free(start);
> + }
> + return is_shared_meter;
> +}
> +
> +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 smap *nb_options)
> +{
> + if (!acl->meter) {
> + return;
> + }
> +
> + /* If ACL log meter uses a shared meter, use unique Meter name. */
> + if (is_a_shared_meter(nb_options, 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 smap *nb_options)
> {
> if (!acl->log) {
> return;
> @@ -5385,9 +5430,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, nb_options);
>
> ds_chomp(actions, ' ');
> ds_chomp(actions, ',');
> @@ -5398,7 +5441,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 smap *nb_options)
> {
> struct ds match = DS_EMPTY_INITIALIZER;
> struct ds actions = DS_EMPTY_INITIALIZER;
> @@ -5410,7 +5454,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, nb_options);
> if (extra_match->length > 0) {
> ds_put_format(&match, "(%s) && ", extra_match->string);
> }
> @@ -5435,7 +5479,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 smap *nb_options)
> {
> bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
> enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL;
> @@ -5449,7 +5494,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, nb_options);
> ds_put_cstr(&actions, "next;");
> ovn_lflow_add_with_hint(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5475,7 +5520,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, nb_options);
> ds_put_cstr(&actions, "next;");
> ovn_lflow_add_with_hint(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5494,7 +5539,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, nb_options);
> ds_put_cstr(&actions, "next;");
> ovn_lflow_add_with_hint(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5519,10 +5564,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_, nb_options);
> } else {
> ds_put_format(&match, " && (%s)", acl->match);
> - build_acl_log(&actions, acl);
> + build_acl_log(&actions, acl, nb_options);
> ds_put_cstr(&actions, "/* drop */");
> ovn_lflow_add_with_hint(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5546,10 +5591,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_, nb_options);
> } else {
> ds_put_format(&match, " && (%s)", acl->match);
> - build_acl_log(&actions, acl);
> + build_acl_log(&actions, acl, nb_options);
> ds_put_cstr(&actions, "/* drop */");
> ovn_lflow_add_with_hint(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5562,9 +5607,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_, nb_options);
> } else {
> - build_acl_log(&actions, acl);
> + build_acl_log(&actions, acl, nb_options);
> ds_put_cstr(&actions, "/* drop */");
> ovn_lflow_add_with_hint(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5644,7 +5689,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 smap *nb_options)
> {
> bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od));
>
> @@ -5747,13 +5792,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, nb_options);
> }
> 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,
> + nb_options);
> }
> }
> }
> @@ -6776,7 +6822,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
> struct hmap *port_groups, struct hmap *lflows,
> struct hmap *mcgroups, struct hmap *igmp_groups,
> struct shash *meter_groups,
> - struct hmap *lbs)
> + struct hmap *lbs, const struct smap *nb_options)
> {
> /* This flow table structure is documented in ovn-northd(8), so please
> * update ovn-northd.8.xml if you change anything. */
> @@ -6796,7 +6842,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
> 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, nb_options);
> build_qos(od, lflows);
> build_lb(od, lflows);
> build_stateful(od, lflows, lbs);
> @@ -11379,8 +11425,12 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> {
> struct hmap lflows = HMAP_INITIALIZER(&lflows);
>
> + const struct nbrec_nb_global *nb_global =
> + nbrec_nb_global_first(ctx->ovnnb_idl);
> + ovs_assert(nb_global);
> +
> build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups,
> - igmp_groups, meter_groups, lbs);
> + igmp_groups, meter_groups, lbs, &nb_global->options);
> build_lrouter_flows(datapaths, ports, &lflows, meter_groups, lbs);
>
> /* Push changes to the Logical_Flow table to database. */
> @@ -11706,15 +11756,85 @@ 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_shared_meters(struct northd_context *ctx,
> + struct hmap *datapaths,
> + const struct smap *nb_options,
> + const struct nbrec_meter *nb_meter,
> + struct shash *sb_meters)
> +{
> + if (!is_a_shared_meter(nb_options, nb_meter->name)) {
> + 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 shared 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);
>
> + const struct nbrec_nb_global *nb_global =
> + nbrec_nb_global_first(ctx->ovnnb_idl);
> + ovs_assert(nb_global);
> +
> const struct sbrec_meter *sb_meter;
> SBREC_METER_FOR_EACH (sb_meter, ctx->ovnsb_idl) {
> shash_add(&sb_meters, sb_meter->name, sb_meter);
> @@ -11722,33 +11842,10 @@ 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);
> + sync_acl_shared_meters(ctx, datapaths, &nb_global->options, nb_meter,
> + &sb_meters);
> }
>
> struct shash_node *node, *next;
> @@ -12241,7 +12338,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/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 23e140148c35..135ff559520b 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -27,6 +27,19 @@ output relation Warning[string]
>
> index Logical_Flow_Index() on sb::Out_Logical_Flow()
>
> +/* Single-row relation that contains the set of shared meters. */
> +relation AclSharedLogMeters[Set<string>]
> +AclSharedLogMeters[meters] :- AclSharedLogMeters0[meters].
> +AclSharedLogMeters[set_empty()] :-
> + Unit(),
> + not AclSharedLogMeters0[_].
> +
> +relation AclSharedLogMeters0[Set<string>]
> +AclSharedLogMeters0[meters] :-
> + nb in nb::NB_Global(),
> + Some{var s} = nb.options.get("acl_shared_log_meters"),
> + var meters = s.split(",").to_set().
> +
> /* Meter_Band table */
> for (mb in nb::Meter_Band) {
> sb::Out_Meter_Band(._uuid = mb._uuid,
> @@ -42,6 +55,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) :-
> + AclSharedLogMeters[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). */
> @@ -2008,7 +2029,7 @@ for (&Switch(.ls = ls)) {
> .external_ids = map_empty())
> }
>
> -function build_acl_log(acl: nb::ACL): string =
> +function build_acl_log(acl: nb::ACL, shared_meters: Set<string>): string =
> {
> if (not acl.log) {
> ""
> @@ -2040,7 +2061,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 (shared_meters.contains(meter)) {
> + "${meter}__${uuid2str(acl._uuid)}"
> + } else {
> + meter
> + };
> + vec_push(strs, "meter=${json_string_escape(s)}")
> + }
> };
> "log(${string_join(strs, \", \")}); "
> }
> @@ -2058,31 +2086,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 (AclSharedLogMeters[shared_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, shared_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))
> + }
> }
> }
>
> @@ -2338,114 +2368,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 (AclSharedLogMeters[shared_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, shared_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/ovn-nb.xml b/ovn-nb.xml
> index 5e8635992e49..51b186b84419 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -193,6 +193,20 @@
> </p>
> </column>
>
> + <column name="options" key="acl_shared_log_meters">
> + <p>
> + A string value that contains a list of meter names delimited by ",".
> + The <ref column="meter" table="ACL"/> column is used by ACL to rate
> + limit log events. As multiple ACL rows may opt to use the same meter
> + name, a potentially adverse effect is that a "noisy" ACL log could
> + consume all the allowable events for the shared
> + <ref column="name" table="meter"/>. This global option can be used to
> + eliminate that behavior, by listing the meters that are meant to
> + provide its "complete" rate to each one of the <ref table="ACL"/>
> + logs that use it.
> + </p>
> + </column>
> +
> <group title="Options for configuring interconnection route advertisement">
> <p>
> These options control how routes are advertised between OVN
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d2aefa8d840a..778482741b10 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1920,6 +1920,105 @@ sw1flows3: table=5 (ls_out_acl ), priority=2003 , match=((reg0[[9]] ==
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-northd -- ACL shared Meter])
> +AT_KEYWORDS([acl meter])
> +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 remove nb_global . options acl_shared_log_meters
> +ovn-nbctl meter-add meter_me drop 1 pktps
> +
> +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 in $* ; do
> + # echo checking for $expected_count $meter in sb meter table
> + check_row_count meter $expected_count name=$meter
> + done
> +}
> +
> +# With acl_shared_log_meters unset, make sure no unique meters are in SB
> +check_meter_by_name meter_me
> +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
> +for other_meters in 'm1' 'm1,m2,m3' 'meter_Xe'; do
> + ovn-nbctl --wait=sb set nb_global . options:acl_shared_log_meters="$other_meters"
> + check_meter_by_name meter_me
> + check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
> +done
> +
> +# Check variations where unique meters are in SB
> +for other_meters in 'm1,meter_me' 'm1,m2,meter_me,m3' 'meter_me,m1' 'meter_me'; do
> + ovn-nbctl --wait=sb set nb_global . options:acl_shared_log_meters="$other_meters"
> + check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2}
> +done
> +
> +# Change template meter and make sure that is reflected on acl meters as well
> +template_band=$(ovn-nbctl --bare --column=bands find meter 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. It refers to a named row that does not exist.
> +check_acl_lflow acl_two meter_me__${acl2}
> +
> +# Re-add template meter and make sure acl2's meter is back in sb
> +ovn-nbctl --wait=sb 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=$(ovn-nbctl --bare --column=_uuid find logical_switch 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])
> --
> 2.26.2
>
More information about the dev
mailing list