Skip to content

Commit c1f177f

Browse files
therealak12therealak12
authored
Merge pull request #3 from snapp-incubator/ipam-ip-equality
allow create/update when ipam ips values are equal
2 parents 815272b + 27951ee commit c1f177f

3 files changed

Lines changed: 193 additions & 47 deletions

File tree

‎config/manager/kustomization.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1
44
kind: Kustomization
55
images:
66
- name: controller
7-
newName: ghcr.io/snapp-incubator/kubernetes-webhooks
8-
newTag: v0.1.3
7+
newName: registry.teh-1.snappcloud.io/library/kubernetes-webhooks
8+
newTag: v0.1.4

‎internal/webhook/v1/service_webhook.go‎

Lines changed: 67 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"net"
77
"net/netip"
8+
"slices"
89
"strings"
910

1011
ciliumv2alpha1 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
@@ -100,54 +101,35 @@ func (v *ServiceCustomValidator) ValidateDelete(ctx context.Context, obj runtime
100101
// the address-pool label is absent.
101102
func (v *ServiceCustomValidator) validateIPSources(ctx context.Context, service *corev1.Service) (admission.Warnings, error) {
102103
annotations := service.GetAnnotations()
103-
ipamOriginalRaw := annotations[lbIpamIpsAnnotation]
104-
ipamAliasRaw := annotations[lbIpamIpsAnnotationAlias]
105-
loadBalancerIP := strings.TrimSpace(service.Spec.LoadBalancerIP)
106-
107-
// Count how many sources are set.
108-
setCount := 0
109-
if ipamOriginalRaw != "" {
110-
setCount++
111-
}
112-
if ipamAliasRaw != "" {
113-
setCount++
104+
if annotations == nil {
105+
annotations = map[string]string{}
114106
}
115-
if loadBalancerIP != "" {
116-
setCount++
107+
sources := []ipSourceRequest{
108+
{name: lbIpamIpsAnnotation, ips: parseRequestedIPs(annotations[lbIpamIpsAnnotation])},
109+
{name: lbIpamIpsAnnotationAlias, ips: parseRequestedIPs(annotations[lbIpamIpsAnnotationAlias])},
110+
{name: "spec.loadBalancerIP", ips: parseRequestedIPs(service.Spec.LoadBalancerIP)},
117111
}
118112

119-
if setCount > 1 {
120-
return nil, fmt.Errorf(
121-
"at most one of %s, %s, and spec.loadBalancerIP may be set",
122-
lbIpamIpsAnnotation, lbIpamIpsAnnotationAlias,
123-
)
113+
var configuredSources []ipSourceRequest
114+
for _, source := range sources {
115+
if len(source.ips) > 0 {
116+
configuredSources = append(configuredSources, source)
117+
}
124118
}
125-
if setCount == 0 {
119+
120+
if len(configuredSources) == 0 {
126121
return nil, nil
127122
}
123+
if err := validateMatchingIPSources(configuredSources); err != nil {
124+
return nil, err
125+
}
128126

129127
cidrs, ranges, poolName, err := v.fetchPoolBlocks(ctx, service)
130128
if err != nil {
131129
return nil, err
132130
}
133131

134-
// Collect the IPs to validate.
135-
var ipsToCheck []string
136-
if loadBalancerIP != "" {
137-
ipsToCheck = []string{loadBalancerIP}
138-
} else {
139-
raw := ipamOriginalRaw
140-
if raw == "" {
141-
raw = ipamAliasRaw
142-
}
143-
for _, s := range strings.Split(raw, ",") {
144-
if t := strings.TrimSpace(s); t != "" {
145-
ipsToCheck = append(ipsToCheck, t)
146-
}
147-
}
148-
}
149-
150-
for _, ip := range ipsToCheck {
132+
for _, ip := range configuredSources[0].ips {
151133
if !isIPInPool(ip, cidrs, ranges) {
152134
return nil, fmt.Errorf(
153135
"IP %s is not within any block of CiliumLoadBalancerIPPool %q",
@@ -159,6 +141,50 @@ func (v *ServiceCustomValidator) validateIPSources(ctx context.Context, service
159141
return nil, nil
160142
}
161143

144+
func validateMatchingIPSources(sources []ipSourceRequest) error {
145+
if len(sources) < 2 {
146+
return nil
147+
}
148+
149+
baseline := sources[0]
150+
for _, source := range sources[1:] {
151+
if !equalRequestedIPs(baseline.ips, source.ips) {
152+
return fmt.Errorf(
153+
"when multiple IP sources are set, %s and %s must specify the same IP value(s)",
154+
baseline.name,
155+
source.name,
156+
)
157+
}
158+
}
159+
160+
return nil
161+
}
162+
163+
func parseRequestedIPs(raw string) []string {
164+
var ips []string
165+
for _, part := range strings.Split(raw, ",") {
166+
if ip := strings.TrimSpace(part); ip != "" {
167+
ips = append(ips, ip)
168+
}
169+
}
170+
171+
return ips
172+
}
173+
174+
func equalRequestedIPs(left, right []string) bool {
175+
if len(left) != len(right) {
176+
return false
177+
}
178+
179+
// Create new slices for sorting to keep order of original slices
180+
leftSorted := append([]string(nil), left...)
181+
rightSorted := append([]string(nil), right...)
182+
slices.Sort(leftSorted)
183+
slices.Sort(rightSorted)
184+
185+
return slices.Equal(leftSorted, rightSorted)
186+
}
187+
162188
// fetchPoolBlocks retrieves the CiliumLoadBalancerIPPool named by the service's
163189
// `network.snappcloud.io/address-pool` label and parses its blocks.
164190
// When the label is absent the pool name defaults to "default".
@@ -249,3 +275,8 @@ func isIPInPool(ipStr string, cidrs []*net.IPNet, ranges []ipRange) bool {
249275
type ipRange struct {
250276
start, end netip.Addr
251277
}
278+
279+
type ipSourceRequest struct {
280+
name string
281+
ips []string
282+
}

‎internal/webhook/v1/service_webhook_test.go‎

Lines changed: 124 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,18 @@ var _ = Describe("Service Webhook", func() {
320320
Expect(err).To(HaveOccurred())
321321
Expect(err.Error()).To(ContainSubstring("192.168.1.1"))
322322
})
323-
It("Should reject creation when both ipam annotations are set", func() {
323+
It("Should allow creation when both ipam annotations are set to the same value", func() {
324+
pool := &ciliumv2alpha1.CiliumLoadBalancerIPPool{
325+
ObjectMeta: metav1.ObjectMeta{
326+
Name: "my-pool",
327+
},
328+
Spec: ciliumv2alpha1.CiliumLoadBalancerIPPoolSpec{
329+
Blocks: []ciliumv2alpha1.CiliumLoadBalancerIPPoolIPBlock{
330+
{Cidr: "10.0.0.0/24"},
331+
},
332+
},
333+
}
334+
324335
obj := &corev1.Service{
325336
ObjectMeta: metav1.ObjectMeta{
326337
Name: "test-svc",
@@ -329,7 +340,32 @@ var _ = Describe("Service Webhook", func() {
329340
"network.snappcloud.io/address-pool": "my",
330341
},
331342
Annotations: map[string]string{
332-
"io.cilium/lb-ipam-ips": "10.0.0.5,192.168.1.1",
343+
"io.cilium/lb-ipam-ips": "10.0.0.5,10.0.0.10",
344+
"lbipam.cilium.io/ips": "10.0.0.10,10.0.0.5",
345+
},
346+
},
347+
}
348+
349+
scheme := runtime.NewScheme()
350+
Expect(ciliumv2alpha1.AddToScheme(scheme)).To(Succeed())
351+
352+
validator := ServiceCustomValidator{
353+
client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(pool).Build(),
354+
}
355+
_, err := validator.ValidateCreate(ctx, obj)
356+
Expect(err).NotTo(HaveOccurred())
357+
})
358+
359+
It("Should reject creation when the two ipam annotations are set to different values", func() {
360+
obj := &corev1.Service{
361+
ObjectMeta: metav1.ObjectMeta{
362+
Name: "test-svc",
363+
Namespace: "default",
364+
Labels: map[string]string{
365+
"network.snappcloud.io/address-pool": "my",
366+
},
367+
Annotations: map[string]string{
368+
"io.cilium/lb-ipam-ips": "10.0.0.4,192.168.1.1",
333369
"lbipam.cilium.io/ips": "10.0.0.4,192.168.1.2",
334370
},
335371
},
@@ -339,10 +375,22 @@ var _ = Describe("Service Webhook", func() {
339375
}
340376
_, err := validator.ValidateCreate(ctx, obj)
341377
Expect(err).To(HaveOccurred())
342-
Expect(err.Error()).To(ContainSubstring("at most one"))
378+
Expect(err.Error()).To(ContainSubstring(lbIpamIpsAnnotation))
379+
Expect(err.Error()).To(ContainSubstring(lbIpamIpsAnnotationAlias))
343380
})
344381

345-
It("Should reject creation when lbIpamIps annotation and loadBalancerIP are both set", func() {
382+
It("Should allow creation when lbIpamIps annotation and loadBalancerIP are set to the same value", func() {
383+
pool := &ciliumv2alpha1.CiliumLoadBalancerIPPool{
384+
ObjectMeta: metav1.ObjectMeta{
385+
Name: "my-pool",
386+
},
387+
Spec: ciliumv2alpha1.CiliumLoadBalancerIPPoolSpec{
388+
Blocks: []ciliumv2alpha1.CiliumLoadBalancerIPPoolIPBlock{
389+
{Cidr: "10.0.0.0/24"},
390+
},
391+
},
392+
}
393+
346394
obj := &corev1.Service{
347395
ObjectMeta: metav1.ObjectMeta{
348396
Name: "test-svc",
@@ -351,22 +399,61 @@ var _ = Describe("Service Webhook", func() {
351399
"network.snappcloud.io/address-pool": "my",
352400
},
353401
Annotations: map[string]string{
354-
"io.cilium/lb-ipam-ips": "10.0.0.5",
402+
"io.cilium/lb-ipam-ips": "10.0.0.5,10.0.0.3",
355403
},
356404
},
357405
Spec: corev1.ServiceSpec{
358-
LoadBalancerIP: "10.0.0.5",
406+
LoadBalancerIP: "10.0.0.3,10.0.0.5",
407+
},
408+
}
409+
410+
scheme := runtime.NewScheme()
411+
Expect(ciliumv2alpha1.AddToScheme(scheme)).To(Succeed())
412+
413+
validator := ServiceCustomValidator{
414+
client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(pool).Build(),
415+
}
416+
_, err := validator.ValidateCreate(ctx, obj)
417+
Expect(err).NotTo(HaveOccurred())
418+
})
419+
420+
It("Should reject creation when lbIpamIps annotation and loadBalancerIP are set to different values", func() {
421+
obj := &corev1.Service{
422+
ObjectMeta: metav1.ObjectMeta{
423+
Name: "test-svc",
424+
Namespace: "default",
425+
Labels: map[string]string{
426+
"network.snappcloud.io/address-pool": "my",
427+
},
428+
Annotations: map[string]string{
429+
"io.cilium/lb-ipam-ips": "10.0.0.5,192.168.1.2",
430+
},
431+
},
432+
Spec: corev1.ServiceSpec{
433+
LoadBalancerIP: "10.0.0.6,192.168.1.2",
359434
},
360435
}
361436
validator := ServiceCustomValidator{
362437
client: fake.NewClientBuilder().Build(),
363438
}
364439
_, err := validator.ValidateCreate(ctx, obj)
365440
Expect(err).To(HaveOccurred())
366-
Expect(err.Error()).To(ContainSubstring("at most one"))
441+
Expect(err.Error()).To(ContainSubstring(lbIpamIpsAnnotation))
442+
Expect(err.Error()).To(ContainSubstring("spec.loadBalancerIP"))
367443
})
368444

369-
It("Should reject creation when lbIpamIps alias annotation and loadBalancerIP are both set", func() {
445+
It("Should allow update when lbIpamIps alias annotation and loadBalancerIP are set to the same value", func() {
446+
pool := &ciliumv2alpha1.CiliumLoadBalancerIPPool{
447+
ObjectMeta: metav1.ObjectMeta{
448+
Name: "my-pool",
449+
},
450+
Spec: ciliumv2alpha1.CiliumLoadBalancerIPPoolSpec{
451+
Blocks: []ciliumv2alpha1.CiliumLoadBalancerIPPoolIPBlock{
452+
{Cidr: "10.0.0.0/24"},
453+
},
454+
},
455+
}
456+
370457
obj := &corev1.Service{
371458
ObjectMeta: metav1.ObjectMeta{
372459
Name: "test-svc",
@@ -382,12 +469,40 @@ var _ = Describe("Service Webhook", func() {
382469
LoadBalancerIP: "10.0.0.5",
383470
},
384471
}
472+
473+
scheme := runtime.NewScheme()
474+
Expect(ciliumv2alpha1.AddToScheme(scheme)).To(Succeed())
475+
476+
validator := ServiceCustomValidator{
477+
client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(pool).Build(),
478+
}
479+
_, err := validator.ValidateUpdate(ctx, nil, obj)
480+
Expect(err).NotTo(HaveOccurred())
481+
})
482+
483+
It("Should reject creation when lbIpamIps alias annotation and loadBalancerIP are set to different values", func() {
484+
obj := &corev1.Service{
485+
ObjectMeta: metav1.ObjectMeta{
486+
Name: "test-svc",
487+
Namespace: "default",
488+
Labels: map[string]string{
489+
"network.snappcloud.io/address-pool": "my",
490+
},
491+
Annotations: map[string]string{
492+
"lbipam.cilium.io/ips": "10.0.0.5",
493+
},
494+
},
495+
Spec: corev1.ServiceSpec{
496+
LoadBalancerIP: "10.0.0.6",
497+
},
498+
}
385499
validator := ServiceCustomValidator{
386500
client: fake.NewClientBuilder().Build(),
387501
}
388502
_, err := validator.ValidateCreate(ctx, obj)
389503
Expect(err).To(HaveOccurred())
390-
Expect(err.Error()).To(ContainSubstring("at most one"))
504+
Expect(err.Error()).To(ContainSubstring(lbIpamIpsAnnotationAlias))
505+
Expect(err.Error()).To(ContainSubstring("spec.loadBalancerIP"))
391506
})
392507
})
393508

0 commit comments

Comments
 (0)