From 2a69143b8b18ddb86a99d2f3ab704aad002d4e73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edgar=20Hern=C3=A1ndez?= Date: Fri, 10 Jan 2025 11:54:52 -0600 Subject: [PATCH] Automatically inject expected ODH annotations to InferenceGraph and InferenceServices (#339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Implementation of ODH defaulters for InferenceGraph and InferenceService On creation of InferenceGraph or InferenceService resources, the following default annotations will be added: * `serving.knative.openshift.io/enablePassthrough: true` * `sidecar.istio.io/inject: true` * `sidecar.istio.io/rewriteAppHTTPProbers: true` The annotations are added only for Serverless mode, and only if they are missing. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> * Feedback: Filippe Extract "ENABLE_WEBHOOKS" string to constant Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> --------- Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> --- Dockerfile | 1 + PROJECT | 10 ++ cmd/main.go | 18 ++- config/webhook/kustomization.yaml | 58 +++++-- config/webhook/manifests.yaml | 44 +++++ internal/controller/constants/constants.go | 8 +- .../serving/inferenceservice_controller.go | 4 +- .../kserve_raw_inferenceservice_reconciler.go | 2 +- ..._serverless_inferenceservice_reconciler.go | 2 +- .../mm_inferenceservice_reconciler.go | 2 +- internal/controller/utils/utils.go | 16 +- internal/webhook/serving/defaulters.go | 51 ++++++ .../v1alpha1/inferencegraph_webhook.go | 73 +++++++++ .../v1alpha1/inferencegraph_webhook_test.go | 111 +++++++++++++ .../serving/v1alpha1/webhook_suite_test.go | 151 ++++++++++++++++++ .../v1beta1/inferenceservice_webhook.go | 33 ++++ .../v1beta1/inferenceservice_webhook_test.go | 93 ++++++++++- test/e2e/e2e_test.go | 14 ++ 18 files changed, 650 insertions(+), 41 deletions(-) create mode 120000 Dockerfile create mode 100644 internal/webhook/serving/defaulters.go create mode 100644 internal/webhook/serving/v1alpha1/inferencegraph_webhook.go create mode 100644 internal/webhook/serving/v1alpha1/inferencegraph_webhook_test.go create mode 100644 internal/webhook/serving/v1alpha1/webhook_suite_test.go diff --git a/Dockerfile b/Dockerfile new file mode 120000 index 00000000..5240dc01 --- /dev/null +++ b/Dockerfile @@ -0,0 +1 @@ +Containerfile \ No newline at end of file diff --git a/PROJECT b/PROJECT index 63560ddb..1abb7ec4 100644 --- a/PROJECT +++ b/PROJECT @@ -30,6 +30,7 @@ resources: version: v1beta1 webhooks: validation: true + defaulting: true webhookVersion: v1 - controller: true core: true @@ -59,4 +60,13 @@ resources: webhooks: validation: true webhookVersion: v1 +- domain: kserve.io + external: true + group: serving + kind: InferenceGraph + path: github.com/kserve/kserve/pkg/apis/serving/v1alpha1 + version: v1alpha1 + webhooks: + defaulting: true + webhookVersion: v1 version: "3" diff --git a/cmd/main.go b/cmd/main.go index 7991b43f..e46936ad 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -51,11 +51,16 @@ import ( "github.com/opendatahub-io/odh-model-controller/internal/controller/utils" webhooknimv1 "github.com/opendatahub-io/odh-model-controller/internal/webhook/nim/v1" webhookservingv1 "github.com/opendatahub-io/odh-model-controller/internal/webhook/serving/v1" + webhookservingv1alpha1 "github.com/opendatahub-io/odh-model-controller/internal/webhook/serving/v1alpha1" webhookservingv1beta1 "github.com/opendatahub-io/odh-model-controller/internal/webhook/serving/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" // +kubebuilder:scaffold:imports ) +const ( + enableWebhooksEnv = "ENABLE_WEBHOOKS" +) + var ( scheme = runtime.NewScheme() setupLog = ctrl.Log.WithName("setup") @@ -289,14 +294,14 @@ func main() { } // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { + if os.Getenv(enableWebhooksEnv) != "false" { if err = webhooknimv1.SetupAccountWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "NIMAccount") os.Exit(1) } } // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { + if os.Getenv(enableWebhooksEnv) != "false" { if kserveWithMeshEnabled { if err = webhookservingv1.SetupServiceWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Knative Service") @@ -308,12 +313,19 @@ func main() { } } // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { + if os.Getenv(enableWebhooksEnv) != "false" { if err = webhookservingv1beta1.SetupInferenceServiceWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "InferenceService") os.Exit(1) } } + // nolint:goconst + if os.Getenv(enableWebhooksEnv) != "false" { + if err = webhookservingv1alpha1.SetupInferenceGraphWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "InferenceGraph") + os.Exit(1) + } + } // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml index 5f5d3f74..5ef35260 100644 --- a/config/webhook/kustomization.yaml +++ b/config/webhook/kustomization.yaml @@ -11,27 +11,44 @@ patches: annotations: service.beta.openshift.io/inject-cabundle: true webhooks: - - name: validating.ksvc.odh-model-controller.opendatahub.io - clientConfig: - service: - name: odh-model-controller-webhook-service - objectSelector: - matchExpressions: - - key: serving.kserve.io/inferenceservice - operator: Exists - - name: validating.nim.account.odh-model-controller.opendatahub.io - clientConfig: - service: - name: odh-model-controller-webhook-service - - name: validating.isvc.odh-model-controller.opendatahub.io - clientConfig: - service: - name: odh-model-controller-webhook-service + - name: validating.ksvc.odh-model-controller.opendatahub.io + clientConfig: + service: + name: odh-model-controller-webhook-service + objectSelector: + matchExpressions: + - key: serving.kserve.io/inferenceservice + operator: Exists + - name: validating.nim.account.odh-model-controller.opendatahub.io + clientConfig: + service: + name: odh-model-controller-webhook-service + - name: validating.isvc.odh-model-controller.opendatahub.io + clientConfig: + service: + name: odh-model-controller-webhook-service target: group: admissionregistration.k8s.io kind: ValidatingWebhookConfiguration name: validating-webhook-configuration version: v1 +- patch: |- + apiVersion: admissionregistration.k8s.io/v1 + kind: MutatingWebhookConfiguration + metadata: + name: mutating-webhook-configuration + annotations: + service.beta.openshift.io/inject-cabundle: true + webhooks: + - name: minferencegraph-v1alpha1.odh-model-controller.opendatahub.io + clientConfig: + service: + name: odh-model-controller-webhook-service + - name: minferenceservice-v1beta1.odh-model-controller.opendatahub.io + clientConfig: + service: + name: odh-model-controller-webhook-service + - patch: |- - op: replace path: /metadata/name @@ -41,6 +58,15 @@ patches: kind: ValidatingWebhookConfiguration name: validating-webhook-configuration version: v1 +- patch: |- + - op: replace + path: /metadata/name + value: mutating.odh-model-controller.opendatahub.io + target: + group: admissionregistration.k8s.io + kind: MutatingWebhookConfiguration + name: mutating-webhook-configuration + version: v1 configurations: - kustomizeconfig.yaml diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 6bf3b9f5..f8fbfeca 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -1,5 +1,49 @@ --- apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + name: mutating-webhook-configuration +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-serving-kserve-io-v1alpha1-inferencegraph + failurePolicy: Fail + name: minferencegraph-v1alpha1.odh-model-controller.opendatahub.io + rules: + - apiGroups: + - serving.kserve.io + apiVersions: + - v1alpha1 + operations: + - CREATE + resources: + - inferencegraphs + sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-serving-kserve-io-v1beta1-inferenceservice + failurePolicy: Fail + name: minferenceservice-v1beta1.odh-model-controller.opendatahub.io + rules: + - apiGroups: + - serving.kserve.io + apiVersions: + - v1beta1 + operations: + - CREATE + resources: + - inferenceservices + sideEffects: None +--- +apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration diff --git a/internal/controller/constants/constants.go b/internal/controller/constants/constants.go index a3c090a4..bfff519a 100644 --- a/internal/controller/constants/constants.go +++ b/internal/controller/constants/constants.go @@ -15,7 +15,7 @@ limitations under the License. package constants -type IsvcDeploymentMode string +type KServeDeploymentMode string const ( InferenceServiceKind = "InferenceService" @@ -43,9 +43,9 @@ const ( // isvc modes var ( - Serverless IsvcDeploymentMode = "Serverless" - RawDeployment IsvcDeploymentMode = "RawDeployment" - ModelMesh IsvcDeploymentMode = "ModelMesh" + Serverless KServeDeploymentMode = "Serverless" + RawDeployment KServeDeploymentMode = "RawDeployment" + ModelMesh KServeDeploymentMode = "ModelMesh" ) // model registry diff --git a/internal/controller/serving/inferenceservice_controller.go b/internal/controller/serving/inferenceservice_controller.go index afb8de8c..fbcccabd 100644 --- a/internal/controller/serving/inferenceservice_controller.go +++ b/internal/controller/serving/inferenceservice_controller.go @@ -138,7 +138,7 @@ func (r *InferenceServiceReconciler) ReconcileServing(ctx context.Context, req c } // Check what deployment mode is used by the InferenceService. We have differing reconciliation logic for Kserve and ModelMesh - IsvcDeploymentMode, err := utils.GetDeploymentModeForIsvc(ctx, r.Client, isvc) + IsvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.Client, isvc.GetAnnotations()) if err != nil { return ctrl.Result{}, err } @@ -259,7 +259,7 @@ func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { func (r *InferenceServiceReconciler) onDeletion(ctx context.Context, log logr.Logger, inferenceService *kservev1beta1.InferenceService) error { log.V(1).Info("Running cleanup logic") - IsvcDeploymentMode, err := utils.GetDeploymentModeForIsvc(ctx, r.Client, inferenceService) + IsvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.Client, inferenceService.GetAnnotations()) if err != nil { log.V(1).Error(err, "Could not determine deployment mode for ISVC. Some resources related to the inferenceservice might not be deleted.") } diff --git a/internal/controller/serving/reconcilers/kserve_raw_inferenceservice_reconciler.go b/internal/controller/serving/reconcilers/kserve_raw_inferenceservice_reconciler.go index f7bf4161..4175e9f0 100644 --- a/internal/controller/serving/reconcilers/kserve_raw_inferenceservice_reconciler.go +++ b/internal/controller/serving/reconcilers/kserve_raw_inferenceservice_reconciler.go @@ -73,7 +73,7 @@ func (r *KserveRawInferenceServiceReconciler) CleanupNamespaceIfNoKserveIsvcExis for i := len(inferenceServiceList.Items) - 1; i >= 0; i-- { inferenceService := inferenceServiceList.Items[i] - isvcDeploymentMode, err := utils.GetDeploymentModeForIsvc(ctx, r.client, &inferenceService) + isvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.client, inferenceService.GetAnnotations()) if err != nil { return err } diff --git a/internal/controller/serving/reconcilers/kserve_serverless_inferenceservice_reconciler.go b/internal/controller/serving/reconcilers/kserve_serverless_inferenceservice_reconciler.go index 34585512..51d00215 100644 --- a/internal/controller/serving/reconcilers/kserve_serverless_inferenceservice_reconciler.go +++ b/internal/controller/serving/reconcilers/kserve_serverless_inferenceservice_reconciler.go @@ -86,7 +86,7 @@ func (r *KserveServerlessInferenceServiceReconciler) CleanupNamespaceIfNoKserveI for i := len(inferenceServiceList.Items) - 1; i >= 0; i-- { inferenceService := inferenceServiceList.Items[i] - isvcDeploymentMode, err := utils.GetDeploymentModeForIsvc(ctx, r.client, &inferenceService) + isvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.client, inferenceService.GetAnnotations()) if err != nil { return err } diff --git a/internal/controller/serving/reconcilers/mm_inferenceservice_reconciler.go b/internal/controller/serving/reconcilers/mm_inferenceservice_reconciler.go index 0d6a4ad3..bc3d6b6d 100644 --- a/internal/controller/serving/reconcilers/mm_inferenceservice_reconciler.go +++ b/internal/controller/serving/reconcilers/mm_inferenceservice_reconciler.go @@ -62,7 +62,7 @@ func (r *ModelMeshInferenceServiceReconciler) DeleteModelMeshResourcesIfNoMMIsvc for i := len(inferenceServiceList.Items) - 1; i >= 0; i-- { inferenceService := inferenceServiceList.Items[i] - isvcDeploymentMode, err := utils.GetDeploymentModeForIsvc(ctx, r.client, &inferenceService) + isvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.client, inferenceService.GetAnnotations()) if err != nil { return err } diff --git a/internal/controller/utils/utils.go b/internal/controller/utils/utils.go index 154cbe54..38eee067 100644 --- a/internal/controller/utils/utils.go +++ b/internal/controller/utils/utils.go @@ -38,15 +38,15 @@ var ( ) const ( - inferenceServiceDeploymentModeAnnotation = "serving.kserve.io/deploymentMode" - KserveConfigMapName = "inferenceservice-config" - KServeWithServiceMeshComponent = "kserve-service-mesh" + KServeDeploymentModeAnnotation = "serving.kserve.io/deploymentMode" + KserveConfigMapName = "inferenceservice-config" + KServeWithServiceMeshComponent = "kserve-service-mesh" ) -func GetDeploymentModeForIsvc(ctx context.Context, cli client.Client, isvc *kservev1beta1.InferenceService) (constants.IsvcDeploymentMode, error) { +func GetDeploymentModeForKServeResource(ctx context.Context, cli client.Client, annotations map[string]string) (constants.KServeDeploymentMode, error) { - // If ISVC specifically sets deployment mode using an annotation, return bool depending on value - value, exists := isvc.Annotations[inferenceServiceDeploymentModeAnnotation] + // If explicitly sets deployment mode using an annotation, return bool depending on value + value, exists := annotations[KServeDeploymentModeAnnotation] if exists { switch value { case string(constants.ModelMesh): @@ -56,10 +56,10 @@ func GetDeploymentModeForIsvc(ctx context.Context, cli client.Client, isvc *kser case string(constants.RawDeployment): return constants.RawDeployment, nil default: - return "", fmt.Errorf("the deployment mode '%s' of the Inference Service is invalid", value) + return "", fmt.Errorf("the deployment mode '%s' of the KServe resource is invalid", value) } } else { - // ISVC does not specifically set deployment mode using an annotation, determine the default from configmap + // There is no explicit deployment mode using an annotation, determine the default from configmap controllerNs := os.Getenv("POD_NAMESPACE") inferenceServiceConfigMap := &corev1.ConfigMap{} err := cli.Get(ctx, client.ObjectKey{ diff --git a/internal/webhook/serving/defaulters.go b/internal/webhook/serving/defaulters.go new file mode 100644 index 00000000..e4ae582d --- /dev/null +++ b/internal/webhook/serving/defaulters.go @@ -0,0 +1,51 @@ +package serving + +import ( + "context" + "fmt" + "strings" + + "github.com/go-logr/logr" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/opendatahub-io/odh-model-controller/internal/controller/constants" + "github.com/opendatahub-io/odh-model-controller/internal/controller/utils" +) + +func ApplyDefaultServerlessAnnotations(ctx context.Context, client client.Client, resourceName string, resourceMetadata *v1.ObjectMeta, logger logr.Logger) error { + deploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, client, resourceMetadata.GetAnnotations()) + if err != nil { + return fmt.Errorf("error resolving deployment mode for resource %s: %w", resourceName, err) + } + + if deploymentMode == constants.Serverless { + logAnnotationsAdded := make([]string, 0, 3) + resourceAnnotations := resourceMetadata.GetAnnotations() + if resourceAnnotations == nil { + resourceAnnotations = make(map[string]string) + } + + if _, exists := resourceAnnotations["serving.knative.openshift.io/enablePassthrough"]; !exists { + resourceAnnotations["serving.knative.openshift.io/enablePassthrough"] = "true" + logAnnotationsAdded = append(logAnnotationsAdded, "serving.knative.openshift.io/enablePassthrough") + } + + if _, exists := resourceAnnotations["sidecar.istio.io/inject"]; !exists { + resourceAnnotations["sidecar.istio.io/inject"] = "true" + logAnnotationsAdded = append(logAnnotationsAdded, "sidecar.istio.io/inject") + } + + if _, exists := resourceAnnotations["sidecar.istio.io/rewriteAppHTTPProbers"]; !exists { + resourceAnnotations["sidecar.istio.io/rewriteAppHTTPProbers"] = "true" + logAnnotationsAdded = append(logAnnotationsAdded, "sidecar.istio.io/rewriteAppHTTPProbers") + } + + if len(logAnnotationsAdded) > 0 { + logger.V(1).Info("Annotations added", "annotations", strings.Join(logAnnotationsAdded, ",")) + } + + resourceMetadata.SetAnnotations(resourceAnnotations) + } + return nil +} diff --git a/internal/webhook/serving/v1alpha1/inferencegraph_webhook.go b/internal/webhook/serving/v1alpha1/inferencegraph_webhook.go new file mode 100644 index 00000000..b00e2fd5 --- /dev/null +++ b/internal/webhook/serving/v1alpha1/inferencegraph_webhook.go @@ -0,0 +1,73 @@ +/* +Copyright 2024. + +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 v1alpha1 + +import ( + "context" + "fmt" + + servingv1alpha1 "github.com/kserve/kserve/pkg/apis/serving/v1alpha1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook" + + "github.com/opendatahub-io/odh-model-controller/internal/webhook/serving" +) + +// nolint:unused +// log is for logging in this package. +var inferencegraphlog = logf.Log.WithName("inferencegraph-resource") + +// SetupInferenceGraphWebhookWithManager registers the webhook for InferenceGraph in the manager. +func SetupInferenceGraphWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr).For(&servingv1alpha1.InferenceGraph{}). + WithDefaulter(&InferenceGraphCustomDefaulter{client: mgr.GetClient()}). + Complete() +} + +// +kubebuilder:webhook:path=/mutate-serving-kserve-io-v1alpha1-inferencegraph,mutating=true,failurePolicy=fail,sideEffects=None,groups=serving.kserve.io,resources=inferencegraphs,verbs=create,versions=v1alpha1,name=minferencegraph-v1alpha1.odh-model-controller.opendatahub.io,admissionReviewVersions=v1 + +// InferenceGraphCustomDefaulter struct is responsible for setting default values on the custom resource of the +// Kind InferenceGraph when those are created or updated. +// +// NOTE: The +kubebuilder:object:generate=false marker prevents controller-gen from generating DeepCopy methods, +// as it is used only for temporary operations and does not need to be deeply copied. +type InferenceGraphCustomDefaulter struct { + client client.Client +} + +var _ webhook.CustomDefaulter = &InferenceGraphCustomDefaulter{} + +// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind InferenceGraph. +func (d *InferenceGraphCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { + inferencegraph, ok := obj.(*servingv1alpha1.InferenceGraph) + + if !ok { + return fmt.Errorf("expected an InferenceGraph object but got %T", obj) + } + logger := inferencegraphlog.WithValues("name", inferencegraph.GetName()) + logger.Info("Defaulting for InferenceGraph") + + err := serving.ApplyDefaultServerlessAnnotations(ctx, d.client, inferencegraph.GetName(), &inferencegraph.ObjectMeta, logger) + if err != nil { + return err + } + + return nil +} diff --git a/internal/webhook/serving/v1alpha1/inferencegraph_webhook_test.go b/internal/webhook/serving/v1alpha1/inferencegraph_webhook_test.go new file mode 100644 index 00000000..42dfef86 --- /dev/null +++ b/internal/webhook/serving/v1alpha1/inferencegraph_webhook_test.go @@ -0,0 +1,111 @@ +/* +Copyright 2024. + +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 v1alpha1 + +import ( + kservev1alpha1 "github.com/kserve/kserve/pkg/apis/serving/v1alpha1" + "github.com/kserve/kserve/pkg/constants" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + testutils "github.com/opendatahub-io/odh-model-controller/test/utils" +) + +const InferenceServiceConfigPath1 = "../../../controller/serving/testdata/configmaps/inferenceservice-config.yaml" + +var _ = Describe("InferenceGraph Webhook", func() { + + Context("When creating InferenceGraph under Defaulting Webhook", func() { + var defaulter InferenceGraphCustomDefaulter + + BeforeEach(func() { + defaulter = InferenceGraphCustomDefaulter{client: k8sClient} + + inferenceServiceConfig := &corev1.ConfigMap{} + Expect(testutils.ConvertToStructuredResource(InferenceServiceConfigPath1, inferenceServiceConfig)).To(Succeed()) + if err := k8sClient.Create(ctx, inferenceServiceConfig); err != nil && !k8sErrors.IsAlreadyExists(err) { + Fail(err.Error()) + } + }) + + It("Should apply default annotations for Serverless mode", func() { + ig := kservev1alpha1.InferenceGraph{} + Expect(defaulter.Default(ctx, &ig)).To(Succeed()) + + annotations := ig.GetAnnotations() + Expect(annotations).To(HaveKeyWithValue("serving.knative.openshift.io/enablePassthrough", "true")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/inject", "true")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/rewriteAppHTTPProbers", "true")) + }) + + It("Should keep user-specified annotations for Serverless mode", func() { + ig := kservev1alpha1.InferenceGraph{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ig", + Namespace: "test-ig", + Annotations: map[string]string{ + "serving.knative.openshift.io/enablePassthrough": "false", + "sidecar.istio.io/rewriteAppHTTPProbers": "false", + "sidecar.istio.io/inject": "false", + }, + }, + } + Expect(defaulter.Default(ctx, &ig)).To(Succeed()) + + annotations := ig.GetAnnotations() + Expect(annotations).To(HaveKeyWithValue("serving.knative.openshift.io/enablePassthrough", "false")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/inject", "false")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/rewriteAppHTTPProbers", "false")) + }) + + It("Should not apply default annotations for Raw mode specified in annotation", func() { + ig := kservev1alpha1.InferenceGraph{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ig", + Namespace: "test-ig", + Annotations: map[string]string{ + constants.DeploymentMode: string(constants.RawDeployment), + }, + }, + } + Expect(defaulter.Default(ctx, &ig)).To(Succeed()) + + annotations := ig.GetAnnotations() + Expect(annotations).ToNot(HaveKey("serving.knative.openshift.io/enablePassthrough")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/inject")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/rewriteAppHTTPProbers")) + }) + + It("Should not apply default annotations for Raw mode specified in default configuration", func() { + inferenceServiceConfig := &corev1.ConfigMap{} + Expect(testutils.ConvertToStructuredResource(InferenceServiceConfigPath1, inferenceServiceConfig)).To(Succeed()) + inferenceServiceConfig.Data["deploy"] = "{\"defaultDeploymentMode\": \"RawDeployment\"}" + Expect(k8sClient.Update(ctx, inferenceServiceConfig)).To(Succeed()) + + ig := kservev1alpha1.InferenceGraph{} + Expect(defaulter.Default(ctx, &ig)).To(Succeed()) + + annotations := ig.GetAnnotations() + Expect(annotations).ToNot(HaveKey("serving.knative.openshift.io/enablePassthrough")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/inject")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/rewriteAppHTTPProbers")) + }) + }) +}) diff --git a/internal/webhook/serving/v1alpha1/webhook_suite_test.go b/internal/webhook/serving/v1alpha1/webhook_suite_test.go new file mode 100644 index 00000000..bb5bb660 --- /dev/null +++ b/internal/webhook/serving/v1alpha1/webhook_suite_test.go @@ -0,0 +1,151 @@ +/* +Copyright 2024. + +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 v1alpha1 + +import ( + "context" + "crypto/tls" + "fmt" + "net" + "path/filepath" + "runtime" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + admissionv1 "k8s.io/api/admission/v1" + + // +kubebuilder:scaffold:imports + apimachineryruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + "sigs.k8s.io/controller-runtime/pkg/webhook" + + "github.com/opendatahub-io/odh-model-controller/internal/controller/utils" +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. + +var ( + cancel context.CancelFunc + cfg *rest.Config + ctx context.Context + k8sClient client.Client + testEnv *envtest.Environment +) + +func TestAPIs(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Webhook Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + ctx, cancel = context.WithCancel(context.TODO()) + + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{ + filepath.Join("..", "..", "..", "..", "config", "crd", "bases"), + filepath.Join("..", "..", "..", "..", "config", "crd", "external")}, + ErrorIfCRDPathMissing: true, + + // The BinaryAssetsDirectory is only required if you want to run the tests directly + // without call the makefile target test. If not informed it will look for the + // default path defined in controller-runtime which is /usr/local/kubebuilder/. + // Note that you must have the required binaries setup under the bin directory to perform + // the tests directly. When we run make test it will be setup and used automatically. + BinaryAssetsDirectory: filepath.Join("..", "..", "..", "..", "bin", "k8s", + fmt.Sprintf("1.31.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + + WebhookInstallOptions: envtest.WebhookInstallOptions{ + Paths: []string{filepath.Join("..", "..", "..", "..", "config", "webhook")}, + }, + } + + var err error + // cfg is defined in this file globally. + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + scheme := apimachineryruntime.NewScheme() + utils.RegisterSchemes(scheme) + + err = admissionv1.AddToScheme(scheme) + Expect(err).NotTo(HaveOccurred()) + + // +kubebuilder:scaffold:scheme + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) + + // start webhook server using Manager. + webhookInstallOptions := &testEnv.WebhookInstallOptions + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme, + WebhookServer: webhook.NewServer(webhook.Options{ + Host: webhookInstallOptions.LocalServingHost, + Port: webhookInstallOptions.LocalServingPort, + CertDir: webhookInstallOptions.LocalServingCertDir, + }), + LeaderElection: false, + Metrics: metricsserver.Options{BindAddress: "0"}, + }) + Expect(err).NotTo(HaveOccurred()) + + err = SetupInferenceGraphWebhookWithManager(mgr) + Expect(err).NotTo(HaveOccurred()) + + // +kubebuilder:scaffold:webhook + + go func() { + defer GinkgoRecover() + err = mgr.Start(ctx) + Expect(err).NotTo(HaveOccurred()) + }() + + // wait for the webhook server to get ready. + dialer := &net.Dialer{Timeout: time.Second} + addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort) + Eventually(func() error { + conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + if err != nil { + return err + } + + return conn.Close() + }).Should(Succeed()) +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + cancel() + err := testEnv.Stop() + Expect(err).NotTo(HaveOccurred()) +}) diff --git a/internal/webhook/serving/v1beta1/inferenceservice_webhook.go b/internal/webhook/serving/v1beta1/inferenceservice_webhook.go index 7ff082ce..b40e6899 100644 --- a/internal/webhook/serving/v1beta1/inferenceservice_webhook.go +++ b/internal/webhook/serving/v1beta1/inferenceservice_webhook.go @@ -32,6 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/opendatahub-io/odh-model-controller/internal/controller/utils" + "github.com/opendatahub-io/odh-model-controller/internal/webhook/serving" ) // nolint:unused @@ -42,6 +43,7 @@ var inferenceservicelog = logf.Log.WithName("inferenceservice-resource") func SetupInferenceServiceWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr).For(&servingv1beta1.InferenceService{}). WithValidator(&InferenceServiceCustomValidator{client: mgr.GetClient()}). + WithDefaulter(&InferenceServiceCustomDefaulter{client: mgr.GetClient()}). Complete() } @@ -127,3 +129,34 @@ func (v *InferenceServiceCustomValidator) ValidateDelete(ctx context.Context, ob return nil, nil } + +// +kubebuilder:webhook:path=/mutate-serving-kserve-io-v1beta1-inferenceservice,mutating=true,failurePolicy=fail,sideEffects=None,groups=serving.kserve.io,resources=inferenceservices,verbs=create,versions=v1beta1,name=minferenceservice-v1beta1.odh-model-controller.opendatahub.io,admissionReviewVersions=v1 + +// InferenceServiceCustomDefaulter struct is responsible for setting default values on the custom resource of the +// Kind InferenceService when those are created or updated. +// +// NOTE: The +kubebuilder:object:generate=false marker prevents controller-gen from generating DeepCopy methods, +// as it is used only for temporary operations and does not need to be deeply copied. +type InferenceServiceCustomDefaulter struct { + client client.Client +} + +var _ webhook.CustomDefaulter = &InferenceServiceCustomDefaulter{} + +// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind InferenceService. +func (d *InferenceServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { + inferenceservice, ok := obj.(*servingv1beta1.InferenceService) + + if !ok { + return fmt.Errorf("expected an InferenceService object but got %T", obj) + } + logger := inferenceservicelog.WithValues("name", inferenceservice.GetName()) + logger.Info("Defaulting for InferenceService", "name", inferenceservice.GetName()) + + err := serving.ApplyDefaultServerlessAnnotations(ctx, d.client, inferenceservice.GetName(), &inferenceservice.ObjectMeta, logger) + if err != nil { + return err + } + + return nil +} diff --git a/internal/webhook/serving/v1beta1/inferenceservice_webhook_test.go b/internal/webhook/serving/v1beta1/inferenceservice_webhook_test.go index fb7b8b91..d00e01a3 100644 --- a/internal/webhook/serving/v1beta1/inferenceservice_webhook_test.go +++ b/internal/webhook/serving/v1beta1/inferenceservice_webhook_test.go @@ -18,20 +18,21 @@ package v1beta1 import ( kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1" + "github.com/kserve/kserve/pkg/constants" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/opendatahub-io/odh-model-controller/internal/controller/utils" testutils "github.com/opendatahub-io/odh-model-controller/test/utils" ) +const InferenceServiceConfigPath1 = "../../../controller/serving/testdata/configmaps/inferenceservice-config.yaml" + var _ = Describe("InferenceService validator Webhook", func() { - var ( - validator InferenceServiceCustomValidator - meshNamespace, appsNamespace string - ) + var meshNamespace, appsNamespace string createInferenceService := func(namespace, name string) *kservev1beta1.InferenceService { inferenceService := &kservev1beta1.InferenceService{} @@ -47,10 +48,15 @@ var _ = Describe("InferenceService validator Webhook", func() { BeforeEach(func() { _, meshNamespace = utils.GetIstioControlPlaneName(ctx, k8sClient) appsNamespace, _ = utils.GetApplicationNamespace(ctx, k8sClient) - validator = InferenceServiceCustomValidator{client: k8sClient} }) Context("When creating or updating InferenceService under Validating Webhook", func() { + var validator InferenceServiceCustomValidator + + BeforeEach(func() { + validator = InferenceServiceCustomValidator{client: k8sClient} + }) + It("Should allow the Inference Service in the test-model namespace", func() { testNs := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -90,4 +96,81 @@ var _ = Describe("InferenceService validator Webhook", func() { Expect(err).To(HaveOccurred()) }) }) + + Context("When creating InferenceService under Defaulting Webhook", func() { + var defaulter InferenceServiceCustomDefaulter + + BeforeEach(func() { + defaulter = InferenceServiceCustomDefaulter{client: k8sClient} + + inferenceServiceConfig := &corev1.ConfigMap{} + Expect(testutils.ConvertToStructuredResource(InferenceServiceConfigPath1, inferenceServiceConfig)).To(Succeed()) + if err := k8sClient.Create(ctx, inferenceServiceConfig); err != nil && !k8sErrors.IsAlreadyExists(err) { + Fail(err.Error()) + } + }) + + It("Should apply default annotations for Serverless mode", func() { + isvc := kservev1beta1.InferenceService{} + Expect(defaulter.Default(ctx, &isvc)).To(Succeed()) + + annotations := isvc.GetAnnotations() + Expect(annotations).To(HaveKeyWithValue("serving.knative.openshift.io/enablePassthrough", "true")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/inject", "true")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/rewriteAppHTTPProbers", "true")) + }) + + It("Should keep user-specified annotations for Serverless mode", func() { + isvc := kservev1beta1.InferenceService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-isvc", + Namespace: "test-isvc", + Annotations: map[string]string{ + "serving.knative.openshift.io/enablePassthrough": "false", + "sidecar.istio.io/rewriteAppHTTPProbers": "false", + "sidecar.istio.io/inject": "false", + }, + }, + } + Expect(defaulter.Default(ctx, &isvc)).To(Succeed()) + + annotations := isvc.GetAnnotations() + Expect(annotations).To(HaveKeyWithValue("serving.knative.openshift.io/enablePassthrough", "false")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/inject", "false")) + Expect(annotations).To(HaveKeyWithValue("sidecar.istio.io/rewriteAppHTTPProbers", "false")) + }) + + It("Should not apply default annotations for Raw mode specified in annotation", func() { + isvc := kservev1beta1.InferenceService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-isvc", + Namespace: "test-isvc", + Annotations: map[string]string{ + constants.DeploymentMode: string(constants.RawDeployment), + }, + }, + } + Expect(defaulter.Default(ctx, &isvc)).To(Succeed()) + + annotations := isvc.GetAnnotations() + Expect(annotations).ToNot(HaveKey("serving.knative.openshift.io/enablePassthrough")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/inject")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/rewriteAppHTTPProbers")) + }) + + It("Should not apply default annotations for Raw mode specified in default configuration", func() { + inferenceServiceConfig := &corev1.ConfigMap{} + Expect(testutils.ConvertToStructuredResource(InferenceServiceConfigPath1, inferenceServiceConfig)).To(Succeed()) + inferenceServiceConfig.Data["deploy"] = "{\"defaultDeploymentMode\": \"RawDeployment\"}" + Expect(k8sClient.Update(ctx, inferenceServiceConfig)).To(Succeed()) + + isvc := kservev1beta1.InferenceService{} + Expect(defaulter.Default(ctx, &isvc)).To(Succeed()) + + annotations := isvc.GetAnnotations() + Expect(annotations).ToNot(HaveKey("serving.knative.openshift.io/enablePassthrough")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/inject")) + Expect(annotations).ToNot(HaveKey("sidecar.istio.io/rewriteAppHTTPProbers")) + }) + }) }) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index f29bf4cb..10967bd2 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -258,6 +258,20 @@ var _ = Describe("Manager", Ordered, func() { Eventually(verifyCAInjection).Should(Succeed()) }) + It("should have CA injection for mutating webhooks", func() { + By("checking CA injection for mutating webhooks") + verifyCAInjection := func(g Gomega) { + cmd := exec.Command("kubectl", "get", + "mutatingwebhookconfigurations.admissionregistration.k8s.io", + "odh-model-controller-mutating-webhook-configuration", + "-o", "go-template={{ range .webhooks }}{{ .clientConfig.caBundle }}{{ end }}") + mwhOutput, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(len(mwhOutput)).To(BeNumerically(">", 10)) + } + Eventually(verifyCAInjection).Should(Succeed()) + }) + // +kubebuilder:scaffold:e2e-webhooks-checks // TODO: Customize the e2e test suite with scenarios specific to your project.