Skip to content

Commit

Permalink
Controller does not allocate reserved addresses
Browse files Browse the repository at this point in the history 
unless `pool.Spec.AllocateReservedIPAddresses` is true.

The validation code continues to accept pools that contain the network
(the first address in the inferred subnet) and broadcast (the
last address in the inferred subnet) addresses, but the controller will
not allocate these addresses. When the pool is configured with IPv6, the
anycast address (the first address in the inferred subnet) will not be
allocated.

This commit also refactors the claim reconciler. Previously, the
controller would continue to reconcile (and ultimately no-op) despite
the pool not being found. The new spec flag,
`AllocateReservedIPAddresses` exposed this oddness, prompting some
refactoring.

Co-authored-by: Aidan Obley <[email protected]>
Co-authored-by: Edwin Xie <[email protected]>
  • Loading branch information
@tylerschultz @flawedmatrix
3 people committed Jul 21, 2023
1 parent 0fbb922 commit 6607a7b
Show file tree
Hide file tree
Showing 13 changed files with 511 additions and 171 deletions.
4 changes: 4 additions & 0 deletions api/v1alpha1/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
16 changes: 6 additions & 10 deletions api/v1alpha1/zz_generated.conversion.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions api/v1alpha2/inclusterippool_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions config/crd/bases/ipam.cluster.x-k8s.io_globalinclusterippools.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions config/crd/bases/ipam.cluster.x-k8s.io_inclusterippools.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/inclusterippool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
56 changes: 41 additions & 15 deletions internal/controllers/inclusterippool_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)).
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)).
Expand All @@ -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),
)
})

Expand Down
90 changes: 48 additions & 42 deletions internal/controllers/ipaddressclaim.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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("Unkown 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
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
}
Expand Down Expand Up @@ -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
}

Expand Down
Loading

0 comments on commit 6607a7b

Please sign in to comment.