| Message ID | 20241204215246.642636-2-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: > This introduces a new southbound table called ACL_ID for storing > persistent ACL conntrack IDs. These IDs are generated by ovn-northd for > any ACL that is of type "allow-established". > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > Hi Mark, thank you for the patch, I have a few comments down below. > northd/automake.mk | 2 + > northd/en-acl-ids.c | 206 +++++++++++++++++++++++++++++++++++++++ > northd/en-acl-ids.h | 17 ++++ > northd/en-northd.c | 6 ++ > northd/inc-proc-northd.c | 15 ++- > northd/northd.c | 3 + > northd/northd.h | 4 + > northd/ovn-northd.c | 4 + > ovn-sb.ovsschema | 14 ++- > ovn-sb.xml | 13 +++ > tests/ovn.at | 39 ++++++++ > 11 files changed, 319 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..897cc2da1 > --- /dev/null > +++ b/northd/en-acl-ids.c > @@ -0,0 +1,206 @@ > +/* > + * 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); > + > +enum id_state { > + /* The ID represents a new northbound ACL that has > + * not yet been synced to the southbound DB > + */ > + ID_NEW, > + /* The ID represents an ACL ID that has been synced > + * with the southbound DB already > + */ > + ID_SYNCED, > + /* The ID represents a deleted NB ACL that also needs > + * to be removed from the southbound DB > + */ > + ID_INACTIVE, > +}; > + > +struct acl_id { > + struct hmap_node node; > + int64_t id; > + struct uuid nb_acl_uuid; > + enum id_state state; > +}; > + > +#define MAX_ACL_ID 65535 > + > +struct acl_id_data { > + struct hmap ids; > + unsigned long *id_bitmap; > +}; > + > +static void > +acl_id_data_init(struct acl_id_data *id_data) > +{ > + hmap_init(&id_data->ids); > + id_data->id_bitmap = bitmap_allocate(MAX_ACL_ID); > +} > + > +static struct acl_id_data * > +acl_id_data_alloc(void) > +{ > + struct acl_id_data *id_data = xzalloc(sizeof *id_data); > + acl_id_data_init(id_data); > + > + return id_data; > +} > + > +static void > +acl_id_data_destroy(struct acl_id_data *id_data) > +{ > + struct acl_id *acl_id; > + HMAP_FOR_EACH_POP (acl_id, node, &id_data->ids) { > + free(acl_id); > + } > + hmap_destroy(&id_data->ids); > + bitmap_free(id_data->id_bitmap); > +} > + > +void * > +en_acl_id_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct acl_id_data *id_data = acl_id_data_alloc(); > + return id_data; > +} > + > +static void > +add_acl_id(struct hmap *id_map, int64_t id, enum id_state state, > + const struct uuid *acl_uuid) > +{ > + struct acl_id *acl_id = xzalloc(sizeof *acl_id); > nit: We don't need to zero-alloc when it's populated right away. + acl_id->id = id; > + acl_id->state = state; > + acl_id->nb_acl_uuid = *acl_uuid; > + hmap_insert(id_map, &acl_id->node, uuid_hash(acl_uuid)); > +} > + > +void > +en_acl_id_run(struct engine_node *node, void *data) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + if (!eng_ctx->ovnnb_idl_txn || !eng_ctx->ovnsb_idl_txn) { > + return; > + } > + > + 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); > + struct acl_id_data *id_data = data; > + > + acl_id_data_destroy(id_data); > + acl_id_data_init(id_data); > + > + const struct nbrec_acl *nb_acl; > + const struct sbrec_acl_id *sb_id; > + SBREC_ACL_ID_TABLE_FOR_EACH (sb_id, sb_acl_id_table) { > + nb_acl = nbrec_acl_table_get_for_uuid(nb_acl_table, > &sb_id->nb_acl); > + if (nb_acl && !strcmp(nb_acl->action, "allow-established")) { > + bitmap_set1(id_data->id_bitmap, sb_id->id); > + uuidset_insert(&visited, &sb_id->nb_acl); > + add_acl_id(&id_data->ids, sb_id->id, ID_SYNCED, > &sb_id->nb_acl); > + } else { > + /* NB ACL is deleted or has changed action type. This > + * ID is no longer active. > + */ > + add_acl_id(&id_data->ids, sb_id->id, ID_INACTIVE, > &sb_id->nb_acl); > + } > + } > + > + size_t scan_start = 1; > The scan_start should be actually the highest number of all assigned up till this point, the reason being that we could in theory reuse the same ID within one transaction that would result in the CT for that ID not being flushed. > + 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 (strcmp(nb_acl->action, "allow-established")) { > + continue; > + } > + int64_t new_id = bitmap_scan(id_data->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; > + } > + add_acl_id(&id_data->ids, new_id, ID_NEW, &nb_acl->header_.uuid); > + bitmap_set1(id_data->id_bitmap, new_id); > + scan_start = new_id + 1; > + } > + > + engine_set_node_state(node, EN_UPDATED); > + uuidset_destroy(&visited); > +} > + > +void > +en_acl_id_cleanup(void *data) > +{ > + acl_id_data_destroy(data); > +} > + > +static const struct sbrec_acl_id * > +acl_id_lookup_by_id(struct ovsdb_idl_index *sbrec_acl_id_by_id, > + int64_t id) > +{ > + struct sbrec_acl_id *target = sbrec_acl_id_index_init_row( > + sbrec_acl_id_by_id); > + sbrec_acl_id_index_set_id(target, id); > + > + struct sbrec_acl_id *retval = sbrec_acl_id_index_find( > + sbrec_acl_id_by_id, target); > + > + sbrec_acl_id_index_destroy_row(target); > + > + return retval; > +} > + > +void sync_acl_ids(const struct acl_id_data *id_data, > nit: Function name on new line. This function also makes me wonder, do we actually need to have a separate sync function? We could do all this sync in one pass by just walking the SBREC_ACL_ID_TABLE_FOR_EACH_SAFE and then adding the missing northd ones. + struct ovsdb_idl_txn *ovnsb_txn, > + struct ovsdb_idl_index *sbrec_acl_id_by_id) > +{ > + struct acl_id *acl_id; > + const struct sbrec_acl_id *sb_id; > + HMAP_FOR_EACH (acl_id, node, &id_data->ids) { > + switch (acl_id->state) { > + case ID_NEW: > + sb_id = sbrec_acl_id_insert(ovnsb_txn); > The lookup and relation can be way simpler by keeping the UUID the same between NB and SB, there is always 1:1 mapping. > + sbrec_acl_id_set_id(sb_id, acl_id->id); > + sbrec_acl_id_set_nb_acl(sb_id, acl_id->nb_acl_uuid); > + break; > + case ID_INACTIVE: > + sb_id = acl_id_lookup_by_id(sbrec_acl_id_by_id, acl_id->id); > + if (sb_id) { > + sbrec_acl_id_delete(sb_id); > + } > + break; > + case ID_SYNCED: > + break; > + } > + } > +} > diff --git a/northd/en-acl-ids.h b/northd/en-acl-ids.h > new file mode 100644 > index 000000000..8b60b3c7c > --- /dev/null > +++ b/northd/en-acl-ids.h > @@ -0,0 +1,17 @@ > +#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); > + > +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); > +#endif > diff --git a/northd/en-northd.c b/northd/en-northd.c > index c7d1ebcb3..5545fe7a6 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -61,6 +61,10 @@ northd_get_input_data(struct engine_node *node, > engine_ovsdb_node_get_index( > engine_get_input("SB_fdb", node), > "sbrec_fdb_by_dp_and_port"); > + input_data->sbrec_acl_id_by_id = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_acl_id", node), > + "sbrec_acl_id_by_id"); > > input_data->nbrec_logical_switch_table = > EN_OVSDB_GET(engine_get_input("NB_logical_switch", node)); > @@ -110,6 +114,8 @@ northd_get_input_data(struct engine_node *node, > input_data->svc_monitor_mac = global_config->svc_monitor_mac; > input_data->svc_monitor_mac_ea = global_config->svc_monitor_mac_ea; > input_data->features = &global_config->features; > + > + input_data->acl_id_data = engine_get_input_data("acl_id", node); > } > > void > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 6e0aa04c4..43abf042d 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); > @@ -201,8 +207,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); > engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL); > engine_add_input(&en_northd, &en_sb_fdb, > northd_sb_fdb_change_handler); > + engine_add_input(&en_northd, &en_sb_acl_id, NULL); > engine_add_input(&en_northd, &en_global_config, > northd_global_config_handler); > + engine_add_input(&en_northd, &en_acl_id, NULL); > > /* northd engine node uses the sb mac binding table to > * cleanup mac binding entries for deleted logical ports > @@ -364,6 +372,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > = mac_binding_by_datapath_index_create(sb->idl); > struct ovsdb_idl_index *fdb_by_dp_key = > ovsdb_idl_index_create1(sb->idl, &sbrec_fdb_col_dp_key); > + struct ovsdb_idl_index *sbrec_acl_id_by_id = > + ovsdb_idl_index_create1(sb->idl, &sbrec_acl_id_col_id); > > engine_init(&en_northd_output, &engine_arg); > > @@ -388,6 +398,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_ovsdb_node_add_index(&en_sb_fdb, > "fdb_by_dp_key", > fdb_by_dp_key); > + engine_ovsdb_node_add_index(&en_sb_acl_id, > + "sbrec_acl_id_by_id", > + sbrec_acl_id_by_id); > > struct ovsdb_idl_index *sbrec_address_set_by_name > = ovsdb_idl_index_create1(sb->idl, &sbrec_address_set_col_name); > diff --git a/northd/northd.c b/northd/northd.c > index 0aae627a2..8dab88f62 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -50,6 +50,7 @@ > #include "en-lr-stateful.h" > #include "en-ls-stateful.h" > #include "en-sampling-app.h" > +#include "en-acl-ids.h" > #include "lib/ovn-parallel-hmap.h" > #include "ovn/actions.h" > #include "ovn/features.h" > @@ -19120,6 +19121,8 @@ ovnnb_db_run(struct northd_input *input_data, > &data->ls_datapaths.datapaths); > sync_template_vars(ovnsb_txn, > input_data->nbrec_chassis_template_var_table, > input_data->sbrec_chassis_template_var_table); > + sync_acl_ids(input_data->acl_id_data, ovnsb_txn, > + input_data->sbrec_acl_id_by_id); > > cleanup_stale_fdb_entries(input_data->sbrec_fdb_table, > &data->ls_datapaths.datapaths); > diff --git a/northd/northd.h b/northd/northd.h > index d60c944df..bccb1c5d8 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -63,12 +63,16 @@ 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; > struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name; > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; > struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port; > + struct ovsdb_idl_index *sbrec_acl_id_by_id; > }; > > /* A collection of datapaths. E.g. all logical switch datapaths, or all > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index fb29c3c21..bbb321248 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -896,6 +896,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]); > + } > Maybe a note that we are just writing those fields so we don't need any DB monitoring. > 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 73abf2c8d..837aee620 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > - "version": "20.37.0", > - "cksum": "1950136776 31493", > + "version": "20.38.0", > + "cksum": "4077385391 31851", > "tables": { > "SB_Global": { > "columns": { > @@ -617,6 +617,14 @@ > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > "indexes": [["chassis"]], > - "isRoot": true} > + "isRoot": true}, > + "ACL_ID": { > + "columns": { > + "id": {"type": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger": 32767}}}, > + "nb_acl" : {"type": {"key": {"type": "uuid"}}}}, > + "indexes": [["id"]], > + "isRoot": true} > } > } > diff --git a/ovn-sb.xml b/ovn-sb.xml > index ea4adc1c3..a3d71e100 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -5217,4 +5217,17 @@ tcp.flags = RST; > The set of variable values for a given chassis. > </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>. > + </p> > + <column name="id"> > + The actual identifier being synchronized. > + </column> > + <column name="nb_acl"> > + The Northbound ACL UUID for which this ID corresponds. > + </column> > + </table> > </database> > diff --git a/tests/ovn.at b/tests/ovn.at > index 2fdf1a88c..53e6712cc 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -39761,3 +39761,42 @@ OVN_CLEANUP([hv1]) > > 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-established". > +dnl > +dnl If an ACL is of a different type, or if an ACL is deleted, > +dnl 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 allow-established, so SBDB should have no rows. > +wait_row_count ACL_ID 0 > + > +dnl When we change to allow-established, the SBDB should pick it up. > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-established > + > +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-established and make sure it shows up in the SBDB. > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-established > + > +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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
On 12/10/24 06:18, Ales Musil wrote: > > > On Wed, Dec 4, 2024 at 10:53 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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 ACL that is of type "allow-established". > > Signed-off-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > --- > > > Hi Mark, > > thank you for the patch, I have a few comments down below. > > northd/automake.mk <http://automake.mk> | 2 + > northd/en-acl-ids.c | 206 +++++++++++++++++++++++++++++++++++++++ > northd/en-acl-ids.h | 17 ++++ > northd/en-northd.c | 6 ++ > northd/inc-proc-northd.c | 15 ++- > northd/northd.c | 3 + > northd/northd.h | 4 + > northd/ovn-northd.c | 4 + > ovn-sb.ovsschema | 14 ++- > ovn-sb.xml | 13 +++ > tests/ovn.at <http://ovn.at> | 39 ++++++++ > 11 files changed, 319 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 <http://automake.mk> > b/northd/automake.mk <http://automake.mk> > index 6566ad299..d46dfd763 100644 > --- a/northd/automake.mk <http://automake.mk> > +++ b/northd/automake.mk <http://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..897cc2da1 > --- /dev/null > +++ b/northd/en-acl-ids.c > @@ -0,0 +1,206 @@ > +/* > + * 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 "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); > + > +enum id_state { > + /* The ID represents a new northbound ACL that has > + * not yet been synced to the southbound DB > + */ > + ID_NEW, > + /* The ID represents an ACL ID that has been synced > + * with the southbound DB already > + */ > + ID_SYNCED, > + /* The ID represents a deleted NB ACL that also needs > + * to be removed from the southbound DB > + */ > + ID_INACTIVE, > +}; > + > +struct acl_id { > + struct hmap_node node; > + int64_t id; > + struct uuid nb_acl_uuid; > + enum id_state state; > +}; > + > +#define MAX_ACL_ID 65535 > + > +struct acl_id_data { > + struct hmap ids; > + unsigned long *id_bitmap; > +}; > + > +static void > +acl_id_data_init(struct acl_id_data *id_data) > +{ > + hmap_init(&id_data->ids); > + id_data->id_bitmap = bitmap_allocate(MAX_ACL_ID); > +} > + > +static struct acl_id_data * > +acl_id_data_alloc(void) > +{ > + struct acl_id_data *id_data = xzalloc(sizeof *id_data); > + acl_id_data_init(id_data); > + > + return id_data; > +} > + > +static void > +acl_id_data_destroy(struct acl_id_data *id_data) > +{ > + struct acl_id *acl_id; > + HMAP_FOR_EACH_POP (acl_id, node, &id_data->ids) { > + free(acl_id); > + } > + hmap_destroy(&id_data->ids); > + bitmap_free(id_data->id_bitmap); > +} > + > +void * > +en_acl_id_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct acl_id_data *id_data = acl_id_data_alloc(); > + return id_data; > +} > + > +static void > +add_acl_id(struct hmap *id_map, int64_t id, enum id_state state, > + const struct uuid *acl_uuid) > +{ > + struct acl_id *acl_id = xzalloc(sizeof *acl_id); > > > nit: We don't need to zero-alloc when it's populated right away. > > + acl_id->id = id; > + acl_id->state = state; > + acl_id->nb_acl_uuid = *acl_uuid; > + hmap_insert(id_map, &acl_id->node, uuid_hash(acl_uuid)); > +} > + > +void > +en_acl_id_run(struct engine_node *node, void *data) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + if (!eng_ctx->ovnnb_idl_txn || !eng_ctx->ovnsb_idl_txn) { > + return; > + } > + > + 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); > + struct acl_id_data *id_data = data; > + > + acl_id_data_destroy(id_data); > + acl_id_data_init(id_data); > + > + const struct nbrec_acl *nb_acl; > + const struct sbrec_acl_id *sb_id; > + SBREC_ACL_ID_TABLE_FOR_EACH (sb_id, sb_acl_id_table) { > + nb_acl = nbrec_acl_table_get_for_uuid(nb_acl_table, > &sb_id->nb_acl); > + if (nb_acl && !strcmp(nb_acl->action, "allow-established")) { > + bitmap_set1(id_data->id_bitmap, sb_id->id); > + uuidset_insert(&visited, &sb_id->nb_acl); > + add_acl_id(&id_data->ids, sb_id->id, ID_SYNCED, > &sb_id->nb_acl); > + } else { > + /* NB ACL is deleted or has changed action type. This > + * ID is no longer active. > + */ > + add_acl_id(&id_data->ids, sb_id->id, ID_INACTIVE, > &sb_id->nb_acl); > + } > + } > + > + size_t scan_start = 1; > > > The scan_start should be actually the highest number of all assigned up > till this point, the reason being that we could in theory reuse the same > ID within one transaction that would result in the CT for that ID not > being flushed. I understand the situation you're describing. The problem is that if I use the highest assigned ID as the start of the scan, then northd will eventually run out of IDs to assign. Instead of using the highest ID as the starting point, I think I'm going to try to devise a way to ensure that IDs can be safely reused. If I can't think of a way to do it, I'll fall back to using the highest ID as the scan start. > > + 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 (strcmp(nb_acl->action, "allow-established")) { > + continue; > + } > + int64_t new_id = bitmap_scan(id_data->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; > + } > + add_acl_id(&id_data->ids, new_id, ID_NEW, > &nb_acl->header_.uuid); > + bitmap_set1(id_data->id_bitmap, new_id); > + scan_start = new_id + 1; > + } > + > + engine_set_node_state(node, EN_UPDATED); > + uuidset_destroy(&visited); > +} > + > +void > +en_acl_id_cleanup(void *data) > +{ > + acl_id_data_destroy(data); > +} > + > +static const struct sbrec_acl_id * > +acl_id_lookup_by_id(struct ovsdb_idl_index *sbrec_acl_id_by_id, > + int64_t id) > +{ > + struct sbrec_acl_id *target = sbrec_acl_id_index_init_row( > + sbrec_acl_id_by_id); > + sbrec_acl_id_index_set_id(target, id); > + > + struct sbrec_acl_id *retval = sbrec_acl_id_index_find( > + sbrec_acl_id_by_id, target); > + > + sbrec_acl_id_index_destroy_row(target); > + > + return retval; > +} > + > +void sync_acl_ids(const struct acl_id_data *id_data, > > > nit: Function name on new line. > This function also makes me wonder, do we actually need to have a > separate sync function? We could do all this sync in one pass by just > walking the SBREC_ACL_ID_TABLE_FOR_EACH_SAFE and then adding the missing > northd ones. > > + struct ovsdb_idl_txn *ovnsb_txn, > + struct ovsdb_idl_index *sbrec_acl_id_by_id) > +{ > + struct acl_id *acl_id; > + const struct sbrec_acl_id *sb_id; > + HMAP_FOR_EACH (acl_id, node, &id_data->ids) { > + switch (acl_id->state) { > + case ID_NEW: > + sb_id = sbrec_acl_id_insert(ovnsb_txn); > > > The lookup and relation can be way simpler by keeping the UUID the same > between NB and SB, there is always 1:1 mapping. > > + sbrec_acl_id_set_id(sb_id, acl_id->id); > + sbrec_acl_id_set_nb_acl(sb_id, acl_id->nb_acl_uuid); > + break; > + case ID_INACTIVE: > + sb_id = acl_id_lookup_by_id(sbrec_acl_id_by_id, > acl_id->id); > + if (sb_id) { > + sbrec_acl_id_delete(sb_id); > + } > + break; > + case ID_SYNCED: > + break; > + } > + } > +} > diff --git a/northd/en-acl-ids.h b/northd/en-acl-ids.h > new file mode 100644 > index 000000000..8b60b3c7c > --- /dev/null > +++ b/northd/en-acl-ids.h > @@ -0,0 +1,17 @@ > +#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); > + > +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); > +#endif > diff --git a/northd/en-northd.c b/northd/en-northd.c > index c7d1ebcb3..5545fe7a6 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -61,6 +61,10 @@ northd_get_input_data(struct engine_node *node, > engine_ovsdb_node_get_index( > engine_get_input("SB_fdb", node), > "sbrec_fdb_by_dp_and_port"); > + input_data->sbrec_acl_id_by_id = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_acl_id", node), > + "sbrec_acl_id_by_id"); > > input_data->nbrec_logical_switch_table = > EN_OVSDB_GET(engine_get_input("NB_logical_switch", node)); > @@ -110,6 +114,8 @@ northd_get_input_data(struct engine_node *node, > input_data->svc_monitor_mac = global_config->svc_monitor_mac; > input_data->svc_monitor_mac_ea = > global_config->svc_monitor_mac_ea; > input_data->features = &global_config->features; > + > + input_data->acl_id_data = engine_get_input_data("acl_id", node); > } > > void > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 6e0aa04c4..43abf042d 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); > @@ -201,8 +207,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > *nb, > engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); > engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL); > engine_add_input(&en_northd, &en_sb_fdb, > northd_sb_fdb_change_handler); > + engine_add_input(&en_northd, &en_sb_acl_id, NULL); > engine_add_input(&en_northd, &en_global_config, > northd_global_config_handler); > + engine_add_input(&en_northd, &en_acl_id, NULL); > > /* northd engine node uses the sb mac binding table to > * cleanup mac binding entries for deleted logical ports > @@ -364,6 +372,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > = mac_binding_by_datapath_index_create(sb->idl); > struct ovsdb_idl_index *fdb_by_dp_key = > ovsdb_idl_index_create1(sb->idl, &sbrec_fdb_col_dp_key); > + struct ovsdb_idl_index *sbrec_acl_id_by_id = > + ovsdb_idl_index_create1(sb->idl, &sbrec_acl_id_col_id); > > engine_init(&en_northd_output, &engine_arg); > > @@ -388,6 +398,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_ovsdb_node_add_index(&en_sb_fdb, > "fdb_by_dp_key", > fdb_by_dp_key); > + engine_ovsdb_node_add_index(&en_sb_acl_id, > + "sbrec_acl_id_by_id", > + sbrec_acl_id_by_id); > > struct ovsdb_idl_index *sbrec_address_set_by_name > = ovsdb_idl_index_create1(sb->idl, > &sbrec_address_set_col_name); > diff --git a/northd/northd.c b/northd/northd.c > index 0aae627a2..8dab88f62 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -50,6 +50,7 @@ > #include "en-lr-stateful.h" > #include "en-ls-stateful.h" > #include "en-sampling-app.h" > +#include "en-acl-ids.h" > #include "lib/ovn-parallel-hmap.h" > #include "ovn/actions.h" > #include "ovn/features.h" > @@ -19120,6 +19121,8 @@ ovnnb_db_run(struct northd_input *input_data, > &data->ls_datapaths.datapaths); > sync_template_vars(ovnsb_txn, > input_data->nbrec_chassis_template_var_table, > input_data->sbrec_chassis_template_var_table); > + sync_acl_ids(input_data->acl_id_data, ovnsb_txn, > + input_data->sbrec_acl_id_by_id); > > cleanup_stale_fdb_entries(input_data->sbrec_fdb_table, > &data->ls_datapaths.datapaths); > diff --git a/northd/northd.h b/northd/northd.h > index d60c944df..bccb1c5d8 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -63,12 +63,16 @@ 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; > struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name; > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; > struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port; > + struct ovsdb_idl_index *sbrec_acl_id_by_id; > }; > > /* A collection of datapaths. E.g. all logical switch datapaths, > or all > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index fb29c3c21..bbb321248 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -896,6 +896,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]); > + } > > > Maybe a note that we are just writing those fields so we don't need any > DB monitoring. > > > 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 73abf2c8d..837aee620 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > - "version": "20.37.0", > - "cksum": "1950136776 31493", > + "version": "20.38.0", > + "cksum": "4077385391 31851", > "tables": { > "SB_Global": { > "columns": { > @@ -617,6 +617,14 @@ > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > "indexes": [["chassis"]], > - "isRoot": true} > + "isRoot": true}, > + "ACL_ID": { > + "columns": { > + "id": {"type": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger": 32767}}}, > + "nb_acl" : {"type": {"key": {"type": "uuid"}}}}, > + "indexes": [["id"]], > + "isRoot": true} > } > } > diff --git a/ovn-sb.xml b/ovn-sb.xml > index ea4adc1c3..a3d71e100 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -5217,4 +5217,17 @@ tcp.flags = RST; > The set of variable values for a given chassis. > </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>. > + </p> > + <column name="id"> > + The actual identifier being synchronized. > + </column> > + <column name="nb_acl"> > + The Northbound ACL UUID for which this ID corresponds. > + </column> > + </table> > </database> > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> > index 2fdf1a88c..53e6712cc 100644 > --- a/tests/ovn.at <http://ovn.at> > +++ b/tests/ovn.at <http://ovn.at> > @@ -39761,3 +39761,42 @@ OVN_CLEANUP([hv1]) > > 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-established". > +dnl > +dnl If an ACL is of a different type, or if an ACL is deleted, > +dnl 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 allow-established, so SBDB should have no rows. > +wait_row_count ACL_ID 0 > + > +dnl When we change to allow-established, the SBDB should pick it up. > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-established > + > +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-established and make sure it shows up in the > SBDB. > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-established > + > +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 > 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/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..897cc2da1 --- /dev/null +++ b/northd/en-acl-ids.c @@ -0,0 +1,206 @@ +/* + * 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); + +enum id_state { + /* The ID represents a new northbound ACL that has + * not yet been synced to the southbound DB + */ + ID_NEW, + /* The ID represents an ACL ID that has been synced + * with the southbound DB already + */ + ID_SYNCED, + /* The ID represents a deleted NB ACL that also needs + * to be removed from the southbound DB + */ + ID_INACTIVE, +}; + +struct acl_id { + struct hmap_node node; + int64_t id; + struct uuid nb_acl_uuid; + enum id_state state; +}; + +#define MAX_ACL_ID 65535 + +struct acl_id_data { + struct hmap ids; + unsigned long *id_bitmap; +}; + +static void +acl_id_data_init(struct acl_id_data *id_data) +{ + hmap_init(&id_data->ids); + id_data->id_bitmap = bitmap_allocate(MAX_ACL_ID); +} + +static struct acl_id_data * +acl_id_data_alloc(void) +{ + struct acl_id_data *id_data = xzalloc(sizeof *id_data); + acl_id_data_init(id_data); + + return id_data; +} + +static void +acl_id_data_destroy(struct acl_id_data *id_data) +{ + struct acl_id *acl_id; + HMAP_FOR_EACH_POP (acl_id, node, &id_data->ids) { + free(acl_id); + } + hmap_destroy(&id_data->ids); + bitmap_free(id_data->id_bitmap); +} + +void * +en_acl_id_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + struct acl_id_data *id_data = acl_id_data_alloc(); + return id_data; +} + +static void +add_acl_id(struct hmap *id_map, int64_t id, enum id_state state, + const struct uuid *acl_uuid) +{ + struct acl_id *acl_id = xzalloc(sizeof *acl_id); + acl_id->id = id; + acl_id->state = state; + acl_id->nb_acl_uuid = *acl_uuid; + hmap_insert(id_map, &acl_id->node, uuid_hash(acl_uuid)); +} + +void +en_acl_id_run(struct engine_node *node, void *data) +{ + const struct engine_context *eng_ctx = engine_get_context(); + if (!eng_ctx->ovnnb_idl_txn || !eng_ctx->ovnsb_idl_txn) { + return; + } + + 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); + struct acl_id_data *id_data = data; + + acl_id_data_destroy(id_data); + acl_id_data_init(id_data); + + const struct nbrec_acl *nb_acl; + const struct sbrec_acl_id *sb_id; + SBREC_ACL_ID_TABLE_FOR_EACH (sb_id, sb_acl_id_table) { + nb_acl = nbrec_acl_table_get_for_uuid(nb_acl_table, &sb_id->nb_acl); + if (nb_acl && !strcmp(nb_acl->action, "allow-established")) { + bitmap_set1(id_data->id_bitmap, sb_id->id); + uuidset_insert(&visited, &sb_id->nb_acl); + add_acl_id(&id_data->ids, sb_id->id, ID_SYNCED, &sb_id->nb_acl); + } else { + /* NB ACL is deleted or has changed action type. This + * ID is no longer active. + */ + add_acl_id(&id_data->ids, sb_id->id, ID_INACTIVE, &sb_id->nb_acl); + } + } + + 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 (strcmp(nb_acl->action, "allow-established")) { + continue; + } + int64_t new_id = bitmap_scan(id_data->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; + } + add_acl_id(&id_data->ids, new_id, ID_NEW, &nb_acl->header_.uuid); + bitmap_set1(id_data->id_bitmap, new_id); + scan_start = new_id + 1; + } + + engine_set_node_state(node, EN_UPDATED); + uuidset_destroy(&visited); +} + +void +en_acl_id_cleanup(void *data) +{ + acl_id_data_destroy(data); +} + +static const struct sbrec_acl_id * +acl_id_lookup_by_id(struct ovsdb_idl_index *sbrec_acl_id_by_id, + int64_t id) +{ + struct sbrec_acl_id *target = sbrec_acl_id_index_init_row( + sbrec_acl_id_by_id); + sbrec_acl_id_index_set_id(target, id); + + struct sbrec_acl_id *retval = sbrec_acl_id_index_find( + sbrec_acl_id_by_id, target); + + sbrec_acl_id_index_destroy_row(target); + + return retval; +} + +void sync_acl_ids(const struct acl_id_data *id_data, + struct ovsdb_idl_txn *ovnsb_txn, + struct ovsdb_idl_index *sbrec_acl_id_by_id) +{ + struct acl_id *acl_id; + const struct sbrec_acl_id *sb_id; + HMAP_FOR_EACH (acl_id, node, &id_data->ids) { + switch (acl_id->state) { + case ID_NEW: + sb_id = sbrec_acl_id_insert(ovnsb_txn); + sbrec_acl_id_set_id(sb_id, acl_id->id); + sbrec_acl_id_set_nb_acl(sb_id, acl_id->nb_acl_uuid); + break; + case ID_INACTIVE: + sb_id = acl_id_lookup_by_id(sbrec_acl_id_by_id, acl_id->id); + if (sb_id) { + sbrec_acl_id_delete(sb_id); + } + break; + case ID_SYNCED: + break; + } + } +} diff --git a/northd/en-acl-ids.h b/northd/en-acl-ids.h new file mode 100644 index 000000000..8b60b3c7c --- /dev/null +++ b/northd/en-acl-ids.h @@ -0,0 +1,17 @@ +#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); + +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); +#endif diff --git a/northd/en-northd.c b/northd/en-northd.c index c7d1ebcb3..5545fe7a6 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -61,6 +61,10 @@ northd_get_input_data(struct engine_node *node, engine_ovsdb_node_get_index( engine_get_input("SB_fdb", node), "sbrec_fdb_by_dp_and_port"); + input_data->sbrec_acl_id_by_id = + engine_ovsdb_node_get_index( + engine_get_input("SB_acl_id", node), + "sbrec_acl_id_by_id"); input_data->nbrec_logical_switch_table = EN_OVSDB_GET(engine_get_input("NB_logical_switch", node)); @@ -110,6 +114,8 @@ northd_get_input_data(struct engine_node *node, input_data->svc_monitor_mac = global_config->svc_monitor_mac; input_data->svc_monitor_mac_ea = global_config->svc_monitor_mac_ea; input_data->features = &global_config->features; + + input_data->acl_id_data = engine_get_input_data("acl_id", node); } void diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 6e0aa04c4..43abf042d 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); @@ -201,8 +207,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL); engine_add_input(&en_northd, &en_sb_fdb, northd_sb_fdb_change_handler); + engine_add_input(&en_northd, &en_sb_acl_id, NULL); engine_add_input(&en_northd, &en_global_config, northd_global_config_handler); + engine_add_input(&en_northd, &en_acl_id, NULL); /* northd engine node uses the sb mac binding table to * cleanup mac binding entries for deleted logical ports @@ -364,6 +372,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, = mac_binding_by_datapath_index_create(sb->idl); struct ovsdb_idl_index *fdb_by_dp_key = ovsdb_idl_index_create1(sb->idl, &sbrec_fdb_col_dp_key); + struct ovsdb_idl_index *sbrec_acl_id_by_id = + ovsdb_idl_index_create1(sb->idl, &sbrec_acl_id_col_id); engine_init(&en_northd_output, &engine_arg); @@ -388,6 +398,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_ovsdb_node_add_index(&en_sb_fdb, "fdb_by_dp_key", fdb_by_dp_key); + engine_ovsdb_node_add_index(&en_sb_acl_id, + "sbrec_acl_id_by_id", + sbrec_acl_id_by_id); struct ovsdb_idl_index *sbrec_address_set_by_name = ovsdb_idl_index_create1(sb->idl, &sbrec_address_set_col_name); diff --git a/northd/northd.c b/northd/northd.c index 0aae627a2..8dab88f62 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -50,6 +50,7 @@ #include "en-lr-stateful.h" #include "en-ls-stateful.h" #include "en-sampling-app.h" +#include "en-acl-ids.h" #include "lib/ovn-parallel-hmap.h" #include "ovn/actions.h" #include "ovn/features.h" @@ -19120,6 +19121,8 @@ ovnnb_db_run(struct northd_input *input_data, &data->ls_datapaths.datapaths); sync_template_vars(ovnsb_txn, input_data->nbrec_chassis_template_var_table, input_data->sbrec_chassis_template_var_table); + sync_acl_ids(input_data->acl_id_data, ovnsb_txn, + input_data->sbrec_acl_id_by_id); cleanup_stale_fdb_entries(input_data->sbrec_fdb_table, &data->ls_datapaths.datapaths); diff --git a/northd/northd.h b/northd/northd.h index d60c944df..bccb1c5d8 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -63,12 +63,16 @@ 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; struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name; struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port; + struct ovsdb_idl_index *sbrec_acl_id_by_id; }; /* A collection of datapaths. E.g. all logical switch datapaths, or all diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index fb29c3c21..bbb321248 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -896,6 +896,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 73abf2c8d..837aee620 100644 --- a/ovn-sb.ovsschema +++ b/ovn-sb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Southbound", - "version": "20.37.0", - "cksum": "1950136776 31493", + "version": "20.38.0", + "cksum": "4077385391 31851", "tables": { "SB_Global": { "columns": { @@ -617,6 +617,14 @@ "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, "indexes": [["chassis"]], - "isRoot": true} + "isRoot": true}, + "ACL_ID": { + "columns": { + "id": {"type": {"key": {"type": "integer", + "minInteger": 0, + "maxInteger": 32767}}}, + "nb_acl" : {"type": {"key": {"type": "uuid"}}}}, + "indexes": [["id"]], + "isRoot": true} } } diff --git a/ovn-sb.xml b/ovn-sb.xml index ea4adc1c3..a3d71e100 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -5217,4 +5217,17 @@ tcp.flags = RST; The set of variable values for a given chassis. </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>. + </p> + <column name="id"> + The actual identifier being synchronized. + </column> + <column name="nb_acl"> + The Northbound ACL UUID for which this ID corresponds. + </column> + </table> </database> diff --git a/tests/ovn.at b/tests/ovn.at index 2fdf1a88c..53e6712cc 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -39761,3 +39761,42 @@ OVN_CLEANUP([hv1]) 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-established". +dnl +dnl If an ACL is of a different type, or if an ACL is deleted, +dnl 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 allow-established, so SBDB should have no rows. +wait_row_count ACL_ID 0 + +dnl When we change to allow-established, the SBDB should pick it up. +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-established + +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-established and make sure it shows up in the SBDB. +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-established + +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 +])
This introduces a new southbound table called ACL_ID for storing persistent ACL conntrack IDs. These IDs are generated by ovn-northd for any ACL that is of type "allow-established". Signed-off-by: Mark Michelson <mmichels@redhat.com> --- northd/automake.mk | 2 + northd/en-acl-ids.c | 206 +++++++++++++++++++++++++++++++++++++++ northd/en-acl-ids.h | 17 ++++ northd/en-northd.c | 6 ++ northd/inc-proc-northd.c | 15 ++- northd/northd.c | 3 + northd/northd.h | 4 + northd/ovn-northd.c | 4 + ovn-sb.ovsschema | 14 ++- ovn-sb.xml | 13 +++ tests/ovn.at | 39 ++++++++ 11 files changed, 319 insertions(+), 4 deletions(-) create mode 100644 northd/en-acl-ids.c create mode 100644 northd/en-acl-ids.h