Merge pull request #440 from rabi/fix_ip_fr3

[18.0-fr3] Fix ip reassignment for ipset after scale-in

Author: openshift-merge-bot[bot]

apis/network/v1beta1/ipset_webhook.go

@@ -82,7 +82,7 @@ func (r *IPSet) ValidateCreate() (admission.Warnings, error) {
 	basePath := field.NewPath("spec")
 
 	// validate requested networks exist in netcfg
-	allErrs = append(allErrs, valiateIPSetNetwork(r.Spec.Networks, basePath, &netcfg.Spec)...)
+	allErrs = append(allErrs, validateIPSetNetwork(r.Spec.Networks, basePath, &netcfg.Spec)...)
 
 	if len(allErrs) == 0 {
 		return nil, nil
@@ -132,10 +132,10 @@ func (r *IPSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
 		basePath := field.NewPath("spec")
 
 		// validate requested networks exist in
-		allErrs = append(allErrs, valiateIPSetNetwork(r.Spec.Networks, basePath, &netcfg.Spec)...)
+		allErrs = append(allErrs, validateIPSetNetwork(r.Spec.Networks, basePath, &netcfg.Spec)...)
 
 		// validate against the previous object only
-		allErrs = append(allErrs, valiateIPSetChanged(r.Spec.Networks, oldIPSet.Spec.Networks, basePath)...)
+		allErrs = append(allErrs, validateIPSetChanged(r.Spec.Networks, oldIPSet.Spec.Networks, basePath)...)
 	}
 
 	if len(allErrs) == 0 {
@@ -153,14 +153,14 @@ func (r *IPSet) ValidateDelete() (admission.Warnings, error) {
 	return nil, nil
 }
 
-// valiateIPSetNetwork
+// validateIPSetNetwork
 // - networks are uniq in the list
 // - networks and subnets exist in netcfg
 // - FixedIP is a valid IP address
 // - FixedIP has correct IP version of subnet
 // - FixedIP is in the subnet cidr
 // - Route is only specified on a single network per IPFamily
-func valiateIPSetNetwork(
+func validateIPSetNetwork(
 	networks []IPSetNetwork,
 	path *field.Path,
 	netCfgSpec *NetConfigSpec,
@@ -246,12 +246,12 @@ func valiateIPSetNetwork(
 	return allErrs
 }
 
-// valiateIPSetChanged
+// validateIPSetChanged
 // - if a previous requested network is still in the list
 // - if subnet changed within a network
 // - if fixedIP changed
 // - if defaultRoute changed
-func valiateIPSetChanged(
+func validateIPSetChanged(
 	networks []IPSetNetwork,
 	oldNetworks []IPSetNetwork,
 	path *field.Path,

apis/network/v1beta1/ipset_webhook_test.go

@@ -289,7 +289,7 @@ func TestIPSetValiateIPSetNetwork(t *testing.T) {
 			basePath := field.NewPath("spec")
 
 			var err error
-			allErrs := valiateIPSetNetwork(tt.c.Spec.Networks, basePath, &tt.n)
+			allErrs := validateIPSetNetwork(tt.c.Spec.Networks, basePath, &tt.n)
 			if len(allErrs) > 0 {
 				err = apierrors.NewInvalid(GroupVersion.WithKind("NetConfig").GroupKind(), tt.c.Name, allErrs)
 			}
@@ -477,12 +477,12 @@ func TestIPSetUpdateValidation(t *testing.T) {
 
 			var err error
 
-			allErrs := valiateIPSetNetwork(tt.newSpec.Networks, basePath, &tt.n)
+			allErrs := validateIPSetNetwork(tt.newSpec.Networks, basePath, &tt.n)
 			if len(allErrs) > 0 {
 				err = apierrors.NewInvalid(GroupVersion.WithKind("IPSet").GroupKind(), newCfg.Name, allErrs)
 			}
 
-			allErrs = valiateIPSetChanged(tt.newSpec.Networks, tt.oldSpec.Networks, basePath)
+			allErrs = validateIPSetChanged(tt.newSpec.Networks, tt.oldSpec.Networks, basePath)
 			if len(allErrs) > 0 {
 				err = apierrors.NewInvalid(GroupVersion.WithKind("IPSet").GroupKind(), newCfg.Name, allErrs)
 			}

controllers/network/ipset_controller.go

@@ -24,7 +24,6 @@ import (
 	"sort"
 	"strings"
 
-	corev1 "k8s.io/api/core/v1"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -383,13 +382,15 @@ func (r *IPSetReconciler) ensureReservation(
 		Namespace: ipset.Namespace,
 		Name:      ipset.Name,
 	}
-	reservationSpec := networkv1.ReservationSpec{
-		IPSetRef: corev1.ObjectReference{
-			Namespace: ipset.Namespace,
-			Name:      ipset.Name,
-			UID:       ipset.GetUID(),
-		},
-		Reservation: map[string]networkv1.IPAddress{},
+	// Get existing reservation to preserve IP assignments
+	reservation, err := r.getReservation(ctx, ipset)
+	var reservationSpec = reservation.Spec
+
+	if err != nil {
+		if !k8s_errors.IsNotFound(err) {
+			return nil, err
+		}
+		reservationSpec.Reservation = map[string]networkv1.IPAddress{}
 	}
 	reservationLabels := map[string]string{}
 
@@ -425,30 +426,39 @@ func (r *IPSetReconciler) ensureReservation(
 				fmt.Sprintf("%s/%s", ipam.IPAMLabelKey, string(netDef.Name)): string(subnetDef.Name),
 			})
 
-		ipDetails := ipam.AssignIPDetails{
-			IPSet:       ipset.Name,
-			NetName:     string(netDef.Name),
-			SubNet:      subnetDef,
-			Reservelist: reservations,
-		}
+		var ip *networkv1.IPAddress
+
+		// Check if we already have an IP for this network
+		if existingIP, exists := reservationSpec.Reservation[string(netDef.Name)]; exists {
+			// Use existing IP assignment
+			ip = &existingIP
+		} else {
+			// Need to assign a new IP
+			ipDetails := ipam.AssignIPDetails{
+				IPSet:       ipset.Name,
+				NetName:     string(netDef.Name),
+				SubNet:      subnetDef,
+				Reservelist: reservations,
+			}
 
-		if ipsetNet.FixedIP != nil {
-			ipDetails.FixedIP, err = netip.ParseAddr(string(*ipsetNet.FixedIP))
-			if err != nil {
-				return nil, fmt.Errorf("failed parse FixedIP %s", string(*ipsetNet.FixedIP))
+			if ipsetNet.FixedIP != nil {
+				ipDetails.FixedIP, err = netip.ParseAddr(string(*ipsetNet.FixedIP))
+				if err != nil {
+					return nil, fmt.Errorf("failed parse FixedIP %s", string(*ipsetNet.FixedIP))
+				}
+				if !ipDetails.FixedIP.IsValid() {
+					return nil, fmt.Errorf("failed parse FixedIP %s", string(*ipsetNet.FixedIP))
+				}
 			}
-			if !ipDetails.FixedIP.IsValid() {
-				return nil, fmt.Errorf("failed parse FixedIP %s", string(*ipsetNet.FixedIP))
+
+			ip, err = ipDetails.AssignIP()
+			if err != nil {
+				return nil, fmt.Errorf("failed to do ip reservation: %w", err)
 			}
-		}
 
-		ip, err := ipDetails.AssignIP()
-		if err != nil {
-			return nil, fmt.Errorf("failed to do ip reservation: %w", err)
+			// Add the new IP to the reservation
+			reservationSpec.Reservation[string(netDef.Name)] = *ip
 		}
-
-		// add IP to the reservation and IPSet status reservations
-		reservationSpec.Reservation[string(netDef.Name)] = *ip
 		ipsetRes := networkv1.IPSetReservation{
 			Network:        netDef.Name,
 			Subnet:         subnetDef.Name,

pkg/ipam/funcs.go

@@ -16,7 +16,7 @@ type AssignIPDetails struct {
 	FixedIP     netip.Addr
 	// internal use only
 	excluded map[netip.Addr]bool
-	reserved map[netip.Addr]string
+	reserved map[netip.Addr]bool
 }
 
 func (a *AssignIPDetails) buildExcluded() error {
@@ -38,11 +38,11 @@ func (a *AssignIPDetails) buildReserved() error {
 	if a.reserved != nil {
 		return nil
 	}
-	a.reserved = make(map[netip.Addr]string, len(a.Reservelist.Items))
+	a.reserved = make(map[netip.Addr]bool, len(a.Reservelist.Items))
 	for _, r := range a.Reservelist.Items {
 		if res, ok := r.Spec.Reservation[a.NetName]; ok {
 			if ip, err := netip.ParseAddr(res.Address); err == nil {
-				a.reserved[ip] = r.Spec.IPSetRef.Name
+				a.reserved[ip] = true
 			} else {
 				return fmt.Errorf("failed to parse reservation ip %s: %w", res.Address, err)
 			}
@@ -87,8 +87,8 @@ func (a *AssignIPDetails) fixedIPExists() (*networkv1.IPAddress, error) {
 		return nil, fmt.Errorf("FixedIP %s is in ExcludeAddresses", a.FixedIP.String())
 	}
 
-	if reservedIPSet, ok := a.reserved[a.FixedIP]; ok && reservedIPSet != a.IPSet {
-		return nil, fmt.Errorf("%s already reserved for %s", a.FixedIP.String(), reservedIPSet)
+	if _, ok := a.reserved[a.FixedIP]; ok {
+		return nil, fmt.Errorf("%s already reserved", a.FixedIP.String())
 	}
 
 	return &networkv1.IPAddress{
@@ -132,7 +132,7 @@ func (a *AssignIPDetails) iterateForAssignment() (*networkv1.IPAddress, error) {
 			if _, ok := a.excluded[ip]; ok {
 				continue
 			}
-			if reservedIPSet, ok := a.reserved[ip]; ok && reservedIPSet != a.IPSet {
+			if _, ok := a.reserved[ip]; ok {
 				continue
 			}
 

tests/functional/ipset_controller_test.go

@@ -392,6 +392,110 @@ var _ = Describe("IPSet controller", func() {
 		})
 	})
 
+	When("IP assignments are preserved during reconciliation", func() {
+		var netCfgName types.NamespacedName
+		var ipSetAName types.NamespacedName
+		var ipSetBName types.NamespacedName
+
+		BeforeEach(func() {
+			// Create NetConfig with allocation range that allows multiple IPs
+			netCfg := CreateNetConfig(namespace, GetDefaultNetConfigSpec())
+			netCfgName.Name = netCfg.GetName()
+			netCfgName.Namespace = netCfg.GetNamespace()
+
+			Eventually(func(g Gomega) {
+				res := GetNetConfig(netCfgName)
+				g.Expect(res).ToNot(BeNil())
+			}, timeout, interval).Should(Succeed())
+
+			// Create IPSet A - will get first available IP (172.17.0.100)
+			ipsetA := CreateIPSet(namespace, GetDefaultIPSetSpec())
+			ipSetAName = types.NamespacedName{
+				Name:      ipsetA.GetName(),
+				Namespace: namespace,
+			}
+
+			// Create IPSet B - will get second available IP (172.17.0.101)
+			ipsetB := CreateIPSet(namespace, GetDefaultIPSetSpec())
+			ipSetBName = types.NamespacedName{
+				Name:      ipsetB.GetName(),
+				Namespace: namespace,
+			}
+
+			// Wait for both IPSets to be ready
+			th.ExpectCondition(
+				ipSetAName,
+				ConditionGetterFunc(IPSetConditionGetter),
+				condition.ReadyCondition,
+				corev1.ConditionTrue,
+			)
+
+			th.ExpectCondition(
+				ipSetBName,
+				ConditionGetterFunc(IPSetConditionGetter),
+				condition.ReadyCondition,
+				corev1.ConditionTrue,
+			)
+
+			DeferCleanup(func(_ SpecContext) {
+				th.DeleteInstance(ipsetA)
+				th.DeleteInstance(ipsetB)
+				th.DeleteInstance(netCfg)
+			}, NodeTimeout(timeout))
+		})
+
+		It("should preserve existing IP assignments when other IPSets are deleted", func() {
+			// Get the initial IP assignments
+			resA := GetReservationFromNet(ipSetAName, "net-1")
+			resB := GetReservationFromNet(ipSetBName, "net-1")
+
+			// Verify they got different IPs
+			Expect(resA.Address).ToNot(Equal(resB.Address))
+			Expect(resA.Address).ToNot(BeEmpty())
+			Expect(resB.Address).ToNot(BeEmpty())
+
+			originalIPSetA := resA.Address
+
+			// Delete IPSet B - this should free up its IP address
+			ipsetB := GetIPSet(ipSetBName)
+			th.DeleteInstance(ipsetB)
+
+			// Wait for IPSet B to be deleted
+			Eventually(func() error {
+				instance := &networkv1.IPSet{}
+				return k8sClient.Get(ctx, ipSetBName, instance)
+			}, timeout, interval).Should(MatchError(ContainSubstring("not found")))
+
+			// Force reconciliation of IPSet A by updating a field
+			Eventually(func(g Gomega) {
+				instance := GetIPSet(ipSetAName)
+				// Add an annotation to trigger reconciliation
+				if instance.Annotations == nil {
+					instance.Annotations = make(map[string]string)
+				}
+				instance.Annotations["test.force.reconcile"] = "true"
+				g.Expect(k8sClient.Update(ctx, instance)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			// Give the controller time to reconcile
+			Eventually(func(g Gomega) {
+				instance := GetIPSet(ipSetAName)
+				g.Expect(instance.Annotations["test.force.reconcile"]).To(Equal("true"))
+				th.ExpectCondition(
+					ipSetAName,
+					ConditionGetterFunc(IPSetConditionGetter),
+					condition.ReadyCondition,
+					corev1.ConditionTrue,
+				)
+			}, timeout, interval).Should(Succeed())
+
+			// Verify IPSet A still has its original IP (not the freed IP from B)
+			resAAfterReconcile := GetReservationFromNet(ipSetAName, "net-1")
+			Expect(resAAfterReconcile.Address).To(Equal(originalIPSetA))
+			Expect(resAAfterReconcile.Address).ToNot(Equal(resB.Address))
+		})
+	})
+
 	When("an IPSet with Immutable flag gets created", func() {
 		BeforeEach(func() {
 			net1Spec := GetNetSpec(net1, GetSubnet1(subnet1))