Skip to content

Commit

Permalink
northd: Restore flows that recirculate packets in the router DNAT zone.
Browse files Browse the repository at this point in the history 
Also improve the tests to make sure this doesn't break again in the
future.

Fixes: 2254260 ("northd: introduce build_lrouter_in_dnat_flow routine")
CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Reported-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
  • Loading branch information
@dceara @numansiddique
dceara authored and numansiddique committed Apr 1, 2021
1 parent 446cf54 commit 82b4c61
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 27 deletions.
26 changes: 12 additions & 14 deletions northd/ovn-northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11240,20 +11240,6 @@ build_lrouter_in_dnat_flow(struct hmap *lflows, struct ovn_datapath *od,
&nat->header_);
}
}

if (!od->l3dgw_port) {
/* For gateway router, re-circulate every packet through
* the DNAT zone. This helps with the following.
*
* Any packet that needs to be unDNATed in the reverse
* direction gets unDNATed. Ideally this could be done in
* the egress pipeline. But since the gateway router
* does not have any feature that depends on the source
* ip address being external IP address for IP routing,
* we can do it here, saving a future re-circulation. */
ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
"ip", "flags.loopback = 1; ct_dnat;");
}
}

static void
Expand Down Expand Up @@ -11716,6 +11702,18 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od,
od->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
}
}

/* For gateway router, re-circulate every packet through
* the DNAT zone. This helps with the following.
*
* Any packet that needs to be unDNATed in the reverse
* direction gets unDNATed. Ideally this could be done in
* the egress pipeline. But since the gateway router
* does not have any feature that depends on the source
* ip address being external IP address for IP routing,
* we can do it here, saving a future re-circulation. */
ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
"ip", "flags.loopback = 1; ct_dnat;");
}

/* Load balancing and packet defrag are only valid on
Expand Down
40 changes: 27 additions & 13 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2700,7 +2700,7 @@ wait_row_count nb:Logical_Switch_Port 1 up=false name=lsp1
AT_CLEANUP

OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn -- lb_force_snat_ip for Gateway Routers])
AT_SETUP([ovn -- Load Balancers and lb_force_snat_ip for Gateway Routers])
ovn_start

check ovn-nbctl ls-add sw0
Expand Down Expand Up @@ -2740,11 +2740,11 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
table=5 (lr_in_unsnat ), priority=0 , match=(1), action=(next;)
])

AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
])


AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(ct_dnat;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(ct_lb(backends=10.0.0.4:8080);)
table=6 (lr_in_dnat ), priority=50 , match=(ip), action=(flags.loopback = 1; ct_dnat;)
])

check ovn-nbctl --wait=sb set logical_router lr0 options:lb_force_snat_ip="20.0.0.4 aef0::4"
Expand All @@ -2759,14 +2759,18 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
table=5 (lr_in_unsnat ), priority=110 , match=(ip6 && ip6.dst == aef0::4), action=(ct_snat;)
])

AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_dnat;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.4:8080);)
table=6 (lr_in_dnat ), priority=50 , match=(ip), action=(flags.loopback = 1; ct_dnat;)
])

AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
table=1 (lr_out_snat ), priority=100 , match=(flags.force_snat_for_lb == 1 && ip4), action=(ct_snat(20.0.0.4);)
table=1 (lr_out_snat ), priority=100 , match=(flags.force_snat_for_lb == 1 && ip6), action=(ct_snat(aef0::4);)
table=1 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;)
])

check ovn-nbctl --wait=sb set logical_router lr0 options:lb_force_snat_ip="router_ip"
Expand All @@ -2784,15 +2788,19 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
table=5 (lr_in_unsnat ), priority=110 , match=(inport == "lr0-sw1" && ip4.dst == 20.0.0.1), action=(ct_snat;)
])

AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_dnat;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.4:8080);)
table=6 (lr_in_dnat ), priority=50 , match=(ip), action=(flags.loopback = 1; ct_dnat;)
])

AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), action=(ct_snat(172.168.0.100);)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"), action=(ct_snat(20.0.0.1);)
table=1 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;)
])

check ovn-nbctl --wait=sb remove logical_router lr0 options chassis
Expand All @@ -2804,7 +2812,9 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
table=5 (lr_in_unsnat ), priority=0 , match=(1), action=(next;)
])

AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
table=1 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;)
])

check ovn-nbctl set logical_router lr0 options:chassis=ch1
Expand All @@ -2821,16 +2831,20 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
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 force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_dnat;)
table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip && ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.4:8080);)
table=6 (lr_in_dnat ), priority=50 , match=(ip), action=(flags.loopback = 1; ct_dnat;)
])

AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0], [dnl
AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), action=(ct_snat(172.168.0.100);)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"), action=(ct_snat(20.0.0.1);)
table=1 (lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw1"), action=(ct_snat(bef0::1);)
table=1 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;)
])

AT_CLEANUP
Expand Down

0 comments on commit 82b4c61

Please sign in to comment.