-
Notifications
You must be signed in to change notification settings - Fork 179
Fix panic for some invalid custom metric requests #141
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
Fix panic for some invalid custom metric requests #141
Conversation
/triage accepted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will take a bit of time for me to review this since I would have to dive into the code again. I opened #145 to make it easier to understand the codebase and will most likely tackle it while diving into the code.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten |
This refactors the handling of metrics related to namespaces, by moving the logic from the storage part to the handler. This allows the handler to perform validation of the request.
6563ef8
to
a528fe4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgrisonnet, olivierlemasle 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 |
Currently, the invalid custom metric request
/apis/custom.metrics.k8s.io/v1beta2/namespaces/default/foo
causes a panic of the adapter implementingcustom-metrics-apiserver
instead of a normal failure.The panic is caused by the error
runtime error: slice bounds out of range [2:1]
here:custom-metrics-apiserver/pkg/apiserver/endpoints/handlers/get.go
Line 146 in e15f12b
In that case,
/namespaces/default/foo
has three parts so it is handled as a root resource metric (/resource/name/subresource
). However, the API serverRequestInfoResolver
handles the request as a namespaced resource request, and removes the/namespaces/default
part.This commit refactors the handling of metrics related to namespaces, by moving the logic from the storage part to the handler. This allows the handler to perform validation of the request. It then rejects (with a HTTP 400 error) the problematic request.
I'll add unit tests when #139 is merged.
/kind bug