Merge pull request #623 from smarterclayton/validate_replication_controllers

Add validation to ReplicationControllers

pkg/api/validation.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/golang/glog"
 )
@@ -286,8 +287,21 @@ func ValidateService(service *Service) []error {
 	if service.ID == "" {
 		allErrs.Append(makeInvalidError("Service.ID", service.ID))
 	}
-	if len(service.Selector) == 0 {
+	if labels.Set(service.Selector).AsSelector().Empty() {
 		allErrs.Append(makeInvalidError("Service.Selector", service.Selector))
 	}
 	return []error(allErrs)
 }
+
+// ValidateReplicationController tests if required fields in the replication controller are set.
+func ValidateReplicationController(controller *ReplicationController) []error {
+	errors := []error{}
+	if controller.ID == "" {
+		errors = append(errors, makeInvalidError("ReplicationController.ID", controller.ID))
+	}
+	if labels.Set(controller.DesiredState.ReplicaSelector).AsSelector().Empty() {
+		errors = append(errors, makeInvalidError("ReplicationController.ReplicaSelector", controller.DesiredState.ReplicaSelector))
+	}
+	errors = append(errors, ValidateManifest(&controller.DesiredState.PodTemplate.DesiredState.Manifest)...)
+	return errors
+}

pkg/api/validation_test.go

@@ -294,3 +294,63 @@ func TestValidateService(t *testing.T) {
 		t.Errorf("Unexpected error list: %#v", errs)
 	}
 }
+
+func TestValidateReplicationController(t *testing.T) {
+	validSelector := map[string]string{"a": "b"}
+	validPodTemplate := PodTemplate{
+		DesiredState: PodState{
+			Manifest: ContainerManifest{
+				Version: "v1beta1",
+			},
+		},
+	}
+
+	successCases := []ReplicationController{
+		{
+			JSONBase: JSONBase{ID: "abc"},
+			DesiredState: ReplicationControllerState{
+				ReplicaSelector: validSelector,
+				PodTemplate:     validPodTemplate,
+			},
+		},
+		{
+			JSONBase: JSONBase{ID: "abc-123"},
+			DesiredState: ReplicationControllerState{
+				ReplicaSelector: validSelector,
+				PodTemplate:     validPodTemplate,
+			},
+		},
+	}
+	for _, successCase := range successCases {
+		if errs := ValidateReplicationController(&successCase); len(errs) != 0 {
+			t.Errorf("expected success: %v", errs)
+		}
+	}
+
+	errorCases := map[string]ReplicationController{
+		"zero-length ID": {
+			JSONBase: JSONBase{ID: ""},
+			DesiredState: ReplicationControllerState{
+				ReplicaSelector: validSelector,
+				PodTemplate:     validPodTemplate,
+			},
+		},
+		"empty selector": {
+			JSONBase: JSONBase{ID: "abc"},
+			DesiredState: ReplicationControllerState{
+				PodTemplate: validPodTemplate,
+			},
+		},
+		"invalid manifest": {
+			JSONBase: JSONBase{ID: "abc"},
+			DesiredState: ReplicationControllerState{
+				ReplicaSelector: validSelector,
+			},
+		},
+	}
+	for k, v := range errorCases {
+		if errs := ValidateReplicationController(&v); len(errs) == 0 {
+			t.Errorf("expected failure for %s", k)
+		}
+	}
+}

pkg/labels/selector.go

@@ -27,6 +27,9 @@ type Selector interface {
 	// Matches returns true if this selector matches the given set of labels.
 	Matches(Labels) bool
 
+	// Empty returns true if this selector does not restrict the selection space.
+	Empty() bool
+
 	// String returns a human readable string that represents this selector.
 	String() string
 }
@@ -44,6 +47,10 @@ func (t *hasTerm) Matches(ls Labels) bool {
 	return ls.Get(t.label) == t.value
 }
 
