feat(ccache-remote): parse password from DNS-SD TXT for redis backend#9864
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds optional Redis/HTTP password discovery from DNS‑SD TXT/SRV records (validated against a URL-safe whitelist) and embeds safe passwords into generated backend URLs; extends Redis stats userinfo parsing to extract passwords from several userinfo forms; and adds an explicit ChangesRedis password support in DNS-SD discovery
ccache numfmt option parsing safety
Sequence DiagramsequenceDiagram
participant Avahi as avahi-browse
participant Discover as ccache_discover_remote_storage
participant Validator as password_validator
participant Builder as backend_url_builder
participant RedisCLI as redis-cli
Avahi->>Discover: advertise/browse service + TXT (password?)
Discover->>Validator: extract TXT/TXT from SRV, validate [A-Za-z0-9._~-]+
alt valid
Validator->>Builder: provide password
Builder->>Discover: produce backend URL (redis://<password>`@host`:port|connect-timeout=...)
Discover->>RedisCLI: stats/operations using extracted password
else invalid
Validator-->>Discover: skip candidate (allow fallback)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/ccache-remote/ccache-remote.sh`:
- Around line 274-277: The current check in ccache_discover_remote_storage()
treats an unsafe svc_password as fatal by calling "return 1", which aborts the
entire discovery and prevents Method 3 (ccache.local) from running; change this
to skip only the current SRV/TXT candidate (e.g., replace "return 1" with a
non-fatal control flow such as "continue" inside the SRV-processing loop or
otherwise proceed to the next candidate) so display_alert still runs but
discovery continues and ccache.local fallback remains reachable; keep the
svc_password check, keep display_alert, and ensure no other callers rely on this
function returning early for validation.
- Around line 198-219: The stats probe is not handling the new password-only
Redis URL form (redis://${svc_password}@...) so ccache_get_remote_stats() fails
to authenticate; update ccache_get_remote_stats() to parse the discovered
redis_url for a userinfo that may be just a password (no colon) and pass that
password to redis-cli (e.g., via -a/--pass or REDISCLI_AUTH) when present, while
preserving the existing behavior for user:pass and no-userinfo cases; look for
code that constructs/consumes redis_url and add a branch that detects the
^[^:]+@ pattern (password-only) and forwards that password to redis-cli during
the probe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87559d7a-e7ce-48ad-8ca6-8fae043e751a
📒 Files selected for processing (2)
extensions/ccache-remote/ccache-remote.shlib/functions/compilation/ccache.sh
iav
force-pushed
the
feat/ccache-remote-password-discovery
branch
2 times, most recently
from
9fcb3d6 to
bfa6084
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/ccache-remote/ccache-remote.sh`:
- Around line 271-283: The validation that rejects non-whitelisted svc_password
currently runs for all SRV records; change it so the regex check is only
performed when svc_type == "redis" (i.e. gate the existing if [[ -n
"${svc_password}" && ! "${svc_password}" =~ ... ]] branch behind a check that
svc_type equals "redis"), leaving the HTTP branch untouched; update the logic
around svc_type, svc_password and the export of CCACHE_REMOTE_STORAGE (and keep
ccache_format_host_port usage) so HTTP records with a password= TXT key are not
skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 035cf8b2-8b5b-46dd-89aa-2e10014e6a90
📒 Files selected for processing (2)
extensions/ccache-remote/ccache-remote.shlib/functions/compilation/ccache.sh
✅ Files skipped from review due to trivial changes (1)
- lib/functions/compilation/ccache.sh
iav
force-pushed
the
feat/ccache-remote-password-discovery
branch
from
bfa6084 to
2433d21
Compare
Extend ccache_discover_remote_storage to consume an optional `password=`
TXT record alongside `type=redis` (avahi-browse branch) and `type=redis`
DNS-SRV TXT (the dig branch). When present, embed it in the resulting
CCACHE_REMOTE_STORAGE URL so ccache authenticates against `requirepass`-
enabled backends discovered via mDNS or DNS-SD without the user having
to set CCACHE_REMOTE_STORAGE manually.
URL form: `redis://<password>@<host>:<port>|...` — plain userinfo with
no leading colon and no `username:` prefix. The other two reasonable
candidates both break:
* `redis://:<password>@<host>:<port>` — ccache 4.9.1 parses but
silently never sends AUTH; backend then rejects every command with
`NOAUTH Authentication required`.
* `redis://default:<password>@<host>:<port>` — ccache sends the
Redis-6 ACL two-arg `AUTH default <password>`, which KvRocks
<= 2.15.0 rejects with `ERR wrong number of arguments`.
The chosen form sends the legacy one-arg `AUTH <password>` that both
Redis and KvRocks accept.
ccache_mask_storage_url() already strips the userinfo, so the password
stays out of build logs:
DNS-SD: discovered Redis ccache redis://****@m1:6666|connect-timeout=500
Verified end-to-end on a KvRocks 2.15.0 instance behind avahi-publish-service
with `requirepass` and `password=<secret>` TXT: ccache logs
`Redis AUTH ********` followed by `Redis GET <key>`,
`remote_storage_error` is 0, and `remote_storage_write` increments on
miss.
iav
force-pushed
the
feat/ccache-remote-password-discovery
branch
from
2433d21 to
1d585ff
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/ccache-remote/ccache-remote.sh`:
- Around line 271-280: The DNS TXT extraction currently uses txt_output and
assigns svc_password via the regex password=([^\"[:space:]]+), which stops at
the first space and yields truncated credentials; change the extraction so
svc_password captures the full quoted password (align with the Avahi branch) —
e.g., update the regex that reads from txt_output to capture the entire quoted
value for the password (so svc_password receives the full string) before the
RFC3986 whitelist check in the same function, leaving the subsequent validation
of svc_password against ^[A-Za-z0-9._~-]+$ unchanged so unsafe passwords will
correctly fall through to Method 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7328144f-4dde-466b-8ba8-47316e6aee34
📒 Files selected for processing (2)
extensions/ccache-remote/ccache-remote.shlib/functions/compilation/ccache.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/ccache-remote/ccache-remote.sh`:
- Around line 286-298: The code currently treats any non-"http" svc_type as
Redis; change the conditional so only svc_type == "redis" sets
CCACHE_REMOTE_STORAGE to a redis:// URL (using svc_password, host_port and
CCACHE_REDIS_CONNECT_TIMEOUT as before) and let any other svc_type fall through
(do not export CCACHE_REMOTE_STORAGE or return 0); update the branch that calls
display_alert and return to only run when a supported type was handled (i.e.,
after the http and explicit "redis" branches), referencing the svc_type
variable, the CCACHE_REMOTE_STORAGE export, the ccache_mask_storage_url call and
display_alert to locate the logic to modify.
- Around line 368-374: The stats probe currently discards the ACL username by
parsing userinfo into only password (variables userinfo and password); update
the parser to also extract a username variable (e.g., username="${userinfo%%:*}"
when userinfo contains a colon, and default to empty or omit otherwise) and
ensure the stats probe invocation of redis-cli includes the --user "$username"
flag when username is non-empty so ACL-backed Redis hosts authenticate
correctly; keep existing password extraction logic for legacy bare-password
forms and only supply --user when a username was present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c5eaefc-3b06-4676-95c8-fe0017c2ca9a
📒 Files selected for processing (2)
extensions/ccache-remote/ccache-remote.shlib/functions/compilation/ccache.sh
✅ Files skipped from review due to trivial changes (1)
- lib/functions/compilation/ccache.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
extensions/ccache-remote/ccache-remote.sh (1)
286-299:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnknown
type=values are silently treated as Redis.A typo like
type=redsiin the TXT record would setsvc_type="redsi"(line 265-266), fail the== "http"check, and fall through to generate a Redis URL. This could cause confusing runtime failures instead of a clear warning.Consider adding explicit
svc_type == "redis"checks and warning/falling through for unknown types:🔧 Suggested fix
if [[ "${svc_type}" == "http" ]]; then if [[ -n "${svc_password}" ]]; then export CCACHE_REMOTE_STORAGE="http://:${svc_password}@${host_port}${svc_path}" else export CCACHE_REMOTE_STORAGE="http://${host_port}${svc_path}" fi - elif [[ -n "${svc_password}" ]]; then + elif [[ "${svc_type}" == "redis" && -n "${svc_password}" ]]; then export CCACHE_REMOTE_STORAGE="redis://${svc_password}@${host_port}|connect-timeout=${CCACHE_REDIS_CONNECT_TIMEOUT}" - else + elif [[ "${svc_type}" == "redis" ]]; then export CCACHE_REMOTE_STORAGE="redis://${host_port}|connect-timeout=${CCACHE_REDIS_CONNECT_TIMEOUT}" + else + display_alert "DNS SRV: unsupported service type — skipping" \ + "_ccache._tcp.${CCACHE_REMOTE_DOMAIN} type=${svc_type}" "wrn" fi - display_alert "DNS SRV: discovered ccache" "$(ccache_mask_storage_url "${CCACHE_REMOTE_STORAGE}")" "info" - return 0 + if [[ -n "${CCACHE_REMOTE_STORAGE}" ]]; then + display_alert "DNS SRV: discovered ccache" "$(ccache_mask_storage_url "${CCACHE_REMOTE_STORAGE}")" "info" + return 0 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/ccache-remote/ccache-remote.sh` around lines 286 - 299, The current branch treats any non-"http" svc_type as Redis and silently builds CCACHE_REMOTE_STORAGE, so typos like svc_type="redsi" go unnoticed; update the conditional logic around svc_type (the block that sets CCACHE_REMOTE_STORAGE and calls display_alert/return) to explicitly check for svc_type == "redis" (in addition to "http"), only construct a Redis URL when svc_type == "redis", and otherwise emit a clear warning/error (use display_alert with the masked URL via ccache_mask_storage_url or a descriptive message) and return a failure code or skip setting CCACHE_REMOTE_STORAGE so unknown types do not silently fall back to Redis.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@extensions/ccache-remote/ccache-remote.sh`:
- Around line 286-299: The current branch treats any non-"http" svc_type as
Redis and silently builds CCACHE_REMOTE_STORAGE, so typos like svc_type="redsi"
go unnoticed; update the conditional logic around svc_type (the block that sets
CCACHE_REMOTE_STORAGE and calls display_alert/return) to explicitly check for
svc_type == "redis" (in addition to "http"), only construct a Redis URL when
svc_type == "redis", and otherwise emit a clear warning/error (use display_alert
with the masked URL via ccache_mask_storage_url or a descriptive message) and
return a failure code or skip setting CCACHE_REMOTE_STORAGE so unknown types do
not silently fall back to Redis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b323d23-6fea-4d5d-8521-0a3f22a077e6
📒 Files selected for processing (2)
extensions/ccache-remote/ccache-remote.shlib/functions/compilation/ccache.sh
igorpecovnik
approved these changes
May 21, 2026
Contributor
|
✅ This PR has been reviewed and approved — all set for merge! |
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
2 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.
Summary
ccache_discover_remote_storageto consume an optionalpassword=TXT record alongside
type=redis(both avahi-browse and DNS-SRV branches).When present, embed it in the resulting
CCACHE_REMOTE_STORAGEURL soccache authenticates against
requirepass-enabled backends discoveredvia mDNS without the user setting the URL manually.
(
[A-Za-z0-9._~-]) before interpolating it into the URL — any URLdelimiter (
/ : @ # ? = + space), the ccache attribute separator|,or the percent-encoding marker
%would break parsing or AUTH-formdisambiguation. Unsafe announces are skipped with a warning so discovery
falls through to the next service (e.g. HTTP).
numfmtindo_with_ccache_statistics()(ccache.sh:103)was passing
ccache_dir_size_diffas a positional argument without anoption-terminator. When ccache cleanups during a build shrink the cache,
the diff is negative and numfmt parses the leading dash as a short-option
chain, exiting with
invalid option -- '2'. The SUBSHELL trap thenaborts the whole build right after a successful u-boot compile. Add
--so the leading dash is treated as part of the numeric argument.
URL form rationale
Plain
redis://<password>@<host>:<port>|...userinfo — no leading colon,no
username:prefix. Two other reasonable candidates break:redis://:<password>@<host>:<port>— ccache 4.9.1 parses but silentlynever sends AUTH; backend rejects every command with
NOAUTH.redis://default:<password>@<host>:<port>— ccache sends theRedis-6 ACL two-arg
AUTH default <password>, which KvRocks<= 2.15.0 rejects with
ERR wrong number of arguments.The chosen form sends the legacy one-arg
AUTH <password>that bothRedis and KvRocks accept.
ccache_mask_storage_urlalready strips the userinfo, so the passwordstays out of build logs:
Test plan
Verified end-to-end with
./compile.sh uboot BOARD=helios64 BRANCH=edge USE_CCACHE=yes ENABLE_EXTENSIONS=ccache-remote ARTIFACT_IGNORE_CACHE=yes SHOW_CCACHE=yesagainst three redis-backend configurations on one LAN host:requirepassand avahi TXTpassword=<secret>— discovery finds it, AUTH succeeds inside docker,93 s build,
Remote ccache result hit=0 miss=75 write=20 err=0.CONFIG SET requirepassand avahi TXT
password=<secret>— same path, 81 s build,Remote ccache result hit=0 miss=53 write=8 err=0.PREFER_DOCKER=no) against KvRocks — discoveryruns from
extension_prepare_config, AUTH succeeds, 1736 writes,err=0. (Surfaced the numfmt bug above, which this PR also fixes.)password=TXT) — backwardscompatibility preserved, URL built without userinfo.
bad:colon) — discoverywarns and skips, falls through to advertised HTTP backend.
Commits
feat(ccache-remote): parse password from DNS-SD TXT for redis backendfix(ccache): pass dir-size diff to numfmt past '--' to allow negativesSummary by CodeRabbit
New Features
Bug Fixes