validation and mutating webhook for experiment

manifests/v1alpha2/katib-controller/katib-controller.yaml

@@ -30,3 +30,12 @@ spec:
           valueFrom:
             fieldRef:
               fieldPath: metadata.namespace
+        volumeMounts:
+        - mountPath: /tmp/cert
+          name: cert
+          readOnly: true
+      volumes:
+      - name: cert
+        secret:
+          defaultMode: 420
+          secretName: katib-controller

manifests/v1alpha2/katib-controller/rbac.yaml

@@ -9,6 +9,7 @@ rules:
   - configmaps
   - serviceaccounts
   - services
+  - secrets
   verbs:
   - "*"
 - apiGroups:

manifests/v1alpha2/katib-controller/secret.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: katib-controller
+  namespace: kubeflow

pkg/controller/v1alpha2/experiment/experiment_controller.go

@@ -18,7 +18,9 @@ package experiment
 
 import (
 	"context"
+	"os"
 
+	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -31,12 +33,15 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 	"sigs.k8s.io/controller-runtime/pkg/source"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission/builder"
 
 	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
 	trialsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/trial/v1alpha2"
 	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/util"
 )
 
+const KatibController = "katib-controller"
 var log = logf.Log.WithName("experiment-controller")
 
 /**
@@ -83,11 +88,66 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 		log.Error(err, "Trial watch failed")
 		return err
 	}
+	if err = addWebhook(mgr); err != nil {
+		log.Error(err, "Failed to create webhook")
+		return err
+	}
 
 	log.Info("Experiment controller created")
 	return nil
 }
 
+func addWebhook(mgr manager.Manager) error {
+	mutatingWebhook, err := builder.NewWebhookBuilder().
+		Name("mutating.experiment.kubeflow.org").
+		Mutating().
+		Operations(admissionregistrationv1beta1.Create, admissionregistrationv1beta1.Update).
+		WithManager(mgr).
+		ForType(&experimentsv1alpha2.Experiment{}).
+		Handlers(&experimentDefaulter{}).
+		Build()
+	if err != nil {
+		return err
+	}
+	validatingWebhook, err := builder.NewWebhookBuilder().
+		Name("validating.experiment.kubeflow.org").
+		Validating().
+		Operations(admissionregistrationv1beta1.Create, admissionregistrationv1beta1.Update).
+		WithManager(mgr).
+		ForType(&experimentsv1alpha2.Experiment{}).
+		Handlers(&experimentValidator{}).
+		Build()
+	if err != nil {
+		return err
+	}
+	as, err := webhook.NewServer("experiment-admission-server", mgr, webhook.ServerOptions{
+		CertDir: "/tmp/cert",
+		BootstrapOptions: &webhook.BootstrapOptions{
+			Secret: &types.NamespacedName{
+				Namespace: os.Getenv(experimentsv1alpha2.DefaultKatibNamespaceEnvName),
+				Name:      KatibController,
+			},
+			Service: &webhook.Service{
+				Namespace: os.Getenv(experimentsv1alpha2.DefaultKatibNamespaceEnvName),
+				Name:      KatibController,
+				Selectors: map[string]string{
+					"app": KatibController,
+				},
+			},
+			ValidatingWebhookConfigName: "experiment-validating-webhook-config",
+			MutatingWebhookConfigName: "experiment-mutating-webhook-config",
+		},
+	})
+	if err != nil {
+		return err
+	}
+	err = as.Register(mutatingWebhook, validatingWebhook)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 var _ reconcile.Reconciler = &ReconcileExperiment{}
 
 // ReconcileExperiment reconciles a Experiment object
@@ -192,13 +252,8 @@ func (r *ReconcileExperiment) ReconcileExperiment(instance *experimentsv1alpha2.
 func (r *ReconcileExperiment) ReconcileTrials(instance *experimentsv1alpha2.Experiment) error {
 
 	logger := log.WithValues("Experiment", types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()})
-	parallelCount := 0
 
-	if instance.Spec.ParallelTrialCount != nil {
-		parallelCount = *instance.Spec.ParallelTrialCount
-	} else {
-		parallelCount = experimentsv1alpha2.DefaultTrialParallelCount
-	}
+	parallelCount := *instance.Spec.ParallelTrialCount
 	activeCount := instance.Status.TrialsPending + instance.Status.TrialsRunning
 	completedCount := instance.Status.TrialsSucceeded + instance.Status.TrialsFailed + instance.Status.TrialsKilled
 
@@ -255,14 +310,8 @@ func (r *ReconcileExperiment) createTrials(instance *experimentsv1alpha2.Experim
 	/*trials := []apiv1alpha2.Trial{
 		apiv1alpha2.Trial{Spec: &apiv1alpha2.TrialSpec{}}, apiv1alpha2.Trial{Spec: &apiv1alpha2.TrialSpec{}},
 	}*/
-
-	trialTemplate, err := r.getTrialTemplate(instance)
-	if err != nil {
-		logger.Error(err, "Get trial template error")
-		return err
-	}
 	for _, trial := range trials {
-		if err = r.createTrialInstance(instance, trial, trialTemplate); err != nil {
+		if err = r.createTrialInstance(instance, trial); err != nil {
 			logger.Error(err, "Create trial instance error", "trial", trial)
 			continue
 		}

pkg/controller/v1alpha2/experiment/experiment_util.go

@@ -1,32 +1,20 @@
 package experiment
 
 import (
-	"bytes"
 	"context"
-	"errors"
 	"fmt"
-	"os"
-	"text/template"
 
-	apiv1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	utilrand "k8s.io/apimachinery/pkg/util/rand"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
 	trialsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/trial/v1alpha2"
 	apiv1alpha2 "github.com/kubeflow/katib/pkg/api/v1alpha2"
+	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/util"
 )
 
-type TrialTemplateParams struct {
-	Experiment      string
-	Trial           string
-	NameSpace       string
-	HyperParameters []*apiv1alpha2.ParameterAssignment
-}
-
-func (r *ReconcileExperiment) createTrialInstance(expInstance *experimentsv1alpha2.Experiment, trialInstance *apiv1alpha2.Trial, trialTemplate *template.Template) error {
+func (r *ReconcileExperiment) createTrialInstance(expInstance *experimentsv1alpha2.Experiment, trialInstance *apiv1alpha2.Trial) error {
 	logger := log.WithValues("Experiment", types.NamespacedName{Name: expInstance.GetName(), Namespace: expInstance.GetNamespace()})
 
 	trial := &trialsv1alpha2.Trial{}
@@ -39,24 +27,24 @@ func (r *ReconcileExperiment) createTrialInstance(expInstance *experimentsv1alph
 		return err
 	}
 
-	trialParams := TrialTemplateParams{
+	trialParams := util.TrialTemplateParams{
 		Experiment: expInstance.GetName(),
 		Trial:      trial.Name,
 		NameSpace:  trial.Namespace,
 	}
-
-	var buf bytes.Buffer
 	if trialInstance.Spec != nil && trialInstance.Spec.ParameterAssignments != nil {
 		for _, p := range trialInstance.Spec.ParameterAssignments.Assignments {
 			trialParams.HyperParameters = append(trialParams.HyperParameters, p)
 		}
 	}
-	if err := trialTemplate.Execute(&buf, trialParams); err != nil {
-		logger.Error(err, "Template execute error")
+
+	runSpec, err := util.GetRunSpec(expInstance, trialParams)
+	if err != nil {
+		logger.Error(err, "Fail to get RunSpec from experiment", expInstance.Name)
 		return err
 	}
 
-	trial.Spec.RunSpec = buf.String()
+	trial.Spec.RunSpec = runSpec
 
 	if err := r.Create(context.TODO(), trial); err != nil {
 		logger.Error(err, "Trial create error", "Trial name", trial.Name)
@@ -65,59 +53,3 @@ func (r *ReconcileExperiment) createTrialInstance(expInstance *experimentsv1alph
 	return nil
 
 }
-
-func (r *ReconcileExperiment) getTrialTemplate(instance *experimentsv1alpha2.Experiment) (*template.Template, error) {
-
-	var err error
-	var tpl *template.Template = nil
-	logger := log.WithValues("Experiment", types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()})
-	trialTemplate := instance.Spec.TrialTemplate
-	if trialTemplate != nil && trialTemplate.GoTemplate.RawTemplate != "" {
-		tpl, err = template.New("Trial").Parse(trialTemplate.GoTemplate.RawTemplate)
-	} else {
-		//default values if user hasn't set
-		configMapNS := os.Getenv(experimentsv1alpha2.DefaultKatibNamespaceEnvName)
-		configMapName := experimentsv1alpha2.DefaultTrialConfigMapName
-		templatePath := experimentsv1alpha2.DefaultTrialTemplatePath
-
-		if trialTemplate != nil && trialTemplate.GoTemplate.TemplateSpec != nil {
-			templateSpec := trialTemplate.GoTemplate.TemplateSpec
-			if templateSpec.ConfigMapName != "" {
-				configMapName = templateSpec.ConfigMapName
-			}
-			if templateSpec.ConfigMapNamespace != "" {
-				configMapNS = templateSpec.ConfigMapNamespace
-			}
-			if templateSpec.TemplatePath != "" {
-				templatePath = templateSpec.TemplatePath
-			}
-		}
-		configMap, err := r.getConfigMap(configMapName, configMapNS)
-		if err != nil {
-			logger.Error(err, "Get config map error", "configMapName", configMapName, "configMapNS", configMapNS)
-			return nil, err
-		}
-		if configMapTemplate, ok := configMap[templatePath]; !ok {
-			err = errors.New(string(metav1.StatusReasonNotFound))
-			logger.Error(err, "Config map template not found", "templatePath", templatePath)
-			return nil, err
-		} else {
-			tpl, err = template.New("Trial").Parse(configMapTemplate)
-		}
-	}
-	if err != nil {
-		logger.Error(err, "Template parse error")
-		return nil, err
-	}
-
-	return tpl, nil
-}
-
-func (r *ReconcileExperiment) getConfigMap(name, namespace string) (map[string]string, error) {
-
-	configMap := &apiv1.ConfigMap{}
-	if err := r.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, configMap); err != nil {
-		return map[string]string{}, err
-	}
-	return configMap.Data, nil
-}

pkg/controller/v1alpha2/experiment/mutate_webhook.go

@@ -0,0 +1,63 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+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 experiment
+
+import (
+        "context"
+        "net/http"
+
+        experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
+        "sigs.k8s.io/controller-runtime/pkg/client"
+        "sigs.k8s.io/controller-runtime/pkg/runtime/inject"
+        "sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+        "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types"
+)
+
+// experimentDefaulter that sets default fields in experiment
+type experimentDefaulter struct {
+	client  client.Client
+	decoder types.Decoder
+}
+
+var _ admission.Handler = &experimentDefaulter{}
+
+func (e *experimentDefaulter) Handle(ctx context.Context, req types.Request) types.Response {
+        inst := &experimentsv1alpha2.Experiment{}
+        err := e.decoder.Decode(req, inst)
+        if err != nil {
+                return admission.ErrorResponse(http.StatusBadRequest, err)
+        }
+
+	copy := inst.DeepCopy()
+	copy.SetDefault()
+
+	return admission.PatchResponse(inst, copy)
+}
+
+var _ inject.Client = &experimentDefaulter{}
+
+func (e *experimentDefaulter) InjectClient(c client.Client) error {
+	e.client = c
+	return nil
+}
+
+var _ inject.Decoder = &experimentDefaulter{}
+
+func (e *experimentDefaulter) InjectDecoder(d types.Decoder) error {
+	e.decoder = d
+	return nil
+}