Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a bug with pluralization of third party resources #25374

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Fix a bug with pluralization of third party resources #25374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions pkg/master/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"time"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/meta"
"k8s.io/kubernetes/pkg/api/rest"
"k8s.io/kubernetes/pkg/api/unversioned"
apiv1 "k8s.io/kubernetes/pkg/api/v1"
Expand Down Expand Up @@ -655,7 +656,13 @@ func (m *Master) InstallThirdPartyResource(rsrc *extensions.ThirdPartyResource)
if err != nil {
return err
}
thirdparty := m.thirdpartyapi(group, kind, rsrc.Versions[0].Name)
plural, _ := meta.KindToResource(unversioned.GroupVersionKind{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking: that second parameter is not an error, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second parameter is the 'singular' version of the resource.

Group: group,
Version: rsrc.Versions[0].Name,
Kind: kind,
})

thirdparty := m.thirdpartyapi(group, kind, rsrc.Versions[0].Name, plural.Resource)
if err := thirdparty.InstallREST(m.HandlerContainer); err != nil {
glog.Fatalf("Unable to setup thirdparty api: %v", err)
}
Expand All @@ -670,19 +677,20 @@ func (m *Master) InstallThirdPartyResource(rsrc *extensions.ThirdPartyResource)
PreferredVersion: groupVersion,
}
apiserver.AddGroupWebService(api.Codecs, m.HandlerContainer, path, apiGroup)
m.addThirdPartyResourceStorage(path, thirdparty.Storage[strings.ToLower(kind)+"s"].(*thirdpartyresourcedataetcd.REST), apiGroup)

m.addThirdPartyResourceStorage(path, thirdparty.Storage[plural.Resource].(*thirdpartyresourcedataetcd.REST), apiGroup)
apiserver.InstallServiceErrorHandler(api.Codecs, m.HandlerContainer, m.NewRequestInfoResolver(), []string{thirdparty.GroupVersion.String()})
return nil
}

