Merge pull request #5443 from bprashanth/rc_validation

Add an rc strategy, start delta validating updates

pkg/api/rest/create_test.go

@@ -17,12 +17,10 @@ limitations under the License.
 package rest
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 )
 
 func TestCheckGeneratedNameError(t *testing.T) {
@@ -41,75 +39,3 @@ func TestCheckGeneratedNameError(t *testing.T) {
 		t.Errorf("expected try again later error: %v", err)
 	}
 }
-
-func TestBeforeCreate(t *testing.T) {
-	failures := []runtime.Object{
-		&api.Service{},
-		&api.Service{
-			ObjectMeta: api.ObjectMeta{
-				Name:      "foo",
-				Namespace: "#$%%invalid",
-			},
-		},
-		&api.Service{
-			ObjectMeta: api.ObjectMeta{
-				Name:      "##&*(&invalid",
-				Namespace: api.NamespaceDefault,
-			},
-		},
-	}
-	for _, test := range failures {
-		ctx := api.NewDefaultContext()
-		err := BeforeCreate(Services, ctx, test)
-		if err == nil {
-			t.Errorf("unexpected non-error for %v", test)
-		}
-	}
-
-	obj := &api.ReplicationController{
-		ObjectMeta: api.ObjectMeta{
-			Name:      "foo",
-			Namespace: api.NamespaceDefault,
-		},
-		Spec: api.ReplicationControllerSpec{
-			Selector: map[string]string{"name": "foo"},
-			Template: &api.PodTemplateSpec{
-				ObjectMeta: api.ObjectMeta{
-					Labels: map[string]string{
-						"name": "foo",
-					},
-				},
-				Spec: api.PodSpec{
-					Containers: []api.Container{
-						{
-							Name:            "foo",
-							Image:           "foo",
-							ImagePullPolicy: api.PullAlways,
-						},
-					},
-					RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}},
-					DNSPolicy:     api.DNSDefault,
-				},
-			},
-		},
-		Status: api.ReplicationControllerStatus{
-			Replicas: 3,
-		},
-	}
-	ctx := api.NewDefaultContext()
-	err := BeforeCreate(ReplicationControllers, ctx, obj)
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-	}
-	if !reflect.DeepEqual(obj.Status, api.ReplicationControllerStatus{}) {
-		t.Errorf("status was not cleared as expected.")
-	}
-	if obj.Name != "foo" || obj.Namespace != api.NamespaceDefault {
-		t.Errorf("unexpected object metadata: %v", obj.ObjectMeta)
-	}
-
-	obj.Spec.Replicas = -1
-	if err := BeforeCreate(ReplicationControllers, ctx, obj); err == nil {
-		t.Errorf("unexpected non-error for invalid replication controller.")
-	}
-}

pkg/api/rest/types.go

@@ -44,34 +44,6 @@ func AllFuncs(fns ...ObjectFunc) ObjectFunc {
 	}
 }
 
-// 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}
-
-// 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)
-	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)
-}
-
 // svcStrategy implements behavior for Services
 // TODO: move to a service specific package.
 type svcStrategy struct {

pkg/registry/controller/rest.go

@@ -29,6 +29,43 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/watch"
 )
 
