logcli: add support for basic token authentication #2889

gcotone · 2020-11-05T18:30:21Z

What this PR does / why we need it:
Add support for basic token authentication in cases where the loki-queriers/frontends are exposed through a reverse proxy doing token/bearer authentication.

Which issue(s) this PR fixes:
Fixes #395 (Partially - it does not implement oauth2)

Special notes for your reviewer:

Checklist

Documentation added
Tests updated

CLAassistant · 2020-11-05T18:30:25Z

All committers have signed the CLA.

achatterjee-grafana · 2020-11-05T18:33:21Z

@grafana/docs-squad, looking at this.

codecov-io · 2020-11-05T18:37:03Z

Codecov Report

Merging #2889 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2889      +/-   ##
==========================================
- Coverage   61.23%   61.20%   -0.04%     
==========================================
  Files         181      181              
  Lines       14574    14579       +5     
==========================================
- Hits         8925     8923       -2     
- Misses       4829     4836       +7     
  Partials      820      820

Impacted Files	Coverage Δ
pkg/logcli/client/client.go	`5.30% <0.00%> (-0.25%)`	⬇️
pkg/querier/queryrange/downstreamer.go	`95.29% <0.00%> (-2.36%)`	⬇️

achatterjee-grafana

Added some copy-edits.

docs/sources/getting-started/logcli.md

Co-authored-by: achatterjee-grafana <[email protected]>

achatterjee-grafana

LGTM! Thank you for your contribution.

jgehrcke · 2020-11-09T19:00:42Z

--token

The world of tokens is huge :-). I think the command line flag's name should use the word "bearer" to narrow down the scope a little, semantically.

Adds Authorization header bearer to API requests

I think this sentence can could still be polished, right? :-).

Maybe the most important feedback: personally, I do like the semantics and documentation of bearer_token_file in the Prometheus ecosystem. It's good to enable/encourage setting this sensitive value from a file as the primary method.

From the Prometheus docs:

# Sets the `Authorization` header on every scrape request with the bearer token
# read from the configured file. It is mutually exclusive with `bearer_token`.
[ bearer_token_file: <filename> ]

Related: #2739

What do you think about adding a --bearer-token-file PATH arg instead of --token VALUE?

daurnimator · 2020-11-10T13:09:44Z

docs/sources/getting-started/logcli.md

@@ -195,6 +197,8 @@ Flags:
      --key=""             Path to the client certificate key. Can also be set using LOKI_CLIENT_KEY_PATH env var.
      --org-id=""          adds X-Scope-OrgID to API requests for representing tenant ID. Useful for requesting tenant data when
                           bypassing an auth gateway.
+      --token=""         Adds Authorization header bearer to API requests for authentication purposes. It can also be set


whitespace is off here

daurnimator · 2020-11-10T13:10:22Z

pkg/logcli/client/client.go

+	//The token presence overrides basic-auth authentication
+	if c.Token != "" {
+		req.Header.Set("Authorization", "Bearer "+c.Token)
+		req.Header.Add("Accept", "application/json")


Why does the Accept header get set here?

logcli does not set this header elsewhere. By setting it here, we'd expect a 406 error if the response is not adequate (! application/json").
IMHO it should be set for all request but this PR's scope is to add support for bearer tokens.

Then leave it out of this PR entirely?

gcotone · 2020-11-12T05:14:24Z

--token

The world of tokens is huge :-). I think the command line flag's name should use the word "bearer" to narrow down the scope a little, semantically.

Adds Authorization header bearer to API requests

I think this sentence can could still be polished, right? :-).

Maybe the most important feedback: personally, I do like the semantics and documentation of bearer_token_file in the Prometheus ecosystem. It's good to enable/encourage setting this sensitive value from a file as the primary method.

From the Prometheus docs:
# Sets the `Authorization` header on every scrape request with the bearer token
# read from the configured file. It is mutually exclusive with `bearer_token`.
[ bearer_token_file: <filename> ]bearer_token_file
Related: #2739

