[ovs-dev] [PATCH v2 2/2 ovn] OVN: Use ipv4.src and ipv4.dst actions for NAT rules

Numan Siddique numans at ovn.org
Thu Oct 31 17:38:57 UTC 2019


On Thu, Oct 31, 2019 at 2:02 AM Ankur Sharma <ankur.sharma at nutanix.com> wrote:
>
> Hi Numan,
>
> Submitted a v3 yesterday, addressing all the review comments.
> Please take a look.

Hi Ankur,

Can you please rebase your v3 patches ? They don't apply on top of master.

Thanks
Numan

>
> Regards,
> Ankur
>
> -----Original Message-----
> From: Numan Siddique <numans at ovn.org>
> Sent: Monday, October 28, 2019 6:40 AM
> To: Ankur Sharma <ankur.sharma at nutanix.com>
> Cc: ovs-dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v2 2/2 ovn] OVN: Use ipv4.src and ipv4.dst actions for NAT rules
>
> On Sat, Oct 5, 2019 at 1:45 AM Ankur Sharma <ankur.sharma at nutanix.com> wrote:
> >
> > For dnat_and_snat rules which are meant to be stateless instead of
> > using ct_snat/dnat OVN actions, we will use ipv4.src/ipv4.dst.
> >
> > This actions will do 1:1 mapping to inner ip to external ip, while
> > recalculating the checksums.
> >
> > Signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>
>
>
> Hi Ankur,
>
> There is a checkpatch warning - WARNING: Line is 80 characters long (recommended limit is 79).
> Please address it.
>
> Please see below for few comments.
>
> Thanks
> Numan
>
> > ---
> >  northd/ovn-northd.8.xml | 34 +++++++++++++++----
> >  northd/ovn-northd.c     | 86 +++++++++++++++++++++++++++++++++++++++++++------
> >  tests/ovn-northd.at     | 50 ++++++++++++++++++++++++++++
> >  3 files changed, 154 insertions(+), 16 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index
> > 0f4f1c1..7d5d102 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -1718,7 +1718,9 @@ icmp6 {
> >            to change the source IP address of a packet from <var>A</var> to
> >            <var>B</var>, a priority-90 flow matches <code>ip &amp;&amp;
> >            ip4.dst == <var>B</var></code> with an action
> > -          <code>ct_snat; </code>.
> > +          <code>ct_snat; </code>. If the NAT rule is of type dnat_and_snat
> > +          and has <code>is_stateless=true</code> in the options, then the action
> > +          would be <code>replace_dst_ip(<var>B</var>)</code>.
>
> I think you need to update the documentation as you removed the action
> - replace_dst_ip in v2.
>
>
> >          </p>
> >
> >          <p>
> > @@ -1738,7 +1740,10 @@ icmp6 {
> >            <var>B</var>, a priority-100 flow matches <code>ip &amp;&amp;
> >            ip4.dst == <var>B</var> &amp;&amp; inport == <var>GW</var></code>,
> >            where <var>GW</var> is the logical router gateway port, with an
> > -          action <code>ct_snat;</code>.
> > +          action <code>ct_snat;</code>. If the NAT rule is of type
> > +          dnat_and_snat and has <code>is_stateless=true</code> in the
> > +          options, then the action would be <code>replace_dst_ip
>
> Same here and other places too.
>
> > +          (<var>B</var>)</code>.
> >          </p>
> >
> >          <p>
> > @@ -1858,7 +1863,10 @@ icmp6 {
> >          Gateway router is configured to force SNAT any DNATed packet,
> >          the above action will be replaced by
> >          <code>flags.force_snat_for_dnat = 1; flags.loopback = 1;
> > -        ct_dnat(<var>B</var>);</code>.
> > +        ct_dnat(<var>B</var>);</code>. If the NAT rule is of type
> > +        dnat_and_snat and has <code>is_stateless=true</code> in the
> > +        options, then the action would be <code>replace_dst_ip
> > +        (<var>B</var>)</code>.
> >        </li>
> >
> >        <li>
> > @@ -1890,7 +1898,10 @@ icmp6 {
> >            <var>B</var>, a priority-100 flow matches <code>ip &amp;&amp;
> >            ip4.dst == <var>B</var> &amp;&amp; inport == <var>GW</var></code>,
> >            where <var>GW</var> is the logical router gateway port, with an
> > -          action <code>ct_dnat(<var>B</var>);</code>.
> > +          action <code>ct_dnat(<var>B</var>);</code>. If the NAT rule is of
> > +          type dnat_and_snat and has <code>is_stateless=true</code> in the
> > +          options, then the action would be <code>replace_dst_ip
> > +          (<var>B</var>)</code>.
> >          </p>
> >
> >          <p>
> > @@ -2553,7 +2564,10 @@ nd_ns {
> >            matches <code>ip &amp;&amp; ip4.src == <var>B</var>
> >            &amp;&amp; outport == <var>GW</var></code>, where <var>GW</var>
> >            is the logical router gateway port, with an action
> > -          <code>ct_dnat;</code>.
> > +          <code>ct_dnat;</code>. If the NAT rule is of type
> > +          dnat_and_snat and has <code>is_stateless=true</code> in the
> > +          options, then the action would be <code>replace_src_ip
> > +          (<var>B</var>)</code>.
> >          </p>
> >
> >          <p>
> > @@ -2611,7 +2625,10 @@ nd_ns {
> >            <code>ip &amp;&amp; ip4.src == <var>A</var></code> with an action
> >            <code>ct_snat(<var>B</var>);</code>.  The priority of the flow
> >            is calculated based on the mask of <var>A</var>, with matches
> > -          having larger masks getting higher priorities.
> > +          having larger masks getting higher priorities. If the NAT rule is
> > +          of type dnat_and_snat and has <code>is_stateless=true</code> in the
> > +          options, then the action would be <code>replace_src_ip
> > +          (<var>B</var>)</code>.
> >          </p>
> >          <p>
> >            A priority-0 logical flow with match <code>1</code> has
> > actions @@ -2634,7 +2651,10 @@ nd_ns {
> >            logical router gateway port, with an action
> >            <code>ct_snat(<var>B</var>);</code>.  The priority of the flow
> >            is calculated based on the mask of <var>A</var>, with matches
> > -          having larger masks getting higher priorities.
> > +          having larger masks getting higher priorities. If the NAT rule
> > +          is of type dnat_and_snat and has <code>is_stateless=true</code>
> > +          in the options, then the action would be <code>replace_src_ip
> > +          (<var>B</var>)</code>.
> >          </p>
> >
> >          <p>
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index
> > f393ceb..4036392 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -6314,6 +6314,18 @@ copy_ra_to_sb(struct ovn_port *op, const char *address_mode)
> >      smap_destroy(&options);
> >  }
> >
> > +static inline bool
> > +lrouter_nat_is_stateless(const struct nbrec_nat *nat) {
> > +    const char *is_stateless = smap_get(&nat->options,
> > +"is_stateless");
> > +
> > +    if (is_stateless && !strcmp(is_stateless, "true")) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static void
> >  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                      struct hmap *lflows, struct shash *meter_groups)
> > @@ -7052,6 +7064,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >              nat = od->nbr->nat[i];
> >
> >              ovs_be32 ip, mask;
> > +            bool is_stateless = lrouter_nat_is_stateless(nat);
> >
> >              char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
> >              if (error || mask != OVS_BE32_MAX) { @@ -7117,15 +7130,26
> > @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                  if (!od->l3dgw_port) {
> >                      /* Gateway router. */
> >                      ds_clear(&match);
> > +                    ds_clear(&actions);
> >                      ds_put_format(&match, "ip && ip4.dst == %s",
> >                                    nat->external_ip);
> > +
> > +                    if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                        ds_put_format(&actions, "ip4.dst=%s; next;",
> > +                                      nat->logical_ip);
> > +                    } else {
> > +                        ds_put_cstr(&actions, "ct_snat;");
> > +                    }
> > +
> >                      ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 90,
> > -                                  ds_cstr(&match), "ct_snat;");
> > +                                  ds_cstr(&match),
> > + ds_cstr(&actions));
> >                  } else {
> >                      /* Distributed router. */
> >
> >                      /* Traffic received on l3dgw_port is subject to NAT. */
> >                      ds_clear(&match);
> > +                    ds_clear(&actions);
> > +
> >                      ds_put_format(&match, "ip && ip4.dst == %s"
> >                                            " && inport == %s",
> >                                    nat->external_ip, @@ -7136,8
> > +7160,16 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                          ds_put_format(&match, " && is_chassis_resident(%s)",
> >                                        od->l3redirect_port->json_key);
> >                      }
> > +
> > +                    if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                        ds_put_format(&actions, "ip4.dst=%s; next;",
> > +                                      nat->logical_ip);
> > +                    } else {
> > +                        ds_put_cstr(&actions, "ct_snat;");
> > +                    }
> > +
> >                      ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 100,
> > -                                  ds_cstr(&match), "ct_snat;");
> > +                                  ds_cstr(&match),
> > + ds_cstr(&actions));
> >
> >                      /* Traffic received on other router ports must be
> >                       * redirected to the central instance of the
> > l3dgw_port @@ -7172,8 +7204,16 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                          ds_put_format(&actions,
> >                                        "flags.force_snat_for_dnat = 1; ");
> >                      }
> > -                    ds_put_format(&actions, "flags.loopback = 1; ct_dnat(%s);",
> > -                                  nat->logical_ip);
> > +
> > +                    if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                        ds_put_format(&actions, "flags.loopback = 1; "
> > +                                      "ip4.dst=%s; next;",
> > +                                      nat->logical_ip);
> > +                    } else {
> > +                        ds_put_format(&actions, "flags.loopback = 1; ct_dnat(%s);",
> > +                                      nat->logical_ip);
> > +                    }
> > +
> >                      ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 100,
> >                                    ds_cstr(&match), ds_cstr(&actions));
> >                  } else {
> > @@ -7192,8 +7232,15 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                                        od->l3redirect_port->json_key);
> >                      }
> >                      ds_clear(&actions);
> > -                    ds_put_format(&actions, "ct_dnat(%s);",
> > -                                  nat->logical_ip);
> > +
> > +                    if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                        ds_put_format(&actions, "ip4.dst=%s; next;",
> > +                                      nat->logical_ip);
> > +                    } else {
> > +                        ds_put_format(&actions, "ct_dnat(%s);",
> > +                                      nat->logical_ip);
> > +                    }
> > +
> >                      ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 100,
> >                                    ds_cstr(&match),
> > ds_cstr(&actions));
> >
> > @@ -7235,7 +7282,14 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                      ds_put_format(&actions, "eth.src = "ETH_ADDR_FMT"; ",
> >                                    ETH_ADDR_ARGS(mac));
> >                  }
> > -                ds_put_format(&actions, "ct_dnat;");
> > +
> > +                if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                    ds_put_format(&actions, "ip4.src=%s; next;",
> > +                                  nat->external_ip);
> > +                } else {
> > +                    ds_put_format(&actions, "ct_dnat;");
> > +                }
> > +
> >                  ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 100,
> >                                ds_cstr(&match), ds_cstr(&actions));
> >              }
> > @@ -7251,7 +7305,14 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                      ds_put_format(&match, "ip && ip4.src == %s",
> >                                    nat->logical_ip);
> >                      ds_clear(&actions);
> > -                    ds_put_format(&actions, "ct_snat(%s);", nat->external_ip);
> > +
> > +                    if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                        ds_put_format(&actions, "ip4.src=%s; next;",
> > +                                      nat->external_ip);
> > +                    } else {
> > +                        ds_put_format(&actions, "ct_snat(%s);",
> > +                                      nat->external_ip);
> > +                    }
> >
> >                      /* The priority here is calculated such that the
> >                       * nat->logical_ip with the longest mask gets a
> > higher @@ -7280,7 +7341,14 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                          ds_put_format(&actions, "eth.src = "ETH_ADDR_FMT"; ",
> >                                        ETH_ADDR_ARGS(mac));
> >                      }
> > -                    ds_put_format(&actions, "ct_snat(%s);", nat->external_ip);
> > +
> > +                    if (!strcmp(nat->type, "dnat_and_snat") && is_stateless) {
> > +                        ds_put_format(&actions, "ip4.src=%s; next;",
> > +                                      nat->external_ip);
> > +                    } else {
> > +                        ds_put_format(&actions, "ct_snat(%s);",
> > +                                      nat->external_ip);
> > +                    }
> >
> >                      /* The priority here is calculated such that the
> >                       * nat->logical_ip with the longest mask gets a
> > higher diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index
> > 42033d5..64511a9 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -966,3 +966,53 @@ OVS_WAIT_UNTIL([ovn-sbctl get Port_Binding
> > ${uuid} options:redirect-type], [0],
> >  ])
> >
> >  AT_CLEANUP
> > +
>
> Can you add a new test case or modify an existing test case in ovn.at to make use of this feature ?
>
> > +AT_SETUP([ovn -- check stateless dnat_and_snat rule])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no]) ovn_start
> > +
> > +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 lr-nat-add R1 dnat_and_snat  172.16.1.1 50.0.0.11
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ct_snat | wc -l], [0], [2
> > +])
>
> There is a chance that the test case might fail on slower systems if ovn-northd is not fast enough to add the necessary logical flows.
>
> For the above first check, I would suggest to use - OVS_WAIT_UNTIL.
>
>
> > +
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ct_dnat | wc -l], [0], [2
> > +])
> > +
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ip4.dst=| wc -l], [0], [0
> > +])
> > +
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ip4.src=| wc -l], [0], [0
> > +])
> > +
> > +ovn-nbctl lr-nat-del R1 dnat_and_snat  172.16.1.1
> > +
> > +ovn-nbctl --stateless lr-nat-add R1 dnat_and_snat  172.16.1.1
> > +50.0.0.11 AT_CHECK([ovn-sbctl dump-flows R1 | grep ct_snat | wc -l],
> > +[0], [0
> > +])
> > +
>
> Same here. Please use OVS_WAIT_UNTIL.
>
>
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ct_dnat | wc -l], [0], [0
> > +])
> > +
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ip4.dst=| wc -l], [0], [2
> > +])
> > +
> > +AT_CHECK([ovn-sbctl dump-flows R1 | grep ip4.src=| wc -l], [0], [2
> > +])
> > +
> > +AT_CLEANUP
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.
> > org_mailman_listinfo_ovs-2Ddev&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=mZw
> > X9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=CouZqk7agKapXA85Hmee871NM2d
> > rC0etOpAwwwF_Qvw&s=yGzdgaPfGMkQpXg_3NmyQCOqkqeJPvomW3QqTsWghPQ&e=
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list