From a7c9a1228692338c842e93a073f3ca4642b0c660 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 27 Jan 2015 18:56:38 -0500 Subject: [PATCH 1/6] 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 From 5603714df85cbba131b337aab1f0de31afa125fa Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 27 Jan 2015 23:50:01 -0500 Subject: [PATCH 2/6] Use name generation on pods via replication controllers The generated name is '-%s', unless controllerName- would be long enough to cause a validation error. --- pkg/api/rest/resttest/resttest.go | 23 ++++++ pkg/api/rest/types.go | 23 ++++++ pkg/api/testing/fuzzer.go | 3 + pkg/api/v1beta1/conversion.go | 2 + pkg/api/v1beta2/conversion.go | 2 + pkg/api/validation/validation.go | 37 ++++++++-- pkg/controller/replication_controller.go | 11 ++- pkg/controller/replication_controller_test.go | 3 +- pkg/registry/pod/rest.go | 16 ++--- pkg/registry/pod/rest_test.go | 72 +++++++++++++++---- test/integration/client_test.go | 3 + 11 files changed, 163 insertions(+), 32 deletions(-) diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go index 1f8ef774470..bfcebf38e37 100644 --- a/pkg/api/rest/resttest/resttest.go +++ b/pkg/api/rest/resttest/resttest.go @@ -24,6 +24,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) type Tester struct { @@ -53,6 +54,28 @@ func (t *Tester) TestCreate(valid runtime.Object, invalid ...runtime.Object) { t.TestCreateInvokesValidation(invalid...) } +func (t *Tester) TestCreateResetsUserData(valid runtime.Object) { + objectMeta, err := api.ObjectMetaFor(valid) + if err != nil { + t.Fatalf("object does not have ObjectMeta: %v\n%#v", err, valid) + } + + now := util.Now() + objectMeta.UID = "bad-uid" + objectMeta.CreationTimestamp = now + + 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 objectMeta.UID == "bad-uid" || objectMeta.CreationTimestamp == now { + t.Errorf("ObjectMeta did not reset basic fields: %#v", objectMeta) + } +} + func (t *Tester) TestCreateHasMetadata(valid runtime.Object) { objectMeta, err := api.ObjectMetaFor(valid) if err != nil { diff --git a/pkg/api/rest/types.go b/pkg/api/rest/types.go index 11b8428ada2..8a08bc5dde7 100644 --- a/pkg/api/rest/types.go +++ b/pkg/api/rest/types.go @@ -45,3 +45,26 @@ func (rcStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { controller := obj.(*api.ReplicationController) return validation.ValidateReplicationController(controller) } + +// podStrategy implements behavior for Pods +// TODO: move to a pod specific package. +type podStrategy struct { + runtime.ObjectTyper + api.NameGenerator +} + +// Pods is the default logic that applies when creating and updating Pod +// objects. +var Pods RESTCreateStrategy = podStrategy{api.Scheme, api.SimpleNameGenerator} + +// ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. +func (podStrategy) ResetBeforeCreate(obj runtime.Object) { + pod := obj.(*api.Pod) + pod.Status = api.PodStatus{} +} + +// Validate validates a new pod. +func (podStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { + pod := obj.(*api.Pod) + return validation.ValidatePod(pod) +} diff --git a/pkg/api/testing/fuzzer.go b/pkg/api/testing/fuzzer.go index 93008d7aa58..a64b2a36ab5 100644 --- a/pkg/api/testing/fuzzer.go +++ b/pkg/api/testing/fuzzer.go @@ -24,6 +24,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" + "github.com/GoogleCloudPlatform/kubernetes/pkg/types" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/fsouza/go-dockerclient" @@ -57,6 +58,8 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer { j.Name = c.RandString() j.ResourceVersion = strconv.FormatUint(c.RandUint64(), 10) j.SelfLink = c.RandString() + j.UID = types.UID(c.RandString()) + j.GenerateName = c.RandString() var sec, nsec int64 c.Fuzz(&sec) diff --git a/pkg/api/v1beta1/conversion.go b/pkg/api/v1beta1/conversion.go index 34432b08c3b..41e21263dcd 100644 --- a/pkg/api/v1beta1/conversion.go +++ b/pkg/api/v1beta1/conversion.go @@ -79,6 +79,7 @@ func init() { func(in *newer.ObjectMeta, out *TypeMeta, s conversion.Scope) error { out.Namespace = in.Namespace out.ID = in.Name + out.GenerateName = in.GenerateName out.UID = in.UID out.CreationTimestamp = in.CreationTimestamp out.SelfLink = in.SelfLink @@ -94,6 +95,7 @@ func init() { func(in *TypeMeta, out *newer.ObjectMeta, s conversion.Scope) error { out.Namespace = in.Namespace out.Name = in.ID + out.GenerateName = in.GenerateName out.UID = in.UID out.CreationTimestamp = in.CreationTimestamp out.SelfLink = in.SelfLink diff --git a/pkg/api/v1beta2/conversion.go b/pkg/api/v1beta2/conversion.go index ccec2ad9e46..94c561f462c 100644 --- a/pkg/api/v1beta2/conversion.go +++ b/pkg/api/v1beta2/conversion.go @@ -79,6 +79,7 @@ func init() { func(in *newer.ObjectMeta, out *TypeMeta, s conversion.Scope) error { out.Namespace = in.Namespace out.ID = in.Name + out.GenerateName = in.GenerateName out.UID = in.UID out.CreationTimestamp = in.CreationTimestamp out.SelfLink = in.SelfLink @@ -94,6 +95,7 @@ func init() { func(in *TypeMeta, out *newer.ObjectMeta, s conversion.Scope) error { out.Namespace = in.Namespace out.Name = in.ID + out.GenerateName = in.GenerateName out.UID = in.UID out.CreationTimestamp = in.CreationTimestamp out.SelfLink = in.SelfLink diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index e7f5d27828a..3387d34a12f 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -66,6 +66,35 @@ func maskTrailingDash(name string) string { return name } +// ValidatePodName can be used to check whether the given pod name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +func ValidatePodName(name string, prefix bool) (bool, string) { + return nameIsDNSSubdomain(name, prefix) +} + +// ValidateReplicationControllerName can be used to check whether the given replication +// controller name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +func ValidateReplicationControllerName(name string, prefix bool) (bool, string) { + return nameIsDNSSubdomain(name, prefix) +} + +// ValidateServiceName can be used to check whether the given service name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +func ValidateServiceName(name string, prefix bool) (bool, string) { + return nameIsDNS952Label(name, prefix) +} + +// ValidateNodeName can be used to check whether the given node name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +func ValidateNodeName(name string, prefix bool) (bool, string) { + return nameIsDNSSubdomain(name, prefix) +} + // nameIsDNSSubdomain is a ValidateNameFunc for names that must be a DNS subdomain. func nameIsDNSSubdomain(name string, prefix bool) (bool, string) { if prefix { @@ -510,7 +539,7 @@ func validateDNSPolicy(dnsPolicy *api.DNSPolicy) errs.ValidationErrorList { // ValidatePod tests if required fields in the pod are set. func ValidatePod(pod *api.Pod) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, ValidateObjectMeta(&pod.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...) + allErrs = append(allErrs, ValidateObjectMeta(&pod.ObjectMeta, true, ValidatePodName).Prefix("metadata")...) allErrs = append(allErrs, ValidatePodSpec(&pod.Spec).Prefix("spec")...) return allErrs @@ -563,7 +592,7 @@ var supportedSessionAffinityType = util.NewStringSet(string(api.AffinityTypeClie // ValidateService tests if required fields in the service are set. func ValidateService(service *api.Service) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, ValidateObjectMeta(&service.ObjectMeta, true, nameIsDNS952Label).Prefix("metadata")...) + allErrs = append(allErrs, ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName).Prefix("metadata")...) if !util.IsValidPortNum(service.Spec.Port) { allErrs = append(allErrs, errs.NewFieldInvalid("spec.port", service.Spec.Port, "")) @@ -604,7 +633,7 @@ func ValidateServiceUpdate(oldService, service *api.Service) errs.ValidationErro // ValidateReplicationController tests if required fields in the replication controller are set. func ValidateReplicationController(controller *api.ReplicationController) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, ValidateObjectMeta(&controller.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...) + allErrs = append(allErrs, ValidateObjectMeta(&controller.ObjectMeta, true, ValidateReplicationControllerName).Prefix("metadata")...) allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...) return allErrs @@ -694,7 +723,7 @@ func ValidateBoundPod(pod *api.BoundPod) errs.ValidationErrorList { // ValidateMinion tests if required fields in the node are set. func ValidateMinion(node *api.Node) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, ValidateObjectMeta(&node.ObjectMeta, false, nameIsDNSSubdomain).Prefix("metadata")...) + allErrs = append(allErrs, ValidateObjectMeta(&node.ObjectMeta, false, ValidateNodeName).Prefix("metadata")...) return allErrs } diff --git a/pkg/controller/replication_controller.go b/pkg/controller/replication_controller.go index 704384edd1b..ef948b57e90 100644 --- a/pkg/controller/replication_controller.go +++ b/pkg/controller/replication_controller.go @@ -23,6 +23,7 @@ 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/client" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" @@ -60,9 +61,17 @@ func (r RealPodControl) createReplica(namespace string, controller api.Replicati for k, v := range controller.Spec.Template.Labels { desiredLabels[k] = v } + + // use the dash (if the name isn't too long) to make the pod name a bit prettier + prefix := fmt.Sprintf("%s-", controller.Name) + if ok, _ := validation.ValidatePodName(prefix, true); !ok { + prefix = controller.Name + } + pod := &api.Pod{ ObjectMeta: api.ObjectMeta{ - Labels: desiredLabels, + Labels: desiredLabels, + GenerateName: prefix, }, } if err := api.Scheme.Convert(&controller.Spec.Template.Spec, &pod.Spec); err != nil { diff --git a/pkg/controller/replication_controller_test.go b/pkg/controller/replication_controller_test.go index a2df341b8f8..58c88e5f186 100644 --- a/pkg/controller/replication_controller_test.go +++ b/pkg/controller/replication_controller_test.go @@ -229,7 +229,8 @@ func TestCreateReplica(t *testing.T) { expectedPod := api.Pod{ ObjectMeta: api.ObjectMeta{ - Labels: controllerSpec.Spec.Template.Labels, + Labels: controllerSpec.Spec.Template.Labels, + GenerateName: fmt.Sprintf("%s-", controllerSpec.Name), }, Spec: controllerSpec.Spec.Template.Spec, } diff --git a/pkg/registry/pod/rest.go b/pkg/registry/pod/rest.go index 2693865bbef..c7f9ee6dfcd 100644 --- a/pkg/registry/pod/rest.go +++ b/pkg/registry/pod/rest.go @@ -22,6 +22,7 @@ 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/v1beta1" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" @@ -56,18 +57,11 @@ func NewREST(config *RESTConfig) *REST { func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RESTResult, error) { pod := obj.(*api.Pod) - if !api.ValidNamespace(ctx, &pod.ObjectMeta) { - return nil, errors.NewConflict("pod", pod.Namespace, fmt.Errorf("Pod.Namespace does not match the provided context")) - } - api.FillObjectMetaSystemFields(ctx, &pod.ObjectMeta) - if len(pod.Name) == 0 { - // TODO properly handle auto-generated names. - // See https://github.com/GoogleCloudPlatform/kubernetes/issues/148 170 & 1135 - pod.Name = string(pod.UID) - } - if errs := validation.ValidatePod(pod); len(errs) > 0 { - return nil, errors.NewInvalid("pod", pod.Name, errs) + + if err := rest.BeforeCreate(rest.Pods, ctx, obj); err != nil { + return nil, err } + return apiserver.MakeAsync(func() (runtime.Object, error) { if err := rs.registry.CreatePod(ctx, pod); err != nil { return nil, err diff --git a/pkg/registry/pod/rest_test.go b/pkg/registry/pod/rest_test.go index 50e0ccd3049..c89d5f26662 100644 --- a/pkg/registry/pod/rest_test.go +++ b/pkg/registry/pod/rest_test.go @@ -26,6 +26,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/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" @@ -83,11 +84,15 @@ func TestCreatePodRegistryError(t *testing.T) { registry: podRegistry, podCache: &fakeCache{statusToReturn: &api.PodStatus{}}, } - pod := &api.Pod{} + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + }, + } ctx := api.NewDefaultContext() ch, err := storage.Create(ctx, pod) if err != nil { - t.Errorf("Expected %#v, Got %#v", nil, err) + t.Fatalf("unexpected error: %v", err) } expectApiStatusError(t, ch, podRegistry.Err.Error()) } @@ -99,11 +104,15 @@ func TestCreatePodSetsIds(t *testing.T) { registry: podRegistry, podCache: &fakeCache{statusToReturn: &api.PodStatus{}}, } - pod := &api.Pod{} + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + }, + } ctx := api.NewDefaultContext() ch, err := storage.Create(ctx, pod) if err != nil { - t.Errorf("Expected %#v, Got %#v", nil, err) + t.Fatalf("unexpected error: %v", err) } expectApiStatusError(t, ch, podRegistry.Err.Error()) @@ -122,11 +131,15 @@ func TestCreatePodSetsUID(t *testing.T) { registry: podRegistry, podCache: &fakeCache{statusToReturn: &api.PodStatus{}}, } - pod := &api.Pod{} + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + }, + } ctx := api.NewDefaultContext() ch, err := storage.Create(ctx, pod) if err != nil { - t.Errorf("Expected %#v, Got %#v", nil, err) + t.Fatalf("unexpected error: %v", err) } expectApiStatusError(t, ch, podRegistry.Err.Error()) @@ -190,7 +203,7 @@ func TestListEmptyPodList(t *testing.T) { ctx := api.NewContext() pods, err := storage.List(ctx, labels.Everything(), labels.Everything()) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } if len(pods.(*api.PodList).Items) != 0 { @@ -225,7 +238,7 @@ func TestListPodList(t *testing.T) { podsObj, err := storage.List(ctx, labels.Everything(), labels.Everything()) pods := podsObj.(*api.PodList) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } if len(pods.Items) != 2 { @@ -336,12 +349,12 @@ func TestPodDecode(t *testing.T) { } body, err := latest.Codec.Encode(expected) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } actual := storage.New() if err := latest.Codec.DecodeInto(body, actual); err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } if !reflect.DeepEqual(expected, actual) { @@ -363,7 +376,7 @@ func TestGetPod(t *testing.T) { obj, err := storage.Get(ctx, "foo") pod := obj.(*api.Pod) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } expect := *podRegistry.Pod @@ -386,7 +399,7 @@ func TestGetPodCacheError(t *testing.T) { obj, err := storage.Get(ctx, "foo") pod := obj.(*api.Pod) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } expect := *podRegistry.Pod @@ -396,6 +409,7 @@ func TestGetPodCacheError(t *testing.T) { } } +// TODO: remove, this is covered by RESTTest.TestCreate func TestPodStorageValidatesCreate(t *testing.T) { podRegistry := registrytest.NewPodRegistry(nil) podRegistry.Err = fmt.Errorf("test error") @@ -420,6 +434,7 @@ func TestPodStorageValidatesCreate(t *testing.T) { } } +// TODO: remove, this is covered by RESTTest.TestCreate func TestCreatePod(t *testing.T) { podRegistry := registrytest.NewPodRegistry(nil) podRegistry.Pod = &api.Pod{ @@ -437,7 +452,7 @@ func TestCreatePod(t *testing.T) { ctx := api.NewDefaultContext() channel, err := storage.Create(ctx, pod) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } select { case <-channel: @@ -450,6 +465,7 @@ func TestCreatePod(t *testing.T) { } } +// TODO: remove, this is covered by RESTTest.TestCreate func TestCreatePodWithConflictingNamespace(t *testing.T) { storage := REST{} pod := &api.Pod{ @@ -463,7 +479,7 @@ func TestCreatePodWithConflictingNamespace(t *testing.T) { } if err == nil { t.Errorf("Expected an error, but we didn't get one") - } else if strings.Index(err.Error(), "Pod.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 'Pod.Namespace does not match the provided context' error, got '%v'", err.Error()) } } @@ -605,7 +621,7 @@ func TestDeletePod(t *testing.T) { ctx := api.NewDefaultContext() channel, err := storage.Delete(ctx, "foo") if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } var result apiserver.RESTResult select { @@ -618,3 +634,29 @@ func TestDeletePod(t *testing.T) { t.Errorf("Unexpeceted cache delete: %s %s %#v", fakeCache.clearedName, fakeCache.clearedNamespace, result.Object) } } + +func TestCreate(t *testing.T) { + test := resttest.New(t, &REST{ + registry: registrytest.NewPodRegistry(nil), + podCache: &fakeCache{statusToReturn: &api.PodStatus{}}, + }) + test.TestCreate( + // valid + &api.Pod{ + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "test1", + Image: "foo", + }, + }, + }, + }, + // invalid + &api.Pod{ + Spec: api.PodSpec{ + Containers: []api.Container{}, + }, + }, + ) +} diff --git a/test/integration/client_test.go b/test/integration/client_test.go index 9ad1bb83c7d..3bb073e60c3 100644 --- a/test/integration/client_test.go +++ b/test/integration/client_test.go @@ -87,6 +87,9 @@ func TestClient(t *testing.T) { // get a validation error pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + GenerateName: "test", + }, Spec: api.PodSpec{ Containers: []api.Container{ { From 7f39a37eee288dfaa773547aee1647f84a9ef3f6 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 28 Jan 2015 17:39:57 -0500 Subject: [PATCH 3/6] Fix integration tests to not depend on setting pod.Status on create Allow the master to have pod/node cache timeouts controlled via a config flag for integration tests. Move integration test to '127.0.0.1' so that it correctly returns a health check, and enable health check testing on the integration test. --- cmd/integration/integration.go | 20 ++++++++++++++++---- pkg/master/master.go | 14 ++++++++++++-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go index 8e4908b7e83..dcd3610157b 100644 --- a/cmd/integration/integration.go +++ b/cmd/integration/integration.go @@ -73,7 +73,7 @@ func (fakeKubeletClient) GetPodStatus(host, podNamespace, podID string) (api.Pod Client: http.DefaultClient, Port: 10250, } - case "machine": + case "127.0.0.1": c = &client.HTTPKubeletClient{ Client: http.DefaultClient, Port: 10251, @@ -81,7 +81,18 @@ func (fakeKubeletClient) GetPodStatus(host, podNamespace, podID string) (api.Pod default: glog.Fatalf("Can't get info for: '%v', '%v - %v'", host, podNamespace, podID) } - return c.GetPodStatus("localhost", podNamespace, podID) + r, err := c.GetPodStatus("localhost", podNamespace, podID) + if err != nil { + return r, err + } + r.Status.PodIP = "1.2.3.4" + m := make(api.PodInfo) + for k, v := range r.Status.Info { + v.PodIP = "1.2.3.4" + m[k] = v + } + r.Status.Info = m + return r, nil } func (fakeKubeletClient) HealthCheck(host string) (probe.Status, error) { @@ -104,7 +115,7 @@ func startComponents(manifestURL string) (apiServerURL string) { // Setup servers := []string{"http://localhost:4001"} glog.Infof("Creating etcd client pointing to %v", servers) - machineList := []string{"localhost", "machine"} + machineList := []string{"localhost", "127.0.0.1"} handler := delegateHandler{} apiServer := httptest.NewServer(&handler) @@ -163,6 +174,7 @@ func startComponents(manifestURL string) (apiServerURL string) { ReadWritePort: portNumber, ReadOnlyPort: portNumber, PublicAddress: net.ParseIP(host), + CacheTimeout: 2 * time.Second, }) handler.delegate = m.Handler @@ -185,7 +197,7 @@ func startComponents(manifestURL string) (apiServerURL string) { nodeResources := &api.NodeResources{} nodeController := nodeControllerPkg.NewNodeController(nil, "", machineList, nodeResources, cl, fakeKubeletClient{}) - nodeController.Run(10*time.Second, 10) + nodeController.Run(5*time.Second, 10) // Kubelet (localhost) testRootDir := makeTempDirOrDie("kubelet_integ_1.") diff --git a/pkg/master/master.go b/pkg/master/master.go index 872c29ac45c..62b67830b57 100644 --- a/pkg/master/master.go +++ b/pkg/master/master.go @@ -100,6 +100,10 @@ type Config struct { // If nil, the first result from net.InterfaceAddrs will be used. PublicAddress net.IP + + // Control the interval that pod, node IP, and node heath status caches + // expire. + CacheTimeout time.Duration } // Master contains state for a Kubernetes cluster master/api server. @@ -117,6 +121,7 @@ type Master struct { storage map[string]apiserver.RESTStorage client *client.Client portalNet *net.IPNet + cacheTimeout time.Duration mux apiserver.Mux muxHelper *apiserver.MuxHelper @@ -179,6 +184,9 @@ func setDefaults(c *Config) { if c.ReadOnlyPort == 0 { c.ReadOnlyPort = 7080 } + if c.CacheTimeout == 0 { + c.CacheTimeout = 5 * time.Second + } if c.ReadWritePort == 0 { c.ReadWritePort = 443 } @@ -283,7 +291,9 @@ func New(c *Config) *Master { authorizer: c.Authorizer, admissionControl: c.AdmissionControl, v1beta3: c.EnableV1Beta3, - nodeIPCache: NewIPCache(c.Cloud, util.RealClock{}, 30*time.Second), + nodeIPCache: NewIPCache(c.Cloud, util.RealClock{}, c.CacheTimeout), + + cacheTimeout: c.CacheTimeout, masterCount: c.MasterCount, publicIP: c.PublicAddress, @@ -365,7 +375,7 @@ func (m *Master) init(c *Config) { RESTStorageToNodes(nodeRESTStorage).Nodes(), m.podRegistry, ) - go util.Forever(func() { podCache.UpdateAllContainers() }, time.Second*5) + go util.Forever(func() { podCache.UpdateAllContainers() }, m.cacheTimeout) go util.Forever(func() { podCache.GarbageCollectPodStatus() }, time.Minute*30) // TODO: Factor out the core API registration From 0a87f0332b21a1f13782035b6dcedd24c11e438e Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 28 Jan 2015 21:36:36 -0500 Subject: [PATCH 4/6] Add name generation to services --- pkg/api/rest/types.go | 25 +++++++++++++++++++++++++ pkg/registry/service/rest.go | 29 ++++++++++++----------------- pkg/registry/service/rest_test.go | 27 ++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/pkg/api/rest/types.go b/pkg/api/rest/types.go index 8a08bc5dde7..9ac9bba9aba 100644 --- a/pkg/api/rest/types.go +++ b/pkg/api/rest/types.go @@ -68,3 +68,28 @@ func (podStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { pod := obj.(*api.Pod) return validation.ValidatePod(pod) } + +// svcStrategy implements behavior for Services +// TODO: move to a service specific package. +type svcStrategy struct { + runtime.ObjectTyper + api.NameGenerator +} + +// Services is the default logic that applies when creating and updating Service +// objects. +var Services RESTCreateStrategy = svcStrategy{api.Scheme, api.SimpleNameGenerator} + +// ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. +func (svcStrategy) ResetBeforeCreate(obj runtime.Object) { + service := obj.(*api.Service) + // TODO: Get rid of ProxyPort. + service.Spec.ProxyPort = 0 + service.Status = api.ServiceStatus{} +} + +// Validate validates a new pod. +func (svcStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { + service := obj.(*api.Service) + return validation.ValidateService(service) +} diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index 718b34b798e..e7af0cd605f 100644 --- a/pkg/registry/service/rest.go +++ b/pkg/registry/service/rest.go @@ -23,6 +23,7 @@ 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" "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider" @@ -81,35 +82,29 @@ func reloadIPsFromStorage(ipa *ipAllocator, registry Registry) { func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RESTResult, error) { service := obj.(*api.Service) - if !api.ValidNamespace(ctx, &service.ObjectMeta) { - return nil, errors.NewConflict("service", service.Namespace, fmt.Errorf("Service.Namespace does not match the provided context")) - } - if errs := validation.ValidateService(service); len(errs) > 0 { - return nil, errors.NewInvalid("service", service.Name, errs) + + if err := rest.BeforeCreate(rest.Services, ctx, obj); err != nil { + return nil, err } - api.FillObjectMetaSystemFields(ctx, &service.ObjectMeta) - - if service.Spec.PortalIP == "" { + if len(service.Spec.PortalIP) == 0 { // Allocate next available. - if ip, err := rs.portalMgr.AllocateNext(); err != nil { + ip, err := rs.portalMgr.AllocateNext() + if err != nil { return nil, err - } else { - service.Spec.PortalIP = ip.String() } + service.Spec.PortalIP = ip.String() } else { // Try to respect the requested IP. if err := rs.portalMgr.Allocate(net.ParseIP(service.Spec.PortalIP)); err != nil { el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, err.Error())} - return nil, errors.NewInvalid("service", service.Name, el) + return nil, errors.NewInvalid("Service", service.Name, el) } } return apiserver.MakeAsync(func() (runtime.Object, error) { - // TODO: Consider moving this to a rectification loop, so that we make/remove external load balancers + // TODO: Move this to post-creation rectification loop, so that we make/remove external load balancers // correctly no matter what http operations happen. - // TODO: Get rid of ProxyPort. - service.Spec.ProxyPort = 0 if service.Spec.CreateExternalLoadBalancer { if rs.cloud == nil { return nil, fmt.Errorf("requested an external service, but no cloud provider supplied.") @@ -155,8 +150,8 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE service.Spec.PublicIPs = []string{ip.String()} } } - err := rs.registry.CreateService(ctx, service) - if err != nil { + + if err := rs.registry.CreateService(ctx, service); err != nil { return nil, err } return rs.registry.GetService(ctx, service.Name) diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index eec35f117be..4c4d1beb5f3 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -24,6 +24,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest/resttest" "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" cloud "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" @@ -621,6 +622,7 @@ func TestServiceRegistryIPReloadFromStorage(t *testing.T) { } } +// TODO: remove, covered by TestCreate func TestCreateServiceWithConflictingNamespace(t *testing.T) { storage := REST{} service := &api.Service{ @@ -634,7 +636,7 @@ func TestCreateServiceWithConflictingNamespace(t *testing.T) { } if err == nil { t.Errorf("Expected an error, but we didn't get one") - } else if strings.Index(err.Error(), "Service.Namespace does not match the provided context") == -1 { + } else if strings.Contains(err.Error(), "Service.Namespace does not match the provided context") { t.Errorf("Expected 'Service.Namespace does not match the provided context' error, got '%s'", err.Error()) } } @@ -656,3 +658,26 @@ func TestUpdateServiceWithConflictingNamespace(t *testing.T) { t.Errorf("Expected 'Service.Namespace does not match the provided context' error, got '%s'", err.Error()) } } + +func TestCreate(t *testing.T) { + registry := registrytest.NewServiceRegistry() + fakeCloud := &cloud.FakeCloud{} + machines := []string{"foo", "bar", "baz"} + rest := NewREST(registry, fakeCloud, registrytest.NewMinionRegistry(machines, api.NodeResources{}), makeIPNet(t)) + rest.portalMgr.randomAttempts = 0 + + test := resttest.New(t, rest) + test.TestCreate( + // valid + &api.Service{ + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + Port: 6502, + }, + }, + // invalid + &api.Service{ + Spec: api.ServiceSpec{}, + }, + ) +} From e485dc93cab272b242f869420c88c342a424e322 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 28 Jan 2015 21:56:57 -0500 Subject: [PATCH 5/6] Minions should have common logic with create TODO: disable / document generate names for cluster scoped resources. --- pkg/api/rest/create.go | 10 +++++-- pkg/api/rest/resttest/resttest.go | 37 ++++++++++++++++++++++--- pkg/api/rest/types.go | 45 ++++++++++++++++++++++++++++++- pkg/registry/minion/rest.go | 7 +++-- pkg/registry/minion/rest_test.go | 25 +++++++++++++++-- 5 files changed, 112 insertions(+), 12 deletions(-) diff --git a/pkg/api/rest/create.go b/pkg/api/rest/create.go index f7601c9668c..67967497c4b 100644 --- a/pkg/api/rest/create.go +++ b/pkg/api/rest/create.go @@ -31,6 +31,8 @@ type RESTCreateStrategy interface { // The NameGenerator will be invoked prior to validation. api.NameGenerator + // NamespaceScoped returns true if the object must be within a namespace. + NamespaceScoped() bool // ResetBeforeCreate is invoked on create before validation to remove any fields // that may not be persisted. ResetBeforeCreate(obj runtime.Object) @@ -52,8 +54,12 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx api.Context, obj runtime.Obje 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") + if strategy.NamespaceScoped() { + if !api.ValidNamespace(ctx, objectMeta) { + return errors.NewBadRequest("the namespace of the provided object does not match the namespace sent on the request") + } + } else { + objectMeta.Namespace = api.NamespaceNone } strategy.ResetBeforeCreate(obj) api.FillObjectMetaSystemFields(ctx, objectMeta) diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go index bfcebf38e37..3b0a855aaef 100644 --- a/pkg/api/rest/resttest/resttest.go +++ b/pkg/api/rest/resttest/resttest.go @@ -29,7 +29,8 @@ import ( type Tester struct { *testing.T - storage apiserver.RESTStorage + storage apiserver.RESTStorage + clusterScope bool } func New(t *testing.T, storage apiserver.RESTStorage) *Tester { @@ -39,6 +40,11 @@ func New(t *testing.T, storage apiserver.RESTStorage) *Tester { } } +func (t *Tester) ClusterScope() *Tester { + t.clusterScope = true + return t +} + func copyOrDie(obj runtime.Object) runtime.Object { out, err := api.Scheme.Copy(obj) if err != nil { @@ -50,7 +56,11 @@ func copyOrDie(obj runtime.Object) runtime.Object { func (t *Tester) TestCreate(valid runtime.Object, invalid ...runtime.Object) { t.TestCreateHasMetadata(copyOrDie(valid)) t.TestCreateGeneratesName(copyOrDie(valid)) - t.TestCreateRejectsMismatchedNamespace(copyOrDie(valid)) + if t.clusterScope { + t.TestCreateRejectsNamespace(copyOrDie(valid)) + } else { + t.TestCreateRejectsMismatchedNamespace(copyOrDie(valid)) + } t.TestCreateInvokesValidation(invalid...) } @@ -84,8 +94,13 @@ func (t *Tester) TestCreateHasMetadata(valid runtime.Object) { objectMeta.Name = "test" objectMeta.Namespace = api.NamespaceDefault + context := api.NewDefaultContext() + if t.clusterScope { + objectMeta.Namespace = api.NamespaceNone + context = api.NewContext() + } - channel, err := t.storage.(apiserver.RESTCreater).Create(api.NewDefaultContext(), valid) + channel, err := t.storage.(apiserver.RESTCreater).Create(context, valid) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -139,3 +154,19 @@ func (t *Tester) TestCreateRejectsMismatchedNamespace(valid runtime.Object) { t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error()) } } + +func (t *Tester) TestCreateRejectsNamespace(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 index 9ac9bba9aba..a43458dd465 100644 --- a/pkg/api/rest/types.go +++ b/pkg/api/rest/types.go @@ -34,6 +34,11 @@ type rcStrategy struct { // objects. var ReplicationControllers RESTCreateStrategy = rcStrategy{api.Scheme, api.SimpleNameGenerator} +// NamespaceScoped is true for replication controllers. +func (rcStrategy) NamespaceScoped() bool { + return true +} + // 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) @@ -57,6 +62,11 @@ type podStrategy struct { // objects. var Pods RESTCreateStrategy = podStrategy{api.Scheme, api.SimpleNameGenerator} +// NamespaceScoped is true for pods. +func (podStrategy) NamespaceScoped() bool { + return true +} + // ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. func (podStrategy) ResetBeforeCreate(obj runtime.Object) { pod := obj.(*api.Pod) @@ -80,6 +90,11 @@ type svcStrategy struct { // objects. var Services RESTCreateStrategy = svcStrategy{api.Scheme, api.SimpleNameGenerator} +// NamespaceScoped is true for services. +func (svcStrategy) NamespaceScoped() bool { + return true +} + // ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. func (svcStrategy) ResetBeforeCreate(obj runtime.Object) { service := obj.(*api.Service) @@ -88,8 +103,36 @@ func (svcStrategy) ResetBeforeCreate(obj runtime.Object) { service.Status = api.ServiceStatus{} } -// Validate validates a new pod. +// Validate validates a new service. func (svcStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { service := obj.(*api.Service) return validation.ValidateService(service) } + +// nodeStrategy implements behavior for nodes +// TODO: move to a node specific package. +type nodeStrategy struct { + runtime.ObjectTyper + api.NameGenerator +} + +// Nodes is the default logic that applies when creating and updating Node +// objects. +var Nodes RESTCreateStrategy = nodeStrategy{api.Scheme, api.SimpleNameGenerator} + +// NamespaceScoped is false for services. +func (nodeStrategy) NamespaceScoped() bool { + return false +} + +// ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. +func (nodeStrategy) ResetBeforeCreate(obj runtime.Object) { + _ = obj.(*api.Node) + // Nodes allow *all* fields, including status, to be set. +} + +// Validate validates a new node. +func (nodeStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { + node := obj.(*api.Node) + return validation.ValidateMinion(node) +} diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index 58f1450cfc1..b67fce85505 100644 --- a/pkg/registry/minion/rest.go +++ b/pkg/registry/minion/rest.go @@ -24,6 +24,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" kerrors "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" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" @@ -54,12 +55,10 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE return nil, fmt.Errorf("not a minion: %#v", obj) } - if errs := validation.ValidateMinion(minion); len(errs) > 0 { - return nil, kerrors.NewInvalid("minion", minion.Name, errs) + if err := rest.BeforeCreate(rest.Nodes, ctx, obj); err != nil { + return nil, err } - api.FillObjectMetaSystemFields(ctx, &minion.ObjectMeta) - return apiserver.MakeAsync(func() (runtime.Object, error) { err := rs.registry.CreateMinion(ctx, minion) if err != nil { diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index a18eade0518..7dfb530d8e3 100644 --- a/pkg/registry/minion/rest_test.go +++ b/pkg/registry/minion/rest_test.go @@ -21,6 +21,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest/resttest" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" ) @@ -107,11 +108,14 @@ func TestMinionRegistryValidUpdate(t *testing.T) { } } +var ( + validSelector = map[string]string{"a": "b"} + invalidSelector = map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} +) + func TestMinionRegistryValidatesCreate(t *testing.T) { storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{})) ctx := api.NewContext() - validSelector := map[string]string{"a": "b"} - invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} failureCases := map[string]api.Node{ "zero-length Name": { ObjectMeta: api.ObjectMeta{ @@ -148,3 +152,20 @@ func contains(nodes *api.NodeList, nodeID string) bool { } return false } + +func TestCreate(t *testing.T) { + test := resttest.New(t, NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))).ClusterScope() + test.TestCreate( + // valid + &api.Node{ + Status: api.NodeStatus{ + HostIP: "something", + }, + }, + // invalid + &api.Node{ + ObjectMeta: api.ObjectMeta{ + Labels: invalidSelector, + }, + }) +} From 1588970ec429df804708e8f00b66e4b94ba70723 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 28 Jan 2015 23:11:29 -0500 Subject: [PATCH 6/6] Turn 409 into 500 Try Again Later when using generateName If a client says they want the name to be generated, a 409 is not appropriate (since they didn't specify a name). Instead, we should return the next most appropriate error, which is a 5xx error indicating the request failed but the client *should* try again. Since there is no 5xx error that exactly fits this purpose, use 500 with StatusReasonTryAgainLater set. This commit does not implement client retry on TryAgainLater, but clients should retry up to a certain number of times. --- pkg/api/errors/errors.go | 27 ++++++++++++++++ pkg/api/errors/errors_test.go | 12 +++++++ pkg/api/rest/create.go | 42 ++++++++++++++++++++----- pkg/api/rest/create_test.go | 41 ++++++++++++++++++++++++ pkg/api/rest/resttest/resttest.go | 36 +++++++++++++++++++-- pkg/api/types.go | 11 +++++++ pkg/api/v1beta1/types.go | 11 +++++++ pkg/api/v1beta2/types.go | 11 +++++++ pkg/api/v1beta3/types.go | 11 +++++++ pkg/registry/controller/rest.go | 1 + pkg/registry/controller/rest_test.go | 7 +++-- pkg/registry/minion/rest.go | 4 +-- pkg/registry/minion/rest_test.go | 3 +- pkg/registry/pod/rest.go | 1 + pkg/registry/pod/rest_test.go | 5 +-- pkg/registry/registrytest/controller.go | 11 +++++++ pkg/registry/registrytest/minion.go | 6 ++++ pkg/registry/registrytest/pod.go | 6 ++++ pkg/registry/registrytest/service.go | 6 ++++ pkg/registry/service/rest.go | 1 + pkg/registry/service/rest_test.go | 2 +- 21 files changed, 237 insertions(+), 18 deletions(-) create mode 100644 pkg/api/rest/create_test.go diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index 930efe7dab2..e4345761ab1 100644 --- a/pkg/api/errors/errors.go +++ b/pkg/api/errors/errors.go @@ -174,6 +174,21 @@ func NewMethodNotSupported(kind, action string) error { }} } +// NewTryAgainLater returns an error indicating the requested action could not be completed due to a +// transient error, and the client should try again. +func NewTryAgainLater(kind, operation string) error { + return &StatusError{api.Status{ + Status: api.StatusFailure, + Code: http.StatusInternalServerError, + Reason: api.StatusReasonTryAgainLater, + Details: &api.StatusDetails{ + Kind: kind, + ID: operation, + }, + Message: fmt.Sprintf("The %s operation against %s could not be completed at this time, please try again.", operation, kind), + }} +} + // NewInternalError returns an error indicating the item is invalid and cannot be processed. func NewInternalError(err error) error { return &StatusError{api.Status{ @@ -218,6 +233,18 @@ func IsBadRequest(err error) bool { return reasonForError(err) == api.StatusReasonBadRequest } +// IsForbidden determines if err is an error which indicates that the request is forbidden and cannot +// be completed as requested. +func IsForbidden(err error) bool { + return reasonForError(err) == api.StatusReasonForbidden +} + +// IsTryAgainLater determines if err is an error which indicates that the request needs to be retried +// by the client. +func IsTryAgainLater(err error) bool { + return reasonForError(err) == api.StatusReasonTryAgainLater +} + func reasonForError(err error) api.StatusReason { switch t := err.(type) { case *StatusError: diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index d452b7cc7aa..a46583e25eb 100644 --- a/pkg/api/errors/errors_test.go +++ b/pkg/api/errors/errors_test.go @@ -43,6 +43,12 @@ func TestErrorNew(t *testing.T) { if IsBadRequest(err) { t.Errorf("expected to not be %s", api.StatusReasonBadRequest) } + if IsForbidden(err) { + t.Errorf("expected to not be %s", api.StatusReasonForbidden) + } + if IsTryAgainLater(err) { + t.Errorf("expected to not be %s", api.StatusReasonTryAgainLater) + } if IsMethodNotSupported(err) { t.Errorf("expected to not be %s", api.StatusReasonMethodNotAllowed) } @@ -59,6 +65,12 @@ func TestErrorNew(t *testing.T) { if !IsBadRequest(NewBadRequest("reason")) { t.Errorf("expected to be %s", api.StatusReasonBadRequest) } + if !IsForbidden(NewForbidden("test", "2", errors.New("reason"))) { + t.Errorf("expected to be %s", api.StatusReasonForbidden) + } + if !IsTryAgainLater(NewTryAgainLater("test", "reason")) { + t.Errorf("expected to be %s", api.StatusReasonTryAgainLater) + } if !IsMethodNotSupported(NewMethodNotSupported("foo", "delete")) { t.Errorf("expected to be %s", api.StatusReasonMethodNotAllowed) } diff --git a/pkg/api/rest/create.go b/pkg/api/rest/create.go index 67967497c4b..36b931b95c5 100644 --- a/pkg/api/rest/create.go +++ b/pkg/api/rest/create.go @@ -45,13 +45,9 @@ type RESTCreateStrategy interface { // 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) + objectMeta, kind, kerr := objectMetaAndKind(strategy, obj) + if kerr != nil { + return kerr } if strategy.NamespaceScoped() { @@ -70,3 +66,35 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx api.Context, obj runtime.Obje } return nil } + +// CheckGeneratedNameError checks whether an error that occured creating a resource is due +// to generation being unable to pick a valid name. +func CheckGeneratedNameError(strategy RESTCreateStrategy, err error, obj runtime.Object) error { + if !errors.IsAlreadyExists(err) { + return err + } + + objectMeta, kind, kerr := objectMetaAndKind(strategy, obj) + if kerr != nil { + return kerr + } + + if len(objectMeta.GenerateName) == 0 { + return err + } + + return errors.NewTryAgainLater(kind, "POST") +} + +// objectMetaAndKind retrieves kind and ObjectMeta from a runtime object, or returns an error. +func objectMetaAndKind(strategy RESTCreateStrategy, obj runtime.Object) (*api.ObjectMeta, string, error) { + objectMeta, err := api.ObjectMetaFor(obj) + if err != nil { + return nil, "", errors.NewInternalError(err) + } + _, kind, err := strategy.ObjectVersionAndKind(obj) + if err != nil { + return nil, "", errors.NewInternalError(err) + } + return objectMeta, kind, nil +} diff --git a/pkg/api/rest/create_test.go b/pkg/api/rest/create_test.go new file mode 100644 index 00000000000..c8ad1490225 --- /dev/null +++ b/pkg/api/rest/create_test.go @@ -0,0 +1,41 @@ +/* +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 ( + "testing" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" +) + +func TestCheckGeneratedNameError(t *testing.T) { + expect := errors.NewNotFound("foo", "bar") + if err := CheckGeneratedNameError(Pods, expect, &api.Pod{}); err != expect { + t.Errorf("NotFoundError should be ignored: %v", err) + } + + expect = errors.NewAlreadyExists("foo", "bar") + if err := CheckGeneratedNameError(Pods, expect, &api.Pod{}); err != expect { + t.Errorf("AlreadyExists should be returned when no GenerateName field: %v", err) + } + + expect = errors.NewAlreadyExists("foo", "bar") + if err := CheckGeneratedNameError(Pods, expect, &api.Pod{ObjectMeta: api.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsTryAgainLater(err) { + t.Errorf("expected try again later error: %v", err) + } +} diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go index 3b0a855aaef..fa3fb7de03e 100644 --- a/pkg/api/rest/resttest/resttest.go +++ b/pkg/api/rest/resttest/resttest.go @@ -30,16 +30,26 @@ import ( type Tester struct { *testing.T storage apiserver.RESTStorage + storageError injectErrorFunc clusterScope bool } -func New(t *testing.T, storage apiserver.RESTStorage) *Tester { +type injectErrorFunc func(err error) + +func New(t *testing.T, storage apiserver.RESTStorage, storageError injectErrorFunc) *Tester { return &Tester{ - T: t, - storage: storage, + T: t, + storage: storage, + storageError: storageError, } } +func (t *Tester) withStorageError(err error, fn func()) { + t.storageError(err) + defer t.storageError(nil) + fn() +} + func (t *Tester) ClusterScope() *Tester { t.clusterScope = true return t @@ -56,6 +66,7 @@ func copyOrDie(obj runtime.Object) runtime.Object { func (t *Tester) TestCreate(valid runtime.Object, invalid ...runtime.Object) { t.TestCreateHasMetadata(copyOrDie(valid)) t.TestCreateGeneratesName(copyOrDie(valid)) + t.TestCreateGeneratesNameReturnsTryAgain(copyOrDie(valid)) if t.clusterScope { t.TestCreateRejectsNamespace(copyOrDie(valid)) } else { @@ -129,6 +140,25 @@ func (t *Tester) TestCreateGeneratesName(valid runtime.Object) { } } +func (t *Tester) TestCreateGeneratesNameReturnsTryAgain(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-" + t.withStorageError(errors.NewAlreadyExists("kind", "thing"), func() { + ch, err := t.storage.(apiserver.RESTCreater).Create(api.NewDefaultContext(), valid) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + res := <-ch + if err := errors.FromObject(res.Object); err == nil || !errors.IsTryAgainLater(err) { + t.Fatalf("Unexpected error: %v", err) + } + }) +} + func (t *Tester) TestCreateInvokesValidation(invalid ...runtime.Object) { for i, obj := range invalid { ctx := api.NewDefaultContext() diff --git a/pkg/api/types.go b/pkg/api/types.go index c9f94215d95..05f1b0d0ac7 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -952,6 +952,17 @@ const ( // Status code 422 StatusReasonInvalid StatusReason = "Invalid" + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" + // StatusReasonTimeout means that the request could not be completed within the given time. // Clients can get this response only when they specified a timeout param in the request. // The request might succeed with an increased value of timeout param. diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index 4c02861ea87..a3e49a49c34 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -745,6 +745,17 @@ const ( // conflict. // Status code 409 StatusReasonConflict StatusReason = "Conflict" + + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" ) // StatusCause provides more information about an api.Status failure, including diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index 2d00faff0aa..79353bf3fd5 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -719,6 +719,17 @@ const ( // field attributes will be set. // Status code 422 StatusReasonInvalid StatusReason = "Invalid" + + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" ) // StatusCause provides more information about an api.Status failure, including diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 761e1c58f20..c8918aa6f69 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -949,6 +949,17 @@ const ( // field attributes will be set. // Status code 422 StatusReasonInvalid StatusReason = "Invalid" + + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" ) // StatusCause provides more information about an api.Status failure, including diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index bd8f75e3259..94c13664ef1 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -62,6 +62,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE return apiserver.MakeAsync(func() (runtime.Object, error) { if err := rs.registry.CreateController(ctx, controller); err != nil { + err = rest.CheckGeneratedNameError(rest.ReplicationControllers, err, controller) return apiserver.RESTResult{}, err } return rs.registry.GetController(ctx, controller.Name) diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index ed76e04ed81..aaaea3e9460 100644 --- a/pkg/registry/controller/rest_test.go +++ b/pkg/registry/controller/rest_test.go @@ -51,7 +51,9 @@ func TestListControllersError(t *testing.T) { } func TestListEmptyControllerList(t *testing.T) { - mockRegistry := registrytest.ControllerRegistry{nil, &api.ReplicationControllerList{ListMeta: api.ListMeta{ResourceVersion: "1"}}} + mockRegistry := registrytest.ControllerRegistry{ + Controllers: &api.ReplicationControllerList{ListMeta: api.ListMeta{ResourceVersion: "1"}}, + } storage := REST{ registry: &mockRegistry, } @@ -444,7 +446,8 @@ func TestUpdateControllerWithConflictingNamespace(t *testing.T) { } func TestCreate(t *testing.T) { - test := resttest.New(t, NewREST(®istrytest.ControllerRegistry{}, nil)) + registry := ®istrytest.ControllerRegistry{} + test := resttest.New(t, NewREST(registry, nil), registry.SetError) test.TestCreate( // valid &api.ReplicationController{ diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index b67fce85505..5c0ab637383 100644 --- a/pkg/registry/minion/rest.go +++ b/pkg/registry/minion/rest.go @@ -60,8 +60,8 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE } return apiserver.MakeAsync(func() (runtime.Object, error) { - err := rs.registry.CreateMinion(ctx, minion) - if err != nil { + if err := rs.registry.CreateMinion(ctx, minion); err != nil { + err = rest.CheckGeneratedNameError(rest.Nodes, err, minion) return nil, err } return minion, nil diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index 7dfb530d8e3..0e3e2c87b83 100644 --- a/pkg/registry/minion/rest_test.go +++ b/pkg/registry/minion/rest_test.go @@ -154,7 +154,8 @@ func contains(nodes *api.NodeList, nodeID string) bool { } func TestCreate(t *testing.T) { - test := resttest.New(t, NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))).ClusterScope() + registry := registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}) + test := resttest.New(t, NewREST(registry), registry.SetError).ClusterScope() test.TestCreate( // valid &api.Node{ diff --git a/pkg/registry/pod/rest.go b/pkg/registry/pod/rest.go index c7f9ee6dfcd..b27290b1fdb 100644 --- a/pkg/registry/pod/rest.go +++ b/pkg/registry/pod/rest.go @@ -64,6 +64,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE return apiserver.MakeAsync(func() (runtime.Object, error) { if err := rs.registry.CreatePod(ctx, pod); err != nil { + err = rest.CheckGeneratedNameError(rest.Pods, err, pod) return nil, err } return rs.registry.GetPod(ctx, pod.Name) diff --git a/pkg/registry/pod/rest_test.go b/pkg/registry/pod/rest_test.go index c89d5f26662..aefbb509fcb 100644 --- a/pkg/registry/pod/rest_test.go +++ b/pkg/registry/pod/rest_test.go @@ -636,10 +636,11 @@ func TestDeletePod(t *testing.T) { } func TestCreate(t *testing.T) { + registry := registrytest.NewPodRegistry(nil) test := resttest.New(t, &REST{ - registry: registrytest.NewPodRegistry(nil), + registry: registry, podCache: &fakeCache{statusToReturn: &api.PodStatus{}}, - }) + }, registry.SetError) test.TestCreate( // valid &api.Pod{ diff --git a/pkg/registry/registrytest/controller.go b/pkg/registry/registrytest/controller.go index e0476574042..9e83ff86668 100644 --- a/pkg/registry/registrytest/controller.go +++ b/pkg/registry/registrytest/controller.go @@ -17,6 +17,8 @@ limitations under the License. package registrytest import ( + "sync" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" @@ -26,6 +28,13 @@ import ( type ControllerRegistry struct { Err error Controllers *api.ReplicationControllerList + sync.Mutex +} + +func (r *ControllerRegistry) SetError(err error) { + r.Lock() + defer r.Unlock() + r.Err = err } func (r *ControllerRegistry) ListControllers(ctx api.Context) (*api.ReplicationControllerList, error) { @@ -37,6 +46,8 @@ func (r *ControllerRegistry) GetController(ctx api.Context, ID string) (*api.Rep } func (r *ControllerRegistry) CreateController(ctx api.Context, controller *api.ReplicationController) error { + r.Lock() + defer r.Unlock() return r.Err } diff --git a/pkg/registry/registrytest/minion.go b/pkg/registry/registrytest/minion.go index 13db8ff2592..c44b2802e2e 100644 --- a/pkg/registry/registrytest/minion.go +++ b/pkg/registry/registrytest/minion.go @@ -52,6 +52,12 @@ func NewMinionRegistry(minions []string, nodeResources api.NodeResources) *Minio } } +func (r *MinionRegistry) SetError(err error) { + r.Lock() + defer r.Unlock() + r.Err = err +} + func (r *MinionRegistry) ListMinions(ctx api.Context) (*api.NodeList, error) { r.Lock() defer r.Unlock() diff --git a/pkg/registry/registrytest/pod.go b/pkg/registry/registrytest/pod.go index 571b61074b2..6bfa0eed471 100644 --- a/pkg/registry/registrytest/pod.go +++ b/pkg/registry/registrytest/pod.go @@ -40,6 +40,12 @@ func NewPodRegistry(pods *api.PodList) *PodRegistry { } } +func (r *PodRegistry) SetError(err error) { + r.Lock() + defer r.Unlock() + r.Err = err +} + func (r *PodRegistry) ListPodsPredicate(ctx api.Context, filter func(*api.Pod) bool) (*api.PodList, error) { r.Lock() defer r.Unlock() diff --git a/pkg/registry/registrytest/service.go b/pkg/registry/registrytest/service.go index ec0d393b3eb..d1cb2ff63a0 100644 --- a/pkg/registry/registrytest/service.go +++ b/pkg/registry/registrytest/service.go @@ -41,6 +41,12 @@ type ServiceRegistry struct { UpdatedID string } +func (r *ServiceRegistry) SetError(err error) { + r.mu.Lock() + defer r.mu.Unlock() + r.Err = err +} + func (r *ServiceRegistry) ListServices(ctx api.Context) (*api.ServiceList, error) { r.mu.Lock() defer r.mu.Unlock() diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index e7af0cd605f..70ed8ca69ef 100644 --- a/pkg/registry/service/rest.go +++ b/pkg/registry/service/rest.go @@ -152,6 +152,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE } if err := rs.registry.CreateService(ctx, service); err != nil { + err = rest.CheckGeneratedNameError(rest.Services, err, service) return nil, err } return rs.registry.GetService(ctx, service.Name) diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index 4c4d1beb5f3..b18d7de0ad8 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -666,7 +666,7 @@ func TestCreate(t *testing.T) { rest := NewREST(registry, fakeCloud, registrytest.NewMinionRegistry(machines, api.NodeResources{}), makeIPNet(t)) rest.portalMgr.randomAttempts = 0 - test := resttest.New(t, rest) + test := resttest.New(t, rest, registry.SetError) test.TestCreate( // valid &api.Service{