Re: [ovs-dev] [PATCH ovn] controller: Remove the ovn-set-local-ip option.

2024-04-22 Thread Dumitru Ceara 
On 4/22/24 08:34, Han Zhou wrote:
> 
> 
> On Fri, Apr 19, 2024 at 4:51 AM Dumitru Ceara  > wrote:
>>
>> On 4/19/24 13:14, Ales Musil wrote:
>> > The local_ip should be present for chassis with single encap whenever
>> > we configure its interface in OvS. Not having the local_ip can lead to
>> > traffic being dropped on the other side of tunnel because the source
>> > IP might be different, this is more likely to happen in pure IPv6
>> > deployments.
>> >
>> > Remove the option as with the local_ip being enforced
>> > also for single encap it became "true" in all scenarios, and it's not
>> > needed anymore.
>> >
>> > Reported-at: https://issues.redhat.com/browse/FDP-570
> 
>> > Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
>> > ---
>>
>> Hi Han,
>>
>> When you have time would you mind double checking this in case we missed
>> some scenario?
>>
> 
> Thanks Ales and Dumitru. I wanted to do the same even when I was working
> on commit 41eefcb280. I kept the default behavior because setting
> local_ip would require incoming tunnel packets' destination IP to match
> the local_ip, which is more strict than the old default settings, and I
> wasn't sure if any existing user would depend on the old behavior.
> Thinking it more carefully, for OVN it seems not possible because the
> ovn-encap-ip used as the local_ip is always the one shared to other
> chassis through SB DB. So now I think we should be safe to change the
> default behavior.
> 
> Acked-by: Han Zhou mailto:hz...@ovn.org>>
> 

Thanks, Ales and Han!  Applied to main.

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] controller: Remove the ovn-set-local-ip option.

2024-04-22 Thread Han Zhou 
On Fri, Apr 19, 2024 at 4:51 AM Dumitru Ceara  wrote:
>
> On 4/19/24 13:14, Ales Musil wrote:
> > The local_ip should be present for chassis with single encap whenever
> > we configure its interface in OvS. Not having the local_ip can lead to
> > traffic being dropped on the other side of tunnel because the source
> > IP might be different, this is more likely to happen in pure IPv6
> > deployments.
> >
> > Remove the option as with the local_ip being enforced
> > also for single encap it became "true" in all scenarios, and it's not
> > needed anymore.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-570
> > Signed-off-by: Ales Musil 
> > ---
>
> Hi Han,
>
> When you have time would you mind double checking this in case we missed
> some scenario?
>

Thanks Ales and Dumitru. I wanted to do the same even when I was working on
commit 41eefcb280. I kept the default behavior because setting local_ip
would require incoming tunnel packets' destination IP to match the
local_ip, which is more strict than the old default settings, and I wasn't
sure if any existing user would depend on the old behavior. Thinking it
more carefully, for OVN it seems not possible because the ovn-encap-ip used
as the local_ip is always the one shared to other chassis through SB DB. So
now I think we should be safe to change the default behavior.

Acked-by: Han Zhou 

