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.

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

Category expansion fully based on discovery #51829

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 11 additions & 6 deletions pkg/kubectl/cmd/util/factory_object_mapping.go
Expand Up @@ -119,17 +119,22 @@ func (f *ring1Factory) UnstructuredObject() (meta.RESTMapper, runtime.ObjectType
}

func (f *ring1Factory) CategoryExpander() resource.CategoryExpander {
var categoryExpander resource.CategoryExpander
categoryExpander = resource.LegacyCategoryExpander
legacyExpander := resource.LegacyCategoryExpander

discoveryClient, err := f.clientAccessFactory.DiscoveryClient()
if err == nil {
// wrap with discovery based filtering
categoryExpander, err = resource.NewDiscoveryFilteredExpander(categoryExpander, discoveryClient)
// you only have an error on missing discoveryClient, so this shouldn't fail. Check anyway.
// fallback is the legacy expander wrapped with discovery based filtering
fallbackExpander, err := resource.NewDiscoveryFilteredExpander(legacyExpander, discoveryClient)
CheckErr(err)

// by default use the expander that discovers based on "categories" field from the API
discoveryCategoryExpander, err := resource.NewDiscoveryCategoryExpander(fallbackExpander, discoveryClient)
CheckErr(err)

return discoveryCategoryExpander
}

return categoryExpander
return legacyExpander
}

func (f *ring1Factory) ClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) {
Expand Down
5 changes: 5 additions & 0 deletions pkg/kubectl/resource/BUILD
Expand Up @@ -60,7 +60,9 @@ go_test(
"//pkg/api:go_default_library",
"//pkg/api/testapi:go_default_library",
"//pkg/api/testing:go_default_library",
"//vendor/github.com/emicklei/go-restful-swagger12:go_default_library",
"//vendor/github.com/ghodss/yaml:go_default_library",
"//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
Expand All @@ -72,7 +74,10 @@ go_test(
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/serializer/streaming:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/version:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/watch:go_default_library",
"//vendor/k8s.io/client-go/discovery:go_default_library",
"//vendor/k8s.io/client-go/rest:go_default_library",
"//vendor/k8s.io/client-go/rest/fake:go_default_library",
"//vendor/k8s.io/client-go/rest/watch:go_default_library",
"//vendor/k8s.io/client-go/util/testing:go_default_library",
Expand Down
56 changes: 53 additions & 3 deletions pkg/kubectl/resource/categories.go
Expand Up @@ -17,8 +17,6 @@ limitations under the License.
package resource

import (
"errors"

"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/discovery"
)
Expand All @@ -36,6 +34,58 @@ func (e SimpleCategoryExpander) Expand(category string) ([]schema.GroupResource,
return ret, ok
}

type discoveryCategoryExpander struct {
fallbackExpander CategoryExpander
discoveryClient discovery.DiscoveryInterface
}

// NewDiscoveryCategoryExpander returns a category expander that makes use of the "categories" fields from
// the API, found through the discovery client. In case of any error or no category found (which likely
// means we're at a cluster prior to categories support, fallback to the expander provided.
func NewDiscoveryCategoryExpander(fallbackExpander CategoryExpander, client discovery.DiscoveryInterface) (discoveryCategoryExpander, error) {
if client == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's coder error, why not panic?

panic("Please provide discovery client to shortcut expander")
}
return discoveryCategoryExpander{fallbackExpander: fallbackExpander, discoveryClient: client}, nil
}

func (e discoveryCategoryExpander) Expand(category string) ([]schema.GroupResource, bool) {
apiResourceLists, _ := e.discoveryClient.ServerResources()
if len(apiResourceLists) == 0 {
return e.fallbackExpander.Expand(category)
}

discoveredExpansions := map[string][]schema.GroupResource{}

for _, apiResourceList := range apiResourceLists {
gv, err := schema.ParseGroupVersion(apiResourceList.GroupVersion)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

this particular error is bad, I would return an error instead of falling back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k should we change the CategoryExpander interface signature? No error returned in Expand ATM.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k should we change the CategoryExpander interface signature? No error returned in Expand ATM.

hmmm... ok. I can deal with a fallback for 1.8 to avoid too many ripples at this stage.

return e.fallbackExpander.Expand(category)
}

for _, apiResource := range apiResourceList.APIResources {
if categories := apiResource.Categories; len(categories) > 0 {
for _, category := range categories {
groupResource := schema.GroupResource{
Group: gv.Group,
Resource: apiResource.Name,
}
discoveredExpansions[category] = append(discoveredExpansions[category], groupResource)
}
}
}
}

if len(discoveredExpansions) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good.

// We don't know if the server really don't have any resource with categories,
// or we're on a cluster version prior to categories support. Anyways, fallback.
return e.fallbackExpander.Expand(category)
}

ret, ok := discoveredExpansions[category]
return ret, ok
}

