Skip to content

feat: add headers attribute for loki-logger (#11420)#12243

Merged
Baoyuantop merged 7 commits into
apache:masterfrom
ctxinf:plugin-loki-logger-add-header-options
Jun 6, 2025
Merged

feat: add headers attribute for loki-logger (#11420)#12243
Baoyuantop merged 7 commits into
apache:masterfrom
ctxinf:plugin-loki-logger-add-header-options

Conversation

@ctxinf

@ctxinf ctxinf commented May 22, 2025

Copy link
Copy Markdown
Contributor

Description

add an attribute/property authorization for loki-logger to enable providing http authorization header for non-local loki service.

Which issue(s) this PR fixes:

Fixes #11420

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels May 22, 2025
Comment thread apisix/plugins/loki-logger.lua Outdated
tenant_id = {type = "string", default = "fake"},

-- authorization header
authorization = { type = "string", minLength = 1, description = "Authorization header" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned in https://grafana.com/docs/loki/latest/operations/authentication/#manage-authentication, Loki does not have any built-in conventions for authentication mechanisms, so the request header used for authentication can be quite flexible; it does not necessarily be Authorization: XXX.
This is different from clickhouse-logger and elasticsearch-logger, which have built-in authentication.

This is exactly why I didn't add any authentication to the current plugin.

Therefore, it is better to design a generic mechanism for it, i.e., provide a user-configurable object to store one or more request headers.

Suggested change
authorization = { type = "string", minLength = 1, description = "Authorization header" },
headers = {
type = "object",
patternProperties = {
[".*"] = {
type = "string",
minLength = 1,
},
},
},

I don't have any idea about the name of the field, it could use headers, request_headers or auth_headers, ask other reviewers to take a look.

Comment thread apisix/plugins/loki-logger.lua
Comment thread apisix/plugins/loki-logger.lua
Comment thread apisix/plugins/loki-logger.lua Outdated
Comment thread t/plugin/loki-logger.t Outdated
"loki-logger": {
"endpoint_addrs": ["http://127.0.0.1:3100"],
"tenant_id": "tenant_1",
"authorization": "Bearer 1234",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO, this tests nothing. There isn't even a validation to prove that the header actually sends to the configured endpoint, it's too perfunctory.

Use the /echo endpoint to validate the request header, which is sent back as a response header.

=== TEST 20: enable basic auth plugin using admin api, hide_credentials = false
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"plugins": {
"basic-auth": {
"hide_credentials": false
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/echo"
}]]
)
if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- request
GET /t
--- response_body
passed
=== TEST 21: verify Authorization request header should not hidden
--- request
GET /echo
--- more_headers
Authorization: Basic Zm9vOmJhcg==
--- response_headers
Authorization: Basic Zm9vOmJhcg==

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, there is no local setup guide, and I cannot run make init and the local tests successfully, I only conducted actual manual testing.
And I don’t know how to use /echo to verify requests sent to the Loki URL.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then, please do this:

  1. Create route with the following upstream and URI
=== TEST XX: setup route (test auth headers)
--- config
    location /t {
        content_by_lua_block {
            local t = require("lib.test_admin").test
            local code, body = t('/apisix/admin/routes/1',
                ngx.HTTP_PUT,
                [[{
                    "plugins": {
                        "loki-logger": {
                            "endpoint_addrs": ["http://127.0.0.1:1980"],
                            "endpoint_uri": "/log_request",
                            "request_headers": {"Authorization": "test1234"},
                            "batch_max_size": 1
                        }
                    },
                    "upstream": {
                        "nodes": {
                            "127.0.0.1:1980": 1
                        },
                        "type": "roundrobin"
                    },
                    "uri": "/hello"
                }]]
            )

            if code >= 300 then
                ngx.status = code
            end
            ngx.say(body)
        }
    }
--- response_body
passed
  1. Send a request to /hello endpoint to trigger the log, then check the error.log.
=== TEST XX+1: hit route
--- request
GET /hello
--- response_body
hello world
--- error_log
go(): authorization: test1234

The /log_request endpoint used by loki will log the following error.log.

image

It contains every request header, which obviously includes the one we configured for loki for authentication.

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels May 28, 2025
Comment thread apisix/plugins/loki-logger.lua Outdated
Comment thread apisix/plugins/loki-logger.lua Outdated
Comment thread t/plugin/loki-logger.t
Comment thread apisix/plugins/loki-logger.lua Outdated
Comment thread docs/en/latest/plugins/loki-logger.md Outdated
Comment thread apisix/plugins/loki-logger.lua Outdated
@bzp2010

bzp2010 commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

It looks much more reliable and once the tests pass I can approve the change.
Then I'll call other reviewers.

bzp2010
bzp2010 previously approved these changes Jun 3, 2025
bzp2010
bzp2010 previously approved these changes Jun 3, 2025

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we need a better title for this PR

should focus on new "header" option



local function send_http_data(conf, log)
local headers = conf.headers or {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
local headers = conf.headers or {}
local headers = conf.headers or {}
headers = core.table.clone(headers) -- avoid changing the original table `conf`

@ctxinf ctxinf changed the title feat: add authorization attribute for loki-logger (#11420) feat: add headers attribute for loki-logger (#11420) Jun 4, 2025
@bzp2010
bzp2010 requested a review from membphis June 5, 2025 03:42

@juzhiyuan juzhiyuan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

nice job ^_^

@Baoyuantop
Baoyuantop merged commit 8c2d17e into apache:master Jun 6, 2025
22 checks passed
laz-xyr pushed a commit to laz-xyr/apisix that referenced this pull request Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loki-logger plugin needs configuration of basic auth username and password.

5 participants