fix(#2860): refuse to write a meta flake with no forge URL
With the agent option nullable, a missing `HIVE_FORGE_URL` would no longer fail anything — it would deploy a whole fleet of agents that silently never log into the forge. The forge is not optional on a running hive, so the hive asserts that itself rather than leaning on a module that legitimately allows "no forge" when evaluated standalone. `sync_agents` checks it before writing anything. That is the moment the hive commits to a flake, and it keeps `render_flake` a pure string operation: the renderer is exercised directly by a dozen tests, so making *it* env-dependent would force each of them to either set a process-wide var — the parallel-test race this module already avoids — or fail for reasons unrelated to what they assert. `require_service_urls` is pure over the already-collected pairs, so its two tests need no process env at all. Refs #2860
This commit is contained in:
parent
bcb9e837f7
commit
dbff9f0987
1 changed files with 91 additions and 10 deletions
|
|
@ -85,6 +85,11 @@ async fn stage_generated_meta_files(dir: &std::path::Path) -> Result<()> {
|
|||
/// no-op.
|
||||
pub async fn sync_agents(hive: &HiveEnv, agents: &[AgentSpec]) -> Result<()> {
|
||||
let _guard = META_LOCK.lock().await;
|
||||
// Before anything is written: a hive without a forge URL would deploy a
|
||||
// whole fleet of agents that silently never log in, because the agent
|
||||
// option treats "unset" as "no forge configured" rather than erroring.
|
||||
// This is the layer that knows a forge is mandatory, so it says so here.
|
||||
require_service_urls(&forwarded_env_vars())?;
|
||||
let dir = crate::paths::meta_root();
|
||||
std::fs::create_dir_all(&dir).with_context(|| format!("create {}", dir.display()))?;
|
||||
|
||||
|
|
@ -715,8 +720,12 @@ const SERVICE_URL_OPTIONS: &[(&str, &str)] = &[
|
|||
/// other under the default parallel test runner. A pure function over the
|
||||
/// already-collected pairs has no such hazard.
|
||||
///
|
||||
/// A var that isn't present emits nothing rather than a guess — see the call
|
||||
/// site for why that silence is the point.
|
||||
/// A var that isn't present emits nothing rather than a guess. For an optional
|
||||
/// service that is the whole point — the agent option defaults to `null`,
|
||||
/// meaning "not configured", and the units that would use it aren't generated.
|
||||
/// For a service the hive cannot run without, silence would instead produce a
|
||||
/// fleet of agents quietly missing an integration, so those are checked by
|
||||
/// [`require_service_urls`] before this is called.
|
||||
fn push_service_url_options(out: &mut String, vars: &[(&'static str, String)]) {
|
||||
use std::fmt::Write as _;
|
||||
for (var, val) in vars {
|
||||
|
|
@ -728,6 +737,48 @@ fn push_service_url_options(out: &mut String, vars: &[(&'static str, String)]) {
|
|||
}
|
||||
}
|
||||
|
||||
/// Service URLs a running hive must always supply, checked before rendering.
|
||||
///
|
||||
/// The forge is not optional on a real hive: `hive-c0re.nix` sets
|
||||
/// `HIVE_FORGE_URL` unconditionally, so its absence means this daemon was
|
||||
/// started outside the NixOS module. The agent option is nullable — `null`
|
||||
/// legitimately means "no forge" when the modules are evaluated on their own —
|
||||
/// which is exactly why the hive has to assert its own requirement here rather
|
||||
/// than leaning on the module to reject the empty case.
|
||||
const REQUIRED_SERVICE_URL_VARS: &[&str] = &["HIVE_FORGE_URL"];
|
||||
|
||||
/// Fails unless every [`REQUIRED_SERVICE_URL_VARS`] entry is present in
|
||||
/// `vars`.
|
||||
///
|
||||
/// Pure over the already-collected pairs so it can be tested without touching
|
||||
/// process env (the same reason [`push_service_url_options`] is split out).
|
||||
///
|
||||
/// Checked by `sync_agents` — the point at which the hive commits a rendered
|
||||
/// flake to disk — rather than inside the renderer. Rendering is a pure string
|
||||
/// operation that many tests exercise directly; making *it* env-dependent
|
||||
/// would mean every one of those tests either sets a process-wide var (the
|
||||
/// parallel-test race this module already avoids) or fails for reasons that
|
||||
/// have nothing to do with what it asserts.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// When a required var is missing. `hive-c0re.nix` sets it unconditionally, so
|
||||
/// this means the daemon is running outside the NixOS module; refusing to
|
||||
/// write the flake beats writing one whose agents would silently all lack a
|
||||
/// forge.
|
||||
fn require_service_urls(vars: &[(&'static str, String)]) -> Result<()> {
|
||||
for required in REQUIRED_SERVICE_URL_VARS {
|
||||
if !vars.iter().any(|(name, _)| name == required) {
|
||||
anyhow::bail!(
|
||||
"{required} is unset — hive-c0re.nix sets it unconditionally, so this process \
|
||||
was started outside the NixOS module. Refusing to write a meta flake whose \
|
||||
agents would every one of them have no forge configured."
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn forwarded_env_vars() -> Vec<(&'static str, String)> {
|
||||
FORWARDED_VARS
|
||||
.iter()
|
||||
|
|
@ -1177,10 +1228,16 @@ where
|
|||
// ever correct when the callee shares the caller's netns, and the forge
|
||||
// and homeserver are moving to swarm level, possibly on other hosts.
|
||||
//
|
||||
// Absent vars emit nothing rather than a guess. Once the defaults are
|
||||
// gone that surfaces as an eval failure, which is the point: better a
|
||||
// build that stops than an agent quietly talking to a port on the wrong
|
||||
// machine.
|
||||
// Absent vars emit nothing rather than a guess, and the agent option
|
||||
// treats "unset" as "this service is not configured" rather than
|
||||
// substituting a loopback address — an absent integration instead of a
|
||||
// misdirected one.
|
||||
//
|
||||
// That is the right default for an optional service and the wrong one for
|
||||
// the forge, which a running hive always has — so `sync_agents` rejects a
|
||||
// missing `HIVE_FORGE_URL` before it writes anything (`require_service_urls`).
|
||||
// The check lives there rather than here because rendering is a pure string
|
||||
// operation the tests exercise directly.
|
||||
push_service_url_options(&mut out, &forwarded_env_vars());
|
||||
// GitHub integration is on by default in every agent
|
||||
// (`hyperhive.github.enable`); the host turns it off hive-wide via
|
||||
|
|
@ -1900,12 +1957,36 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn require_service_urls_accepts_a_rendered_forge_url() {
|
||||
require_service_urls(&[
|
||||
("HIVE_FORGE_URL", "http://forge.example.test".to_string()),
|
||||
("HYPERHIVE_HIVE_NAME", "pr1ma".to_string()),
|
||||
])
|
||||
.expect("a forwarded forge URL satisfies the requirement");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn require_service_urls_refuses_to_write_without_a_forge() {
|
||||
// The agent option is nullable, so nothing downstream would complain:
|
||||
// every agent would simply come up with no forge login and no way to
|
||||
// tell that was unintended. The hive asserts its own requirement
|
||||
// because it is the only layer that knows a forge is mandatory.
|
||||
let err = require_service_urls(&[("HYPERHIVE_HIVE_NAME", "pr1ma".to_string())])
|
||||
.expect_err("a missing forge URL must stop the write");
|
||||
assert!(
|
||||
err.to_string().contains("HIVE_FORGE_URL is unset"),
|
||||
"the error must name the missing variable: {err}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn service_url_options_emit_nothing_when_absent() {
|
||||
// No guess when the host doesn't say. Once the nix-side defaults are
|
||||
// removed this is what turns a missing value into a build failure
|
||||
// rather than an agent quietly talking to a port on the wrong machine,
|
||||
// so the absence has to be as deliberate as the presence.
|
||||
// No guess when the host doesn't say — the agent option stays `null`
|
||||
// ("not configured") and the units that would use it aren't generated,
|
||||
// so absence is an absent integration rather than a misdirected one.
|
||||
// Required services don't reach here: `require_service_urls` rejects
|
||||
// them first.
|
||||
let mut out = String::new();
|
||||
push_service_url_options(&mut out, &[("HYPERHIVE_HIVE_NAME", "pr1ma".to_string())]);
|
||||
assert!(
|
||||
|
|
|
|||
Loading…
Reference in a new issue