On 8/7/20 6:11 PM, Aniket Bhat wrote: > This commit fixes a case where may-exist flag was not honored. > When the nat type and logical ip both match for snat, but the > external_ip is not the same, the may-exist flag was being ignored. > > Signed-off-by: Aniket Bhat <anb...@redhat.com> > ---
Hi Aniket, Thanks for the patch! However, I'm not sure if this is the right approach. Before your patch the semantics of "lr-nat-add --may-exist" were: 1. For SNAT, the "unique key" of a NAT record is nat->logical_ip with the additional constraint that the tuple <nat->logical_ip, nat->external_ip> is unique across records of the same logical router. This means: If a SNAT entry already existed for the Logical IP (private IP) "--may-exist" allows overwriting it with a new entry only if the External IP (public IP) also matches. If the new entry external IP doesn't match the old one, this is essentially a different SNAT entry and "--may-exist" doesn't apply. 2. For DNAT, the "unique key" of a NAT record is nat->external_ip with the additional constraint that the tuple <nat->logical_ip, nat->external_ip> is unique across records of the same logical router. This means: If a DNAT entry already existed for the External IP (public IP) "--may-exist" allows overwriting it with a new entry only if the Logical IP (private IP) also matches. The other fields (external_mac, logical_port) get updated from the new entry. If the new entry logical IP doesn't match the old one, this is essentially a different DNAT entry and "--may-exist" doesn't apply. With your patch applied the following sequence (incorrect in my opinion) is allowed: $ ovn-nbctl lr-nat-add rtr snat 10.0.0.0 20.0.0.0 $ ovn-nbctl lr-nat-list rtr TYPE EXTERNAL_IP EXTERNAL_PORT LOGICAL_IP EXTERNAL_MAC LOGICAL_PORT snat 10.0.0.0 20.0.0.0 $ ovn-nbctl --may-exist lr-nat-add rtr snat 10.0.0.1 20.0.0.0 $ ovn-nbctl lr-nat-list rtr TYPE EXTERNAL_IP EXTERNAL_PORT LOGICAL_IP EXTERNAL_MAC LOGICAL_PORT snat 10.0.0.0 20.0.0.0 Even though we passed "--may-exist" the old SNAT entry doesn't match the new SNAT entry but ovn-nbctl allows it. Furthermore, even though the new external IP should be 10.0.0.1, this doesn't get updated. Another example of similar --may-exist behavior is for logical switch port addition: $ ovn-nbctl lsp-add ls lsp2 $ ovn-nbctl --may-exist lsp-add ls lsp2 $ ovn-nbctl --may-exist lsp-add ls lsp2 lsp1 100 ovn-nbctl: lsp2: port already exists but has no parent Here the unique key is the port name but the constraint is that the parent port should be the same if "--may-exist" is specified. Regards, Dumitru > utilities/ovn-nbctl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index e6d8dbe..8194414 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -4295,11 +4295,13 @@ nbctl_lr_nat_add(struct ctl_context *ctx) > should_return = true; > } > } else { > - ctl_error(ctx, "a NAT with this type (%s) and %s (%s) " > - "already exists", > - nat_type, > - is_snat ? "logical_ip" : "external_ip", > - is_snat ? new_logical_ip : new_external_ip); > + if (!may_exist) { > + ctl_error(ctx, "a NAT with this type (%s) and %s > (%s) " > + "already exists", > + nat_type, > + is_snat ? "logical_ip" : "external_ip", > + is_snat ? new_logical_ip : > new_external_ip); > + } > should_return = true; > } > } > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev