fix: panic in util.GVRFromType for structured types
#2553
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-triage
|
This issue is currently awaiting triage. If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
L3n41c
force-pushed
the
fix_gvrfromtype
branch
from
26c0292 to
8859a42
Compare
k8s-ci-robot
added
size/XS
|
Thanks, makes sense to me. /lgtm /hold |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
lgtm
pkg/util/utils.go
Outdated
| apiVersion := expectedType.(*unstructured.Unstructured).Object["apiVersion"].(string) | ||
| t, err := meta.TypeAccessor(expectedType) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
CRS errors should ideally not crash KSM as that would result in downtime for native metrics that have nothing to do with this logic. Can we surface the error up?
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
pkg/util/utils.go
Outdated
| t, err := meta.TypeAccessor(expectedType) | ||
| if err != nil { | ||
| klog.ErrorS(err, "Failed to get type accessor", "expectedType", expectedType) | ||
| return nil |
There was a problem hiding this comment.
Apologies to block this again, but since this is used in a bunch of critical paths in CRS procedures, continuing silently after an error wouldn't make sense. I'd push for changing the signature here, and surfacing this up, as pointed in my previous review.
There was a problem hiding this comment.
@L3n41c friendly ping here. :) looks like this is the last bit to get this merged.
There was a problem hiding this comment.
@rexagod could you take a look at these changes?
k8s-ci-robot
added
size/M
pkg/customresourcestate/config.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create GVR for %s: %w", resource.GroupVersionKind, err) | ||
| } | ||
| gvrString := gvr.String() |
There was a problem hiding this comment.
We probably need a similar check as 61134a0 (#2553) here.
There was a problem hiding this comment.
I hesitated.
The thing is that the if block that checks if gvrString is nil and adapts if it is that I added in internal/discovery/discovery.go:224 has been copied from internal/store/builder.go:205 and internal/store/builder.go:564 because, in those three cases, if util.GVRFromType(…) returns an error, we only log an error and continue executing the function.
But, in this case and and in pkg/util/utils.go below, if util.GVRFromType(…) returns an error, the function forwards the error and exits early so that this line isn’t executed.
pkg/util/utils.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| customResourceClients[gvr.String()] = customResourceClient |
There was a problem hiding this comment.
Also fixed in c3b1afe
|
/lgtm Thank you for the patch, @L3n41c! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: L3n41c, mrueg, rexagod The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
Make
util.GVRFromTypeable to process structured types and not only unstructured ones.How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Does not change.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):customresourceinterface andcustomresourcecauses panic #2202