On Fri, Apr 19, 2024 at 4:51 AM Dumitru Ceara <[email protected]> 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 <[email protected]> > > --- > > 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 <[email protected]> > 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 > > +]) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
