github/client-go
1
0
Fork 0
mirror of https://github.com/kubernetes/client-go.git synced 2025-07-19 09:38:39 +00:00
Code Issues Projects Releases Wiki Activity

Add shortname ambiguity warning in shortcut expander (#117668)

Browse Source 
* Add warning handler callback function in shortcut expander

Currently, errors in client-go are propagated back to the callers via
function returns. However, there is no elegant way for just warning users.
For example, when user wants to get a resource with it's short name format
and if there are multiple resources belonging to this short name, we need to
warn user about this ambugity which one is picked and which ones are discarded.

Not only to overcome this particular case mentioned above, but also propose a
way for the possible warnings in the future, this commit adds a warningHandler
callback function in shortcutExpander.

* Add warningPrinter functionality in ConfigFlags

ConfigFlags has neither warning user in a standardized
format functionality nor passing warning callback functions to other upper level
libraries such as client-go.

This commit adds an ability that user can set warningPrinters
according to their IOStreams and this warningPrinters will be used
to raise possible warnings happening not only in cli-runtime but
also in client-go.

* Pass warning callback function in ConfigFlags to shortcutExpander

This commit passes warning callback function to print possible
warnings happened in shortcut expander to warn user in a
standardized format.

* Add integration test for CRDs having ambiguous short names

This commit adds integration test to assure that warning message
related to this ambiguity is printed when resources are being retrieved via their short name
representations in cases where multiple resources have same
short names.

This integration test also ensures that the logic behind which resource
will be selected hasn't been changed which may cause disperancies in
clusters.

* Remove defaultConfigFlag global variable

* Move default config flags initialization into function

* Skip warning for versions of same group/resource

* Run update-vendor

* Warn only once when there are multiple versions registered for ambiguous resource

* Apply gocritic review

* Add multi-resource multi-version ambiguity unit test

Kubernetes-commit: a504aed54d028dbc8ea2508142c94d309f5f1ec6
This commit is contained in:
Arda Güçlü 2023-10-11 18:04:11 +03:00 committed by Kubernetes Publisher
parent cf4002bbbf
commit 57d597c1d6
2 changed files with 231 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
restmapper/shortcut.go
View File

@ -17,6 +17,7 @@ limitations under the License.
package restmapper
import (
"fmt"
"strings"
"k8s.io/klog/v2"
@ -32,13 +33,15 @@ type shortcutExpander struct {
RESTMapper meta.RESTMapper
discoveryClient discovery.DiscoveryInterface
warningHandler func(string)
}
var _ meta.ResettableRESTMapper = shortcutExpander{}
// NewShortcutExpander wraps a restmapper in a layer that expands shortcuts found via discovery
func NewShortcutExpander(delegate meta.RESTMapper, client discovery.DiscoveryInterface) meta.RESTMapper {
return shortcutExpander{RESTMapper: delegate, discoveryClient: client}
func NewShortcutExpander(delegate meta.RESTMapper, client discovery.DiscoveryInterface, warningHandler func(string)) meta.RESTMapper {
return shortcutExpander{RESTMapper: delegate, discoveryClient: client, warningHandler: warningHandler}
}
// KindFor fulfills meta.RESTMapper
@ -145,16 +148,37 @@ func (e shortcutExpander) expandResourceShortcut(resource schema.GroupVersionRes
}
}
found := false
var rsc schema.GroupVersionResource
warnedAmbiguousShortcut := make(map[schema.GroupResource]bool)
for _, item := range shortcutResources {
if len(resource.Group) != 0 && resource.Group != item.ShortForm.Group {
continue
}
if resource.Resource == item.ShortForm.Resource {
resource.Resource = item.LongForm.Resource
resource.Group = item.LongForm.Group
return resource
if found {
if item.LongForm.Group == rsc.Group && item.LongForm.Resource == rsc.Resource {
// It is common and acceptable that group/resource has multiple
// versions registered in cluster. This does not introduce ambiguity
// in terms of shortname usage.
continue
}
if !warnedAmbiguousShortcut[item.LongForm] {
if e.warningHandler != nil {
e.warningHandler(fmt.Sprintf("short name %q could also match lower priority resource %s", resource.Resource, item.LongForm.String()))
}
warnedAmbiguousShortcut[item.LongForm] = true
}
continue
}
rsc.Resource = item.LongForm.Resource
rsc.Group = item.LongForm.Group
found = true
}
}
if found {
return rsc
}
// we didn't find exact match so match on group prefixing. This allows autoscal to match autoscaling
if len(resource.Group) == 0 {

205
restmapper/shortcut_test.go
View File

@ -133,7 +133,7 @@ func TestReplaceAliases(t *testing.T) {
ds.serverResourcesHandler = func() ([]*metav1.APIResourceList, error) {
return test.srvRes, nil
}
mapper := NewShortcutExpander(&fakeRESTMapper{}, ds).(shortcutExpander)
mapper := NewShortcutExpander(&fakeRESTMapper{}, ds, nil).(shortcutExpander)
actual := mapper.expandResourceShortcut(schema.GroupVersionResource{Resource: test.arg})
if actual != test.expected {
@ -187,7 +187,9 @@ func TestKindFor(t *testing.T) {
}
delegate := &fakeRESTMapper{}
mapper := NewShortcutExpander(delegate, ds)
mapper := NewShortcutExpander(delegate, ds, func(a string) {
t.Fatalf("unexpected warning message %s", a)
})
mapper.KindFor(test.in)
if delegate.kindForInput != test.expected {
@ -242,7 +244,9 @@ func TestKindForWithNewCRDs(t *testing.T) {
// will answer the initial request, only failure to match will trigger
// the cache invalidation and live discovery call
delegate := NewDeferredDiscoveryRESTMapper(fakeCachedDiscovery)
mapper := NewShortcutExpander(delegate, fakeCachedDiscovery)
mapper := NewShortcutExpander(delegate, fakeCachedDiscovery, func(a string) {
t.Fatalf("unexpected warning message %s", a)
})
gvk, err := mapper.KindFor(test.in)
if err != nil {
@ -255,6 +259,201 @@ func TestKindForWithNewCRDs(t *testing.T) {
}
}
func TestWarnAmbigious(t *testing.T) {
tests := []struct {
name string
arg string
expected schema.GroupVersionResource
expectedWarningLogs []string
srvRes []*metav1.APIResourceList
}{
{
name: "warn ambiguity",
arg: "hpa",
expected: schema.GroupVersionResource{Resource: "superhorizontalpodautoscalers", Group: "autoscaling"},
expectedWarningLogs: []string{`short name "hpa" could also match lower priority resource horizontalpodautoscalers.autoscaling`},
srvRes: []*metav1.APIResourceList{
{
GroupVersion: "autoscaling/v1",
APIResources: []metav1.APIResource{
{
Name: "superhorizontalpodautoscalers",
ShortNames: []string{"hpa"},
},
},
},
{
GroupVersion: "autoscaling/v1",
APIResources: []metav1.APIResource{
{
Name: "horizontalpodautoscalers",
ShortNames: []string{"hpa"},
},
},
},
},
},
{
name: "warn-builtin-shortname-ambugity",
arg: "po",
expected: schema.GroupVersionResource{Resource: "pods", Group: ""},
expectedWarningLogs: []string{`short name "po" could also match lower priority resource poddlers.acme.com`},
srvRes: []*metav1.APIResourceList{
{
GroupVersion: "v1",
APIResources: []metav1.APIResource{{Name: "pods", SingularName: "pod", ShortNames: []string{"po"}}},
},
{
GroupVersion: "acme.com/v1",
APIResources: []metav1.APIResource{{Name: "poddlers", ShortNames: []string{"po"}}},
},
},
},
{
name: "warn-builtin-shortname-ambugity-multi-version",
arg: "po",
expected: schema.GroupVersionResource{Resource: "pods", Group: ""},
expectedWarningLogs: []string{`short name "po" could also match lower priority resource poddlers.acme.com`},
srvRes: []*metav1.APIResourceList{
{
GroupVersion: "v1",
APIResources: []metav1.APIResource{{Name: "pods", SingularName: "pod", ShortNames: []string{"po"}}},
},
{
GroupVersion: "acme.com/v1",
APIResources: []metav1.APIResource{{Name: "poddlers", ShortNames: []string{"po"}}},
},
{
GroupVersion: "acme.com/v1beta1",
APIResources: []metav1.APIResource{{Name: "poddlers", ShortNames: []string{"po"}}},
},
},
},
{
name: "resource-match-singular-preferred",
arg: "pod",
expected: schema.GroupVersionResource{Resource: "pod", Group: ""},
srvRes: []*metav1.APIResourceList{
{
GroupVersion: "v1",
APIResources: []metav1.APIResource{{Name: "pods", SingularName: "pod"}},
},
{
GroupVersion: "acme.com/v1",
APIResources: []metav1.APIResource{{Name: "poddlers", ShortNames: []string{"pods", "pod"}}},
},
},
},
{
name: "resource-multiple-versions-shortform",
arg: "hpa",
expected: schema.GroupVersionResource{Resource: "horizontalpodautoscalers", Group: "autoscaling"},
expectedWarningLogs: []string{},
srvRes: []*metav1.APIResourceList{
{
GroupVersion: "autoscaling/v1alphav1",
APIResources: []metav1.APIResource{
{
Name: "horizontalpodautoscalers",
ShortNames: []string{"hpa"},
},
},
},
{
GroupVersion: "autoscaling/v1",
APIResources: []metav1.APIResource{
{
Name: "horizontalpodautoscalers",
ShortNames: []string{"hpa"},
},
},
},
},
},
{
name: "multi-resource-multiple-versions-shortform",
arg: "hpa",
expected: schema.GroupVersionResource{Resource: "horizontalpodautoscalers", Group: "autoscaling"},
expectedWarningLogs: []string{
`short name "hpa" could also match lower priority resource foo.foo`,
`short name "hpa" could also match lower priority resource bar.bar`,
},
srvRes: []*metav1.APIResourceList{
{
GroupVersion: "autoscaling/v1alphav1",
APIResources: []metav1.APIResource{
{
Name: "horizontalpodautoscalers",
ShortNames: []string{"hpa"},
},
},
},
{
GroupVersion: "autoscaling/v1",
APIResources: []metav1.APIResource{
{
Name: "horizontalpodautoscalers",
ShortNames: []string{"hpa"},
},
},
},
{
GroupVersion: "foo/v1",
APIResources: []metav1.APIResource{
{
Name: "foo",
ShortNames: []string{"hpa"},
},
},
},
{
GroupVersion: "foo/v1beta1",
APIResources: []metav1.APIResource{
{
Name: "foo",
ShortNames: []string{"hpa"},
},
},
},
{
GroupVersion: "bar/v1",
APIResources: []metav1.APIResource{
{
Name: "bar",
ShortNames: []string{"hpa"},
},
},
},
},
},
}
for _, test := range tests {
ds := &fakeDiscoveryClient{}
ds.serverResourcesHandler = func() ([]*metav1.APIResourceList, error) {
return test.srvRes, nil
}
var actualWarnings []string
mapper := NewShortcutExpander(&fakeRESTMapper{}, ds, func(a string) {
actualWarnings = append(actualWarnings, a)
}).(shortcutExpander)
actual := mapper.expandResourceShortcut(schema.GroupVersionResource{Resource: test.arg})
if actual != test.expected {
t.Errorf("%s: unexpected argument: expected %s, got %s", test.name, test.expected, actual)
}
if len(actualWarnings) == 0 && len(test.expectedWarningLogs) == 0 {
continue
}
if !cmp.Equal(test.expectedWarningLogs, actualWarnings) {
t.Fatalf("expected warning message %s but got %s", test.expectedWarningLogs, actualWarnings)
}
}
}
type fakeRESTMapper struct {
kindForInput schema.GroupVersionResource
}