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

FluentD loki plugin: add support for bearer_token_file parameter #2739

Merged
merged 9 commits into from
Oct 16, 2020

Conversation

jgehrcke
Copy link
Contributor

@jgehrcke jgehrcke commented Oct 8, 2020

This pull request adds a configuration parameter bearer_token_file for the Loki FluentD plugin.

The value is expected to be a file path. When specified, the corresponding file is read (once, during startup). The content is assumed to be an authentication token. In all subsequent POST HTTP requests towards the Loki API, the plugin then sets an HTTP header of the shape Authorization: Bearer <token>.

In the cloud native ecosystem the Bearer authentication scheme gains more and more popularity(*) for communicating authentication proof within HTTP requests, and it is not uncommon to have an authenticator in front of the Loki API that requires the presentation of such authentication proof.

The name of the configuration parameter and the concept in general is inspired by Prometheus' bearer_token_file parameter, documented here: https://prometheus.io/docs/prometheus/latest/configuration/configuration/

In addition to this feature change, this PR also contains

  • a few code clarifications, both via function/variable naming changes as well as via code comments / parameter descriptions
  • new log statements

Two new log statements in action (helping with debugging)

...
2020-10-08 10:27:30 +0000 [info]: will use Bearer token from bearer_token_file /tmp/api-token in Authorization header
...
2020-10-08 10:27:34 +0000 [debug]: #0 POST request was responded to with status code 204

Security note: the token itself is sensitive data, and with the file-based approach the FluentD config document can stay non-sensitive, and the FluentD log will also not show the value of the token (at least not that I am aware of).

*: The Bearer scheme was once reserved for usage in the context of OAuth2, see https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml and https://tools.ietf.org/html/rfc6750. But it has morphed into something more generic in the meantime, at least in practice.

Which issue(s) this PR fixes:

I did not create an issue for this, can do if desired :-).

Special notes for your reviewer:

  • This is manually tested and seems to work well.
  • I would love to get some initial feedback here @cyriltovena maybe :-) -- thanks for taking a look! Please let me know which changes/additions you'd like to see.

Checklist

  • Documentation added
  • Tests updated

@codecov-io
Copy link

Codecov Report

Merging #2739 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2739      +/-   ##
==========================================
+ Coverage   61.39%   61.47%   +0.07%     
==========================================
  Files         173      173              
  Lines       13465    13465              
==========================================
+ Hits         8267     8277      +10     
+ Misses       4445     4433      -12     
- Partials      753      755       +2     
Impacted Files Coverage Δ
pkg/promtail/targets/file/tailer.go 70.09% <0.00%> (-5.61%) ⬇️
pkg/logql/evaluator.go 92.81% <0.00%> (+0.42%) ⬆️
pkg/promtail/targets/file/filetarget.go 66.19% <0.00%> (+2.11%) ⬆️
pkg/promtail/positions/positions.go 58.51% <0.00%> (+11.70%) ⬆️

validate_client_cert_key
end

raise "bearer_token_file #{@bearer_token_file} not found" if !@bearer_token_file.nil? && !File.exist?(@bearer_token_file)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

self review: need to remove that line

@jeschkies
Copy link
Contributor

👋 @jgehrcke, small world 🙂

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

All good anything you wanted to change before we merge this ?

@jgehrcke
Copy link
Contributor Author

wave @jgehrcke, small world slightly_smiling_face

Hello @jeschkies -- so cool! Grafana Labs should be honored to have you (: small world, indeed!

LGTM

@cyriltovena we're good here, I think you can merge this! Works fine in our testing. Happy to submit follow-up patches as we think they're necessary.

@cyriltovena
Copy link
Contributor

wave @jgehrcke, small world slightly_smiling_face

Hello @jeschkies -- so cool! Grafana Labs should be honored to have you (: small world, indeed!

I'm honored ❤️

@cyriltovena cyriltovena merged commit 4e661cd into grafana:master Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants