Gateway - Check new settings against new secrets

crates/gateway/app/src/admin/walled/config-tests.rs

@@ -85,6 +85,19 @@ async fn put_config(addr: std::net::SocketAddr, body: &serde_json::Value) -> req
         .expect("put sends")
 }
 
+/// Add a call to the Add Model endpoint to a request body
+fn push_cloud_endpoint(body: &mut serde_json::Value, var: &str) {
+    body["endpoint"]
+        .as_array_mut()
+        .expect("endpoints are an array")
+        .push(serde_json::json!({
+            "id": "staged",
+            "protocol": "openai",
+            "base_url": "http://127.0.0.1:9",
+            "api_key": format!("${{{var}}}"),
+        }));
+}
+
 #[tokio::test]
 async fn a_save_setting_active_profile_is_rejected_and_stages_nothing() {
     let (_temp, config, paths) = fixture();
@@ -129,3 +142,65 @@ async fn a_save_replies_with_the_config_shadow_alone() {
     assert!(shadow_path(&config_path).is_file());
     assert!(!shadow_path(&profile_state_path(&config_path)).exists());
 }
+
+#[tokio::test]
+async fn adding_a_cloud_provider_validates_against_a_key_staged_only_in_the_env_shadow() {
+    // After a 'Save' operation in the UI the saved key is stored in the shadow
+    // env file and is not yet available in the process's environment.
+    //
+    // When a Cloud Model is added the handler should validate against the running
+    // environment and  pending keys in the shadow env.
+
+    let (_temp, config, paths) = fixture();
+    let config_path = paths.config_path.clone();
+
+    std::fs::write(
+        shadow_path(&config_path.with_extension("env")),
+        "PF_TEST_STAGED_KEY=sk-staged\n",
+    )
+    .expect("write env shadow");
+
+    let addr = serve_with_paths(config, paths).await;
+
+    let mut body = save_body(addr).await;
+    push_cloud_endpoint(&mut body, "PF_TEST_STAGED_KEY");
+
+    let response = put_config(addr, &body).await;
+
+    let expected = reqwest::StatusCode::OK;
+    let actual = response.status();
+    assert_eq!(
+        actual,
+        expected,
+        "Got `{actual}`, expected `{expected}`. Response: {}",
+        response.text().await.unwrap_or_default()
+    );
+    assert!(shadow_path(&config_path).is_file());
+}
+
+#[tokio::test]
+async fn adding_a_cloud_provider_without_its_key_is_rejected_and_stages_nothing() {
+    let (_temp, config, paths) = fixture();
+    let config_path = paths.config_path.clone();
+    let addr = serve_with_paths(config, paths).await;
+
+    let mut body = save_body(addr).await;
+    push_cloud_endpoint(&mut body, "PF_TEST_MISSING_KEY");
+
+    let response = put_config(addr, &body).await;
+
+    assert_eq!(response.status(), reqwest::StatusCode::UNPROCESSABLE_ENTITY);
+
+    let error: serde_json::Value = response.json().await.expect("error envelope");
+
+    assert_eq!(error["error"]["code"], "config_write_rejected");
+
+    assert!(
+        error["error"]["message"]
+            .as_str()
+            .is_some_and(|message| message.contains("PF_TEST_MISSING_KEY")),
+        "the error message does not name the missing variable: {error}"
+    );
+
+    assert!(!shadow_path(&config_path).exists());
+}

crates/gateway/app/src/admin/walled/config.rs

@@ -22,6 +22,7 @@ use serde::Serialize;
 
 use crate::AppState;
 use crate::auth::LoopbackCaller;
+use crate::config_shadow::PendingEnv;
 use crate::error::{GatewayError, WireJson, blocking, config_write_error};
 use crate::registry::RouteInfo;
 
@@ -82,11 +83,16 @@ async fn admin_put_config(
     // save validated whole - saves serialize with apply, revert, and each
     // other.
     let _guard = state.apply.lock().await;
-    let config = crate::admin::config_path(&state)?.to_path_buf();
+    let config_path = crate::admin::config_path(&state)?.to_path_buf();
     let document = toml_document(body)?;
-    let shadows = blocking(move || save_config_shadow(&config, document))
-        .await?
-        .map_err(config_write_error)?;
+    let shadows = blocking(move || {
+        let pending_env = PendingEnv::new(&config_path)?;
+        save_config_shadow(&config_path, document, &|name: &str| {
+            pending_env.resolve_var(name)
+        })
+        .map_err(config_write_error)
+    })
+    .await??;
     Ok(Json(ShadowReply::staged(&shadows.config)))
 }
 

crates/gateway/app/src/admin/walled/config_pending.rs

@@ -24,7 +24,7 @@ use serde::Serialize;
 
 use crate::AppState;
 use crate::auth::LoopbackCaller;
-use crate::config_shadow::{config_root, relative_name, shadow_census};
+use crate::config_shadow::{PendingEnv, config_root, relative_name, shadow_census};
 use crate::error::{GatewayError, blocking, pending_read_error};
 use crate::registry::RouteInfo;
 
@@ -107,8 +107,13 @@ pub(super) fn load_pending_for_running(
     config_path: &Path,
     running_profile: Option<&str>,
 ) -> Result<Config, GatewayError> {
-    load_pending_config(config_path, &ProfileSelection::new(running_profile, None))
-        .map_err(|error| pending_read_error(&error))
+    let pending_env = PendingEnv::new(config_path)?;
+    load_pending_config(
+        config_path,
+        &ProfileSelection::new(running_profile, None),
+        &|name: &str| pending_env.resolve_var(name),
+    )
+    .map_err(|error| pending_read_error(&error))
 }
 
 /// The profile name the real state file persists, `None` when the file is

crates/gateway/app/src/admin/walled/env_file.rs

@@ -22,6 +22,7 @@ use serde::Serialize;
 use super::config::ShadowReply;
 use crate::AppState;
 use crate::auth::LoopbackCaller;
+use crate::config_shadow::read_env_file;
 use crate::error::{GatewayError, WireJson, WireQuery, blocking, config_write_error};
 use crate::registry::RouteInfo;
 
@@ -42,7 +43,7 @@ struct EnvReply {
 #[derive(Debug, Serialize)]
 struct EnvSection {
     path: String,
-    vars: serde_json::Map<String, serde_json::Value>,
+    vars: BTreeMap<String, String>,
 }
 
 const ENV: RouteInfo = RouteInfo::walled("/admin/env", &[Method::GET, Method::PUT]);
@@ -130,26 +131,10 @@ async fn admin_put_env(
 fn env_section(path: &Path) -> Result<EnvSection, GatewayError> {
     Ok(EnvSection {
         path: path.display().to_string(),
-        vars: parse_env(path)?,
+        vars: read_env_file(path)?,
     })
 }
 
-/// Parses one `.env` file into a map, without touching the process
-/// environment. A missing file is an empty map.
-fn parse_env(path: &Path) -> Result<serde_json::Map<String, serde_json::Value>, GatewayError> {
-    let mut vars = serde_json::Map::new();
-    if !path.is_file() {
-        return Ok(vars);
-    }
-    let entries =
-        dotenvy::from_path_iter(path).map_err(|error| GatewayError::EnvFile(Box::new(error)))?;
-    for entry in entries {
-        let (key, value) = entry.map_err(|error| GatewayError::EnvFile(Box::new(error)))?;
-        vars.insert(key, serde_json::Value::from(value));
-    }
-    Ok(vars)
-}
-
 /// Renders the variables as dotenv lines, refusing names and values the
 /// boot-time parser could not round-trip.
 fn render_env(vars: &BTreeMap<String, String>) -> Result<String, GatewayError> {

crates/gateway/app/src/commands/apply.rs

@@ -20,7 +20,7 @@ use tokio_util::sync::CancellationToken;
 
 use super::{APPLY_CONFIG_LABEL, Outcome};
 use crate::AppState;
-use crate::config_shadow::{canonical_form, config_root, relative_name, shadow_census};
+use crate::config_shadow::{PendingEnv, canonical_form, config_root, relative_name, shadow_census};
 use crate::error::{GatewayError, blocking, config_write_error};
 use crate::routing::Routing;
 
@@ -132,9 +132,12 @@ pub(crate) fn capture_apply(config_path: &Path) -> Result<ApplyPlan, GatewayErro
     // persisted name can differ from the running profile (a switch that
     // persisted a new name and is waiting on a restart) and must not be
     // published as the live document's selection.
-    let config = load_pending_config(config_path, &ProfileSelection::default())
-        .and_then(|config| config.select_profile(None))
-        .map_err(config_write_error)?;
+    let pending_env = PendingEnv::new(config_path)?;
+    let config = load_pending_config(config_path, &ProfileSelection::default(), &|name: &str| {
+        pending_env.resolve_var(name)
+    })
+    .and_then(|config| config.select_profile(None))
+    .map_err(config_write_error)?;
     Ok(ApplyPlan::Reload(ApplySnapshot {
         config: Box::new(config),
         files,

crates/gateway/app/src/config_shadow-tests.rs

@@ -0,0 +1,84 @@
+use gateway_config::shadow_path;
+
+use super::*;
+
+/// A config path in a fresh directory; nothing is written.
+fn config_path() -> (tempfile::TempDir, PathBuf) {
+    let temp = tempfile::TempDir::new().expect("temp dir");
+    let path = temp.path().join("gateway.toml");
+    (temp, path)
+}
+
+#[test]
+fn when_reading_the_pending_env_the_shadow_wins_over_the_real_env_file() {
+    let (_temp, config_path) = config_path();
+    let env_path = config_path.with_extension("env");
+
+    let var_name = "PF_TEST_KEY";
+    let value = "sk-A";
+    let shadow_value = "sk-B";
+
+    std::fs::write(&env_path, format!("{var_name}={value}\n")).expect("error writing env file");
+    std::fs::write(
+        shadow_path(&env_path),
+        format!("{var_name}={shadow_value}\n"),
+    )
+    .expect("error writing env shadow");
+
+    let pending = PendingEnv::new(&config_path).expect("pending env reads");
+
+    let actual = pending
+        .resolve_var(var_name)
+        .expect("Could not resolve test var");
+    let expected = shadow_value;
+
+    assert_eq!(actual, expected, "Got `{actual}` but expected `{expected}`");
+}
+
+#[test]
+fn no_shadow_env_file_is_an_empty_pending_env() {
+    let (_temp, config_path) = config_path();
+
+    let pending = PendingEnv::new(&config_path).expect("Error building pending env");
+
+    assert!(pending.data.is_empty());
+    assert_eq!(
+        pending.resolve_var("PF_TEST_UNSET_EVERYWHERE"),
+        Err(VarError::NotPresent)
+    );
+}
+
+#[test]
+fn when_reading_the_pending_env_the_shadow_file_wins_over_the_process_environment() {
+    // PATH is set in every test process, so it stands in for a variable
+    // both the file and the process define.
+
+    let (_temp, config_path) = config_path();
+    std::fs::write(config_path.with_extension("env.next"), "PATH=from-file\n")
+        .expect("Error writing env file");
+
+    let pending = PendingEnv::new(&config_path).expect("Error building pending env");
+
+    assert_eq!(pending.resolve_var("PATH"), Ok("from-file".to_owned()));
+}
+
+#[test]
+fn a_variable_only_in_the_process_environment_resolves_from_it() {
+    let (_temp, config_path) = config_path();
+
+    let pending = PendingEnv::new(&config_path).expect("Error building pending env");
+
+    assert_eq!(pending.resolve_var("PATH"), std::env::var("PATH"));
+}
+
+#[test]
+fn a_malformed_env_file_is_an_error() {
+    let (_temp, config_path) = config_path();
+    std::fs::write(
+        config_path.with_extension("env.next"),
+        r#"="I'm only a value with no name!"#,
+    )
+    .expect("error writing env file");
+
+    assert!(PendingEnv::new(&config_path).is_err());
+}

crates/gateway/app/src/config_shadow.rs

@@ -1,5 +1,6 @@
 //! Shadow-file bookkeeping: which real config files have a pending
 //! `.next` shadow, and how those paths are rendered for the wire.
+//! Provides a struct for reading the pending environment from the shadow files.
 //!
 //! Three readers share this. `GET /admin/config-dirty` reports the census
 //! as pending state, `POST /admin/config-apply` takes it under the apply
@@ -8,6 +9,8 @@
 //! `gateway-config`; this module only assembles the census and puts its
 //! paths in comparable and displayable form.
 
+use std::collections::BTreeMap;
+use std::env::VarError;
 use std::path::{Path, PathBuf};
 
 use gateway_config::{pending_report, shadow_path};
@@ -85,3 +88,68 @@ pub(crate) fn relative_name(file: &Path, root: Option<&Path>) -> String {
         .collect();
     parts.join("/")
 }
+
+/// Struct for accessing the environment variables the next boot will run with.
+pub(crate) struct PendingEnv {
+    data: BTreeMap<String, String>,
+}
+
+impl PendingEnv {
+    /// Constructs `Self` from env files.
+    /// If a shadow env file is staged, it reads that, otherwise the real file.
+    pub(crate) fn new(config_path: &Path) -> Result<Self, GatewayError> {
+        let path = {
+            let env_path = config_path.with_extension("env");
+            let shadow_path = shadow_path(&env_path);
+
+            if shadow_path.is_file() {
+                shadow_path
+            } else {
+                env_path
+            }
+        };
+
+        Ok(Self {
+            data: read_env_file(&path)?,
+        })
+    }
+
+    /// Resolves a `${VAR}` from the pending environment.
+    /// If a value is not present in the pending environment, it checks the current
+    /// running environment.
+    ///
+    /// TODO: This function is not able to distinguish between variables set in an
+    /// env file and overrides set in the shell. If the pending environment defines a
+    /// variable `VAR=abc` but the shell environment overrides it with `VAR=def`,
+    /// this will return `abc` but when the gateway restarts the value will be `def`
+    pub(crate) fn resolve_var(&self, name: &str) -> Result<String, VarError> {
+        match self.data.get(name) {
+            Some(value) => Ok(value.clone()),
+            None => std::env::var(name),
+        }
+    }
+}
+
+/// Parses a dotenv file and returns a map without changing the process environment; a
+/// missing file is an empty map.
+pub(crate) fn read_env_file(path: &Path) -> Result<BTreeMap<String, String>, GatewayError> {
+    let mut vars = BTreeMap::new();
+
+    if !path.is_file() {
+        return Ok(vars);
+    }
+
+    let entries =
+        dotenvy::from_path_iter(path).map_err(|error| GatewayError::EnvFile(Box::new(error)))?;
+
+    for entry in entries {
+        let (key, value) = entry.map_err(|error| GatewayError::EnvFile(Box::new(error)))?;
+        vars.insert(key, value);
+    }
+
+    Ok(vars)
+}
+
+#[cfg(test)]
+#[path = "config_shadow-tests.rs"]
+mod tests;

crates/gateway/config/src/config.rs

@@ -20,6 +20,7 @@ pub use companion::{
     SpeculativeConfig,
 };
 pub(crate) use imp::reject_profiles_directory;
+pub use interpolate::VarLookupFn;
 use interpolate::interpolate_value;
 // The canonical home of the model-metadata types is `gateway-api-types`.
 pub use gateway_api_types::{Capabilities, ModelKind, ThinkingMode};