From 8d0016b54216f526bba7daadb743a1e7d5401906 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Mon, 14 Sep 2015 13:37:40 -0700 Subject: [PATCH] Address comments. --- pkg/master/master.go | 6 ++-- pkg/master/master_test.go | 13 ++++---- pkg/master/thirdparty_controller.go | 11 ++++--- pkg/master/thirdparty_controller_test.go | 38 +++++++++++++++++++----- 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/pkg/master/master.go b/pkg/master/master.go index 6e702fc8f38..76dd3dc658a 100644 --- a/pkg/master/master.go +++ b/pkg/master/master.go @@ -783,8 +783,8 @@ func (m *Master) InstallThirdPartyAPI(rsrc *experimental.ThirdPartyResource) err if err := thirdparty.InstallREST(m.handlerContainer); err != nil { glog.Fatalf("Unable to setup thirdparty api: %v", err) } - thirdPartyPrefix := "/thirdparty/" + group + "/" - apiserver.AddApiWebService(m.handlerContainer, thirdPartyPrefix, []string{rsrc.Versions[0].Name}) + thirdPartyAPIPrefix := makeThirdPartyPath(group) + "/" + apiserver.AddApiWebService(m.handlerContainer, thirdPartyAPIPrefix, []string{rsrc.Versions[0].Name}) thirdPartyRequestInfoResolver := &apiserver.APIRequestInfoResolver{APIPrefixes: sets.NewString(strings.TrimPrefix(group, "/")), RestMapper: thirdparty.Mapper} apiserver.InstallServiceErrorHandler(m.handlerContainer, thirdPartyRequestInfoResolver, []string{thirdparty.Version}) return nil @@ -793,7 +793,7 @@ func (m *Master) InstallThirdPartyAPI(rsrc *experimental.ThirdPartyResource) err func (m *Master) thirdpartyapi(group, kind, version string) *apiserver.APIGroupVersion { resourceStorage := thirdpartyresourcedataetcd.NewREST(m.thirdPartyStorage, group, kind) - apiRoot := "/thirdparty/" + group + "/" + apiRoot := makeThirdPartyPath(group) + "/" storage := map[string]rest.Storage{ strings.ToLower(kind) + "s": resourceStorage, diff --git a/pkg/master/master_test.go b/pkg/master/master_test.go index 2e2a1c340aa..8671a4c927a 100644 --- a/pkg/master/master_test.go +++ b/pkg/master/master_test.go @@ -479,7 +479,7 @@ func testInstallThirdPartyAPIListVersion(t *testing.T, version string) { fakeClient.ExpectNotFoundGet(etcdtest.PathPrefix() + "/ThirdPartyResourceData/company.com/foos/default") - resp, err := http.Get(server.URL + "/thirdparty/company.com/" + version + "/namespaces/default/foos") + resp, err := http.Get(server.URL + "/apis/company.com/" + version + "/namespaces/default/foos") if !assert.NoError(err) { return } @@ -558,7 +558,7 @@ func testInstallThirdPartyAPIGetVersion(t *testing.T, version string) { return } - resp, err := http.Get(server.URL + "/thirdparty/company.com/" + version + "/namespaces/default/foos/test") + resp, err := http.Get(server.URL + "/apis/company.com/" + version + "/namespaces/default/foos/test") if !assert.NoError(err) { return } @@ -603,8 +603,9 @@ func testInstallThirdPartyAPIPostForVersion(t *testing.T, version string) { return } - resp, err := http.Post(server.URL+"/thirdparty/company.com/"+version+"/namespaces/default/foos", "application/json", bytes.NewBuffer(data)) + resp, err := http.Post(server.URL+"/apis/company.com/"+version+"/namespaces/default/foos", "application/json", bytes.NewBuffer(data)) if !assert.NoError(err) { + t.Errorf("unexpected error: %v", err) return } @@ -670,7 +671,7 @@ func testInstallThirdPartyAPIDeleteVersion(t *testing.T, version string) { return } - resp, err := http.Get(server.URL + "/thirdparty/company.com/" + version + "/namespaces/default/foos/test") + resp, err := http.Get(server.URL + "/apis/company.com/" + version + "/namespaces/default/foos/test") if !assert.NoError(err) { return } @@ -687,14 +688,14 @@ func testInstallThirdPartyAPIDeleteVersion(t *testing.T, version string) { t.Errorf("expected:\n%v\nsaw:\n%v\n", expectedObj, item) } - resp, err = httpDelete(server.URL + "/thirdparty/company.com/" + version + "/namespaces/default/foos/test") + resp, err = httpDelete(server.URL + "/apis/company.com/" + version + "/namespaces/default/foos/test") if !assert.NoError(err) { return } assert.Equal(http.StatusOK, resp.StatusCode) - resp, err = http.Get(server.URL + "/thirdparty/company.com/" + version + "/namespaces/default/foos/test") + resp, err = http.Get(server.URL + "/apis/company.com/" + version + "/namespaces/default/foos/test") if !assert.NoError(err) { return } diff --git a/pkg/master/thirdparty_controller.go b/pkg/master/thirdparty_controller.go index 6101b7f7c12..e1fe03100a6 100644 --- a/pkg/master/thirdparty_controller.go +++ b/pkg/master/thirdparty_controller.go @@ -21,7 +21,7 @@ import ( "strings" "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/expapi" + expapi "k8s.io/kubernetes/pkg/apis/experimental" "k8s.io/kubernetes/pkg/fields" "k8s.io/kubernetes/pkg/labels" thirdpartyresourceetcd "k8s.io/kubernetes/pkg/registry/thirdpartyresource/etcd" @@ -30,13 +30,13 @@ import ( "k8s.io/kubernetes/pkg/util/sets" ) -const thirdpartyprefix = "/thirdparty/" +const thirdpartyprefix = "/apis/" func makeThirdPartyPath(group string) string { return thirdpartyprefix + group } -// resourceInterface is the interface for the parts of the master than know how to add/remove +// resourceInterface is the interface for the parts of the master that know how to add/remove // third party resources. Extracted into an interface for injection for testing. type resourceInterface interface { // Remove a third party resource based on the RESTful path for that resource @@ -58,6 +58,9 @@ type ThirdPartyController struct { // Synchronize a single resource with RESTful resources on the master func (t *ThirdPartyController) SyncOneResource(rsrc *expapi.ThirdPartyResource) error { + // TODO: we also need to test if the existing installed resource matches the resource we are sync-ing. + // Currently, if there is an older, incompatible resource installed, we won't remove it. We should detect + // older, incompatible resources and remove them before testing if the resource exists. hasResource, err := t.master.HasThirdPartyResource(rsrc) if err != nil { return err @@ -105,7 +108,7 @@ func (t *ThirdPartyController) syncResourceList(list runtime.Object) error { found := false // search across the expected restful resources to see if this resource belongs to one of the expected ones for _, apiPath := range existing.List() { - if strings.HasPrefix(installedAPI, apiPath) { + if installedAPI == apiPath || strings.HasPrefix(installedAPI, apiPath+"/") { found = true break } diff --git a/pkg/master/thirdparty_controller_test.go b/pkg/master/thirdparty_controller_test.go index 920124f7f44..4fe37584065 100644 --- a/pkg/master/thirdparty_controller_test.go +++ b/pkg/master/thirdparty_controller_test.go @@ -20,7 +20,7 @@ import ( "testing" "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/expapi" + expapi "k8s.io/kubernetes/pkg/apis/experimental" "k8s.io/kubernetes/pkg/registry/thirdpartyresourcedata" "k8s.io/kubernetes/pkg/util/sets" ) @@ -94,11 +94,33 @@ func TestSyncAPIs(t *testing.T) { }, }, apis: []string{ - "/thirdparty/example.com", - "/thirdparty/example.com/v1", + "/apis/example.com", + "/apis/example.com/v1", }, name: "does nothing", }, + { + list: &expapi.ThirdPartyResourceList{ + Items: []expapi.ThirdPartyResource{ + { + ObjectMeta: api.ObjectMeta{ + Name: "foo.example.com", + }, + }, + }, + }, + apis: []string{ + "/apis/example.com", + "/apis/example.com/v1", + "/apis/example.co", + "/apis/example.co/v1", + }, + name: "deletes substring API", + expectedRemoved: []string{ + "/apis/example.co", + "/apis/example.co/v1", + }, + }, { list: &expapi.ThirdPartyResourceList{ Items: []expapi.ThirdPartyResource{ @@ -115,8 +137,8 @@ func TestSyncAPIs(t *testing.T) { }, }, apis: []string{ - "/thirdparty/company.com", - "/thirdparty/company.com/v1", + "/apis/company.com", + "/apis/company.com/v1", }, expectedInstalled: []string{"foo.example.com"}, name: "adds with existing", @@ -132,11 +154,11 @@ func TestSyncAPIs(t *testing.T) { }, }, apis: []string{ - "/thirdparty/company.com", - "/thirdparty/company.com/v1", + "/apis/company.com", + "/apis/company.com/v1", }, expectedInstalled: []string{"foo.example.com"}, - expectedRemoved: []string{"/thirdparty/company.com", "/thirdparty/company.com/v1"}, + expectedRemoved: []string{"/apis/company.com", "/apis/company.com/v1"}, name: "removes with existing", }, }