Ensure that pods are replaced if we change the Pod IP family (#2247)

* Ensure that pods are replaced if we change the Pod IP family

Author: johscheuer

api/v1beta2/foundationdb_labels.go

@@ -40,6 +40,9 @@ const (
 	// gets its public IP from.
 	PublicIPSourceAnnotation = "foundationdb.org/public-ip-source"
 
+	// IPFamilyAnnotation is an annotation key that specifies the IP family of the pod.
+	IPFamilyAnnotation = "foundationdb.org/ip-family"
+
 	// PublicIPAnnotation is an annotation key that specifies the current public
 	// IP for a pod.
 	PublicIPAnnotation = "foundationdb.org/public-ip"

api/v1beta2/foundationdbcluster_types.go

@@ -2138,8 +2138,7 @@ type RoutingConfig struct {
 
 	// PodIPFamily tells the pod which family of IP addresses to use.
 	// You can use 4 to represent IPv4, and 6 to represent IPv6.
-	// This feature is only supported in FDB 7.0 or later, and requires
-	// dual-stack support in your Kubernetes environment.
+	// This feature requires dual-stack support in your Kubernetes environment.
 	PodIPFamily *int `json:"podIPFamily,omitempty"`
 
 	// UseDNSInClusterFile determines whether to use DNS names rather than IP
@@ -2941,6 +2940,13 @@ func (cluster *FoundationDBCluster) Validate() error {
 		}
 	}
 
+	if cluster.Spec.Routing.PodIPFamily != nil {
+		ipFamily := *cluster.Spec.Routing.PodIPFamily
+		if ipFamily != 4 && ipFamily != 6 {
+			validations = append(validations, fmt.Sprintf("Pod IP Family %d is not valid, only 4 or 6 are allowed.", ipFamily))
+		}
+	}
+
 	if len(validations) == 0 {
 		return nil
 	}
@@ -3086,9 +3092,16 @@ func (cluster *FoundationDBCluster) GetProcessGroupID(processClass ProcessClass,
 	return fmt.Sprintf("%s-%s-%d", cluster.Name, processClass.GetProcessClassForPodName(), idNum), processGroupID
 }
 
+var (
+	// PodIPFamilyIPv4 represents the desired IP family as IPv4.
+	PodIPFamilyIPv4 = 4
+	// PodIPFamilyIPv6 represents the desired IP family as IPv6.
+	PodIPFamilyIPv6 = 6
+)
+
 // IsPodIPFamily6 determines whether the podIPFamily setting in cluster is set to use the IPv6 family.
 func (cluster *FoundationDBCluster) IsPodIPFamily6() bool {
-	return pointer.IntDeref(cluster.Spec.Routing.PodIPFamily, 4) == 6
+	return pointer.IntDeref(cluster.Spec.Routing.PodIPFamily, PodIPFamilyIPv4) == PodIPFamilyIPv6
 }
 
 // ProcessSharesDC returns true if the process's locality matches the cluster's Datacenter.

docs/cluster_spec.md

@@ -468,7 +468,7 @@ RoutingConfig allows configuring routing to our pods, and services that sit in f
 | ----- | ----------- | ------ | -------- |
 | headlessService | Headless determines whether we want to run a headless service for the cluster. | *bool | false |
 | publicIPSource | PublicIPSource specifies what source a process should use to get its public IPs.  This supports the values `pod` and `service`. | *[PublicIPSource](#publicipsource) | false |
-| podIPFamily | PodIPFamily tells the pod which family of IP addresses to use. You can use 4 to represent IPv4, and 6 to represent IPv6. This feature is only supported in FDB 7.0 or later, and requires dual-stack support in your Kubernetes environment. | *int | false |
+| podIPFamily | PodIPFamily tells the pod which family of IP addresses to use. You can use 4 to represent IPv4, and 6 to represent IPv6. This feature requires dual-stack support in your Kubernetes environment. | *int | false |
 | useDNSInClusterFile | UseDNSInClusterFile determines whether to use DNS names rather than IP addresses to identify coordinators in the cluster file. This requires FoundationDB 7.0+. | *bool | false |
 | defineDNSLocalityFields | DefineDNSLocalityFields determines whether to define pod DNS names on pod specs and provide them in the locality arguments to fdbserver.  This is ignored if UseDNSInCluster is true. | *bool | false |
 | dnsDomain | DNSDomain defines the cluster domain used in a DNS name generated for a service. The default is `cluster.local`. | *string | false |

e2e/test_operator/operator_test.go

@@ -36,6 +36,7 @@ import (
 	"fmt"
 	"log"
 	"math"
+	"strconv"
 	"strings"
 	"time"
 
@@ -2737,4 +2738,44 @@ var _ = Describe("Operator", Label("e2e", "pr"), func() {
 			})
 		})
 	})
