| Message ID | 20240419111444.1223539-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] controller: Remove the ovn-set-local-ip option. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
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 <amusil@redhat.com> > --- 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(&options); > smap_add(&options, "remote_ip", encap->ip); > + smap_add(&options, "local_ip", local_ip); > smap_add(&options, "key", "flow"); > const char *dst_port = smap_get(&encap->options, "dst_port"); > const char *csum = smap_get(&encap->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(&options, "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( > - &cfg->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(&options, "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(&options, "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 > 30000 (30 seconds). > </dd> > - <dt><code>external_ids:ovn-set-local-ip</code></dt> > - <dd> > - The boolean flag indicates if <code>ovn-controller</code> when create > - tunnel ports should set <code>local_ip</code> parameter. Can be > - heplful to pin source outer IP for the tunnel when multiple interfaces > - are used on the host for overlay traffic. This is also useful when > - running multiple <code>ovn-controller</code> instances on the same > - chassis, in which case this setting will guarantee that their tunnel > - ports have unique configuration and can exist in parallel. > - </dd> > <dt><code>external_ids:garp-max-timeout-sec</code></dt> > <dd> > When used, this configuration value specifies the maximum timeout > @@ -410,9 +400,7 @@ > names on the same host using the same <code>vswitchd</code> instance. > This may be useful when running a hybrid setup with more than one CMS > managing ports on the host, or to use different datapath types on the > - same host. Make sure you also set > - <code>external_ids:ovn-set-local-ip</code> when using such > - configuration. Also note that this ability is highly experimental and > + same host. Also note that this ability is highly experimental and > has known limitations (for example, stateful ACLs are not supported). > Use at your own risk. > </p> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index f2c792c9c..be198e00d 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -318,11 +318,6 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > ovs-vsctl del-port ovn-fakech-0 > OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > -# set `ovn-set-local-ip` option to true and check if tunnel parameters > -OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > -ovs-vsctl set open . external_ids:ovn-set-local-ip=true > -OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > - > # Change the local_ip on the OVS side and check than OVN fixes it > ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > @@ -817,7 +812,7 @@ check_tunnel_property () { > } > > # without any tos options > -no_tos_options="{csum=\"true\", key=flow, remote_ip=\"192.168.0.2\"}" > +no_tos_options="{csum=\"true\", key=flow, local_ip=\"192.168.0.1\", remote_ip=\"192.168.0.2\"}" > > # > # Start off with a remote chassis supporting geneve > @@ -2880,3 +2875,34 @@ AT_CHECK([test x"$port_uuid"=$(ovs-vsctl get port $fakech_tunnel _uuid)]) > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([Encap enforce local_ip]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.11 > + > +sim_add hv2 > +as hv2 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.12 > + > +as hv1 > +OVS_WAIT_UNTIL([ > + test $(ovs-vsctl --bare --columns _uuid find interface options:local_ip="192.168.0.11" | wc -l) -eq 1 > +]) > + > +as hv2 > +OVS_WAIT_UNTIL([ > + test $(ovs-vsctl --bare --columns _uuid find interface options:local_ip="192.168.0.12" | wc -l) -eq 1 > +]) > + > +OVN_CLEANUP([hv1],[hv2]) > + > +AT_CLEANUP > +])
On Fri, Apr 19, 2024 at 4:51 AM Dumitru Ceara <dceara@redhat.com> 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 <amusil@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 <hzhou@ovn.org> > 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(&options); > > smap_add(&options, "remote_ip", encap->ip); > > + smap_add(&options, "local_ip", local_ip); > > smap_add(&options, "key", "flow"); > > const char *dst_port = smap_get(&encap->options, "dst_port"); > > const char *csum = smap_get(&encap->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(&options, "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( > > - &cfg->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(&options, "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(&options, "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 > > 30000 (30 seconds). > > </dd> > > - <dt><code>external_ids:ovn-set-local-ip</code></dt> > > - <dd> > > - The boolean flag indicates if <code>ovn-controller</code> when create > > - tunnel ports should set <code>local_ip</code> parameter. Can be > > - heplful to pin source outer IP for the tunnel when multiple interfaces > > - are used on the host for overlay traffic. This is also useful when > > - running multiple <code>ovn-controller</code> instances on the same > > - chassis, in which case this setting will guarantee that their tunnel > > - ports have unique configuration and can exist in parallel. > > - </dd> > > <dt><code>external_ids:garp-max-timeout-sec</code></dt> > > <dd> > > When used, this configuration value specifies the maximum timeout > > @@ -410,9 +400,7 @@ > > names on the same host using the same <code>vswitchd</code> instance. > > This may be useful when running a hybrid setup with more than one CMS > > managing ports on the host, or to use different datapath types on the > > - same host. Make sure you also set > > - <code>external_ids:ovn-set-local-ip</code> when using such > > - configuration. Also note that this ability is highly experimental and > > + same host. Also note that this ability is highly experimental and > > has known limitations (for example, stateful ACLs are not supported). > > Use at your own risk. > > </p> > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > index f2c792c9c..be198e00d 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -318,11 +318,6 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > ovs-vsctl del-port ovn-fakech-0 > > OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > > > -# set `ovn-set-local-ip` option to true and check if tunnel parameters > > -OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > > -ovs-vsctl set open . external_ids:ovn-set-local-ip=true > > -OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > > - > > # Change the local_ip on the OVS side and check than OVN fixes it > > ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > > OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > > @@ -817,7 +812,7 @@ check_tunnel_property () { > > } > > > > # without any tos options > > -no_tos_options="{csum=\"true\", key=flow, remote_ip=\"192.168.0.2\"}" > > +no_tos_options="{csum=\"true\", key=flow, local_ip=\"192.168.0.1\", remote_ip=\"192.168.0.2\"}" > > > > # > > # Start off with a remote chassis supporting geneve > > @@ -2880,3 +2875,34 @@ AT_CHECK([test x"$port_uuid"=$(ovs-vsctl get port $fakech_tunnel _uuid)]) > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([Encap enforce local_ip]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +check ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.11 > > + > > +sim_add hv2 > > +as hv2 > > +check ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.12 > > + > > +as hv1 > > +OVS_WAIT_UNTIL([ > > + test $(ovs-vsctl --bare --columns _uuid find interface options:local_ip="192.168.0.11" | wc -l) -eq 1 > > +]) > > + > > +as hv2 > > +OVS_WAIT_UNTIL([ > > + test $(ovs-vsctl --bare --columns _uuid find interface options:local_ip="192.168.0.12" | wc -l) -eq 1 > > +]) > > + > > +OVN_CLEANUP([hv1],[hv2]) > > + > > +AT_CLEANUP > > +]) >
On 4/22/24 08:34, Han Zhou wrote: > > > On Fri, Apr 19, 2024 at 4:51 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> 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 > <https://issues.redhat.com/browse/FDP-570> >> > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:amusil@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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> > Thanks, Ales and Han! Applied to main. Regards, Dumitru
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(&options); smap_add(&options, "remote_ip", encap->ip); + smap_add(&options, "local_ip", local_ip); smap_add(&options, "key", "flow"); const char *dst_port = smap_get(&encap->options, "dst_port"); const char *csum = smap_get(&encap->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(&options, "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( - &cfg->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(&options, "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(&options, "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 30000 (30 seconds). </dd> - <dt><code>external_ids:ovn-set-local-ip</code></dt> - <dd> - The boolean flag indicates if <code>ovn-controller</code> when create - tunnel ports should set <code>local_ip</code> parameter. Can be - heplful to pin source outer IP for the tunnel when multiple interfaces - are used on the host for overlay traffic. This is also useful when - running multiple <code>ovn-controller</code> instances on the same - chassis, in which case this setting will guarantee that their tunnel - ports have unique configuration and can exist in parallel. - </dd> <dt><code>external_ids:garp-max-timeout-sec</code></dt> <dd> When used, this configuration value specifies the maximum timeout @@ -410,9 +400,7 @@ names on the same host using the same <code>vswitchd</code> instance. This may be useful when running a hybrid setup with more than one CMS managing ports on the host, or to use different datapath types on the - same host. Make sure you also set - <code>external_ids:ovn-set-local-ip</code> when using such - configuration. Also note that this ability is highly experimental and + same host. Also note that this ability is highly experimental and has known limitations (for example, stateful ACLs are not supported). Use at your own risk. </p> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index f2c792c9c..be198e00d 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -318,11 +318,6 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) ovs-vsctl del-port ovn-fakech-0 OVS_WAIT_UNTIL([check_tunnel_property type geneve]) -# set `ovn-set-local-ip` option to true and check if tunnel parameters -OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) -ovs-vsctl set open . external_ids:ovn-set-local-ip=true -OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) - # Change the local_ip on the OVS side and check than OVN fixes it ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) @@ -817,7 +812,7 @@ check_tunnel_property () { } # without any tos options -no_tos_options="{csum=\"true\", key=flow, remote_ip=\"192.168.0.2\"}" +no_tos_options="{csum=\"true\", key=flow, local_ip=\"192.168.0.1\", remote_ip=\"192.168.0.2\"}" # # Start off with a remote chassis supporting geneve @@ -2880,3 +2875,34 @@ AT_CHECK([test x"$port_uuid"=$(ovs-vsctl get port $fakech_tunnel _uuid)]) OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([Encap enforce local_ip]) +ovn_start + +net_add n1 + +sim_add hv1 +as hv1 +check ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.11 + +sim_add hv2 +as hv2 +check ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.12 + +as hv1 +OVS_WAIT_UNTIL([ + test $(ovs-vsctl --bare --columns _uuid find interface options:local_ip="192.168.0.11" | wc -l) -eq 1 +]) + +as hv2 +OVS_WAIT_UNTIL([ + test $(ovs-vsctl --bare --columns _uuid find interface options:local_ip="192.168.0.12" | wc -l) -eq 1 +]) + +OVN_CLEANUP([hv1],[hv2]) + +AT_CLEANUP +])
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 <amusil@redhat.com> --- NEWS | 3 +++ controller/encaps.c | 31 +++------------------------ controller/ovn-controller.8.xml | 14 +----------- tests/ovn-controller.at | 38 +++++++++++++++++++++++++++------ 4 files changed, 39 insertions(+), 47 deletions(-)