feat(fleet): flag a dial-back service whose advertised address the machine no longer has - #320
Conversation
…chine no longer has The check that catches a stale advertised IP the day the ISP re-provisions it, instead of days later via the provider's offline emails. - advertised_address_env in the service schema: the ONE env var holding the address the network dials the service back at. The worker copies that single variable's value into heartbeat container entries — never any other env, which holds credentials — at the cost of one inspect call per declaring container. storj declares ADDRESS. - The hub compares it against that worker's detected egress IP in producer state: a stale public literal, a private literal, and a hostname resolving away from the egress are FAILING findings that outrank PRODUCING (storage and held components keep ticking while inbound work is dead). Undetected egress, transient DNS trouble, and unreported addresses are NO CLAIM — a wrong 'your address is stale' sends the operator to fix DNS that is fine. Only a definitive NXDOMAIN counts as a resolution verdict. - The mismatch reason names its assumption (inbound rides the machine's default egress) so a deliberate second-WAN forward can be read and dismissed rather than mistrusted. Tests: decision-table over the verdict (findings AND no-claim rows), the hub helper with an injectable resolver, worker-side extraction proving the declared var and ONLY the declared var leaves the container inspect, a catalog guard that every declared advertised_address_env names a real env var, and a control that undeclared services carry no key at all.
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds catalog-controlled advertised dial-back addresses, propagates them through container status records, validates them against worker egress addresses, and reports confirmed mismatches in producer state. ChangesAdvertised Address Validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 95.57% 95.51% -0.06%
==========================================
Files 51 51
Lines 6685 6828 +143
==========================================
+ Hits 6389 6522 +133
- Misses 296 306 +10
🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
|
… positives Round 2 from independent review: - getaddrinfo raises UnicodeError (a ValueError, NOT an OSError) for an IDNA-invalid label; uncaught it escaped to the route umbrella and silently zeroed the log-signal scan sharing the try block — the address check disabling the very detection (#318) it complements. Caught, and the call site got its own suppress guard so no future failure mode can shadow log signals again. Worker-supplied hostnames are shape-validated before the resolver ever sees them, and resolutions are memoized 60s so a blackholed resolver costs one executor thread per window. - Judge only RUNNING containers on the node the caller asked about: an exited container elsewhere in the fleet carries its last run's env, and judging it produced findings about the wrong machine. - Cross-family comparisons are silence: the egress detectors are dual-stack, so a v6 egress against a v4 literal (or a v6-only DDNS name against v4 egress) says nothing about staleness. Same-family filtering before the membership check. - Resolved IPs are redacted to public-only before being echoed: they originate from a worker-supplied name, and repeating a private answer would let a rogue worker read the hub's internal DNS view. - A dangling 'host:' colon is a typo, stripped instead of earning a confident NXDOMAIN about a name never looked up (bare v6 keeps its trailing colons). - advertised_address_env may never name a secret-flagged var: CI guard in the catalog tests plus a runtime backstop in the worker. - External (image-matched) containers now carry advertised_address too — running a storagenode BEFORE installing CashPilot is the common storj adoption path, and those nodes were blind spots. - Stale-egress caveat in both mismatch messages (worker egress readings are cached up to an hour; the first hour after an IP change can read stale and self-heals). Tests: resolver three-valued contract on the REAL function (NXDOMAIN, EAI_AGAIN, timeout, UnicodeError), family-guard decision rows, private-IP redaction, running/worker_id filtering, secret backstop, external-node coverage.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_orchestrator_coverage.py (1)
439-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the imported label constants here.
_mock_containerat Line 38 usesLABEL_SERVICEandLABEL_MANAGED, but_labeledhardcodes"cashpilot.service"and"cashpilot.managed". If a constant value changes, this helper silently produces slug"unknown"and the advertised-address assertions still pass.♻️ Proposed change
- c.labels = {"cashpilot.managed": "true", "cashpilot.service": slug} + c.labels = {LABEL_MANAGED: "true", LABEL_SERVICE: slug}🤖 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 `@tests/test_orchestrator_coverage.py` at line 439, Update the label assignment in _labeled to use the imported LABEL_MANAGED and LABEL_SERVICE constants instead of hardcoded label keys, preserving the existing values and advertised-address assertions.app/orchestrator.py (1)
605-624: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the inspect failure instead of swallowing it silently.
Both
except Exceptionblocks returnNonewith no record. The surrounding status loops log warnings for every other Docker failure (Lines 682, 690), so an inspect error here is the only failure in this file that leaves no trace. A permanently failing inspect then looks identical to "the service does not declare an address", and the address check stays silent forever with nothing to debug.Log the exception, not the env value, so no credential can reach the log.
♻️ Proposed logging
try: env = (container.client.api.inspect_container(container.id).get("Config") or {}).get("Env") or [] - except Exception: + except Exception as exc: + logger.debug("Could not inspect %s for its advertised address: %s", getattr(container, "short_id", "?"), exc) return NoneAs per path instructions, "Check for proper error handling and logging".
🤖 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 `@app/orchestrator.py` around lines 605 - 624, Update the exception handling around service lookup and container inspection in the address-resolution flow to log each caught exception before returning None. Use the existing module logging pattern, include operation context and the exception object, and never log the inspected environment or credential values.Source: Path instructions
app/main.py (1)
2816-2823: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the suppressed address-check failure
_advertised_address_mismatch()is guarded bycontextlib.suppress(Exception), so an exception makesaddress_mismatchremain unset and leaves no diagnostic. If the check fails, log it inside the guard; do not suppress the entirecontextlibimport, which is already present.🤖 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 `@app/main.py` around lines 2816 - 2823, Update the _advertised_address_mismatch call within the running guard to catch the exception explicitly and log the failure with the existing logger, while preserving suppression so the surrounding verdict continues and address_mismatch remains safely unset. Keep the existing contextlib import and do not suppress the entire import.Source: Path instructions
🤖 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 `@app/main.py`:
- Around line 2724-2753: Update _resolve_advertised_host to purge expired
_RESOLVE_CACHE entries before storing a new result, then enforce a fixed maximum
cache size by evicting older entries when the limit is exceeded. Keep valid
cached lookups and the existing resolution result semantics unchanged, and
define or reuse a clear cache-size limit alongside _RESOLVE_CACHE_TTL.
- Around line 2696-2716: Update the advertised-address evaluation around
reported and egress.advertised_address_verdict so every running candidate with
an advertised address is assessed rather than stopping at the first match, while
preserving the worker_id filtering. Include each candidate’s _node value in the
verdict reason so fleet-wide failures identify the affected node, and return the
combined per-candidate verdicts using the existing verdict structure.
In `@tests/test_producer_state.py`:
- Around line 517-527: Update the test helper _resolve to clear
main._RESOLVE_CACHE before invoking _resolve_advertised_host, then use a fixed
host value instead of deriving the hostname from id(side_effect). Keep the
existing wait_for patching and asyncio execution unchanged.
---
Nitpick comments:
In `@app/main.py`:
- Around line 2816-2823: Update the _advertised_address_mismatch call within the
running guard to catch the exception explicitly and log the failure with the
existing logger, while preserving suppression so the surrounding verdict
continues and address_mismatch remains safely unset. Keep the existing
contextlib import and do not suppress the entire import.
In `@app/orchestrator.py`:
- Around line 605-624: Update the exception handling around service lookup and
container inspection in the address-resolution flow to log each caught exception
before returning None. Use the existing module logging pattern, include
operation context and the exception object, and never log the inspected
environment or credential values.
In `@tests/test_orchestrator_coverage.py`:
- Line 439: Update the label assignment in _labeled to use the imported
LABEL_MANAGED and LABEL_SERVICE constants instead of hardcoded label keys,
preserving the existing values and advertised-address assertions.
🪄 Autofix
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 Plus
Run ID: 70fad649-c7fd-427f-a698-6dd12403de17
📒 Files selected for processing (9)
app/egress.pyapp/main.pyapp/orchestrator.pyapp/producer_state.pyservices/_schema.ymlservices/storage/storj.ymltests/test_egress.pytests/test_orchestrator_coverage.pytests/test_producer_state.py
CodeRabbit round on the review commit: - _RESOLVE_CACHE keys are worker-supplied hostnames and nothing ever evicted them — steady memory growth for the hub's lifetime. Expired entries are purged on every write and the table is capped at 256, oldest-first. - The mismatch reason said 'this machine' without saying WHICH machine — a fleet finding nobody can act on. The worker's node name is prefixed into the reason. - The resolver tests keyed cache uniqueness on id(), which CPython reuses; fixed host + explicit cache clear instead.
2 participants
Why
Fix 2 of the storj dial-back incident review (follows #318). The incident: a silent ISP re-provision left the node advertising a dead literal IP — satellites dialled it for days while the container looked healthy. #318 added the log-pattern alarm; this PR adds the check that catches the mismatch the day it happens, from data the fleet already collects.
What
docker.advertised_address_env— the ONE env var holding the address the network dials the service back at. storj declaresADDRESS.advertised_address— never any other env (env holds credentials); one inspect call per declaring container, absent key when undeclared/unset.producer-state): compares it against that worker's detected egress IP. Findings (FAILING, outranking PRODUCING since storage/held income keeps ticking while inbound is dead): stale public literal, private literal, hostname resolving away from the egress, definitive NXDOMAIN. No claim (silent): undetected egress, transient DNS trouble, unreported address — a wrong "your address is stale" sends the operator to fix DNS that is fine. The reason text names its assumption so a deliberate second-WAN forward can be read and dismissed.Testing
advertised_address_verdictcovering findings and every no-claim row (the strongest finding input must stay silent when egress is undetected).advertised_address_envnames a real env var in that service's own env list.Summary by CodeRabbit
New Features
Bug Fixes