On Mon, Dec 16, 2024 at 8:03 PM Mark Michelson <[email protected]> wrote:
> Using the ACL IDs created in the previous commit, northd adds this ID to > the conntrack entry for allow-established ACLs. > > ovn-controller keeps track of the southbound ACL IDs. If an ACL ID is > removed from the southbound database, then ovn-controller flushes the > conntrack entry whose label contains the deleted ACL ID. > > With this change, it means that deleting an allow-established ACL, or > changing its action type to something else, will result in the conntrack > entry being flushed. This will cause the traffic to no longer > automatically be allowed. > > Reported-at: https://issues.redhat.com/browse/FDP-815 > Signed-off-by: Mark Michelson <[email protected]> > --- > Hi Mark, thank you for the follow up, I have one small comment down below. v2 -> v3: > * There are no functional changes in this version. > > v1 -> v2: > * We no longer use a separate rconn. We hook into the ofctrl code > instead. (There is probably some room for some refactoring in > ofctrl.c, similar to what is implemented in features.c, but the > implementation introduced in this patch is not horrible). > * We store both the ID and UUID of the southbound ACL ID in the engine > node data. This way, if an ID is reused within the same transaction > by northd, we will recognize it as a distinct ACL ID and flush the > old entry. Since we are using the ofctrl rconn, it means we will > simultaneously flush the CT entry with the old ID and install flows > that use the reused ID. Therefore, there is no risk of accidentally > flushing CT entries that are using the reused ID. > > Feedback not addressed in this version: > * It was suggested in tests/ovn-northd.at to check that the flow > commits the ID into ct_label. The flow that commits the ID in to > ct_label is static. It always commits the value of reg7[0..15] to the > label. What varies is whether reg7[0..15] gets set to the value of > the ID. Therefore, checking the ct_commit action doesn't give any > extra information. > --- > controller/acl_ids.c | 245 ++++++++++++++++++++++++++++++++++++ > controller/acl_ids.h | 43 +++++++ > controller/automake.mk | 2 + > controller/ofctrl.c | 87 ++++++++----- > controller/ofctrl.h | 7 +- > controller/ovn-controller.c | 17 ++- > lib/logical-fields.c | 2 + > northd/en-lflow.c | 2 + > northd/inc-proc-northd.c | 3 +- > northd/northd.c | 52 ++++++-- > northd/northd.h | 1 + > tests/ovn-northd.at | 18 ++- > tests/system-ovn.at | 20 +++ > 13 files changed, 445 insertions(+), 54 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..24d0d4071 > --- /dev/null > +++ b/controller/acl_ids.c > @@ -0,0 +1,245 @@ > +/* 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/ovn-sb-idl.h" > +#include "ovn/features.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, > +}; > + > +struct acl_id { > + int64_t id; > + struct uuid uuid; > + enum acl_id_state state; > + struct hmap_node hmap_node; > + ovs_be32 xid; > + ovs_be32 barrier_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, > + const struct sbrec_acl_id *sb_id) > +{ > + uint32_t hash = hash_2words(uuid_hash(&sb_id->header_.uuid), > + hash_uint64(sb_id->id)); > + struct acl_id *acl_id; > + HMAP_FOR_EACH_WITH_HASH (acl_id, hmap_node, hash, &tracked_ids->ids) { > + if (acl_id->id == sb_id->id && > + uuid_equals(&sb_id->header_.uuid, &acl_id->uuid)) { > + 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) > +{ > + if (!ovs_feature_is_supported(OVS_CT_LABEL_FLUSH_SUPPORT)) { > + engine_set_node_state(node, EN_UNCHANGED); > + return; > + } > + > + 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); > + if (!id) { > + id = xzalloc(sizeof *id); > + id->id = sb_id->id; > + id->uuid = sb_id->header_.uuid; > + > + uint32_t hash = hash_2words(uuid_hash(&sb_id->header_.uuid), > + hash_uint64(sb_id->id)); > + hmap_insert(&ids->ids, &id->hmap_node, hash); > + } > + 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); > +} > + > +void > +acl_ids_handle_barrier_reply(struct tracked_acl_ids *tracked_acl_ids, > + ovs_be32 barrier_xid) > +{ > + struct acl_id *acl_id; > + HMAP_FOR_EACH_SAFE (acl_id, hmap_node, &tracked_acl_ids->ids) { > + if (acl_id->state != FLUSHING || acl_id->barrier_xid != > barrier_xid) { > + continue; > + } > + hmap_remove(&tracked_acl_ids->ids, &acl_id->hmap_node); > + acl_id_destroy(acl_id); > + } > +} > + > +#define MAX_FLUSHES 3 > + > +bool > +acl_ids_handle_non_barrier_reply(const struct ofp_header *oh, > + enum ofptype type, > + struct tracked_acl_ids *tracked_acl_ids) > +{ > + /* Since ofctrl_run() runs before engine_run(), there is a chance that > + * tracked_acl_ids may be NULL. It shouldn't happen since the ofctrl > + * state machine has to advance to the point that it can even call > this > + * function, but this is a safeguard. > + */ > + if (!tracked_acl_ids) { > + return false; > + } > + > + if (type != OFPTYPE_ERROR) { > + return false; > + } > + > + struct acl_id *acl_id; > + bool handled = false; > + HMAP_FOR_EACH_SAFE (acl_id, hmap_node, &tracked_acl_ids->ids) { > + if (acl_id->xid != oh->xid) { > + continue; > + } > + handled = true; > + > + /* 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 > + * delete it since we are unlikely to be able to > + * successfully flush it. > + */ > + acl_id->xid = 0; > + acl_id->barrier_xid = 0; > + acl_id->flush_count++; > + if (acl_id->flush_count >= MAX_FLUSHES) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Failed to flush conntrack entry for ACL id > " > + "%"PRId64".", acl_id->id); > + hmap_remove(&tracked_acl_ids->ids, &acl_id->hmap_node); > + acl_id_destroy(acl_id); > + } else { > + acl_id->state = SB_DELETED; > + } > + break; > + } > + > + return handled; > +} > + > +void > +acl_ids_flush_expired(struct tracked_acl_ids *acl_ids, int rconn_version, > + struct ovs_list *msgs) > +{ > + ovs_u128 mask = { > + /* ct_labels.label BITS[80-95] */ > + .u64.hi = 0xffff0000, > + }; > + struct acl_id *acl_id; > + 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_version); > + const struct ofp_header *oh = msg->data; > + acl_id->xid = oh->xid; > + acl_id->state = FLUSHING; > + ovs_list_push_back(msgs, &msg->list_node); > + } > +} > + > +void > +acl_ids_record_barrier_xid(struct tracked_acl_ids *acl_ids, > + ovs_be32 barrier_xid) > +{ > + struct acl_id *acl_id; > + HMAP_FOR_EACH (acl_id, hmap_node, &acl_ids->ids) { > + if (acl_id->state == FLUSHING && !acl_id->barrier_xid) { > + acl_id->barrier_xid = barrier_xid; > + } > + } > +} > diff --git a/controller/acl_ids.h b/controller/acl_ids.h > new file mode 100644 > index 000000000..09680af2c > --- /dev/null > +++ b/controller/acl_ids.h > @@ -0,0 +1,43 @@ > +/* 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" > +#include "openvswitch/types.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; > +struct ofp_header; > +enum ofptype; > +struct ovs_list; > + > +void acl_ids_handle_barrier_reply(struct tracked_acl_ids *acl_ids, > + ovs_be32 barrier_xid); > +bool acl_ids_handle_non_barrier_reply(const struct ofp_header *oh, > + enum ofptype type, > + struct tracked_acl_ids *acl_ids); > +void acl_ids_flush_expired(struct tracked_acl_ids *acl_ids, > + int rconn_version, > + struct ovs_list *msgs); > +void acl_ids_record_barrier_xid(struct tracked_acl_ids *acl_ids, > + ovs_be32 barrier_xid); > + > +#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/ofctrl.c b/controller/ofctrl.c > index f9387d375..36efebf1c 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -54,6 +54,7 @@ > #include "vswitch-idl.h" > #include "ovn-sb-idl.h" > #include "ct-zone.h" > +#include "acl_ids.h" > > VLOG_DEFINE_THIS_MODULE(ofctrl); > > @@ -449,7 +450,8 @@ run_S_NEW(void) > static void > recv_S_NEW(const struct ofp_header *oh OVS_UNUSED, > enum ofptype type OVS_UNUSED, > - struct shash *pending_ct_zones OVS_UNUSED) > + struct shash *pending_ct_zones OVS_UNUSED, > + struct tracked_acl_ids *tracked_acl_ids OVS_UNUSED) > { > OVS_NOT_REACHED(); > } > @@ -537,7 +539,8 @@ process_tlv_table_reply(const struct > ofputil_tlv_table_reply *reply) > > static void > recv_S_TLV_TABLE_REQUESTED(const struct ofp_header *oh, enum ofptype type, > - struct shash *pending_ct_zones OVS_UNUSED) > + struct shash *pending_ct_zones OVS_UNUSED, > + struct tracked_acl_ids *tracked_acl_ids > OVS_UNUSED) > { > if (oh->xid != xid) { > ofctrl_recv(oh, type); > @@ -593,7 +596,8 @@ run_S_TLV_TABLE_MOD_SENT(void) > > static void > recv_S_TLV_TABLE_MOD_SENT(const struct ofp_header *oh, enum ofptype type, > - struct shash *pending_ct_zones OVS_UNUSED) > + struct shash *pending_ct_zones OVS_UNUSED, > + struct tracked_acl_ids *tracked_acl_ids > OVS_UNUSED) > { > if (oh->xid != xid && oh->xid != xid2) { > ofctrl_recv(oh, type); > @@ -648,7 +652,8 @@ run_S_WAIT_BEFORE_CLEAR(void) > > static void > recv_S_WAIT_BEFORE_CLEAR(const struct ofp_header *oh, enum ofptype type, > - struct shash *pending_ct_zones OVS_UNUSED) > + struct shash *pending_ct_zones OVS_UNUSED, > + struct tracked_acl_ids *tracked_acl_ids > OVS_UNUSED) > { > ofctrl_recv(oh, type); > } > @@ -700,7 +705,8 @@ run_S_CLEAR_FLOWS(void) > > static void > recv_S_CLEAR_FLOWS(const struct ofp_header *oh, enum ofptype type, > - struct shash *pending_ct_zones OVS_UNUSED) > + struct shash *pending_ct_zones OVS_UNUSED, > + struct tracked_acl_ids *tracked_acl_ids OVS_UNUSED) > { > ofctrl_recv(oh, type); > } > @@ -722,32 +728,46 @@ run_S_UPDATE_FLOWS(void) > } > > static void > -recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum ofptype type, > - struct shash *pending_ct_zones) > +flow_updates_handle_barrier_reply(const struct ofp_header *oh, > + struct shash *pending_ct_zones) > { > - if (type == OFPTYPE_BARRIER_REPLY && > !ovs_list_is_empty(&flow_updates)) { > - struct ofctrl_flow_update *fup = > ofctrl_flow_update_from_list_node( > - ovs_list_front(&flow_updates)); > - if (fup->xid == oh->xid) { > - if (fup->req_cfg >= cur_cfg) { > - cur_cfg = fup->req_cfg; > - } > - mem_stats.oflow_update_usage -= ofctrl_flow_update_size(fup); > - ovs_list_remove(&fup->list_node); > - free(fup); > + if (ovs_list_is_empty(&flow_updates)) { > + return; > + } > + > + struct ofctrl_flow_update *fup = ofctrl_flow_update_from_list_node( > + ovs_list_front(&flow_updates)); > + if (fup->xid == oh->xid) { > + if (fup->req_cfg >= cur_cfg) { > + cur_cfg = fup->req_cfg; > } > + mem_stats.oflow_update_usage -= ofctrl_flow_update_size(fup); > + ovs_list_remove(&fup->list_node); > + free(fup); > + } > > - /* If the barrier xid is associated with an outstanding conntrack > - * flush, the flush succeeded. Move the pending ct zone entry > - * to the next stage. */ > - struct shash_node *iter; > - SHASH_FOR_EACH(iter, pending_ct_zones) { > - struct ct_zone_pending_entry *ctzpe = iter->data; > - if (ctzpe->state == CT_ZONE_OF_SENT && ctzpe->of_xid == > oh->xid) { > - ctzpe->state = CT_ZONE_DB_QUEUED; > - } > + /* If the barrier xid is associated with an outstanding conntrack > + * flush, the flush succeeded. Move the pending ct zone entry > + * to the next stage. */ > + struct shash_node *iter; > + SHASH_FOR_EACH (iter, pending_ct_zones) { > + struct ct_zone_pending_entry *ctzpe = iter->data; > + if (ctzpe->state == CT_ZONE_OF_SENT && ctzpe->of_xid == oh->xid) { > + ctzpe->state = CT_ZONE_DB_QUEUED; > } > - } else { > + } > + > +} > + > +static void > +recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum ofptype type, > + struct shash *pending_ct_zones, > + struct tracked_acl_ids *tracked_acl_ids) > +{ > + if (type == OFPTYPE_BARRIER_REPLY) { > + flow_updates_handle_barrier_reply(oh, pending_ct_zones); > + acl_ids_handle_barrier_reply(tracked_acl_ids, oh->xid); > + } else if (!acl_ids_handle_non_barrier_reply(oh, type, > tracked_acl_ids)) { > ofctrl_recv(oh, type); > } > } > @@ -774,7 +794,8 @@ ofctrl_get_mf_field_id(void) > bool > ofctrl_run(const char *conn_target, int probe_interval, > const struct ovsrec_open_vswitch_table *ovs_table, > - struct shash *pending_ct_zones) > + struct shash *pending_ct_zones, > + struct tracked_acl_ids *tracked_acl_ids) > { > bool reconnected = false; > > @@ -832,7 +853,8 @@ ofctrl_run(const char *conn_target, int probe_interval, > error = ofptype_decode(&type, oh); > if (!error) { > switch (state) { > -#define STATE(NAME) case NAME: recv_##NAME(oh, type, pending_ct_zones); > break; > +#define STATE(NAME) case NAME: recv_##NAME(oh, type, pending_ct_zones, \ > + tracked_acl_ids); break; > STATES > #undef STATE > default: > @@ -2666,7 +2688,8 @@ ofctrl_put(struct ovn_desired_flow_table > *lflow_table, > struct ovsdb_idl_index *sbrec_meter_by_name, > uint64_t req_cfg, > bool lflows_changed, > - bool pflows_changed) > + bool pflows_changed, > + struct tracked_acl_ids *tracked_acl_ids) > { > static bool skipped_last_time = false; > static uint64_t old_req_cfg = 0; > @@ -2901,6 +2924,8 @@ ofctrl_put(struct ovn_desired_flow_table > *lflow_table, > } > } > > + acl_ids_flush_expired(tracked_acl_ids, rconn_get_version(swconn), > &msgs); > + > if (!ovs_list_is_empty(&msgs)) { > /* Add a barrier to the list of messages. */ > struct ofpbuf *barrier = > ofputil_encode_barrier_request(OFP15_VERSION); > @@ -2908,6 +2933,8 @@ ofctrl_put(struct ovn_desired_flow_table > *lflow_table, > ovs_be32 xid_ = oh->xid; > ovs_list_push_back(&msgs, &barrier->list_node); > > + acl_ids_record_barrier_xid(tracked_acl_ids, xid_); > + > /* Queue the messages. */ > struct ofpbuf *msg; > LIST_FOR_EACH_POP (msg, list_node, &msgs) { > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > index 129e3b6ad..a7e3f29c2 100644 > --- a/controller/ofctrl.h > +++ b/controller/ofctrl.h > @@ -32,6 +32,7 @@ struct ovsrec_bridge; > struct ovsrec_open_vswitch_table; > struct sbrec_meter_table; > struct shash; > +struct tracked_acl_ids; > > struct ovn_desired_flow_table { > /* Hash map flow table using flow match conditions as hash key.*/ > @@ -52,7 +53,8 @@ void ofctrl_init(struct ovn_extend_table *group_table, > struct ovn_extend_table *meter_table); > bool ofctrl_run(const char *conn_target, int probe_interval, > const struct ovsrec_open_vswitch_table *ovs_table, > - struct shash *pending_ct_zones); > + struct shash *pending_ct_zones, > + struct tracked_acl_ids *tracked_acl_ids); > enum mf_field_id ofctrl_get_mf_field_id(void); > void ofctrl_put(struct ovn_desired_flow_table *lflow_table, > struct ovn_desired_flow_table *pflow_table, > @@ -61,7 +63,8 @@ void ofctrl_put(struct ovn_desired_flow_table > *lflow_table, > struct ovsdb_idl_index *sbrec_meter_by_name, > uint64_t nb_cfg, > bool lflow_changed, > - bool pflow_changed); > + bool pflow_changed, > + struct tracked_acl_ids *tracked_acl_ids); > bool ofctrl_has_backlog(void); > void ofctrl_wait(void); > void ofctrl_destroy(void); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 157def2a3..9be577224 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); > nit: We should add an empty handler. > + > struct engine_arg engine_arg = { > .sb_idl = ovnsb_idl_loop.idl, > .ovs_idl = ovs_idl_loop.idl, > @@ -5455,6 +5461,7 @@ main(int argc, char *argv[]) > > /* Main loop. */ > bool sb_monitor_all = false; > + struct tracked_acl_ids *tracked_acl_ids = NULL; > while (!exit_args.exiting) { > memory_run(); > if (memory_should_report()) { > @@ -5609,7 +5616,8 @@ main(int argc, char *argv[]) > if (ofctrl_run(br_int_remote.target, > br_int_remote.probe_interval, ovs_table, > ct_zones_data ? &ct_zones_data->ctx.pending > - : NULL)) { > + : NULL, > + tracked_acl_ids)) { > static struct vlog_rate_limit rl > = VLOG_RATE_LIMIT_INIT(1, 1); > > @@ -5656,6 +5664,8 @@ main(int argc, char *argv[]) > bool recompute_allowed = (ovnsb_idl_txn && > !ofctrl_has_backlog()); > engine_run(recompute_allowed); > + tracked_acl_ids = engine_get_data(&en_acl_id); > + > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, > time_msec()); > if (engine_has_updated()) { > @@ -5824,7 +5834,8 @@ main(int argc, char *argv[]) > sbrec_meter_by_name, > ofctrl_seqno_get_req_cfg(), > engine_node_changed(&en_lflow_output), > - engine_node_changed(&en_pflow_output)); > + engine_node_changed(&en_pflow_output), > + tracked_acl_ids); > stopwatch_stop(OFCTRL_PUT_STOPWATCH_NAME, > time_msec()); > } > stopwatch_start(OFCTRL_SEQNO_RUN_STOPWATCH_NAME, > 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-lflow.c b/northd/en-lflow.c > index fa1f0236d..8c500a055 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -65,6 +65,8 @@ lflow_get_input_data(struct engine_node *node, > EN_OVSDB_GET(engine_get_input("SB_igmp_group", node)); > lflow_input->sbrec_logical_dp_group_table = > EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); > + lflow_input->sbrec_acl_id_table = > + EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); > > lflow_input->sbrec_mcast_group_by_name_dp = > engine_ovsdb_node_get_index( > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 8025f1c28..694d03992 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -291,6 +291,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_sb_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); > @@ -341,6 +342,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_sync_from_sb, &en_sb_port_binding, NULL); > engine_add_input(&en_sync_from_sb, &en_sb_ha_chassis_group, NULL); > > + engine_add_input(&en_northd_output, &en_acl_id, NULL); > engine_add_input(&en_northd_output, &en_sync_from_sb, NULL); > engine_add_input(&en_northd_output, &en_sync_to_sb, > northd_output_sync_to_sb_handler); > @@ -350,7 +352,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > northd_output_mac_binding_aging_handler); > engine_add_input(&en_northd_output, &en_fdb_aging, > northd_output_fdb_aging_handler); > - engine_add_input(&en_northd_output, &en_acl_id, NULL); > > struct engine_arg engine_arg = { > .nb_idl = nb->idl, > diff --git a/northd/northd.c b/northd/northd.c > index eee5d55ff..602da215f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -205,6 +205,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 shash *meter_groups, uint64_t max_acl_tier, > struct ds *match, struct ds *actions, > struct lflow_ref *lflow_ref, > - const struct chassis_features *features) > + const struct chassis_features *features, > + const struct sbrec_acl_id_table *sbrec_acl_id_table) > { > bool ingress = !strcmp(acl->direction, "from-lport") ? true :false; > enum ovn_stage stage; > @@ -7151,15 +7155,24 @@ consider_acl(struct lflow_table *lflows, const > struct ovn_datapath *od, > > if (smap_get_bool(&acl->options, "bypass_match_for_established", > false)) { > + const struct sbrec_acl_id *sb_id; > + sb_id = sbrec_acl_id_table_get_for_uuid(sbrec_acl_id_table, > + &acl->header_.uuid); > if (!features->ct_label_flush) { > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "OVS does not support ct label flush. " > "bypass_match_for_established option cannot " > "be honored for ACL "UUID_FMT".", > UUID_ARGS(&acl->header_.uuid)); > + } else if (!sb_id) { > + 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); > } else { > ds_put_format(actions, > - REGBIT_ACL_PERSIST_ID " = 1; "); > + REG_ACL_ID " = %"PRId64 "; " > + REGBIT_ACL_PERSIST_ID " = 1; ", > + sb_id->id); > } > } > > @@ -7484,7 +7497,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 sbrec_acl_id_table *sbrec_acl_id_table) > { > const char *default_acl_action = default_acl_drop > ? debug_implicit_drop_action() > @@ -7694,7 +7708,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, features); > + &match, &actions, lflow_ref, features, > + sbrec_acl_id_table); > build_acl_sample_flows(ls_stateful_rec, od, lflows, acl, > &match, &actions, sampling_apps, > features, lflow_ref); > @@ -7713,7 +7728,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, features); > + &match, &actions, lflow_ref, features, > + sbrec_acl_id_table); > build_acl_sample_flows(ls_stateful_rec, od, lflows, acl, > &match, &actions, sampling_apps, > features, lflow_ref); > @@ -8402,6 +8418,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 && " > @@ -8423,6 +8440,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 && " > @@ -17224,7 +17242,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 sbrec_acl_id_table > *sbrec_acl_id_table) > { > build_ls_stateful_rec_pre_acls(ls_stateful_rec, od, ls_pgs, lflows, > ls_stateful_rec->lflow_ref); > @@ -17233,7 +17252,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, > + sbrec_acl_id_table); > build_lb_hairpin(ls_stateful_rec, od, lflows, > ls_stateful_rec->lflow_ref); > } > > @@ -17261,6 +17281,7 @@ struct lswitch_flow_build_info { > struct hmap *parsed_routes; > struct hmap *route_policies; > struct simap *route_tables; > + const struct sbrec_acl_id_table *sbrec_acl_id_table; > }; > > /* Helper function to combine all lflow generation which is iterated by > @@ -17560,7 +17581,8 @@ build_lflows_thread(void *arg) > lsi->meter_groups, > lsi->sampling_apps, > lsi->features, > - lsi->lflows); > + lsi->lflows, > + lsi->sbrec_acl_id_table); > } > } > > @@ -17637,7 +17659,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 sbrec_acl_id_table *sbrec_acl_id_table) > { > > char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); > @@ -17675,6 +17698,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].sbrec_acl_id_table = sbrec_acl_id_table; > ds_init(&lsiv[index].match); > ds_init(&lsiv[index].actions); > > @@ -17721,6 +17745,7 @@ build_lswitch_and_lrouter_flows( > .route_policies = route_policies, > .match = DS_EMPTY_INITIALIZER, > .actions = DS_EMPTY_INITIALIZER, > + .sbrec_acl_id_table = sbrec_acl_id_table, > }; > > /* Combined build - all lflow generation from lswitch and lrouter > @@ -17794,7 +17819,8 @@ build_lswitch_and_lrouter_flows( > lsi.meter_groups, > lsi.sampling_apps, > lsi.features, > - lsi.lflows); > + lsi.lflows, > + lsi.sbrec_acl_id_table); > } > stopwatch_stop(LFLOWS_LS_STATEFUL_STOPWATCH_NAME, time_msec()); > stopwatch_start(LFLOWS_IGMP_STOPWATCH_NAME, time_msec()); > @@ -17887,7 +17913,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->sbrec_acl_id_table); > > if (parallelization_state == STATE_INIT_HASH_SIZES) { > parallelization_state = STATE_USE_PARALLELIZATION; > @@ -18314,7 +18341,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->sbrec_acl_id_table); > > /* Sync the new flows to SB. */ > bool handled = lflow_ref_sync_lflows( > diff --git a/northd/northd.h b/northd/northd.h > index bccb1c5d8..1c3eacf83 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -210,6 +210,7 @@ struct lflow_input { > const struct sbrec_multicast_group_table *sbrec_multicast_group_table; > const struct sbrec_igmp_group_table *sbrec_igmp_group_table; > const struct sbrec_logical_dp_group_table > *sbrec_logical_dp_group_table; > + const struct sbrec_acl_id_table *sbrec_acl_id_table; > > /* Indexes */ > struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp; > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index a44b4c43c..7b2c64f7a 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -14299,19 +14299,25 @@ check ovn-nbctl set acl $ingress_uuid > options:bypass_match_for_established=true > check ovn-nbctl set acl $egress_uuid > options:bypass_match_for_established=true > check ovn-nbctl set acl $after_lb_uuid > options:bypass_match_for_established=true > > +dnl Retrieve the IDs for the ACLs so we can check them properly. > + > +ingress_id=$(ovn-sbctl get ACL_ID $ingress_uuid id) > +egress_id=$(ovn-sbctl get ACL_ID $egress_uuid id) > +after_lb_id=$(ovn-sbctl get ACL_ID $after_lb_uuid id) > + > 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 db134c2ab..28920875c 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -14252,6 +14252,18 @@ test > > : > output.txt > > +# Get the ID for this ACL > +acl_id=$(ovn-sbctl get ACL_ID $acl_uuid id) > +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\"" > > @@ -14265,6 +14277,14 @@ test > > : > output.txt > > +# Now remove the ACL. This should remove the conntrack entry as well. > +check ovn-nbctl --wait=hv acl-del sw from-lport 1000 'ip4.dst == > 192.168.1.3' > + > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | > FORMAT_CT(192.168.1.2) | \ > +grep "labels=0x"$acl_id"00000000000000000000" | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > +]) > + > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > -- > 2.45.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > With the above addressed: Acked-by: Ales Musil <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
