Skip to content

fix(kserve): add annotation to use HTTP logger endpoint for Raw deployments #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ruivieira wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(kserve): add annotation to use HTTP logger endpoint for Raw deployments #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions controllers/tas/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,9 @@ const (
)

const migrationAnnotationKey = "trustyai.opendatahub.io/db-migration"

// KServe logger HTTP annotation: when set to "true" on a TrustyAIService CR,
// the operator injects an HTTP (not HTTPS) logger URL into KServe InferenceServices.
// This avoids x509 certificate verification failures for cluster-internal traffic.
// See: RHOAIENG-38132
const kserveLoggerHTTPAnnotationKey = "trustyai.opendatahub.io/kserve-logger-http"
10 changes: 9 additions & 1 deletion controllers/tas/inference_services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,13 @@ func (r *TrustyAIServiceReconciler) handleInferenceServices(ctx context.Context,
return true, nil
}

// Check if the TrustyAIService CR has the annotation to use HTTP for KServe logger.
// When "trustyai.opendatahub.io/kserve-logger-http" is set to "true", the operator
// uses HTTP instead of HTTPS to avoid x509 certificate verification failures
// with cluster-internal Service CA certificates. (RHOAIENG-38132)
instanceAnnotations := instance.GetAnnotations()
useHTTPLogger := instanceAnnotations[kserveLoggerHTTPAnnotationKey] == "true"

