From b8bb585052331a48764d6d5d21ee23e2f82f1078 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Apr 2021 12:14:13 +0200 Subject: [PATCH 01/17] glib-aux: add _nm_utils_ascii_str_to_int64_bin() helper (cherry picked from commit 70b7ad1a761405f4ac9398832d0365c47cd5aa0f) --- shared/nm-glib-aux/nm-shared-utils.c | 39 ++++++++++++++++++++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 7 +++++ 2 files changed, 46 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index bdf2f67093..6c8e8f991c 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -1383,6 +1383,45 @@ _nm_utils_ascii_str_to_uint64(const char *str, /*****************************************************************************/ +gint64 +_nm_utils_ascii_str_to_int64_bin(const char *str, + gssize len, + guint base, + gint64 min, + gint64 max, + gint64 fallback) +{ + gs_free char *str_clone = NULL; + + /* This is like _nm_utils_ascii_str_to_int64(), but the user may provide + * an optional string length, in which case str is not assumed to be NUL + * terminated. In that case, any NUL characters inside the first len characters + * lead to a failure, except one last NUL character is allowed. */ + + if (len >= 0) { + gsize l = len; + + nm_assert(l == 0 || str); + + if (l > 0 && str[l - 1u] == '\0') { + /* we accept one '\0' at the end of the string. */ + l--; + } + + if (l > 0 && memchr(str, '\0', l)) { + /* but we don't accept other NUL characters in the middle. */ + errno = EINVAL; + return fallback; + } + + str = nm_strndup_a(300, str, len, &str_clone); + } + + return _nm_utils_ascii_str_to_int64(str, base, min, max, fallback); +} + +/*****************************************************************************/ + int nm_strcmp_with_data(gconstpointer a, gconstpointer b, gpointer user_data) { diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 922e0813df..83404ff323 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -760,6 +760,13 @@ guint64 _nm_utils_ascii_str_to_uint64(const char *str, guint64 max, guint64 fallback); +gint64 _nm_utils_ascii_str_to_int64_bin(const char *str, + gssize len, + guint base, + gint64 min, + gint64 max, + gint64 fallback); + int _nm_utils_ascii_str_to_bool(const char *str, int default_value); /*****************************************************************************/ From 2585e34e598074e41445e4dd25d1b496ffbcea3f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Jun 2021 19:04:39 +0200 Subject: [PATCH 02/17] libnm: expose nm_ip_address_dup(), nm_ip_route_dup() API in libnm This fixes commit 21c8a6b20eff ('libnm-core, all: merge IPv4 and IPv6 address/route types'), which introduced this API but didn't export it in the library. In practice this API is thus only usable since 1.32.0. (cherry picked from commit 05f2a0b0247ee4aa3da371658f310bc655cbf1af) (cherry picked from commit eea912dfb31a40909cad7b784a82bbfbe56ef486) --- libnm-core/nm-setting-ip-config.c | 12 ++++++++++++ libnm-core/nm-setting-ip-config.h | 3 +++ libnm/libnm.ver | 2 ++ 3 files changed, 17 insertions(+) diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index eaee2c5533..4ef73273ed 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -363,6 +363,12 @@ nm_ip_address_equal(NMIPAddress *address, NMIPAddress *other) * Creates a copy of @address * * Returns: (transfer full): a copy of @address + * + * This API was part of public headers before 1.32.0 but + * was erroneously not exported in the ABI. It is thus only + * usable since 1.32.0, 1.30.8. + * + * Since: 1.32, 1.30.8 **/ NMIPAddress * nm_ip_address_dup(NMIPAddress *address) @@ -820,6 +826,12 @@ nm_ip_route_equal(NMIPRoute *route, NMIPRoute *other) * Creates a copy of @route * * Returns: (transfer full): a copy of @route + * + * This API was part of public headers before 1.32.0 but + * was erroneously not exported in the ABI. It is thus only + * usable since 1.32.0, 1.30.8. + * + * Since: 1.32, 1.30.8 **/ NMIPRoute * nm_ip_route_dup(NMIPRoute *route) diff --git a/libnm-core/nm-setting-ip-config.h b/libnm-core/nm-setting-ip-config.h index 27c14f392e..50eee0561c 100644 --- a/libnm-core/nm-setting-ip-config.h +++ b/libnm-core/nm-setting-ip-config.h @@ -49,6 +49,8 @@ gboolean nm_ip_address_equal(NMIPAddress *address, NMIPAddress *other); NM_AVAILABLE_IN_1_22 int nm_ip_address_cmp_full(const NMIPAddress *a, const NMIPAddress *b, NMIPAddressCmpFlags cmp_flags); + +NM_AVAILABLE_IN_1_30_8 NMIPAddress *nm_ip_address_dup(NMIPAddress *address); int nm_ip_address_get_family(NMIPAddress *address); @@ -92,6 +94,7 @@ enum { /*< flags >*/ NM_AVAILABLE_IN_1_10 gboolean nm_ip_route_equal_full(NMIPRoute *route, NMIPRoute *other, guint cmp_flags); +NM_AVAILABLE_IN_1_30_8 NMIPRoute *nm_ip_route_dup(NMIPRoute *route); int nm_ip_route_get_family(NMIPRoute *route); diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 7cdcf8104f..8451a493a9 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1787,5 +1787,7 @@ global: libnm_1_30_8 { global: + nm_ip_address_dup; + nm_ip_route_dup; nm_setting_ip_config_get_required_timeout; } libnm_1_30_0; From b2ed9e7d5df01dc20ae3ce18066f38f69fe31e9d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 09:30:29 +0200 Subject: [PATCH 03/17] cloud-setup: return structure for get_config() result instead of generic hash table Returning a struct seems easier to understand, because then the result is typed. Also, we might return additional results, which are system wide and not per-interface. (cherry picked from commit 323e18276894591712a5e29f6e907562c79c5216) (cherry picked from commit c94b1c43d4b5c5b88d67d7966d23a005028e78d8) --- clients/cloud-setup/main.c | 54 +++++++++++++++-------------- clients/cloud-setup/nmcs-provider.c | 28 +++++++++++++-- clients/cloud-setup/nmcs-provider.h | 20 ++++++++++- 3 files changed, 72 insertions(+), 30 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index 065901760a..c57c5ab5ca 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -198,30 +198,30 @@ _nmc_get_device_by_hwaddr(NMClient *nmc, const char *hwaddr) /*****************************************************************************/ typedef struct { - GMainLoop * main_loop; - GHashTable *config_dict; + GMainLoop * main_loop; + NMCSProviderGetConfigResult *result; } GetConfigData; static void -_get_config_cb(GObject *source, GAsyncResult *result, gpointer user_data) +_get_config_cb(GObject *source, GAsyncResult *res, gpointer user_data) { - GetConfigData * data = user_data; - gs_unref_hashtable GHashTable *config_dict = NULL; - gs_free_error GError *error = NULL; + GetConfigData * data = user_data; + nm_auto_free_nmcs_provider_get_config_result NMCSProviderGetConfigResult *result = NULL; + gs_free_error GError *error = NULL; - config_dict = nmcs_provider_get_config_finish(NMCS_PROVIDER(source), result, &error); + result = nmcs_provider_get_config_finish(NMCS_PROVIDER(source), res, &error); - if (!config_dict) { + if (!result) { if (!nm_utils_error_is_cancelled(error)) _LOGI("failure to get meta data: %s", error->message); } else _LOGD("meta data received"); - data->config_dict = g_steal_pointer(&config_dict); + data->result = g_steal_pointer(&result); g_main_loop_quit(data->main_loop); } -static GHashTable * +static NMCSProviderGetConfigResult * _get_config(GCancellable *sigterm_cancellable, NMCSProvider *provider, NMClient *nmc) { nm_auto_unref_gmainloop GMainLoop *main_loop = g_main_loop_new(NULL, FALSE); @@ -241,7 +241,7 @@ _get_config(GCancellable *sigterm_cancellable, NMCSProvider *provider, NMClient g_main_loop_run(main_loop); - return data.config_dict; + return data.result; } /*****************************************************************************/ @@ -361,13 +361,13 @@ _nmc_mangle_connection(NMDevice * device, /*****************************************************************************/ static guint -_config_data_get_num_valid(GHashTable *config_dict) +_config_data_get_num_valid(const NMCSProviderGetConfigResult *result) { const NMCSProviderGetConfigIfaceData *config_data; GHashTableIter h_iter; guint n = 0; - g_hash_table_iter_init(&h_iter, config_dict); + g_hash_table_iter_init(&h_iter, result->iface_datas); while (g_hash_table_iter_next(&h_iter, NULL, (gpointer *) &config_data)) { if (nmcs_provider_get_config_iface_data_is_valid(config_data)) n++; @@ -490,7 +490,9 @@ try_again: } static gboolean -_config_all(GCancellable *sigterm_cancellable, NMClient *nmc, GHashTable *config_dict) +_config_all(GCancellable * sigterm_cancellable, + NMClient * nmc, + const NMCSProviderGetConfigResult *result) { GHashTableIter h_iter; const NMCSProviderGetConfigIfaceData *c_config_data; @@ -498,9 +500,9 @@ _config_all(GCancellable *sigterm_cancellable, NMClient *nmc, GHashTable *config gboolean is_single_nic; gboolean any_changes = FALSE; - is_single_nic = (_config_data_get_num_valid(config_dict) <= 1); + is_single_nic = (_config_data_get_num_valid(result) <= 1); - g_hash_table_iter_init(&h_iter, config_dict); + g_hash_table_iter_init(&h_iter, result->iface_datas); while (g_hash_table_iter_next(&h_iter, (gpointer *) &c_hwaddr, (gpointer *) &c_config_data)) { if (_config_one(sigterm_cancellable, nmc, is_single_nic, c_hwaddr, c_config_data)) any_changes = TRUE; @@ -529,12 +531,12 @@ sigterm_handler(gpointer user_data) int main(int argc, const char *const *argv) { - gs_unref_object GCancellable * sigterm_cancellable = NULL; - nm_auto_destroy_and_unref_gsource GSource *sigterm_source = NULL; - gs_unref_object NMCSProvider *provider = NULL; - gs_unref_object NMClient *nmc = NULL; - gs_unref_hashtable GHashTable *config_dict = NULL; - gs_free_error GError *error = NULL; + gs_unref_object GCancellable * sigterm_cancellable = NULL; + nm_auto_destroy_and_unref_gsource GSource *sigterm_source = NULL; + gs_unref_object NMCSProvider *provider = NULL; + gs_unref_object NMClient * nmc = NULL; + nm_auto_free_nmcs_provider_get_config_result NMCSProviderGetConfigResult *result = NULL; + gs_free_error GError *error = NULL; _nm_logging_enabled_init(g_getenv(NMCS_ENV_VARIABLE("NM_CLOUD_SETUP_LOG"))); @@ -579,17 +581,17 @@ main(int argc, const char *const *argv) goto done; } - config_dict = _get_config(sigterm_cancellable, provider, nmc); - if (!config_dict) + result = _get_config(sigterm_cancellable, provider, nmc); + if (!result) goto done; - if (_config_all(sigterm_cancellable, nmc, config_dict)) + if (_config_all(sigterm_cancellable, nmc, result)) _LOGI("some changes were applied for provider %s", nmcs_provider_get_name(provider)); else _LOGD("no changes were applied for provider %s", nmcs_provider_get_name(provider)); done: - nm_clear_pointer(&config_dict, g_hash_table_unref); + nm_clear_pointer(&result, nmcs_provider_get_config_result_free); g_clear_object(&nmc); g_clear_object(&provider); diff --git a/clients/cloud-setup/nmcs-provider.c b/clients/cloud-setup/nmcs-provider.c index c700d8e1a3..d335193166 100644 --- a/clients/cloud-setup/nmcs-provider.c +++ b/clients/cloud-setup/nmcs-provider.c @@ -49,6 +49,28 @@ nmcs_provider_get_main_context(NMCSProvider *self) return nm_http_client_get_main_context(NMCS_PROVIDER_GET_PRIVATE(self)->http_client); } +/*****************************************************************************/ + +static NMCSProviderGetConfigResult * +nmcs_provider_get_config_result_new(GHashTable *iface_datas) +{ + NMCSProviderGetConfigResult *result; + + result = g_new(NMCSProviderGetConfigResult, 1); + *result = (NMCSProviderGetConfigResult){ + .iface_datas = g_hash_table_ref(iface_datas), + }; + return result; +} + +void +nmcs_provider_get_config_result_free(NMCSProviderGetConfigResult *result) +{ + if (result) { + nm_g_hash_table_unref(result->iface_datas); + g_free(result); + } +} /*****************************************************************************/ @@ -137,8 +159,8 @@ _get_config_task_maybe_return(NMCSProviderGetConfigTaskData *get_config_data, GE } else { _LOGD("get-config: success"); g_task_return_pointer(get_config_data->task, - g_hash_table_ref(get_config_data->result_dict), - (GDestroyNotify) g_hash_table_unref); + nmcs_provider_get_config_result_new(get_config_data->result_dict), + (GDestroyNotify) nmcs_provider_get_config_result_free); } nm_clear_g_signal_handler(g_task_get_cancellable(get_config_data->task), @@ -217,7 +239,7 @@ nmcs_provider_get_config(NMCSProvider * self, NMCS_PROVIDER_GET_CLASS(self)->get_config(self, get_config_data); } -GHashTable * +NMCSProviderGetConfigResult * nmcs_provider_get_config_finish(NMCSProvider *self, GAsyncResult *result, GError **error) { g_return_val_if_fail(NMCS_IS_PROVIDER(self), FALSE); diff --git a/clients/cloud-setup/nmcs-provider.h b/clients/cloud-setup/nmcs-provider.h index 3b0c2529ed..81aca966e4 100644 --- a/clients/cloud-setup/nmcs-provider.h +++ b/clients/cloud-setup/nmcs-provider.h @@ -41,6 +41,24 @@ nmcs_provider_get_config_iface_data_is_valid(const NMCSProviderGetConfigIfaceDat NMCSProviderGetConfigIfaceData *nmcs_provider_get_config_iface_data_new(gboolean was_requested); +/*****************************************************************************/ + +typedef struct { + /* A dictionary of (const char *) -> (NMCSProviderGetConfigIfaceData *). + * This is the per-interface result of get_config(). */ + GHashTable *iface_datas; +} NMCSProviderGetConfigResult; + +void nmcs_provider_get_config_result_free(NMCSProviderGetConfigResult *result); + +NM_AUTO_DEFINE_FCN0(NMCSProviderGetConfigResult *, + _nm_auto_free_nmcs_provider_get_config_result, + nmcs_provider_get_config_result_free); +#define nm_auto_free_nmcs_provider_get_config_result \ + nm_auto(_nm_auto_free_nmcs_provider_get_config_result) + +/*****************************************************************************/ + typedef struct { GTask *task; @@ -122,7 +140,7 @@ void nmcs_provider_get_config(NMCSProvider * provider, GAsyncReadyCallback callback, gpointer user_data); -GHashTable * +NMCSProviderGetConfigResult * nmcs_provider_get_config_finish(NMCSProvider *provider, GAsyncResult *result, GError **error); #endif /* __NMCS_PROVIDER_H__ */ From 7fcc89db6ee3701511f80efea5498bf1533dea8a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 09:42:37 +0200 Subject: [PATCH 04/17] cloud-setup: cache number of valid interfaces in get-config result Now that we return a struct from get_config(), we can have system-wide properties returned. Let it count and cache the number of valid iface_datas. Currently that is not yet used, but it will be. (cherry picked from commit a3cd66d3fadcecab9b186cc7f634f6ec6a5a92ee) (cherry picked from commit e74375fc3b68b07d1ed5f6ebca40cbe5b20ed47b) --- clients/cloud-setup/main.c | 22 +--------------------- clients/cloud-setup/nmcs-provider.c | 15 +++++++++++++-- clients/cloud-setup/nmcs-provider.h | 3 +++ 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index c57c5ab5ca..32d29ef4fa 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -360,26 +360,9 @@ _nmc_mangle_connection(NMDevice * device, /*****************************************************************************/ -static guint -_config_data_get_num_valid(const NMCSProviderGetConfigResult *result) -{ - const NMCSProviderGetConfigIfaceData *config_data; - GHashTableIter h_iter; - guint n = 0; - - g_hash_table_iter_init(&h_iter, result->iface_datas); - while (g_hash_table_iter_next(&h_iter, NULL, (gpointer *) &config_data)) { - if (nmcs_provider_get_config_iface_data_is_valid(config_data)) - n++; - } - - return n; -} - static gboolean _config_one(GCancellable * sigterm_cancellable, NMClient * nmc, - gboolean is_single_nic, const char * hwaddr, const NMCSProviderGetConfigIfaceData *config_data) { @@ -497,14 +480,11 @@ _config_all(GCancellable * sigterm_cancellable, GHashTableIter h_iter; const NMCSProviderGetConfigIfaceData *c_config_data; const char * c_hwaddr; - gboolean is_single_nic; gboolean any_changes = FALSE; - is_single_nic = (_config_data_get_num_valid(result) <= 1); - g_hash_table_iter_init(&h_iter, result->iface_datas); while (g_hash_table_iter_next(&h_iter, (gpointer *) &c_hwaddr, (gpointer *) &c_config_data)) { - if (_config_one(sigterm_cancellable, nmc, is_single_nic, c_hwaddr, c_config_data)) + if (_config_one(sigterm_cancellable, nmc, c_hwaddr, c_config_data)) any_changes = TRUE; } diff --git a/clients/cloud-setup/nmcs-provider.c b/clients/cloud-setup/nmcs-provider.c index d335193166..795532d344 100644 --- a/clients/cloud-setup/nmcs-provider.c +++ b/clients/cloud-setup/nmcs-provider.c @@ -54,12 +54,23 @@ nmcs_provider_get_main_context(NMCSProvider *self) static NMCSProviderGetConfigResult * nmcs_provider_get_config_result_new(GHashTable *iface_datas) { - NMCSProviderGetConfigResult *result; + const NMCSProviderGetConfigIfaceData *iface_data; + NMCSProviderGetConfigResult * result; + GHashTableIter h_iter; + guint num_valid_ifaces = 0; + + g_hash_table_iter_init(&h_iter, iface_datas); + while (g_hash_table_iter_next(&h_iter, NULL, (gpointer *) &iface_data)) { + if (nmcs_provider_get_config_iface_data_is_valid(iface_data)) + num_valid_ifaces++; + } result = g_new(NMCSProviderGetConfigResult, 1); *result = (NMCSProviderGetConfigResult){ - .iface_datas = g_hash_table_ref(iface_datas), + .iface_datas = g_hash_table_ref(iface_datas), + .num_valid_ifaces = num_valid_ifaces, }; + return result; } diff --git a/clients/cloud-setup/nmcs-provider.h b/clients/cloud-setup/nmcs-provider.h index 81aca966e4..6832aa7f07 100644 --- a/clients/cloud-setup/nmcs-provider.h +++ b/clients/cloud-setup/nmcs-provider.h @@ -47,6 +47,9 @@ typedef struct { /* A dictionary of (const char *) -> (NMCSProviderGetConfigIfaceData *). * This is the per-interface result of get_config(). */ GHashTable *iface_datas; + + /* The number of iface_datas that are nmcs_provider_get_config_iface_data_is_valid(). */ + guint num_valid_ifaces; } NMCSProviderGetConfigResult; void nmcs_provider_get_config_result_free(NMCSProviderGetConfigResult *result); From ef7a97977a9a0327f084eec9dfa34750c6ee7959 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 10:11:31 +0200 Subject: [PATCH 05/17] cloud-setup: count numbers of valid IPv4 addresses in get-config result Will be used next. (cherry picked from commit 7969ae1a82b90c3a9dbe33875d138c7b55cf6ac8) (cherry picked from commit ae504433f11480fde2436d3a5acba026db6c47bd) --- clients/cloud-setup/nmcs-provider.c | 6 +++++- clients/cloud-setup/nmcs-provider.h | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/clients/cloud-setup/nmcs-provider.c b/clients/cloud-setup/nmcs-provider.c index 795532d344..152b874a15 100644 --- a/clients/cloud-setup/nmcs-provider.c +++ b/clients/cloud-setup/nmcs-provider.c @@ -58,17 +58,21 @@ nmcs_provider_get_config_result_new(GHashTable *iface_datas) NMCSProviderGetConfigResult * result; GHashTableIter h_iter; guint num_valid_ifaces = 0; + guint num_ipv4s = 0; g_hash_table_iter_init(&h_iter, iface_datas); while (g_hash_table_iter_next(&h_iter, NULL, (gpointer *) &iface_data)) { - if (nmcs_provider_get_config_iface_data_is_valid(iface_data)) + if (nmcs_provider_get_config_iface_data_is_valid(iface_data)) { num_valid_ifaces++; + num_ipv4s += iface_data->ipv4s_len; + } } result = g_new(NMCSProviderGetConfigResult, 1); *result = (NMCSProviderGetConfigResult){ .iface_datas = g_hash_table_ref(iface_datas), .num_valid_ifaces = num_valid_ifaces, + .num_ipv4s = num_ipv4s, }; return result; diff --git a/clients/cloud-setup/nmcs-provider.h b/clients/cloud-setup/nmcs-provider.h index 6832aa7f07..9e6fb2cb87 100644 --- a/clients/cloud-setup/nmcs-provider.h +++ b/clients/cloud-setup/nmcs-provider.h @@ -50,6 +50,9 @@ typedef struct { /* The number of iface_datas that are nmcs_provider_get_config_iface_data_is_valid(). */ guint num_valid_ifaces; + + /* the number of IPv4 addresses over all valid iface_datas. */ + guint num_ipv4s; } NMCSProviderGetConfigResult; void nmcs_provider_get_config_result_free(NMCSProviderGetConfigResult *result); From 2b96e9314cb7d886b11e8787d28f540ef50ddd3c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Jun 2021 18:58:52 +0200 Subject: [PATCH 06/17] cloud-setup: preserve IPv4 addresses/routes/rules from profile nm-cloud-setup automatically detects routes, addresses and rules and configures them on the device using the emphermal Reapply() API. That is, it does not modify the existing profile (on disk), but changes the runtime configuration only. As such, it used to wipe otherwise statically configured IP addresses, routes and rules. That seems unnecessary. Let's keep the configuration from the (persistent) configuration. There is of course the problem that nm-cloud-setup doesn't really understand the existing IP configuration, and it can only hope that it can be meaningfully combined with what nm-cloud-setup wants to configure. This should cover most simple cases, for more complex setups, the user probably should disable nm-cloud-setup and configure the network explicitly to their liking. https://bugzilla.redhat.com/show_bug.cgi?id=1971527 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/893 (cherry picked from commit 4201ee5119e58ffef87115e5d12f928f728dba30) (cherry picked from commit 9541b0bea4082b25bdc11c7cfa0087d549e5ceeb) --- clients/cloud-setup/main.c | 49 +++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index 32d29ef4fa..9dadf3c618 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -270,15 +270,18 @@ _nmc_mangle_connection(NMDevice * device, const NMCSProviderGetConfigIfaceData *config_data, gboolean * out_changed) { - NMSettingIPConfig *s_ip; - gsize i; - in_addr_t gateway; - gint64 rt_metric; - guint32 rt_table; - NMIPRoute * route_entry; - gboolean addrs_changed = FALSE; - gboolean rules_changed = FALSE; - gboolean routes_changed = FALSE; + NMSettingIPConfig * s_ip; + NMActiveConnection *ac; + NMConnection * remote_connection; + NMSettingIPConfig * remote_s_ip = NULL; + gsize i; + in_addr_t gateway; + gint64 rt_metric; + guint32 rt_table; + NMIPRoute * route_entry; + gboolean addrs_changed = FALSE; + gboolean rules_changed = FALSE; + gboolean routes_changed = FALSE; gs_unref_ptrarray GPtrArray *addrs_new = NULL; gs_unref_ptrarray GPtrArray *rules_new = NULL; gs_unref_ptrarray GPtrArray *routes_new = NULL; @@ -290,12 +293,40 @@ _nmc_mangle_connection(NMDevice * device, if (!s_ip) return FALSE; + if ((ac = nm_device_get_active_connection(device)) + && (remote_connection = NM_CONNECTION(nm_active_connection_get_connection(ac)))) + remote_s_ip = nm_connection_get_setting_ip4_config(remote_connection); + addrs_new = g_ptr_array_new_full(config_data->ipv4s_len, (GDestroyNotify) nm_ip_address_unref); rules_new = g_ptr_array_new_full(config_data->ipv4s_len, (GDestroyNotify) nm_ip_routing_rule_unref); routes_new = g_ptr_array_new_full(config_data->iproutes_len + !!config_data->ipv4s_len, (GDestroyNotify) nm_ip_route_unref); + if (remote_s_ip) { + guint len; + guint j; + + len = nm_setting_ip_config_get_num_addresses(remote_s_ip); + for (j = 0; j < len; j++) { + g_ptr_array_add(addrs_new, + nm_ip_address_dup(nm_setting_ip_config_get_address(remote_s_ip, j))); + } + + len = nm_setting_ip_config_get_num_routes(remote_s_ip); + for (j = 0; j < len; j++) { + g_ptr_array_add(routes_new, + nm_ip_route_dup(nm_setting_ip_config_get_route(remote_s_ip, j))); + } + + len = nm_setting_ip_config_get_num_routing_rules(remote_s_ip); + for (j = 0; j < len; j++) { + g_ptr_array_add( + rules_new, + nm_ip_routing_rule_ref(nm_setting_ip_config_get_routing_rule(remote_s_ip, j))); + } + } + if (config_data->has_ipv4s && config_data->has_cidr) { for (i = 0; i < config_data->ipv4s_len; i++) { NMIPAddress *entry; From 2e4b73c7fe8168701c2561dc6563c027ac92f456 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 10:15:37 +0200 Subject: [PATCH 07/17] cloud-setup: skip configuring policy routing if there is only one interface/address nm-cloud-setup automatically configures the network. That may conflict with what the user wants. In case the user configures some specific setup, they are encouraged to disable nm-cloud-setup (and its automatism). Still, what we do by default matters, and should play as well with user's expectations. Configuring policy routing and a higher priority table (30400+) that hijacks the traffic can cause problems. If the system only has one IPv4 address and one interface, then there is no point in configuring policy routing at all. Detect that, and skip the change in that case. Note that of course we need to handle the case where previously multiple IP addresses were configured and an update gives only one address. In that case we need to clear the previously configured rules/routes. The patch achieves this. (cherry picked from commit 5f047968d7a48999d20001f83e2005caa43c80ce) (cherry picked from commit 8bc8a0f56b97c28cf26fd678a549db41399adcb7) --- clients/cloud-setup/main.c | 41 ++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index 9dadf3c618..5c6d70c07b 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -267,7 +267,9 @@ _nmc_skip_connection(NMConnection *connection) static gboolean _nmc_mangle_connection(NMDevice * device, NMConnection * connection, + const NMCSProviderGetConfigResult * result, const NMCSProviderGetConfigIfaceData *config_data, + gboolean * out_skipped_single_addr, gboolean * out_changed) { NMSettingIPConfig * s_ip; @@ -286,6 +288,9 @@ _nmc_mangle_connection(NMDevice * device, gs_unref_ptrarray GPtrArray *rules_new = NULL; gs_unref_ptrarray GPtrArray *routes_new = NULL; + NM_SET_OUT(out_skipped_single_addr, FALSE); + NM_SET_OUT(out_changed, FALSE); + if (!nm_streq0(nm_connection_get_connection_type(connection), NM_SETTING_WIRED_SETTING_NAME)) return FALSE; @@ -327,7 +332,11 @@ _nmc_mangle_connection(NMDevice * device, } } - if (config_data->has_ipv4s && config_data->has_cidr) { + if (result->num_valid_ifaces <= 1 && result->num_ipv4s <= 1) { + /* this setup only has one interface and one IPv4 address (or less). + * We don't need to configure policy routing in this case. */ + NM_SET_OUT(out_skipped_single_addr, TRUE); + } else if (config_data->has_ipv4s && config_data->has_cidr) { for (i = 0; i < config_data->ipv4s_len; i++) { NMIPAddress *entry; @@ -394,6 +403,7 @@ _nmc_mangle_connection(NMDevice * device, static gboolean _config_one(GCancellable * sigterm_cancellable, NMClient * nmc, + const NMCSProviderGetConfigResult * result, const char * hwaddr, const NMCSProviderGetConfigIfaceData *config_data) { @@ -402,6 +412,7 @@ _config_one(GCancellable * sigterm_cancellable, guint64 applied_version_id; gs_free_error GError *error = NULL; gboolean changed; + gboolean skipped_single_addr; gboolean version_id_changed; guint try_count; gboolean any_changes = FALSE; @@ -450,16 +461,30 @@ try_again: return any_changes; } - if (!_nmc_mangle_connection(device, applied_connection, config_data, &changed)) { + if (!_nmc_mangle_connection(device, + applied_connection, + result, + config_data, + &skipped_single_addr, + &changed)) { _LOGD("config device %s: device has no suitable applied connection. Skip", hwaddr); return any_changes; } if (!changed) { - _LOGD("config device %s: device needs no update to applied connection \"%s\" (%s). Skip", - hwaddr, - nm_connection_get_id(applied_connection), - nm_connection_get_uuid(applied_connection)); + if (skipped_single_addr) { + _LOGD("config device %s: device needs no update to applied connection \"%s\" (%s) " + "because there are not multiple IP addresses. Skip", + hwaddr, + nm_connection_get_id(applied_connection), + nm_connection_get_uuid(applied_connection)); + } else { + _LOGD( + "config device %s: device needs no update to applied connection \"%s\" (%s). Skip", + hwaddr, + nm_connection_get_id(applied_connection), + nm_connection_get_uuid(applied_connection)); + } return any_changes; } @@ -468,7 +493,7 @@ try_again: nm_connection_get_id(applied_connection), nm_connection_get_uuid(applied_connection)); - /* we are about to call Reapply(). If if that fails, it counts as if we changed something. */ + /* we are about to call Reapply(). Even if that fails, it counts as if we changed something. */ any_changes = TRUE; if (!nmcs_device_reapply(device, @@ -515,7 +540,7 @@ _config_all(GCancellable * sigterm_cancellable, g_hash_table_iter_init(&h_iter, result->iface_datas); while (g_hash_table_iter_next(&h_iter, (gpointer *) &c_hwaddr, (gpointer *) &c_config_data)) { - if (_config_one(sigterm_cancellable, nmc, c_hwaddr, c_config_data)) + if (_config_one(sigterm_cancellable, nmc, result, c_hwaddr, c_config_data)) any_changes = TRUE; } From 91ddd5f5bacf75f8819fbb014ff4bfd47c0c1ae3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Apr 2021 13:46:47 +0200 Subject: [PATCH 08/17] cloud-setup: use _nm_utils_ascii_str_to_int64_bin() in Azure's _get_config_fetch_done_cb() (cherry picked from commit a2fded3cee0f6d4da86962bfc671f03bfe4c5da4) --- clients/cloud-setup/nmcs-provider-azure.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-azure.c b/clients/cloud-setup/nmcs-provider-azure.c index 0a5d522cc3..cedaad6b77 100644 --- a/clients/cloud-setup/nmcs-provider-azure.c +++ b/clients/cloud-setup/nmcs-provider-azure.c @@ -154,14 +154,9 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, iface_get_config->has_ipv4s = TRUE; iface_get_config->ipv4s_len++; } else { - int tmp_prefix = -1; - - if (fip_len > 0 && memchr(fip_str, '\0', fip_len - 1)) { - /* we have an embedded "\0" inside the string (except trailing). That is not - * allowed*/ - } else - tmp_prefix = _nm_utils_ascii_str_to_int64(fip_str, 10, 0, 32, -1); + int tmp_prefix; + tmp_prefix = _nm_utils_ascii_str_to_int64_bin(fip_str, fip_len, 10, 0, 32, -1); if (tmp_prefix == -1) { _LOGD("interface[%" G_GSSIZE_FORMAT "]: invalid prefix", iface_data->intern_iface_idx); error = @@ -244,7 +239,6 @@ _get_config_ips_prefix_list_cb(GObject *source, GAsyncResult *result, gpointer u ((char *) line)[--line_len] = '\0'; ips_prefix_idx = _nm_utils_ascii_str_to_int64(line, 10, 0, G_MAXINT64, -1); - if (ips_prefix_idx < 0) continue; From 8bad924931176c487776ab16c0768c54a6ab2002 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Apr 2021 13:49:04 +0200 Subject: [PATCH 09/17] cloud-setup/trivial: rename variables in Azure's _get_config_fetch_done_cb() The previous name seem not very expressive/fitting. Naming is hard, but I think these are better names. (cherry picked from commit 89f326785910ca7a4e6994b9d4052f8ab43fc63e) --- clients/cloud-setup/nmcs-provider-azure.c | 26 +++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-azure.c b/clients/cloud-setup/nmcs-provider-azure.c index cedaad6b77..33d94db94e 100644 --- a/clients/cloud-setup/nmcs-provider-azure.c +++ b/clients/cloud-setup/nmcs-provider-azure.c @@ -98,7 +98,7 @@ typedef struct { NMCSProviderGetConfigIfaceData *iface_get_config; gssize intern_iface_idx; gssize extern_iface_idx; - guint n_ips_prefix_pending; + guint n_iface_data_pending; const char * hwaddr; } AzureIfaceData; @@ -118,8 +118,8 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, NMCSProviderGetConfigIfaceData *iface_get_config; gs_unref_bytes GBytes *response = NULL; gs_free_error GError *error = NULL; - const char * fip_str = NULL; - gsize fip_len; + const char * resp_str = NULL; + gsize resp_len; nm_http_client_poll_get_finish(http_client, result, NULL, &response, &error); @@ -131,8 +131,8 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, if (error) goto out_done; - fip_str = g_bytes_get_data(response, &fip_len); - nm_assert(fip_str[fip_len] == '\0'); + resp_str = g_bytes_get_data(response, &resp_len); + nm_assert(resp_str[resp_len] == '\0'); iface_data->iface_get_config = g_hash_table_lookup(get_config_data->result_dict, iface_data->hwaddr); @@ -142,7 +142,7 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, char tmp_addr_str[NM_UTILS_INET_ADDRSTRLEN]; in_addr_t tmp_addr; - if (!nmcs_utils_ipaddr_normalize_bin(AF_INET, fip_str, fip_len, NULL, &tmp_addr)) { + if (!nmcs_utils_ipaddr_normalize_bin(AF_INET, resp_str, resp_len, NULL, &tmp_addr)) { error = nm_utils_error_new(NM_UTILS_ERROR_UNKNOWN, "ip is not a valid private ip address"); goto out_done; @@ -156,7 +156,7 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, } else { int tmp_prefix; - tmp_prefix = _nm_utils_ascii_str_to_int64_bin(fip_str, fip_len, 10, 0, 32, -1); + tmp_prefix = _nm_utils_ascii_str_to_int64_bin(resp_str, resp_len, 10, 0, 32, -1); if (tmp_prefix == -1) { _LOGD("interface[%" G_GSSIZE_FORMAT "]: invalid prefix", iface_data->intern_iface_idx); error = @@ -173,8 +173,8 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, out_done: if (!error) { - --iface_data->n_ips_prefix_pending; - if (iface_data->n_ips_prefix_pending > 0) + --iface_data->n_iface_data_pending; + if (iface_data->n_iface_data_pending > 0) return; } @@ -246,7 +246,7 @@ _get_config_ips_prefix_list_cb(GObject *source, GAsyncResult *result, gpointer u gs_free const char *uri = NULL; char buf[100]; - iface_data->n_ips_prefix_pending++; + iface_data->n_iface_data_pending++; nm_http_client_poll_get( NM_HTTP_CLIENT(source), @@ -269,13 +269,13 @@ _get_config_ips_prefix_list_cb(GObject *source, GAsyncResult *result, gpointer u } iface_data->iface_get_config->ipv4s_len = 0; - iface_data->iface_get_config->ipv4s_arr = g_new(in_addr_t, iface_data->n_ips_prefix_pending); + iface_data->iface_get_config->ipv4s_arr = g_new(in_addr_t, iface_data->n_iface_data_pending); { gs_free const char *uri = NULL; char buf[30]; - iface_data->n_ips_prefix_pending++; + iface_data->n_iface_data_pending++; nm_http_client_poll_get( NM_HTTP_CLIENT(source), (uri = _azure_uri_interfaces( @@ -439,7 +439,7 @@ _get_net_ifaces_list_cb(GObject *source, GAsyncResult *result, gpointer user_dat .iface_get_config = NULL, .intern_iface_idx = intern_iface_idx, .extern_iface_idx = extern_iface_idx_cnt++, - .n_ips_prefix_pending = 0, + .n_iface_data_pending = 0, .hwaddr = NULL, }; g_ptr_array_add(ifaces_arr, iface_data); From c36e42dbd9c0be96146888d610ad187fa57802e2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 14:34:51 +0200 Subject: [PATCH 10/17] cloud-setup: add "hwaddr" to NMCSProviderGetConfigIfaceData struct get-config() gives a NMCSProviderGetConfigResult structure, and the main part of data is the GHashTable of MAC addresses and NMCSProviderGetConfigIfaceData instances. Let NMCSProviderGetConfigIfaceData also have a reference to the MAC address. This way, I'll be able to create a (sorted) list of interface datas, that also contain the MAC address. (cherry picked from commit ec56fe60fbf31768d0d1cae8bb325c1fdf7dbf07) (cherry picked from commit cc289e53699872a3617aef321453ef6a885d0148) --- clients/cloud-setup/nmcs-provider-azure.c | 35 ++++++++++++++--------- clients/cloud-setup/nmcs-provider-ec2.c | 27 +++++++---------- clients/cloud-setup/nmcs-provider-gcp.c | 20 ++++++------- clients/cloud-setup/nmcs-provider.c | 22 +++++++++----- clients/cloud-setup/nmcs-provider.h | 12 ++++++-- 5 files changed, 66 insertions(+), 50 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-azure.c b/clients/cloud-setup/nmcs-provider-azure.c index 33d94db94e..3b923e4ce5 100644 --- a/clients/cloud-setup/nmcs-provider-azure.c +++ b/clients/cloud-setup/nmcs-provider-azure.c @@ -99,7 +99,6 @@ typedef struct { gssize intern_iface_idx; gssize extern_iface_idx; guint n_iface_data_pending; - const char * hwaddr; } AzureIfaceData; static void @@ -118,6 +117,7 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, NMCSProviderGetConfigIfaceData *iface_get_config; gs_unref_bytes GBytes *response = NULL; gs_free_error GError *error = NULL; + gs_free char * v_hwaddr = NULL; const char * resp_str = NULL; gsize resp_len; @@ -134,8 +134,17 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, resp_str = g_bytes_get_data(response, &resp_len); nm_assert(resp_str[resp_len] == '\0'); - iface_data->iface_get_config = - g_hash_table_lookup(get_config_data->result_dict, iface_data->hwaddr); + v_hwaddr = nmcs_utils_hwaddr_normalize_gbytes(response); + if (!v_hwaddr) { + _LOGI("interface[%" G_GSSIZE_FORMAT "]: invalid MAC address returned", + iface_data->intern_iface_idx); + error = nm_utils_error_new(NM_UTILS_ERROR_UNKNOWN, + "invalid MAC address for index %" G_GSSIZE_FORMAT, + iface_data->intern_iface_idx); + goto out_done; + } + + iface_data->iface_get_config = g_hash_table_lookup(get_config_data->result_dict, v_hwaddr); iface_get_config = iface_data->iface_get_config; if (is_ipv4) { @@ -330,25 +339,24 @@ _get_config_iface_cb(GObject *source, GAsyncResult *result, gpointer user_data) goto out_done; } - if (!g_hash_table_lookup_extended(get_config_data->result_dict, - v_hwaddr, - (gpointer *) &iface_data->hwaddr, - (gpointer *) &iface_data->iface_get_config)) { + iface_data->iface_get_config = g_hash_table_lookup(get_config_data->result_dict, v_hwaddr); + + if (!iface_data->iface_get_config) { if (!get_config_data->any) { _LOGD("get-config: skip fetching meta data for %s (%" G_GSSIZE_FORMAT ")", v_hwaddr, iface_data->intern_iface_idx); goto out_done; } - iface_data->iface_get_config = nmcs_provider_get_config_iface_data_new(FALSE); - g_hash_table_insert(get_config_data->result_dict, - (char *) (iface_data->hwaddr = g_steal_pointer(&v_hwaddr)), - iface_data->iface_get_config); + iface_data->iface_get_config = + nmcs_provider_get_config_iface_data_create(get_config_data->result_dict, + FALSE, + v_hwaddr); } else { if (iface_data->iface_get_config->iface_idx >= 0) { _LOGI("interface[%" G_GSSIZE_FORMAT "]: duplicate MAC address %s returned", iface_data->intern_iface_idx, - iface_data->hwaddr); + iface_data->iface_get_config->hwaddr); error = nm_utils_error_new(NM_UTILS_ERROR_UNKNOWN, "duplicate MAC address for index %" G_GSSIZE_FORMAT, iface_data->intern_iface_idx); @@ -360,7 +368,7 @@ _get_config_iface_cb(GObject *source, GAsyncResult *result, gpointer user_data) _LOGD("interface[%" G_GSSIZE_FORMAT "]: found a matching device with hwaddr %s", iface_data->intern_iface_idx, - iface_data->hwaddr); + iface_data->iface_get_config->hwaddr); nm_sprintf_buf(buf, "%" G_GSSIZE_FORMAT "/ipv4/ipAddress/", iface_data->intern_iface_idx); @@ -440,7 +448,6 @@ _get_net_ifaces_list_cb(GObject *source, GAsyncResult *result, gpointer user_dat .intern_iface_idx = intern_iface_idx, .extern_iface_idx = extern_iface_idx_cnt++, .n_iface_data_pending = 0, - .hwaddr = NULL, }; g_ptr_array_add(ifaces_arr, iface_data); } diff --git a/clients/cloud-setup/nmcs-provider-ec2.c b/clients/cloud-setup/nmcs-provider-ec2.c index 1e060034f5..ac449271fb 100644 --- a/clients/cloud-setup/nmcs-provider-ec2.c +++ b/clients/cloud-setup/nmcs-provider-ec2.c @@ -136,14 +136,13 @@ _get_config_fetch_done_cb(NMHttpClient *http_client, gboolean is_local_ipv4) { NMCSProviderGetConfigTaskData *get_config_data; - const char * hwaddr = NULL; gs_unref_bytes GBytes *response = NULL; gs_free_error GError * error = NULL; NMCSProviderGetConfigIfaceData *config_iface_data; in_addr_t tmp_addr; int tmp_prefix; - nm_utils_user_data_unpack(user_data, &get_config_data, &hwaddr); + nm_utils_user_data_unpack(user_data, &get_config_data, &config_iface_data); nm_http_client_poll_get_finish(http_client, result, NULL, &response, &error); @@ -153,8 +152,6 @@ _get_config_fetch_done_cb(NMHttpClient *http_client, if (error) goto out; - config_iface_data = g_hash_table_lookup(get_config_data->result_dict, hwaddr); - if (is_local_ipv4) { gs_free const char **s_addrs = NULL; gsize i, len; @@ -250,22 +247,20 @@ _get_config_metadata_ready_cb(GObject *source, GAsyncResult *result, gpointer us NMCSProviderGetConfigIfaceData *config_iface_data; gs_free char * uri1 = NULL; gs_free char * uri2 = NULL; - const char * hwaddr; - if (!g_hash_table_lookup_extended(get_config_data->result_dict, - v_hwaddr, - (gpointer *) &hwaddr, - (gpointer *) &config_iface_data)) { + config_iface_data = g_hash_table_lookup(get_config_data->result_dict, v_hwaddr); + + if (!config_iface_data) { if (!get_config_data->any) { _LOGD("get-config: skip fetching meta data for %s (%s)", v_hwaddr, v_mac_data->path); continue; } - config_iface_data = nmcs_provider_get_config_iface_data_new(FALSE); - g_hash_table_insert(get_config_data->result_dict, - (char *) (hwaddr = g_strdup(v_hwaddr)), - config_iface_data); + config_iface_data = + nmcs_provider_get_config_iface_data_create(get_config_data->result_dict, + FALSE, + v_hwaddr); } nm_assert(config_iface_data->iface_idx == -1); @@ -274,7 +269,7 @@ _get_config_metadata_ready_cb(GObject *source, GAsyncResult *result, gpointer us _LOGD("get-config: start fetching meta data for #%" G_GSSIZE_FORMAT ", %s (%s)", config_iface_data->iface_idx, - hwaddr, + config_iface_data->hwaddr, v_mac_data->path); get_config_data->n_pending++; @@ -292,7 +287,7 @@ _get_config_metadata_ready_cb(GObject *source, GAsyncResult *result, gpointer us NULL, NULL, _get_config_fetch_done_cb_subnet_ipv4_cidr_block, - nm_utils_user_data_pack(get_config_data, hwaddr)); + nm_utils_user_data_pack(get_config_data, config_iface_data)); get_config_data->n_pending++; nm_http_client_poll_get( @@ -309,7 +304,7 @@ _get_config_metadata_ready_cb(GObject *source, GAsyncResult *result, gpointer us NULL, NULL, _get_config_fetch_done_cb_local_ipv4s, - nm_utils_user_data_pack(get_config_data, hwaddr)); + nm_utils_user_data_pack(get_config_data, config_iface_data)); } _nmcs_provider_get_config_task_maybe_return(get_config_data, NULL); diff --git a/clients/cloud-setup/nmcs-provider-gcp.c b/clients/cloud-setup/nmcs-provider-gcp.c index 1deaea10f3..2389c9ee18 100644 --- a/clients/cloud-setup/nmcs-provider-gcp.c +++ b/clients/cloud-setup/nmcs-provider-gcp.c @@ -247,7 +247,6 @@ _get_config_iface_cb(GObject *source, GAsyncResult *result, gpointer user_data) GCPIfaceData * iface_data = user_data; gs_free_error GError * error = NULL; gs_free char * v_hwaddr = NULL; - const char * hwaddr = NULL; gs_free const char * uri = NULL; char sbuf[100]; NMCSProviderGetConfigTaskData *get_config_data; @@ -273,26 +272,25 @@ _get_config_iface_cb(GObject *source, GAsyncResult *result, gpointer user_data) goto out_done; } - if (!g_hash_table_lookup_extended(get_config_data->result_dict, - v_hwaddr, - (gpointer *) &hwaddr, - (gpointer *) &iface_data->iface_get_config)) { + iface_data->iface_get_config = g_hash_table_lookup(get_config_data->result_dict, v_hwaddr); + + if (!iface_data->iface_get_config) { if (!get_config_data->any) { _LOGD("get-config: skip fetching meta data for %s (%" G_GSSIZE_FORMAT ")", v_hwaddr, iface_data->intern_iface_idx); goto out_done; } - iface_data->iface_get_config = nmcs_provider_get_config_iface_data_new(FALSE); - g_hash_table_insert(get_config_data->result_dict, - (char *) (hwaddr = g_steal_pointer(&v_hwaddr)), - iface_data->iface_get_config); + iface_data->iface_get_config = + nmcs_provider_get_config_iface_data_create(get_config_data->result_dict, + FALSE, + v_hwaddr); is_requested = FALSE; } else { if (iface_data->iface_get_config->iface_idx >= 0) { _LOGI("GCP interface[%" G_GSSIZE_FORMAT "]: duplicate MAC address %s returned", iface_data->intern_iface_idx, - hwaddr); + iface_data->iface_get_config->hwaddr); error = nm_utils_error_new(NM_UTILS_ERROR_UNKNOWN, "duplicate MAC address for index %" G_GSSIZE_FORMAT, iface_data->intern_iface_idx); @@ -306,7 +304,7 @@ _get_config_iface_cb(GObject *source, GAsyncResult *result, gpointer user_data) _LOGI("GCP interface[%" G_GSSIZE_FORMAT "]: found a %sdevice with hwaddr %s", iface_data->intern_iface_idx, is_requested ? "requested " : "", - hwaddr); + iface_data->iface_get_config->hwaddr); nm_sprintf_buf(sbuf, "%" G_GSSIZE_FORMAT "/forwarded-ips/", iface_data->intern_iface_idx); diff --git a/clients/cloud-setup/nmcs-provider.c b/clients/cloud-setup/nmcs-provider.c index 152b874a15..1ac5cd42bb 100644 --- a/clients/cloud-setup/nmcs-provider.c +++ b/clients/cloud-setup/nmcs-provider.c @@ -127,15 +127,25 @@ nmcs_provider_detect_finish(NMCSProvider *self, GAsyncResult *result, GError **e /*****************************************************************************/ NMCSProviderGetConfigIfaceData * -nmcs_provider_get_config_iface_data_new(gboolean was_requested) +nmcs_provider_get_config_iface_data_create(GHashTable *iface_datas, + gboolean was_requested, + const char *hwaddr) { NMCSProviderGetConfigIfaceData *iface_data; + nm_assert(hwaddr); + iface_data = g_slice_new(NMCSProviderGetConfigIfaceData); *iface_data = (NMCSProviderGetConfigIfaceData){ + .hwaddr = g_strdup(hwaddr), .iface_idx = -1, .was_requested = was_requested, }; + + /* the has does not own the key (iface_datta->hwaddr), the lifetime of the + * key is associated with the iface_data instance. */ + g_hash_table_replace(iface_datas, (char *) iface_data->hwaddr, iface_data); + return iface_data; } @@ -146,6 +156,7 @@ _iface_data_free(gpointer data) g_free(iface_data->ipv4s_arr); g_free(iface_data->iproutes_arr); + g_free((char *) iface_data->hwaddr); nm_g_slice_free(iface_data); } @@ -224,16 +235,13 @@ nmcs_provider_get_config(NMCSProvider * self, *get_config_data = (NMCSProviderGetConfigTaskData){ .task = nm_g_task_new(self, cancellable, nmcs_provider_get_config, callback, user_data), .any = any, - .result_dict = g_hash_table_new_full(nm_str_hash, g_str_equal, g_free, _iface_data_free), + .result_dict = g_hash_table_new_full(nm_str_hash, g_str_equal, NULL, _iface_data_free), }; nmcs_wait_for_objects_register(get_config_data->task); - for (; hwaddrs && hwaddrs[0]; hwaddrs++) { - g_hash_table_insert(get_config_data->result_dict, - g_strdup(hwaddrs[0]), - nmcs_provider_get_config_iface_data_new(TRUE)); - } + for (; hwaddrs && hwaddrs[0]; hwaddrs++) + nmcs_provider_get_config_iface_data_create(get_config_data->result_dict, TRUE, hwaddrs[0]); if (cancellable) { gulong cancelled_id; diff --git a/clients/cloud-setup/nmcs-provider.h b/clients/cloud-setup/nmcs-provider.h index 9e6fb2cb87..19bf56bd05 100644 --- a/clients/cloud-setup/nmcs-provider.h +++ b/clients/cloud-setup/nmcs-provider.h @@ -10,6 +10,10 @@ /*****************************************************************************/ typedef struct { + /* And it's exactly the same pointer that is also the key for the iface_datas + * dictionary. */ + const char *hwaddr; + in_addr_t *ipv4s_arr; gsize ipv4s_len; @@ -39,13 +43,17 @@ nmcs_provider_get_config_iface_data_is_valid(const NMCSProviderGetConfigIfaceDat && ((config_data->has_ipv4s && config_data->has_cidr) || config_data->iproutes_len); } -NMCSProviderGetConfigIfaceData *nmcs_provider_get_config_iface_data_new(gboolean was_requested); +NMCSProviderGetConfigIfaceData *nmcs_provider_get_config_iface_data_create(GHashTable *iface_datas, + gboolean was_requested, + const char *hwaddr); /*****************************************************************************/ typedef struct { /* A dictionary of (const char *) -> (NMCSProviderGetConfigIfaceData *). - * This is the per-interface result of get_config(). */ + * This is the per-interface result of get_config(). + * + * The key is the same pointer as NMCSProviderGetConfigIfaceData's hwaddr. */ GHashTable *iface_datas; /* The number of iface_datas that are nmcs_provider_get_config_iface_data_is_valid(). */ From 48e79fb4b29f297d29502e75a8956ffe65fe51db Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 16:59:19 +0200 Subject: [PATCH 11/17] cloud-setup: track sorted list of NMCSProviderGetConfigIfaceData Sorted by iface_idx. The iface_idx is probably something useful and stable, provided by the provider. E.g. it's the order in which interfaces are exposed on the meta data. (cherry picked from commit 1c5cb9d3c2be5addd3b011873cfc6b99204955d1) (cherry picked from commit 0a2ed627038349d838add8f3fd72e2d282e74693) --- clients/cloud-setup/nmcs-provider.c | 49 ++++++++++++++++++++++++++++- clients/cloud-setup/nmcs-provider.h | 8 +++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/clients/cloud-setup/nmcs-provider.c b/clients/cloud-setup/nmcs-provider.c index 1ac5cd42bb..8dd5cc1997 100644 --- a/clients/cloud-setup/nmcs-provider.c +++ b/clients/cloud-setup/nmcs-provider.c @@ -51,6 +51,19 @@ nmcs_provider_get_main_context(NMCSProvider *self) } /*****************************************************************************/ +static int +_result_new_sort_iface_data(gconstpointer pa, gconstpointer pb) +{ + const NMCSProviderGetConfigIfaceData *a = *((const NMCSProviderGetConfigIfaceData *const *) pa); + const NMCSProviderGetConfigIfaceData *b = *((const NMCSProviderGetConfigIfaceData *const *) pb); + + /* negative iface_idx are sorted to the end. */ + NM_CMP_DIRECT((a->iface_idx < 0), (b->iface_idx < 0)); + + NM_CMP_FIELD(a, b, iface_idx); + return 0; +} + static NMCSProviderGetConfigResult * nmcs_provider_get_config_result_new(GHashTable *iface_datas) { @@ -59,6 +72,12 @@ nmcs_provider_get_config_result_new(GHashTable *iface_datas) GHashTableIter h_iter; guint num_valid_ifaces = 0; guint num_ipv4s = 0; + GPtrArray * ptrarr; + guint n_iface_datas; + + n_iface_datas = g_hash_table_size(iface_datas); + + ptrarr = g_ptr_array_sized_new(n_iface_datas + 1u); g_hash_table_iter_init(&h_iter, iface_datas); while (g_hash_table_iter_next(&h_iter, NULL, (gpointer *) &iface_data)) { @@ -66,15 +85,42 @@ nmcs_provider_get_config_result_new(GHashTable *iface_datas) num_valid_ifaces++; num_ipv4s += iface_data->ipv4s_len; } + g_ptr_array_add(ptrarr, (gpointer) iface_data); } + g_ptr_array_sort(ptrarr, _result_new_sort_iface_data); + + nm_assert(n_iface_datas == ptrarr->len); + + g_ptr_array_add(ptrarr, NULL); + result = g_new(NMCSProviderGetConfigResult, 1); *result = (NMCSProviderGetConfigResult){ - .iface_datas = g_hash_table_ref(iface_datas), + .iface_datas = g_hash_table_ref(iface_datas), + .n_iface_datas = n_iface_datas, + .iface_datas_arr = + (const NMCSProviderGetConfigIfaceData **) g_ptr_array_free(ptrarr, FALSE), .num_valid_ifaces = num_valid_ifaces, .num_ipv4s = num_ipv4s, }; +#if NM_MORE_ASSERTS > 5 + { + gsize iface_idx_expected = 0; + guint i; + + for (i = 0; i < result->n_iface_datas; i++) { + if (result->iface_datas_arr[i]->iface_idx < 0) { + nm_assert(result->iface_datas_arr[i]->iface_idx == -1); + iface_idx_expected = -1; + continue; + } + nm_assert(result->iface_datas_arr[i]->iface_idx == iface_idx_expected); + iface_idx_expected++; + } + } +#endif + return result; } @@ -83,6 +129,7 @@ nmcs_provider_get_config_result_free(NMCSProviderGetConfigResult *result) { if (result) { nm_g_hash_table_unref(result->iface_datas); + g_free((gpointer) result->iface_datas_arr); g_free(result); } } diff --git a/clients/cloud-setup/nmcs-provider.h b/clients/cloud-setup/nmcs-provider.h index 19bf56bd05..261f589f2f 100644 --- a/clients/cloud-setup/nmcs-provider.h +++ b/clients/cloud-setup/nmcs-provider.h @@ -61,6 +61,14 @@ typedef struct { /* the number of IPv4 addresses over all valid iface_datas. */ guint num_ipv4s; + + guint n_iface_datas; + + /* The sorted value of @iface_datas, sorted by iface_idx. + * + * Not found entries (iface_idx == -1) are sorted at the end. */ + const NMCSProviderGetConfigIfaceData *const *iface_datas_arr; + } NMCSProviderGetConfigResult; void nmcs_provider_get_config_result_free(NMCSProviderGetConfigResult *result); From 67a83b54cd58e44ef50e5e3a7c6a9463995235dd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 17:23:09 +0200 Subject: [PATCH 12/17] cloud-setup: process iface-datas in sorted order The routes/rules that are configured are independent of the order in which we process the devices. That is, because they use the "iface_idx" for cases where there is ambiguity. Still, it feels nicer to always process them in a defined order. (cherry picked from commit a95ea0eb294d646f17b5e1cf4f17cb1540f8af3a) (cherry picked from commit 6302cd416d92a8c2f5b4a6be9dded71af4cf7ced) --- clients/cloud-setup/main.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index 5c6d70c07b..cbea266813 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -401,14 +401,15 @@ _nmc_mangle_connection(NMDevice * device, /*****************************************************************************/ static gboolean -_config_one(GCancellable * sigterm_cancellable, - NMClient * nmc, - const NMCSProviderGetConfigResult * result, - const char * hwaddr, - const NMCSProviderGetConfigIfaceData *config_data) +_config_one(GCancellable * sigterm_cancellable, + NMClient * nmc, + const NMCSProviderGetConfigResult *result, + guint idx) { - gs_unref_object NMDevice *device = NULL; - gs_unref_object NMConnection *applied_connection = NULL; + const NMCSProviderGetConfigIfaceData *config_data = result->iface_datas_arr[idx]; + const char * hwaddr = config_data->hwaddr; + gs_unref_object NMDevice *device = NULL; + gs_unref_object NMConnection *applied_connection = NULL; guint64 applied_version_id; gs_free_error GError *error = NULL; gboolean changed; @@ -533,14 +534,11 @@ _config_all(GCancellable * sigterm_cancellable, NMClient * nmc, const NMCSProviderGetConfigResult *result) { - GHashTableIter h_iter; - const NMCSProviderGetConfigIfaceData *c_config_data; - const char * c_hwaddr; - gboolean any_changes = FALSE; + gboolean any_changes = FALSE; + guint i; - g_hash_table_iter_init(&h_iter, result->iface_datas); - while (g_hash_table_iter_next(&h_iter, (gpointer *) &c_hwaddr, (gpointer *) &c_config_data)) { - if (_config_one(sigterm_cancellable, nmc, result, c_hwaddr, c_config_data)) + for (i = 0; i < result->n_iface_datas; i++) { + if (_config_one(sigterm_cancellable, nmc, result, i)) any_changes = TRUE; } From 661da869b3419c0761296caaf12002e2eba25f48 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 19:23:46 +0200 Subject: [PATCH 13/17] cloud-setup: limit number of supported interfaces to avoid overlapping table numbers The table number is chosen as 30400 + iface_idx. That is, the range is limited and we shouldn't handle more than 100 devices. Add a check for that and error out. (cherry picked from commit b68d694b78fd9b4b63b0592a2518f387aaa35f87) (cherry picked from commit 292233e16ed1f60499c2676611d59c271352e2c3) --- clients/cloud-setup/main.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index cbea266813..9fa4cd3c84 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -434,6 +434,14 @@ _config_one(GCancellable * sigterm_cancellable, return FALSE; } + if (config_data->iface_idx >= 100) { + /* since we use the iface_idx to select a table number, the range is limited from + * 0 to 99. Note that the providers are required to provide increasing numbers, + * so this means we bail out after the first 100 devices. */ + _LOGD("config device %s: skip because number of supported interfaces reached", hwaddr); + return FALSE; + } + _LOGD("config device %s: configuring \"%s\" (%s)...", hwaddr, nm_device_get_iface(device) ?: "/unknown/", From a9e6aa663e1cf9d155ddfa49a75ac8ecd1982a17 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Thu, 5 Aug 2021 07:44:05 -0400 Subject: [PATCH 14/17] aliyun: reuse ipv4 gateway address returned by metadata server The default ipv4 gateway address of the VPC in Aliyun cloud is not the first IP address in the CIDR subnet block, we should instead use the ipv4 gateway address retrieved from the metadata server in `_nmc_mangle_connection()`. https://bugzilla.redhat.com/show_bug.cgi?id=1823315 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/958 Signed-off-by: Wen Liang (cherry picked from commit 778e1f8493babb8e015e5334806de7099c66bafb) (cherry picked from commit 59633dbe117f97f5b453df6a4b1e5a918d2160e8) --- clients/cloud-setup/main.c | 12 +++++++----- clients/cloud-setup/nmcs-provider.h | 2 ++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index 9fa4cd3c84..b2b2eabe76 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -347,11 +347,13 @@ _nmc_mangle_connection(NMDevice * device, if (entry) g_ptr_array_add(addrs_new, entry); } - - gateway = nm_utils_ip4_address_clear_host_address(config_data->cidr_addr, - config_data->cidr_prefix); - ((guint8 *) &gateway)[3] += 1; - + if (config_data->has_gateway && config_data->gateway) { + gateway = config_data->gateway; + } else { + gateway = nm_utils_ip4_address_clear_host_address(config_data->cidr_addr, + config_data->cidr_prefix); + ((guint8 *) &gateway)[3] += 1; + } rt_metric = 10; rt_table = 30400 + config_data->iface_idx; diff --git a/clients/cloud-setup/nmcs-provider.h b/clients/cloud-setup/nmcs-provider.h index 261f589f2f..3663205964 100644 --- a/clients/cloud-setup/nmcs-provider.h +++ b/clients/cloud-setup/nmcs-provider.h @@ -23,9 +23,11 @@ typedef struct { gssize iface_idx; in_addr_t cidr_addr; + in_addr_t gateway; guint8 cidr_prefix; bool has_ipv4s : 1; bool has_cidr : 1; + bool has_gateway : 1; NMIPRoute **iproutes_arr; gsize iproutes_len; From b33eac5281935a6a07aa66938a9130cfd0b84180 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 6 Sep 2021 10:35:36 +0200 Subject: [PATCH 15/17] cloud-setup: cleanup configuring addresses/routes/rules in _nmc_mangle_connection() (cherry picked from commit 0978be5e43f142ec5c6062dcfe1c2f4aa834464b) (cherry picked from commit ce24b4bca5d3fbad65d4065325cfa80ac05fbfdb) --- clients/cloud-setup/main.c | 52 +++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index b2b2eabe76..e7979ed564 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -277,10 +277,6 @@ _nmc_mangle_connection(NMDevice * device, NMConnection * remote_connection; NMSettingIPConfig * remote_s_ip = NULL; gsize i; - in_addr_t gateway; - gint64 rt_metric; - guint32 rt_table; - NMIPRoute * route_entry; gboolean addrs_changed = FALSE; gboolean rules_changed = FALSE; gboolean routes_changed = FALSE; @@ -337,47 +333,45 @@ _nmc_mangle_connection(NMDevice * device, * We don't need to configure policy routing in this case. */ NM_SET_OUT(out_skipped_single_addr, TRUE); } else if (config_data->has_ipv4s && config_data->has_cidr) { - for (i = 0; i < config_data->ipv4s_len; i++) { - NMIPAddress *entry; + NMIPAddress * addr_entry; + NMIPRoute * route_entry; + NMIPRoutingRule *rule_entry; + in_addr_t gateway; + char sbuf[NM_UTILS_INET_ADDRSTRLEN]; - entry = nm_ip_address_new_binary(AF_INET, - &config_data->ipv4s_arr[i], - config_data->cidr_prefix, - NULL); - if (entry) - g_ptr_array_add(addrs_new, entry); + for (i = 0; i < config_data->ipv4s_len; i++) { + addr_entry = nm_ip_address_new_binary(AF_INET, + &config_data->ipv4s_arr[i], + config_data->cidr_prefix, + NULL); + nm_assert(addr_entry); + g_ptr_array_add(addrs_new, addr_entry); } + if (config_data->has_gateway && config_data->gateway) { gateway = config_data->gateway; } else { gateway = nm_utils_ip4_address_clear_host_address(config_data->cidr_addr, config_data->cidr_prefix); - ((guint8 *) &gateway)[3] += 1; + if (config_data->cidr_prefix < 32) + ((guint8 *) &gateway)[3] += 1; } - rt_metric = 10; - rt_table = 30400 + config_data->iface_idx; - route_entry = - nm_ip_route_new_binary(AF_INET, &nm_ip_addr_zero, 0, &gateway, rt_metric, NULL); + route_entry = nm_ip_route_new_binary(AF_INET, &nm_ip_addr_zero, 0, &gateway, 10, NULL); nm_ip_route_set_attribute(route_entry, NM_IP_ROUTE_ATTRIBUTE_TABLE, - g_variant_new_uint32(rt_table)); + g_variant_new_uint32(30400 + config_data->iface_idx)); g_ptr_array_add(routes_new, route_entry); for (i = 0; i < config_data->ipv4s_len; i++) { - NMIPRoutingRule *entry; - char sbuf[NM_UTILS_INET_ADDRSTRLEN]; - - entry = nm_ip_routing_rule_new(AF_INET); - nm_ip_routing_rule_set_priority(entry, rt_table); - nm_ip_routing_rule_set_from(entry, + rule_entry = nm_ip_routing_rule_new(AF_INET); + nm_ip_routing_rule_set_priority(rule_entry, 30400 + config_data->iface_idx); + nm_ip_routing_rule_set_from(rule_entry, _nm_utils_inet4_ntop(config_data->ipv4s_arr[i], sbuf), 32); - nm_ip_routing_rule_set_table(entry, rt_table); - - nm_assert(nm_ip_routing_rule_validate(entry, NULL)); - - g_ptr_array_add(rules_new, entry); + nm_ip_routing_rule_set_table(rule_entry, 30400 + config_data->iface_idx); + nm_assert(nm_ip_routing_rule_validate(rule_entry, NULL)); + g_ptr_array_add(rules_new, rule_entry); } } From 6fef8c7235235011d83681e89e8fa02dc89ffb2c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 Sep 2021 10:31:55 +0200 Subject: [PATCH 16/17] cloud-setup: use suppress_prefixlength rule to honor non-default-routes in the main table Background ========== Imagine you run a container on your machine. Then the routing table might look like: default via 10.0.10.1 dev eth0 proto dhcp metric 100 10.0.10.0/28 dev eth0 proto kernel scope link src 10.0.10.5 metric 100 [...] 10.42.0.0/24 via 10.42.0.0 dev flannel.1 onlink 10.42.1.2 dev cali02ad7e68ce1 scope link 10.42.1.3 dev cali8fcecf5aaff scope link 10.42.2.0/24 via 10.42.2.0 dev flannel.1 onlink 10.42.3.0/24 via 10.42.3.0 dev flannel.1 onlink That is, there are another interfaces with subnets and specific routes. If nm-cloud-setup now configures rules: 0: from all lookup local 30400: from 10.0.10.5 lookup 30400 32766: from all lookup main 32767: from all lookup default and default via 10.0.10.1 dev eth0 table 30400 proto static metric 10 10.0.10.1 dev eth0 table 30400 proto static scope link metric 10 then these other subnets will also be reached via the default route. This container example is just one case where this is a problem. In general, if you have specific routes on another interface, then the default route in the 30400+ table will interfere badly. The idea of nm-cloud-setup is to automatically configure the network for secondary IP addresses. When the user has special requirements, then they should disable nm-cloud-setup and configure whatever they want. But the container use case is popular and important. It is not something where the user actively configures the network. This case needs to work better, out of the box. In general, nm-cloud-setup should work better with the existing network configuration. Change ====== Add new routing tables 30200+ with the individual subnets of the interface: 10.0.10.0/24 dev eth0 table 30200 proto static metric 10 [...] default via 10.0.10.1 dev eth0 table 30400 proto static metric 10 10.0.10.1 dev eth0 table 30400 proto static scope link metric 10 Also add more important routing rules with priority 30200+, which select these tables based on the source address: 30200: from 10.0.10.5 lookup 30200 These will do source based routing for the subnets on these interfaces. Then, add a rule with priority 30350 30350: lookup main suppress_prefixlength 0 which processes the routes from the main table, but ignores the default routes. 30350 was chosen, because it's in between the rules 30200+ and 30400+, leaving a range for the user to configure their own rules. Then, as before, the rules 30400+ again look at the corresponding 30400+ table, to find a default route. Finally, process the main table again, this time honoring the default route. That is for packets that have a different source address. This change means that the source based routing is used for the subnets that are configured on the interface and for the default route. Whereas, if there are any more specific routes in the main table, they will be preferred over the default route. Apparently Amazon Linux solves this differently, by not configuring a routing table for addresses on interface "eth0". That might be an alternative, but it's not clear to me what is special about eth0 to warrant this treatment. It also would imply that we somehow recognize this primary interface. In practise that would be doable by selecting the interface with "iface_idx" zero. Instead choose this approach. This is remotely similar to what WireGuard does for configuring the default route ([1]), however WireGuard uses fwmark to match the packets instead of the source address. [1] https://www.wireguard.com/netns/#improved-rule-based-routing (cherry picked from commit fe80b2d1ecd94639573a944633a5d960db316f60) (cherry picked from commit 58e58361bd1666e5af822a4bc970bebb8a44bd6e) --- clients/cloud-setup/main.c | 36 ++++++++++++++++++++++++++++++++++++ man/nm-cloud-setup.xml | 30 ++++++++++++++++++------------ 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/clients/cloud-setup/main.c b/clients/cloud-setup/main.c index e7979ed564..fd5d2b613f 100644 --- a/clients/cloud-setup/main.c +++ b/clients/cloud-setup/main.c @@ -4,6 +4,8 @@ #include "nm-libnm-aux/nm-libnm-aux.h" +#include + #include "nm-cloud-setup-utils.h" #include "nmcs-provider-ec2.h" #include "nmcs-provider-gcp.h" @@ -333,6 +335,8 @@ _nmc_mangle_connection(NMDevice * device, * We don't need to configure policy routing in this case. */ NM_SET_OUT(out_skipped_single_addr, TRUE); } else if (config_data->has_ipv4s && config_data->has_cidr) { + gs_unref_hashtable GHashTable *unique_subnets = + g_hash_table_new(nm_direct_hash, g_direct_equal); NMIPAddress * addr_entry; NMIPRoute * route_entry; NMIPRoutingRule *rule_entry; @@ -357,6 +361,38 @@ _nmc_mangle_connection(NMDevice * device, ((guint8 *) &gateway)[3] += 1; } + for (i = 0; i < config_data->ipv4s_len; i++) { + in_addr_t a = config_data->ipv4s_arr[i]; + + a = nm_utils_ip4_address_clear_host_address(a, config_data->cidr_prefix); + + G_STATIC_ASSERT_EXPR(sizeof(gsize) >= sizeof(in_addr_t)); + if (g_hash_table_add(unique_subnets, GSIZE_TO_POINTER(a))) { + route_entry = + nm_ip_route_new_binary(AF_INET, &a, config_data->cidr_prefix, NULL, 10, NULL); + nm_ip_route_set_attribute(route_entry, + NM_IP_ROUTE_ATTRIBUTE_TABLE, + g_variant_new_uint32(30200 + config_data->iface_idx)); + g_ptr_array_add(routes_new, route_entry); + } + + rule_entry = nm_ip_routing_rule_new(AF_INET); + nm_ip_routing_rule_set_priority(rule_entry, 30200 + config_data->iface_idx); + nm_ip_routing_rule_set_from(rule_entry, + _nm_utils_inet4_ntop(config_data->ipv4s_arr[i], sbuf), + 32); + nm_ip_routing_rule_set_table(rule_entry, 30200 + config_data->iface_idx); + nm_assert(nm_ip_routing_rule_validate(rule_entry, NULL)); + g_ptr_array_add(rules_new, rule_entry); + } + + rule_entry = nm_ip_routing_rule_new(AF_INET); + nm_ip_routing_rule_set_priority(rule_entry, 30350); + nm_ip_routing_rule_set_table(rule_entry, RT_TABLE_MAIN); + nm_ip_routing_rule_set_suppress_prefixlength(rule_entry, 0); + nm_assert(nm_ip_routing_rule_validate(rule_entry, NULL)); + g_ptr_array_add(rules_new, rule_entry); + route_entry = nm_ip_route_new_binary(AF_INET, &nm_ip_addr_zero, 0, &gateway, 10, NULL); nm_ip_route_set_attribute(route_entry, NM_IP_ROUTE_ATTRIBUTE_TABLE, diff --git a/man/nm-cloud-setup.xml b/man/nm-cloud-setup.xml index 388ef3ba91..4ae4042f84 100644 --- a/man/nm-cloud-setup.xml +++ b/man/nm-cloud-setup.xml @@ -221,7 +221,9 @@ Also, if the device is currently not activated in NetworkManager or if the currently activated profile has a user-data org.freedesktop.nm-cloud-setup.skip=yes, it is skipped. - Then, the tool will change the runtime configuration of the device. + If only one interface and one address is configured, then the tool does nothing + and leaves the automatic configuration that was obtained via DHCP. + Otherwise, the tool will change the runtime configuration of the device. Add static IPv4 addresses for all the configured addresses from local-ipv4s with @@ -232,15 +234,25 @@ Choose a route table 30400 + the index of the interface and add a default route 0.0.0.0/0. The gateway is the first IP address in the CIDR subnet block. For - example, we might get a route "0.0.0.0/0 172.16.5.1 10 table=30401". + example, we might get a route "0.0.0.0/0 172.16.5.1 10 table=30400". + Also choose a route table 30200 + the interface index. This + contains a direct routes to the subnets of this interface. Finally, add a policy routing rule for each address. For example - "priority 30401 from 172.16.5.3/32 table 30401, priority 30401 from 172.16.5.4/32 table 30401". + "priority 30200 from 172.16.5.3/32 table 30200, priority 30200 from 172.16.5.4/32 table 30200". + and + "priority 30400 from 172.16.5.3/32 table 30400, priority 30400 from 172.16.5.4/32 table 30400" + The 30200+ rules select the table to reach the subnet directly, while the 30400+ rules use the + default route. Also add a rule + "priority 30350 table main suppress_prefixlength 0". This has a priority between + the two previous rules and causes a lookup of routes in the main table while ignoring the default + route. The purpose of this is so that other specific routes in the main table are honored over + the default route in table 30400+. With above example, this roughly corresponds for interface eth0 to - nmcli device modify "eth0" ipv4.addresses "172.16.5.3/24,172.16.5.4/24" ipv4.routes "0.0.0.0/0 172.16.5.1 10 table=30401" ipv4.routing-rules "priority 30401 from 172.16.5.3/32 table 30401, priority 30401 from 172.16.5.4/32 table 30401". + nmcli device modify "eth0" ipv4.addresses "172.16.5.3/24,172.16.5.4/24" ipv4.routes "172.16.5.0/24 0.0.0.0 10 table=30200, 0.0.0.0/0 172.16.5.1 10 table=30400" ipv4.routing-rules "priority 30200 from 172.16.5.3/32 table 30200, priority 30200 from 172.16.5.4/32 table 30200, priority 20350 table main suppress_prefixlength 0, priority 30400 from 172.16.5.3/32 table 30400, priority 30400 from 172.16.5.4/32 table 30400". Note that this replaces the previous addresses, routes and rules with the new information. But also note that this only changes the run time configuration of the device. The connection profile on disk is not affected. @@ -323,14 +335,8 @@ At this point, we have a list of all interfaces (by MAC address) and their configured IPv4 addresses. - For each device, we lookup the currently applied connection in NetworkManager. That implies, that the device is currently activated - in NetworkManager. If no such device was in NetworkManager, or if the profile has user-data org.freedesktop.nm-cloud-setup.skip=yes, - we skip the device. Now for each found IP address we add a static address "$ADDR/$SUBNET_PREFIX". Also we configure policy routing - by adding a static route "$ADDR/$SUBNET_PREFIX $GATEWAY 10, table=$TABLE" where $GATEWAY is the first IP address in the subnet and table - is 30400 plus the interface index. Also we add a policy routing rule "priority $TABLE from $ADDR/32 table $TABLE". - The effect is not unlike calling - nmcli device modify "$DEVICE" ipv4.addresses "$ADDR/$SUBNET [,...]" ipv4.routes "$ADDR/32 $GATEWAY 10 table=$TABLE" ipv4.routing-rules "priority $TABLE from $ADDR/32 table $TABLE" - for all relevant devices and all found addresses. + Then the tool configures the system like doing for AWS environment. That is, using source based policy routing + with the tables/rules 30200/30400. From 9cb01cdefd43c954653202eada0b094e61c86015 Mon Sep 17 00:00:00 2001 From: Fernando Fernandez Mancera Date: Mon, 4 Oct 2021 18:26:11 +0200 Subject: [PATCH 17/17] cloud-setup: follow the clang-format Signed-off-by: Fernando Fernandez Mancera --- clients/cloud-setup/nmcs-provider-azure.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/cloud-setup/nmcs-provider-azure.c b/clients/cloud-setup/nmcs-provider-azure.c index 3b923e4ce5..a0e6076fd3 100644 --- a/clients/cloud-setup/nmcs-provider-azure.c +++ b/clients/cloud-setup/nmcs-provider-azure.c @@ -145,7 +145,7 @@ _get_config_fetch_done_cb(NMHttpClient * http_client, } iface_data->iface_get_config = g_hash_table_lookup(get_config_data->result_dict, v_hwaddr); - iface_get_config = iface_data->iface_get_config; + iface_get_config = iface_data->iface_get_config; if (is_ipv4) { char tmp_addr_str[NM_UTILS_INET_ADDRSTRLEN];