[WIP] Use generated defaulters instead of webhook defaulters

hack/update-codegen.sh

@@ -30,5 +30,13 @@ ${CODEGEN_PKG}/generate-groups.sh "deepcopy,client,informer,lister" \
   "serving:v1alpha1 istio:v1alpha2" \
   --go-header-file ${SERVING_ROOT}/hack/boilerplate/boilerplate.go.txt
 
+# Generate defaulters manually because generate-groups.sh doesn't, contrary to
+# the usage docs. This can be removed when/if generate-groups.sh allows adding
+# "defaulter" to the list of generators.
+${GOPATH}/bin/defaulter-gen \
+  --input-dirs github.com/knative/serving/pkg/apis/serving/v1alpha1 \
+  -O zz_generated.defaulters \
+  --go-header-file ${SERVING_ROOT}/hack/boilerplate/boilerplate.go.txt
+
 # Make sure our dependencies are up-to-date
 ${SERVING_ROOT}/hack/update-deps.sh

pkg/apis/serving/v1alpha1/defaults.go

@@ -0,0 +1,51 @@
+/*
+Copyright 2018 Google LLC.
+
+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 (
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+// Use these defaulting functions by calling the Default() method on the Scheme
+// var in the generated clientset scheme package.
+//
+// Example:
+//
+// import (
+//   "thisrepo/pkg/client/clientset/versioned/scheme"
+//   "thisrepo/pkg/apis/whatever/v1"
+// )
+// func main() {
+//   obj := &v1.SomeObject{}
+//   scheme.Scheme.Default(obj)
+// }
+
+func addDefaultingFuncs(scheme *runtime.Scheme) error {
+	return RegisterDefaults(scheme)
+}
+
+// SetDefaults_Revision sets default values for a Revision resource.
+// TODO When https://github.com/kubernetes/kubernetes/pull/63604 is merged and
+// available in beta, OpenAPI defaults should probably replace this.
+func SetDefaults_Revision(rev *Revision) {
+	if rev.Spec.ServingState == "" {
+		rev.Spec.ServingState = RevisionServingStateActive
+	}
+	if rev.Spec.ConcurrencyModel == "" {
+		rev.Spec.ConcurrencyModel = RevisionRequestConcurrencyModelMulti
+	}
+}

pkg/apis/serving/v1alpha1/defaults_test.go

@@ -0,0 +1,59 @@
+/*
+Copyright 2018 Google LLC. All rights reserved.
+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 (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+var testCases = []struct {
+	name   string
+	before runtime.Object
+	after  runtime.Object
+}{
+
+	{
+		name:   "Revision",
+		before: &Revision{},
+		after: &Revision{
+			Spec: RevisionSpec{
+				ServingState:     RevisionServingStateActive,
+				ConcurrencyModel: RevisionRequestConcurrencyModelMulti,
+			},
+		},
+	},
+}
+
+func TestDefaults(t *testing.T) {
+	var testScheme = getTestScheme()
+
+	for _, tc := range testCases {
+		testScheme.Default(tc.before)
+		if diff := cmp.Diff(tc.after, tc.before); diff != "" {
+			t.Errorf("%s: Unexpected default (-want +got): %v", tc.name, diff)
+		}
+	}
+}
+
+// We can't import the generated scheme because it depends on this package,
+// creating an import cycle.
+func getTestScheme() *runtime.Scheme {
+	scheme := runtime.NewScheme()
+	if err := AddToScheme(scheme); err != nil {
+		panic(err)
+	}
+	return scheme
+}

pkg/apis/serving/v1alpha1/doc.go

@@ -20,4 +20,5 @@ limitations under the License.
 
 // +k8s:deepcopy-gen=package
 // +groupName=serving.knative.dev
+// +k8s:defaulter-gen=TypeMeta
 package v1alpha1

pkg/apis/serving/v1alpha1/register.go

@@ -38,7 +38,7 @@ func Resource(resource string) schema.GroupResource {
 }
 
 var (
-	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes)
+	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addDefaultingFuncs)
 	AddToScheme   = SchemeBuilder.AddToScheme
 )
 

pkg/controller/configuration/configuration.go

@@ -22,6 +22,7 @@ import (
 	buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
 	"github.com/knative/serving/pkg/apis/serving"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
+	"github.com/knative/serving/pkg/client/clientset/versioned/scheme"
 	servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1"
 	listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
@@ -114,6 +115,8 @@ func (c *Controller) Reconcile(key string) error {
 
 	// Don't modify the informer's copy.
 	config = config.DeepCopy()
+	// Set configuration defaults.
+	scheme.Scheme.Default(config)
 	config.Status.InitializeConditions()
 
 	// First, fetch the revision that should exist for the current generation

pkg/controller/revision/revision.go

@@ -53,6 +53,7 @@ import (
 
 	buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
+	"github.com/knative/serving/pkg/client/clientset/versioned/scheme"
 	listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
 )
@@ -266,6 +267,8 @@ func (c *Controller) syncHandler(key string) error {
 	}
 	// Don't modify the informer's copy.
 	rev = rev.DeepCopy()
+	// Set revision defaults.
+	scheme.Scheme.Default(rev)
 	rev.Status.InitializeConditions()
 
 	if err := c.updateRevisionLoggingURL(rev); err != nil {

pkg/controller/route/route.go

@@ -36,6 +36,7 @@ import (
 
 	"github.com/knative/serving/pkg/apis/serving"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
+	"github.com/knative/serving/pkg/client/clientset/versioned/scheme"
 	servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1"
 	listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
@@ -199,6 +200,8 @@ func (c *Controller) updateRouteEvent(key string) error {
 	}
 	// Don't modify the informers copy
 	route = route.DeepCopy()
+	// Set route defaults.
+	scheme.Scheme.Default(route)
 	route.Status.InitializeConditions()
 
 	logger.Infof("Reconciling route :%v", route)

pkg/controller/service/service.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
+	"github.com/knative/serving/pkg/client/clientset/versioned/scheme"
 	servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1"
 	listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
@@ -130,6 +131,8 @@ func (c *Controller) Reconcile(key string) error {
 
 	// Don't modify the informers copy
 	service = service.DeepCopy()
+	// Set service defaults.
+	scheme.Scheme.Default(service)
 	service.Status.InitializeConditions()
 
 	configName := controller.GetServiceConfigurationName(service)

pkg/webhook/configuration.go

@@ -19,7 +19,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"path"
 	"reflect"
 	"strings"
 
@@ -125,29 +124,6 @@ func validateContainer(container corev1.Container) error {
 	return nil
 }
 
-// SetConfigurationDefaults set defaults on an configurations.
-func SetConfigurationDefaults(ctx context.Context) ResourceDefaulter {
-	return func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error {
-		_, config, err := unmarshalConfigurations(ctx, nil, crd, "SetConfigurationDefaults")
-		if err != nil {
-			return err
-		}
-
-		return setConfigurationSpecDefaults(patches, "/spec", config.Spec)
-	}
-}
-
-func setConfigurationSpecDefaults(patches *[]jsonpatch.JsonPatchOperation, patchBase string, spec v1alpha1.ConfigurationSpec) error {
-	if spec.RevisionTemplate.Spec.ConcurrencyModel == "" {
-		*patches = append(*patches, jsonpatch.JsonPatchOperation{
-			Operation: "add",
-			Path:      path.Join(patchBase, "revisionTemplate/spec/concurrencyModel"),
-			Value:     v1alpha1.RevisionRequestConcurrencyModelMulti,
-		})
-	}
-	return nil
-}
-
 func unmarshalConfigurations(
 	ctx context.Context, old GenericCRD, new GenericCRD, fnName string) (*v1alpha1.Configuration, *v1alpha1.Configuration, error) {
 	logger := logging.FromContext(ctx)

pkg/webhook/revision.go

@@ -19,11 +19,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"path"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/logging"
-	"github.com/google/go-cmp/cmp"
 	"github.com/mattbaird/jsonpatch"
 )
 
@@ -54,38 +53,6 @@ func ValidateRevision(ctx context.Context) ResourceCallback {
 	}
 }
 
-// SetRevisionDefaults set defaults on an revisions.
-func SetRevisionDefaults(ctx context.Context) ResourceDefaulter {
-	return func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error {
-		_, revision, err := unmarshalRevisions(ctx, nil, crd, "SetRevisionDefaults")
-		if err != nil {
-			return err
-		}
-
-		return setRevisionSpecDefaults(patches, "/spec", revision.Spec)
-	}
-}
-
-func setRevisionSpecDefaults(patches *[]jsonpatch.JsonPatchOperation, patchBase string, spec v1alpha1.RevisionSpec) error {
-	if spec.ServingState == "" {
-		*patches = append(*patches, jsonpatch.JsonPatchOperation{
-			Operation: "add",
-			Path:      path.Join(patchBase, "servingState"),
-			Value:     v1alpha1.RevisionServingStateActive,
-		})
-	}
-
-	if spec.ConcurrencyModel == "" {
-		*patches = append(*patches, jsonpatch.JsonPatchOperation{
-			Operation: "add",
-			Path:      path.Join(patchBase, "concurrencyModel"),
-			Value:     v1alpha1.RevisionRequestConcurrencyModelMulti,
-		})
-	}
-
-	return nil
-}
-
 func unmarshalRevisions(ctx context.Context, old GenericCRD, new GenericCRD, fnName string) (*v1alpha1.Revision, *v1alpha1.Revision, error) {
 	logger := logging.FromContext(ctx)
 	var oldRevision *v1alpha1.Revision

pkg/webhook/service.go

@@ -96,36 +96,3 @@ func unmarshalServices(
 
 	return oldService, newService, nil
 }
-
-// SetServiceDefaults set defaults on an services.
-// Service does not have any defaults, per-se, but because it holds a Configuration,
-// we need to set the Configuration's defaults. SetServiceDefaults dispatches to
-// SetConfigurationSpecDefaults to accomplish this.
-func SetServiceDefaults(ctx context.Context) ResourceDefaulter {
-	return func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error {
-		logger := logging.FromContext(ctx)
-		_, service, err := unmarshalServices(ctx, nil, crd, "SetServiceDefaults")
-		if err != nil {
-			return err
-		}
-
-		var (
-			configSpec v1alpha1.ConfigurationSpec
-			patchBase  string
-		)
-
-		if service.Spec.RunLatest != nil {
-			configSpec = service.Spec.RunLatest.Configuration
-			patchBase = "/spec/runLatest/configuration"
-		} else if service.Spec.Pinned != nil {
-			configSpec = service.Spec.Pinned.Configuration
-			patchBase = "/spec/pinned/configuration"
-		} else {
-			// We could error here, but validateSpec should catch this.
-			logger.Info("could not find config in SetServiceDefaults")
-			return nil
-		}
-
-		return setConfigurationSpecDefaults(patches, patchBase, configSpec)
-	}
-}