Move modelAdapter runtime validatioin to webhook

Signed-off-by: kerthcet <[email protected]>

pkg/controller/modeladapter/modeladapter_controller.go

@@ -335,20 +335,6 @@ func (r *ModelAdapterReconciler) DoReconcile(ctx context.Context, req ctrl.Reque
 	}
 
 	oldInstance := instance.DeepCopy()
-	// Step 0: Validate ModelAdapter configurations
-	if err := validateModelAdapter(instance); err != nil {
-		klog.Error(err, "Failed to validate the ModelAdapter")
-		instance.Status.Phase = modelv1alpha1.ModelAdapterFailed
-		condition := NewCondition(string(modelv1alpha1.ModelAdapterPending), metav1.ConditionFalse,
-			ValidationFailedReason, "ModelAdapter resource is not valid")
-		// TODO: no need to update the status if the status remain the same
-		if updateErr := r.updateStatus(ctx, instance, condition); updateErr != nil {
-			klog.ErrorS(err, "Failed to update ModelAdapter status", "modelAdapter", klog.KObj(instance))
-			return reconcile.Result{}, updateErr
-		}
-
-		return ctrl.Result{}, err
-	}
 
 	// Step 1: Schedule Pod for ModelAdapter
 	selectedPod := &corev1.Pod{}

pkg/controller/modeladapter/utils.go

@@ -23,46 +23,10 @@ import (
 	"os"
 	"strings"
 
-	modelv1alpha1 "github.com/vllm-project/aibrix/api/model/v1alpha1"
 	"github.com/vllm-project/aibrix/pkg/config"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func validateModelAdapter(instance *modelv1alpha1.ModelAdapter) error {
-	if instance.Spec.ArtifactURL == "" {
-		return fmt.Errorf("artifactURL is required")
-	}
-
-	if instance.Spec.PodSelector == nil {
-		return fmt.Errorf("podSelector is required")
-	}
-
-	if _, err := url.ParseRequestURI(instance.Spec.ArtifactURL); err != nil {
-		return fmt.Errorf("artifactURL is not a valid URL: %v", err)
-	}
-
-	if err := validateArtifactURL(instance.Spec.ArtifactURL); err != nil {
-		return err
-	}
-
-	if instance.Spec.Replicas != nil && *instance.Spec.Replicas <= 0 {
-		return fmt.Errorf("replicas must be greater than 0")
-	}
-
-	return nil
-}
-
-// validateArtifactURL checks if the ArtifactURL has a valid schema (s3://, gcs://, huggingface://, https://, /)
-func validateArtifactURL(artifactURL string) error {
-	allowedSchemes := []string{"s3://", "gcs://", "huggingface://", "hf://", "/"}
-	for _, scheme := range allowedSchemes {
-		if strings.HasPrefix(artifactURL, scheme) {
-			return nil
-		}
-	}
-	return fmt.Errorf("artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://")
-}
-
 func equalStringSlices(a, b []string) bool {
 	if len(a) != len(b) {
 		return false

pkg/controller/modeladapter/utils_test.go

@@ -22,119 +22,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/utils/ptr"
 
-	modelv1alpha1 "github.com/vllm-project/aibrix/api/model/v1alpha1"
 	"github.com/vllm-project/aibrix/pkg/config"
 )
 
-// Test for validateModelAdapter function
-func TestValidateModelAdapter(t *testing.T) {
-	// Case 1: All valid fields
-	t.Run("valid input", func(t *testing.T) {
-		instance := &modelv1alpha1.ModelAdapter{
-			Spec: modelv1alpha1.ModelAdapterSpec{
-				ArtifactURL: "s3://bucket/path/to/artifact",
-				PodSelector: &metav1.LabelSelector{
-					MatchLabels: map[string]string{"app": "test"},
-				},
-				Replicas: ptr.To(int32(1)),
-			},
-		}
-
-		err := validateModelAdapter(instance)
-		assert.NoError(t, err)
-	})
-
-	// Case 2: Missing ArtifactURL
-	t.Run("missing ArtifactURL", func(t *testing.T) {
-		instance := &modelv1alpha1.ModelAdapter{
-			Spec: modelv1alpha1.ModelAdapterSpec{
-				ArtifactURL: "",
-				PodSelector: &metav1.LabelSelector{
-					MatchLabels: map[string]string{"app": "test"},
-				},
-			},
-		}
-
-		err := validateModelAdapter(instance)
-		assert.EqualError(t, err, "artifactURL is required")
-	})
-
-	// Case 3: Invalid ArtifactURL format
-	t.Run("invalid ArtifactURL", func(t *testing.T) {
-		instance := &modelv1alpha1.ModelAdapter{
-			Spec: modelv1alpha1.ModelAdapterSpec{
-				ArtifactURL: "ftp://bucket/path/to/artifact",
-				PodSelector: &metav1.LabelSelector{
-					MatchLabels: map[string]string{"app": "test"},
-				},
-			},
-		}
-
-		err := validateModelAdapter(instance)
-		assert.EqualError(t, err, "artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://")
-	})
-
-	// Case 4: Missing PodSelector
-	t.Run("missing PodSelector", func(t *testing.T) {
-		instance := &modelv1alpha1.ModelAdapter{
-			Spec: modelv1alpha1.ModelAdapterSpec{
-				ArtifactURL: "s3://bucket/path/to/artifact",
-				PodSelector: nil,
-			},
-		}
-
-		err := validateModelAdapter(instance)
-		assert.EqualError(t, err, "podSelector is required")
-	})
-
-	// Case 5: Invalid Replicas
-	t.Run("invalid Replicas", func(t *testing.T) {
-		replicas := int32(0)
-		instance := &modelv1alpha1.ModelAdapter{
-			Spec: modelv1alpha1.ModelAdapterSpec{
-				ArtifactURL: "s3://bucket/path/to/artifact",
-				PodSelector: &metav1.LabelSelector{
-					MatchLabels: map[string]string{"app": "test"},
-				},
-				Replicas: &replicas,
-			},
-		}
-
-		err := validateModelAdapter(instance)
-		assert.EqualError(t, err, "replicas must be greater than 0")
-	})
-}
-
-// Test for validateArtifactURL function
-func TestValidateArtifactURL(t *testing.T) {
-	// Case 1: Valid s3 URL
-	t.Run("valid s3 URL", func(t *testing.T) {
-		err := validateArtifactURL("s3://bucket/path")
-		assert.NoError(t, err)
-	})
-
-	// Case 2: Valid gcs URL
-	t.Run("valid gcs URL", func(t *testing.T) {
-		err := validateArtifactURL("gcs://bucket/path")
-		assert.NoError(t, err)
-	})
-
-	// Case 3: Valid huggingface URL
-	t.Run("valid huggingface URL", func(t *testing.T) {
-		err := validateArtifactURL("huggingface://path/to/model")
-		assert.NoError(t, err)
-	})
-
-	// Case 4: Invalid scheme
-	t.Run("invalid scheme", func(t *testing.T) {
-		err := validateArtifactURL("ftp://bucket/path")
-		assert.EqualError(t, err, "artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://")
-	})
-}
-
 // Test for equalStringSlices function
 func TestEqualStringSlices(t *testing.T) {
 	// Case 1: Equal slices

pkg/utils/modeladapter.go

@@ -0,0 +1,34 @@
+/*
+Copyright 2025 The Aibrix Team.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package utils
+
+import (
+	"fmt"
+	"strings"
+)
+
+var AllowedSchemas = []string{"s3://", "gcs://", "huggingface://", "hf://", "/"}
+
+// ValidateArtifactURL checks if the ArtifactURL has a valid schema (s3://, gcs://, huggingface://, https://, /)
+func ValidateArtifactURL(artifactURL string) error {
+	for _, schema := range AllowedSchemas {
+		if strings.HasPrefix(artifactURL, schema) {
+			return nil
+		}
+	}
+	return fmt.Errorf("unsupported schema")
+}

pkg/utils/modeladapter_test.go

@@ -0,0 +1,34 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// Test for ValidateArtifactURL function
+func TestValidateArtifactURL(t *testing.T) {
+	// Case 1: Valid s3 URL
+	t.Run("valid s3 URL", func(t *testing.T) {
+		err := ValidateArtifactURL("s3://bucket/path")
+		assert.NoError(t, err)
+	})
+
+	// Case 2: Valid gcs URL
+	t.Run("valid gcs URL", func(t *testing.T) {
+		err := ValidateArtifactURL("gcs://bucket/path")
+		assert.NoError(t, err)
+	})
+
+	// Case 3: Valid huggingface URL
+	t.Run("valid huggingface URL", func(t *testing.T) {
+		err := ValidateArtifactURL("huggingface://path/to/model")
+		assert.NoError(t, err)
+	})
+
+	// Case 4: Invalid scheme
+	t.Run("invalid scheme", func(t *testing.T) {
+		err := ValidateArtifactURL("ftp://bucket/path")
+		assert.EqualError(t, err, "artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://")
+	})
+}

pkg/webhook/modeladapter_webhook.go

@@ -28,6 +28,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	modelapi "github.com/vllm-project/aibrix/api/model/v1alpha1"
+	"github.com/vllm-project/aibrix/pkg/utils"
 )
 
 type ModelAdapterWebhook struct{}
@@ -65,6 +66,14 @@ func (w *ModelAdapterWebhook) ValidateCreate(ctx context.Context, obj runtime.Ob
 		allErrs = append(allErrs, field.Invalid(specPath.Child("artifactURL"), adapter.Spec.ArtifactURL, fmt.Sprintf("artifactURL is invalid: %v", err)))
 	}
 
+	if adapter.Spec.Replicas != nil && *adapter.Spec.Replicas <= 0 {
+		allErrs = append(allErrs, field.Invalid(specPath.Child("replicas"), adapter.Spec.Replicas, "replicas must be greater than 0"))
+	}
+
+	if err := utils.ValidateArtifactURL(adapter.Spec.ArtifactURL); err != nil {
+		allErrs = append(allErrs, field.NotSupported(specPath.Child("artifactURL"), adapter.Spec.ArtifactURL, utils.AllowedSchemas))
+	}
+
 	return nil, allErrs.ToAggregate()
 }
 

test/integration/webhook/modeladapter_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
 
 	modelapi "github.com/vllm-project/aibrix/api/model/v1alpha1"
 )
@@ -60,6 +61,22 @@ var _ = ginkgo.Describe("modelAdapter default and validation", func() {
 				gomega.Expect(k8sClient.Create(ctx, tc.adapter())).To(gomega.Succeed())
 			}
 		},
+		ginkgo.Entry("normal creation", &testValidatingCase{
+			adapter: func() *modelapi.ModelAdapter {
+				adapter := modelapi.ModelAdapter{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-adapter",
+						Namespace: ns.Name,
+					},
+					Spec: modelapi.ModelAdapterSpec{
+						ArtifactURL: "s3://test-bucket/test-model",
+						PodSelector: &metav1.LabelSelector{},
+					},
+				}
+				return &adapter
+			},
+			failed: false,
+		}),
 		ginkgo.Entry("adapter creation with invalid artifactURL should be failed", &testValidatingCase{
 			adapter: func() *modelapi.ModelAdapter {
 				adapter := modelapi.ModelAdapter{
@@ -76,5 +93,70 @@ var _ = ginkgo.Describe("modelAdapter default and validation", func() {
 			},
 			failed: true,
 		}),
+		ginkgo.Entry("adapter creation with empty artifactURL should be failed", &testValidatingCase{
+			adapter: func() *modelapi.ModelAdapter {
+				adapter := modelapi.ModelAdapter{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-adapter",
+						Namespace: ns.Name,
+					},
+					Spec: modelapi.ModelAdapterSpec{
+						ArtifactURL: "",
+						PodSelector: &metav1.LabelSelector{},
+					},
+				}
+				return &adapter
+			},
+			failed: true,
+		}),
+		ginkgo.Entry("adapter creation with empty podSelector should be failed", &testValidatingCase{
+			adapter: func() *modelapi.ModelAdapter {
+				adapter := modelapi.ModelAdapter{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-adapter",
+						Namespace: ns.Name,
+					},
+					Spec: modelapi.ModelAdapterSpec{
+						ArtifactURL: "s3://test-bucket/test-model",
+						PodSelector: &metav1.LabelSelector{},
+						Replicas:    ptr.To[int32](0),
+					},
+				}
+				return &adapter
+			},
+			failed: true,
+		}),
+		ginkgo.Entry("adapter creation with replicas less than 0 should be failed", &testValidatingCase{
+			adapter: func() *modelapi.ModelAdapter {
+				adapter := modelapi.ModelAdapter{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-adapter",
+						Namespace: ns.Name,
+					},
+					Spec: modelapi.ModelAdapterSpec{
+						ArtifactURL: "s3://test-bucket/test-model",
+						PodSelector: nil,
+					},
+				}
+				return &adapter
+			},
+			failed: true,
+		}),
+		ginkgo.Entry("adapter creation with unsupported schema should be failed", &testValidatingCase{
+			adapter: func() *modelapi.ModelAdapter {
+				adapter := modelapi.ModelAdapter{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-adapter",
+						Namespace: ns.Name,
+					},
+					Spec: modelapi.ModelAdapterSpec{
+						ArtifactURL: "ms://",
+						PodSelector: &metav1.LabelSelector{},
+					},
+				}
+				return &adapter
+			},
+			failed: true,
+		}),
 	)
 })