Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions crates/edgezero-adapter-fastly/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ const FASTLY_INSTALL_HINT: &str = "install the Fastly CLI (https://www.fastly.co

/// The config store the runtime opens for `EDGEZERO__*` overrides. Compute@Edge
/// has no process env, so the runtime reads its config-store KEY selector from
/// here (see `env_config_from_runtime_dictionary` in lib.rs).
/// here (see `runtime_env_config` in lib.rs).
const RUNTIME_ENV_STORE: &str = "edgezero_runtime_env";

/// Base name of the staging twin of [`RUNTIME_ENV_STORE`]. The actual store is
Expand Down Expand Up @@ -546,12 +546,11 @@ impl Adapter for FastlyCliAdapter {
// Store named `edgezero_runtime_env`. Compute@Edge has no
// process env, so `EDGEZERO__STORES__CONFIG__<ID>__KEY` and
// similar overrides have to come from a platform Config Store
// the runtime opens by name (see
// `env_config_from_runtime_dictionary` in lib.rs). Provision
// owns the store creation alongside the operator's declared
// stores so the runtime override path is wired correctly out
// of the box; if the store already appears in
// `[setup.config_stores.edgezero_runtime_env]`, skip.
// the runtime opens by name (see `runtime_env_config` in
// lib.rs). Provision owns the store creation alongside the
// operator's declared stores so the runtime override path is
// wired correctly out of the box; if the store already appears
// in `[setup.config_stores.edgezero_runtime_env]`, skip.
let runtime_env_kind = "config";
let runtime_env_name = "edgezero_runtime_env";
if dry_run {
Expand Down
98 changes: 78 additions & 20 deletions crates/edgezero-adapter-fastly/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ pub mod response;
pub mod secret_store;

#[cfg(feature = "fastly")]
use edgezero_core::app::{Hooks, StoresMetadata};
use edgezero_core::app::Hooks;
#[cfg(any(feature = "fastly", test))]
use edgezero_core::app::StoresMetadata;
#[cfg(feature = "fastly")]
use edgezero_core::env_config::EnvConfig;
#[cfg(feature = "fastly")]
Expand Down Expand Up @@ -139,7 +141,7 @@ where
F: FnOnce(&fastly::Request, &mut Extensions),
{
let stores = A::stores();
let env = env_config_from_runtime_dictionary(stores);
let env = runtime_env_config(stores);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 The missing-store warning is dropped on Compute. runtime_env_config runs here, at line 144, but init_logger only runs at line 148. With no global logger installed yet, the log::warn! inside the try_open failure arm (lines 191-197) is a no-op, and Compute@Edge hands out a fresh Wasm instance per request, so there is no later request that would see it either.

The result: the warning whose own doc comment says operators "can spot the gap in their Fastly logs and run edgezero provision --adapter fastly" never actually reaches those logs. This predates the PR, but the PR body lists "keeping the missing-store warning" as preserved behaviour, so it is worth knowing the behaviour being preserved is currently inert.

A real fix means resolving logging config before the env store read, or deferring the warning until after init_logger — both wider than this change. Flagging for a follow-up issue rather than asking for it here.

let logging = logging_from_env(&env);
if logging.use_fastly_logger && !A::owns_logging() {
let endpoint = logging.endpoint.as_deref().unwrap_or("stdout");
Expand All @@ -158,23 +160,24 @@ where
}

/// Build an [`EnvConfig`] from the optional `edgezero_runtime_env`
/// Fastly Config Store. Compute@Edge has no process env -- the
/// `EDGEZERO__*` runtime overrides spec 5.2/5.4 expects must come
/// from a Config Store the operator pre-populates (locally via
/// `fastly.toml`'s `[local_server.config_stores.edgezero_runtime_env]`
/// block; remotely via a `fastly config-store` named `edgezero_runtime_env`).
/// Fastly Config Store.
///
/// The Cloudflare adapter does the same thing through `env.var(...)`
/// (lib.rs:55) -- Workers also have no `std::env`. Mirroring the
/// approach here closes the spec 12.7 gap where `__KEY` runtime
/// overrides silently fell back to the binding's default id.
/// Compute@Edge has no process env, so the `EDGEZERO__*` runtime overrides
/// (logging settings, per-store platform names, the config-store `__KEY`
/// selector) come from a Config Store the operator pre-populates: locally via
/// `fastly.toml`'s `[local_server.config_stores.edgezero_runtime_env]` block,
/// remotely via a `fastly config-store` named `edgezero_runtime_env`.
///
/// If the store is missing or empty, returns an empty `EnvConfig` --
/// the rest of the runtime then uses the baked-in defaults (which is
/// what the pre-fix code did, just without the env-driven override
/// path the spec promises).
/// If the store is missing or empty, returns an empty `EnvConfig` and the rest
/// of the runtime uses its baked-in defaults.
///
/// [`run_app`] calls this itself. A custom Fastly entry point that bypasses

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ Name the entry point that does not call this. "[run_app] calls this itself" is accurate but incomplete in the direction that matters for the audience this doc is written for:

  • the direct caller is run_app_with_request_extensions (line 144); run_app reaches it by delegation
  • run_app_with_config (line 246) never resolves an EnvConfig at all

run_app_with_config is the existing explicit-wiring entry point, so it is exactly what a custom entry point is most likely to reach for — and it is the one path where calling runtime_env_config is mandatory rather than redundant. Saying so here turns a trap into a signpost.

Suggested wording:

/// [`run_app`] and [`run_app_with_request_extensions`] call this themselves.
/// [`run_app_with_config`] does NOT — a custom entry point on that path (or one
/// building its own [`request::FastlyService`]) must call this with its own
/// `A::stores()` so staged and overridden store selectors resolve identically.

/// [`run_app`] should call it with its own `A::stores()` so staged and
/// overridden store selectors resolve identically.
#[cfg(feature = "fastly")]
fn env_config_from_runtime_dictionary(stores: StoresMetadata) -> EnvConfig {
#[must_use]
#[inline]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#[inline] on a non-trivial pub fn duplicates codegen downstream. Now that this is public, #[inline] makes the body a candidate for instantiation in every downstream crate that calls it — and the body is not small: a Vec<String> build, a per-key dict.get loop, and a ~250-byte log literal.

The crate's other #[inline] public functions (run_app, run_app_with_request_extensions, init_logger) are all thin delegating wrappers, which is what makes the attribute free there. For a wasm target where binary size is a real budget, dropping #[inline] here is the more consistent choice. #[must_use] is right and should stay.

pub fn runtime_env_config(stores: StoresMetadata) -> EnvConfig {
use fastly::ConfigStore;
use std::iter::empty;
let Ok(dict) = ConfigStore::try_open("edgezero_runtime_env") else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ The store name is still duplicated — issue #349 asked for one source of truth for it. This literal "edgezero_runtime_env" is the same value as RUNTIME_ENV_STORE at cli.rs:136, and cli.rs:555 carries a third copy as runtime_env_name. The two consts cannot share today because they sit behind different feature gates (fastly vs cli).

The PR closes the duplication for downstream by exporting the loader, which is the larger half. An ungated const closes it internally too, and hands downstream the name for their own fastly config-store-entry tooling:

/// The Fastly Config Store the runtime opens for `EDGEZERO__*` overrides. A staged
/// deploy links its per-service staging twin under this same name, which is why the
/// runtime resolves staged selectors without knowing the twin exists.
pub const RUNTIME_ENV_STORE_NAME: &str = "edgezero_runtime_env";

Then ConfigStore::try_open(RUNTIME_ENV_STORE_NAME) here, and cli.rs drops its own const.

📝 Worth recording somewhere that the name being a hardcoded constant is load-bearing, not an oversight: relink_staged_runtime_env (cli.rs:4814-4912) links edgezero_runtime_env_staging_<service-id> under the name edgezero_runtime_env, so a fixed name is precisely what makes staged resolution work. I verified that path; the PR's central claim holds.

Expand All @@ -194,6 +197,17 @@ fn env_config_from_runtime_dictionary(stores: StoresMetadata) -> EnvConfig {
);
return EnvConfig::from_vars(empty::<(String, String)>());
};
let vars = runtime_env_keys(stores)
.into_iter()
.filter_map(|key| dict.get(&key).map(|value| (key, value)));
EnvConfig::from_vars(vars)
}

/// The `EDGEZERO__*` keys the Fastly runtime looks up: the fixed adapter and
/// logging settings, plus a `__NAME` selector for every declared store id and
/// a `__KEY` selector for config-store ids only.
#[cfg(any(feature = "fastly", test))]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ Explain the test in the cfg gate. #[cfg(any(feature = "fastly", test))] on a helper whose only non-test caller requires fastly reads like an accident. The reason is a good one and it is the load-bearing detail of the whole test strategy: the crate's default features exclude fastly, so without the test arm the new unit test could not run in the plain cargo test --workspace suite at all.

That reason currently lives only in the PR description, which future readers of this file will not have. The crate already sets the precedent for exactly this kind of note at lines 4-6, above the chunked_config gate.

// The `test` arm is load-bearing: the crate's default features exclude `fastly`,
// so gating on the feature alone would keep this helper — and the test pinning its
// key-derivation rules — out of the plain `cargo test --workspace` run.
#[cfg(any(feature = "fastly", test))]

fn runtime_env_keys(stores: StoresMetadata) -> Vec<String> {
let mut keys: Vec<String> = vec![

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 Four of these six fixed keys have no consumer on the Fastly path. I checked each across the workspace:

Key Consumer
EDGEZERO__ADAPTER__HOST axum dev server only — adapter-axum/src/dev_server.rs:517
EDGEZERO__ADAPTER__PORT axum dev server only — same call
EDGEZERO__LOGGING__LEVEL logging_from_env (lib.rs:92) ✅
EDGEZERO__LOGGING__ENDPOINT logging_from_env (lib.rs:101) ✅
EDGEZERO__LOGGING__USE_FASTLY_LOGGER none — logging_from_env:102 derives it from endpoint.is_some()
EDGEZERO__LOGGING__ECHO_STDOUT none — logging_from_env:104 hardcodes true

Compute@Edge does not bind a socket, so host/port can never apply there. Each entry is a Config Store host lookup per request, and the new test (lines 313-318) now pins all six as contract.

This is pre-existing behaviour, and going public arguably justifies keeping them — a custom entry point building its own FastlyLogging may well want ECHO_STDOUT and USE_FASTLY_LOGGER via EnvConfig::get. If that is the intent, this is now public API and the doc should say so, because the current phrasing — "the keys the Fastly runtime looks up" — is not true of four of them: the runtime fetches them and then ignores them.

Either state the downstream-reader intent, or drop the four and save the lookups.

"EDGEZERO__ADAPTER__HOST".to_owned(),
"EDGEZERO__ADAPTER__PORT".to_owned(),
Expand All @@ -217,10 +231,7 @@ fn env_config_from_runtime_dictionary(stores: StoresMetadata) -> EnvConfig {
}
}
}
let vars = keys
.into_iter()
.filter_map(|key| dict.get(&key).map(|value| (key, value)));
EnvConfig::from_vars(vars)
keys
}

/// Dispatch with a config store wired explicitly. Use `run_app` for
Expand Down Expand Up @@ -270,3 +281,50 @@ mod tests {
assert!(logging.use_fastly_logger);
}
}

#[cfg(test)]
mod runtime_env_key_tests {
use super::runtime_env_keys;
use edgezero_core::app::{StoreMetadata, StoresMetadata};

fn contains(keys: &[String], key: &str) -> bool {
keys.iter().any(|candidate| candidate.as_str() == key)
}

#[test]
fn runtime_env_keys_name_every_store_and_key_only_config_stores() {
let stores = StoresMetadata {
config: Some(StoreMetadata {
default: "main",
ids: &["main", "edge"],
}),
kv: Some(StoreMetadata {
default: "cache",
ids: &["cache"],
}),
secrets: Some(StoreMetadata {
default: "vault",
ids: &["vault"],
}),
};

let keys = runtime_env_keys(stores);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ Pin the set, not just membership. Fifteen contains assertions prove no key is missing, but nothing here fails if a seventh fixed key is added, if a key is renamed while the old spelling stays, or if a __NAME key is emitted twice. Given that the whole point of this test is to pin the derivation rules against drift, an exact comparison is the stronger contract and drops the contains helper:

let mut keys = runtime_env_keys(stores);
keys.sort();
assert_eq!(
    keys,
    vec![
        "EDGEZERO__ADAPTER__HOST",
        "EDGEZERO__ADAPTER__PORT",
        "EDGEZERO__LOGGING__ECHO_STDOUT",
        "EDGEZERO__LOGGING__ENDPOINT",
        "EDGEZERO__LOGGING__LEVEL",
        "EDGEZERO__LOGGING__USE_FASTLY_LOGGER",
        "EDGEZERO__STORES__CONFIG__EDGE__KEY",
        "EDGEZERO__STORES__CONFIG__EDGE__NAME",
        "EDGEZERO__STORES__CONFIG__MAIN__KEY",
        "EDGEZERO__STORES__CONFIG__MAIN__NAME",
        "EDGEZERO__STORES__KV__CACHE__NAME",
        "EDGEZERO__STORES__SECRETS__VAULT__NAME",
    ],
);

This keeps every current assertion (including the two negative ones — absence of KV__CACHE__KEY and SECRETS__VAULT__KEY falls out of the equality) and adds the no-extras guarantee.

One case stays uncovered either way: StoresMetadata::default(), all three fields None. That is the hand-written-Hooks shape core explicitly documents at app.rs:91-93, and it should yield exactly the six fixed keys. Cheap second #[test].


assert!(contains(&keys, "EDGEZERO__ADAPTER__HOST"));
assert!(contains(&keys, "EDGEZERO__ADAPTER__PORT"));
assert!(contains(&keys, "EDGEZERO__LOGGING__LEVEL"));
assert!(contains(&keys, "EDGEZERO__LOGGING__ENDPOINT"));
assert!(contains(&keys, "EDGEZERO__LOGGING__USE_FASTLY_LOGGER"));
assert!(contains(&keys, "EDGEZERO__LOGGING__ECHO_STDOUT"));

assert!(contains(&keys, "EDGEZERO__STORES__CONFIG__MAIN__NAME"));
assert!(contains(&keys, "EDGEZERO__STORES__CONFIG__EDGE__NAME"));
assert!(contains(&keys, "EDGEZERO__STORES__KV__CACHE__NAME"));
assert!(contains(&keys, "EDGEZERO__STORES__SECRETS__VAULT__NAME"));

assert!(contains(&keys, "EDGEZERO__STORES__CONFIG__MAIN__KEY"));
assert!(contains(&keys, "EDGEZERO__STORES__CONFIG__EDGE__KEY"));
assert!(!contains(&keys, "EDGEZERO__STORES__KV__CACHE__KEY"));
assert!(!contains(&keys, "EDGEZERO__STORES__SECRETS__VAULT__KEY"));
}
}
2 changes: 1 addition & 1 deletion scripts/smoke_test_config_key_override.sh
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ upper() {
# Seed the Fastly local config store `edgezero_runtime_env` with the
# runtime override env vars. The Fastly Compute@Edge runtime has no
# process env, so EDGEZERO__* overrides are read from this dedicated
# Config Store (see env_config_from_runtime_dictionary in
# Config Store (see runtime_env_config in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😃 The rename is propagated everywhere, including down here in the smoke script and both comment sites in cli.rs (lines 135 and 549). I grepped the tree: no stale env_config_from_runtime_dictionary reference survives outside one historical plan doc. Renames that reach the shell scripts are the ones that do not rot.

# crates/edgezero-adapter-fastly/src/lib.rs). $1 is the fastly.toml
# path; $2 is the per-row __KEY override value (empty -> no override).
seed_fastly_runtime_env() {
Expand Down
Loading