| Message ID | f6e750f396f21f43056bbb19dc6d370252560637.1715704710.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] northd: Skip arp-proxy flows if the lsp is a router port. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 5/14/24 18:44, Lorenzo Bianconi wrote: > Skip proxy-arp logical flows for traffic that is entering the logical > switch pipeline from a lsp of type router. This patch will avoid > recirculating back the traffic entering by the logical router > pipeline if proxy-arp hasn been configured by the CMS. > > Reported-at: https://issues.redhat.com/browse/FDP-96 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Hi Lorenzo, This change looks OK but I'd like to double check that it doesn't break CNV/ovn-kubernetes use cases. Hi Enrique, Would you mind double checking that on one of your setups? If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests don't enable anything CNV specific: https://github.com/ovsrobot/ovn/actions/runs/9083258143 Thanks, Dumitru > northd/northd.c | 15 +++++++++++++-- > tests/ovn.at | 8 ++++---- > tests/system-ovn.at | 22 +++++++++++++++++++++- > 3 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 0cabda7ea..29dc58ef4 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -118,6 +118,7 @@ static bool default_acl_drop; > #define REGBIT_PORT_SEC_DROP "reg0[15]" > #define REGBIT_ACL_STATELESS "reg0[16]" > #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" > +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" > > #define REG_ORIG_DIP_IPV4 "reg1" > #define REG_ORIG_DIP_IPV6 "xxreg1" > @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, struct lflow_table *lflows, > &op->od->localnet_ports[0]->nbsp->header_, > op->lflow_ref); > } > + } else if (lsp_is_router(op->nbsp)) { > + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); > + ovn_lflow_add_with_lport_and_hint(lflows, op->od, > + S_SWITCH_IN_CHECK_PORT_SEC, 70, > + ds_cstr(match), ds_cstr(actions), > + op->key, &op->nbsp->header_, > + op->lflow_ref); > } > } > > @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > if (op->proxy_arp_addrs.n_ipv4_addrs) { > /* Match rule on all proxy ARP IPs. */ > ds_clear(match); > - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); > + ds_put_cstr(match, > + REGBIT_FROM_ROUTER_PORT" == 0 " > + "&& arp.op == 1 && arp.tpa == {"); > > for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { > ds_put_format(match, "%s/%u,", > @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > ds_truncate(&nd_target_match, nd_target_match.length - 2); > ds_clear(match); > ds_put_format(match, > - "nd_ns " > + REGBIT_FROM_ROUTER_PORT" == 0 " > + "&& nd_ns " > "&& ip6.dst == { %s } " > "&& nd.target == { %s }", > ds_cstr(&ip6_dst_match), > diff --git a/tests/ovn.at b/tests/ovn.at > index 486680649..e419516a7 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | > grep ls_in_arp_rsp | > grep "${arp_proxy_ls1[[1]]}" | > ovn_strip_lflows], [0], [dnl > - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 && dnl > + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == 0 && arp.op == 1 && dnl > arp.tpa == {169.254.238.0/24,169.254.239.2/32}), dnl > action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; dnl > /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl > @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | > grep "${arp_proxy_ls1[[3]]}" | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_arp_rsp ), priority=30 , dnl > -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl > +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl > fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl > nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 }), dnl > action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = nd.target; dnl > @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | > grep "${arp_proxy_ls2[[2]]}" | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_arp_rsp ), priority=30 , dnl > -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32}), dnl > +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32}), dnl > action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; dnl > /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl > arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) > @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | > grep "${arp_proxy_ls2[[4]]}" | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_arp_rsp ), priority=30 , dnl > -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl > +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl > fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl > nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 }), dnl > action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = nd.target; dnl > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 86fd240d2..e6cfb07f6 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar > # Connect foo to R1 > check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 > check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \ > - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo addresses='"router"' > + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" options:router-port=foo addresses='"router"' > > # Connect bar to R1 > check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 > @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, "192.168.1.4/24", "f0:00:00:01:02:05", \ > check ovn-nbctl lsp-add foo foo3 \ > -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" > > +ADD_NAMESPACES(foo4) > +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ > + "192.168.1.1") > +check ovn-nbctl lsp-add foo foo4 \ > +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" > + > # Logical port 'ext1' in switch 'foo' > ADD_NAMESPACES(ext1) > ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", "f0:00:00:01:02:06", \ > @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:07", \ > check ovn-nbctl lsp-add bar bar1 \ > -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" > > +ADD_NAMESPACES(bar2) > +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ > +"192.168.2.1") > +check ovn-nbctl lsp-add bar bar2 \ > +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" > + > # wait for ovn-controller to catch up. > check ovn-nbctl --wait=hv sync > > @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ > test "${total_pkts}" = "3" > ]) > > +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 > +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 arp[[24:4]]=0xc0a801c8], [foo4-arp]) > + > +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 169.254.240.10],[ignore],[ignore]) > +OVS_WAIT_UNTIL([ > + total_pkts=$(cat foo4-arp.tcpdump| wc -l) > + test "${total_pkts}" = "4" > +]) > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) >
> On 5/14/24 18:44, Lorenzo Bianconi wrote: > > Skip proxy-arp logical flows for traffic that is entering the logical > > switch pipeline from a lsp of type router. This patch will avoid > > recirculating back the traffic entering by the logical router > > pipeline if proxy-arp hasn been configured by the CMS. > > > > Reported-at: https://issues.redhat.com/browse/FDP-96 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Hi Lorenzo, > > This change looks OK but I'd like to double check that it doesn't break > CNV/ovn-kubernetes use cases. > > Hi Enrique, > > Would you mind double checking that on one of your setups? > > If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests > don't enable anything CNV specific: > https://github.com/ovsrobot/ovn/actions/runs/9083258143 > > Thanks, > Dumitru Hi Dumitru, Thx, for the review. That's a good idea :) Regards, Lorenzo > > > northd/northd.c | 15 +++++++++++++-- > > tests/ovn.at | 8 ++++---- > > tests/system-ovn.at | 22 +++++++++++++++++++++- > > 3 files changed, 38 insertions(+), 7 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 0cabda7ea..29dc58ef4 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -118,6 +118,7 @@ static bool default_acl_drop; > > #define REGBIT_PORT_SEC_DROP "reg0[15]" > > #define REGBIT_ACL_STATELESS "reg0[16]" > > #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" > > +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" > > > > #define REG_ORIG_DIP_IPV4 "reg1" > > #define REG_ORIG_DIP_IPV6 "xxreg1" > > @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, struct lflow_table *lflows, > > &op->od->localnet_ports[0]->nbsp->header_, > > op->lflow_ref); > > } > > + } else if (lsp_is_router(op->nbsp)) { > > + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); > > + ovn_lflow_add_with_lport_and_hint(lflows, op->od, > > + S_SWITCH_IN_CHECK_PORT_SEC, 70, > > + ds_cstr(match), ds_cstr(actions), > > + op->key, &op->nbsp->header_, > > + op->lflow_ref); > > } > > } > > > > @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > > if (op->proxy_arp_addrs.n_ipv4_addrs) { > > /* Match rule on all proxy ARP IPs. */ > > ds_clear(match); > > - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); > > + ds_put_cstr(match, > > + REGBIT_FROM_ROUTER_PORT" == 0 " > > + "&& arp.op == 1 && arp.tpa == {"); > > > > for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { > > ds_put_format(match, "%s/%u,", > > @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > > ds_truncate(&nd_target_match, nd_target_match.length - 2); > > ds_clear(match); > > ds_put_format(match, > > - "nd_ns " > > + REGBIT_FROM_ROUTER_PORT" == 0 " > > + "&& nd_ns " > > "&& ip6.dst == { %s } " > > "&& nd.target == { %s }", > > ds_cstr(&ip6_dst_match), > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 486680649..e419516a7 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > grep ls_in_arp_rsp | > > grep "${arp_proxy_ls1[[1]]}" | > > ovn_strip_lflows], [0], [dnl > > - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 && dnl > > + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == 0 && arp.op == 1 && dnl > > arp.tpa == {169.254.238.0/24,169.254.239.2/32}), dnl > > action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; dnl > > /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl > > @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > grep "${arp_proxy_ls1[[3]]}" | > > ovn_strip_lflows], [0], [dnl > > table=??(ls_in_arp_rsp ), priority=30 , dnl > > -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl > > +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl > > fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl > > nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 }), dnl > > action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = nd.target; dnl > > @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > grep "${arp_proxy_ls2[[2]]}" | > > ovn_strip_lflows], [0], [dnl > > table=??(ls_in_arp_rsp ), priority=30 , dnl > > -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32}), dnl > > +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32}), dnl > > action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; dnl > > /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl > > arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) > > @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > grep "${arp_proxy_ls2[[4]]}" | > > ovn_strip_lflows], [0], [dnl > > table=??(ls_in_arp_rsp ), priority=30 , dnl > > -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl > > +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl > > fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl > > nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 }), dnl > > action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = nd.target; dnl > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 86fd240d2..e6cfb07f6 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar > > # Connect foo to R1 > > check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 > > check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \ > > - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo addresses='"router"' > > + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" options:router-port=foo addresses='"router"' > > > > # Connect bar to R1 > > check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 > > @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, "192.168.1.4/24", "f0:00:00:01:02:05", \ > > check ovn-nbctl lsp-add foo foo3 \ > > -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" > > > > +ADD_NAMESPACES(foo4) > > +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ > > + "192.168.1.1") > > +check ovn-nbctl lsp-add foo foo4 \ > > +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" > > + > > # Logical port 'ext1' in switch 'foo' > > ADD_NAMESPACES(ext1) > > ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", "f0:00:00:01:02:06", \ > > @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:07", \ > > check ovn-nbctl lsp-add bar bar1 \ > > -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" > > > > +ADD_NAMESPACES(bar2) > > +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ > > +"192.168.2.1") > > +check ovn-nbctl lsp-add bar bar2 \ > > +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" > > + > > # wait for ovn-controller to catch up. > > check ovn-nbctl --wait=hv sync > > > > @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ > > test "${total_pkts}" = "3" > > ]) > > > > +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 > > +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 arp[[24:4]]=0xc0a801c8], [foo4-arp]) > > + > > +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 169.254.240.10],[ignore],[ignore]) > > +OVS_WAIT_UNTIL([ > > + total_pkts=$(cat foo4-arp.tcpdump| wc -l) > > + test "${total_pkts}" = "4" > > +]) > > > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > >
On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > On 5/14/24 18:44, Lorenzo Bianconi wrote: > > > Skip proxy-arp logical flows for traffic that is entering the logical > > > switch pipeline from a lsp of type router. This patch will avoid > > > recirculating back the traffic entering by the logical router > > > pipeline if proxy-arp hasn been configured by the CMS. > > > > > > Reported-at: https://issues.redhat.com/browse/FDP-96 > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > --- > > > > Hi Lorenzo, > > > > This change looks OK but I'd like to double check that it doesn't break > > CNV/ovn-kubernetes use cases. > > > > Hi Enrique, > > > > Would you mind double checking that on one of your setups? > > > > If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests > > don't enable anything CNV specific: > > https://github.com/ovsrobot/ovn/actions/runs/9083258143 > > > > Thanks, > > Dumitru > > Hi Dumitru, > > Thx, for the review. That's a good idea :) > Live migration tests look fine with this change, both IC and non IC, let's also activate the kubevirt lanes so we gate with them too. Tested-by: <ellorent@redhat.com> > > Regards, > Lorenzo > > > > > > northd/northd.c | 15 +++++++++++++-- > > > tests/ovn.at | 8 ++++---- > > > tests/system-ovn.at | 22 +++++++++++++++++++++- > > > 3 files changed, 38 insertions(+), 7 deletions(-) > > > > > > diff --git a/northd/northd.c b/northd/northd.c > > > index 0cabda7ea..29dc58ef4 100644 > > > --- a/northd/northd.c > > > +++ b/northd/northd.c > > > @@ -118,6 +118,7 @@ static bool default_acl_drop; > > > #define REGBIT_PORT_SEC_DROP "reg0[15]" > > > #define REGBIT_ACL_STATELESS "reg0[16]" > > > #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" > > > +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" > > > > > > #define REG_ORIG_DIP_IPV4 "reg1" > > > #define REG_ORIG_DIP_IPV6 "xxreg1" > > > @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, > struct lflow_table *lflows, > > > &op->od->localnet_ports[0]->nbsp->header_, > > > op->lflow_ref); > > > } > > > + } else if (lsp_is_router(op->nbsp)) { > > > + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); > > > + ovn_lflow_add_with_lport_and_hint(lflows, op->od, > > > + S_SWITCH_IN_CHECK_PORT_SEC, > 70, > > > + ds_cstr(match), > ds_cstr(actions), > > > + op->key, &op->nbsp->header_, > > > + op->lflow_ref); > > > } > > > } > > > > > > @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct > ovn_port *op, > > > if (op->proxy_arp_addrs.n_ipv4_addrs) { > > > /* Match rule on all proxy ARP IPs. */ > > > ds_clear(match); > > > - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); > > > + ds_put_cstr(match, > > > + REGBIT_FROM_ROUTER_PORT" == 0 " > > > + "&& arp.op == 1 && arp.tpa == {"); > > > > > > for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { > > > ds_put_format(match, "%s/%u,", > > > @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct > ovn_port *op, > > > ds_truncate(&nd_target_match, nd_target_match.length - 2); > > > ds_clear(match); > > > ds_put_format(match, > > > - "nd_ns " > > > + REGBIT_FROM_ROUTER_PORT" == 0 " > > > + "&& nd_ns " > > > "&& ip6.dst == { %s } " > > > "&& nd.target == { %s }", > > > ds_cstr(&ip6_dst_match), > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index 486680649..e419516a7 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > > grep ls_in_arp_rsp | > > > grep "${arp_proxy_ls1[[1]]}" | > > > ovn_strip_lflows], [0], [dnl > > > - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 > && dnl > > > + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == > 0 && arp.op == 1 && dnl > > > arp.tpa == {169.254.238.0/24,169.254.239.2/32} > <http://169.254.238.0/24,169.254.239.2/32%7D>), dnl > > > action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; > dnl > > > /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl > > > @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > > grep "${arp_proxy_ls1[[3]]}" | > > > ovn_strip_lflows], [0], [dnl > > > table=??(ls_in_arp_rsp ), priority=30 , dnl > > > -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, > ff02::1:ff00:0/64, dnl > > > +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { > fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl > > > fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl > > > nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 > }), dnl > > > action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = > nd.target; dnl > > > @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > > grep "${arp_proxy_ls2[[2]]}" | > > > ovn_strip_lflows], [0], [dnl > > > table=??(ls_in_arp_rsp ), priority=30 , dnl > > > -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32} > <http://169.254.236.0/24,169.254.237.2/32%7D>), dnl > > > +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == { > 169.254.236.0/24,169.254.237.2/32} > <http://169.254.236.0/24,169.254.237.2/32%7D>), dnl > > > action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; > dnl > > > /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl > > > arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) > > > @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | > > > grep "${arp_proxy_ls2[[4]]}" | > > > ovn_strip_lflows], [0], [dnl > > > table=??(ls_in_arp_rsp ), priority=30 , dnl > > > -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, > ff02::1:ff00:0/64, dnl > > > +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { > fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl > > > fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl > > > nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 > }), dnl > > > action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = > nd.target; dnl > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > > index 86fd240d2..e6cfb07f6 100644 > > > --- a/tests/system-ovn.at > > > +++ b/tests/system-ovn.at > > > @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar > > > # Connect foo to R1 > > > check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 > > > check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \ > > > - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 > 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo > addresses='"router"' > > > + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 > 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" > options:router-port=foo addresses='"router"' > > > > > > # Connect bar to R1 > > > check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 > > > @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, "192.168.1.4/24", > "f0:00:00:01:02:05", \ > > > check ovn-nbctl lsp-add foo foo3 \ > > > -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" > > > > > > +ADD_NAMESPACES(foo4) > > > +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ > > > + "192.168.1.1") > > > +check ovn-nbctl lsp-add foo foo4 \ > > > +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" > > > + > > > # Logical port 'ext1' in switch 'foo' > > > ADD_NAMESPACES(ext1) > > > ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", "f0:00:00:01:02:06", \ > > > @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", > "f0:00:00:01:02:07", \ > > > check ovn-nbctl lsp-add bar bar1 \ > > > -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" > > > > > > +ADD_NAMESPACES(bar2) > > > +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ > > > +"192.168.2.1") > > > +check ovn-nbctl lsp-add bar bar2 \ > > > +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" > > > + > > > # wait for ovn-controller to catch up. > > > check ovn-nbctl --wait=hv sync > > > > > > @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ > > > test "${total_pkts}" = "3" > > > ]) > > > > > > +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 > > > +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 > arp[[24:4]]=0xc0a801c8], [foo4-arp]) > > > + > > > +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 > 169.254.240.10],[ignore],[ignore]) > > > +OVS_WAIT_UNTIL([ > > > + total_pkts=$(cat foo4-arp.tcpdump| wc -l) > > > + test "${total_pkts}" = "4" > > > +]) > > > > > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > > > > >
So I'm wondering will this break the LSP option: > *o**p**t**i**o**n**s* *:* *a**r**p**_**p**r**o**x**y*: optional string > Optional. A list of MAC and addresses/cidrs or just ad‐ > dresses/cidrs that this logical switch*r**o**u**t**e**r* port will reply to > ARP/NDP requests. Examples:*1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* *1**6**9**.**2**5**4**.**2**3**9**.**2*, > *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* *1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* *1**6**9**.**2**5**4**.**2**3**9**.**2* > *1**6**9**.**2**5**4**.**2**3**8**.**0**/**2**4*,*f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**2*, > *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**0**/**6**4*. The*o**p**t**i**o**n**s**:**r**o**u**t**e**r**-* > *p**o**r**t*’s logical router should have a route to forward packets > sent to configured proxy ARP MAC/IPs to an appropriate destina‐ > tion. On 05/06/2024 13:03, Enrique Llorente Pastora wrote: > On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > >>> On 5/14/24 18:44, Lorenzo Bianconi wrote: >>>> Skip proxy-arp logical flows for traffic that is entering the logical >>>> switch pipeline from a lsp of type router. This patch will avoid >>>> recirculating back the traffic entering by the logical router >>>> pipeline if proxy-arp hasn been configured by the CMS. >>>> >>>> Reported-at:https://urldefense.com/v3/__https://issues.redhat.com/browse/FDP-96__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfnKJQEhn$ >>>> Signed-off-by: Lorenzo Bianconi<lorenzo.bianconi@redhat.com> >>>> --- >>> Hi Lorenzo, >>> >>> This change looks OK but I'd like to double check that it doesn't break >>> CNV/ovn-kubernetes use cases. >>> >>> Hi Enrique, >>> >>> Would you mind double checking that on one of your setups? >>> >>> If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests >>> don't enable anything CNV specific: >>> https://urldefense.com/v3/__https://github.com/ovsrobot/ovn/actions/runs/9083258143__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MflAqga4Q$ >>> >>> Thanks, >>> Dumitru >> Hi Dumitru, >> >> Thx, for the review. That's a good idea :) >> > Live migration tests look fine with this change, both IC and non IC, let's > also activate the > kubevirt lanes so we gate with them too. > > Tested-by:<ellorent@redhat.com> > > >> Regards, >> Lorenzo >> >>>> northd/northd.c | 15 +++++++++++++-- >>>> tests/ovn.at | 8 ++++---- >>>> tests/system-ovn.at | 22 +++++++++++++++++++++- >>>> 3 files changed, 38 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index 0cabda7ea..29dc58ef4 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -118,6 +118,7 @@ static bool default_acl_drop; >>>> #define REGBIT_PORT_SEC_DROP "reg0[15]" >>>> #define REGBIT_ACL_STATELESS "reg0[16]" >>>> #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" >>>> +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" >>>> >>>> #define REG_ORIG_DIP_IPV4 "reg1" >>>> #define REG_ORIG_DIP_IPV6 "xxreg1" >>>> @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, >> struct lflow_table *lflows, >>>> &op->od->localnet_ports[0]->nbsp->header_, >>>> op->lflow_ref); >>>> } >>>> + } else if (lsp_is_router(op->nbsp)) { >>>> + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); >>>> + ovn_lflow_add_with_lport_and_hint(lflows, op->od, >>>> + S_SWITCH_IN_CHECK_PORT_SEC, >> 70, >>>> + ds_cstr(match), >> ds_cstr(actions), >>>> + op->key, &op->nbsp->header_, >>>> + op->lflow_ref); >>>> } >>>> } >>>> >>>> @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct >> ovn_port *op, >>>> if (op->proxy_arp_addrs.n_ipv4_addrs) { >>>> /* Match rule on all proxy ARP IPs. */ >>>> ds_clear(match); >>>> - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); >>>> + ds_put_cstr(match, >>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>> + "&& arp.op == 1 && arp.tpa == {"); >>>> >>>> for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { >>>> ds_put_format(match, "%s/%u,", >>>> @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct >> ovn_port *op, >>>> ds_truncate(&nd_target_match, nd_target_match.length - 2); >>>> ds_clear(match); >>>> ds_put_format(match, >>>> - "nd_ns " >>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>> + "&& nd_ns " >>>> "&& ip6.dst == { %s } " >>>> "&& nd.target == { %s }", >>>> ds_cstr(&ip6_dst_match), >>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>> index 486680649..e419516a7 100644 >>>> --- a/tests/ovn.at >>>> +++ b/tests/ovn.at >>>> @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>> grep ls_in_arp_rsp | >>>> grep "${arp_proxy_ls1[[1]]}" | >>>> ovn_strip_lflows], [0], [dnl >>>> - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 >> && dnl >>>> + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == >> 0 && arp.op == 1 && dnl >>>> arp.tpa == {169.254.238.0/24,169.254.239.2/32} >> <https://urldefense.com/v3/__http://169.254.238.0/24,169.254.239.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfjM6HO9o$ >> >), dnl >>>> action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; >> dnl >>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl >>>> @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>> grep "${arp_proxy_ls1[[3]]}" | >>>> ovn_strip_lflows], [0], [dnl >>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, >> ff02::1:ff00:0/64, dnl >>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >> fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl >>>> fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl >>>> nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 >> }), dnl >>>> action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = >> nd.target; dnl >>>> @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>> grep "${arp_proxy_ls2[[2]]}" | >>>> ovn_strip_lflows], [0], [dnl >>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>> -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32} >> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >> >), dnl >>>> +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == { >> 169.254.236.0/24,169.254.237.2/32} >> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >> >), dnl >>>> action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; >> dnl >>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl >>>> arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) >>>> @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>> grep "${arp_proxy_ls2[[4]]}" | >>>> ovn_strip_lflows], [0], [dnl >>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, >> ff02::1:ff00:0/64, dnl >>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >> fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl >>>> fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl >>>> nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 >> }), dnl >>>> action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = >> nd.target; dnl >>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>>> index 86fd240d2..e6cfb07f6 100644 >>>> --- a/tests/system-ovn.at >>>> +++ b/tests/system-ovn.at >>>> @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar >>>> # Connect foo to R1 >>>> check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 >>>> check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \ >>>> - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 >> 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo >> addresses='"router"' >>>> + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 >> 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" >> options:router-port=foo addresses='"router"' >>>> # Connect bar to R1 >>>> check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 >>>> @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, "192.168.1.4/24", >> "f0:00:00:01:02:05", \ >>>> check ovn-nbctl lsp-add foo foo3 \ >>>> -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" >>>> >>>> +ADD_NAMESPACES(foo4) >>>> +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ >>>> + "192.168.1.1") >>>> +check ovn-nbctl lsp-add foo foo4 \ >>>> +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" >>>> + >>>> # Logical port 'ext1' in switch 'foo' >>>> ADD_NAMESPACES(ext1) >>>> ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", "f0:00:00:01:02:06", \ >>>> @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", >> "f0:00:00:01:02:07", \ >>>> check ovn-nbctl lsp-add bar bar1 \ >>>> -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" >>>> >>>> +ADD_NAMESPACES(bar2) >>>> +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ >>>> +"192.168.2.1") >>>> +check ovn-nbctl lsp-add bar bar2 \ >>>> +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" >>>> + >>>> # wait for ovn-controller to catch up. >>>> check ovn-nbctl --wait=hv sync >>>> >>>> @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ >>>> test "${total_pkts}" = "3" >>>> ]) >>>> >>>> +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 >>>> +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 >> arp[[24:4]]=0xc0a801c8], [foo4-arp]) >>>> + >>>> +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 >> 169.254.240.10],[ignore],[ignore]) >>>> +OVS_WAIT_UNTIL([ >>>> + total_pkts=$(cat foo4-arp.tcpdump| wc -l) >>>> + test "${total_pkts}" = "4" >>>> +]) >>>> >>>> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >>>> >
On 6/5/24 16:52, Brendan Doyle via dev wrote: > > > So I'm wondering will this break the LSP option: >> *o**p**t**i**o**n**s* *:* *a**r**p**_**p**r**o**x**y*: >> optional string >> Optional. A list of MAC and addresses/cidrs or >> just ad‐ >> dresses/cidrs that this logical >> switch*r**o**u**t**e**r* port will reply to >> ARP/NDP requests. >> Examples:*1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >> *1**6**9**.**2**5**4**.**2**3**9**.**2*, >> >> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >> *1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >> *1**6**9**.**2**5**4**.**2**3**9**.**2* >> >> *1**6**9**.**2**5**4**.**2**3**8**.**0**/**2**4*,*f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**2*, >> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >> *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**0**/**6**4*. The*o**p**t**i**o**n**s**:**r**o**u**t**e**r**-* >> *p**o**r**t*’s logical router should have a route >> to forward packets >> sent to configured proxy ARP MAC/IPs to an appropriate >> destina‐ >> tion. > Hi Brendan, I'm not sure I understand what breaks. Do you have a specific scenario in mind? The patch is for the arp-proxy feature. I doubt that people rely on ARP requests originated by logical routers being handled by the arp proxy on the connected logical switch port. But I might be wrong. Thanks, Dumitru > > On 05/06/2024 13:03, Enrique Llorente Pastora wrote: >> On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < >> lorenzo.bianconi@redhat.com> wrote: >> >>>> On 5/14/24 18:44, Lorenzo Bianconi wrote: >>>>> Skip proxy-arp logical flows for traffic that is entering the logical >>>>> switch pipeline from a lsp of type router. This patch will avoid >>>>> recirculating back the traffic entering by the logical router >>>>> pipeline if proxy-arp hasn been configured by the CMS. >>>>> >>>>> Reported-at:https://urldefense.com/v3/__https://issues.redhat.com/browse/FDP-96__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfnKJQEhn$ Signed-off-by: Lorenzo Bianconi<lorenzo.bianconi@redhat.com> >>>>> --- >>>> Hi Lorenzo, >>>> >>>> This change looks OK but I'd like to double check that it doesn't break >>>> CNV/ovn-kubernetes use cases. >>>> >>>> Hi Enrique, >>>> >>>> Would you mind double checking that on one of your setups? >>>> >>>> If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests >>>> don't enable anything CNV specific: >>>> https://urldefense.com/v3/__https://github.com/ovsrobot/ovn/actions/runs/9083258143__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MflAqga4Q$ >>>> Thanks, >>>> Dumitru >>> Hi Dumitru, >>> >>> Thx, for the review. That's a good idea :) >>> >> Live migration tests look fine with this change, both IC and non IC, >> let's >> also activate the >> kubevirt lanes so we gate with them too. >> >> Tested-by:<ellorent@redhat.com> >> >> >>> Regards, >>> Lorenzo >>> >>>>> northd/northd.c | 15 +++++++++++++-- >>>>> tests/ovn.at | 8 ++++---- >>>>> tests/system-ovn.at | 22 +++++++++++++++++++++- >>>>> 3 files changed, 38 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>> index 0cabda7ea..29dc58ef4 100644 >>>>> --- a/northd/northd.c >>>>> +++ b/northd/northd.c >>>>> @@ -118,6 +118,7 @@ static bool default_acl_drop; >>>>> #define REGBIT_PORT_SEC_DROP "reg0[15]" >>>>> #define REGBIT_ACL_STATELESS "reg0[16]" >>>>> #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" >>>>> +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" >>>>> >>>>> #define REG_ORIG_DIP_IPV4 "reg1" >>>>> #define REG_ORIG_DIP_IPV6 "xxreg1" >>>>> @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, >>> struct lflow_table *lflows, >>>>> &op->od->localnet_ports[0]->nbsp->header_, >>>>> op->lflow_ref); >>>>> } >>>>> + } else if (lsp_is_router(op->nbsp)) { >>>>> + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); >>>>> + ovn_lflow_add_with_lport_and_hint(lflows, op->od, >>>>> + S_SWITCH_IN_CHECK_PORT_SEC, >>> 70, >>>>> + ds_cstr(match), >>> ds_cstr(actions), >>>>> + op->key, >>>>> &op->nbsp->header_, >>>>> + op->lflow_ref); >>>>> } >>>>> } >>>>> >>>>> @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct >>> ovn_port *op, >>>>> if (op->proxy_arp_addrs.n_ipv4_addrs) { >>>>> /* Match rule on all proxy ARP IPs. */ >>>>> ds_clear(match); >>>>> - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); >>>>> + ds_put_cstr(match, >>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>> + "&& arp.op == 1 && arp.tpa == {"); >>>>> >>>>> for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { >>>>> ds_put_format(match, "%s/%u,", >>>>> @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct >>> ovn_port *op, >>>>> ds_truncate(&nd_target_match, nd_target_match.length >>>>> - 2); >>>>> ds_clear(match); >>>>> ds_put_format(match, >>>>> - "nd_ns " >>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>> + "&& nd_ns " >>>>> "&& ip6.dst == { %s } " >>>>> "&& nd.target == { %s }", >>>>> ds_cstr(&ip6_dst_match), >>>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>>> index 486680649..e419516a7 100644 >>>>> --- a/tests/ovn.at >>>>> +++ b/tests/ovn.at >>>>> @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>> grep ls_in_arp_rsp | >>>>> grep "${arp_proxy_ls1[[1]]}" | >>>>> ovn_strip_lflows], [0], [dnl >>>>> - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 >>> && dnl >>>>> + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == >>> 0 && arp.op == 1 && dnl >>>>> arp.tpa == {169.254.238.0/24,169.254.239.2/32} >>> <https://urldefense.com/v3/__http://169.254.238.0/24,169.254.239.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfjM6HO9o$ >), dnl >>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; >>> dnl >>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl >>>>> @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>> grep "${arp_proxy_ls1[[3]]}" | >>>>> ovn_strip_lflows], [0], [dnl >>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, >>> ff02::1:ff00:0/64, dnl >>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>> fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl >>>>> fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl >>>>> nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 >>> }), dnl >>>>> action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = >>> nd.target; dnl >>>>> @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>> grep "${arp_proxy_ls2[[2]]}" | >>>>> ovn_strip_lflows], [0], [dnl >>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>> -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32} >>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>> +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == { >>> 169.254.236.0/24,169.254.237.2/32} >>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; >>> dnl >>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl >>>>> arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) >>>>> @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>> grep "${arp_proxy_ls2[[4]]}" | >>>>> ovn_strip_lflows], [0], [dnl >>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, >>> ff02::1:ff00:0/64, dnl >>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>> fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl >>>>> fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl >>>>> nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 >>> }), dnl >>>>> action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = >>> nd.target; dnl >>>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>>>> index 86fd240d2..e6cfb07f6 100644 >>>>> --- a/tests/system-ovn.at >>>>> +++ b/tests/system-ovn.at >>>>> @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar >>>>> # Connect foo to R1 >>>>> check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 >>>>> check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port >>>>> rp-foo \ >>>>> - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 >>> 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo >>> addresses='"router"' >>>>> + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 >>> 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" >>> options:router-port=foo addresses='"router"' >>>>> # Connect bar to R1 >>>>> check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 >>>>> @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, >>>>> "192.168.1.4/24", >>> "f0:00:00:01:02:05", \ >>>>> check ovn-nbctl lsp-add foo foo3 \ >>>>> -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" >>>>> >>>>> +ADD_NAMESPACES(foo4) >>>>> +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ >>>>> + "192.168.1.1") >>>>> +check ovn-nbctl lsp-add foo foo4 \ >>>>> +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" >>>>> + >>>>> # Logical port 'ext1' in switch 'foo' >>>>> ADD_NAMESPACES(ext1) >>>>> ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", >>>>> "f0:00:00:01:02:06", \ >>>>> @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, >>>>> "192.168.2.2/24", >>> "f0:00:00:01:02:07", \ >>>>> check ovn-nbctl lsp-add bar bar1 \ >>>>> -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" >>>>> >>>>> +ADD_NAMESPACES(bar2) >>>>> +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ >>>>> +"192.168.2.1") >>>>> +check ovn-nbctl lsp-add bar bar2 \ >>>>> +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" >>>>> + >>>>> # wait for ovn-controller to catch up. >>>>> check ovn-nbctl --wait=hv sync >>>>> >>>>> @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ >>>>> test "${total_pkts}" = "3" >>>>> ]) >>>>> >>>>> +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 >>>>> +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 >>> arp[[24:4]]=0xc0a801c8], [foo4-arp]) >>>>> + >>>>> +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 >>> 169.254.240.10],[ignore],[ignore]) >>>>> +OVS_WAIT_UNTIL([ >>>>> + total_pkts=$(cat foo4-arp.tcpdump| wc -l) >>>>> + test "${total_pkts}" = "4" >>>>> +]) >>>>> >>>>> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >>>>> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 06/06/2024 17:34, Dumitru Ceara wrote: > On 6/5/24 16:52, Brendan Doyle via dev wrote: >> >> So I'm wondering will this break the LSP option: >>> *o**p**t**i**o**n**s* *:* *a**r**p**_**p**r**o**x**y*: >>> optional string >>> Optional. A list of MAC and addresses/cidrs or >>> just ad‐ >>> dresses/cidrs that this logical >>> switch*r**o**u**t**e**r* port will reply to >>> ARP/NDP requests. >>> Examples:*1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>> *1**6**9**.**2**5**4**.**2**3**9**.**2*, >>> >>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>> *1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>> *1**6**9**.**2**5**4**.**2**3**9**.**2* >>> >>> *1**6**9**.**2**5**4**.**2**3**8**.**0**/**2**4*,*f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**2*, >>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>> *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**0**/**6**4*. The*o**p**t**i**o**n**s**:**r**o**u**t**e**r**-* >>> *p**o**r**t*’s logical router should have a route >>> to forward packets >>> sent to configured proxy ARP MAC/IPs to an appropriate >>> destina‐ >>> tion. > Hi Brendan, > > I'm not sure I understand what breaks. Do you have a specific scenario > in mind? The patch is for the arp-proxy feature. I doubt that people > rely on ARP requests originated by logical routers being handled by the > arp proxy on the connected logical switch port. But I might be wrong. > > Thanks, > Dumitru The arp_proxy option allows an lsp prot connected to an LR to respond to ARP requests sent from say a VM connected to the same LS. >> On 05/06/2024 13:03, Enrique Llorente Pastora wrote: >>> On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < >>> lorenzo.bianconi@redhat.com> wrote: >>> >>>>> On 5/14/24 18:44, Lorenzo Bianconi wrote: >>>>>> Skip proxy-arp logical flows for traffic that is entering the logical >>>>>> switch pipeline from a lsp of type router. This patch will avoid >>>>>> recirculating back the traffic entering by the logical router >>>>>> pipeline if proxy-arp hasn been configured by the CMS. >>>>>> >>>>>> Reported-at:https://urldefense.com/v3/__https://issues.redhat.com/browse/FDP-96__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfnKJQEhn$ Signed-off-by: Lorenzo Bianconi<lorenzo.bianconi@redhat.com> >>>>>> --- >>>>> Hi Lorenzo, >>>>> >>>>> This change looks OK but I'd like to double check that it doesn't break >>>>> CNV/ovn-kubernetes use cases. >>>>> >>>>> Hi Enrique, >>>>> >>>>> Would you mind double checking that on one of your setups? >>>>> >>>>> If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests >>>>> don't enable anything CNV specific: >>>>> https://urldefense.com/v3/__https://github.com/ovsrobot/ovn/actions/runs/9083258143__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MflAqga4Q$ >>>>> Thanks, >>>>> Dumitru >>>> Hi Dumitru, >>>> >>>> Thx, for the review. That's a good idea :) >>>> >>> Live migration tests look fine with this change, both IC and non IC, >>> let's >>> also activate the >>> kubevirt lanes so we gate with them too. >>> >>> Tested-by:<ellorent@redhat.com> >>> >>> >>>> Regards, >>>> Lorenzo >>>> >>>>>> northd/northd.c | 15 +++++++++++++-- >>>>>> tests/ovn.at | 8 ++++---- >>>>>> tests/system-ovn.at | 22 +++++++++++++++++++++- >>>>>> 3 files changed, 38 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>>> index 0cabda7ea..29dc58ef4 100644 >>>>>> --- a/northd/northd.c >>>>>> +++ b/northd/northd.c >>>>>> @@ -118,6 +118,7 @@ static bool default_acl_drop; >>>>>> #define REGBIT_PORT_SEC_DROP "reg0[15]" >>>>>> #define REGBIT_ACL_STATELESS "reg0[16]" >>>>>> #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" >>>>>> +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" >>>>>> >>>>>> #define REG_ORIG_DIP_IPV4 "reg1" >>>>>> #define REG_ORIG_DIP_IPV6 "xxreg1" >>>>>> @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, >>>> struct lflow_table *lflows, >>>>>> &op->od->localnet_ports[0]->nbsp->header_, >>>>>> op->lflow_ref); >>>>>> } >>>>>> + } else if (lsp_is_router(op->nbsp)) { >>>>>> + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); >>>>>> + ovn_lflow_add_with_lport_and_hint(lflows, op->od, >>>>>> + S_SWITCH_IN_CHECK_PORT_SEC, >>>> 70, >>>>>> + ds_cstr(match), >>>> ds_cstr(actions), >>>>>> + op->key, >>>>>> &op->nbsp->header_, >>>>>> + op->lflow_ref); >>>>>> } >>>>>> } >>>>>> >>>>>> @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct >>>> ovn_port *op, >>>>>> if (op->proxy_arp_addrs.n_ipv4_addrs) { >>>>>> /* Match rule on all proxy ARP IPs. */ >>>>>> ds_clear(match); >>>>>> - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); >>>>>> + ds_put_cstr(match, >>>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>>> + "&& arp.op == 1 && arp.tpa == {"); >>>>>> >>>>>> for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { >>>>>> ds_put_format(match, "%s/%u,", >>>>>> @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct >>>> ovn_port *op, >>>>>> ds_truncate(&nd_target_match, nd_target_match.length >>>>>> - 2); >>>>>> ds_clear(match); >>>>>> ds_put_format(match, >>>>>> - "nd_ns " >>>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>>> + "&& nd_ns " >>>>>> "&& ip6.dst == { %s } " >>>>>> "&& nd.target == { %s }", >>>>>> ds_cstr(&ip6_dst_match), >>>>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>>>> index 486680649..e419516a7 100644 >>>>>> --- a/tests/ovn.at >>>>>> +++ b/tests/ovn.at >>>>>> @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>> grep ls_in_arp_rsp | >>>>>> grep "${arp_proxy_ls1[[1]]}" | >>>>>> ovn_strip_lflows], [0], [dnl >>>>>> - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 >>>> && dnl >>>>>> + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == >>>> 0 && arp.op == 1 && dnl >>>>>> arp.tpa == {169.254.238.0/24,169.254.239.2/32} >>>> <https://urldefense.com/v3/__http://169.254.238.0/24,169.254.239.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfjM6HO9o$ >), dnl >>>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; >>>> dnl >>>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl >>>>>> @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>> grep "${arp_proxy_ls1[[3]]}" | >>>>>> ovn_strip_lflows], [0], [dnl >>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, >>>> ff02::1:ff00:0/64, dnl >>>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>>> fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl >>>>>> fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl >>>>>> nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 >>>> }), dnl >>>>>> action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = >>>> nd.target; dnl >>>>>> @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>> grep "${arp_proxy_ls2[[2]]}" | >>>>>> ovn_strip_lflows], [0], [dnl >>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>> -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32} >>>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>>> +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == { >>>> 169.254.236.0/24,169.254.237.2/32} >>>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; >>>> dnl >>>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl >>>>>> arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) >>>>>> @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>> grep "${arp_proxy_ls2[[4]]}" | >>>>>> ovn_strip_lflows], [0], [dnl >>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, >>>> ff02::1:ff00:0/64, dnl >>>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>>> fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl >>>>>> fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl >>>>>> nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 >>>> }), dnl >>>>>> action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = >>>> nd.target; dnl >>>>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>>>>> index 86fd240d2..e6cfb07f6 100644 >>>>>> --- a/tests/system-ovn.at >>>>>> +++ b/tests/system-ovn.at >>>>>> @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar >>>>>> # Connect foo to R1 >>>>>> check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 >>>>>> check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port >>>>>> rp-foo \ >>>>>> - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 >>>> 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo >>>> addresses='"router"' >>>>>> + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 >>>> 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" >>>> options:router-port=foo addresses='"router"' >>>>>> # Connect bar to R1 >>>>>> check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 >>>>>> @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, >>>>>> "192.168.1.4/24", >>>> "f0:00:00:01:02:05", \ >>>>>> check ovn-nbctl lsp-add foo foo3 \ >>>>>> -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" >>>>>> >>>>>> +ADD_NAMESPACES(foo4) >>>>>> +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ >>>>>> + "192.168.1.1") >>>>>> +check ovn-nbctl lsp-add foo foo4 \ >>>>>> +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" >>>>>> + >>>>>> # Logical port 'ext1' in switch 'foo' >>>>>> ADD_NAMESPACES(ext1) >>>>>> ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", >>>>>> "f0:00:00:01:02:06", \ >>>>>> @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, >>>>>> "192.168.2.2/24", >>>> "f0:00:00:01:02:07", \ >>>>>> check ovn-nbctl lsp-add bar bar1 \ >>>>>> -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" >>>>>> >>>>>> +ADD_NAMESPACES(bar2) >>>>>> +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ >>>>>> +"192.168.2.1") >>>>>> +check ovn-nbctl lsp-add bar bar2 \ >>>>>> +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" >>>>>> + >>>>>> # wait for ovn-controller to catch up. >>>>>> check ovn-nbctl --wait=hv sync >>>>>> >>>>>> @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ >>>>>> test "${total_pkts}" = "3" >>>>>> ]) >>>>>> >>>>>> +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 >>>>>> +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 >>>> arp[[24:4]]=0xc0a801c8], [foo4-arp]) >>>>>> + >>>>>> +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 >>>> 169.254.240.10],[ignore],[ignore]) >>>>>> +OVS_WAIT_UNTIL([ >>>>>> + total_pkts=$(cat foo4-arp.tcpdump| wc -l) >>>>>> + test "${total_pkts}" = "4" >>>>>> +]) >>>>>> >>>>>> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >>>>>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!ACWV5N9M2RV99hQ!O8DJQyfkM12yHUI2p0lARreiYWj6CBQnuFOWboNDonA-W-FvQYibstvjYS5OYCN67lmPyzTRIEY2JVNjyw$
On 6/7/24 12:24, brendan.doyle@oracle.com wrote: > > > On 06/06/2024 17:34, Dumitru Ceara wrote: >> On 6/5/24 16:52, Brendan Doyle via dev wrote: >>> >>> So I'm wondering will this break the LSP option: >>>> *o**p**t**i**o**n**s* *:* *a**r**p**_**p**r**o**x**y*: >>>> optional string >>>> Optional. A list of MAC and addresses/cidrs or >>>> just ad‐ >>>> dresses/cidrs that this logical >>>> switch*r**o**u**t**e**r* port will reply to >>>> ARP/NDP requests. >>>> Examples:*1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>>> *1**6**9**.**2**5**4**.**2**3**9**.**2*, >>>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>>> *1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>>> *1**6**9**.**2**5**4**.**2**3**9**.**2* >>>> >>>> *1**6**9**.**2**5**4**.**2**3**8**.**0**/**2**4*,*f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**2*, >>>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>>> *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**0**/**6**4*. The*o**p**t**i**o**n**s**:**r**o**u**t**e**r**-* >>>> *p**o**r**t*’s logical router should have a route >>>> to forward packets >>>> sent to configured proxy ARP MAC/IPs to an appropriate >>>> destina‐ >>>> tion. >> Hi Brendan, >> >> I'm not sure I understand what breaks. Do you have a specific scenario >> in mind? The patch is for the arp-proxy feature. I doubt that people >> rely on ARP requests originated by logical routers being handled by the >> arp proxy on the connected logical switch port. But I might be wrong. >> >> Thanks, >> Dumitru > > The arp_proxy option allows an lsp prot connected to an LR to respond > to ARP requests sent from say a VM connected to the same LS. > Right, we have a test for that: https://github.com/ovn-org/ovn/blob/main/tests/system-ovn.at#L10721-L10869 And the patch doesn't change it, it just expands it. Or am I missing something? Regards, Dumitru > >>> On 05/06/2024 13:03, Enrique Llorente Pastora wrote: >>>> On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < >>>> lorenzo.bianconi@redhat.com> wrote: >>>> >>>>>> On 5/14/24 18:44, Lorenzo Bianconi wrote: >>>>>>> Skip proxy-arp logical flows for traffic that is entering the >>>>>>> logical >>>>>>> switch pipeline from a lsp of type router. This patch will avoid >>>>>>> recirculating back the traffic entering by the logical router >>>>>>> pipeline if proxy-arp hasn been configured by the CMS. >>>>>>> >>>>>>> Reported-at:https://urldefense.com/v3/__https://issues.redhat.com/browse/FDP-96__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfnKJQEhn$ Signed-off-by: Lorenzo Bianconi<lorenzo.bianconi@redhat.com> >>>>>>> --- >>>>>> Hi Lorenzo, >>>>>> >>>>>> This change looks OK but I'd like to double check that it doesn't >>>>>> break >>>>>> CNV/ovn-kubernetes use cases. >>>>>> >>>>>> Hi Enrique, >>>>>> >>>>>> Would you mind double checking that on one of your setups? >>>>>> >>>>>> If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests >>>>>> don't enable anything CNV specific: >>>>>> https://urldefense.com/v3/__https://github.com/ovsrobot/ovn/actions/runs/9083258143__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MflAqga4Q$ >>>>>> Thanks, >>>>>> Dumitru >>>>> Hi Dumitru, >>>>> >>>>> Thx, for the review. That's a good idea :) >>>>> >>>> Live migration tests look fine with this change, both IC and non IC, >>>> let's >>>> also activate the >>>> kubevirt lanes so we gate with them too. >>>> >>>> Tested-by:<ellorent@redhat.com> >>>> >>>> >>>>> Regards, >>>>> Lorenzo >>>>> >>>>>>> northd/northd.c | 15 +++++++++++++-- >>>>>>> tests/ovn.at | 8 ++++---- >>>>>>> tests/system-ovn.at | 22 +++++++++++++++++++++- >>>>>>> 3 files changed, 38 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>>>> index 0cabda7ea..29dc58ef4 100644 >>>>>>> --- a/northd/northd.c >>>>>>> +++ b/northd/northd.c >>>>>>> @@ -118,6 +118,7 @@ static bool default_acl_drop; >>>>>>> #define REGBIT_PORT_SEC_DROP "reg0[15]" >>>>>>> #define REGBIT_ACL_STATELESS "reg0[16]" >>>>>>> #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" >>>>>>> +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" >>>>>>> >>>>>>> #define REG_ORIG_DIP_IPV4 "reg1" >>>>>>> #define REG_ORIG_DIP_IPV6 "xxreg1" >>>>>>> @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port >>>>>>> *op, >>>>> struct lflow_table *lflows, >>>>>>> &op->od->localnet_ports[0]->nbsp->header_, >>>>>>> op->lflow_ref); >>>>>>> } >>>>>>> + } else if (lsp_is_router(op->nbsp)) { >>>>>>> + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; >>>>>>> next;"); >>>>>>> + ovn_lflow_add_with_lport_and_hint(lflows, op->od, >>>>>>> + >>>>>>> S_SWITCH_IN_CHECK_PORT_SEC, >>>>> 70, >>>>>>> + ds_cstr(match), >>>>> ds_cstr(actions), >>>>>>> + op->key, >>>>>>> &op->nbsp->header_, >>>>>>> + op->lflow_ref); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> @@ -9051,7 +9059,9 @@ >>>>>>> build_lswitch_arp_nd_responder_known_ips(struct >>>>> ovn_port *op, >>>>>>> if (op->proxy_arp_addrs.n_ipv4_addrs) { >>>>>>> /* Match rule on all proxy ARP IPs. */ >>>>>>> ds_clear(match); >>>>>>> - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); >>>>>>> + ds_put_cstr(match, >>>>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>>>> + "&& arp.op == 1 && arp.tpa == {"); >>>>>>> >>>>>>> for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; >>>>>>> i++) { >>>>>>> ds_put_format(match, "%s/%u,", >>>>>>> @@ -9105,7 +9115,8 @@ >>>>>>> build_lswitch_arp_nd_responder_known_ips(struct >>>>> ovn_port *op, >>>>>>> ds_truncate(&nd_target_match, nd_target_match.length >>>>>>> - 2); >>>>>>> ds_clear(match); >>>>>>> ds_put_format(match, >>>>>>> - "nd_ns " >>>>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>>>> + "&& nd_ns " >>>>>>> "&& ip6.dst == { %s } " >>>>>>> "&& nd.target == { %s }", >>>>>>> ds_cstr(&ip6_dst_match), >>>>>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>>>>> index 486680649..e419516a7 100644 >>>>>>> --- a/tests/ovn.at >>>>>>> +++ b/tests/ovn.at >>>>>>> @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>> grep ls_in_arp_rsp | >>>>>>> grep "${arp_proxy_ls1[[1]]}" | >>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>> - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 >>>>> && dnl >>>>>>> + table=??(ls_in_arp_rsp ), priority=30 , >>>>>>> match=(reg0[[18]] == >>>>> 0 && arp.op == 1 && dnl >>>>>>> arp.tpa == {169.254.238.0/24,169.254.239.2/32} >>>>> <https://urldefense.com/v3/__http://169.254.238.0/24,169.254.239.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfjM6HO9o$ >), dnl >>>>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op >>>>>>> = 2; >>>>> dnl >>>>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = >>>>>>> 00:00:00:01:02:f1; dnl >>>>>>> @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>> grep "${arp_proxy_ls1[[3]]}" | >>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, >>>>> ff02::1:ff00:0/64, dnl >>>>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>>>> fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl >>>>>>> fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl >>>>>>> nd.target == { fd7b:6b4d:7b25:d22d::/64, >>>>>>> fd7b:6b4d:7b25:d22f::1/128 >>>>> }), dnl >>>>>>> action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = >>>>> nd.target; dnl >>>>>>> @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>> grep "${arp_proxy_ls2[[2]]}" | >>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>>> -match=(arp.op == 1 && arp.tpa == >>>>>>> {169.254.236.0/24,169.254.237.2/32} >>>>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>>>> +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == { >>>>> 169.254.236.0/24,169.254.237.2/32} >>>>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op >>>>>>> = 2; >>>>> dnl >>>>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = >>>>>>> 00:00:00:02:02:f1; dnl >>>>>>> arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; >>>>>>> output;) >>>>>>> @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>> grep "${arp_proxy_ls2[[4]]}" | >>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, >>>>> ff02::1:ff00:0/64, dnl >>>>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>>>> fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl >>>>>>> fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl >>>>>>> nd.target == { fd7b:6b4d:7b25:d22b::/64, >>>>>>> fd7b:6b4d:7b25:d22c::1/128 >>>>> }), dnl >>>>>>> action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = >>>>> nd.target; dnl >>>>>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>>>>>> index 86fd240d2..e6cfb07f6 100644 >>>>>>> --- a/tests/system-ovn.at >>>>>>> +++ b/tests/system-ovn.at >>>>>>> @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar >>>>>>> # Connect foo to R1 >>>>>>> check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 >>>>>>> check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port >>>>>>> rp-foo \ >>>>>>> - type=router options:arp_proxy="0a:58:a9:fe:01:01 >>>>>>> 169.254.239.254 >>>>> 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo >>>>> addresses='"router"' >>>>>>> + type=router options:arp_proxy="0a:58:a9:fe:01:01 >>>>>>> 169.254.239.254 >>>>> 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" >>>>> options:router-port=foo addresses='"router"' >>>>>>> # Connect bar to R1 >>>>>>> check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 >>>>>>> @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, >>>>>>> "192.168.1.4/24", >>>>> "f0:00:00:01:02:05", \ >>>>>>> check ovn-nbctl lsp-add foo foo3 \ >>>>>>> -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" >>>>>>> >>>>>>> +ADD_NAMESPACES(foo4) >>>>>>> +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", >>>>>>> "f0:00:00:01:02:11", \ >>>>>>> + "192.168.1.1") >>>>>>> +check ovn-nbctl lsp-add foo foo4 \ >>>>>>> +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" >>>>>>> + >>>>>>> # Logical port 'ext1' in switch 'foo' >>>>>>> ADD_NAMESPACES(ext1) >>>>>>> ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", >>>>>>> "f0:00:00:01:02:06", \ >>>>>>> @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, >>>>>>> "192.168.2.2/24", >>>>> "f0:00:00:01:02:07", \ >>>>>>> check ovn-nbctl lsp-add bar bar1 \ >>>>>>> -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" >>>>>>> >>>>>>> +ADD_NAMESPACES(bar2) >>>>>>> +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", >>>>>>> "f0:00:00:01:02:10", \ >>>>>>> +"192.168.2.1") >>>>>>> +check ovn-nbctl lsp-add bar bar2 \ >>>>>>> +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" >>>>>>> + >>>>>>> # wait for ovn-controller to catch up. >>>>>>> check ovn-nbctl --wait=hv sync >>>>>>> >>>>>>> @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ >>>>>>> test "${total_pkts}" = "3" >>>>>>> ]) >>>>>>> >>>>>>> +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 >>>>>>> +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 >>>>> arp[[24:4]]=0xc0a801c8], [foo4-arp]) >>>>>>> + >>>>>>> +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 >>>>> 169.254.240.10],[ignore],[ignore]) >>>>>>> +OVS_WAIT_UNTIL([ >>>>>>> + total_pkts=$(cat foo4-arp.tcpdump| wc -l) >>>>>>> + test "${total_pkts}" = "4" >>>>>>> +]) >>>>>>> >>>>>>> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >>>>>>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!ACWV5N9M2RV99hQ!O8DJQyfkM12yHUI2p0lARreiYWj6CBQnuFOWboNDonA-W-FvQYibstvjYS5OYCN67lmPyzTRIEY2JVNjyw$ >
On 07/06/2024 14:20, Dumitru Ceara wrote: > On 6/7/24 12:24, brendan.doyle@oracle.com wrote: >> >> On 06/06/2024 17:34, Dumitru Ceara wrote: >>> On 6/5/24 16:52, Brendan Doyle via dev wrote: >>>> So I'm wondering will this break the LSP option: >>>>> *o**p**t**i**o**n**s* *:* *a**r**p**_**p**r**o**x**y*: >>>>> optional string >>>>> Optional. A list of MAC and addresses/cidrs or >>>>> just ad‐ >>>>> dresses/cidrs that this logical >>>>> switch*r**o**u**t**e**r* port will reply to >>>>> ARP/NDP requests. >>>>> Examples:*1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>>>> *1**6**9**.**2**5**4**.**2**3**9**.**2*, >>>>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>>>> *1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>>>> *1**6**9**.**2**5**4**.**2**3**9**.**2* >>>>> >>>>> *1**6**9**.**2**5**4**.**2**3**8**.**0**/**2**4*,*f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**2*, >>>>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>>>> *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**0**/**6**4*. The*o**p**t**i**o**n**s**:**r**o**u**t**e**r**-* >>>>> *p**o**r**t*’s logical router should have a route >>>>> to forward packets >>>>> sent to configured proxy ARP MAC/IPs to an appropriate >>>>> destina‐ >>>>> tion. >>> Hi Brendan, >>> >>> I'm not sure I understand what breaks. Do you have a specific scenario >>> in mind? The patch is for the arp-proxy feature. I doubt that people >>> rely on ARP requests originated by logical routers being handled by the >>> arp proxy on the connected logical switch port. But I might be wrong. >>> >>> Thanks, >>> Dumitru >> The arp_proxy option allows an lsp prot connected to an LR to respond >> to ARP requests sent from say a VM connected to the same LS. >> > Right, we have a test for that: > > https://urldefense.com/v3/__https://github.com/ovn-org/ovn/blob/main/tests/system-ovn.at*L10721-L10869__;Iw!!ACWV5N9M2RV99hQ!K8qbDDzmm6Fz7yPm3lubG0kMbM_MmKQPqAW5ewepc1iqUAdif248P613_b0CNhmCXnEozbrwpIVCStm3Jg$ > > And the patch doesn't change it, it just expands it. Or am I missing > something? No, I think we are good then just checking. > Regards, > Dumitru > >>>> On 05/06/2024 13:03, Enrique Llorente Pastora wrote: >>>>> On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < >>>>> lorenzo.bianconi@redhat.com> wrote: >>>>> >>>>>>> On 5/14/24 18:44, Lorenzo Bianconi wrote: >>>>>>>> Skip proxy-arp logical flows for traffic that is entering the >>>>>>>> logical >>>>>>>> switch pipeline from a lsp of type router. This patch will avoid >>>>>>>> recirculating back the traffic entering by the logical router >>>>>>>> pipeline if proxy-arp hasn been configured by the CMS. >>>>>>>> >>>>>>>> Reported-at:https://urldefense.com/v3/__https://issues.redhat.com/browse/FDP-96__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfnKJQEhn$ Signed-off-by: Lorenzo Bianconi<lorenzo.bianconi@redhat.com> >>>>>>>> --- >>>>>>> Hi Lorenzo, >>>>>>> >>>>>>> This change looks OK but I'd like to double check that it doesn't >>>>>>> break >>>>>>> CNV/ovn-kubernetes use cases. >>>>>>> >>>>>>> Hi Enrique, >>>>>>> >>>>>>> Would you mind double checking that on one of your setups? >>>>>>> >>>>>>> If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests >>>>>>> don't enable anything CNV specific: >>>>>>> https://urldefense.com/v3/__https://github.com/ovsrobot/ovn/actions/runs/9083258143__;!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MflAqga4Q$ >>>>>>> Thanks, >>>>>>> Dumitru >>>>>> Hi Dumitru, >>>>>> >>>>>> Thx, for the review. That's a good idea :) >>>>>> >>>>> Live migration tests look fine with this change, both IC and non IC, >>>>> let's >>>>> also activate the >>>>> kubevirt lanes so we gate with them too. >>>>> >>>>> Tested-by:<ellorent@redhat.com> >>>>> >>>>> >>>>>> Regards, >>>>>> Lorenzo >>>>>> >>>>>>>> northd/northd.c | 15 +++++++++++++-- >>>>>>>> tests/ovn.at | 8 ++++---- >>>>>>>> tests/system-ovn.at | 22 +++++++++++++++++++++- >>>>>>>> 3 files changed, 38 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>>>>> index 0cabda7ea..29dc58ef4 100644 >>>>>>>> --- a/northd/northd.c >>>>>>>> +++ b/northd/northd.c >>>>>>>> @@ -118,6 +118,7 @@ static bool default_acl_drop; >>>>>>>> #define REGBIT_PORT_SEC_DROP "reg0[15]" >>>>>>>> #define REGBIT_ACL_STATELESS "reg0[16]" >>>>>>>> #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" >>>>>>>> +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" >>>>>>>> >>>>>>>> #define REG_ORIG_DIP_IPV4 "reg1" >>>>>>>> #define REG_ORIG_DIP_IPV6 "xxreg1" >>>>>>>> @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port >>>>>>>> *op, >>>>>> struct lflow_table *lflows, >>>>>>>> &op->od->localnet_ports[0]->nbsp->header_, >>>>>>>> op->lflow_ref); >>>>>>>> } >>>>>>>> + } else if (lsp_is_router(op->nbsp)) { >>>>>>>> + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; >>>>>>>> next;"); >>>>>>>> + ovn_lflow_add_with_lport_and_hint(lflows, op->od, >>>>>>>> + >>>>>>>> S_SWITCH_IN_CHECK_PORT_SEC, >>>>>> 70, >>>>>>>> + ds_cstr(match), >>>>>> ds_cstr(actions), >>>>>>>> + op->key, >>>>>>>> &op->nbsp->header_, >>>>>>>> + op->lflow_ref); >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> @@ -9051,7 +9059,9 @@ >>>>>>>> build_lswitch_arp_nd_responder_known_ips(struct >>>>>> ovn_port *op, >>>>>>>> if (op->proxy_arp_addrs.n_ipv4_addrs) { >>>>>>>> /* Match rule on all proxy ARP IPs. */ >>>>>>>> ds_clear(match); >>>>>>>> - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); >>>>>>>> + ds_put_cstr(match, >>>>>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>>>>> + "&& arp.op == 1 && arp.tpa == {"); >>>>>>>> >>>>>>>> for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; >>>>>>>> i++) { >>>>>>>> ds_put_format(match, "%s/%u,", >>>>>>>> @@ -9105,7 +9115,8 @@ >>>>>>>> build_lswitch_arp_nd_responder_known_ips(struct >>>>>> ovn_port *op, >>>>>>>> ds_truncate(&nd_target_match, nd_target_match.length >>>>>>>> - 2); >>>>>>>> ds_clear(match); >>>>>>>> ds_put_format(match, >>>>>>>> - "nd_ns " >>>>>>>> + REGBIT_FROM_ROUTER_PORT" == 0 " >>>>>>>> + "&& nd_ns " >>>>>>>> "&& ip6.dst == { %s } " >>>>>>>> "&& nd.target == { %s }", >>>>>>>> ds_cstr(&ip6_dst_match), >>>>>>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>>>>>> index 486680649..e419516a7 100644 >>>>>>>> --- a/tests/ovn.at >>>>>>>> +++ b/tests/ovn.at >>>>>>>> @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>>> grep ls_in_arp_rsp | >>>>>>>> grep "${arp_proxy_ls1[[1]]}" | >>>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>>> - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 >>>>>> && dnl >>>>>>>> + table=??(ls_in_arp_rsp ), priority=30 , >>>>>>>> match=(reg0[[18]] == >>>>>> 0 && arp.op == 1 && dnl >>>>>>>> arp.tpa == {169.254.238.0/24,169.254.239.2/32} >>>>>> <https://urldefense.com/v3/__http://169.254.238.0/24,169.254.239.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2MfjM6HO9o$ >), dnl >>>>>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op >>>>>>>> = 2; >>>>>> dnl >>>>>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = >>>>>>>> 00:00:00:01:02:f1; dnl >>>>>>>> @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>>> grep "${arp_proxy_ls1[[3]]}" | >>>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, >>>>>> ff02::1:ff00:0/64, dnl >>>>>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>>>>> fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl >>>>>>>> fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl >>>>>>>> nd.target == { fd7b:6b4d:7b25:d22d::/64, >>>>>>>> fd7b:6b4d:7b25:d22f::1/128 >>>>>> }), dnl >>>>>>>> action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = >>>>>> nd.target; dnl >>>>>>>> @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>>> grep "${arp_proxy_ls2[[2]]}" | >>>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>>>> -match=(arp.op == 1 && arp.tpa == >>>>>>>> {169.254.236.0/24,169.254.237.2/32} >>>>>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>>>>> +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == { >>>>>> 169.254.236.0/24,169.254.237.2/32} >>>>>> <https://urldefense.com/v3/__http://169.254.236.0/24,169.254.237.2/32*7D__;JQ!!ACWV5N9M2RV99hQ!LZCOnrD03VQXa0QBfUgAD0IntqpkCU4P2Mz0jhlfAcJ0Joe1ehsAIloa28JszFGBjchui8U4OL2Mfuwz0Yd3$ >), dnl >>>>>>>> action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op >>>>>>>> = 2; >>>>>> dnl >>>>>>>> /* ARP reply */ arp.tha = arp.sha; arp.sha = >>>>>>>> 00:00:00:02:02:f1; dnl >>>>>>>> arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; >>>>>>>> output;) >>>>>>>> @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | >>>>>>>> grep "${arp_proxy_ls2[[4]]}" | >>>>>>>> ovn_strip_lflows], [0], [dnl >>>>>>>> table=??(ls_in_arp_rsp ), priority=30 , dnl >>>>>>>> -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, >>>>>> ff02::1:ff00:0/64, dnl >>>>>>>> +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { >>>>>> fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl >>>>>>>> fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl >>>>>>>> nd.target == { fd7b:6b4d:7b25:d22b::/64, >>>>>>>> fd7b:6b4d:7b25:d22c::1/128 >>>>>> }), dnl >>>>>>>> action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = >>>>>> nd.target; dnl >>>>>>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>>>>>>> index 86fd240d2..e6cfb07f6 100644 >>>>>>>> --- a/tests/system-ovn.at >>>>>>>> +++ b/tests/system-ovn.at >>>>>>>> @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar >>>>>>>> # Connect foo to R1 >>>>>>>> check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 >>>>>>>> check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port >>>>>>>> rp-foo \ >>>>>>>> - type=router options:arp_proxy="0a:58:a9:fe:01:01 >>>>>>>> 169.254.239.254 >>>>>> 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo >>>>>> addresses='"router"' >>>>>>>> + type=router options:arp_proxy="0a:58:a9:fe:01:01 >>>>>>>> 169.254.239.254 >>>>>> 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" >>>>>> options:router-port=foo addresses='"router"' >>>>>>>> # Connect bar to R1 >>>>>>>> check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 >>>>>>>> @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, >>>>>>>> "192.168.1.4/24", >>>>>> "f0:00:00:01:02:05", \ >>>>>>>> check ovn-nbctl lsp-add foo foo3 \ >>>>>>>> -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" >>>>>>>> >>>>>>>> +ADD_NAMESPACES(foo4) >>>>>>>> +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", >>>>>>>> "f0:00:00:01:02:11", \ >>>>>>>> + "192.168.1.1") >>>>>>>> +check ovn-nbctl lsp-add foo foo4 \ >>>>>>>> +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" >>>>>>>> + >>>>>>>> # Logical port 'ext1' in switch 'foo' >>>>>>>> ADD_NAMESPACES(ext1) >>>>>>>> ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", >>>>>>>> "f0:00:00:01:02:06", \ >>>>>>>> @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, >>>>>>>> "192.168.2.2/24", >>>>>> "f0:00:00:01:02:07", \ >>>>>>>> check ovn-nbctl lsp-add bar bar1 \ >>>>>>>> -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" >>>>>>>> >>>>>>>> +ADD_NAMESPACES(bar2) >>>>>>>> +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", >>>>>>>> "f0:00:00:01:02:10", \ >>>>>>>> +"192.168.2.1") >>>>>>>> +check ovn-nbctl lsp-add bar bar2 \ >>>>>>>> +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" >>>>>>>> + >>>>>>>> # wait for ovn-controller to catch up. >>>>>>>> check ovn-nbctl --wait=hv sync >>>>>>>> >>>>>>>> @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ >>>>>>>> test "${total_pkts}" = "3" >>>>>>>> ]) >>>>>>>> >>>>>>>> +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 >>>>>>>> +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 >>>>>> arp[[24:4]]=0xc0a801c8], [foo4-arp]) >>>>>>>> + >>>>>>>> +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 >>>>>> 169.254.240.10],[ignore],[ignore]) >>>>>>>> +OVS_WAIT_UNTIL([ >>>>>>>> + total_pkts=$(cat foo4-arp.tcpdump| wc -l) >>>>>>>> + test "${total_pkts}" = "4" >>>>>>>> +]) >>>>>>>> >>>>>>>> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >>>>>>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!ACWV5N9M2RV99hQ!O8DJQyfkM12yHUI2p0lARreiYWj6CBQnuFOWboNDonA-W-FvQYibstvjYS5OYCN67lmPyzTRIEY2JVNjyw$
On 6/7/24 15:26, brendan.doyle@oracle.com wrote: > > > On 07/06/2024 14:20, Dumitru Ceara wrote: >> On 6/7/24 12:24, brendan.doyle@oracle.com wrote: >>> >>> On 06/06/2024 17:34, Dumitru Ceara wrote: >>>> On 6/5/24 16:52, Brendan Doyle via dev wrote: >>>>> So I'm wondering will this break the LSP option: >>>>>> *o**p**t**i**o**n**s* *:* *a**r**p**_**p**r**o**x**y*: >>>>>> optional string >>>>>> Optional. A list of MAC and >>>>>> addresses/cidrs or >>>>>> just ad‐ >>>>>> dresses/cidrs that this logical >>>>>> switch*r**o**u**t**e**r* port will reply to >>>>>> ARP/NDP requests. >>>>>> Examples:*1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>>>>> *1**6**9**.**2**5**4**.**2**3**9**.**2*, >>>>>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>>>>> *1**6**9**.**2**5**4**.**2**3**9**.**2**5**4* >>>>>> *1**6**9**.**2**5**4**.**2**3**9**.**2* >>>>>> >>>>>> *1**6**9**.**2**5**4**.**2**3**8**.**0**/**2**4*,*f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**1* *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**2*, >>>>>> *0**a**:**5**8**:**a**9**:**f**e**:**0**1**:**0**1* >>>>>> *f**d**7**b**:**6**b**4**d**:**7**b**2**5**:**d**2**2**f**:**:**0**/**6**4*. The*o**p**t**i**o**n**s**:**r**o**u**t**e**r**-* >>>>>> *p**o**r**t*’s logical router should have a >>>>>> route >>>>>> to forward packets >>>>>> sent to configured proxy ARP MAC/IPs to an >>>>>> appropriate >>>>>> destina‐ >>>>>> tion. >>>> Hi Brendan, >>>> >>>> I'm not sure I understand what breaks. Do you have a specific scenario >>>> in mind? The patch is for the arp-proxy feature. I doubt that people >>>> rely on ARP requests originated by logical routers being handled by the >>>> arp proxy on the connected logical switch port. But I might be wrong. >>>> >>>> Thanks, >>>> Dumitru >>> The arp_proxy option allows an lsp prot connected to an LR to respond >>> to ARP requests sent from say a VM connected to the same LS. >>> >> Right, we have a test for that: >> >> https://urldefense.com/v3/__https://github.com/ovn-org/ovn/blob/main/tests/system-ovn.at*L10721-L10869__;Iw!!ACWV5N9M2RV99hQ!K8qbDDzmm6Fz7yPm3lubG0kMbM_MmKQPqAW5ewepc1iqUAdif248P613_b0CNhmCXnEozbrwpIVCStm3Jg$ >> >> And the patch doesn't change it, it just expands it. Or am I missing >> something? > > No, I think we are good then just checking. > Cool, thanks for that!
On 6/5/24 14:03, Enrique Llorente Pastora wrote: > On Wed, Jun 5, 2024 at 12:12 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > >>> On 5/14/24 18:44, Lorenzo Bianconi wrote: >>>> Skip proxy-arp logical flows for traffic that is entering the logical >>>> switch pipeline from a lsp of type router. This patch will avoid >>>> recirculating back the traffic entering by the logical router >>>> pipeline if proxy-arp hasn been configured by the CMS. >>>> >>>> Reported-at: https://issues.redhat.com/browse/FDP-96 >>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >>>> --- >>> >>> Hi Lorenzo, >>> >>> This change looks OK but I'd like to double check that it doesn't break >>> CNV/ovn-kubernetes use cases. >>> >>> Hi Enrique, >>> >>> Would you mind double checking that on one of your setups? >>> >>> If I'm not wrong our upstream ovn-kubernetes (in ovn-org/ovn) tests >>> don't enable anything CNV specific: >>> https://github.com/ovsrobot/ovn/actions/runs/9083258143 >>> >>> Thanks, >>> Dumitru >> >> Hi Dumitru, >> >> Thx, for the review. That's a good idea :) >> > > Live migration tests look fine with this change, both IC and non IC, let's > also activate the > kubevirt lanes so we gate with them too. > > Tested-by: <ellorent@redhat.com> > Thanks, Lorenzo, Enrique, Brendan! Applied to main, 24.03, 23.09 and 23.06. Regards, Dumitru
diff --git a/northd/northd.c b/northd/northd.c index 0cabda7ea..29dc58ef4 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -118,6 +118,7 @@ static bool default_acl_drop; #define REGBIT_PORT_SEC_DROP "reg0[15]" #define REGBIT_ACL_STATELESS "reg0[16]" #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" +#define REGBIT_FROM_ROUTER_PORT "reg0[18]" #define REG_ORIG_DIP_IPV4 "reg1" #define REG_ORIG_DIP_IPV6 "xxreg1" @@ -5785,6 +5786,13 @@ build_lswitch_port_sec_op(struct ovn_port *op, struct lflow_table *lflows, &op->od->localnet_ports[0]->nbsp->header_, op->lflow_ref); } + } else if (lsp_is_router(op->nbsp)) { + ds_put_format(actions, REGBIT_FROM_ROUTER_PORT" = 1; next;"); + ovn_lflow_add_with_lport_and_hint(lflows, op->od, + S_SWITCH_IN_CHECK_PORT_SEC, 70, + ds_cstr(match), ds_cstr(actions), + op->key, &op->nbsp->header_, + op->lflow_ref); } } @@ -9051,7 +9059,9 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, if (op->proxy_arp_addrs.n_ipv4_addrs) { /* Match rule on all proxy ARP IPs. */ ds_clear(match); - ds_put_cstr(match, "arp.op == 1 && arp.tpa == {"); + ds_put_cstr(match, + REGBIT_FROM_ROUTER_PORT" == 0 " + "&& arp.op == 1 && arp.tpa == {"); for (i = 0; i < op->proxy_arp_addrs.n_ipv4_addrs; i++) { ds_put_format(match, "%s/%u,", @@ -9105,7 +9115,8 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, ds_truncate(&nd_target_match, nd_target_match.length - 2); ds_clear(match); ds_put_format(match, - "nd_ns " + REGBIT_FROM_ROUTER_PORT" == 0 " + "&& nd_ns " "&& ip6.dst == { %s } " "&& nd.target == { %s }", ds_cstr(&ip6_dst_match), diff --git a/tests/ovn.at b/tests/ovn.at index 486680649..e419516a7 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32640,7 +32640,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "${arp_proxy_ls1[[1]]}" | ovn_strip_lflows], [0], [dnl - table=??(ls_in_arp_rsp ), priority=30 , match=(arp.op == 1 && dnl + table=??(ls_in_arp_rsp ), priority=30 , match=(reg0[[18]] == 0 && arp.op == 1 && dnl arp.tpa == {169.254.238.0/24,169.254.239.2/32}), dnl action=(eth.dst = eth.src; eth.src = 00:00:00:01:02:f1; arp.op = 2; dnl /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:01:02:f1; dnl @@ -32653,7 +32653,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep "${arp_proxy_ls1[[3]]}" | ovn_strip_lflows], [0], [dnl table=??(ls_in_arp_rsp ), priority=30 , dnl -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22d::/64, ff02::1:ff00:0/64, dnl fd7b:6b4d:7b25:d22f::1/128, ff02::1:ff00:1/128 } && dnl nd.target == { fd7b:6b4d:7b25:d22d::/64, fd7b:6b4d:7b25:d22f::1/128 }), dnl action=(nd_na_router { eth.src = 00:00:00:01:02:f1; ip6.src = nd.target; dnl @@ -32667,7 +32667,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep "${arp_proxy_ls2[[2]]}" | ovn_strip_lflows], [0], [dnl table=??(ls_in_arp_rsp ), priority=30 , dnl -match=(arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32}), dnl +match=(reg0[[18]] == 0 && arp.op == 1 && arp.tpa == {169.254.236.0/24,169.254.237.2/32}), dnl action=(eth.dst = eth.src; eth.src = 00:00:00:02:02:f1; arp.op = 2; dnl /* ARP reply */ arp.tha = arp.sha; arp.sha = 00:00:00:02:02:f1; dnl arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;) @@ -32679,7 +32679,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep "${arp_proxy_ls2[[4]]}" | ovn_strip_lflows], [0], [dnl table=??(ls_in_arp_rsp ), priority=30 , dnl -match=(nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl +match=(reg0[[18]] == 0 && nd_ns && ip6.dst == { fd7b:6b4d:7b25:d22b::/64, ff02::1:ff00:0/64, dnl fd7b:6b4d:7b25:d22c::1/128, ff02::1:ff00:1/128 } && dnl nd.target == { fd7b:6b4d:7b25:d22b::/64, fd7b:6b4d:7b25:d22c::1/128 }), dnl action=(nd_na_router { eth.src = 00:00:00:02:02:f1; ip6.src = nd.target; dnl diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 86fd240d2..e6cfb07f6 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10756,7 +10756,7 @@ check ovn-nbctl ls-add bar # Connect foo to R1 check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \ - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo addresses='"router"' + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254 169.254.239.2 169.254.238.0/24 192.168.1.100 192.168.1.200" options:router-port=foo addresses='"router"' # Connect bar to R1 check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 @@ -10785,6 +10785,12 @@ ADD_VETH(foo3, foo3, br-int, "192.168.1.4/24", "f0:00:00:01:02:05", \ check ovn-nbctl lsp-add foo foo3 \ -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4" +ADD_NAMESPACES(foo4) +ADD_VETH(foo4, foo4, br-int, "192.168.1.6/24", "f0:00:00:01:02:11", \ + "192.168.1.1") +check ovn-nbctl lsp-add foo foo4 \ +-- lsp-set-addresses foo4 "f0:00:00:01:02:11 192.168.1.6" + # Logical port 'ext1' in switch 'foo' ADD_NAMESPACES(ext1) ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", "f0:00:00:01:02:06", \ @@ -10800,6 +10806,12 @@ ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:07", \ check ovn-nbctl lsp-add bar bar1 \ -- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2" +ADD_NAMESPACES(bar2) +ADD_VETH(bar2, bar2, br-int, "192.168.2.3/24", "f0:00:00:01:02:10", \ +"192.168.2.1") +check ovn-nbctl lsp-add bar bar2 \ +-- lsp-set-addresses bar2 "f0:00:00:01:10:10 192.168.2.3" + # wait for ovn-controller to catch up. check ovn-nbctl --wait=hv sync @@ -10851,6 +10863,14 @@ OVS_WAIT_UNTIL([ test "${total_pkts}" = "3" ]) +check ovn-nbctl lr-route-add R1 169.254.240.0/24 192.168.1.200 +NETNS_START_TCPDUMP([foo4], [-nn -c 4 -e -i foo4 arp[[24:4]]=0xc0a801c8], [foo4-arp]) + +NS_CHECK_EXEC([bar2], [ping -q -c 5 -i 0.3 -w 2 169.254.240.10],[ignore],[ignore]) +OVS_WAIT_UNTIL([ + total_pkts=$(cat foo4-arp.tcpdump| wc -l) + test "${total_pkts}" = "4" +]) OVS_APP_EXIT_AND_WAIT([ovn-controller])
Skip proxy-arp logical flows for traffic that is entering the logical switch pipeline from a lsp of type router. This patch will avoid recirculating back the traffic entering by the logical router pipeline if proxy-arp hasn been configured by the CMS. Reported-at: https://issues.redhat.com/browse/FDP-96 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 15 +++++++++++++-- tests/ovn.at | 8 ++++---- tests/system-ovn.at | 22 +++++++++++++++++++++- 3 files changed, 38 insertions(+), 7 deletions(-)