[ovs-dev] [PATCH v2 ovn] northd: introduce per-lb lb_skip_snat option

Mark Michelson mmichels at redhat.com
Fri Apr 9 15:36:00 UTC 2021


Thanks for the update Lorenzo. Just fix the one typo down below, and:

Acked-by: Mark Michelson <mmichels at redhat.com>

On 4/6/21 2:56 PM, Lorenzo Bianconi wrote:
> Introduce skip_snat in load balancer option column in order to not
> 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 <astoycos at redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
> Changes since v1:
> - rename lb_skip_snat in skip_snat
> - add skip_snat documentation in ovn-nb.xml
> - fix typos
> - introduce enum for return value of snat_for_lb/
> ---
>   include/ovn/logical-fields.h |  5 ++++
>   lib/logical-fields.c         |  4 +++
>   northd/lrouter.dl            | 14 ++++++++--
>   northd/ovn-northd.8.xml      | 24 ++++++++++++++++-
>   northd/ovn-northd.c          | 51 +++++++++++++++++++++++++-----------
>   northd/ovn_northd.dl         | 32 ++++++++++++++++------
>   ovn-nb.xml                   |  6 +++++
>   tests/ovn-northd.at          | 25 +++++++++++++++++-
>   8 files changed, 134 insertions(+), 27 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..312ceb9fc 100644
> --- a/northd/lrouter.dl
> +++ b/northd/lrouter.dl
> @@ -355,8 +355,18 @@ 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)
> +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("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
>   }
>   
>   /* 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..8197aa513 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 balancing rule is configured with <code>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>.
> +        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 balancing rule is configured with <code>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 balancing rule is configured with <code>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 balancing rule is configured with <code>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,15 @@ nd_ns {
>           </p>
>         </li>
>   
> +      <li>
> +        <p>
> +          If a load balancer configured to skip snat has been applied to
> +          the Gateway router pipeline, 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..8de106c84 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -8573,10 +8573,16 @@ get_force_snat_ip(struct ovn_datapath *od, const char *key_type,
>       return true;
>   }
>   
> +enum lb_snat_type {
> +    NO_FORCE_SNAT,
> +    FORCE_SNAT,
> +    SKIP_SNAT,
> +};
> +
>   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,
> +                   enum lb_snat_type 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,9 +8591,10 @@ 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",
> -                                      ds_cstr(actions));
> +    if (snat_type == FORCE_SNAT || snat_type == SKIP_SNAT) {
> +        char *new_actions = xasprintf("flags.%s_snat_for_lb = 1; %s",
> +                snat_type == SKIP_SNAT ? "skip" : "force",
> +                ds_cstr(actions));
>           ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, priority,
>                                   new_match, new_actions, &lb->header_);
>           free(new_actions);
> @@ -8598,11 +8605,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 == FORCE_SNAT || snat_type == SKIP_SNAT) {
> +        char *est_actions = xasprintf("flags.%s_snat_for_lb = 1; ct_dnat;",
> +                snat_type == SKIP_SNAT ? "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 +8683,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 == FORCE_SNAT || snat_type == SKIP_SNAT) {
> +        char *action = xasprintf("flags.%s_snat_for_lb = 1; ct_dnat;",
> +                                 snat_type == SKIP_SNAT ? "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 +8716,12 @@ 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, "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;");
> +        }
> +
>           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 +8783,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;
> +
> +            enum lb_snat_type snat_type = NO_FORCE_SNAT;
> +            if (lb_skip_snat) {
> +                snat_type = SKIP_SNAT;
> +            } else if (lb_force_snat_ip || od->lb_force_snat_router_ip) {
> +                snat_type = FORCE_SNAT;
> +            }
>               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..4c02e2d5a 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("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) {
> +                    SkipSNAT -> "flags.skip_snat_for_lb = 1; ct_dnat;",
> +                    ForceSNAT -> "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) {
> +                    SkipSNAT -> "flags.skip_snat_for_lb = 1; ct_dnat;",
> +                    ForceSNAT -> "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)) {
> +        SkipSNAT -> "flags.skip_snat_for_lb = 1; ",
> +        ForceSNAT -> "flags.force_snat_for_lb = 1; ",
> +        _ -> ""
> +    },
>       var actions = build_lb_vip_actions(lbvip, s_ROUTER_OUT_SNAT(), force_snat).
>   
>   
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index b0a4adffe..fe8ebf776 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1653,6 +1653,12 @@
>           exactly one IPv4 and/or one IPv6 address on it, separated by a space
>           character.
>         </column>
> +
> +      <column name="options" key="skip_snat">
> +        If the load balancig rule is configured with <code>skip_snat</code>

s/balancig/balancing/

:)

> +        option, the force_snat_for_lb option configured for the router
> +        pipeline will not be applied for this load balancer.
> +      </column>
>       </group>
>     </table>
>   
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 96476497d..037ea98c5 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: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