Skip to content

feat: make axum host configurable via manifest and env vars#231

Open
ChristianPavilonis wants to merge 3 commits intomainfrom
feature/configureable-axum-host
Open

feat: make axum host configurable via manifest and env vars#231
ChristianPavilonis wants to merge 3 commits intomainfrom
feature/configureable-axum-host

Conversation

@ChristianPavilonis
Copy link
Copy Markdown
Contributor

Summary

  • Add host and port fields to ManifestAdapterDefinition so adapters can declare bind addresses in edgezero.toml under [adapters.<name>.adapter]
  • Parse adapter.host from axum.toml with IP validation, defaulting to 127.0.0.1
  • run_app() now reads host/port from manifest config with EDGEZERO_HOST / EDGEZERO_PORT env var overrides
  • CLI fallback dev server and run_cargo subprocess both respect the same env vars
  • Invalid values produce warnings instead of silently falling back to defaults

Configuration Precedence

  1. EDGEZERO_HOST / EDGEZERO_PORT environment variables (highest)
  2. Manifest value (edgezero.toml or axum.toml)
  3. Default: 127.0.0.1:8787

Usage

# edgezero.toml
[adapters.axum.adapter]
host = "0.0.0.0"
port = 3000
# axum.toml
[adapter]
host = "0.0.0.0"
port = 3000
# Or via environment
EDGEZERO_HOST=0.0.0.0 EDGEZERO_PORT=3000 cargo run -p my-adapter

Files Changed

File Change
crates/edgezero-core/src/manifest.rs Add host + port to ManifestAdapterDefinition + tests
crates/edgezero-adapter-axum/src/cli.rs Parse host from axum.toml, propagate via env vars to subprocess
crates/edgezero-adapter-axum/src/dev_server.rs resolve_addr() with manifest + env var precedence + warnings
crates/edgezero-adapter-axum/src/lib.rs Export resolve_addr
crates/edgezero-cli/src/dev_server.rs Fallback path checks env vars with warnings

Copy link
Copy Markdown
Contributor

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Summary

Well-structured feature that adds configurable host/port binding for the Axum adapter with a clean precedence model (env vars → manifest → defaults). CI gates all pass. One behavioral inconsistency around port 0 validation needs fixing.

😃 Praise

  • Clean configuration precedence design with good separation of concerns between CLI, adapter, and manifest layers
  • Excellent test isolation: using resolve_addr_from_parts with explicit parameters avoids env var races in parallel test runners
  • Thorough test coverage for the happy paths in all three layers (manifest, CLI, adapter)

Findings

Blocking

  • 🔧 Port 0 inconsistency: cli.rs rejects port 0 from axum.toml but resolve_addr_from_parts and resolve_dev_addr accept EDGEZERO_PORT=0 and manifest port 0, causing random OS port binding (dev_server.rs:188)

Non-blocking

  • 🤔 resolve_addr public API surface: exported publicly but only used internally by run_app — consider pub(crate) or documenting the external use case (lib.rs:22)
  • 🤔 host field as String, not IpAddr: accepts any string at parse time, IP validation deferred to adapter runtime (manifest.rs:285)
  • 🤔 Duplicated resolution logic: resolve_dev_addr in edgezero-cli duplicates resolve_addr_from_parts logic with different logging (cli/dev_server.rs:88)
  • 🤔 Inconsistent error strictness: invalid host in axum.toml → hard error; invalid host in edgezero.toml → warning + fallback. Reasonable per context but worth documenting.
  • Incidental #[derive(Debug)] on AxumProject — harmless (cli.rs:143)

📌 Out of Scope

  • Missing tests for invalid env var fallback paths (e.g. EDGEZERO_HOST="not-an-ip", EDGEZERO_PORT="abc") — good to add but not blocking
  • Two config sources (axum.toml vs edgezero.toml) for the same adapter is pre-existing complexity worth documenting

CI Status

  • fmt: ✅ PASS
  • clippy: ✅ PASS
  • tests: ✅ PASS (322 passed)

Add host/port configuration to the axum adapter so the bind address
can be set to 0.0.0.0 or any other IP instead of the hardcoded
127.0.0.1:8787 default.

Configuration precedence (highest wins):
1. EDGEZERO_HOST / EDGEZERO_PORT environment variables
2. Manifest value (edgezero.toml or axum.toml)
3. Default: 127.0.0.1:8787

Invalid values produce warnings instead of silently falling back to
defaults. The CLI subprocess now receives EDGEZERO_HOST/EDGEZERO_PORT
so resolved values propagate to the running server.
…dr visibility

- Extract resolve_bind_addr into edgezero_core::addr so both the axum
  adapter and the CLI dev server share a single code path for resolving
  bind addresses from env vars and config.
- Reject port 0 (random OS port) with a warning and fallback to 8787,
  matching the existing cli.rs validation.
- Narrow resolve_addr to pub(crate) since it has no external consumers.
@ChristianPavilonis ChristianPavilonis force-pushed the feature/configureable-axum-host branch from 5f95ce3 to 9561502 Compare March 31, 2026 16:07
Copy link
Copy Markdown
Contributor

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Summary

Well-structured feature that adds configurable host/port binding with clean precedence logic (env > manifest > defaults). The centralised addr.rs module and backward-compatible manifest changes are solid. Main concern is a validation inconsistency between the axum.toml path and the shared resolve_bind_addr path.

😃 Praise

  • Extracting resolve_bind_addr as a pure function with comprehensive tests (including IPv6, port 0, partial overrides) is excellent — avoids env var race conditions in parallel tests
  • Backward-compatible manifest extension using Option + #[serde(default)] is the right pattern
  • Port 0 rejection in addr.rs prevents confusing random-port behavior

Findings

Blocking

  • Two separate validation paths: read_axum_project (axum.toml) hard-errors on invalid IP, while resolve_bind_addr warns and falls back. Same user mistake → different behavior depending on config source. See inline comment on cli.rs.

Non-blocking

  • 🤔 pub mod addr API surface: resolve_bind_addr is now part of edgezero_core's public API. Confirm this is intentional vs pub(crate). See inline on lib.rs.
  • 🤔 Silent warning drop in CLI fallback: log::warn! in resolve_bind_addr may fire before the logger is initialised in the CLI fallback path, silently swallowing invalid-value warnings. See inline on addr.rs.
  • 🏕 Magic 8787 in cli.rs: The default port 8787 is now defined as DEFAULT_PORT in edgezero_core::addr but remains hardcoded in read_axum_project (None => 8787). Consider using the constant or a local const to keep values in sync.
  • 🌱 Late host validation: Manifest accepts arbitrary strings for host, validated only at bind time. Defensible for WASM compat but worth a doc comment. See inline on manifest.rs.

CI Status

  • fmt: ✅ PASS
  • clippy: ✅ PASS
  • tests: ✅ PASS (322 tests)

- resolve_bind_addr now returns Result<SocketAddr, String> instead of
  silently falling back on invalid values (addresses silent warning drop)
- read_axum_project delegates host/port validation to resolve_bind_addr,
  eliminating the inconsistency between axum.toml and edgezero.toml paths
- DEFAULT_HOST and DEFAULT_PORT are now pub const for cross-crate reuse
- CLI fallback path surfaces errors via eprintln instead of dropped log::warn
- Added doc comment on manifest host field explaining late IP validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants