On Wed, Feb 23, 2022 at 7:22 AM Frode Nordahl
<frode.nord...@canonical.com> wrote:
>
> Hello Mark,
>
> This overall looks great, it also looks like you addressed Numan's
> comments from the previous iteration.
>
> I have added a nit in-line to save you from a conversation with 0-day
> / checkpatch.
>
> In addition to reviewing I took your patch for a ride and it works as
> expected.
>
> Would you perhaps want to re-base?
>
>
> On Fri, Feb 11, 2022 at 10:47 PM Mark Michelson <mmich...@redhat.com> wrote:
> >
> > It can be desirable for replies to stateful ACLs to be logged. And in
> > some cases, it can actually be a bit confusing why they aren't logged.
>
> I would agree, this is a nice addition!
>
> > Consider a situation where a port group called "port_group" exists and
> > logical switch ports swp1 and swp2 belong to it. We create the following
> > ACL, where logging is enabled:
> >
> > from-lport 100 'inport == @port_group' allow-stateless
> >
> > swp1 sends traffic to swp2 and swp2 responds within the same connection.
> > In this case, the logs will show both the packets from swp1 to swp2, as
> > well as the response packets from swp2 to swp1.
> >
> > Now change the ACL:
> >
> > from-lport 100 'inport == @port_group' allow-related
> >
> > Now with the same traffic pattern, the packets from swp1 to swp2 are
> > logged, but the packets from swp2 to swp1 are not. Why is that?
> >
> > The reason is that as a shortcut, when stateful ACLs are used, a single
> > priority 65532 flow is programmed to allow reply traffic to pass. When
> > no stateful ACL is present, the reply traffic is at the mercy of the
> > stateless ACL on the reply. Therefore, with the stateful ACL, the reply
> > traffic is not actually hitting an ACL but is let through by default,
> > but with the stateless ACL, the reply traffic does hit the ACL
> > evaluation, so the reply traffic is logged.
> >
> > This change adds a feature that allows for reply traffic to be
> > optionally logged for stateful ACLs, therefore allowing for the behavior
> > to be similar for both ACL types. Since logging reply traffic requires
> > adding more flows, it is not enabled by default. In order to have reply
> > traffic logged, the ACL must have logging enabled, be stateful, have a
> > label, and have the new log_related column set to true,
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2031150
> >
> > Signed-off-by: Mark Michelson <mmich...@redhat.com>
Hi Mark,
Thanks for addressing the comments in v3.
The patch LGTM. Before providing the Ack, I've one question.
Why do we need to set the acl label in order to make use of this feature ?
Numan
> > ---
> > v2 -> v3:
> > * Added item to NEWS
> > * Added flow documentation to ovn-northd.8.xml
> > * Added "options" column to ACL instead of having a dedicated
> > log_related column
> > ---
> > NEWS | 4 +
> > northd/northd.c | 42 +++++
> > northd/ovn-northd.8.xml | 8 +-
> > ovn-nb.ovsschema | 9 +-
> > ovn-nb.xml | 15 ++
> > tests/automake.mk | 3 +-
> > tests/check_acl_log.py | 107 ++++++++++++
> > tests/ovn-northd.at | 253 ++++++++++++++++++++++++++++
> > tests/system-ovn.at | 359 ++++++++++++++++++++++++++++++++++++++++
> > 9 files changed, 796 insertions(+), 4 deletions(-)
> > create mode 100644 tests/check_acl_log.py
> >
> > diff --git a/NEWS b/NEWS
> > index 194557410..5435aa606 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,5 +1,9 @@
> > Post v21.12.0
> > -------------
> > + - ACLs now have an "options" column for configuration of extra options.
> > + - A new ACL option, "log-related" has been added that allows for reply
> > and
> > + related traffic to be logged for an ACL in addition to the traffic that
> > + directly matches the ACL.
> >
> > OVN v21.12.0 - 22 Dec 2021
> > --------------------------
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 22e783ff6..f545891f8 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -6242,6 +6242,48 @@ consider_acl(struct hmap *lflows, struct
> > ovn_datapath *od,
> > acl->priority + OVN_ACL_PRI_OFFSET,
> > ds_cstr(match), ds_cstr(actions),
> > &acl->header_);
> > +
> > + /* Related and reply traffic are universally allowed by
> > priority
> > + * 65532 flows created in build_acls(). If logging is enabled
> > on
> > + * the ACL, then we need to ensure that the related and reply
> > + * traffic is logged, so we install a slightly higher-priority
> > + * flow that matches the ACL, allows the traffic, and logs it.
> > + */
> > + bool log_related = smap_get_bool(&acl->options, "log-related",
> > false);
>
> Checkpatch will have opinions on the length of this line.
>
> > + if (acl->log && acl->label && log_related) {
> > + /* Related/reply flows need to be set on the opposite
> > pipeline
> > + * from where the ACL itself is set.
> > + */
> > + enum ovn_stage log_related_stage = ingress ?
> > + S_SWITCH_OUT_ACL :
> > + S_SWITCH_IN_ACL;
> > + ds_clear(match);
> > + ds_clear(actions);
> > +
> > + ds_put_format(match, "ct.est && !ct.rel && !ct.new%s && "
> > + "ct.rpl && ct_label.blocked == 0 && "
> > + "ct_label.label == %" PRId64,
> > + use_ct_inv_match ? " && !ct.inv" : "",
> > + acl->label);
> > + build_acl_log(actions, acl, meter_groups);
> > + ds_put_cstr(actions, "next;");
> > + ovn_lflow_add_with_hint(lflows, od, log_related_stage,
> > + UINT16_MAX - 2,
> > + ds_cstr(match), ds_cstr(actions),
> > + &acl->header_);
> > +
> > + ds_clear(match);
> > + ds_put_format(match, "!ct.est && ct.rel && !ct.new%s && "
> > + "ct_label.blocked == 0 && "
> > + "ct_label.label == %" PRId64,
> > + use_ct_inv_match ? " && !ct.inv" : "",
> > + acl->label);
> > + ovn_lflow_add_with_hint(lflows, od, log_related_stage,
> > + UINT16_MAX - 2,
> > + ds_cstr(match), ds_cstr(actions),
> > + &acl->header_);
> > + }
> > +
> > }
> > } else if (!strcmp(acl->action, "drop")
> > || !strcmp(acl->action, "reject")) {
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 79f35bc16..cfe694831 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -746,7 +746,10 @@
> > go through the flows that implement the currently defined
> > policy based on ACLs. If a connection is no longer allowed by
> > policy, <code>ct_label.blocked</code> will get set and packets in
> > the
> > - reply direction will no longer be allowed, either.
> > + reply direction will no longer be allowed, either. If ACL logging
> > + and logging of related packets is enabled, then a companion
> > priority-
> > + 65533 flow will be installed that accomplishes the same thing but
> > + also logs the traffic.
> > </li>
> >
> > <li>
> > @@ -754,6 +757,9 @@
> > related to a committed flow in the connection tracker (e.g., an
> > ICMP Port Unreachable from a non-listening UDP port), as long
> > as the committed flow does not have <code>ct_label.blocked</code>
> > set.
> > + If ACL logging and logging of related packets is enabled, then a
> > + companion priority-65533 flow will be installed that accomplishes
> > the
> > + same thing but also logs the traffic.
> > </li>
> >
> > <li>
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > index 55977339a..530028018 100644
> > --- a/ovn-nb.ovsschema
> > +++ b/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Northbound",
> > - "version": "5.34.1",
> > - "cksum": "2177334725 30782",
> > + "version": "5.35.0",
> > + "cksum": "131378816 30999",
> > "tables": {
> > "NB_Global": {
> > "columns": {
>
> This part is now out of date with the main branch, but you know the
> drill for that.
>
> > @@ -262,6 +262,11 @@
> > "label": {"type": {"key": {"type": "integer",
> > "minInteger": 0,
> > "maxInteger": 4294967295}}},
> > + "options": {
> > + "type": {"key": "string",
> > + "value": "string",
> > + "min": 0,
> > + "max": "unlimited"}},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 6a6972856..645c75e3c 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -2064,6 +2064,21 @@
> > </group>
> >
> > <group title="Common Columns">
> > + <column name="options">
> > + This column provides general key/value settings. The supported
> > + options are described individually below.
> > + </column>
> > +
> > + <group title="ACL configuration options">
> > + <column name="options" key="log-related">
> > + If set to <code>true</code>, then log when reply or related
> > + traffic is admitted from a stateful ACL. In order for this
> > + option to function, the <ref column="log"/> option must be
> > + set to <code>true</code> and a <ref column="label"/> must
> > + be set.
> > + </column>
> > + </group>
> > +
> > <column name="external_ids">
> > See <em>External IDs</em> at the beginning of this document.
> > </column>
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index a08dfa632..a5934d2b9 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -270,7 +270,8 @@ tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
> > CHECK_PYFILES = \
> > tests/test-l7.py \
> > tests/uuidfilt.py \
> > - tests/test-tcp-rst.py
> > + tests/test-tcp-rst.py \
> > + tests/check_acl_log.py
> >
> > EXTRA_DIST += $(CHECK_PYFILES)
> > PYCOV_CLEAN_FILES += $(CHECK_PYFILES:.py=.py,cover) .coverage
> > diff --git a/tests/check_acl_log.py b/tests/check_acl_log.py
> > new file mode 100644
> > index 000000000..1dd9630c0
> > --- /dev/null
> > +++ b/tests/check_acl_log.py
> > @@ -0,0 +1,107 @@
> > +#!/usr/bin/env python3
> > +import argparse
> > +import string
> > +
> > +
> > +def strip(val):
> > + """Strip whitespace and quotation marks from val"""
> > + return val.strip(f"{string.whitespace}\"'")
> > +
> > +
> > +def parse_acl_log(line):
> > + """Convert an ACL log string into a dict"""
> > + # First cut off the logging preamble.
> > + # We're assuming the default log format.
> > + acl_log = {}
> > + _, _, details = line.rpartition("|")
> > +
> > + # acl_details are things like the acl name, direction,
> > + # verdict, and severity. packet_details are things like
> > + # the protocol, addresses, and ports of the packet being
> > + # logged.
> > + acl_details, _, packet_details = details.partition(":")
> > + for datum in acl_details.split(","):
> > + name, _, value = datum.rpartition("=")
> > + acl_log[strip(name)] = strip(value)
> > +
> > + for datum in packet_details.split(","):
> > + name, _, value = datum.rpartition("=")
> > + if not name:
> > + # The protocol is not preceded by "protocol="
> > + # so we need to add it manually.
> > + name = "protocol"
> > + acl_log[strip(name)] = strip(value)
> > +
> > + return acl_log
> > +
> > +
> > +def get_acl_log(entry_num=1):
> > + with open("ovn-controller.log", "r") as controller_log:
> > + acl_logs = [line for line in controller_log if "acl_log" in line]
> > + try:
> > + return acl_logs[entry_num - 1]
> > + except IndexError:
> > + print(
> > + f"There were not {entry_num} acl_log entries, \
> > + only {len(acl_logs)}"
> > + )
> > + exit(1)
> > +
> > +
> > +def add_parser_args(parser):
> > + parser.add_argument("--entry-num", type=int, default=1)
> > +
> > + # There are other possible things that can be in an ACL log,
> > + # and if we need those in the future, we can add them later.
> > + parser.add_argument("--name")
> > + parser.add_argument("--verdict")
> > + parser.add_argument("--severity")
> > + parser.add_argument("--protocol")
> > + parser.add_argument("--vlan_tci")
> > + parser.add_argument("--dl_src")
> > + parser.add_argument("--dl_dst")
> > + parser.add_argument("--nw_src")
> > + parser.add_argument("--nw_dst")
> > + parser.add_argument("--nw_tos")
> > + parser.add_argument("--nw_ecn")
> > + parser.add_argument("--nw_ttl")
> > + parser.add_argument("--icmp_type")
> > + parser.add_argument("--icmp_code")
> > + parser.add_argument("--tp_src")
> > + parser.add_argument("--tp_dst")
> > + parser.add_argument("--tcp_flags")
> > + parser.add_argument("--ipv6_src")
> > + parser.add_argument("--ipv6_dst")
> > +
> > +
> > +def main():
> > + parser = argparse.ArgumentParser()
> > + add_parser_args(parser)
> > + args = parser.parse_args()
> > +
> > + acl_log = get_acl_log(args.entry_num)
> > + parsed_log = parse_acl_log(acl_log)
> > +
> > + # Express command line arguments as a dict, omitting any arguments that
> > + # were not provided by the user.
> > + expected = {k: v for k, v in vars(args).items() if v is not None}
> > + del expected["entry_num"]
> > +
> > + for key, val in expected.items():
> > + try:
> > + if parsed_log[key] != val:
> > + print(
> > + f"Expected log {key}={val} but got
> > {key}={parsed_log[key]} \
> > + in:\n\t'{acl_log}"
> > + )
> > + exit(1)
> > + except KeyError:
> > + print(
> > + f"Expected log {key}={val} but {key} does not exist \
> > + in:\n\t'{acl_log}'"
> > + )
> > + exit(1)
> > +
> > +
> > +if __name__ == "__main__":
> > + main()
>
> Love the elaborate validation script.
>
> --
> Frode Nordahl
>
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 652903761..999c31335 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -5888,5 +5888,258 @@ AT_CHECK([grep -e "(lr_in_ip_routing ).*outport"
> > lr0flows | sed 's/table=../ta
> > table=??(lr_in_ip_routing ), priority=97 , match=(reg7 == 2 &&
> > ip4.dst == 1.1.1.1/32), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 =
> > 192.168.0.20; reg1 = 192.168.0.1; eth.src = 00:00:00:00:00:01; outport =
> > "lrp0"; flags.loopback = 1; next;)
> > ])
> >
> > +AT_CLEANUP
> > +])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ACL log replies -- flows])
> > +
> > +set_acl_options() {
> > + local acl_name=$1
> > + local label=$2
> > + local log_related=$3
> > +
> > + local acl_uuid=$(fetch_column nb:ACL _uuid name=$acl_name)
> > + check ovn-nbctl set ACL $acl_uuid label=$label
> > options:log-related=$log_related
> > +}
> > +
> > +record_log_flows() {
> > + ovn-sbctl lflow-list sw0 | grep -E 'ls_(out|in)_acl.*, priority=65533'
> > | sed 's/table=../table=??/' | sort > log_flows
> > +}
> > +
> > +check_log_flows_count() {
> > + local expected=$1
> > + local table=$2
> > + local count=
> > +
> > + echo $table
> > + if test -f log_flows; then
> > + count=$(grep -c -E ls_${table}_acl log_flows)
> > + else
> > + count=$(ovn-sbctl lflow-list sw0 | grep -c -E "ls_$table_acl.*,
> > priority=65533")
> > + fi
> > +
> > + check test "$count" -eq "$expected"
> > +}
> > +
> > +ovn_start
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1
> > "00:00:00:00:00:01 10.0.0.1"
> > +check ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2
> > "00:00:00:00:00:02 10.0.0.2"
> > +
> > +check ovn-nbctl pg-add pg1 sw0-p1 sw0-p2
> > +check ovn-nbctl pg-add pg2 sw0-p1 sw0-p2
> > +check ovn-nbctl pg-add pg3 sw0-p1 sw0-p2
> > +
> > +check ovn-nbctl --log --name=allow_acl acl-add pg1 from-lport 100
> > 'inport=@pg1 && ip4' allow
> > +set_acl_options allow_acl 1 true
> > +
> > +check ovn-nbctl --wait=sb sync
> > +
> > +# An allow ACL should *not* result in a priority 65533 log flow being
> > installed
> > +# since there are no stateful ACLs on the system.
> > +check_log_flows_count 0 in
> > +check_log_flows_count 0 out
> > +
> > +# Now add an allow-related ACL. This should result in both the
> > allow-related
> > +# ACL and the allow ACL having priority 65533 log flows added.
> > +check ovn-nbctl --log --name=allow_related_acl acl-add pg2 from-lport 100
> > 'inport=@pg2 && ip4' allow-related
> > +set_acl_options allow_related_acl 2 true
> > +check ovn-nbctl --wait=sb sync
> > +
> > +record_log_flows
> > +
> > +# The count will be 4 since we have
> > +# 2 flows for reply traffic for each ACL
> > +# 2 flows for related traffic for each ACL
> > +check_log_flows_count 4 out
> > +# Since the ACLs are ingress, the ingress table
> > +# should have no log flows
> > +check_log_flows_count 0 in
> > +
> > +# Now ensure the flows are what we expect them to be for the ACLs we
> > created
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_out_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 2),
> > action=(log(name="allow_related_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 2), action=(log(name="allow_related_acl", severity=info, verdict=allow);
> > next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# Now add a stateless-allow ACL.
> > +check ovn-nbctl --log --name=allow_stateless_acl acl-add pg3 from-lport
> > 100 'inport=@pg3 && ip4' allow-stateless
> > +set_acl_options allow_stateless_acl 3 true
> > +check ovn-nbctl --wait=sb sync
> > +
> > +record_log_flows
> > +
> > +# The count will still be 4 since the stateless ACL should not have
> > special log flows created
> > +check_log_flows_count 4 out
> > +check_log_flows_count 0 in
> > +
> > +# And the log flows will remain the same since the stateless ACL will not
> > be represented.
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_out_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 2),
> > action=(log(name="allow_related_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 2), action=(log(name="allow_related_acl", severity=info, verdict=allow);
> > next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# Now remove the label from the allow-related ACL.
> > +set_acl_options allow_related_acl 0 true
> > +ovn-nbctl --wait=sb sync
> > +
> > +record_log_flows
> > +
> > +# The count should now be 2 since the allow_related ACL will not have
> > special
> > +# log flows created. But since there there is an allow-related ACL
> > present, the
> > +# allow ACL will be stateful and have special log flows created.
> > +check_log_flows_count 2 out
> > +check_log_flows_count 0 in
> > +
> > +# And make sure only the allow ACL has the log flows installed
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_out_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# And now add the label back, but disable log_related on the allow-related
> > ACL.
> > +set_acl_options allow_related_acl 2 false
> > +
> > +record_log_flows
> > +
> > +# The count will again be 2 because only the allow ACL will have log flows
> > installed.
> > +check_log_flows_count 2 out
> > +check_log_flows_count 0 in
> > +
> > +# And make sure only the allow ACL has the log flows installed
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_out_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_out_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# And just for sanity's sake, let's remove the allow-related ACL and make
> > sure
> > +# all the special log messages are gone.
> > +check ovn-nbctl acl-del pg2
> > +check ovn-nbctl --wait=sb sync
> > +
> > +check_log_flows_count 0 out
> > +check_log_flows_count 0 in
> > +
> > +# Now let's clear out all the ACLs, and re-do everything but with egress
> > ACLs.
> > +check ovn-nbctl acl-del pg1
> > +check ovn-nbctl acl-del pg3
> > +check_row_count nb:ACL 0
> > +
> > +# Start again with an allow_acl only
> > +check ovn-nbctl --log --name=allow_acl acl-add pg1 to-lport 100
> > 'inport=@pg1 && ip4' allow
> > +set_acl_options allow_acl 1 true
> > +
> > +check ovn-nbctl --wait=sb sync
> > +
> > +# Again, the allow ACL is stateless, so no related log flows.
> > +check_log_flows_count 0 in
> > +check_log_flows_count 0 out
> > +
> > +# Adding a new allow-related ACL...
> > +check ovn-nbctl --log --name=allow_related_acl acl-add pg2 to-lport 100
> > 'inport=@pg2 && ip4' allow-related
> > +set_acl_options allow_related_acl 2 true
> > +check ovn-nbctl --wait=sb sync
> > +
> > +record_log_flows
> > +
> > +# The count will be 4 since we have
> > +# 2 flows for reply traffic for each ACL
> > +# 2 flows for related traffic for each ACL
> > +check_log_flows_count 4 in
> > +# And this time, we should have no egress flows
> > +check_log_flows_count 0 out
> > +
> > +# Now ensure the flows are what we expect them to be for the ACLs we
> > created
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_in_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 2),
> > action=(log(name="allow_related_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 2), action=(log(name="allow_related_acl", severity=info, verdict=allow);
> > next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# Now add a stateless-allow ACL.
> > +check ovn-nbctl --log --name=allow_stateless_acl acl-add pg3 from-lport
> > 100 'inport=@pg3 && ip4' allow-stateless
> > +set_acl_options allow_stateless_acl 3 true
> > +check ovn-nbctl --wait=sb sync
> > +
> > +record_log_flows
> > +
> > +# The count will still be 4 since the stateless ACL should not have
> > special log flows created
> > +check_log_flows_count 4 in
> > +check_log_flows_count 0 out
> > +
> > +# And the log flows will remain the same since the stateless ACL will not
> > be represented.
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_in_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 2),
> > action=(log(name="allow_related_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 2), action=(log(name="allow_related_acl", severity=info, verdict=allow);
> > next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# Now remove the label from the allow-related ACL.
> > +set_acl_options allow_related_acl 0 true
> > +ovn-nbctl --wait=sb sync
> > +
> > +record_log_flows
> > +
> > +# The count should now be 2 since the allow_related ACL will not have
> > special
> > +# log flows created. But since there there is an allow-related ACL
> > present, the
> > +# allow ACL will be stateful and have special log flows created.
> > +check_log_flows_count 2 in
> > +check_log_flows_count 0 out
> > +
> > +# And make sure only the allow ACL has the log flows installed
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_in_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# And now add the label back, but disable log_related on the allow-related
> > ACL.
> > +set_acl_options allow_related_acl 2 false
> > +
> > +record_log_flows
> > +
> > +# The count will again be 2 because only the allow ACL will have log flows
> > installed.
> > +check_log_flows_count 2 in
> > +check_log_flows_count 0 out
> > +
> > +# And make sure only the allow ACL has the log flows installed
> > +AT_CHECK([cat log_flows], [0], [dnl
> > + table=??(ls_in_acl ), priority=65533, match=(!ct.est && ct.rel
> > && !ct.new && !ct.inv && ct_label.blocked == 0 && ct_label.label == 1),
> > action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > + table=??(ls_in_acl ), priority=65533, match=(ct.est && !ct.rel
> > && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0 && ct_label.label
> > == 1), action=(log(name="allow_acl", severity=info, verdict=allow); next;)
> > +])
> > +
> > +rm log_flows
> > +
> > +# And just for sanity's sake, let's remove the allow-related ACL and make
> > sure
> > +# all the special log messages are gone.
> > +check ovn-nbctl acl-del pg2
> > +check ovn-nbctl --wait=sb sync
> > +
> > +check_log_flows_count 0 out
> > +check_log_flows_count 0 in
> > +
> > +
> > +
> > AT_CLEANUP
> > ])
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 3ae812296..c70fb2a84 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -7002,3 +7002,362 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> > patch-.*/d
> >
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ACL log_related])
> > +
> > +CHECK_CONNTRACK()
> > +ovn_start
> > +
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +ADD_BR([br-int])
> > +
> > +set_acl_options() {
> > + local acl_name=$1; shift
> > +
> > + local acl_uuid=$(fetch_column nb:ACL _uuid name=$acl_name)
> > + check ovn-nbctl set ACL $acl_uuid "$@"
> > +}
> > +
> > +clear_log() {
> > + ovn-appctl -t ovn-controller vlog/close
> > + rm ovn-controller.log
> > + ovn-appctl -t ovn-controller vlog/reopen
> > +}
> > +
> > +test_ping() {
> > + NS_CHECK_EXEC([sw0-p1], [ping -q -c 1 -i 0.3 -w 2 10.0.0.2 |
> > FORMAT_PING], \
> > +[0], [dnl
> > +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> > +])
> > +}
> > +
> > +check_acl_log_count() {
> > + local expected_count=$1
> > +
> > + AT_CHECK_UNQUOTED([grep -c acl_log ovn-controller.log], [0], [dnl
> > +$expected_count
> > +])
> > +}
> > +
> > +# Set external-ids in br-int needed for ovn-controller
> > +ovs-vsctl \
> > + -- set Open_vSwitch . external-ids:system-id=hv1 \
> > + -- set Open_vSwitch .
> > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > + -- set bridge br-int fail-mode=secure
> > other-config:disable-in-band=true
> > +
> > +# Start ovn-controller
> > +start_daemon ovn-controller
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1
> > "00:00:00:00:00:01 10.0.0.1"
> > +check ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2
> > "00:00:00:00:00:02 10.0.0.2"
> > +
> > +check ovn-nbctl pg-add pg1 sw0-p1 sw0-p2
> > +
> > +ADD_NAMESPACES(sw0-p1)
> > +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.1/24", "00:00:00:00:00:01")
> > +ADD_NAMESPACES(sw0-p2)
> > +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.2/24", "00:00:00:00:00:02")
> > +
> > +wait_for_ports_up
> > +
> > +check ovn-nbctl --log --name=allow_acl acl-add pg1 from-lport 100 'inport
> > == @pg1 && ip4' allow
> > +
> > +check ovn-nbctl --wait=hv sync
> > +
> > +test_ping
> > +
> > +# The allow ACL should match on the request and reply traffic, resulting
> > in 2 logs.
> > +check_acl_log_count 2
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=2 \
> > + --name=allow_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:02 \
> > + --dl_dst=00:00:00:00:00:01 \
> > + --nw_src=10.0.0.2 \
> > + --nw_dst=10.0.0.1 \
> > + --icmp_type=0 \
> > + --icmp_code=0
> > +
> > +# Now add a higher-priority stateful ACL that matches on the same
> > +# parameters. Don't enable reply logging.
> > +check ovn-nbctl --log --name=allow_related_acl acl-add pg1 from-lport 200
> > 'inport == @pg1 && ip4' allow-related
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# Since reply logging is not enabled, the allow-related ACL should match
> > on the
> > +# request, but the reply will not be logged.
> > +check_acl_log_count 1
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +# As a control, set a label on the allow-related ACL, but still don't
> > enable
> > +# reply traffic logging.
> > +set_acl_options allow_related_acl label=1 options:log-related=false
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# This should have the same result as the previous ping
> > +check_acl_log_count 1
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +# As another control, remove the label but enable reply logging.
> > +set_acl_options allow_related_acl label=0 options:log-related=true
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# This should have the same result as the previous ping
> > +check_acl_log_count 1
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +# This time, add a label and enable reply logging on the allow_related ACL.
> > +set_acl_options allow_related_acl label=1 options:log-related=true
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# Now we should have the request and reply logged.
> > +check_acl_log_count 2
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=2 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:02 \
> > + --dl_dst=00:00:00:00:00:01 \
> > + --nw_src=10.0.0.2 \
> > + --nw_dst=10.0.0.1 \
> > + --icmp_type=0 \
> > + --icmp_code=0
> > +
> > +
> > +# And now, let's start from scratch but make sure everything works when
> > +# using egress ACLs.
> > +check ovn-nbctl acl-del pg1
> > +check_row_count nb:ACL 0
> > +
> > +check ovn-nbctl --log --name=allow_acl acl-add pg1 to-lport 100 'outport
> > == @pg1 && ip4' allow
> > +
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# The allow ACL should match on the request and reply traffic, resulting
> > in 2 logs.
> > +check_acl_log_count 2
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=2 \
> > + --name=allow_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:02 \
> > + --dl_dst=00:00:00:00:00:01 \
> > + --nw_src=10.0.0.2 \
> > + --nw_dst=10.0.0.1 \
> > + --icmp_type=0 \
> > + --icmp_code=0
> > +
> > +# Now add a higher-priority stateful ACL that matches on the same
> > +# parameters. Don't enable reply logging.
> > +check ovn-nbctl --log --name=allow_related_acl acl-add pg1 to-lport 200
> > 'outport == @pg1 && ip4' allow-related
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# Since reply logging is not enabled, the allow-related ACL should match
> > on the
> > +# request, but the reply will not be logged.
> > +check_acl_log_count 1
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +# As a control, set a label on the allow-related ACL, but still don't
> > enable
> > +# reply traffic logging.
> > +set_acl_options allow_related_acl label=1 options:log-related=false
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# This should have the same result as the previous ping
> > +check_acl_log_count 1
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +# As another control, remove the label but enable reply logging.
> > +set_acl_options allow_related_acl label=0 options:log-related=true
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# This should have the same result as the previous ping
> > +check_acl_log_count 1
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +# This time, add a label and enable reply logging on the allow_related ACL.
> > +set_acl_options allow_related_acl label=1 options:log-related=true
> > +check ovn-nbctl --wait=hv sync
> > +
> > +clear_log
> > +test_ping
> > +
> > +# Now we should have the request and reply logged.
> > +check_acl_log_count 2
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=1 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:01 \
> > + --dl_dst=00:00:00:00:00:02 \
> > + --nw_src=10.0.0.1 \
> > + --nw_dst=10.0.0.2 \
> > + --icmp_type=8 \
> > + --icmp_code=0
> > +
> > +check $PYTHON $srcdir/check_acl_log.py \
> > + --entry-num=2 \
> > + --name=allow_related_acl \
> > + --verdict=allow \
> > + --protocol=icmp \
> > + --dl_src=00:00:00:00:00:02 \
> > + --dl_dst=00:00:00:00:00:01 \
> > + --nw_src=10.0.0.2 \
> > + --nw_dst=10.0.0.1 \
> > + --icmp_type=0 \
> > + --icmp_code=0
> > +
> > +
> > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > +/connection dropped.*/d"])
> > +
> > +AT_CLEANUP
> > +])
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
