diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index 624bcbd6c44e..b5b30c01f33c 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 ${SERVING_ROOT}/hack/boilerplate/boilerplate.go.txt + # Make sure our dependencies are up-to-date ${SERVING_ROOT}/hack/update-deps.sh diff --git a/pkg/apis/serving/v1alpha1/defaults.go b/pkg/apis/serving/v1alpha1/defaults.go new file mode 100644 index 000000000000..1a0c47d161f8 --- /dev/null +++ b/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 + } +} diff --git a/pkg/apis/serving/v1alpha1/defaults_test.go b/pkg/apis/serving/v1alpha1/defaults_test.go new file mode 100644 index 000000000000..a56edded43c0 --- /dev/null +++ b/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 +} 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/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) + } +} diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index 78729fc270c8..78bd1c1c2682 100644 --- a/pkg/controller/configuration/configuration.go +++ b/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 diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 0aa65d56a82d..038c38f0106d 100644 --- a/pkg/controller/revision/revision.go +++ b/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 { diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 61c0e3a5f289..a9d1889c8ba7 100644 --- a/pkg/controller/route/route.go +++ b/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) diff --git a/pkg/controller/service/service.go b/pkg/controller/service/service.go index 8b21ecc1434d..c6565f636932 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" 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) 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, }, } }