Skip to content

Commit

Permalink
Fix validation for custom storageUri (kubeflow#3134)
Browse files Browse the repository at this point in the history 
* Move storage uri check from validator to pod mutator

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

* Move storage uri validation to controller

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

* Fix test

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

* Fix tests, add comments, and remove model transition status

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

---------

Signed-off-by: Jin Dong <jdong183@bloomberg.net>
  • Loading branch information
@greenmoon55
greenmoon55 authored Sep 22, 2023
1 parent 8231072 commit a6af401
Show file tree
Hide file tree
Showing 31 changed files with 165 additions and 473 deletions.
29 changes: 0 additions & 29 deletions pkg/apis/serving/v1beta1/component.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package v1beta1
import (
"fmt"
"reflect"
"regexp"
"strconv"
"strings"

Expand All @@ -46,10 +45,7 @@ const (

// Constants
var (
SupportedStorageURIPrefixList = []string{"gs://", "s3://", "pvc://", "file://", "https://", "http://", "hdfs://", "webhdfs://"}
SupportedStorageSpecURIPrefixList = []string{"s3://", "hdfs://", "webhdfs://"}
AzureBlobURL = "blob.core.windows.net"
AzureBlobURIRegEx = "https://(.+?).blob.core.windows.net/(.+)"
)

// ComponentImplementation interface is implemented by predictor, transformer, and explainer implementations
Expand Down Expand Up @@ -163,31 +159,6 @@ func validateStorageSpec(storageSpec *StorageSpec, storageURI *string) error {
return nil
}

func validateStorageURI(storageURI *string) error {
if storageURI == nil {
return nil
}

// local path (not some protocol?)
if !regexp.MustCompile("\\w+?://").MatchString(*storageURI) {
return nil
}

// need to verify Azure Blob first, because it uses http(s):// prefix
if strings.Contains(*storageURI, AzureBlobURL) {
azureURIMatcher := regexp.MustCompile(AzureBlobURIRegEx)
if parts := azureURIMatcher.FindStringSubmatch(*storageURI); parts != nil {
return nil
}
} else {
if utils.IsPrefixSupported(*storageURI, SupportedStorageURIPrefixList) {
return nil
}
}

return fmt.Errorf(UnsupportedStorageURIFormatError, strings.Join(SupportedStorageURIPrefixList, ", "), *storageURI)
}

func validateReplicas(minReplicas *int, maxReplicas int) error {
if minReplicas == nil {
minReplicas = &constants.DefaultMinReplicas
Expand Down
1 change: 0 additions & 1 deletion pkg/apis/serving/v1beta1/explainer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ var _ Component = &ExplainerSpec{}
// Validate returns an error if invalid
func (e *ExplainerExtensionSpec) Validate() error {
return utils.FirstNonNilError([]error{
validateStorageURI(e.GetStorageUri()),
validateStorageSpec(e.GetStorageSpec(), e.GetStorageUri()),
})
}
Expand Down
21 changes: 0 additions & 21 deletions pkg/apis/serving/v1beta1/explainer_alibi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,27 +54,6 @@ func TestAlibiValidation(t *testing.T) {
},
matcher: gomega.BeNil(),
},
"ValidStorageUri": {
spec: ExplainerSpec{
Alibi: &AlibiExplainerSpec{
Type: "AnchorTabular",
ExplainerExtensionSpec: ExplainerExtensionSpec{
StorageURI: "s3://modelzoo",
},
},
},
matcher: gomega.BeNil(),
},
"InvalidStorageUri": {
spec: ExplainerSpec{
Alibi: &AlibiExplainerSpec{
ExplainerExtensionSpec: ExplainerExtensionSpec{
StorageURI: "invaliduri://modelzoo",
},
},
},
matcher: gomega.Not(gomega.BeNil()),
},
}

for name, scenario := range scenarios {
Expand Down
4 changes: 1 addition & 3 deletions pkg/apis/serving/v1beta1/explainer_custom.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ func NewCustomExplainer(podSpec *PodSpec) *CustomExplainer {

// Validate the spec
func (s *CustomExplainer) Validate() error {
return utils.FirstNonNilError([]error{
validateStorageURI(s.GetStorageUri()),
})
return utils.FirstNonNilError([]error{})
}

// Default sets defaults on the resource
Expand Down
58 changes: 0 additions & 58 deletions pkg/apis/serving/v1beta1/explainer_custom_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,69 +23,11 @@ import (
"github.com/golang/protobuf/proto"
"github.com/kserve/kserve/pkg/constants"
"github.com/onsi/gomega"
"github.com/onsi/gomega/types"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestCustomExplainerValidation(t *testing.T) {
g := gomega.NewGomegaWithT(t)
config := InferenceServicesConfig{
Explainers: ExplainersConfig{},
}
scenarios := map[string]struct {
spec ExplainerSpec
matcher types.GomegaMatcher
}{
"ValidStorageUri": {
spec: ExplainerSpec{
PodSpec: PodSpec{
Containers: []v1.Container{
{
Env: []v1.EnvVar{
{
Name: "STORAGE_URI",
Value: "s3://modelzoo",
},
},
},
},
},
},
matcher: gomega.BeNil(),
},
"InvalidStorageUri": {
spec: ExplainerSpec{
PodSpec: PodSpec{
Containers: []v1.Container{
{
Env: []v1.EnvVar{
{
Name: "STORAGE_URI",
Value: "invaliduri://modelzoo",
},
},
},
},
},
},
matcher: gomega.Not(gomega.BeNil()),
},
}

for name, scenario := range scenarios {
t.Run(name, func(t *testing.T) {
explainer := CustomExplainer{PodSpec: v1.PodSpec(scenario.spec.PodSpec)}
explainer.Default(&config)
res := explainer.Validate()
if !g.Expect(res).To(scenario.matcher) {
t.Errorf("got %q, want %q", res, scenario.matcher)
}
})
}
}

func TestCustomExplainerDefaulter(t *testing.T) {
g := gomega.NewGomegaWithT(t)
config := InferenceServicesConfig{
Expand Down
72 changes: 0 additions & 72 deletions pkg/apis/serving/v1beta1/inference_service_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,78 +116,6 @@ func TestInvalidAutoscalerHPAMetrics(t *testing.T) {
g.Expect(isvc.ValidateCreate()).ShouldNot(gomega.Succeed())
}

func TestValidStorageURIPrefixOK(t *testing.T) {
g := gomega.NewGomegaWithT(t)
for _, prefix := range SupportedStorageURIPrefixList {
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String(prefix + "foo/bar")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
}
}

func TestEmptyStorageURIPrefixOK(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
}

func TestLocalPathStorageURIPrefixOK(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("some/relative/path")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("/some/absolute/path")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("/")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("foo")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
}

func TestAzureBlobOK(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://kfserving.blob.core.windows.net/triton/simple_string/")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://kfserving.blob.core.windows.net/triton/simple_string")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://kfserving.blob.core.windows.net/triton/")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://kfserving.blob.core.windows.net/triton")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
}

func TestAzureBlobNoAccountFails(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://blob.core.windows.net/triton/simple_string/")
g.Expect(isvc.ValidateCreate()).ShouldNot(gomega.Succeed())
}

func TestAzureBlobNoContainerFails(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://foo.blob.core.windows.net/")
g.Expect(isvc.ValidateCreate()).ShouldNot(gomega.Succeed())
}

func TestHttpStorageURIPrefixOK(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("https://raw.githubusercontent.com/someOrg/someRepo/model.tar.gz")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("http://raw.githubusercontent.com/someOrg/someRepo/model.tar.gz")
g.Expect(isvc.ValidateCreate()).Should(gomega.Succeed())
}

func TestUnknownStorageURIPrefixFails(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
isvc.Spec.Predictor.Tensorflow.StorageURI = proto.String("blob://foo/bar")
g.Expect(isvc.ValidateCreate()).ShouldNot(gomega.Succeed())
}

func TestRejectMultipleModelSpecs(t *testing.T) {
g := gomega.NewGomegaWithT(t)
isvc := makeTestInferenceService()
Expand Down
1 change: 0 additions & 1 deletion pkg/apis/serving/v1beta1/predictor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ func (s *PredictorSpec) GetExtensions() *ComponentExtensionSpec {
// Validate returns an error if invalid
func (p *PredictorExtensionSpec) Validate() error {
return utils.FirstNonNilError([]error{
validateStorageURI(p.GetStorageUri()),
// TODO: Re-enable storage spec validation once azure/gcs are supported.
// Enabling this currently prevents those storage types from working with ModelMesh.
// validateStorageSpec(p.GetStorageSpec(), p.GetStorageUri()),
Expand Down
1 change: 0 additions & 1 deletion pkg/apis/serving/v1beta1/predictor_custom.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ func NewCustomPredictor(podSpec *PodSpec) *CustomPredictor {
// Validate returns an error if invalid
func (c *CustomPredictor) Validate() error {
return utils.FirstNonNilError([]error{
validateStorageURI(c.GetStorageUri()),
c.validateCustomProtocol(),
})
}
Expand Down
34 changes: 0 additions & 34 deletions pkg/apis/serving/v1beta1/predictor_custom_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,40 +35,6 @@ func TestCustomPredictorValidation(t *testing.T) {
spec PredictorSpec
matcher types.GomegaMatcher
}{
"ValidStorageUri": {
spec: PredictorSpec{
PodSpec: PodSpec{
Containers: []v1.Container{
{
Env: []v1.EnvVar{
{
Name: "STORAGE_URI",
Value: "s3://modelzoo",
},
},
},
},
},
},
matcher: gomega.BeNil(),
},
"InvalidStorageUri": {
spec: PredictorSpec{
PodSpec: PodSpec{
Containers: []v1.Container{
{
Env: []v1.EnvVar{
{
Name: "STORAGE_URI",
Value: "invaliduri://modelzoo",
},
},
},
},
},
},
matcher: gomega.Not(gomega.BeNil()),
},
"ValidProtocolV1": {
spec: PredictorSpec{
ComponentExtensionSpec: ComponentExtensionSpec{
Expand Down
20 changes: 0 additions & 20 deletions pkg/apis/serving/v1beta1/predictor_lightgbm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,26 +46,6 @@ func TestLightGBMValidation(t *testing.T) {
},
matcher: gomega.BeNil(),
},
"ValidStorageUri": {
spec: PredictorSpec{
LightGBM: &LightGBMSpec{
PredictorExtensionSpec: PredictorExtensionSpec{
StorageURI: proto.String("s3://modelzoo"),
},
},
},
matcher: gomega.BeNil(),
},
"InvalidStorageUri": {
spec: PredictorSpec{
LightGBM: &LightGBMSpec{
PredictorExtensionSpec: PredictorExtensionSpec{
StorageURI: proto.String("invaliduri://modelzoo"),
},
},
},
matcher: gomega.Not(gomega.BeNil()),
},
}

for name, scenario := range scenarios {
Expand Down
1 change: 0 additions & 1 deletion pkg/apis/serving/v1beta1/predictor_onnxruntime.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ func (o *ONNXRuntimeSpec) Validate() error {
}

return utils.FirstNonNilError([]error{
validateStorageURI(o.GetStorageUri()),
validateStorageSpec(o.GetStorageSpec(), o.GetStorageUri()),
})
}
Expand Down
23 changes: 2 additions & 21 deletions pkg/apis/serving/v1beta1/predictor_onnxruntime_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ limitations under the License.
package v1beta1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/golang/protobuf/proto"
"github.com/kserve/kserve/pkg/constants"
"github.com/onsi/gomega"
Expand All @@ -45,26 +46,6 @@ func TestOnnxRuntimeValidation(t *testing.T) {
},
matcher: gomega.BeNil(),
},
"ValidStorageUri": {
spec: PredictorSpec{
ONNX: &ONNXRuntimeSpec{
PredictorExtensionSpec: PredictorExtensionSpec{
StorageURI: proto.String("s3://modelzoo"),
},
},
},
matcher: gomega.BeNil(),
},
"InvalidStorageUri": {
spec: PredictorSpec{
ONNX: &ONNXRuntimeSpec{
PredictorExtensionSpec: PredictorExtensionSpec{
StorageURI: proto.String("invaliduri://modelzoo"),
},
},
},
matcher: gomega.Not(gomega.BeNil()),
},
"ValidModelExtension": {
spec: PredictorSpec{
ONNX: &ONNXRuntimeSpec{
Expand Down
Loading

0 comments on commit a6af401

Please sign in to comment.