Support Basic/Bearer Authorization for prometheus metrics query #2961

trsteel · 2023-07-12T04:34:36Z

For security reasons, Prometheus generally adds some permission verification, such as Basic Authorization, Bearer Authorization. Someone may have encountered similar problems, so I support this feature.

Signed-off-by: trsteel <trsteel@github.com>

william-wang · 2023-07-13T06:27:45Z

@trsteel Thanks for your contribution. Would you provide test result in the description? Great thanks.

trsteel · 2023-07-13T06:53:57Z

@trsteel Thanks for your contribution. Would you provide test result in the description? Great thanks.

I'm not sure what kind of test result should i provide here.
In my scenario，the bearer token is required to query prometheus.
Such as: curl -H 'Authorization: Bearer xxx' -v -X GET http://${prom-gateway-ip}:9090/api/v1/query?query=mem_usage_avg_5m.
If bearer token is miss, you can see some err log in volcano-scheduler(kubectl logs -n volcano-system volcano-scheduler): E0713 14:51:57.020468 39736 metrics_client_prometheus.go:85] Error querying Prometheus: client_error: client error: 401.

lowang-bh · 2023-07-13T12:42:22Z

In my mind, query in promethuse server usally does not need authorization. It depends on the configuration of promethuse server. So the authorization conf should be optional.

trsteel · 2023-07-14T02:14:51Z

In my mind, query in promethuse server usally does not need authorization. It depends on the configuration of promethuse server. So the authorization conf should be optional.

If you do not config username/password/bearertoken/bearertokenfile, transport.HTTPWrappersForConfig will do nothing.

lowang-bh · 2023-07-14T06:12:00Z

Please update relative docs at docs/design/usage-based-scheduling.md

volcano/docs/design/usage-based-scheduling.md

Lines 39 to 69 in 5302995

    
           ### Scheduler Configuration 
        
           ``` 
        
           actions: "enqueue, allocate, backfill"   
        
           tiers: 
        
             - plugins: 
        
                 - name: priority 
        
                 - name: gang 
        
                 - name: conformance 
        
                 - name: usage  # usage based scheduling plugin 
        
                   arguments: 
        
                     thresholds: 
        
                       CPUUsageAvg.5m: 90 # The node whose average usage in 5 minute is higher than 90% will be filtered in predicating stage 
        
                       MEMUsageAvg.5m: 80 # The node whose average usage in 5 minute is higher than 80% will be filtered in predicating stage 
        
             - plugins: 
        
                 - name: overcommit 
        
                 - name: drf 
        
                 - name: predicates 
        
                 - name: proportion 
        
                 - name: nodeorder 
        
                 - name: binpack 
        
           metrics:                               # metrics server related configuration 
        
             type: prometheus                     # Optional, The metrics source type, prometheus by default, support prometheus and elasticsearch 
        
             address: http://192.168.0.10:9090    # Mandatory, The metrics source address 
        
             interval: 30s                        # Optional, The scheduler pull metrics from Prometheus with this interval, 5s by default 
        
             tls:                                 # Optional, The tls configuration 
        
               insecureSkipVerify: "false"        # Optional, Skip the certificate verification, false by default 
        
             elasticsearch:                       # Optional, The elasticsearch configuration 
        
               index: "custom-index-name"         # Optional, The elasticsearch index name, "metricbeat-*" by default 
        
               username: ""                       # Optional, The elasticsearch username 
        
               password: ""                       # Optional, The elasticsearch password 
        
               hostnameFieldName: "host.hostname" # Optional, The elasticsearch hostname field name, "host.hostname" by default

Signed-off-by: trsteel <trsteel@github.com>

volcano-sh-bot · 2023-07-14T06:24:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign wpeng102
You can assign the PR to them by writing /assign @wpeng102 in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

trsteel · 2023-07-14T06:26:37Z

Please update relative docs at docs/design/usage-based-scheduling.md

volcano/docs/design/usage-based-scheduling.md

Lines 39 to 69 in 5302995

### Scheduler Configuration

```

actions: "enqueue, allocate, backfill"

tiers:

- plugins:

- name: priority

- name: gang

- name: conformance

- name: usage # usage based scheduling plugin

arguments:

thresholds:

CPUUsageAvg.5m: 90 # The node whose average usage in 5 minute is higher than 90% will be filtered in predicating stage

MEMUsageAvg.5m: 80 # The node whose average usage in 5 minute is higher than 80% will be filtered in predicating stage

- plugins:

- name: overcommit

- name: drf

- name: predicates

- name: proportion

- name: nodeorder

- name: binpack

metrics: # metrics server related configuration

type: prometheus # Optional, The metrics source type, prometheus by default, support prometheus and elasticsearch

address: http://192.168.0.10:9090 # Mandatory, The metrics source address

interval: 30s # Optional, The scheduler pull metrics from Prometheus with this interval, 5s by default

tls: # Optional, The tls configuration

insecureSkipVerify: "false" # Optional, Skip the certificate verification, false by default

elasticsearch: # Optional, The elasticsearch configuration

index: "custom-index-name" # Optional, The elasticsearch index name, "metricbeat-*" by default

username: "" # Optional, The elasticsearch username

password: "" # Optional, The elasticsearch password

hostnameFieldName: "host.hostname" # Optional, The elasticsearch hostname field name, "host.hostname" by default

I pushed a new commit to update docs/design/usage-based-scheduling.md, please review again.

lowang-bh · 2023-07-14T09:19:15Z

I pushed a new commit to update docs/design/usage-based-scheduling.md, please review again.

that's ok. How about another review-comment?

trsteel · 2023-07-14T09:31:32Z

I pushed a new commit to update docs/design/usage-based-scheduling.md, please review again.

that's ok. How about another review-comment?

the first review-comment I already replied. The auth config is aleady optional. If prometheus do not need auth, you just leave auth config empty. I'm not sure if there have any other comment I missed.

lowang-bh · 2023-07-14T12:37:57Z

Hi, @trsteel , can you see this comment as the following screenshot? I mean if both basic auth and token auth are set, there will be a error, right? Maybe we need to do some check about the conf?

And another comment about UT: you use different path to test the promethuse server, but in product env the promthuse server address is configurated at root path. The UT can not cover the code. If the basic auth and token auth are both set, there may will be error, right?

lowang-bh · 2023-07-12T13:28:34Z

pkg/scheduler/metrics/source/metrics_client_prometheus.go

+	tConf := &transport.Config{
+		Username:        p.conf["username"],
+		Password:        p.conf["password"],
+		BearerToken:     p.conf["bearertoken"],


Would you please update the relative help docs?

How about both basic auth and token auth are set? I find HTTPWrappersForConfig will check it and return error. link: https://github.com/kubernetes/client-go/blob/0c7c900fd55e3f77909f123388a3171d56346d7d/transport/round_trippers.go#L48-L49

already updated

yes，we can trust the check in HTTPWrapperForConfig.

If we don't handle this error, then the node metrics will not take effect. How bout do some check on the conf?

Here is aleady many checks in HTTPWrappersForConfig, our check will same with it. Add a check function, and returns an error if check failed. I think it change nothing and improve nothing.

// After adding checkSomehing function, we just return another error(different with `HTTPWrappersForConfig`) if auth config wrong. if err := checkSomehing(p.conf); err != nil { return nil, err } tConf := &transport.Config{ Username: p.conf["username"], Password: p.conf["password"], BearerToken: p.conf["bearertoken"], BearerTokenFile: p.conf["bearertokenfile"], }

Add a check function, and returns an error if check failed. I think it change nothing and improve nothing.

How about give a default setting to make it works if both basic auth and token auth are set?

That's a good ideal. I create a new commit for it. If basic auth is set, token will be clear.

That's ok. How about the UT? I think UT should test server with root path and let conf with only basic auth, token auth, or both basic auth and token auth are set.

UT is added now.

lowang-bh · 2023-07-14T06:27:55Z

pkg/scheduler/metrics/source/metrics_client_prometheus_test.go

+	defer testServer.Close()
+	// test basic auth
+	conf := map[string]string{"username": basicAuthUser, "password": basicAuthPwd}
+	metricsClient, _ := NewPrometheusMetricsClient(testServer.URL+"/basic-auth", conf)


You use different path to test the promethuse server, but in product env the promthuse server address is configurated at root path. If the basic auth and token auth are both set, there may will be error, right?

NewPrometheusMetricsClient will returns an error if auth config wrong. My test focus on checking auth configs appear in http req Header.

lowang-bh · 2023-08-14T07:43:22Z

Hi, @hwdef @wangyang0616 , would you please help to review it? thanks.

Signed-off-by: trsteel <trsteel@github.com>

stale · 2023-10-15T09:14:35Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

hwdef · 2023-12-15T08:52:12Z

/reopen
/assign @hwdef

volcano-sh-bot · 2023-12-15T08:52:16Z

@hwdef: Reopened this PR.

In response to this:

/reopen
/assign @hwdef

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/test-infra repository.

stale · 2024-03-17T09:30:41Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Monokaix · 2024-04-07T13:23:18Z

/ok-to-test

volcano-sh-bot · 2024-04-08T02:03:32Z

@trsteel: PR needs rebase.

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/test-infra repository.

Support Basic/Bearer Authorization for prometheus metrics query

b3dd9d3

Signed-off-by: trsteel <trsteel@github.com>

trsteel force-pushed the master branch from c61fd2f to b3dd9d3 Compare July 12, 2023 04:36

Update usage design doc with prometheus auth parameters.

7de30a0

Signed-off-by: trsteel <trsteel@github.com>

volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 14, 2023

lowang-bh reviewed Jul 15, 2023

View reviewed changes

trsteel added 2 commits August 14, 2023 15:52

Clear BearerToken/BearerTokenFile if has basic auth

5d51fa0

Signed-off-by: trsteel <trsteel@github.com>

Add unit test for both auth

6135364

Signed-off-by: trsteel <trsteel@github.com>

trsteel force-pushed the master branch from 2ee588e to 6135364 Compare August 14, 2023 09:46

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 15, 2023

stale bot closed this Dec 15, 2023

volcano-sh-bot assigned hwdef Dec 15, 2023

volcano-sh-bot reopened this Dec 15, 2023

stale bot removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 15, 2023

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 17, 2024

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 7, 2024

volcano-sh-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 7, 2024

lowang-bh mentioned this pull request Sep 19, 2024

Support to get the metric data from a prometheus metric api with basic auth enabled #3737

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Basic/Bearer Authorization for prometheus metrics query #2961

Support Basic/Bearer Authorization for prometheus metrics query #2961

trsteel commented Jul 12, 2023 •

edited

Loading

william-wang commented Jul 13, 2023

trsteel commented Jul 13, 2023

lowang-bh commented Jul 13, 2023

trsteel commented Jul 14, 2023

lowang-bh commented Jul 14, 2023

volcano-sh-bot commented Jul 14, 2023

trsteel commented Jul 14, 2023

lowang-bh commented Jul 14, 2023

trsteel commented Jul 14, 2023 •

edited

Loading

lowang-bh commented Jul 14, 2023 •

edited

Loading

lowang-bh Jul 12, 2023

trsteel Jul 15, 2023

lowang-bh Jul 15, 2023

trsteel Jul 15, 2023 •

edited

Loading

lowang-bh Aug 14, 2023

trsteel Aug 14, 2023

lowang-bh Aug 14, 2023

trsteel Aug 14, 2023

lowang-bh Jul 14, 2023

trsteel Jul 15, 2023

lowang-bh commented Aug 14, 2023

stale bot commented Oct 15, 2023

hwdef commented Dec 15, 2023

volcano-sh-bot commented Dec 15, 2023

stale bot commented Mar 17, 2024

Monokaix commented Apr 7, 2024

volcano-sh-bot commented Apr 8, 2024

Support Basic/Bearer Authorization for prometheus metrics query #2961

Are you sure you want to change the base?

Support Basic/Bearer Authorization for prometheus metrics query #2961

Conversation

trsteel commented Jul 12, 2023 • edited Loading

william-wang commented Jul 13, 2023

trsteel commented Jul 13, 2023

lowang-bh commented Jul 13, 2023

trsteel commented Jul 14, 2023

lowang-bh commented Jul 14, 2023

volcano-sh-bot commented Jul 14, 2023

trsteel commented Jul 14, 2023

lowang-bh commented Jul 14, 2023

trsteel commented Jul 14, 2023 • edited Loading

lowang-bh commented Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trsteel Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowang-bh commented Aug 14, 2023

stale bot commented Oct 15, 2023

hwdef commented Dec 15, 2023

volcano-sh-bot commented Dec 15, 2023

stale bot commented Mar 17, 2024

Monokaix commented Apr 7, 2024

volcano-sh-bot commented Apr 8, 2024

trsteel commented Jul 12, 2023 •

edited

Loading

trsteel commented Jul 14, 2023 •

edited

Loading

lowang-bh commented Jul 14, 2023 •

edited

Loading

trsteel Jul 15, 2023 •

edited

Loading