Thanks for the review Ales, I have some responses below.
On 1/14/25 03:27, Ales Musil wrote:
On Mon, Dec 16, 2024 at 8:03 PM Mark Michelson <[email protected] <mailto:[email protected]>> wrote:On allow-related ACLs, if the ACL changes and no longer matches an established session, then traffic will no longer automatically be allowed. Instead, traffic on the established session will be subject to ACL matching, and therefore the traffic may be dropped. This behavior can be altered by setting the "bypass_match_for_established" option on allow-related ACLs. When set to true, we set a specific bit in the ct_mark when the conntrack entry is committed. If this bit is set, then established session traffic will always be allowed, even if the ACL is altered to no longer match the traffic. Upcoming commits will put in place methods so that deleting the ACL or changing the action type on the ACL will remove the conntrack entry that allows the established traffic. Signed-off-by: Mark Michelson <[email protected] <mailto:[email protected]>> --- Hi Mark,thank you for the next version, it seems that it will need rebase. I have a few minor comments down below.v2 -> v3: * The configuration mechanism changed from a new ACL action to being an option that supplements "allow-related" ACLs. The new option is called bypass_match_for_established. A suggestion for the option was "flush_ct_on_removal". I elected not to go with this because flushing CT on removal isn't the real draw of the option. Admins set the option so that the ACL does not have to be matched once the connection is established. The flush of CT is a necessity of the feature, but it's not why the admin is setting the option. v1 -> v2: * Fixed formatting issues * Fixed flake8 issues * Check for CT label flush chassis feature --- include/ovn/logical-fields.h | 1 + lib/logical-fields.c | 5 ++ northd/en-ls-stateful.c | 4 +- northd/northd.c | 49 ++++++++++- ovn-nb.ovsschema | 4 +- ovn-nb.xml | 26 ++++++ tests/automake.mk <http://automake.mk> | 4 +- tests/client.py | 36 ++++++++ tests/ovn-northd.at <http://ovn-northd.at> | 134 ++++++++++++++++++++++++++++ tests/server.py | 31 +++++++ tests/system-ovn.at <http://system-ovn.at> | 163 +++++++++++++++++++++++++++++++++++ 11 files changed, 448 insertions(+), 9 deletions(-) create mode 100755 tests/client.py create mode 100755 tests/server.py diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index d563e044c..c7e4baa51 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -203,6 +203,7 @@ const struct ovn_field *ovn_field_from_name(const char *name); #define OVN_CT_LB_FORCE_SNAT_BIT 3 #define OVN_CT_OBS_STAGE_1ST_BIT 4 #define OVN_CT_OBS_STAGE_END_BIT 5 +#define OVN_CT_ALLOW_ESTABLISHED_BIT 6 #define OVN_CT_BLOCKED 1 #define OVN_CT_NATTED 2 diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 5a8b53f2b..5b578b5c2 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -171,6 +171,11 @@ ovn_init_symtab(struct shash *symtab)OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT)"]", WR_CT_COMMIT); + expr_symtab_add_subfield_scoped(symtab, "ct_mark.allow_established", NULL, + "ct_mark["+ OVN_CT_STR(OVN_CT_ALLOW_ESTABLISHED_BIT)+ "]", + WR_CT_COMMIT); expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_collector_id", NULL, "ct_mark[16..23]", WR_CT_COMMIT); diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c index 44f74ea08..561f47695 100644 --- a/northd/en-ls-stateful.c +++ b/northd/en-ls-stateful.c @@ -411,8 +411,8 @@ ls_stateful_record_set_acl_flags_(struct ls_stateful_record *ls_stateful_rec, if (acl->tier > ls_stateful_rec->max_acl_tier) { ls_stateful_rec->max_acl_tier = acl->tier; } - if (!ls_stateful_rec->has_stateful_acl - && !strcmp(acl->action, "allow-related")) { + if (!ls_stateful_rec->has_stateful_acl && + !strcmp(acl->action, "allow-related")) { nit: Unrelated change. ls_stateful_rec->has_stateful_acl = true; } if (ls_stateful_rec->has_stateful_acl && diff --git a/northd/northd.c b/northd/northd.c index 3a488ff3d..eee5d55ff 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -124,6 +124,7 @@ static bool vxlan_mode; #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" #define REGBIT_FROM_ROUTER_PORT "reg0[18]" #define REGBIT_IP_FRAG "reg0[19]" +#define REGBIT_ACL_PERSIST_ID "reg0[20]" #define REG_ORIG_DIP_IPV4 "reg1" #define REG_ORIG_DIP_IPV6 "xxreg1" @@ -7061,7 +7062,8 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, const struct nbrec_acl *acl, bool has_stateful, const struct shash *meter_groups, uint64_t max_acl_tier, struct ds *match, struct ds *actions, - struct lflow_ref *lflow_ref) + struct lflow_ref *lflow_ref, + const struct chassis_features *features) { bool ingress = !strcmp(acl->direction, "from-lport") ? true :false; enum ovn_stage stage; @@ -7147,6 +7149,20 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, ds_truncate(actions, log_verdict_len); ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); + if (smap_get_bool(&acl->options, "bypass_match_for_established", This option name is kind of long, how about "persist_established"? + false)) { + if (!features->ct_label_flush) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "OVS does not support ct label flush. " nit: Capital CT + "bypass_match_for_established option cannot " + "be honored for ACL "UUID_FMT".", + UUID_ARGS(&acl->header_.uuid)); + } else { + ds_put_format(actions, + REGBIT_ACL_PERSIST_ID " = 1; "); + } + } + /* For stateful ACLs sample "new" and "established" packets. */ build_acl_sample_label_action(actions, acl, acl->sample_new, acl->sample_est, obs_stage); @@ -7626,6 +7642,26 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, REGBIT_ACL_HINT_ALLOW_REL" == 1", REGBIT_ACL_VERDICT_ALLOW " = 1; next;", lflow_ref); + + /* Ingress and egress ACL Table (Priority 65535). + * + * Allow traffic that is established if the ACL has a persistent + * conntrack ID configured. + */ + ds_clear(&match); + ds_put_format(&match, "ct.est && ct_mark.allow_established == 1"); + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_EVAL, UINT16_MAX, + ds_cstr(&match), + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", + lflow_ref); + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB_EVAL, UINT16_MAX, + ds_cstr(&match), + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", + lflow_ref); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL_EVAL, UINT16_MAX, + ds_cstr(&match), + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", + lflow_ref); } /* Ingress and Egress ACL Table (Priority 65532). @@ -7658,7 +7694,7 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, lflow_ref); consider_acl(lflows, od, acl, has_stateful, meter_groups, ls_stateful_rec->max_acl_tier, - &match, &actions, lflow_ref); + &match, &actions, lflow_ref, features); build_acl_sample_flows(ls_stateful_rec, od, lflows, acl, &match, &actions, sampling_apps, features, lflow_ref); @@ -7677,7 +7713,7 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, lflow_ref); consider_acl(lflows, od, acl, has_stateful, meter_groups, ls_stateful_rec->max_acl_tier, - &match, &actions, lflow_ref); + &match, &actions, lflow_ref, features); build_acl_sample_flows(ls_stateful_rec, od, lflows, acl, &match, &actions, sampling_apps, features, lflow_ref); @@ -8362,6 +8398,7 @@ build_stateful(struct ovn_datapath *od, struct lflow_table *lflows, ds_put_cstr(&actions, "ct_commit { " "ct_mark.blocked = 0; " + "ct_mark.allow_established = " REGBIT_ACL_PERSIST_ID "; " "ct_mark.obs_stage = " REGBIT_ACL_OBS_STAGE "; " "ct_mark.obs_collector_id = " REG_OBS_COLLECTOR_ID_EST "; " "ct_label.obs_point_id = " REG_OBS_POINT_ID_EST "; " @@ -8382,7 +8419,11 @@ build_stateful(struct ovn_datapath *od, struct lflow_table *lflows, * any packet that makes it this far is part of a connection we * want to allow to continue. */ ds_clear(&actions); - ds_put_cstr(&actions, "ct_commit { ct_mark.blocked = 0; }; next;"); + ds_put_cstr(&actions, + "ct_commit { " + "ct_mark.blocked = 0; " + "ct_mark.allow_established = " REGBIT_ACL_PERSIST_ID "; " + "}; next;"); ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, REGBIT_CONNTRACK_COMMIT" == 1 && " REGBIT_ACL_LABEL" == 0", diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index c4a48183d..ab1cd4344 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "7.7.0", - "cksum": "116357561 38626", + "version": "7.8.0", + "cksum": "3383352767 38626", nit: Unrelated change
I bumped the minor version here because I added a new option to ACLs. Should that not result in a version bump?
"tables": { "NB_Global": { "columns": { diff --git a/ovn-nb.xml b/ovn-nb.xml index 5114bbc2e..e6eb2400b 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2558,6 +2558,32 @@ or of all the ACLs and the default deny/allow ACLs if any. </p> </column> + + <column name="options" key="bypass_match_for_established"> + <p> + This option applies only to ACLs whose <ref column="action"/> is set + to <code>allow-related</code>. + </p> + + <p> + <code>allow-related</code> ACLs create a conntrack entry when a + packet matches the ACL's <ref column="match"/> column. Typically, + traffic must continue to match these conditions in order to continue + to be allowed by the ACL. With this option set to <code>true</code>, + then the ACL match is bypassed once the original match occurs. + Instead, a mark bit in the conntrack entry is used to allow the + traffic. This means that traffic will continue to be allowed even if + the ACL's match changes and no longer matches the established + traffic. + </p> + + <p> + The traffic will stop being allowed automatically if this option is + set to <code>false</code>, if the ACL's <ref column="action"/> is + changed to something other than <code>allow-related</code>, or if the + ACL is destroyed. + </p> + </column> </group> <group title="Logging"> diff --git a/tests/automake.mk <http://automake.mk> b/tests/automake.mk <http://automake.mk> index 3899c9e80..940f5b923 100644 --- a/tests/automake.mk <http://automake.mk> +++ b/tests/automake.mk <http://automake.mk> @@ -313,7 +313,9 @@ CHECK_PYFILES = \ tests/uuidfilt.py \ tests/test-tcp-rst.py \ tests/check_acl_log.py \ - tests/scapy-server.py + tests/scapy-server.py \ + tests/client.py \ + tests/server.py EXTRA_DIST += $(CHECK_PYFILES) PYCOV_CLEAN_FILES += $(CHECK_PYFILES:.py=.py,cover) .coverage diff --git a/tests/client.py b/tests/client.py new file mode 100755 index 000000000..1caabce7b --- /dev/null +++ b/tests/client.py @@ -0,0 +1,36 @@ +#!/usr/bin/env python3 + +import socket +import time +import argparse + + +def send_data_from_fifo_to_server( + fifo_path='/tmp/myfifo', host='127.0.0.1', port=10000 +): + # Open the FIFO for reading (blocking mode) + with open(fifo_path, 'r') as fifo_file: + with socket.socket( + socket.AF_INET, socket.SOCK_STREAM + ) as client_socket: + client_socket.connect((host, port)) + # Continuously read from the FIFO and send to the server + while True: + data = fifo_file.readline().strip() + if data: + client_socket.sendall(data.encode()) + else: + time.sleep(0.1) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + group = parser.add_argument_group() + group.add_argument("-f", "--fifo_path") + group.add_argument("-i", "--server-host") + group.add_argument("-p", "--server-port", type=int) + args = parser.parse_args() + + send_data_from_fifo_to_server( + args.fifo_path, args.server_host, args.server_port + ) diff --git a/tests/ovn-northd.at <http://ovn-northd.at> b/tests/ovn-northd.at <http://ovn-northd.at> index 4eae1c67c..a44b4c43c 100644 --- a/tests/ovn-northd.at <http://ovn-northd.at> +++ b/tests/ovn-northd.at <http://ovn-northd.at> @@ -14253,3 +14253,137 @@ AT_CHECK([grep "lr_in_dnat" lr1flows | ovn_strip_lflows | grep "30.0.0.1"], [0], AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([ACL persistent ID - Logical Flows]) +ovn_start + +dnl For this test, we want to ensure that the logical flows for ACLs are +dnl what we expect. +dnl +dnl First, we'll ensure that an ACL that does not have +dnl "bypass_match_for_established" sets the relevant CT values to 0. +dnl +dnl Then we'll change the ACL to have "bypass_match_for_established" to true +dnl and ensure the logical flows do set the appropriate values. +dnl +dnl Then finally, we'll check other ACL actions and ensure that +dnl "bypass_match_for_established" sets the relevant CT values to 0. + +check ovn-nbctl ls-add sw + +check ovn-nbctl acl-add sw from-lport 1001 "tcp" allow-related +check ovn-nbctl acl-add sw to-lport 1002 "ip" allow-related +check ovn-nbctl --apply-after-lb acl-add sw from-lport 1003 "udp" allow-related + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && (tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && (tcp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == 1 && (udp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == 1 && (udp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && (ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && (ip)), action=(reg8[[16]] = 1; next;) +]) + +ingress_uuid=$(fetch_column nb:ACL _uuid priority=1001) +egress_uuid=$(fetch_column nb:ACL _uuid priority=1002) +after_lb_uuid=$(fetch_column nb:ACL _uuid priority=1003) + +check ovn-nbctl set acl $ingress_uuid options:bypass_match_for_established=true +check ovn-nbctl set acl $egress_uuid options:bypass_match_for_established=true +check ovn-nbctl set acl $after_lb_uuid options:bypass_match_for_established=true + +dnl Now we should see the registers being set to the appropriate values. +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && (tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && (tcp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == 1 && (udp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == 1 && (udp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && (ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && (ip)), action=(reg8[[16]] = 1; next;) +]) + +dnl Now try the other ACL verdicts and ensure that they do not +dnl try to set the values. +for verdict in allow allow-stateless +do + echo "verdict is $verdict" + check ovn-nbctl set acl $ingress_uuid action=$verdict + check ovn-nbctl set acl $egress_uuid action=$verdict + check ovn-nbctl set acl $after_lb_uuid action=$verdict + + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(reg8[[16]] = 1; next;) +]) + + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(reg8[[16]] = 1; next;) +]) + + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(reg8[[16]] = 1; next;) +]) +done + +check ovn-nbctl set acl $ingress_uuid action=drop +check ovn-nbctl set acl $egress_uuid action=drop +check ovn-nbctl set acl $after_lb_uuid action=drop + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(reg8[[17]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(reg8[[17]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(reg8[[17]] = 1; next;) +]) + +check ovn-nbctl set acl $ingress_uuid action=reject +check ovn-nbctl set acl $egress_uuid action=reject +check ovn-nbctl set acl $after_lb_uuid action=reject + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(reg8[[18]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(reg8[[18]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(reg8[[18]] = 1; next;) +]) + +check ovn-nbctl set acl $ingress_uuid action=pass +check ovn-nbctl set acl $egress_uuid action=pass +check ovn-nbctl set acl $after_lb_uuid action=pass + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(next;) +]) + +AT_CLEANUP +]) diff --git a/tests/server.py b/tests/server.py new file mode 100755 index 000000000..857328a59 --- /dev/null +++ b/tests/server.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python3 + +import socket +import argparse + + +def start_server(host='127.0.0.1', port=10000): + # Create a TCP/IP socket + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as server_socket: + server_socket.bind((host, port)) + server_socket.listen() + while True: + client_socket, client_address = server_socket.accept() + with client_socket: + # Receive the data from the client in chunks and write + # to a file + data = client_socket.recv(1024) + while data: + with open("output.txt", "a") as f: + f.write(data.decode() + "\n")Should we add the newline in the server? I suppose it's unlikely that the test will ever use more than 1024 characters per line, however we could just leave the line break encoded in the stream WDYT?+ data = client_socket.recv(1024) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + group = parser.add_argument_group() + group.add_argument("-i", "--bind-host") + group.add_argument("-p", "--bind-port", type=int) + args = parser.parse_args() + + start_server(args.bind_host, args.bind_port) diff --git a/tests/system-ovn.at <http://system-ovn.at> b/tests/system-ovn.at <http://system-ovn.at> index 4452d5676..db134c2ab 100644 --- a/tests/system-ovn.at <http://system-ovn.at> +++ b/tests/system-ovn.at <http://system-ovn.at> @@ -14117,5 +14117,168 @@ as OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d /failed to query port patch-.*/d /.*terminating with signal 15.*/d"]) + +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ACLs - persistent sessions]) + +CHECK_CONNTRACK() +CHECK_CONNTRACK_NAT() +ovn_start +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +ovs-vsctl set-fail-mode br-ext standalone +# 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_daemon ovn-controller + +# For this test, we want to ensure that established traffic +# is allowed on ACLs when the bypass_match_for_established option +# is enabled. +# +# To start, we will set up allow-related ACLs. +# We will send traffic and ensure it is allowed. Then we will adjust +# the ACL so it no longer matches, and we will ensure that the traffic +# is no longer allowed. +# +# Next, we will reset the ACL to its initial state, but we will also +# change the ACL to have bypass_match_for_established enabled. +# We will flush conntrack, and rerun the test exactly as before. +# The difference this time is that after we adjust the ACL so it no +# longer matches, the traffic should still be allowed. + +check ovn-nbctl ls-add sw +check ovn-nbctl lsp-add sw swp1 -- lsp-set-addresses swp1 "00:00:00:00:00:01 192.168.1.1" +check ovn-nbctl lsp-add sw swp2 -- lsp-set-addresses swp2 "00:00:00:00:00:02 192.168.1.2" + +ADD_NAMESPACES(swp1) +ADD_VETH(swp1, swp1, br-int, "192.168.1.1/24 <http://192.168.1.1/24>", "00:00:00:00:00:01") + +ADD_NAMESPACES(swp2) +ADD_VETH(swp2, swp2, br-int, "192.168.1.2/24 <http://192.168.1.2/24>", "00:00:00:00:00:02") + +# Start a TCP server on swp2. +NETNS_DAEMONIZE(swp2, [server.py -i 192.168.1.2 -p 10000], [server.pid]) + +# Make a FIFO and send its output to a client +# from swp1 +mkfifo /tmp/myfifo +on_exit 'rm -rf /tmp/myfifo' + +NETNS_DAEMONIZE(swp1, [client.py -f "/tmp/myfifo" -i 192.168.1.2 -p 10000], [client.pid]) + +# First, ensure that we have basic connectivity before we even start setting +# up ACLs. +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +check ovn-nbctl acl-add sw from-lport 1000 'ip4.dst == 192.168.1.2' allow-related +check ovn-nbctl acl-add sw from-lport 0 '1' drop + +# Do another basic connectivity check to ensure the ACL is allowing traffic as expected. +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +# At this point, I need to adjust the ACL so it no longer matches. We then need +# to ensure that the traffic does not pass. How we test this is...interesting. I'm +# not sure how to test for a negative condition accurately. + +acl_uuid=$(fetch_column nb:ACL _uuid priority=1000) + +# Update the ACL so that it no longer matches our client-server traffic +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.3\"" + +# Send another packet from the client to the server. +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +# The traffic should be blocked. We'll check the "drop" ACL to see if it has +# been hit. We can't predict the number of packets that will be seen, but we know +# it will be non-zero. +oftable=$(ovn-debug lflow-stage-to-oftable ls_in_acl_eval) +OVS_WAIT_FOR_OUTPUT([ovs-ofctl dump-flows br-int | grep table="$oftable" | grep "priority=1000" | grep -v commit | grep -c "n_packets=[[1-9]]"], [0], [dnlMaybe a more precise match would be to use cookie with "ovn-debug uuid-to-cookie". Also is it guaranteed that it will be a single digit number of packets? Maybe it's safer to allow more than one digit.
If I understand your suggestion, the idea is to use `ovn-debug uuid-to-cookie` on the drop ACL. Then I can match on the table and cookie instead of the table and priority. Is that right?
I can also adjust the n_packet regex to match more than one digit.
+1 +]) + +# And just to be safe, let's make sure the output file is still empty +AT_CHECK([cat output.txt], [0], [dnl +]) + +# Reset the client and server processes so that we create a new connection +client_pid=$(cat client.pid) +server_pid=$(cat server.pid) +kill $server_pid +kill $client_pid +OVS_WAIT_WHILE([kill -0 $server_pid 2>/dev/null]) +OVS_WAIT_WHILE([kill -0 $client_pid 2>/dev/null]) + +NETNS_DAEMONIZE(swp2, [server.py -i 192.168.1.2 -p 20000], [server.pid]) +NETNS_DAEMONIZE(swp1, [client.py -f "/tmp/myfifo" -i 192.168.1.2 -p 20000], [client.pid]) + +# Now we'll re-set the ACL to allow the traffic. +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.2\"" + +# We'll also enable bypass_match_for_established. +check ovn-nbctl --wait=hv set ACL $acl_uuid options:bypass_match_for_established=true + +# Make sure traffic passes +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +# Adjust the ACL so that it no longer matches +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.3\"" + +# Traffic should still pass +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +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([ovn-northd]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ])-- 2.45.2_______________________________________________ dev mailing list [email protected] <mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> Thanks, Ales
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