+// rcStrategy implements verification logic for Replication Controllers.
+type rcStrategy struct {
+	runtime.ObjectTyper
+	api.NameGenerator
+}
+
+// Strategy is the default logic that applies when creating and updating Replication Controller objects.
+var Strategy = rcStrategy{api.Scheme, api.SimpleNameGenerator}
+
+// NamespaceScoped returns true because all Replication Controllers need to be within a namespace.
+func (rcStrategy) NamespaceScoped() bool {
+	return true
+}
+
+// ResetBeforeCreate clears the status of a replication controller before 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)
+}
+
+// AllowCreateOnUpdate is false for replication controllers; this means a POST is
+// needed to create one.
+func (rcStrategy) AllowCreateOnUpdate() bool {
+	return false
+}
+
+// ValidateUpdate is the default update validation for an end user.
+func (rcStrategy) ValidateUpdate(obj, old runtime.Object) errors.ValidationErrorList {
+	return validation.ValidateReplicationControllerUpdate(old.(*api.ReplicationController), obj.(*api.ReplicationController))
+}
+
 // PodLister is anything that knows how to list pods.
 type PodLister interface {
 	ListPods(ctx api.Context, labels labels.Selector) (*api.PodList, error)
@@ -38,6 +75,7 @@ type PodLister interface {
 type REST struct {
 	registry  Registry
 	podLister PodLister
+	strategy  rcStrategy
 }
 
 // NewREST returns a new apiserver.RESTStorage for the given registry and PodLister.
@@ -45,23 +83,25 @@ func NewREST(registry Registry, podLister PodLister) *REST {
 	return &REST{
 		registry:  registry,
 		podLister: podLister,
+		strategy:  Strategy,
 	}
 }
 
-// Create registers the given ReplicationController.
+// Create registers the given ReplicationController with the system,
+// which eventually leads to the controller manager acting on its behalf.
 func (rs *REST) Create(ctx api.Context, obj runtime.Object) (runtime.Object, error) {
 	controller, ok := obj.(*api.ReplicationController)
 	if !ok {
 		return nil, fmt.Errorf("not a replication controller: %#v", obj)
 	}
 
-	if err := rest.BeforeCreate(rest.ReplicationControllers, ctx, obj); err != nil {
+	if err := rest.BeforeCreate(rs.strategy, ctx, obj); err != nil {
 		return nil, err
 	}
 
 	out, err := rs.registry.CreateController(ctx, controller)
 	if err != nil {
-		err = rest.CheckGeneratedNameError(rest.ReplicationControllers, err, controller)
+		err = rest.CheckGeneratedNameError(rs.strategy, err, controller)
 	}
 	return out, err
 }
@@ -115,11 +155,12 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (runtime.Object, boo
 	if !ok {
 		return nil, false, fmt.Errorf("not a replication controller: %#v", obj)
 	}
-	if !api.ValidNamespace(ctx, &controller.ObjectMeta) {
-		return nil, false, errors.NewConflict("controller", controller.Namespace, fmt.Errorf("Controller.Namespace does not match the provided context"))
+	existingController, err := rs.registry.GetController(ctx, controller.Name)
+	if err != nil {
+		return nil, false, err
 	}
-	if errs := validation.ValidateReplicationController(controller); len(errs) > 0 {
-		return nil, false, errors.NewInvalid("replicationController", controller.Name, errs)
+	if err := rest.BeforeUpdate(rs.strategy, ctx, controller, existingController); err != nil {
+		return nil, false, err
 	}
 	out, err := rs.registry.UpdateController(ctx, controller)
 	return out, false, err

pkg/registry/controller/rest_test.go

@@ -20,17 +20,20 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"reflect"
 	"strings"
 	"testing"
 
 	"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"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest/resttest"
 	_ "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 )
 
 func TestListControllersError(t *testing.T) {
@@ -258,6 +261,7 @@ func TestCreateController(t *testing.T) {
 	storage := REST{
 		registry:  &mockRegistry,
 		podLister: &mockPodRegistry,
+		strategy:  Strategy,
 	}
 	controller := &api.ReplicationController{
 		ObjectMeta: api.ObjectMeta{Name: "test"},
@@ -287,6 +291,7 @@ func TestControllerStorageValidatesCreate(t *testing.T) {
 	storage := REST{
 		registry:  &mockRegistry,
 		podLister: nil,
+		strategy:  Strategy,
 	}
 	failureCases := map[string]api.ReplicationController{
 		"empty ID": {
@@ -312,34 +317,71 @@ func TestControllerStorageValidatesCreate(t *testing.T) {
 	}
 }
 
-func TestControllerStorageValidatesUpdate(t *testing.T) {
-	mockRegistry := registrytest.ControllerRegistry{}
+func TestControllerValidatesUpdate(t *testing.T) {
+	mockRegistry := registrytest.ControllerRegistry{
+		Controllers: &api.ReplicationControllerList{},
+	}
 	storage := REST{
 		registry:  &mockRegistry,
 		podLister: nil,
+		strategy:  Strategy,
 	}
-	failureCases := map[string]api.ReplicationController{
-		"empty ID": {
-			ObjectMeta: api.ObjectMeta{Name: ""},
-			Spec: api.ReplicationControllerSpec{
-				Selector: map[string]string{"bar": "baz"},
-			},
-		},
-		"empty selector": {
-			ObjectMeta: api.ObjectMeta{Name: "abc"},
-			Spec:       api.ReplicationControllerSpec{},
-		},
+
+	var validControllerSpec = api.ReplicationControllerSpec{
+		Selector: validPodTemplate.Spec.Labels,
+		Template: &validPodTemplate.Spec,
 	}
+
+	var validController = api.ReplicationController{
+		ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "default"},
+		Spec:       validControllerSpec,
+	}
+
 	ctx := api.NewDefaultContext()
-	for _, failureCase := range failureCases {
-		c, created, err := storage.Update(ctx, &failureCase)
-		if c != nil || created {
+	storage.Create(ctx, &validController)
+	ns := "newNamespace"
+
+	updaters := []func(rc api.ReplicationController) (runtime.Object, bool, error){
+		func(rc api.ReplicationController) (runtime.Object, bool, error) {
+			rc.UID = "newUID"
+			return storage.Update(ctx, &rc)
+		},
+		func(rc api.ReplicationController) (runtime.Object, bool, error) {
+			rc.Name = ""
+			return storage.Update(ctx, &rc)
+		},
+		func(rc api.ReplicationController) (runtime.Object, bool, error) {
+			rc.Namespace = ns
+			return storage.Update(api.WithNamespace(ctx, ns), &rc)
+		},
+		func(rc api.ReplicationController) (runtime.Object, bool, error) {
+			rc.Spec.Selector = map[string]string{}
+			return storage.Update(ctx, &rc)
+		},
+	}
+	for _, u := range updaters {
+		c, updated, err := u(validController)
+		if c != nil || updated {
 			t.Errorf("Expected nil object and not created")
 		}
 		if !errors.IsInvalid(err) {
 			t.Errorf("Expected to get an invalid resource error, got %v", err)
 		}
 	}
+	// The update should fail if the namespace on the controller is set to something
+	// other than the namespace on the given context, even if the namespace on the
+	// controller is valid.
+	c, updated, err := storage.Update(api.WithNamespace(ctx, ns), &validController)
+	if c != nil || updated {
+		t.Errorf("Expected nil object and not created")
+	}
+	errSubString := "namespace"
+	if err == nil {
+		t.Errorf("Expected an error, but we didn't get one")
+	} else if !errors.IsBadRequest(err) ||
+		strings.Index(err.Error(), errSubString) == -1 {
+		t.Errorf("Expected a Bad Request error with the sub string '%s', got %v", errSubString, err)
+	}
 }
 
 type fakePodLister struct {
@@ -380,7 +422,7 @@ func TestCreateControllerWithGeneratedName(t *testing.T) {
 
 // TODO: remove, covered by TestCreate
 func TestCreateControllerWithConflictingNamespace(t *testing.T) {
-	storage := REST{}
+	storage := REST{strategy: Strategy}
 	controller := &api.ReplicationController{
 		ObjectMeta: api.ObjectMeta{Name: "test", Namespace: "not-default"},
 	}
@@ -390,28 +432,12 @@ func TestCreateControllerWithConflictingNamespace(t *testing.T) {
 	if channel != nil {
 		t.Error("Expected a nil channel, but we got a value")
 	}
+	errSubString := "namespace"
 	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())
-	}
-}
-
-func TestUpdateControllerWithConflictingNamespace(t *testing.T) {
-	storage := REST{}
-	controller := &api.ReplicationController{
-		ObjectMeta: api.ObjectMeta{Name: "test", Namespace: "not-default"},
-	}
-
-	ctx := api.NewDefaultContext()
-	obj, created, err := storage.Update(ctx, controller)
-	if obj != nil || created {
-		t.Error("Expected a nil object, but we got a value or created was true")
-	}
-	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 {
-		t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error())
+	} else if !errors.IsBadRequest(err) ||
+		strings.Index(err.Error(), errSubString) == -1 {
+		t.Errorf("Expected a Bad Request error with the sub string '%s', got %v", errSubString, err)
 	}
 }
 
@@ -437,3 +463,75 @@ func TestCreate(t *testing.T) {
 		},
 	)
 }
+
+func TestBeforeCreate(t *testing.T) {
+	failures := []runtime.Object{
+		&api.Service{},
+		&api.Service{
+			ObjectMeta: api.ObjectMeta{
+				Name:      "foo",
+				Namespace: "#$%%invalid",
+			},
+		},
+		&api.Service{
+			ObjectMeta: api.ObjectMeta{
+				Name:      "##&*(&invalid",
+				Namespace: api.NamespaceDefault,
+			},
+		},
+	}
+	for _, test := range failures {
+		ctx := api.NewDefaultContext()
+		err := rest.BeforeCreate(rest.Services, ctx, test)
+		if err == nil {
+			t.Errorf("unexpected non-error for %v", test)
+		}
+	}
+
+	obj := &api.ReplicationController{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "foo",
+			Namespace: api.NamespaceDefault,
+		},
+		Spec: api.ReplicationControllerSpec{
+			Selector: map[string]string{"name": "foo"},
+			Template: &api.PodTemplateSpec{
+				ObjectMeta: api.ObjectMeta{
+					Labels: map[string]string{
+						"name": "foo",
+					},
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:            "foo",
+							Image:           "foo",
+							ImagePullPolicy: api.PullAlways,
+						},
+					},
+					RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}},
+					DNSPolicy:     api.DNSDefault,
+				},
+			},
+		},
+		Status: api.ReplicationControllerStatus{
+			Replicas: 3,
+		},
+	}
+	ctx := api.NewDefaultContext()
+	err := rest.BeforeCreate(Strategy, ctx, obj)
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+	if !reflect.DeepEqual(obj.Status, api.ReplicationControllerStatus{}) {
+		t.Errorf("status was not cleared as expected.")
+	}
+	if obj.Name != "foo" || obj.Namespace != api.NamespaceDefault {
+		t.Errorf("unexpected object metadata: %v", obj.ObjectMeta)
+	}
+
+	obj.Spec.Replicas = -1
+	if err := rest.BeforeCreate(Strategy, ctx, obj); err == nil {
+		t.Errorf("unexpected non-error for invalid replication controller.")
+	}
+}

pkg/registry/registrytest/controller.go

@@ -47,12 +47,22 @@ func (r *ControllerRegistry) ListControllers(ctx api.Context) (*api.ReplicationC
 func (r *ControllerRegistry) GetController(ctx api.Context, ID string) (*api.ReplicationController, error) {
 	r.Lock()
 	defer r.Unlock()
+	if r.Controllers != nil {
+		for _, rc := range r.Controllers.Items {
+			if ID == rc.Name {
+				return &r.Controllers.Items[0], r.Err
+			}
+		}
+	}
 	return &api.ReplicationController{}, r.Err
 }
 
 func (r *ControllerRegistry) CreateController(ctx api.Context, controller *api.ReplicationController) (*api.ReplicationController, error) {
 	r.Lock()
 	defer r.Unlock()
+	if r.Controllers != nil {
+		r.Controllers.Items = append(r.Controllers.Items, *controller)
+	}
 	return controller, r.Err
 }