Skip to content
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

Support Basic/Bearer Authorization for prometheus metrics query #2961

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/design/usage-based-scheduling.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ 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
username: "" # Optional, The basic auth username for prometheus client
password: "" # Optional, The basic auth password for prometheus client
bearertoken: "" # Optional, The bearer auth token for prometheus client
bearertokenfile: "" # Optional, The bearer auth token file for prometheus client
tls: # Optional, The tls configuration
insecureSkipVerify: "false" # Optional, Skip the certificate verification, false by default
elasticsearch: # Optional, The elasticsearch configuration
Expand Down
13 changes: 12 additions & 1 deletion pkg/scheduler/metrics/source/metrics_client_prometheus.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/prometheus/client_golang/api"
prometheusv1 "github.com/prometheus/client_golang/api/prometheus/v1"
pmodel "github.com/prometheus/common/model"
"k8s.io/client-go/transport"
"k8s.io/klog/v2"
)

Expand All @@ -52,11 +53,21 @@ func (p *PrometheusMetricsClient) NodeMetricsAvg(ctx context.Context, nodeName s
var client api.Client
var err error
insecureSkipVerify := p.conf["tls.insecureSkipVerify"] == "true"
tr := &http.Transport{
var tr http.RoundTripper = &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: insecureSkipVerify,
},
}
tConf := &transport.Config{
Username: p.conf["username"],
Password: p.conf["password"],
BearerToken: p.conf["bearertoken"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Would you please update the relative help docs?
  2. 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. already updated
  2. yes,we can trust the check in HTTPWrapperForConfig.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@trsteel trsteel Jul 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"],
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UT is added now.

BearerTokenFile: p.conf["bearertokenfile"],
}
if tr, err = transport.HTTPWrappersForConfig(tConf, tr); err != nil {
klog.Errorf("Error while wrap http round tripper with error: %v\n", err)
return nil, err
}
client, err = api.NewClient(api.Config{
Address: p.address,
RoundTripper: tr,
Expand Down
48 changes: 48 additions & 0 deletions pkg/scheduler/metrics/source/metrics_client_prometheus_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package source

import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"
)

const (
basicAuthUser = "username"
basicAuthPwd = "password"
bearerAuthToken = "bearertoken"
)

func TestPrometheusMetricsClient_NodeMetricsAvg(t *testing.T) {
testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
if strings.HasPrefix(req.URL.Path, "/basic-auth") {
userName, password, ok := req.BasicAuth()
if !ok || userName != basicAuthUser || password != basicAuthPwd {
t.Errorf("basic auth missmatch, ok: %v, userName: %v, password: %v", ok, userName, password)
res.WriteHeader(http.StatusUnauthorized)
}

} else if strings.HasPrefix(req.URL.Path, "/token-auth") {
if auth := req.Header.Get("Authorization"); auth != "Bearer "+bearerAuthToken {
t.Errorf("bearer token missmatch, token: %s", auth)
res.WriteHeader(http.StatusUnauthorized)
}
}
res.Write([]byte(`{"status":"success","data":{"resultType":"vector","result":[]}}`))
res.WriteHeader(http.StatusOK)
}))
defer testServer.Close()
// test basic auth
conf := map[string]string{"username": basicAuthUser, "password": basicAuthPwd}
metricsClient, _ := NewPrometheusMetricsClient(testServer.URL+"/basic-auth", conf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if _, err := metricsClient.NodeMetricsAvg(context.TODO(), "node-name", "5m"); err != nil {
t.Errorf("Get Node Metric Avg with basic auth err %v", err)
}
// test token auth
conf = map[string]string{"bearertoken": bearerAuthToken}
metricsClient, _ = NewPrometheusMetricsClient(testServer.URL+"/token-auth", conf)
if _, err := metricsClient.NodeMetricsAvg(context.TODO(), "node-name", "5m"); err != nil {
t.Errorf("Get Node Metric Avg with token auth err %v", err)
}
}