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 |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/github-robot | fail | github build: failed |
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
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 >
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
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 >
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 > >
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 >
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 --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