diff mbox series

[ovs-dev] northd: Skip arp-proxy flows if the lsp is a router port.

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

Checks

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

Commit Message

Lorenzo Bianconi May 14, 2024, 4:44 p.m. UTC
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(-)

Comments

Dumitru Ceara June 5, 2024, 10:06 a.m. UTC | #1
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])
>
Lorenzo Bianconi June 5, 2024, 10:11 a.m. UTC | #2
> 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])
> >  
>
Enrique Llorente Pastora June 5, 2024, 12:03 p.m. UTC | #3
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])
> > >
> >
>
Brendan Doyle via dev June 5, 2024, 2:52 p.m. UTC | #4
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])
>>>>
>
Dumitru Ceara June 6, 2024, 4:34 p.m. UTC | #5
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
Brendan Doyle via dev June 7, 2024, 10:24 a.m. UTC | #6
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$
Dumitru Ceara June 7, 2024, 1:20 p.m. UTC | #7
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$
>
Brendan Doyle via dev June 7, 2024, 1:26 p.m. UTC | #8
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$
Dumitru Ceara June 7, 2024, 1:28 p.m. UTC | #9
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!
Dumitru Ceara June 7, 2024, 3:25 p.m. UTC | #10
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 mbox series

Patch

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])