From 2aa3de12d4199e380701e78c9ecdcefcec6ad4d9 Mon Sep 17 00:00:00 2001 From: Yuki Yugui Sonoda Date: Wed, 16 Jul 2014 14:27:15 +0900 Subject: [PATCH] Make RESTful operations return 404 Not Found when the target resource does not exist. In the original implementation, GET, DELETE and PUT operations on non-existent resources used to return 500 but not 404. --- pkg/apiserver/apiserver.go | 56 +++++- pkg/apiserver/apiserver_test.go | 122 ++++++++++--- pkg/registry/etcd_registry.go | 20 ++- pkg/registry/memory_registry.go | 25 ++- pkg/registry/memory_registry_test.go | 236 ++++++++++++++++++++++++-- pkg/registry/service_registry_test.go | 28 ++- pkg/tools/etcd_tools.go | 16 +- 7 files changed, 433 insertions(+), 70 deletions(-) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 3e0bbadb2b781..004aa434dacfb 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -34,11 +34,39 @@ import ( "github.com/golang/glog" ) +// errNotFound is an error which indicates that a specified resource is not found. +type errNotFound string + +// Error returns a string representation of the err. +func (err errNotFound) Error() string { + return string(err) +} + +// IsNotFound determines if the err is an error which indicates that a specified resource was not found. +func IsNotFound(err error) bool { + _, ok := err.(errNotFound) + return ok +} + +// NewNotFoundErr returns a new error which indicates that the resource of the kind and the name was not found. +func NewNotFoundErr(kind, name string) error { + return errNotFound(fmt.Sprintf("%s %q not found", kind, name)) +} + // RESTStorage is a generic interface for RESTful storage services +// Resources whicih are exported to the RESTful API of apiserver need to implement this interface. type RESTStorage interface { + // List selects resources in the storage which match to the selector. List(labels.Selector) (interface{}, error) + + // Get finds a resource in the storage by id and returns it. + // Although it can return an arbitrary error value, IsNotFound(err) is true for the returned error value err when the specified resource is not found. Get(id string) (interface{}, error) + + // Delete finds a resource in the storage and deletes it. + // Although it can return an arbitrary error value, IsNotFound(err) is true for the returned error value err when the specified resource is not found. Delete(id string) (<-chan interface{}, error) + Extract(body []byte) (interface{}, error) Create(interface{}) (<-chan interface{}, error) Update(interface{}) (<-chan interface{}, error) @@ -260,12 +288,12 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re server.write(http.StatusOK, list, w) case 2: item, err := storage.Get(parts[1]) - if err != nil { - server.error(err, w) + if IsNotFound(err) { + server.notFound(req, w) return } - if item == nil { - server.notFound(req, w) + if err != nil { + server.error(err, w) return } server.write(http.StatusOK, item, w) @@ -283,11 +311,19 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re return } obj, err := storage.Extract(body) + if IsNotFound(err) { + server.notFound(req, w) + return + } if err != nil { server.error(err, w) return } out, err := storage.Create(obj) + if IsNotFound(err) { + server.notFound(req, w) + return + } if err != nil { server.error(err, w) return @@ -299,6 +335,10 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re return } out, err := storage.Delete(parts[1]) + if IsNotFound(err) { + server.notFound(req, w) + return + } if err != nil { server.error(err, w) return @@ -314,11 +354,19 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re server.error(err, w) } obj, err := storage.Extract(body) + if IsNotFound(err) { + server.notFound(req, w) + return + } if err != nil { server.error(err, w) return } out, err := storage.Update(obj) + if IsNotFound(err) { + server.notFound(req, w) + return + } if err != nil { server.error(err, w) return diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index f7d47ce1d4bc7..01e50acdf1e9d 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -53,7 +53,7 @@ type SimpleList struct { } type SimpleRESTStorage struct { - err error + errors map[string]error list []Simple item Simple deleted string @@ -69,17 +69,17 @@ func (storage *SimpleRESTStorage) List(labels.Selector) (interface{}, error) { result := &SimpleList{ Items: storage.list, } - return result, storage.err + return result, storage.errors["list"] } func (storage *SimpleRESTStorage) Get(id string) (interface{}, error) { - return storage.item, storage.err + return storage.item, storage.errors["get"] } func (storage *SimpleRESTStorage) Delete(id string) (<-chan interface{}, error) { storage.deleted = id - if storage.err != nil { - return nil, storage.err + if storage.errors["delete"] != nil { + return nil, storage.errors["delete"] } return MakeAsync(func() (interface{}, error) { if storage.injectedFunction != nil { @@ -92,13 +92,13 @@ func (storage *SimpleRESTStorage) Delete(id string) (<-chan interface{}, error) func (storage *SimpleRESTStorage) Extract(body []byte) (interface{}, error) { var item Simple api.DecodeInto(body, &item) - return item, storage.err + return item, storage.errors["extract"] } func (storage *SimpleRESTStorage) Create(obj interface{}) (<-chan interface{}, error) { storage.created = obj.(Simple) - if storage.err != nil { - return nil, storage.err + if storage.errors["create"] != nil { + return nil, storage.errors["create"] } return MakeAsync(func() (interface{}, error) { if storage.injectedFunction != nil { @@ -110,8 +110,8 @@ func (storage *SimpleRESTStorage) Create(obj interface{}) (<-chan interface{}, e func (storage *SimpleRESTStorage) Update(obj interface{}) (<-chan interface{}, error) { storage.updated = obj.(Simple) - if storage.err != nil { - return nil, storage.err + if storage.errors["update"] != nil { + return nil, storage.errors["update"] } return MakeAsync(func() (interface{}, error) { if storage.injectedFunction != nil { @@ -141,15 +141,15 @@ func TestSimpleList(t *testing.T) { resp, err := http.Get(server.URL + "/prefix/version/simple") expectNoError(t, err) - if resp.StatusCode != 200 { - t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, 200, resp) + if resp.StatusCode != http.StatusOK { + t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, http.StatusOK, resp) } } func TestErrorList(t *testing.T) { storage := map[string]RESTStorage{} simpleStorage := SimpleRESTStorage{ - err: fmt.Errorf("test Error"), + errors: map[string]error{"list": fmt.Errorf("test Error")}, } storage["simple"] = &simpleStorage handler := New(storage, "/prefix/version") @@ -158,8 +158,8 @@ func TestErrorList(t *testing.T) { resp, err := http.Get(server.URL + "/prefix/version/simple") expectNoError(t, err) - if resp.StatusCode != 500 { - t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, 200, resp) + if resp.StatusCode != http.StatusInternalServerError { + t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, http.StatusOK, resp) } } @@ -179,8 +179,8 @@ func TestNonEmptyList(t *testing.T) { resp, err := http.Get(server.URL + "/prefix/version/simple") expectNoError(t, err) - if resp.StatusCode != 200 { - t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, 200, resp) + if resp.StatusCode != http.StatusOK { + t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, http.StatusOK, resp) } var listOut SimpleList @@ -215,6 +215,22 @@ func TestGet(t *testing.T) { } } +func TestGetMissing(t *testing.T) { + storage := map[string]RESTStorage{} + simpleStorage := SimpleRESTStorage{ + errors: map[string]error{"get": NewNotFoundErr("simple", "id")}, + } + storage["simple"] = &simpleStorage + handler := New(storage, "/prefix/version") + server := httptest.NewServer(handler) + + resp, err := http.Get(server.URL + "/prefix/version/simple/id") + expectNoError(t, err) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("Unexpected response %#v", resp) + } +} + func TestDelete(t *testing.T) { storage := map[string]RESTStorage{} simpleStorage := SimpleRESTStorage{} @@ -232,6 +248,25 @@ func TestDelete(t *testing.T) { } } +func TestDeleteMissing(t *testing.T) { + storage := map[string]RESTStorage{} + ID := "id" + simpleStorage := SimpleRESTStorage{ + errors: map[string]error{"delete": NewNotFoundErr("simple", ID)}, + } + storage["simple"] = &simpleStorage + handler := New(storage, "/prefix/version") + server := httptest.NewServer(handler) + + client := http.Client{} + request, err := http.NewRequest("DELETE", server.URL+"/prefix/version/simple/"+ID, nil) + response, err := client.Do(request) + expectNoError(t, err) + if response.StatusCode != http.StatusNotFound { + t.Errorf("Unexpected response %#v", response) + } +} + func TestUpdate(t *testing.T) { storage := map[string]RESTStorage{} simpleStorage := SimpleRESTStorage{} @@ -254,6 +289,30 @@ func TestUpdate(t *testing.T) { } } +func TestUpdateMissing(t *testing.T) { + storage := map[string]RESTStorage{} + ID := "id" + simpleStorage := SimpleRESTStorage{ + errors: map[string]error{"update": NewNotFoundErr("simple", ID)}, + } + storage["simple"] = &simpleStorage + handler := New(storage, "/prefix/version") + server := httptest.NewServer(handler) + + item := Simple{ + Name: "bar", + } + body, err := api.Encode(item) + expectNoError(t, err) + client := http.Client{} + request, err := http.NewRequest("PUT", server.URL+"/prefix/version/simple/"+ID, bytes.NewReader(body)) + response, err := client.Do(request) + expectNoError(t, err) + if response.StatusCode != http.StatusNotFound { + t.Errorf("Unexpected response %#v", response) + } +} + func TestBadPath(t *testing.T) { handler := New(map[string]RESTStorage{}, "/prefix/version") server := httptest.NewServer(handler) @@ -263,7 +322,7 @@ func TestBadPath(t *testing.T) { expectNoError(t, err) response, err := client.Do(request) expectNoError(t, err) - if response.StatusCode != 404 { + if response.StatusCode != http.StatusNotFound { t.Errorf("Unexpected response %#v", response) } } @@ -277,7 +336,7 @@ func TestMissingPath(t *testing.T) { expectNoError(t, err) response, err := client.Do(request) expectNoError(t, err) - if response.StatusCode != 404 { + if response.StatusCode != http.StatusNotFound { t.Errorf("Unexpected response %#v", response) } } @@ -293,7 +352,7 @@ func TestMissingStorage(t *testing.T) { expectNoError(t, err) response, err := client.Do(request) expectNoError(t, err) - if response.StatusCode != 404 { + if response.StatusCode != http.StatusNotFound { t.Errorf("Unexpected response %#v", response) } } @@ -324,6 +383,29 @@ func TestCreate(t *testing.T) { } } +func TestCreateNotFound(t *testing.T) { + simpleStorage := &SimpleRESTStorage{ + // storage.Create can fail with not found error in theory. + // See https://github.com/GoogleCloudPlatform/kubernetes/pull/486#discussion_r15037092. + errors: map[string]error{"create": NewNotFoundErr("simple", "id")}, + } + handler := New(map[string]RESTStorage{ + "foo": simpleStorage, + }, "/prefix/version") + server := httptest.NewServer(handler) + client := http.Client{} + + simple := Simple{Name: "foo"} + data, _ := api.Encode(simple) + request, err := http.NewRequest("POST", server.URL+"/prefix/version/foo", bytes.NewBuffer(data)) + expectNoError(t, err) + response, err := client.Do(request) + expectNoError(t, err) + if response.StatusCode != http.StatusNotFound { + t.Errorf("Unexpected response %#v", response) + } +} + func TestParseTimeout(t *testing.T) { if d := parseTimeout(""); d != 30*time.Second { t.Errorf("blank timeout produces %v", d) diff --git a/pkg/registry/etcd_registry.go b/pkg/registry/etcd_registry.go index 5f9c75be758a5..626f9ba083edb 100644 --- a/pkg/registry/etcd_registry.go +++ b/pkg/registry/etcd_registry.go @@ -20,6 +20,7 @@ import ( "fmt" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" "github.com/golang/glog" @@ -28,7 +29,7 @@ import ( // TODO: Need to add a reconciler loop that makes sure that things in pods are reflected into // kubelet (and vice versa) -// EtcdRegistry is an implementation of both ControllerRegistry and PodRegistry which is backed with etcd. +// EtcdRegistry implements PodRegistry, ControllerRegistry and ServiceRegistry with backed by etcd. type EtcdRegistry struct { etcdClient tools.EtcdClient machines MinionRegistry @@ -143,6 +144,9 @@ func (registry *EtcdRegistry) deletePodFromMachine(machine, podID string) error // machine and attempt to put it somewhere. podKey := makePodKey(machine, podID) _, err := registry.etcdClient.Delete(podKey, true) + if tools.IsEtcdNotFound(err) { + return apiserver.NewNotFoundErr("pod", podID) + } if err != nil { return err } @@ -191,7 +195,7 @@ func (registry *EtcdRegistry) findPod(podID string) (api.Pod, string, error) { return pod, machine, nil } } - return api.Pod{}, "", fmt.Errorf("pod not found %s", podID) + return api.Pod{}, "", apiserver.NewNotFoundErr("pod", podID) } // ListControllers obtains a list of ReplicationControllers. @@ -210,6 +214,9 @@ func (registry *EtcdRegistry) GetController(controllerID string) (*api.Replicati var controller api.ReplicationController key := makeControllerKey(controllerID) err := registry.helper().ExtractObj(key, &controller, false) + if tools.IsEtcdNotFound(err) { + return nil, apiserver.NewNotFoundErr("replicationController", controllerID) + } if err != nil { return nil, err } @@ -231,6 +238,9 @@ func (registry *EtcdRegistry) UpdateController(controller api.ReplicationControl func (registry *EtcdRegistry) DeleteController(controllerID string) error { key := makeControllerKey(controllerID) _, err := registry.etcdClient.Delete(key, false) + if tools.IsEtcdNotFound(err) { + return apiserver.NewNotFoundErr("replicationController", controllerID) + } return err } @@ -255,6 +265,9 @@ func (registry *EtcdRegistry) GetService(name string) (*api.Service, error) { key := makeServiceKey(name) var svc api.Service err := registry.helper().ExtractObj(key, &svc, false) + if tools.IsEtcdNotFound(err) { + return nil, apiserver.NewNotFoundErr("service", name) + } if err != nil { return nil, err } @@ -265,6 +278,9 @@ func (registry *EtcdRegistry) GetService(name string) (*api.Service, error) { func (registry *EtcdRegistry) DeleteService(name string) error { key := makeServiceKey(name) _, err := registry.etcdClient.Delete(key, true) + if tools.IsEtcdNotFound(err) { + return apiserver.NewNotFoundErr("service", name) + } if err != nil { return err } diff --git a/pkg/registry/memory_registry.go b/pkg/registry/memory_registry.go index 63dee81d0cb46..51d7c5f34c6b7 100644 --- a/pkg/registry/memory_registry.go +++ b/pkg/registry/memory_registry.go @@ -18,6 +18,7 @@ package registry import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" ) @@ -52,7 +53,7 @@ func (registry *MemoryRegistry) GetPod(podID string) (*api.Pod, error) { if found { return &pod, nil } else { - return nil, nil + return nil, apiserver.NewNotFoundErr("pod", podID) } } @@ -62,11 +63,17 @@ func (registry *MemoryRegistry) CreatePod(machine string, pod api.Pod) error { } func (registry *MemoryRegistry) DeletePod(podID string) error { + if _, ok := registry.podData[podID]; !ok { + return apiserver.NewNotFoundErr("pod", podID) + } delete(registry.podData, podID) return nil } func (registry *MemoryRegistry) UpdatePod(pod api.Pod) error { + if _, ok := registry.podData[pod.ID]; !ok { + return apiserver.NewNotFoundErr("pod", pod.ID) + } registry.podData[pod.ID] = pod return nil } @@ -84,7 +91,7 @@ func (registry *MemoryRegistry) GetController(controllerID string) (*api.Replica if found { return &controller, nil } else { - return nil, nil + return nil, apiserver.NewNotFoundErr("replicationController", controllerID) } } @@ -94,11 +101,17 @@ func (registry *MemoryRegistry) CreateController(controller api.ReplicationContr } func (registry *MemoryRegistry) DeleteController(controllerID string) error { + if _, ok := registry.controllerData[controllerID]; !ok { + return apiserver.NewNotFoundErr("replicationController", controllerID) + } delete(registry.controllerData, controllerID) return nil } func (registry *MemoryRegistry) UpdateController(controller api.ReplicationController) error { + if _, ok := registry.controllerData[controller.ID]; !ok { + return apiserver.NewNotFoundErr("replicationController", controller.ID) + } registry.controllerData[controller.ID] = controller return nil } @@ -121,16 +134,22 @@ func (registry *MemoryRegistry) GetService(name string) (*api.Service, error) { if found { return &svc, nil } else { - return nil, nil + return nil, apiserver.NewNotFoundErr("service", name) } } func (registry *MemoryRegistry) DeleteService(name string) error { + if _, ok := registry.serviceData[name]; !ok { + return apiserver.NewNotFoundErr("service", name) + } delete(registry.serviceData, name) return nil } func (registry *MemoryRegistry) UpdateService(svc api.Service) error { + if _, ok := registry.serviceData[svc.ID]; !ok { + return apiserver.NewNotFoundErr("service", svc.ID) + } return registry.CreateService(svc) } diff --git a/pkg/registry/memory_registry_test.go b/pkg/registry/memory_registry_test.go index 3409ce0921894..ffbafc4a43d9b 100644 --- a/pkg/registry/memory_registry_test.go +++ b/pkg/registry/memory_registry_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" ) @@ -42,6 +43,18 @@ func TestMemoryListPods(t *testing.T) { } } +func TestMemoryGetPods(t *testing.T) { + registry := MakeMemoryRegistry() + pod, err := registry.GetPod("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.GetPod(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.GetPod(%q) = %v; expected failure with not found error", "foo", pod) + } + } +} + func TestMemorySetGetPods(t *testing.T) { registry := MakeMemoryRegistry() expectedPod := api.Pod{JSONBase: api.JSONBase{ID: "foo"}} @@ -53,6 +66,26 @@ func TestMemorySetGetPods(t *testing.T) { } } +func TestMemoryUpdatePods(t *testing.T) { + registry := MakeMemoryRegistry() + pod := api.Pod{ + JSONBase: api.JSONBase{ + ID: "foo", + }, + DesiredState: api.PodState{ + Host: "foo.com", + }, + } + err := registry.UpdatePod(pod) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.UpdatePod(%q) failed with %v; expected failure with not found error", pod, err) + } else { + t.Errorf("registry.UpdatePod(%q) succeeded; expected failure with not found error", pod) + } + } +} + func TestMemorySetUpdateGetPods(t *testing.T) { registry := MakeMemoryRegistry() oldPod := api.Pod{JSONBase: api.JSONBase{ID: "foo"}} @@ -73,34 +106,61 @@ func TestMemorySetUpdateGetPods(t *testing.T) { } } +func TestMemoryDeletePods(t *testing.T) { + registry := MakeMemoryRegistry() + err := registry.DeletePod("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.DeletePod(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.DeletePod(%q) succeeded; expected failure with not found error", "foo") + } + } +} + func TestMemorySetDeleteGetPods(t *testing.T) { registry := MakeMemoryRegistry() expectedPod := api.Pod{JSONBase: api.JSONBase{ID: "foo"}} registry.CreatePod("machine", expectedPod) registry.DeletePod("foo") pod, err := registry.GetPod("foo") - expectNoError(t, err) - if pod != nil { - t.Errorf("Unexpected pod: %#v", pod) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.GetPod(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.GetPod(%q) = %v; expected failure with not found error", "foo", pod) + } } } func TestListControllersEmpty(t *testing.T) { registry := MakeMemoryRegistry() - pods, err := registry.ListControllers() + ctls, err := registry.ListControllers() expectNoError(t, err) - if len(pods) != 0 { - t.Errorf("Unexpected pod list: %#v", pods) + if len(ctls) != 0 { + t.Errorf("Unexpected controller list: %#v", ctls) } } func TestMemoryListControllers(t *testing.T) { registry := MakeMemoryRegistry() registry.CreateController(api.ReplicationController{JSONBase: api.JSONBase{ID: "foo"}}) - pods, err := registry.ListControllers() + ctls, err := registry.ListControllers() expectNoError(t, err) - if len(pods) != 1 || pods[0].ID != "foo" { - t.Errorf("Unexpected pod list: %#v", pods) + if len(ctls) != 1 || ctls[0].ID != "foo" { + t.Errorf("Unexpected controller list: %#v", ctls) + } +} + +func TestMemoryGetController(t *testing.T) { + registry := MakeMemoryRegistry() + ctl, err := registry.GetController("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.GetController(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.GetController(%q) = %v; expected failure with not found error", "foo", ctl) + } } } @@ -108,10 +168,30 @@ func TestMemorySetGetControllers(t *testing.T) { registry := MakeMemoryRegistry() expectedController := api.ReplicationController{JSONBase: api.JSONBase{ID: "foo"}} registry.CreateController(expectedController) - pod, err := registry.GetController("foo") + ctl, err := registry.GetController("foo") expectNoError(t, err) - if expectedController.ID != pod.ID { - t.Errorf("Unexpected pod, expected %#v, actual %#v", expectedController, pod) + if expectedController.ID != ctl.ID { + t.Errorf("Unexpected controller, expected %#v, actual %#v", expectedController, ctl) + } +} + +func TestMemoryUpdateController(t *testing.T) { + registry := MakeMemoryRegistry() + ctl := api.ReplicationController{ + JSONBase: api.JSONBase{ + ID: "foo", + }, + DesiredState: api.ReplicationControllerState{ + Replicas: 2, + }, + } + err := registry.UpdateController(ctl) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.UpdateController(%q) failed with %v; expected failure with not found error", ctl, err) + } else { + t.Errorf("registry.UpdateController(%q) succeeded; expected failure with not found error", ctl) + } } } @@ -128,10 +208,22 @@ func TestMemorySetUpdateGetControllers(t *testing.T) { } registry.CreateController(oldController) registry.UpdateController(expectedController) - pod, err := registry.GetController("foo") + ctl, err := registry.GetController("foo") expectNoError(t, err) - if expectedController.ID != pod.ID || pod.DesiredState.Replicas != expectedController.DesiredState.Replicas { - t.Errorf("Unexpected pod, expected %#v, actual %#v", expectedController, pod) + if expectedController.ID != ctl.ID || ctl.DesiredState.Replicas != expectedController.DesiredState.Replicas { + t.Errorf("Unexpected controller, expected %#v, actual %#v", expectedController, ctl) + } +} + +func TestMemoryDeleteController(t *testing.T) { + registry := MakeMemoryRegistry() + err := registry.DeleteController("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.DeleteController(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.DeleteController(%q) succeeded; expected failure with not found error", "foo") + } } } @@ -140,9 +232,117 @@ func TestMemorySetDeleteGetControllers(t *testing.T) { expectedController := api.ReplicationController{JSONBase: api.JSONBase{ID: "foo"}} registry.CreateController(expectedController) registry.DeleteController("foo") - pod, err := registry.GetController("foo") + ctl, err := registry.GetController("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.GetController(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.GetController(%q) = %v; expected failure with not found error", "foo", ctl) + } + } +} + +func TestListServicesEmpty(t *testing.T) { + registry := MakeMemoryRegistry() + svcs, err := registry.ListServices() + expectNoError(t, err) + if len(svcs.Items) != 0 { + t.Errorf("Unexpected service list: %#v", svcs) + } +} + +func TestMemoryListServices(t *testing.T) { + registry := MakeMemoryRegistry() + registry.CreateService(api.Service{JSONBase: api.JSONBase{ID: "foo"}}) + svcs, err := registry.ListServices() + expectNoError(t, err) + if len(svcs.Items) != 1 || svcs.Items[0].ID != "foo" { + t.Errorf("Unexpected service list: %#v", svcs) + } +} + +func TestMemoryGetService(t *testing.T) { + registry := MakeMemoryRegistry() + svc, err := registry.GetService("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.GetService(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.GetService(%q) = %v; expected failure with not found error", "foo", svc) + } + } +} + +func TestMemorySetGetServices(t *testing.T) { + registry := MakeMemoryRegistry() + expectedService := api.Service{JSONBase: api.JSONBase{ID: "foo"}} + registry.CreateService(expectedService) + svc, err := registry.GetService("foo") expectNoError(t, err) - if pod != nil { - t.Errorf("Unexpected pod: %#v", pod) + if expectedService.ID != svc.ID { + t.Errorf("Unexpected service, expected %#v, actual %#v", expectedService, svc) + } +} + +func TestMemoryUpdateService(t *testing.T) { + registry := MakeMemoryRegistry() + svc := api.Service{ + JSONBase: api.JSONBase{ + ID: "foo", + }, + Port: 9000, + } + err := registry.UpdateService(svc) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.UpdateService(%q) failed with %v; expected failure with not found error", svc, err) + } else { + t.Errorf("registry.UpdateService(%q) succeeded; expected failure with not found error", svc) + } + } +} + +func TestMemorySetUpdateGetServices(t *testing.T) { + registry := MakeMemoryRegistry() + oldService := api.Service{JSONBase: api.JSONBase{ID: "foo"}} + expectedService := api.Service{ + JSONBase: api.JSONBase{ + ID: "foo", + }, + Port: 9000, + } + registry.CreateService(oldService) + registry.UpdateService(expectedService) + svc, err := registry.GetService("foo") + expectNoError(t, err) + if expectedService.ID != svc.ID || svc.Port != expectedService.Port { + t.Errorf("Unexpected service, expected %#v, actual %#v", expectedService, svc) + } +} + +func TestMemoryDeleteService(t *testing.T) { + registry := MakeMemoryRegistry() + err := registry.DeleteService("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.DeleteService(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.DeleteService(%q) succeeded; expected failure with not found error", "foo") + } + } +} + +func TestMemorySetDeleteGetServices(t *testing.T) { + registry := MakeMemoryRegistry() + expectedService := api.Service{JSONBase: api.JSONBase{ID: "foo"}} + registry.CreateService(expectedService) + registry.DeleteService("foo") + svc, err := registry.GetService("foo") + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("registry.GetService(%q) failed with %v; expected failure with not found error", "foo", err) + } else { + t.Errorf("registry.GetService(%q) = %v; expected failure with not found error", "foo", svc) + } } } diff --git a/pkg/registry/service_registry_test.go b/pkg/registry/service_registry_test.go index d16f4e8ea235c..67f7d74e34824 100644 --- a/pkg/registry/service_registry_test.go +++ b/pkg/registry/service_registry_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider" ) @@ -94,9 +95,12 @@ func TestServiceRegistryExternalServiceError(t *testing.T) { t.Errorf("Unexpected call(s): %#v", fakeCloud.Calls) } srv, err := memory.GetService("foo") - expectNoError(t, err) - if srv != nil { - t.Errorf("Unexpected service: %#v", *srv) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("memory.GetService(%q) failed with %v; expected failure with not found error", svc.ID, err) + } else { + t.Errorf("memory.GetService(%q) = %v; expected failure with not found error", svc.ID, srv) + } } } @@ -120,9 +124,12 @@ func TestServiceRegistryDelete(t *testing.T) { t.Errorf("Unexpected call(s): %#v", fakeCloud.Calls) } srv, err := memory.GetService(svc.ID) - expectNoError(t, err) - if srv != nil { - t.Errorf("Unexpected service: %#v", *srv) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("memory.GetService(%q) failed with %v; expected failure with not found error", svc.ID, err) + } else { + t.Errorf("memory.GetService(%q) = %v; expected failure with not found error", svc.ID, srv) + } } } @@ -147,8 +154,11 @@ func TestServiceRegistryDeleteExternal(t *testing.T) { t.Errorf("Unexpected call(s): %#v", fakeCloud.Calls) } srv, err := memory.GetService(svc.ID) - expectNoError(t, err) - if srv != nil { - t.Errorf("Unexpected service: %#v", *srv) + if !apiserver.IsNotFound(err) { + if err != nil { + t.Errorf("memory.GetService(%q) failed with %v; expected failure with not found error", svc.ID, err) + } else { + t.Errorf("memory.GetService(%q) = %v; expected failure with not found error", svc.ID, srv) + } } } diff --git a/pkg/tools/etcd_tools.go b/pkg/tools/etcd_tools.go index 9a94416ff60b3..1d3e39d03e4fc 100644 --- a/pkg/tools/etcd_tools.go +++ b/pkg/tools/etcd_tools.go @@ -72,20 +72,8 @@ func IsEtcdConflict(err error) bool { // Returns true iff err is an etcd error, whose errorCode matches errorCode func isEtcdErrorNum(err error, errorCode int) bool { - if err == nil { - return false - } - switch err.(type) { - case *etcd.EtcdError: - etcdError := err.(*etcd.EtcdError) - if etcdError == nil { - return false - } - if etcdError.ErrorCode == errorCode { - return true - } - } - return false + etcdError, ok := err.(*etcd.EtcdError) + return ok && etcdError != nil && etcdError.ErrorCode == errorCode } func (h *EtcdHelper) listEtcdNode(key string) ([]*etcd.Node, error) {