+
+	When("the pod IP family is changed", func() {
+		var testStartTime time.Time
+		// TODO (johscheuer): Make the IP family configurable in the future.
+		var podIPFamily = fdbv1beta2.PodIPFamilyIPv4
+
+		BeforeEach(func() {
+			testStartTime = time.Now()
+			spec := fdbCluster.GetCluster().Spec.DeepCopy()
+			spec.Routing.PodIPFamily = pointer.Int(podIPFamily)
+			fdbCluster.UpdateClusterSpecWithSpec(spec)
+			Expect(fdbCluster.WaitForReconciliation()).To(Succeed())
+		})
+
+		AfterEach(func() {
+			spec := fdbCluster.GetCluster().Spec.DeepCopy()
+			spec.Routing.PodIPFamily = nil
+			fdbCluster.UpdateClusterSpecWithSpec(spec)
+			Expect(fdbCluster.WaitForReconciliation(fixtures.SoftReconcileOption(true))).To(Succeed())
+		})
+
+		It("should replace all pods and configure them properly", func() {
+			pods := fdbCluster.GetPods()
+			podIPFamilyString := strconv.Itoa(podIPFamily)
+
+			for _, pod := range pods.Items {
+				if !pod.DeletionTimestamp.IsZero() {
+					continue
+				}
+
+				if pod.Status.Phase != corev1.PodRunning {
+					log.Println("ignoring pod:", pod.Name, "with pod phase", pod.Status.Phase, "message:", pod.Status.Message)
+					continue
+				}
+
+				Expect(pod.CreationTimestamp.After(testStartTime)).To(BeTrue())
+				Expect(pod.Annotations).To(HaveKeyWithValue(fdbv1beta2.IPFamilyAnnotation, podIPFamilyString))
+			}
+		})
+	})
 })

internal/pod_helper.go

@@ -25,72 +25,57 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
+	monitorapi "github.com/apple/foundationdb/fdbkubernetesmonitor/api"
 	"net"
 	"strconv"
 
-	"github.com/go-logr/logr"
-
-	"k8s.io/utils/pointer"
-
 	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/v2/api/v1beta2"
