Skip to content

Commit 3440c24

Browse files
Add unit tests for LLMInferenceService and InferenceService URL construction (opendatahub-io#5742)
1 parent 7815699 commit 3440c24

File tree

2 files changed

+238
-9
lines changed

2 files changed

+238
-9
lines changed

packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ const (
6262
LLMInferenceServiceName = "app.kubernetes.io/name"
6363
LLMInferenceServiceComponent = "app.kubernetes.io/component"
6464
LLMInferenceServiceWorkloadComponent = "llminferenceservice-workload"
65+
66+
// Annotation for authentication
67+
authAnnotationKey = "security.opendatahub.io/enable-auth"
6568
)
6669

6770
type TokenKubernetesClient struct {
@@ -684,6 +687,37 @@ func ExtractStatusFromInferenceService(isvc *kservev1beta1.InferenceService) str
684687
return "Stop"
685688
}
686689

690+
// ConstructLLMInferenceServiceURL constructs the internal URL for an LLMInferenceService
691+
// This function is exported for testing purposes
692+
func ConstructLLMInferenceServiceURL(scheme, serviceName, namespace string, port int32) string {
693+
return fmt.Sprintf("%s://%s.%s.svc.cluster.local:%d/v1", scheme, serviceName, namespace, port)
694+
}
695+
696+
// EnsureV1Suffix ensures that the URL ends with /v1 suffix
697+
// This function is exported for testing purposes
698+
func EnsureV1Suffix(url string) string {
699+
if !strings.HasSuffix(url, "/v1") {
700+
return url + "/v1"
701+
}
702+
return url
703+
}
704+
705+
// DetermineSchemeFromAuth determines the URL scheme (http/https) based on auth annotation
706+
// This function is exported for testing purposes
707+
func DetermineSchemeFromAuth(authAnnotation string) string {
708+
if authAnnotation == "true" {
709+
return "https"
710+
}
711+
return "http"
712+
}
713+
714+
// ShouldAddPortToURL determines if a port should be added to the URL
715+
// for InferenceService endpoints only.
716+
// This function is exported for testing purposes
717+
func ShouldAddPortToURL(isHeadless bool, urlHasPort bool) bool {
718+
return isHeadless && !urlHasPort
719+
}
720+
687721
func (kc *TokenKubernetesClient) extractDisplayNameFromInferenceService(isvc *kservev1beta1.InferenceService) string {
688722
if isvc == nil || isvc.Annotations == nil {
689723
return ""
@@ -1247,7 +1281,7 @@ func (kc *TokenKubernetesClient) getModelDetailsFromServingRuntime(ctx context.C
12471281
// When routes are enabled, the Status.URL is the route URL, not the internal URL so we use the Address.URL
12481282
internalURL := targetISVC.Status.Address.URL.URL()
12491283

1250-
if targetISVC.Annotations["security.opendatahub.io/enable-auth"] != "true" {
1284+
if targetISVC.Annotations[authAnnotationKey] != "true" {
12511285
// For non-auth services, ensure http scheme
12521286
if internalURL.Scheme == "https" {
12531287
internalURL.Scheme = "http"
@@ -1264,8 +1298,9 @@ func (kc *TokenKubernetesClient) getModelDetailsFromServingRuntime(ctx context.C
12641298
svc := services[0]
12651299
isHeadless := kc.isHeadlessService(ctx, namespace, svc.Name)
12661300
port := kc.getServingPort(ctx, namespace, svc.Name)
1301+
urlHasPort := internalURL.Port() != ""
12671302

1268-
if isHeadless && internalURL.Port() == "" {
1303+
if ShouldAddPortToURL(isHeadless, urlHasPort) {
12691304
internalURL.Host = fmt.Sprintf("%s:%d", internalURL.Hostname(), port)
12701305
kc.Logger.Debug("headless kserve detected: HeadlessService is used; adding target port to internal URL",
12711306
"service", svc.Name, "port", port, "url", internalURL.String())
@@ -1274,9 +1309,7 @@ func (kc *TokenKubernetesClient) getModelDetailsFromServingRuntime(ctx context.C
12741309

12751310
internalURLStr := internalURL.String()
12761311
// Add /v1 suffix if not present
1277-
if !strings.HasSuffix(internalURLStr, "/v1") {
1278-
internalURLStr = internalURLStr + "/v1"
1279-
}
1312+
internalURLStr = EnsureV1Suffix(internalURLStr)
12801313

12811314
// Extract additional metadata from the InferenceService
12821315
metadata := map[string]interface{}{}
@@ -1432,14 +1465,15 @@ func (kc *TokenKubernetesClient) extractEndpointFromLLMInferenceService(ctx cont
14321465
port := kc.getServingPort(ctx, llmSvc.Namespace, svc.Name)
14331466

14341467
// Determine scheme based on authentication annotation
1435-
scheme := "http"
1436-
if llmSvc.Annotations["security.opendatahub.io/enable-auth"] == "true" {
1437-
scheme = "https"
1468+
var authAnnotation string
1469+
if llmSvc.Annotations != nil {
1470+
authAnnotation = llmSvc.Annotations[authAnnotationKey]
14381471
}
1472+
scheme := DetermineSchemeFromAuth(authAnnotation)
14391473

14401474
// Construct internal URL from service name with port
14411475
// LLMInferenceService workload services (KServe headless mode) always require port
1442-
internalURL := fmt.Sprintf("%s://%s.%s.svc.cluster.local:%d/v1", scheme, svc.Name, llmSvc.Namespace, port)
1476+
internalURL := ConstructLLMInferenceServiceURL(scheme, svc.Name, llmSvc.Namespace, port)
14431477

14441478
kc.Logger.Debug("constructed internal URL for LLMInferenceService",
14451479
"llmServiceName", llmSvc.Name,

packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client_test.go

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,3 +233,198 @@ func TestGenerateLlamaStackConfigWithMaaSModels(t *testing.T) {
233233
assert.Contains(t, err.Error(), "not found")
234234
})
235235
}
236+
237+
// TestLLMInferenceServiceURLConstruction tests that the URL format for LLMInferenceService
238+
// remains consistent and doesn't accidentally change
239+
func TestLLMInferenceServiceURLConstruction(t *testing.T) {
240+
tests := []struct {
241+
name string
242+
scheme string
243+
serviceName string
244+
namespace string
245+
port int32
246+
expected string
247+
}{
248+
{
249+
name: "http URL without auth",
250+
scheme: "http",
251+
serviceName: "test-service",
252+
namespace: "test-namespace",
253+
port: 8080,
254+
expected: "http://test-service.test-namespace.svc.cluster.local:8080/v1",
255+
},
256+
{
257+
name: "https URL with auth",
258+
scheme: "https",
259+
serviceName: "secure-service",
260+
namespace: "prod-namespace",
261+
port: 8443,
262+
expected: "https://secure-service.prod-namespace.svc.cluster.local:8443/v1",
263+
},
264+
}
265+
266+
for _, tt := range tests {
267+
t.Run(tt.name, func(t *testing.T) {
268+
// Call the actual function used in extractEndpointFromLLMInferenceService
269+
actual := ConstructLLMInferenceServiceURL(tt.scheme, tt.serviceName, tt.namespace, tt.port)
270+
271+
assert.Equal(t, tt.expected, actual,
272+
"LLMInferenceService URL format must remain: {scheme}://{service}.{namespace}.svc.cluster.local:{port}/v1")
273+
})
274+
}
275+
}
276+
277+
// TestInferenceServiceURLSuffixConstruction tests that InferenceService URLs
278+
// always get the /v1 suffix appended correctly
279+
func TestInferenceServiceURLSuffixConstruction(t *testing.T) {
280+
tests := []struct {
281+
name string
282+
inputURL string
283+
expected string
284+
}{
285+
{
286+
name: "URL without /v1 suffix gets it added",
287+
inputURL: "http://my-service.namespace.svc.cluster.local",
288+
expected: "http://my-service.namespace.svc.cluster.local/v1",
289+
},
290+
{
291+
name: "URL with /v1 suffix remains unchanged",
292+
inputURL: "http://my-service.namespace.svc.cluster.local/v1",
293+
expected: "http://my-service.namespace.svc.cluster.local/v1",
294+
},
295+
{
296+
name: "URL with port without /v1 suffix gets it added",
297+
inputURL: "http://my-service.namespace.svc.cluster.local:8080",
298+
expected: "http://my-service.namespace.svc.cluster.local:8080/v1",
299+
},
300+
{
301+
name: "URL with port and /v1 suffix remains unchanged",
302+
inputURL: "https://my-service.namespace.svc.cluster.local:8443/v1",
303+
expected: "https://my-service.namespace.svc.cluster.local:8443/v1",
304+
},
305+
}
306+
307+
for _, tt := range tests {
308+
t.Run(tt.name, func(t *testing.T) {
309+
// Call the actual function used for InferenceService URLs
310+
actual := EnsureV1Suffix(tt.inputURL)
311+
312+
assert.Equal(t, tt.expected, actual,
313+
"InferenceService URLs must always end with /v1")
314+
})
315+
}
316+
}
317+
318+
// TestLLMInferenceServiceSchemeSelection tests that the scheme is correctly
319+
// determined based on the auth annotation
320+
func TestLLMInferenceServiceSchemeSelection(t *testing.T) {
321+
tests := []struct {
322+
name string
323+
authAnnotation string
324+
expected string
325+
}{
326+
{
327+
name: "no auth annotation defaults to http",
328+
authAnnotation: "",
329+
expected: "http",
330+
},
331+
{
332+
name: "auth annotation set to false uses http",
333+
authAnnotation: "false",
334+
expected: "http",
335+
},
336+
{
337+
name: "auth annotation set to true uses https",
338+
authAnnotation: "true",
339+
expected: "https",
340+
},
341+
}
342+
343+
for _, tt := range tests {
344+
t.Run(tt.name, func(t *testing.T) {
345+
// Call the actual function used in extractEndpointFromLLMInferenceService
346+
actual := DetermineSchemeFromAuth(tt.authAnnotation)
347+
348+
assert.Equal(t, tt.expected, actual,
349+
"Scheme must be 'https' only when security.opendatahub.io/enable-auth annotation is 'true'")
350+
})
351+
}
352+
}
353+
354+
// TestKServeServiceConstants tests that the label keys used for finding services
355+
// remain consistent and don't accidentally change
356+
func TestKServeServiceConstants(t *testing.T) {
357+
t.Run("InferenceService label key must be serving.kserve.io/inferenceservice", func(t *testing.T) {
358+
assert.Equal(t, "serving.kserve.io/inferenceservice", InferenceServiceName,
359+
"InferenceService label key must not change - this would break service discovery")
360+
})
361+
362+
t.Run("LLMInferenceService name label key must be app.kubernetes.io/name", func(t *testing.T) {
363+
assert.Equal(t, "app.kubernetes.io/name", LLMInferenceServiceName,
364+
"LLMInferenceService name label key must not change - this would break service discovery")
365+
})
366+
367+
t.Run("LLMInferenceService component label key must be app.kubernetes.io/component", func(t *testing.T) {
368+
assert.Equal(t, "app.kubernetes.io/component", LLMInferenceServiceComponent,
369+
"LLMInferenceService component label key must not change - this would break service discovery")
370+
})
371+
372+
t.Run("LLMInferenceService workload component value must be llminferenceservice-workload", func(t *testing.T) {
373+
assert.Equal(t, "llminferenceservice-workload", LLMInferenceServiceWorkloadComponent,
374+
"LLMInferenceService workload component value must not change - this would break service discovery")
375+
})
376+
t.Run("LLMInferenceService auth annotation key must be security.opendatahub.io/enable-auth", func(t *testing.T) {
377+
assert.Equal(t, "security.opendatahub.io/enable-auth", authAnnotationKey,
378+
"LLMInferenceService auth annotation key must not change - this would break authentication scheme determination")
379+
})
380+
}
381+
382+
// TestHeadlessServicePortLogic tests that port is added to URL only when
383+
// the service is headless AND the URL doesn't already have a port
384+
func TestHeadlessServicePortLogic(t *testing.T) {
385+
tests := []struct {
386+
name string
387+
isHeadless bool
388+
urlHasPort bool
389+
expectedAdded bool
390+
description string
391+
}{
392+
{
393+
name: "headless service without port in URL adds port",
394+
isHeadless: true,
395+
urlHasPort: false,
396+
expectedAdded: true,
397+
description: "Port should be added when service is headless and URL has no port",
398+
},
399+
{
400+
name: "headless service with port in URL does not add port",
401+
isHeadless: true,
402+
urlHasPort: true,
403+
expectedAdded: false,
404+
description: "Port should NOT be added when service is headless but URL already has port",
405+
},
406+
{
407+
name: "non-headless service without port in URL does not add port",
408+
isHeadless: false,
409+
urlHasPort: false,
410+
expectedAdded: false,
411+
description: "Port should NOT be added when service is not headless",
412+
},
413+
{
414+
name: "non-headless service with port in URL does not add port",
415+
isHeadless: false,
416+
urlHasPort: true,
417+
expectedAdded: false,
418+
description: "Port should NOT be added when service is not headless (even with port already present)",
419+
},
420+
}
421+
422+
for _, tt := range tests {
423+
t.Run(tt.name, func(t *testing.T) {
424+
// Call the actual function used in getModelDetailsFromServingRuntime
425+
shouldAddPort := ShouldAddPortToURL(tt.isHeadless, tt.urlHasPort)
426+
427+
assert.Equal(t, tt.expectedAdded, shouldAddPort, tt.description)
428+
})
429+
}
430+
}

0 commit comments

Comments
 (0)