feat(relay-server): report configuration state instead of failing silently - #294
feat(relay-server): report configuration state instead of failing silently#294rabbitson87 wants to merge 8 commits into
Conversation
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe relay now provides configuration inspection, feature-state reporting, environment metadata, invalid-value detection, generated references, startup validation, and documented deployment settings. ChangesRelay configuration tooling
Sequence Diagram(s)sequenceDiagram
participant Operator
participant relay_server
participant resolveRelayServerConfig
participant EnvVars
participant evaluateFeatures
Operator->>relay_server: Run config with format and env-file
relay_server->>resolveRelayServerConfig: Resolve relay settings
resolveRelayServerConfig->>EnvVars: Read metadata and parse issues
relay_server->>evaluateFeatures: Evaluate relay capabilities
evaluateFeatures-->>relay_server: Return feature states and diagnostics
relay_server-->>Operator: Print configuration report or generated reference
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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: 3
🤖 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 `@cmd/relay-server/config.go`:
- Around line 153-166: Update ensGaslessFeature to reuse the DNS provider
validation performed by acmeFeature, rather than only checking whether
ACMEDNSProvider is non-empty. When that provider is blocked, propagate the
blocked state and missing requirement; only report ENS gasless as enabled when
the provider validation succeeds.
- Around line 362-405: Update writeConfigReport to report resolved effective
values rather than raw envFileEntry.Value: derive values and sources from cfg
and the resolver metadata, including utils.EnvVar.SetBy, so ignored aliases are
not displayed as active. Always render a Keys section for every registered relay
key, including when entries is empty and values come from the process
environment, while preserving unknown-key reporting for supplied entries.
In `@cmd/relay-server/main.go`:
- Around line 79-80: Update resolveRelayServerConfig to clear the process-global
utils.EnvIssues registry at the start of each resolution pass, before any
environment validation or issue registration occurs. Preserve accumulation of
all issues generated during that pass so envIssueError() evaluates only the
current configuration attempt.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b146079f-a361-4050-8c4b-e2ed71c09adb
📒 Files selected for processing (7)
.env.exampleMakefilecmd/relay-server/config.gocmd/relay-server/envcatalog.gocmd/relay-server/main.godocs/src/routes/configuration/+page.mdutils/cmd.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
cmd/relay-server/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
cmd/relay-server/**/*.go: All JSON control-plane responses must use theAPIEnvelopewrapper:{ ok: bool, data?: any, error?: { code, message } }. Write responses throughwriteAPIData(),writeAPIOK(), orwriteAPIError(). Exceptions: admin HTML pages, tunnel script/binary responses, and other non-JSON endpoints.
Relay-local frontend asset paths stay local tocmd/relay-server. Filenames likefavicon.svgorportal.jpgare frontend serving details, not cross-package API contract.
Files:
cmd/relay-server/envcatalog.gocmd/relay-server/main.gocmd/relay-server/config.go
{cmd,sdk}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not import
portalfromcmd/*orsdkjust to reach shared DTOs or constants. Shared public shapes belong intypes/.
Files:
cmd/relay-server/envcatalog.gocmd/relay-server/main.gocmd/relay-server/config.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Imports follow the order: stdlib -> external -> internal (blank-line separated), local prefixgosuda.org/portal/v2.
Useerrgroup.GroupoverWaitGroup. Useerrgroup.SetLimitfor bounded work. Usecontext.WithTimeoutovertime.Afterin loops. No barego func()without owned lifecycle.
Prefer directory-scoped operations when touching filesystem trees in Go code.
Files:
cmd/relay-server/envcatalog.gocmd/relay-server/main.goutils/cmd.gocmd/relay-server/config.go
🪛 checkmake (0.3.2)
Makefile
[warning] 66-66: Target body for "check-env-example" exceeds allowed length of 5 lines (25).
(maxbodylength)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 16-16: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 30-30: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 47-47: [UnorderedKey] The MAX_PORT key should go before the MIN_PORT key
(UnorderedKey)
[warning] 49-49: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 63-63: [UnorderedKey] The TCP_ENABLED key should go before the UDP_ENABLED key
(UnorderedKey)
[warning] 78-78: [UnorderedKey] The TRUSTED_PROXY_CIDRS key should go before the TRUST_PROXY_HEADERS key
(UnorderedKey)
[warning] 84-84: [UnorderedKey] The X402_PAY_TO key should go before the X402_TESTNET key
(UnorderedKey)
[warning] 90-90: [UnorderedKey] The PPROF_ADDR key should go before the PPROF_ENABLED key
(UnorderedKey)
[warning] 93-93: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🔇 Additional comments (7)
utils/cmd.go (1)
12-94: LGTM!Also applies to: 100-158, 210-228
cmd/relay-server/envcatalog.go (1)
1-74: LGTM!.env.example (1)
1-145: LGTM!cmd/relay-server/config.go (1)
22-151: LGTM!Also applies to: 169-285, 287-360, 430-593, 595-681
cmd/relay-server/main.go (1)
27-30: LGTM!Makefile (1)
1-1: LGTM!Also applies to: 19-20, 57-95
docs/src/routes/configuration/+page.md (1)
29-88: LGTM!
gosunuts
left a comment
gosunuts
left a comment
There was a problem hiding this comment.
The configuration report can currently diverge from the deployment it is intended to validate in two cases. Please address the inline findings before merging.
| return fmt.Errorf("read env file: %w", err) | ||
| } | ||
| for _, entry := range loaded { | ||
| if err := os.Setenv(entry.Name, entry.Value); err != nil { |
There was a problem hiding this comment.
--env-file is documented as being inspected instead of the process environment, but this only overwrites keys present in the file. Any relay variable absent from the file remains inherited, and a higher-priority alias from the shell can override a value that is present (for example, process AWS_REGION overrides file AWS_DEFAULT_REGION). This can make the report validate a different configuration from the one Compose will deploy. Please resolve against an isolated environment—e.g. clear/restore all registered names around this pass, or pass an explicit environment map to the resolver.
| return f | ||
| } | ||
|
|
||
| f.State, f.By = stateEnabled, "ACME_DNS_PROVIDER="+provider |
There was a problem hiding this comment.
This reports ACME as enabled whenever the provider/credentials look valid, even when PORTAL_URL has a local-only host. The runtime does not enable managed ACME in that case: acme.NewManager returns before creating a DNS provider for local hosts, and ENS status is disabled as well. With PORTAL_URL=https://localhost, the report therefore says ACME (and potentially ENS gasless) is enabled while neither automation path runs. Please include the local-host condition in both feature evaluations so the report reflects runtime state.
|
Both findings were reproducible and are fixed in 1ecc3b0. Thanks — they were
I took the clear/restore option rather than threading an environment map Inspecting the process environment (no Local-only host. Confirmed against
Tests. Added |
…ently Configuration problems in a relay deployment are hard to see. A renamed or mistyped variable looks set but is never read, and a value that cannot be parsed falls back to its default without a word. Both failures look identical to a working deployment until something is missing much later. Add a config subcommand and a startup report that make the effective configuration visible, and stop the silent fallbacks that hid it. - utils: record the env name, flag, usage text and default behind every Flag*Env call. The helpers already received all of it and discarded it, so the descriptions keep a single owner in the flag definitions. - utils: a value that is present but unusable is recorded as an issue rather than swallowed. DISCOVERY=yes is now a startup error instead of a silent false, and an out-of-range port is reported rather than clamped. - relay-server config: prints every key with its effective value, its source, the component that reads it, and its usage text; names keys nothing reads with a nearest-match suggestion; and evaluates each feature. Runs without starting the server, so a deployment can be checked before it is applied. - Startup logs the same report. Each feature is enabled, disabled or blocked: "you switched this off" and "you switched this on but it cannot run" were previously indistinguishable, and only the second is a misconfiguration. Blocked and unprotected states log at warning level. - envcatalog: keys read by Compose, the Google Cloud SDK or the image are catalogued so they are not reported as unknown, and keys the bundled topology pins are marked so documenting them cannot invite an override. - .env.example is grouped by what an operator actually has to decide, and make check-env-example fails when a flag is added without documenting it in .env.example and the configuration reference. The blocked conditions are not new rules; they are failure modes already described in flag usage strings and the docs, moved to startup where they are seen.
… in sync Three problems found in review. The report printed the raw text of each env-file line rather than the value the flag resolved to. A key overridden by a higher-priority name, or an alias that was never consulted, read as though it were in effect — the exact confusion the report exists to remove. It also skipped the key listing entirely when no env file was given, so a process-environment deployment got no listing at all. The registry now carries the resolved value, and every relay key is listed with it plus where it came from: the env file, the process environment, or the default. When an alias supplied the value the alias is named, so "I set AWS_REGION, why is it different?" is answered by seeing AWS_DEFAULT_REGION was consulted first. Keys owned by another component are still only shown when supplied, since the relay has no effective value for them. ens-gasless repeated only the "is a provider set" half of the ACME check, so it reported enabled while acme was blocked on a missing credential. It now defers to acmeFeature and propagates the reason. The registry is process-global, and the config subcommand resolves a second time after loading an env file. Issues accumulated across passes, so a stale one could fail a configuration that no longer contained it. Resolution now starts from an empty registry.
…checks Two cases where the report could validate a configuration different from the one that would actually run. `--env-file` is documented as inspecting a file instead of the process environment, but it only set the file's own keys. A relay variable absent from the file stayed inherited from the shell, and a higher-priority alias in the shell beat a value the file did supply — process AWS_REGION over file AWS_DEFAULT_REGION. Compose passes only the file, so the report could describe a different deployment than the one being checked. The file is now the whole environment for that pass: every name the deployment understands is cleared first, the file applied, and the previous environment restored afterwards. Inspecting the process environment, where isolation is not the intent, is unchanged. ACME was reported as enabled whenever the provider and credential looked valid, including when PORTAL_URL had a local-only host. acme.NewManager returns before it builds a DNS provider for a local base domain, so managed issuance never runs in that case and a development certificate is used instead. The report now treats a configured provider on a local host as blocked, which is what asking for automation that cannot start should look like. ens-gasless already defers to acmeFeature, so it inherits the state and the reason. Both are the mismatch this report exists to surface, so both are covered by tests that fail when either fix is reverted.
The Keys section runs every value through displayValue, which prints <set> for a name that looks like a credential. The Invalid values section printed the raw text instead. Nothing leaks today: an issue is only recorded when a boolean or integer fails to parse, and no credential is either. But the two lists are read the same way, and a report that masks a secret in one place and prints it in another is one numeric credential away from pasting it into a bug thread.
The documented check reads the file from stdin, but `docker compose run` asks for a TTY unless told not to. Older Compose versions then fail with "the input device is not a TTY" and the operator never sees the report -- which is the one command this feature exists to offer. Newer versions detect the redirect and work either way, so the omission survives a local test and only shows up on the server.
0e9e8ae to
e78c232
Compare
gosunuts
left a comment
gosunuts
left a comment
There was a problem hiding this comment.
The flag-derived EnvVar{Value, SetBy, Usage, Default} metadata and strict bool/int parsing are worth keeping, and the earlier shell-environment leakage and local-host ACME mismatches are fixed at this head. I still do not think this is mergeable yet: the command can report a different configuration from both Compose and the runtime, and the env parser introduces another silent typo path. Please resolve the first two inline blockers and rebase onto current main (mergeable: false) before merging. During that rebase, preserve the newer relay/tunnel ECH wording that main added to .env.example; this branch rewrites that section extensively.
Architecturally, config should display validation owned by the runtime rather than grow into a competing configuration model. Reusing pure normalization/validation from portal and portal/acme should also reduce much of the new config.go surface.
| // the file would stay inherited from the shell, and a higher-priority alias in | ||
| // the shell would beat a value the file does supply — process AWS_REGION over | ||
| // file AWS_DEFAULT_REGION, for instance. Either way the report would describe a | ||
| // configuration different from the one Compose is going to deploy, which is the |
There was a problem hiding this comment.
[P1] Do not claim this pass reproduces the Compose deployment
Isolation fixes shell leakage, but after clearing the container environment this code calls resolveRelayServerConfig(nil), so every key omitted from the file gets the relay binary default. Compose applies a different layer first: for example, it injects MIN_PORT=40000, MAX_PORT=40009, and IDENTITY_PATH=/portal-certs, while the binary defaults are 0, 0, and ./.portal-certs. Consequently an env file containing only PORTAL_URL=https://relay.example.com and UDP_ENABLED=true is reported as udp-transport blocked, although docker compose up supplies 40000-40009 and enables it. Either scope --env-file explicitly to the relay binary's environment semantics and remove the Compose/deployment claims, or inspect Compose's fully interpolated container environment. Reimplementing Compose defaults here would create another configuration engine.
| f.State, f.By = stateDisabled, "DISCOVERY=false" | ||
| return f | ||
| } | ||
| host := portalURLHost(cfg.PortalURL) |
There was a problem hiding this comment.
[P1] Reuse runtime normalization before reporting this feature as enabled
This checks only the parsed hostname. PORTAL_URL=http://relay.example.com therefore reports discovery as enabled, but portal.NewServer calls utils.NormalizeRelayURL and rejects the non-HTTPS URL. BOOTSTRAPS has the same split: this report only SplitCSVs/counts entries while the runtime requires NormalizeRelayURLs to succeed. Similar provider and port rules are duplicated in this file and dnsProviderCredential separately duplicates the provider set owned by portal/acme. Please have the report consume shared pure runtime normalization/validation results instead of reconstructing feature validity here; otherwise startup can log enabled immediately before rejecting the same configuration.
| line = strings.TrimPrefix(line, "export ") | ||
| name, value, found := strings.Cut(line, "=") | ||
| if !found { | ||
| continue |
There was a problem hiding this comment.
[P2] Reject malformed non-comment lines instead of silently dropping them
A typo such as DISCOVERY true reaches this branch and disappears, causing discovery to fall back to its default without any warning. An empty assignment name is discarded the same way below. That recreates the silent-misconfiguration failure this PR is intended to eliminate. Track the scanner line number and return a filename:line parse error for every non-empty, non-comment line that is not a valid assignment.
…ot asked
Three findings from review, all cases where the report was confident about
something it had not actually checked.
`--env-file` was documented as showing what a deployment will run, but it
resolves the file against the relay binary's defaults. Compose supplies its
own first: a file carrying only PORTAL_URL and UDP_ENABLED=true reported
`udp-transport blocked` while `docker compose up` would give it
MIN_PORT=40000 and enable it. Reimplementing Compose's defaults here would
build a second configuration engine, so the claim is narrowed instead. The
accurate check needs no new code — running the subcommand inside the
container lets Compose build the environment first:
docker compose run --rm -T portal config
That is now what .env.example, the configuration page and the command's own
usage recommend, and a file-scoped report says in its header which of the
two questions it answered.
`discoveryFeature` inspected only the parsed hostname, so
PORTAL_URL=http://relay.example.com reported enabled and portal.NewServer
rejected the same value seconds later — the exact divergence this feature
exists to remove. It now calls utils.NormalizeRelayURL and
utils.NormalizeRelayURLs, the same normalization the server applies, rather
than holding a second opinion about it. Bootstraps are counted after
normalization for the same reason.
loadEnvFile silently skipped any line without an `=`. `DISCOVERY true`
vanished and discovery reported its default with nothing to explain why,
which is the silent misconfiguration this command was written to expose.
Malformed lines now fail with file:line.
Also keys dnsProviderCredential by acme's exported Type* constants rather
than repeating the provider names, so acme stays the one place that decides
what is supported and this map only adds the credential each one needs.
|
All three addressed in
I took the first option. Reimplementing Compose's defaults here would build a second configuration engine, and the accurate check turns out to need no new code at all — running the subcommand inside the container lets Compose build the environment first: docker compose run --rm -T portal configThat is now what
On Malformed env-file lines. Fixed with Empty assignment names are rejected the same way. Comments, blanks and Five tests pin the three: malformed line and empty name both rejected with the line number, comments/blanks still accepted, non-https |
main gained an embedded authoritative DNS server that is now the default DNS provider, which changes what this report has to say. An unset ACME_DNS_PROVIDER no longer means "manual certificates". It selects the embedded server, so reporting that as disabled would describe a relay that is in fact serving its own zone and answering ACME challenges from it. acme now reports embedded as enabled and says what it needs: the NS delegation and 53/tcp+udp, with manual certificate files still taking over when present. ENS gasless gains a blocked state for the same reason. The embedded server cannot manage zone DNSSEC yet, and it is exactly what an operator gets by turning ENS on and changing nothing else, so that combination now says so instead of claiming to work. .env.example keeps this branch's four-tier layout with the new keys folded in: ACME_DNS_PROVIDER=embedded as the documented default, EMBEDDED_DNS_PORT in the provider section beside the credentials it replaces, and the ENS entry corrected to name the DNSSEC requirement rather than "provider must be set". The startup log keeps the feature report rather than main's flat field dump; that substitution is the point of the branch.
The flag that binds CLOUDFLARE_TOKEN to relayServerConfig.CloudflareToken
is gone from main. The struct field is still there and is still passed to
acme.Config, so nothing fails to compile — it is simply always empty, and
a relay configured with ACME_DNS_PROVIDER=cloudflare starts, reports
itself configured, and then fails DNS-01 with "cloudflare token is
required". The token appears in .env.example, docker-compose.yml and three
documentation pages; it appears in no Go file.
Every other provider credential is still wired, so this is Cloudflare
alone.
This branch's own report is what surfaced it, from two directions at once:
UNKNOWN 1 key(s) are not read by any component and are silently ignored:
CLOUDFLARE_TOKEN did you mean HCLOUD_TOKEN?
! acme blocked ACME_DNS_PROVIDER=cloudflare
missing: CLOUDFLARE_TOKEN is empty
Kept as its own commit rather than folded into the merge, so it can be
taken separately or ahead of the rest.
2 participants
Problem
Configuration problems in a relay deployment are hard to see.
A renamed or mistyped variable looks set but is never read. A value that cannot
be parsed falls back to its default without a word —
DISCOVERY=yessilentlybecomes
false. And a feature that was switched off looks exactly like one thatwas switched on but cannot run: both log as
false.All three failures are indistinguishable from a working deployment until
something turns out to be missing much later.
What this adds
A
configsubcommand and a startup report that make the effectiveconfiguration visible, plus removal of the silent fallbacks that hid it.
relay-server configRuns without starting the server, so a deployment can be checked before it is
applied:
Every line carries the value, where it came from, which component reads it, and
the flag's own usage text. Keys nothing reads are named with a nearest-match
suggestion.
Startup report
The same report is logged at startup. Each feature is
enabled,disabledorblocked, and only the last two are worth attention:disabledmeans you turned it off and nothing is wrong.blockedmeans youturned it on and it cannot run. Those were previously the same line.
The blocked conditions are not new rules. They are failure modes already
described in flag usage strings and the docs, moved to startup where they are
seen.
Values that cannot be parsed now fail
DISCOVERY=yesis a startup error instead of a silentfalse, and anout-of-range port is reported rather than clamped.
How it stays correct
The descriptions are not a second list.
utils.StringFlagEnvand friendsalready received the env name, flag name, usage text and default, and discarded
all of it; they now record it. The report, the generated reference, and the docs
check all read from the flag definitions.
make check-env-examplefails when a flag is added without documenting it in.env.exampleand the configuration reference. Keys the bundled topology pins(
API_PORT,SNI_PORT,TZ) are marked inenvcatalog.goand excluded onpurpose, so documenting them cannot invite an override that breaks the wiring.
Keys read by Compose or the Google Cloud SDK rather than the relay are
catalogued so they are never reported as unknown.
Verification
make vet,make lint,make test,gofmtcleanmake check-env-examplepasses, and fails as intended when a key is removedfrom either file
.env.examplereports 0 unknown keysADMIN_WALLETS→ suggestsADMIN_TOKEN,BOOTSTRAP_URIS→suggests
BOOTSTRAPSDISCOVERY=yesrefuses to startdocker compose build portal), including thefrontendandlanding-pagefeatures new to the single-container topologyNot included
An earlier draft also reworked the bundled nginx edge and the multi-container
Compose topology. That work targets files this branch's base has removed, and
argues against the direction the deployment guide now documents, so it is left
out.