diff mbox series

[ovs-dev] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.

Message ID 20210629160849.4130753-1-numans@ovn.org
State Superseded
Headers show
Series [ovs-dev] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot fail github build: failed

Commit Message

Numan Siddique June 29, 2021, 4:08 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

The commit [1] didn't add the ddlog part.

[1] - 8087cbc7462("ovn-northd.c: Add proxy ARP support to OVN")

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/ovn.dl        |  1 +
 northd/ovn.rs        | 13 +++++++++++++
 northd/ovn_northd.dl | 38 ++++++++++++++++++++++++++++++++++++++
 tests/ovn.at         |  4 ++--
 4 files changed, 54 insertions(+), 2 deletions(-)

Comments

0-day Robot June 29, 2021, 4:21 p.m. UTC | #1 
Bleep bloop.  Greetings Numan Siddique, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 85 characters long (recommended limit is 79)
#38 FILE: northd/ovn.rs:187:
pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {

WARNING: Line is 105 characters long (recommended limit is 79)
#57 FILE: northd/ovn.rs:638:
        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;

Lines checked: 134, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Numan Siddique June 29, 2021, 6:01 p.m. UTC | #2 
Hi Ben,

This patch is not working as expected.   Need your help here.  Not very urgent.

Please see below.

Thanks
Numan

On Tue, Jun 29, 2021 at 12:09 PM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> The commit [1] didn't add the ddlog part.
>
> [1] - 8087cbc7462("ovn-northd.c: Add proxy ARP support to OVN")
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/ovn.dl        |  1 +
>  northd/ovn.rs        | 13 +++++++++++++
>  northd/ovn_northd.dl | 38 ++++++++++++++++++++++++++++++++++++++
>  tests/ovn.at         |  4 ++--
>  4 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/northd/ovn.dl b/northd/ovn.dl
> index f23ea3b9e1..3c7a734ddb 100644
> --- a/northd/ovn.dl
> +++ b/northd/ovn.dl
> @@ -364,6 +364,7 @@ extern function is_dynamic_lsp_address(addr: string): bool
>  extern function extract_lsp_addresses(address: string): Option<lport_addresses>
>  extern function extract_addresses(address: string): Option<lport_addresses>
>  extern function extract_lrp_networks(mac: string, networks: Set<string>): Option<lport_addresses>
> +extern function extract_ip_addresses(address: string): Option<lport_addresses>
>
>  extern function split_addresses(addr: string): (Set<string>, Set<string>)
>
> diff --git a/northd/ovn.rs b/northd/ovn.rs
> index d44f83bc75..5f0939409c 100644
> --- a/northd/ovn.rs
> +++ b/northd/ovn.rs
> @@ -184,6 +184,18 @@ pub fn extract_lrp_networks(mac: &String, networks: &ddlog_std::Set<String>) ->
>      }
>  }
>
> +pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {
> +    unsafe {
> +        let mut laddrs: lport_addresses_c = Default::default();
> +        if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> +                                       &mut laddrs as *mut lport_addresses_c) {
> +            ddlog_std::Option::Some{x: laddrs.into_ddlog()}
> +        } else {
> +            ddlog_std::Option::None
> +        }
> +    }
> +}
> +
>  pub fn ovn_internal_version() -> String {
>      unsafe {
>          let s = ovn_c::ovn_get_internal_version();
> @@ -623,6 +635,7 @@ mod ovn_c {
>          pub fn extract_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c, ofs: *mut raw::c_int) -> bool;
>          pub fn extract_lrp_networks__(mac: *const raw::c_char, networks: *const *const raw::c_char,
>                                        n_networks: libc::size_t, laddrs: *mut lport_addresses_c) -> bool;
> +        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;
>          pub fn destroy_lport_addresses(addrs: *mut lport_addresses_c);
>          pub fn is_dynamic_lsp_address(address: *const raw::c_char) -> bool;
>          pub fn split_addresses(addresses: *const raw::c_char, ip4_addrs: *mut ovs_svec, ipv6_addrs: *mut ovs_svec);
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 52a6206a18..a7a327c7f0 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -3360,6 +3360,44 @@ for (CheckLspIsUp[check_lsp_is_up]) {
>      }
>  }
>
> +Flow(.logical_datapath = sw._uuid,
> +         .stage            = s_SWITCH_IN_ARP_ND_RSP(),
> +         .priority         = 50,
> +         .__match          = __match,
> +         .actions          = __actions,
> +         .external_ids     = stage_hint(sp.lsp._uuid)) :-
> +
> +    sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> +    rp.is_enabled(),
> +    var proxy_ips = {
> +        match (sp.lsp.options.get("arp_proxy")) {
> +            None -> "",
> +            Some {addresses} -> {
> +                match (extract_ip_addresses(addresses)) {
> +                    None -> "",
> +                    Some{addr} -> {
> +                        var ip4_addrs = vec_empty();
> +                        for (ip4 in addr.ipv4_addrs) {
> +                            ip4_addrs.push("${ip4.addr}")
> +                        };
> +                        string_join(ip4_addrs, ",")
> +                    }
> +                }
> +            }

If the - sp.lsp.option.get("arp_proxy") has 2 IP addresses -
"10.0.0.4, 10.0.0.5",  then the above
code sets only "10.0.0.4" into the variable  "proxy_ips".  I was
expecting it to have "10.0.0.4,10.0.0.5".

I'm not sure if the issue is in "extract_ip_addresses" or somewhere else.


> +        }
> +    },
> +    proxy_ips != "",
> +    var __match = "arp.op == 1 && arp.tpa == {" ++ proxy_ips ++ "}",
> +    var __actions = "eth.dst = eth.src; "
> +                    "eth.src = ${rp.networks.ea}; "
> +                    "arp.op = 2; /* ARP reply */ "
> +                    "arp.tha = arp.sha; "
> +                    "arp.sha = %s; "
> +                    "arp.tpa <-> arp.spa; "
> +                    "outport = inport; "
> +                    "flags.loopback = 1; "
> +                    "output;".
> +
>  /* For ND solicitations, we need to listen for both the
>   * unicast IPv6 address and its all-nodes multicast address,
>   * but always respond with the unicast IPv6 address. */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index db1a0a35c2..31f0b90996 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -26940,7 +26940,7 @@ ovs-vsctl -- add-port br-int vif1 -- \
>  # And proxy ARP flows for 69.254.239.254 and 169.254.239.2
>  # and check that SB flows have been added.
>  ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> -options arp_proxy='"169.254.239.254 169.254.239.2"'
> +options arp_proxy='"169.254.239.254,169.254.239.2"'
>  ovn-sbctl dump-flows > sbflows
>  AT_CAPTURE_FILE([sbflows])
>
> @@ -26957,7 +26957,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "169.254.239.2"], [1]
>
>  # Add the flows back send arp request and check we see an ARP response
>  ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> -options arp_proxy='"169.254.239.254 169.254.239.2"'
> +options arp_proxy='"169.254.239.254,169.254.239.2"'
>
>  ls1_p1_mac=00:00:00:01:02:03
>  ls1_p1_ip=192.16.1.6
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Brendan Doyle June 29, 2021, 6:28 p.m. UTC | #3 
Thanks for doing the ddlog for this, comment below

