From e485dc93cab272b242f869420c88c342a424e322 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 28 Jan 2015 21:56:57 -0500 Subject: [PATCH] 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, + }, + }) +}