On Wed, Aug 5, 2020 at 1:56 AM Ankur Sharma <svc.mail....@nutanix.com> wrote: > > From: Ankur Sharma <ankur.sha...@nutanix.com> > > This patch has northd changes which consumes applied/exempted external ip > configuration per NAT rule in logical flow. > > Applied/Exempted external ip range adds an additional match criteria in > snat/dnat/unsnat/undant logical flow rules. > > For example, if an allowed_external_ip address set ("efgh") > 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 == $efgh), > action=(ct_snat(...);) > ...(lr_in_unsnat...)match=(ip && ..... && ip4.src == $efgh), action=(ct_snat;) > > Signed-off-by: Ankur Sharma <ankur.sha...@nutanix.com> > --- > northd/ovn-northd.c | 61 +++++++++++++++++++++++++ > tests/ovn-northd.at | 127 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 188 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 03c62ba..17ae6a6 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -8250,6 +8250,23 @@ lrouter_nat_is_stateless(const struct nbrec_nat *nat) > return false; > } > > +static inline void > +lrouter_nat_add_ext_ip_match(struct ds *match, const struct nbrec_nat *nat, > + bool is_v6, bool is_src) > +{ > + struct nbrec_address_set *applied_ext_ips = nat->applied_ext_ips; > + struct nbrec_address_set *exempted_ext_ips = nat->exempted_ext_ips; > + > + ds_put_format(match, " && ip%s.%s %s $%s", > + is_v6 ? "6" : "4", > + is_src ? "src" : "dst", > + applied_ext_ips? "==" : "!=",
I Ankur, I think this is a big problem here. We should not use "!=" in logical flows, although OVN allows. This results in lots of lots of OF flows. In my testing with the below logical flow table=5 (lr_in_unsnat ), priority=100 , match=(ip && ip4.dst == 172.168.0.100 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public") && ip4.src != $disallowed_range), action=(ct_snat;) table=1 (lr_out_snat ), priority=153 , match=(ip && ip4.src == 10.0.0.0/24 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public") && ip4.dst != $disallowed_range), action=(ct_snat(172.168.0.100);) If the address set 'disallowed_range' has 1 IP, it results in around 74 Of flows because of these 2 logical flows. With 2 addresses in the addr set, it results in around 1200 OF flows. With 3 addresses, it results in around 1500 OF flows And with 4 addresses, it results in around 155622 OF flows. I don't think this is acceptable. And we should not use the "!=" match. The approach seems fine to me for 'applied_ext_ips', but not for 'exempted_ext_ips'. I'd suggest exploring other alternatives. Thanks Numan > + applied_ext_ips? > + applied_ext_ips->name : > + exempted_ext_ips->name); > + 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. > @@ -9199,6 +9216,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 *applied_ext_ips = > + nat->applied_ext_ips; > + struct nbrec_address_set *exempted_ext_ips = > + nat->exempted_ext_ips; > + > + if (applied_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 applied 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) { > @@ -9286,6 +9315,10 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&match, "ip && ip%s.dst == %s", > is_v6 ? "6" : "4", > nat->external_ip); > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, > true); > + } > + > if (!strcmp(nat->type, "dnat_and_snat") && stateless) { > ds_put_format(&actions, "ip%s.dst=%s; next;", > is_v6 ? "6" : "4", nat->logical_ip); > @@ -9315,6 +9348,10 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > od->l3redirect_port->json_key); > } > > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, > true); > + } > + > if (!strcmp(nat->type, "dnat_and_snat") && stateless) { > ds_put_format(&actions, "ip%s.dst=%s; next;", > is_v6 ? "6" : "4", nat->logical_ip); > @@ -9343,6 +9380,10 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&match, "ip && ip%s.dst == %s", > is_v6 ? "6" : "4", > nat->external_ip); > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, > true); > + } > + > ds_clear(&actions); > if (dnat_force_snat_ip) { > /* Indicate to the future tables that a DNAT has > taken > @@ -9386,6 +9427,11 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&match, " && is_chassis_resident(%s)", > od->l3redirect_port->json_key); > } > + > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, > true); > + } > + > ds_clear(&actions); > > if (!strcmp(nat->type, "dnat_and_snat") && stateless) { > @@ -9467,6 +9513,11 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&match, " && is_chassis_resident(%s)", > od->l3redirect_port->json_key); > } > + > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, false); > + } > + > ds_clear(&actions); > if (distributed) { > ds_put_format(&actions, "eth.src = "ETH_ADDR_FMT"; ", > @@ -9496,6 +9547,11 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&match, "ip && ip%s.src == %s", > is_v6 ? "6" : "4", > nat->logical_ip); > + > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, > false); > + } > + > ds_clear(&actions); > > if (!strcmp(nat->type, "dnat_and_snat") && stateless) { > @@ -9536,6 +9592,11 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&match, " && is_chassis_resident(%s)", > od->l3redirect_port->json_key); > } > + > + if (applied_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(&match, nat, is_v6, > false); > + } > + > ds_clear(&actions); > if (distributed) { > ds_put_format(&actions, "eth.src = "ETH_ADDR_FMT"; ", > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 7872d50..a97a776 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1140,6 +1140,133 @@ 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 > + > +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 --is-applied lr-nat-update-ext-ip R1 snat 50.0.0.11 allowed_range > + > +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_unsnat | grep "ip4.dst == > 172.16.1.1" | grep "ip4.src == $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 lr-nat-update-ext-ip R1 snat 50.0.0.11 disallowed_range > + > +OVS_WAIT_UNTIL([test 3 = `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" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_unsnat | grep "ip4.dst == > 172.16.1.1" | grep "ip4.src != $disallowed_range" | 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 --is-applied 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_unsnat | grep "ip4.dst == > 172.16.1.2" | grep "ip4.src == $allowed_range" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_undnat | 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 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 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 != $disallowed_range" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_unsnat | grep "ip4.dst == > 172.16.1.2" | grep "ip4.src != $disallowed_range" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_undnat | grep "ip4.src == > 50.0.0.11" | grep "ip4.dst != $disallowed_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 != $disallowed_range" | 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 --is-applied 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_unsnat | grep "ip4.dst == > 172.16.1.2" | grep "ip4.src == $allowed_range" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_undnat | 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 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 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 != $disallowed_range" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_in_unsnat | grep "ip4.dst == > 172.16.1.2" | grep "ip4.src != $disallowed_range" | wc -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_undnat | grep "ip4.src == > 50.0.0.11" | grep "ip4.dst != $disallowed_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 != $disallowed_range" | 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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev