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,
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
