If a chain is not specified when deleting a logical router policy, then the passed-in chain value is normalized to an empty string. This value is then always used in comparisons to existing logical router policies when trying to find which should be deleted.
What this means is that any policies that have chain values cannot be deleted with a command such as lr-policy-del lr0 100. A user likely assumes that all policies with priority 100 would be deleted, but it actually only deletes policies with priority 100 and no chain. With this commit, if a chain is not specified when attempting to delete logical router policies, it can delete policies with any chain value. Signed-off-by: Mark Michelson <[email protected]> --- This patch is marked RFC because there were existing tests that explicitly checked that an lr-policy-del call with no chain specified did NOT delete policies that had a chain specified. This could potentially break some setups, even though IMO it makes more sense to operate this way. --- NEWS | 2 ++ tests/ovn-nbctl.at | 32 ++++++++++++++------------------ utilities/ovn-nbctl.c | 2 +- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/NEWS b/NEWS index 3724e0b15..ae5156689 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,7 @@ Post v26.09.0 -------------- + - ovn-nbctl lr-policy-del, when not passed a --chain value, can now delete + matching policies with any chain value. - Logical_Router_Policy priorities now have a minimum value of 1 instead of 0. This is to prevent a potentially ambiguity with default flows installed by OVN. diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 89cfe4c03..65d8571be 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -2555,53 +2555,36 @@ Chain <Default>: 101 ip6.src == 2002::/64 drop 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar -Chain inbound: - 100 ip4.src == 1.1.1.0/24 drop - Chain outbound: 100 ip4.src == 2.1.1.0/24 jump -> inbound ]) -AT_CHECK([ovn-nbctl --chain inbound lr-policy-del lr0 100 "ip4.src == 1.1.1.0/24"]) +AT_CHECK([ovn-nbctl --chain outbound lr-policy-del lr0 100 "ip4.src == 2.1.1.0/24"]) AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl Routing Policies -Chain <Default>: 101 ip4.src == 2.1.1.0/24 allow 101 ip4.src == 2.1.2.0/24 drop 101 ip4.src == 3.1.1.0/24 reroute 192.168.0.10 lrp0 101 ip6.src == 2002::/64 drop 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar - -Chain outbound: - 100 ip4.src == 2.1.1.0/24 jump -> inbound ]) dnl Delete all policies for given priority AT_CHECK([ovn-nbctl lr-policy-del lr0 101]) AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl Routing Policies -Chain <Default>: 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar - -Chain outbound: - 100 ip4.src == 2.1.1.0/24 jump -> inbound ]) dnl Delete policy by specified uuid uuid=$(ovn-nbctl --bare --column _uuid find logical_router_policy 'chain{<=}""') AT_CHECK([ovn-nbctl lr-policy-del lr0 $uuid]) AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl -Routing Policies - -Chain outbound: - 100 ip4.src == 2.1.1.0/24 jump -> inbound ]) AT_CHECK([ovn-nbctl --if-exists lr-policy-del lr0 $uuid]) -AT_CHECK([ovn-nbctl --chain outbound lr-policy-del lr0]) - AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl ]) @@ -3819,3 +3802,16 @@ check ovn-nbctl lr-policy-del lr0 0 "1" # Make sure that it was actually deleted. wait_row_count nb:Logical_Router_Policy 0 ]) + +OVN_NBCTL_TEST([route_policy_delete_no_chain], [Router policy deletion with no specified chain], [ +check ovn-nbctl lr-add lr0 +check ovn-nbctl --chain=foo lr-policy-add lr0 100 "1" allow + +# Let's make sure that when we try to delete all policies of +# priority 100, it gets the policy we just added. We should not +# need to specify a chain. +check ovn-nbctl lr-policy-del lr0 100 + +# Double check it was actually deleted. +wait_row_count nb:Logical_Router_Policy 0 +]) diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 77fdbbba5..a52fc5503 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -4965,7 +4965,7 @@ nbctl_lr_policy_del(struct ctl_context *ctx) /* Delete policies selected by chain, priority (if set), match (if set). Sure, at least one option is set here */ if ((!priority || priority == policy->priority) - && (!strcmp(chain, string_ptr(policy->chain))) + && (!*chain || !strcmp(chain, string_ptr(policy->chain))) && (!match || !strcmp(match, policy->match))) { nbrec_logical_router_update_policies_delvalue(lr, policy); removed = true; -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