> Thanks,
> Dumitru
>
> >  NEWS|  3 +++
> >  controller/encaps.c | 31 +++
> >  controller/ovn-controller.8.xml | 14 +---
> >  tests/ovn-controller.at | 38 +++--
> >  4 files changed, 39 insertions(+), 47 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 141f1831c..9adf6a31c 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -13,6 +13,9 @@ Post v24.03.0
> >  "lflow-stage-to-oftable STAGE_NAME" that converts stage name into
OpenFlow
> >  table id.
> >- Rename the ovs-sandbox script to ovn-sandbox.
> > +  - Remove "ovn-set-local-ip" config option from vswitchd
> > +external-ids, the option is no longer needed as it became
effectively
> > +"true" for all scenarios.
> >
> >  OVN v24.03.0 - 01 Mar 2024
> >  --
> > diff --git a/controller/encaps.c b/controller/encaps.c
> > index a9cb604b8..b5ef66371 100644
> > --- a/controller/encaps.c
> > +++ b/controller/encaps.c
> > @@ -208,11 +208,12 @@ out:
> >  static void
> >  tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
> > const char *new_chassis_id, const struct sbrec_encap *encap,
> > -   bool must_set_local_ip, const char *local_ip,
> > +   const char *local_ip,
> > const struct ovsrec_open_vswitch_table *ovs_table)
> >  {
> >  struct smap options = SMAP_INITIALIZER();
> >  smap_add(, "remote_ip", encap->ip);
> > +smap_add(, "local_ip", local_ip);
> >  smap_add(, "key", "flow");
> >  const char *dst_port = smap_get(>options, "dst_port");
> >  const char *csum = smap_get(>options, "csum");
> > @@ -239,7 +240,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct
sbrec_sb_global *sbg,
> >  const struct ovsrec_open_vswitch *cfg =
> >  ovsrec_open_vswitch_table_first(ovs_table);
> >
> > -bool set_local_ip = must_set_local_ip;
> >  if (cfg) {
> >  /* If the tos option is configured, get it */
> >  const char *encap_tos =
> > @@ -259,19 +259,10 @@ tunnel_add(struct tunnel_ctx *tc, const struct
sbrec_sb_global *sbg,
> >  if (encap_df) {
> >  smap_add(, "df_default", encap_df);
> >  }
> > -
> > -if (!set_local_ip) {
> > -/* If ovn-set-local-ip option is configured, get it */
> > -set_local_ip =
> > -get_chassis_external_id_value_bool(
> > ->external_ids, tc->this_chassis->name,
> > -"ovn-set-local-ip", false);
> > -}
> >  }
> >
> >  /* Add auth info if ipsec is enabled. */
> >  if (sbg->ipsec) {
> > -set_local_ip = true;
> >  smap_add(, "remote_name", new_chassis_id);
> >
> >  /* Force NAT-T traversal via configuration */
> > @@ -290,10 +281,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct
sbrec_sb_global *sbg,
> >  }
> >  }
> >
> > -if (set_local_ip) {
> > -smap_add(, "local_ip", local_ip);
> > -}
> > -
> >  /* If there's an existing tunnel record that does not need any
change,
> >   * keep it.  Otherwise, create a new record (if there was an
existing
> >   * record, the new record will supplant it and encaps_run() will
delete
> > @@ -412,18 +399,6 @@ chassis_tunnel_add(const struct sbrec_chassis
*chassis_rec,
> >  continue;
> >  }
> >
> > -/* Check if need to pass the local ip. We always set local ip
if there
> > - * are multiple local IPs for the selected encap type. */
> > -int count = 0;
> > -bool set_local_ip =

Re: [ovs-dev] [PATCH ovn] controller: Remove the ovn-set-local-ip option.

2024-04-19 Thread Dumitru Ceara 
On 4/19/24 13:14, Ales Musil wrote:
> The local_ip should be present for chassis with single encap whenever
> we configure its interface in OvS. Not having the local_ip can lead to
> traffic being dropped on the other side of tunnel because the source
> IP might be different, this is more likely to happen in pure IPv6
> deployments.
> 
> Remove the option as with the local_ip being enforced
> also for single encap it became "true" in all scenarios, and it's not
> needed anymore.
> 
> Reported-at: https://issues.redhat.com/browse/FDP-570
> Signed-off-by: Ales Musil 
> ---

Hi Han,

When you have time would you mind double checking this in case we missed
some scenario?

Thanks,
Dumitru

>  NEWS|  3 +++
>  controller/encaps.c | 31 +++
>  controller/ovn-controller.8.xml | 14 +---
>  tests/ovn-controller.at | 38 +++--
>  4 files changed, 39 insertions(+), 47 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 141f1831c..9adf6a31c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -13,6 +13,9 @@ Post v24.03.0
>  "lflow-stage-to-oftable STAGE_NAME" that converts stage name into 
> OpenFlow
>  table id.
>- Rename the ovs-sandbox script to ovn-sandbox.
> +  - Remove "ovn-set-local-ip" config option from vswitchd
> +external-ids, the option is no longer needed as it became effectively
> +"true" for all scenarios.
>  
>  OVN v24.03.0 - 01 Mar 2024
>  --
> diff --git a/controller/encaps.c b/controller/encaps.c
> index a9cb604b8..b5ef66371 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -208,11 +208,12 @@ out:
>  static void
>  tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
> const char *new_chassis_id, const struct sbrec_encap *encap,
> -   bool must_set_local_ip, const char *local_ip,
> +   const char *local_ip,
> const struct ovsrec_open_vswitch_table *ovs_table)
>  {
>  struct smap options = SMAP_INITIALIZER();
>  smap_add(, "remote_ip", encap->ip);
> +smap_add(, "local_ip", local_ip);
>  smap_add(, "key", "flow");
>  const char *dst_port = smap_get(>options, "dst_port");
>  const char *csum = smap_get(>options, "csum");
> @@ -239,7 +240,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
> sbrec_sb_global *sbg,
>  const struct ovsrec_open_vswitch *cfg =
>  ovsrec_open_vswitch_table_first(ovs_table);
>  
> -bool set_local_ip = must_set_local_ip;
>  if (cfg) {
>  /* If the tos option is configured, get it */
>  const char *encap_tos =
> @@ -259,19 +259,10 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
> sbrec_sb_global *sbg,
>  if (encap_df) {
>  smap_add(, "df_default", encap_df);
>  }
> -
> -if (!set_local_ip) {
> -/* If ovn-set-local-ip option is configured, get it */
> -set_local_ip =
> -get_chassis_external_id_value_bool(
> ->external_ids, tc->this_chassis->name,
> -"ovn-set-local-ip", false);
> -}
>  }
>  
>  /* Add auth info if ipsec is enabled. */
>  if (sbg->ipsec) {
> -set_local_ip = true;
>  smap_add(, "remote_name", new_chassis_id);
>  
>  /* Force NAT-T traversal via configuration */
> @@ -290,10 +281,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
> sbrec_sb_global *sbg,
>  }
>  }
>  
> -if (set_local_ip) {
> -smap_add(, "local_ip", local_ip);
> -}
> -
>  /* If there's an existing tunnel record that does not need any change,
>   * keep it.  Otherwise, create a new record (if there was an existing
>   * record, the new record will supplant it and encaps_run() will delete
> @@ -412,18 +399,6 @@ chassis_tunnel_add(const struct sbrec_chassis 
> *chassis_rec,
>  continue;
>  }
>  
> -/* Check if need to pass the local ip. We always set local ip if 
> there
> - * are multiple local IPs for the selected encap type. */
> -int count = 0;
> -bool set_local_ip = false;
> -for (int j = 0; j < this_chassis->n_encaps; j++) {
> -if (pref_type == get_tunnel_type(this_chassis->encaps[j]->type) 
> &&
> -count++ > 0) {
> -set_local_ip = true;
> -break;
> -}
> -}
> -
>  for (int j = 0; j < this_chassis->n_encaps; j++) {
>  if (pref_type != get_tunnel_type(this_chassis->encaps[j]->type)) 
> {
>  continue;
> @@ -431,7 +406,7 @@ chassis_tunnel_add(const struct sbrec_chassis 
> *chassis_rec,
>  VLOG_DBG("tunnel_add: '%s', local ip: %s", chassis_rec->name,
>   this_chassis->encaps[j]->ip);
>  tunnel_add(tc, sbg, chassis_rec->name, chassis_rec->encaps[i],
> -   set_local_ip, this_chassis->encaps[j]->ip,

[ovs-dev] [PATCH ovn] controller: Remove the ovn-set-local-ip option.

2024-04-19 Thread Ales Musil 
The local_ip should be present for chassis with single encap whenever
we configure its interface in OvS. Not having the local_ip can lead to
traffic being dropped on the other side of tunnel because the source
IP might be different, this is more likely to happen in pure IPv6
deployments.

Remove the option as with the local_ip being enforced
also for single encap it became "true" in all scenarios, and it's not
needed anymore.

Reported-at: https://issues.redhat.com/browse/FDP-570
Signed-off-by: Ales Musil 
---
 NEWS|  3 +++
 controller/encaps.c | 31 +++
 controller/ovn-controller.8.xml | 14 +---
 tests/ovn-controller.at | 38 +++--
 4 files changed, 39 insertions(+), 47 deletions(-)

diff --git a/NEWS b/NEWS
index 141f1831c..9adf6a31c 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,9 @@ Post v24.03.0
 "lflow-stage-to-oftable STAGE_NAME" that converts stage name into OpenFlow
 table id.
   - Rename the ovs-sandbox script to ovn-sandbox.
+  - Remove "ovn-set-local-ip" config option from vswitchd
+external-ids, the option is no longer needed as it became effectively
+"true" for all scenarios.
 
 OVN v24.03.0 - 01 Mar 2024
 --
diff --git a/controller/encaps.c b/controller/encaps.c
index a9cb604b8..b5ef66371 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -208,11 +208,12 @@ out:
 static void
 tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
const char *new_chassis_id, const struct sbrec_encap *encap,
-   bool must_set_local_ip, const char *local_ip,
+   const char *local_ip,
const struct ovsrec_open_vswitch_table *ovs_table)
 {
 struct smap options = SMAP_INITIALIZER();
 smap_add(, "remote_ip", encap->ip);
+smap_add(, "local_ip", local_ip);
 smap_add(, "key", "flow");
 const char *dst_port = smap_get(>options, "dst_port");
 const char *csum = smap_get(>options, "csum");
@@ -239,7 +240,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 const struct ovsrec_open_vswitch *cfg =
 ovsrec_open_vswitch_table_first(ovs_table);
 
-bool set_local_ip = must_set_local_ip;
 if (cfg) {
 /* If the tos option is configured, get it */
 const char *encap_tos =
@@ -259,19 +259,10 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 if (encap_df) {
 smap_add(, "df_default", encap_df);
 }
-
-if (!set_local_ip) {
-/* If ovn-set-local-ip option is configured, get it */
-set_local_ip =
-get_chassis_external_id_value_bool(
->external_ids, tc->this_chassis->name,
-"ovn-set-local-ip", false);
-}
 }
 
 /* Add auth info if ipsec is enabled. */
 if (sbg->ipsec) {
-set_local_ip = true;
 smap_add(, "remote_name", new_chassis_id);
 
 /* Force NAT-T traversal via configuration */
@@ -290,10 +281,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 }
 }
 
-if (set_local_ip) {
-smap_add(, "local_ip", local_ip);
-}
-
 /* If there's an existing tunnel record that does not need any change,
  * keep it.  Otherwise, create a new record (if there was an existing
  * record, the new record will supplant it and encaps_run() will delete
@@ -412,18 +399,6 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec,
 continue;
 }
 
-/* Check if need to pass the local ip. We always set local ip if there
- * are multiple local IPs for the selected encap type. */
-int count = 0;
-bool set_local_ip = false;
-for (int j = 0; j < this_chassis->n_encaps; j++) {
-if (pref_type == get_tunnel_type(this_chassis->encaps[j]->type) &&
-count++ > 0) {
-set_local_ip = true;
-break;
-}
-}
-
 for (int j = 0; j < this_chassis->n_encaps; j++) {
 if (pref_type != get_tunnel_type(this_chassis->encaps[j]->type)) {
 continue;
@@ -431,7 +406,7 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec,
 VLOG_DBG("tunnel_add: '%s', local ip: %s", chassis_rec->name,
  this_chassis->encaps[j]->ip);
 tunnel_add(tc, sbg, chassis_rec->name, chassis_rec->encaps[i],
-   set_local_ip, this_chassis->encaps[j]->ip, ovs_table);
+   this_chassis->encaps[j]->ip, ovs_table);
 tuncnt++;
 }
 }
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 5ebef048d..85e7966d7 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -367,16 +367,6 @@
 of how many entries there are in the cache.  By default this is set to