Re: [ovs-dev] [PATCH ovn v3] ovn-controller: Add 'local_ip' option to tunnel ports for IPsec case

2021-03-25 Thread Mark Gray
On 24/03/2021 11:12, Numan Siddique wrote:
> On Tue, Mar 16, 2021 at 2:32 AM Mark Michelson  wrote:
>>
>> LGMT
>>
>> Acked-by: Mark Michelson 
> 
> Thank you Mark G (and Mark M for the reviews).
> 
> I applied this patch to master.
> 
> Numan
> 
Thank everyone.

>>
>> On 2/16/21 6:55 AM, Mark Gray wrote:
>>> If a chassis has multiple interfaces, 'ovn-encap-ip' can be used
>>> to specify the IP address of the interface that is used for tunnel
>>> traffic. OVN uses that IP address to configure the 'remote_ip' of
>>> a tunnel port. OVS tunnel ports also accept 'options:local_ip', which,
>>> according to the OVS documentation specifies "the tunnel destination
>>> IP that received packets must match. Default is to match all addresses".
>>> OVN does not set 'local_ip'.
>>>
>>> 'ovs-monitor-ipsec' is an OVS daemon that is used to configure and IPsec
>>> IKE daemon on the host. In order to correctly specify an IPsec
>>> connection, it requires the source and destination IP address of
>>> that connection. In the OVN case, as 'local_ip' is not specified, it
>>> is unable to infer the IP address of both sides of a tunnel and, therefore,
>>> cannot setup an IPsec connection.
>>>
>>> This patch configures 'local_ip' on tunnel ports when IPsec has
>>> been enabled. This allows for OVS/OVN IPsec to work when 'ovn-encap-ip'
>>> is not specified as the chassis default gateway interface.
>>>
>>> This patch also adds some unit tests. The OVS daemon 'ovs-monitor-ipsec'
>>> requires a number of options to be configured on OVS tunnel ports in order
>>> to function correctly. These unit tests ensure that these options are
>>> configured correctly when IPsec has been enabled through the northbound
>>> database.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1924041
>>> Signed-off-by: Mark Gray 
>>> ---
>>>
>>> v2: Updated topic filter to "PATCH ovn"
>>> v3: Rebased due to 0-day bot warning
>>>
>>>   controller/chassis.c |   5 +++
>>>   controller/encaps.c  |  26 ++-
>>>   tests/automake.mk|   3 +-
>>>   tests/ovn-ipsec.at   | 104 +++
>>>   tests/testsuite.at   |   1 +
>>>   5 files changed, 137 insertions(+), 2 deletions(-)
>>>   create mode 100644 tests/ovn-ipsec.at
>>>
>>> diff --git a/controller/chassis.c b/controller/chassis.c
>>> index 310132d09d2e..9b0a36cf076f 100644
>>> --- a/controller/chassis.c
>>> +++ b/controller/chassis.c
>>> @@ -279,6 +279,11 @@ chassis_parse_ovs_config(const struct 
>>> ovsrec_open_vswitch_table *ovs_table,
>>>   return false;
>>>   }
>>>
>>> +/* 'ovn-encap-ip' can accept a comma-delimited list of IP addresses 
>>> instead
>>> + * of a single IP address. Although this is undocumented, it can be 
>>> used
>>> + * to enable certain hardware-offloaded use cases in which a host has
>>> + * multiple NICs and is assigning SR-IOV VFs to a guest (as logical 
>>> ports).
>>> + */
>>>   if (!chassis_parse_ovs_encap_ip(encap_ips, _cfg->encap_ip_set)) {
>>>   sset_destroy(_cfg->encap_type_set);
>>>   return false;
>>> diff --git a/controller/encaps.c b/controller/encaps.c
>>> index 7eac4bb064ac..fc93bf1eeb87 100644
>>> --- a/controller/encaps.c
>>> +++ b/controller/encaps.c
>>> @@ -59,6 +59,7 @@ struct tunnel_ctx {
>>>
>>>   struct ovsdb_idl_txn *ovs_txn;
>>>   const struct ovsrec_bridge *br_int;
>>> +const struct sbrec_chassis *this_chassis;
>>>   };
>>>
>>>   struct chassis_node {
>>> @@ -176,6 +177,28 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
>>> sbrec_sb_global *sbg,
>>>
>>>   /* Add auth info if ipsec is enabled. */
>>>   if (sbg->ipsec) {
>>> +const struct sbrec_chassis *this_chassis = tc->this_chassis;
>>> +const char *local_ip = NULL;
>>> +
>>> +/* Determine 'ovn-encap-ip' of the local chassis as this will be 
>>> the
>>> + * tunnel port's 'local_ip'. We do not support the case in which
>>> + * 'ovn-encap-ip' holds multiple comma-delimited IP addresses.
>>> + */
>>> +for (int i = 0; i < this_chassis->n_encaps; i++) {
>>> +if (local_ip && strcmp(local_ip, this_chassis->encaps[i]->ip)) 
>>> {
>>> +VLOG_ERR("ovn-encap-ip has been configured as a list. This 
>>> "
>>> + "is unsupported for IPsec.");
>>> +/* No need to loop further as we know this condition has 
>>> been
>>> + * hit */
>>> +break;
>>> +} else {
>>> +local_ip = this_chassis->encaps[i]->ip;
>>> +}
>>> +}
>>> +
>>> +if (local_ip) {
>>> +smap_add(, "local_ip", local_ip);
>>> +}
>>>   smap_add(, "remote_name", new_chassis_id);
>>>   }
>>>
>>> @@ -310,7 +333,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
>>>   struct tunnel_ctx tc = {
>>>   .chassis = SHASH_INITIALIZER(),
>>>   .port_names = SSET_INITIALIZER(_names),
>>> -

Re: [ovs-dev] [PATCH ovn v3] ovn-controller: Add 'local_ip' option to tunnel ports for IPsec case

2021-03-24 Thread Numan Siddique
On Tue, Mar 16, 2021 at 2:32 AM Mark Michelson  wrote:
>
> LGMT
>
> Acked-by: Mark Michelson 

Thank you Mark G (and Mark M for the reviews).

I applied this patch to master.

Numan

>
> On 2/16/21 6:55 AM, Mark Gray wrote:
> > If a chassis has multiple interfaces, 'ovn-encap-ip' can be used
> > to specify the IP address of the interface that is used for tunnel
> > traffic. OVN uses that IP address to configure the 'remote_ip' of
> > a tunnel port. OVS tunnel ports also accept 'options:local_ip', which,
> > according to the OVS documentation specifies "the tunnel destination
> > IP that received packets must match. Default is to match all addresses".
> > OVN does not set 'local_ip'.
> >
> > 'ovs-monitor-ipsec' is an OVS daemon that is used to configure and IPsec
> > IKE daemon on the host. In order to correctly specify an IPsec
> > connection, it requires the source and destination IP address of
> > that connection. In the OVN case, as 'local_ip' is not specified, it
> > is unable to infer the IP address of both sides of a tunnel and, therefore,
> > cannot setup an IPsec connection.
> >
> > This patch configures 'local_ip' on tunnel ports when IPsec has
> > been enabled. This allows for OVS/OVN IPsec to work when 'ovn-encap-ip'
> > is not specified as the chassis default gateway interface.
> >
> > This patch also adds some unit tests. The OVS daemon 'ovs-monitor-ipsec'
> > requires a number of options to be configured on OVS tunnel ports in order
> > to function correctly. These unit tests ensure that these options are
> > configured correctly when IPsec has been enabled through the northbound
> > database.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1924041
> > Signed-off-by: Mark Gray 
> > ---
> >
> > v2: Updated topic filter to "PATCH ovn"
> > v3: Rebased due to 0-day bot warning
> >
> >   controller/chassis.c |   5 +++
> >   controller/encaps.c  |  26 ++-
> >   tests/automake.mk|   3 +-
> >   tests/ovn-ipsec.at   | 104 +++
> >   tests/testsuite.at   |   1 +
> >   5 files changed, 137 insertions(+), 2 deletions(-)
> >   create mode 100644 tests/ovn-ipsec.at
> >
> > diff --git a/controller/chassis.c b/controller/chassis.c
> > index 310132d09d2e..9b0a36cf076f 100644
> > --- a/controller/chassis.c
> > +++ b/controller/chassis.c
> > @@ -279,6 +279,11 @@ chassis_parse_ovs_config(const struct 
> > ovsrec_open_vswitch_table *ovs_table,
> >   return false;
> >   }
> >
> > +/* 'ovn-encap-ip' can accept a comma-delimited list of IP addresses 
> > instead
> > + * of a single IP address. Although this is undocumented, it can be 
> > used
> > + * to enable certain hardware-offloaded use cases in which a host has
> > + * multiple NICs and is assigning SR-IOV VFs to a guest (as logical 
> > ports).
> > + */
> >   if (!chassis_parse_ovs_encap_ip(encap_ips, _cfg->encap_ip_set)) {
> >   sset_destroy(_cfg->encap_type_set);
> >   return false;
> > diff --git a/controller/encaps.c b/controller/encaps.c
> > index 7eac4bb064ac..fc93bf1eeb87 100644
> > --- a/controller/encaps.c
> > +++ b/controller/encaps.c
> > @@ -59,6 +59,7 @@ struct tunnel_ctx {
> >
> >   struct ovsdb_idl_txn *ovs_txn;
> >   const struct ovsrec_bridge *br_int;
> > +const struct sbrec_chassis *this_chassis;
> >   };
> >
> >   struct chassis_node {
> > @@ -176,6 +177,28 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
> > sbrec_sb_global *sbg,
> >
> >   /* Add auth info if ipsec is enabled. */
> >   if (sbg->ipsec) {
> > +const struct sbrec_chassis *this_chassis = tc->this_chassis;
> > +const char *local_ip = NULL;
> > +
> > +/* Determine 'ovn-encap-ip' of the local chassis as this will be 
> > the
> > + * tunnel port's 'local_ip'. We do not support the case in which
> > + * 'ovn-encap-ip' holds multiple comma-delimited IP addresses.
> > + */
> > +for (int i = 0; i < this_chassis->n_encaps; i++) {
> > +if (local_ip && strcmp(local_ip, this_chassis->encaps[i]->ip)) 
> > {
> > +VLOG_ERR("ovn-encap-ip has been configured as a list. This 
> > "
> > + "is unsupported for IPsec.");
> > +/* No need to loop further as we know this condition has 
> > been
> > + * hit */
> > +break;
> > +} else {
> > +local_ip = this_chassis->encaps[i]->ip;
> > +}
> > +}
> > +
> > +if (local_ip) {
> > +smap_add(, "local_ip", local_ip);
> > +}
> >   smap_add(, "remote_name", new_chassis_id);
> >   }
> >
> > @@ -310,7 +333,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> >   struct tunnel_ctx tc = {
> >   .chassis = SHASH_INITIALIZER(),
> >   .port_names = SSET_INITIALIZER(_names),
> > -.br_int = br_int
> > +.br_int = br_int,
> > +.this_chassis = 

Re: [ovs-dev] [PATCH ovn v3] ovn-controller: Add 'local_ip' option to tunnel ports for IPsec case

2021-03-15 Thread Mark Michelson

LGMT

Acked-by: Mark Michelson 

On 2/16/21 6:55 AM, Mark Gray wrote:

If a chassis has multiple interfaces, 'ovn-encap-ip' can be used
to specify the IP address of the interface that is used for tunnel
traffic. OVN uses that IP address to configure the 'remote_ip' of
a tunnel port. OVS tunnel ports also accept 'options:local_ip', which,
according to the OVS documentation specifies "the tunnel destination
IP that received packets must match. Default is to match all addresses".
OVN does not set 'local_ip'.

'ovs-monitor-ipsec' is an OVS daemon that is used to configure and IPsec
IKE daemon on the host. In order to correctly specify an IPsec
connection, it requires the source and destination IP address of
that connection. In the OVN case, as 'local_ip' is not specified, it
is unable to infer the IP address of both sides of a tunnel and, therefore,
cannot setup an IPsec connection.

This patch configures 'local_ip' on tunnel ports when IPsec has
been enabled. This allows for OVS/OVN IPsec to work when 'ovn-encap-ip'
is not specified as the chassis default gateway interface.

This patch also adds some unit tests. The OVS daemon 'ovs-monitor-ipsec'
requires a number of options to be configured on OVS tunnel ports in order
to function correctly. These unit tests ensure that these options are
configured correctly when IPsec has been enabled through the northbound
database.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1924041
Signed-off-by: Mark Gray 
---

v2: Updated topic filter to "PATCH ovn"
v3: Rebased due to 0-day bot warning

  controller/chassis.c |   5 +++
  controller/encaps.c  |  26 ++-
  tests/automake.mk|   3 +-
  tests/ovn-ipsec.at   | 104 +++
  tests/testsuite.at   |   1 +
  5 files changed, 137 insertions(+), 2 deletions(-)
  create mode 100644 tests/ovn-ipsec.at

diff --git a/controller/chassis.c b/controller/chassis.c
index 310132d09d2e..9b0a36cf076f 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -279,6 +279,11 @@ chassis_parse_ovs_config(const struct 
ovsrec_open_vswitch_table *ovs_table,
  return false;
  }
  
+/* 'ovn-encap-ip' can accept a comma-delimited list of IP addresses instead

+ * of a single IP address. Although this is undocumented, it can be used
+ * to enable certain hardware-offloaded use cases in which a host has
+ * multiple NICs and is assigning SR-IOV VFs to a guest (as logical ports).
+ */
  if (!chassis_parse_ovs_encap_ip(encap_ips, _cfg->encap_ip_set)) {
  sset_destroy(_cfg->encap_type_set);
  return false;
diff --git a/controller/encaps.c b/controller/encaps.c
index 7eac4bb064ac..fc93bf1eeb87 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -59,6 +59,7 @@ struct tunnel_ctx {
  
  struct ovsdb_idl_txn *ovs_txn;

  const struct ovsrec_bridge *br_int;
+const struct sbrec_chassis *this_chassis;
  };
  
  struct chassis_node {

@@ -176,6 +177,28 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
  
  /* Add auth info if ipsec is enabled. */

  if (sbg->ipsec) {
+const struct sbrec_chassis *this_chassis = tc->this_chassis;
+const char *local_ip = NULL;
+
+/* Determine 'ovn-encap-ip' of the local chassis as this will be the
+ * tunnel port's 'local_ip'. We do not support the case in which
+ * 'ovn-encap-ip' holds multiple comma-delimited IP addresses.
+ */
+for (int i = 0; i < this_chassis->n_encaps; i++) {
+if (local_ip && strcmp(local_ip, this_chassis->encaps[i]->ip)) {
+VLOG_ERR("ovn-encap-ip has been configured as a list. This "
+ "is unsupported for IPsec.");
+/* No need to loop further as we know this condition has been
+ * hit */
+break;
+} else {
+local_ip = this_chassis->encaps[i]->ip;
+}
+}
+
+if (local_ip) {
+smap_add(, "local_ip", local_ip);
+}
  smap_add(, "remote_name", new_chassis_id);
  }
  
@@ -310,7 +333,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,

  struct tunnel_ctx tc = {
  .chassis = SHASH_INITIALIZER(),
  .port_names = SSET_INITIALIZER(_names),
-.br_int = br_int
+.br_int = br_int,
+.this_chassis = this_chassis
  };
  
  tc.ovs_txn = ovs_idl_txn;

diff --git a/tests/automake.mk b/tests/automake.mk
index df6d0a2a9074..bef40bde07bb 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -34,7 +34,8 @@ TESTSUITE_AT = \
tests/ovn-performance.at \
tests/ovn-ofctrl-seqno.at \
tests/ovn-ipam.at \
-   tests/ovn-lflow-cache.at
+   tests/ovn-lflow-cache.at \
+   tests/ovn-ipsec.at
  
  SYSTEM_KMOD_TESTSUITE_AT = \

tests/system-common-macros.at \
diff --git a/tests/ovn-ipsec.at b/tests/ovn-ipsec.at
new file mode 100644
index 

[ovs-dev] [PATCH ovn v3] ovn-controller: Add 'local_ip' option to tunnel ports for IPsec case

2021-02-16 Thread Mark Gray
If a chassis has multiple interfaces, 'ovn-encap-ip' can be used
to specify the IP address of the interface that is used for tunnel
traffic. OVN uses that IP address to configure the 'remote_ip' of
a tunnel port. OVS tunnel ports also accept 'options:local_ip', which,
according to the OVS documentation specifies "the tunnel destination
IP that received packets must match. Default is to match all addresses".
OVN does not set 'local_ip'.

'ovs-monitor-ipsec' is an OVS daemon that is used to configure and IPsec
IKE daemon on the host. In order to correctly specify an IPsec
connection, it requires the source and destination IP address of
that connection. In the OVN case, as 'local_ip' is not specified, it
is unable to infer the IP address of both sides of a tunnel and, therefore,
cannot setup an IPsec connection.

This patch configures 'local_ip' on tunnel ports when IPsec has
been enabled. This allows for OVS/OVN IPsec to work when 'ovn-encap-ip'
is not specified as the chassis default gateway interface.

This patch also adds some unit tests. The OVS daemon 'ovs-monitor-ipsec'
requires a number of options to be configured on OVS tunnel ports in order
to function correctly. These unit tests ensure that these options are
configured correctly when IPsec has been enabled through the northbound
database.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1924041
Signed-off-by: Mark Gray 
---

v2: Updated topic filter to "PATCH ovn"
v3: Rebased due to 0-day bot warning

 controller/chassis.c |   5 +++
 controller/encaps.c  |  26 ++-
 tests/automake.mk|   3 +-
 tests/ovn-ipsec.at   | 104 +++
 tests/testsuite.at   |   1 +
 5 files changed, 137 insertions(+), 2 deletions(-)
 create mode 100644 tests/ovn-ipsec.at

diff --git a/controller/chassis.c b/controller/chassis.c
index 310132d09d2e..9b0a36cf076f 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -279,6 +279,11 @@ chassis_parse_ovs_config(const struct 
ovsrec_open_vswitch_table *ovs_table,
 return false;
 }
 
+/* 'ovn-encap-ip' can accept a comma-delimited list of IP addresses instead
+ * of a single IP address. Although this is undocumented, it can be used
+ * to enable certain hardware-offloaded use cases in which a host has
+ * multiple NICs and is assigning SR-IOV VFs to a guest (as logical ports).
+ */
 if (!chassis_parse_ovs_encap_ip(encap_ips, _cfg->encap_ip_set)) {
 sset_destroy(_cfg->encap_type_set);
 return false;
diff --git a/controller/encaps.c b/controller/encaps.c
index 7eac4bb064ac..fc93bf1eeb87 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -59,6 +59,7 @@ struct tunnel_ctx {
 
 struct ovsdb_idl_txn *ovs_txn;
 const struct ovsrec_bridge *br_int;
+const struct sbrec_chassis *this_chassis;
 };
 
 struct chassis_node {
@@ -176,6 +177,28 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 
 /* Add auth info if ipsec is enabled. */
 if (sbg->ipsec) {
+const struct sbrec_chassis *this_chassis = tc->this_chassis;
+const char *local_ip = NULL;
+
+/* Determine 'ovn-encap-ip' of the local chassis as this will be the
+ * tunnel port's 'local_ip'. We do not support the case in which
+ * 'ovn-encap-ip' holds multiple comma-delimited IP addresses.
+ */
+for (int i = 0; i < this_chassis->n_encaps; i++) {
+if (local_ip && strcmp(local_ip, this_chassis->encaps[i]->ip)) {
+VLOG_ERR("ovn-encap-ip has been configured as a list. This "
+ "is unsupported for IPsec.");
+/* No need to loop further as we know this condition has been
+ * hit */
+break;
+} else {
+local_ip = this_chassis->encaps[i]->ip;
+}
+}
+
+if (local_ip) {
+smap_add(, "local_ip", local_ip);
+}
 smap_add(, "remote_name", new_chassis_id);
 }
 
@@ -310,7 +333,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 struct tunnel_ctx tc = {
 .chassis = SHASH_INITIALIZER(),
 .port_names = SSET_INITIALIZER(_names),
-.br_int = br_int
+.br_int = br_int,
+.this_chassis = this_chassis
 };
 
 tc.ovs_txn = ovs_idl_txn;
diff --git a/tests/automake.mk b/tests/automake.mk
index df6d0a2a9074..bef40bde07bb 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -34,7 +34,8 @@ TESTSUITE_AT = \
tests/ovn-performance.at \
tests/ovn-ofctrl-seqno.at \
tests/ovn-ipam.at \
-   tests/ovn-lflow-cache.at
+   tests/ovn-lflow-cache.at \
+   tests/ovn-ipsec.at
 
 SYSTEM_KMOD_TESTSUITE_AT = \
tests/system-common-macros.at \
diff --git a/tests/ovn-ipsec.at b/tests/ovn-ipsec.at
new file mode 100644
index ..887281d5be0e
--- /dev/null
+++ b/tests/ovn-ipsec.at
@@ -0,0 +1,104 @@
+AT_BANNER([OVN - IPsec])
+