Implements Validating and Defaulting AdmissionWebhooks for KFServices (…

…#45)

* Implements ValidatingAdmissionWebhooks for KFServices

* Implements DefaultingAdmissionWebhooks for KFServices

* Fixes JSONPatch merge issues maps

* Extracted constants and read namespace from environment variables

Makefile

@@ -60,7 +60,10 @@ endif
 docker-build: test
 	docker build . -t ${IMG}
 	@echo "updating kustomize image patch file for manager resource"
-	sed -i'' -e 's@image: .*@image: '"${IMG}"'@' ./config/default/manager_image_patch.yaml
+
+	# Use perl instead of sed to avoid OSX/Linux compatibility issue:
+	# https://stackoverflow.com/questions/34533893/sed-command-creating-unwanted-duplicates-of-file-with-e-extension
+	perl -pi -e 's@image: .*@image: '"${IMG}"'@' ./config/default/manager_image_patch.yaml
 
 # Push the docker image
 docker-push:

cmd/manager/main.go

@@ -23,7 +23,7 @@ import (
 	knservingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/kubeflow/kfserving/pkg/apis"
 	"github.com/kubeflow/kfserving/pkg/controller"
-
+	"github.com/kubeflow/kfserving/pkg/webhook"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -39,15 +39,15 @@ func main() {
 	log := logf.Log.WithName("entrypoint")
 
 	// Get a config to talk to the apiserver
-	log.Info("setting up client for manager")
+	log.Info("Setting up client for manager")
 	cfg, err := config.GetConfig()
 	if err != nil {
 		log.Error(err, "unable to set up client config")
 		os.Exit(1)
 	}
 
 	// Create a new Cmd to provide shared dependencies and start components
-	log.Info("setting up manager")
+	log.Info("Setting up manager")
 	mgr, err := manager.New(cfg, manager.Options{MetricsBindAddress: metricsAddr})
 	if err != nil {
 		log.Error(err, "unable to set up overall controller manager")
@@ -57,14 +57,14 @@ func main() {
 	log.Info("Registering Components.")
 
 	// Setup Scheme for all resources
-	log.Info("setting up scheme")
+	log.Info("Setting up scheme")
 	if err := apis.AddToScheme(mgr.GetScheme()); err != nil {
 		log.Error(err, "unable add APIs to scheme")
 		os.Exit(1)
 	}
 
 	// Setup Scheme for all resources
-	log.Info("setting up Knative scheme")
+	log.Info("Setting up Knative scheme")
 	if err := knservingv1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
 		log.Error(err, "unable add Knative APIs to scheme")
 		os.Exit(1)
@@ -77,6 +77,12 @@ func main() {
 		os.Exit(1)
 	}
 
+	log.Info("Setting up webhooks")
+	if err := webhook.AddToManager(mgr); err != nil {
+		log.Error(err, "unable to register webhooks to the manager")
+		os.Exit(1)
+	}
+
 	// Start the Cmd
 	log.Info("Starting the Cmd.")
 	if err := mgr.Start(signals.SetupSignalHandler()); err != nil {

config/default/kustomization.yaml

@@ -6,7 +6,7 @@ namespace: kubeflow-system
 # "wordpress" becomes "alices-wordpress".
 # Note that it should also match with the prefix (text before '-') of the namespace
 # field above.
-namePrefix: kubeflow-
+namePrefix: kfserving-
 
 # Labels to add to all resources and selectors.
 #commonLabels:

config/manager/manager.yaml

@@ -2,7 +2,7 @@ apiVersion: v1
 kind: Namespace
 metadata:
   labels:
-    control-plane: controller-manager
+    control-plane: kfserving-controller-manager
     controller-tools.k8s.io: "1.0"
   name: system
 ---
@@ -12,11 +12,11 @@ metadata:
   name: controller-manager-service
   namespace: system
   labels:
-    control-plane: controller-manager
+    control-plane: kfserving-controller-manager
     controller-tools.k8s.io: "1.0"
 spec:
   selector:
-    control-plane: controller-manager
+    control-plane: kfserving-controller-manager
     controller-tools.k8s.io: "1.0"
   ports:
   - port: 443
@@ -27,18 +27,18 @@ metadata:
   name: controller-manager
   namespace: system
   labels:
-    control-plane: controller-manager
+    control-plane: kfserving-controller-manager
     controller-tools.k8s.io: "1.0"
 spec:
   selector:
     matchLabels:
-      control-plane: controller-manager
+      control-plane: kfserving-controller-manager
       controller-tools.k8s.io: "1.0"
   serviceName: controller-manager-service
   template:
     metadata:
       labels:
-        control-plane: controller-manager
+        control-plane: kfserving-controller-manager
         controller-tools.k8s.io: "1.0"
     spec:
       containers:

config/rbac/rbac_role.yaml

@@ -44,3 +44,40 @@ rules:
   - get
   - update
   - patch
+- apiGroups:
+  - admissionregistration.k8s.io
+  resources:
+  - mutatingwebhookconfigurations
+  - validatingwebhookconfigurations
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete
+- apiGroups:
+  - ""
+  resources:
+  - secrets
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete
+- apiGroups:
+  - ""
+  resources:
+  - services
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete

docs/samples/tensorflow.yaml

@@ -1,7 +1,7 @@
 apiVersion: "serving.kubeflow.org/v1alpha1"
 kind: "KFService"
 metadata:
-  name: "myModel"
+  name: "my-model"
 spec:
   default:
     tensorflow:

pkg/apis/serving/v1alpha1/doc.go

@@ -21,3 +21,9 @@ limitations under the License.
 // +k8s:defaulter-gen=TypeMeta
 // +groupName=serving.kubeflow.org
 package v1alpha1
+
+import (
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+)
+
+var logger = logf.Log.WithName("kfserving-v1alpha1-types")

pkg/apis/serving/v1alpha1/kfservice_defaults.go

@@ -2,31 +2,22 @@ package v1alpha1
 
 import (
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
 	resource "k8s.io/apimachinery/pkg/api/resource"
 )
 
-// DefaultTensorflowVersion runs if not provided in TensorflowSpec
-const (
+// Default Values
+var (
 	DefaultTensorflowVersion  = "1.12"
 	DefaultXGBoostVersion     = "1.12"
 	DefaultScikitLearnVersion = "1.12"
-)
 
-// DefaultModelServerResourceRequirements sets initial limits on serving pods
-var DefaultModelServerResourceRequirements = v1.ResourceRequirements{
-	Requests: v1.ResourceList{
-		"cpu": resource.MustParse("1"),
-		"mem": resource.MustParse("2Gi"),
-	},
-	Limits: v1.ResourceList{
-		"cpu": resource.MustParse("1"),
-		"mem": resource.MustParse("2Gi"),
-	},
-}
+	DefaultMemoryRequests = resource.MustParse("2Gi")
+	DefaultCPURequests    = resource.MustParse("1")
+)
 
 // Default implements https://godoc.org/sigs.k8s.io/controller-runtime/pkg/webhook/admission#Defaulter
 func (kfsvc *KFService) Default() {
+	logger.Info("Defaulting KFService", "namespace", kfsvc.Namespace, "name", kfsvc.Name)
 	setModelSpecDefaults(&kfsvc.Spec.Default)
 	if kfsvc.Spec.Canary != nil {
 		setModelSpecDefaults(&kfsvc.Spec.Canary.ModelSpec)
@@ -43,32 +34,38 @@ func setModelSpecDefaults(modelSpec *ModelSpec) {
 	if modelSpec.ScikitLearn != nil {
 		setScikitLearnDefaults(modelSpec.ScikitLearn)
 	}
-	// todo(ellis-bigelow) Custom container
 }
 
 func setTensorflowDefaults(tensorflowSpec *TensorflowSpec) {
 	if tensorflowSpec.RuntimeVersion == "" {
 		tensorflowSpec.RuntimeVersion = DefaultTensorflowVersion
 	}
-	if equality.Semantic.DeepEqual(tensorflowSpec.Resources, v1.ResourceRequirements{}) {
-		tensorflowSpec.Resources = DefaultModelServerResourceRequirements
-	}
+	setResourceRequirementDefaults(&tensorflowSpec.Resources)
 }
 
 func setXGBoostDefaults(xgBoostSpec *XGBoostSpec) {
 	if xgBoostSpec.RuntimeVersion == "" {
 		xgBoostSpec.RuntimeVersion = DefaultXGBoostVersion
 	}
-	if equality.Semantic.DeepEqual(xgBoostSpec.Resources, v1.ResourceRequirements{}) {
-		xgBoostSpec.Resources = DefaultModelServerResourceRequirements
-	}
+	setResourceRequirementDefaults(&xgBoostSpec.Resources)
 }
 
 func setScikitLearnDefaults(scikitLearnSpec *ScikitLearnSpec) {
 	if scikitLearnSpec.RuntimeVersion == "" {
 		scikitLearnSpec.RuntimeVersion = DefaultScikitLearnVersion
 	}
-	if equality.Semantic.DeepEqual(scikitLearnSpec.Resources, v1.ResourceRequirements{}) {
-		scikitLearnSpec.Resources = DefaultModelServerResourceRequirements
+	setResourceRequirementDefaults(&scikitLearnSpec.Resources)
+}
+
+func setResourceRequirementDefaults(requirements *v1.ResourceRequirements) {
+	if requirements.Requests == nil {
+		requirements.Requests = v1.ResourceList{}
+	}
+
+	if _, ok := requirements.Requests[v1.ResourceCPU]; !ok {
+		requirements.Requests[v1.ResourceCPU] = DefaultCPURequests
+	}
+	if _, ok := requirements.Requests[v1.ResourceMemory]; !ok {
+		requirements.Requests[v1.ResourceMemory] = DefaultMemoryRequests
 	}
 }

pkg/apis/serving/v1alpha1/kfservice_defaults_test.go

@@ -29,11 +29,13 @@ func TestTensorflowDefaults(t *testing.T) {
 	kfsvc := TFExampleKFService.DeepCopy()
 	kfsvc.Spec.Canary = &CanarySpec{ModelSpec: *kfsvc.Spec.Default.DeepCopy()}
 	kfsvc.Spec.Canary.Tensorflow.RuntimeVersion = "1.11"
-	kfsvc.Spec.Canary.Tensorflow.Resources.Requests = v1.ResourceList{"mem": resource.MustParse("3Gi")}
+	kfsvc.Spec.Canary.Tensorflow.Resources.Requests = v1.ResourceList{v1.ResourceMemory: resource.MustParse("3Gi")}
 	kfsvc.Default()
 
 	g.Expect(kfsvc.Spec.Default.Tensorflow.RuntimeVersion).To(gomega.Equal(DefaultTensorflowVersion))
-	g.Expect(kfsvc.Spec.Default.Tensorflow.Resources).To(gomega.Equal(DefaultModelServerResourceRequirements))
+	g.Expect(kfsvc.Spec.Default.Tensorflow.Resources.Requests[v1.ResourceCPU]).To(gomega.Equal(DefaultCPURequests))
+	g.Expect(kfsvc.Spec.Default.Tensorflow.Resources.Requests[v1.ResourceMemory]).To(gomega.Equal(DefaultMemoryRequests))
 	g.Expect(kfsvc.Spec.Canary.Tensorflow.RuntimeVersion).To(gomega.Equal("1.11"))
-	g.Expect(kfsvc.Spec.Canary.Tensorflow.Resources.Requests).To(gomega.Equal(v1.ResourceList{"mem": resource.MustParse("3Gi")}))
+	g.Expect(kfsvc.Spec.Canary.Tensorflow.Resources.Requests[v1.ResourceCPU]).To(gomega.Equal(DefaultCPURequests))
+	g.Expect(kfsvc.Spec.Canary.Tensorflow.Resources.Requests[v1.ResourceMemory]).To(gomega.Equal(resource.MustParse("3Gi")))
 }

pkg/apis/serving/v1alpha1/kfservice_validation.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 
 	runtime "k8s.io/apimachinery/pkg/runtime"
-	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
 
 const (
@@ -36,8 +35,6 @@ const (
 	ExactlyOneModelSpecViolatedError = "Exactly one of [Custom, Tensorflow, ScikitLearn, XGBoost] should be specified in ModelSpec"
 )
 
-var logger = logf.Log.WithName("kfservice-validation")
-
 // ValidateCreate implements https://godoc.org/sigs.k8s.io/controller-runtime/pkg/webhook/admission#Validator
 func (kfsvc *KFService) ValidateCreate() error {
 	return kfsvc.validate()

pkg/apis/serving/v1alpha1/kfservice_validation_test.go

@@ -51,7 +51,6 @@ func TestRejectCanaryModelSpecMissing(t *testing.T) {
 	kfsvc.Spec.Canary = &CanarySpec{ModelSpec: ModelSpec{}}
 	g.Expect(kfsvc.ValidateCreate()).Should(gomega.MatchError(ExactlyOneModelSpecViolatedError))
 }
-
 func TestRejectBadCanaryTrafficValues(t *testing.T) {
 	g := gomega.NewGomegaWithT(t)
 	kfsvc := TFExampleKFService.DeepCopy()