From 94c5dddc8bf7f055949848ff9e22228a9957aa19 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 12:00:55 -0700 Subject: [PATCH 1/8] Generate defaulters for serving/v1alpha1 generate-groups.sh won't generate defaulters, contrary to what the usage docs say. Until it does, this command will generate defaulters for the only package that wants to use them, serving/v1alpha1. --- hack/update-codegen.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index 624bcbd6c44e..00f4b5bcf4f7 100755 --- a/hack/update-codegen.sh +++ b/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 hack/../hack/boilerplate/boilerplate.go.txt + # Make sure our dependencies are up-to-date ${SERVING_ROOT}/hack/update-deps.sh From ead63cf7f21f5c4a79808cfcc7cf47bdbc281382 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 12:08:40 -0700 Subject: [PATCH 2/8] Add Revision defaulter Defaulters are a feature of runtime.Scheme that can set default values for any runtime.Object. We can use kubernetes/code-generator's defaulter-gen to generate defaulters for use with CRDs like Revision. The k8s:defaulter-gen=TypeMeta comment tells defaulter-gen to generate defaulters for every struct with a TypeMeta field. In this case, we want to set the ServingState of a RevisionSpec to Active by default, so we need a defaulter for Revision since RevisionSpec doesn't embed TypeMeta. The actual defaulting function is SetDefault_Revision. The generated code makes it easy to hook this function into the Scheme. To set defaults on an object, import the clientset's generated scheme package and call scheme.Scheme.Default(object). The test can't do this because of an import cycle, so it creates a new scheme. --- pkg/apis/serving/v1alpha1/defaults.go | 43 ++++++++++++++++++ pkg/apis/serving/v1alpha1/doc.go | 1 + pkg/apis/serving/v1alpha1/register.go | 2 +- .../serving/v1alpha1/revision_types_test.go | 27 +++++++++++ .../v1alpha1/zz_generated.defaulters.go | 45 +++++++++++++++++++ 5 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 pkg/apis/serving/v1alpha1/defaults.go create mode 100644 pkg/apis/serving/v1alpha1/zz_generated.defaulters.go diff --git a/pkg/apis/serving/v1alpha1/defaults.go b/pkg/apis/serving/v1alpha1/defaults.go new file mode 100644 index 000000000000..0372b6aebda2 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/defaults.go @@ -0,0 +1,43 @@ +/* +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) +} + +func SetDefaults_Revision(rev *Revision) { + if rev.Spec.ServingState == "" { + rev.Spec.ServingState = RevisionServingStateActive + } +} diff --git a/pkg/apis/serving/v1alpha1/doc.go b/pkg/apis/serving/v1alpha1/doc.go index 89ac34ba6f0d..755689881f43 100644 --- a/pkg/apis/serving/v1alpha1/doc.go +++ b/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 diff --git a/pkg/apis/serving/v1alpha1/register.go b/pkg/apis/serving/v1alpha1/register.go index b7b8e51fefa1..d4d9e9c915ec 100644 --- a/pkg/apis/serving/v1alpha1/register.go +++ b/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 ) diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go index 628625f0a95e..fd57faf5b650 100644 --- a/pkg/apis/serving/v1alpha1/revision_types_test.go +++ b/pkg/apis/serving/v1alpha1/revision_types_test.go @@ -17,10 +17,37 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "github.com/google/go-cmp/cmp" buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" ) +// 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 +} + +func TestRevisionDefaults(t *testing.T) { + var testScheme = getTestScheme() + + wantRev := &Revision{ + Spec: RevisionSpec{ + ServingState: RevisionServingStateActive, + }, + } + rev := &Revision{} + testScheme.Default(rev) + if diff := cmp.Diff(wantRev, rev); diff != "" { + t.Errorf("Unexpected default revision (-want +got): %v", diff) + } +} + func TestGeneration(t *testing.T) { r := Revision{} if a := r.GetGeneration(); a != 0 { diff --git a/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go b/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go new file mode 100644 index 000000000000..4cf285435295 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go @@ -0,0 +1,45 @@ +// +build !ignore_autogenerated + +/* +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. +*/ + +// This file was autogenerated by defaulter-gen. Do not edit it manually! + +package v1alpha1 + +import ( + runtime "k8s.io/apimachinery/pkg/runtime" +) + +// RegisterDefaults adds defaulters functions to the given scheme. +// Public to allow building arbitrary schemes. +// All generated defaulters are covering - they call all nested defaulters. +func RegisterDefaults(scheme *runtime.Scheme) error { + scheme.AddTypeDefaultingFunc(&Revision{}, func(obj interface{}) { SetObjectDefaults_Revision(obj.(*Revision)) }) + scheme.AddTypeDefaultingFunc(&RevisionList{}, func(obj interface{}) { SetObjectDefaults_RevisionList(obj.(*RevisionList)) }) + return nil +} + +func SetObjectDefaults_Revision(in *Revision) { + SetDefaults_Revision(in) +} + +func SetObjectDefaults_RevisionList(in *RevisionList) { + for i := range in.Items { + a := &in.Items[i] + SetObjectDefaults_Revision(a) + } +} From cd58ee1ccc3288f26830f94fb33040b2002a89a4 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 14:00:32 -0700 Subject: [PATCH 3/8] Unexpand boilerplate path --- hack/update-codegen.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index 00f4b5bcf4f7..b5b30c01f33c 100755 --- a/hack/update-codegen.sh +++ b/hack/update-codegen.sh @@ -35,8 +35,8 @@ ${CODEGEN_PKG}/generate-groups.sh "deepcopy,client,informer,lister" \ # "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 hack/../hack/boilerplate/boilerplate.go.txt + -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 From 03a555a9d4f7230bf5a2092510f5378dba086e59 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 15:39:36 -0700 Subject: [PATCH 4/8] Add defaulters for Configuration and Service These were copied from the defaulters in pkg/webhook. The webhook defaulters are still used; these new ones remain unused. Moved defaulter tests to a new file and made them table-driven. --- pkg/apis/serving/v1alpha1/defaults.go | 26 +++- pkg/apis/serving/v1alpha1/defaults_test.go | 117 ++++++++++++++++++ .../serving/v1alpha1/revision_types_test.go | 27 ---- .../v1alpha1/zz_generated.defaulters.go | 26 ++++ 4 files changed, 168 insertions(+), 28 deletions(-) create mode 100644 pkg/apis/serving/v1alpha1/defaults_test.go diff --git a/pkg/apis/serving/v1alpha1/defaults.go b/pkg/apis/serving/v1alpha1/defaults.go index 0372b6aebda2..2e58d92cbcd8 100644 --- a/pkg/apis/serving/v1alpha1/defaults.go +++ b/pkg/apis/serving/v1alpha1/defaults.go @@ -16,7 +16,9 @@ limitations under the License. package v1alpha1 -import "k8s.io/apimachinery/pkg/runtime" +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. @@ -40,4 +42,26 @@ func SetDefaults_Revision(rev *Revision) { if rev.Spec.ServingState == "" { rev.Spec.ServingState = RevisionServingStateActive } + if rev.Spec.ConcurrencyModel == "" { + rev.Spec.ConcurrencyModel = RevisionRequestConcurrencyModelMulti + } +} + +func SetDefaults_Configuration(config *Configuration) { + if config.Spec.RevisionTemplate.Spec.ConcurrencyModel == "" { + config.Spec.RevisionTemplate.Spec.ConcurrencyModel = RevisionRequestConcurrencyModelMulti + } +} + +func SetDefaults_Service(svc *Service) { + config := &Configuration{} + if svc.Spec.RunLatest != nil { + config.Spec = svc.Spec.RunLatest.Configuration + SetDefaults_Configuration(config) + svc.Spec.RunLatest.Configuration = config.Spec + } else if svc.Spec.Pinned != nil { + config.Spec = svc.Spec.Pinned.Configuration + SetDefaults_Configuration(config) + svc.Spec.Pinned.Configuration = config.Spec + } } diff --git a/pkg/apis/serving/v1alpha1/defaults_test.go b/pkg/apis/serving/v1alpha1/defaults_test.go new file mode 100644 index 000000000000..52cd6c22292c --- /dev/null +++ b/pkg/apis/serving/v1alpha1/defaults_test.go @@ -0,0 +1,117 @@ +/* +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, + }, + }, + }, + + { + name: "Configuration", + before: &Configuration{}, + after: &Configuration{ + Spec: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + ConcurrencyModel: RevisionRequestConcurrencyModelMulti, + }, + }, + }, + }, + }, + + { + name: "Service RunLatest", + before: &Service{ + Spec: ServiceSpec{ + RunLatest: &RunLatestType{}, + }, + }, + after: &Service{ + Spec: ServiceSpec{ + RunLatest: &RunLatestType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + ConcurrencyModel: RevisionRequestConcurrencyModelMulti, + }, + }, + }, + }, + }, + }, + }, + + { + name: "Service Pinned", + before: &Service{ + Spec: ServiceSpec{ + Pinned: &PinnedType{}, + }, + }, + after: &Service{ + Spec: ServiceSpec{ + Pinned: &PinnedType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + 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 +} diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go index fd57faf5b650..628625f0a95e 100644 --- a/pkg/apis/serving/v1alpha1/revision_types_test.go +++ b/pkg/apis/serving/v1alpha1/revision_types_test.go @@ -17,37 +17,10 @@ import ( "testing" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" - "github.com/google/go-cmp/cmp" buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" ) -// 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 -} - -func TestRevisionDefaults(t *testing.T) { - var testScheme = getTestScheme() - - wantRev := &Revision{ - Spec: RevisionSpec{ - ServingState: RevisionServingStateActive, - }, - } - rev := &Revision{} - testScheme.Default(rev) - if diff := cmp.Diff(wantRev, rev); diff != "" { - t.Errorf("Unexpected default revision (-want +got): %v", diff) - } -} - func TestGeneration(t *testing.T) { r := Revision{} if a := r.GetGeneration(); a != 0 { diff --git a/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go b/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go index 4cf285435295..1dbda3eeb10b 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go @@ -28,11 +28,26 @@ import ( // Public to allow building arbitrary schemes. // All generated defaulters are covering - they call all nested defaulters. func RegisterDefaults(scheme *runtime.Scheme) error { + scheme.AddTypeDefaultingFunc(&Configuration{}, func(obj interface{}) { SetObjectDefaults_Configuration(obj.(*Configuration)) }) + scheme.AddTypeDefaultingFunc(&ConfigurationList{}, func(obj interface{}) { SetObjectDefaults_ConfigurationList(obj.(*ConfigurationList)) }) scheme.AddTypeDefaultingFunc(&Revision{}, func(obj interface{}) { SetObjectDefaults_Revision(obj.(*Revision)) }) scheme.AddTypeDefaultingFunc(&RevisionList{}, func(obj interface{}) { SetObjectDefaults_RevisionList(obj.(*RevisionList)) }) + scheme.AddTypeDefaultingFunc(&Service{}, func(obj interface{}) { SetObjectDefaults_Service(obj.(*Service)) }) + scheme.AddTypeDefaultingFunc(&ServiceList{}, func(obj interface{}) { SetObjectDefaults_ServiceList(obj.(*ServiceList)) }) return nil } +func SetObjectDefaults_Configuration(in *Configuration) { + SetDefaults_Configuration(in) +} + +func SetObjectDefaults_ConfigurationList(in *ConfigurationList) { + for i := range in.Items { + a := &in.Items[i] + SetObjectDefaults_Configuration(a) + } +} + func SetObjectDefaults_Revision(in *Revision) { SetDefaults_Revision(in) } @@ -43,3 +58,14 @@ func SetObjectDefaults_RevisionList(in *RevisionList) { SetObjectDefaults_Revision(a) } } + +func SetObjectDefaults_Service(in *Service) { + SetDefaults_Service(in) +} + +func SetObjectDefaults_ServiceList(in *ServiceList) { + for i := range in.Items { + a := &in.Items[i] + SetObjectDefaults_Service(a) + } +} From 81433870e95cc995ac0daebf6a00d3fa6d9a8068 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 16:15:20 -0700 Subject: [PATCH 5/8] Remove Configuration and Service defaulters Both of these defaulters were setting defaults on nested revision templates. I think it's better to let the defaults be bound when the templated object is created instead of when the template is created. --- pkg/apis/serving/v1alpha1/defaults.go | 19 ------ pkg/apis/serving/v1alpha1/defaults_test.go | 58 ------------------- .../v1alpha1/zz_generated.defaulters.go | 26 --------- 3 files changed, 103 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/defaults.go b/pkg/apis/serving/v1alpha1/defaults.go index 2e58d92cbcd8..2231c7839ad7 100644 --- a/pkg/apis/serving/v1alpha1/defaults.go +++ b/pkg/apis/serving/v1alpha1/defaults.go @@ -46,22 +46,3 @@ func SetDefaults_Revision(rev *Revision) { rev.Spec.ConcurrencyModel = RevisionRequestConcurrencyModelMulti } } - -func SetDefaults_Configuration(config *Configuration) { - if config.Spec.RevisionTemplate.Spec.ConcurrencyModel == "" { - config.Spec.RevisionTemplate.Spec.ConcurrencyModel = RevisionRequestConcurrencyModelMulti - } -} - -func SetDefaults_Service(svc *Service) { - config := &Configuration{} - if svc.Spec.RunLatest != nil { - config.Spec = svc.Spec.RunLatest.Configuration - SetDefaults_Configuration(config) - svc.Spec.RunLatest.Configuration = config.Spec - } else if svc.Spec.Pinned != nil { - config.Spec = svc.Spec.Pinned.Configuration - SetDefaults_Configuration(config) - svc.Spec.Pinned.Configuration = config.Spec - } -} diff --git a/pkg/apis/serving/v1alpha1/defaults_test.go b/pkg/apis/serving/v1alpha1/defaults_test.go index 52cd6c22292c..a56edded43c0 100644 --- a/pkg/apis/serving/v1alpha1/defaults_test.go +++ b/pkg/apis/serving/v1alpha1/defaults_test.go @@ -35,64 +35,6 @@ var testCases = []struct { }, }, }, - - { - name: "Configuration", - before: &Configuration{}, - after: &Configuration{ - Spec: ConfigurationSpec{ - RevisionTemplate: RevisionTemplateSpec{ - Spec: RevisionSpec{ - ConcurrencyModel: RevisionRequestConcurrencyModelMulti, - }, - }, - }, - }, - }, - - { - name: "Service RunLatest", - before: &Service{ - Spec: ServiceSpec{ - RunLatest: &RunLatestType{}, - }, - }, - after: &Service{ - Spec: ServiceSpec{ - RunLatest: &RunLatestType{ - Configuration: ConfigurationSpec{ - RevisionTemplate: RevisionTemplateSpec{ - Spec: RevisionSpec{ - ConcurrencyModel: RevisionRequestConcurrencyModelMulti, - }, - }, - }, - }, - }, - }, - }, - - { - name: "Service Pinned", - before: &Service{ - Spec: ServiceSpec{ - Pinned: &PinnedType{}, - }, - }, - after: &Service{ - Spec: ServiceSpec{ - Pinned: &PinnedType{ - Configuration: ConfigurationSpec{ - RevisionTemplate: RevisionTemplateSpec{ - Spec: RevisionSpec{ - ConcurrencyModel: RevisionRequestConcurrencyModelMulti, - }, - }, - }, - }, - }, - }, - }, } func TestDefaults(t *testing.T) { diff --git a/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go b/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go index 1dbda3eeb10b..4cf285435295 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.defaulters.go @@ -28,26 +28,11 @@ import ( // Public to allow building arbitrary schemes. // All generated defaulters are covering - they call all nested defaulters. func RegisterDefaults(scheme *runtime.Scheme) error { - scheme.AddTypeDefaultingFunc(&Configuration{}, func(obj interface{}) { SetObjectDefaults_Configuration(obj.(*Configuration)) }) - scheme.AddTypeDefaultingFunc(&ConfigurationList{}, func(obj interface{}) { SetObjectDefaults_ConfigurationList(obj.(*ConfigurationList)) }) scheme.AddTypeDefaultingFunc(&Revision{}, func(obj interface{}) { SetObjectDefaults_Revision(obj.(*Revision)) }) scheme.AddTypeDefaultingFunc(&RevisionList{}, func(obj interface{}) { SetObjectDefaults_RevisionList(obj.(*RevisionList)) }) - scheme.AddTypeDefaultingFunc(&Service{}, func(obj interface{}) { SetObjectDefaults_Service(obj.(*Service)) }) - scheme.AddTypeDefaultingFunc(&ServiceList{}, func(obj interface{}) { SetObjectDefaults_ServiceList(obj.(*ServiceList)) }) return nil } -func SetObjectDefaults_Configuration(in *Configuration) { - SetDefaults_Configuration(in) -} - -func SetObjectDefaults_ConfigurationList(in *ConfigurationList) { - for i := range in.Items { - a := &in.Items[i] - SetObjectDefaults_Configuration(a) - } -} - func SetObjectDefaults_Revision(in *Revision) { SetDefaults_Revision(in) } @@ -58,14 +43,3 @@ func SetObjectDefaults_RevisionList(in *RevisionList) { SetObjectDefaults_Revision(a) } } - -func SetObjectDefaults_Service(in *Service) { - SetDefaults_Service(in) -} - -func SetObjectDefaults_ServiceList(in *ServiceList) { - for i := range in.Items { - a := &in.Items[i] - SetObjectDefaults_Service(a) - } -} From 1c6b37882405b5d1b4d42966149b79fdf5e85284 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 15:45:17 -0700 Subject: [PATCH 6/8] Set defaults on reconciled objects Before reconciling a Revision, Configuration, Route, or Service, set its defaults using the scheme. --- pkg/controller/configuration/configuration.go | 3 +++ pkg/controller/revision/revision.go | 3 +++ pkg/controller/route/route.go | 3 +++ pkg/controller/service/service.go | 3 +++ 4 files changed, 12 insertions(+) diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index 240d71453449..74e0cf948147 100644 --- a/pkg/controller/configuration/configuration.go +++ b/pkg/controller/configuration/configuration.go @@ -23,6 +23,7 @@ import ( buildclientset "github.com/knative/build/pkg/client/clientset/versioned" "github.com/knative/serving/pkg/apis/serving" "github.com/knative/serving/pkg/apis/serving/v1alpha1" + "github.com/knative/serving/pkg/client/clientset/versioned/scheme" informers "github.com/knative/serving/pkg/client/informers/externalversions" listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" "github.com/knative/serving/pkg/controller" @@ -128,6 +129,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 diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 1be7742a30e6..065cf7f0d040 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -51,6 +51,7 @@ import ( buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" buildinformers "github.com/knative/build/pkg/client/informers/externalversions" "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" ) @@ -280,6 +281,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 { diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index f8431de472ba..981c332ea128 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -37,6 +37,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" informers "github.com/knative/serving/pkg/client/informers/externalversions" listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" "github.com/knative/serving/pkg/controller" @@ -211,6 +212,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) diff --git a/pkg/controller/service/service.go b/pkg/controller/service/service.go index d899d8682c48..36e62eb264d3 100644 --- a/pkg/controller/service/service.go +++ b/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" informers "github.com/knative/serving/pkg/client/informers/externalversions" listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" "github.com/knative/serving/pkg/controller" @@ -139,6 +140,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) From d4af96ad21b5081f311a33c6252e974585430cc1 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Fri, 15 Jun 2018 16:29:09 -0700 Subject: [PATCH 7/8] Remove defaulting from webhook Defaults are now set by controllers when objects with defaults are reconciled. As a consequence of this, setting defaults in templates, as in the Service and Configuration specs, is no longer necessary. --- pkg/webhook/configuration.go | 24 -------------- pkg/webhook/revision.go | 35 +-------------------- pkg/webhook/service.go | 33 ------------------- pkg/webhook/service_test.go | 61 ------------------------------------ pkg/webhook/webhook.go | 18 ----------- pkg/webhook/webhook_test.go | 7 ----- 6 files changed, 1 insertion(+), 177 deletions(-) diff --git a/pkg/webhook/configuration.go b/pkg/webhook/configuration.go index bcbcb583315c..5395c61e7001 100644 --- a/pkg/webhook/configuration.go +++ b/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) diff --git a/pkg/webhook/revision.go b/pkg/webhook/revision.go index 585b0b4b0246..957ae12aac54 100644 --- a/pkg/webhook/revision.go +++ b/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 diff --git a/pkg/webhook/service.go b/pkg/webhook/service.go index 2790112b68af..52c4e492aafe 100644 --- a/pkg/webhook/service.go +++ b/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) - } -} diff --git a/pkg/webhook/service_test.go b/pkg/webhook/service_test.go index 4afdbf975abe..08a9f4bf3909 100644 --- a/pkg/webhook/service_test.go +++ b/pkg/webhook/service_test.go @@ -19,7 +19,6 @@ import ( "testing" "github.com/knative/serving/pkg/apis/serving/v1alpha1" - "github.com/mattbaird/jsonpatch" ) func TestEmptySpec(t *testing.T) { @@ -112,63 +111,3 @@ func TestPinnedFailsWithNoConfiguration(t *testing.T) { t.Errorf("Expected %s got %s", e, a) } } - -func TestPinnedSetsDefaults(t *testing.T) { - s := v1alpha1.Service{ - Spec: v1alpha1.ServiceSpec{ - Pinned: &v1alpha1.PinnedType{ - Configuration: createConfiguration(1, "config").Spec, - }, - }, - } - - // Drop the ConcurrencyModel. - s.Spec.Pinned.Configuration.RevisionTemplate.Spec.ConcurrencyModel = "" - - var patches []jsonpatch.JsonPatchOperation - if err := SetServiceDefaults(testCtx)(&patches, &s); err != nil { - t.Errorf("Expected success, but failed with: %s", err) - } - - expected := jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/pinned/configuration/revisionTemplate/spec/concurrencyModel", - Value: v1alpha1.RevisionRequestConcurrencyModelMulti, - } - - if len(patches) != 1 { - t.Errorf("Unexpected number of patches: want 1, got %d", len(patches)) - } else if got, want := patches[0].Json(), expected.Json(); got != want { - t.Errorf("Unexpected patch: want %v, got %v", want, got) - } -} - -func TestLatestSetsDefaults(t *testing.T) { - s := v1alpha1.Service{ - Spec: v1alpha1.ServiceSpec{ - RunLatest: &v1alpha1.RunLatestType{ - Configuration: createConfiguration(1, "config").Spec, - }, - }, - } - - // Drop the ConcurrencyModel. - s.Spec.RunLatest.Configuration.RevisionTemplate.Spec.ConcurrencyModel = "" - - var patches []jsonpatch.JsonPatchOperation - if err := SetServiceDefaults(testCtx)(&patches, &s); err != nil { - t.Errorf("Expected success, but failed with: %s", err) - } - - expected := jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/runLatest/configuration/revisionTemplate/spec/concurrencyModel", - Value: v1alpha1.RevisionRequestConcurrencyModelMulti, - } - - if len(patches) != 1 { - t.Errorf("Unexpected number of patches: want 1, got %d", len(patches)) - } else if got, want := patches[0].Json(), expected.Json(); got != want { - t.Errorf("Unexpected patch: want %v, got %v", want, got) - } -} diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 0bc2200f170d..dfcabf5d3ceb 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -97,15 +97,9 @@ type ControllerOptions struct { // is denied. Mutations should be appended to the patches operations. type ResourceCallback func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error -// ResourceDefaulter defines a signature for resource specific (Route, Configuration, etc.) -// handlers that can set defaults on an object. If non-nil error is returned, object creation -// is denied. Mutations should be appended to the patches operations. -type ResourceDefaulter func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error - // GenericCRDHandler defines the factory object to use for unmarshaling incoming objects type GenericCRDHandler struct { Factory runtime.Object - Defaulter ResourceDefaulter Validator ResourceCallback } @@ -214,12 +208,10 @@ func NewAdmissionController(client kubernetes.Interface, options ControllerOptio handlers: map[string]GenericCRDHandler{ "Revision": GenericCRDHandler{ Factory: &v1alpha1.Revision{}, - Defaulter: SetRevisionDefaults(ctx), Validator: ValidateRevision(ctx), }, "Configuration": GenericCRDHandler{ Factory: &v1alpha1.Configuration{}, - Defaulter: SetConfigurationDefaults(ctx), Validator: ValidateConfiguration(ctx), }, "Route": GenericCRDHandler{ @@ -228,7 +220,6 @@ func NewAdmissionController(client kubernetes.Interface, options ControllerOptio }, "Service": GenericCRDHandler{ Factory: &v1alpha1.Service{}, - Defaulter: SetServiceDefaults(ctx), Validator: ValidateService(ctx), }, }, @@ -499,15 +490,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind string, oldBytes return nil, fmt.Errorf("Failed to update generation: %s", err) } - if defaulter := handler.Defaulter; defaulter != nil { - if err := defaulter(&patches, newObj); err != nil { - logger.Error("Failed the resource specific defaulter", zap.Error(err)) - // Return the error message as-is to give the defaulter callback - // discretion over (our portion of) the message that the user sees. - return nil, err - } - } - if err := handler.Validator(&patches, oldObj, newObj); err != nil { logger.Error("Failed the resource specific validation", zap.Error(err)) // Return the error message as-is to give the validation callback diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index c7e445dd234c..f7059abfbcb7 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -307,11 +307,6 @@ func TestValidNewRevisionObject(t *testing.T) { Path: "/spec/generation", Value: 1, }, - jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/servingState", - Value: v1alpha1.RevisionServingStateActive, - }, }) } @@ -691,7 +686,6 @@ func createConfiguration(generation int64, configurationName string) v1alpha1.Co Value: envVarValue, }}, }, - ConcurrencyModel: v1alpha1.RevisionRequestConcurrencyModelMulti, }, }, }, @@ -727,7 +721,6 @@ func createRevision(revName string) v1alpha1.Revision { Container: corev1.Container{ Image: "test-image", }, - ConcurrencyModel: v1alpha1.RevisionRequestConcurrencyModelMulti, }, } } From 25cab146c2ebccc453a38826b0d2555499e1c718 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 18 Jun 2018 14:12:51 -0700 Subject: [PATCH 8/8] Note future availability of OpenAPI defaults --- pkg/apis/serving/v1alpha1/defaults.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/apis/serving/v1alpha1/defaults.go b/pkg/apis/serving/v1alpha1/defaults.go index 2231c7839ad7..1a0c47d161f8 100644 --- a/pkg/apis/serving/v1alpha1/defaults.go +++ b/pkg/apis/serving/v1alpha1/defaults.go @@ -38,6 +38,9 @@ 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