| Message ID | 20210630140054.29134-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] ovn-controller: Fix port group I-P when they contain non-vif ports. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
On Wed, Jun 30, 2021 at 7:01 AM Dumitru Ceara <dceara@redhat.com> wrote: > > It's valid that port_groups contain non-vif ports, they can actually > contain any type of logical_switch_port. > > Also, there's no need to allocate a new sset containing the local ports' > names every time the I-P engine processes a change. We were already > maintaining a set of "local_lport_ids". These correspond to port > bindings that are relevant locally (including non-vif ports). Extend > it to include the locally relevant lport names too and rename the > structure an its helper functions to related_lport*. > > Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 > Reported-by: Antonio Ojea <aojea@redhat.com> > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > v2: > - Addressed Numan's and Han's comments: > - add struct related_lports > - add test case. > --- > controller/binding.c | 79 ++++++++++++++++++------------------- > controller/binding.h | 31 ++++++++------- > controller/lflow.c | 2 +- > controller/lflow.h | 2 +- > controller/ovn-controller.c | 48 +++++++++------------- > tests/ovn.at | 44 +++++++++++++++++++++ > 6 files changed, 120 insertions(+), 86 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 7fde0fdbb..594babc98 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx) > } > } > > -/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of local > - * lport IDs. Also track if the set has changed. > +/* Add a port binding to the set of locally relevant lports. > + * Also track if the set has changed. > */ > static void > -update_local_lport_ids(const struct sbrec_port_binding *pb, > - struct binding_ctx_out *b_ctx) > +update_related_lport(const struct sbrec_port_binding *pb, > + struct binding_ctx_out *b_ctx) > { > char buf[16]; > get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key, > buf, sizeof(buf)); > - if (sset_add(b_ctx->local_lport_ids, buf) != NULL) { > - b_ctx->local_lport_ids_changed = true; > + if (sset_add(&b_ctx->related_lports->lport_ids, buf) != NULL) { > + b_ctx->related_lports_changed = true; > > if (b_ctx->tracked_dp_bindings) { > /* Add the 'pb' to the tracked_datapaths. */ > tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings); > } > } > + sset_add(&b_ctx->related_lports->lport_names, pb->logical_port); > } > > -/* Remove a port binding id from the set of local lport IDs. Also track if > - * the set has changed. > +/* Remove a port binding id from the set of locally relevant lports. > + * Also track if the set has changed. > */ > static void > -remove_local_lport_ids(const struct sbrec_port_binding *pb, > - struct binding_ctx_out *b_ctx) > +remove_related_lport(const struct sbrec_port_binding *pb, > + struct binding_ctx_out *b_ctx) > { > char buf[16]; > get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key, > buf, sizeof(buf)); > - if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) { > - b_ctx->local_lport_ids_changed = true; > + sset_find_and_delete(&b_ctx->related_lports->lport_names, > + pb->logical_port); > + if (sset_find_and_delete(&b_ctx->related_lports->lport_ids, buf)) { > + b_ctx->related_lports_changed = true; > > if (b_ctx->tracked_dp_bindings) { > /* Add the 'pb' to the tracked_datapaths. */ > @@ -678,6 +681,20 @@ static struct binding_lport *binding_lport_check_and_cleanup( > > static char *get_lport_type_str(enum en_lport_type lport_type); > > +void > +related_lports_init(struct related_lports *rp) > +{ > + sset_init(&rp->lport_names); > + sset_init(&rp->lport_ids); > +} > + > +void > +related_lports_destroy(struct related_lports *rp) > +{ > + sset_destroy(&rp->lport_names); > + sset_destroy(&rp->lport_ids); > +} > + > void > local_binding_data_init(struct local_binding_data *lbinding_data) > { > @@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis *chassis_rec, > struct binding_ctx_out *b_ctx_out) > { > if (is_binding_lport_this_chassis(b_lport, chassis_rec)) { > - remove_local_lport_ids(b_lport->pb, b_ctx_out); > + remove_related_lport(b_lport->pb, b_ctx_out); > if (!release_lport(b_lport->pb, sb_readonly, > b_ctx_out->tracked_dp_bindings, > b_ctx_out->if_mgr)) { > @@ -1214,7 +1231,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > pb->datapath, false, > b_ctx_out->local_datapaths, > b_ctx_out->tracked_dp_bindings); > - update_local_lport_ids(pb, b_ctx_out); > + update_related_lport(pb, b_ctx_out); > update_local_lports(pb->logical_port, b_ctx_out); > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > get_qos_params(pb, qos_map); > @@ -1405,7 +1422,7 @@ consider_virtual_lport(const struct sbrec_port_binding *pb, > * its entry from the local_lport_ids if present. This is required > * when a virtual port moves from one chassis to other.*/ > if (!virtual_b_lport) { > - remove_local_lport_ids(pb, b_ctx_out); > + remove_related_lport(pb, b_ctx_out); > } > > return true; > @@ -1430,7 +1447,7 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, > b_ctx_out->local_datapaths, > b_ctx_out->tracked_dp_bindings); > > - update_local_lport_ids(pb, b_ctx_out); > + update_related_lport(pb, b_ctx_out); > return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, > !b_ctx_in->ovnsb_idl_txn, false, > b_ctx_out->tracked_dp_bindings, > @@ -1482,7 +1499,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > get_qos_params(pb, qos_map); > } > > - update_local_lport_ids(pb, b_ctx_out); > + update_related_lport(pb, b_ctx_out); > } > > static bool > @@ -1512,7 +1529,7 @@ consider_ha_lport(const struct sbrec_port_binding *pb, > pb->datapath, false, > b_ctx_out->local_datapaths, > b_ctx_out->tracked_dp_bindings); > - update_local_lport_ids(pb, b_ctx_out); > + update_related_lport(pb, b_ctx_out); > } > > return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out); > @@ -1634,7 +1651,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > case LP_PATCH: > case LP_LOCALPORT: > case LP_VTEP: > - update_local_lport_ids(pb, b_ctx_out); > + update_related_lport(pb, b_ctx_out); > break; > > case LP_VIF: > @@ -1895,7 +1912,7 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, > struct binding_ctx_out *b_ctx_out, > struct local_datapath *ld) > { > - remove_local_lport_ids(pb, b_ctx_out); > + remove_related_lport(pb, b_ctx_out); > if (!strcmp(pb->type, "patch") || > !strcmp(pb->type, "l3gateway")) { > remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths); > @@ -2502,7 +2519,7 @@ delete_done: > case LP_PATCH: > case LP_LOCALPORT: > case LP_VTEP: > - update_local_lport_ids(pb, b_ctx_out); > + update_related_lport(pb, b_ctx_out); > if (lport_type == LP_PATCH) { > if (!ld) { > /* If 'ld' for this lport is not present, then check if > @@ -2926,23 +2943,3 @@ cleanup: > > return b_lport; > } > - > -struct sset * > -binding_collect_local_binding_lports(struct local_binding_data *lbinding_data) > -{ > - struct sset *lports = xzalloc(sizeof *lports); > - sset_init(lports); > - struct shash_node *shash_node; > - SHASH_FOR_EACH (shash_node, &lbinding_data->lports) { > - struct binding_lport *b_lport = shash_node->data; > - sset_add(lports, b_lport->name); > - } > - return lports; > -} > - > -void > -binding_destroy_local_binding_lports(struct sset *lports) > -{ > - sset_destroy(lports); > - free(lports); > -} > diff --git a/controller/binding.h b/controller/binding.h > index 8f3289476..a08011ae2 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -22,6 +22,7 @@ > #include "openvswitch/hmap.h" > #include "openvswitch/uuid.h" > #include "openvswitch/list.h" > +#include "sset.h" > > struct hmap; > struct ovsdb_idl; > @@ -56,6 +57,19 @@ struct binding_ctx_in { > const struct ovsrec_interface_table *iface_table; > }; > > +/* Locally relevant port bindings, e.g., VIFs that might be bound locally, > + * patch ports. > + */ > +struct related_lports { > + struct sset lport_names; /* Set of port names. */ > + struct sset lport_ids; /* Set of <datapath-tunnel-key>_<port-tunnel-key> > + * IDs for fast lookup. > + */ > +}; > + > +void related_lports_init(struct related_lports *); > +void related_lports_destroy(struct related_lports *); > + > struct binding_ctx_out { > struct hmap *local_datapaths; > struct local_binding_data *lbinding_data; > @@ -65,11 +79,9 @@ struct binding_ctx_out { > /* Track if local_lports have been updated. */ > bool local_lports_changed; > > - /* sset of local lport ids in the format > - * <datapath-tunnel-key>_<port-tunnel-key>. */ > - struct sset *local_lport_ids; > - /* Track if local_lport_ids has been updated. */ > - bool local_lport_ids_changed; > + /* Port bindings that are relevant to the local chassis. */ > + struct related_lports *related_lports; > + bool related_lports_changed; > > /* Track if non-vif port bindings (e.g., patch, external) have been > * added/deleted. > @@ -133,13 +145,4 @@ bool binding_handle_port_binding_changes(struct binding_ctx_in *, > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > void binding_dump_local_bindings(struct local_binding_data *, struct ds *); > - > -/* Generates a sset of lport names from local_binding_data. > - * Note: the caller is responsible for destroying and freeing the returned > - * sset, by calling binding_detroy_local_binding_lports(). */ > -struct sset *binding_collect_local_binding_lports(struct local_binding_data *); > - > -/* Destroy and free the lports sset returned by > - * binding_collect_local_binding_lports(). */ > -void binding_destroy_local_binding_lports(struct sset *lports); > #endif /* controller/binding.h */ > diff --git a/controller/lflow.c b/controller/lflow.c > index 34eca135a..abb01f0ce 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -625,7 +625,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); > lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, > &lflow->header_.uuid); > - if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { > + if (!sset_contains(l_ctx_in->related_lport_ids, buf)) { > VLOG_DBG("lflow "UUID_FMT > " port %s in match is not local, skip", > UUID_ARGS(&lflow->header_.uuid), > diff --git a/controller/lflow.h b/controller/lflow.h > index e98edf81d..699f9c2d5 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -144,7 +144,7 @@ struct lflow_ctx_in { > const struct shash *addr_sets; > const struct shash *port_groups; > const struct sset *active_tunnels; > - const struct sset *local_lport_ids; > + const struct sset *related_lport_ids; > }; > > struct lflow_ctx_out { > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index b6afb8fb9..3bb8b22eb 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1014,9 +1014,10 @@ struct ed_type_runtime_data { > * local hypervisor, and localnet ports. */ > struct sset local_lports; > > - /* Contains the same ports as local_lports, but in the format: > - * <datapath-tunnel-key>_<port-tunnel-key> */ > - struct sset local_lport_ids; > + /* Port bindings that are relevant to the local chassis (VIFs bound > + * localy, patch ports). > + */ > + struct related_lports related_lports; > struct sset active_tunnels; > > /* runtime data engine private data. */ > @@ -1109,7 +1110,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > hmap_init(&data->local_datapaths); > sset_init(&data->local_lports); > - sset_init(&data->local_lport_ids); > + related_lports_init(&data->related_lports); > sset_init(&data->active_tunnels); > sset_init(&data->egress_ifaces); > smap_init(&data->local_iface_ids); > @@ -1127,7 +1128,7 @@ en_runtime_data_cleanup(void *data) > struct ed_type_runtime_data *rt_data = data; > > sset_destroy(&rt_data->local_lports); > - sset_destroy(&rt_data->local_lport_ids); > + related_lports_destroy(&rt_data->related_lports); > sset_destroy(&rt_data->active_tunnels); > sset_destroy(&rt_data->egress_ifaces); > smap_destroy(&rt_data->local_iface_ids); > @@ -1219,8 +1220,8 @@ init_binding_ctx(struct engine_node *node, > b_ctx_out->local_datapaths = &rt_data->local_datapaths; > b_ctx_out->local_lports = &rt_data->local_lports; > b_ctx_out->local_lports_changed = false; > - b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; > - b_ctx_out->local_lport_ids_changed = false; > + b_ctx_out->related_lports = &rt_data->related_lports; > + b_ctx_out->related_lports_changed = false; > b_ctx_out->non_vif_ports_changed = false; > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > @@ -1235,7 +1236,6 @@ en_runtime_data_run(struct engine_node *node, void *data) > struct ed_type_runtime_data *rt_data = data; > struct hmap *local_datapaths = &rt_data->local_datapaths; > struct sset *local_lports = &rt_data->local_lports; > - struct sset *local_lport_ids = &rt_data->local_lport_ids; > struct sset *active_tunnels = &rt_data->active_tunnels; > > static bool first_run = true; > @@ -1252,12 +1252,12 @@ en_runtime_data_run(struct engine_node *node, void *data) > hmap_clear(local_datapaths); > local_binding_data_destroy(&rt_data->lbinding_data); > sset_destroy(local_lports); > - sset_destroy(local_lport_ids); > + related_lports_destroy(&rt_data->related_lports); > sset_destroy(active_tunnels); > sset_destroy(&rt_data->egress_ifaces); > smap_destroy(&rt_data->local_iface_ids); > sset_init(local_lports); > - sset_init(local_lport_ids); > + related_lports_init(&rt_data->related_lports); > sset_init(active_tunnels); > sset_init(&rt_data->egress_ifaces); > smap_init(&rt_data->local_iface_ids); > @@ -1327,7 +1327,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > } > > rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > - if (b_ctx_out.local_lport_ids_changed || > + if (b_ctx_out.related_lports_changed || > b_ctx_out.non_vif_ports_changed || > b_ctx_out.local_lports_changed || > !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > @@ -1638,11 +1638,8 @@ en_port_groups_run(struct engine_node *node, void *data) > struct ed_type_runtime_data *rt_data = > engine_get_input_data("runtime_data", node); > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > - &rt_data->lbinding_data); > - port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets, > - &pg->port_groups_cs_local); > - binding_destroy_local_binding_lports(local_b_lports); > + port_groups_init(pg_table, &rt_data->related_lports.lport_names, > + &pg->port_group_ssets, &pg->port_groups_cs_local); > > engine_set_node_state(node, EN_UPDATED); > } > @@ -1659,12 +1656,9 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > struct ed_type_runtime_data *rt_data = > engine_get_input_data("runtime_data", node); > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > - &rt_data->lbinding_data); > - port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets, > - &pg->port_groups_cs_local, &pg->new, &pg->deleted, > - &pg->updated); > - binding_destroy_local_binding_lports(local_b_lports); > + port_groups_update(pg_table, &rt_data->related_lports.lport_names, > + &pg->port_group_ssets, &pg->port_groups_cs_local, > + &pg->new, &pg->deleted, &pg->updated); > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > !sset_is_empty(&pg->updated)) { > @@ -1697,9 +1691,6 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data) > goto out; > } > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > - &rt_data->lbinding_data); > - > const struct sbrec_port_group *pg_sb; > SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) { > struct sset *pg_lports = shash_find_data(&pg->port_group_ssets, > @@ -1726,13 +1717,12 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data) > if (need_update) { > expr_const_sets_add_strings(&pg->port_groups_cs_local, pg_sb->name, > (const char *const *) pg_sb->ports, > - pg_sb->n_ports, local_b_lports); > + pg_sb->n_ports, > + &rt_data->related_lports.lport_names); > sset_add(&pg->updated, pg_sb->name); > } > } > > - binding_destroy_local_binding_lports(local_b_lports); > - > out: > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > !sset_is_empty(&pg->updated)) { > @@ -2042,7 +2032,7 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_in->addr_sets = addr_sets; > l_ctx_in->port_groups = port_groups; > l_ctx_in->active_tunnels = &rt_data->active_tunnels; > - l_ctx_in->local_lport_ids = &rt_data->local_lport_ids; > + l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids; > > l_ctx_out->flow_table = &fo->flow_table; > l_ctx_out->group_table = &fo->group_table; > diff --git a/tests/ovn.at b/tests/ovn.at > index db1a0a35c..7a718d4b6 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -26805,6 +26805,50 @@ OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > > +# Tests that ACLs referencing port groups that include ports connected to > +# logical routers are correctly applied. > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- ACL with Port Group including router ports]) > +ovn_start > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +check ovn-nbctl \ > + -- lr-add lr \ > + -- ls-add ls \ > + -- lrp-add lr lrp_ls 00:00:00:00:00:01 42.42.42.1/24 \ > + -- lsp-add ls ls_lr \ > + -- lsp-set-addresses ls_lr router \ > + -- lsp-set-type ls_lr router \ > + -- lsp-set-options ls_lr router-port=lr_ls \ > + -- lsp-add ls vm1 > + > +check ovn-nbctl pg-add pg ls_lr \ > + -- acl-add pg from-lport 1 'inport == @pg && ip4.dst == 42.42.42.42' drop > + > +check ovs-vsctl add-port br-int vm1 \ > + -- set interface vm1 external_ids:iface-id=vm1 > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=ls) > +rtr_port_key=$(fetch_column Port_Binding tunnel_key logical_port=ls_lr) > + > +# Check that ovn-controller adds a flow to drop packets with dest IP > +# 42.42.42.42 coming from the router port. > +AT_CHECK([ovs-ofctl dump-flows br-int table=17 | grep "reg14=0x${rtr_port_key},metadata=0x${dp_key},nw_dst=42.42.42.42 actions=drop" -c], [0], [dnl > +1 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn -- Static route with discard nexthop]) > ovn_start > -- > 2.27.0 > Thanks Dumitru! Acked-by: Han Zhou <hzhou@ovn.org> Not sure if Numan would like to take a second look as well, so let's wait for one or two days before merging.
On Wed, Jun 30, 2021 at 7:04 PM Han Zhou <hzhou@ovn.org> wrote: > > On Wed, Jun 30, 2021 at 7:01 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > It's valid that port_groups contain non-vif ports, they can actually > > contain any type of logical_switch_port. > > > > Also, there's no need to allocate a new sset containing the local ports' > > names every time the I-P engine processes a change. We were already > > maintaining a set of "local_lport_ids". These correspond to port > > bindings that are relevant locally (including non-vif ports). Extend > > it to include the locally relevant lport names too and rename the > > structure an its helper functions to related_lport*. > > > > Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 > > Reported-by: Antonio Ojea <aojea@redhat.com> > > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow > explosion problem.") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > v2: > > - Addressed Numan's and Han's comments: > > - add struct related_lports > > - add test case. > > --- > > controller/binding.c | 79 ++++++++++++++++++------------------- > > controller/binding.h | 31 ++++++++------- > > controller/lflow.c | 2 +- > > controller/lflow.h | 2 +- > > controller/ovn-controller.c | 48 +++++++++------------- > > tests/ovn.at | 44 +++++++++++++++++++++ > > 6 files changed, 120 insertions(+), 86 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 7fde0fdbb..594babc98 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct > binding_ctx_out *b_ctx) > > } > > } > > > > -/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of > local > > - * lport IDs. Also track if the set has changed. > > +/* Add a port binding to the set of locally relevant lports. > > + * Also track if the set has changed. > > */ > > static void > > -update_local_lport_ids(const struct sbrec_port_binding *pb, > > - struct binding_ctx_out *b_ctx) > > +update_related_lport(const struct sbrec_port_binding *pb, > > + struct binding_ctx_out *b_ctx) > > { > > char buf[16]; > > get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key, > > buf, sizeof(buf)); > > - if (sset_add(b_ctx->local_lport_ids, buf) != NULL) { > > - b_ctx->local_lport_ids_changed = true; > > + if (sset_add(&b_ctx->related_lports->lport_ids, buf) != NULL) { > > + b_ctx->related_lports_changed = true; > > > > if (b_ctx->tracked_dp_bindings) { > > /* Add the 'pb' to the tracked_datapaths. */ > > tracked_binding_datapath_lport_add(pb, > b_ctx->tracked_dp_bindings); > > } > > } > > + sset_add(&b_ctx->related_lports->lport_names, pb->logical_port); > > } > > > > -/* Remove a port binding id from the set of local lport IDs. Also track > if > > - * the set has changed. > > +/* Remove a port binding id from the set of locally relevant lports. > > + * Also track if the set has changed. > > */ > > static void > > -remove_local_lport_ids(const struct sbrec_port_binding *pb, > > - struct binding_ctx_out *b_ctx) > > +remove_related_lport(const struct sbrec_port_binding *pb, > > + struct binding_ctx_out *b_ctx) > > { > > char buf[16]; > > get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key, > > buf, sizeof(buf)); > > - if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) { > > - b_ctx->local_lport_ids_changed = true; > > + sset_find_and_delete(&b_ctx->related_lports->lport_names, > > + pb->logical_port); > > + if (sset_find_and_delete(&b_ctx->related_lports->lport_ids, buf)) { > > + b_ctx->related_lports_changed = true; > > > > if (b_ctx->tracked_dp_bindings) { > > /* Add the 'pb' to the tracked_datapaths. */ > > @@ -678,6 +681,20 @@ static struct binding_lport > *binding_lport_check_and_cleanup( > > > > static char *get_lport_type_str(enum en_lport_type lport_type); > > > > +void > > +related_lports_init(struct related_lports *rp) > > +{ > > + sset_init(&rp->lport_names); > > + sset_init(&rp->lport_ids); > > +} > > + > > +void > > +related_lports_destroy(struct related_lports *rp) > > +{ > > + sset_destroy(&rp->lport_names); > > + sset_destroy(&rp->lport_ids); > > +} > > + > > void > > local_binding_data_init(struct local_binding_data *lbinding_data) > > { > > @@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis > *chassis_rec, > > struct binding_ctx_out *b_ctx_out) > > { > > if (is_binding_lport_this_chassis(b_lport, chassis_rec)) { > > - remove_local_lport_ids(b_lport->pb, b_ctx_out); > > + remove_related_lport(b_lport->pb, b_ctx_out); > > if (!release_lport(b_lport->pb, sb_readonly, > > b_ctx_out->tracked_dp_bindings, > > b_ctx_out->if_mgr)) { > > @@ -1214,7 +1231,7 @@ consider_vif_lport_(const struct sbrec_port_binding > *pb, > > pb->datapath, false, > > b_ctx_out->local_datapaths, > > b_ctx_out->tracked_dp_bindings); > > - update_local_lport_ids(pb, b_ctx_out); > > + update_related_lport(pb, b_ctx_out); > > update_local_lports(pb->logical_port, b_ctx_out); > > if (b_lport->lbinding->iface && qos_map && > b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > @@ -1405,7 +1422,7 @@ consider_virtual_lport(const struct > sbrec_port_binding *pb, > > * its entry from the local_lport_ids if present. This is required > > * when a virtual port moves from one chassis to other.*/ > > if (!virtual_b_lport) { > > - remove_local_lport_ids(pb, b_ctx_out); > > + remove_related_lport(pb, b_ctx_out); > > } > > > > return true; > > @@ -1430,7 +1447,7 @@ consider_nonvif_lport_(const struct > sbrec_port_binding *pb, > > b_ctx_out->local_datapaths, > > b_ctx_out->tracked_dp_bindings); > > > > - update_local_lport_ids(pb, b_ctx_out); > > + update_related_lport(pb, b_ctx_out); > > return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, > > !b_ctx_in->ovnsb_idl_txn, false, > > b_ctx_out->tracked_dp_bindings, > > @@ -1482,7 +1499,7 @@ consider_localnet_lport(const struct > sbrec_port_binding *pb, > > get_qos_params(pb, qos_map); > > } > > > > - update_local_lport_ids(pb, b_ctx_out); > > + update_related_lport(pb, b_ctx_out); > > } > > > > static bool > > @@ -1512,7 +1529,7 @@ consider_ha_lport(const struct sbrec_port_binding > *pb, > > pb->datapath, false, > > b_ctx_out->local_datapaths, > > b_ctx_out->tracked_dp_bindings); > > - update_local_lport_ids(pb, b_ctx_out); > > + update_related_lport(pb, b_ctx_out); > > } > > > > return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, > b_ctx_out); > > @@ -1634,7 +1651,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > binding_ctx_out *b_ctx_out) > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - update_local_lport_ids(pb, b_ctx_out); > > + update_related_lport(pb, b_ctx_out); > > break; > > > > case LP_VIF: > > @@ -1895,7 +1912,7 @@ remove_pb_from_local_datapath(const struct > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct local_datapath *ld) > > { > > - remove_local_lport_ids(pb, b_ctx_out); > > + remove_related_lport(pb, b_ctx_out); > > if (!strcmp(pb->type, "patch") || > > !strcmp(pb->type, "l3gateway")) { > > remove_local_datapath_peer_port(pb, ld, > b_ctx_out->local_datapaths); > > @@ -2502,7 +2519,7 @@ delete_done: > > case LP_PATCH: > > case LP_LOCALPORT: > > case LP_VTEP: > > - update_local_lport_ids(pb, b_ctx_out); > > + update_related_lport(pb, b_ctx_out); > > if (lport_type == LP_PATCH) { > > if (!ld) { > > /* If 'ld' for this lport is not present, then check > if > > @@ -2926,23 +2943,3 @@ cleanup: > > > > return b_lport; > > } > > - > > -struct sset * > > -binding_collect_local_binding_lports(struct local_binding_data > *lbinding_data) > > -{ > > - struct sset *lports = xzalloc(sizeof *lports); > > - sset_init(lports); > > - struct shash_node *shash_node; > > - SHASH_FOR_EACH (shash_node, &lbinding_data->lports) { > > - struct binding_lport *b_lport = shash_node->data; > > - sset_add(lports, b_lport->name); > > - } > > - return lports; > > -} > > - > > -void > > -binding_destroy_local_binding_lports(struct sset *lports) > > -{ > > - sset_destroy(lports); > > - free(lports); > > -} > > diff --git a/controller/binding.h b/controller/binding.h > > index 8f3289476..a08011ae2 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -22,6 +22,7 @@ > > #include "openvswitch/hmap.h" > > #include "openvswitch/uuid.h" > > #include "openvswitch/list.h" > > +#include "sset.h" > > > > struct hmap; > > struct ovsdb_idl; > > @@ -56,6 +57,19 @@ struct binding_ctx_in { > > const struct ovsrec_interface_table *iface_table; > > }; > > > > +/* Locally relevant port bindings, e.g., VIFs that might be bound > locally, > > + * patch ports. > > + */ > > +struct related_lports { > > + struct sset lport_names; /* Set of port names. */ > > + struct sset lport_ids; /* Set of > <datapath-tunnel-key>_<port-tunnel-key> > > + * IDs for fast lookup. > > + */ > > +}; > > + > > +void related_lports_init(struct related_lports *); > > +void related_lports_destroy(struct related_lports *); > > + > > struct binding_ctx_out { > > struct hmap *local_datapaths; > > struct local_binding_data *lbinding_data; > > @@ -65,11 +79,9 @@ struct binding_ctx_out { > > /* Track if local_lports have been updated. */ > > bool local_lports_changed; > > > > - /* sset of local lport ids in the format > > - * <datapath-tunnel-key>_<port-tunnel-key>. */ > > - struct sset *local_lport_ids; > > - /* Track if local_lport_ids has been updated. */ > > - bool local_lport_ids_changed; > > + /* Port bindings that are relevant to the local chassis. */ > > + struct related_lports *related_lports; > > + bool related_lports_changed; > > > > /* Track if non-vif port bindings (e.g., patch, external) have been > > * added/deleted. > > @@ -133,13 +145,4 @@ bool binding_handle_port_binding_changes(struct > binding_ctx_in *, > > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > > > void binding_dump_local_bindings(struct local_binding_data *, struct ds > *); > > - > > -/* Generates a sset of lport names from local_binding_data. > > - * Note: the caller is responsible for destroying and freeing the > returned > > - * sset, by calling binding_detroy_local_binding_lports(). */ > > -struct sset *binding_collect_local_binding_lports(struct > local_binding_data *); > > - > > -/* Destroy and free the lports sset returned by > > - * binding_collect_local_binding_lports(). */ > > -void binding_destroy_local_binding_lports(struct sset *lports); > > #endif /* controller/binding.h */ > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 34eca135a..abb01f0ce 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -625,7 +625,7 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *lflow, > > get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); > > lflow_resource_add(l_ctx_out->lfrr, > REF_TYPE_PORTBINDING, buf, > > &lflow->header_.uuid); > > - if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { > > + if (!sset_contains(l_ctx_in->related_lport_ids, buf)) { > > VLOG_DBG("lflow "UUID_FMT > > " port %s in match is not local, skip", > > UUID_ARGS(&lflow->header_.uuid), > > diff --git a/controller/lflow.h b/controller/lflow.h > > index e98edf81d..699f9c2d5 100644 > > --- a/controller/lflow.h > > +++ b/controller/lflow.h > > @@ -144,7 +144,7 @@ struct lflow_ctx_in { > > const struct shash *addr_sets; > > const struct shash *port_groups; > > const struct sset *active_tunnels; > > - const struct sset *local_lport_ids; > > + const struct sset *related_lport_ids; > > }; > > > > struct lflow_ctx_out { > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index b6afb8fb9..3bb8b22eb 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1014,9 +1014,10 @@ struct ed_type_runtime_data { > > * local hypervisor, and localnet ports. */ > > struct sset local_lports; > > > > - /* Contains the same ports as local_lports, but in the format: > > - * <datapath-tunnel-key>_<port-tunnel-key> */ > > - struct sset local_lport_ids; > > + /* Port bindings that are relevant to the local chassis (VIFs bound > > + * localy, patch ports). > > + */ > > + struct related_lports related_lports; > > struct sset active_tunnels; > > > > /* runtime data engine private data. */ > > @@ -1109,7 +1110,7 @@ en_runtime_data_init(struct engine_node *node > OVS_UNUSED, > > > > hmap_init(&data->local_datapaths); > > sset_init(&data->local_lports); > > - sset_init(&data->local_lport_ids); > > + related_lports_init(&data->related_lports); > > sset_init(&data->active_tunnels); > > sset_init(&data->egress_ifaces); > > smap_init(&data->local_iface_ids); > > @@ -1127,7 +1128,7 @@ en_runtime_data_cleanup(void *data) > > struct ed_type_runtime_data *rt_data = data; > > > > sset_destroy(&rt_data->local_lports); > > - sset_destroy(&rt_data->local_lport_ids); > > + related_lports_destroy(&rt_data->related_lports); > > sset_destroy(&rt_data->active_tunnels); > > sset_destroy(&rt_data->egress_ifaces); > > smap_destroy(&rt_data->local_iface_ids); > > @@ -1219,8 +1220,8 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->local_datapaths = &rt_data->local_datapaths; > > b_ctx_out->local_lports = &rt_data->local_lports; > > b_ctx_out->local_lports_changed = false; > > - b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; > > - b_ctx_out->local_lport_ids_changed = false; > > + b_ctx_out->related_lports = &rt_data->related_lports; > > + b_ctx_out->related_lports_changed = false; > > b_ctx_out->non_vif_ports_changed = false; > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > @@ -1235,7 +1236,6 @@ en_runtime_data_run(struct engine_node *node, void > *data) > > struct ed_type_runtime_data *rt_data = data; > > struct hmap *local_datapaths = &rt_data->local_datapaths; > > struct sset *local_lports = &rt_data->local_lports; > > - struct sset *local_lport_ids = &rt_data->local_lport_ids; > > struct sset *active_tunnels = &rt_data->active_tunnels; > > > > static bool first_run = true; > > @@ -1252,12 +1252,12 @@ en_runtime_data_run(struct engine_node *node, > void *data) > > hmap_clear(local_datapaths); > > local_binding_data_destroy(&rt_data->lbinding_data); > > sset_destroy(local_lports); > > - sset_destroy(local_lport_ids); > > + related_lports_destroy(&rt_data->related_lports); > > sset_destroy(active_tunnels); > > sset_destroy(&rt_data->egress_ifaces); > > smap_destroy(&rt_data->local_iface_ids); > > sset_init(local_lports); > > - sset_init(local_lport_ids); > > + related_lports_init(&rt_data->related_lports); > > sset_init(active_tunnels); > > sset_init(&rt_data->egress_ifaces); > > smap_init(&rt_data->local_iface_ids); > > @@ -1327,7 +1327,7 @@ runtime_data_sb_port_binding_handler(struct > engine_node *node, void *data) > > } > > > > rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > > - if (b_ctx_out.local_lport_ids_changed || > > + if (b_ctx_out.related_lports_changed || > > b_ctx_out.non_vif_ports_changed || > > b_ctx_out.local_lports_changed || > > !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > > @@ -1638,11 +1638,8 @@ en_port_groups_run(struct engine_node *node, void > *data) > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > > - &rt_data->lbinding_data); > > - port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets, > > - &pg->port_groups_cs_local); > > - binding_destroy_local_binding_lports(local_b_lports); > > + port_groups_init(pg_table, &rt_data->related_lports.lport_names, > > + &pg->port_group_ssets, &pg->port_groups_cs_local); > > > > engine_set_node_state(node, EN_UPDATED); > > } > > @@ -1659,12 +1656,9 @@ port_groups_sb_port_group_handler(struct > engine_node *node, void *data) > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > > - &rt_data->lbinding_data); > > - port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets, > > - &pg->port_groups_cs_local, &pg->new, &pg->deleted, > > - &pg->updated); > > - binding_destroy_local_binding_lports(local_b_lports); > > + port_groups_update(pg_table, &rt_data->related_lports.lport_names, > > + &pg->port_group_ssets, &pg->port_groups_cs_local, > > + &pg->new, &pg->deleted, &pg->updated); > > > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > !sset_is_empty(&pg->updated)) { > > @@ -1697,9 +1691,6 @@ port_groups_runtime_data_handler(struct engine_node > *node, void *data) > > goto out; > > } > > > > - struct sset *local_b_lports = binding_collect_local_binding_lports( > > - &rt_data->lbinding_data); > > - > > const struct sbrec_port_group *pg_sb; > > SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) { > > struct sset *pg_lports = shash_find_data(&pg->port_group_ssets, > > @@ -1726,13 +1717,12 @@ port_groups_runtime_data_handler(struct > engine_node *node, void *data) > > if (need_update) { > > expr_const_sets_add_strings(&pg->port_groups_cs_local, > pg_sb->name, > > (const char *const *) > pg_sb->ports, > > - pg_sb->n_ports, local_b_lports); > > + pg_sb->n_ports, > > + > &rt_data->related_lports.lport_names); > > sset_add(&pg->updated, pg_sb->name); > > } > > } > > > > - binding_destroy_local_binding_lports(local_b_lports); > > - > > out: > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > !sset_is_empty(&pg->updated)) { > > @@ -2042,7 +2032,7 @@ init_lflow_ctx(struct engine_node *node, > > l_ctx_in->addr_sets = addr_sets; > > l_ctx_in->port_groups = port_groups; > > l_ctx_in->active_tunnels = &rt_data->active_tunnels; > > - l_ctx_in->local_lport_ids = &rt_data->local_lport_ids; > > + l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids; > > > > l_ctx_out->flow_table = &fo->flow_table; > > l_ctx_out->group_table = &fo->group_table; > > diff --git a/tests/ovn.at b/tests/ovn.at > > index db1a0a35c..7a718d4b6 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -26805,6 +26805,50 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > > > +# Tests that ACLs referencing port groups that include ports connected to > > +# logical routers are correctly applied. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- ACL with Port Group including router ports]) > > +ovn_start > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > + > > +check ovn-nbctl \ > > + -- lr-add lr \ > > + -- ls-add ls \ > > + -- lrp-add lr lrp_ls 00:00:00:00:00:01 42.42.42.1/24 \ > > + -- lsp-add ls ls_lr \ > > + -- lsp-set-addresses ls_lr router \ > > + -- lsp-set-type ls_lr router \ > > + -- lsp-set-options ls_lr router-port=lr_ls \ > > + -- lsp-add ls vm1 > > + > > +check ovn-nbctl pg-add pg ls_lr \ > > + -- acl-add pg from-lport 1 'inport == @pg && ip4.dst == 42.42.42.42' > drop > > + > > +check ovs-vsctl add-port br-int vm1 \ > > + -- set interface vm1 external_ids:iface-id=vm1 > > + > > +wait_for_ports_up > > +check ovn-nbctl --wait=hv sync > > + > > +dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=ls) > > +rtr_port_key=$(fetch_column Port_Binding tunnel_key logical_port=ls_lr) > > + > > +# Check that ovn-controller adds a flow to drop packets with dest IP > > +# 42.42.42.42 coming from the router port. > > +AT_CHECK([ovs-ofctl dump-flows br-int table=17 | grep > "reg14=0x${rtr_port_key},metadata=0x${dp_key},nw_dst=42.42.42.42 > actions=drop" -c], [0], [dnl > > +1 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn -- Static route with discard nexthop]) > > ovn_start > > -- > > 2.27.0 > > > > Thanks Dumitru! > Acked-by: Han Zhou <hzhou@ovn.org> > > Not sure if Numan would like to take a second look as well, so let's wait > for one or two days before merging. Thanks Dumitru and Han. I looked at it and LGTM. I applied the patch to the main branch. Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 7/1/21 2:53 PM, Numan Siddique wrote: >> Thanks Dumitru! >> Acked-by: Han Zhou <hzhou@ovn.org> >> >> Not sure if Numan would like to take a second look as well, so let's wait >> for one or two days before merging. > Thanks Dumitru and Han. > > I looked at it and LGTM. I applied the patch to the main branch. > > Numan > Thanks!
On Thu, Jul 1, 2021 at 6:05 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/1/21 2:53 PM, Numan Siddique wrote: > >> Thanks Dumitru! > >> Acked-by: Han Zhou <hzhou@ovn.org> > >> > >> Not sure if Numan would like to take a second look as well, so let's wait > >> for one or two days before merging. > > Thanks Dumitru and Han. > > > > I looked at it and LGTM. I applied the patch to the main branch. > > > > Numan > > > > Thanks! > Do we need this in branch-21.06 and 21.03?
On 7/1/21 6:39 PM, Han Zhou wrote: > On Thu, Jul 1, 2021 at 6:05 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 7/1/21 2:53 PM, Numan Siddique wrote: >>>> Thanks Dumitru! >>>> Acked-by: Han Zhou <hzhou@ovn.org> >>>> >>>> Not sure if Numan would like to take a second look as well, so let's > wait >>>> for one or two days before merging. >>> Thanks Dumitru and Han. >>> >>> I looked at it and LGTM. I applied the patch to the main branch. >>> >>> Numan >>> >> >> Thanks! >> > > Do we need this in branch-21.06 and 21.03? > Right, that would be great, thanks!
On Thu, Jul 1, 2021 at 10:18 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/1/21 6:39 PM, Han Zhou wrote: > > On Thu, Jul 1, 2021 at 6:05 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On 7/1/21 2:53 PM, Numan Siddique wrote: > >>>> Thanks Dumitru! > >>>> Acked-by: Han Zhou <hzhou@ovn.org> > >>>> > >>>> Not sure if Numan would like to take a second look as well, so let's > > wait > >>>> for one or two days before merging. > >>> Thanks Dumitru and Han. > >>> > >>> I looked at it and LGTM. I applied the patch to the main branch. > >>> > >>> Numan > >>> > >> > >> Thanks! > >> > > > > Do we need this in branch-21.06 and 21.03? > > > > Right, that would be great, thanks! > Ok, I rebased and pushed to 21.06 and 21.03. Thanks, Han
diff --git a/controller/binding.c b/controller/binding.c index 7fde0fdbb..594babc98 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx) } } -/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of local - * lport IDs. Also track if the set has changed. +/* Add a port binding to the set of locally relevant lports. + * Also track if the set has changed. */ static void -update_local_lport_ids(const struct sbrec_port_binding *pb, - struct binding_ctx_out *b_ctx) +update_related_lport(const struct sbrec_port_binding *pb, + struct binding_ctx_out *b_ctx) { char buf[16]; get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key, buf, sizeof(buf)); - if (sset_add(b_ctx->local_lport_ids, buf) != NULL) { - b_ctx->local_lport_ids_changed = true; + if (sset_add(&b_ctx->related_lports->lport_ids, buf) != NULL) { + b_ctx->related_lports_changed = true; if (b_ctx->tracked_dp_bindings) { /* Add the 'pb' to the tracked_datapaths. */ tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings); } } + sset_add(&b_ctx->related_lports->lport_names, pb->logical_port); } -/* Remove a port binding id from the set of local lport IDs. Also track if - * the set has changed. +/* Remove a port binding id from the set of locally relevant lports. + * Also track if the set has changed. */ static void -remove_local_lport_ids(const struct sbrec_port_binding *pb, - struct binding_ctx_out *b_ctx) +remove_related_lport(const struct sbrec_port_binding *pb, + struct binding_ctx_out *b_ctx) { char buf[16]; get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key, buf, sizeof(buf)); - if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) { - b_ctx->local_lport_ids_changed = true; + sset_find_and_delete(&b_ctx->related_lports->lport_names, + pb->logical_port); + if (sset_find_and_delete(&b_ctx->related_lports->lport_ids, buf)) { + b_ctx->related_lports_changed = true; if (b_ctx->tracked_dp_bindings) { /* Add the 'pb' to the tracked_datapaths. */ @@ -678,6 +681,20 @@ static struct binding_lport *binding_lport_check_and_cleanup( static char *get_lport_type_str(enum en_lport_type lport_type); +void +related_lports_init(struct related_lports *rp) +{ + sset_init(&rp->lport_names); + sset_init(&rp->lport_ids); +} + +void +related_lports_destroy(struct related_lports *rp) +{ + sset_destroy(&rp->lport_names); + sset_destroy(&rp->lport_ids); +} + void local_binding_data_init(struct local_binding_data *lbinding_data) { @@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis *chassis_rec, struct binding_ctx_out *b_ctx_out) { if (is_binding_lport_this_chassis(b_lport, chassis_rec)) { - remove_local_lport_ids(b_lport->pb, b_ctx_out); + remove_related_lport(b_lport->pb, b_ctx_out); if (!release_lport(b_lport->pb, sb_readonly, b_ctx_out->tracked_dp_bindings, b_ctx_out->if_mgr)) { @@ -1214,7 +1231,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, pb->datapath, false, b_ctx_out->local_datapaths, b_ctx_out->tracked_dp_bindings); - update_local_lport_ids(pb, b_ctx_out); + update_related_lport(pb, b_ctx_out); update_local_lports(pb->logical_port, b_ctx_out); if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(pb, qos_map); @@ -1405,7 +1422,7 @@ consider_virtual_lport(const struct sbrec_port_binding *pb, * its entry from the local_lport_ids if present. This is required * when a virtual port moves from one chassis to other.*/ if (!virtual_b_lport) { - remove_local_lport_ids(pb, b_ctx_out); + remove_related_lport(pb, b_ctx_out); } return true; @@ -1430,7 +1447,7 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, b_ctx_out->local_datapaths, b_ctx_out->tracked_dp_bindings); - update_local_lport_ids(pb, b_ctx_out); + update_related_lport(pb, b_ctx_out); return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, !b_ctx_in->ovnsb_idl_txn, false, b_ctx_out->tracked_dp_bindings, @@ -1482,7 +1499,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, get_qos_params(pb, qos_map); } - update_local_lport_ids(pb, b_ctx_out); + update_related_lport(pb, b_ctx_out); } static bool @@ -1512,7 +1529,7 @@ consider_ha_lport(const struct sbrec_port_binding *pb, pb->datapath, false, b_ctx_out->local_datapaths, b_ctx_out->tracked_dp_bindings); - update_local_lport_ids(pb, b_ctx_out); + update_related_lport(pb, b_ctx_out); } return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out); @@ -1634,7 +1651,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) case LP_PATCH: case LP_LOCALPORT: case LP_VTEP: - update_local_lport_ids(pb, b_ctx_out); + update_related_lport(pb, b_ctx_out); break; case LP_VIF: @@ -1895,7 +1912,7 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out, struct local_datapath *ld) { - remove_local_lport_ids(pb, b_ctx_out); + remove_related_lport(pb, b_ctx_out); if (!strcmp(pb->type, "patch") || !strcmp(pb->type, "l3gateway")) { remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths); @@ -2502,7 +2519,7 @@ delete_done: case LP_PATCH: case LP_LOCALPORT: case LP_VTEP: - update_local_lport_ids(pb, b_ctx_out); + update_related_lport(pb, b_ctx_out); if (lport_type == LP_PATCH) { if (!ld) { /* If 'ld' for this lport is not present, then check if @@ -2926,23 +2943,3 @@ cleanup: return b_lport; } - -struct sset * -binding_collect_local_binding_lports(struct local_binding_data *lbinding_data) -{ - struct sset *lports = xzalloc(sizeof *lports); - sset_init(lports); - struct shash_node *shash_node; - SHASH_FOR_EACH (shash_node, &lbinding_data->lports) { - struct binding_lport *b_lport = shash_node->data; - sset_add(lports, b_lport->name); - } - return lports; -} - -void -binding_destroy_local_binding_lports(struct sset *lports) -{ - sset_destroy(lports); - free(lports); -} diff --git a/controller/binding.h b/controller/binding.h index 8f3289476..a08011ae2 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -22,6 +22,7 @@ #include "openvswitch/hmap.h" #include "openvswitch/uuid.h" #include "openvswitch/list.h" +#include "sset.h" struct hmap; struct ovsdb_idl; @@ -56,6 +57,19 @@ struct binding_ctx_in { const struct ovsrec_interface_table *iface_table; }; +/* Locally relevant port bindings, e.g., VIFs that might be bound locally, + * patch ports. + */ +struct related_lports { + struct sset lport_names; /* Set of port names. */ + struct sset lport_ids; /* Set of <datapath-tunnel-key>_<port-tunnel-key> + * IDs for fast lookup. + */ +}; + +void related_lports_init(struct related_lports *); +void related_lports_destroy(struct related_lports *); + struct binding_ctx_out { struct hmap *local_datapaths; struct local_binding_data *lbinding_data; @@ -65,11 +79,9 @@ struct binding_ctx_out { /* Track if local_lports have been updated. */ bool local_lports_changed; - /* sset of local lport ids in the format - * <datapath-tunnel-key>_<port-tunnel-key>. */ - struct sset *local_lport_ids; - /* Track if local_lport_ids has been updated. */ - bool local_lport_ids_changed; + /* Port bindings that are relevant to the local chassis. */ + struct related_lports *related_lports; + bool related_lports_changed; /* Track if non-vif port bindings (e.g., patch, external) have been * added/deleted. @@ -133,13 +145,4 @@ bool binding_handle_port_binding_changes(struct binding_ctx_in *, void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); void binding_dump_local_bindings(struct local_binding_data *, struct ds *); - -/* Generates a sset of lport names from local_binding_data. - * Note: the caller is responsible for destroying and freeing the returned - * sset, by calling binding_detroy_local_binding_lports(). */ -struct sset *binding_collect_local_binding_lports(struct local_binding_data *); - -/* Destroy and free the lports sset returned by - * binding_collect_local_binding_lports(). */ -void binding_destroy_local_binding_lports(struct sset *lports); #endif /* controller/binding.h */ diff --git a/controller/lflow.c b/controller/lflow.c index 34eca135a..abb01f0ce 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -625,7 +625,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, &lflow->header_.uuid); - if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { + if (!sset_contains(l_ctx_in->related_lport_ids, buf)) { VLOG_DBG("lflow "UUID_FMT " port %s in match is not local, skip", UUID_ARGS(&lflow->header_.uuid), diff --git a/controller/lflow.h b/controller/lflow.h index e98edf81d..699f9c2d5 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -144,7 +144,7 @@ struct lflow_ctx_in { const struct shash *addr_sets; const struct shash *port_groups; const struct sset *active_tunnels; - const struct sset *local_lport_ids; + const struct sset *related_lport_ids; }; struct lflow_ctx_out { diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index b6afb8fb9..3bb8b22eb 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1014,9 +1014,10 @@ struct ed_type_runtime_data { * local hypervisor, and localnet ports. */ struct sset local_lports; - /* Contains the same ports as local_lports, but in the format: - * <datapath-tunnel-key>_<port-tunnel-key> */ - struct sset local_lport_ids; + /* Port bindings that are relevant to the local chassis (VIFs bound + * localy, patch ports). + */ + struct related_lports related_lports; struct sset active_tunnels; /* runtime data engine private data. */ @@ -1109,7 +1110,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, hmap_init(&data->local_datapaths); sset_init(&data->local_lports); - sset_init(&data->local_lport_ids); + related_lports_init(&data->related_lports); sset_init(&data->active_tunnels); sset_init(&data->egress_ifaces); smap_init(&data->local_iface_ids); @@ -1127,7 +1128,7 @@ en_runtime_data_cleanup(void *data) struct ed_type_runtime_data *rt_data = data; sset_destroy(&rt_data->local_lports); - sset_destroy(&rt_data->local_lport_ids); + related_lports_destroy(&rt_data->related_lports); sset_destroy(&rt_data->active_tunnels); sset_destroy(&rt_data->egress_ifaces); smap_destroy(&rt_data->local_iface_ids); @@ -1219,8 +1220,8 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->local_datapaths = &rt_data->local_datapaths; b_ctx_out->local_lports = &rt_data->local_lports; b_ctx_out->local_lports_changed = false; - b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; - b_ctx_out->local_lport_ids_changed = false; + b_ctx_out->related_lports = &rt_data->related_lports; + b_ctx_out->related_lports_changed = false; b_ctx_out->non_vif_ports_changed = false; b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; b_ctx_out->lbinding_data = &rt_data->lbinding_data; @@ -1235,7 +1236,6 @@ en_runtime_data_run(struct engine_node *node, void *data) struct ed_type_runtime_data *rt_data = data; struct hmap *local_datapaths = &rt_data->local_datapaths; struct sset *local_lports = &rt_data->local_lports; - struct sset *local_lport_ids = &rt_data->local_lport_ids; struct sset *active_tunnels = &rt_data->active_tunnels; static bool first_run = true; @@ -1252,12 +1252,12 @@ en_runtime_data_run(struct engine_node *node, void *data) hmap_clear(local_datapaths); local_binding_data_destroy(&rt_data->lbinding_data); sset_destroy(local_lports); - sset_destroy(local_lport_ids); + related_lports_destroy(&rt_data->related_lports); sset_destroy(active_tunnels); sset_destroy(&rt_data->egress_ifaces); smap_destroy(&rt_data->local_iface_ids); sset_init(local_lports); - sset_init(local_lport_ids); + related_lports_init(&rt_data->related_lports); sset_init(active_tunnels); sset_init(&rt_data->egress_ifaces); smap_init(&rt_data->local_iface_ids); @@ -1327,7 +1327,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) } rt_data->local_lports_changed = b_ctx_out.local_lports_changed; - if (b_ctx_out.local_lport_ids_changed || + if (b_ctx_out.related_lports_changed || b_ctx_out.non_vif_ports_changed || b_ctx_out.local_lports_changed || !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { @@ -1638,11 +1638,8 @@ en_port_groups_run(struct engine_node *node, void *data) struct ed_type_runtime_data *rt_data = engine_get_input_data("runtime_data", node); - struct sset *local_b_lports = binding_collect_local_binding_lports( - &rt_data->lbinding_data); - port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets, - &pg->port_groups_cs_local); - binding_destroy_local_binding_lports(local_b_lports); + port_groups_init(pg_table, &rt_data->related_lports.lport_names, + &pg->port_group_ssets, &pg->port_groups_cs_local); engine_set_node_state(node, EN_UPDATED); } @@ -1659,12 +1656,9 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) struct ed_type_runtime_data *rt_data = engine_get_input_data("runtime_data", node); - struct sset *local_b_lports = binding_collect_local_binding_lports( - &rt_data->lbinding_data); - port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets, - &pg->port_groups_cs_local, &pg->new, &pg->deleted, - &pg->updated); - binding_destroy_local_binding_lports(local_b_lports); + port_groups_update(pg_table, &rt_data->related_lports.lport_names, + &pg->port_group_ssets, &pg->port_groups_cs_local, + &pg->new, &pg->deleted, &pg->updated); if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || !sset_is_empty(&pg->updated)) { @@ -1697,9 +1691,6 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data) goto out; } - struct sset *local_b_lports = binding_collect_local_binding_lports( - &rt_data->lbinding_data); - const struct sbrec_port_group *pg_sb; SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) { struct sset *pg_lports = shash_find_data(&pg->port_group_ssets, @@ -1726,13 +1717,12 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data) if (need_update) { expr_const_sets_add_strings(&pg->port_groups_cs_local, pg_sb->name, (const char *const *) pg_sb->ports, - pg_sb->n_ports, local_b_lports); + pg_sb->n_ports, + &rt_data->related_lports.lport_names); sset_add(&pg->updated, pg_sb->name); } } - binding_destroy_local_binding_lports(local_b_lports); - out: if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || !sset_is_empty(&pg->updated)) { @@ -2042,7 +2032,7 @@ init_lflow_ctx(struct engine_node *node, l_ctx_in->addr_sets = addr_sets; l_ctx_in->port_groups = port_groups; l_ctx_in->active_tunnels = &rt_data->active_tunnels; - l_ctx_in->local_lport_ids = &rt_data->local_lport_ids; + l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids; l_ctx_out->flow_table = &fo->flow_table; l_ctx_out->group_table = &fo->group_table; diff --git a/tests/ovn.at b/tests/ovn.at index db1a0a35c..7a718d4b6 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -26805,6 +26805,50 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) +# Tests that ACLs referencing port groups that include ports connected to +# logical routers are correctly applied. +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- ACL with Port Group including router ports]) +ovn_start +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +check ovn-nbctl \ + -- lr-add lr \ + -- ls-add ls \ + -- lrp-add lr lrp_ls 00:00:00:00:00:01 42.42.42.1/24 \ + -- lsp-add ls ls_lr \ + -- lsp-set-addresses ls_lr router \ + -- lsp-set-type ls_lr router \ + -- lsp-set-options ls_lr router-port=lr_ls \ + -- lsp-add ls vm1 + +check ovn-nbctl pg-add pg ls_lr \ + -- acl-add pg from-lport 1 'inport == @pg && ip4.dst == 42.42.42.42' drop + +check ovs-vsctl add-port br-int vm1 \ + -- set interface vm1 external_ids:iface-id=vm1 + +wait_for_ports_up +check ovn-nbctl --wait=hv sync + +dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=ls) +rtr_port_key=$(fetch_column Port_Binding tunnel_key logical_port=ls_lr) + +# Check that ovn-controller adds a flow to drop packets with dest IP +# 42.42.42.42 coming from the router port. +AT_CHECK([ovs-ofctl dump-flows br-int table=17 | grep "reg14=0x${rtr_port_key},metadata=0x${dp_key},nw_dst=42.42.42.42 actions=drop" -c], [0], [dnl +1 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn -- Static route with discard nexthop]) ovn_start
It's valid that port_groups contain non-vif ports, they can actually contain any type of logical_switch_port. Also, there's no need to allocate a new sset containing the local ports' names every time the I-P engine processes a change. We were already maintaining a set of "local_lport_ids". These correspond to port bindings that are relevant locally (including non-vif ports). Extend it to include the locally relevant lport names too and rename the structure an its helper functions to related_lport*. Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163 Reported-by: Antonio Ojea <aojea@redhat.com> Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v2: - Addressed Numan's and Han's comments: - add struct related_lports - add test case. --- controller/binding.c | 79 ++++++++++++++++++------------------- controller/binding.h | 31 ++++++++------- controller/lflow.c | 2 +- controller/lflow.h | 2 +- controller/ovn-controller.c | 48 +++++++++------------- tests/ovn.at | 44 +++++++++++++++++++++ 6 files changed, 120 insertions(+), 86 deletions(-)