Skip to content

Commit d4f4eb4

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #5885 from jwtty/lb-src-range
fix: allow space separated load balancer source ranges
2 parents a0241fc + 7d94786 commit d4f4eb4

File tree

2 files changed

+71
-106
lines changed

2 files changed

+71
-106
lines changed

‎pkg/provider/loadbalancer/configuration.go

Lines changed: 31 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -46,76 +46,63 @@ func AllowedServiceTags(svc *v1.Service) ([]string, error) {
4646
return nil, nil
4747
}
4848

49-
return strings.Split(strings.TrimSpace(value), Sep), nil
49+
tags := strings.Split(strings.TrimSpace(value), Sep)
50+
for i := range tags {
51+
tags[i] = strings.TrimSpace(tags[i])
52+
}
53+
return tags, nil
5054
}
5155

52-
// AllowedIPRanges returns the allowed IP ranges configured by user through AKS custom annotation.
56+
// AllowedIPRanges returns the allowed IP ranges configured by user through AKS custom annotations:
57+
// service.beta.kubernetes.io/azure-allowed-ip-ranges and service.beta.kubernetes.io/load-balancer-source-ranges
5358
func AllowedIPRanges(svc *v1.Service) ([]netip.Prefix, []string, error) {
5459
const (
5560
Sep = ","
56-
Key = consts.ServiceAnnotationAllowedIPRanges
5761
)
5862
var (
5963
errs []error
6064
validRanges []netip.Prefix
6165
invalidRanges []string
6266
)
6367

64-
value, found := svc.Annotations[Key]
65-
if !found {
66-
return nil, nil, nil
67-
}
68+
for _, key := range []string{consts.ServiceAnnotationAllowedIPRanges, v1.AnnotationLoadBalancerSourceRangesKey} {
69+
value, found := svc.Annotations[key]
70+
if !found {
71+
continue
72+
}
6873

69-
for _, p := range strings.Split(strings.TrimSpace(value), Sep) {
70-
prefix, err := iputil.ParsePrefix(p)
71-
if err != nil {
72-
errs = append(errs, err)
73-
invalidRanges = append(invalidRanges, p)
74-
} else {
75-
validRanges = append(validRanges, prefix)
74+
var errsByKey []error
75+
for _, p := range strings.Split(strings.TrimSpace(value), Sep) {
76+
p = strings.TrimSpace(p)
77+
prefix, err := iputil.ParsePrefix(p)
78+
if err != nil {
79+
errsByKey = append(errsByKey, err)
80+
invalidRanges = append(invalidRanges, p)
81+
} else {
82+
validRanges = append(validRanges, prefix)
83+
}
84+
}
85+
if len(errsByKey) > 0 {
86+
errs = append(errs, NewErrAnnotationValue(key, value, errors.Join(errsByKey...)))
7687
}
7788
}
89+
7890
if len(errs) > 0 {
79-
return validRanges, invalidRanges, NewErrAnnotationValue(Key, value, errors.Join(errs...))
91+
return validRanges, invalidRanges, errors.Join(errs...)
8092
}
8193
return validRanges, invalidRanges, nil
8294
}
8395

84-
// SourceRanges returns the allowed IP ranges configured by user through `spec.LoadBalancerSourceRanges` and standard annotation.
85-
// If `spec.LoadBalancerSourceRanges` is not set, it will try to parse the annotation.
96+
// SourceRanges returns the allowed IP ranges configured by user through `spec.LoadBalancerSourceRanges`.
8697
func SourceRanges(svc *v1.Service) ([]netip.Prefix, []string, error) {
8798
var (
8899
errs []error
89100
validRanges []netip.Prefix
90101
invalidRanges []string
91102
)
92-
// Read from spec
93-
if len(svc.Spec.LoadBalancerSourceRanges) > 0 {
94-
for _, p := range svc.Spec.LoadBalancerSourceRanges {
95-
prefix, err := iputil.ParsePrefix(p)
96-
if err != nil {
97-
errs = append(errs, err)
98-
invalidRanges = append(invalidRanges, p)
99-
} else {
100-
validRanges = append(validRanges, prefix)
101-
}
102-
}
103-
if len(errs) > 0 {
104-
return validRanges, invalidRanges, fmt.Errorf("invalid service.Spec.LoadBalancerSourceRanges [%v]: %w", svc.Spec.LoadBalancerSourceRanges, errors.Join(errs...))
105-
}
106-
return validRanges, invalidRanges, nil
107-
}
108103

109-
// Read from annotation
110-
const (
111-
Sep = ","
112-
Key = v1.AnnotationLoadBalancerSourceRangesKey
113-
)
114-
value, found := svc.Annotations[Key]
115-
if !found {
116-
return nil, nil, nil
117-
}
118-
for _, p := range strings.Split(strings.TrimSpace(value), Sep) {
104+
for _, p := range svc.Spec.LoadBalancerSourceRanges {
105+
p = strings.TrimSpace(p)
119106
prefix, err := iputil.ParsePrefix(p)
120107
if err != nil {
121108
errs = append(errs, err)
@@ -125,7 +112,7 @@ func SourceRanges(svc *v1.Service) ([]netip.Prefix, []string, error) {
125112
}
126113
}
127114
if len(errs) > 0 {
128-
return validRanges, invalidRanges, NewErrAnnotationValue(Key, value, errors.Join(errs...))
115+
return validRanges, invalidRanges, fmt.Errorf("invalid service.Spec.LoadBalancerSourceRanges [%v]: %w", svc.Spec.LoadBalancerSourceRanges, errors.Join(errs...))
129116
}
130117
return validRanges, invalidRanges, nil
131118
}

‎pkg/provider/loadbalancer/configuration_test.go

Lines changed: 40 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ func TestAllowedServiceTags(t *testing.T) {
102102
},
103103
ObjectMeta: metav1.ObjectMeta{
104104
Annotations: map[string]string{
105-
consts.ServiceAnnotationAllowedServiceTags: "Microsoft.ContainerInstance/containerGroups,foo,bar",
105+
consts.ServiceAnnotationAllowedServiceTags: " Microsoft.ContainerInstance/containerGroups, foo, bar ",
106106
},
107107
},
108108
})
@@ -125,7 +125,7 @@ func TestAllowedIPRanges(t *testing.T) {
125125
assert.Empty(t, actual)
126126
assert.Empty(t, invalid)
127127
})
128-
t.Run("with 1 IPv4 range", func(t *testing.T) {
128+
t.Run("with 1 IPv4 range in allowed ip ranges", func(t *testing.T) {
129129
actual, invalid, err := AllowedIPRanges(&v1.Service{
130130
Spec: v1.ServiceSpec{
131131
Type: v1.ServiceTypeLoadBalancer,
@@ -140,7 +140,22 @@ func TestAllowedIPRanges(t *testing.T) {
140140
assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.10.0.0/24")}, actual)
141141
assert.Empty(t, invalid)
142142
})
143-
t.Run("with 1 IPv6 range", func(t *testing.T) {
143+
t.Run("with 1 IPv4 range in load balancer source ranges", func(t *testing.T) {
144+
actual, invalid, err := AllowedIPRanges(&v1.Service{
145+
Spec: v1.ServiceSpec{
146+
Type: v1.ServiceTypeLoadBalancer,
147+
},
148+
ObjectMeta: metav1.ObjectMeta{
149+
Annotations: map[string]string{
150+
v1.AnnotationLoadBalancerSourceRangesKey: "10.10.0.0/24",
151+
},
152+
},
153+
})
154+
assert.NoError(t, err)
155+
assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.10.0.0/24")}, actual)
156+
assert.Empty(t, invalid)
157+
})
158+
t.Run("with 1 IPv6 range in allowed ip ranges", func(t *testing.T) {
144159
actual, invalid, err := AllowedIPRanges(&v1.Service{
145160
Spec: v1.ServiceSpec{
146161
Type: v1.ServiceTypeLoadBalancer,
@@ -155,21 +170,39 @@ func TestAllowedIPRanges(t *testing.T) {
155170
assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("2001:db8::/32")}, actual)
156171
assert.Empty(t, invalid)
157172
})
173+
t.Run("with 1 IPv6 range in load balancer source ranges", func(t *testing.T) {
174+
actual, invalid, err := AllowedIPRanges(&v1.Service{
175+
Spec: v1.ServiceSpec{
176+
Type: v1.ServiceTypeLoadBalancer,
177+
},
178+
ObjectMeta: metav1.ObjectMeta{
179+
Annotations: map[string]string{
180+
v1.AnnotationLoadBalancerSourceRangesKey: "2001:db8::/32",
181+
},
182+
},
183+
})
184+
assert.NoError(t, err)
185+
assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("2001:db8::/32")}, actual)
186+
assert.Empty(t, invalid)
187+
})
158188
t.Run("with multiple IP ranges", func(t *testing.T) {
159189
actual, invalid, err := AllowedIPRanges(&v1.Service{
160190
Spec: v1.ServiceSpec{
161191
Type: v1.ServiceTypeLoadBalancer,
162192
},
163193
ObjectMeta: metav1.ObjectMeta{
164194
Annotations: map[string]string{
165-
consts.ServiceAnnotationAllowedIPRanges: "10.10.0.0/24,2001:db8::/32",
195+
consts.ServiceAnnotationAllowedIPRanges: " 10.10.0.0/24, 2001:db8::/32 ",
196+
v1.AnnotationLoadBalancerSourceRangesKey: " 10.20.0.0/24, 2002:db8::/32 ",
166197
},
167198
},
168199
})
169200
assert.NoError(t, err)
170201
assert.Equal(t, []netip.Prefix{
171202
netip.MustParsePrefix("10.10.0.0/24"),
172203
netip.MustParsePrefix("2001:db8::/32"),
204+
netip.MustParsePrefix("10.20.0.0/24"),
205+
netip.MustParsePrefix("2002:db8::/32"),
173206
}, actual)
174207
assert.Empty(t, invalid)
175208
})
@@ -180,16 +213,16 @@ func TestAllowedIPRanges(t *testing.T) {
180213
},
181214
ObjectMeta: metav1.ObjectMeta{
182215
Annotations: map[string]string{
183-
consts.ServiceAnnotationAllowedIPRanges: "foobar,10.0.0.1/24",
216+
consts.ServiceAnnotationAllowedIPRanges: "foobar,10.0.0.1/24",
217+
v1.AnnotationLoadBalancerSourceRangesKey: "barfoo,2002:db8::1/32",
184218
},
185219
},
186220
})
187221
assert.Error(t, err)
188222

189223
var e *ErrAnnotationValue
190224
assert.ErrorAs(t, err, &e)
191-
assert.Equal(t, e.AnnotationKey, consts.ServiceAnnotationAllowedIPRanges)
192-
assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid)
225+
assert.Equal(t, []string{"foobar", "10.0.0.1/24", "barfoo", "2002:db8::1/32"}, invalid)
193226
})
194227
}
195228

@@ -218,42 +251,6 @@ func TestSourceRanges(t *testing.T) {
218251
}, actual)
219252
assert.Empty(t, invalid)
220253
})
221-
t.Run("specified in annotation", func(t *testing.T) {
222-
actual, invalid, err := SourceRanges(&v1.Service{
223-
Spec: v1.ServiceSpec{
224-
Type: v1.ServiceTypeLoadBalancer,
225-
},
226-
ObjectMeta: metav1.ObjectMeta{
227-
Annotations: map[string]string{
228-
v1.AnnotationLoadBalancerSourceRangesKey: "10.10.0.0/24,2001:db8::/32",
229-
},
230-
},
231-
})
232-
assert.NoError(t, err)
233-
assert.Equal(t, []netip.Prefix{
234-
netip.MustParsePrefix("10.10.0.0/24"),
235-
netip.MustParsePrefix("2001:db8::/32"),
236-
}, actual)
237-
assert.Empty(t, invalid)
238-
})
239-
t.Run("specified in both spec and annotation", func(t *testing.T) {
240-
actual, invalid, err := SourceRanges(&v1.Service{
241-
Spec: v1.ServiceSpec{
242-
Type: v1.ServiceTypeLoadBalancer,
243-
LoadBalancerSourceRanges: []string{"10.10.0.0/24"},
244-
},
245-
ObjectMeta: metav1.ObjectMeta{
246-
Annotations: map[string]string{
247-
v1.AnnotationLoadBalancerSourceRangesKey: "2001:db8::/32",
248-
},
249-
},
250-
})
251-
assert.NoError(t, err)
252-
assert.Equal(t, []netip.Prefix{
253-
netip.MustParsePrefix("10.10.0.0/24"),
254-
}, actual, "spec should take precedence over annotation")
255-
assert.Empty(t, invalid)
256-
})
257254
t.Run("with invalid IP range in spec", func(t *testing.T) {
258255
_, invalid, err := SourceRanges(&v1.Service{
259256
Spec: v1.ServiceSpec{
@@ -267,25 +264,6 @@ func TestSourceRanges(t *testing.T) {
267264
assert.Error(t, err)
268265
assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid)
269266
})
270-
271-
t.Run("with invalid IP range in annotation", func(t *testing.T) {
272-
_, invalid, err := SourceRanges(&v1.Service{
273-
Spec: v1.ServiceSpec{
274-
Type: v1.ServiceTypeLoadBalancer,
275-
},
276-
ObjectMeta: metav1.ObjectMeta{
277-
Annotations: map[string]string{
278-
v1.AnnotationLoadBalancerSourceRangesKey: "foobar,10.0.0.1/24",
279-
},
280-
},
281-
})
282-
assert.Error(t, err)
283-
284-
var e *ErrAnnotationValue
285-
assert.ErrorAs(t, err, &e)
286-
assert.Equal(t, e.AnnotationKey, v1.AnnotationLoadBalancerSourceRangesKey)
287-
assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid)
288-
})
289267
}
290268

291269
func TestAdditionalPublicIPs(t *testing.T) {

0 commit comments

Comments
 (0)