Skip to content

Commit 18daecb

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. Removing the redundant check for nil or empty service override. As noted in PR comments, this check could be bypassed by providing an empty map or invalid keys. The actual validation that matters is whether hasPublic and hasInternal are both true, which is checked later in the function. This makes the validation more robust and prevents bypassing the check. 3. Adding comprehensive test coverage for external Keystone API validation including: - Rejection when service override is nil (checks for missing endpoints) - Rejection when service override is empty (checks for missing endpoints) - 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, test coverage, and removing bypassable validation checks
1 parent d0e2685 commit 18daecb

File tree

2 files changed

+332
-11
lines changed

2 files changed

+332
-11
lines changed

‎api/v1beta1/keystoneapi_webhook.go‎

Lines changed: 41 additions & 11 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"
@@ -186,38 +187,67 @@ func (spec *KeystoneAPISpecCore) ValidateExternalKeystoneAPI(basePath *field.Pat
186187

187188
overridePath := basePath.Child("override").Child("service")
188189

189-
// When ExternalKeystoneAPI is true, service overrides must be provided
190-
if spec.Override.Service == nil || len(spec.Override.Service) == 0 {
191-
allErrs = append(allErrs, field.Required(
192-
overridePath,
193-
"external Keystone API requires service override configuration",
194-
))
195-
return allErrs
196-
}
197-
198190
// Both public and internal endpoints must be defined with EndpointURL set
199191
// This ensures services that depend on both endpoints (like Glance) don't fail
200192
// when rendering templates
193+
// Note: We don't check for nil or empty service override here because the
194+
// hasPublic and hasInternal checks below will catch missing endpoints regardless
195+
// of whether the map is nil, empty, or contains invalid keys.
201196
hasPublic := false
202197
hasInternal := false
203198

204199
for endpointType, overrideSpec := range spec.Override.Service {
200+
endpointURLPath := overridePath.Key(string(endpointType)).Child("endpointURL")
205201
if endpointType == service.EndpointPublic {
206202
hasPublic = true
207203
if overrideSpec.EndpointURL == nil || *overrideSpec.EndpointURL == "" {
208204
allErrs = append(allErrs, field.Required(
209-
overridePath.Key(string(endpointType)).Child("endpointURL"),
205+
endpointURLPath,
210206
"external Keystone API requires endpointURL to be set for public endpoint",
211207
))
208+
} else {
209+
// Validate URL format
210+
parsedURL, err := url.Parse(*overrideSpec.EndpointURL)
211+
if err != nil {
212+
allErrs = append(allErrs, field.Invalid(
213+
endpointURLPath,
214+
*overrideSpec.EndpointURL,
215+
fmt.Sprintf("invalid URL format: %v", err),
216+
))
217+
} else if parsedURL.Scheme == "" {
218+
// Require a scheme (http:// or https://)
219+
allErrs = append(allErrs, field.Invalid(
220+
endpointURLPath,
221+
*overrideSpec.EndpointURL,
222+
"URL must include a scheme (e.g., http:// or https://)",
223+
))
224+
}
212225
}
213226
}
214227
if endpointType == service.EndpointInternal {
215228
hasInternal = true
216229
if overrideSpec.EndpointURL == nil || *overrideSpec.EndpointURL == "" {
217230
allErrs = append(allErrs, field.Required(
218-
overridePath.Key(string(endpointType)).Child("endpointURL"),
231+
endpointURLPath,
219232
"external Keystone API requires endpointURL to be set for internal endpoint",
220233
))
234+
} else {
235+
// Validate URL format
236+
parsedURL, err := url.Parse(*overrideSpec.EndpointURL)
237+
if err != nil {
238+
allErrs = append(allErrs, field.Invalid(
239+
endpointURLPath,
240+
*overrideSpec.EndpointURL,
241+
fmt.Sprintf("invalid URL format: %v", err),
242+
))
243+
} else if parsedURL.Scheme == "" {
244+
// Require a scheme (http:// or https://)
245+
allErrs = append(allErrs, field.Invalid(
246+
endpointURLPath,
247+
*overrideSpec.EndpointURL,
248+
"URL must include a scheme (e.g., http:// or https://)",
249+
))
250+
}
221251
}
222252
}
223253
}

‎test/functional/keystoneapi_webhook_test.go‎

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

0 commit comments

Comments
 (0)