On Fri, Jan 17, 2025 at 5:13 AM Mark Michelson <[email protected]> wrote:
> This introduces a new southbound table called ACL_ID for storing > persistent ACL conntrack IDs. These IDs are generated by ovn-northd for > any allow-related ACL that has the "persist-established" option set. > > Signed-off-by: Mark Michelson <[email protected]> > --- > v4 -> v5: > * No changes, other than conforming to the option name changed > introduced in patch 2. > > v3 -> v4: > * Rebased. > * Removed unnecesssary NULL checks for engine context transactions. > * Removed unused ACL ID index. > * Added a no-op handler that returns true for the new input to > en_northd_output. > > v2 -> v3: > * Since there are now two factors that determine if an ACL should be > synced to the SB DB, there is a helper function in en-acl-ids.c that > can be used to tell if an ACL is a candidate for having an ID synced. > > v1 -> v2: > * Formatting issues are fixed. > * UUIDs of NB ACLs and SB ACL_IDs are the same. > * There is no separate sync function. The incremental processing > run function syncs the NB and SB data. > * The engine node no longer needs to allocate data. > > Feedback not addressed in this version: > * The bitmap_scan() code is unchanged. This is because a change > in the next patch addresses the issue that Ales pointed out. > * I did not add a comment around the ovsdb_idl_omit_alert() call > because the comment at the top of the section already says > what was suggested. > --- > > northd/automake.mk | 2 + > northd/en-acl-ids.c | 100 ++++++++++++++++++++++++++++++++++++++ > northd/en-acl-ids.h | 13 +++++ > northd/en-northd-output.c | 8 +++ > northd/en-northd-output.h | 2 + > northd/inc-proc-northd.c | 10 +++- > northd/northd.h | 3 ++ > northd/ovn-northd.c | 4 ++ > ovn-sb.ovsschema | 12 +++-- > ovn-sb.xml | 13 +++++ > tests/ovn.at | 40 +++++++++++++++ > 11 files changed, 203 insertions(+), 4 deletions(-) > create mode 100644 northd/en-acl-ids.c > create mode 100644 northd/en-acl-ids.h > > diff --git a/northd/automake.mk b/northd/automake.mk > index 6566ad299..d46dfd763 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -34,6 +34,8 @@ northd_ovn_northd_SOURCES = \ > northd/en-ls-stateful.h \ > northd/en-sampling-app.c \ > northd/en-sampling-app.h \ > + northd/en-acl-ids.c \ > + northd/en-acl-ids.h \ > northd/inc-proc-northd.c \ > northd/inc-proc-northd.h \ > northd/ipam.c \ > diff --git a/northd/en-acl-ids.c b/northd/en-acl-ids.c > new file mode 100644 > index 000000000..998b7a7fa > --- /dev/null > +++ b/northd/en-acl-ids.c > @@ -0,0 +1,100 @@ > +/* > + * 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 "en-acl-ids.h" > +#include "lib/uuidset.h" > +#include "lib/ovn-sb-idl.h" > +#include "lib/ovn-nb-idl.h" > +#include "lib/bitmap.h" > + > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(northd_acl_ids); > + > +#define MAX_ACL_ID 65535 > + > +void * > +en_acl_id_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +static bool > +should_sync_to_sb(const struct nbrec_acl *nb_acl) > +{ > + return !strcmp(nb_acl->action, "allow-related") && > + smap_get_bool(&nb_acl->options, > + "persist-established", > + false); > +} > + > +void > +en_acl_id_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + const struct nbrec_acl_table *nb_acl_table = > + EN_OVSDB_GET(engine_get_input("NB_acl", node)); > + const struct sbrec_acl_id_table *sb_acl_id_table = > + EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); > + struct uuidset visited = UUIDSET_INITIALIZER(&visited); > + unsigned long *id_bitmap = bitmap_allocate(MAX_ACL_ID); > + > + const struct nbrec_acl *nb_acl; > + const struct sbrec_acl_id *sb_id; > + SBREC_ACL_ID_TABLE_FOR_EACH_SAFE (sb_id, sb_acl_id_table) { > + nb_acl = nbrec_acl_table_get_for_uuid(nb_acl_table, > + &sb_id->header_.uuid); > + if (nb_acl && should_sync_to_sb(nb_acl)) { > + bitmap_set1(id_bitmap, sb_id->id); > + uuidset_insert(&visited, &sb_id->header_.uuid); > + } else { > + sbrec_acl_id_delete(sb_id); > + } > + } > + > + size_t scan_start = 1; > + size_t scan_end = MAX_ACL_ID; > + NBREC_ACL_TABLE_FOR_EACH (nb_acl, nb_acl_table) { > + if (uuidset_find_and_delete(&visited, &nb_acl->header_.uuid)) { > + continue; > + } > + if (!should_sync_to_sb(nb_acl)) { > + continue; > + } > + int64_t new_id = bitmap_scan(id_bitmap, 0, > + scan_start, scan_end + 1); > + if (new_id == scan_end + 1) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Exhausted all ACL IDs"); > + break; > + } > + sb_id = sbrec_acl_id_insert_persist_uuid(eng_ctx->ovnsb_idl_txn, > + &nb_acl->header_.uuid); > + sbrec_acl_id_set_id(sb_id, new_id); > + bitmap_set1(id_bitmap, new_id); > + scan_start = new_id + 1; > + } > + > + engine_set_node_state(node, EN_UPDATED); > + uuidset_destroy(&visited); > + bitmap_free(id_bitmap); > +} > + > +void > +en_acl_id_cleanup(void *data OVS_UNUSED) > +{ > +} > diff --git a/northd/en-acl-ids.h b/northd/en-acl-ids.h > new file mode 100644 > index 000000000..f878f55d9 > --- /dev/null > +++ b/northd/en-acl-ids.h > @@ -0,0 +1,13 @@ > +#ifndef EN_ACL_IDS_H > +#define EN_ACL_IDS_H > + > +#include <config.h> > +#include <stdbool.h> > + > +#include "lib/inc-proc-eng.h" > + > +bool northd_acl_id_handler(struct engine_node *node, void *data); > +void *en_acl_id_init(struct engine_node *, struct engine_arg *); > +void en_acl_id_run(struct engine_node *, void *data); > +void en_acl_id_cleanup(void *data); > +#endif > diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c > index 98098d974..1736a26bb 100644 > --- a/northd/en-northd-output.c > +++ b/northd/en-northd-output.c > @@ -72,3 +72,11 @@ northd_output_fdb_aging_handler(struct engine_node > *node, > engine_set_node_state(node, EN_UPDATED); > return true; > } > + > +bool > +northd_output_acl_id_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h > index 5f577b89c..424f98253 100644 > --- a/northd/en-northd-output.h > +++ b/northd/en-northd-output.h > @@ -17,5 +17,7 @@ bool northd_output_mac_binding_aging_handler(struct > engine_node *node, > void *data OVS_UNUSED); > bool northd_output_fdb_aging_handler(struct engine_node *node, > void *data OVS_UNUSED); > +bool northd_output_acl_id_handler(struct engine_node *node, > + void *data OVS_UNUSED); > > #endif > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 6e0aa04c4..0a6fdfb3b 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -41,6 +41,7 @@ > #include "en-sampling-app.h" > #include "en-sync-sb.h" > #include "en-sync-from-sb.h" > +#include "en-acl-ids.h" > #include "unixctl.h" > #include "util.h" > > @@ -102,7 +103,8 @@ static unixctl_cb_func chassis_features_list; > SB_NODE(fdb, "fdb") \ > SB_NODE(static_mac_binding, "static_mac_binding") \ > SB_NODE(chassis_template_var, "chassis_template_var") \ > - SB_NODE(logical_dp_group, "logical_dp_group") > + SB_NODE(logical_dp_group, "logical_dp_group") \ > + SB_NODE(acl_id, "acl_id") > > enum sb_engine_node { > #define SB_NODE(NAME, NAME_STR) SB_##NAME, > @@ -161,6 +163,7 @@ static ENGINE_NODE(route_policies, "route_policies"); > static ENGINE_NODE(routes, "routes"); > static ENGINE_NODE(bfd, "bfd"); > static ENGINE_NODE(bfd_sync, "bfd_sync"); > +static ENGINE_NODE(acl_id, "acl_id"); > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > struct ovsdb_idl_loop *sb) > @@ -186,6 +189,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > global_config_sb_chassis_handler); > engine_add_input(&en_global_config, &en_sampling_app, NULL); > > + engine_add_input(&en_acl_id, &en_nb_acl, NULL); > + engine_add_input(&en_acl_id, &en_sb_acl_id, NULL); > + > engine_add_input(&en_northd, &en_nb_mirror, NULL); > engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); > engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL); > @@ -344,6 +350,8 @@ 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, > + northd_output_acl_id_handler); > > struct engine_arg engine_arg = { > .nb_idl = nb->idl, > diff --git a/northd/northd.h b/northd/northd.h > index 9457a7be6..67f7b2a59 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -63,6 +63,9 @@ struct northd_input { > struct eth_addr svc_monitor_mac_ea; > const struct chassis_features *features; > > + /* ACL ID inputs. */ > + const struct acl_id_data *acl_id_data; > + > /* Indexes */ > struct ovsdb_idl_index *sbrec_chassis_by_name; > struct ovsdb_idl_index *sbrec_chassis_by_hostname; > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index aa4c57663..562572efe 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -941,6 +941,10 @@ main(int argc, char *argv[]) > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > &sbrec_logical_dp_group_columns[i]); > } > + for (size_t i = 0; i < SBREC_ACL_ID_N_COLUMNS; i++) { > + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > + &sbrec_acl_id_columns[i]); > + } > > unixctl_command_register("sb-connection-status", "", 0, 0, > ovn_conn_show, ovnsb_idl_loop.idl); > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > index e461a518b..682cee6e5 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > - "version": "20.39.0", > - "cksum": "6416280 33547", > + "version": "20.40.0", > + "cksum": "3668645444 31756", > "tables": { > "SB_Global": { > "columns": { > @@ -653,6 +653,12 @@ > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > "indexes": [["datapath", "logical_port", "ip_prefix", > "nexthop"]], > - "isRoot": true} > + "isRoot": true}, > + "ACL_ID": { > + "columns": { > + "id": {"type": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger": 32767}}}}, > + "isRoot": true} > } > } > diff --git a/ovn-sb.xml b/ovn-sb.xml > index f612eb2b7..59f4518cd 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -5316,4 +5316,17 @@ tcp.flags = RST; > See <em>External IDs</em> at the beginning of this document. > </column> > </table> > + > + <table name="ACL_ID"> > + <p> > + Each record represents an identifier that <code>ovn-northd</code> > needs > + to synchronize with instances of <code>ovn-controller</code>. The > UUID > + of each record corresponds directly with an <ref table="ACL"/> > record > + in the northbound database. > + </p> > + <column name="id"> > + An identifier corresponding to a northbound > + <code>allow-established</code> ACL. > + </column> > + </table> > </database> > diff --git a/tests/ovn.at b/tests/ovn.at > index ae316d19e..206361f2c 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -40753,3 +40753,43 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int > table=OFTABLE_PHY_TO_LOG | grep -v > OVN_CLEANUP([hv1],[hv2]) > AT_CLEANUP > ]) > + > +AT_SETUP([ACL Conntrack ID propagation]) > +ovn_start > + > +dnl In this test, we want to ensure that southbound ACL_ID > +dnl entries are created for northbound ACLs of type "allow-related" > +dnl when the "persist-established" option is set to true. > +dnl > +dnl If an ACL is of a different type, does not have the option set > +dnl to true, or is deleted, then there should be no soutbhound ACL_ID. > + > +check ovn-nbctl ls-add sw > +check ovn-nbctl --wait=sb acl-add sw from-lport 1000 1 allow-related > +acl_uuid=$(fetch_column nb:ACL _uuid priority=1000) > + > +dnl The ACL is not have the bypass option set, so SBDB should have no > rows. > +wait_row_count ACL_ID 0 > + > +dnl When we set the option, the SBDB should pick it up. > +check ovn-nbctl --wait=sb set ACL $acl_uuid > options:persist-established=true > + > +wait_row_count ACL_ID 1 > + > +dnl Setting to a new action should remove the row from the SBDB. > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=drop > + > +wait_row_count ACL_ID 0 > + > +dnl Set back to allow-related and make sure it shows up in the SBDB. > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-related > + > +wait_row_count ACL_ID 1 > + > +dnl Delete the ACL and ensure the SBDB entry is deleted. > +check ovn-nbctl --wait=sb acl-del sw from-lport 1000 1 > + > +wait_row_count ACL_ID 0 > + > +AT_CLEANUP > +]) > -- > 2.45.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
