diff --git a/Cargo.toml b/Cargo.toml index 27ee552..97e910b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,7 +96,12 @@ tokio = { version = "1", features = ["full"] } # Serialization serde = { version = "1", features = ["derive"] } serde_json = "1" -serde_yaml = "0.9" +# The config file and the sections an embedder hands over as YAML. Safe Rust; +# deserialization only. +serde-saphyr = { version = "1.3", default-features = false, features = ["deserialize"] } +# The key path (`runtime.worker_threads`) a config error names; already in the +# tree through axum. +serde_path_to_error = "0.1" # Streaming (server-streaming RPC handler) futures = "0.3" @@ -229,7 +234,7 @@ chrono = "0.4" tokio-stream = "0.1" # Compiles the in-memory test protos (google.api.http routes) of the # error-details integration test without a protoc binary. -protox = "0.9" +protox = "0.10" # Runs the built binary, so it needs the binary built. [[test]] diff --git a/src/auth/tests.rs b/src/auth/tests.rs index 769fe0d..cb553d0 100644 --- a/src/auth/tests.rs +++ b/src/auth/tests.rs @@ -581,7 +581,7 @@ mod builtin { issuer: Some("test-iss".into()), audience: Some("test-aud".into()), public_key_pem_file: Some(temp_pub_pem()), - cache: serde_yaml::from_str("max_token_bytes: 64").unwrap(), + cache: crate::config::parse_yaml("max_token_bytes: 64").unwrap(), ..jwt_claims_only() }), forward_auth: Some(secure_policy(&[])), @@ -643,7 +643,7 @@ mod builtin { issuer: Some("test-iss".into()), audience: Some("test-aud".into()), public_key_pem_file: Some(temp_pub_pem()), - cache: serde_yaml::from_str("enabled: false").unwrap(), + cache: crate::config::parse_yaml("enabled: false").unwrap(), ..jwt_claims_only() }), forward_auth: Some(secure_policy(&[])), @@ -693,7 +693,7 @@ mod builtin { mode: "jwt".into(), jwt: Some(JwtConfig { public_key_pem_file: Some(temp_pub_pem()), - cache: serde_yaml::from_str("max_entries: 0").unwrap(), + cache: crate::config::parse_yaml("max_entries: 0").unwrap(), ..jwt_claims_only() }), forward_auth: None, diff --git a/src/config.rs b/src/config.rs index de953fc..eb40b6a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -326,9 +326,50 @@ pub struct TranscodeConfig { /// assert!(shield.enabled); /// ``` pub fn from_yaml(yaml: &str) -> anyhow::Result { - Ok(serde_yaml::from_str(yaml)?) + Ok(parse_yaml(yaml)?) +} + +/// The YAML reader of every config path, with the YAML 1.2 core schema's +/// types: a boolean is `true` or `false` in any letter case (`True`, `TRUE`), +/// while `yes`, `no`, `on` and `off` are refused; an unquoted number or +/// boolean where text is expected is an error, not the text it spells, so +/// `name: true` does not start a service called "true". Errors stay on one +/// line, without a snippet of the document, so a log record holds the whole +/// message. +pub(crate) fn parse_yaml(yaml: &str) -> Result { + let mut path = None; + serde_saphyr::with_deserializer_from_str_with_options( + yaml, + serde_saphyr::options! { + // Matches `true`/`false` ignoring case, the core schema's spellings. + strict_booleans: true, + no_schema: true, + with_snippet: false, + }, + |de| { + serde_path_to_error::deserialize(de).map_err(|e| { + if e.path().iter().next().is_some() { + path = Some(e.path().to_string()); + } + e.into_inner() + }) + }, + ) + .map_err(|e| { + YamlError(match path { + Some(path) => format!("{path}: {e}"), + // The document as a whole is at fault (not YAML, not a map). + None => e.to_string(), + }) + }) } +/// A YAML document that does not read as the config it should be, naming the +/// key at fault the way the document spells its path (`runtime.worker_threads`). +#[derive(Debug, thiserror::Error)] +#[error("{0}")] +pub(crate) struct YamlError(String); + /// How gRPC-Web calls reach the upstream. #[derive(Debug, Clone, Default, Deserialize)] #[serde(deny_unknown_fields)] @@ -500,40 +541,163 @@ pub(crate) const KNOWN_STREAMING_KEYS: &[&str] = &["sse_keep_alive_secs", "ndjso /// force. They are reported, not rejected, so a file that loaded before keeps /// loading. pub(crate) fn unknown_config_keys(yaml: &str) -> Vec { - let Ok(serde_yaml::Value::Mapping(map)) = serde_yaml::from_str(yaml) else { + let Ok(ConfigKeys { top, streaming }) = parse_yaml(yaml) else { return Vec::new(); }; - let mut unknown: Vec = map - .keys() - .filter_map(|key| key.as_str()) - .filter(|key| !KNOWN_TOP_LEVEL_KEYS.contains(key)) - .map(str::to_owned) + let mut unknown: Vec = top + .into_iter() + .filter(|key| !KNOWN_TOP_LEVEL_KEYS.contains(&key.as_str())) .collect(); // `streaming:` is split between StreamingConfig and StreamingFileConfig, // so neither can deny unknown fields; its keys are checked here instead. - if let Some(serde_yaml::Value::Mapping(streaming)) = map.get("streaming") { - unknown.extend( - streaming - .keys() - .filter_map(|key| key.as_str()) - .filter(|key| !KNOWN_STREAMING_KEYS.contains(key)) - .map(|key| format!("streaming.{key}")), - ); - } + unknown.extend( + streaming + .iter() + .filter(|key| !KNOWN_STREAMING_KEYS.contains(&key.as_str())) + .map(|key| format!("streaming.{key}")), + ); unknown } +/// The keys of a config document in their order: the top-level ones and those +/// of `streaming:`. A document that is empty or not a map has none. +#[derive(Default)] +struct ConfigKeys { + top: Vec, + streaming: Vec, +} + +impl<'de> Deserialize<'de> for ConfigKeys { + fn deserialize>(de: D) -> Result { + struct Visitor; + impl<'de> serde::de::Visitor<'de> for Visitor { + type Value = ConfigKeys; + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a config map") + } + fn visit_unit(self) -> Result { + Ok(ConfigKeys::default()) + } + fn visit_none(self) -> Result { + Ok(ConfigKeys::default()) + } + fn visit_map>( + self, + mut map: A, + ) -> Result { + let mut keys = ConfigKeys::default(); + while let Some(TextKey(key)) = map.next_key()? { + match key { + Some(key) if key == "streaming" => { + keys.streaming = map.next_value::()?.0; + keys.top.push(key); + } + Some(key) => { + map.next_value::()?; + keys.top.push(key); + } + None => { + map.next_value::()?; + } + } + } + Ok(keys) + } + } + de.deserialize_any(Visitor) + } +} + +/// The keys of a map in their order; an empty value has none. +struct MapKeys(Vec); + +impl<'de> Deserialize<'de> for MapKeys { + fn deserialize>(de: D) -> Result { + struct Visitor; + impl<'de> serde::de::Visitor<'de> for Visitor { + type Value = MapKeys; + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a map") + } + fn visit_unit(self) -> Result { + Ok(MapKeys(Vec::new())) + } + fn visit_none(self) -> Result { + Ok(MapKeys(Vec::new())) + } + fn visit_map>( + self, + mut map: A, + ) -> Result { + let mut keys = Vec::new(); + while let Some(TextKey(key)) = map.next_key()? { + map.next_value::()?; + keys.extend(key); + } + Ok(MapKeys(keys)) + } + } + de.deserialize_any(Visitor) + } +} + +/// A map key as text; `None` for a key that is a number or a boolean, which +/// no setting is named by and which the reader refuses to take for text. +struct TextKey(Option); + +impl<'de> Deserialize<'de> for TextKey { + fn deserialize>(de: D) -> Result { + struct Visitor; + impl serde::de::Visitor<'_> for Visitor { + type Value = TextKey; + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a scalar key") + } + fn visit_str(self, key: &str) -> Result { + Ok(TextKey(Some(key.to_owned()))) + } + fn visit_string(self, key: String) -> Result { + Ok(TextKey(Some(key))) + } + fn visit_bool(self, _: bool) -> Result { + Ok(TextKey(None)) + } + fn visit_i64(self, _: i64) -> Result { + Ok(TextKey(None)) + } + fn visit_u64(self, _: u64) -> Result { + Ok(TextKey(None)) + } + fn visit_f64(self, _: f64) -> Result { + Ok(TextKey(None)) + } + } + de.deserialize_any(Visitor) + } +} + /// The error for a config that sets `shield.trusted_proxies`, which became /// `client_address.trusted_proxies`: the parse error alone would name an /// unknown field, not where its value belongs now. fn moved_setting(yaml: &str) -> Option { - let serde_yaml::Value::Mapping(map) = serde_yaml::from_str(yaml).ok()? else { - return None; - }; - let serde_yaml::Value::Mapping(shield) = map.get("shield")? else { - return None; - }; - shield.contains_key("trusted_proxies").then(|| { + /// The settings that moved, each `true` when the document names it. + #[derive(Deserialize)] + struct Moved { + #[serde(default)] + shield: Option, + } + #[derive(Deserialize)] + struct MovedFromShield { + #[serde(default, deserialize_with = "named")] + trusted_proxies: bool, + } + /// A key that is present, whatever its value, even `null`. + fn named<'de, D: serde::Deserializer<'de>>(de: D) -> Result { + serde::de::IgnoredAny::deserialize(de).map(|_| true) + } + + let moved: Moved = parse_yaml(yaml).ok()?; + moved.shield?.trusted_proxies.then(|| { anyhow::anyhow!( "shield.trusted_proxies is now client_address.trusted_proxies: the client \ address is resolved once for the rate limits, the hooks and the upstream" @@ -1597,7 +1761,7 @@ impl ProxyConfig { /// Useful for embedding the proxy: load a baked-in config (e.g. via /// `include_str!`) without touching the filesystem. pub fn from_yaml_str(yaml: &str) -> anyhow::Result { - let config: Self = match serde_yaml::from_str(yaml) { + let config: Self = match parse_yaml(yaml) { Ok(config) => config, Err(error) => return Err(moved_setting(yaml).map_or_else(|| error.into(), Into::into)), }; diff --git a/src/config/tests.rs b/src/config/tests.rs index 2c643f0..54092c9 100644 --- a/src/config/tests.rs +++ b/src/config/tests.rs @@ -6,7 +6,7 @@ fn test_minimal_config_deserialize() { upstream: default: "grpc://localhost:4180" "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!(config.upstream.unwrap().default, "grpc://localhost:4180"); assert_eq!(config.listen.http, "0.0.0.0:8080"); assert_eq!(config.service.name, "structured-proxy"); @@ -21,7 +21,7 @@ fn cors_defaults_cover_grpc_web_and_nothing_extra() { // Absent `cors:` and an empty one agree: permissive origins, gRPC-Web // included, no extra exposed headers, the browser's own preflight cache. for yaml in ["service:\n name: demo\n", "cors: {}\n"] { - let cors = serde_yaml::from_str::(yaml).unwrap().cors; + let cors = parse_yaml::(yaml).unwrap().cors; assert!(cors.origins.is_empty(), "{yaml}"); assert!(cors.expose_headers.is_empty(), "{yaml}"); assert_eq!(cors.max_age_secs, None, "{yaml}"); @@ -32,7 +32,7 @@ fn cors_defaults_cover_grpc_web_and_nothing_extra() { #[test] fn cors_settings_are_read() { let yaml = "cors:\n origins: [\"https://app.example\"]\n expose_headers: [\"x-request-id\"]\n max_age_secs: 600\n grpc_web: false\n"; - let cors = serde_yaml::from_str::(yaml).unwrap().cors; + let cors = parse_yaml::(yaml).unwrap().cors; assert_eq!(cors.origins, ["https://app.example"]); assert_eq!(cors.expose_headers, ["x-request-id"]); assert_eq!(cors.max_age_secs, Some(600)); @@ -42,7 +42,7 @@ fn cors_settings_are_read() { #[test] fn upstream_is_optional() { // An embedder whose upstream is in process names no address. - let config: ProxyConfig = serde_yaml::from_str("service:\n name: demo\n").unwrap(); + let config: ProxyConfig = parse_yaml("service:\n name: demo\n").unwrap(); assert!(config.upstream.is_none()); } @@ -50,18 +50,18 @@ fn upstream_is_optional() { fn upstream_without_its_address_is_rejected() { // A present `upstream:` block must say where: an empty one is a mistake, // not a request for an in-process upstream. - let err = serde_yaml::from_str::("upstream: {}\n").unwrap_err(); + let err = parse_yaml::("upstream: {}\n").unwrap_err(); assert!(err.to_string().contains("default"), "{err}"); } #[test] fn jwks_max_age_defaults_and_overrides() { let yaml = "upstream:\n default: \"grpc://x:1\"\nauth:\n mode: jwt\n jwt:\n jwks_uri: \"https://idp/jwks\"\n"; - let cfg: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let cfg: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!(cfg.auth.unwrap().jwt.unwrap().jwks_max_age_secs, 300); let yaml = "upstream:\n default: \"grpc://x:1\"\nauth:\n mode: jwt\n jwt:\n jwks_uri: \"https://idp/jwks\"\n jwks_max_age_secs: 120\n"; - let cfg: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let cfg: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!(cfg.auth.unwrap().jwt.unwrap().jwks_max_age_secs, 120); } @@ -69,7 +69,7 @@ fn jwks_max_age_defaults_and_overrides() { fn jwt_cache_defaults_and_overrides() { // Without a `cache:` block the cache is on with its defaults. let yaml = "upstream:\n default: \"grpc://x:1\"\nauth:\n mode: jwt\n jwt:\n jwks_uri: \"https://idp/jwks\"\n"; - let cfg: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let cfg: ProxyConfig = parse_yaml(yaml).unwrap(); let cache = &cfg.auth.unwrap().jwt.unwrap().cache; assert!(cache.enabled); assert_eq!(cache.max_entries, 10_000); @@ -78,7 +78,7 @@ fn jwt_cache_defaults_and_overrides() { // Keys given keep the rest at their defaults. let yaml = "upstream:\n default: \"grpc://x:1\"\nauth:\n mode: jwt\n jwt:\n jwks_uri: \"https://idp/jwks\"\n cache:\n max_ttl_secs: 15\n max_token_bytes: 2048\n"; - let cfg: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let cfg: ProxyConfig = parse_yaml(yaml).unwrap(); let cache = &cfg.auth.unwrap().jwt.unwrap().cache; assert!(cache.enabled); assert_eq!(cache.max_entries, 10_000); @@ -90,14 +90,14 @@ fn jwt_cache_defaults_and_overrides() { fn jwt_cache_rejects_unknown_keys() { // `ttl` for `max_ttl_secs` would otherwise leave the default in force. let yaml = "upstream:\n default: \"grpc://x:1\"\nauth:\n mode: jwt\n jwt:\n cache:\n ttl: 15\n"; - let err = serde_yaml::from_str::(yaml).unwrap_err(); + let err = parse_yaml::(yaml).unwrap_err(); assert!(err.to_string().contains("ttl"), "{err}"); } #[test] fn health_and_metrics_defaults_and_overrides() { // Defaults: enabled, conventional paths. - let min: ProxyConfig = serde_yaml::from_str("upstream:\n default: \"grpc://x:1\"\n").unwrap(); + let min: ProxyConfig = parse_yaml("upstream:\n default: \"grpc://x:1\"\n").unwrap(); assert!(min.health.enabled); assert_eq!(min.health.path, "/health"); assert_eq!(min.health.ready_path, "/health/ready"); @@ -114,7 +114,7 @@ metrics: enabled: false path: "/internal/metrics" "#; - let cfg: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let cfg: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!(cfg.health.path, "/internal/health"); // live_path was not overridden, so it stays at the default. assert_eq!(cfg.health.live_path, "/health/live"); @@ -270,7 +270,7 @@ forwarded_headers: - "dpop" - "x-request-id" "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!( config.upstream.as_ref().unwrap().default, "grpc://sid-identity:4180" @@ -305,7 +305,7 @@ auth: authz: enabled: false "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); let authz = config.auth.unwrap().authz.unwrap(); assert!(!authz.enabled); assert_eq!(authz.endpoint, ""); @@ -319,7 +319,7 @@ upstream: descriptors: - file: "/etc/proxy/service.descriptor.bin" "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!(config.descriptors.len(), 1); match &config.descriptors[0] { DescriptorSource::File { file } => { @@ -337,7 +337,7 @@ upstream: descriptors: - reflection: "grpc://localhost:4180" "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); match &config.descriptors[0] { DescriptorSource::Reflection { reflection } => { assert_eq!(reflection, "grpc://localhost:4180"); @@ -372,7 +372,7 @@ shield: key: { type: ip } profil: "auth" "#; - let err = serde_yaml::from_str::(yaml); + let err = parse_yaml::(yaml); assert!(err.is_err(), "unknown shield field must be rejected"); } @@ -393,7 +393,7 @@ shield: key: { type: ip, name: x-api-key } profile: "auth" "#; - let err = serde_yaml::from_str::(yaml); + let err = parse_yaml::(yaml); assert!(err.is_err(), "unknown field in a rule key must be rejected"); } @@ -409,7 +409,7 @@ openapi: title: "Test API" version: "2.0.0" "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); let openapi = config.openapi.unwrap(); assert!(openapi.enabled); assert_eq!(openapi.path, "/api/openapi.json"); @@ -426,7 +426,7 @@ upstream: openapi: enabled: true "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); let openapi = config.openapi.unwrap(); assert!(openapi.enabled); assert_eq!(openapi.path, "/openapi.json"); @@ -481,8 +481,7 @@ streaming: fn known_top_level_keys_cover_every_proxy_config_field() { // Exhaustive destructuring: adding a ProxyConfig field fails to compile // here until the field is also listed as a known top-level key. - let config: ProxyConfig = - serde_yaml::from_str("upstream:\n default: \"grpc://x:1\"\n").unwrap(); + let config: ProxyConfig = parse_yaml("upstream:\n default: \"grpc://x:1\"\n").unwrap(); let ProxyConfig { upstream: _, descriptors: _, @@ -735,7 +734,7 @@ fn known_streaming_keys_cover_both_streaming_structs() { /// The transcoding options a YAML document compiles to. fn transcode_options(yaml: &str) -> Result { - serde_yaml::from_str::(yaml) + parse_yaml::(yaml) .map_err(|e| e.to_string())? .options() } @@ -775,7 +774,7 @@ error_details: .enabled_for("/v1/public/internal/{id}")); assert!(!options.error_details.enabled_for("/v1/admin/items")); // The ProxyConfig still reads its own streaming key from the same section. - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = parse_yaml(yaml).unwrap(); assert_eq!(config.streaming.sse_keep_alive_secs, 30); } @@ -809,3 +808,182 @@ fn transcode_settings_reject_a_relative_route_pattern() { .unwrap_err(); assert!(err.contains("must start with '/'"), "{err}"); } + +#[test] +fn json_text_reads_as_a_config() { + // Embedders build configurations as JSON and hand the text to + // `from_yaml_str`: JSON is YAML, and must stay accepted. + let json = r#"{"upstream": {"default": "grpc://x:1"}, "service": {"name": "demo"}, + "cors": {"origins": ["https://app.example"], "grpc_web": false}, + "streaming": {"sse_keep_alive_secs": 5, "ndjson_envelope": true}}"#; + let config = ProxyConfig::from_yaml_str(json).unwrap(); + assert_eq!(config.upstream.unwrap().default, "grpc://x:1"); + assert_eq!(config.service.name, "demo"); + assert_eq!(config.cors.origins, ["https://app.example"]); + assert!(!config.cors.grpc_web); + assert_eq!(config.streaming.sse_keep_alive_secs, 5); + assert!(transcode_options(json).unwrap().ndjson_envelope); + assert!(unknown_config_keys(json).is_empty()); +} + +#[test] +fn only_yaml_1_2_booleans_are_booleans() { + // `yes`, `on` and friends are YAML 1.1; a reader that took them for + // booleans would turn on settings the format never defined that way. + for value in ["yes", "no", "on", "off", "Y", "n"] { + let err = ProxyConfig::from_yaml_str(&format!("cors:\n grpc_web: {value}\n")).unwrap_err(); + assert!(err.to_string().contains("cors.grpc_web"), "{value}: {err}"); + } + // The YAML 1.2 core schema spells them in three cases; files written for + // the earlier reader use all of them. + for (value, expected) in [ + ("true", true), + ("True", true), + ("TRUE", true), + ("false", false), + ("False", false), + ("FALSE", false), + ] { + let config = ProxyConfig::from_yaml_str(&format!("cors:\n grpc_web: {value}\n")).unwrap(); + assert_eq!(config.cors.grpc_web, expected, "{value}"); + } +} + +#[test] +fn the_packaged_config_template_loads() { + // The RPM/DEB packages install it as the starting config. + let yaml = include_str!("../../packaging/config.yaml"); + let config = ProxyConfig::from_yaml_str(yaml).unwrap(); + assert_eq!(config.service.name, "structured-proxy"); + assert!(unknown_config_keys(yaml).is_empty()); +} + +#[test] +fn a_number_or_boolean_is_not_taken_for_text() { + // A setting that names something (a service, a header) holds text; an + // unquoted `true` or `123` there is a mistake to report, not a name to + // start with. Quoted, it is the text it spells. + for yaml in [ + "service:\n name: true\n", + "service:\n name: 8080\n", + "service:\n name: 1.5\n", + "forwarded_headers: [123]\n", + "cors:\n origins: [false]\n", + ] { + assert!(ProxyConfig::from_yaml_str(yaml).is_err(), "{yaml}"); + } + let config = ProxyConfig::from_yaml_str("service:\n name: \"8080\"\n").unwrap(); + assert_eq!(config.service.name, "8080"); + let config = ProxyConfig::from_yaml_str("forwarded_headers: ['123', x-request-id]\n").unwrap(); + assert_eq!(config.forwarded_headers, ["123", "x-request-id"]); +} + +#[test] +fn a_parse_error_names_the_key_path_on_one_line() { + // The key path tells the operator where the mistake is; the message stays + // on one line so a log record holds all of it. + let err = ProxyConfig::from_yaml_str("cors:\n max_age_secs: soon\n").unwrap_err(); + let message = err.to_string(); + assert!(message.starts_with("cors.max_age_secs: "), "{message}"); + assert!(!message.contains('\n'), "{message}"); + + // Inside a list, the index is part of the path. + let yaml = + "shield:\n rules:\n - pattern: \"/**\"\n key: { type: ip }\n profil: anon\n"; + let message = ProxyConfig::from_yaml_str(yaml).unwrap_err().to_string(); + assert!(message.contains("shield.rules[0]"), "{message}"); + assert!(message.contains("profil"), "{message}"); +} + +#[test] +fn a_document_that_is_not_a_config_map_is_refused_without_a_path() { + // Nothing inside the document is at fault, so no path is named. + for yaml in ["just text", "[1, 2]", "upstream: [unclosed"] { + let message = parse_yaml::(yaml).unwrap_err().to_string(); + assert!(!message.is_empty(), "{yaml}"); + assert!(!message.starts_with(':'), "{yaml}: {message}"); + assert!(!message.contains('\n'), "{yaml}: {message}"); + } +} + +#[test] +fn an_empty_document_is_the_default_config() { + // A file with only comments, like an empty one, sets nothing. + for yaml in ["", "# nothing set yet\n", "---\n"] { + let config = ProxyConfig::from_yaml_str(yaml).unwrap(); + assert!(config.upstream.is_none(), "{yaml:?}"); + assert_eq!(config.listen.http, "0.0.0.0:8080", "{yaml:?}"); + assert!(unknown_config_keys(yaml).is_empty(), "{yaml:?}"); + } +} + +#[test] +fn a_repeated_key_is_refused() { + // Which of two values would win is not something to guess for a config. + let err = ProxyConfig::from_yaml_str("service:\n name: a\nservice:\n name: b\n").unwrap_err(); + assert!(err.to_string().contains("service"), "{err}"); +} + +#[test] +fn more_than_one_document_is_refused() { + // Only the first would be read; the rest would be silently dropped. + assert!(ProxyConfig::from_yaml_str("service:\n name: a\n---\nservice:\n name: b\n").is_err()); +} + +#[test] +fn unknown_keys_keep_the_document_order() { + // Reported in the order the operator wrote them, streaming ones by path. + let yaml = + "zeta: 1\nupstream:\n default: \"grpc://x:1\"\nalpha: 2\nstreaming:\n zz: 1\n aa: 2\n"; + assert_eq!( + unknown_config_keys(yaml), + ["zeta", "alpha", "streaming.zz", "streaming.aa"] + ); +} + +#[test] +fn unknown_keys_of_an_unreadable_document_are_none() { + // The parse error is the one to report; there is no key list to warn on. + for yaml in [ + "just text", + "[1, 2]", + "upstream: [unclosed", + "streaming: 5\n", + ] { + assert!(unknown_config_keys(yaml).is_empty(), "{yaml}"); + } +} + +#[test] +fn an_empty_streaming_section_has_no_unknown_keys() { + // `streaming:` with no value sets nothing; it is no typo. + assert!(unknown_config_keys("streaming:\nstreamin:\n") == ["streamin"]); +} + +#[test] +fn a_number_or_boolean_key_leaves_the_other_keys_reported() { + // No setting is named by a number or a boolean, so such a key is skipped, + // and the typos beside it are still reported. + assert_eq!( + unknown_config_keys( + "8080: listen\ntrue: 1\nmaintenence: {}\nstreaming:\n 5: x\n ndjson_envelop: true\n" + ), + ["maintenence", "streaming.ndjson_envelop"] + ); +} + +#[test] +fn shield_trusted_proxies_names_its_new_place_even_when_empty() { + // An empty value still shows where the operator meant to set the list. + let err = ProxyConfig::from_yaml_str("shield:\n trusted_proxies:\n").unwrap_err(); + assert!( + err.to_string().contains("client_address.trusted_proxies"), + "{err}" + ); + // A `shield:` that is not a map has its own error, not this one. + let err = ProxyConfig::from_yaml_str("shield: 5\n").unwrap_err(); + assert!( + !err.to_string().contains("client_address.trusted_proxies"), + "{err}" + ); +} diff --git a/src/lib.rs b/src/lib.rs index e5b4eea..06b6aa7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -205,7 +205,7 @@ impl ProxyServer { for key in config::unknown_config_keys(yaml) { tracing::warn!(%key, "unknown config key is ignored"); } - let settings: config::TranscodeFileConfig = serde_yaml::from_str(yaml)?; + let settings: config::TranscodeFileConfig = config::parse_yaml(yaml)?; let options = settings .options() .map_err(|e| anyhow::anyhow!("invalid transcoding config: {e}"))?; diff --git a/src/openapi/tests.rs b/src/openapi/tests.rs index 2490b5e..96b83e5 100644 --- a/src/openapi/tests.rs +++ b/src/openapi/tests.rs @@ -421,7 +421,7 @@ fn http_body_request_and_response_are_raw_content() { #[test] fn aliases_get_their_own_operation_ids() { - let alias: AliasConfig = serde_yaml::from_str("from: /api/{path}\nto: /v1").unwrap(); + let alias: AliasConfig = crate::config::parse_yaml("from: /api/{path}\nto: /v1").unwrap(); let spec = spec(&[alias]); assert_eq!( spec["paths"]["/v1/items"]["post"]["operationId"], diff --git a/src/runtime.rs b/src/runtime.rs index b5ec8da..2204b2c 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -72,8 +72,8 @@ impl FileConfig { /// A `runtime:` section that is not a map, holds an unknown key, or sets /// `worker_threads` to anything but a positive integer; the message names /// the key. - pub fn from_yaml_str(yaml: &str) -> Result { - serde_yaml::from_str(yaml) + pub fn from_yaml_str(yaml: &str) -> anyhow::Result { + structured_proxy::config::from_yaml(yaml) } } diff --git a/src/tests.rs b/src/tests.rs index d881ab5..0ae3ff4 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -22,7 +22,7 @@ fn test_minimal_config_server() { upstream: default: "http://127.0.0.1:50051" "#; - let config: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + let config: ProxyConfig = config::parse_yaml(yaml).unwrap(); let server = ProxyServer::from_config(config); assert!(server.descriptor_pool.is_none()); } diff --git a/src/transcode/tests.rs b/src/transcode/tests.rs index e6cc3ce..2f5ebec 100644 --- a/src/transcode/tests.rs +++ b/src/transcode/tests.rs @@ -78,7 +78,7 @@ service S { } "#, ); - let alias: AliasConfig = serde_yaml::from_str("from: /api/{path}\nto: /v1").unwrap(); + let alias: AliasConfig = crate::config::parse_yaml("from: /api/{path}\nto: /v1").unwrap(); let paths = route_paths(&pool, &[alias], &RpcSelection::default()); for expected in [ "/v1/files/{*path}",