Skip to content

Commit e06fc42

Browse files
vakwetuvakwetu
committed
Add URL validation and comprehensive tests for external Keystone API
This commit enhances the external Keystone API webhook validation by: 1. Adding URL format validation using net/url.Parse() to ensure endpointURL values are valid URLs for both public and internal endpoints. The validation checks both for parsing errors and requires a URL scheme (http:// or https://) to catch URLs without schemes that url.Parse() would otherwise accept. 2. Adding comprehensive test coverage for external Keystone API validation including: - Rejection when service override is nil - Rejection when service override is empty - Rejection when only admin endpoint is provided (missing public/internal) - Rejection when public endpoint is missing - Rejection when internal endpoint is missing - Rejection when endpointURL is empty string - Rejection of URLs without scheme for public endpoint - Rejection of malformed URLs for internal endpoint - Acceptance when both public and internal endpoints are valid All 79 tests pass, including 9 new tests for external Keystone API validation. Related: PR comments requesting URL validation and test coverage
1 parent d0e2685 commit e06fc42

File tree

2 files changed

+323
-2
lines changed

2 files changed

+323
-2
lines changed

‎api/v1beta1/keystoneapi_webhook.go‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ package v1beta1
2424

2525
import (
2626
"fmt"
27+
"net/url"
2728

2829
"github.com/openstack-k8s-operators/lib-common/modules/common/service"
2930
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -202,22 +203,57 @@ func (spec *KeystoneAPISpecCore) ValidateExternalKeystoneAPI(basePath *field.Pat
202203
hasInternal := false
203204

204205
for endpointType, overrideSpec := range spec.Override.Service {
206+
endpointURLPath := overridePath.Key(string(endpointType)).Child("endpointURL")
205207
if endpointType == service.EndpointPublic {
206208
hasPublic = true
207209
if overrideSpec.EndpointURL == nil || *overrideSpec.EndpointURL == "" {
208210
allErrs = append(allErrs, field.Required(
209-
overridePath.Key(string(endpointType)).Child("endpointURL"),
211+
endpointURLPath,
210212
"external Keystone API requires endpointURL to be set for public endpoint",
211213
))
214+
} else {
215+
// Validate URL format
216+
parsedURL, err := url.Parse(*overrideSpec.EndpointURL)
217+
if err != nil {
218+
allErrs = append(allErrs, field.Invalid(
219+
endpointURLPath,
220+
*overrideSpec.EndpointURL,
221+
fmt.Sprintf("invalid URL format: %v", err),
222+
))
223+
} else if parsedURL.Scheme == "" {
224+
// Require a scheme (http:// or https://)
225+
allErrs = append(allErrs, field.Invalid(
226+
endpointURLPath,
227+
*overrideSpec.EndpointURL,
228+
"URL must include a scheme (e.g., http:// or https://)",
229+
))
230+
}
212231
}
213232
}
214233
if endpointType == service.EndpointInternal {
215234
hasInternal = true
216235
if overrideSpec.EndpointURL == nil || *overrideSpec.EndpointURL == "" {
217236
allErrs = append(allErrs, field.Required(
218-
overridePath.Key(string(endpointType)).Child("endpointURL"),
237+
endpointURLPath,
219238
"external Keystone API requires endpointURL to be set for internal endpoint",
220239
))
240+
} else {
241+
// Validate URL format
242+
parsedURL, err := url.Parse(*overrideSpec.EndpointURL)
243+
if err != nil {
244+
allErrs = append(allErrs, field.Invalid(
245+
endpointURLPath,
246+
*overrideSpec.EndpointURL,
247+
fmt.Sprintf("invalid URL format: %v", err),
248+
))
249+
} else if parsedURL.Scheme == "" {
250+
// Require a scheme (http:// or https://)
251+
allErrs = append(allErrs, field.Invalid(
252+
endpointURLPath,
253+
*overrideSpec.EndpointURL,
254+
"URL must include a scheme (e.g., http:// or https://)",
255+
))
256+
}
221257
}
222258
}
223259
}

‎test/functional/keystoneapi_webhook_test.go‎

Lines changed: 285 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,4 +216,289 @@ var _ = Describe("KeystoneAPI Webhook", func() {
216216
"spec.topologyRef.namespace: Invalid value: \"namespace\": Customizing namespace field is not supported"),
217217
)
218218
})
219+
220+
When("ExternalKeystoneAPI validation", func() {
221+
It("rejects ExternalKeystoneAPI=true with nil service override", func() {
222+
spec := GetDefaultKeystoneAPISpec()
223+
spec["externalKeystoneAPI"] = true
224+
// Don't set any override
225+
226+
raw := map[string]any{
227+
"apiVersion": "keystone.openstack.org/v1beta1",
228+
"kind": "KeystoneAPI",
229+
"metadata": map[string]any{
230+
"name": keystoneAPIName.Name,
231+
"namespace": keystoneAPIName.Namespace,
232+
},
233+
"spec": spec,
234+
}
235+
236+
unstructuredObj := &unstructured.Unstructured{Object: raw}
237+
_, err := controllerutil.CreateOrPatch(
238+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
239+
Expect(err).To(HaveOccurred())
240+
Expect(err.Error()).To(
241+
ContainSubstring(
242+
"external Keystone API requires service override configuration"),
243+
)
244+
})
245+
246+
It("rejects ExternalKeystoneAPI=true with empty service override", func() {
247+
spec := GetDefaultKeystoneAPISpec()
248+
spec["externalKeystoneAPI"] = true
249+
spec["override"] = map[string]any{
250+
"service": map[string]any{},
251+
}
252+
253+
raw := map[string]any{
254+
"apiVersion": "keystone.openstack.org/v1beta1",
255+
"kind": "KeystoneAPI",
256+
"metadata": map[string]any{
257+
"name": keystoneAPIName.Name,
258+
"namespace": keystoneAPIName.Namespace,
259+
},
260+
"spec": spec,
261+
}
262+
263+
unstructuredObj := &unstructured.Unstructured{Object: raw}
264+
_, err := controllerutil.CreateOrPatch(
265+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
266+
Expect(err).To(HaveOccurred())
267+
Expect(err.Error()).To(
268+
ContainSubstring(
269+
"external Keystone API requires service override configuration"),
270+
)
271+
})
272+
273+
It("rejects ExternalKeystoneAPI=true with service override but no endpoints", func() {
274+
spec := GetDefaultKeystoneAPISpec()
275+
spec["externalKeystoneAPI"] = true
276+
spec["override"] = map[string]any{
277+
"service": map[string]any{
278+
"admin": map[string]any{
279+
"metadata": map[string]any{
280+
"labels": map[string]any{
281+
"custom": "label",
282+
},
283+
},
284+
},
285+
},
286+
}
287+
288+
raw := map[string]any{
289+
"apiVersion": "keystone.openstack.org/v1beta1",
290+
"kind": "KeystoneAPI",
291+
"metadata": map[string]any{
292+
"name": keystoneAPIName.Name,
293+
"namespace": keystoneAPIName.Namespace,
294+
},
295+
"spec": spec,
296+
}
297+
298+
unstructuredObj := &unstructured.Unstructured{Object: raw}
299+
_, err := controllerutil.CreateOrPatch(
300+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
301+
Expect(err).To(HaveOccurred())
302+
Expect(err.Error()).To(
303+
ContainSubstring(
304+
"external Keystone API requires public endpoint to be defined"),
305+
)
306+
})
307+
308+
It("rejects ExternalKeystoneAPI=true with missing public endpoint", func() {
309+
spec := GetDefaultKeystoneAPISpec()
310+
spec["externalKeystoneAPI"] = true
311+
spec["override"] = map[string]any{
312+
"service": map[string]any{
313+
"internal": map[string]any{
314+
"endpointURL": "http://internal.keystone.example.com:5000",
315+
},
316+
},
317+
}
318+
319+
raw := map[string]any{
320+
"apiVersion": "keystone.openstack.org/v1beta1",
321+
"kind": "KeystoneAPI",
322+
"metadata": map[string]any{
323+
"name": keystoneAPIName.Name,
324+
"namespace": keystoneAPIName.Namespace,
325+
},
326+
"spec": spec,
327+
}
328+
329+
unstructuredObj := &unstructured.Unstructured{Object: raw}
330+
_, err := controllerutil.CreateOrPatch(
331+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
332+
Expect(err).To(HaveOccurred())
333+
Expect(err.Error()).To(
334+
ContainSubstring(
335+
"external Keystone API requires public endpoint to be defined"),
336+
)
337+
})
338+
339+
It("rejects ExternalKeystoneAPI=true with missing internal endpoint", func() {
340+
spec := GetDefaultKeystoneAPISpec()
341+
spec["externalKeystoneAPI"] = true
342+
spec["override"] = map[string]any{
343+
"service": map[string]any{
344+
"public": map[string]any{
345+
"endpointURL": "http://public.keystone.example.com:5000",
346+
},
347+
},
348+
}
349+
350+
raw := map[string]any{
351+
"apiVersion": "keystone.openstack.org/v1beta1",
352+
"kind": "KeystoneAPI",
353+
"metadata": map[string]any{
354+
"name": keystoneAPIName.Name,
355+
"namespace": keystoneAPIName.Namespace,
356+
},
357+
"spec": spec,
358+
}
359+
360+
unstructuredObj := &unstructured.Unstructured{Object: raw}
361+
_, err := controllerutil.CreateOrPatch(
362+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
363+
Expect(err).To(HaveOccurred())
364+
Expect(err.Error()).To(
365+
ContainSubstring(
366+
"external Keystone API requires internal endpoint to be defined"),
367+
)
368+
})
369+
370+
It("rejects ExternalKeystoneAPI=true with empty endpointURL", func() {
371+
spec := GetDefaultKeystoneAPISpec()
372+
spec["externalKeystoneAPI"] = true
373+
spec["override"] = map[string]any{
374+
"service": map[string]any{
375+
"public": map[string]any{
376+
"endpointURL": "",
377+
},
378+
"internal": map[string]any{
379+
"endpointURL": "http://internal.keystone.example.com:5000",
380+
},
381+
},
382+
}
383+
384+
raw := map[string]any{
385+
"apiVersion": "keystone.openstack.org/v1beta1",
386+
"kind": "KeystoneAPI",
387+
"metadata": map[string]any{
388+
"name": keystoneAPIName.Name,
389+
"namespace": keystoneAPIName.Namespace,
390+
},
391+
"spec": spec,
392+
}
393+
394+
unstructuredObj := &unstructured.Unstructured{Object: raw}
395+
_, err := controllerutil.CreateOrPatch(
396+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
397+
Expect(err).To(HaveOccurred())
398+
Expect(err.Error()).To(
399+
ContainSubstring(
400+
"external Keystone API requires endpointURL to be set for public endpoint"),
401+
)
402+
})
403+
404+
It("rejects ExternalKeystoneAPI=true with invalid URL format for public endpoint", func() {
405+
spec := GetDefaultKeystoneAPISpec()
406+
spec["externalKeystoneAPI"] = true
407+
spec["override"] = map[string]any{
408+
"service": map[string]any{
409+
"public": map[string]any{
410+
"endpointURL": "not-a-valid-url",
411+
},
412+
"internal": map[string]any{
413+
"endpointURL": "http://internal.keystone.example.com:5000",
414+
},
415+
},
416+
}
417+
418+
raw := map[string]any{
419+
"apiVersion": "keystone.openstack.org/v1beta1",
420+
"kind": "KeystoneAPI",
421+
"metadata": map[string]any{
422+
"name": keystoneAPIName.Name,
423+
"namespace": keystoneAPIName.Namespace,
424+
},
425+
"spec": spec,
426+
}
427+
428+
unstructuredObj := &unstructured.Unstructured{Object: raw}
429+
_, err := controllerutil.CreateOrPatch(
430+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
431+
Expect(err).To(HaveOccurred())
432+
Expect(err.Error()).To(
433+
ContainSubstring(
434+
"URL must include a scheme"),
435+
)
436+
})
437+
438+
It("rejects ExternalKeystoneAPI=true with invalid URL format for internal endpoint", func() {
439+
spec := GetDefaultKeystoneAPISpec()
440+
spec["externalKeystoneAPI"] = true
441+
spec["override"] = map[string]any{
442+
"service": map[string]any{
443+
"public": map[string]any{
444+
"endpointURL": "http://public.keystone.example.com:5000",
445+
},
446+
"internal": map[string]any{
447+
"endpointURL": "://invalid-url",
448+
},
449+
},
450+
}
451+
452+
raw := map[string]any{
453+
"apiVersion": "keystone.openstack.org/v1beta1",
454+
"kind": "KeystoneAPI",
455+
"metadata": map[string]any{
456+
"name": keystoneAPIName.Name,
457+
"namespace": keystoneAPIName.Namespace,
458+
},
459+
"spec": spec,
460+
}
461+
462+
unstructuredObj := &unstructured.Unstructured{Object: raw}
463+
_, err := controllerutil.CreateOrPatch(
464+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
465+
Expect(err).To(HaveOccurred())
466+
Expect(err.Error()).To(
467+
Or(
468+
ContainSubstring("invalid URL format"),
469+
ContainSubstring("URL must include a scheme"),
470+
),
471+
)
472+
})
473+
474+
It("accepts ExternalKeystoneAPI=true with valid endpoints", func() {
475+
spec := GetDefaultKeystoneAPISpec()
476+
spec["externalKeystoneAPI"] = true
477+
spec["override"] = map[string]any{
478+
"service": map[string]any{
479+
"public": map[string]any{
480+
"endpointURL": "http://public.keystone.example.com:5000",
481+
},
482+
"internal": map[string]any{
483+
"endpointURL": "http://internal.keystone.example.com:5000",
484+
},
485+
},
486+
}
487+
488+
raw := map[string]any{
489+
"apiVersion": "keystone.openstack.org/v1beta1",
490+
"kind": "KeystoneAPI",
491+
"metadata": map[string]any{
492+
"name": keystoneAPIName.Name,
493+
"namespace": keystoneAPIName.Namespace,
494+
},
495+
"spec": spec,
496+
}
497+
498+
unstructuredObj := &unstructured.Unstructured{Object: raw}
499+
_, err := controllerutil.CreateOrPatch(
500+
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
501+
Expect(err).NotTo(HaveOccurred())
502+
})
503+
})
219504
})

0 commit comments

Comments
 (0)