On 29/06/2021 17:08, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
>
> The commit [1] didn't add the ddlog part.
>
> [1] - 8087cbc7462("ovn-northd.c: Add proxy ARP support to OVN")
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   northd/ovn.dl        |  1 +
>   northd/ovn.rs        | 13 +++++++++++++
>   northd/ovn_northd.dl | 38 ++++++++++++++++++++++++++++++++++++++
>   tests/ovn.at         |  4 ++--
>   4 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/northd/ovn.dl b/northd/ovn.dl
> index f23ea3b9e1..3c7a734ddb 100644
> --- a/northd/ovn.dl
> +++ b/northd/ovn.dl
> @@ -364,6 +364,7 @@ extern function is_dynamic_lsp_address(addr: string): bool
>   extern function extract_lsp_addresses(address: string): Option<lport_addresses>
>   extern function extract_addresses(address: string): Option<lport_addresses>
>   extern function extract_lrp_networks(mac: string, networks: Set<string>): Option<lport_addresses>
> +extern function extract_ip_addresses(address: string): Option<lport_addresses>
>   
>   extern function split_addresses(addr: string): (Set<string>, Set<string>)
>   
> diff --git a/northd/ovn.rs b/northd/ovn.rs
> index d44f83bc75..5f0939409c 100644
> --- a/northd/ovn.rs
> +++ b/northd/ovn.rs
> @@ -184,6 +184,18 @@ pub fn extract_lrp_networks(mac: &String, networks: &ddlog_std::Set<String>) ->
>       }
>   }
>   
> +pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {
> +    unsafe {
> +        let mut laddrs: lport_addresses_c = Default::default();
> +        if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> +                                       &mut laddrs as *mut lport_addresses_c) {
> +            ddlog_std::Option::Some{x: laddrs.into_ddlog()}
> +        } else {
> +            ddlog_std::Option::None
> +        }
> +    }
> +}
> +
>   pub fn ovn_internal_version() -> String {
>       unsafe {
>           let s = ovn_c::ovn_get_internal_version();
> @@ -623,6 +635,7 @@ mod ovn_c {
>           pub fn extract_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c, ofs: *mut raw::c_int) -> bool;
>           pub fn extract_lrp_networks__(mac: *const raw::c_char, networks: *const *const raw::c_char,
>                                         n_networks: libc::size_t, laddrs: *mut lport_addresses_c) -> bool;
> +        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;
>           pub fn destroy_lport_addresses(addrs: *mut lport_addresses_c);
>           pub fn is_dynamic_lsp_address(address: *const raw::c_char) -> bool;
>           pub fn split_addresses(addresses: *const raw::c_char, ip4_addrs: *mut ovs_svec, ipv6_addrs: *mut ovs_svec);
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 52a6206a18..a7a327c7f0 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -3360,6 +3360,44 @@ for (CheckLspIsUp[check_lsp_is_up]) {
>       }
>   }
>   
> +Flow(.logical_datapath = sw._uuid,
> +         .stage            = s_SWITCH_IN_ARP_ND_RSP(),
> +         .priority         = 50,
> +         .__match          = __match,
> +         .actions          = __actions,
> +         .external_ids     = stage_hint(sp.lsp._uuid)) :-
> +
> +    sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> +    rp.is_enabled(),
> +    var proxy_ips = {
> +        match (sp.lsp.options.get("arp_proxy")) {
> +            None -> "",
> +            Some {addresses} -> {
> +                match (extract_ip_addresses(addresses)) {
> +                    None -> "",
> +                    Some{addr} -> {
> +                        var ip4_addrs = vec_empty();
> +                        for (ip4 in addr.ipv4_addrs) {
> +                            ip4_addrs.push("${ip4.addr}")
> +                        };
> +                        string_join(ip4_addrs, ",")
> +                    }
> +                }
> +            }
> +        }
> +    },
> +    proxy_ips != "",
> +    var __match = "arp.op == 1 && arp.tpa == {" ++ proxy_ips ++ "}",
> +    var __actions = "eth.dst = eth.src; "
> +                    "eth.src = ${rp.networks.ea}; "
> +                    "arp.op = 2; /* ARP reply */ "
> +                    "arp.tha = arp.sha; "
> +                    "arp.sha = %s; "
> +                    "arp.tpa <-> arp.spa; "
> +                    "outport = inport; "
> +                    "flags.loopback = 1; "
> +                    "output;".
> +
>   /* For ND solicitations, we need to listen for both the
>    * unicast IPv6 address and its all-nodes multicast address,
>    * but always respond with the unicast IPv6 address. */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index db1a0a35c2..31f0b90996 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -26940,7 +26940,7 @@ ovs-vsctl -- add-port br-int vif1 -- \
>   # And proxy ARP flows for 69.254.239.254 and 169.254.239.2
>   # and check that SB flows have been added.
>   ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> -options arp_proxy='"169.254.239.254 169.254.239.2"'
> +options arp_proxy='"169.254.239.254,169.254.239.2"'
>   ovn-sbctl dump-flows > sbflows
>   AT_CAPTURE_FILE([sbflows])
>   
> @@ -26957,7 +26957,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "169.254.239.2"], [1]
>   
>   # Add the flows back send arp request and check we see an ARP response
>   ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> -options arp_proxy='"169.254.239.254 169.254.239.2"'
> +options arp_proxy='"169.254.239.254,169.254.239.2"'
I think this change may break the non ddlog test. When I switched the 
northd.c code
to use 'extract_ip_addresses()', I found that it expected the address 
list to be of the form
"10.0.0.4 10.0.0.5" rather than comma separated. Hence I updated the 
test in ovn.at
to use a non comma separated address list.
>   
>   ls1_p1_mac=00:00:00:01:02:03
>   ls1_p1_ip=192.16.1.6
Numan Siddique June 29, 2021, 8:16 p.m. UTC | #4 
On Tue, Jun 29, 2021 at 2:29 PM Brendan Doyle <brendan.doyle@oracle.com> wrote:
>
>
> Thanks for doing the ddlog for this, comment below
>
> On 29/06/2021 17:08, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > The commit [1] didn't add the ddlog part.
> >
> > [1] - 8087cbc7462("ovn-northd.c: Add proxy ARP support to OVN")
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   northd/ovn.dl        |  1 +
> >   northd/ovn.rs        | 13 +++++++++++++
> >   northd/ovn_northd.dl | 38 ++++++++++++++++++++++++++++++++++++++
> >   tests/ovn.at         |  4 ++--
> >   4 files changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/northd/ovn.dl b/northd/ovn.dl
> > index f23ea3b9e1..3c7a734ddb 100644
> > --- a/northd/ovn.dl
> > +++ b/northd/ovn.dl
> > @@ -364,6 +364,7 @@ extern function is_dynamic_lsp_address(addr: string): bool
> >   extern function extract_lsp_addresses(address: string): Option<lport_addresses>
> >   extern function extract_addresses(address: string): Option<lport_addresses>
> >   extern function extract_lrp_networks(mac: string, networks: Set<string>): Option<lport_addresses>
> > +extern function extract_ip_addresses(address: string): Option<lport_addresses>
> >
> >   extern function split_addresses(addr: string): (Set<string>, Set<string>)
> >
> > diff --git a/northd/ovn.rs b/northd/ovn.rs
> > index d44f83bc75..5f0939409c 100644
> > --- a/northd/ovn.rs
> > +++ b/northd/ovn.rs
> > @@ -184,6 +184,18 @@ pub fn extract_lrp_networks(mac: &String, networks: &ddlog_std::Set<String>) ->
> >       }
> >   }
> >
> > +pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {
> > +    unsafe {
> > +        let mut laddrs: lport_addresses_c = Default::default();
> > +        if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> > +                                       &mut laddrs as *mut lport_addresses_c) {
> > +            ddlog_std::Option::Some{x: laddrs.into_ddlog()}
> > +        } else {
> > +            ddlog_std::Option::None
> > +        }
> > +    }
> > +}
> > +
> >   pub fn ovn_internal_version() -> String {
> >       unsafe {
> >           let s = ovn_c::ovn_get_internal_version();
> > @@ -623,6 +635,7 @@ mod ovn_c {
> >           pub fn extract_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c, ofs: *mut raw::c_int) -> bool;
> >           pub fn extract_lrp_networks__(mac: *const raw::c_char, networks: *const *const raw::c_char,
> >                                         n_networks: libc::size_t, laddrs: *mut lport_addresses_c) -> bool;
> > +        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;
> >           pub fn destroy_lport_addresses(addrs: *mut lport_addresses_c);
> >           pub fn is_dynamic_lsp_address(address: *const raw::c_char) -> bool;
> >           pub fn split_addresses(addresses: *const raw::c_char, ip4_addrs: *mut ovs_svec, ipv6_addrs: *mut ovs_svec);
> > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > index 52a6206a18..a7a327c7f0 100644
> > --- a/northd/ovn_northd.dl
> > +++ b/northd/ovn_northd.dl
> > @@ -3360,6 +3360,44 @@ for (CheckLspIsUp[check_lsp_is_up]) {
> >       }
> >   }
> >
> > +Flow(.logical_datapath = sw._uuid,
> > +         .stage            = s_SWITCH_IN_ARP_ND_RSP(),
> > +         .priority         = 50,
> > +         .__match          = __match,
> > +         .actions          = __actions,
> > +         .external_ids     = stage_hint(sp.lsp._uuid)) :-
> > +
> > +    sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> > +    rp.is_enabled(),
> > +    var proxy_ips = {
> > +        match (sp.lsp.options.get("arp_proxy")) {
> > +            None -> "",
> > +            Some {addresses} -> {
> > +                match (extract_ip_addresses(addresses)) {
> > +                    None -> "",
> > +                    Some{addr} -> {
> > +                        var ip4_addrs = vec_empty();
> > +                        for (ip4 in addr.ipv4_addrs) {
> > +                            ip4_addrs.push("${ip4.addr}")
> > +                        };
> > +                        string_join(ip4_addrs, ",")
> > +                    }
> > +                }
> > +            }
> > +        }
> > +    },
> > +    proxy_ips != "",
> > +    var __match = "arp.op == 1 && arp.tpa == {" ++ proxy_ips ++ "}",
> > +    var __actions = "eth.dst = eth.src; "
> > +                    "eth.src = ${rp.networks.ea}; "
> > +                    "arp.op = 2; /* ARP reply */ "
> > +                    "arp.tha = arp.sha; "
> > +                    "arp.sha = %s; "
> > +                    "arp.tpa <-> arp.spa; "
> > +                    "outport = inport; "
> > +                    "flags.loopback = 1; "
> > +                    "output;".
> > +
> >   /* For ND solicitations, we need to listen for both the
> >    * unicast IPv6 address and its all-nodes multicast address,
> >    * but always respond with the unicast IPv6 address. */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index db1a0a35c2..31f0b90996 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26940,7 +26940,7 @@ ovs-vsctl -- add-port br-int vif1 -- \
> >   # And proxy ARP flows for 69.254.239.254 and 169.254.239.2
> >   # and check that SB flows have been added.
> >   ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> > -options arp_proxy='"169.254.239.254 169.254.239.2"'
> > +options arp_proxy='"169.254.239.254,169.254.239.2"'
> >   ovn-sbctl dump-flows > sbflows
> >   AT_CAPTURE_FILE([sbflows])
> >
> > @@ -26957,7 +26957,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "169.254.239.2"], [1]
> >
> >   # Add the flows back send arp request and check we see an ARP response
> >   ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> > -options arp_proxy='"169.254.239.254 169.254.239.2"'
> > +options arp_proxy='"169.254.239.254,169.254.239.2"'
> I think this change may break the non ddlog test. When I switched the
> northd.c code
> to use 'extract_ip_addresses()', I found that it expected the address
> list to be of the form
> "10.0.0.4 10.0.0.5" rather than comma separated. Hence I updated the
> test in ovn.at
> to use a non comma separated address list.

Thanks.  I think  that's why I'm seeing the issue with this patch.

When I tested manually I used "," instead of space.   I'll test it out again.

Thanks
Numan


> >
> >   ls1_p1_mac=00:00:00:01:02:03
> >   ls1_p1_ip=192.16.1.6
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff July 2, 2021, 6:25 p.m. UTC | #5 
Did Brendan's feedback allow you to figure it out?  Otherwise, I will
look at it.

On Tue, Jun 29, 2021 at 02:01:19PM -0400, Numan Siddique wrote:
> Hi Ben,
> 
> This patch is not working as expected.   Need your help here.  Not very urgent.
> 
> Please see below.
> 
> Thanks
> Numan
> 
> On Tue, Jun 29, 2021 at 12:09 PM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@ovn.org>
> >
> > The commit [1] didn't add the ddlog part.
> >
> > [1] - 8087cbc7462("ovn-northd.c: Add proxy ARP support to OVN")
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  northd/ovn.dl        |  1 +
> >  northd/ovn.rs        | 13 +++++++++++++
> >  northd/ovn_northd.dl | 38 ++++++++++++++++++++++++++++++++++++++
> >  tests/ovn.at         |  4 ++--
> >  4 files changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/northd/ovn.dl b/northd/ovn.dl
> > index f23ea3b9e1..3c7a734ddb 100644
> > --- a/northd/ovn.dl
> > +++ b/northd/ovn.dl
> > @@ -364,6 +364,7 @@ extern function is_dynamic_lsp_address(addr: string): bool
> >  extern function extract_lsp_addresses(address: string): Option<lport_addresses>
> >  extern function extract_addresses(address: string): Option<lport_addresses>
> >  extern function extract_lrp_networks(mac: string, networks: Set<string>): Option<lport_addresses>
> > +extern function extract_ip_addresses(address: string): Option<lport_addresses>
> >
> >  extern function split_addresses(addr: string): (Set<string>, Set<string>)
> >
> > diff --git a/northd/ovn.rs b/northd/ovn.rs
> > index d44f83bc75..5f0939409c 100644
> > --- a/northd/ovn.rs
> > +++ b/northd/ovn.rs
> > @@ -184,6 +184,18 @@ pub fn extract_lrp_networks(mac: &String, networks: &ddlog_std::Set<String>) ->
> >      }
> >  }
> >
> > +pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {
> > +    unsafe {
> > +        let mut laddrs: lport_addresses_c = Default::default();
> > +        if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> > +                                       &mut laddrs as *mut lport_addresses_c) {
> > +            ddlog_std::Option::Some{x: laddrs.into_ddlog()}
> > +        } else {
> > +            ddlog_std::Option::None
> > +        }
> > +    }
> > +}
> > +
> >  pub fn ovn_internal_version() -> String {
> >      unsafe {
> >          let s = ovn_c::ovn_get_internal_version();
> > @@ -623,6 +635,7 @@ mod ovn_c {
> >          pub fn extract_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c, ofs: *mut raw::c_int) -> bool;
> >          pub fn extract_lrp_networks__(mac: *const raw::c_char, networks: *const *const raw::c_char,
> >                                        n_networks: libc::size_t, laddrs: *mut lport_addresses_c) -> bool;
> > +        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;
> >          pub fn destroy_lport_addresses(addrs: *mut lport_addresses_c);
> >          pub fn is_dynamic_lsp_address(address: *const raw::c_char) -> bool;
> >          pub fn split_addresses(addresses: *const raw::c_char, ip4_addrs: *mut ovs_svec, ipv6_addrs: *mut ovs_svec);
> > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > index 52a6206a18..a7a327c7f0 100644
> > --- a/northd/ovn_northd.dl
> > +++ b/northd/ovn_northd.dl
> > @@ -3360,6 +3360,44 @@ for (CheckLspIsUp[check_lsp_is_up]) {
> >      }
> >  }
> >
> > +Flow(.logical_datapath = sw._uuid,
> > +         .stage            = s_SWITCH_IN_ARP_ND_RSP(),
> > +         .priority         = 50,
> > +         .__match          = __match,
> > +         .actions          = __actions,
> > +         .external_ids     = stage_hint(sp.lsp._uuid)) :-
> > +
> > +    sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> > +    rp.is_enabled(),
> > +    var proxy_ips = {
> > +        match (sp.lsp.options.get("arp_proxy")) {
> > +            None -> "",
> > +            Some {addresses} -> {
> > +                match (extract_ip_addresses(addresses)) {
> > +                    None -> "",
> > +                    Some{addr} -> {
> > +                        var ip4_addrs = vec_empty();
> > +                        for (ip4 in addr.ipv4_addrs) {
> > +                            ip4_addrs.push("${ip4.addr}")
> > +                        };
> > +                        string_join(ip4_addrs, ",")
> > +                    }
> > +                }
> > +            }
> 
> If the - sp.lsp.option.get("arp_proxy") has 2 IP addresses -
> "10.0.0.4, 10.0.0.5",  then the above
> code sets only "10.0.0.4" into the variable  "proxy_ips".  I was
> expecting it to have "10.0.0.4,10.0.0.5".
> 
> I'm not sure if the issue is in "extract_ip_addresses" or somewhere else.
> 
> 
> > +        }
> > +    },
> > +    proxy_ips != "",
> > +    var __match = "arp.op == 1 && arp.tpa == {" ++ proxy_ips ++ "}",
> > +    var __actions = "eth.dst = eth.src; "
> > +                    "eth.src = ${rp.networks.ea}; "
> > +                    "arp.op = 2; /* ARP reply */ "
> > +                    "arp.tha = arp.sha; "
> > +                    "arp.sha = %s; "
> > +                    "arp.tpa <-> arp.spa; "
> > +                    "outport = inport; "
> > +                    "flags.loopback = 1; "
> > +                    "output;".
> > +
> >  /* For ND solicitations, we need to listen for both the
> >   * unicast IPv6 address and its all-nodes multicast address,
> >   * but always respond with the unicast IPv6 address. */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index db1a0a35c2..31f0b90996 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26940,7 +26940,7 @@ ovs-vsctl -- add-port br-int vif1 -- \
> >  # And proxy ARP flows for 69.254.239.254 and 169.254.239.2
> >  # and check that SB flows have been added.
> >  ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> > -options arp_proxy='"169.254.239.254 169.254.239.2"'
> > +options arp_proxy='"169.254.239.254,169.254.239.2"'
> >  ovn-sbctl dump-flows > sbflows
> >  AT_CAPTURE_FILE([sbflows])
> >
> > @@ -26957,7 +26957,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "169.254.239.2"], [1]
> >
> >  # Add the flows back send arp request and check we see an ARP response
> >  ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> > -options arp_proxy='"169.254.239.254 169.254.239.2"'
> > +options arp_proxy='"169.254.239.254,169.254.239.2"'
> >
> >  ls1_p1_mac=00:00:00:01:02:03
> >  ls1_p1_ip=192.16.1.6
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
Numan Siddique July 2, 2021, 6:28 p.m. UTC | #6 
On Fri, Jul 2, 2021 at 2:26 PM Ben Pfaff <blp@ovn.org> wrote:
>
> Did Brendan's feedback allow you to figure it out?  Otherwise, I will
> look at it.

