| Message ID | 20241204215246.642636-3-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/3] northd: Add "allow-established" ACL action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Wed, Dec 4, 2024 at 10:53 PM Mark Michelson <mmichels@redhat.com> 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 > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > 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. controller/acl_ids.c | 279 ++++++++++++++++++++++++++++++++++++ > controller/acl_ids.h | 30 ++++ > controller/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 | 20 ++- > tests/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 > + * > + * 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? +} > + > +#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 > + * > + * 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 b/controller/automake.mk > index bb0bf2d33..c19ae27be 100644 > --- a/controller/automake.mk > +++ b/controller/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 b/tests/ovn-northd.at > index 1d90622e1..b5b94330b 100644 > --- a/tests/ovn-northd.at > +++ b/tests/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 b/tests/system-ovn.at > index a6ddda0f7..27e112a26 100644 > --- a/tests/system-ovn.at > +++ b/tests/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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
On 12/10/24 06:30, Ales Musil wrote: > > > On Wed, Dec 4, 2024 at 10:53 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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 <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > --- > > > 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? > > 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 > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > Thanks, > Ales
On Tue, Dec 10, 2024 at 3:35 PM Mark Michelson <mmichels@redhat.com> wrote: > On 12/10/24 06:30, Ales Musil wrote: > > > > > > On Wed, Dec 4, 2024 at 10:53 PM Mark Michelson <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> 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 <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> > > --- > > > > > > 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 > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > > > > Thanks, > > Ales > >
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 + * + * 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"); +} + +#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 + * + * 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 b/controller/automake.mk index bb0bf2d33..c19ae27be 100644 --- a/controller/automake.mk +++ b/controller/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; +} 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 b/tests/ovn-northd.at index 1d90622e1..b5b94330b 100644 --- a/tests/ovn-northd.at +++ b/tests/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;) ]) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index a6ddda0f7..27e112a26 100644 --- a/tests/system-ovn.at +++ b/tests/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
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 Signed-off-by: Mark Michelson <mmichels@redhat.com> --- controller/acl_ids.c | 279 ++++++++++++++++++++++++++++++++++++ controller/acl_ids.h | 30 ++++ controller/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 | 20 ++- tests/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