Repository navigation
PMM-14678 Rework vmagent remote-write routing - #5887
Merged
Merged
Conversation
In HA, PMM Server told every PMM Client vmagent to write to the in-cluster vmauth address, which clients outside the cluster cannot reach: services added with pmm-admin showed no dashboard data while QAN kept working (PMM-14678, PMM-14705). Select the remote-write configuration once, by deployment mode, and keep the two paths independent: - Standalone: unchanged routing. Internal VictoriaMetrics: clients write through PMM Server with their own PMM credentials. External VictoriaMetrics: clients and the server's own agent write to PMM_VM_URL directly. - HA: every write carries the VictoriaMetrics credential from PMM_VM_URL; only the URL differs. Clients write to the PMM Server address they already use, which the pmm-ha chart's HAProxy routes to vmauth (chart change shipped alongside). The server's own agent, which runs inside the cluster, writes to vmauth directly. Credentials now reach vmagent through its environment only: they are no longer embedded in the write URL or passed as command-line flags, so an operator's VMAGENT_remoteWrite_basicAuth_* override works in every mode. PMM's default credential belongs to PMM's default URL: when an operator injects VMAGENT_remoteWrite_url, no default credential is attached to it. The deployment flags are computed in one helper on StateUpdater and covered by a unit test; a golden test pins the complete environment for every deployment shape as literal strings; pmm-managed logs the chart requirement once when HA is enabled. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
VMAGENT_remoteWrite_url redirects every PMM Client's metric writes, and PMM sends none of its own credentials to an endpoint it did not choose. Warn at startup when the URL is injected without VMAGENT_remoteWrite_basicAuth_*, so the resulting unauthenticated writes are not silent. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Describe PMM_VM_URL with and without credentials, the precedence of VMAGENT_remoteWrite_basicAuth_* over URL credentials, and the global VMAGENT_remoteWrite_url override. In the HA guide, explain how client metrics reach VictoriaMetrics through HAProxy, the renamed secret keys, the chart-first upgrade order, and drop the PMM-14705 known issue. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5887 +/- ##
==========================================
+ Coverage 43.59% 51.33% +7.73%
==========================================
Files 415 420 +5
Lines 43134 39531 -3603
==========================================
+ Hits 18804 20292 +1488
+ Misses 22454 19239 -3215
+ Partials 1876 0 -1876 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 task
Follow-up to the review of the vmagent remote-write paths. PMM_VM_URL is parsed and validated in one place, models.ParseVictoriaMetricsURL: it must be an http or https URL with a host. A scheme-less value now fails at startup instead of silently truncating the remote-write URL handed to every vmagent. vmAgentConfig returns an error for a URL it cannot parse, and the error never echoes the URL because the URL may carry a password. In HA, pmm-managed warns at startup when PMM_VM_URL carries no credentials and no VMAGENT_remoteWrite_basicAuth_* is injected, since every PMM Client write would then be rejected with 401, and when HA is enabled with the built-in VictoriaMetrics. Environment validation rejects an empty VMAGENT_remoteWrite_url, warns when only one half of the basic-auth pair accompanies an injected URL, and stays quiet when the URL carries userinfo or another vmagent auth method is set. The URL-only warning no longer suggests a fix that is wrong in HA. Also drop the unused URLFor from the VictoriaMetrics params interface, log the agent id and the injected URL (without userinfo) at debug level, correct comments that misdescribed the built-in agent's route, remove inert VMAGENT_* rows from the docs, and delete a dead gosec exclusion. Tests clear VMAGENT_* from the process before running, cover the new error and warning paths, and add golden rows for an injected URL with one credential, an HA client without credentials, and a password-only external VictoriaMetrics. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
The env entries of an AgentProcess are marked REDACT_TYPE_DSN, which only masks credentials inside URL userinfo. vmagent now receives its remote-write password as a plain KEY=value entry (VMAGENT_remoteWrite_basicAuth_password), so the VictoriaMetrics password reached pmm-managed's debug dump of SetStateRequest in clear text. MaskDSN now also masks the value of any KEY=value entry whose key names a password, secret, or token. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
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⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change centralizes VictoriaMetrics URL parsing and validation, including credential-safe errors and parsed URL copies. Vmagent configuration now selects standalone or HA routing from deployment context. URL credentials are separated from endpoints, and injected authentication can replace PMM-derived credentials. HA startup logs now describe routing and credential requirements. Environment validation classifies authentication and redacts secrets more broadly. Tests cover routing, credential precedence, URL validation, startup warnings, and log masking. Sequence Diagram(s)sequenceDiagram
participant StateUpdater
participant vmAgentConfig
participant RemoteWriteBuilder
participant buildVMAgentProcess
participant vmagent
StateUpdater->>vmAgentConfig: deployment context and parsed VM URL
vmAgentConfig->>RemoteWriteBuilder: select HA or standalone routing
RemoteWriteBuilder-->>vmAgentConfig: endpoint and credential source
vmAgentConfig->>buildVMAgentProcess: scrape configuration and remote-write settings
buildVMAgentProcess-->>vmagent: arguments and environment variables
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Credentialed HTTP targets may expose remote-write credentials, and malformed override URLs can leave vmagent running without delivering metrics. Constrain the former and validate concrete overrides before merging. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 3b543a67-fd54-4ecc-ac00-42860a6ab58a
📒 Files selected for processing (20)
documentation/docs/install-pmm/install-HA-clustered.mddocumentation/docs/install-pmm/install-pmm-server/deployment-options/docker/env_var.mddocumentation/docs/reference/third-party/victoria.mdmanaged/cmd/pmm-managed/main.gomanaged/models/victoriametrics_params.gomanaged/models/victoriametrics_params_test.gomanaged/services/agents/deps.gomanaged/services/agents/state.gomanaged/services/agents/state_test.gomanaged/services/agents/vmagent.gomanaged/services/agents/vmagent_golden_test.gomanaged/services/agents/vmagent_ha.gomanaged/services/agents/vmagent_ha_test.gomanaged/services/agents/vmagent_standalone.gomanaged/services/agents/vmagent_standalone_test.gomanaged/services/agents/vmagent_test.gomanaged/utils/envvars/parser.gomanaged/utils/envvars/parser_test.goutils/logger/protobuf.goutils/logger/protobuf_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
💤 Files with no reviewable changes (1)
- managed/services/agents/deps.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
The standalone and HA split exists to keep the two deployment modes maintainable, not to describe the configuration to operators. The debug line keeps the remote-write URL and the credential source. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Address the CodeRabbit review on the vmagent remote-write paths. The HA startup warning accepted half a basic-auth pair as a credential and treated a bearer token or custom headers as none. Environment validation and HARemoteWriteWarning now share one predicate, envvars.VMAgentRemoteWriteAuthFromEnv, which counts both basic-auth variables or any other vmagent authentication method as complete. Half a pair is warned about in every mode, not only next to an injected VMAGENT_remoteWrite_url, because it breaks authentication everywhere. MaskDSN evaluated its "@" heuristic before the KEY=value secret check, so a password containing "@" leaked everything after it into the SetStateRequest debug dump. The secret check now runs first. The ParseEnvVars trace line printed URL userinfo for PMM_VM_URL and VMAGENT_remoteWrite_url; redactSecretEnvVar now drops it. Docs: credentials in PMM_VM_URL travel in clear text over http, and the VMAGENT_remoteWrite_url override also redirects PMM Server's own vmagent when an external VictoriaMetrics is configured. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
4nte
marked this pull request as ready for review
September 4, 2026 15:53
4nte
requested review from
JiriCtvrtka and
ademidoff
and removed request for
a team
September 4, 2026 15:53
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 4d2db86f-a134-42ea-b88a-38e8668367a8
📒 Files selected for processing (9)
documentation/docs/reference/third-party/victoria.mdmanaged/services/agents/vmagent.gomanaged/services/agents/vmagent_ha.gomanaged/services/agents/vmagent_ha_test.gomanaged/services/agents/vmagent_test.gomanaged/utils/envvars/parser.gomanaged/utils/envvars/parser_test.goutils/logger/protobuf.goutils/logger/protobuf_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- documentation/docs/reference/third-party/victoria.md
- managed/services/agents/vmagent_test.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
When VMAGENT_remoteWrite_url is injected every vmagent writes there and PMM_VM_URL takes no part in writes, yet HARemoteWriteWarning still inspected it and told the operator to add credentials to a URL nobody writes to. Environment validation already reports the injected endpoint's credential situation, so the HA warning now stays silent whenever the write URL is injected. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
envSecretRe used "." and "$" without the s flag, so a KEY=value entry whose value spans lines never matched and fell through to the DSN heuristic, which returns anything without "@" unmasked and leaks the tail after "@" otherwise. pmm-agent passes every process env entry through RedactString, so a multiline password would have reached its log. The pattern now spans newlines; a regression test covers it. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
…te-paths Signed-off-by: Ante Gulin <ante.gulin@percona.com>
AGENTS.md forbids %q in error and log messages. Replace the five uses this branch introduced: the VictoriaMetrics URL validation error, the PMM_VM_URL environment error, and three test assertion messages. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Contributor
Author
|
Referring to @coderabbitai finding / concern, Why we are not enforcing
What the PR does do to limit exposure:
|
Redact KEY=value environment entries whose key contains characters outside the shell alphabet, such as my-token or npm's npm_config_//registry.npmjs.org/:_authToken. The Nomad agent inherits pmm-agent's whole environment and pmm-agent logs it at debug level, so such host variables can reach MaskDSN. The key may not contain "@" or whitespace, otherwise a DSN whose user or database name contains a marker word ("tokens", "secrets") would be read as a key and its password echoed. Two rows guard that. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
JiriCtvrtka
reviewed
Sep 16, 2026
JiriCtvrtka
reviewed
Sep 16, 2026
JiriCtvrtka
reviewed
Sep 16, 2026
JiriCtvrtka
reviewed
Sep 16, 2026
JiriCtvrtka
reviewed
Sep 16, 2026
An empty VMAGENT_* variable is dropped with a startup warning instead of being forwarded or rejected. vmagent cannot use an empty value: an empty remote-write URL or logger level stops it, and an empty basic-auth pair silently disables authentication while it counted as a complete credential and displaced PMM's own. Empty values are a routine Helm and compose artifact, so they no longer keep pmm-managed-init from starting. redactSecretEnvVar parses a scheme-less user:pass@host as an authority, so its credentials no longer reach the trace log. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
JiriCtvrtka
reviewed
Sep 21, 2026
JiriCtvrtka
reviewed
Sep 21, 2026
Member
|
Re-reviewed at Nothing outstanding from my side. The only things I'd still call blocking are Jiri's two most recent comments, both of which I reproduced:
One more from inside Jiri's second comment that has no thread of its own, so it may get lost: a comma-separated |
A value that is set but empty was accepted before PMM_VM_URL gained its own validation, and kingpin falls back to the flag default for it, so rejecting it stopped pmm-managed-init on a configuration that had always started. Ignore it with a warning naming the variable, the way an empty VMAGENT_* variable is already handled. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
An additive method shared a case with the complete-pair check, so it returned Complete before the half-a-pair branch could return Partial. Setting a tenant header or a client certificate alongside a lone basicAuth username therefore silenced both the half-pair warning and the HA warning, while the credential was still withheld and vmagent was handed a username with no password. Additive methods now fall through the Partial check; exclusive ones stay above it. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Two copies of the same redaction had drifted apart and both leaked. Parsing a comma-separated remote-write URL as a single URL leaves everything after the first comma in the path, so only the first element's userinfo was dropped, in the trace of the environment and in the vmagent debug line. ParseVictoriaMetricsURL redacted by clearing URL.User, which a scheme-less value never populates because url.Parse leaves it in Opaque, so a credentialed PMM_VM_URL without a scheme was named in full in an error logged at Error level. Replace both with one helper that splits the value on commas and redacts each element, parsing a scheme-less one as an authority. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
…te-paths Signed-off-by: Ante Gulin <ante.gulin@percona.com>
JiriCtvrtka
reviewed
Sep 21, 2026
JiriCtvrtka
reviewed
Sep 21, 2026
A comma separates the URLs of a remote-write list and is also legal inside userinfo, so splitting on it first hands the front of a password to an element of its own, where it no longer looks like a credential and was printed as it stood. A password with three commas kept three of its four segments, in a startup error, in the environment dump and in the vmagent debug line. Split only a value whose every element carries a scheme, which is what vmagent requires of a remote-write URL. Anything else is redacted as the single URL it is, so a comma in a password no longer splits anything, and a value that cannot be parsed, or that still shows an '@' after a comma, is redacted whole. Parsing alone would not have been enough: in user:1234,5678@host the first fragment parses as a host and a port and raises no error at all. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Any remote-write header counted as a complete credential, so an operator who set a tenant identifier lost the warning that nothing authenticates the writes, and lost the one about half a credential in PMM_VM_URL with it. The writes then failed with nothing to explain them. Classify the header by what it carries: an Authorization or Proxy-Authorization header with a value authenticates the request, anything else does not. A client certificate keeps counting, since it authenticates whatever it is sent with. Whether PMM withholds its own credential is a separate question and is unchanged: a header composes with a basic-auth pair rather than replacing one. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
JiriCtvrtka
approved these changes
Sep 21, 2026
ademidoff
approved these changes
Sep 21, 2026
…te-paths Signed-off-by: Ante Gulin <ante.gulin@percona.com>
ademidoff
temporarily deployed
to
PMM-14678-vmagent-write-paths - pmm-doc-3 PR #5887
September 28, 2026 08:16 — with 
Render
Destroyed
Render
Destroyed
Signed-off-by: Ante Gulin <ante.gulin@percona.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket number: PMM-14678
Feature build: Percona-Lab/pmm-submodules#4556
Problem
In PMM HA, PMM Server told every PMM Client
vmagentto write metrics to the in-cluster vmauth address (PMM_VM_URL). Clients outside the cluster cannot reach it, so services added withpmm-adminshowedUNSPECIFIEDstatus and empty dashboards while QAN, which rides the gRPC channel, kept working.Change
The remote-write configuration is selected once, by deployment mode, and the two paths are independent:
vmagent_standalone.go): unchanged routing. Internal VictoriaMetrics: clients write through PMM Server with their own PMM credentials. External VictoriaMetrics (PMM_VM_URL): clients and the server's own agent write to it directly.vmagent_ha.go): every write carries the VictoriaMetrics credential fromPMM_VM_URL; only the URL differs. Clients write to the PMM Server address they already use, and thepmm-hachart's HAProxy routes/victoriametrics/api/v1/writeto vmauth (chart PR below). The server's own agent, which runs inside the cluster, writes to vmauth directly. PMM Server pods are not on the metrics write path; vmproxy stays a read-path component.A single deployment-agnostic builder applies the operator's
VMAGENT_*environment on top with two rules: an injected variable wins over PMM's default of the same name (the documented passthrough), and PMM's default credential is emitted only when the operator injected neitherVMAGENT_remoteWrite_urlnor a credential of their own (any half of a basic-auth pair, a bearer token or an OAuth2 client). An endpoint PMM did not choose never receives a credential PMM derived, half an injected pair is not completed with PMM's other half, and PMM's pair is never combined with a bearer token or OAuth2 client, which vmagent refuses to start with; custom headers and a client TLS certificate compose with basic auth and leave PMM's pair in place. An emptyVMAGENT_*variable is ignored with a startup warning, because vmagent cannot use an empty value (an empty URL or log level stops it, an empty credential disables authentication). Startup validation rejects an injectedVMAGENT_remoteWrite_urlthat does not parse, since vmagent exits on it, warns when a URL is injected without credentials or when only half of a basic-auth pair is set, and stays quiet when the URL carries userinfo or another vmagent authentication method is set.Credentials reach
vmagentthrough its environment only: no longer inside the write URL and no longer as-remoteWrite.basicAuth.*flags (VMAgentArgs()removed), so the documentedVMAGENT_remoteWrite_basicAuth_*override works in every mode.PMM_VM_URLis validated once at startup (models.ParseVictoriaMetricsURL): it must be an http or https URL with a host, so a scheme-less value fails fast instead of silently truncating the write URL. In HA,pmm-managedalso warns whenPMM_VM_URLcarries no credentials and nothing is injected (every client write would get401), and when HA runs against the built-in VictoriaMetrics; aPMM_VM_URLthat carries only a username or only a password is reported too, and the HA startup line describes the write path actually configured. The debug dump ofSetStateRequestmasks env values whose key names a password, secret, or token, whatever characters the key contains, and startup environment tracing redacts URL userinfo, scheme-less values included, so the VictoriaMetrics password no longer appears in logs.Behavior relative to
main:main{{.server_url}}/victoriametrics/api/v1/writevia HAProxy, same VM credentialsVMAGENT_remoteWrite_urlwithout credentialsVMAGENT_*variableChart dependency
Requires the
percona-helm-chartschange that adds the HAProxy route and renames the secret keys toPMM_HA_VM_USERNAME/PMM_HA_VM_PASSWORD: percona/percona-helm-charts#952. Upgrade the chart before PMM Server: a PMM Server with this change behind an older chart gets401for every client write (the pods reject the VictoriaMetrics credential). The chart README and the docs PR document the order;pmm-managedlogs the requirement once when HA is enabled.Documentation
User documentation for this change is in #5952 against
doc-3.10.0, because docs merge ahead of the code: the VictoriaMetrics reference (PMM_VM_URLwith and without credentials,VMAGENT_remoteWrite_url), the vmagent environment variables, and the HA guide (client write path through HAProxy, renamedpmm-secretkeys, chart-first upgrade order). This PR carries no documentation changes.Verification
managed/services/agents(vmagent_test.go,vmagent_standalone_test.go,vmagent_ha_test.go,vmagent_golden_test.go,state_test.go), plus new tests inmanaged/utils/envvars,managed/models, andutils/logger; all four packages pass with-race. The golden test pins every rendered configuration as literal strings, and mutation checks withgo test -overlay(typo in the proxy URL, partial-credential leak, HA client routed direct, built-in agent routed via the proxy, warning ignoring missing credentials, log leaking userinfo, error echoing the URL) all fail the suite.percona/pmm-server:3.9.1base withmain's nginx config and thispmm-managed): ten cells covering internal VM, external VM with and without auth, and every injection variant (credentials only, URL only, URL + credentials, URL +{{.server_*}}templates). Rendered env matches the design in every cell; the warning fires only in the URL-only cells.STATUS_UP; HAProxy's vmauth backend served 2600+ writes, all 2xx; zero write requests reached any pod's nginx or vmproxy; the three built-in agents write to vmauth directly (1000+ requests each, all 2xx); leader failover left writes uninterrupted; the escape hatch (injected vmauth URL + credentials) and the published-chart mismatch (401 at the pods) behaved as documented.Supersedes #5581, whose review findings are all addressed here (vmproxy read-only, per-scenario paths, no
dropInjectedAuth, credential withholding, env-only credentials,state.gowiring test).