Ifcfg config file support in NetworkManger is deprecated. This patch
provides support for the new keyfile config format for connection
profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
to generate the new network configuration in keyfile
format(.ini-style format) along with a ifcfg format configuration.
The ifcfg format configuration is also retained to support easy
backward compatibility for distro vendors. These configurations are
stored in temp files which are further translated using the
hv_set_ifconfig.sh script. This script is implemented by individual
distros based on the network management commands supported.
For example, RHEL's implementation could be found here:
https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
Debian's implementation could be found here:
https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
The next part of this support is to let the Distro vendors consume
these modified implementations to the new configuration format.
Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
Signed-off-by: Shradha Gupta <[email protected]>
Reviewed-by: Saurabh Sengar <[email protected]>
---
Changes v7->v8
* Fix some filename variable names to avoid confusion
* Add initialization of is_ipv6 variable
* Add a few comments
---
tools/hv/hv_kvp_daemon.c | 233 +++++++++++++++++++++++++++++++-----
tools/hv/hv_set_ifconfig.sh | 39 +++++-
2 files changed, 235 insertions(+), 37 deletions(-)
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 27f5e7dfc2f7..264eeb9c46a9 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -1171,12 +1171,79 @@ static int process_ip_string(FILE *f, char *ip_string, int type)
return 0;
}
+/*
+ * Only IPv4 subnet strings needs to be converted to plen
+ * For IPv6 the subnet is already privided in plen format
+ */
+static int kvp_subnet_to_plen(char *subnet_addr_str)
+{
+ int plen = 0;
+ struct in_addr subnet_addr4;
+
+ /*
+ * Convert subnet address to binary representation
+ */
+ if (inet_pton(AF_INET, subnet_addr_str, &subnet_addr4) == 1) {
+ uint32_t subnet_mask = ntohl(subnet_addr4.s_addr);
+
+ while (subnet_mask & 0x80000000) {
+ plen++;
+ subnet_mask <<= 1;
+ }
+ } else {
+ return -1;
+ }
+
+ return plen;
+}
+
+static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
+ int is_ipv6)
+{
+ char addr[INET6_ADDRSTRLEN];
+ char subnet_addr[INET6_ADDRSTRLEN];
+ int error, i = 0;
+ int ip_offset = 0, subnet_offset = 0;
+ int plen;
+
+ memset(addr, 0, sizeof(addr));
+ memset(subnet_addr, 0, sizeof(subnet_addr));
+
+ while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
+ (MAX_IP_ADDR_SIZE * 2)) &&
+ parse_ip_val_buffer(subnet,
+ &subnet_offset,
+ subnet_addr,
+ (MAX_IP_ADDR_SIZE *
+ 2))) {
+ if (!is_ipv6)
+ plen = kvp_subnet_to_plen((char *)subnet_addr);
+ else
+ plen = atoi(subnet_addr);
+
+ if (plen < 0)
+ return plen;
+
+ error = fprintf(f, "address%d=%s/%d\n", ++i, (char *)addr,
+ plen);
+ if (error < 0)
+ return error;
+
+ memset(addr, 0, sizeof(addr));
+ memset(subnet_addr, 0, sizeof(subnet_addr));
+ }
+
+ return 0;
+}
+
static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
{
int error = 0;
- char if_file[PATH_MAX];
- FILE *file;
+ char if_filename[PATH_MAX];
+ char nm_filename[PATH_MAX];
+ FILE *ifcfg_file, *nmfile;
char cmd[PATH_MAX];
+ int is_ipv6 = 0;
char *mac_addr;
int str_len;
@@ -1197,7 +1264,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
* in a given distro to configure the interface and so are free
* ignore information that may not be relevant.
*
- * Here is the format of the ip configuration file:
+ * Here is the ifcfg format of the ip configuration file:
*
* HWADDR=macaddr
* DEVICE=interface name
@@ -1220,6 +1287,32 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
* tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
* IPV6NETMASK.
*
+ * Here is the keyfile format of the ip configuration file:
+ *
+ * [ethernet]
+ * mac-address=macaddr
+ * [connection]
+ * interface-name=interface name
+ *
+ * [ipv4]
+ * method=<protocol> (where <protocol> is "auto" if DHCP is configured
+ * or "manual" if no boot-time protocol should be used)
+ *
+ * address1=ipaddr1/plen
+ * address2=ipaddr2/plen
+ *
+ * gateway=gateway1;gateway2
+ *
+ * dns=dns1;dns2
+ *
+ * [ipv6]
+ * address1=ipaddr1/plen
+ * address2=ipaddr2/plen
+ *
+ * gateway=gateway1;gateway2
+ *
+ * dns=dns1;dns2
+ *
* The host can specify multiple ipv4 and ipv6 addresses to be
* configured for the interface. Furthermore, the configuration
* needs to be persistent. A subsequent GET call on the interface
@@ -1227,14 +1320,29 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
* call.
*/
- snprintf(if_file, sizeof(if_file), "%s%s%s", KVP_CONFIG_LOC,
- "/ifcfg-", if_name);
+ /*
+ * We are populating both ifcfg and nmconnection files
+ */
+ snprintf(if_filename, sizeof(if_filename), "%s%s%s", KVP_CONFIG_LOC,
+ "/ifcfg-", if_name);
- file = fopen(if_file, "w");
+ ifcfg_file = fopen(if_filename, "w");
- if (file == NULL) {
+ if (!ifcfg_file) {
syslog(LOG_ERR, "Failed to open config file; error: %d %s",
- errno, strerror(errno));
+ errno, strerror(errno));
+ return HV_E_FAIL;
+ }
+
+ snprintf(nm_filename, sizeof(nm_filename), "%s%s%s%s", KVP_CONFIG_LOC,
+ "/", if_name, ".nmconnection");
+
+ nmfile = fopen(nm_filename, "w");
+
+ if (!nmfile) {
+ syslog(LOG_ERR, "Failed to open config file; error: %d %s",
+ errno, strerror(errno));
+ fclose(ifcfg_file);
return HV_E_FAIL;
}
@@ -1248,14 +1356,31 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
goto setval_error;
}
- error = kvp_write_file(file, "HWADDR", "", mac_addr);
- free(mac_addr);
+ error = kvp_write_file(ifcfg_file, "HWADDR", "", mac_addr);
+ if (error < 0)
+ goto setmac_error;
+
+ error = kvp_write_file(ifcfg_file, "DEVICE", "", if_name);
+ if (error < 0)
+ goto setmac_error;
+
+ error = fprintf(nmfile, "\n[connection]\n");
+ if (error < 0)
+ goto setmac_error;
+
+ error = kvp_write_file(nmfile, "interface-name", "", if_name);
if (error)
- goto setval_error;
+ goto setmac_error;
- error = kvp_write_file(file, "DEVICE", "", if_name);
+ error = fprintf(nmfile, "\n[ethernet]\n");
+ if (error < 0)
+ goto setmac_error;
+
+ error = kvp_write_file(nmfile, "mac-address", "", mac_addr);
if (error)
- goto setval_error;
+ goto setmac_error;
+
+ free(mac_addr);
/*
* The dhcp_enabled flag is only for IPv4. In the case the host only
@@ -1263,47 +1388,91 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
* proceed to parse and pass the IPv6 information to the
* disto-specific script hv_set_ifconfig.
*/
+
+ /*
+ * First populate the ifcfg file format
+ */
if (new_val->dhcp_enabled) {
- error = kvp_write_file(file, "BOOTPROTO", "", "dhcp");
+ error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "dhcp");
if (error)
goto setval_error;
-
} else {
- error = kvp_write_file(file, "BOOTPROTO", "", "none");
+ error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "none");
if (error)
goto setval_error;
}
- /*
- * Write the configuration for ipaddress, netmask, gateway and
- * name servers.
- */
-
- error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
+ error = process_ip_string(ifcfg_file, (char *)new_val->ip_addr,
+ IPADDR);
if (error)
goto setval_error;
- error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
+ error = process_ip_string(ifcfg_file, (char *)new_val->sub_net,
+ NETMASK);
if (error)
goto setval_error;
- error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
+ error = process_ip_string(ifcfg_file, (char *)new_val->gate_way,
+ GATEWAY);
if (error)
goto setval_error;
- error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
+ error = process_ip_string(ifcfg_file, (char *)new_val->dns_addr, DNS);
if (error)
goto setval_error;
- fclose(file);
+ if (new_val->addr_family == ADDR_FAMILY_IPV6) {
+ error = fprintf(nmfile, "\n[ipv6]\n");
+ if (error < 0)
+ goto setval_error;
+ is_ipv6 = 1;
+ } else {
+ error = fprintf(nmfile, "\n[ipv4]\n");
+ if (error < 0)
+ goto setval_error;
+ }
+
+ /*
+ * Now we populate the keyfile format
+ */
+
+ if (new_val->dhcp_enabled) {
+ error = kvp_write_file(nmfile, "method", "", "auto");
+ if (error < 0)
+ goto setval_error;
+ } else {
+ error = kvp_write_file(nmfile, "method", "", "manual");
+ if (error < 0)
+ goto setval_error;
+ }
+
+ /*
+ * Write the configuration for ipaddress, netmask, gateway and
+ * name services
+ */
+ error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
+ (char *)new_val->sub_net, is_ipv6);
+ if (error < 0)
+ goto setval_error;
+
+ error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
+ if (error < 0)
+ goto setval_error;
+
+ error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
+ if (error < 0)
+ goto setval_error;
+
+ fclose(nmfile);
+ fclose(ifcfg_file);
/*
* Now that we have populated the configuration file,
* invoke the external script to do its magic.
*/
- str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s",
- "hv_set_ifconfig", if_file);
+ str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s %s",
+ "hv_set_ifconfig", if_filename, nm_filename);
/*
* This is a little overcautious, but it's necessary to suppress some
* false warnings from gcc 8.0.1.
@@ -1316,14 +1485,16 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
if (system(cmd)) {
syslog(LOG_ERR, "Failed to execute cmd '%s'; error: %d %s",
- cmd, errno, strerror(errno));
+ cmd, errno, strerror(errno));
return HV_E_FAIL;
}
return 0;
-
+setmac_error:
+ free(mac_addr);
setval_error:
syslog(LOG_ERR, "Failed to write config file");
- fclose(file);
+ fclose(ifcfg_file);
+ fclose(nmfile);
return error;
}
diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
index d10fe35b7f25..ae5a7a8249a2 100755
--- a/tools/hv/hv_set_ifconfig.sh
+++ b/tools/hv/hv_set_ifconfig.sh
@@ -18,12 +18,12 @@
#
# This example script is based on a RHEL environment.
#
-# Here is the format of the ip configuration file:
+# Here is the ifcfg format of the ip configuration file:
#
# HWADDR=macaddr
# DEVICE=interface name
# BOOTPROTO=<protocol> (where <protocol> is "dhcp" if DHCP is configured
-# or "none" if no boot-time protocol should be used)
+# or "none" if no boot-time protocol should be used)
#
# IPADDR0=ipaddr1
# IPADDR1=ipaddr2
@@ -41,6 +41,32 @@
# tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
# IPV6NETMASK.
#
+# Here is the keyfile format of the ip configuration file:
+#
+# [ethernet]
+# mac-address=macaddr
+# [connection]
+# interface-name=interface name
+#
+# [ipv4]
+# method=<protocol> (where <protocol> is "auto" if DHCP is configured
+# or "manual" if no boot-time protocol should be used)
+#
+# address1=ipaddr1/plen
+# address=ipaddr2/plen
+#
+# gateway=gateway1;gateway2
+#
+# dns=dns1;
+#
+# [ipv6]
+# address1=ipaddr1/plen
+# address2=ipaddr1/plen
+#
+# gateway=gateway1;gateway2
+#
+# dns=dns1;dns2
+#
# The host can specify multiple ipv4 and ipv6 addresses to be
# configured for the interface. Furthermore, the configuration
# needs to be persistent. A subsequent GET call on the interface
@@ -48,18 +74,19 @@
# call.
#
-
-
echo "IPV6INIT=yes" >> $1
echo "NM_CONTROLLED=no" >> $1
echo "PEERDNS=yes" >> $1
echo "ONBOOT=yes" >> $1
-
cp $1 /etc/sysconfig/network-scripts/
+chmod 600 $2
+interface=$(echo $2 | awk -F - '{ print $2 }')
+filename="${2##*/}"
+
+sed '/\[connection\]/a autoconnect=true' $2 > /etc/NetworkManager/system-connections/${filename}
-interface=$(echo $1 | awk -F - '{ print $2 }')
/sbin/ifdown $interface 2>/dev/null
/sbin/ifup $interface 2>/dev/null
--
2.34.1
> On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
>
> Ifcfg config file support in NetworkManger is deprecated. This patch
> provides support for the new keyfile config format for connection
> profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> to generate the new network configuration in keyfile
> format(.ini-style format) along with a ifcfg format configuration.
> The ifcfg format configuration is also retained to support easy
> backward compatibility for distro vendors. These configurations are
> stored in temp files which are further translated using the
> hv_set_ifconfig.sh script. This script is implemented by individual
> distros based on the network management commands supported.
> For example, RHEL's implementation could be found here:
> https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> Debian's implementation could be found here:
> https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
>
> The next part of this support is to let the Distro vendors consume
> these modified implementations to the new configuration format.
>
> Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
> Signed-off-by: Shradha Gupta <[email protected]>
> Reviewed-by: Saurabh Sengar <[email protected]>
Reviewed-by: Ani Sinha <[email protected]>
> ---
> Changes v7->v8
> * Fix some filename variable names to avoid confusion
> * Add initialization of is_ipv6 variable
> * Add a few comments
> ---
> tools/hv/hv_kvp_daemon.c | 233 +++++++++++++++++++++++++++++++-----
> tools/hv/hv_set_ifconfig.sh | 39 +++++-
> 2 files changed, 235 insertions(+), 37 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 27f5e7dfc2f7..264eeb9c46a9 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -1171,12 +1171,79 @@ static int process_ip_string(FILE *f, char *ip_string, int type)
> return 0;
> }
>
> +/*
> + * Only IPv4 subnet strings needs to be converted to plen
> + * For IPv6 the subnet is already privided in plen format
> + */
> +static int kvp_subnet_to_plen(char *subnet_addr_str)
> +{
> + int plen = 0;
> + struct in_addr subnet_addr4;
> +
> + /*
> + * Convert subnet address to binary representation
> + */
> + if (inet_pton(AF_INET, subnet_addr_str, &subnet_addr4) == 1) {
> + uint32_t subnet_mask = ntohl(subnet_addr4.s_addr);
> +
> + while (subnet_mask & 0x80000000) {
> + plen++;
> + subnet_mask <<= 1;
> + }
> + } else {
> + return -1;
> + }
> +
> + return plen;
> +}
> +
> +static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> + int is_ipv6)
> +{
> + char addr[INET6_ADDRSTRLEN];
> + char subnet_addr[INET6_ADDRSTRLEN];
> + int error, i = 0;
> + int ip_offset = 0, subnet_offset = 0;
> + int plen;
> +
> + memset(addr, 0, sizeof(addr));
> + memset(subnet_addr, 0, sizeof(subnet_addr));
> +
> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> + (MAX_IP_ADDR_SIZE * 2)) &&
> + parse_ip_val_buffer(subnet,
> + &subnet_offset,
> + subnet_addr,
> + (MAX_IP_ADDR_SIZE *
> + 2))) {
> + if (!is_ipv6)
> + plen = kvp_subnet_to_plen((char *)subnet_addr);
> + else
> + plen = atoi(subnet_addr);
> +
> + if (plen < 0)
> + return plen;
> +
> + error = fprintf(f, "address%d=%s/%d\n", ++i, (char *)addr,
> + plen);
> + if (error < 0)
> + return error;
> +
> + memset(addr, 0, sizeof(addr));
> + memset(subnet_addr, 0, sizeof(subnet_addr));
> + }
> +
> + return 0;
> +}
> +
> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> {
> int error = 0;
> - char if_file[PATH_MAX];
> - FILE *file;
> + char if_filename[PATH_MAX];
> + char nm_filename[PATH_MAX];
> + FILE *ifcfg_file, *nmfile;
> char cmd[PATH_MAX];
> + int is_ipv6 = 0;
> char *mac_addr;
> int str_len;
>
> @@ -1197,7 +1264,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * in a given distro to configure the interface and so are free
> * ignore information that may not be relevant.
> *
> - * Here is the format of the ip configuration file:
> + * Here is the ifcfg format of the ip configuration file:
> *
> * HWADDR=macaddr
> * DEVICE=interface name
> @@ -1220,6 +1287,32 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> * IPV6NETMASK.
> *
> + * Here is the keyfile format of the ip configuration file:
> + *
> + * [ethernet]
> + * mac-address=macaddr
> + * [connection]
> + * interface-name=interface name
> + *
> + * [ipv4]
> + * method=<protocol> (where <protocol> is "auto" if DHCP is configured
> + * or "manual" if no boot-time protocol should be used)
> + *
> + * address1=ipaddr1/plen
> + * address2=ipaddr2/plen
> + *
> + * gateway=gateway1;gateway2
> + *
> + * dns=dns1;dns2
> + *
> + * [ipv6]
> + * address1=ipaddr1/plen
> + * address2=ipaddr2/plen
> + *
> + * gateway=gateway1;gateway2
> + *
> + * dns=dns1;dns2
> + *
> * The host can specify multiple ipv4 and ipv6 addresses to be
> * configured for the interface. Furthermore, the configuration
> * needs to be persistent. A subsequent GET call on the interface
> @@ -1227,14 +1320,29 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * call.
> */
>
> - snprintf(if_file, sizeof(if_file), "%s%s%s", KVP_CONFIG_LOC,
> - "/ifcfg-", if_name);
> + /*
> + * We are populating both ifcfg and nmconnection files
> + */
> + snprintf(if_filename, sizeof(if_filename), "%s%s%s", KVP_CONFIG_LOC,
> + "/ifcfg-", if_name);
>
> - file = fopen(if_file, "w");
> + ifcfg_file = fopen(if_filename, "w");
>
> - if (file == NULL) {
> + if (!ifcfg_file) {
> syslog(LOG_ERR, "Failed to open config file; error: %d %s",
> - errno, strerror(errno));
> + errno, strerror(errno));
> + return HV_E_FAIL;
> + }
> +
> + snprintf(nm_filename, sizeof(nm_filename), "%s%s%s%s", KVP_CONFIG_LOC,
> + "/", if_name, ".nmconnection");
> +
> + nmfile = fopen(nm_filename, "w");
> +
> + if (!nmfile) {
> + syslog(LOG_ERR, "Failed to open config file; error: %d %s",
> + errno, strerror(errno));
> + fclose(ifcfg_file);
> return HV_E_FAIL;
> }
>
> @@ -1248,14 +1356,31 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> goto setval_error;
> }
>
> - error = kvp_write_file(file, "HWADDR", "", mac_addr);
> - free(mac_addr);
> + error = kvp_write_file(ifcfg_file, "HWADDR", "", mac_addr);
> + if (error < 0)
> + goto setmac_error;
> +
> + error = kvp_write_file(ifcfg_file, "DEVICE", "", if_name);
> + if (error < 0)
> + goto setmac_error;
> +
> + error = fprintf(nmfile, "\n[connection]\n");
> + if (error < 0)
> + goto setmac_error;
> +
> + error = kvp_write_file(nmfile, "interface-name", "", if_name);
> if (error)
> - goto setval_error;
> + goto setmac_error;
>
> - error = kvp_write_file(file, "DEVICE", "", if_name);
> + error = fprintf(nmfile, "\n[ethernet]\n");
> + if (error < 0)
> + goto setmac_error;
> +
> + error = kvp_write_file(nmfile, "mac-address", "", mac_addr);
> if (error)
> - goto setval_error;
> + goto setmac_error;
> +
> + free(mac_addr);
>
> /*
> * The dhcp_enabled flag is only for IPv4. In the case the host only
> @@ -1263,47 +1388,91 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * proceed to parse and pass the IPv6 information to the
> * disto-specific script hv_set_ifconfig.
> */
> +
> + /*
> + * First populate the ifcfg file format
> + */
> if (new_val->dhcp_enabled) {
> - error = kvp_write_file(file, "BOOTPROTO", "", "dhcp");
> + error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "dhcp");
> if (error)
> goto setval_error;
> -
> } else {
> - error = kvp_write_file(file, "BOOTPROTO", "", "none");
> + error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "none");
> if (error)
> goto setval_error;
> }
>
> - /*
> - * Write the configuration for ipaddress, netmask, gateway and
> - * name servers.
> - */
> -
> - error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> + error = process_ip_string(ifcfg_file, (char *)new_val->ip_addr,
> + IPADDR);
> if (error)
> goto setval_error;
>
> - error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> + error = process_ip_string(ifcfg_file, (char *)new_val->sub_net,
> + NETMASK);
> if (error)
> goto setval_error;
>
> - error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> + error = process_ip_string(ifcfg_file, (char *)new_val->gate_way,
> + GATEWAY);
> if (error)
> goto setval_error;
>
> - error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> + error = process_ip_string(ifcfg_file, (char *)new_val->dns_addr, DNS);
> if (error)
> goto setval_error;
>
> - fclose(file);
> + if (new_val->addr_family == ADDR_FAMILY_IPV6) {
> + error = fprintf(nmfile, "\n[ipv6]\n");
> + if (error < 0)
> + goto setval_error;
> + is_ipv6 = 1;
> + } else {
> + error = fprintf(nmfile, "\n[ipv4]\n");
> + if (error < 0)
> + goto setval_error;
> + }
> +
> + /*
> + * Now we populate the keyfile format
> + */
> +
> + if (new_val->dhcp_enabled) {
> + error = kvp_write_file(nmfile, "method", "", "auto");
> + if (error < 0)
> + goto setval_error;
> + } else {
> + error = kvp_write_file(nmfile, "method", "", "manual");
> + if (error < 0)
> + goto setval_error;
> + }
> +
> + /*
> + * Write the configuration for ipaddress, netmask, gateway and
> + * name services
> + */
> + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> + (char *)new_val->sub_net, is_ipv6);
> + if (error < 0)
> + goto setval_error;
> +
> + error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
> + if (error < 0)
> + goto setval_error;
> +
> + error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
> + if (error < 0)
> + goto setval_error;
> +
> + fclose(nmfile);
> + fclose(ifcfg_file);
>
> /*
> * Now that we have populated the configuration file,
> * invoke the external script to do its magic.
> */
>
> - str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s",
> - "hv_set_ifconfig", if_file);
> + str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s %s",
> + "hv_set_ifconfig", if_filename, nm_filename);
> /*
> * This is a little overcautious, but it's necessary to suppress some
> * false warnings from gcc 8.0.1.
> @@ -1316,14 +1485,16 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>
> if (system(cmd)) {
> syslog(LOG_ERR, "Failed to execute cmd '%s'; error: %d %s",
> - cmd, errno, strerror(errno));
> + cmd, errno, strerror(errno));
> return HV_E_FAIL;
> }
> return 0;
> -
> +setmac_error:
> + free(mac_addr);
> setval_error:
> syslog(LOG_ERR, "Failed to write config file");
> - fclose(file);
> + fclose(ifcfg_file);
> + fclose(nmfile);
> return error;
> }
>
> diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
> index d10fe35b7f25..ae5a7a8249a2 100755
> --- a/tools/hv/hv_set_ifconfig.sh
> +++ b/tools/hv/hv_set_ifconfig.sh
> @@ -18,12 +18,12 @@
> #
> # This example script is based on a RHEL environment.
> #
> -# Here is the format of the ip configuration file:
> +# Here is the ifcfg format of the ip configuration file:
> #
> # HWADDR=macaddr
> # DEVICE=interface name
> # BOOTPROTO=<protocol> (where <protocol> is "dhcp" if DHCP is configured
> -# or "none" if no boot-time protocol should be used)
> +# or "none" if no boot-time protocol should be used)
> #
> # IPADDR0=ipaddr1
> # IPADDR1=ipaddr2
> @@ -41,6 +41,32 @@
> # tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> # IPV6NETMASK.
> #
> +# Here is the keyfile format of the ip configuration file:
> +#
> +# [ethernet]
> +# mac-address=macaddr
> +# [connection]
> +# interface-name=interface name
> +#
> +# [ipv4]
> +# method=<protocol> (where <protocol> is "auto" if DHCP is configured
> +# or "manual" if no boot-time protocol should be used)
> +#
> +# address1=ipaddr1/plen
> +# address=ipaddr2/plen
> +#
> +# gateway=gateway1;gateway2
> +#
> +# dns=dns1;
> +#
> +# [ipv6]
> +# address1=ipaddr1/plen
> +# address2=ipaddr1/plen
> +#
> +# gateway=gateway1;gateway2
> +#
> +# dns=dns1;dns2
> +#
> # The host can specify multiple ipv4 and ipv6 addresses to be
> # configured for the interface. Furthermore, the configuration
> # needs to be persistent. A subsequent GET call on the interface
> @@ -48,18 +74,19 @@
> # call.
> #
>
> -
> -
> echo "IPV6INIT=yes" >> $1
> echo "NM_CONTROLLED=no" >> $1
> echo "PEERDNS=yes" >> $1
> echo "ONBOOT=yes" >> $1
>
> -
> cp $1 /etc/sysconfig/network-scripts/
>
> +chmod 600 $2
> +interface=$(echo $2 | awk -F - '{ print $2 }')
> +filename="${2##*/}"
> +
> +sed '/\[connection\]/a autoconnect=true' $2 > /etc/NetworkManager/system-connections/${filename}
>
> -interface=$(echo $1 | awk -F - '{ print $2 }')
>
> /sbin/ifdown $interface 2>/dev/null
> /sbin/ifup $interface 2>/dev/null
> --
> 2.34.1
>
On Mon, Oct 09, 2023 at 05:32:35PM +0530, Ani Sinha wrote:
>
>
> > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
> >
> > Ifcfg config file support in NetworkManger is deprecated. This patch
> > provides support for the new keyfile config format for connection
> > profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> > to generate the new network configuration in keyfile
> > format(.ini-style format) along with a ifcfg format configuration.
> > The ifcfg format configuration is also retained to support easy
> > backward compatibility for distro vendors. These configurations are
> > stored in temp files which are further translated using the
> > hv_set_ifconfig.sh script. This script is implemented by individual
> > distros based on the network management commands supported.
> > For example, RHEL's implementation could be found here:
> > https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> > Debian's implementation could be found here:
> > https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
> >
> > The next part of this support is to let the Distro vendors consume
> > these modified implementations to the new configuration format.
> >
> > Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
> > Signed-off-by: Shradha Gupta <[email protected]>
> > Reviewed-by: Saurabh Sengar <[email protected]>
>
> Reviewed-by: Ani Sinha <[email protected]>
Applied to hyperv-fixes. Thanks.
> On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
>
> Ifcfg config file support in NetworkManger is deprecated. This patch
> provides support for the new keyfile config format for connection
> profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> to generate the new network configuration in keyfile
> format(.ini-style format) along with a ifcfg format configuration.
> The ifcfg format configuration is also retained to support easy
> backward compatibility for distro vendors. These configurations are
> stored in temp files which are further translated using the
> hv_set_ifconfig.sh script. This script is implemented by individual
> distros based on the network management commands supported.
> For example, RHEL's implementation could be found here:
> https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> Debian's implementation could be found here:
> https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
>
> The next part of this support is to let the Distro vendors consume
> these modified implementations to the new configuration format.
>
> Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
Was this patch tested with ipv6? We are seeing a mix of ipv6 and ipv4 addresses in ipv6 section:
]# cat eth0.nmconnection
[connection]
autoconnect=yes
interface-name=eth0
[ethernet]
mac-address=00:15:5D:C4:63:45
[ipv6]
method=manual
address=1234:1234:1234:1234:0000:0000:0000:0113/120
gateway=10.73.199.254
dns=10.72.17.5
Gateway and dns should be in ipv6.
Ipv4 dns addresses comes from /etc/resolv.conf and in our system:
# /usr/libexec/hypervkvpd/hv_get_dns_info
10.72.17.5
10.68.5.26
[root@vm-197-248 ~]# cat /etc/resolv.conf
# Generated by NetworkManager
search lab.eng.pek2.redhat.com
nameserver 10.72.17.5
nameserver 10.68.5.26
So we should check if the addresses are indeed ipv6 or not and should not print it if !is_ipv4(). Ipv6 addresses may not be provisioned.
I will post a patch fix when we complete our internal testing.
> Signed-off-by: Shradha Gupta <[email protected]>
> Reviewed-by: Saurabh Sengar <[email protected]>
> ---
> Changes v7->v8
> * Fix some filename variable names to avoid confusion
> * Add initialization of is_ipv6 variable
> * Add a few comments
> ---
> tools/hv/hv_kvp_daemon.c | 233 +++++++++++++++++++++++++++++++-----
> tools/hv/hv_set_ifconfig.sh | 39 +++++-
> 2 files changed, 235 insertions(+), 37 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 27f5e7dfc2f7..264eeb9c46a9 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -1171,12 +1171,79 @@ static int process_ip_string(FILE *f, char *ip_string, int type)
> return 0;
> }
>
> +/*
> + * Only IPv4 subnet strings needs to be converted to plen
> + * For IPv6 the subnet is already privided in plen format
> + */
> +static int kvp_subnet_to_plen(char *subnet_addr_str)
> +{
> + int plen = 0;
> + struct in_addr subnet_addr4;
> +
> + /*
> + * Convert subnet address to binary representation
> + */
> + if (inet_pton(AF_INET, subnet_addr_str, &subnet_addr4) == 1) {
> + uint32_t subnet_mask = ntohl(subnet_addr4.s_addr);
> +
> + while (subnet_mask & 0x80000000) {
> + plen++;
> + subnet_mask <<= 1;
> + }
> + } else {
> + return -1;
> + }
> +
> + return plen;
> +}
> +
> +static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> + int is_ipv6)
> +{
> + char addr[INET6_ADDRSTRLEN];
> + char subnet_addr[INET6_ADDRSTRLEN];
> + int error, i = 0;
This should be initialised to 1
https://developer-old.gnome.org/NetworkManager/stable/nm-settings-keyfile.html#:~:text=File%20Format,%2Dsettings(5)).
Also see below.
> + int ip_offset = 0, subnet_offset = 0;
> + int plen;
> +
> + memset(addr, 0, sizeof(addr));
> + memset(subnet_addr, 0, sizeof(subnet_addr));
> +
> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> + (MAX_IP_ADDR_SIZE * 2)) &&
> + parse_ip_val_buffer(subnet,
> + &subnet_offset,
> + subnet_addr,
> + (MAX_IP_ADDR_SIZE *
> + 2))) {
> + if (!is_ipv6)
> + plen = kvp_subnet_to_plen((char *)subnet_addr);
> + else
> + plen = atoi(subnet_addr);
> +
> + if (plen < 0)
> + return plen;
> +
> + error = fprintf(f, "address%d=%s/%d\n", ++i, (char *)addr,
> + plen);
> + if (error < 0)
> + return error;
> +
> + memset(addr, 0, sizeof(addr));
> + memset(subnet_addr, 0, sizeof(subnet_addr));
> + }
> +
> + return 0;
> +}
> +
> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> {
> int error = 0;
> - char if_file[PATH_MAX];
> - FILE *file;
> + char if_filename[PATH_MAX];
> + char nm_filename[PATH_MAX];
> + FILE *ifcfg_file, *nmfile;
> char cmd[PATH_MAX];
> + int is_ipv6 = 0;
> char *mac_addr;
> int str_len;
>
> @@ -1197,7 +1264,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * in a given distro to configure the interface and so are free
> * ignore information that may not be relevant.
> *
> - * Here is the format of the ip configuration file:
> + * Here is the ifcfg format of the ip configuration file:
> *
> * HWADDR=macaddr
> * DEVICE=interface name
> @@ -1220,6 +1287,32 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> * IPV6NETMASK.
> *
> + * Here is the keyfile format of the ip configuration file:
> + *
> + * [ethernet]
> + * mac-address=macaddr
> + * [connection]
> + * interface-name=interface name
> + *
> + * [ipv4]
> + * method=<protocol> (where <protocol> is "auto" if DHCP is configured
> + * or "manual" if no boot-time protocol should be used)
> + *
> + * address1=ipaddr1/plen
> + * address2=ipaddr2/plen
> + *
> + * gateway=gateway1;gateway2
> + *
> + * dns=dns1;dns2
> + *
> + * [ipv6]
> + * address1=ipaddr1/plen
> + * address2=ipaddr2/plen
> + *
> + * gateway=gateway1;gateway2
> + *
> + * dns=dns1;dns2
> + *
> * The host can specify multiple ipv4 and ipv6 addresses to be
> * configured for the interface. Furthermore, the configuration
> * needs to be persistent. A subsequent GET call on the interface
> @@ -1227,14 +1320,29 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * call.
> */
>
> - snprintf(if_file, sizeof(if_file), "%s%s%s", KVP_CONFIG_LOC,
> - "/ifcfg-", if_name);
> + /*
> + * We are populating both ifcfg and nmconnection files
> + */
> + snprintf(if_filename, sizeof(if_filename), "%s%s%s", KVP_CONFIG_LOC,
> + "/ifcfg-", if_name);
>
> - file = fopen(if_file, "w");
> + ifcfg_file = fopen(if_filename, "w");
>
> - if (file == NULL) {
> + if (!ifcfg_file) {
> syslog(LOG_ERR, "Failed to open config file; error: %d %s",
> - errno, strerror(errno));
> + errno, strerror(errno));
> + return HV_E_FAIL;
> + }
> +
> + snprintf(nm_filename, sizeof(nm_filename), "%s%s%s%s", KVP_CONFIG_LOC,
> + "/", if_name, ".nmconnection");
> +
> + nmfile = fopen(nm_filename, "w");
> +
> + if (!nmfile) {
> + syslog(LOG_ERR, "Failed to open config file; error: %d %s",
> + errno, strerror(errno));
> + fclose(ifcfg_file);
> return HV_E_FAIL;
> }
>
> @@ -1248,14 +1356,31 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> goto setval_error;
> }
>
> - error = kvp_write_file(file, "HWADDR", "", mac_addr);
> - free(mac_addr);
> + error = kvp_write_file(ifcfg_file, "HWADDR", "", mac_addr);
> + if (error < 0)
> + goto setmac_error;
> +
> + error = kvp_write_file(ifcfg_file, "DEVICE", "", if_name);
> + if (error < 0)
> + goto setmac_error;
> +
> + error = fprintf(nmfile, "\n[connection]\n");
> + if (error < 0)
> + goto setmac_error;
> +
> + error = kvp_write_file(nmfile, "interface-name", "", if_name);
> if (error)
> - goto setval_error;
> + goto setmac_error;
>
> - error = kvp_write_file(file, "DEVICE", "", if_name);
> + error = fprintf(nmfile, "\n[ethernet]\n");
> + if (error < 0)
> + goto setmac_error;
> +
> + error = kvp_write_file(nmfile, "mac-address", "", mac_addr);
> if (error)
> - goto setval_error;
> + goto setmac_error;
> +
> + free(mac_addr);
>
> /*
> * The dhcp_enabled flag is only for IPv4. In the case the host only
> @@ -1263,47 +1388,91 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> * proceed to parse and pass the IPv6 information to the
> * disto-specific script hv_set_ifconfig.
> */
> +
> + /*
> + * First populate the ifcfg file format
> + */
> if (new_val->dhcp_enabled) {
> - error = kvp_write_file(file, "BOOTPROTO", "", "dhcp");
> + error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "dhcp");
> if (error)
> goto setval_error;
> -
> } else {
> - error = kvp_write_file(file, "BOOTPROTO", "", "none");
> + error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "none");
> if (error)
> goto setval_error;
> }
>
> - /*
> - * Write the configuration for ipaddress, netmask, gateway and
> - * name servers.
> - */
> -
> - error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> + error = process_ip_string(ifcfg_file, (char *)new_val->ip_addr,
> + IPADDR);
> if (error)
> goto setval_error;
>
> - error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> + error = process_ip_string(ifcfg_file, (char *)new_val->sub_net,
> + NETMASK);
> if (error)
> goto setval_error;
>
> - error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> + error = process_ip_string(ifcfg_file, (char *)new_val->gate_way,
> + GATEWAY);
> if (error)
> goto setval_error;
>
> - error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> + error = process_ip_string(ifcfg_file, (char *)new_val->dns_addr, DNS);
> if (error)
> goto setval_error;
>
> - fclose(file);
> + if (new_val->addr_family == ADDR_FAMILY_IPV6) {
I think we should have done something like
new_val->addr_family & ADDR_FAMILY_IPV6 here.
> + error = fprintf(nmfile, "\n[ipv6]\n");
> + if (error < 0)
> + goto setval_error;
> + is_ipv6 = 1;
> + } else {
> + error = fprintf(nmfile, "\n[ipv4]\n");
> + if (error < 0)
> + goto setval_error;
> + }
> +
> + /*
> + * Now we populate the keyfile format
> + */
> +
> + if (new_val->dhcp_enabled) {
> + error = kvp_write_file(nmfile, "method", "", "auto");
> + if (error < 0)
> + goto setval_error;
> + } else {
> + error = kvp_write_file(nmfile, "method", "", "manual");
> + if (error < 0)
> + goto setval_error;
> + }
> +
> + /*
> + * Write the configuration for ipaddress, netmask, gateway and
> + * name services
> + */
> + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> + (char *)new_val->sub_net, is_ipv6);
> + if (error < 0)
> + goto setval_error;
> +
> + error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
> + if (error < 0)
> + goto setval_error;
> +
> + error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
> + if (error < 0)
> + goto setval_error;
> +
> + fclose(nmfile);
> + fclose(ifcfg_file);
>
> /*
> * Now that we have populated the configuration file,
> * invoke the external script to do its magic.
> */
>
> - str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s",
> - "hv_set_ifconfig", if_file);
> + str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s %s",
> + "hv_set_ifconfig", if_filename, nm_filename);
> /*
> * This is a little overcautious, but it's necessary to suppress some
> * false warnings from gcc 8.0.1.
> @@ -1316,14 +1485,16 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>
> if (system(cmd)) {
> syslog(LOG_ERR, "Failed to execute cmd '%s'; error: %d %s",
> - cmd, errno, strerror(errno));
> + cmd, errno, strerror(errno));
> return HV_E_FAIL;
> }
> return 0;
> -
> +setmac_error:
> + free(mac_addr);
> setval_error:
> syslog(LOG_ERR, "Failed to write config file");
> - fclose(file);
> + fclose(ifcfg_file);
> + fclose(nmfile);
> return error;
> }
>
> diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
> index d10fe35b7f25..ae5a7a8249a2 100755
> --- a/tools/hv/hv_set_ifconfig.sh
> +++ b/tools/hv/hv_set_ifconfig.sh
> @@ -18,12 +18,12 @@
> #
> # This example script is based on a RHEL environment.
> #
> -# Here is the format of the ip configuration file:
> +# Here is the ifcfg format of the ip configuration file:
> #
> # HWADDR=macaddr
> # DEVICE=interface name
> # BOOTPROTO=<protocol> (where <protocol> is "dhcp" if DHCP is configured
> -# or "none" if no boot-time protocol should be used)
> +# or "none" if no boot-time protocol should be used)
> #
> # IPADDR0=ipaddr1
> # IPADDR1=ipaddr2
> @@ -41,6 +41,32 @@
> # tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> # IPV6NETMASK.
> #
> +# Here is the keyfile format of the ip configuration file:
> +#
> +# [ethernet]
> +# mac-address=macaddr
> +# [connection]
> +# interface-name=interface name
> +#
> +# [ipv4]
> +# method=<protocol> (where <protocol> is "auto" if DHCP is configured
> +# or "manual" if no boot-time protocol should be used)
> +#
> +# address1=ipaddr1/plen
> +# address=ipaddr2/plen
address2 here?
> +#
> +# gateway=gateway1;gateway2
> +#
> +# dns=dns1;
> +#
> +# [ipv6]
> +# address1=ipaddr1/plen
> +# address2=ipaddr1/plen
ipaddr2 ?
> +#
> +# gateway=gateway1;gateway2
> +#
> +# dns=dns1;dns2
> +#
> # The host can specify multiple ipv4 and ipv6 addresses to be
> # configured for the interface. Furthermore, the configuration
> # needs to be persistent. A subsequent GET call on the interface
> @@ -48,18 +74,19 @@
> # call.
> #
>
> -
> -
> echo "IPV6INIT=yes" >> $1
> echo "NM_CONTROLLED=no" >> $1
> echo "PEERDNS=yes" >> $1
> echo "ONBOOT=yes" >> $1
>
> -
> cp $1 /etc/sysconfig/network-scripts/
>
> +chmod 600 $2
> +interface=$(echo $2 | awk -F - '{ print $2 }')
> +filename="${2##*/}"
> +
> +sed '/\[connection\]/a autoconnect=true' $2 > /etc/NetworkManager/system-connections/${filename}
>
> -interface=$(echo $1 | awk -F - '{ print $2 }')
>
> /sbin/ifdown $interface 2>/dev/null
> /sbin/ifup $interface 2>/dev/null
> --
> 2.34.1
>
> On 13-Oct-2023, at 3:06 PM, Ani Sinha <[email protected]> wrote:
>
>
>
>> On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
>>
>> Ifcfg config file support in NetworkManger is deprecated. This patch
>> provides support for the new keyfile config format for connection
>> profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
>> to generate the new network configuration in keyfile
>> format(.ini-style format) along with a ifcfg format configuration.
>> The ifcfg format configuration is also retained to support easy
>> backward compatibility for distro vendors. These configurations are
>> stored in temp files which are further translated using the
>> hv_set_ifconfig.sh script. This script is implemented by individual
>> distros based on the network management commands supported.
>> For example, RHEL's implementation could be found here:
>> https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
>> Debian's implementation could be found here:
>> https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
>>
>> The next part of this support is to let the Distro vendors consume
>> these modified implementations to the new configuration format.
>>
>> Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
>
> Was this patch tested with ipv6? We are seeing a mix of ipv6 and ipv4 addresses in ipv6 section:
>
> ]# cat eth0.nmconnection
>
> [connection]
> autoconnect=yes
> interface-name=eth0
>
> [ethernet]
> mac-address=00:15:5D:C4:63:45
>
> [ipv6]
> method=manual
> address=1234:1234:1234:1234:0000:0000:0000:0113/120
> gateway=10.73.199.254
> dns=10.72.17.5
>
> Gateway and dns should be in ipv6.
>
> Ipv4 dns addresses comes from /etc/resolv.conf and in our system:
>
> # /usr/libexec/hypervkvpd/hv_get_dns_info
> 10.72.17.5
> 10.68.5.26
>
> [root@vm-197-248 ~]# cat /etc/resolv.conf
> # Generated by NetworkManager
> search lab.eng.pek2.redhat.com
> nameserver 10.72.17.5
> nameserver 10.68.5.26
>
> So we should check if the addresses are indeed ipv6 or not and should not print it if !is_ipv4(). Ipv6 addresses may not be provisioned.
>
> I will post a patch fix when we complete our internal testing.
>
>
>> Signed-off-by: Shradha Gupta <[email protected]>
>> Reviewed-by: Saurabh Sengar <[email protected]>
>> ---
>> Changes v7->v8
>> * Fix some filename variable names to avoid confusion
>> * Add initialization of is_ipv6 variable
>> * Add a few comments
>> ---
>> tools/hv/hv_kvp_daemon.c | 233 +++++++++++++++++++++++++++++++-----
>> tools/hv/hv_set_ifconfig.sh | 39 +++++-
>> 2 files changed, 235 insertions(+), 37 deletions(-)
>>
>> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
>> index 27f5e7dfc2f7..264eeb9c46a9 100644
>> --- a/tools/hv/hv_kvp_daemon.c
>> +++ b/tools/hv/hv_kvp_daemon.c
>> @@ -1171,12 +1171,79 @@ static int process_ip_string(FILE *f, char *ip_string, int type)
>> return 0;
>> }
>>
>> +/*
>> + * Only IPv4 subnet strings needs to be converted to plen
>> + * For IPv6 the subnet is already privided in plen format
>> + */
>> +static int kvp_subnet_to_plen(char *subnet_addr_str)
>> +{
>> + int plen = 0;
>> + struct in_addr subnet_addr4;
>> +
>> + /*
>> + * Convert subnet address to binary representation
>> + */
>> + if (inet_pton(AF_INET, subnet_addr_str, &subnet_addr4) == 1) {
>> + uint32_t subnet_mask = ntohl(subnet_addr4.s_addr);
>> +
>> + while (subnet_mask & 0x80000000) {
>> + plen++;
>> + subnet_mask <<= 1;
>> + }
>> + } else {
>> + return -1;
>> + }
>> +
>> + return plen;
>> +}
>> +
>> +static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
>> + int is_ipv6)
>> +{
>> + char addr[INET6_ADDRSTRLEN];
>> + char subnet_addr[INET6_ADDRSTRLEN];
>> + int error, i = 0;
>
> This should be initialised to 1
Sorry we already do a ++i and not i++ so it should be fine.
>
> https://developer-old.gnome.org/NetworkManager/stable/nm-settings-keyfile.html#:~:text=File%20Format,%2Dsettings(5)).
>
> Also see below.
>
>> + int ip_offset = 0, subnet_offset = 0;
>> + int plen;
>> +
>> + memset(addr, 0, sizeof(addr));
>> + memset(subnet_addr, 0, sizeof(subnet_addr));
>> +
>> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
>> + (MAX_IP_ADDR_SIZE * 2)) &&
>> + parse_ip_val_buffer(subnet,
>> + &subnet_offset,
>> + subnet_addr,
>> + (MAX_IP_ADDR_SIZE *
>> + 2))) {
>> + if (!is_ipv6)
>> + plen = kvp_subnet_to_plen((char *)subnet_addr);
>> + else
>> + plen = atoi(subnet_addr);
>> +
>> + if (plen < 0)
>> + return plen;
>> +
>> + error = fprintf(f, "address%d=%s/%d\n", ++i, (char *)addr,
>> + plen);
>> + if (error < 0)
>> + return error;
>> +
>> + memset(addr, 0, sizeof(addr));
>> + memset(subnet_addr, 0, sizeof(subnet_addr));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>> {
>> int error = 0;
>> - char if_file[PATH_MAX];
>> - FILE *file;
>> + char if_filename[PATH_MAX];
>> + char nm_filename[PATH_MAX];
>> + FILE *ifcfg_file, *nmfile;
>> char cmd[PATH_MAX];
>> + int is_ipv6 = 0;
>> char *mac_addr;
>> int str_len;
>>
>> @@ -1197,7 +1264,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>> * in a given distro to configure the interface and so are free
>> * ignore information that may not be relevant.
>> *
>> - * Here is the format of the ip configuration file:
>> + * Here is the ifcfg format of the ip configuration file:
>> *
>> * HWADDR=macaddr
>> * DEVICE=interface name
>> @@ -1220,6 +1287,32 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>> * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
>> * IPV6NETMASK.
>> *
>> + * Here is the keyfile format of the ip configuration file:
>> + *
>> + * [ethernet]
>> + * mac-address=macaddr
>> + * [connection]
>> + * interface-name=interface name
>> + *
>> + * [ipv4]
>> + * method=<protocol> (where <protocol> is "auto" if DHCP is configured
>> + * or "manual" if no boot-time protocol should be used)
>> + *
>> + * address1=ipaddr1/plen
>> + * address2=ipaddr2/plen
>> + *
>> + * gateway=gateway1;gateway2
>> + *
>> + * dns=dns1;dns2
>> + *
>> + * [ipv6]
>> + * address1=ipaddr1/plen
>> + * address2=ipaddr2/plen
>> + *
>> + * gateway=gateway1;gateway2
>> + *
>> + * dns=dns1;dns2
>> + *
>> * The host can specify multiple ipv4 and ipv6 addresses to be
>> * configured for the interface. Furthermore, the configuration
>> * needs to be persistent. A subsequent GET call on the interface
>> @@ -1227,14 +1320,29 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>> * call.
>> */
>>
>> - snprintf(if_file, sizeof(if_file), "%s%s%s", KVP_CONFIG_LOC,
>> - "/ifcfg-", if_name);
>> + /*
>> + * We are populating both ifcfg and nmconnection files
>> + */
>> + snprintf(if_filename, sizeof(if_filename), "%s%s%s", KVP_CONFIG_LOC,
>> + "/ifcfg-", if_name);
>>
>> - file = fopen(if_file, "w");
>> + ifcfg_file = fopen(if_filename, "w");
>>
>> - if (file == NULL) {
>> + if (!ifcfg_file) {
>> syslog(LOG_ERR, "Failed to open config file; error: %d %s",
>> - errno, strerror(errno));
>> + errno, strerror(errno));
>> + return HV_E_FAIL;
>> + }
>> +
>> + snprintf(nm_filename, sizeof(nm_filename), "%s%s%s%s", KVP_CONFIG_LOC,
>> + "/", if_name, ".nmconnection");
>> +
>> + nmfile = fopen(nm_filename, "w");
>> +
>> + if (!nmfile) {
>> + syslog(LOG_ERR, "Failed to open config file; error: %d %s",
>> + errno, strerror(errno));
>> + fclose(ifcfg_file);
>> return HV_E_FAIL;
>> }
>>
>> @@ -1248,14 +1356,31 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>> goto setval_error;
>> }
>>
>> - error = kvp_write_file(file, "HWADDR", "", mac_addr);
>> - free(mac_addr);
>> + error = kvp_write_file(ifcfg_file, "HWADDR", "", mac_addr);
>> + if (error < 0)
>> + goto setmac_error;
>> +
>> + error = kvp_write_file(ifcfg_file, "DEVICE", "", if_name);
>> + if (error < 0)
>> + goto setmac_error;
>> +
>> + error = fprintf(nmfile, "\n[connection]\n");
>> + if (error < 0)
>> + goto setmac_error;
>> +
>> + error = kvp_write_file(nmfile, "interface-name", "", if_name);
>> if (error)
>> - goto setval_error;
>> + goto setmac_error;
>>
>> - error = kvp_write_file(file, "DEVICE", "", if_name);
>> + error = fprintf(nmfile, "\n[ethernet]\n");
>> + if (error < 0)
>> + goto setmac_error;
>> +
>> + error = kvp_write_file(nmfile, "mac-address", "", mac_addr);
>> if (error)
>> - goto setval_error;
>> + goto setmac_error;
>> +
>> + free(mac_addr);
>>
>> /*
>> * The dhcp_enabled flag is only for IPv4. In the case the host only
>> @@ -1263,47 +1388,91 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>> * proceed to parse and pass the IPv6 information to the
>> * disto-specific script hv_set_ifconfig.
>> */
>> +
>> + /*
>> + * First populate the ifcfg file format
>> + */
>> if (new_val->dhcp_enabled) {
>> - error = kvp_write_file(file, "BOOTPROTO", "", "dhcp");
>> + error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "dhcp");
>> if (error)
>> goto setval_error;
>> -
>> } else {
>> - error = kvp_write_file(file, "BOOTPROTO", "", "none");
>> + error = kvp_write_file(ifcfg_file, "BOOTPROTO", "", "none");
>> if (error)
>> goto setval_error;
>> }
>>
>> - /*
>> - * Write the configuration for ipaddress, netmask, gateway and
>> - * name servers.
>> - */
>> -
>> - error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
>> + error = process_ip_string(ifcfg_file, (char *)new_val->ip_addr,
>> + IPADDR);
>> if (error)
>> goto setval_error;
>>
>> - error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
>> + error = process_ip_string(ifcfg_file, (char *)new_val->sub_net,
>> + NETMASK);
>> if (error)
>> goto setval_error;
>>
>> - error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
>> + error = process_ip_string(ifcfg_file, (char *)new_val->gate_way,
>> + GATEWAY);
>> if (error)
>> goto setval_error;
>>
>> - error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
>> + error = process_ip_string(ifcfg_file, (char *)new_val->dns_addr, DNS);
>> if (error)
>> goto setval_error;
>>
>> - fclose(file);
>> + if (new_val->addr_family == ADDR_FAMILY_IPV6) {
>
> I think we should have done something like
>
> new_val->addr_family & ADDR_FAMILY_IPV6 here.
>
>> + error = fprintf(nmfile, "\n[ipv6]\n");
>> + if (error < 0)
>> + goto setval_error;
>> + is_ipv6 = 1;
>> + } else {
>> + error = fprintf(nmfile, "\n[ipv4]\n");
>> + if (error < 0)
>> + goto setval_error;
>> + }
>> +
>> + /*
>> + * Now we populate the keyfile format
>> + */
>> +
>> + if (new_val->dhcp_enabled) {
>> + error = kvp_write_file(nmfile, "method", "", "auto");
>> + if (error < 0)
>> + goto setval_error;
>> + } else {
>> + error = kvp_write_file(nmfile, "method", "", "manual");
>> + if (error < 0)
>> + goto setval_error;
>> + }
>> +
>> + /*
>> + * Write the configuration for ipaddress, netmask, gateway and
>> + * name services
>> + */
>> + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
>> + (char *)new_val->sub_net, is_ipv6);
>> + if (error < 0)
>> + goto setval_error;
>> +
>> + error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
>> + if (error < 0)
>> + goto setval_error;
>> +
>> + error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
>> + if (error < 0)
>> + goto setval_error;
>> +
>> + fclose(nmfile);
>> + fclose(ifcfg_file);
>>
>> /*
>> * Now that we have populated the configuration file,
>> * invoke the external script to do its magic.
>> */
>>
>> - str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s",
>> - "hv_set_ifconfig", if_file);
>> + str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s %s",
>> + "hv_set_ifconfig", if_filename, nm_filename);
>> /*
>> * This is a little overcautious, but it's necessary to suppress some
>> * false warnings from gcc 8.0.1.
>> @@ -1316,14 +1485,16 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>>
>> if (system(cmd)) {
>> syslog(LOG_ERR, "Failed to execute cmd '%s'; error: %d %s",
>> - cmd, errno, strerror(errno));
>> + cmd, errno, strerror(errno));
>> return HV_E_FAIL;
>> }
>> return 0;
>> -
>> +setmac_error:
>> + free(mac_addr);
>> setval_error:
>> syslog(LOG_ERR, "Failed to write config file");
>> - fclose(file);
>> + fclose(ifcfg_file);
>> + fclose(nmfile);
>> return error;
>> }
>>
>> diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
>> index d10fe35b7f25..ae5a7a8249a2 100755
>> --- a/tools/hv/hv_set_ifconfig.sh
>> +++ b/tools/hv/hv_set_ifconfig.sh
>> @@ -18,12 +18,12 @@
>> #
>> # This example script is based on a RHEL environment.
>> #
>> -# Here is the format of the ip configuration file:
>> +# Here is the ifcfg format of the ip configuration file:
>> #
>> # HWADDR=macaddr
>> # DEVICE=interface name
>> # BOOTPROTO=<protocol> (where <protocol> is "dhcp" if DHCP is configured
>> -# or "none" if no boot-time protocol should be used)
>> +# or "none" if no boot-time protocol should be used)
>> #
>> # IPADDR0=ipaddr1
>> # IPADDR1=ipaddr2
>> @@ -41,6 +41,32 @@
>> # tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
>> # IPV6NETMASK.
>> #
>> +# Here is the keyfile format of the ip configuration file:
>> +#
>> +# [ethernet]
>> +# mac-address=macaddr
>> +# [connection]
>> +# interface-name=interface name
>> +#
>> +# [ipv4]
>> +# method=<protocol> (where <protocol> is "auto" if DHCP is configured
>> +# or "manual" if no boot-time protocol should be used)
>> +#
>> +# address1=ipaddr1/plen
>> +# address=ipaddr2/plen
>
> address2 here?
>
>> +#
>> +# gateway=gateway1;gateway2
>> +#
>> +# dns=dns1;
>> +#
>> +# [ipv6]
>> +# address1=ipaddr1/plen
>> +# address2=ipaddr1/plen
>
> ipaddr2 ?
>
>> +#
>> +# gateway=gateway1;gateway2
>> +#
>> +# dns=dns1;dns2
>> +#
>> # The host can specify multiple ipv4 and ipv6 addresses to be
>> # configured for the interface. Furthermore, the configuration
>> # needs to be persistent. A subsequent GET call on the interface
>> @@ -48,18 +74,19 @@
>> # call.
>> #
>>
>> -
>> -
>> echo "IPV6INIT=yes" >> $1
>> echo "NM_CONTROLLED=no" >> $1
>> echo "PEERDNS=yes" >> $1
>> echo "ONBOOT=yes" >> $1
>>
>> -
>> cp $1 /etc/sysconfig/network-scripts/
>>
>> +chmod 600 $2
>> +interface=$(echo $2 | awk -F - '{ print $2 }')
>> +filename="${2##*/}"
>> +
>> +sed '/\[connection\]/a autoconnect=true' $2 > /etc/NetworkManager/system-connections/${filename}
>>
>> -interface=$(echo $1 | awk -F - '{ print $2 }')
>>
>> /sbin/ifdown $interface 2>/dev/null
>> /sbin/ifup $interface 2>/dev/null
>> --
>> 2.34.1
On Fri, Oct 13, 2023 at 3:06 PM Ani Sinha <[email protected]> wrote:
>
>
>
> > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
> >
> > Ifcfg config file support in NetworkManger is deprecated. This patch
> > provides support for the new keyfile config format for connection
> > profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> > to generate the new network configuration in keyfile
> > format(.ini-style format) along with a ifcfg format configuration.
> > The ifcfg format configuration is also retained to support easy
> > backward compatibility for distro vendors. These configurations are
> > stored in temp files which are further translated using the
> > hv_set_ifconfig.sh script. This script is implemented by individual
> > distros based on the network management commands supported.
> > For example, RHEL's implementation could be found here:
> > https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> > Debian's implementation could be found here:
> > https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
> >
> > The next part of this support is to let the Distro vendors consume
> > these modified implementations to the new configuration format.
> >
> > Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
>
> Was this patch tested with ipv6? We are seeing a mix of ipv6 and ipv4 addresses in ipv6 section:
There is also another issue which is kind of a design problem that
existed from the get go but now is exposed since keyfile support was
added.
Imagine we configure both ipv6 and ipv4 and some interfaces have ipv4
addresses and some have ipv6.
getifaddres() call in kvp_get_ip_info() will return a linked list per
interface. The code sets ip_buffer->addr_family based on the address
family of the address set for the interface. We use this to determine
which section in the keyfile to use, ipv6 or ipv4. However, once we
make this decision, we are locked in. The issue here is that
kvp_process_ip_address() that extracts the IP addresses concatenate
the addresses in a single buffer separating the IPs with ";". Thus
across interfaces, the buffer can contain both ipv4 and ipv6 addresses
separated by ";" if both v4 and v6 are configured. This is problematic
as the addr_family can be either ipv4 or ipv6 but not both.
Essentially, we can have a situation that for a single addr_family in
hv_kvp_ipaddr_value struct, the ip_addr member can be a buffer
containing both ipv6 and ipv4 addresses. Notice that
process_ip_string() handles this by iterating through the string and
for each ip extracted, it individually determines if the IP is a v6 or
a v4 and adds "IPV6ADDR" or "IPADDR" to the ifcfg file accordingly.
process_ip_string_nm() does not do that and solely makes the
determination based on is_ipv6 values which is based on a single
addr_family value above. Thus, it cannot possibly know whether the
specific IP address extracted from the string is a v4 or v6. Unlike
for ifcfg files, fir nm keyfiles, we need to add v4 and v6 addresses
in specific sections and we cannot mix the two. So we need to make two
passes. One for v4 and one for v6 and then add IPs in the respective
sections.
This issue needs to be looked into and unless it's resolved, we cannot
support both ipv4 and ipv6 addresses at the same time.
Thoughts?
On Sat, Dec 23, 2023 at 12:43 PM Ani Sinha <[email protected]> wrote:
>
> On Fri, Oct 13, 2023 at 3:06 PM Ani Sinha <[email protected]> wrote:
> >
> >
> >
> > > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
> > >
> > > Ifcfg config file support in NetworkManger is deprecated. This patch
> > > provides support for the new keyfile config format for connection
> > > profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> > > to generate the new network configuration in keyfile
> > > format(.ini-style format) along with a ifcfg format configuration.
> > > The ifcfg format configuration is also retained to support easy
> > > backward compatibility for distro vendors. These configurations are
> > > stored in temp files which are further translated using the
> > > hv_set_ifconfig.sh script. This script is implemented by individual
> > > distros based on the network management commands supported.
> > > For example, RHEL's implementation could be found here:
> > > https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> > > Debian's implementation could be found here:
> > > https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
> > >
> > > The next part of this support is to let the Distro vendors consume
> > > these modified implementations to the new configuration format.
> > >
> > > Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
> >
> > Was this patch tested with ipv6? We are seeing a mix of ipv6 and ipv4 addresses in ipv6 section:
>
> There is also another issue which is kind of a design problem that
> existed from the get go but now is exposed since keyfile support was
> added.
> Imagine we configure both ipv6 and ipv4 and some interfaces have ipv4
> addresses and some have ipv6.
> getifaddres() call in kvp_get_ip_info() will return a linked list per
> interface. The code sets ip_buffer->addr_family based on the address
> family of the address set for the interface. We use this to determine
> which section in the keyfile to use, ipv6 or ipv4. However, once we
> make this decision, we are locked in. The issue here is that
> kvp_process_ip_address() that extracts the IP addresses concatenate
> the addresses in a single buffer separating the IPs with ";". Thus
> across interfaces, the buffer can contain both ipv4 and ipv6 addresses
> separated by ";" if both v4 and v6 are configured. This is problematic
> as the addr_family can be either ipv4 or ipv6 but not both.
> Essentially, we can have a situation that for a single addr_family in
> hv_kvp_ipaddr_value struct, the ip_addr member can be a buffer
> containing both ipv6 and ipv4 addresses. Notice that
> process_ip_string() handles this by iterating through the string and
> for each ip extracted, it individually determines if the IP is a v6 or
> a v4 and adds "IPV6ADDR" or "IPADDR" to the ifcfg file accordingly.
> process_ip_string_nm() does not do that and solely makes the
> determination based on is_ipv6 values which is based on a single
> addr_family value above. Thus, it cannot possibly know whether the
> specific IP address extracted from the string is a v4 or v6. Unlike
> for ifcfg files, fir nm keyfiles, we need to add v4 and v6 addresses
> in specific sections and we cannot mix the two. So we need to make two
> passes. One for v4 and one for v6 and then add IPs in the respective
> sections.
>
> This issue needs to be looked into and unless it's resolved, we cannot
> support both ipv4 and ipv6 addresses at the same time.
In the short term, we should probably do this to avoid the mismatch :
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 318e2dad27e0..b77c8edfe663 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -1216,6 +1216,9 @@ static int process_ip_string_nm(FILE *f, char
*ip_string, char *subnet,
subnet_addr,
(MAX_IP_ADDR_SIZE *
2))) {
+ if (is_ipv6 == is_ipv4((char*) addr))
+ continue;
+
if (!is_ipv6)
plen = kvp_subnet_to_plen((char *)subnet_addr);
else
But this is really a short term hack.
On Sat, Dec 23, 2023 at 01:35:26PM +0530, Ani Sinha wrote:
> On Sat, Dec 23, 2023 at 12:43???PM Ani Sinha <[email protected]> wrote:
> >
> > On Fri, Oct 13, 2023 at 3:06???PM Ani Sinha <[email protected]> wrote:
> > >
> > >
> > >
> > > > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <[email protected]> wrote:
> > > >
> > > > Ifcfg config file support in NetworkManger is deprecated. This patch
> > > > provides support for the new keyfile config format for connection
> > > > profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> > > > to generate the new network configuration in keyfile
> > > > format(.ini-style format) along with a ifcfg format configuration.
> > > > The ifcfg format configuration is also retained to support easy
> > > > backward compatibility for distro vendors. These configurations are
> > > > stored in temp files which are further translated using the
> > > > hv_set_ifconfig.sh script. This script is implemented by individual
> > > > distros based on the network management commands supported.
> > > > For example, RHEL's implementation could be found here:
> > > > https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> > > > Debian's implementation could be found here:
> > > > https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
> > > >
> > > > The next part of this support is to let the Distro vendors consume
> > > > these modified implementations to the new configuration format.
> > > >
> > > > Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
> > >
> > > Was this patch tested with ipv6? We are seeing a mix of ipv6 and ipv4 addresses in ipv6 section:
> >
> > There is also another issue which is kind of a design problem that
> > existed from the get go but now is exposed since keyfile support was
> > added.
> > Imagine we configure both ipv6 and ipv4 and some interfaces have ipv4
> > addresses and some have ipv6.
> > getifaddres() call in kvp_get_ip_info() will return a linked list per
> > interface. The code sets ip_buffer->addr_family based on the address
> > family of the address set for the interface. We use this to determine
> > which section in the keyfile to use, ipv6 or ipv4. However, once we
> > make this decision, we are locked in. The issue here is that
> > kvp_process_ip_address() that extracts the IP addresses concatenate
> > the addresses in a single buffer separating the IPs with ";". Thus
> > across interfaces, the buffer can contain both ipv4 and ipv6 addresses
> > separated by ";" if both v4 and v6 are configured. This is problematic
> > as the addr_family can be either ipv4 or ipv6 but not both.
> > Essentially, we can have a situation that for a single addr_family in
> > hv_kvp_ipaddr_value struct, the ip_addr member can be a buffer
> > containing both ipv6 and ipv4 addresses. Notice that
> > process_ip_string() handles this by iterating through the string and
> > for each ip extracted, it individually determines if the IP is a v6 or
> > a v4 and adds "IPV6ADDR" or "IPADDR" to the ifcfg file accordingly.
> > process_ip_string_nm() does not do that and solely makes the
> > determination based on is_ipv6 values which is based on a single
> > addr_family value above. Thus, it cannot possibly know whether the
> > specific IP address extracted from the string is a v4 or v6. Unlike
> > for ifcfg files, fir nm keyfiles, we need to add v4 and v6 addresses
> > in specific sections and we cannot mix the two. So we need to make two
> > passes. One for v4 and one for v6 and then add IPs in the respective
> > sections.
> >
> > This issue needs to be looked into and unless it's resolved, we cannot
> > support both ipv4 and ipv6 addresses at the same time.
>
> In the short term, we should probably do this to avoid the mismatch :
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 318e2dad27e0..b77c8edfe663 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -1216,6 +1216,9 @@ static int process_ip_string_nm(FILE *f, char
> *ip_string, char *subnet,
> subnet_addr,
> (MAX_IP_ADDR_SIZE *
> 2))) {
> + if (is_ipv6 == is_ipv4((char*) addr))
> + continue;
> +
> if (!is_ipv6)
> plen = kvp_subnet_to_plen((char *)subnet_addr);
> else
>
> But this is really a short term hack.
Thanks for bringing this up Ani. I will try to fix this in a new patch
(would get the new testcases added in our suite as well)
Hi Ani,
This patch should be out for review by mid next week.
Thanks and Regards,
Shradha.
On Sat, Feb 24, 2024 at 09:48:17PM +0530, Ani Sinha wrote:
> On Fri, 5 Jan, 2024, 11:12 am Shradha Gupta, <
> [email protected]> wrote:
>
> > On Sat, Dec 23, 2023 at 01:35:26PM +0530, Ani Sinha wrote:
> > > On Sat, Dec 23, 2023 at 12:43???PM Ani Sinha <[email protected]>
> > wrote:
> > > >
> > > > On Fri, Oct 13, 2023 at 3:06???PM Ani Sinha <[email protected]>
> > wrote:
> > > > >
> > > > >
> > > > >
> > > > > > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <
> > [email protected]> wrote:
> > > > > >
> > > > > > Ifcfg config file support in NetworkManger is deprecated. This
> > patch
> > > > > > provides support for the new keyfile config format for connection
> > > > > > profiles in NetworkManager. The patch modifies the hv_kvp_daemon
> > code
> > > > > > to generate the new network configuration in keyfile
> > > > > > format(.ini-style format) along with a ifcfg format configuration.
> > > > > > The ifcfg format configuration is also retained to support easy
> > > > > > backward compatibility for distro vendors. These configurations are
> > > > > > stored in temp files which are further translated using the
> > > > > > hv_set_ifconfig.sh script. This script is implemented by individual
> > > > > > distros based on the network management commands supported.
> > > > > > For example, RHEL's implementation could be found here:
> > > > > >
> > https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> > > > > > Debian's implementation could be found here:
> > > > > >
> > https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
> > > > > >
> > > > > > The next part of this support is to let the Distro vendors consume
> > > > > > these modified implementations to the new configuration format.
> > > > > >
> > > > > > Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
> > > > >
> > > > > Was this patch tested with ipv6? We are seeing a mix of ipv6 and
> > ipv4 addresses in ipv6 section:
> > > >
> > > > There is also another issue which is kind of a design problem that
> > > > existed from the get go but now is exposed since keyfile support was
> > > > added.
> > > > Imagine we configure both ipv6 and ipv4 and some interfaces have ipv4
> > > > addresses and some have ipv6.
> > > > getifaddres() call in kvp_get_ip_info() will return a linked list per
> > > > interface. The code sets ip_buffer->addr_family based on the address
> > > > family of the address set for the interface. We use this to determine
> > > > which section in the keyfile to use, ipv6 or ipv4. However, once we
> > > > make this decision, we are locked in. The issue here is that
> > > > kvp_process_ip_address() that extracts the IP addresses concatenate
> > > > the addresses in a single buffer separating the IPs with ";". Thus
> > > > across interfaces, the buffer can contain both ipv4 and ipv6 addresses
> > > > separated by ";" if both v4 and v6 are configured. This is problematic
> > > > as the addr_family can be either ipv4 or ipv6 but not both.
> > > > Essentially, we can have a situation that for a single addr_family in
> > > > hv_kvp_ipaddr_value struct, the ip_addr member can be a buffer
> > > > containing both ipv6 and ipv4 addresses. Notice that
> > > > process_ip_string() handles this by iterating through the string and
> > > > for each ip extracted, it individually determines if the IP is a v6 or
> > > > a v4 and adds "IPV6ADDR" or "IPADDR" to the ifcfg file accordingly.
> > > > process_ip_string_nm() does not do that and solely makes the
> > > > determination based on is_ipv6 values which is based on a single
> > > > addr_family value above. Thus, it cannot possibly know whether the
> > > > specific IP address extracted from the string is a v4 or v6. Unlike
> > > > for ifcfg files, fir nm keyfiles, we need to add v4 and v6 addresses
> > > > in specific sections and we cannot mix the two. So we need to make two
> > > > passes. One for v4 and one for v6 and then add IPs in the respective
> > > > sections.
> > > >
> > > > This issue needs to be looked into and unless it's resolved, we cannot
> > > > support both ipv4 and ipv6 addresses at the same time.
> > >
> > > In the short term, we should probably do this to avoid the mismatch :
> > >
> > > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > > index 318e2dad27e0..b77c8edfe663 100644
> > > --- a/tools/hv/hv_kvp_daemon.c
> > > +++ b/tools/hv/hv_kvp_daemon.c
> > > @@ -1216,6 +1216,9 @@ static int process_ip_string_nm(FILE *f, char
> > > *ip_string, char *subnet,
> > > subnet_addr,
> > > (MAX_IP_ADDR_SIZE
> > *
> > > 2))) {
> > > + if (is_ipv6 == is_ipv4((char*) addr))
> > > + continue;
> > > +
> > > if (!is_ipv6)
> > > plen = kvp_subnet_to_plen((char *)subnet_addr);
> > > else
> > >
> > > But this is really a short term hack.
> > Thanks for bringing this up Ani. I will try to fix this in a new patch
> > (would get the new testcases added in our suite as well)
> >
>
> Has there been any progress on this front?
>
>
> >