On 15/07/2021 14:16, Mark Michelson wrote:
> Hi Mark,
>
> I'm a bit curious about this change. Does the removal of the protocol
> from the match mean that traffic that is not of the protocol specified
> in the load balancer will be ct_dnat()'ed? Does that constitute
> unexpected behavior?
>
Yes, this is the case. It's a tradeoff between number of flows and
reirculations but thinking about it again, it may be better to have more
flows. I will create a v2.
On 7/15/21 8:14 AM, Mark Gray wrote:
>> When adding two SB flows with the same vip but different protocols, only
>> the most recent flow will be added due to the `if` statement:
>>
>> if (!sset_contains(&all_ips, lb_vip->vip_str)) {
>> sset_add(&all_ips, lb_vip->vip_str);
>>
>> This can cause unexpected behaviour when two load balancers with
>> the same VIP (and different protocols) are added to a logical router.
>>
>> This is due to the addition of "protocol" to the match in
>> defrag table flows in a previous commit. Revert that change.
>>
>> This bug was discovered through the OVN CI (ovn-kubernetes.yml).
>>
>> Fixes: 384a7c6237da ("northd: Refactor Logical Flows for routers with
>> DNAT/Load Balancers")
>> Signed-off-by: Mark Gray <[email protected]>
>> ---
>> northd/ovn-northd.c | 8 --------
>> northd/ovn_northd.dl | 9 +--------
>> tests/ovn-northd.at | 46 ++++++++++++++++++++++----------------------
>> 3 files changed, 24 insertions(+), 39 deletions(-)
>>
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 999c3f482c29..5fab62c0fcf7 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -9219,11 +9219,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct
>> ovn_datapath *od,
>> for (size_t j = 0; j < lb->n_vips; j++) {
>> struct ovn_lb_vip *lb_vip = &lb->vips[j];
>>
>> - bool is_udp = nullable_string_is_equal(nb_lb->protocol, "udp");
>> - bool is_sctp = nullable_string_is_equal(nb_lb->protocol,
>> - "sctp");
>> - const char *proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
>> -
>> struct ds defrag_actions = DS_EMPTY_INITIALIZER;
>> if (!sset_contains(&all_ips, lb_vip->vip_str)) {
>> sset_add(&all_ips, lb_vip->vip_str);
>> @@ -9249,9 +9244,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct
>> ovn_datapath *od,
>> lb_vip->vip_str);
>> }
>>
>> - if (lb_vip->vip_port) {
>> - ds_put_format(match, " && %s", proto);
>> - }
>> ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG,
>> 100, ds_cstr(match),
>> ds_cstr(&defrag_actions),
>> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
>> index ceeabe6f384e..b37da86f76aa 100644
>> --- a/northd/ovn_northd.dl
>> +++ b/northd/ovn_northd.dl
>> @@ -6167,14 +6167,7 @@ for (RouterLBVIP(
>> * pick a DNAT ip address from a group.
>> * 2. If there are L4 ports in load balancing rules, we
>> * need the defragmentation to match on L4 ports. */
>> - var match1 = "ip && ${ipX}.dst == ${ip_address}" in
>> - var match2 =
>> - if (port != 0) {
>> - " && ${proto}"
>> - } else {
>> - ""
>> - } in
>> - var __match = match1 ++ match2 in
>> + var __match = "ip && ${ipX}.dst == ${ip_address}" in
>> var xx = ip_address.xxreg() in
>> var __actions = "${xx}${rEG_NEXT_HOP()} = ${ip_address}; ct_dnat;"
>> in
>> /* One of these flows must be created for each unique LB VIP
>> address.
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 11461d3f4c2a..072616898d63 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -3167,7 +3167,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -3200,7 +3200,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -3243,7 +3243,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -3300,7 +3300,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -3344,7 +3344,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.20 && tcp), action=(reg0 = 10.0.0.20; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.20), action=(reg0 = 10.0.0.20; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | grep skip_snat_for_lb | sort], [0],
>> [dnl
>> @@ -4361,10 +4361,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>> table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -4421,10 +4421,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>> table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -4476,10 +4476,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>> table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -4534,11 +4534,11 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.10), action=(reg0 = 172.168.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>> table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>> @@ -4605,12 +4605,12 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
>> [dnl
>>
>> AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>> table=5 (lr_in_defrag ), priority=0 , match=(1), action=(next;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.10), action=(reg0 = 172.168.0.10; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>> table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
>> - table=5 (lr_in_defrag ), priority=100 , match=(ip && ip6.dst ==
>> def0::2 && tcp), action=(xxreg0 = def0::2; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip4.dst ==
>> 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>> + table=5 (lr_in_defrag ), priority=100 , match=(ip && ip6.dst ==
>> def0::2), action=(xxreg0 = def0::2; ct_dnat;)
>> ])
>>
>> AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
