On Sat, Oct 5, 2019 at 1:45 AM Ankur Sharma <[email protected]> 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 <[email protected]>
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 &&
> 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 &&
> ip4.dst == <var>B</var> && 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 &&
> ip4.dst == <var>B</var> && 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 && ip4.src == <var>B</var>
> && 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 && 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