Yes.  You tested and acked the v2 of this patch.  The patch is merged now.

The test case should use space separated ip addresses instead of commas, since
extract_ip_addresses() doesn't support comma separated strings of ip addresses.

Thanks
Numan

>
> On Tue, Jun 29, 2021 at 02:01:19PM -0400, Numan Siddique wrote:
> > Hi Ben,
> >
> > This patch is not working as expected.   Need your help here.  Not very urgent.
> >
> > Please see below.
> >
> > Thanks
> > Numan
> >
> > On Tue, Jun 29, 2021 at 12:09 PM <numans@ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans@ovn.org>
> > >
> > > The commit [1] didn't add the ddlog part.
> > >
> > > [1] - 8087cbc7462("ovn-northd.c: Add proxy ARP support to OVN")
> > >
> > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > ---
> > >  northd/ovn.dl        |  1 +
> > >  northd/ovn.rs        | 13 +++++++++++++
> > >  northd/ovn_northd.dl | 38 ++++++++++++++++++++++++++++++++++++++
> > >  tests/ovn.at         |  4 ++--
> > >  4 files changed, 54 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/northd/ovn.dl b/northd/ovn.dl
> > > index f23ea3b9e1..3c7a734ddb 100644
> > > --- a/northd/ovn.dl
> > > +++ b/northd/ovn.dl
> > > @@ -364,6 +364,7 @@ extern function is_dynamic_lsp_address(addr: string): bool
> > >  extern function extract_lsp_addresses(address: string): Option<lport_addresses>
> > >  extern function extract_addresses(address: string): Option<lport_addresses>
> > >  extern function extract_lrp_networks(mac: string, networks: Set<string>): Option<lport_addresses>
> > > +extern function extract_ip_addresses(address: string): Option<lport_addresses>
> > >
> > >  extern function split_addresses(addr: string): (Set<string>, Set<string>)
> > >
> > > diff --git a/northd/ovn.rs b/northd/ovn.rs
> > > index d44f83bc75..5f0939409c 100644
> > > --- a/northd/ovn.rs
> > > +++ b/northd/ovn.rs
> > > @@ -184,6 +184,18 @@ pub fn extract_lrp_networks(mac: &String, networks: &ddlog_std::Set<String>) ->
> > >      }
> > >  }
> > >
> > > +pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {
> > > +    unsafe {
> > > +        let mut laddrs: lport_addresses_c = Default::default();
> > > +        if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> > > +                                       &mut laddrs as *mut lport_addresses_c) {
> > > +            ddlog_std::Option::Some{x: laddrs.into_ddlog()}
> > > +        } else {
> > > +            ddlog_std::Option::None
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  pub fn ovn_internal_version() -> String {
> > >      unsafe {
> > >          let s = ovn_c::ovn_get_internal_version();
> > > @@ -623,6 +635,7 @@ mod ovn_c {
> > >          pub fn extract_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c, ofs: *mut raw::c_int) -> bool;
> > >          pub fn extract_lrp_networks__(mac: *const raw::c_char, networks: *const *const raw::c_char,
> > >                                        n_networks: libc::size_t, laddrs: *mut lport_addresses_c) -> bool;
> > > +        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;
> > >          pub fn destroy_lport_addresses(addrs: *mut lport_addresses_c);
> > >          pub fn is_dynamic_lsp_address(address: *const raw::c_char) -> bool;
> > >          pub fn split_addresses(addresses: *const raw::c_char, ip4_addrs: *mut ovs_svec, ipv6_addrs: *mut ovs_svec);
> > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > > index 52a6206a18..a7a327c7f0 100644
> > > --- a/northd/ovn_northd.dl
> > > +++ b/northd/ovn_northd.dl
> > > @@ -3360,6 +3360,44 @@ for (CheckLspIsUp[check_lsp_is_up]) {
> > >      }
> > >  }
> > >
> > > +Flow(.logical_datapath = sw._uuid,
> > > +         .stage            = s_SWITCH_IN_ARP_ND_RSP(),
> > > +         .priority         = 50,
> > > +         .__match          = __match,
> > > +         .actions          = __actions,
> > > +         .external_ids     = stage_hint(sp.lsp._uuid)) :-
> > > +
> > > +    sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> > > +    rp.is_enabled(),
> > > +    var proxy_ips = {
> > > +        match (sp.lsp.options.get("arp_proxy")) {
> > > +            None -> "",
> > > +            Some {addresses} -> {
> > > +                match (extract_ip_addresses(addresses)) {
> > > +                    None -> "",
> > > +                    Some{addr} -> {
> > > +                        var ip4_addrs = vec_empty();
> > > +                        for (ip4 in addr.ipv4_addrs) {
> > > +                            ip4_addrs.push("${ip4.addr}")
> > > +                        };
> > > +                        string_join(ip4_addrs, ",")
> > > +                    }
> > > +                }
> > > +            }
> >
> > If the - sp.lsp.option.get("arp_proxy") has 2 IP addresses -
> > "10.0.0.4, 10.0.0.5",  then the above
> > code sets only "10.0.0.4" into the variable  "proxy_ips".  I was
> > expecting it to have "10.0.0.4,10.0.0.5".
> >
> > I'm not sure if the issue is in "extract_ip_addresses" or somewhere else.
> >
> >
> > > +        }
> > > +    },
> > > +    proxy_ips != "",
> > > +    var __match = "arp.op == 1 && arp.tpa == {" ++ proxy_ips ++ "}",
> > > +    var __actions = "eth.dst = eth.src; "
> > > +                    "eth.src = ${rp.networks.ea}; "
> > > +                    "arp.op = 2; /* ARP reply */ "
> > > +                    "arp.tha = arp.sha; "
> > > +                    "arp.sha = %s; "
> > > +                    "arp.tpa <-> arp.spa; "
> > > +                    "outport = inport; "
> > > +                    "flags.loopback = 1; "
> > > +                    "output;".
> > > +
> > >  /* For ND solicitations, we need to listen for both the
> > >   * unicast IPv6 address and its all-nodes multicast address,
> > >   * but always respond with the unicast IPv6 address. */
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index db1a0a35c2..31f0b90996 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -26940,7 +26940,7 @@ ovs-vsctl -- add-port br-int vif1 -- \
> > >  # And proxy ARP flows for 69.254.239.254 and 169.254.239.2
> > >  # and check that SB flows have been added.
> > >  ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> > > -options arp_proxy='"169.254.239.254 169.254.239.2"'
> > > +options arp_proxy='"169.254.239.254,169.254.239.2"'
> > >  ovn-sbctl dump-flows > sbflows
> > >  AT_CAPTURE_FILE([sbflows])
> > >
> > > @@ -26957,7 +26957,7 @@ AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "169.254.239.2"], [1]
> > >
> > >  # Add the flows back send arp request and check we see an ARP response
> > >  ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
> > > -options arp_proxy='"169.254.239.254 169.254.239.2"'
> > > +options arp_proxy='"169.254.239.254,169.254.239.2"'
> > >
> > >  ls1_p1_mac=00:00:00:01:02:03
> > >  ls1_p1_ip=192.16.1.6
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff July 2, 2021, 6:47 p.m. UTC | #7 
On Fri, Jul 02, 2021 at 02:28:23PM -0400, Numan Siddique wrote:
> On Fri, Jul 2, 2021 at 2:26 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > Did Brendan's feedback allow you to figure it out?  Otherwise, I will
> > look at it.
> 
> Yes.  You tested and acked the v2 of this patch.  The patch is merged now.

Oh, sorry, I'm going back through my inbox and I forget stuff sometimes
:-)
diff mbox series

Patch

diff --git a/northd/ovn.dl b/northd/ovn.dl
index f23ea3b9e1..3c7a734ddb 100644
--- a/northd/ovn.dl
+++ b/northd/ovn.dl
@@ -364,6 +364,7 @@  extern function is_dynamic_lsp_address(addr: string): bool
 extern function extract_lsp_addresses(address: string): Option<lport_addresses>
 extern function extract_addresses(address: string): Option<lport_addresses>
 extern function extract_lrp_networks(mac: string, networks: Set<string>): Option<lport_addresses>
+extern function extract_ip_addresses(address: string): Option<lport_addresses>
 
 extern function split_addresses(addr: string): (Set<string>, Set<string>)
 
diff --git a/northd/ovn.rs b/northd/ovn.rs
index d44f83bc75..5f0939409c 100644
--- a/northd/ovn.rs
+++ b/northd/ovn.rs
@@ -184,6 +184,18 @@  pub fn extract_lrp_networks(mac: &String, networks: &ddlog_std::Set<String>) ->
     }
 }
 