type discoveryFilteredExpander struct {
delegate CategoryExpander

Expand All @@ -46,7 +96,7 @@ type discoveryFilteredExpander struct {
// what the server has available
func NewDiscoveryFilteredExpander(delegate CategoryExpander, client discovery.DiscoveryInterface) (discoveryFilteredExpander, error) {
if client == nil {
return discoveryFilteredExpander{}, errors.New("Please provide discovery client to shortcut expander")
panic("Please provide discovery client to shortcut expander")
}
return discoveryFilteredExpander{delegate: delegate, discoveryClient: client}, nil
}
Expand Down
125 changes: 125 additions & 0 deletions pkg/kubectl/resource/categories_test.go
Expand Up @@ -20,7 +20,15 @@ import (
"reflect"
"testing"

swagger "github.com/emicklei/go-restful-swagger12"

"github.com/googleapis/gnostic/OpenAPIv2"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/version"
"k8s.io/client-go/discovery"
restclient "k8s.io/client-go/rest"
"k8s.io/client-go/rest/fake"
)

func TestCategoryExpansion(t *testing.T) {
Expand Down Expand Up @@ -65,3 +73,120 @@ func TestCategoryExpansion(t *testing.T) {
}
}
}

func TestDiscoveryCategoryExpander(t *testing.T) {
tests := []struct {
category string
serverResponse []*metav1.APIResourceList
expected []schema.GroupResource
}{
{
category: "all",
serverResponse: []*metav1.APIResourceList{
{
GroupVersion: "batch/v1",
APIResources: []metav1.APIResource{
{
Name: "jobs",
ShortNames: []string{"jz"},
Categories: []string{"all"},
},
},
},
},
expected: []schema.GroupResource{
{
Group: "batch",
Resource: "jobs",
},
},
},
{
category: "all",
serverResponse: []*metav1.APIResourceList{
{
GroupVersion: "batch/v1",
APIResources: []metav1.APIResource{
{
Name: "jobs",
ShortNames: []string{"jz"},
},
},
},
},
},
{
category: "targaryens",
serverResponse: []*metav1.APIResourceList{
{
GroupVersion: "batch/v1",
APIResources: []metav1.APIResource{
{
Name: "jobs",
ShortNames: []string{"jz"},
Categories: []string{"all"},
},
},
},
},
},
}

dc := &fakeDiscoveryClient{}
for _, test := range tests {
dc.serverResourcesHandler = func() ([]*metav1.APIResourceList, error) {
return test.serverResponse, nil
}
expander, err := NewDiscoveryCategoryExpander(SimpleCategoryExpander{}, dc)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
expanded, _ := expander.Expand(test.category)
if !reflect.DeepEqual(expanded, test.expected) {
t.Errorf("expected %v, got %v", test.expected, expanded)
}
}

}

type fakeDiscoveryClient struct {
serverResourcesHandler func() ([]*metav1.APIResourceList, error)
}

var _ discovery.DiscoveryInterface = &fakeDiscoveryClient{}

func (c *fakeDiscoveryClient) RESTClient() restclient.Interface {
return &fake.RESTClient{}
}

func (c *fakeDiscoveryClient) ServerGroups() (*metav1.APIGroupList, error) {
return nil, nil
}

func (c *fakeDiscoveryClient) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) {
return &metav1.APIResourceList{}, nil
}

func (c *fakeDiscoveryClient) ServerResources() ([]*metav1.APIResourceList, error) {
return c.serverResourcesHandler()
}

func (c *fakeDiscoveryClient) ServerPreferredResources() ([]*metav1.APIResourceList, error) {
return nil, nil
}

func (c *fakeDiscoveryClient) ServerPreferredNamespacedResources() ([]*metav1.APIResourceList, error) {
return nil, nil
}

func (c *fakeDiscoveryClient) ServerVersion() (*version.Info, error) {
return &version.Info{}, nil
}

func (c *fakeDiscoveryClient) SwaggerSchema(version schema.GroupVersion) (*swagger.ApiDeclaration, error) {
return &swagger.ApiDeclaration{}, nil
}

func (c *fakeDiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) {
return &openapi_v2.Document{}, nil
}