Repository navigation
PMM-15692 Report rejected tokens as Unauthenticated. - #6087
Conversation
Grafana's /api/auth/serviceaccount has no sign-in middleware, so a token it rejects (wrong, expired, revoked, or of a disabled service account) gets 400 "Auth method is not service account token" instead of 401. pmm-managed mapped that 400 to codes.Internal. Since PMM-15186 the CLIs pick their hint from the code, so pmm-admin told users to check PMM Server logs instead of their credentials. Before PMM-13463 that 400 fell through to the API-key check, which answered 401; the live "Token auth - fail" test was switched to Internal when that fallback went away. Map 400 to Unauthenticated alongside 401 and 403, and restore the test's original expectation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6087 +/- ##
===========================================
+ Coverage 43.59% 57.81% +14.22%
===========================================
Files 415 373 -42
Lines 43134 39988 -3146
===========================================
+ Hits 18804 23121 +4317
+ Misses 22454 16867 -5587
+ Partials 1876 0 -1876 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pmm-managed now answers a service token Grafana rejects with codes.Unauthenticated instead of codes.Internal. pmm-agent setup read Unauthenticated as errCredentialsRejected and registered the Node again without asking with the credentials given to setup. With the Node still registered that failed on its name ("already exists", exit 1, which stops a container running setup on start) or added a second Node under the hostname. A brief Grafana DB failure ends in the same 400, so it could trigger this too. withGivenCredentials now treats a rejection like a refusal: it asks again with the credentials given to setup and only registers when PMM Server does not know the Agent; otherwise the registration is left unverified and kept, as before. The agent tests modelled a refused token as 401 with codes.Internal; they now cover 401 with codes.Unauthenticated, end to end through checkRegistrationOnServer. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
AGENTS.md allows a comment one line, two at most; the explanation of Grafana's 400 lives in the ticket and the first commit's message. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
The test called retrieveRole directly, so it checked the authError but not what a client receives. It now goes through ServeHTTP and asserts the HTTP status and the JSON body, the "code" the CLIs pick their hint from included. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughAgent setup retries a rejected service-token lookup with setup credentials when they differ from the running credentials. A successful retry returns the registered Node, and a retry that finds no Agent returns the not-found result. Grafana now identifies a rejected service token and maps that error to Sequence Diagram(s)sequenceDiagram
participant AgentSetup
participant PMM_Server
participant RegistrationCheck
AgentSetup->>PMM_Server: Lookup with running credentials
PMM_Server-->>AgentSetup: Credential rejection
AgentSetup->>PMM_Server: Retry lookup with setup credentials
PMM_Server-->>AgentSetup: Agent found or not found
AgentSetup->>RegistrationCheck: Return lookup outcome
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the main change: reporting rejected tokens as Unauthenticated. It is concise and specific. |
| Description check | |
The description includes the ticket number, feature build, change summary, testing details, API documentation status, and related work. It is complete and aligned with the repository template. |
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
ademidoff
left a comment
ademidoff
left a comment
There was a problem hiding this comment.
Reporting a disabled service-account token as Unauthenticated instead of Internal is the right goal, but the change is broader than the Grafana quirk it fixes. Together with the new branch in withGivenCredentials, it makes a few pmm-agent setup cases worse than before.
The main concerns:
- The 400 mapping is too broad (
auth_server.go:712). It catches Grafana-side lookup failures and also covers/api/userand/api/user/orgs. Moving it intogetRoleForServiceTokenwould keep the fix and avoid most of the knock-on effects below. - A brief Grafana failure can now re-register the Node when setup runs with the Agent's own credentials (
setup.go:98). - The rejected-token branch in
withGivenCredentialsmakes some cases worse: it hides a mistyped password (setup.go:115), throws away a successful second lookup in the very case this PR targets (setup.go:117), and ignoresinsecure-tlsfrom the config file (setup.go:97).
The remaining comments are about doc comments, test coverage and nits.
Generated by Claude Code
| if !serverRefused(err) || sameCredentials(running, given) { | ||
| // A token of a disabled service account, or one Grafana failed to look up, is rejected too. | ||
| rejected := errors.Is(err, errCredentialsRejected) | ||
| if (!rejected && !serverRefused(err)) || sameCredentials(running, given) { |
There was a problem hiding this comment.
When sameCredentials(running, given) is true, a rejected token still returns errCredentialsRejected unchanged, and checkRegistration maps that straight to registrationMissing. With the server change, pmm-managed now sends code 16 for every Grafana 400, and the comment above says that includes a token Grafana "failed to look up". So a brief Grafana failure now registers the Node again in this case.
Scenario: a container re-runs pmm-agent setup --config-file=... with no credential flags. Grafana's token lookup fails for a moment and answers 400.
- Before: code 13,
serverRefused,registrationUnverified, WARNING, exit 0. - Now:
registrationMissingandregister(). If Grafana is still failing, setup exits 1 and the container goes down, which is the outageregistrationUnverifiedis there to avoid. If Grafana has recovered,RegisterNodefails with "Node ... already exists, use --force", and--forcedeletes the Node with all its Services. If the Node is registered under a non-hostname name and there is no--region, a second Node is created under the hostname.
Narrowing the 400 mapping on the server (see the comment on auth_server.go) would avoid most of this.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 7f75cd0. A rejection of the token the Agent runs with becomes errTokenRejected and is treated like a refusal, so with the Agent's own credentials it ends registrationUnverified and nothing is registered. Covered end to end by "a token alone leaves the registration unverified".
| if cErr.Code == 401 || cErr.Code == 403 { | ||
| switch cErr.Code { | ||
| // Grafana answers a token it rejects with 400 on /api/auth/serviceaccount, see PMM-15692. | ||
| case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden: |
There was a problem hiding this comment.
Every Grafana 400 during the auth check now becomes codes.Unauthenticated. That includes a valid glsa_ token whose lookup failed on Grafana's side: on any authn error the request reaches the handler unauthenticated and gets the same 400.
Scenario: Grafana's database is slow or unavailable (startup, HA failover) and authn fails while looking up a valid token. Grafana answers 400 "Auth method is not service account token", and pmm-managed answers code 16. pmm-admin and pmm-agent then print "Please check username and password" instead of "Please check PMM Server logs", and pmm-agent setup treats it as errCredentialsRejected (see the comment on setup.go:98). Before this PR it was code 13, which correctly read as a server-side failure.
Generated by Claude Code
There was a problem hiding this comment.
Narrowing doesn't remove this one: Grafana puts every authn error, a DB error from GetAPIKeyByHash included, into LookupTokenErr and answers 400 on /api/auth/serviceaccount itself, so pmm-managed can't tell it from a disabled token. Setup no longer acts on a token rejection (7f75cd0), so it can't re-register the Node; what remains is the CLI hint during such a failure. The real fix is in percona/grafana: CurrentServiceAccount answering 500 when the lookup error isn't an Unauthorized one. Happy to open that as a follow-up.
| switch cErr.Code { | ||
| // Grafana answers a token it rejects with 400 on /api/auth/serviceaccount, see PMM-15692. | ||
| case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden: |
There was a problem hiding this comment.
The quirk is specific to /api/auth/serviceaccount, but the mapping is in the shared retrieveRole switch, so it also covers /api/user and /api/user/orgs. Any future 400 from the basic-auth or cookie paths (a malformed request, a Grafana validation error) would be reported as bad credentials.
Suggestion: turn the 400 into an "unauthenticated" error inside getRoleForServiceToken (client.go:550), where the quirk actually is. That fixes the disabled-token case without changing how the other endpoints are mapped.
Generated by Claude Code
There was a problem hiding this comment.
Done in ddd2e00: getRoleForServiceToken marks its 400 with errServiceTokenRejected, and retrieveRole maps only that, alongside 401 and 403 as before. A basic-auth 400 now stays Internal, with a test case for it.
| if errors.Is(e, errAgentNotFound) { | ||
| return serverNode{}, e | ||
| } | ||
| if rejected { |
There was a problem hiding this comment.
When the running credentials are rejected and the credentials given to setup are rejected too, setup now ends as registrationUnverified and exits 0, so the wrong password the operator typed is no longer reported.
Scenario: the Agent config holds basic-auth credentials (for example an upgraded v2 config with admin:oldpass), and the operator runs pmm-admin config with a mistyped new password.
- Before:
errCredentialsRejected,registrationMissing,RegisterNodeanswers 401/16, "Invalid username or password. Please check username and password", exit 1. - Now: the second lookup is rejected and this branch returns an error that no longer matches the sentinel. Setup prints "Failed to check the registration ...: credentials rejected." and the WARNING, exits 0, and keeps the broken credentials. The only hint is "Settings --server-username, --server-password only take effect when the Node is registered."
Generated by Claude Code
| } | ||
| if rejected { | ||
| // Unwrapped, so that the rejection leaves the registration unverified instead of registering again. | ||
| return node, errors.New(err.Error()) |
There was a problem hiding this comment.
In the main case this PR targets (service account disabled, Node still present, operator passes valid admin credentials), the second lookup does find the Node. That answer is thrown away (_, e := lookup(...)), and the operator sees:
Failed to check the registration of pmm-agent X with Y: credentials rejected.
WARNING: PMM Server at Y did not confirm that pmm-agent X is registered. Nothing was verified and nothing was changed; the existing registration is kept. Re-run once PMM Server answers, or use --force to register the Node again.
PMM Server did answer and did confirm the Node. Re-running can never help, because the token is rejected for good, and the only remedy offered (--force) removes the Node with every Service on it. The Agent is reloaded with the disabled token and stays disconnected, and nothing tells the operator that the Agent's own token is the problem.
When the second lookup succeeds, it would help to report that the Node is registered but the Agent's token is rejected, and say what to do about it.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 6bdb479. When the credentials given to setup find the Node, that is the answer: setup prints that PMM Server has the Agent on that Node but does not accept its token, points to the Node's service account, says --force removes the Node with its Services, and the registration is confirmed. A plain refusal (401 without Unauthenticated) still keeps the first answer.
There was a problem hiding this comment.
Two doc comments outside the diff are now stale:
serverNodeOfAgent(line 159): "The errors errAgentNotFound and errCredentialsRejected mean that the Node has to be registered again."withGivenCredentialsno longer treatserrCredentialsRejectedas a re-register verdict.serverRefused(line 222) still describes a 401 without Unauthenticated as the only grounds for asking again.
A new caller of serverNodeOfAgent that trusts the doc would re-register on errCredentialsRejected, which is what this PR guards against.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 8bbd7c1: serverNodeOfAgent no longer says errCredentialsRejected means re-register, and serverRefused names a rejected token as grounds for asking again too.
| // The credentials the Agent runs with are gone, so withGivenCredentials asks again with the ones given | ||
| // to setup, and registering with the same ones reports an actionable message. codes.PermissionDenied is | ||
| // deliberately not here: a service account below the admin role still holds valid credentials, it | ||
| // just cannot read the inventory, and registering the Node again over that would only add a second. |
There was a problem hiding this comment.
Nit: this comment block was rewritten in this diff and is still four lines. AGENTS.md (Code Style) asks for comments of "one line, two at most".
Generated by Claude Code
|
|
||
| _, authError := s.authenticate(ctx, req, req.URL.Path, logrus.WithField("test", t.Name())) | ||
| assert.Equal(t, codes.Internal, authError.code) | ||
| assert.Equal(t, codes.Unauthenticated, authError.code) |
There was a problem hiding this comment.
This live-Grafana assertion for Bearer wrong was flipped from Internal to Unauthenticated without being run (the PR description says so). It passes only if Grafana actually answers 400, 401 or 403 for that token. The old codes.Internal expectation shows Grafana did not answer 401/403. If it answers 5xx or 404 for a malformed (non-glsa_) token rather than the assumed 400, TestServerClientConnection/Token auth - fail fails in CI. Worth running it against a live Grafana before merging.
Generated by Claude Code
There was a problem hiding this comment.
It has run: the Managed "Unit tests" job runs go test ./... without skips inside a live PMM Server, and it passed on 00a7296. Grafana answers Bearer wrong with 400, so the assertion holds.
| results: []agentLookup{failed(rejected)}, | ||
| calls: 1, | ||
| err: rejected, | ||
| }, |
There was a problem hiding this comment.
Branches with no tests:
- the
u == nilbranch: a rejected token with no server address returnserrCredentialsRejected, soregistrationMissing. - a second lookup that fails for a reason other than credentials (timeout, PermissionDenied, TLS).
On the server side, TestAuthServerRejectedCredentials covers only the token endpoint, not the basic-auth endpoints that the 400 mapping also changes. A regression in any of these would pass the suite.
Generated by Claude Code
| }) | ||
| } | ||
|
|
||
| // TestAuthServerRejectedCredentials checks the answer clients get when Grafana rejects the credentials, whose code the CLIs pick their hint from. |
There was a problem hiding this comment.
Nit: the doc comment says the test covers what happens "when Grafana rejects the credentials", but the table also has a 500 "server error" case, which is not a rejection. The PR description also calls this test TestRetrieveRoleRejectedCredentials, while the code names it TestAuthServerRejectedCredentials.
Generated by Claude Code
There was a problem hiding this comment.
Renamed to TestAuthServerGrafanaErrors, with a doc comment that covers the whole table (bda1f50), and the PR description now uses that name.
The 400 Grafana gives a token it did not authenticate is specific to /api/auth/serviceaccount, which has no sign-in middleware. Mapping every 400 in retrieveRole also covered /api/user and /api/user/orgs, so a future 400 on the basic-auth or cookie path would have read as bad credentials. getRoleForServiceToken now marks its 400 with errServiceTokenRejected, and retrieveRole maps only that, alongside 401 and 403 as before. The log keeps Grafana's real status. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
The table holds a server error and a basic-auth 400 next to the rejected credentials, so "RejectedCredentials" and its doc comment described only part of it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
PMM Server answers a token Grafana failed to look up exactly like one it rejects, so a rejected token says nothing about the registration - the rule #5890 already applies to a refused token. The previous commit handled every errCredentialsRejected that way instead, which: - registered the Node again when setup had only the Agent's own token, so a brief Grafana failure could exit 1 or add a second Node; - hid a mistyped password: a rejected password was asked about again and ended unverified, where #5890 reports it and exits 1; - relied on errors.New(err.Error()) not matching the sentinel, and printed only "credentials rejected". withGivenCredentials now turns errCredentialsRejected into errTokenRejected only when the Agent runs with a token, and treats it like a refusal through a single predicate. A rejected password takes the #5890 path again, so the lookupError comment is back to its original text. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
With the service account disabled and the Node still there, the credentials given to setup find the Node, but that answer was thrown away: setup said "did not confirm that pmm-agent is registered", told the operator to re-run once PMM Server answers, and offered --force, which removes the Node with every Service on it. For a rejected token, the Node the second lookup finds is now the answer. Setup says that PMM Server has the Agent on that Node but does not accept its token, points to the Node's service account, and spells out what --force would remove; the registration is then confirmed and kept. A refused (401 without Unauthenticated) answer keeps the first answer as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
The second lookup set up its transport with the --server-insecure-tls given to setup only, ignoring insecure-tls from the configuration file that runningServer merges in for the first lookup. With the flag left off, it failed PMM Server's self-signed certificate instead of finding the Node, or finding it gone, and setup ended unverified. It now uses the merged setting, as the first lookup does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
serverNodeOfAgent and runningServer still said that a rejected token registers the Node again, and serverRefused named a 401 without Unauthenticated as the only grounds for asking again. A caller trusting them would re-register on errCredentialsRejected, which is what withGivenCredentials now guards against for a token. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
A rejected token with no PMM Server address to ask again, and a second lookup that fails for something other than the credentials - a deadline or credentials that cannot read the inventory - all have to keep the registration rather than register the Node again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
|
Thanks, I went through the fixes on One ask before merge: please open the percona/grafana follow-up you offered ( Non-blocking, take them or leave them:
Generated by Claude Code |
pmm-admin now reports "check username and password" for a disabled service account (percona/pmm#6087). pmm-managed caches a token's auth result for up to a minute, so poll for the failure for two minutes instead of ten seconds. Claude-Session: https://claude.ai/code/session_01Pc6CwnZoTC4TU48MW68CNb Co-authored-by: Claude <noreply@anthropic.com>
Ticket number: PMM-15692
Feature build: Percona-Lab/pmm-submodules#4628
Summary
When a service account or its token is disabled,
pmm-adminagain prints "Auth method is not service account token. Please check username and password." instead of "Please check PMM Server logs."Grafana's
/api/auth/serviceaccountroute has no sign-in middleware. A token Grafana does not authenticate (wrong, expired, revoked, or from a disabled account) therefore never gets a 401: the request reaches the handler unauthenticated and gets 400 "Auth method is not service account token". Until PMM-13463 that 400 fell through to the API-key check, which answered 401. Since API keys were removed, pmm-managed has passed the 400 on ascodes.Internal. Nobody saw this until #5707 made the CLIs choose their hint from the gRPC code.getRoleForServiceTokennow marks that 400 witherrServiceTokenRejected, andretrieveRolemaps it tocodes.Unauthenticated, the same as 401 and 403. A 400 on the basic-auth or cookie path staysInternal. The HTTP status (401) and the message are unchanged; onlycodechanges from 13 to 16. Grafana answers a token it failed to look up, such as during a DB failure, with the same 400, so that case also reads as rejected credentials. Telling the two apart needs a change in percona/grafana.Also in this PR:
pmm-agent setupno longer re-registers a Node over a rejected token. The registration check from PMM-15260 Make pmm-agent Node registration idempotent #5890 relied on a rejected token carrying code 13. Setup now treats a rejection of the token the Agent runs with like a refusal: it asks again with the credentials given to setup, re-registers only if PMM Server doesn't know the Agent, and otherwise keeps the registration. A rejected password keeps the PMM-15260 Make pmm-agent Node registration idempotent #5890 behaviour, which reports a mistyped password and exits 1.--forcewould remove. Before, it printed a "did not confirm" warning.insecure-tlsfrom the configuration file, as the first lookup does.TestServerClientConnection/Token auth - failexpectscodes.Unauthenticatedagain; CI runs it against a live Grafana. NewTestAuthServerGrafanaErrorssends requests throughServeHTTPto a fake Grafana and checks the status and bodycode. The agent tests model a rejected token as 401 with code 16, end to end throughcheckRegistrationOnServer.How to test: register a client with
PMM_AGENT_SERVER_USERNAME=service_tokenand a service-account token as the password. Disable the service account and wait 60 s for the auth cache to expire.pmm-admin listshould then print "Auth method is not service account token. Please check username and password." With the Node still registered, runpmm-agent setupwith admin credentials and without--force. It should report that PMM Server has the Agent on the Node but rejects its token, keep the registration and exit 0. After removing the Node in PMM, the same command registers it again. Re-running setup with only the token should keep the registration with a warning, not register the Node again. The e2e test PMM-T1884 (@service-account) covers thepmm-adminpath.If this PR adds, removes or alters one or more API endpoints, please review and update the relevant API documentation as well:
No API changes
If this PR is related to other PRs, contributions, or ongoing work in this or other repositories, please reference them here: