ovn: Add rate-limiting for ACL logs.

Signed-off-by: Justin Pettit <[email protected]>
Acked-by: Ben Pfaff <[email protected]>

include/ovn/actions.h

@@ -284,6 +284,7 @@ struct ovnact_log {
     uint8_t verdict;            /* One of LOG_VERDICT_*. */
     uint8_t severity;           /* One of LOG_SEVERITY_*. */
     char *name;
+    char *meter;
 };
 
 /* OVNACT_SET_METER. */

ovn/lib/actions.c

@@ -78,14 +78,15 @@ ovnact_init(struct ovnact *ovnact, enum ovnact_type type, size_t len)
 
 static size_t
 encode_start_controller_op(enum action_opcode opcode, bool pause,
-                           struct ofpbuf *ofpacts)
+                           uint32_t meter_id, struct ofpbuf *ofpacts)
 {
     size_t ofs = ofpacts->size;
 
     struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts);
     oc->max_len = UINT16_MAX;
     oc->reason = OFPR_ACTION;
     oc->pause = pause;
+    oc->meter_id = meter_id;
 
     struct action_header ah = { .opcode = htonl(opcode) };
     ofpbuf_put(ofpacts, &ah, sizeof ah);
@@ -105,7 +106,8 @@ encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts)
 static void
 encode_controller_op(enum action_opcode opcode, struct ofpbuf *ofpacts)
 {
-    size_t ofs = encode_start_controller_op(opcode, false, ofpacts);
+    size_t ofs = encode_start_controller_op(opcode, false, NX_CTLR_NO_METER,
+                                            ofpacts);
     encode_finish_controller_op(ofs, ofpacts);
 }
 
@@ -1245,7 +1247,8 @@ encode_nested_actions(const struct ovnact_nest *on,
      * converted to OpenFlow, as its userdata.  ovn-controller will convert the
      * packet to ARP or NA and then send the packet and actions back to the
      * switch inside an OFPT_PACKET_OUT message. */
-    size_t oc_offset = encode_start_controller_op(opcode, false, ofpacts);
+    size_t oc_offset = encode_start_controller_op(opcode, false,
+                                                  NX_CTLR_NO_METER, ofpacts);
     ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size,
                                  ofpacts, OFP13_VERSION);
     encode_finish_controller_op(oc_offset, ofpacts);
@@ -1738,7 +1741,8 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo,
     struct mf_subfield dst = expr_resolve_field(&pdo->dst);
 
     size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_PUT_DHCP_OPTS,
-                                                  true, ofpacts);
+                                                  true, NX_CTLR_NO_METER,
+                                                  ofpacts);
     nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
     ovs_be32 ofs = htonl(dst.ofs);
     ofpbuf_put(ofpacts, &ofs, sizeof ofs);
@@ -1769,7 +1773,7 @@ encode_PUT_DHCPV6_OPTS(const struct ovnact_put_opts *pdo,
     struct mf_subfield dst = expr_resolve_field(&pdo->dst);
 
     size_t oc_offset = encode_start_controller_op(
-        ACTION_OPCODE_PUT_DHCPV6_OPTS, true, ofpacts);
+        ACTION_OPCODE_PUT_DHCPV6_OPTS, true, NX_CTLR_NO_METER, ofpacts);
     nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
     ovs_be32 ofs = htonl(dst.ofs);
     ofpbuf_put(ofpacts, &ofs, sizeof ofs);
@@ -1864,7 +1868,8 @@ encode_DNS_LOOKUP(const struct ovnact_dns_lookup *dl,
     struct mf_subfield dst = expr_resolve_field(&dl->dst);
 
     size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_DNS_LOOKUP,
-                                                  true, ofpacts);
+                                                  true, NX_CTLR_NO_METER,
+                                                  ofpacts);
     nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
     ovs_be32 ofs = htonl(dst.ofs);
     ofpbuf_put(ofpacts, &ofs, sizeof ofs);
@@ -2027,7 +2032,7 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po,
     struct mf_subfield dst = expr_resolve_field(&po->dst);
 
     size_t oc_offset = encode_start_controller_op(
-        ACTION_OPCODE_PUT_ND_RA_OPTS, true, ofpacts);
+        ACTION_OPCODE_PUT_ND_RA_OPTS, true, NX_CTLR_NO_METER, ofpacts);
     nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
     ovs_be32 ofs = htonl(dst.ofs);
     ofpbuf_put(ofpacts, &ofs, sizeof ofs);
@@ -2101,6 +2106,19 @@ parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
             }
         }
         lexer_syntax_error(ctx->lexer, "expecting severity");
+    } else if (lexer_match_id(ctx->lexer, "meter")) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+            return;
+        }
+        /* If multiple meters are given, use the most recent. */
+        if (ctx->lexer->token.type == LEX_T_STRING) {
+            free(log->meter);
+            log->meter = xstrdup(ctx->lexer->token.s);
+        } else {
+            lexer_syntax_error(ctx->lexer, "expecting string");
+            return;
+        }
+        lexer_get(ctx->lexer);
     } else {
         lexer_syntax_error(ctx->lexer, NULL);
     }
@@ -2136,16 +2154,31 @@ format_LOG(const struct ovnact_log *log, struct ds *s)
     }
 
     ds_put_format(s, "verdict=%s, ", log_verdict_to_string(log->verdict));
-    ds_put_format(s, "severity=%s);", log_severity_to_string(log->severity));
+    ds_put_format(s, "severity=%s", log_severity_to_string(log->severity));
+
+    if (log->meter) {
+        ds_put_format(s, ", meter=\"%s\"", log->meter);
+    }
+
+    ds_put_cstr(s, ");");
 }
 
 static void
 encode_LOG(const struct ovnact_log *log,
-           const struct ovnact_encode_params *ep OVS_UNUSED,
-           struct ofpbuf *ofpacts)
+           const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts)
 {
+    uint32_t meter_id = NX_CTLR_NO_METER;
+
+    if (log->meter) {
+        meter_id = ovn_extend_table_assign_id(ep->meter_table, log->meter);
+        if (meter_id == EXT_TABLE_ID_INVALID) {
+            VLOG_WARN("log specified unknown meter: %"PRIu32, meter_id);
+            return;
+        }
+    }
+
     size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_LOG, false,
-                                                  ofpacts);
+                                                  meter_id, ofpacts);
 
     struct log_pin_header *lph = ofpbuf_put_uninit(ofpacts, sizeof *lph);
     lph->verdict = log->verdict;
@@ -2163,6 +2196,7 @@ static void
 ovnact_log_free(struct ovnact_log *log)
 {
     free(log->name);
+    free(log->meter);
 }
 
 static void

ovn/northd/ovn-northd.c

@@ -3140,6 +3140,10 @@ 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);
+    }
+
     ds_chomp(actions, ' ');
     ds_chomp(actions, ',');
     ds_put_cstr(actions, "); ");

ovn/ovn-nb.ovsschema

@@ -1,7 +1,7 @@
 {
     "name": "OVN_Northbound",
-    "version": "5.12.0",
-    "cksum": "2812995200 20238",
+    "version": "5.13.0",
+    "cksum": "1278623084 20312",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -166,6 +166,7 @@
                                                         "notice", "info",
                                                         "debug"]]},
                                       "min": 0, "max": 1}},
+                "meter": {"type": {"key": "string", "min": 0, "max": 1}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}}},

ovn/ovn-nb.xml

@@ -1159,6 +1159,15 @@
           <code>info</code>.
         </p>
       </column>
+
+      <column name="meter">
+        <p>
+            The name of a meter to rate-limit log messages for the ACL.
+            The string must match the <ref column="name" table="meter"/>
+            column of a row in the <ref table="Meter"/> table.  By
+            default, log messages are not rate-limited.
+        </p>
+      </column>
     </group>
 
     <group title="Common Columns">

ovn/ovn-sb.xml

@@ -1761,6 +1761,15 @@
               The verdict for packets matching the flow.  The value must be one
               of <code>allow</code>, <code>deny</code>, or <code>reject</code>.
             </dd>
+            <dt><code>meter=</code><var>string</var></dt>
+            <dd>
+              An optional rate-limiting meter to be applied to the logs.
+              The <var>string</var> should reference a
+              <ref column="name" table="Meter"/> entry from the
+              <ref table="Meter"/> table.  The only meter
+              <ref column="action" table="meter"/> that is appriopriate
+              is <code>drop</code>.
+            </dd>
           </dl>
         </dd>
       </dl>

ovn/utilities/ovn-nbctl.8.xml

@@ -85,7 +85,7 @@
       must be either <code>switch</code> or <code>port-group</code>.
     </p>
     <dl>
-      <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
+      <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
       <dd>
         <p>
           Adds the specified ACL to <var>entity</var>.  <var>direction</var>
@@ -105,7 +105,9 @@
           logging).  The severity must be one of <code>alert</code>,
           <code>warning</code>, <code>notice</code>, <code>info</code>, or
           <code>debug</code>.  If a severity is not specified, the default is
-          <code>info</code>.
+          <code>info</code>.  The <code>--meter=<var>meter</var></code> option
+          is used to rate-limit packet logging.  The <var>meter</var> argument
+          names a meter configured by <code>meter-add</code>.
         </p>
       </dd>
 

ovn/utilities/ovn-nbctl.c

@@ -1822,7 +1822,10 @@ nbctl_acl_list(struct ctl_context *ctx)
                 ds_put_format(&ctx->output, "name=%s,", acl->name);
             }
             if (acl->severity) {
-                ds_put_format(&ctx->output, "severity=%s", acl->severity);
+                ds_put_format(&ctx->output, "severity=%s,", acl->severity);
+            }
+            if (acl->meter) {
+                ds_put_format(&ctx->output, "meter=\"%s\",", acl->meter);
             }
             ds_chomp(&ctx->output, ',');
             ds_put_cstr(&ctx->output, ")");
@@ -1927,7 +1930,8 @@ nbctl_acl_add(struct ctl_context *ctx)
     bool log = shash_find(&ctx->options, "--log") != NULL;
     const char *severity = shash_find_data(&ctx->options, "--severity");
     const char *name = shash_find_data(&ctx->options, "--name");
-    if (log || severity || name) {
+    const char *meter = shash_find_data(&ctx->options, "--meter");
+    if (log || severity || name || meter) {
         nbrec_acl_set_log(acl, true);
     }
     if (severity) {
@@ -1940,6 +1944,9 @@ nbctl_acl_add(struct ctl_context *ctx)
     if (name) {
         nbrec_acl_set_name(acl, name);
     }
+    if (meter) {
+        nbrec_acl_set_meter(acl, meter);
+    }
 
     /* Check if same acl already exists for the ls/portgroup */
     size_t n_acls = pg ? pg->n_acls : ls->n_acls;
@@ -4801,7 +4808,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
     /* acl commands. */
     { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION",
       NULL, nbctl_acl_add, NULL,
-      "--log,--may-exist,--type=,--name=,--severity=", RW },
+      "--log,--may-exist,--type=,--name=,--severity=,--meter=", RW },
     { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]",
       NULL, nbctl_acl_del, NULL, "--type=", RW },
     { "acl-list", 1, 1, "{SWITCH | PORTGROUP}",

tests/ovn.at

@@ -6153,6 +6153,79 @@ OVN_CLEANUP([hv])
 AT_CLEANUP
 
 
+AT_SETUP([ovn -- ACL rate-limited logging])
+AT_KEYWORDS([ovn])
+ovn_start
+
+net_add n1
+
+sim_add hv
+as hv
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+for i in lp1 lp2; do
+    ovs-vsctl -- add-port br-int $i -- \
+        set interface $i external-ids:iface-id=$i \
+        options:tx_pcap=hv/$i-tx.pcap \
+        options:rxq_pcap=hv/$i-rx.pcap
+done
+
+lp1_mac="f0:00:00:00:00:01"
+lp1_ip="192.168.1.2"
+
+lp2_mac="f0:00:00:00:00:02"
+lp2_ip="192.168.1.3"
+
+ovn-nbctl ls-add lsw0
+ovn-nbctl --wait=sb lsp-add lsw0 lp1
+ovn-nbctl --wait=sb lsp-add lsw0 lp2
+ovn-nbctl lsp-set-addresses lp1 $lp1_mac
+ovn-nbctl lsp-set-addresses lp2 $lp2_mac
+ovn-nbctl --wait=sb sync
+
+
+# Add an ACL that rate-limits logs at 10 per second.
+ovn-nbctl meter-add http-rl1 drop 10 pktps
+ovn-nbctl --log --severity=alert --meter=http-rl1 --name=http-acl1 acl-add lsw0 to-lport 1000 'tcp.dst==80' drop
+
+# Add an ACL that rate-limits logs at 5 per second.
+ovn-nbctl meter-add http-rl2 drop 5 pktps
+ovn-nbctl --log --severity=alert --meter=http-rl2 --name=http-acl2 acl-add lsw0 to-lport 1000 'tcp.dst==81' allow
+
+# Add an ACL that doesn't rate-limit logs.
+ovn-nbctl --log --severity=alert --name=http-acl3 acl-add lsw0 to-lport 1000 'tcp.dst==82' drop
+
+
+# For each ACL, send 100 packets.
+for i in `seq 1 100`; do
+    ovs-appctl netdev-dummy/receive lp1 'in_port(1),eth(src=f0:00:00:00:00:01,dst=f0:00:00:00:00:02),eth_type(0x0800),ipv4(src=192.168.1.2,dst=192.168.1.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=7777,dst=80)'
+
+    ovs-appctl netdev-dummy/receive lp1 'in_port(1),eth(src=f0:00:00:00:00:01,dst=f0:00:00:00:00:02),eth_type(0x0800),ipv4(src=192.168.1.2,dst=192.168.1.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=7777,dst=81)'
+
+    ovs-appctl netdev-dummy/receive lp1 'in_port(1),eth(src=f0:00:00:00:00:01,dst=f0:00:00:00:00:02),eth_type(0x0800),ipv4(src=192.168.1.2,dst=192.168.1.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=7777,dst=82)'
+done
+
+# Print some information that may help debugging.
+as hv ovs-appctl -t ovn-controller meter-table-list
+as hv ovs-ofctl -O OpenFlow13 meter-stats br-int
+
+# The userspace meter implementation doesn't precisely enforce counts,
+# so we just check that exactly 100 "http-acl3" actions were logged and
+# that there were more "http-acl1" actions than "http-acl2" ones.
+OVS_WAIT_UNTIL([ test 100 = $(grep -c 'http-acl3' hv/ovn-controller.log) ])
+
+n_acl1=$(grep -c 'http-acl1' hv/ovn-controller.log)
+n_acl2=$(grep -c 'http-acl2' hv/ovn-controller.log)
+n_acl3=$(grep -c 'http-acl3' hv/ovn-controller.log)
+
+AT_CHECK([ test $n_acl3 -gt $n_acl1 ], [0], [])
+AT_CHECK([ test $n_acl1 -gt $n_acl2 ], [0], [])
+
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
+
+
 AT_SETUP([ovn -- DSCP marking and meter check])
 AT_KEYWORDS([ovn])
 ovn_start