Re: [ovs-dev] [PATCH ovn] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.
On Fri, Jul 02, 2021 at 02:28:23PM -0400, Numan Siddique wrote: > On Fri, Jul 2, 2021 at 2:26 PM Ben Pfaff 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 :-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.
On Fri, Jul 2, 2021 at 2:26 PM Ben Pfaff 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 wrote:
> > >
> > > From: Numan Siddique
> > >
> > > 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
> > > ---
> > > 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
> > > extern function extract_addresses(address: string):
> > > Option
> > > extern function extract_lrp_networks(mac: string, networks:
> > > Set): Option
> > > +extern function extract_ip_addresses(address: string):
> > > Option
> > >
> > > extern function split_addresses(addr: string): (Set, Set)
> > >
> > > 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: , networks:
> > > _std::Set) ->
> > > }
> > > }
> > >
> > > +pub fn extract_ip_addresses(address: ) ->
> > > ddlog_std::Option {
> > > +unsafe {
> > > +let mut laddrs: lport_addresses_c = Default::default();
> > > +if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> > > +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 (.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
Re: [ovs-dev] [PATCH ovn] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.
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 wrote:
> >
> > From: Numan Siddique
> >
> > 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
> > ---
> > 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
> > extern function extract_addresses(address: string): Option
> > extern function extract_lrp_networks(mac: string, networks: Set):
> > Option
> > +extern function extract_ip_addresses(address: string):
> > Option
> >
> > extern function split_addresses(addr: string): (Set, Set)
> >
> > 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: , networks:
> > _std::Set) ->
> > }
> > }
> >
> > +pub fn extract_ip_addresses(address: ) ->
> > ddlog_std::Option {
> > +unsafe {
> > +let mut laddrs: lport_addresses_c = Default::default();
> > +if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> > +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 (.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; "
> > +
Re: [ovs-dev] [PATCH ovn] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.
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 wrote:
>
> From: Numan Siddique
>
> 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
> ---
> 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
> extern function extract_addresses(address: string): Option
> extern function extract_lrp_networks(mac: string, networks: Set):
> Option
> +extern function extract_ip_addresses(address: string):
> Option
>
> extern function split_addresses(addr: string): (Set, Set)
>
> 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: , networks:
> _std::Set) ->
> }
> }
>
> +pub fn extract_ip_addresses(address: ) ->
> ddlog_std::Option {
> +unsafe {
> +let mut laddrs: lport_addresses_c = Default::default();
> +if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
> +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 (.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
> +++
Re: [ovs-dev] [PATCH ovn] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.
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: ) ->
ddlog_std::Option {
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
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd-ddlog: Add proxy arp flows for configured addresses in lsp router port.
From: Numan Siddique
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
---
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
extern function extract_addresses(address: string): Option
extern function extract_lrp_networks(mac: string, networks: Set):
Option
+extern function extract_ip_addresses(address: string): Option
extern function split_addresses(addr: string): (Set, Set)
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: , networks:
_std::Set) ->
}
}
+pub fn extract_ip_addresses(address: ) ->
ddlog_std::Option {
+unsafe {
+let mut laddrs: lport_addresses_c = Default::default();
+if ovn_c::extract_ip_addresses(string2cstr(address).as_ptr(),
+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 (.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"'
