Minions should have common logic with create

TODO: disable / document generate names for cluster scoped resources.

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)

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())
+	}
+}

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)
+}

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 {

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,
+			},
+		})
+}