On Tue, Dec 10, 2024 at 3:35 PM Mark Michelson <[email protected]> wrote:
> On 12/10/24 06:30, Ales Musil wrote: > > > > > > On Wed, Dec 4, 2024 at 10:53 PM Mark Michelson <[email protected] > > <mailto:[email protected]>> wrote: > > > > Using the ACL IDs created in the previous commit, northd adds this > ID to > > the conntrack entry for allow-established ACLs. > > > > ovn-controller keeps track of the southbound ACL IDs. If an ACL ID is > > removed from the southbound database, then ovn-controller flushes the > > conntrack entry whose label contains the deleted ACL ID. > > > > With this change, it means that deleting an allow-established ACL, or > > changing its action type to something else, will result in the > conntrack > > entry being flushed. This will cause the traffic to no longer > > automatically be allowed. > > > > Reported-at: https://issues.redhat.com/browse/FDP-815 > > <https://issues.redhat.com/browse/FDP-815> > > Signed-off-by: Mark Michelson <[email protected] > > <mailto:[email protected]>> > > --- > > > > > > Hi Mark, > > > > thank you for the patch. We are missing the check that ct flush with the > > label is supported. I have some additional comments down below. > > I have a question about this. What is the appropriate action to take if > ct flush with label is not supported? If we can't flush the ct entries, > then there is a risk that the ct entries will remain indefinitely. My > instinct is to: > > 1) Check for the feature in all instances of ovn-controller. > 2) In northd, if all instances of ovn-controller support ct flush with > label, then proceed as normal. > 3) In northd, if any instances of ovn-controller do not support ct flush > with label, then treat "allow-established" exactly the same as > "allow-related" ACLs, and emit a warning. > > What do you think? > Yeah that sounds like a good way to approach this. > > > > controller/acl_ids.c | 279 > ++++++++++++++++++++++++++++++++++++ > > controller/acl_ids.h | 30 ++++ > > controller/automake.mk <http://automake.mk> | 2 + > > controller/ovn-controller.c | 12 +- > > lib/logical-fields.c | 2 + > > northd/en-acl-ids.c | 13 ++ > > northd/en-acl-ids.h | 3 + > > northd/en-lflow.c | 1 + > > northd/inc-proc-northd.c | 1 + > > northd/northd.c | 54 +++++-- > > northd/northd.h | 1 + > > tests/ovn-northd.at <http://ovn-northd.at> | 20 ++- > > tests/system-ovn.at <http://system-ovn.at> | 20 +++ > > 13 files changed, 417 insertions(+), 21 deletions(-) > > create mode 100644 controller/acl_ids.c > > create mode 100644 controller/acl_ids.h > > > > diff --git a/controller/acl_ids.c b/controller/acl_ids.c > > new file mode 100644 > > index 000000000..6267f3399 > > --- /dev/null > > +++ b/controller/acl_ids.c > > @@ -0,0 +1,279 @@ > > +/* Copyright (c) 2024, Red Hat, Inc. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > <http://www.apache.org/licenses/LICENSE-2.0> > > + * > > + * Unless required by applicable law or agreed to in writing, > software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > > implied. > > + * See the License for the specific language governing permissions > and > > + * limitations under the License. > > + */ > > + > > +#include <config.h> > > + > > +#include "openvswitch/hmap.h" > > +#include "openvswitch/rconn.h" > > +#include "openvswitch/ofp-ct.h" > > +#include "openvswitch/ofp-util.h" > > +#include "openvswitch/ofp-msgs.h" > > +#include "openvswitch/vlog.h" > > +#include "lib/socket-util.h" > > + > > +#include "lib/ovn-sb-idl.h" > > +#include "acl_ids.h" > > + > > +VLOG_DEFINE_THIS_MODULE(acl_ids); > > + > > +enum acl_id_state { > > + /* The ID exists in the SB DB. */ > > + ACTIVE, > > + /* The ID has been removed from the DB and needs to have its > > conntrack > > + * entries flushed. > > + */ > > + SB_DELETED, > > + /* We have sent the conntrack flush request to OVS for this ACL > > ID. */ > > + FLUSHING, > > + /* We have either successfully flushed the ID, or we have > > failed enough > > + * times that we have given up. > > + */ > > + TO_DELETE, > > +}; > > + > > +struct acl_id { > > + int64_t id; > > + enum acl_id_state state; > > + struct hmap_node hmap_node; > > + ovs_be32 xid; > > + int flush_count; > > +}; > > + > > +struct tracked_acl_ids { > > + struct hmap ids; > > +}; > > + > > +static struct acl_id * > > +find_tracked_acl_id(struct tracked_acl_ids *tracked_ids, int64_t id) > > +{ > > + uint32_t hash = hash_uint64(id); > > + struct acl_id *acl_id; > > + HMAP_FOR_EACH_WITH_HASH (acl_id, hmap_node, hash, > > &tracked_ids->ids) { > > + if (acl_id->id == id) { > > + return acl_id; > > + } > > + } > > + return NULL; > > +} > > + > > +static void > > +acl_id_destroy(struct acl_id *acl_id) > > +{ > > + free(acl_id); > > +} > > + > > +void * > > +en_acl_id_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + struct tracked_acl_ids *ids = xzalloc(sizeof *ids); > > + hmap_init(&ids->ids); > > + return ids; > > +} > > + > > +void > > +en_acl_id_run(struct engine_node *node, void *data) > > +{ > > + const struct sbrec_acl_id_table *sb_acl_id_table = > > + EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); > > + const struct sbrec_acl_id *sb_id; > > + > > + struct tracked_acl_ids *ids = data; > > + struct acl_id *id; > > + > > + /* Pre-mark each active ID as SB_DELETED. */ > > + HMAP_FOR_EACH (id, hmap_node, &ids->ids) { > > + if (id->state == ACTIVE) { > > + id->state = SB_DELETED; > > + } > > + } > > + > > + SBREC_ACL_ID_TABLE_FOR_EACH (sb_id, sb_acl_id_table) { > > + id = find_tracked_acl_id(ids, sb_id->id); > > + if (!id) { > > + id = xzalloc(sizeof *id); > > + id->id = sb_id->id; > > + hmap_insert(&ids->ids, &id->hmap_node, > > hash_uint64(sb_id->id)); > > + } > > + id->state = ACTIVE; > > + } > > + > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +void > > +en_acl_id_cleanup(void *data) > > +{ > > + struct tracked_acl_ids *tracked_ids = data; > > + struct acl_id *id; > > + HMAP_FOR_EACH_POP (id, hmap_node, &tracked_ids->ids) { > > + acl_id_destroy(id); > > + } > > + hmap_destroy(&tracked_ids->ids); > > +} > > + > > +static struct rconn *swconn; > > +static ovs_be32 barrier_xid; > > + > > +void > > +acl_ids_update_swconn(const char *target, int probe_interval) > > +{ > > + if (!swconn) { > > + swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << > OFP15_VERSION); > > + } > > + ovn_update_swconn_at(swconn, target, probe_interval, "acl_ids"); > > > > > > > > I wonder if we need to have separate connection just to flush those > > entries, could we utilize ofctrl as we do for flushing of LB backends? > > With that it would be enough to just keep the bitmap of ids to flush. > WDYT? > > I can look into this. I suppose as long as I keep track of the xids of > the flushes and barriers, then I can identify whether an error I receive > is for a flush. > > > > > +} > > + > > +#define MAX_FLUSHES 3 > > + > > +static void > > +acl_ids_handle_rconn_msg(struct ofpbuf *msg, struct tracked_acl_ids > > *acl_ids) > > +{ > > + const struct ofp_header *oh = msg->data; > > + > > + enum ofptype type; > > + ofptype_decode(&type, oh); > > + > > + if (type == OFPTYPE_ECHO_REQUEST) { > > + rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL); > > + return; > > + } > > + > > + struct acl_id *acl_id; > > + if (oh->xid != barrier_xid) { > > + if (type != OFPTYPE_ERROR) { > > + return; > > + } > > + /* Uh oh! It looks like one of the flushes failed :( > > + * Let's find this particular one and move its state > > + * back to SB_DELETED so we can retry the flush. Of > > + * course, if this is a naughty little ID and has > > + * been flushed unsuccessfully too many times, we'll > > + * set it to TO_DELETE so it doesn't cause any more > > + * trouble. > > + */ > > + HMAP_FOR_EACH (acl_id, hmap_node, &acl_ids->ids) { > > + if (acl_id->xid != oh->xid) { > > + continue; > > + } > > + > > + acl_id->xid = 0; > > + acl_id->flush_count++; > > + if (acl_id->flush_count >= MAX_FLUSHES) { > > + acl_id->state = TO_DELETE; > > + } else { > > + acl_id->state = SB_DELETED; > > + } > > + > > + break; > > + } > > + } else { > > + HMAP_FOR_EACH (acl_id, hmap_node, &acl_ids->ids) { > > + if (acl_id->state != FLUSHING) { > > + continue; > > + } > > + acl_id->state = TO_DELETE; > > + } > > + barrier_xid = 0; > > + } > > +} > > + > > +static void > > +flush_expired_ids(struct tracked_acl_ids *acl_ids) > > +{ > > + if (barrier_xid != 0) { > > + /* We haven't received the previous barrier's reply, so > > + * hold off on sending new flushes until we get the > > + * reply. > > + */ > > + return; > > + } > > + > > + ovs_u128 mask = { > > + /* ct_labels.label BITS[80-95] */ > > + .u64.hi = 0xffff0000, > > + }; > > + struct acl_id *acl_id; > > + bool send_barrier = false; > > + HMAP_FOR_EACH (acl_id, hmap_node, &acl_ids->ids) { > > + if (acl_id->state != SB_DELETED) { > > + continue; > > + } > > + ovs_u128 ct_id = { > > + .u64.hi = acl_id->id << 16, > > + }; > > + VLOG_DBG("Flushing conntrack entry for ACL id %"PRId64, > > acl_id->id); > > + struct ofp_ct_match match = { > > + .labels = ct_id, > > + .labels_mask = mask, > > + }; > > + struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL, > > + > > rconn_get_version(swconn)); > > + const struct ofp_header *oh = msg->data; > > + acl_id->xid = oh->xid; > > + acl_id->state = FLUSHING; > > + rconn_send(swconn, msg, NULL); > > + send_barrier = true; > > + } > > + > > + if (!send_barrier) { > > + return; > > + } > > + > > + struct ofpbuf *barrier = > > ofputil_encode_barrier_request(OFP15_VERSION); > > + const struct ofp_header *oh = barrier->data; > > + barrier_xid = oh->xid; > > + rconn_send(swconn, barrier, NULL); > > +} > > + > > +static void > > +clear_flushed_ids(struct tracked_acl_ids *acl_ids) > > +{ > > + struct acl_id *acl_id; > > + HMAP_FOR_EACH_SAFE (acl_id, hmap_node, &acl_ids->ids) { > > + if (acl_id->state != TO_DELETE) { > > + continue; > > + } > > + hmap_remove(&acl_ids->ids, &acl_id->hmap_node); > > + acl_id_destroy(acl_id); > > + } > > +} > > + > > +#define MAX_RECV_MSGS 50 > > + > > +void > > +acl_ids_run(struct tracked_acl_ids *acl_ids) > > +{ > > + rconn_run(swconn); > > + if (!rconn_is_connected(swconn)) { > > + rconn_run_wait(swconn); > > + rconn_recv_wait(swconn); > > + return; > > + } > > + > > + for (int i = 0; i < MAX_RECV_MSGS; i++) { > > + struct ofpbuf *msg = rconn_recv(swconn); > > + if (!msg) { > > + break; > > + } > > + acl_ids_handle_rconn_msg(msg, acl_ids); > > + ofpbuf_delete(msg); > > + } > > + flush_expired_ids(acl_ids); > > + clear_flushed_ids(acl_ids); > > + > > + rconn_run_wait(swconn); > > + rconn_recv_wait(swconn); > > +} > > diff --git a/controller/acl_ids.h b/controller/acl_ids.h > > new file mode 100644 > > index 000000000..e3556c37e > > --- /dev/null > > +++ b/controller/acl_ids.h > > @@ -0,0 +1,30 @@ > > +/* Copyright (c) 2024 Red Hat, INc. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > <http://www.apache.org/licenses/LICENSE-2.0> > > + * > > + * Unless required by applicable law or agreed to in writing, > software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > > implied. > > + * See the License for the specific language governing permissions > and > > + * limitations under the License. > > + */ > > + > > +#ifndef OVN_ACL_IDS_H > > +#define OVN_ACL_IDS_H > > + > > +#include <config.h> > > +#include "lib/inc-proc-eng.h" > > + > > +void *en_acl_id_init(struct engine_node *, struct engine_arg *); > > +void en_acl_id_run(struct engine_node *, void *); > > +void en_acl_id_cleanup(void *); > > + > > +struct tracked_acl_ids; > > +void acl_ids_update_swconn(const char *target, int probe_interval); > > +void acl_ids_run(struct tracked_acl_ids *); > > + > > +#endif /* OVN_ACL_IDS_H */ > > diff --git a/controller/automake.mk <http://automake.mk> > > b/controller/automake.mk <http://automake.mk> > > index bb0bf2d33..c19ae27be 100644 > > --- a/controller/automake.mk <http://automake.mk> > > +++ b/controller/automake.mk <http://automake.mk> > > @@ -1,5 +1,7 @@ > > bin_PROGRAMS += controller/ovn-controller > > controller_ovn_controller_SOURCES = \ > > + controller/acl_ids.c \ > > + controller/acl_ids.h \ > > controller/bfd.c \ > > controller/bfd.h \ > > controller/binding.c \ > > diff --git a/controller/ovn-controller.c > b/controller/ovn-controller.c > > index 157def2a3..f19c291d6 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -88,6 +88,7 @@ > > #include "lib/dns-resolve.h" > > #include "ct-zone.h" > > #include "ovn-dns.h" > > +#include "acl_ids.h" > > > > VLOG_DEFINE_THIS_MODULE(main); > > > > @@ -864,7 +865,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > SB_NODE(fdb, "fdb") \ > > SB_NODE(meter, "meter") \ > > SB_NODE(static_mac_binding, "static_mac_binding") \ > > - SB_NODE(chassis_template_var, "chassis_template_var") > > + SB_NODE(chassis_template_var, "chassis_template_var") \ > > + SB_NODE(acl_id, "acl_id") > > > > enum sb_engine_node { > > #define SB_NODE(NAME, NAME_STR) SB_##NAME, > > @@ -5095,6 +5097,7 @@ main(int argc, char *argv[]) > > ENGINE_NODE(mac_cache, "mac_cache"); > > ENGINE_NODE(bfd_chassis, "bfd_chassis"); > > ENGINE_NODE(dns_cache, "dns_cache"); > > + ENGINE_NODE(acl_id, "acl_id"); > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > > SB_NODES > > @@ -5303,6 +5306,9 @@ main(int argc, char *argv[]) > > engine_add_input(&en_controller_output, &en_bfd_chassis, > > controller_output_bfd_chassis_handler); > > > > + engine_add_input(&en_acl_id, &en_sb_acl_id, NULL); > > + engine_add_input(&en_controller_output, &en_acl_id, NULL); > > + > > struct engine_arg engine_arg = { > > .sb_idl = ovnsb_idl_loop.idl, > > .ovs_idl = ovs_idl_loop.idl, > > @@ -5538,6 +5544,8 @@ main(int argc, char *argv[]) > > br_int_remote.probe_interval); > > pinctrl_update_swconn(br_int_remote.target, > > br_int_remote.probe_interval); > > + acl_ids_update_swconn(br_int_remote.target, > > + br_int_remote.probe_interval); > > > > /* Enable ACL matching for double tagged traffic. */ > > if (ovs_idl_txn && cfg) { > > @@ -5842,6 +5850,8 @@ main(int argc, char *argv[]) > > !ovnsb_idl_txn, > !ovs_idl_txn); > > > stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, > > time_msec()); > > + > > + acl_ids_run(engine_get_data(&en_acl_id)); > > } > > } > > > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > > index 5b578b5c2..89431b939 100644 > > --- a/lib/logical-fields.c > > +++ b/lib/logical-fields.c > > @@ -192,6 +192,8 @@ ovn_init_symtab(struct shash *symtab) > > "ct_label[96..127]", > > WR_CT_COMMIT); > > expr_symtab_add_subfield_scoped(symtab, "ct_label.obs_unused", > > NULL, > > "ct_label[0..95]", > WR_CT_COMMIT); > > + expr_symtab_add_subfield_scoped(symtab, "ct_label.acl_id", NULL, > > + "ct_label[80..95]", > WR_CT_COMMIT); > > > > expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, > > false); > > > > diff --git a/northd/en-acl-ids.c b/northd/en-acl-ids.c > > index 897cc2da1..cb220dd80 100644 > > --- a/northd/en-acl-ids.c > > +++ b/northd/en-acl-ids.c > > @@ -204,3 +204,16 @@ void sync_acl_ids(const struct acl_id_data > > *id_data, > > } > > } > > } > > + > > +int64_t > > +get_acl_id(const struct acl_id_data *acl_id_data, const struct > > nbrec_acl *acl) > > +{ > > + struct acl_id *acl_id; > > + uint32_t hash = uuid_hash(&acl->header_.uuid); > > + HMAP_FOR_EACH_WITH_HASH (acl_id, node, hash, &acl_id_data->ids) > { > > + if (uuid_equals(&acl_id->nb_acl_uuid, &acl->header_.uuid)) { > > + return acl_id->id; > > + } > > + } > > + return 0; > > +} > > > > > > As mentioned on the previous patch, if we do the UUID 1:1 mapping this > > search becomes redundant. > > > > diff --git a/northd/en-acl-ids.h b/northd/en-acl-ids.h > > index 8b60b3c7c..8bea2e078 100644 > > --- a/northd/en-acl-ids.h > > +++ b/northd/en-acl-ids.h > > @@ -14,4 +14,7 @@ void en_acl_id_cleanup(void *data); > > struct acl_id_data; > > void sync_acl_ids(const struct acl_id_data *, struct ovsdb_idl_txn > *, > > struct ovsdb_idl_index *sbrec_acl_id_by_id); > > + > > +struct nbrec_acl; > > +int64_t get_acl_id(const struct acl_id_data *, const struct > > nbrec_acl *); > > #endif > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > > index fa1f0236d..3d57a469b 100644 > > --- a/northd/en-lflow.c > > +++ b/northd/en-lflow.c > > @@ -96,6 +96,7 @@ lflow_get_input_data(struct engine_node *node, > > struct ed_type_sampling_app_data *sampling_app_data = > > engine_get_input_data("sampling_app", node); > > lflow_input->sampling_apps = &sampling_app_data->apps; > > + lflow_input->acl_id_data = engine_get_input_data("acl_id", > node); > > } > > > > void en_lflow_run(struct engine_node *node, void *data) > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 43abf042d..04c208c97 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -293,6 +293,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > *nb, > > engine_add_input(&en_lflow, &en_port_group, > > lflow_port_group_handler); > > engine_add_input(&en_lflow, &en_lr_stateful, > > lflow_lr_stateful_handler); > > engine_add_input(&en_lflow, &en_ls_stateful, > > lflow_ls_stateful_handler); > > + engine_add_input(&en_lflow, &en_acl_id, NULL); > > > > engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL); > > engine_add_input(&en_sync_to_sb_addr_set, &en_lr_stateful, > NULL); > > diff --git a/northd/northd.c b/northd/northd.c > > index 8dab88f62..081305b09 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -206,6 +206,9 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); > > #define REG_OBS_COLLECTOR_ID_NEW "reg8[0..7]" > > #define REG_OBS_COLLECTOR_ID_EST "reg8[8..15]" > > > > +/* Register used for storing persistent ACL IDs */ > > +#define REG_ACL_ID "reg7[0..15]" > > + > > /* Register used for temporarily store ECMP eth.src to avoid > > masked ct_label > > * access. It doesn't really occupy registers because the content > > of the > > * register is saved to stack and then restored in the same flow. > > @@ -7063,7 +7066,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 acl_id_data *acl_id_data) > > { > > bool ingress = !strcmp(acl->direction, "from-lport") ? true > > :false; > > enum ovn_stage stage; > > @@ -7151,8 +7155,17 @@ consider_acl(struct lflow_table *lflows, > > const struct ovn_datapath *od, > > ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > > > > if (!strcmp(acl->action, "allow-established")) { > > - ds_put_format(actions, > > - REGBIT_ACL_PERSIST_ID " = 1; "); > > + int64_t id = get_acl_id(acl_id_data, acl); > > + if (id) { > > + ds_put_format(actions, > > + REG_ACL_ID " = %"PRId64 "; " > > + REGBIT_ACL_PERSIST_ID " = 1; ", > > + id); > > + } else { > > + static struct vlog_rate_limit rl = > > VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_WARN_RL(&rl, "No ID found for ACL "UUID_FMT" > > (%s)", > > + UUID_ARGS(&acl->header_.uuid), > > acl->match); > > + } > > } > > > > /* For stateful ACLs sample "new" and "established" > > packets. */ > > @@ -7476,7 +7489,8 @@ build_acls(const struct ls_stateful_record > > *ls_stateful_rec, > > const struct shash *meter_groups, > > const struct sampling_app_table *sampling_apps, > > const struct chassis_features *features, > > - struct lflow_ref *lflow_ref) > > + struct lflow_ref *lflow_ref, > > + const struct acl_id_data *acl_id_data) > > { > > const char *default_acl_action = default_acl_drop > > ? debug_implicit_drop_action() > > @@ -7686,7 +7700,8 @@ 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, > > + acl_id_data); > > build_acl_sample_flows(ls_stateful_rec, od, lflows, acl, > > &match, &actions, sampling_apps, > > features, lflow_ref); > > @@ -7705,7 +7720,8 @@ 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, > > + acl_id_data); > > build_acl_sample_flows(ls_stateful_rec, od, > > lflows, acl, > > &match, &actions, > > sampling_apps, > > features, lflow_ref); > > @@ -8394,6 +8410,7 @@ build_stateful(struct ovn_datapath *od, struct > > lflow_table *lflows, > > "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 "; " > > + "ct_label.acl_id = " REG_ACL_ID "; " > > "}; next;"); > > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > > REGBIT_CONNTRACK_COMMIT" == 1 && " > > @@ -8415,6 +8432,7 @@ build_stateful(struct ovn_datapath *od, struct > > lflow_table *lflows, > > "ct_commit { " > > "ct_mark.blocked = 0; " > > "ct_mark.allow_established = " > > REGBIT_ACL_PERSIST_ID "; " > > + "ct_label.acl_id = " REG_ACL_ID "; " > > "}; next;"); > > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > > REGBIT_CONNTRACK_COMMIT" == 1 && " > > @@ -17216,7 +17234,8 @@ build_ls_stateful_flows(const struct > > ls_stateful_record *ls_stateful_rec, > > const struct shash *meter_groups, > > const struct sampling_app_table > > *sampling_apps, > > const struct chassis_features *features, > > - struct lflow_table *lflows) > > + struct lflow_table *lflows, > > + const struct acl_id_data *acl_id_data) > > { > > build_ls_stateful_rec_pre_acls(ls_stateful_rec, od, ls_pgs, > > lflows, > > ls_stateful_rec->lflow_ref); > > @@ -17225,7 +17244,8 @@ build_ls_stateful_flows(const struct > > ls_stateful_record *ls_stateful_rec, > > build_acl_hints(ls_stateful_rec, od, lflows, > > ls_stateful_rec->lflow_ref); > > build_acls(ls_stateful_rec, od, lflows, ls_pgs, meter_groups, > > - sampling_apps, features, ls_stateful_rec->lflow_ref); > > + sampling_apps, features, ls_stateful_rec->lflow_ref, > > + acl_id_data); > > build_lb_hairpin(ls_stateful_rec, od, lflows, > > ls_stateful_rec->lflow_ref); > > } > > > > @@ -17253,6 +17273,7 @@ struct lswitch_flow_build_info { > > struct hmap *parsed_routes; > > struct hmap *route_policies; > > struct simap *route_tables; > > + const struct acl_id_data *acl_id_data; > > }; > > > > /* Helper function to combine all lflow generation which is > > iterated by > > @@ -17552,7 +17573,8 @@ build_lflows_thread(void *arg) > > lsi->meter_groups, > > lsi->sampling_apps, > > lsi->features, > > - lsi->lflows); > > + lsi->lflows, > > + lsi->acl_id_data); > > } > > } > > > > @@ -17629,7 +17651,8 @@ build_lswitch_and_lrouter_flows( > > const struct sampling_app_table *sampling_apps, > > struct hmap *parsed_routes, > > struct hmap *route_policies, > > - struct simap *route_tables) > > + struct simap *route_tables, > > + const struct acl_id_data *acl_id_data) > > { > > > > char *svc_check_match = xasprintf("eth.dst == %s", > > svc_monitor_mac); > > @@ -17667,6 +17690,7 @@ build_lswitch_and_lrouter_flows( > > lsiv[index].parsed_routes = parsed_routes; > > lsiv[index].route_tables = route_tables; > > lsiv[index].route_policies = route_policies; > > + lsiv[index].acl_id_data = acl_id_data; > > ds_init(&lsiv[index].match); > > ds_init(&lsiv[index].actions); > > > > @@ -17713,6 +17737,7 @@ build_lswitch_and_lrouter_flows( > > .route_policies = route_policies, > > .match = DS_EMPTY_INITIALIZER, > > .actions = DS_EMPTY_INITIALIZER, > > + .acl_id_data = acl_id_data, > > }; > > > > /* Combined build - all lflow generation from lswitch and > > lrouter > > @@ -17786,7 +17811,8 @@ build_lswitch_and_lrouter_flows( > > lsi.meter_groups, > > lsi.sampling_apps, > > lsi.features, > > - lsi.lflows); > > + lsi.lflows, > > + lsi.acl_id_data); > > } > > stopwatch_stop(LFLOWS_LS_STATEFUL_STOPWATCH_NAME, > > time_msec()); > > stopwatch_start(LFLOWS_IGMP_STOPWATCH_NAME, time_msec()); > > @@ -17879,7 +17905,8 @@ void build_lflows(struct ovsdb_idl_txn > > *ovnsb_txn, > > input_data->sampling_apps, > > input_data->parsed_routes, > > input_data->route_policies, > > - input_data->route_tables); > > + input_data->route_tables, > > + input_data->acl_id_data); > > > > if (parallelization_state == STATE_INIT_HASH_SIZES) { > > parallelization_state = STATE_USE_PARALLELIZATION; > > @@ -18306,7 +18333,8 @@ lflow_handle_ls_stateful_changes(struct > > ovsdb_idl_txn *ovnsb_txn, > > lflow_input->meter_groups, > > lflow_input->sampling_apps, > > lflow_input->features, > > - lflows); > > + lflows, > > + lflow_input->acl_id_data); > > > > /* Sync the new flows to SB. */ > > bool handled = lflow_ref_sync_lflows( > > diff --git a/northd/northd.h b/northd/northd.h > > index bccb1c5d8..175c318af 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -232,6 +232,7 @@ struct lflow_input { > > struct hmap *parsed_routes; > > struct hmap *route_policies; > > struct simap *route_tables; > > + const struct acl_id_data *acl_id_data; > > }; > > > > extern int parallelization_state; > > diff --git a/tests/ovn-northd.at <http://ovn-northd.at> > > b/tests/ovn-northd.at <http://ovn-northd.at> > > index 1d90622e1..b5b94330b 100644 > > --- a/tests/ovn-northd.at <http://ovn-northd.at> > > +++ b/tests/ovn-northd.at <http://ovn-northd.at> > > @@ -14297,21 +14297,27 @@ after_lb_uuid=$(fetch_column nb:ACL _uuid > > priority=1003) > > > > check ovn-nbctl set acl $ingress_uuid action=allow-established > > check ovn-nbctl set acl $egress_uuid action=allow-established > > -check ovn-nbctl set acl $after_lb_uuid action=allow-established > > +check ovn-nbctl --wait=sb set acl $after_lb_uuid > > action=allow-established > > + > > +dnl Retrieve the IDs for the ACLs so we can check them properly. > > + > > +ingress_id=$(fetch_column ACL_ID id nb_acl=$ingress_uuid) > > +egress_id=$(fetch_column ACL_ID id nb_acl=$egress_uuid) > > +after_lb_id=$(fetch_column ACL_ID id nb_acl=$after_lb_uuid) > > > > 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;) > > +AT_CHECK_UNQUOTED([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; reg7[[0..15]] > > = $ingress_id; 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;) > > +AT_CHECK_UNQUOTED([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; reg7[[0..15]] = $after_lb_id; 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;) > > +AT_CHECK_UNQUOTED([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; reg7[[0..15]] > > = $egress_id; reg0[[20]] = 1; next;) > > table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] > > == 1 && (ip)), action=(reg8[[16]] = 1; next;) > > ]) > > > > > > > > We should also check that the flow indeed commits the ID into ct_label. > > > > > > diff --git a/tests/system-ovn.at <http://system-ovn.at> > > b/tests/system-ovn.at <http://system-ovn.at> > > index a6ddda0f7..27e112a26 100644 > > --- a/tests/system-ovn.at <http://system-ovn.at> > > +++ b/tests/system-ovn.at <http://system-ovn.at> > > @@ -14251,6 +14251,18 @@ test > > > > : > output.txt > > > > +# Get the ID for this ACL > > +acl_id=$(fetch_column ACL_ID id nb_acl=$acl_uuid) > > +acl_id=$(printf %x $acl_id) > > + > > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | > > FORMAT_CT(192.168.1.2) | \ > > +grep "labels=0x"$acl_id"00000000000000000000" | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > > +sed -e 's/mark=[[0-9]]*/mark=<cleared>/' | \ > > +sed -e 's/labels=0x[[0-9a-f]]*/labels=<cleared>/'], [0], [dnl > > > > +tcp,orig=(src=192.168.1.1,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=192.168.1.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=<cleared>,protoinfo=(state=<cleared>) > > +]) > > + > > # Adjust the ACL so that it no longer matches > > check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == > 192.168.1.3\"" > > > > @@ -14264,6 +14276,14 @@ test > > > > : > output.txt > > > > +# Now remove the ACL. This should remove the conntrack entry as > well. > > +check ovn-nbctl --wait=hv acl-del sw from-lport 1000 'ip4.dst == > > 192.168.1.3' > > + > > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | > > FORMAT_CT(192.168.1.2) | \ > > +grep "labels=0x"$acl_id"00000000000000000000" | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > > +]) > > + > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > > > as ovn-sb > > -- > > 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