+pub fn extract_ip_addresses(address: &String) -> ddlog_std::Option<lport_addresses> {
+    unsafe {
+        let mut laddrs: lport_addresses_c = Default::default();
+        if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
+                                       &mut laddrs as *mut lport_addresses_c) {
+            ddlog_std::Option::Some{x: laddrs.into_ddlog()}
+        } else {
+            ddlog_std::Option::None
+        }
+    }
+}
+
 pub fn ovn_internal_version() -> String {
     unsafe {
         let s = ovn_c::ovn_get_internal_version();
@@ -623,6 +635,7 @@  mod ovn_c {
         pub fn extract_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c, ofs: *mut raw::c_int) -> bool;
         pub fn extract_lrp_networks__(mac: *const raw::c_char, networks: *const *const raw::c_char,
                                       n_networks: libc::size_t, laddrs: *mut lport_addresses_c) -> bool;
+        pub fn extract_ip_addresses(address: *const raw::c_char, laddrs: *mut lport_addresses_c) -> bool;
         pub fn destroy_lport_addresses(addrs: *mut lport_addresses_c);
         pub fn is_dynamic_lsp_address(address: *const raw::c_char) -> bool;
         pub fn split_addresses(addresses: *const raw::c_char, ip4_addrs: *mut ovs_svec, ipv6_addrs: *mut ovs_svec);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 52a6206a18..a7a327c7f0 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -3360,6 +3360,44 @@  for (CheckLspIsUp[check_lsp_is_up]) {
     }
 }
 
+Flow(.logical_datapath = sw._uuid,
+         .stage            = s_SWITCH_IN_ARP_ND_RSP(),
+         .priority         = 50,
+         .__match          = __match,
+         .actions          = __actions,
+         .external_ids     = stage_hint(sp.lsp._uuid)) :-
+
+    sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
+    rp.is_enabled(),
+    var proxy_ips = {
+        match (sp.lsp.options.get("arp_proxy")) {
+            None -> "",
+            Some {addresses} -> {
+                match (extract_ip_addresses(addresses)) {
+                    None -> "",
+                    Some{addr} -> {
+                        var ip4_addrs = vec_empty();
+                        for (ip4 in addr.ipv4_addrs) {
+                            ip4_addrs.push("${ip4.addr}")
+                        };
+                        string_join(ip4_addrs, ",")
+                    }
+                }
+            }
+        }
+    },
+    proxy_ips != "",
+    var __match = "arp.op == 1 && arp.tpa == {" ++ proxy_ips ++ "}",
+    var __actions = "eth.dst = eth.src; "
+                    "eth.src = ${rp.networks.ea}; "
+                    "arp.op = 2; /* ARP reply */ "
+                    "arp.tha = arp.sha; "
+                    "arp.sha = %s; "
+                    "arp.tpa <-> arp.spa; "
+                    "outport = inport; "
+                    "flags.loopback = 1; "
+                    "output;".
+
 /* For ND solicitations, we need to listen for both the
  * unicast IPv6 address and its all-nodes multicast address,
  * but always respond with the unicast IPv6 address. */
diff --git a/tests/ovn.at b/tests/ovn.at
index db1a0a35c2..31f0b90996 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -26940,7 +26940,7 @@  ovs-vsctl -- add-port br-int vif1 -- \
 # And proxy ARP flows for 69.254.239.254 and 169.254.239.2
 # and check that SB flows have been added.
 ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
-options arp_proxy='"169.254.239.254 169.254.239.2"'
+options arp_proxy='"169.254.239.254,169.254.239.2"'
 ovn-sbctl dump-flows > sbflows
 AT_CAPTURE_FILE([sbflows])
 
@@ -26957,7 +26957,7 @@  AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep "169.254.239.2"], [1]
 
 # Add the flows back send arp request and check we see an ARP response
 ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
-options arp_proxy='"169.254.239.254 169.254.239.2"'
+options arp_proxy='"169.254.239.254,169.254.239.2"'
 
 ls1_p1_mac=00:00:00:01:02:03
 ls1_p1_ip=192.16.1.6