Re: [PATCH v8] hv/hv_kvp_daemon:Support for keyfile based connection profile

From: Ani Sinha
Date: Fri Oct 13 2023 - 07:37:59 EST

Next message: Maciej Wieczor-Retman: "[PATCH v6 6/8] selftests/kvm: Replace attribute with macro"
Previous message: Maciej Wieczor-Retman: "[PATCH v6 5/8] selftests/sigaltstack: Fix wrong format specifier"
In reply to: Ani Sinha: "Re: [PATCH v8] hv/hv_kvp_daemon:Support for keyfile based connection profile"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On 13-Oct-2023, at 3:06 PM, Ani Sinha <anisinha@xxxxxxxxxx> wrote:
>
>
>
>> On 09-Oct-2023, at 4:08 PM, Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx> 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 <shradhagupta@xxxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
>> ---
>> 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

Next message: Maciej Wieczor-Retman: "[PATCH v6 6/8] selftests/kvm: Replace attribute with macro"
Previous message: Maciej Wieczor-Retman: "[PATCH v6 5/8] selftests/sigaltstack: Fix wrong format specifier"
In reply to: Ani Sinha: "Re: [PATCH v8] hv/hv_kvp_daemon:Support for keyfile based connection profile"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]