Message ID | 20200807161156.9438-1-anbhat@redhat.com |
---|---|
State | Rejected |
Headers | show |
Series | [ovs-dev,ovn] Fix an issue with may-exist flag for lr_nat_add | expand |
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 <anbhat@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; > } > } >
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; } }
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 <anbhat@redhat.com> --- utilities/ovn-nbctl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)