for _, infService := range inferenceServices.Items {
annotations := infService.GetAnnotations()
deploymentMode := annotations["serving.kserve.io/deploymentMode"]
Expand All @@ -245,7 +252,8 @@ func (r *TrustyAIServiceReconciler) handleInferenceServices(ctx context.Context,

case DEPLOYMENT_MODE_RAW:
// Handle KServe Raw deployments
err := r.patchKServe(ctx, instance, infService, namespace, crName, remove, true)
// Use HTTPS by default, unless the HTTP logger annotation is set
err := r.patchKServe(ctx, instance, infService, namespace, crName, remove, !useHTTPLogger)
Comment on lines +255 to +256
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Line 256 introduces scheme-coupled cleanup risk for remove=true.

patchKServe removal currently only deletes when the existing logger URL equals the computed scheme URL. After this change, if the annotation value changes before cleanup, removal can be skipped and the old logger remains configured.

💡 Proposed fix (make removal accept either operator-managed scheme) 
 func (r *TrustyAIServiceReconciler) patchKServe(ctx context.Context, instance *trustyaiopendatahubiov1alpha1.TrustyAIService, infService kservev1beta1.InferenceService, namespace string, crName string, remove bool, useHTTPS bool) error {
@@
 	if remove {
-		if infService.Spec.Predictor.Logger == nil || *infService.Spec.Predictor.Logger.URL != url {
+		if infService.Spec.Predictor.Logger == nil || infService.Spec.Predictor.Logger.URL == nil {
 			return nil // Removing, but InferenceLogger is not set or is not set to the expected URL. Do nothing.
 		}
+		currentURL := *infService.Spec.Predictor.Logger.URL
+		httpsURL := utils.GenerateHTTPSKServeLoggerURL(crName, namespace)
+		httpURL := utils.GenerateKServeLoggerURL(crName, namespace)
+		if currentURL != httpsURL && currentURL != httpURL {
+			return nil // Do not remove non-operator-managed logger URLs.
+		}
 
 		// Remove the InferenceLogger
 		infService.Spec.Predictor.Logger = nil
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@controllers/tas/inference_services.go` around lines 255 - 256, When removing
operator-managed logger config (remove==true) patchKServe currently only
compares the existing logger URL against one computed scheme and can skip
deletion if the annotation changed (HTTP vs HTTPS); update patchKServe to
compute both operator-managed variants (HTTP and HTTPS) and treat the existing
logger URL as removable if it equals either variant (or matches the
operator-managed host pattern) so removal does not depend on which scheme was
last stored; adjust logic where patchKServe is called (the call with
!useHTTPLogger) and inside patchKServe to check both schemes when remove is
true.

if err != nil {
log.FromContext(ctx).Error(err, "Could not patch InferenceLogger for KServe Raw deployment")
return false, err
Expand Down
205 changes: 205 additions & 0 deletions controllers/tas/inference_services_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
package tas

import (
"context"

kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/trustyai-explainability/trustyai-service-operator/controllers/utils"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

var _ = Describe("KServe logger HTTP annotation", func() {
var (
testReconciler *TrustyAIServiceReconciler
testCtx context.Context
)

BeforeEach(func() {
testReconciler = &TrustyAIServiceReconciler{
Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(),
Scheme: scheme.Scheme,
EventRecorder: record.NewFakeRecorder(10),
Namespace: operatorNamespace,
}
testCtx = context.Background()
})
Comment on lines +23 to +31
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use envtest-backed controller tests instead of fake client.

Line 25 builds the reconciler with fake.NewClientBuilder(), which does not meet the controller test harness requirement and can miss API-server behavior differences.

As per coding guidelines, "Use Ginkgo v2 with Gomega assertions and controller-runtime envtest for all controller tests."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@controllers/tas/inference_services_test.go` around lines 23 - 31, Replace the
fake client usage in the BeforeEach when constructing testReconciler with an
envtest-backed controller-runtime client: start an envtest.Environment, register
the scheme entries (same scheme.Scheme used now), create a manager and use
mgr.GetClient() (or client.New with mgr.GetConfig()) as the Client for
TrustyAIServiceReconciler, and assign testCtx; ensure you stop/tear down the
envtest environment in AfterEach and load any required CRDs so the envtest API
server behavior matches real controller tests instead of using
fake.NewClientBuilder().


Context("When patchKServe is called with useHTTPS=true (default for Raw)", func() {
It("should set an HTTPS logger URL on the InferenceService", func() {
namespace := "trusty-kserve-https-test"
Expect(createNamespace(testCtx, testReconciler.Client, namespace)).To(Succeed())

instance := createDefaultPVCCustomResource(namespace)
Expect(createTestPVC(testCtx, testReconciler.Client, instance)).To(Succeed())

inferenceService := createInferenceService("my-model-https", namespace)
Expect(testReconciler.Client.Create(testCtx, inferenceService)).To(Succeed())

// Call patchKServe with useHTTPS=true (default Raw behavior)
Expect(testReconciler.patchKServe(testCtx, instance, *inferenceService, namespace, instance.Name, false, true)).To(Succeed())

// Fetch the updated InferenceService
updated := &kservev1beta1.InferenceService{}
Expect(testReconciler.Client.Get(testCtx, types.NamespacedName{Name: "my-model-https", Namespace: namespace}, updated)).To(Succeed())

Expect(updated.Spec.Predictor.Logger).NotTo(BeNil())
expectedURL := utils.GenerateHTTPSKServeLoggerURL(instance.Name, namespace)
Expect(*updated.Spec.Predictor.Logger.URL).To(Equal(expectedURL))
Expect(*updated.Spec.Predictor.Logger.URL).To(HavePrefix("https://"))
})
})

Context("When patchKServe is called with useHTTPS=false (annotation-driven)", func() {
It("should set an HTTP logger URL on the InferenceService", func() {
namespace := "trusty-kserve-http-test"
Expect(createNamespace(testCtx, testReconciler.Client, namespace)).To(Succeed())

instance := createDefaultPVCCustomResource(namespace)
Expect(createTestPVC(testCtx, testReconciler.Client, instance)).To(Succeed())

inferenceService := createInferenceService("my-model-http", namespace)
Expect(testReconciler.Client.Create(testCtx, inferenceService)).To(Succeed())

// Call patchKServe with useHTTPS=false (HTTP logger annotation behavior)
Expect(testReconciler.patchKServe(testCtx, instance, *inferenceService, namespace, instance.Name, false, false)).To(Succeed())

// Fetch the updated InferenceService
updated := &kservev1beta1.InferenceService{}
Expect(testReconciler.Client.Get(testCtx, types.NamespacedName{Name: "my-model-http", Namespace: namespace}, updated)).To(Succeed())

Expect(updated.Spec.Predictor.Logger).NotTo(BeNil())
expectedURL := utils.GenerateKServeLoggerURL(instance.Name, namespace)
Expect(*updated.Spec.Predictor.Logger.URL).To(Equal(expectedURL))
Expect(*updated.Spec.Predictor.Logger.URL).To(HavePrefix("http://"))
})
})

Context("When handleInferenceServices processes a Raw deployment without the annotation", func() {
It("should default to HTTPS logger URL", func() {
namespace := "trusty-kserve-raw-default"
Expect(createNamespace(testCtx, testReconciler.Client, namespace)).To(Succeed())

instance := createDefaultPVCCustomResource(namespace)
// No annotation set — default behavior

// Create a Raw deployment InferenceService
infService := &kservev1beta1.InferenceService{
ObjectMeta: metav1.ObjectMeta{
Name: "raw-model",
Namespace: namespace,
Annotations: map[string]string{
"serving.kserve.io/deploymentMode": DEPLOYMENT_MODE_RAW,
},
},
Spec: kservev1beta1.InferenceServiceSpec{
Predictor: kservev1beta1.PredictorSpec{
Model: &kservev1beta1.ModelSpec{
ModelFormat: kservev1beta1.ModelFormat{Name: "sklearn"},
},
},
},
}
Expect(testReconciler.Client.Create(testCtx, infService)).To(Succeed())

_, err := testReconciler.handleInferenceServices(testCtx, instance, namespace, modelMeshLabelKey, modelMeshLabelValue, payloadProcessorName, instance.Name, false)
Expect(err).ToNot(HaveOccurred())

// Fetch updated InferenceService — should have HTTPS URL
updated := &kservev1beta1.InferenceService{}
Expect(testReconciler.Client.Get(testCtx, types.NamespacedName{Name: "raw-model", Namespace: namespace}, updated)).To(Succeed())

Expect(updated.Spec.Predictor.Logger).NotTo(BeNil())
Expect(*updated.Spec.Predictor.Logger.URL).To(HavePrefix("https://"))
})
})

Context("When handleInferenceServices processes a Raw deployment with the HTTP annotation", func() {
It("should use HTTP logger URL", func() {
namespace := "trusty-kserve-raw-http"
Expect(createNamespace(testCtx, testReconciler.Client, namespace)).To(Succeed())

instance := createDefaultPVCCustomResource(namespace)
// Set the HTTP logger annotation on the TrustyAIService CR
instance.Annotations = map[string]string{
kserveLoggerHTTPAnnotationKey: "true",
}

// Create a Raw deployment InferenceService
infService := &kservev1beta1.InferenceService{
ObjectMeta: metav1.ObjectMeta{
Name: "raw-model-http",
Namespace: namespace,
Annotations: map[string]string{
"serving.kserve.io/deploymentMode": DEPLOYMENT_MODE_RAW,
},
},
Spec: kservev1beta1.InferenceServiceSpec{
Predictor: kservev1beta1.PredictorSpec{
Model: &kservev1beta1.ModelSpec{
ModelFormat: kservev1beta1.ModelFormat{Name: "sklearn"},
},
},
},
}
Expect(testReconciler.Client.Create(testCtx, infService)).To(Succeed())

_, err := testReconciler.handleInferenceServices(testCtx, instance, namespace, modelMeshLabelKey, modelMeshLabelValue, payloadProcessorName, instance.Name, false)
Expect(err).ToNot(HaveOccurred())

// Fetch updated InferenceService — should have HTTP URL
updated := &kservev1beta1.InferenceService{}
Expect(testReconciler.Client.Get(testCtx, types.NamespacedName{Name: "raw-model-http", Namespace: namespace}, updated)).To(Succeed())

Expect(updated.Spec.Predictor.Logger).NotTo(BeNil())
Expect(*updated.Spec.Predictor.Logger.URL).To(HavePrefix("http://"))
expectedURL := utils.GenerateKServeLoggerURL(instance.Name, namespace)
Expect(*updated.Spec.Predictor.Logger.URL).To(Equal(expectedURL))
})
})

Context("When handleInferenceServices processes a Raw deployment with annotation set to non-true value", func() {
It("should default to HTTPS logger URL", func() {
namespace := "trusty-kserve-raw-nontrue"
Expect(createNamespace(testCtx, testReconciler.Client, namespace)).To(Succeed())

instance := createDefaultPVCCustomResource(namespace)
// Set annotation to a non-"true" value
instance.Annotations = map[string]string{
kserveLoggerHTTPAnnotationKey: "false",
}

infService := &kservev1beta1.InferenceService{
ObjectMeta: metav1.ObjectMeta{
Name: "raw-model-nontrue",
Namespace: namespace,
Annotations: map[string]string{
"serving.kserve.io/deploymentMode": DEPLOYMENT_MODE_RAW,
},
},
Spec: kservev1beta1.InferenceServiceSpec{
Predictor: kservev1beta1.PredictorSpec{
Model: &kservev1beta1.ModelSpec{
ModelFormat: kservev1beta1.ModelFormat{Name: "sklearn"},
},
},
},
}
Expect(testReconciler.Client.Create(testCtx, infService)).To(Succeed())

_, err := testReconciler.handleInferenceServices(testCtx, instance, namespace, modelMeshLabelKey, modelMeshLabelValue, payloadProcessorName, instance.Name, false)
Expect(err).ToNot(HaveOccurred())

updated := &kservev1beta1.InferenceService{}
Expect(testReconciler.Client.Get(testCtx, types.NamespacedName{Name: "raw-model-nontrue", Namespace: namespace}, updated)).To(Succeed())

Expect(updated.Spec.Predictor.Logger).NotTo(BeNil())
Expect(*updated.Spec.Predictor.Logger.URL).To(HavePrefix("https://"))
})
})
})
Loading