grafana / agent

Vendor-neutral programmable observability pipelines.
https://grafana.com/docs/agent/
Apache License 2.0
1.6k stars 487 forks source link

traces.basic_auth's behavior is different than logs and metrics #975

Closed nicoche closed 3 years ago

nicoche commented 3 years ago

Hi!

I run a setup with metrics and logs activated, reporting a server protected by basic auth. I wanted to add traces but I could not get basic_auth to work, kept hitting HTTP 401. I realized that it is because I used the option password_file and that my file contained the password and a final \n. It seems that prometheus and loki strip extra line feeds at the end of password files but traces module does not. I've attached a reproduction case below, if you are curious.

Now, I think that it's strange that the final line feed is silently stripped for logs and metrics, but I believe that it's more important that all 3 modules work the same. wdyt?


Repro:

docker-compose:

services:
  agent:
    image: docker.io/grafana/agent:v0.19.0
    command: ["-config.file", "/shared/config.yaml", "-log.level", "debug"]
    ports:
      - "127.0.0.1:3500:3500" # Push logs here
      - "127.0.0.1:9411:9411" # Push traces here
    volumes:
      - .:/shared
  web:
    image: tutum/hello-world:latest
  basicauthserver:
    image: quay.io/dtan4/nginx-basic-auth-proxy
    environment:
      - BASIC_AUTH_USERNAME=user
      - BASIC_AUTH_PASSWORD=password
      - PROXY_PASS=http://web/

config.yaml:

logs:
  configs:
  - name: default
    positions:
      filename: /tmp/positions.yaml
    scrape_configs:
    - job_name: push1
      loki_push_api:
        server:
          http_listen_port: 3500
    clients:
      - url: http://basicauthserver
        basic_auth:
          username: user
          password_file: /shared/pw_file
traces:
  configs:
    - name: default
      receivers:
        zipkin:
      remote_write:
        - endpoint: http://basicauthserver
          protocol: http
          retry_on_failure:
            enabled: false
          basic_auth:
            username: user
            password_file: /shared/pw_file

Launch with:

$echo password > pw_file # pw_file will include a final \n
$docker-compose up
(...)

Then, in another terminal, try:

$curl -X POST 127.0.0.1:3500/loki/api/v1/push -H 'Content-Type: application/json' -d '{"streams": [{ "stream": { "foo": "bar2" }, "values": [ [ "1570818238000000000", "fizzbuzz" ] ] }]}'
$curl -X POST 127.0.0.1:9411/v1/traces -H 'Content-Type: application/json' -d '[{ "id": "f5402e865c61400e", "traceId": "0123456789abcdef", "timestamp": 1608239395286533, "duration": 100000, "name": "span from bash!", "tags": { "http.method": "GET", "http.path": "/api" }, "localEndpoint": { "serviceName": "shell script" } }]'

Loki will work, tempo won't:

(...) #docker-compose logs
basicauthserver_1  | 172.19.0.2 - user [09/Oct/2021:21:17:50 +0000] "POST / HTTP/1.1" 200 490 "-" "GrafanaAgent/v0.19.0"
basicauthserver_1  | 2021/10/09 21:17:50 [info] 14#14: *1 client 172.19.0.2 closed keepalive connection
web_1              | 172.19.0.4 - - [09/Oct/2021:21:17:50 +0000] "POST / HTTP/1.0" 200 478 "-" "GrafanaAgent/v0.19.0"
basicauthserver_1  | 2021/10/09 21:18:20 [error] 15#15: *3 user "user": password mismatch, client: 172.19.0.2, server: example.com, request: "POST /v1/traces HTTP/1.1", host: "basicauthserver"
basicauthserver_1  | 172.19.0.2 - user [09/Oct/2021:21:18:20 +0000] "POST /v1/traces HTTP/1.1" 401 195 "-" "Go-http-client/1.1"
agent_1            | ts=2021-10-09T21:18:20Z level=error caller=exporterhelper/queued_retry.go:254 msg="Exporting failed. Try enabling retry_on_failure config option." component=traces traces_config=default kind=exporter name=otlphttp/0 error="error exporting items, request to http://basicauthserver/v1/traces responded with HTTP Status Code 401"
nicoche commented 3 years ago

https://github.com/grafana/agent/pull/976

I don't have a working setup to test the changes. Does the test suite cover this part of the code?

mapno commented 3 years ago

976

I don't have a working setup to test the changes. Does the test suite cover this part of the code?

Hello! Thanks for the contribution, nice catch.

Unfortunately, the test suite does not specifically cover this case. config_test.go does indeed test using a file for the password, but it's added to the file without a trailing new line. To test the change, it would be sufficient to add a new line in that line.