hive-priv: delete the runner credentials before restarting the CI runner
Upstream's ExecStartPre re-registers only when .runner is absent, the labels changed, or the registration token hash changed -- never when the instance URL changed. So writing a fresh token and restarting the runner registers only if the hash happens to differ, which is Forgejo's choice to make: whether the admin registration-token endpoint mints a new token per request or hands back a stable one is not ours to depend on. hive-c0re already decides correctly -- ensure_ci_runner_registered only reaches this helper once it has concluded the credentials are absent or stale -- but the remediation was a no-op, so re-registration was requested every boot and never happened. Remove .runner before the restart so upstream takes its absence branch, the one it evaluates unconditionally. NotFound is success; any other error propagates rather than reporting Ok for a registration that never ran.
This commit is contained in:
parent
8ed4b8c9c3
commit
39a0a313dc
1 changed files with 108 additions and 2 deletions
|
|
@ -853,6 +853,37 @@ async fn restart_matrix_daemon(agent_name: &str) -> Result<(String, String)> {
|
|||
))
|
||||
}
|
||||
|
||||
/// Host path to the hive-ci runner's persisted registration credentials.
|
||||
///
|
||||
/// Paired with `hive-c0re`'s `forge::ci_runner::RUNNER_FILE`, which reads the
|
||||
/// same file to decide whether a runner is registered and whether it still
|
||||
/// names the configured forge host. Deliberately duplicated rather than shared:
|
||||
/// `hive-priv` is the minimal root helper and does not depend on `hive-c0re`.
|
||||
const RUNNER_CREDENTIALS: &str =
|
||||
"/var/lib/nixos-containers/hive-ci/var/lib/gitea-runner/hive/.runner";
|
||||
|
||||
/// Delete the runner's persisted credentials so upstream's `ExecStartPre` takes
|
||||
/// its **absence** branch on the next start.
|
||||
///
|
||||
/// Absence is the state we want, so `NotFound` is success. Anything else — a
|
||||
/// permission error above all — is NOT swallowed: it means the file is still
|
||||
/// there, the restart will take upstream's already-registered branch, and the
|
||||
/// caller would return `Ok` for a registration that never happened. That is the
|
||||
/// same shape as a precondition that "passes" because it could not read the file
|
||||
/// it was checking, and it is worth failing loudly to avoid.
|
||||
///
|
||||
/// Split from [`register_ci_runner`] purely so this rule is testable without a
|
||||
/// container or a `systemctl`.
|
||||
fn clear_runner_credentials(path: &str) -> Result<()> {
|
||||
match std::fs::remove_file(path) {
|
||||
Ok(()) => Ok(()),
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()),
|
||||
Err(e) => {
|
||||
Err(anyhow::Error::new(e).context(format!("remove stale runner credentials {path}")))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// `RegisterCiRunner` — write the runner registration token to the host-side
|
||||
/// `/run/hive-ci/runner-token` env-file, then restart the in-container runner
|
||||
/// so it re-registers. The forge admin token never enters the container; only
|
||||
|
|
@ -875,6 +906,27 @@ async fn register_ci_runner(token: &str) -> Result<(String, String)> {
|
|||
.with_context(|| format!("write {token_path}"))?;
|
||||
std::fs::set_permissions(token_path, std::fs::Permissions::from_mode(0o600))
|
||||
.with_context(|| format!("chmod {token_path}"))?;
|
||||
// Remove the persisted credentials BEFORE restarting, or the restart is a
|
||||
// no-op as far as registration goes.
|
||||
//
|
||||
// Upstream's `ExecStartPre` only re-registers when `.runner` is absent, the
|
||||
// labels changed, or the *registration token hash* changed — never when the
|
||||
// instance URL changed. c0re only calls this helper once it has already
|
||||
// decided the existing credentials are absent or stale
|
||||
// (`forge::ci_runner::ensure_ci_runner_registered` returns early otherwise),
|
||||
// so by the time we are here a re-registration is exactly what is wanted and
|
||||
// deleting the file is the narrow way to guarantee it happens.
|
||||
//
|
||||
// Writing a fresh token is NOT sufficient on its own: whether the hash
|
||||
// changes depends on whether the forge mints a new registration token per
|
||||
// request or hands back a stable one, which is Forgejo's behaviour to
|
||||
// choose and change. Gating our remediation on the absence branch — the one
|
||||
// upstream evaluates unconditionally — makes that question moot instead of
|
||||
// load-bearing.
|
||||
//
|
||||
// See [`clear_runner_credentials`] for why absence is the branch we aim at
|
||||
// and why only `NotFound` counts as success.
|
||||
clear_runner_credentials(RUNNER_CREDENTIALS)?;
|
||||
// Restart the in-container runner so it reads the new token and registers.
|
||||
let out = Command::new("systemctl")
|
||||
.args(["--machine=hive-ci", "restart", "gitea-runner-hive.service"])
|
||||
|
|
@ -2624,8 +2676,8 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S
|
|||
mod tests {
|
||||
use super::{
|
||||
BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement,
|
||||
contains_secret_shaped_run, git_overlay_flags, limits_dropin_body, redact_secret_line,
|
||||
remove_marker_in, write_state_file_nofollow,
|
||||
clear_runner_credentials, contains_secret_shaped_run, git_overlay_flags,
|
||||
limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow,
|
||||
};
|
||||
use std::path::PathBuf;
|
||||
use std::sync::atomic::{AtomicU32, Ordering};
|
||||
|
|
@ -2947,6 +2999,60 @@ mod tests {
|
|||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
|
||||
/// The runner-credential clear, in all three states that matter. The
|
||||
/// PRESENCE arm is the load-bearing one: an implementation that did nothing
|
||||
/// at all would pass the "absent is fine" arm perfectly, and the whole point
|
||||
/// of the call is that the file is *gone* afterwards — upstream re-registers
|
||||
/// on absence and on nothing else we control.
|
||||
#[test]
|
||||
fn clearing_runner_credentials_removes_it_and_tolerates_absence() {
|
||||
let dir = scratch();
|
||||
let path = dir.join(".runner");
|
||||
let path_str = path.to_str().unwrap();
|
||||
|
||||
// Absent → success (this is the state we are aiming for).
|
||||
assert!(!path.exists());
|
||||
clear_runner_credentials(path_str).unwrap();
|
||||
|
||||
// Present → success AND actually gone. Without this arm a no-op passes.
|
||||
std::fs::write(&path, r#"{"id":7,"address":"http://old.invalid"}"#).unwrap();
|
||||
assert!(
|
||||
path.exists(),
|
||||
"control: the file must exist before the clear"
|
||||
);
|
||||
clear_runner_credentials(path_str).unwrap();
|
||||
assert!(
|
||||
!path.exists(),
|
||||
"stale credentials must be GONE, or the restart takes upstream's \
|
||||
already-registered branch and registration silently never happens"
|
||||
);
|
||||
|
||||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
|
||||
/// A failure that is not `NotFound` must propagate, never read as success.
|
||||
/// A directory in the file's place makes `remove_file` fail with a non-
|
||||
/// `NotFound` error without needing to drop privileges in a test.
|
||||
#[test]
|
||||
fn clearing_runner_credentials_propagates_a_real_failure() {
|
||||
let dir = scratch();
|
||||
let path = dir.join(".runner");
|
||||
std::fs::create_dir(&path).unwrap();
|
||||
|
||||
let err = clear_runner_credentials(path.to_str().unwrap())
|
||||
.expect_err("a non-NotFound failure must NOT be reported as success");
|
||||
assert!(
|
||||
format!("{err:#}").contains("remove stale runner credentials"),
|
||||
"error must name what it failed to do, got: {err:#}"
|
||||
);
|
||||
assert!(
|
||||
path.exists(),
|
||||
"nothing was removed, and the caller must know"
|
||||
);
|
||||
|
||||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
|
||||
/// The pause marker round-trips through the same root-only path the
|
||||
/// credential writes use, and BOTH directions are idempotent — the
|
||||
/// dashboard toggle and `hivectl pause|resume` fire blind, without
|
||||
|
|
|
|||
Loading…
Reference in a new issue