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> > --- > 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