On Sat, Dec 23, 2023 at 01:35:26PM +0530, Ani Sinha wrote: > On Sat, Dec 23, 2023 at 12:43???PM Ani Sinha <anisinha@xxxxxxxxxx> wrote: > > > > On Fri, Oct 13, 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: > > > > 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)