func (m *Master) thirdpartyapi(group, kind, version string) *apiserver.APIGroupVersion {
func (m *Master) thirdpartyapi(group, kind, version, pluralResource string) *apiserver.APIGroupVersion {
resourceStorage := thirdpartyresourcedataetcd.NewREST(
generic.RESTOptions{Storage: m.thirdPartyStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: m.deleteCollectionWorkers}, group, kind)

apiRoot := makeThirdPartyPath("")

storage := map[string]rest.Storage{
strings.ToLower(kind) + "s": resourceStorage,
pluralResource: resourceStorage,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're relying on meta.KindToResource's pluralization code working for arbitrary strings? I guess that's an improvement to just appending an 's'. Should we instead allow them to put a pluralized version in the specification? @smarterclayton here's another great reason not to use plurals in our paths...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KindToResource is what the kubectl client code uses afaik, so at least its consistent?

}

optionsExternalVersion := registered.GroupOrDie(api.GroupName).GroupVersion
Expand Down
69 changes: 49 additions & 20 deletions pkg/master/master_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"testing"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/meta"
"k8s.io/kubernetes/pkg/api/testapi"
"k8s.io/kubernetes/pkg/api/unversioned"
apiv1 "k8s.io/kubernetes/pkg/api/v1"
Expand All @@ -48,6 +49,7 @@ import (
"k8s.io/kubernetes/pkg/registry/endpoint"
"k8s.io/kubernetes/pkg/registry/namespace"
"k8s.io/kubernetes/pkg/registry/registrytest"
"k8s.io/kubernetes/pkg/registry/thirdpartyresourcedata"
"k8s.io/kubernetes/pkg/runtime"
"k8s.io/kubernetes/pkg/storage"
"k8s.io/kubernetes/pkg/storage/etcd/etcdtest"
Expand Down Expand Up @@ -519,12 +521,12 @@ type FooList struct {
Items []Foo `json:"items"`
}

func initThirdParty(t *testing.T, version string) (*Master, *etcdtesting.EtcdTestServer, *httptest.Server, *assert.Assertions) {
func initThirdParty(t *testing.T, version, name string) (*Master, *etcdtesting.EtcdTestServer, *httptest.Server, *assert.Assertions) {
master, etcdserver, _, assert := newMaster(t)

api := &extensions.ThirdPartyResource{
ObjectMeta: api.ObjectMeta{
Name: "foo.company.com",
Name: name,
},
Versions: []extensions.APIVersion{
{
Expand All @@ -551,10 +553,22 @@ func TestInstallThirdPartyAPIList(t *testing.T) {
func testInstallThirdPartyAPIListVersion(t *testing.T, version string) {
tests := []struct {
items []Foo
name string
test string
}{
{},
{
name: "foo.company.com",
test: "null",
},
{
items: []Foo{},
name: "foo.company.com",
test: "empty",
},
{
items: []Foo{},
name: "policy.company.com",
test: "plurals",
},
{
items: []Foo{
Expand All @@ -581,46 +595,61 @@ func testInstallThirdPartyAPIListVersion(t *testing.T, version string) {
OtherField: 20,
},
},
name: "foo.company.com",
test: "real list",
},
}
for _, test := range tests {
func() {
master, etcdserver, server, assert := initThirdParty(t, version)
master, etcdserver, server, assert := initThirdParty(t, version, test.name)
defer server.Close()
defer etcdserver.Terminate(t)

kind, group, err := thirdpartyresourcedata.ExtractApiGroupAndKind(
&extensions.ThirdPartyResource{ObjectMeta: api.ObjectMeta{Name: test.name}})
assert.NoError(err, test.test)

plural, _ := meta.KindToResource(unversioned.GroupVersionKind{
Group: group,
Version: version,
Kind: kind,
})

if test.items != nil {
err := createThirdPartyList(master.thirdPartyStorage, "/ThirdPartyResourceData/company.com/foos/default", test.items)
if !assert.NoError(err) {
err := createThirdPartyList(master.thirdPartyStorage,
fmt.Sprintf("/ThirdPartyResourceData/%s/%s/default", group, plural.Resource),
test.items)
if !assert.NoError(err, test.test) {
return
}
}

resp, err := http.Get(server.URL + "/apis/company.com/" + version + "/namespaces/default/foos")
if !assert.NoError(err) {
resp, err := http.Get(
fmt.Sprintf("%s/apis/%s/%s/namespaces/default/%s", server.URL, group, version, plural.Resource))
if !assert.NoError(err, test.test) {
return
}
defer resp.Body.Close()

assert.Equal(http.StatusOK, resp.StatusCode)
assert.Equal(http.StatusOK, resp.StatusCode, test.test)

data, err := ioutil.ReadAll(resp.Body)
assert.NoError(err)
assert.NoError(err, test.test)

list := FooList{}
if err = json.Unmarshal(data, &list); err != nil {
t.Errorf("unexpected error: %v", err)
assert.NoError(err, "unexpected error: %v %s", err, test.test)
}

if test.items == nil {
if len(list.Items) != 0 {
t.Errorf("expected no items, saw: %v", list.Items)
assert.NoError(err, "expected no items, saw: %v %s", err, list.Items, test.test)
}
return
}

if len(list.Items) != len(test.items) {
t.Fatalf("unexpected length: %d vs %d", len(list.Items), len(test.items))
t.Fatalf("(%s) unexpected length: %d vs %d", test.name, len(list.Items), len(test.items))
}
// The order of elements in LIST is not guaranteed.
mapping := make(map[string]int)
Expand All @@ -639,7 +668,7 @@ func testInstallThirdPartyAPIListVersion(t *testing.T, version string) {
// We endure the order of items by sorting them (using 'mapping')
// so that this function passes.
if !reflect.DeepEqual(list.Items[ix], expectedObj) {
t.Errorf("expected:\n%#v\nsaw:\n%#v\n", expectedObj, list.Items[ix])
t.Errorf("(%s) expected:\n%#v\nsaw:\n%#v\n", test.name, expectedObj, list.Items[ix])
}
}
}()
Expand Down Expand Up @@ -695,7 +724,7 @@ func TestInstallThirdPartyAPIGet(t *testing.T) {
}

func testInstallThirdPartyAPIGetVersion(t *testing.T, version string) {
master, etcdserver, server, assert := initThirdParty(t, version)
master, etcdserver, server, assert := initThirdParty(t, version, "foo.company.com")
defer server.Close()
defer etcdserver.Terminate(t)

Expand Down Expand Up @@ -742,7 +771,7 @@ func TestInstallThirdPartyAPIPost(t *testing.T) {
}

func testInstallThirdPartyAPIPostForVersion(t *testing.T, version string) {
master, etcdserver, server, assert := initThirdParty(t, version)
master, etcdserver, server, assert := initThirdParty(t, version, "foo.company.com")
defer server.Close()
defer etcdserver.Terminate(t)

Expand Down Expand Up @@ -806,7 +835,7 @@ func TestInstallThirdPartyAPIDelete(t *testing.T) {
}

func testInstallThirdPartyAPIDeleteVersion(t *testing.T, version string) {
master, etcdserver, server, assert := initThirdParty(t, version)
master, etcdserver, server, assert := initThirdParty(t, version, "foo.company.com")
defer server.Close()
defer etcdserver.Terminate(t)

Expand Down Expand Up @@ -883,7 +912,7 @@ func TestInstallThirdPartyAPIListOptions(t *testing.T) {
}

func testInstallThirdPartyAPIListOptionsForVersion(t *testing.T, version string) {
_, etcdserver, server, assert := initThirdParty(t, version)
_, etcdserver, server, assert := initThirdParty(t, version, "foo.company.com")
defer server.Close()
defer etcdserver.Terminate(t)

Expand Down Expand Up @@ -915,7 +944,7 @@ func TestInstallThirdPartyResourceRemove(t *testing.T) {
}

func testInstallThirdPartyResourceRemove(t *testing.T, version string) {
master, etcdserver, server, assert := initThirdParty(t, version)
master, etcdserver, server, assert := initThirdParty(t, version, "foo.company.com")
defer server.Close()
defer etcdserver.Terminate(t)

Expand Down Expand Up @@ -1036,7 +1065,7 @@ func TestIsTunnelSyncHealthy(t *testing.T) {
}

func testThirdPartyDiscovery(t *testing.T, version string) {
_, etcdserver, server, assert := initThirdParty(t, version)
_, etcdserver, server, assert := initThirdParty(t, version, "foo.company.com")
defer server.Close()
defer etcdserver.Terminate(t)

Expand Down