[VPA] Add prometheus bearer auth support #8263

yalosev · 2025-06-21T20:08:02Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds several flags for the Prometheus historical provider:

prometheus-insecure – Skip TLS verification. Prometheus may use a TLS certificate, and accessing it over https:// could result in an unknown CA certificate error if the certificate is self-signed or untrusted.
prometheus-bearer-token – Use a bearer token for authentication.
prometheus-bearer-token-file – Read the bearer token from a file. This is useful when using kube-rbac-proxy and a service account token is mounted into the pod.

Bearer token authentication and basic authentication are mutually exclusive, as both use the Authorization header. Only one of the following combinations should be set: username + password, bearer-token, or bearer-token-file.

Additionally, the authentication middleware logic was updated to follow best practices by cloning the request rather than modifying the original one.

Which issue(s) this PR fixes:

Fixes N/A

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-06-21T20:08:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yalosev
Once this PR has been reviewed and has the lgtm label, please assign omerap12 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

vertical-pod-autoscaler/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-06-21T20:08:12Z

Hi @yalosev. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Signed-off-by: Yuriy Losev <[email protected]>

adrianmoisey · 2025-06-22T18:13:55Z

/ok-to-test

adrianmoisey · 2025-06-22T18:14:41Z

vertical-pod-autoscaler/pkg/recommender/main.go

+	bearerToken         = flag.String("bearer-token", "", "The bearer token used in the Prometheus server bearer token auth")
+	bearerTokenFile     = flag.String("bearer-token-file", "", "Path to the bearer token file used for authentication by the Prometheus server")


Could these both be prepended with prometheus?
That way all the Prometheus options are bundled together when sorted alphabetically

Yes, sure. Changed.
I'm actually was surprised that not all prometheus options have this prefix. For example username / password are not linked with prometheus at all, as well as all -label options

Signed-off-by: Yuriy Losev <[email protected]>

omerap12 · 2025-06-23T08:27:24Z

/label tide/merge-method-squash

adrianmoisey · 2025-06-26T10:57:23Z

vertical-pod-autoscaler/pkg/recommender/input/history/history_provider.go

+		rt = http.DefaultTransport
+	}
+
+	// Clone the request to avoid modifying the original


This comment says what the code is doing, but not why the code is doing it. Could you add a comment linking to the best practise?

Well, it's a good point. It's more a self-control comment that I didn't remove. The main link is in the Golang source code. Also we have an example in the ReverseProxy.
I have changed the comment, is it ok for you?

While this code likely won’t cause undefined behavior now, I think we can avoid keeping it in such a risky state for future modifications.

That new comment is great, thank you!
It's the kind of thing that I imagine someone may come refactor later, and decide to remove. The new comment will at least give them the context to know why it's like that (if they didn't know before hand).

vertical-pod-autoscaler/pkg/recommender/input/history/history_provider_test.go

adrianmoisey · 2025-06-26T11:00:23Z

vertical-pod-autoscaler/pkg/recommender/main.go

+	bearerToken         = flag.String("prometheus-bearer-token", "", "The bearer token used in the Prometheus server bearer token auth")
+	bearerTokenFile     = flag.String("prometheus-bearer-token-file", "", "Path to the bearer token file used for authentication by the Prometheus server")


More nits: can the variable also start with prometheus? In case we need a bearer token and bearer token file in the future for other purposes.

vertical-pod-autoscaler/pkg/recommender/main.go

…provider_test.go Co-authored-by: Adrian Moisey <[email protected]>

Co-authored-by: Adrian Moisey <[email protected]>

Signed-off-by: Yuriy Losev <[email protected]>

adrianmoisey · 2025-07-01T19:32:17Z

vertical-pod-autoscaler/pkg/recommender/input/history/history_provider_test.go

Could you add another test that specifies the token and a username/password, and verifies that the bearer token is used?

We have a check in the main.go to make these flags exclusive.
But no problems, I have added the test.

Don't we want to move an error from the main.go to the HistoryProvider constructor. And check this condition here?

I think it's fine as is. I figured the test was worth adding to ensure that the behaviour doesn't change over time.

Signed-off-by: Yuriy Losev <[email protected]>

adrianmoisey · 2025-07-03T18:59:48Z

/lgtm
/assign @omerap12

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 21, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 21, 2025

k8s-ci-robot requested review from jbartosik and kwiesmueller June 21, 2025 20:08

k8s-ci-robot added area/vertical-pod-autoscaler size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2025

yalosev force-pushed the feature/prometheus-bearer branch from ea4d11f to 751c5b9 Compare June 22, 2025 14:14

Add prometheus bearer auth

ef07fa1

Signed-off-by: Yuriy Losev <[email protected]>

yalosev force-pushed the feature/prometheus-bearer branch from 05c38de to ef07fa1 Compare June 22, 2025 14:28

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 22, 2025

adrianmoisey reviewed Jun 22, 2025

View reviewed changes

Prefix for prometheus bearer flags

35a31d7

Signed-off-by: Yuriy Losev <[email protected]>

yalosev requested a review from adrianmoisey June 23, 2025 06:49

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 23, 2025

adrianmoisey reviewed Jun 26, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/recommender/input/history/history_provider_test.go Outdated Show resolved Hide resolved

adrianmoisey reviewed Jun 26, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/recommender/main.go Outdated Show resolved Hide resolved

yalosev and others added 3 commits June 26, 2025 18:30

Update vertical-pod-autoscaler/pkg/recommender/input/history/history_…

835847f

…provider_test.go Co-authored-by: Adrian Moisey <[email protected]>

Update vertical-pod-autoscaler/pkg/recommender/main.go

2e6de8c

Co-authored-by: Adrian Moisey <[email protected]>

rename the flags and change comment

e3bf1ba

Signed-off-by: Yuriy Losev <[email protected]>

yalosev requested a review from adrianmoisey June 26, 2025 14:55

adrianmoisey reviewed Jul 1, 2025

View reviewed changes

Add a test for both methods

53cf7d1

Signed-off-by: Yuriy Losev <[email protected]>

k8s-ci-robot assigned omerap12 and adrianmoisey Jul 3, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2025

		bearerToken = flag.String("bearer-token", "", "The bearer token used in the Prometheus server bearer token auth")
		bearerTokenFile = flag.String("bearer-token-file", "", "Path to the bearer token file used for authentication by the Prometheus server")

		bearerToken = flag.String("prometheus-bearer-token", "", "The bearer token used in the Prometheus server bearer token auth")
		bearerTokenFile = flag.String("prometheus-bearer-token-file", "", "Path to the bearer token file used for authentication by the Prometheus server")

[VPA] Add prometheus bearer auth support #8263

Are you sure you want to change the base?

[VPA] Add prometheus bearer auth support #8263

Conversation

yalosev commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jun 21, 2025

Uh oh!

k8s-ci-robot commented Jun 21, 2025

Uh oh!

adrianmoisey commented Jun 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omerap12 commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yalosev Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey commented Jul 3, 2025

Uh oh!

Uh oh!

yalosev commented Jun 21, 2025 •

edited

Loading

yalosev Jun 26, 2025 •

edited

Loading