+	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // GetPublicIPsForPod returns the public IPs for a Pod
 func GetPublicIPsForPod(pod *corev1.Pod, log logr.Logger) []string {
-	var podIPFamily *int
-
 	if pod == nil {
 		return []string{}
 	}
 
-	for _, container := range pod.Spec.Containers {
-		if container.Name != fdbv1beta2.SidecarContainerName {
-			continue
-		}
-		for indexOfArgument, argument := range container.Args {
-			if argument == "--public-ip-family" && indexOfArgument < len(container.Args)-1 {
-				familyString := container.Args[indexOfArgument+1]
-				family, err := strconv.Atoi(familyString)
-				if err != nil {
-					log.Error(err, "Error parsing public IP family", "family", familyString)
-					return nil
-				}
-				podIPFamily = &family
-				break
-			}
-		}
+	podIPFamily, err := GetIPFamily(pod)
+	if err != nil {
+		log.Error(err, "Could not parse IP family")
+		return []string{pod.Status.PodIP}
 	}
 
-	if podIPFamily != nil {
-		podIPs := pod.Status.PodIPs
-		matchingIPs := make([]string, 0, len(podIPs))
+	if podIPFamily == nil {
+		return []string{pod.Status.PodIP}
+	}
 
-		for _, podIP := range podIPs {
-			ip := net.ParseIP(podIP.IP)
-			if ip == nil {
-				log.Error(nil, "Failed to parse IP from pod", "ip", podIP)
-				continue
-			}
-			matches := false
-			switch *podIPFamily {
-			case 4:
-				matches = ip.To4() != nil
-			case 6:
-				matches = ip.To4() == nil
-			default:
-				log.Error(nil, "Could not match IP address against IP family", "family", *podIPFamily)
-			}
-			if matches {
-				matchingIPs = append(matchingIPs, podIP.IP)
-			}
+	podIPs := pod.Status.PodIPs
+	matchingIPs := make([]string, 0, len(podIPs))
+	for _, podIP := range podIPs {
+		ip := net.ParseIP(podIP.IP)
+		if ip == nil {
+			log.Error(nil, "Failed to parse IP from pod", "ip", podIP)
+			continue
+		}
+		matches := false
+		switch *podIPFamily {
+		case fdbv1beta2.PodIPFamilyIPv4:
+			matches = ip.To4() != nil
+		case fdbv1beta2.PodIPFamilyIPv6:
+			matches = ip.To4() == nil
+		default:
+			log.Error(nil, "Could not match IP address against IP family", "family", *podIPFamily)
+		}
+		if matches {
+			matchingIPs = append(matchingIPs, podIP.IP)
 		}
-		return matchingIPs
 	}
 
-	return []string{pod.Status.PodIP}
+	return matchingIPs
 }
 
 // GetProcessGroupIDFromMeta fetches the process group ID from an object's metadata.
@@ -237,6 +222,82 @@ func GetPublicIPSource(pod *corev1.Pod) (fdbv1beta2.PublicIPSource, error) {
 	return fdbv1beta2.PublicIPSource(source), nil
 }
 
+// getIPFamilyFromPod returns the IP family from the pod configuration.
+func getIPFamilyFromPod(pod *corev1.Pod) (*int, error) {
+	if GetImageType(pod) == fdbv1beta2.ImageTypeUnified {
+		currentData, present := pod.Annotations[monitorapi.CurrentConfigurationAnnotation]
+		if !present {
+			return nil, fmt.Errorf("could not read current launcher configuration")
+		}
+
+		currentConfiguration := monitorapi.ProcessConfiguration{}
+		err := json.Unmarshal([]byte(currentData), &currentConfiguration)
+		if err != nil {
+			return nil, fmt.Errorf("could not parse current process configuration: %w", err)
+		}
+
+		for _, argument := range currentConfiguration.Arguments {
+			if argument.ArgumentType != monitorapi.IPListArgumentType {
+				continue
+			}
+
+			return validateIPFamily(argument.IPFamily)
+		}
+
+		// No IP List setting is defined.
+		return nil, nil
+	}
+
+	for _, container := range pod.Spec.Containers {
+		if container.Name != fdbv1beta2.SidecarContainerName {
+			continue
+		}
+
+		for indexOfArgument, argument := range container.Args {
+			if argument == "--public-ip-family" && indexOfArgument < len(container.Args)-1 {
+				return parseIPFamily(container.Args[indexOfArgument+1])
+			}
+		}
+	}
+
+	return nil, nil
+}
+
+// validateIPFamily will validate that the IP family is valid and return a pointer.
+func validateIPFamily(ipFamily int) (*int, error) {
+	if ipFamily != fdbv1beta2.PodIPFamilyIPv4 && ipFamily != fdbv1beta2.PodIPFamilyIPv6 {
+		return nil, fmt.Errorf("unsupported IP family %d", ipFamily)
+	}
+
+	return pointer.Int(ipFamily), nil
+}
+
+// parseIPFamily will convert a string into the IP family (int pointer) and validate that the value is a valid IP family.
+func parseIPFamily(ipFamilyValue string) (*int, error) {
+	ipFamily, err := strconv.Atoi(ipFamilyValue)
+	if err != nil {
+		return nil, err
+	}
+
+	return validateIPFamily(ipFamily)
+}
+
+// GetIPFamily determines the IP family based on the annotation. If the annotation is not present the method will try to
+// get the ip family based on the pod spec.
+func GetIPFamily(pod *corev1.Pod) (*int, error) {
+	if pod == nil {
+		return nil, fmt.Errorf("failed to fetch IP family from nil Pod")
+	}
+
+	ipFamilyValue, ok := pod.ObjectMeta.Annotations[fdbv1beta2.IPFamilyAnnotation]
+	if ipFamilyValue != "" && ok {
+		return parseIPFamily(ipFamilyValue)
+	}
+
+	// If the annotation is missing, try to get the pod IP family from the pod spec.
+	return getIPFamilyFromPod(pod)
+}
+
 // PodHasSidecarTLS determines whether a pod currently has TLS enabled for the sidecar process.
 // This method should only be used for split images.
 func PodHasSidecarTLS(pod *corev1.Pod) bool {