On Tue, Jan 14, 2025 at 11:32 PM Mark Michelson <[email protected]> wrote:
> On 1/14/25 13:30, Mark Michelson wrote: > > 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? > I don't think we need a version bump because the new option is not represented in the schema itself. > > > >> > >> "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], [dnl > >> > >> > >> Maybe 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 misunderstood how logical flow stage hints work. The drop ACL UUID is > not how the OF cookie is determined. It's based on the logical flow UUID > instead. So I would need to use `ovn-debug uuid-to-cookie` on the drop > ACL's logical flow instead. > Yes, that should make it more reliable IMO. > > 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
