diff --git a/pkg/api/validation.go b/pkg/api/validation.go index ecef6e88985..3da4bdeda5b 100644 --- a/pkg/api/validation.go +++ b/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 +} diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 6916db9da51..ec5f432d1d4 100644 --- a/pkg/api/validation_test.go +++ b/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) + } + } +} diff --git a/pkg/labels/selector.go b/pkg/labels/selector.go index 42adf008acd..0845d82305f 100644 --- a/pkg/labels/selector.go +++ b/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}) diff --git a/pkg/labels/selector_test.go b/pkg/labels/selector_test.go index 30d6f265895..6c5b81296e3 100644 --- a/pkg/labels/selector_test.go +++ b/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") + } +} diff --git a/pkg/registry/controller_registry.go b/pkg/registry/controller_registry.go index a91bed859a4..39fd1ebedf4 100644 --- a/pkg/registry/controller_registry.go +++ b/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 { - return nil, fmt.Errorf("ID should not be empty: %#v", controller) + 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.UpdateController(controller) diff --git a/pkg/registry/controller_registry_test.go b/pkg/registry/controller_registry_test.go index ffcbb2a6ea9..8e9a46160df 100644 --- a/pkg/registry/controller_registry_test.go +++ b/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") + } + } +} diff --git a/pkg/registry/service_registry.go b/pkg/registry/service_registry.go index 70f8bb75f51..efc48a36a74 100644 --- a/pkg/registry/service_registry.go +++ b/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 len(errs) > 0 { + if errs := api.ValidateService(&srv); 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) diff --git a/pkg/registry/service_registry_test.go b/pkg/registry/service_registry_test.go index 67f7d74e348..1754d982827 100644 --- a/pkg/registry/service_registry_test.go +++ b/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{}