diff --git a/docs/STACKER_YML_REFERENCE.md b/docs/STACKER_YML_REFERENCE.md index da7658fe..e705a9ca 100644 --- a/docs/STACKER_YML_REFERENCE.md +++ b/docs/STACKER_YML_REFERENCE.md @@ -1540,6 +1540,10 @@ Stacker validates your configuration both syntactically (YAML structure) and sem | `E002` | Server deployment requires `deploy.server.host` | `deploy.server.host` | | `E003` | Custom app type requires `app.image` or `app.dockerfile` | `app` | | `E004` | `deploy.environment` references an undefined environment key | `deploy.environment` / `environments` | +| `E005` | `deploy.default_target` missing or naming an undefined target; invalid `deploy.cloud.public_ports` entry | `deploy.default_target` / `deploy.cloud.public_ports` | +| `E006` | A `deploy.targets` profile defines both `server` and `cloud` | `deploy.targets.` | +| `E007` | Invalid port mapping in `app.ports` or `services.*.ports` | `app.ports` / `services.ports` | +| `E008` | A proxy is enabled but a `proxy.domains` entry is incomplete — empty domain, or an empty/malformed upstream *(added in 0.3.3)* | `proxy.domains[N]` | ### Warnings (deployment may have issues) diff --git a/src/cli/config_parser.rs b/src/cli/config_parser.rs index c88f777e..4317b914 100644 --- a/src/cli/config_parser.rs +++ b/src/cli/config_parser.rs @@ -1305,6 +1305,20 @@ impl<'de> Deserialize<'de> for TargetConfigContract { .or_insert_with(|| FieldPolicy::fixed(false)); } + // A block that declares nothing is meaningless — a service with no + // policy is simply left out. In practice it means a declaration was + // dropped somewhere between the file and here, which is what an older + // CLI did with `volumes:` before that kind reached `Serialize`: the + // submit succeeded, the stored contract held `"my-service": {}`, and + // the bake reset a volume the author had asked to keep. + if fields.is_empty() && raw.volumes.is_empty() { + return Err(serde::de::Error::custom( + "declares neither `fields:` nor `volumes:`. A service with no policy \ + does not need a block at all — an empty one usually means a \ + declaration was lost, so it is refused rather than silently ignored.", + )); + } + Ok(TargetConfigContract { fields, volumes: raw.volumes, @@ -1793,6 +1807,78 @@ impl StackerConfig { } } + // E008 — a proxy that is switched on must be given everything it needs + // to route. A routing entry with no domain is not a route: NPM rejects + // a proxy host with no name, nginx and caddy have no server_name to + // match on, and traefik's Host() rule matches nothing. It used to reach + // the target anyway, where the NPM role answered with a censored 4xx + // and took the whole deploy with it — after the server was already + // provisioned. The usual cause is an unset `${commonDomain}`: an + // `env_file` line like `commonDomain=` resolves to the empty string + // instead of failing, so the blank travels all the way to Ansible. + // + // Checked for every proxy type but `none`, since all of them route by + // name. An empty `domains:` list is not an error — a proxy can be + // deployed to be configured through its own UI later. + if self.proxy.proxy_type != ProxyType::None { + for (index, domain_config) in self.proxy.domains.iter().enumerate() { + let field = format!("proxy.domains[{index}]"); + + if domain_config.domain.trim().is_empty() { + issues.push(ValidationIssue { + severity: Severity::Error, + code: "E008".to_string(), + message: format!( + "proxy.domains[{index}] has an empty domain, but proxy.type is \ + '{}'. A reverse proxy routes by name, so a blank domain cannot be \ + configured and the deploy fails on the target. If the domain comes \ + from a variable such as ${{commonDomain}}, set it in the env_file or \ + the environment; if this route is not needed yet, remove the entry.", + self.proxy.proxy_type + ), + field: Some(format!("{field}.domain")), + }); + } + + if domain_config.upstream.trim().is_empty() { + issues.push(ValidationIssue { + severity: Severity::Error, + code: "E008".to_string(), + message: format!( + "proxy.domains[{index}] has an empty upstream. The proxy needs a \ + target to forward to, in the form 'service:port' (e.g. 'app:8080')." + ), + field: Some(format!("{field}.upstream")), + }); + } else if let Some((host, port)) = domain_config.upstream.rsplit_once(':') { + // The port half is only a port when the upstream is written + // `host:port`; a bare hostname is accepted and defaults to 80. + if host.trim().is_empty() { + issues.push(ValidationIssue { + severity: Severity::Error, + code: "E008".to_string(), + message: format!( + "proxy.domains[{index}] upstream '{}' has no host before the \ + port. Expected 'service:port', e.g. 'app:8080'.", + domain_config.upstream + ), + field: Some(format!("{field}.upstream")), + }); + } else if let Err(err) = validate_port_number(port) { + issues.push(ValidationIssue { + severity: Severity::Error, + code: "E008".to_string(), + message: format!( + "proxy.domains[{index}] upstream '{}' has an invalid port: {err}.", + domain_config.upstream + ), + field: Some(format!("{field}.upstream")), + }); + } + } + } + } + // W003 — a `proxy:` block deploys a *platform-managed* reverse proxy // that owns the ingress host ports on the target. Any app/service that // also publishes one of those host ports collides with it: the managed @@ -2802,6 +2888,29 @@ config_contract: ); } + /// A service block that declares nothing is almost always a declaration + /// that got lost on the way out — which is exactly what an older CLI did + /// with `volumes:` before it was added to `Serialize`. The submit + /// succeeded, the stored contract had `"my-service": {}`, and the bake + /// then reset a volume the author had asked to keep. Nothing complained. + /// + /// An empty block is also meaningless on its own: a service with no policy + /// simply goes unmentioned. + #[test] + fn an_empty_service_block_is_rejected() { + let yaml = r#" +name: s +config_contract: + services: + my-service: {} +"#; + let err = StackerConfig::from_str(yaml).expect_err("an empty block means nothing"); + assert!( + err.to_string().contains("my-service"), + "the error should name the service: {err}" + ); + } + #[test] fn volume_policy_survives_a_round_trip() { let yaml = r#" @@ -3808,6 +3917,113 @@ app: // into the generated compose verbatim with no range check; a bare // out-of-range entry is read by Docker as a container port. + fn proxy_config_with(proxy_type: ProxyType, domains: Vec<(&str, &str)>) -> StackerConfig { + ConfigBuilder::new() + .name("proxy-validation") + .proxy(ProxyConfig { + proxy_type, + auto_detect: true, + domains: domains + .into_iter() + .map(|(domain, upstream)| DomainConfig { + domain: domain.to_string(), + ssl: SslMode::Auto, + upstream: upstream.to_string(), + }) + .collect(), + config: None, + }) + .build() + .unwrap() + } + + fn errors_with_code(config: &StackerConfig, code: &str) -> Vec { + config + .validate_semantics() + .into_iter() + .filter(|issue| issue.severity == Severity::Error && issue.code == code) + .collect() + } + + #[test] + fn e008_rejects_a_blank_domain_when_a_proxy_is_enabled() { + // The failure this guards: `${commonDomain}` resolved to "" (an + // `env_file` line `commonDomain=`), the blank reached the NPM role, and + // the deploy died with a censored error after the server existed. + for proxy_type in [ + ProxyType::NginxProxyManager, + ProxyType::Nginx, + ProxyType::Caddy, + ProxyType::Traefik, + ] { + let config = proxy_config_with(proxy_type, vec![("", "app:8080")]); + let errors = errors_with_code(&config, "E008"); + assert_eq!( + errors.len(), + 1, + "{proxy_type} should reject a blank domain: {:?}", + config.validate_semantics() + ); + assert_eq!(errors[0].field.as_deref(), Some("proxy.domains[0].domain")); + } + } + + #[test] + fn e008_rejects_a_whitespace_only_domain() { + let config = proxy_config_with(ProxyType::NginxProxyManager, vec![(" ", "app:8080")]); + assert_eq!(errors_with_code(&config, "E008").len(), 1); + } + + #[test] + fn e008_reports_the_index_of_each_bad_entry() { + let config = proxy_config_with( + ProxyType::NginxProxyManager, + vec![("app.example.com", "app:8080"), ("", "api:9000")], + ); + let errors = errors_with_code(&config, "E008"); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].field.as_deref(), Some("proxy.domains[1].domain")); + } + + #[test] + fn e008_rejects_a_blank_or_malformed_upstream() { + let blank = proxy_config_with(ProxyType::Nginx, vec![("app.example.com", "")]); + assert_eq!(errors_with_code(&blank, "E008").len(), 1); + + let no_host = proxy_config_with(ProxyType::Nginx, vec![("app.example.com", ":8080")]); + assert_eq!(errors_with_code(&no_host, "E008").len(), 1); + + let bad_port = proxy_config_with(ProxyType::Nginx, vec![("app.example.com", "app:99999")]); + assert_eq!(errors_with_code(&bad_port, "E008").len(), 1); + } + + #[test] + fn e008_accepts_a_complete_routing_entry() { + let with_port = proxy_config_with( + ProxyType::NginxProxyManager, + vec![("app.example.com", "app:8080")], + ); + assert!(errors_with_code(&with_port, "E008").is_empty()); + + // A bare hostname is legal — the proxy defaults the port to 80. + let no_port = proxy_config_with(ProxyType::Nginx, vec![("app.example.com", "app")]); + assert!(errors_with_code(&no_port, "E008").is_empty()); + } + + #[test] + fn e008_ignores_domains_when_no_proxy_is_enabled() { + // Without a proxy nothing reads these entries, so a blank is inert. + let config = proxy_config_with(ProxyType::None, vec![("", "")]); + assert!(errors_with_code(&config, "E008").is_empty()); + } + + #[test] + fn e008_allows_a_proxy_with_no_routing_entries() { + // A proxy can be deployed to be configured through its own UI later. + let config = proxy_config_with(ProxyType::NginxProxyManager, vec![]); + assert!(errors_with_code(&config, "E008").is_empty()); + } + #[test] fn validate_port_mapping_accepts_every_compose_shape() { for good in [ diff --git a/src/cli/stacker_client.rs b/src/cli/stacker_client.rs index cec28ba8..444d2f25 100644 --- a/src/cli/stacker_client.rs +++ b/src/cli/stacker_client.rs @@ -4396,10 +4396,18 @@ pub fn build_deploy_form(config: &StackerConfig) -> serde_json::Value { // to the Install Service, which passes them to the proxy role as the // `stacker_proxy_domains` extra var. (Traefik routes via container labels // generated into the compose, so it does not need this.) - if !config.proxy.domains.is_empty() { - let domains: Vec = config - .proxy - .domains + // A domain that resolved to nothing (an unset `${commonDomain}`, say) cannot + // become a routing entry: the proxy has no name to match on, and NPM refuses + // the request outright, failing the whole deploy over a route nobody asked + // for. Drop the blanks and route whatever is left. + let routed_domains: Vec<&crate::cli::config_parser::DomainConfig> = config + .proxy + .domains + .iter() + .filter(|d| !d.domain.trim().is_empty()) + .collect(); + if !routed_domains.is_empty() { + let domains: Vec = routed_domains .iter() .map(|d| { let ssl = match d.ssl { @@ -5109,6 +5117,75 @@ mod tests { assert_eq!(domains[1]["ssl"], "off"); } + #[test] + fn test_build_deploy_form_drops_blank_proxy_domains() { + // `${commonDomain}` that resolved to nothing leaves an entry whose domain + // is empty. It reached the NPM role, which answered with a censored 4xx + // and took the whole deploy down (install 4068). A route with no name is + // not a route — it must never leave the CLI. + let config = crate::cli::config_parser::ConfigBuilder::new() + .name("myproject") + .deploy_target(crate::cli::config_parser::DeployTarget::Cloud) + .proxy(crate::cli::config_parser::ProxyConfig { + proxy_type: crate::cli::config_parser::ProxyType::NginxProxyManager, + auto_detect: true, + domains: vec![ + crate::cli::config_parser::DomainConfig { + domain: String::new(), + ssl: crate::cli::config_parser::SslMode::Auto, + upstream: "app:8080".to_string(), + }, + crate::cli::config_parser::DomainConfig { + domain: " ".to_string(), + ssl: crate::cli::config_parser::SslMode::Auto, + upstream: "app:8080".to_string(), + }, + crate::cli::config_parser::DomainConfig { + domain: "app.example.com".to_string(), + ssl: crate::cli::config_parser::SslMode::Auto, + upstream: "app:8080".to_string(), + }, + ], + config: None, + }) + .build() + .unwrap(); + + let form = build_deploy_form(&config); + let domains = form["proxy_domains"] + .as_array() + .expect("the one named domain should still be routed"); + assert_eq!(domains.len(), 1); + assert_eq!(domains[0]["domain"], "app.example.com"); + } + + #[test] + fn test_build_deploy_form_omits_proxy_domains_when_every_domain_is_blank() { + // Nothing left to route: the key must be absent rather than an empty + // array, so the Install Service treats it as "no proxy domains". + let config = crate::cli::config_parser::ConfigBuilder::new() + .name("myproject") + .deploy_target(crate::cli::config_parser::DeployTarget::Cloud) + .proxy(crate::cli::config_parser::ProxyConfig { + proxy_type: crate::cli::config_parser::ProxyType::NginxProxyManager, + auto_detect: true, + domains: vec![crate::cli::config_parser::DomainConfig { + domain: String::new(), + ssl: crate::cli::config_parser::SslMode::Auto, + upstream: "app:8080".to_string(), + }], + config: None, + }) + .build() + .unwrap(); + + let form = build_deploy_form(&config); + assert!( + form.get("proxy_domains").is_none(), + "an all-blank domain list must not produce a proxy_domains key" + ); + } + #[test] fn test_build_deploy_form_omits_proxy_domains_when_none() { let config = crate::cli::config_parser::ConfigBuilder::new()