What do you think about adding a --bearer-token-file PATH arg instead of --token VALUE?

@jgehrcke fair point. Prometheus actually supports both bearer_token and bearer_token_file. I'll adapt this PR to implement them following prometheus semantics.

stale · 2020-12-12T15:20:22Z

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

dbluxo · 2021-01-22T12:51:35Z

@gcotone We would need that too, do you still plan to work on it?

gcotone · 2021-02-02T09:06:25Z

@dbluxo not in the next month or so.

dbluxo · 2021-04-07T15:42:27Z

@gcotone I have created a new PR: #3585 (based on yours)

logcli: add support for basic token authentication

acbfb2d

gcotone requested review from achatterjee-grafana and oddlittlebird as code owners November 5, 2020 18:30

pull-request-size bot added the size/S label Nov 5, 2020

achatterjee-grafana reviewed Nov 5, 2020

View reviewed changes

gcotone and others added 2 commits November 6, 2020 08:44

Update docs for logcli token authentication

fef9d42

Co-authored-by: achatterjee-grafana <[email protected]>

logcli: add support for basic token authentication II

b5c8d51

Co-authored-by: achatterjee-grafana <[email protected]>

achatterjee-grafana approved these changes Nov 6, 2020

View reviewed changes

daurnimator reviewed Nov 10, 2020

View reviewed changes

stale bot added the stale A stale issue or PR that will automatically be closed. label Dec 12, 2020

stale bot closed this Dec 20, 2020

dbluxo mentioned this pull request Apr 7, 2021

logcli: add support for bearer token authentication #3585

Closed

2 tasks

dbluxo mentioned this pull request May 19, 2021

logcli: add support for bearer token authentication #3749

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logcli: add support for basic token authentication #2889

logcli: add support for basic token authentication #2889

gcotone commented Nov 5, 2020

CLAassistant commented Nov 5, 2020 •

edited

Loading

achatterjee-grafana commented Nov 5, 2020

codecov-io commented Nov 5, 2020 •

edited

Loading

achatterjee-grafana left a comment

achatterjee-grafana left a comment •

edited

Loading

jgehrcke commented Nov 9, 2020

daurnimator Nov 10, 2020

daurnimator Nov 10, 2020

gcotone Nov 12, 2020

daurnimator Nov 12, 2020

gcotone commented Nov 12, 2020

stale bot commented Dec 12, 2020

dbluxo commented Jan 22, 2021 •

edited

Loading

gcotone commented Feb 2, 2021

dbluxo commented Apr 7, 2021

logcli: add support for basic token authentication #2889

logcli: add support for basic token authentication #2889

Conversation

gcotone commented Nov 5, 2020

CLAassistant commented Nov 5, 2020 • edited Loading

achatterjee-grafana commented Nov 5, 2020

codecov-io commented Nov 5, 2020 • edited Loading

Codecov Report

achatterjee-grafana left a comment

Choose a reason for hiding this comment

achatterjee-grafana left a comment • edited Loading

Choose a reason for hiding this comment

jgehrcke commented Nov 9, 2020

daurnimator Nov 10, 2020

Choose a reason for hiding this comment

daurnimator Nov 10, 2020

Choose a reason for hiding this comment

gcotone Nov 12, 2020

Choose a reason for hiding this comment

daurnimator Nov 12, 2020

Choose a reason for hiding this comment

gcotone commented Nov 12, 2020

stale bot commented Dec 12, 2020

dbluxo commented Jan 22, 2021 • edited Loading

gcotone commented Feb 2, 2021

dbluxo commented Apr 7, 2021

CLAassistant commented Nov 5, 2020 •

edited

Loading

codecov-io commented Nov 5, 2020 •

edited

Loading

achatterjee-grafana left a comment •

edited

Loading

dbluxo commented Jan 22, 2021 •

edited

Loading