-
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
Conversation
@vjsamuel, I think this should deprecate the |
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.
Are these HTTP options documented in Asciidoc somewhere? It would be nice if we could point people to bearer_token_file
on the website if needed.
@@ -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 comment
The 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
It reads the contents of the file once at initialization and then uses the value in an HTTP Authorization header.
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.
Makes sense, I updated docs to include this longer explanation
Needs a changelog entry? |
Thanks for the reviews, I have added more docs & changelog entry |
@@ -0,0 +1,54 @@ | |||
package helper |
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
In the meanwhile, I've pushed the header to this file
Latest K8s version (1.11, just released) makes the safe port mandatory, so Metricbeat kubernetes module cannot work without this. I'm adding the |
@@ -41,7 +41,7 @@ type tokenAppender struct { | |||
// NewTokenAppender creates a token appender that can append a bearer token required to authenticate with | |||
// protected endpoints | |||
func NewTokenAppender(cfg *common.Config) (autodiscover.Appender, error) { | |||
cfgwarn.Beta("The token appender is beta") | |||
cfgwarn.Deprecate("7.0.0", "token appender is deprecated in favor of bearer_token_file config parameter") |
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 set bearer_token_file
wherever you were using the token
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.
@@ -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 comment
The 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 comment
The 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
This change allows to load barer tokens from files in modules using the HTTP helper. For instance: ``` - module: kubernetes metricsets: - pod bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token ssl.certificate_authorities: - /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt ```
Seems like something in prometheus breaks the doc build :-( |
ups, my bad, should be ok now, sorry |
2c5c381
to
bac35e7
Compare
This change allows to load bearer tokens from files in modules using the HTTP helper. This is especially useful for Kubernetes and Prometheus, as some deployments enforce SSL access (like OpenShift): ``` - module: kubernetes metricsets: - pod bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token ssl.certificate_authorities: - /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt ``` Closes elastic#7518 (cherry picked from commit 1d3109f)
…lper (#7577) * Add `bearer_token_file` paramter to HTTP helper (#7527) This change allows to load bearer tokens from files in modules using the HTTP helper. This is especially useful for Kubernetes and Prometheus, as some deployments enforce SSL access (like OpenShift): ``` - module: kubernetes metricsets: - pod bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token ssl.certificate_authorities: - /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt ``` Closes #7518 (cherry picked from commit 1d3109f) * Update CHANGELOG.asciidoc
…HTTP helper (elastic#7577) * Add `bearer_token_file` paramter to HTTP helper (elastic#7527) This change allows to load bearer tokens from files in modules using the HTTP helper. This is especially useful for Kubernetes and Prometheus, as some deployments enforce SSL access (like OpenShift): ``` - module: kubernetes metricsets: - pod bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token ssl.certificate_authorities: - /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt ``` Closes elastic#7518 (cherry picked from commit 7b90836) * Update CHANGELOG.asciidoc
This change allows to load bearer tokens from files in modules using
the HTTP helper. This is especially useful for Kubernetes and Prometheus, as some deployments enforce SSL access (like OpenShift):
Closes #7518