From 5603714df85cbba131b337aab1f0de31afa125fa Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 27 Jan 2015 23:50:01 -0500 Subject: [PATCH] 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{ {