On Thu, Aug 20, 2020 at 8:04 AM Ankur Sharma <[email protected]> wrote:
> From: Ankur Sharma <[email protected]> > > This patch has northd changes which consumes allowed/exempted external ip > configuration per NAT rule in logical flow. > > Allowed external ip range adds an additional match criteria in > snat/dnat logical flow rules. > > For example, if an allowed_external_ip address set ("abcd") > 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 == $abcd), > action=(ct_snat(...);) > > Exempted external ip range adds an additional flow at priority+1 > to bypass the NAT pipeline if external ip is in extempted external > ip address set. > For example, if the same NAT rule mentioned aboe has an > exempted_external_ip address set ("efgh"), then > logical flow will look like following: > > ..(lr_out_snat), priority=162...match=(ip && .... && ip4.dst == $efgh), > action=(next;) > ..(lr_out_snat), priority=161...match=(ip && ....), > action=(ct_snat(10.15.24.135);) > > Signed-off-by: Ankur Sharma <[email protected]> > Hi Ankur, You have missed adding documentation for the updated/new logical flows in ovn-northd.8.xml. There are few checkpatch warnings. **** == Checking 3a813c606ac4 ("External IP based NAT: NORTHD changes to use allowed/exempted external ip") == WARNING: Line is 95 characters long (recommended limit is 79) #85 FILE: northd/ovn-northd.c:8325: * lr_out_snat...priority=162 , match=(....ip4.dst == $exempted_range), action=(next;) WARNING: Line is 88 characters long (recommended limit is 79) #86 FILE: northd/ovn-northd.c:8326: * lr_out_snat...priority=161 , match=(......), action=(ct_snat(10.15.24.139);) WARNING: Empty return followed by brace, consider omitting #111 FILE: northd/ovn-northd.c:8351: } Lines checked: 307, Warnings: 3, Errors: 0 ***** --- > northd/ovn-northd.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++ > tests/ovn-northd.at | 111 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 212 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 212de2f..9f703d7 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -8280,6 +8280,75 @@ lrouter_nat_is_stateless(const struct nbrec_nat > *nat) > return false; > } > > +/* Handles the match criteria and actions in logical flow > + * based on external ip based NAT rule filter. > + * > + * For ALLOWED_EXT_IPs, we will add an additional match criteria > + * of comparing ip*.src/dst with the allowed external ip address set. > + * > + * For EXEMPTED_EXT_IPs, we will have an additional logical flow > + * where we compare ip*.src/dst with the exempted external ip address set > + * and action says "next" instead of ct*. > + */ > +static inline void > +lrouter_nat_add_ext_ip_match(struct ovn_datapath *od, > + struct hmap *lflows, struct ds *match, > + const struct nbrec_nat *nat, > + bool is_v6, bool is_src, ovs_be32 mask) > +{ > + struct nbrec_address_set *allowed_ext_ips = nat->allowed_ext_ips; > + struct nbrec_address_set *exempted_ext_ips = nat->exempted_ext_ips; > + > + ovs_assert(allowed_ext_ips || exempted_ext_ips); > + > + if (allowed_ext_ips) { > + ds_put_format(match, " && ip%s.%s == $%s", > + is_v6 ? "6" : "4", > + is_src ? "src" : "dst", > + allowed_ext_ips->name); > + } else if (exempted_ext_ips) { > + struct ds match_exempt = DS_EMPTY_INITIALIZER; > + enum ovn_stage stage = is_src ? S_ROUTER_IN_DNAT : > S_ROUTER_OUT_SNAT; > + uint16_t priority = count_1bits(ntohl(mask)) + 2; > + priority += 128; > The above 2 lines need to be removed as you are overriding the value of 'priority' below in the if(is_src)/else. > + > + /* Priority of logical flows corresponding to exempted_ext_ips is > + * +1 of the corresponding regulr NAT rule. > + * For example, if we have following NAT rule and we associate > + * exempted external ips to it: > + * "ovn-nbctl lr-nat-add router dnat_and_snat 10.15.24.139 > 50.0.0.11" > + * > + * And now we associate exempted external ip address set to it. > + * Now corresponding to above rule we will have following logical > + * flows: > + * lr_out_snat...priority=162 , match=(....ip4.dst == > $exempted_range), action=(next;) > + * lr_out_snat...priority=161 , match=(......), > action=(ct_snat(10.15.24.139);) > + * > + */ > + if (is_src) { > + /* S_ROUTER_IN_DNAT uses priority 100 */ > + priority = 100 + 1; > + } else { > + /* S_ROUTER_OUT_SNAT uses priority (mask + 1 + 128) */ > + priority = count_1bits(ntohl(mask)) + 2; > + priority += 128; > + } > + > + ds_clone(&match_exempt, match); > + ds_put_format(&match_exempt, " && ip%s.%s == $%s", > + is_v6 ? "6" : "4", > + is_src ? "src" : "dst", > + exempted_ext_ips->name); > + > + ovn_lflow_add_with_hint(lflows, od, stage, priority, > + ds_cstr(&match_exempt), "next;", > + &nat->header_); > + ds_destroy(&match_exempt); > + } > + > + return; > You can remove this '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. > @@ -9341,6 +9410,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 *allowed_ext_ips = > + nat->allowed_ext_ips; > + struct nbrec_address_set *exempted_ext_ips = > + nat->exempted_ext_ips; > + > + if (allowed_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 allowed 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) { > @@ -9486,6 +9567,11 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > is_v6 ? "6" : "4", > nat->external_ip); > ds_clear(&actions); > + if (allowed_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(od, lflows, &match, > nat, > + is_v6, true, mask); > + } > + > if (dnat_force_snat_ip) { > /* Indicate to the future tables that a DNAT has > taken > * place and a force SNAT needs to be done in the > @@ -9529,6 +9615,10 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > od->l3redirect_port->json_key); > } > ds_clear(&actions); > + if (allowed_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(od, lflows, &match, > nat, > + is_v6, true, mask); > + } > > if (!strcmp(nat->type, "dnat_and_snat") && stateless) > { > ds_put_format(&actions, "ip%s.dst=%s; next;", > @@ -9640,6 +9730,11 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > nat->logical_ip); > ds_clear(&actions); > > + if (allowed_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(od, lflows, &match, > nat, > + is_v6, false, mask); > + } > + > if (!strcmp(nat->type, "dnat_and_snat") && stateless) > { > ds_put_format(&actions, "ip%s.src=%s; next;", > is_v6 ? "6" : "4", > nat->external_ip); > @@ -9679,6 +9774,12 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > od->l3redirect_port->json_key); > } > ds_clear(&actions); > + > + if (allowed_ext_ips || exempted_ext_ips) { > + lrouter_nat_add_ext_ip_match(od, lflows, &match, > nat, > + is_v6, false, mask); > + } > + > if (distributed) { > ds_put_format(&actions, "eth.src = > "ETH_ADDR_FMT"; ", > ETH_ADDR_ARGS(mac)); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 8344c7f..e2fdba3 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1140,6 +1140,117 @@ 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 > + > Can you please extend this test case for gateway logical router too ? As the patch adds lflows for both gateway router and router with gw port. Thanks Numan +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 lr-nat-update-ext-ip R1 snat 50.0.0.11 allowed_range > + > +#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 > +]) > + > +# 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 --is-exempted lr-nat-update-ext-ip R1 snat 50.0.0.11 > disallowed_range > + > +ovn-sbctl dump-flows R1 > +OVS_WAIT_UNTIL([test 4 = `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" | grep "priority=162" | wc > -l], [0], [1 > +]) > +AT_CHECK([ovn-sbctl dump-flows R1 | grep lr_out_snat | grep "ip4.src == > 50.0.0.11" | grep "priority=161" | 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 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_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 --is-exempted 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 4 = `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" | grep "priority=162" | 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" | grep "priority=101" | > 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 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_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 --is-exempted 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 4 = `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" | grep "priority=162" | 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" | grep "priority=101" | > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
