[ovs-dev] [PATCH ovn] Fix an issue with may-exist flag for lr_nat_add

[Dumitru Ceara]

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 at 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;
>                  }
>              }
>