There is a DNAT-SNAT(other IP) test case in system-ovn.at, but easy
SNAT(port IP) is not included.
I added a new test, failed currently and passed with the modification.
Will update my patch V2.
On 2016/7/26 10:57, Ryan Moats wrote:
"dev" <dev-boun...@openvswitch.org> wrote on 07/25/2016 08:49:38 PM:
> From: Dongjun <do...@dtdream.com>
> To: "dev@openvswitch.org" <dev@openvswitch.org>
> Date: 07/25/2016 08:45 PM
> Subject: [ovs-dev] [PATACH]ovn: fix bug of dropping flows building
> in build_lrouter_flows
> Sent by: "dev" <dev-boun...@openvswitch.org>
>
> In build_lrouter_flows, it says that:" Drop ip traffic to this router,
> unless the router ip is used as SNAT ip. "
> But there is a bug, the "continue" only effect the inner loop, dropping
> flows is still built.
>
> Example: Dropping flow c8726aed-0dd0-41b6-bb8c-13ca1e2164c9 should not
> be exist for "ct_snat(192.168.246.200);".
> _uuid actions external_ids logical_datapath match
> pipeline priority table_id
> a29d2785-af1d-45ec-ae60-677356c48f24 "drop;"
> {stage-name=lr_in_ip_input} 1915535e-7738-43db-8341-2306221b0691
> "ip4.dst == {169.254.128.1}" ingress 60 1
> c8726aed-0dd0-41b6-bb8c-13ca1e2164c9 "drop;"
> {stage-name=lr_in_ip_input} 1915535e-7738-43db-8341-2306221b0691
> "ip4.dst == {192.168.246.200}" ingress 60 1
> 0dd66b7e-d11b-4d13-86d1-55ebe9d7e85f "ct_snat(192.168.246.200);"
> {stage-name=lr_out_snat} 1915535e-7738-43db-8341-2306221b0691 "ip &&
> ip4.src == 101.0.0.0/24" egress 25 0
> c3d56ca6-38c6-4943-8d89-2c39cbc3cd9b "next;"
{stage-name=lr_out_snat}
> 1915535e-7738-43db-8341-2306221b0691 "1" egress 0 0
>
> This commit fixed this bug.
> NAT configured, the flows:
> a29d2785-af1d-45ec-ae60-677356c48f24 "drop;"
> {stage-name=lr_in_ip_input} 1915535e-7738-43db-8341-2306221b0691
> "ip4.dst == {169.254.128.1}" ingress 60 1
> 0dd66b7e-d11b-4d13-86d1-55ebe9d7e85f "ct_snat(192.168.246.200);"
> {stage-name=lr_out_snat} 1915535e-7738-43db-8341-2306221b0691 "ip &&
> ip4.src == 101.0.0.0/24" egress 25 0
> c3d56ca6-38c6-4943-8d89-2c39cbc3cd9b "next;"
{stage-name=lr_out_snat}
> 1915535e-7738-43db-8341-2306221b0691 "1" egress 0 0
> NAT not configured, the flows:
> a29d2785-af1d-45ec-ae60-677356c48f24 "drop;"
> {stage-name=lr_in_ip_input} 1915535e-7738-43db-8341-2306221b0691
> "ip4.dst == {169.254.128.1}" ingress 60 1
> c8726aed-0dd0-41b6-bb8c-13ca1e2164c9 "drop;"
> {stage-name=lr_in_ip_input} 1915535e-7738-43db-8341-2306221b0691
> "ip4.dst == {192.168.246.200}" ingress 60 1
> c3d56ca6-38c6-4943-8d89-2c39cbc3cd9b "next;"
{stage-name=lr_out_snat}
> 1915535e-7738-43db-8341-2306221b0691 "1" egress 0 0
>
>
> Signed-off-by: Dongjun <do...@dtdream.com>
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> old mode 100644
> new mode 100755
> index a3d1672..0e4fc93
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2324,6 +2324,16 @@ op_put_networks(struct ds *ds, const struct
> ovn_port *op, bool add_bcast)
> ds_put_cstr(ds, "}");
> }
>
> +static bool
> +has_ip(ovs_be32 *ips, size_t size, ovs_be32 ip){
> + for (int i = 0; i < size; i++) {
> + if (ip == ips[i]) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> static void
> build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> struct hmap *lflows)
> @@ -2543,10 +2553,8 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> ds_put_cstr(&match, "ip4.dst == {");
> bool has_drop_ips = false;
> for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> - for (int j = 0; j < n_nat_ips; j++) {
> - if (op->lrp_networks.ipv4_addrs[i].addr ==
nat_ips[j]) {
> - continue;
> - }
> + if(has_ip(nat_ips, n_nat_ips,
> op->lrp_networks.ipv4_addrs[i].addr)){
> + continue;
> }
> ds_put_format(&match, "%s, ",
> op->lrp_networks.ipv4_addrs[i].addr_s);
Since there is a bug claimed here, is there a unit test that currently
fails without this patch that then passes with it? If there isn't one
could you please add one to the next version of the patch?
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
