Merge pull request kubernetes-sigs#1721 from adobley/ipam-condition-bound-ips-count

✨ ipam display count of address claims bound in condition

Author: k8s-ci-robot

pkg/services/govmomi/service.go

@@ -20,6 +20,7 @@ import (
 	"encoding/base64"
 	"fmt"
 	"net/netip"
+	"strings"
 
 	"github.com/pkg/errors"
 	"github.com/vmware/govmomi/object"
@@ -53,6 +54,18 @@ import (
 // VMService provdes API to interact with the VMs using govmomi.
 type VMService struct{}
 
+// ipamDeviceConfig aids and holds state for the process of parsing IPAM
+// addresses for a given device.
+type ipamDeviceConfig struct {
+	DeviceIndex         int
+	IPAMAddresses       []*ipamv1.IPAddress
+	MACAddress          string
+	NetworkSpecGateway4 string
+	IPAMConfigGateway4  string
+	NetworkSpecGateway6 string
+	IPAMConfigGateway6  string
+}
+
 // ReconcileVM makes sure that the VM is in the desired state by:
 //  1. Creating the VM if it does not exist, then...
 //  2. Updating the VM with the bootstrap data, such as the cloud-init meta and user data, before...
@@ -279,17 +292,8 @@ func (vms *VMService) reconcileNetworkStatus(ctx *virtualMachineContext) error {
 func (vms *VMService) reconcileIPAddressClaims(ctx *context.VMContext) (bool, error) {
 	for devIdx, device := range ctx.VSphereVM.Spec.Network.Devices {
 		for poolRefIdx, poolRef := range device.AddressesFromPools {
-			// check if claim exists
-			ipAddrClaim := &ipamv1.IPAddressClaim{}
 			ipAddrClaimName := IPAddressClaimName(ctx.VSphereVM.Name, devIdx, poolRefIdx)
-			ipAddrClaimKey := apitypes.NamespacedName{
-				Namespace: ctx.VSphereVM.Namespace,
-				Name:      ipAddrClaimName,
-			}
-			var err error
-			if err = ctx.Client.Get(ctx, ipAddrClaimKey, ipAddrClaim); err != nil && !apierrors.IsNotFound(err) {
-				return false, err
-			}
+			_, err := getIPAddrClaim(ctx, ipAddrClaimName)
 			if err == nil {
 				ctx.Logger.V(5).Info("IPAddressClaim found", "name", ipAddrClaimName)
 			}
@@ -335,122 +339,241 @@ func createIPAddressClaim(ctx *context.VMContext, ipAddrClaimName string, poolRe
 // expected to contain a valid IP, Prefix and Gateway.
 func (vms *VMService) reconcileIPAddresses(ctx *virtualMachineContext) (bool, error) {
 	ctx.IPAMState = map[string]infrav1.NetworkDeviceSpec{}
-	for devIdx, device := range ctx.VSphereVM.Spec.Network.Devices {
-		var ipAddrs []string
-		var gateway4 string
-		var gateway6 string
-
-		//TODO: Break this up into smaller functions
-		for poolRefIdx := range device.AddressesFromPools {
-			// check if claim exists
-			ipAddrClaim := &ipamv1.IPAddressClaim{}
-			ipAddrClaimName := IPAddressClaimName(ctx.VSphereVM.Name, devIdx, poolRefIdx)
-			ipAddrClaimKey := apitypes.NamespacedName{
-				Namespace: ctx.VSphereVM.Namespace,
-				Name:      ipAddrClaimName,
+
+	ipamDeviceConfigs, err := buildIPAMDeviceConfigs(ctx)
+	if err != nil {
+		return false, err
+	}
+
+	var errs []error
+	for _, ipamDeviceConfig := range ipamDeviceConfigs {
+		var addressWithPrefixes []netip.Prefix
+		for _, ipamAddress := range ipamDeviceConfig.IPAMAddresses {
+			addressWithPrefix, err := parseAddressWithPrefix(ipamAddress)
+			if err != nil {
+				errs = append(errs, err)
+				continue
+			}
+
+			if slices.Contains(addressWithPrefixes, addressWithPrefix) {
+				errs = append(errs,
+					fmt.Errorf("IPAddress %s/%s is a duplicate of another address: %q",
+						ipamAddress.Namespace,
+						ipamAddress.Name,
+						addressWithPrefix))
+				continue
 			}
-			var err error
-			ctx.Logger.V(5).Info("fetching IPAddressClaim", "name", ipAddrClaimKey.String())
-			if err = ctx.Client.Get(ctx, ipAddrClaimKey, ipAddrClaim); err != nil && !apierrors.IsNotFound(err) {
+
+			gatewayAddr, err := parseGateway(ipamAddress, addressWithPrefix, ipamDeviceConfig)
+			if err != nil {
+				errs = append(errs, err)
+				continue
+			}
+
+			if gatewayAddr.Is4() {
+				ipamDeviceConfig.IPAMConfigGateway4 = ipamAddress.Spec.Gateway
+			} else {
+				ipamDeviceConfig.IPAMConfigGateway6 = ipamAddress.Spec.Gateway
+			}
+
+			addressWithPrefixes = append(addressWithPrefixes, addressWithPrefix)
+		}
+
+		if len(addressWithPrefixes) > 0 {
+			ctx.IPAMState[ipamDeviceConfig.MACAddress] = infrav1.NetworkDeviceSpec{
+				IPAddrs:  prefixesAsStrings(addressWithPrefixes),
+				Gateway4: ipamDeviceConfig.IPAMConfigGateway4,
+				Gateway6: ipamDeviceConfig.IPAMConfigGateway6,
+			}
+		}
+	}
+
+	if len(errs) > 0 {
+		var msgs []string
+		for _, err := range errs {
+			msgs = append(msgs, err.Error())
+		}
+		msg := strings.Join(msgs, "\n")
+		conditions.MarkFalse(ctx.VSphereVM,
+			infrav1.IPAddressClaimedCondition,
+			infrav1.IPAddressInvalidReason,
+			clusterv1.ConditionSeverityError,
+			msg)
+		return false, errors.New(msg)
+	}
+
+	if len(ctx.IPAMState) > 0 {
+		conditions.MarkTrue(ctx.VSphereVM, infrav1.IPAddressClaimedCondition)
+	}
+
+	return true, nil
+}
+
+// prefixesAsStrings converts []netip.Prefix to []string.
+func prefixesAsStrings(prefixes []netip.Prefix) []string {
+	prefixSrings := make([]string, 0, len(prefixes))
+	for _, prefix := range prefixes {
+		prefixSrings = append(prefixSrings, prefix.String())
+	}
+	return prefixSrings
+}
+
+// parseAddressWithPrefix converts a *ipamv1.IPAddress to a string, e.g. '10.0.0.1/24'.
+func parseAddressWithPrefix(ipamAddress *ipamv1.IPAddress) (netip.Prefix, error) {
+	addressWithPrefix := fmt.Sprintf("%s/%d", ipamAddress.Spec.Address, ipamAddress.Spec.Prefix)
+	parsedPrefix, err := netip.ParsePrefix(addressWithPrefix)
+	if err != nil {
+		return netip.Prefix{}, fmt.Errorf("IPAddress %s/%s has invalid ip address: %q",
+			ipamAddress.Namespace,
+			ipamAddress.Name,
+			addressWithPrefix,
+		)
+	}
+
+	return parsedPrefix, nil
+}
+
+// parseGateway parses the gateway address on a ipamv1.IPAddress and ensures it
+// does not conflict with the gateway addresses parsed from other
+// ipamv1.IPAddresses on the current device. Gateway addresses must be the same
+// family as the address on the ipamv1.IPAddress. Gateway addresses of one
+// family must match the other addresses of the same family.
+func parseGateway(ipamAddress *ipamv1.IPAddress, addressWithPrefix netip.Prefix, ipamDeviceConfig ipamDeviceConfig) (netip.Addr, error) {
+	gatewayAddr, err := netip.ParseAddr(ipamAddress.Spec.Gateway)
+	if err != nil {
+		return netip.Addr{}, fmt.Errorf("IPAddress %s/%s has invalid gateway: %q",
+			ipamAddress.Namespace,
+			ipamAddress.Name,
+			ipamAddress.Spec.Gateway,
+		)
+	}
+
+	if addressWithPrefix.Addr().Is4() != gatewayAddr.Is4() {
+		return netip.Addr{}, fmt.Errorf("IPAddress %s/%s has mismatched gateway and address IP families",
+			ipamAddress.Namespace,
+			ipamAddress.Name,
+		)
+	}
+
+	if gatewayAddr.Is4() {
+		if areGatewaysMismatched(ipamDeviceConfig.NetworkSpecGateway4, ipamAddress.Spec.Gateway) {
+			return netip.Addr{}, fmt.Errorf("the IPv4 Gateway for IPAddress %s does not match the Gateway4 already configured on device (index %d)",
+				ipamAddress.Name,
+				ipamDeviceConfig.DeviceIndex,
+			)
+		}
+		if areGatewaysMismatched(ipamDeviceConfig.IPAMConfigGateway4, ipamAddress.Spec.Gateway) {
+			return netip.Addr{}, fmt.Errorf("the IPv4 IPAddresses assigned to the same device (index %d) do not have the same gateway",
+				ipamDeviceConfig.DeviceIndex,
+			)
+		}
+	} else {
+		if areGatewaysMismatched(ipamDeviceConfig.NetworkSpecGateway6, ipamAddress.Spec.Gateway) {
+			return netip.Addr{}, fmt.Errorf("the IPv6 Gateway for IPAddress %s does not match the Gateway6 already configured on device (index %d)",
+				ipamAddress.Name,
+				ipamDeviceConfig.DeviceIndex,
+			)
+		}
+		if areGatewaysMismatched(ipamDeviceConfig.IPAMConfigGateway6, ipamAddress.Spec.Gateway) {
+			return netip.Addr{}, fmt.Errorf("the IPv6 IPAddresses assigned to the same device (index %d) do not have the same gateway",
+				ipamDeviceConfig.DeviceIndex,
+			)
+		}
+	}
+
+	return gatewayAddr, nil
+}
+
+// buildIPAMDeviceConfigs checks that all the IPAddressClaims have been
+// satisfied.
+// If each IPAddressClaim has an associated IPAddress, a slice of
+// ipamDeviceConfig is returned, one for each device with addressesFromPools.
+// If any of the IPAddressClaims do not have an associated IPAddress yet,
+// a false condition is set and an error is returned, effectively stopping the
+// current reconcilliation loop.
+func buildIPAMDeviceConfigs(ctx *virtualMachineContext) ([]ipamDeviceConfig, error) {
+	boundClaims := 0
+	totalClaims := 0
+	ipamDeviceConfigs := []ipamDeviceConfig{}
+	for devIdx, networkSpecDevice := range ctx.VSphereVM.Spec.Network.Devices {
+		ipamDeviceConfig := ipamDeviceConfig{
+			IPAMAddresses:       []*ipamv1.IPAddress{},
+			MACAddress:          networkSpecDevice.MACAddr,
+			NetworkSpecGateway4: networkSpecDevice.Gateway4,
+			NetworkSpecGateway6: networkSpecDevice.Gateway6,
+			DeviceIndex:         devIdx,
+		}
+
+		for poolRefIdx := range networkSpecDevice.AddressesFromPools {
+			totalClaims++
+
+			ipAddrClaimName := IPAddressClaimName(ctx.VSphereVM.Name, ipamDeviceConfig.DeviceIndex, poolRefIdx)
+
+			ipAddrClaim, err := getIPAddrClaim(&ctx.VMContext, ipAddrClaimName)
+			if err != nil {
 				ctx.Logger.Error(err, "error fetching IPAddressClaim", "name", ipAddrClaimName)
-				return false, err
+				if apierrors.IsNotFound(err) {
+					// it would be odd for this to occur, a findorcreate just happened in a previous step
+					continue
+				}
+				return nil, err
 			}
 
+			ctx.Logger.V(5).Info("fetched IPAddressClaim", "name", ipAddrClaimName, "namespace", ctx.VSphereVM.Namespace)
+
 			ipAddrName := ipAddrClaim.Status.AddressRef.Name
-			ctx.Logger.V(5).Info("fetched IPAddressClaim", "name", ipAddrClaimName, "IPAddressClaim.Status.AddressRef.Name", ipAddrName)
 			if ipAddrName == "" {
-				ctx.Logger.V(5).Info("IPAddress name was empty on IPAddressClaim", "name", ipAddrClaimName, "IPAddressClaim.Status.AddressRef.Name", ipAddrName)
-				msg := "Waiting for IPAddressClaim to have an IPAddress bound"
-				markIPAddressClaimedConditionWaitingForClaimAddress(ctx.VSphereVM, msg)
-				return false, errors.New(msg)
+				ctx.Logger.V(5).Info("IPAddress not yet bound to IPAddressClaim", "name", ipAddrClaimName, "namespace", ctx.VSphereVM.Namespace)
+				continue
 			}
 
 			ipAddr := &ipamv1.IPAddress{}
 			ipAddrKey := apitypes.NamespacedName{
 				Namespace: ctx.VSphereVM.Namespace,
 				Name:      ipAddrName,
 			}
-			if err = ctx.Client.Get(ctx, ipAddrKey, ipAddr); err != nil {
-				return false, err
-			}
 
-			toAdd := fmt.Sprintf("%s/%d", ipAddr.Spec.Address, ipAddr.Spec.Prefix)
-			parsedPrefix, err := netip.ParsePrefix(toAdd)
-			if err != nil {
-				msg := fmt.Sprintf("IPAddress %s/%s has invalid ip address: %q",
-					ipAddrKey.Namespace,
-					ipAddrKey.Name,
-					toAdd,
-				)
-				return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
+			if err := ctx.Client.Get(ctx, ipAddrKey, ipAddr); err != nil {
+				// because the ref was set on the claim, it is expected this error should not occur
+				return nil, err
 			}
 
-			if !slices.Contains(ipAddrs, toAdd) {
-				ipAddrs = append(ipAddrs, toAdd)
-
-				gatewayAddr, err := netip.ParseAddr(ipAddr.Spec.Gateway)
-				if err != nil {
-					msg := fmt.Sprintf("IPAddress %s/%s has invalid gateway: %q",
-						ipAddrKey.Namespace,
-						ipAddrKey.Name,
-						ipAddr.Spec.Gateway,
-					)
-					return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
-				}
-
-				if parsedPrefix.Addr().Is4() != gatewayAddr.Is4() {
-					msg := fmt.Sprintf("IPAddress %s/%s has mismatched gateway and address IP families",
-						ipAddrKey.Namespace,
-						ipAddrKey.Name,
-					)
-					return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
-				}
-
-				if gatewayAddr.Is4() {
-					if device.Gateway4 != "" && device.Gateway4 != ipAddr.Spec.Gateway {
-						msg := fmt.Sprintf("The IPv4 Gateway for IPAddress %s does not match the Gateway4 already configured on device (index %d)",
-							ipAddrName,
-							devIdx,
-						)
-						return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
-					}
-					if gateway4 != "" && gateway4 != ipAddr.Spec.Gateway {
-						msg := fmt.Sprintf("The IPv4 IPAddresses assigned to the same device (index %d) do not have the same gateway",
-							devIdx,
-						)
-						return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
-					}
-					gateway4 = ipAddr.Spec.Gateway
-				} else {
-					if device.Gateway6 != "" && device.Gateway6 != ipAddr.Spec.Gateway {
-						msg := fmt.Sprintf("The IPv6 Gateway for IPAddress %s does not match the Gateway6 already configured on device (index %d)",
-							ipAddrName,
-							devIdx,
-						)
-						return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
-					}
-					if gateway6 != "" && gateway6 != ipAddr.Spec.Gateway {
-						msg := fmt.Sprintf("The IPv6 IPAddresses assigned to the same device (index %d) do not have the same gateway",
-							devIdx,
-						)
-						return markIPAddressClaimedConditionInvalidIPWithError(ctx.VSphereVM, msg)
-					}
-					gateway6 = ipAddr.Spec.Gateway
-				}
-			}
-			ctx.IPAMState[device.MACAddr] = infrav1.NetworkDeviceSpec{
-				IPAddrs:  ipAddrs,
-				Gateway4: gateway4,
-				Gateway6: gateway6,
-			}
+			ipamDeviceConfig.IPAMAddresses = append(ipamDeviceConfig.IPAMAddresses, ipAddr)
+			boundClaims++
 		}
+		ipamDeviceConfigs = append(ipamDeviceConfigs, ipamDeviceConfig)
 	}
 
-	if len(ctx.IPAMState) > 0 {
-		conditions.MarkTrue(ctx.VSphereVM, infrav1.IPAddressClaimedCondition)
+	if boundClaims < totalClaims {
+		msg := fmt.Sprintf("Waiting for IPAddressClaim to have an IPAddress bound, %d out of %d bound", boundClaims, totalClaims)
+		markIPAddressClaimedConditionWaitingForClaimAddress(ctx.VSphereVM, msg)
+		return nil, errors.New(msg)
 	}
 
-	return true, nil
+	return ipamDeviceConfigs, nil
+}
+
+// areGatewaysMismatched checks that a gateway for a device is equal to an
+// IPAddresses gateway. We can assume that IPAddresses will always have
+// gateways so we do not need to check for empty string. It is possible to
+// configure a device and not a gateway, we don't want to fail in that case.
+func areGatewaysMismatched(deviceGateway, ipAddressGateway string) bool {
+	return deviceGateway != "" && deviceGateway != ipAddressGateway
+}
+
+// getIPAddrClaim fetches an IPAddressClaim from the api with the given name.
+func getIPAddrClaim(ctx *context.VMContext, ipAddrClaimName string) (*ipamv1.IPAddressClaim, error) {
+	ipAddrClaim := &ipamv1.IPAddressClaim{}
+	ipAddrClaimKey := apitypes.NamespacedName{
+		Namespace: ctx.VSphereVM.Namespace,
+		Name:      ipAddrClaimName,
+	}
+
+	ctx.Logger.V(5).Info("fetching IPAddressClaim", "name", ipAddrClaimKey.String())
+	if err := ctx.Client.Get(ctx, ipAddrClaimKey, ipAddrClaim); err != nil {
+		return nil, err
+	}
+	return ipAddrClaim, nil
 }
 
 func (vms *VMService) reconcileMetadata(ctx *virtualMachineContext) (bool, error) {
@@ -854,15 +977,6 @@ func (vms *VMService) reconcileClusterModuleMembership(ctx *virtualMachineContex
 	return nil
 }
 
-func markIPAddressClaimedConditionInvalidIPWithError(vm *infrav1.VSphereVM, msg string) (bool, error) {
-	conditions.MarkFalse(vm,
-		infrav1.IPAddressClaimedCondition,
-		infrav1.IPAddressInvalidReason,
-		clusterv1.ConditionSeverityError,
-		msg)
-	return false, errors.New(msg)
-}
-
 func markIPAddressClaimedConditionWaitingForClaimAddress(vm *infrav1.VSphereVM, msg string) {
 	conditions.MarkFalse(vm,
 		infrav1.IPAddressClaimedCondition,