Merge pull request #171 from adobley/reserve-network-broadcast-addrs

🐛 Controller does not allocate reserved addresses

Author: k8s-ci-robot

api/v1alpha1/conversion.go

@@ -57,3 +57,7 @@ func Convert_v1alpha1_InClusterIPPoolSpec_To_v1alpha2_InClusterIPPoolSpec(in *In
 
 	return nil
 }
+
+func Convert_v1alpha2_InClusterIPPoolSpec_To_v1alpha1_InClusterIPPoolSpec(in *v1alpha2.InClusterIPPoolSpec, out *InClusterIPPoolSpec, s conversion.Scope) error {
+	return autoConvert_v1alpha2_InClusterIPPoolSpec_To_v1alpha1_InClusterIPPoolSpec(in, out, s)
+}

api/v1alpha1/zz_generated.conversion.go

@@ -33,6 +33,14 @@ type InClusterIPPoolSpec struct {
 	// Gateway
 	// +optional
 	Gateway string `json:"gateway,omitempty"`
+
+	// AllocateReservedIPAddresses causes the provider to allocate the network
+	// address (the first address in the inferred subnet) and broadcast address
+	// (the last address in the inferred subnet) when IPv4. The provider will
+	// allocate the anycast address address (the first address in the inferred
+	// subnet) when IPv6.
+	// +optional
+	AllocateReservedIPAddresses bool `json:"allocateReservedIPAddresses,omitempty"`
 }
 
 // InClusterIPPoolStatus defines the observed state of InClusterIPPool.

api/v1alpha2/inclusterippool_types.go

@@ -174,6 +174,13 @@ spec:
                 items:
                   type: string
                 type: array
+              allocateReservedIPAddresses:
+                description: AllocateReservedIPAddresses causes the provider to allocate
+                  the network address (the first address in the inferred subnet) and
+                  broadcast address (the last address in the inferred subnet) when
+                  IPv4. The provider will allocate the anycast address address (the
+                  first address in the inferred subnet) when IPv6.
+                type: boolean
               gateway:
                 description: Gateway
                 type: string

config/crd/bases/ipam.cluster.x-k8s.io_globalinclusterippools.yaml

@@ -174,6 +174,13 @@ spec:
                 items:
                   type: string
                 type: array
+              allocateReservedIPAddresses:
+                description: AllocateReservedIPAddresses causes the provider to allocate
+                  the network address (the first address in the inferred subnet) and
+                  broadcast address (the last address in the inferred subnet) when
+                  IPv4. The provider will allocate the anycast address address (the
+                  first address in the inferred subnet) when IPv6.
+                type: boolean
               gateway:
                 description: Gateway
                 type: string

config/crd/bases/ipam.cluster.x-k8s.io_inclusterippools.yaml

@@ -197,7 +197,7 @@ func genericReconcile(ctx context.Context, c client.Client, pool pooltypes.Gener
 		return ctrl.Result{}, nil
 	}
 
-	poolIPSet, err := poolutil.AddressesToIPSet(pool.PoolSpec().Addresses)
+	poolIPSet, err := poolutil.PoolSpecToIPSet(pool.PoolSpec())
 	if err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to build ip set from pool spec")
 	}

internal/controllers/inclusterippool.go

@@ -56,8 +56,8 @@ var _ = Describe("IP Pool Reconciler", func() {
 		})
 
 		DescribeTable("it shows the total, used, free ip addresses in the pool",
-			func(poolType string, addresses []string, gateway string, expectedTotal, expectedUsed, expectedFree int) {
-				genericPool = newPool(poolType, testPool, namespace, gateway, addresses, 24)
+			func(poolType string, prefix int, addresses []string, gateway string, expectedTotal, expectedUsed, expectedFree int) {
+				genericPool = newPool(poolType, testPool, namespace, gateway, addresses, prefix)
 				Expect(k8sClient.Create(context.Background(), genericPool)).To(Succeed())
 
 				Eventually(Object(genericPool)).
@@ -94,34 +94,47 @@ var _ = Describe("IP Pool Reconciler", func() {
 			},
 
 			Entry("When there is 1 claim and no gateway - InClusterIPPool",
-				"InClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "", 11, 1, 10),
+				"InClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "", 11, 1, 10),
 			Entry("When there are 2 claims and no gateway - InClusterIPPool",
-				"InClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "", 11, 2, 9),
+				"InClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "", 11, 2, 9),
 			Entry("When there is 1 claim with gateway in range - InClusterIPPool",
-				"InClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 1, 9),
+				"InClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 1, 9),
 			Entry("When there are 2 claims with gateway in range - InClusterIPPool",
-				"InClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 2, 8),
+				"InClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 2, 8),
 			Entry("When there is 1 claim with gateway outside of range - InClusterIPPool",
-				"InClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.1", 11, 1, 10),
+				"InClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "10.0.0.1", 11, 1, 10),
+			Entry("When the addresses range includes network addr, it is not available - InClusterIPPool",
+				"InClusterIPPool", 24, []string{"10.0.0.0-10.0.0.1"}, "10.0.0.2", 1, 1, 0),
+			Entry("When the addresses range includes broadcast, it is not available - InClusterIPPool",
+				"InClusterIPPool", 24, []string{"10.0.0.254-10.0.0.255"}, "10.0.0.1", 1, 1, 0),
+			Entry("When the addresses range is IPv6 and the last range in the subnet, it is available - InClusterIPPool",
+				"InClusterIPPool", 120, []string{"fe80::ffff"}, "fe80::a", 1, 1, 0),
 
 			Entry("When there is 1 claim and no gateway - GlobalInClusterIPPool",
-				"GlobalInClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "", 11, 1, 10),
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "", 11, 1, 10),
 			Entry("When there are 2 claims and no gateway - GlobalInClusterIPPool",
-				"GlobalInClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "", 11, 2, 9),
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "", 11, 2, 9),
 			Entry("When there is 1 claim with gateway in range - GlobalInClusterIPPool",
-				"GlobalInClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 1, 9),
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 1, 9),
 			Entry("When there are 2 claims with gateway in range - GlobalInClusterIPPool",
-				"GlobalInClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 2, 8),
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "10.0.0.10", 10, 2, 8),
 			Entry("When there is 1 claim with gateway outside of range - GlobalInClusterIPPool",
-				"GlobalInClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.1", 11, 1, 10),
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.10-10.0.0.20"}, "10.0.0.1", 11, 1, 10),
+			Entry("When the addresses range includes network addr, it is not available - GlobalInClusterIPPool",
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.0-10.0.0.1"}, "10.0.0.2", 1, 1, 0),
+			Entry("When the addresses range includes broadcast, it is not available - GlobalInClusterIPPool",
+				"GlobalInClusterIPPool", 24, []string{"10.0.0.254-10.0.0.255"}, "10.0.0.1", 1, 1, 0),
+			Entry("When the addresses range is IPv6 and the last range in the subnet, it is available - GlobalInClusterIPPool",
+				"GlobalInClusterIPPool", 120, []string{"fe80::ffff"}, "fe80::a", 1, 1, 0),
 		)
 
 		DescribeTable("it shows the out of range ips if any",
 			func(poolType string, addresses []string, gateway string, updatedAddresses []string, numClaims, expectedOutOfRange int) {
 				poolSpec := v1alpha2.InClusterIPPoolSpec{
-					Prefix:    24,
-					Gateway:   gateway,
-					Addresses: addresses,
+					Prefix:                      24,
+					Gateway:                     gateway,
+					Addresses:                   addresses,
+					AllocateReservedIPAddresses: true,
 				}
 
 				switch poolType {
@@ -164,6 +177,7 @@ var _ = Describe("IP Pool Reconciler", func() {
 					HaveField("Status.Addresses.Used", Equal(numClaims)))
 
 				genericPool.PoolSpec().Addresses = updatedAddresses
+				genericPool.PoolSpec().AllocateReservedIPAddresses = false
 				Expect(k8sClient.Update(context.Background(), genericPool)).To(Succeed())
 
 				Eventually(Object(genericPool)).
@@ -173,8 +187,20 @@ var _ = Describe("IP Pool Reconciler", func() {
 
 			Entry("InClusterIPPool",
 				"InClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.1", []string{"10.0.0.13-10.0.0.20"}, 5, 3),
+			Entry("InClusterIPPool when removing network address",
+				"InClusterIPPool", []string{"10.0.0.0-10.0.0.255"}, "10.0.0.1", []string{"10.0.0.1-10.0.0.255"}, 4, 0),
+			Entry("InClusterIPPool when removing gateway address",
+				"InClusterIPPool", []string{"10.0.0.0-10.0.0.255"}, "10.0.0.1", []string{"10.0.0.0", "10.0.0.2-10.0.0.255"}, 5, 1),
+			Entry("InClusterIPPool when removing broadcast address",
+				"InClusterIPPool", []string{"10.0.0.251-10.0.0.255"}, "10.0.0.1", []string{"10.0.0.251-10.0.0.254"}, 5, 1),
 			Entry("GlobalInClusterIPPool",
 				"GlobalInClusterIPPool", []string{"10.0.0.10-10.0.0.20"}, "10.0.0.1", []string{"10.0.0.13-10.0.0.20"}, 5, 3),
+			Entry("GlobalInClusterIPPool when removing network address",
+				"GlobalInClusterIPPool", []string{"10.0.0.0-10.0.0.255"}, "10.0.0.1", []string{"10.0.0.1-10.0.0.255"}, 4, 0),
+			Entry("GlobalInClusterIPPool when removing gateway address",
+				"GlobalInClusterIPPool", []string{"10.0.0.0-10.0.0.255"}, "10.0.0.1", []string{"10.0.0.0", "10.0.0.2-10.0.0.255"}, 5, 1),
+			Entry("GlobalInClusterIPPool when removing broadcast address",
+				"GlobalInClusterIPPool", []string{"10.0.0.251-10.0.0.255"}, "10.0.0.1", []string{"10.0.0.251-10.0.0.254"}, 5, 1),
 		)
 	})
 

internal/controllers/inclusterippool_test.go

@@ -211,57 +211,60 @@ func (r *IPAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		}
 	}()
 
-	var pool pooltypes.GenericInClusterPool
+	if !controllerutil.ContainsFinalizer(claim, ReleaseAddressFinalizer) {
+		controllerutil.AddFinalizer(claim, ReleaseAddressFinalizer)
+	}
 
-	if claim.Spec.PoolRef.Kind == inClusterIPPoolKind {
-		icippool := &v1alpha2.InClusterIPPool{}
-		if err := r.Client.Get(ctx, types.NamespacedName{Namespace: claim.Namespace, Name: claim.Spec.PoolRef.Name}, icippool); err != nil && !apierrors.IsNotFound(err) {
-			return ctrl.Result{}, errors.Wrap(err, "failed to fetch pool")
-		}
-		pool = icippool
-	} else if claim.Spec.PoolRef.Kind == globalInClusterIPPoolKind {
-		gicippool := &v1alpha2.GlobalInClusterIPPool{}
-		if err := r.Client.Get(ctx, types.NamespacedName{Name: claim.Spec.PoolRef.Name}, gicippool); err != nil && !apierrors.IsNotFound(err) {
-			return ctrl.Result{}, errors.Wrap(err, "failed to fetch pool")
+	pool, err := r.findPool(ctx, claim)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			err := errors.New("pool not found")
+			log.Error(err, "the referenced pool could not be found")
+			if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
+				return r.reconcileDelete(ctx, claim)
+			}
+			return ctrl.Result{}, nil
 		}
-		pool = gicippool
+		return ctrl.Result{}, errors.Wrap(err, "failed to fetch pool")
 	}
 
 	if pool != nil && annotations.HasPaused(pool) {
 		log.Info("IPAddressClaim references Pool which is paused, skipping reconciliation.", "IPAddressClaim", claim.GetName(), "Pool", pool.GetName())
 		return ctrl.Result{}, nil
 	}
 
-	address := &ipamv1.IPAddress{}
-	if err := r.Client.Get(ctx, req.NamespacedName, address); err != nil && !apierrors.IsNotFound(err) {
-		return ctrl.Result{}, errors.Wrap(err, "failed to fetch address")
+	if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(ctx, claim)
 	}
 
-	if !controllerutil.ContainsFinalizer(claim, ReleaseAddressFinalizer) {
-		controllerutil.AddFinalizer(claim, ReleaseAddressFinalizer)
-	}
+	return r.reconcileNormal(ctx, claim, pool)
+}
 
-	if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(ctx, claim, address)
+func (r *IPAddressClaimReconciler) findPool(ctx context.Context, claim *ipamv1.IPAddressClaim) (pooltypes.GenericInClusterPool, error) {
+	if claim.Spec.PoolRef.Kind == inClusterIPPoolKind {
+		icippool := &v1alpha2.InClusterIPPool{}
+		if err := r.Client.Get(ctx, types.NamespacedName{Namespace: claim.Namespace, Name: claim.Spec.PoolRef.Name}, icippool); err != nil {
+			return nil, err
+		}
+		return icippool, nil
+	} else if claim.Spec.PoolRef.Kind == globalInClusterIPPoolKind {
+		gicippool := &v1alpha2.GlobalInClusterIPPool{}
+		if err := r.Client.Get(ctx, types.NamespacedName{Name: claim.Spec.PoolRef.Name}, gicippool); err != nil {
+			return nil, err
+		}
+		return gicippool, nil
 	}
+	return nil, fmt.Errorf("unknown pool type: %s", claim.Spec.PoolRef.Kind)
+}
 
-	if pool == nil {
-		err := errors.New("pool not found")
-		log.Error(err, "the referenced pool could not be found")
-		return ctrl.Result{}, nil
-	}
+func (r *IPAddressClaimReconciler) reconcileNormal(ctx context.Context, claim *ipamv1.IPAddressClaim, pool pooltypes.GenericInClusterPool) (ctrl.Result, error) {
+	log := ctrl.LoggerFrom(ctx).WithValues(pool.GetObjectKind().GroupVersionKind().Kind, fmt.Sprintf("%s/%s", pool.GetNamespace(), pool.GetName()))
 
 	addressesInUse, err := poolutil.ListAddressesInUse(ctx, r.Client, pool.GetNamespace(), claim.Spec.PoolRef)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to list addresses: %w", err)
 	}
 
-	return r.reconcile(ctx, claim, pool, addressesInUse)
-}
-
-func (r *IPAddressClaimReconciler) reconcile(ctx context.Context, claim *ipamv1.IPAddressClaim, pool pooltypes.GenericInClusterPool, addressesInUse []ipamv1.IPAddress) (ctrl.Result, error) {
-	log := ctrl.LoggerFrom(ctx).WithValues(pool.GetObjectKind().GroupVersionKind().Kind, fmt.Sprintf("%s/%s", pool.GetNamespace(), pool.GetName()))
-
 	address := poolutil.AddressByNamespacedName(addressesInUse, claim.Namespace, claim.Name)
 	if address == nil {
 		var err error
@@ -298,7 +301,16 @@ func (r *IPAddressClaimReconciler) reconcile(ctx context.Context, claim *ipamv1.
 	return ctrl.Result{}, nil
 }
 
-func (r *IPAddressClaimReconciler) reconcileDelete(ctx context.Context, claim *ipamv1.IPAddressClaim, address *ipamv1.IPAddress) (ctrl.Result, error) {
+func (r *IPAddressClaimReconciler) reconcileDelete(ctx context.Context, claim *ipamv1.IPAddressClaim) (ctrl.Result, error) {
+	address := &ipamv1.IPAddress{}
+	namespacedName := types.NamespacedName{
+		Namespace: claim.Namespace,
+		Name:      claim.Name,
+	}
+	if err := r.Client.Get(ctx, namespacedName, address); err != nil && !apierrors.IsNotFound(err) {
+		return ctrl.Result{}, errors.Wrap(err, "failed to fetch address")
+	}
+
 	if address.Name != "" {
 		var err error
 		if controllerutil.RemoveFinalizer(address, ProtectAddressFinalizer) {
@@ -321,12 +333,12 @@ func (r *IPAddressClaimReconciler) reconcileDelete(ctx context.Context, claim *i
 func (r *IPAddressClaimReconciler) allocateAddress(claim *ipamv1.IPAddressClaim, pool pooltypes.GenericInClusterPool, addressesInUse []ipamv1.IPAddress) (*ipamv1.IPAddress, error) {
 	poolSpec := pool.PoolSpec()
 
-	inUseIPSet, err := poolutil.AddressesToIPSet(buildAddressList(addressesInUse, poolSpec.Gateway))
+	inUseIPSet, err := poolutil.AddressesToIPSet(buildAddressList(addressesInUse))
 	if err != nil {
 		return nil, fmt.Errorf("failed to convert IPAddressList to IPSet: %w", err)
 	}
 
-	poolIPSet, err := poolutil.AddressesToIPSet(pool.PoolSpec().Addresses)
+	poolIPSet, err := poolutil.PoolSpecToIPSet(poolSpec)
 	if err != nil {
 		return nil, fmt.Errorf("failed to convert pool to range: %w", err)
 	}
@@ -367,17 +379,11 @@ func (r *IPAddressClaimReconciler) clusterToIPClaims(a client.Object) []reconcil
 	return requests
 }
 
-func buildAddressList(addressesInUse []ipamv1.IPAddress, gateway string) []string {
-	// Add extra capacity for the case that the pool's gateway is specified
-	addrStrings := make([]string, len(addressesInUse), len(addressesInUse)+1)
+func buildAddressList(addressesInUse []ipamv1.IPAddress) []string {
+	addrStrings := make([]string, len(addressesInUse))
 	for i, address := range addressesInUse {
 		addrStrings[i] = address.Spec.Address
 	}
-
-	if gateway != "" {
-		addrStrings = append(addrStrings, gateway)
-	}
-
 	return addrStrings
 }