Hi Lorenzo,
In general, this patch seems overly complicated based on the linked bug
report.
The bug report wants to make the lb_force_snat_ip behavior to only be
applied to certain load balancers, and not all load balancers on that
router. To me, this simply means that if you have
options:lb_skip_snat=true on your load balancer, you just act as though
lb_force_snat_ip were not set on the logical router for that load balancer.
A couple of notes about the new option:
1) The "lb_" prefix on "lb_skip_snat" is not necessary since the option
is on the load balancer itself. No other columns or options on load
balancers have the "lb_" prefix.
2) The option is not documented in ovn-nb.xml.
I'm going to review the code below as written, because I may be mistaken
about the expected logic of lb_skip_snat.
On 4/1/21 10:43 AM, Lorenzo Bianconi wrote:
Inotroduce lb_skip_snat in load balancer option column in order to not
s/Inotrudoce/Introduce/
force_snat traffic that is hitting a given load balancer applied on a
logical router where lb_force_snat has been configured
https://bugzilla.redhat.com/show_bug.cgi?id=1927540
Tested-by: Andrew Stoycos <[email protected]>
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
include/ovn/logical-fields.h | 5 ++++
lib/logical-fields.c | 4 ++++
northd/lrouter.dl | 10 ++++++--
northd/ovn-northd.8.xml | 25 +++++++++++++++++++-
northd/ovn-northd.c | 44 ++++++++++++++++++++++++------------
northd/ovn_northd.dl | 32 +++++++++++++++++++-------
tests/ovn-northd.at | 25 +++++++++++++++++++-
7 files changed, 119 insertions(+), 26 deletions(-)
diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
index 017176f98..d44b30b30 100644
--- a/include/ovn/logical-fields.h
+++ b/include/ovn/logical-fields.h
@@ -66,6 +66,7 @@ enum mff_log_flags_bits {
MLF_LOOKUP_MAC_BIT = 6,
MLF_LOOKUP_LB_HAIRPIN_BIT = 7,
MLF_LOOKUP_FDB_BIT = 8,
+ MLF_SKIP_SNAT_FOR_LB_BIT = 9,
};
/* MFF_LOG_FLAGS_REG flag assignments */
@@ -102,6 +103,10 @@ enum mff_log_flags {
/* Indicate that the lookup in the fdb table was successful. */
MLF_LOOKUP_FDB = (1 << MLF_LOOKUP_FDB_BIT),
+
+ /* Indicate that a packet must not SNAT in the gateway router when
+ * load-balancing has taken place. */
+ MLF_SKIP_SNAT_FOR_LB = (1 << MLF_SKIP_SNAT_FOR_LB_BIT),
};
/* OVN logical fields
diff --git a/lib/logical-fields.c b/lib/logical-fields.c
index 9d08b44c2..72853013e 100644
--- a/lib/logical-fields.c
+++ b/lib/logical-fields.c
@@ -121,6 +121,10 @@ ovn_init_symtab(struct shash *symtab)
MLF_FORCE_SNAT_FOR_LB_BIT);
expr_symtab_add_subfield(symtab, "flags.force_snat_for_lb", NULL,
flags_str);
+ snprintf(flags_str, sizeof flags_str, "flags[%d]",
+ MLF_SKIP_SNAT_FOR_LB_BIT);
+ expr_symtab_add_subfield(symtab, "flags.skip_snat_for_lb", NULL,
+ flags_str);
/* Connection tracking state. */
expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL, false,
diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 36cedd2dc..5eb06c77f 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -355,8 +355,14 @@ function lb_force_snat_router_ip(lr_options: Map<string,
string>): bool {
lr_options.contains_key("chassis")
}
-function force_snat_for_lb(lr: nb::Logical_Router): bool {
- not get_force_snat_ip(lr, "lb").is_empty() or
lb_force_snat_router_ip(lr.options)
+function snat_for_lb(lr: nb::Logical_Router, lb: Ref<nb::Load_Balancer>):
bit<8> {
+ if (lb.options.get_bool_def("lb_skip_snat", false)) {
+ return 2
+ };
+ if (not get_force_snat_ip(lr, "lb").is_empty() or
lb_force_snat_router_ip(lr.options)) {
+ return 1
+ };
+ return 0
0, 1, and 2 are not very good names for the states represented. I'm not
that well-versed in DDLog, but I think you can do something like:
typedef LBForceSNAT = NoForceSNAT
| ForceSNAT
| SkipSNAT
function snat_for_lb(lr: nb::Logical_Router, lb:
Ref<nb::Load_Balancer>): LBForceSNAT {
if (lb.options.get_bool_def("lb_skip_snat", false)) {
return SkipSNAT
};
if (not get_force_snat_ip(lr, "lb").is_empty() or
lb_force_snat_router_ip(lr.options)) {
return ForceSNAT
};
return NoForceSNAT
Then the caller of snat_for_lb() can check for those specific constants
to be returned instead of 0, 1, and 2. Ben or Leonid can correct my
syntax if I've messed it up :)
}
/* For each router, collect the set of IPv4 and IPv6 addresses used for SNAT,
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index a62f5c057..96ab11892 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2754,7 +2754,11 @@ icmp6 {
(and optional port numbers) to load balance to. If the router is
configured to force SNAT any load-balanced packets, the above action
will be replaced by <code>flags.force_snat_for_lb = 1;
- ct_lb(<var>args</var>);</code>. If health check is enabled, then
+ ct_lb(<var>args</var>);</code>.
+ If the load balancig rule is configured with <code>lb_skip_snat</code>
s/balancig/balancing/
This mistake seems to be repeated throughout the file.
+ set to true, the above action will be replaced by
+ <code>flags.skip_snat_for_lb = 1; ct_lb(<var>args</var>);</code>.
+ If health check is enabled, then
<var>args</var> will only contain those endpoints whose service
monitor status entry in <code>OVN_Southbound</code> db is
either <code>online</code> or empty.
@@ -2771,6 +2775,9 @@ icmp6 {
with an action of <code>ct_dnat;</code>. If the router is
configured to force SNAT any load-balanced packets, the above action
will be replaced by <code>flags.force_snat_for_lb = 1;
ct_dnat;</code>.
+ If the load balancig rule is configured with <code>lb_skip_snat</code>
+ set to true, the above action will be replaced by
+ <code>flags.skip_snat_for_lb = 1; ct_dnat;</code>.
</li>
<li>
@@ -2785,6 +2792,9 @@ icmp6 {
to force SNAT any load-balanced packets, the above action will be
replaced by <code>flags.force_snat_for_lb = 1;
ct_lb(<var>args</var>);</code>.
+ If the load balancig rule is configured with <code>lb_skip_snat</code>
+ set to true, the above action will be replaced by
+ <code>flags.skip_snat_for_lb = 1; ct_lb(<var>args</var>);</code>.
</li>
<li>
@@ -2797,6 +2807,9 @@ icmp6 {
If the router is configured to force SNAT any load-balanced
packets, the above action will be replaced by
<code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
+ If the load balancig rule is configured with <code>lb_skip_snat</code>
+ set to true, the above action will be replaced by
+ <code>flags.skip_snat_for_lb = 1; ct_dnat;</code>.
</li>
<li>
@@ -3829,6 +3842,16 @@ nd_ns {
</p>
</li>
+ <li>
+ <p>
+ If the Gateway router in the OVN Northbound database has been
+ configured to skip SNAT a packet (that has been previously
This is a bit misleading. The gateway router is not what gets configured
to skip SNAT; it's the load balancer.
+ load-balanced), a priority-120 flow matches
+ <code>flags.skip_snat_for_lb == 1 && ip</code> with an
+ action <code>next;</code>.
+ </p>
+ </li>
+
<li>
<p>
If the Gateway router in the OVN Northbound database has been
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 9839b8c4f..cc07e3a21 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -8576,7 +8576,7 @@ get_force_snat_ip(struct ovn_datapath *od, const char
*key_type,
static void
add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
struct ds *match, struct ds *actions, int priority,
- bool force_snat_for_lb, struct ovn_lb_vip *lb_vip,
+ int snat_type, struct ovn_lb_vip *lb_vip,
const char *proto, struct nbrec_load_balancer *lb,
struct shash *meter_groups, struct sset *nat_entries)
{
@@ -8585,8 +8585,9 @@ add_router_lb_flow(struct hmap *lflows, struct
ovn_datapath *od,
/* A match and actions for new connections. */
char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
- if (force_snat_for_lb) {
- char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s",
+ if (snat_type) {
+ char *new_actions = xasprintf("flags.%s_snat_for_lb = 1; %s",
+ snat_type == 2 ? "skip" : "force",
ds_cstr(actions));
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, priority,
new_match, new_actions, &lb->header_);
@@ -8598,11 +8599,12 @@ add_router_lb_flow(struct hmap *lflows, struct
ovn_datapath *od,
/* A match and actions for established connections. */
char *est_match = xasprintf("ct.est && %s", ds_cstr(match));
- if (force_snat_for_lb) {
+ if (snat_type) {
+ char *est_actions = xasprintf("flags.%s_snat_for_lb = 1; ct_dnat;",
+ snat_type == 2 ? "skip" : "force");
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, priority,
- est_match,
- "flags.force_snat_for_lb = 1; ct_dnat;",
- &lb->header_);
+ est_match, est_actions, &lb->header_);
+ free(est_actions);
} else {
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, priority,
est_match, "ct_dnat;", &lb->header_);
@@ -8675,11 +8677,13 @@ add_router_lb_flow(struct hmap *lflows, struct
ovn_datapath *od,
ds_put_format(&undnat_match, ") && outport == %s && "
"is_chassis_resident(%s)", od->l3dgw_port->json_key,
od->l3redirect_port->json_key);
- if (force_snat_for_lb) {
+ if (snat_type) {
+ char *action = xasprintf("flags.%s_snat_for_lb = 1; ct_dnat;",
+ snat_type == 2 ? "skip" : "force");
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
- ds_cstr(&undnat_match),
- "flags.force_snat_for_lb = 1; ct_dnat;",
+ ds_cstr(&undnat_match), action,
&lb->header_);
+ free(action);
} else {
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
ds_cstr(&undnat_match), "ct_dnat;",
@@ -8706,6 +8710,13 @@ build_lrouter_lb_flows(struct hmap *lflows, struct
ovn_datapath *od,
ovn_northd_lb_find(lbs, &nb_lb->header_.uuid);
ovs_assert(lb);
+ bool lb_skip_snat = smap_get_bool(&nb_lb->options, "lb_skip_snat",
+ false);
+ if (lb_skip_snat) {
+ ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120,
+ "flags.skip_snat_for_lb == 1 && ip", "next;");
+ }
+
If at all possible, could this flow be added in the function
build_lrouter_force_snat_flows()? That way, it's added in the same
function as the force_snat_for_lb S_ROUTER_OUT_SNAT flow, so it's easier
to see how the results contrast based on the options in use.
for (size_t j = 0; j < lb->n_vips; j++) {
struct ovn_lb_vip *lb_vip = &lb->vips[j];
struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[j];
@@ -8767,11 +8778,16 @@ build_lrouter_lb_flows(struct hmap *lflows, struct
ovn_datapath *od,
ds_put_format(match, " && is_chassis_resident(%s)",
od->l3redirect_port->json_key);
}
- bool force_snat_for_lb =
- lb_force_snat_ip || od->lb_force_snat_router_ip;
+
+ int snat_type = 0;
+ if (lb_skip_snat) {
+ snat_type = 2;
+ } else if (lb_force_snat_ip || od->lb_force_snat_router_ip) {
+ snat_type = 1;
+ }
Like I mentioned with DDLog, using 0, 1, and 2 does not intuitively say
anything about the SNAT settting. Please use an enum.
(Also, this section is what I was referring to at the top of the review
where I think the code can be simplified. Essentially, you can do:
bool force_snat_for_lb = !lb_skip_snat && (lb_force_snat_ip ||
od->lb_force_snat_router_ip);
Then you can keep the original logic for force_snat_for_lb that was
already there.)
add_router_lb_flow(lflows, od, match, actions, prio,
- force_snat_for_lb, lb_vip, proto,
- nb_lb, meter_groups, nat_entries);
+ snat_type, lb_vip, proto, nb_lb,
+ meter_groups, nat_entries);
}
}
sset_destroy(&all_ips);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 0063021e1..8dc07c02f 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -5367,6 +5367,16 @@ for (&Router(.lr = lr)) {
.external_ids = map_empty())
}
+Flow(.logical_datapath = lr,
+ .stage = s_ROUTER_OUT_SNAT(),
+ .priority = 120,
+ .__match = "flags.skip_snat_for_lb == 1 && ip",
+ .actions = "next;",
+ .external_ids = stage_hint(lb._uuid)) :-
+ LogicalRouterLB(lr, lb),
+ lb.options.get_bool_def("lb_skip_snat", false)
+ .
+
function lrouter_nat_is_stateless(nat: NAT): bool = {
Some{"true"} == nat.nat.options.get("stateless")
}
@@ -6010,14 +6020,15 @@ for (RouterLBVIP(
(Some{gwport}, true) -> " &&
is_chassis_resident(${redirect_port_name})",
_ -> ""
} in
- var force_snat_for_lb = force_snat_for_lb(lr) in
+ var snat_for_lb = snat_for_lb(lr, lb) in
{
/* A match and actions for established connections. */
var est_match = "ct.est && " ++ __match in
var actions =
- match (force_snat_for_lb) {
- true -> "flags.force_snat_for_lb = 1; ct_dnat;",
- false -> "ct_dnat;"
+ match (snat_for_lb) {
+ 2 -> "flags.skip_snat_for_lb = 1; ct_dnat;",
+ 1 -> "flags.force_snat_for_lb = 1; ct_dnat;",
+ _ -> "ct_dnat;"
} in
Flow(.logical_datapath = lr._uuid,
.stage = s_ROUTER_IN_DNAT(),
@@ -6076,9 +6087,10 @@ for (RouterLBVIP(
") && outport == ${json_string_escape(gwport.name)} && "
"is_chassis_resident(${redirect_port_name})" in
var action =
- match (force_snat_for_lb) {
- true -> "flags.force_snat_for_lb = 1; ct_dnat;",
- false -> "ct_dnat;"
+ match (snat_for_lb) {
+ 2 -> "flags.skip_snat_for_lb = 1; ct_dnat;",
+ 1 -> "flags.force_snat_for_lb = 1; ct_dnat;",
+ _ -> "ct_dnat;"
} in
Flow(.logical_datapath = lr._uuid,
.stage = s_ROUTER_OUT_UNDNAT(),
@@ -6113,7 +6125,11 @@ Flow(.logical_datapath = r.lr._uuid,
_ -> ""
},
var priority = if (lbvip.vip_port != 0) 120 else 110,
- var force_snat = if (force_snat_for_lb(r.lr)) "flags.force_snat_for_lb = 1; " else
"",
+ var force_snat = match (snat_for_lb(r.lr, lb)) {
+ 2 -> "flags.skip_snat_for_lb = 1; ",
+ 1 -> "flags.force_snat_for_lb = 1; ",
+ _ -> ""
+ },
var actions = build_lb_vip_actions(lbvip, s_ROUTER_OUT_SNAT(),
force_snat).
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 96476497d..c63b514a9 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2847,6 +2847,29 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
table=1 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;)
])
+check ovn-nbctl --wait=sb lb-add lb2 10.0.0.20:80 10.0.0.40:8080
+check ovn-nbctl --wait=sb set load_balancer lb2 options:lb_skip_snat=true
+check ovn-nbctl lr-lb-add lr0 lb2
+check ovn-nbctl --wait=sb lb-del lb1
+ovn-sbctl dump-flows lr0 > lr0flows
+
+AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
+ table=5 (lr_in_unsnat ), priority=0 , match=(1), action=(next;)
+ table=5 (lr_in_unsnat ), priority=110 , match=(inport == "lr0-public"
&& ip4.dst == 172.168.0.100), action=(ct_snat;)
+ table=5 (lr_in_unsnat ), priority=110 , match=(inport == "lr0-sw0" &&
ip4.dst == 10.0.0.1), action=(ct_snat;)
+ table=5 (lr_in_unsnat ), priority=110 , match=(inport == "lr0-sw1" &&
ip4.dst == 20.0.0.1), action=(ct_snat;)
+ table=5 (lr_in_unsnat ), priority=110 , match=(inport == "lr0-sw1" &&
ip6.dst == bef0::1), action=(ct_snat;)
+])
+
+AT_CHECK([grep "lr_in_dnat" lr0flows | grep skip_snat_for_lb | sort], [0], [dnl
+ table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip && ip4.dst == 10.0.0.20
&& tcp && tcp.dst == 80), action=(flags.skip_snat_for_lb = 1; ct_dnat;)
+ table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip && ip4.dst == 10.0.0.20
&& tcp && tcp.dst == 80), action=(flags.skip_snat_for_lb = 1; ct_lb(backends=10.0.0.40:8080);)
+])
+
+AT_CHECK([grep "lr_out_snat" lr0flows | grep skip_snat_for_lb | sort], [0],
[dnl
+ table=1 (lr_out_snat ), priority=120 , match=(flags.skip_snat_for_lb == 1
&& ip), action=(next;)
+])
+
AT_CLEANUP
])
@@ -2950,4 +2973,4 @@ wait_row_count FDB 0
ovn-sbctl list FDB
AT_CLEANUP
-])
\ No newline at end of file
+])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
