Re: [pve-devel] [PATCH ifupdown2 1/1] Correctly handle IPv6 addresses in vxlan

[Andrew via pve-devel <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 11178 bytes --]

From: Andrew <andrew@apalrd.net>
To: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
Cc: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [PATCH ifupdown2 1/1] Correctly handle IPv6 addresses in vxlan
Date: Wed, 09 Oct 2024 16:55:50 +0000
Message-ID: <17214981-4406-4100-AFF6-9F70E12E421B@apalrd.net>

Yes, I read all of the PRs and discussion on ifupdown2 GitHub before implementing this.

Ultimately I disagreed with the solution to use a separate parameter for IPv6, for the following reasons:
- We can only have one local tunnel IP, so having two parameters means we need to check if the other one has been set (since setting both would be invalid)
- There are already other cases in ifupdown2 which do ip.version == 6 including common parameters like address and gateway, and most parameters do take both IPv4 and IPv6 addresses (such as the remoteip field in vxlan), so having one parameter for both families would be consistent with other parameters in the interfaces file which take both families (regardless of the kernel implementation having two fields instead of one in this case)
- ifupdown-ng already solved this problem using a single parameter instead of two, so doing something else would diverge ifupdown2 vs ifupdown-ng syntax which should be the same

I could add additional error checking to ensure that remoteip[] and localip are the same address family if you’d like. Currently that results in a Netlink exception which gets passed back as an error message. The kernel only allows one address family for a vxlan interface.

Thanks,

Andrew

