On 3/15/22 10:35, Abhiram Sangana wrote:
Hey Mark,
Thanks for reviewing the patch. Regarding `ovn-nbctl lr-nat-del`, I
have updated the command to have the following structure:
`lr-nat-del ROUTER [TYPE [IP [GATEWAY_PORT]]]`. Most of the
earlier checks are not enforced unless `GATEWAY_PORT` is also
passed.
With the new patch, a NAT rule is no longer uniquely identified by
`TYPE` and `IP`. The user also needs to pass `GATEWAY_PORT` to
uniquely identify a NAT rule. So, I have updated the behavior of
`lr-nat-del` to not raise an error when `IP` is passed but no NAT
rules match, similar to its behavior when `TYPE` is passed and no
NAT rules match.
Ah, I didn't realize that this case didn't return an error. It seems odd
to me that we wouldn't return an error here, but I won't try to rock the
boat too much with regards to existing behavior.
Also, with the new patch, if `IP` is passed without
passing `GATEWAY_PORT`, all NAT rules (possibly None) with the
`TYPE` and `IP` values are deleted irrespective of their
`GATEWAY_PORT` value.
I'm a bit surprised that with this change, the hierarchy for specificity
is TYPE < IP < GATEWAY_PORT . This makes it sound as though the primary
use case would be to use the same IP for multiple NAT rules across
different gateway ports. Wouldn't it be just as likely that the same
gateway port would be used for multiple NAT rules all with different IPs?
In other words, I suspect that something like this:
ovn-nbctl lr-nat-del my_router dnat_and_snat my_router_gateway_port_1
where the intent is to remove all NAT rules from my_router whose gateway
port is my_router_gateway_port_1, would be just as useful as something like:
ovn-nbctl lr-nat-del my_router dnat 10.0.0.1
where the intent is to remove all NAT rules from my_router whose
external IP address is 10.0.0.1 .
Would it make sense to change the syntax from
ovn-nbctl lr-nat-del [TYPE [IP [GATEWAY_PORT]]]
to
ovn-nbctl lr-nat-del [TYPE [IP] [GATEWAY_PORT]]
?
It would be a bit of work to determine if the two-parameter version of
the command is giving an IP address or gateway port, but it wouldn't be
too difficult I don't think.
Should we retain the previous behavior of `lr-nat-del` when LR has a
single DGP - raise an error if no NAT rules match when `IP` is passed?
Short version: no.
Long version:
If I had it my way, then lr-nat-del would raise an error any time it is
run and no NAT rules can be found to delete (unless --if-exists is
passed). This way it would be very easy to explain when an error is
returned and when one isn't.
However, as you pointed out there is a precedent where an attempted
deletion of multiple rows does not raise an error if matches are not
found. Since it is now possible for a router to have multiple NAT
entries with the same IP address (as long as they are on different
gateway ports), then I guess we should follow that same precedent.
I think you should leave it the way you have it since it is easiest to
explain (deleting multiple rules == never raise an error, deleting a
specific rule == raise an error if it doesn't exist). Otherwise, the
nuances are difficult to explain and difficult to maintain.
On 14 Mar 2022, at 20:55, Mark Michelson <[email protected]
<mailto:[email protected]>> wrote:
Hi Abhiram,
I had a look through the code and I'm happy with how it looks. I also
did a quick check through the testsuite and it all seems good. All
that being said,
I nearly acked this, but I have one question down below in the test
code. It may indicate some issue in `ovn-nbctl lr-nat-del` so I'd like
to get that cleared up first.
-AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.3], [1], [],
-[ovn-nbctl: no matching NAT with the type (dnat_and_snat) and
external_ip (30.0.0.3)
-])
-AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat 30.0.0.2], [1], [],
-[ovn-nbctl: no matching NAT with the type (dnat) and external_ip
(30.0.0.2)
-])
-AT_CHECK([ovn-nbctl lr-nat-del lr0 snat 192.168.10.0/24], [1], [],
-[ovn-nbctl: no matching NAT with the type (snat) and logical_ip
(192.168.10.0/24)
-])
-AT_CHECK([ovn-nbctl --if-exists lr-nat-del lr0 snat 192.168.10.0/24])
+AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.3])
I don't understand the above change. First, I don't understand why
many of the lr-nat-del checks were removed. Second, I don't understand
why the lr-nat-del of 30.0.0.3 doesn't return an error. There is no
NAT with that address, so shouldn't that return an error?
AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.1])
AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat fd01::1])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
