In the current code, the map of chain IDs is kept in a global simap. This means that when a particular router policy is encountered, it will always be given the same numerical ID given its string chain ID.
This commit changes the code so that the numerical IDs are only consistent for a logical router policy within a particular logical router. This does not require any changes to logical flows, since the logical flows already discriminate based on datapath tunnel key. First, this prevents users from jumping from a policy attached to logical router A to a policy attached to logical router B. You may only jump to policies that are attached to the same logical router. Second, this allows for more chain IDs to be used. Previously, the global simap would only allow for 65535 total chain IDs to be used. Now, 65535 chain IDs can be used per logical router. This is not likely to be useful, but it's worth noting. Finally, this change makes logical flow generation more consistent in the case where chain IDs are used only on particular datapaths. The code would set a policy's chain_id to -1 if the global simap of chain_ids is empty. However, if chain_ids are not empty, and a policy does not specify a chain_id, then the chain_id is set to 0. Let's picture a scenario where logical router A does not use chain_ids or jump_chains, and logical router B uses chain_ids and jump_chains. If the route policy code evaluates A before B, then all of A's policies will have chain_id set to -1. This results in the matches for logical router policies to look like this: match = (<policy match>) If the route policy code evaluates B before A, then all of A's policies will have chain_ids set to 0. This results in the matches for logical router policies to look like this: match = (REG_POLICY_CHAIN_ID == 0 && (<policy_match>)) The resulting behavior ends up being the same for router A's policies, since REG_POLICY_CHAIN_ID is never explicitly set and defaults to 0. With this change, now router A's policies will be consistently set to -1, no matter whether they are evaluated before or after B's policies. Signed-off-by: Mark Michelson <[email protected]> --- northd/en-route-policies.c | 7 ++-- northd/en-route-policies.h | 2 -- tests/ovn-northd.at | 71 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c index dd6d182e8..9597980ec 100644 --- a/northd/en-route-policies.c +++ b/northd/en-route-policies.c @@ -288,7 +288,6 @@ route_policies_init(struct route_policies_data *data) { hmap_init(&data->route_policies); hmap_init(&data->bfd_active_connections); - simap_init(&data->chain_ids); } static void @@ -301,7 +300,6 @@ route_policies_destroy(struct route_policies_data *data) }; hmap_destroy(&data->route_policies); bfd_destroy(&data->bfd_active_connections); - simap_destroy(&data->chain_ids); } enum engine_node_state @@ -316,10 +314,13 @@ en_route_policies_run(struct engine_node *node, void *data) struct ovn_datapath *od; HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) { + struct simap chain_ids = SIMAP_INITIALIZER(&chain_ids); + build_route_policies(od, &bfd_data->bfd_connections, &route_policies_data->route_policies, &route_policies_data->bfd_active_connections, - &route_policies_data->chain_ids); + &chain_ids); + simap_destroy(&chain_ids); } return EN_UPDATED; diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h index 1d2288e46..1b13eb4cf 100644 --- a/northd/en-route-policies.h +++ b/northd/en-route-policies.h @@ -19,7 +19,6 @@ #include "inc-proc-eng.h" -#include "lib/simap.h" #include "openvswitch/hmap.h" /* Represents the data associated with an instance of a northbound @@ -38,7 +37,6 @@ struct route_policy { struct route_policies_data { struct hmap route_policies; struct hmap bfd_active_connections; - struct simap chain_ids; }; void en_route_policies_cleanup(void *data); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 0a488ed64..ef56a2882 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -24725,3 +24725,74 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE OVN_CLEANUP_NORTHD AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Router policy invalid chain IDs]) +ovn_start + +check ovn-nbctl lr-add lr0 +check ovn-nbctl lr-add lr1 + +# Set up a policy on lr0 to jump to a policy on chain lr1 +check ovn-nbctl lr-policy-add lr0 102 "ip4.src == 10.0.0.0/24" jump inbound \ + -- --chain=inbound lr-policy-add lr1 201 "1" allow +check ovn-nbctl --wait=sb sync + +# The policy on lr0 should not be installed since it jumps to +# a chain on a different router. +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | ovn_strip_lflows | sort], [0], [dnl + table=??(lr_in_policy ), priority=0 , match=(1), action=(reg8[[0..15]] = 0; next;) +]) + +# Double-check that we see the expected warning in the logs. +check grep -qE "Logical router: lr0, policy action 'jump' follows to non-existent chain inbound" northd/ovn-northd.log + +OVN_CLEANUP_NORTHD +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Router policy chain IDs consistent lflows]) +ovn_start + +# For this test, we create two logical routers, one with policies with jump chains, and one with +# simple allow policies. We then will do the same but swap the policies so that they are on the +# opposite routers. We want to ensure that in both scenarios, the generated flows are consistent +# for the router without the jump chains. +check ovn-nbctl lr-add lr0 +check ovn-nbctl lr-add lr1 + +# First, let's put the jump policy on lr0 +check ovn-nbctl lr-policy-add lr0 102 "ip4.src == 10.0.0.0/24" jump inbound \ + -- --chain=inbound lr-policy-add lr0 201 "1" allow + +# lr1 just gets an allow policy. +check ovn-nbctl lr-policy-add lr1 102 "ip4.src == 10.0.0.0/24" allow +check ovn-nbctl --wait=sb sync + +# The flow on lr1 should not check the chain ID register as part of its match. +AT_CHECK([ovn-sbctl lflow-list lr1 | grep "lr_in_policy[[^_]]" | ovn_strip_lflows | sort], [0], [dnl + table=??(lr_in_policy ), priority=0 , match=(1), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=102 , match=(ip4.src == 10.0.0.0/24), action=(reg8[[0..15]] = 0; next;) +]) + +# Delete the policies from both routers +check ovn-nbctl lr-policy-del lr0 +check ovn-nbctl lr-policy-del lr1 + +# Install the same policies, but on the opposite routers. +check ovn-nbctl lr-policy-add lr1 102 "ip4.src == 10.0.0.0/24" jump inbound \ + -- --chain=inbound lr-policy-add lr1 201 "1" allow +check ovn-nbctl lr-policy-add lr0 102 "ip4.src == 10.0.0.0/24" allow +check ovn-nbctl --wait=sb sync + +# The flow on lr0 should not check the chain ID register as part of its match. +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | ovn_strip_lflows | sort], [0], [dnl + table=??(lr_in_policy ), priority=0 , match=(1), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=102 , match=(ip4.src == 10.0.0.0/24), action=(reg8[[0..15]] = 0; next;) +]) + +CHECK_NO_CHANGE_AFTER_RECOMPUTE +OVN_CLEANUP_NORTHD +AT_CLEANUP +]) -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
