Merge pull request #1004 from shiftstack/alladdresses

✨Return all NodeAddresses in OpenStackMachine.Status.Addresses

Author: k8s-ci-robot

controllers/openstackcluster_controller.go

@@ -27,6 +27,7 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
 	"github.com/gophercloud/utils/openstack/clientconfig"
 	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
@@ -206,16 +207,14 @@ func deleteBastion(log logr.Logger, osProviderClient *gophercloud.ProviderClient
 		if err != nil {
 			return err
 		}
+		addresses := instanceNS.Addresses()
 
-		floatingIP := instanceNS.FloatingIP()
-		if floatingIP != "" {
-			if err = networkingService.DisassociateFloatingIP(openStackCluster, floatingIP); err != nil {
-				handleUpdateOSCError(openStackCluster, errors.Errorf("failed to disassociate floating IP: %v", err))
-				return errors.Errorf("failed to disassociate floating IP: %v", err)
-			}
-			if err = networkingService.DeleteFloatingIP(openStackCluster, floatingIP); err != nil {
-				handleUpdateOSCError(openStackCluster, errors.Errorf("failed to delete floating IP: %v", err))
-				return errors.Errorf("failed to delete floating IP: %v", err)
+		for _, address := range addresses {
+			if address.Type == corev1.NodeExternalIP {
+				if err = networkingService.DeleteFloatingIP(openStackCluster, address.Address); err != nil {
+					handleUpdateOSCError(openStackCluster, errors.Errorf("failed to delete floating IP: %v", err))
+					return errors.Errorf("failed to delete floating IP: %v", err)
+				}
 			}
 		}
 
@@ -319,7 +318,7 @@ func reconcileBastion(log logr.Logger, osProviderClient *gophercloud.ProviderCli
 		return err
 	}
 	if instanceStatus != nil {
-		bastion, err := instanceStatus.APIInstance()
+		bastion, err := instanceStatus.APIInstance(openStackCluster)
 		if err != nil {
 			return err
 		}
@@ -343,7 +342,7 @@ func reconcileBastion(log logr.Logger, osProviderClient *gophercloud.ProviderCli
 		handleUpdateOSCError(openStackCluster, errors.Errorf("failed to get or create floating IP for bastion: %v", err))
 		return errors.Errorf("failed to get or create floating IP for bastion: %v", err)
 	}
-	port, err := computeService.GetManagementPort(instanceStatus)
+	port, err := computeService.GetManagementPort(openStackCluster, instanceStatus)
 	if err != nil {
 		err = errors.Errorf("getting management port for bastion: %v", err)
 		handleUpdateOSCError(openStackCluster, err)
@@ -355,7 +354,7 @@ func reconcileBastion(log logr.Logger, osProviderClient *gophercloud.ProviderCli
 		return errors.Errorf("failed to associate floating IP with bastion: %v", err)
 	}
 
-	bastion, err := instanceStatus.APIInstance()
+	bastion, err := instanceStatus.APIInstance(openStackCluster)
 	if err != nil {
 		return err
 	}

controllers/openstackmachine_controller.go

@@ -224,28 +224,27 @@ func (r *OpenStackMachineReconciler) reconcileDelete(ctx context.Context, logger
 
 	if instanceStatus == nil {
 		logger.Info("Skipped deleting machine that is already deleted")
-		controllerutil.RemoveFinalizer(openStackMachine, infrav1.MachineFinalizer)
-		if err := patchHelper.Patch(ctx, openStackMachine); err != nil {
-			return ctrl.Result{}, err
-		}
-		return ctrl.Result{}, nil
-	}
-
-	instanceNS, err := instanceStatus.NetworkStatus()
-	if err != nil {
-		handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error getting network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
-		return ctrl.Result{}, nil
-	}
+	} else {
+		if !openStackCluster.Spec.ManagedAPIServerLoadBalancer && util.IsControlPlaneMachine(machine) && openStackCluster.Spec.APIServerFloatingIP == "" {
+			instanceNS, err := instanceStatus.NetworkStatus()
+			if err != nil {
+				handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error getting network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
+				return ctrl.Result{}, nil
+			}
 
-	if err = computeService.DeleteInstance(openStackMachine, instanceStatus); err != nil {
-		handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error deleting OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
-		return ctrl.Result{}, nil
-	}
+			addresses := instanceNS.Addresses()
+			for _, address := range addresses {
+				if address.Type == corev1.NodeExternalIP {
+					if err = networkingService.DeleteFloatingIP(openStackCluster, address.Address); err != nil {
+						handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error deleting Openstack floating IP: %v", err))
+						return ctrl.Result{}, nil
+					}
+				}
+			}
+		}
 
-	floatingIP := instanceNS.FloatingIP()
-	if !openStackCluster.Spec.ManagedAPIServerLoadBalancer && util.IsControlPlaneMachine(machine) && openStackCluster.Spec.APIServerFloatingIP == "" && floatingIP != "" {
-		if err = networkingService.DeleteFloatingIP(openStackCluster, floatingIP); err != nil {
-			handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error deleting Openstack floating IP: %v", err))
+		if err := computeService.DeleteInstance(openStackMachine, instanceStatus); err != nil {
+			handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error deleting OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
 			return ctrl.Result{}, nil
 		}
 	}
@@ -331,10 +330,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, logger
 		return ctrl.Result{}, nil
 	}
 
-	addresses := []corev1.NodeAddress{{Type: corev1.NodeInternalIP, Address: instanceNS.IP()}}
-	if instanceNS.FloatingIP() != "" {
-		addresses = append(addresses, []corev1.NodeAddress{{Type: corev1.NodeExternalIP, Address: instanceNS.FloatingIP()}}...)
-	}
+	addresses := instanceNS.Addresses()
 	openStackMachine.Status.Addresses = addresses
 
 	// TODO(sbueringer) From CAPA: TODO(vincepri): Remove this annotation when clusterctl is no longer relevant.
@@ -370,7 +366,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, logger
 			handleUpdateMachineError(logger, openStackMachine, errors.Errorf("Floating IP cannot be got or created: %v", err))
 			return ctrl.Result{}, nil
 		}
-		port, err := computeService.GetManagementPort(instanceStatus)
+		port, err := computeService.GetManagementPort(openStackCluster, instanceStatus)
 		if err != nil {
 			err = errors.Errorf("getting management port for control plane machine %s: %v", machine.Name, err)
 			handleUpdateMachineError(logger, openStackMachine, err)
@@ -413,7 +409,7 @@ func handleUpdateMachineError(logger logr.Logger, openstackMachine *infrav1.Open
 }
 
 func (r *OpenStackMachineReconciler) reconcileLoadBalancerMember(logger logr.Logger, osProviderClient *gophercloud.ProviderClient, clientOpts *clientconfig.ClientOpts, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, instanceNS *compute.InstanceNetworkStatus, clusterName string) error {
-	ip := instanceNS.IP()
+	ip := instanceNS.IP(openStackCluster.Status.Network.Name)
 	loadbalancerService, err := loadbalancer.NewService(osProviderClient, clientOpts, logger)
 	if err != nil {
 		return err

pkg/cloud/services/compute/instance.go

@@ -417,12 +417,12 @@ func (s *Service) getImageID(imageName string) (string, error) {
 
 // GetManagementPort returns the port which is used for management and external
 // traffic. Cluster floating IPs must be associated with this port.
-func (s *Service) GetManagementPort(instanceStatus *InstanceStatus) (*ports.Port, error) {
+func (s *Service) GetManagementPort(openStackCluster *infrav1.OpenStackCluster, instanceStatus *InstanceStatus) (*ports.Port, error) {
 	ns, err := instanceStatus.NetworkStatus()
 	if err != nil {
 		return nil, err
 	}
-	allPorts, err := s.networkingService.GetPortFromInstanceIP(instanceStatus.ID(), ns.IP())
+	allPorts, err := s.networkingService.GetPortFromInstanceIP(instanceStatus.ID(), ns.IP(openStackCluster.Status.Network.Name))
 	if err != nil {
 		return nil, fmt.Errorf("lookup management port for server %s: %w", instanceStatus.ID(), err)
 	}
@@ -551,7 +551,7 @@ func (s *Service) GetInstanceStatus(resourceID string) (instance *InstanceStatus
 		return nil, fmt.Errorf("get server %q detail failed: %v", resourceID, err)
 	}
 
-	return &InstanceStatus{server: &server}, nil
+	return &InstanceStatus{&server, s.logger}, nil
 }
 
 func (s *Service) GetInstanceStatusByName(eventObject runtime.Object, name string) (instance *InstanceStatus, err error) {
@@ -584,7 +584,7 @@ func (s *Service) GetInstanceStatusByName(eventObject runtime.Object, name strin
 
 	// Return the first returned server, if any
 	for i := range serverList {
-		return &InstanceStatus{server: &serverList[i]}, nil
+		return &InstanceStatus{&serverList[i], s.logger}, nil
 	}
 	return nil, nil
 }

pkg/cloud/services/compute/instance_types.go

@@ -19,9 +19,12 @@ package compute
 import (
 	"encoding/json"
 	"fmt"
+	"sort"
 
+	"github.com/go-logr/logr"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/availabilityzones"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
+	corev1 "k8s.io/api/core/v1"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha4"
 )
@@ -63,10 +66,11 @@ type InstanceIdentifier struct {
 // InstanceStatus represents instance data which has been returned by OpenStack.
 type InstanceStatus struct {
 	server *ServerExt
+	logger logr.Logger
 }
 
-func NewInstanceStatusFromServer(server *ServerExt) *InstanceStatus {
-	return &InstanceStatus{server}
+func NewInstanceStatusFromServer(server *ServerExt, logger logr.Logger) *InstanceStatus {
+	return &InstanceStatus{server, logger}
 }
 
 type networkInterface struct {
@@ -79,7 +83,7 @@ type networkInterface struct {
 // as used by CAPO. Therefore it may use more context than just data which was
 // returned by OpenStack.
 type InstanceNetworkStatus struct {
-	addresses map[string][]networkInterface
+	addresses map[string][]corev1.NodeAddress
 }
 
 func (is *InstanceStatus) ID() string {
@@ -103,7 +107,7 @@ func (is *InstanceStatus) AvailabilityZone() string {
 }
 
 // APIInstance returns an infrav1.Instance object for use by the API.
-func (is *InstanceStatus) APIInstance() (*infrav1.Instance, error) {
+func (is *InstanceStatus) APIInstance(openStackCluster *infrav1.OpenStackCluster) (*infrav1.Instance, error) {
 	i := infrav1.Instance{
 		ID:         is.ID(),
 		Name:       is.Name(),
@@ -116,8 +120,9 @@ func (is *InstanceStatus) APIInstance() (*infrav1.Instance, error) {
 		return nil, err
 	}
 
-	i.IP = ns.IP()
-	i.FloatingIP = ns.FloatingIP()
+	clusterNetwork := openStackCluster.Status.Network.Name
+	i.IP = ns.IP(clusterNetwork)
+	i.FloatingIP = ns.FloatingIP(clusterNetwork)
 
 	return &i, nil
 }
@@ -132,51 +137,102 @@ func (is *InstanceStatus) InstanceIdentifier() *InstanceIdentifier {
 
 // NetworkStatus returns an InstanceNetworkStatus object for an InstanceStatus.
 func (is *InstanceStatus) NetworkStatus() (*InstanceNetworkStatus, error) {
-	addresses := make(map[string][]networkInterface)
-
+	// Gophercloud doesn't give us a struct for server addresses: we get a
+	// map of networkname -> interface{}. That interface{} is a list of
+	// addresses as in the example output here:
+	// https://docs.openstack.org/api-ref/compute/?expanded=show-server-details-detail#show-server-details
+	//
+	// Here we convert the interface{} into something more usable by
+	// marshalling it to json, then unmarshalling it back into our own
+	// struct.
+	addressesByNetwork := make(map[string][]corev1.NodeAddress)
 	for networkName, b := range is.server.Addresses {
 		list, err := json.Marshal(b)
 		if err != nil {
 			return nil, fmt.Errorf("error marshalling addresses for instance %s: %w", is.ID(), err)
 		}
-		var networkList []networkInterface
-		err = json.Unmarshal(list, &networkList)
+		var interfaceList []networkInterface
+		err = json.Unmarshal(list, &interfaceList)
 		if err != nil {
 			return nil, fmt.Errorf("error unmarshalling addresses for instance %s: %w", is.ID(), err)
 		}
 
-		addresses[networkName] = networkList
-	}
+		var addresses []corev1.NodeAddress
+		for i := range interfaceList {
+			address := &interfaceList[i]
 
-	return &InstanceNetworkStatus{addresses}, nil
-}
+			// Only consider IPv4
+			if address.Version != 4 {
+				is.logger.V(6).Info("Ignoring IPv%d address %s: only IPv4 is supported", address.Version, address.Address)
+				continue
+			}
 
-func (ns *InstanceNetworkStatus) IP() string {
-	// Return the last listed non-floating IPv4 from the last listed network
-	// This behaviour is wrong, but consistent with the previous behaviour
-	// https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/4debc1fc4742e483302b0c36b16c076977bd165d/pkg/cloud/services/compute/instance.go#L973-L998
-	// XXX: Fix this
-	for _, vifs := range ns.addresses {
-		for _, vif := range vifs {
-			if vif.Version == 4.0 && vif.Type != "floating" {
-				return vif.Address
+			var addressType corev1.NodeAddressType
+			switch address.Type {
+			case "floating":
+				addressType = corev1.NodeExternalIP
+			case "fixed":
+				addressType = corev1.NodeInternalIP
+			default:
+				is.logger.V(6).Info("Ignoring address %s with unknown type '%s'", address.Address, address.Type)
+				continue
 			}
+
+			addresses = append(addresses, corev1.NodeAddress{
+				Type:    addressType,
+				Address: address.Address,
+			})
 		}
+
+		addressesByNetwork[networkName] = addresses
 	}
-	return ""
+
+	return &InstanceNetworkStatus{addressesByNetwork}, nil
+}
+
+// Addresses returns a list of NodeAddresses containing all addresses which will
+// be reported on the OpenStackMachine object.
+func (ns *InstanceNetworkStatus) Addresses() []corev1.NodeAddress {
+	// We want the returned order of addresses to be deterministic to make
+	// it easy to detect changes and avoid unnecessary updates. Iteration
+	// over maps is non-deterministic, so we explicitly iterate over the
+	// address map in lexical order of network names. This order is
+	// arbitrary.
+	// Pull out addresses map keys (network names) and sort them lexically
+	networks := make([]string, 0, len(ns.addresses))
+	for network := range ns.addresses {
+		networks = append(networks, network)
+	}
+	sort.Strings(networks)
+
+	var addresses []corev1.NodeAddress
+	for _, network := range networks {
+		addressList := ns.addresses[network]
+		addresses = append(addresses, addressList...)
+	}
+
+	return addresses
 }
 
-func (ns *InstanceNetworkStatus) FloatingIP() string {
-	// Return the last listed floating IPv4 from the last listed network
-	// This behaviour is wrong, but consistent with the previous behaviour
-	// https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/4debc1fc4742e483302b0c36b16c076977bd165d/pkg/cloud/services/compute/instance.go#L973-L998
-	// XXX: Fix this
-	for _, vifs := range ns.addresses {
-		for _, vif := range vifs {
-			if vif.Version == 4.0 && vif.Type == "floating" {
-				return vif.Address
+func (ns *InstanceNetworkStatus) firstAddressByNetworkAndType(networkName string, addressType corev1.NodeAddressType) string {
+	if addressList, ok := ns.addresses[networkName]; ok {
+		for i := range addressList {
+			address := &addressList[i]
+			if address.Type == addressType {
+				return address.Address
 			}
 		}
 	}
 	return ""
 }
+
+// IP returns the first listed ip of an instance for the given network name.
+func (ns *InstanceNetworkStatus) IP(networkName string) string {
+	return ns.firstAddressByNetworkAndType(networkName, corev1.NodeInternalIP)
+}
+
+// FloatingIP returns the first listed floating ip of an instance for the given
+// network name.
+func (ns *InstanceNetworkStatus) FloatingIP(networkName string) string {
+	return ns.firstAddressByNetworkAndType(networkName, corev1.NodeExternalIP)
+}