> On Oct 9, 2024, at 11:37, DERUMIER, Alexandre <alexandre.derumier@groupe-cyllene.com> wrote:
> 
> Try to look at ifupdown2 github, their are 2 old pull request about
> this (never merged/ never completed)
> 
> 
> 
> https://github.com/CumulusNetworks/ifupdown2/pull/172
> 
> "
> For this we would need a new attribute vxlan-local-tunnelip6, we don't
> want to reuse the same attribute for ipv6.
> We are using netlink to configure vxlans, so it's important to use a
> different attribute to set the proper netlink attribute (I don't want
> to have things like if IPAddress(value).version == 6:  set
> Link.IFLA_VXLAN_LOCAL
> "
> 
> https://github.com/CumulusNetworks/ifupdown2/pull/182
> 
> 
> so, at minimum, this need to use a different "vxlan-local-tunnelip6"
> attribute for ipv6
> 
> 
> -------- Message initial --------
> De: apalrd <andrew@apalrd.net>
> À: pve-devel@lists.proxmox.com
> Cc: apalrd <andrew@apalrd.net>
> Objet: [PATCH ifupdown2 1/1] Correctly handle IPv6 addresses in vxlan
> Date: 08/10/2024 06:01:09
> 
> ---
>  ifupdown2/addons/vxlan.py | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/ifupdown2/addons/vxlan.py b/ifupdown2/addons/vxlan.py
> index 084aec9..4aa8e50 100644
> --- a/ifupdown2/addons/vxlan.py
> +++ b/ifupdown2/addons/vxlan.py
> @@ -51,7 +51,7 @@ class vxlan(Vxlan, moduleBase):
>              },
>              "vxlan-local-tunnelip": {
>                  "help": "vxlan local tunnel ip",
> -                "validvals": ["<ipv4>"],
> +                "validvals": ["<ipv4>,<ipv6>"],
>                  "example": ["vxlan-local-tunnelip 172.16.20.103"]
>              },
>              "vxlan-svcnodeip": {
> @@ -66,7 +66,7 @@ class vxlan(Vxlan, moduleBase):
>              },
>              "vxlan-remoteip": {
>                  "help": "vxlan remote ip",
> -                "validvals": ["<ipv4>"],
> +                "validvals": ["<ipv4>,<ipv6>"],
>                  "example": ["vxlan-remoteip 172.16.22.127"],
>                  "multiline": True
>              },
> @@ -521,7 +521,7 @@ class vxlan(Vxlan, moduleBase):
>              local = self._vxlan_local_tunnelip
>  
>          if link_exists:
> -            cached_ifla_vxlan_local =
> cached_vxlan_ifla_info_data.get(Link.IFLA_VXLAN_LOCAL)
> +            cached_ifla_vxlan_local =
> cached_vxlan_ifla_info_data.get(Link.IFLA_VXLAN_LOCAL) or
> cached_vxlan_ifla_info_data.get(Link.IFLA_VXLAN_LOCAL6)
>  
>              # on ifreload do not overwrite anycast_ip to individual ip
>              # if clagd has modified
> @@ -547,7 +547,7 @@ class vxlan(Vxlan, moduleBase):
>  
>          if local:
>              try:
> -                local = ipnetwork.IPv4Address(local)
> +                local = ipnetwork.IPAddress(local)
>  
>                  if local.initialized_with_prefixlen:
>                      self.logger.warning("%s: vxlan-local-tunnelip %s:
> netmask ignored" % (ifname, local))
> @@ -559,13 +559,19 @@ class vxlan(Vxlan, moduleBase):
>          if local:
>              if local != cached_ifla_vxlan_local:
>                  self.logger.info("%s: set vxlan-local-tunnelip %s" %
> (ifname, local))
> -                user_request_vxlan_info_data[Link.IFLA_VXLAN_LOCAL] =
> local
> +                if local.version == 6:
> +                   
> user_request_vxlan_info_data[Link.IFLA_VXLAN_LOCAL6] = local
> +                else:
> +                   
> user_request_vxlan_info_data[Link.IFLA_VXLAN_LOCAL] = local
>  
>                  # if both local-ip and anycast-ip are identical the
> function prints a warning
>                  self.syntax_check_localip_anycastip_equal(ifname,
> local, self._clagd_vxlan_anycast_ip)
>          elif cached_ifla_vxlan_local:
>              self.logger.info("%s: removing vxlan-local-tunnelip (cache
> %s)" % (ifname, cached_ifla_vxlan_local))
> -            user_request_vxlan_info_data[Link.IFLA_VXLAN_LOCAL] = None
> +            if cached_ifla_vxlan_local.version == 6:
> +                user_request_vxlan_info_data[Link.IFLA_VXLAN_LOCAL6] =
> None
> +            else:
> +                user_request_vxlan_info_data[Link.IFLA_VXLAN_LOCAL] =
> None
>  
>          return local
>  
> @@ -1236,7 +1242,7 @@ class vxlan(Vxlan, moduleBase):
>          if remoteips:
>              try:
>                  for remoteip in remoteips:
> -                    ipnetwork.IPv4Address(remoteip)
> +                    ipnetwork.IPAddress(remoteip)
>              except Exception as e:
>                  self.log_error('%s: vxlan-remoteip: %s' %
> (ifaceobj.name, str(e)))
>  
> @@ -1244,7 +1250,7 @@ class vxlan(Vxlan, moduleBase):
>          # purge any removed remote ip
>          old_remoteips = self.get_old_remote_ips(ifaceobj.name)
>  
> -        if vxlan_purge_remotes or remoteips or (remoteips !=
> old_remoteips):
> +        if vxlan_purge_remotes or (isinstance(remoteips,list) and
> remoteips != old_remoteips):
>              # figure out the diff for remotes and do the bridge fdb
> updates
>              # only if provisioned by user and not by an vxlan external
>              # controller.
> @@ -1281,8 +1287,8 @@ class vxlan(Vxlan, moduleBase):
>                          "00:00:00:00:00:00",
>                          None, True, addr
>                      )
> -                except Exception:
> -                    pass
> +                except Exception as e:
> +                    self.log_error('%s: vxlan-remoteip<add>: %s' %
> (ifaceobj.name, str(e)))
>  
>          self.vxlan_remote_ip_map(ifaceobj, vxlan_mcast_grp_map)
>  
> 




[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel