-
Notifications
You must be signed in to change notification settings - Fork 998
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) | ||
} | ||
} |
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.
HTTPWrappersForConfig
will check it and return error. link: https://github.com/kubernetes/client-go/blob/0c7c900fd55e3f77909f123388a3171d56346d7d/transport/round_trippers.go#L48-L49There 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.
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.
If we don't handle this error, then the node metrics will not take effect. How bout do some check on the conf?
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.
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.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.
How about give a default setting to make it works if both basic auth and token auth are set?
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.
That's a good ideal. I create a new commit for it. If basic auth is set, token will be clear.
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.
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.
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.
UT is added now.