Message ID | 1596572757-5511-3-git-send-email-svc.mail.git@nutanix.com |
---|---|
State | Changes Requested |
Headers | show |
Series | External IP based NAT | expand |
Bleep bloop. Greetings svc.mail.git, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Empty return followed by brace, consider omitting #50 FILE: northd/ovn-northd.c:8268: } WARNING: Line is 80 characters long (recommended limit is 79) #137 FILE: northd/ovn-northd.c:9552: lrouter_nat_add_ext_ip_match(&match, nat, is_v6, false); WARNING: Line is 80 characters long (recommended limit is 79) #149 FILE: northd/ovn-northd.c:9597: lrouter_nat_add_ext_ip_match(&match, nat, is_v6, false); Lines checked: 295, Warnings: 3, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Wed, Aug 5, 2020 at 1:56 AM Ankur Sharma <svc.mail.git@nutanix.com> wrote: > > From: Ankur Sharma <ankur.sharma@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.sharma@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> -----Original Message----- > From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Numan Siddique > Sent: Friday, August 7, 2020 2:38 AM > To: Ankur Sharma <svc.mail.git@nutanix.com> > Cc: ovs-dev <ovs-dev@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH v4 2/2 ovn] External IP based NAT: NORTHD > changes to use applied/exempted external ip > > On Wed, Aug 5, 2020 at 1:56 AM Ankur Sharma <svc.mail.git@nutanix.com> > wrote: > > > > From: Ankur Sharma <ankur.sharma@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.sharma@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. Is this a generic recommendation or for certain cases? Is it OK to add an ACL with "!=", like below? ovn-nbctl acl-add 12b1681c-b3e7-4ec9-b324-e780d9dfdc0d from-lport 1005 'ip4.dst == 192.168.0.0/16 && inport != "d93619c3-dab9-4f6d-8261-4211f6937fd1"' drop Thanks! Tony > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Sun, Aug 9, 2020 at 11:39 PM Tony Liu <tonyliu0592@hotmail.com> wrote: > > > > > -----Original Message----- > > From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Numan Siddique > > Sent: Friday, August 7, 2020 2:38 AM > > To: Ankur Sharma <svc.mail.git@nutanix.com> > > Cc: ovs-dev <ovs-dev@openvswitch.org> > > Subject: Re: [ovs-dev] [PATCH v4 2/2 ovn] External IP based NAT: NORTHD > > changes to use applied/exempted external ip > > > > On Wed, Aug 5, 2020 at 1:56 AM Ankur Sharma <svc.mail.git@nutanix.com> > > wrote: > > > > > > From: Ankur Sharma <ankur.sharma@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.sharma@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. > > Is this a generic recommendation or for certain cases? > Is it OK to add an ACL with "!=", like below? > > ovn-nbctl acl-add 12b1681c-b3e7-4ec9-b324-e780d9dfdc0d from-lport 1005 'ip4.dst == 192.168.0.0/16 && inport != "d93619c3-dab9-4f6d-8261-4211f6937fd1"' drop This is a generic recommendation. The above ACL would also result in many OF flows. To handle cases like above, you can add a couple of ACLs like below with high priority flow to allow the desired inport and low priority ACL to drop all the traffic. ovn-nbctl acl-add 12b1681c-b3e7-4ec9-b324-e780d9dfdc0d from-lport 1006 'ip4.dst == 192.168.0.0/16 && inport == "d93619c3-dab9-4f6d-8261-4211f6937fd1"' allow ovn-nbctl acl-add 12b1681c-b3e7-4ec9-b324-e780d9dfdc0d from-lport 1005 'ip4.dst == 192.168.0.0/16"' drop Thanks Numan > > Thanks! > Tony > > > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Numan, Sure, yes thats a valid concern. I will try to address this in V5. Regards, Ankur
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? "==" : "!=", + 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