-
Notifications
You must be signed in to change notification settings - Fork 0
Expose runtime_env_config for custom Fastly entry points #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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")] | ||||||||||||||||
|
|
@@ -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); | ||||||||||||||||
| let logging = logging_from_env(&env); | ||||||||||||||||
| if logging.use_fastly_logger && !A::owns_logging() { | ||||||||||||||||
| let endpoint = logging.endpoint.as_deref().unwrap_or("stdout"); | ||||||||||||||||
|
|
@@ -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 | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ Name the entry point that does not call this. "[
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] | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏ The crate's other |
||||||||||||||||
| 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 { | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 /// 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 📝 Worth recording somewhere that the name being a hardcoded constant is load-bearing, not an oversight: |
||||||||||||||||
|
|
@@ -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))] | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ Explain the 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 // 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![ | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 Either state the downstream-reader intent, or drop the four and save the lookups. |
||||||||||||||||
| "EDGEZERO__ADAPTER__HOST".to_owned(), | ||||||||||||||||
| "EDGEZERO__ADAPTER__PORT".to_owned(), | ||||||||||||||||
|
|
@@ -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 | ||||||||||||||||
|
|
@@ -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); | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ Pin the set, not just membership. Fifteen 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 One case stays uncovered either way: |
||||||||||||||||
|
|
||||||||||||||||
| 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")); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| # 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() { | ||
|
|
||
There was a problem hiding this comment.
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_configruns here, at line 144, butinit_loggeronly runs at line 148. With no global logger installed yet, thelog::warn!inside thetry_openfailure 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.