PMM-15322 Pass TLS flags to valkey_exporter - #5854
Conversation
pmm-admin add valkey accepts --tls-ca, --tls-cert, --tls-key and --tls-skip-verify, and the certificates already reach the agent host as text files, but valkeyExporterConfig never pointed the exporter at them. TLS reached valkey_exporter only through the rediss:// scheme, so any instance behind a private CA or requiring mutual TLS reported redis_up 0 while the CLI reported success. Emit --tls-ca-cert-file, --tls-client-cert-file, --tls-client-key-file and --skip-tls-verification when TLS is enabled, following the pattern already used for mysqld_exporter. Also fix two adjacent defects on the same path: - redactWords omitted ValkeyOptions.SSLKey, so the private key could appear verbatim in exporter logs and API output. - TemplateDelimiters had no Valkey case, so a certificate containing the active delimiter broke exporter startup. pmm-agent renders text file contents as templates, so all three certificates are checked. Signed-off-by: Marcus Cruz <marcus.cruz@percona.com>
The valkey_exporter arguments matched the text file keys by string literal, duplicating the names produced by Agent.Files(). Renaming a key in one place would have silently stopped the flags from resolving. Export TLSCaFileName, TLSCertFileName and TLSKeyFileName from models and use them on both sides, and build the flags from a fixed table instead of ranging over the map, so emission no longer depends on iteration order. Signed-off-by: Marcus Cruz <marcus.cruz@percona.com>
Fold the TLS cases into TestValkeyExporterConfig, which covers the same unit, and drop the duplicated fixtures and the inlined helper copy. Add the scenarios the first round missed: unix socket connections, a private key without a certificate, an unknown pmm-agent version, and material surviving an exposeSecrets request. The delimiter cases now assert the rewritten flag rather than only the absence of the default delimiter. Signed-off-by: Marcus Cruz <marcus.cruz@percona.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughValkey TLS support now uses shared names for CA, client certificate, and client key files. Exporter configuration adds TLS file and skip-verification arguments, passes the same files to the agent process, and uses safe template delimiters. Valkey private keys are redacted from output. Tests cover TLS combinations, delimiter escaping, socket connections, unknown versions, and deterministic arguments. Documentation adds a mutual TLS connection example. Sequence Diagram(s)sequenceDiagram
participant ValkeyOptions
participant valkeyExporterConfig
participant AgentProcess
ValkeyOptions->>valkeyExporterConfig: TLS options and certificate contents
valkeyExporterConfig->>AgentProcess: TLS flags and TextFiles
AgentProcess->>AgentProcess: Redact SSLKey
Merge Risk: ⚪ Minimal · up to The PR now passes configured Valkey TLS certificates and verification settings to the exporter while protecting certificate rendering and private-key output. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
There was a problem hiding this comment.
Pull request overview
Adds Valkey exporter TLS/mTLS argument propagation and improves certificate handling.
Changes:
- Passes TLS certificate and verification flags to
valkey_exporter. - Adds private-key redaction, safe template delimiters, shared file constants, and tests.
- Documents mutual TLS setup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
managed/services/agents/valkey.go |
Builds exporter TLS arguments. |
managed/services/agents/valkey_test.go |
Covers TLS configurations and redaction. |
managed/services/agents/agents.go |
Redacts Valkey private keys. |
managed/models/agent_model.go |
Shares TLS filenames and selects safe delimiters. |
managed/models/agent_model_test.go |
Tests files and delimiters. |
documentation/docs/install-pmm/install-pmm-client/connect-database/valkey-redis.md |
Adds an mTLS example. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5854 +/- ##
==========================================
+ Coverage 43.59% 45.60% +2.00%
==========================================
Files 415 217 -198
Lines 43134 28127 -15007
==========================================
- Hits 18804 12827 -5977
+ Misses 22454 13915 -8539
+ Partials 1876 1385 -491 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The mutual TLS example passed the address via --address, which pmm-admin add valkey does not accept: name and address are positional. The command failed at flag parsing before reaching TLS. Reword the skip-verify note. Supplying --tls-ca already anchors trust, so a self-signed certificate does not need verification disabled. Replace the paired positional booleans in the test exporter helper with a named-field fixture struct. Signed-off-by: Marcus Cruz <marcus.cruz@percona.com>
Ticket number: PMM-15322
Feature build: SUBMODULES-0
Problem
pmm-admin add valkeyaccepts--tls,--tls-skip-verify,--tls-ca,--tls-certand--tls-key, and the command succeeds, but none of them reachedvalkey_exporter. Only therediss://scheme was encoded into--redis.addr. The certificates were shipped to the agent host as text files and then never referenced by any argument.Monitoring a Valkey/Redis instance behind a private CA, with a certificate whose SAN does not match, or requiring client certificates therefore left
redis_upat0while the CLI reported the service was added successfully.Fix
valkeyExporterConfignow emits--tls-ca-cert-file,--tls-client-cert-fileand--tls-client-key-filefor whichever certificates are present, plus--skip-tls-verificationwhen skip-verify is set. Everything is gated on TLS being enabled, matching therediss://decision inDSN(), so nothing points the exporter at certificates over a plaintext link.No
pmmAgentVersiongate is needed: all four flags exist inoliver006/redis_exporterv1.72.1 — the build shipped with PMM 3.5.0, the first release with Valkey support — through the currently pinned v1.89.0.Two adjacent defects found while investigating, both fixed here:
redactWords()covered the MySQL and PostgreSQL private keys but notValkeyOptions.SSLKey, so the Valkey private key could appear verbatim in exporter logs and API output.TemplateDelimiters()had no Valkey case. pmm-agent renders each text file's content as a Go template withmissingkey=error, so a PEM containing the active delimiter broke exporter startup. All three certificates feed delimiter selection, not only the private key, because all three are rendered.The text file names (
tlsCa,tlsCert,tlsKey) are now shared constants inmodelsrather than string literals duplicated betweenAgent.Files()and the argument builder — renaming one used to silently stop the flags from resolving.Docs: the Valkey/Redis connect page gained a mutual-TLS example, now that
--tls-certand--tls-keytake effect.Testing
go test ./managed/services/agents/... ./managed/models/... -run TestValkey, also under-race -count=5.exposeSecretsrequest, PEM content containing the active delimiter, and argument determinism.golangci-lintclean on every changed file, and one pre-existingexhaustiveerror is resolved by the newTemplateDelimiterscase.Not verified by me: the live end-to-end run against a real TLS/mTLS Valkey instance. Worth confirming on the feature build that
ps aux | grep valkey_exportershows the four flags pointing at real files and thatredis_upreports1.Related work
mysqld_exporterpattern inmanaged/services/agents/mysql.go, avoids callingFiles()twice, and adds the redaction and delimiter fixes.connection_checker.goandservice_info_broker.gonever setTlsSkipVerifyon the Valkey requests, so "Check connection" and version detection still fail with skip-verify. That is the exporter-adjacent half of the problem and stays on its own ticket.Follow-up suggestions (deliberately not addressed)
AddValkeyExporter/ChangeValkeyExporter) acceptstls_ca/tls_cert/tls_keywithtls=false. The material is stored and shipped, then silently ignored — the same failure mode as this ticket, through a path the CLI cannot produce (add_valkey.goonly reads the PEM files when--tlsis set). Rejecting it withInvalidArgumentwould make later, unrelatedChangecalls fail on any row already stored in that shape, so it is left alone;CertificatesIgnoredWhenTLSDisableddocuments the current contract.valkey_exporter, not formysqld_exporter. A typo in any of these four strings passes every unit test and reproducesredis_up 0. Real coverage needs an e2e test against a live TLS Valkey, which belongs in pmm-qa.Agent.Files()andmysql.gostill match the MySQL text file names by string literal. The newmodels.TLSCaFileNameconstants are used on the Valkey path only; converting the MySQL path is a separate cleanup.pmm-admin add valkeytakes the service name and address as positional arguments — there is no--addressflag — yet the "With remote monitoring" and "With TLS connection" examples pass--address=, and the basic example passes three positionals where the command accepts two. Theinventory add service valkeyexamples have the same--addressproblem (Addressisarg:""there too). Every one of these fails at flag parsing. Left out to keep this PR to the ticket.No API endpoints were added, removed or altered.