Skip to content

Validate metric name while listing available metrics and getting metric values #95

@invidian

Description

@invidian

Problem

Right now, it is possible for custom/external metrics adapter implemented using this library to expose and list metrics with names, which do not match Kubernetes resource naming convention, being as example:

The Pod "Foo" is invalid: metadata.name: Invalid value: "Foo": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

Listing and getting metrics using uppercase characters work fine when you use kubectl get --raw, however, it does not work when one requests the metric e.g. using HPA object. Attempt to do so, results in the following error on HPA object:

invalid metrics (1 invalid out of 1), first error is: failed to get external metric E2E: unable to get external metric newrelic-metrics-adapter-e2e-tests-5rl58/E2E/nil: unable to fetch metrics from external metrics API: getting fresh external metric value: getting metric value: metric "e2e" not configured

This is because either a client or API server downcases the requested resource name, so requesting metric named E2E results in client sending request for resource e2e.

Solution

I think values received from methods in ExternalMetricsProvider should be sanitized before sending this data back to the client.

Notes

If needed, this can be implemented as an opt-in feature, to avoid breaking changes.

kubernetes/kubernetes#72996 also talks about it. Perhaps other characters like / should also be not allowed. However, I'm not able to find a full list of forbidden characters.

EDIT:

It seems to me that ideally https://pkg.go.dev/k8s.io/apimachinery@v0.22.2/pkg/api/validation/path#IsValidPathSegmentName should be used.

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind/bugCategorizes issue or PR as related to a bug.lifecycle/frozenIndicates that an issue or PR should not be auto-closed due to staleness.needs-triageIndicates an issue or PR lacks a `triage/foo` label and requires one.priority/important-soonMust be staffed and worked on either currently, or very soon, ideally in time for the next release.

    Type

    No type

    Projects

    Status

    Backlog (stale)

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions