From 8859a4289d9226fe6cfb59d046fc24047f126472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9na=C3=AFc=20Huard?= Date: Fri, 8 Nov 2024 15:43:21 +0100 Subject: [PATCH 1/4] Fix a panic in GVRFromType for structured types --- pkg/util/utils.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 837a0fd56..c8bf14908 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -21,7 +21,7 @@ import ( "runtime" "strings" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" @@ -142,7 +142,11 @@ func GVRFromType(resourceName string, expectedType interface{}) *schema.GroupVer // testUnstructuredMock.Foo is a mock type for testing return nil } - apiVersion := expectedType.(*unstructured.Unstructured).Object["apiVersion"].(string) + t, err := meta.TypeAccessor(expectedType) + if err != nil { + panic(err) + } + apiVersion := t.GetAPIVersion() g, v, found := strings.Cut(apiVersion, "/") if !found { g = "core" From 4a96a782fcbfd0b26ee88ff832f79b0af118f667 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9na=C3=AFc=20Huard?= Date: Tue, 10 Dec 2024 17:49:40 +0100 Subject: [PATCH 2/4] Log an error instead of panicking --- pkg/util/utils.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/util/utils.go b/pkg/util/utils.go index c8bf14908..49eb6e219 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -144,7 +144,8 @@ func GVRFromType(resourceName string, expectedType interface{}) *schema.GroupVer } t, err := meta.TypeAccessor(expectedType) if err != nil { - panic(err) + klog.ErrorS(err, "Failed to get type accessor", "expectedType", expectedType) + return nil } apiVersion := t.GetAPIVersion() g, v, found := strings.Cut(apiVersion, "/") From 61134a08b649dcf33f1f54db6a9d6d3a4e52a067 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9na=C3=AFc=20Huard?= Date: Thu, 23 Jan 2025 15:55:11 +0100 Subject: [PATCH 3/4] Return an error instead of only logging --- internal/discovery/discovery.go | 11 ++++++++++- internal/store/builder.go | 10 ++++++++-- pkg/customresourcestate/config.go | 6 +++++- pkg/util/utils.go | 16 +++++++++------- 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index bd764ab70..b2df94437 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -216,7 +216,16 @@ func (r *CRDiscoverer) PollForCacheUpdates( // Update the list of enabled custom resources. var enabledCustomResources []string for _, factory := range customFactories { - gvrString := util.GVRFromType(factory.Name(), factory.ExpectedType()).String() + gvr, err := util.GVRFromType(factory.Name(), factory.ExpectedType()) + if err != nil { + klog.ErrorS(err, "failed to update custom resource stores") + } + var gvrString string + if gvr != nil { + gvrString = gvr.String() + } else { + gvrString = factory.Name() + } enabledCustomResources = append(enabledCustomResources, gvrString) } // Create clients for discovered factories. diff --git a/internal/store/builder.go b/internal/store/builder.go index 5c38857d7..2c3ebfe57 100644 --- a/internal/store/builder.go +++ b/internal/store/builder.go @@ -197,7 +197,10 @@ func (b *Builder) DefaultGenerateCustomResourceStoresFunc() ksmtypes.BuildCustom func (b *Builder) WithCustomResourceStoreFactories(fs ...customresource.RegistryFactory) { for i := range fs { f := fs[i] - gvr := util.GVRFromType(f.Name(), f.ExpectedType()) + gvr, err := util.GVRFromType(f.Name(), f.ExpectedType()) + if err != nil { + klog.ErrorS(err, "Failed to get GVR from type", "resourceName", f.Name(), "expectedType", f.ExpectedType()) + } var gvrString string if gvr != nil { gvrString = gvr.String() @@ -553,7 +556,10 @@ func (b *Builder) buildCustomResourceStores(resourceName string, familyHeaders := generator.ExtractMetricFamilyHeaders(metricFamilies) - gvr := util.GVRFromType(resourceName, expectedType) + gvr, err := util.GVRFromType(resourceName, expectedType) + if err != nil { + klog.ErrorS(err, "Failed to get GVR from type", "resourceName", resourceName, "expectedType", expectedType) + } var gvrString string if gvr != nil { gvrString = gvr.String() diff --git a/pkg/customresourcestate/config.go b/pkg/customresourcestate/config.go index 873591372..09b4d7f59 100644 --- a/pkg/customresourcestate/config.go +++ b/pkg/customresourcestate/config.go @@ -204,7 +204,11 @@ func FromConfig(decoder ConfigDecoder, discovererInstance *discovery.CRDiscovere if err != nil { return nil, fmt.Errorf("failed to create metrics factory for %s: %w", resource.GroupVersionKind, err) } - gvrString := util.GVRFromType(factory.Name(), factory.ExpectedType()).String() + gvr, err := util.GVRFromType(factory.Name(), factory.ExpectedType()) + if err != nil { + return nil, fmt.Errorf("failed to create GVR for %s: %w", resource.GroupVersionKind, err) + } + gvrString := gvr.String() if _, ok := factoriesIndex[gvrString]; ok { klog.InfoS("reloaded factory", "GVR", gvrString) } diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 49eb6e219..0bfbd1a60 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -97,8 +97,11 @@ func CreateCustomResourceClients(apiserver string, kubeconfig string, factories if err != nil { return nil, err } - gvrString := GVRFromType(f.Name(), f.ExpectedType()).String() - customResourceClients[gvrString] = customResourceClient + gvr, err := GVRFromType(f.Name(), f.ExpectedType()) + if err != nil { + return nil, err + } + customResourceClients[gvr.String()] = customResourceClient } return customResourceClients, nil } @@ -137,15 +140,14 @@ func CreateDynamicClient(apiserver string, kubeconfig string) (*dynamic.DynamicC } // GVRFromType returns the GroupVersionResource for a given type. -func GVRFromType(resourceName string, expectedType interface{}) *schema.GroupVersionResource { +func GVRFromType(resourceName string, expectedType interface{}) (*schema.GroupVersionResource, error) { if _, ok := expectedType.(*testUnstructuredMock.Foo); ok { // testUnstructuredMock.Foo is a mock type for testing - return nil + return nil, nil } t, err := meta.TypeAccessor(expectedType) if err != nil { - klog.ErrorS(err, "Failed to get type accessor", "expectedType", expectedType) - return nil + return nil, fmt.Errorf("Failed to get type accessor for %T: %w", expectedType, err) } apiVersion := t.GetAPIVersion() g, v, found := strings.Cut(apiVersion, "/") @@ -158,7 +160,7 @@ func GVRFromType(resourceName string, expectedType interface{}) *schema.GroupVer Group: g, Version: v, Resource: r, - } + }, nil } // GatherAndCount gathers all metrics from the provided Gatherer and counts From c3b1afe61c2a1b7335d4caf99f1ae228dfadbc18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9na=C3=AFc=20Huard?= Date: Tue, 28 Jan 2025 21:30:58 +0100 Subject: [PATCH 4/4] Handle the cases where GVRFromType returns neither a GVR nor an error --- pkg/customresourcestate/config.go | 7 ++++++- pkg/util/utils.go | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/customresourcestate/config.go b/pkg/customresourcestate/config.go index 09b4d7f59..f20eea20e 100644 --- a/pkg/customresourcestate/config.go +++ b/pkg/customresourcestate/config.go @@ -208,7 +208,12 @@ func FromConfig(decoder ConfigDecoder, discovererInstance *discovery.CRDiscovere if err != nil { return nil, fmt.Errorf("failed to create GVR for %s: %w", resource.GroupVersionKind, err) } - gvrString := gvr.String() + var gvrString string + if gvr != nil { + gvrString = gvr.String() + } else { + gvrString = factory.Name() + } if _, ok := factoriesIndex[gvrString]; ok { klog.InfoS("reloaded factory", "GVR", gvrString) } diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 0bfbd1a60..4af1d0442 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -101,7 +101,13 @@ func CreateCustomResourceClients(apiserver string, kubeconfig string, factories if err != nil { return nil, err } - customResourceClients[gvr.String()] = customResourceClient + var gvrString string + if gvr != nil { + gvrString = gvr.String() + } else { + gvrString = f.Name() + } + customResourceClients[gvrString] = customResourceClient } return customResourceClients, nil }