[ovs-dev] [PATCH ovn] northd: introduce per-lb lb_skip_snat option
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Tue Apr 6 18:55:51 UTC 2021
> Hi Lorenzo,
>
> In general, this patch seems overly complicated based on the linked bug
> report.
Hi Mark,
thx for the review.
>
> 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.
please see below
>
> 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.
ack, I will fix them in v2
>
> 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/
ack, I will fix it in v2
>
> > 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
> >
[...]
> > + 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 :)
ack, I will fix it in v2
>
> > }
> > /* 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.
ack, I will fix it in v2
>
> > + 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.
ack, I will fix it in v2
>
> > + 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.
we need to loop over load_balancer to add the flow above while
build_lrouter_force_snat_flows is run just per-datapath in
build_lrouter_nat_defrag_and_lb so I guess it is clearer to add it in
build_lrouter_lb_flows
>
> > 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.
ack, I will fix it in v2
>
> (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.)
as we discussed on irc, doing so we will not be able to decide if the traffic
has hit the load balancer in ingress pipeline and so will not be able to skip
the snat in the router egress pipeline.
Regards,
Lorenzo
>
> > 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
> > +])
> >
>
More information about the dev
mailing list