From bd2ea7a6f8d8a330dd02c197d67185203c25955d Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sat, 21 Jan 2023 04:03:47 +0530 Subject: [PATCH 1/2] Handle unit length `valueFrom` values Handle unit length `valueFrom` values and skip strings where we expect them to be type-cast-able to `float64`, instead of erroring, since that is the expected behavior, and what's being done for other types. --- pkg/customresourcestate/registry_factory.go | 56 +++++++++++++++++-- .../registry_factory_test.go | 11 ++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/pkg/customresourcestate/registry_factory.go b/pkg/customresourcestate/registry_factory.go index 7e9eca535c..b477856d62 100644 --- a/pkg/customresourcestate/registry_factory.go +++ b/pkg/customresourcestate/registry_factory.go @@ -143,7 +143,7 @@ type compiledMetric interface { Type() metric.Type } -// newCompiledMetric returns a compiledMetric depending given the metric type. +// newCompiledMetric returns a compiledMetric depending on given the metric type. func newCompiledMetric(m Metric) (compiledMetric, error) { switch m.Type { case MetricTypeGauge: @@ -217,7 +217,40 @@ func (c *compiledGauge) Values(v interface{}) (result []eachValue, errs []error) switch iter := v.(type) { case map[string]interface{}: for key, it := range iter { - ev, err := c.value(it) + // Try to deduce `valueFrom`'s value from the current element. + var ev *eachValue + var err error + var didResolveValueFrom bool + // `valueFrom` will ultimately be rendered into a string and sent to the fallback in place, which also expects a string. + // So we'll do the same and operate on the string representation of `valueFrom`'s value. + sValueFrom := c.ValueFrom.String() + // No comma means we're looking at a unit-length path (in an array). + if !strings.Contains(sValueFrom, ",") && + sValueFrom[0] == '[' && + // "[...]" and not "[]". + len(sValueFrom) > 2 { + extractedValueFrom := sValueFrom[1 : len(sValueFrom)-1] + if key == extractedValueFrom { + gotFloat, err := toFloat64(it, c.NilIsZero) + if err != nil { + onError(fmt.Errorf("[%s]: %w", key, err)) + continue + } + labels := make(map[string]string) + ev = &eachValue{ + Labels: labels, + Value: gotFloat, + } + didResolveValueFrom = true + } + } + // Fallback to the regular path resolution, if we didn't manage to resolve `valueFrom`'s value. + if !didResolveValueFrom { + ev, err = c.value(it) + if ev == nil { + continue + } + } if err != nil { onError(fmt.Errorf("[%s]: %w", key, err)) continue @@ -387,7 +420,19 @@ func less(a, b map[string]string) bool { func (c compiledGauge) value(it interface{}) (*eachValue, error) { labels := make(map[string]string) - value, err := toFloat64(c.ValueFrom.Get(it), c.NilIsZero) + got := c.ValueFrom.Get(it) + // If `valueFrom` was not resolved, respect `NilIsZero` and return. + if got == nil { + if c.NilIsZero { + return &eachValue{ + Labels: labels, + Value: 0, + }, nil + } + // Don't error if there was not a type-casting issue (`toFloat64`), but rather a failed lookup. + return nil, nil + } + value, err := toFloat64(got, c.NilIsZero) if err != nil { return nil, fmt.Errorf("%s: %w", c.ValueFrom, err) } @@ -554,7 +599,10 @@ func compilePath(path []string) (out valuePath, _ error) { } else if s, ok := m.([]interface{}); ok { i, err := strconv.Atoi(part) if err != nil { - return fmt.Errorf("invalid list index: %s", part) + // This means we are here: [ , , ... ] (eg., [ "foo", "0", ... ], i.e., .foo[0]... + // ^ + // Skip over. + return nil } if i < 0 { // negative index diff --git a/pkg/customresourcestate/registry_factory_test.go b/pkg/customresourcestate/registry_factory_test.go index 1b42a57029..008fd543c8 100644 --- a/pkg/customresourcestate/registry_factory_test.go +++ b/pkg/customresourcestate/registry_factory_test.go @@ -173,6 +173,17 @@ func Test_values(t *testing.T) { newEachValue(t, 2, "type", "type-a", "active", "1"), newEachValue(t, 4, "type", "type-b", "active", "3"), }}, + {name: "path-relative valueFrom value", each: &compiledGauge{ + compiledCommon: compiledCommon{ + path: mustCompilePath(t, "metadata"), + labelFromPath: map[string]valuePath{ + "name": mustCompilePath(t, "name"), + }, + }, + ValueFrom: mustCompilePath(t, "creationTimestamp"), + }, wantResult: []eachValue{ + newEachValue(t, 1.6563744e+09), + }}, {name: "array", each: &compiledGauge{ compiledCommon: compiledCommon{ path: mustCompilePath(t, "status", "conditions"), From 10324307bd44145caa6941bf6ceaf4a718c7196b Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 2 Feb 2023 19:07:45 +0530 Subject: [PATCH 2/2] fixup! Handle unit length `valueFrom` values --- pkg/customresourcestate/registry_factory.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/customresourcestate/registry_factory.go b/pkg/customresourcestate/registry_factory.go index b477856d62..b69e11c38e 100644 --- a/pkg/customresourcestate/registry_factory.go +++ b/pkg/customresourcestate/registry_factory.go @@ -143,7 +143,7 @@ type compiledMetric interface { Type() metric.Type } -// newCompiledMetric returns a compiledMetric depending on given the metric type. +// newCompiledMetric returns a compiledMetric depending on the given metric type. func newCompiledMetric(m Metric) (compiledMetric, error) { switch m.Type { case MetricTypeGauge: @@ -217,6 +217,7 @@ func (c *compiledGauge) Values(v interface{}) (result []eachValue, errs []error) switch iter := v.(type) { case map[string]interface{}: for key, it := range iter { + // TODO: Handle multi-length valueFrom paths (https://github.com/kubernetes/kube-state-metrics/pull/1958#discussion_r1099243161). // Try to deduce `valueFrom`'s value from the current element. var ev *eachValue var err error @@ -226,7 +227,7 @@ func (c *compiledGauge) Values(v interface{}) (result []eachValue, errs []error) sValueFrom := c.ValueFrom.String() // No comma means we're looking at a unit-length path (in an array). if !strings.Contains(sValueFrom, ",") && - sValueFrom[0] == '[' && + sValueFrom[0] == '[' && sValueFrom[len(sValueFrom)-1] == ']' && // "[...]" and not "[]". len(sValueFrom) > 2 { extractedValueFrom := sValueFrom[1 : len(sValueFrom)-1]