+func (t *hasTerm) Empty() bool {
+	return false
+}
+
 func (t *hasTerm) String() string {
 	return fmt.Sprintf("%v=%v", t.label, t.value)
 }
@@ -56,6 +63,10 @@ func (t *notHasTerm) Matches(ls Labels) bool {
 	return ls.Get(t.label) != t.value
 }
 
+func (t *notHasTerm) Empty() bool {
+	return false
+}
+
 func (t *notHasTerm) String() string {
 	return fmt.Sprintf("%v!=%v", t.label, t.value)
 }
@@ -71,6 +82,21 @@ func (t andTerm) Matches(ls Labels) bool {
 	return true
 }
 
+func (t andTerm) Empty() bool {
+	if t == nil {
+		return true
+	}
+	if len([]Selector(t)) == 0 {
+		return true
+	}
+	for i := range t {
+		if !t[i].Empty() {
+			return false
+		}
+	}
+	return true
+}
+
 func (t andTerm) String() string {
 	var terms []string
 	for _, q := range t {
@@ -87,8 +113,12 @@ func try(selectorPiece, op string) (lhs, rhs string, ok bool) {
 	return "", "", false
 }
 
-// SelectorFromSet returns a Selector which will match exactly the given Set.
+// SelectorFromSet returns a Selector which will match exactly the given Set. A
+// nil Set is considered equivalent to Everything().
 func SelectorFromSet(ls Set) Selector {
+	if ls == nil {
+		return Everything()
+	}
 	items := make([]Selector, 0, len(ls))
 	for label, value := range ls {
 		items = append(items, &hasTerm{label: label, value: value})

pkg/labels/selector_test.go

@@ -84,6 +84,9 @@ func TestEverything(t *testing.T) {
 	if !Everything().Matches(Set{"x": "y"}) {
 		t.Errorf("Nil selector didn't match")
 	}
+	if !Everything().Empty() {
+		t.Errorf("Everything was not empty")
+	}
 }
 
 func TestSelectorMatches(t *testing.T) {
@@ -132,3 +135,31 @@ func TestSetMatches(t *testing.T) {
 	expectNoMatchDirect(t, Set{"baz": "=bar"}, labelset)
 	expectNoMatchDirect(t, Set{"foo": "=bar", "foobar": "bar", "baz": "blah"}, labelset)
 }
+
+func TestNilMapIsValid(t *testing.T) {
+	selector := Set(nil).AsSelector()
+	if selector == nil {
+		t.Errorf("Selector for nil set should be Everything")
+	}
+	if !selector.Empty() {
+		t.Errorf("Selector for nil set should be Empty")
+	}
+}
+
+func TestSetIsEmpty(t *testing.T) {
+	if !(Set{}).AsSelector().Empty() {
+		t.Errorf("Empty set should be empty")
+	}
+	if !(andTerm(nil)).Empty() {
+		t.Errorf("Nil andTerm should be empty")
+	}
+	if (&hasTerm{}).Empty() {
+		t.Errorf("hasTerm should not be empty")
+	}
+	if !(andTerm{andTerm{}}).Empty() {
+		t.Errorf("Nested andTerm should be empty")
+	}
+	if (andTerm{&hasTerm{"a", "b"}}).Empty() {
+		t.Errorf("Nested andTerm should not be empty")
+	}
+}

pkg/registry/controller_registry.go

@@ -91,6 +91,10 @@ func (storage *ControllerRegistryStorage) Create(obj interface{}) (<-chan interf
 	// Pod Manifest ID should be assigned by the pod API
 	controller.DesiredState.PodTemplate.DesiredState.Manifest.ID = ""
 
+	if errs := api.ValidateReplicationController(&controller); len(errs) > 0 {
+		return nil, fmt.Errorf("Validation errors: %v", errs)
+	}
+
 	return apiserver.MakeAsync(func() (interface{}, error) {
 		err := storage.registry.CreateController(controller)
 		if err != nil {
@@ -106,8 +110,8 @@ func (storage *ControllerRegistryStorage) Update(obj interface{}) (<-chan interf
 	if !ok {
 		return nil, fmt.Errorf("not a replication controller: %#v", obj)
 	}
-	if len(controller.ID) == 0 {
+	if errs := api.ValidateReplicationController(&controller); len(errs) > 0 {
-		return nil, fmt.Errorf("ID should not be empty: %#v", controller)
+		return nil, fmt.Errorf("Validation errors: %v", errs)
 	}
 	return apiserver.MakeAsync(func() (interface{}, error) {
 		err := storage.registry.UpdateController(controller)

pkg/registry/controller_registry_test.go

@@ -187,12 +187,27 @@ func TestControllerParsing(t *testing.T) {
 	}
 }
 
+var validPodTemplate = api.PodTemplate{
+	DesiredState: api.PodState{
+		Manifest: api.ContainerManifest{
+			Version: "v1beta1",
+			Containers: []api.Container{
+				{
+					Name:  "test",
+					Image: "test_image",
+				},
+			},
+		},
+	},
+}
+
 func TestCreateController(t *testing.T) {
 	mockRegistry := MockControllerRegistry{}
 	mockPodRegistry := MockPodRegistry{
 		pods: []api.Pod{
 			{
 				JSONBase: api.JSONBase{ID: "foo"},
+				Labels:   map[string]string{"a": "b"},
 			},
 		},
 	}
@@ -204,10 +219,15 @@ func TestCreateController(t *testing.T) {
 	controller := api.ReplicationController{
 		JSONBase: api.JSONBase{ID: "test"},
 		DesiredState: api.ReplicationControllerState{
-			Replicas: 2,
+			Replicas:        2,
+			ReplicaSelector: map[string]string{"a": "b"},
+			PodTemplate:     validPodTemplate,
 		},
 	}
 	channel, err := storage.Create(controller)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	expectNoError(t, err)
 
 	select {
@@ -221,9 +241,11 @@ func TestCreateController(t *testing.T) {
 	mockPodRegistry.pods = []api.Pod{
 		{
 			JSONBase: api.JSONBase{ID: "foo"},
+			Labels:   map[string]string{"a": "b"},
 		},
 		{
 			JSONBase: api.JSONBase{ID: "bar"},
+			Labels:   map[string]string{"a": "b"},
 		},
 	}
 	mockPodRegistry.Unlock()
@@ -235,3 +257,65 @@ func TestCreateController(t *testing.T) {
 		// Do nothing, this is expected
 	}
 }
+
+func TestControllerStorageValidatesCreate(t *testing.T) {
+	mockRegistry := MockControllerRegistry{}
+	storage := ControllerRegistryStorage{
+		registry:    &mockRegistry,
+		podRegistry: nil,
+		pollPeriod:  time.Millisecond * 1,
+	}
+
+	failureCases := map[string]api.ReplicationController{
+		"empty ID": api.ReplicationController{
+			JSONBase: api.JSONBase{ID: ""},
+			DesiredState: api.ReplicationControllerState{
+				ReplicaSelector: map[string]string{"bar": "baz"},
+			},
+		},
+		"empty selector": api.ReplicationController{
+			JSONBase:     api.JSONBase{ID: "abc"},
+			DesiredState: api.ReplicationControllerState{},
+		},
+	}
+	for _, failureCase := range failureCases {
+		c, err := storage.Create(failureCase)
+		if c != nil {
+			t.Errorf("Expected nil channel")
+		}
+		if err == nil {
+			t.Errorf("Expected to get an error")
+		}
+	}
+}
+
+func TestControllerStorageValidatesUpdate(t *testing.T) {
+	mockRegistry := MockControllerRegistry{}
+	storage := ControllerRegistryStorage{
+		registry:    &mockRegistry,
+		podRegistry: nil,
+		pollPeriod:  time.Millisecond * 1,
+	}
+
+	failureCases := map[string]api.ReplicationController{
+		"empty ID": api.ReplicationController{
+			JSONBase: api.JSONBase{ID: ""},
+			DesiredState: api.ReplicationControllerState{
+				ReplicaSelector: map[string]string{"bar": "baz"},
+			},
+		},
+		"empty selector": api.ReplicationController{
+			JSONBase:     api.JSONBase{ID: "abc"},
+			DesiredState: api.ReplicationControllerState{},
+		},
+	}
+	for _, failureCase := range failureCases {
+		c, err := storage.Update(failureCase)
+		if c != nil {
+			t.Errorf("Expected nil channel")
+		}
+		if err == nil {
+			t.Errorf("Expected to get an error")
+		}
+	}
+}

pkg/registry/service_registry.go

@@ -147,8 +147,7 @@ func (sr *ServiceRegistryStorage) Extract(body []byte) (interface{}, error) {
 
 func (sr *ServiceRegistryStorage) Create(obj interface{}) (<-chan interface{}, error) {
 	srv := obj.(api.Service)
-	errs := api.ValidateService(&srv)
+	if errs := api.ValidateService(&srv); len(errs) > 0 {
-	if len(errs) > 0 {
 		return nil, fmt.Errorf("Validation errors: %v", errs)
 	}
 	return apiserver.MakeAsync(func() (interface{}, error) {
@@ -187,6 +186,9 @@ func (sr *ServiceRegistryStorage) Update(obj interface{}) (<-chan interface{}, e
 	if srv.ID == "" {
 		return nil, fmt.Errorf("ID should not be empty: %#v", srv)
 	}
+	if errs := api.ValidateService(&srv); len(errs) > 0 {
+		return nil, fmt.Errorf("Validation errors: %v", errs)
+	}
 	return apiserver.MakeAsync(func() (interface{}, error) {
 		// TODO: check to see if external load balancer status changed
 		err := sr.registry.UpdateService(srv)

pkg/registry/service_registry_test.go

@@ -49,6 +49,60 @@ func TestServiceRegistry(t *testing.T) {
 	}
 }
 
+func TestServiceStorageValidatesCreate(t *testing.T) {
+	memory := MakeMemoryRegistry()
+	storage := MakeServiceRegistryStorage(memory, nil, nil)
+
+	failureCases := map[string]api.Service{
+		"empty ID": api.Service{
+			JSONBase: api.JSONBase{ID: ""},
+			Selector: map[string]string{"bar": "baz"},
+		},
+		"empty selector": api.Service{
+			JSONBase: api.JSONBase{ID: "foo"},
+			Selector: map[string]string{},
+		},
+	}
+	for _, failureCase := range failureCases {
+		c, err := storage.Create(failureCase)
+		if c != nil {
+			t.Errorf("Expected nil channel")
+		}
+		if err == nil {
+			t.Errorf("Expected to get an error")
+		}
+	}
+}
+
+func TestServiceStorageValidatesUpdate(t *testing.T) {
+	memory := MakeMemoryRegistry()
+	memory.CreateService(api.Service{
+		JSONBase: api.JSONBase{ID: "foo"},
+		Selector: map[string]string{"bar": "baz"},
+	})
+	storage := MakeServiceRegistryStorage(memory, nil, nil)
+
+	failureCases := map[string]api.Service{
+		"empty ID": api.Service{
+			JSONBase: api.JSONBase{ID: ""},
+			Selector: map[string]string{"bar": "baz"},
+		},
+		"empty selector": api.Service{
+			JSONBase: api.JSONBase{ID: "foo"},
+			Selector: map[string]string{},
+		},
+	}
+	for _, failureCase := range failureCases {
+		c, err := storage.Update(failureCase)
+		if c != nil {
+			t.Errorf("Expected nil channel")
+		}
+		if err == nil {
+			t.Errorf("Expected to get an error")
+		}
+	}
+}
+
 func TestServiceRegistryExternalService(t *testing.T) {
 	memory := MakeMemoryRegistry()
 	fakeCloud := &cloudprovider.FakeCloud{}