From 6d31c2bf8ab4ca4c7c039bb1d2da72e80f265729 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 13 Nov 2014 10:38:13 -0500 Subject: [PATCH] util.EncodeJSON proven harmful, remove it everywhere People were misusing EncodeJSON in tests when they should be using runtime.EncodeOrDie(testapi.Codec(), obj). Removing the potential for cutting self on sharp objects. --- .../controller/minioncontroller_test.go | 6 ++-- pkg/controller/replication_controller_test.go | 4 +-- pkg/kubelet/server_test.go | 14 ++++++-- pkg/service/endpoints_controller_test.go | 36 +++++++++---------- pkg/tools/etcd_tools_test.go | 31 ++++++++-------- pkg/util/util.go | 6 ---- pkg/util/util_test.go | 33 ----------------- 7 files changed, 49 insertions(+), 81 deletions(-) diff --git a/pkg/cloudprovider/controller/minioncontroller_test.go b/pkg/cloudprovider/controller/minioncontroller_test.go index 1bb82eb0f87..d12b8ccf5a0 100644 --- a/pkg/cloudprovider/controller/minioncontroller_test.go +++ b/pkg/cloudprovider/controller/minioncontroller_test.go @@ -30,7 +30,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) -func newMinionList(count int) api.MinionList { +func newMinionList(count int) *api.MinionList { minions := []api.Minion{} for i := 0; i < count; i++ { minions = append(minions, api.Minion{ @@ -39,7 +39,7 @@ func newMinionList(count int) api.MinionList { }, }) } - return api.MinionList{ + return &api.MinionList{ Items: minions, } } @@ -52,7 +52,7 @@ type serverResponse struct { func makeTestServer(t *testing.T, minionResponse serverResponse) (*httptest.Server, *util.FakeHandler) { fakeMinionHandler := util.FakeHandler{ StatusCode: minionResponse.statusCode, - ResponseBody: util.EncodeJSON(minionResponse.obj), + ResponseBody: runtime.EncodeOrDie(testapi.Codec(), minionResponse.obj.(runtime.Object)), } mux := http.NewServeMux() mux.Handle("/api/"+testapi.Version()+"/minions", &fakeMinionHandler) diff --git a/pkg/controller/replication_controller_test.go b/pkg/controller/replication_controller_test.go index 39c9716627e..4c728510cb6 100644 --- a/pkg/controller/replication_controller_test.go +++ b/pkg/controller/replication_controller_test.go @@ -284,10 +284,10 @@ func TestSynchonize(t *testing.T) { Node: &etcd.Node{ Nodes: []*etcd.Node{ { - Value: util.EncodeJSON(controllerSpec1), + Value: runtime.EncodeOrDie(testapi.Codec(), &controllerSpec1), }, { - Value: util.EncodeJSON(controllerSpec2), + Value: runtime.EncodeOrDie(testapi.Codec(), &controllerSpec2), }, }, }, diff --git a/pkg/kubelet/server_test.go b/pkg/kubelet/server_test.go index 4c879a5b772..dc535cc4405 100644 --- a/pkg/kubelet/server_test.go +++ b/pkg/kubelet/server_test.go @@ -30,7 +30,6 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/google/cadvisor/info" ) @@ -97,6 +96,15 @@ func newServerTest() *serverTestFramework { return fw } +// encodeJSON returns obj marshalled as a JSON string, panicing on any errors +func encodeJSON(obj interface{}) string { + data, err := json.Marshal(obj) + if err != nil { + panic(err) + } + return string(data) +} + func readResp(resp *http.Response) (string, error) { defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) @@ -124,7 +132,7 @@ func TestContainer(t *testing.T) { }, }, } - body := bytes.NewBuffer([]byte(util.EncodeJSON(expected[0]))) // Only send a single ContainerManifest + body := bytes.NewBuffer([]byte(encodeJSON(expected[0]))) // Only send a single ContainerManifest resp, err := http.Post(fw.testHTTPServer.URL+"/container", "application/json", body) if err != nil { t.Errorf("Post returned: %v", err) @@ -199,7 +207,7 @@ func TestContainers(t *testing.T) { }, }, } - body := bytes.NewBuffer([]byte(util.EncodeJSON(expected))) + body := bytes.NewBuffer([]byte(encodeJSON(expected))) resp, err := http.Post(fw.testHTTPServer.URL+"/containers", "application/json", body) if err != nil { t.Errorf("Post returned: %v", err) diff --git a/pkg/service/endpoints_controller_test.go b/pkg/service/endpoints_controller_test.go index e456bd3c97a..319dac157c6 100644 --- a/pkg/service/endpoints_controller_test.go +++ b/pkg/service/endpoints_controller_test.go @@ -30,7 +30,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) -func newPodList(count int) api.PodList { +func newPodList(count int) *api.PodList { pods := []api.Pod{} for i := 0; i < count; i++ { pods = append(pods, api.Pod{ @@ -54,7 +54,7 @@ func newPodList(count int) api.PodList { }, }) } - return api.PodList{ + return &api.PodList{ TypeMeta: api.TypeMeta{APIVersion: testapi.Version(), Kind: "PodList"}, Items: pods, } @@ -170,15 +170,15 @@ type serverResponse struct { func makeTestServer(t *testing.T, podResponse serverResponse, serviceResponse serverResponse, endpointsResponse serverResponse) (*httptest.Server, *util.FakeHandler) { fakePodHandler := util.FakeHandler{ StatusCode: podResponse.statusCode, - ResponseBody: util.EncodeJSON(podResponse.obj), + ResponseBody: runtime.EncodeOrDie(testapi.Codec(), podResponse.obj.(runtime.Object)), } fakeServiceHandler := util.FakeHandler{ StatusCode: serviceResponse.statusCode, - ResponseBody: util.EncodeJSON(serviceResponse.obj), + ResponseBody: runtime.EncodeOrDie(testapi.Codec(), serviceResponse.obj.(runtime.Object)), } fakeEndpointsHandler := util.FakeHandler{ StatusCode: endpointsResponse.statusCode, - ResponseBody: util.EncodeJSON(endpointsResponse.obj), + ResponseBody: runtime.EncodeOrDie(testapi.Codec(), endpointsResponse.obj.(runtime.Object)), } mux := http.NewServeMux() mux.Handle("/api/"+testapi.Version()+"/pods", &fakePodHandler) @@ -195,8 +195,8 @@ func makeTestServer(t *testing.T, podResponse serverResponse, serviceResponse se func TestSyncEndpointsEmpty(t *testing.T) { testServer, _ := makeTestServer(t, serverResponse{http.StatusOK, newPodList(0)}, - serverResponse{http.StatusOK, api.ServiceList{}}, - serverResponse{http.StatusOK, api.Endpoints{}}) + serverResponse{http.StatusOK, &api.ServiceList{}}, + serverResponse{http.StatusOK, &api.Endpoints{}}) defer testServer.Close() client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) endpoints := NewEndpointController(client) @@ -208,8 +208,8 @@ func TestSyncEndpointsEmpty(t *testing.T) { func TestSyncEndpointsError(t *testing.T) { testServer, _ := makeTestServer(t, serverResponse{http.StatusOK, newPodList(0)}, - serverResponse{http.StatusInternalServerError, api.ServiceList{}}, - serverResponse{http.StatusOK, api.Endpoints{}}) + serverResponse{http.StatusInternalServerError, &api.ServiceList{}}, + serverResponse{http.StatusOK, &api.Endpoints{}}) defer testServer.Close() client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) endpoints := NewEndpointController(client) @@ -233,8 +233,8 @@ func TestSyncEndpointsItemsPreexisting(t *testing.T) { } testServer, endpointsHandler := makeTestServer(t, serverResponse{http.StatusOK, newPodList(1)}, - serverResponse{http.StatusOK, serviceList}, - serverResponse{http.StatusOK, api.Endpoints{ + serverResponse{http.StatusOK, &serviceList}, + serverResponse{http.StatusOK, &api.Endpoints{ ObjectMeta: api.ObjectMeta{ Name: "foo", ResourceVersion: "1", @@ -272,8 +272,8 @@ func TestSyncEndpointsItemsPreexistingIdentical(t *testing.T) { } testServer, endpointsHandler := makeTestServer(t, serverResponse{http.StatusOK, newPodList(1)}, - serverResponse{http.StatusOK, serviceList}, - serverResponse{http.StatusOK, api.Endpoints{ + serverResponse{http.StatusOK, &serviceList}, + serverResponse{http.StatusOK, &api.Endpoints{ ObjectMeta: api.ObjectMeta{ ResourceVersion: "1", }, @@ -303,8 +303,8 @@ func TestSyncEndpointsItems(t *testing.T) { } testServer, endpointsHandler := makeTestServer(t, serverResponse{http.StatusOK, newPodList(1)}, - serverResponse{http.StatusOK, serviceList}, - serverResponse{http.StatusOK, api.Endpoints{}}) + serverResponse{http.StatusOK, &serviceList}, + serverResponse{http.StatusOK, &api.Endpoints{}}) defer testServer.Close() client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) endpoints := NewEndpointController(client) @@ -333,9 +333,9 @@ func TestSyncEndpointsPodError(t *testing.T) { }, } testServer, _ := makeTestServer(t, - serverResponse{http.StatusInternalServerError, api.PodList{}}, - serverResponse{http.StatusOK, serviceList}, - serverResponse{http.StatusOK, api.Endpoints{}}) + serverResponse{http.StatusInternalServerError, &api.PodList{}}, + serverResponse{http.StatusOK, &serviceList}, + serverResponse{http.StatusOK, &api.Endpoints{}}) defer testServer.Close() client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) endpoints := NewEndpointController(client) diff --git a/pkg/tools/etcd_tools_test.go b/pkg/tools/etcd_tools_test.go index 532c07c06a0..7bf2950b19d 100644 --- a/pkg/tools/etcd_tools_test.go +++ b/pkg/tools/etcd_tools_test.go @@ -24,10 +24,9 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" - "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/coreos/go-etcd/etcd" ) @@ -100,7 +99,7 @@ func TestExtractToList(t *testing.T) { } var got api.PodList - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} err := helper.ExtractToList("/some/key", &got) if err != nil { t.Errorf("Unexpected error %v", err) @@ -151,7 +150,7 @@ func TestExtractToListAcrossDirectories(t *testing.T) { } var got api.PodList - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} err := helper.ExtractToList("/some/key", &got) if err != nil { t.Errorf("Unexpected error %v", err) @@ -198,7 +197,7 @@ func TestExtractToListExcludesDirectories(t *testing.T) { } var got api.PodList - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} err := helper.ExtractToList("/some/key", &got) if err != nil { t.Errorf("Unexpected error %v", err) @@ -211,8 +210,8 @@ func TestExtractToListExcludesDirectories(t *testing.T) { func TestExtractObj(t *testing.T) { fakeClient := NewFakeEtcdClient(t) expect := api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}} - fakeClient.Set("/some/key", util.EncodeJSON(expect), 0) - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + fakeClient.Set("/some/key", runtime.EncodeOrDie(testapi.Codec(), &expect), 0) + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} var got api.Pod err := helper.ExtractObj("/some/key", &got, false) if err != nil { @@ -266,12 +265,12 @@ func TestExtractObjNotFoundErr(t *testing.T) { func TestCreateObj(t *testing.T) { obj := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}} fakeClient := NewFakeEtcdClient(t) - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} err := helper.CreateObj("/some/key", obj, 5) if err != nil { t.Errorf("Unexpected error %#v", err) } - data, err := latest.Codec.Encode(obj) + data, err := testapi.Codec().Encode(obj) if err != nil { t.Errorf("Unexpected error %#v", err) } @@ -287,12 +286,12 @@ func TestCreateObj(t *testing.T) { func TestSetObj(t *testing.T) { obj := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}} fakeClient := NewFakeEtcdClient(t) - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} err := helper.SetObj("/some/key", obj) if err != nil { t.Errorf("Unexpected error %#v", err) } - data, err := latest.Codec.Encode(obj) + data, err := testapi.Codec().Encode(obj) if err != nil { t.Errorf("Unexpected error %#v", err) } @@ -310,18 +309,18 @@ func TestSetObjWithVersion(t *testing.T) { fakeClient.Data["/some/key"] = EtcdResponseWithError{ R: &etcd.Response{ Node: &etcd.Node{ - Value: runtime.EncodeOrDie(latest.Codec, obj), + Value: runtime.EncodeOrDie(testapi.Codec(), obj), ModifiedIndex: 1, }, }, } - helper := EtcdHelper{fakeClient, latest.Codec, versioner} + helper := EtcdHelper{fakeClient, testapi.Codec(), versioner} err := helper.SetObj("/some/key", obj) if err != nil { t.Fatalf("Unexpected error %#v", err) } - data, err := latest.Codec.Encode(obj) + data, err := testapi.Codec().Encode(obj) if err != nil { t.Fatalf("Unexpected error %#v", err) } @@ -335,12 +334,12 @@ func TestSetObjWithVersion(t *testing.T) { func TestSetObjWithoutResourceVersioner(t *testing.T) { obj := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}} fakeClient := NewFakeEtcdClient(t) - helper := EtcdHelper{fakeClient, latest.Codec, nil} + helper := EtcdHelper{fakeClient, testapi.Codec(), nil} err := helper.SetObj("/some/key", obj) if err != nil { t.Errorf("Unexpected error %#v", err) } - data, err := latest.Codec.Encode(obj) + data, err := testapi.Codec().Encode(obj) if err != nil { t.Errorf("Unexpected error %#v", err) } diff --git a/pkg/util/util.go b/pkg/util/util.go index 515d5754c30..7baffc31af7 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -60,12 +60,6 @@ func Forever(f func(), period time.Duration) { } } -// EncodeJSON returns obj marshalled as a JSON string, ignoring any errors. -func EncodeJSON(obj interface{}) string { - data, _ := json.Marshal(obj) - return string(data) -} - // IntOrString is a type that can hold an int or a string. When used in // JSON or YAML marshalling and unmarshalling, it produces or consumes the // inner type. This allows you to have, for example, a JSON field that can diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index 8b181ccf31b..2c253c33a27 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -24,39 +24,6 @@ import ( "gopkg.in/v1/yaml" ) -type FakeTypeMeta struct { - ID string -} -type FakePod struct { - FakeTypeMeta `json:",inline" yaml:",inline"` - Labels map[string]string - Int int - Str string -} - -func TestEncodeJSON(t *testing.T) { - pod := FakePod{ - FakeTypeMeta: FakeTypeMeta{ID: "foo"}, - Labels: map[string]string{ - "foo": "bar", - "baz": "blah", - }, - Int: -6, - Str: "a string", - } - - body := EncodeJSON(pod) - - expectedBody, err := json.Marshal(pod) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - if string(expectedBody) != body { - t.Errorf("JSON doesn't match. Expected %s, saw %s", expectedBody, body) - } -} - func TestHandleCrash(t *testing.T) { count := 0 expect := 10