Hi Lorenzo,
I have a question about the intent of this patch. In both the cited BZ,
as well as the altered test, the procedure is to create a stateless NAT,
and then try to create an identical stateless NAT with --may-exist.
However, what about the case when the existing NAT is not stateless, but
we try to add a stateless NAT with --may-exist?
With the old code, this would result in an error message. With this
change, the error message is not output, and the new stateless NAT is
not added.
Is this intentional? If there is a mismatch between statelessness in the
old and new NAT, should there be an error message output even if
--may-exist is present?
I have one additional note down below.
On 4/11/22 13:18, Lorenzo Bianconi wrote:
Do not provide an error log if there is an already created stateless nat entry
with the same external_ip and if the --may-exist option is provided in
the ovn-nbctl command.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2066551
Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
---
tests/ovn-nbctl.at | 2 ++
utilities/ovn-nbctl.c | 9 ++++++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index f9b9defd0..dd1626a96 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -597,6 +597,8 @@ AT_CHECK([ovn-nbctl --stateless lr-nat-add lr0
dnat_and_snat 40.0.0.3 192.168.1.
[ovn-nbctl: 40.0.0.3, 192.168.1.7: External ip cannot be shared across
stateless and stateful NATs
])
+AT_CHECK([ovn-nbctl --stateless --may-exist lr-nat-add lr0 dnat_and_snat 40.0.0.3 192.168.1.7])
+
dnl Deletes the NATs
AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.3])
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 4ed4d27c6..81913f559 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -4825,9 +4825,12 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
struct smap nat_options = SMAP_INITIALIZER(&nat_options);
I noticed that the above smap is declared but not used. If you're
messing around in this area, you can just remove this.
if (!strcmp(smap_get(&nat->options, "stateless"),
"true") || stateless) {
- ctl_error(ctx, "%s, %s: External ip cannot be shared "
- "across stateless and stateful NATs",
- new_external_ip, new_logical_ip);
+ if (!may_exist) {
+ ctl_error(ctx, "%s, %s: External ip cannot be shared "
+ "across stateless and stateful NATs",
+ new_external_ip, new_logical_ip);
+ }
+ should_return = true;
}
}
}
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
