-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add bearer_token_file
paramter to HTTP helper
#7527
Changes from all commits
5b0f68f
4134f37
404c4fc
1019312
3965a1d
5bd726e
833d0a5
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,71 @@ | ||
// Licensed to Elasticsearch B.V. under one or more contributor | ||
// license agreements. See the NOTICE file distributed with | ||
// this work for additional information regarding copyright | ||
// ownership. Elasticsearch B.V. licenses this file to you under | ||
// the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package helper | ||
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. I realize now this is a new file without license headers, but CI is not complaining. Are we checking that somewhere @andrewkroh? 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. @exekias Was also looking into this this morning. We check but return 0 after the check. Not sure if that was on purpose (probably not). 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. In the meanwhile, I've pushed the header to this file |
||
|
||
import ( | ||
"io/ioutil" | ||
"os" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestGetAuthHeaderFromToken(t *testing.T) { | ||
tests := []struct { | ||
Name, Content, Expected string | ||
}{ | ||
{ | ||
"Test a token is read", | ||
"testtoken", | ||
"Bearer testtoken", | ||
}, | ||
{ | ||
"Test a token is trimmed", | ||
"testtoken\n", | ||
"Bearer testtoken", | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
t.Run(test.Name, func(t *testing.T) { | ||
content := []byte(test.Content) | ||
tmpfile, err := ioutil.TempFile("", "token") | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer os.Remove(tmpfile.Name()) | ||
|
||
if _, err := tmpfile.Write(content); err != nil { | ||
t.Fatal(err) | ||
} | ||
if err := tmpfile.Close(); err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
header, err := getAuthHeaderFromToken(tmpfile.Name()) | ||
assert.NoError(t, err) | ||
assert.Equal(t, test.Expected, header) | ||
}) | ||
} | ||
} | ||
|
||
func TestGetAuthHeaderFromTokenNoFile(t *testing.T) { | ||
header, err := getAuthHeaderFromToken("nonexistingfile") | ||
assert.Equal(t, "", header) | ||
assert.Error(t, err) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,8 @@ | |
#namespace: example | ||
#username: "user" | ||
#password: "secret" | ||
|
||
# This can be used for service account based authorization: | ||
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. I think it would be useful to say what it does. Something like
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. Makes sense, I updated docs to include this longer explanation 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. Should there also be an update to the reference config files? Also applies to the k8s one. For the paths here for the prometheus config: Should we have k8s as default ones? 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. I think Prometheus is mostly used in k8s scenarios, it should not harm other use cases anyway? I've pushed a commit to include it in |
||
# bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token | ||
#ssl.certificate_authorities: | ||
# - /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt |
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.
Do you suggest moving to
config
appender for use cases that require a bearer token then? I wouldnt do a blanket token path on all configs with the hints builder.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.
Yeah, my thought was to use the
config
appender to setbearer_token_file
wherever you were using thetoken
appender before. No need to create new hints for this.Would that work for you?
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.
im ok with that. we will do the needful once this is merged.