From a7c9a1228692338c842e93a073f3ca4642b0c660 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 27 Jan 2015 18:56:38 -0500 Subject: [PATCH] Add name generation and normalize common create flows Adds `ObjectMeta.GenerateName`, an optional string field that defines name generation behavior if a Name is not provided. Adds `pkg/api/rest`, which defines the default Kubernetes API pattern for creation (and will cover update as well). Will allow registries and REST objects to be merged by moving logic on api out of those places. Add `pkg/api/rest/resttest`, which will be the test suite that verifies a RESTStorage object follows the Kubernetes API conventions and begin reducing our duplicated tests. --- pkg/api/generate.go | 74 ++++++++++++++++ pkg/api/generate_test.go | 79 +++++++++++++++++ pkg/api/meta.go | 17 ++++ pkg/api/meta/meta.go | 46 +++------- pkg/api/rest/create.go | 66 ++++++++++++++ pkg/api/rest/doc.go | 18 ++++ pkg/api/rest/resttest/resttest.go | 118 ++++++++++++++++++++++++++ pkg/api/rest/types.go | 47 ++++++++++ pkg/api/types.go | 15 +++- pkg/api/v1beta1/types.go | 13 +++ pkg/api/v1beta2/types.go | 13 +++ pkg/api/v1beta3/types.go | 13 +++ pkg/api/validation/validation.go | 48 +++++++---- pkg/api/validation/validation_test.go | 62 +++++++++++++- pkg/registry/controller/rest.go | 19 ++--- pkg/registry/controller/rest_test.go | 55 +++++++++++- pkg/runtime/helper.go | 24 ++++++ pkg/runtime/types.go | 5 +- 18 files changed, 662 insertions(+), 70 deletions(-) create mode 100644 pkg/api/generate.go create mode 100644 pkg/api/generate_test.go create mode 100644 pkg/api/rest/create.go create mode 100644 pkg/api/rest/doc.go create mode 100644 pkg/api/rest/resttest/resttest.go create mode 100644 pkg/api/rest/types.go diff --git a/pkg/api/generate.go b/pkg/api/generate.go new file mode 100644 index 00000000000..5447ff722d5 --- /dev/null +++ b/pkg/api/generate.go @@ -0,0 +1,74 @@ +/* +Copyright 2014 Google Inc. 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 api + +import ( + "fmt" + "math/rand" +) + +// NameGenerator generates names for objects. Some backends may have more information +// available to guide selection of new names and this interface hides those details. +type NameGenerator interface { + // GenerateName generates a valid name from the base name, adding a random suffix to the + // the base. If base is valid, the returned name must also be valid. The generator is + // responsible for knowing the maximum valid name length. + GenerateName(base string) string +} + +// GenerateName will resolve the object name of the provided ObjectMeta to a generated version if +// necessary. It expects that validation for ObjectMeta has already completed (that Base is a +// valid name) and that the NameGenerator generates a name that is also valid. +func GenerateName(u NameGenerator, meta *ObjectMeta) { + if len(meta.GenerateName) == 0 || len(meta.Name) != 0 { + return + } + meta.Name = u.GenerateName(meta.GenerateName) +} + +// simpleNameGenerator generates random names. +type simpleNameGenerator struct{} + +// SimpleNameGenerator is a generator that returns the name plus a random suffix of five alphanumerics +// when a name is requested. The string is guaranteed to not exceed the length of a standard Kubernetes +// name (63 characters) +var SimpleNameGenerator NameGenerator = simpleNameGenerator{} + +const ( + // TODO: make this flexible for non-core resources with alternate naming rules. + maxNameLength = 63 + randomLength = 5 + maxGeneratedNameLength = maxNameLength - randomLength +) + +func (simpleNameGenerator) GenerateName(base string) string { + if len(base) > maxGeneratedNameLength { + base = base[:maxGeneratedNameLength] + } + value := randSeq(randomLength) + return fmt.Sprintf("%s%s", base, value) +} + +var letters = []rune("abcdefghijklmnopqrstuvwxyz0123456789") + +func randSeq(n int) string { + b := make([]rune, n) + for i := range b { + b[i] = letters[rand.Intn(len(letters))] + } + return string(b) +} diff --git a/pkg/api/generate_test.go b/pkg/api/generate_test.go new file mode 100644 index 00000000000..7f943f21dac --- /dev/null +++ b/pkg/api/generate_test.go @@ -0,0 +1,79 @@ +/* +Copyright 2014 Google Inc. 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 api + +import ( + "strings" + "testing" +) + +type nameGeneratorFunc func(base string) string + +func (fn nameGeneratorFunc) GenerateName(base string) string { + return fn(base) +} + +func TestGenerateName(t *testing.T) { + testCases := []struct { + meta ObjectMeta + + base string + returned string + }{ + { + returned: "", + }, + { + meta: ObjectMeta{ + GenerateName: "test", + }, + base: "test", + returned: "test", + }, + { + meta: ObjectMeta{ + Name: "foo", + GenerateName: "test", + }, + base: "test", + returned: "foo", + }, + } + + for i, testCase := range testCases { + GenerateName(nameGeneratorFunc(func(base string) string { + if base != testCase.base { + t.Errorf("%d: unexpected call with base", i) + } + return "test" + }), &testCase.meta) + expect := testCase.returned + if expect != testCase.meta.Name { + t.Errorf("%d: unexpected name: %#v", i, testCase.meta) + } + } +} + +func TestSimpleNameGenerator(t *testing.T) { + meta := &ObjectMeta{ + GenerateName: "foo", + } + GenerateName(SimpleNameGenerator, meta) + if !strings.HasPrefix(meta.Name, "foo") || meta.Name == "foo" { + t.Errorf("unexpected name: %#v", meta) + } +} diff --git a/pkg/api/meta.go b/pkg/api/meta.go index 4bdfcdacd73..5782be6558f 100644 --- a/pkg/api/meta.go +++ b/pkg/api/meta.go @@ -17,6 +17,8 @@ limitations under the License. package api import ( + "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) @@ -24,6 +26,7 @@ import ( func FillObjectMetaSystemFields(ctx Context, meta *ObjectMeta) { meta.CreationTimestamp = util.Now() meta.UID = util.NewUUID() + meta.SelfLink = "" } // HasObjectMetaSystemFieldValues returns true if fields that are managed by the system on ObjectMeta have values. @@ -31,3 +34,17 @@ func HasObjectMetaSystemFieldValues(meta *ObjectMeta) bool { return !meta.CreationTimestamp.Time.IsZero() || len(meta.UID) != 0 } + +// GetObjectMetaPtr returns a pointer to a provided object's ObjectMeta. +// TODO: allow runtime.Unknown to extract this object +func ObjectMetaFor(obj runtime.Object) (*ObjectMeta, error) { + v, err := conversion.EnforcePtr(obj) + if err != nil { + return nil, err + } + var objectMeta *ObjectMeta + if err := runtime.FieldPtr(v, "ObjectMeta", &objectMeta); err != nil { + return nil, err + } + return objectMeta, nil +} diff --git a/pkg/api/meta/meta.go b/pkg/api/meta/meta.go index 1353083474d..ce11b272e39 100644 --- a/pkg/api/meta/meta.go +++ b/pkg/api/meta/meta.go @@ -373,36 +373,12 @@ func (a genericAccessor) SetAnnotations(annotations map[string]string) { *a.annotations = annotations } -// fieldPtr puts the address of fieldName, which must be a member of v, -// into dest, which must be an address of a variable to which this field's -// address can be assigned. -func fieldPtr(v reflect.Value, fieldName string, dest interface{}) error { - field := v.FieldByName(fieldName) - if !field.IsValid() { - return fmt.Errorf("couldn't find %v field in %#v", fieldName, v.Interface()) - } - v, err := conversion.EnforcePtr(dest) - if err != nil { - return err - } - field = field.Addr() - if field.Type().AssignableTo(v.Type()) { - v.Set(field) - return nil - } - if field.Type().ConvertibleTo(v.Type()) { - v.Set(field.Convert(v.Type())) - return nil - } - return fmt.Errorf("couldn't assign/convert %v to %v", field.Type(), v.Type()) -} - // extractFromTypeMeta extracts pointers to version and kind fields from an object func extractFromTypeMeta(v reflect.Value, a *genericAccessor) error { - if err := fieldPtr(v, "APIVersion", &a.apiVersion); err != nil { + if err := runtime.FieldPtr(v, "APIVersion", &a.apiVersion); err != nil { return err } - if err := fieldPtr(v, "Kind", &a.kind); err != nil { + if err := runtime.FieldPtr(v, "Kind", &a.kind); err != nil { return err } return nil @@ -410,25 +386,25 @@ func extractFromTypeMeta(v reflect.Value, a *genericAccessor) error { // extractFromObjectMeta extracts pointers to metadata fields from an object func extractFromObjectMeta(v reflect.Value, a *genericAccessor) error { - if err := fieldPtr(v, "Namespace", &a.namespace); err != nil { + if err := runtime.FieldPtr(v, "Namespace", &a.namespace); err != nil { return err } - if err := fieldPtr(v, "Name", &a.name); err != nil { + if err := runtime.FieldPtr(v, "Name", &a.name); err != nil { return err } - if err := fieldPtr(v, "UID", &a.uid); err != nil { + if err := runtime.FieldPtr(v, "UID", &a.uid); err != nil { return err } - if err := fieldPtr(v, "ResourceVersion", &a.resourceVersion); err != nil { + if err := runtime.FieldPtr(v, "ResourceVersion", &a.resourceVersion); err != nil { return err } - if err := fieldPtr(v, "SelfLink", &a.selfLink); err != nil { + if err := runtime.FieldPtr(v, "SelfLink", &a.selfLink); err != nil { return err } - if err := fieldPtr(v, "Labels", &a.labels); err != nil { + if err := runtime.FieldPtr(v, "Labels", &a.labels); err != nil { return err } - if err := fieldPtr(v, "Annotations", &a.annotations); err != nil { + if err := runtime.FieldPtr(v, "Annotations", &a.annotations); err != nil { return err } return nil @@ -436,10 +412,10 @@ func extractFromObjectMeta(v reflect.Value, a *genericAccessor) error { // extractFromObjectMeta extracts pointers to metadata fields from a list object func extractFromListMeta(v reflect.Value, a *genericAccessor) error { - if err := fieldPtr(v, "ResourceVersion", &a.resourceVersion); err != nil { + if err := runtime.FieldPtr(v, "ResourceVersion", &a.resourceVersion); err != nil { return err } - if err := fieldPtr(v, "SelfLink", &a.selfLink); err != nil { + if err := runtime.FieldPtr(v, "SelfLink", &a.selfLink); err != nil { return err } return nil diff --git a/pkg/api/rest/create.go b/pkg/api/rest/create.go new file mode 100644 index 00000000000..f7601c9668c --- /dev/null +++ b/pkg/api/rest/create.go @@ -0,0 +1,66 @@ +/* +Copyright 2014 Google Inc. 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 rest + +import ( + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" +) + +// RESTCreateStrategy defines the minimum validation, accepted input, and +// name generation behavior to create an object that follows Kubernetes +// API conventions. +type RESTCreateStrategy interface { + runtime.ObjectTyper + // The name generate is used when the standard GenerateName field is set. + // The NameGenerator will be invoked prior to validation. + api.NameGenerator + + // ResetBeforeCreate is invoked on create before validation to remove any fields + // that may not be persisted. + ResetBeforeCreate(obj runtime.Object) + // Validate is invoked after default fields in the object have been filled in before + // the object is persisted. + Validate(obj runtime.Object) errors.ValidationErrorList +} + +// BeforeCreate ensures that common operations for all resources are performed on creation. It only returns +// errors that can be converted to api.Status. It invokes ResetBeforeCreate, then GenerateName, then Validate. +// It returns nil if the object should be created. +func BeforeCreate(strategy RESTCreateStrategy, ctx api.Context, obj runtime.Object) error { + _, kind, err := strategy.ObjectVersionAndKind(obj) + if err != nil { + return errors.NewInternalError(err) + } + objectMeta, err := api.ObjectMetaFor(obj) + if err != nil { + return errors.NewInternalError(err) + } + + if !api.ValidNamespace(ctx, objectMeta) { + return errors.NewBadRequest("the namespace of the provided object does not match the namespace sent on the request") + } + strategy.ResetBeforeCreate(obj) + api.FillObjectMetaSystemFields(ctx, objectMeta) + api.GenerateName(strategy, objectMeta) + + if errs := strategy.Validate(obj); len(errs) > 0 { + return errors.NewInvalid(kind, objectMeta.Name, errs) + } + return nil +} diff --git a/pkg/api/rest/doc.go b/pkg/api/rest/doc.go new file mode 100644 index 00000000000..01d0e6ea553 --- /dev/null +++ b/pkg/api/rest/doc.go @@ -0,0 +1,18 @@ +/* +Copyright 2014 Google Inc. 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 rest defines common logic around changes to Kubernetes resources. +package rest diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go new file mode 100644 index 00000000000..1f8ef774470 --- /dev/null +++ b/pkg/api/rest/resttest/resttest.go @@ -0,0 +1,118 @@ +/* +Copyright 2014 Google Inc. 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 resttest + +import ( + "strings" + "testing" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" +) + +type Tester struct { + *testing.T + storage apiserver.RESTStorage +} + +func New(t *testing.T, storage apiserver.RESTStorage) *Tester { + return &Tester{ + T: t, + storage: storage, + } +} + +func copyOrDie(obj runtime.Object) runtime.Object { + out, err := api.Scheme.Copy(obj) + if err != nil { + panic(err) + } + return out +} + +func (t *Tester) TestCreate(valid runtime.Object, invalid ...runtime.Object) { + t.TestCreateHasMetadata(copyOrDie(valid)) + t.TestCreateGeneratesName(copyOrDie(valid)) + t.TestCreateRejectsMismatchedNamespace(copyOrDie(valid)) + t.TestCreateInvokesValidation(invalid...) +} + +func (t *Tester) TestCreateHasMetadata(valid runtime.Object) { + objectMeta, err := api.ObjectMetaFor(valid) + if err != nil { + t.Fatalf("object does not have ObjectMeta: %v\n%#v", err, valid) + } + + objectMeta.Name = "test" + objectMeta.Namespace = api.NamespaceDefault + + channel, err := t.storage.(apiserver.RESTCreater).Create(api.NewDefaultContext(), valid) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if obj := <-channel; obj.Object == nil { + t.Fatalf("Unexpected object from channel: %#v", obj) + } + if !api.HasObjectMetaSystemFieldValues(objectMeta) { + t.Errorf("storage did not populate object meta field values") + } +} + +func (t *Tester) TestCreateGeneratesName(valid runtime.Object) { + objectMeta, err := api.ObjectMetaFor(valid) + if err != nil { + t.Fatalf("object does not have ObjectMeta: %v\n%#v", err, valid) + } + + objectMeta.GenerateName = "test-" + + _, err = t.storage.(apiserver.RESTCreater).Create(api.NewDefaultContext(), valid) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if objectMeta.Name == "test-" || !strings.HasPrefix(objectMeta.Name, "test-") { + t.Errorf("unexpected name: %#v", valid) + } +} + +func (t *Tester) TestCreateInvokesValidation(invalid ...runtime.Object) { + for i, obj := range invalid { + ctx := api.NewDefaultContext() + _, err := t.storage.(apiserver.RESTCreater).Create(ctx, obj) + if !errors.IsInvalid(err) { + t.Errorf("%d: Expected to get an invalid resource error, got %v", i, err) + } + } +} + +func (t *Tester) TestCreateRejectsMismatchedNamespace(valid runtime.Object) { + objectMeta, err := api.ObjectMetaFor(valid) + if err != nil { + t.Fatalf("object does not have ObjectMeta: %v\n%#v", err, valid) + } + + objectMeta.Namespace = "not-default" + + _, err = t.storage.(apiserver.RESTCreater).Create(api.NewDefaultContext(), valid) + if err == nil { + t.Errorf("Expected an error, but we didn't get one") + } else if strings.Contains(err.Error(), "Controller.Namespace does not match the provided context") { + t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error()) + } +} diff --git a/pkg/api/rest/types.go b/pkg/api/rest/types.go new file mode 100644 index 00000000000..11b8428ada2 --- /dev/null +++ b/pkg/api/rest/types.go @@ -0,0 +1,47 @@ +/* +Copyright 2014 Google Inc. 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 rest + +import ( + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" +) + +// rcStrategy implements behavior for Replication Controllers. +// TODO: move to a replicationcontroller specific package. +type rcStrategy struct { + runtime.ObjectTyper + api.NameGenerator +} + +// ReplicationControllers is the default logic that applies when creating and updating Replication Controller +// objects. +var ReplicationControllers RESTCreateStrategy = rcStrategy{api.Scheme, api.SimpleNameGenerator} + +// ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. +func (rcStrategy) ResetBeforeCreate(obj runtime.Object) { + controller := obj.(*api.ReplicationController) + controller.Status = api.ReplicationControllerStatus{} +} + +// Validate validates a new replication controller. +func (rcStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { + controller := obj.(*api.ReplicationController) + return validation.ValidateReplicationController(controller) +} diff --git a/pkg/api/types.go b/pkg/api/types.go index 3d72b98ed9e..c9f94215d95 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -74,7 +74,7 @@ type ListMeta struct { } // ObjectMeta is metadata that all persisted resources must have, which includes all objects -// users must create. A resource may have only one of {ObjectMeta, ListMeta}. +// users must create. type ObjectMeta struct { // Name is unique within a namespace. Name is required when creating resources, although // some resources may allow a client to request the generation of an appropriate name @@ -82,6 +82,19 @@ type ObjectMeta struct { // definition. Name string `json:"name,omitempty"` + // GenerateName indicates that the name should be made unique by the server prior to persisting + // it. A non-empty value for the field indicates the name will be made unique (and the name + // returned to the client will be different than the name passed). The value of this field will + // be combined with a unique suffix on the server if the Name field has not been provided. + // The provided value must be valid within the rules for Name, and may be truncated by the length + // of the suffix required to make the value unique on the server. + // + // If this field is specified, and Name is not present, the server will NOT return a 409 if the + // generated name exists - instead, it will either return 201 Created or 500 with Reason + // TryAgainLater indicating a unique name could not be found in the time allotted, and the client + // should retry (optionally after the time indicated in the Retry-After header). + GenerateName string `json:"generateName,omitempty"` + // Namespace defines the space within which name must be unique. An empty namespace is // equivalent to the "default" namespace, but "default" is the canonical representation. // Not all objects are required to be scoped to a namespace - the value of this field for diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index 652080d8bc4..4c02861ea87 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -326,6 +326,19 @@ type TypeMeta struct { APIVersion string `json:"apiVersion,omitempty" description:"version of the schema the object should have"` Namespace string `json:"namespace,omitempty" description:"namespace to which the object belongs; must be a DNS_SUBDOMAIN; 'default' by default"` + // GenerateName indicates that the name should be made unique by the server prior to persisting + // it. A non-empty value for the field indicates the name will be made unique (and the name + // returned to the client will be different than the name passed). The value of this field will + // be combined with a unique suffix on the server if the Name field has not been provided. + // The provided value must be valid within the rules for Name, and may be truncated by the length + // of the suffix required to make the value unique on the server. + // + // If this field is specified, and Name is not present, the server will NOT return a 409 if the + // generated name exists - instead, it will either return 201 Created or 500 with Reason + // TryAgainLater indicating a unique name could not be found in the time allotted, and the client + // should retry (optionally after the time indicated in the Retry-After header). + GenerateName string `json:"generateName,omitempty" description:"an optional prefix to use to generate a unique name; has the same validation rules as name; optional, and is applied only name if is not specified"` + // Annotations are unstructured key value data stored with a resource that may be set by // external tooling. They are not queryable and should be preserved when modifying // objects. diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index d2c52c4b3eb..2d00faff0aa 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -290,6 +290,19 @@ type TypeMeta struct { APIVersion string `json:"apiVersion,omitempty" description:"version of the schema the object should have"` Namespace string `json:"namespace,omitempty" description:"namespace to which the object belongs; must be a DNS_SUBDOMAIN; 'default' by default"` + // GenerateName indicates that the name should be made unique by the server prior to persisting + // it. A non-empty value for the field indicates the name will be made unique (and the name + // returned to the client will be different than the name passed). The value of this field will + // be combined with a unique suffix on the server if the Name field has not been provided. + // The provided value must be valid within the rules for Name, and may be truncated by the length + // of the suffix required to make the value unique on the server. + // + // If this field is specified, and Name is not present, the server will NOT return a 409 if the + // generated name exists - instead, it will either return 201 Created or 500 with Reason + // TryAgainLater indicating a unique name could not be found in the time allotted, and the client + // should retry (optionally after the time indicated in the Retry-After header). + GenerateName string `json:"generateName,omitempty" description:"an optional prefix to use to generate a unique name; has the same validation rules as name; optional, and is applied only name if is not specified"` + // Annotations are unstructured key value data stored with a resource that may be set by // external tooling. They are not queryable and should be preserved when modifying // objects. diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 1088d68a956..761e1c58f20 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -82,6 +82,19 @@ type ObjectMeta struct { // definition. Name string `json:"name,omitempty"` + // GenerateName indicates that the name should be made unique by the server prior to persisting + // it. A non-empty value for the field indicates the name will be made unique (and the name + // returned to the client will be different than the name passed). The value of this field will + // be combined with a unique suffix on the server if the Name field has not been provided. + // The provided value must be valid within the rules for Name, and may be truncated by the length + // of the suffix required to make the value unique on the server. + // + // If this field is specified, and Name is not present, the server will NOT return a 409 if the + // generated name exists - instead, it will either return 201 Created or 500 with Reason + // TryAgainLater indicating a unique name could not be found in the time allotted, and the client + // should retry (optionally after the time indicated in the Retry-After header). + GenerateName string `json:"generateName,omitempty" description:"an optional prefix to use to generate a unique name; has the same validation rules as name; optional, and is applied only name if is not specified"` + // Namespace defines the space within which name must be unique. An empty namespace is // equivalent to the "default" namespace, but "default" is the canonical representation. // Not all objects are required to be scoped to a namespace - the value of this field for diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 93d62246ea7..e7f5d27828a 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -53,11 +53,24 @@ func ValidateAnnotations(annotations map[string]string, field string) errs.Valid } // ValidateNameFunc validates that the provided name is valid for a given resource type. -// Not all resources have the same validation rules for names. -type ValidateNameFunc func(name string) (bool, string) +// Not all resources have the same validation rules for names. Prefix is true if the +// name will have a value appended to it. +type ValidateNameFunc func(name string, prefix bool) (bool, string) + +// maskTrailingDash replaces the final character of a string with a subdomain safe +// value if is a dash. +func maskTrailingDash(name string) string { + if strings.HasSuffix(name, "-") { + return name[:len(name)-2] + "a" + } + return name +} // nameIsDNSSubdomain is a ValidateNameFunc for names that must be a DNS subdomain. -func nameIsDNSSubdomain(name string) (bool, string) { +func nameIsDNSSubdomain(name string, prefix bool) (bool, string) { + if prefix { + name = maskTrailingDash(name) + } if util.IsDNSSubdomain(name) { return true, "" } @@ -65,23 +78,36 @@ func nameIsDNSSubdomain(name string) (bool, string) { } // nameIsDNS952Label is a ValidateNameFunc for names that must be a DNS 952 label. -func nameIsDNS952Label(name string) (bool, string) { +func nameIsDNS952Label(name string, prefix bool) (bool, string) { + if prefix { + name = maskTrailingDash(name) + } if util.IsDNS952Label(name) { return true, "" } return false, "name must be lowercase letters, numbers, and dashes" } -// ValidateObjectMeta validates an object's metadata. +// ValidateObjectMeta validates an object's metadata on creation. It expects that name generation has already +// been performed. func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn ValidateNameFunc) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} + + if len(meta.GenerateName) != 0 { + if ok, qualifier := nameFn(meta.GenerateName, true); !ok { + allErrs = append(allErrs, errs.NewFieldInvalid("generateName", meta.GenerateName, qualifier)) + } + } + // if the generated name validates, but the calculated value does not, it's a problem with generation, and we + // report it here. This may confuse users, but indicates a programming bug and still must be validated. if len(meta.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", meta.Name)) } else { - if ok, qualifier := nameFn(meta.Name); !ok { + if ok, qualifier := nameFn(meta.Name, false); !ok { allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, qualifier)) } } + if requiresNamespace { if len(meta.Namespace) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("namespace", meta.Namespace)) @@ -96,10 +122,6 @@ func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn Val allErrs = append(allErrs, ValidateLabels(meta.Labels, "labels")...) allErrs = append(allErrs, ValidateAnnotations(meta.Annotations, "annotations")...) - // Clear self link internally - // TODO: move to its own area - meta.SelfLink = "" - return allErrs } @@ -131,10 +153,6 @@ func ValidateObjectMetaUpdate(old, meta *api.ObjectMeta) errs.ValidationErrorLis allErrs = append(allErrs, ValidateLabels(meta.Labels, "labels")...) allErrs = append(allErrs, ValidateAnnotations(meta.Annotations, "annotations")...) - // Clear self link internally - // TODO: move to its own area - meta.SelfLink = "" - return allErrs } @@ -660,7 +678,7 @@ func ValidateBoundPod(pod *api.BoundPod) errs.ValidationErrorList { if len(pod.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name)) } else { - if ok, qualifier := nameIsDNSSubdomain(pod.Name); !ok { + if ok, qualifier := nameIsDNSSubdomain(pod.Name, false); !ok { allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, qualifier)) } } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 745e653ddea..210f4c72963 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -37,6 +37,30 @@ func expectPrefix(t *testing.T, prefix string, errs errors.ValidationErrorList) } } +// Ensure custom name functions are allowed +func TestValidateObjectMetaCustomName(t *testing.T) { + errs := ValidateObjectMeta(&api.ObjectMeta{Name: "test", GenerateName: "foo"}, false, func(s string, prefix bool) (bool, string) { + if s == "test" { + return true, "" + } + return false, "name-gen" + }) + if len(errs) != 1 { + t.Fatalf("unexpected errors: %v", errs) + } + if !strings.Contains(errs[0].Error(), "name-gen") { + t.Errorf("unexpected error message: %v", errs) + } +} + +// Ensure trailing slash is allowed in generate name +func TestValidateObjectMetaTrimsTrailingSlash(t *testing.T) { + errs := ValidateObjectMeta(&api.ObjectMeta{Name: "test", GenerateName: "foo-"}, false, nameIsDNSSubdomain) + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } +} + func TestValidateLabels(t *testing.T) { successCases := []map[string]string{ {"simple": "bar"}, @@ -953,7 +977,7 @@ func TestValidateService(t *testing.T) { { name: "invalid id", svc: api.Service{ - ObjectMeta: api.ObjectMeta{Name: "123abc", Namespace: api.NamespaceDefault}, + ObjectMeta: api.ObjectMeta{Name: "-123abc", Namespace: api.NamespaceDefault}, Spec: api.ServiceSpec{ Port: 8675, Selector: map[string]string{"foo": "bar"}, @@ -962,6 +986,38 @@ func TestValidateService(t *testing.T) { // Should fail because the ID is invalid. numErrs: 1, }, + { + name: "invalid generate.base", + svc: api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "valid", + GenerateName: "-123abc", + Namespace: api.NamespaceDefault, + }, + Spec: api.ServiceSpec{ + Port: 8675, + Selector: map[string]string{"foo": "bar"}, + }, + }, + // Should fail because the Base value for generation is invalid + numErrs: 1, + }, + { + name: "invalid generateName", + svc: api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "valid", + GenerateName: "abc1234567abc1234567abc1234567abc1234567abc1234567abc1234567", + Namespace: api.NamespaceDefault, + }, + Spec: api.ServiceSpec{ + Port: 8675, + Selector: map[string]string{"foo": "bar"}, + }, + }, + // Should fail because the generate name type is invalid. + numErrs: 1, + }, { name: "missing port", svc: api.Service{ @@ -1426,7 +1482,9 @@ func TestValidateMinion(t *testing.T) { }, }, { - ObjectMeta: api.ObjectMeta{Name: "abc"}, + ObjectMeta: api.ObjectMeta{ + Name: "abc", + }, Status: api.NodeStatus{ HostIP: "something", }, diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index 5171914af59..bd8f75e3259 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -21,12 +21,12 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" rc "github.com/GoogleCloudPlatform/kubernetes/pkg/controller" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" - "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" ) @@ -55,23 +55,14 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE if !ok { return nil, fmt.Errorf("not a replication controller: %#v", obj) } - if !api.ValidNamespace(ctx, &controller.ObjectMeta) { - return nil, errors.NewConflict("controller", controller.Namespace, fmt.Errorf("Controller.Namespace does not match the provided context")) - } - if len(controller.Name) == 0 { - controller.Name = string(util.NewUUID()) + if err := rest.BeforeCreate(rest.ReplicationControllers, ctx, obj); err != nil { + return nil, err } - if errs := validation.ValidateReplicationController(controller); len(errs) > 0 { - return nil, errors.NewInvalid("replicationController", controller.Name, errs) - } - - api.FillObjectMetaSystemFields(ctx, &controller.ObjectMeta) return apiserver.MakeAsync(func() (runtime.Object, error) { - err := rs.registry.CreateController(ctx, controller) - if err != nil { - return nil, err + if err := rs.registry.CreateController(ctx, controller); err != nil { + return apiserver.RESTResult{}, err } return rs.registry.GetController(ctx, controller.Name) }), nil diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index dc5455959b1..ed76e04ed81 100644 --- a/pkg/registry/controller/rest_test.go +++ b/pkg/registry/controller/rest_test.go @@ -27,6 +27,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest/resttest" _ "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" @@ -106,6 +107,7 @@ func TestListControllerList(t *testing.T) { } } +// TODO: remove, this is sufficiently covered by other tests func TestControllerDecode(t *testing.T) { mockRegistry := registrytest.ControllerRegistry{} storage := REST{ @@ -140,6 +142,7 @@ func TestControllerDecode(t *testing.T) { } } +// TODO: this is sufficiently covered by other tetss func TestControllerParsing(t *testing.T) { expectedController := api.ReplicationController{ ObjectMeta: api.ObjectMeta{ @@ -228,6 +231,7 @@ var validPodTemplate = api.PodTemplate{ }, } +// TODO: remove, this is sufficiently covered by other tests func TestCreateController(t *testing.T) { mockRegistry := registrytest.ControllerRegistry{} mockPodRegistry := registrytest.PodRegistry{ @@ -274,6 +278,7 @@ func TestCreateController(t *testing.T) { } } +// TODO: remove, covered by TestCreate func TestControllerStorageValidatesCreate(t *testing.T) { mockRegistry := registrytest.ControllerRegistry{} storage := REST{ @@ -376,6 +381,32 @@ func TestFillCurrentState(t *testing.T) { } } +// TODO: remove, covered by TestCreate +func TestCreateControllerWithGeneratedName(t *testing.T) { + storage := NewREST(®istrytest.ControllerRegistry{}, nil) + controller := &api.ReplicationController{ + ObjectMeta: api.ObjectMeta{ + Namespace: api.NamespaceDefault, + GenerateName: "rc-", + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 2, + Selector: map[string]string{"a": "b"}, + Template: &validPodTemplate.Spec, + }, + } + + ctx := api.NewDefaultContext() + _, err := storage.Create(ctx, controller) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if controller.Name == "rc-" || !strings.HasPrefix(controller.Name, "rc-") { + t.Errorf("unexpected name: %#v", controller) + } +} + +// TODO: remove, covered by TestCreate func TestCreateControllerWithConflictingNamespace(t *testing.T) { storage := REST{} controller := &api.ReplicationController{ @@ -389,7 +420,7 @@ func TestCreateControllerWithConflictingNamespace(t *testing.T) { } if err == nil { t.Errorf("Expected an error, but we didn't get one") - } else if strings.Index(err.Error(), "Controller.Namespace does not match the provided context") == -1 { + } else if strings.Contains(err.Error(), "Controller.Namespace does not match the provided context") { t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error()) } } @@ -411,3 +442,25 @@ func TestUpdateControllerWithConflictingNamespace(t *testing.T) { t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error()) } } + +func TestCreate(t *testing.T) { + test := resttest.New(t, NewREST(®istrytest.ControllerRegistry{}, nil)) + test.TestCreate( + // valid + &api.ReplicationController{ + Spec: api.ReplicationControllerSpec{ + Replicas: 2, + Selector: map[string]string{"a": "b"}, + Template: &validPodTemplate.Spec, + }, + }, + // invalid + &api.ReplicationController{ + Spec: api.ReplicationControllerSpec{ + Replicas: 2, + Selector: map[string]string{}, + Template: &validPodTemplate.Spec, + }, + }, + ) +} diff --git a/pkg/runtime/helper.go b/pkg/runtime/helper.go index 8681912a869..aba69f3cb53 100644 --- a/pkg/runtime/helper.go +++ b/pkg/runtime/helper.go @@ -114,3 +114,27 @@ func SetList(list Object, objects []Object) error { items.Set(slice) return nil } + +// fieldPtr puts the address of fieldName, which must be a member of v, +// into dest, which must be an address of a variable to which this field's +// address can be assigned. +func FieldPtr(v reflect.Value, fieldName string, dest interface{}) error { + field := v.FieldByName(fieldName) + if !field.IsValid() { + return fmt.Errorf("couldn't find %v field in %#v", fieldName, v.Interface()) + } + v, err := conversion.EnforcePtr(dest) + if err != nil { + return err + } + field = field.Addr() + if field.Type().AssignableTo(v.Type()) { + v.Set(field) + return nil + } + if field.Type().ConvertibleTo(v.Type()) { + v.Set(field.Convert(v.Type())) + return nil + } + return fmt.Errorf("couldn't assign/convert %v to %v", field.Type(), v.Type()) +} diff --git a/pkg/runtime/types.go b/pkg/runtime/types.go index fec13030206..8f8a857c1a2 100644 --- a/pkg/runtime/types.go +++ b/pkg/runtime/types.go @@ -104,8 +104,9 @@ type RawExtension struct { // Unknown allows api objects with unknown types to be passed-through. This can be used // to deal with the API objects from a plug-in. Unknown objects still have functioning -// TypeMeta features-- kind, version, resourceVersion, etc. -// TODO: Not implemented yet! +// TypeMeta features-- kind, version, etc. +// TODO: Make this object have easy access to field based accessors and settors for +// metadata and field mutatation. type Unknown struct { TypeMeta `json:",inline"` // RawJSON will hold the complete JSON of the object which couldn't be matched