[ovs-dev] [PATCH v5 2/2 ovn] External IP based NAT: NORTHD changes to use allowed/exempted external ip

Numan Siddique numans at ovn.org
Tue Sep 1 08:44:03 UTC 2020


On Thu, Aug 20, 2020 at 8:04 AM Ankur Sharma <svc.mail.git at nutanix.com>
wrote:

> From: Ankur Sharma <ankur.sharma at nutanix.com>
>
> This patch has northd changes which consumes allowed/exempted external ip
> configuration per NAT rule in logical flow.
>
> Allowed external ip range adds an additional match criteria in
> snat/dnat logical flow rules.
>
> For example, if an allowed_external_ip address set ("abcd")
> is configured for following NAT rule.
> TYPE             EXTERNAL_IP        LOGICAL_IP
> snat             10.15.24.135       50.0.0.10
>
> Then logical flow will look like following:
> ..(lr_out_snat)...match=(ip && .... && ip4.dst == $abcd),
> action=(ct_snat(...);)
>
> Exempted external ip range adds an additional flow at priority+1
> to bypass the NAT pipeline if external ip is in extempted external
> ip address set.
> For example, if the same NAT rule mentioned aboe has an
> exempted_external_ip address set ("efgh"), then
> logical flow will look like following:
>
> ..(lr_out_snat), priority=162...match=(ip && .... && ip4.dst == $efgh),
> action=(next;)
> ..(lr_out_snat), priority=161...match=(ip && ....),
> action=(ct_snat(10.15.24.135);)
>
> Signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>
>

Hi Ankur,

You have missed adding documentation for the updated/new logical flows in
ovn-northd.8.xml.

There are few checkpatch warnings.

****
 == Checking 3a813c606ac4 ("External IP based NAT: NORTHD changes to use
allowed/exempted external ip") ==
WARNING: Line is 95 characters long (recommended limit is 79)
#85 FILE: northd/ovn-northd.c:8325:
         * lr_out_snat...priority=162  , match=(....ip4.dst ==
$exempted_range), action=(next;)

WARNING: Line is 88 characters long (recommended limit is 79)
#86 FILE: northd/ovn-northd.c:8326:
         * lr_out_snat...priority=161  , match=(......),
action=(ct_snat(10.15.24.139);)

WARNING: Empty return followed by brace, consider omitting
#111 FILE: northd/ovn-northd.c:8351:
}

Lines checked: 307, Warnings: 3, Errors: 0
*****

---
>  northd/ovn-northd.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ovn-northd.at | 111
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 212 insertions(+)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 212de2f..9f703d7 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -8280,6 +8280,75 @@ lrouter_nat_is_stateless(const struct nbrec_nat
> *nat)
>      return false;
>  }
>
> +/* Handles the match criteria and actions in logical flow
> + * based on external ip based NAT rule filter.
> + *
> + * For ALLOWED_EXT_IPs, we will add an additional match criteria
> + * of comparing ip*.src/dst with the allowed external ip address set.
> + *
> + * For EXEMPTED_EXT_IPs, we will have an additional logical flow
> + * where we compare ip*.src/dst with the exempted external ip address set
> + * and action says "next" instead of ct*.
> + */
> +static inline void
> +lrouter_nat_add_ext_ip_match(struct ovn_datapath *od,
> +                             struct hmap *lflows, struct ds *match,
> +                             const struct nbrec_nat *nat,
> +                             bool is_v6, bool is_src, ovs_be32 mask)
> +{
> +    struct nbrec_address_set *allowed_ext_ips = nat->allowed_ext_ips;
> +    struct nbrec_address_set *exempted_ext_ips = nat->exempted_ext_ips;
> +
> +    ovs_assert(allowed_ext_ips || exempted_ext_ips);
> +
> +    if (allowed_ext_ips) {
> +        ds_put_format(match, " && ip%s.%s == $%s",
> +                      is_v6 ? "6" : "4",
> +                      is_src ? "src" : "dst",
> +                      allowed_ext_ips->name);
> +    } else if (exempted_ext_ips) {
> +        struct ds match_exempt = DS_EMPTY_INITIALIZER;
> +        enum ovn_stage stage = is_src ? S_ROUTER_IN_DNAT :
> S_ROUTER_OUT_SNAT;
> +        uint16_t priority = count_1bits(ntohl(mask)) + 2;
> +        priority += 128;
>

The above 2 lines need to be removed as you are overriding the value of
'priority' below
in the if(is_src)/else.


> +
> +        /* Priority of logical flows corresponding to exempted_ext_ips is
> +         * +1 of the corresponding regulr NAT rule.
> +         * For example, if we have following NAT rule and we associate
> +         * exempted external ips to it:
> +         * "ovn-nbctl lr-nat-add router dnat_and_snat 10.15.24.139
> 50.0.0.11"
> +         *
> +         * And now we associate exempted external ip address set to it.
> +         * Now corresponding to above rule we will have following logical
> +         * flows:
> +         * lr_out_snat...priority=162  , match=(....ip4.dst ==
> $exempted_range), action=(next;)
> +         * lr_out_snat...priority=161  , match=(......),
> action=(ct_snat(10.15.24.139);)
> +         *
> +         */
> +        if (is_src) {
> +            /* S_ROUTER_IN_DNAT uses priority 100 */
> +            priority = 100 + 1;
> +        } else {
> +            /* S_ROUTER_OUT_SNAT uses priority (mask + 1 + 128) */
> +            priority = count_1bits(ntohl(mask)) + 2;
> +            priority += 128;
> +        }
> +
> +        ds_clone(&match_exempt, match);
> +        ds_put_format(&match_exempt, " && ip%s.%s == $%s",
> +                      is_v6 ? "6" : "4",
> +                      is_src ? "src" : "dst",
> +                      exempted_ext_ips->name);
> +
> +        ovn_lflow_add_with_hint(lflows, od, stage, priority,
> +                                ds_cstr(&match_exempt), "next;",
> +                                &nat->header_);
> +        ds_destroy(&match_exempt);
> +    }
> +
> +    return;
>

You can remove this 'return'.


> +}
> +
>  /* Builds the logical flow that replies to ARP requests for an
> 'ip_address'
>   * owned by the router. The flow is inserted in table S_ROUTER_IN_IP_INPUT
>   * with the given priority.
> @@ -9341,6 +9410,18 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>              struct in6_addr ipv6, mask_v6, v6_exact = IN6ADDR_EXACT_INIT;
>              bool is_v6 = false;
>              bool stateless = lrouter_nat_is_stateless(nat);
> +            struct nbrec_address_set *allowed_ext_ips =
> +                                      nat->allowed_ext_ips;
> +            struct nbrec_address_set *exempted_ext_ips =
> +                                      nat->exempted_ext_ips;
> +
> +            if (allowed_ext_ips && exempted_ext_ips) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> +                VLOG_WARN_RL(&rl, "NAT rule: "UUID_FMT" not applied,
> since"
> +                             "both allowed and exempt external ips set",
> +                             UUID_ARGS(&(nat->header_.uuid)));
> +                continue;
> +            }
>
>              char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
>              if (error || mask != OVS_BE32_MAX) {
> @@ -9486,6 +9567,11 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                    is_v6 ? "6" : "4",
>                                    nat->external_ip);
>                      ds_clear(&actions);
> +                    if (allowed_ext_ips || exempted_ext_ips) {
> +                        lrouter_nat_add_ext_ip_match(od, lflows, &match,
> nat,
> +                                                     is_v6, true, mask);
> +                    }
> +
>                      if (dnat_force_snat_ip) {
>                          /* Indicate to the future tables that a DNAT has
> taken
>                           * place and a force SNAT needs to be done in the
> @@ -9529,6 +9615,10 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                        od->l3redirect_port->json_key);
>                      }
>                      ds_clear(&actions);
> +                    if (allowed_ext_ips || exempted_ext_ips) {
> +                        lrouter_nat_add_ext_ip_match(od, lflows, &match,
> nat,
> +                                                     is_v6, true, mask);
> +                    }
>
>                      if (!strcmp(nat->type, "dnat_and_snat") && stateless)
> {
>                          ds_put_format(&actions, "ip%s.dst=%s; next;",
> @@ -9640,6 +9730,11 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                    nat->logical_ip);
>                      ds_clear(&actions);
>
> +                    if (allowed_ext_ips || exempted_ext_ips) {
> +                        lrouter_nat_add_ext_ip_match(od, lflows, &match,
> nat,
> +                                                     is_v6, false, mask);
> +                    }
> +
>                      if (!strcmp(nat->type, "dnat_and_snat") && stateless)
> {
>                          ds_put_format(&actions, "ip%s.src=%s; next;",
>                                        is_v6 ? "6" : "4",
> nat->external_ip);
> @@ -9679,6 +9774,12 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                        od->l3redirect_port->json_key);
>                      }
>                      ds_clear(&actions);
> +
> +                    if (allowed_ext_ips || exempted_ext_ips) {
> +                        lrouter_nat_add_ext_ip_match(od, lflows, &match,
> nat,
> +                                                     is_v6, false, mask);
> +                    }
> +
>                      if (distributed) {
>                          ds_put_format(&actions, "eth.src =
> "ETH_ADDR_FMT"; ",
>                                        ETH_ADDR_ARGS(mac));
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 8344c7f..e2fdba3 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1140,6 +1140,117 @@ ovn-nbctl lr-nat-del R1 dnat_and_snat  172.16.1.1
>
>  AT_CLEANUP
>
> +AT_SETUP([ovn -- check allowed/disallowed external dnat, snat and
> dnat_and_snat rules])
> +ovn_start
> +
>

Can you please extend this test case for gateway logical router too ? As
the patch adds lflows
for both gateway router and router with gw port.

Thanks
Numan

+ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> +
> +ovn-nbctl lr-add R1
> +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> +
> +ovn-nbctl ls-add S1
> +ovn-nbctl lsp-add S1 S1-R1
> +ovn-nbctl lsp-set-type S1-R1 router
> +ovn-nbctl lsp-set-addresses S1-R1 router
> +ovn-nbctl --wait=sb lsp-set-options S1-R1 router-port=R1-S1
> +
> +ovn-nbctl lrp-set-gateway-chassis R1-S1 gw1
> +
> +uuid=`ovn-sbctl --columns=_uuid --bare find Port_Binding
> logical_port=cr-R1-S1`
> +echo "CR-LRP UUID is: " $uuid
> +
> +ovn-nbctl create Address_Set name=allowed_range addresses=\"1.1.1.1\"
> +ovn-nbctl create Address_Set name=disallowed_range addresses=\"2.2.2.2\"
> +
> +# SNAT with ALLOWED_IPs
> +ovn-nbctl lr-nat-add R1 snat  172.16.1.1 50.0.0.11
> +ovn-nbctl lr-nat-update-ext-ip R1 snat 50.0.0.11 allowed_range
> +
> +#ovn-sbctl dump-flows R1
> +OVS_WAIT_UNTIL([test 3 = `ovn-sbctl dump-flows R1 | grep lr_out_snat | wc
> -l`])
> +
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "ip4.dst == $allowed_range" | wc -l], [0], [1
> +])
> +
> +# SNAT with DISALLOWED_IPs
> +ovn-nbctl lr-nat-del R1 snat  50.0.0.11
> +ovn-nbctl lr-nat-add R1 snat  172.16.1.1 50.0.0.11
> +ovn-nbctl --is-exempted lr-nat-update-ext-ip R1 snat 50.0.0.11
> disallowed_range
> +
> +ovn-sbctl dump-flows R1
> +OVS_WAIT_UNTIL([test 4 = `ovn-sbctl dump-flows R1 | grep lr_out_snat | \
> +wc -l`])
> +
> +ovn-nbctl show R1
> +ovn-sbctl dump-flows R1
> +
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "ip4.dst == $disallowed_range" | grep "priority=162" | wc
> -l], [0], [1
> +])
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "priority=161" | wc -l], [0], [1
> +])
> +
> +# Stateful FIP with ALLOWED_IPs
> +ovn-nbctl lr-nat-del R1 snat  50.0.0.11
> +ovn-nbctl lr-nat-add R1 dnat_and_snat  172.16.1.2 50.0.0.11
> +ovn-nbctl lr-nat-update-ext-ip R1 dnat_and_snat 172.16.1.2 allowed_range
> +ovn-nbctl show R1
> +ovn-sbctl dump-flows R1
> +
> +OVS_WAIT_UNTIL([test 3 = `ovn-sbctl dump-flows R1 | grep lr_out_snat | \
> +wc -l`])
> +
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "ip4.dst == $allowed_range" | wc -l], [0], [1
> +])
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_dnat | grep "ip4.dst ==
> 172.16.1.2" | grep "ip4.src == $allowed_range" | wc -l], [0], [1
> +])
> +
> +# Stateful FIP with DISALLOWED_IPs
> +ovn-nbctl lr-nat-del R1 dnat_and_snat  172.16.1.2
> +ovn-nbctl lr-nat-add R1 dnat_and_snat  172.16.1.2 50.0.0.11
> +ovn-nbctl --is-exempted lr-nat-update-ext-ip R1 dnat_and_snat 172.16.1.2
> disallowed_range
> +ovn-nbctl show R1
> +ovn-sbctl dump-flows R1
> +
> +OVS_WAIT_UNTIL([test 4 = `ovn-sbctl dump-flows R1 | grep lr_out_snat | \
> +wc -l`])
> +
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "ip4.dst == $disallowed_range" | grep "priority=162" | wc
> -l], [0], [1
> +])
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_dnat | grep "ip4.dst ==
> 172.16.1.2" | grep "ip4.src == $disallowed_range" | grep "priority=101" |
> wc -l], [0], [1
> +])
> +
> +# Stateless FIP with DISALLOWED_IPs
> +ovn-nbctl lr-nat-del R1 dnat_and_snat  172.16.1.2
> +ovn-nbctl --stateless lr-nat-add R1 dnat_and_snat  172.16.1.2 50.0.0.11
> +ovn-nbctl lr-nat-update-ext-ip R1 dnat_and_snat 172.16.1.2 allowed_range
> +ovn-nbctl show R1
> +ovn-sbctl dump-flows R1
> +
> +OVS_WAIT_UNTIL([test 3 = `ovn-sbctl dump-flows R1 | grep lr_out_snat | \
> +wc -l`])
> +
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "ip4.dst == $allowed_range" | wc -l], [0], [1
> +])
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_dnat | grep "ip4.dst ==
> 172.16.1.2" | grep "ip4.src == $allowed_range" | wc -l], [0], [1
> +])
> +
> +# Stateful FIP with DISALLOWED_IPs
> +ovn-nbctl lr-nat-del R1 dnat_and_snat  172.16.1.2
> +ovn-nbctl --stateless lr-nat-add R1 dnat_and_snat  172.16.1.2 50.0.0.11
> +ovn-nbctl --is-exempted lr-nat-update-ext-ip R1 dnat_and_snat 172.16.1.2
> disallowed_range
> +ovn-nbctl show R1
> +ovn-sbctl dump-flows R1
> +
> +OVS_WAIT_UNTIL([test 4 = `ovn-sbctl dump-flows R1 | grep lr_out_snat | \
> +wc -l`])
> +
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src ==
> 50.0.0.11" | grep "ip4.dst == $disallowed_range" | grep "priority=162" | wc
> -l], [0], [1
> +])
> +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_dnat | grep "ip4.dst ==
> 172.16.1.2" | grep "ip4.src == $disallowed_range" | grep "priority=101" |
> wc -l], [0], [1
> +])
> +
> +AT_CLEANUP
> +
>  AT_SETUP([ovn -- check Load balancer health check and Service Monitor
> sync])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list