From b25ee84c97661a77aea6b442796ca1eda9e6288e Mon Sep 17 00:00:00 2001 From: Mohamed Chiheb Ben Jemaa Date: Wed, 18 Sep 2024 09:48:55 +0200 Subject: [PATCH 1/2] Allow IPPool to have Prefix /0 --- internal/poolutil/pool_test.go | 17 ++++++ internal/webhooks/inclusterippool.go | 4 -- internal/webhooks/inclusterippool_test.go | 68 +++++++++++++++++++---- 3 files changed, 74 insertions(+), 15 deletions(-) diff --git a/internal/poolutil/pool_test.go b/internal/poolutil/pool_test.go index 9e940d0..7b294c6 100644 --- a/internal/poolutil/pool_test.go +++ b/internal/poolutil/pool_test.go @@ -28,6 +28,23 @@ import ( ) var _ = Describe("PoolSpecToIPSet", func() { + It("should allow a pool spec with prefix /0", func() { + spec := &v1alpha2.InClusterIPPoolSpec{ + Gateway: "192.168.0.1", + Prefix: 0, + Addresses: []string{ + "192.168.0.3-192.168.0.10", + }, + } + ipSet, err := PoolSpecToIPSet(spec) + Expect(err).ToNot(HaveOccurred()) + Expect(ipSet.Contains(mustParse("192.168.0.3"))).To(BeTrue()) + Expect(ipSet.Contains(mustParse("192.168.0.3"))).To(BeTrue()) + Expect(ipSet.Contains(mustParse("192.168.0.5"))).To(BeTrue()) + Expect(ipSet.Contains(mustParse("192.168.0.6"))).To(BeTrue()) + Expect(ipSet.Contains(mustParse("192.168.0.7"))).To(BeTrue()) + Expect(IPSetCount(ipSet)).To(Equal(8)) + }) It("converts a pool spec to a set", func() { spec := &v1alpha2.InClusterIPPoolSpec{ Gateway: "192.168.0.1", diff --git a/internal/webhooks/inclusterippool.go b/internal/webhooks/inclusterippool.go index 454846f..d2ff510 100644 --- a/internal/webhooks/inclusterippool.go +++ b/internal/webhooks/inclusterippool.go @@ -185,10 +185,6 @@ func (webhook *InClusterIPPool) validate(_, newPool types.GenericInClusterPool) allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "addresses"), newPool.PoolSpec().Addresses, "addresses is required")) } - if newPool.PoolSpec().Prefix == 0 { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "prefix"), newPool.PoolSpec().Prefix, "a valid prefix is required")) - } - var hasIPv4Addr, hasIPv6Addr bool for _, address := range newPool.PoolSpec().Addresses { ipSet, err := poolutil.AddressToIPSet(address) diff --git a/internal/webhooks/inclusterippool_test.go b/internal/webhooks/inclusterippool_test.go index 993f5c1..49abfe0 100644 --- a/internal/webhooks/inclusterippool_test.go +++ b/internal/webhooks/inclusterippool_test.go @@ -418,17 +418,6 @@ func TestInvalidScenarios(t *testing.T) { }, expectedError: "provided address is not a valid IP, range, nor CIDR", }, - { - testcase: "omitting a prefix should not be allowed", - spec: v1alpha2.InClusterIPPoolSpec{ - Addresses: []string{ - "10.0.0.25", - "10.0.0.26", - }, - Gateway: "10.0.0.1", - }, - expectedError: "a valid prefix is required", - }, { testcase: "specifying an invalid prefix", spec: v1alpha2.InClusterIPPoolSpec{ @@ -648,6 +637,63 @@ func TestInvalidScenarios(t *testing.T) { } } +func TestIPPool_Prefix(t *testing.T) { + g := NewWithT(t) + + scheme := runtime.NewScheme() + g.Expect(ipamv1.AddToScheme(scheme)).To(Succeed()) + + namespacedPool := &v1alpha2.InClusterIPPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pool", + }, + Spec: v1alpha2.InClusterIPPoolSpec{ + Addresses: []string{"10.0.0.10-10.0.0.20"}, + Prefix: 0, + Gateway: "10.0.0.1", + }, + } + + globalPool := &v1alpha2.GlobalInClusterIPPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pool", + }, + Spec: v1alpha2.InClusterIPPoolSpec{ + Addresses: []string{"10.0.0.10-10.0.0.20"}, + Prefix: 0, + Gateway: "10.0.0.1", + }, + } + emptyPrefixPool := &v1alpha2.InClusterIPPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pool", + }, + Spec: v1alpha2.InClusterIPPoolSpec{ + Addresses: []string{"10.0.0.10-10.0.0.20"}, + Gateway: "10.0.0.1", + }, + } + + ips := []client.Object{ + createIP("my-ip", "10.0.0.10", namespacedPool), + createIP("my-ip-2", "10.0.0.10", globalPool), + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ips...). + WithIndex(&ipamv1.IPAddress{}, index.IPAddressPoolRefCombinedField, index.IPAddressByCombinedPoolRef). + Build() + + webhook := InClusterIPPool{ + Client: fakeClient, + } + + g.Expect(webhook.validate(&v1alpha2.InClusterIPPool{}, namespacedPool)).Error().To(BeNil(), "should allow /0 prefix InClusterIPPool") + g.Expect(webhook.validate(&v1alpha2.GlobalInClusterIPPool{}, globalPool)).Error().To(BeNil(), "should allow /0 prefix GlobalInClusterIPPool") + g.Expect(webhook.validate(&v1alpha2.InClusterIPPool{}, emptyPrefixPool)).Error().To(BeNil(), "should allow empty prefix InClusterIPPool") +} + func runInvalidScenarioTests(t *testing.T, tt invalidScenarioTest, pool types.GenericInClusterPool, webhook InClusterIPPool) { t.Helper() t.Run(tt.testcase, func(t *testing.T) { From f99333dcda4fc50c7f2334d574c9baec967b2c2c Mon Sep 17 00:00:00 2001 From: Mohamed Chiheb Ben Jemaa Date: Wed, 18 Sep 2024 10:13:51 +0200 Subject: [PATCH 2/2] Do not allow negative prefix --- api/v1alpha2/inclusterippool_types.go | 1 + ...ipam.cluster.x-k8s.io_globalinclusterippools.yaml | 1 + .../ipam.cluster.x-k8s.io_inclusterippools.yaml | 1 + internal/webhooks/inclusterippool.go | 4 ++++ internal/webhooks/inclusterippool_test.go | 12 ++++++++++++ 5 files changed, 19 insertions(+) diff --git a/api/v1alpha2/inclusterippool_types.go b/api/v1alpha2/inclusterippool_types.go index eb0609c..f87d5da 100644 --- a/api/v1alpha2/inclusterippool_types.go +++ b/api/v1alpha2/inclusterippool_types.go @@ -28,6 +28,7 @@ type InClusterIPPoolSpec struct { // Prefix is the network prefix to use. // +kubebuilder:validation:Maximum=128 + // +kubebuilder:validation:Minimum=0 Prefix int `json:"prefix"` // Gateway diff --git a/config/crd/bases/ipam.cluster.x-k8s.io_globalinclusterippools.yaml b/config/crd/bases/ipam.cluster.x-k8s.io_globalinclusterippools.yaml index 5e8205a..af7e770 100644 --- a/config/crd/bases/ipam.cluster.x-k8s.io_globalinclusterippools.yaml +++ b/config/crd/bases/ipam.cluster.x-k8s.io_globalinclusterippools.yaml @@ -217,6 +217,7 @@ spec: prefix: description: Prefix is the network prefix to use. maximum: 128 + minimum: 0 type: integer required: - addresses diff --git a/config/crd/bases/ipam.cluster.x-k8s.io_inclusterippools.yaml b/config/crd/bases/ipam.cluster.x-k8s.io_inclusterippools.yaml index e4f0c36..cba9991 100644 --- a/config/crd/bases/ipam.cluster.x-k8s.io_inclusterippools.yaml +++ b/config/crd/bases/ipam.cluster.x-k8s.io_inclusterippools.yaml @@ -215,6 +215,7 @@ spec: prefix: description: Prefix is the network prefix to use. maximum: 128 + minimum: 0 type: integer required: - addresses diff --git a/internal/webhooks/inclusterippool.go b/internal/webhooks/inclusterippool.go index d2ff510..e1efb98 100644 --- a/internal/webhooks/inclusterippool.go +++ b/internal/webhooks/inclusterippool.go @@ -185,6 +185,10 @@ func (webhook *InClusterIPPool) validate(_, newPool types.GenericInClusterPool) allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "addresses"), newPool.PoolSpec().Addresses, "addresses is required")) } + if newPool.PoolSpec().Prefix < 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "prefix"), newPool.PoolSpec().Addresses, "a valid prefix is required")) + } + var hasIPv4Addr, hasIPv6Addr bool for _, address := range newPool.PoolSpec().Addresses { ipSet, err := poolutil.AddressToIPSet(address) diff --git a/internal/webhooks/inclusterippool_test.go b/internal/webhooks/inclusterippool_test.go index 49abfe0..78e3cab 100644 --- a/internal/webhooks/inclusterippool_test.go +++ b/internal/webhooks/inclusterippool_test.go @@ -418,6 +418,18 @@ func TestInvalidScenarios(t *testing.T) { }, expectedError: "provided address is not a valid IP, range, nor CIDR", }, + { + testcase: "negative prefix not allowed", + spec: v1alpha2.InClusterIPPoolSpec{ + Addresses: []string{ + "10.0.0.25", + "10.0.0.26", + }, + Prefix: -1, + Gateway: "10.0.0.1", + }, + expectedError: "a valid prefix is required", + }, { testcase: "specifying an invalid prefix", spec: v1alpha2.InClusterIPPoolSpec{