Skip to content

fix(release): repair four gates that reported success without checking #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
loispostula wants to merge 1 commit into from
Open

fix(release): repair four gates that reported success without checking #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 93 additions & 11 deletions src/commands/release/bundle_linux.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,32 @@ async fn download(url: &str) -> anyhow::Result<Vec<u8>> {
super::http::get_bytes(&super::http::client()?, url).await
}

/// The one .deb in `dir`. More than one is a hard failure rather than a choice:
/// picking by sort order would silently ship a stale .deb from an earlier
/// version, or, in cargo-deb's untripled default directory, another package's.
fn select_single_deb(dir: &Path) -> anyhow::Result<PathBuf> {
let mut debs: Vec<PathBuf> = std::fs::read_dir(dir)
.with_context(|| format!("cannot list {}", dir.display()))?
.filter_map(Result::ok)
.map(|entry| entry.path())
.filter(|path| path.extension().is_some_and(|ext| ext == "deb"))
.collect();
debs.sort();
match debs.len() {
1 => Ok(debs.remove(0)),
0 => bail!("cargo deb produced no .deb under {}", dir.display()),
n => bail!(
"expected exactly one .deb under {}, found {n}: {}. \
Refusing to guess which one to ship.",
dir.display(),
debs.iter()
.map(|p| p.file_name().unwrap_or_default().to_string_lossy())
.collect::<Vec<_>>()
.join(", ")
),
}
}

fn make_executable(path: &Path) -> anyhow::Result<()> {
std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755))
.with_context(|| format!("cannot chmod {}", path.display()))
Expand Down Expand Up @@ -247,6 +273,35 @@ pub async fn run(
}

// Package the .deb from the already-built binary.
//
// `-o` is not optional. cargo-deb's default output directory is
// `target_dir_base.join("debian")` with NO target triple (config.rs
// `default_deb_output_dir`; the triple is appended only by
// `target_dependent_path`, which the deb output does not go through), so
// `--target` does NOT move the artifact into a per-triple directory.
// Reading a tripled path without passing `-o` is an unconditional ENOENT.
// The per-triple directory is chosen over cargo-deb's untripled default
// because that default is shared by every package in the workspace, and the
// selection below refuses to choose between multiple .debs.
//
// Cleared, not just created: the target dir survives between runs on the
// persistent build pool, and a .deb left by an earlier version would now be
// a hard failure rather than something sort order silently resolved. The
// AppImage path below clears its own directory for the same reason.
let debian_dir = target_dir.join(TARGET_LINUX).join("debian");
if debian_dir.exists() {
for entry in std::fs::read_dir(&debian_dir)
.with_context(|| format!("cannot list {}", debian_dir.display()))?
.filter_map(Result::ok)
.map(|entry| entry.path())
.filter(|path| path.extension().is_some_and(|ext| ext == "deb"))
{
std::fs::remove_file(&entry)
.with_context(|| format!("cannot remove the stale {}", entry.display()))?;
}
}
std::fs::create_dir_all(&debian_dir)
.with_context(|| format!("cannot create {}", debian_dir.display()))?;
run_step(
"cargo",
&[
Expand All @@ -258,22 +313,16 @@ pub async fn run(
TARGET_LINUX,
"--deb-version",
&options.version,
// An existing directory puts the .deb inside it under cargo-deb's
// own generated filename.
"-o",
&debian_dir.to_string_lossy(),
],
&package_dir,
&[],
)
.await?;
let debian_dir = target_dir.join(TARGET_LINUX).join("debian");
let mut debs: Vec<PathBuf> = std::fs::read_dir(&debian_dir)
.with_context(|| format!("cannot list {}", debian_dir.display()))?
.filter_map(Result::ok)
.map(|entry| entry.path())
.filter(|path| path.extension().is_some_and(|ext| ext == "deb"))
.collect();
debs.sort();
let Some(deb) = debs.into_iter().next() else {
bail!("cargo deb produced no .deb under {}", debian_dir.display());
};
let deb = select_single_deb(&debian_dir)?;

// Assemble the AppDir: binary, desktop entry, icon, AppRun. Nothing else.
let out = target_dir.join(TARGET_LINUX).join("appimage");
Expand Down Expand Up @@ -438,6 +487,39 @@ DYNAMIC SYMBOL TABLE:
assert!(exceeds_floor("x.y", "2.31").is_err());
}

#[test]
fn exactly_one_deb_is_required() {
let dir = tempfile::tempdir().unwrap();
// None: cargo-deb wrote nowhere we looked. This is the shape the
// missing -o produced, except there the directory did not exist at all.
let err = select_single_deb(dir.path()).unwrap_err().to_string();
assert!(err.contains("produced no .deb"), "{err}");

std::fs::write(dir.path().join("app_1.2.3_amd64.deb"), "x").unwrap();
assert_eq!(
select_single_deb(dir.path()).unwrap().file_name().unwrap(),
"app_1.2.3_amd64.deb"
);

// Two: a stale build, or another package sharing cargo-deb's untripled
// default directory. Sort order would have shipped 1.2.3 as 1.2.4.
std::fs::write(dir.path().join("app_1.2.4_amd64.deb"), "x").unwrap();
let err = select_single_deb(dir.path()).unwrap_err().to_string();
assert!(err.contains("found 2"), "{err}");
assert!(err.contains("Refusing to guess"), "{err}");
assert!(err.contains("app_1.2.3_amd64.deb"), "names both: {err}");
assert!(err.contains("app_1.2.4_amd64.deb"), "names both: {err}");
}

#[test]
fn a_missing_directory_is_an_error_not_an_empty_list() {
let dir = tempfile::tempdir().unwrap();
let err = select_single_deb(&dir.path().join("nope"))
.unwrap_err()
.to_string();
assert!(err.contains("cannot list"), "{err}");
}

#[test]
fn desktop_icon_line_is_parsed() {
assert_eq!(
Expand Down
179 changes: 139 additions & 40 deletions src/commands/release/healthcheck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,17 @@
//! credential-less, exactly as a client would: fetch channels.json,
//! deserialize it against the contract types (stronger than schema
//! validation - these ARE the schema), follow every channel/target pointer
//! to its manifest, confirm the index lists the version, HEAD every artifact
//! and detached signature, and download + digest-verify the artifacts
//! pointers actually expose (pointed versions only; full-history sweeps
//! would move hundreds of megabytes per run).
//! to its manifest, confirm the index lists the version, and download +
//! digest-verify the artifacts pointers actually expose (pointed versions
//! only; full-history sweeps would move hundreds of megabytes per run).
//! Detached signatures are HEADed for presence; in `--skip-digest-verification`
//! mode artifacts are HEADed too instead of downloaded.
//!
//! A missing channels.json is "nothing promoted yet", which is healthy. An
//! index gap is reported as repairable. Problems make the command exit
//! nonzero with the report; the workflow maintains the single tracking
//! issue.
//! A missing channels.json is "nothing promoted yet", which is healthy, but
//! ONLY on a definite 404: anything else means we could not find out, and
//! reporting that as healthy would let the workflow close its own tracking
//! issue during an outage. An index gap is reported as repairable. Problems
//! make the command exit nonzero with the report.

use std::collections::BTreeSet;
use std::fmt::{Display, Formatter};
Expand Down Expand Up @@ -79,15 +81,27 @@ impl PrettyPrintable for HealthcheckResult {

use super::http::{Client, client as https_client, head_present};

/// GET collapsing every failure to `None`: a non-2xx status, an unreachable
/// host, or an unreadable body are all the same answer a client would
/// experience.
async fn get_bytes(client: &Client, url: &str) -> Option<Vec<u8>> {
super::http::get_bytes(client, url).await.ok()
}

async fn head_ok(client: &Client, url: &str) -> bool {
head_present(client, url).await
/// `Ok(None)` means the object genuinely is not there (404). An `Err` means we
/// could not find out: a transport failure, DNS, TLS, 5xx.
///
/// The distinction is the difference between "nothing promoted yet, healthy"
/// and "the store is down". Collapsing both to `None` made a full store outage
/// report every app as checked-and-healthy, and the workflow then CLOSED the
/// tracking issue with "Healthy again".
async fn get_bytes(client: &Client, url: &str) -> anyhow::Result<Option<Vec<u8>>> {
match super::http::get_bytes(client, url).await {
Ok(bytes) => Ok(Some(bytes)),
// ONLY 404. 403 is deliberately not treated as absent: this check is
// credential-less against an anonymously readable bucket, so a 403 means
// the read policy is broken, not that the object is missing. Calling
// that "nothing promoted yet" would report a whole store as healthy and
// let the workflow close its own tracking issue. The two mistakes are
// not equally cheap, so anything other than a definite 404 is a problem.
Err(e) => match e.downcast_ref::<super::http::HttpStatus>() {
Some(status) if status.status == hyper::StatusCode::NOT_FOUND => Ok(None),
_ => Err(e),
},
}
}

pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {
Expand All @@ -98,10 +112,18 @@ pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {

for app in &options.apps {
let channels_url = format!("{base}/{}/{app}/channels.json", options.channels_bucket);
let Some(channels_bytes) = get_bytes(&client, &channels_url).await else {
// Healthy: nothing has been promoted for this app yet.
checked.push(format!("{app}: no channels.json yet; nothing promoted"));
continue;
let channels_bytes = match get_bytes(&client, &channels_url).await {
Ok(Some(bytes)) => bytes,
Ok(None) => {
// Healthy: nothing has been promoted for this app yet.
checked.push(format!("{app}: no channels.json yet; nothing promoted"));
continue;
}
Err(e) => {
// Could not determine. Never report this as healthy.
problems.push(format!("{app}: cannot reach {channels_url}: {e:#}"));
continue;
}
};
let channels: Channels = match serde_json::from_slice(&channels_bytes) {
Ok(channels) => channels,
Expand All @@ -121,13 +143,17 @@ pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {

let index_url = format!("{base}/{}/{app}/index.json", options.prod_bucket);
let index: Option<Index> = match get_bytes(&client, &index_url).await {
None => {
Ok(None) => {
problems.push(format!(
"{app}: channels exist but index.json is missing at {index_url}"
));
None
}
Some(bytes) => match serde_json::from_slice::<Index>(&bytes) {
Err(e) => {
problems.push(format!("{app}: cannot reach {index_url}: {e:#}"));
None
}
Ok(Some(bytes)) => match serde_json::from_slice::<Index>(&bytes) {
Ok(index) => {
if index.schema_version != SCHEMA_VERSION {
problems.push(format!(
Expand Down Expand Up @@ -162,11 +188,18 @@ pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {
"{base}/{}/{app}/{version}/manifest.json",
options.prod_bucket
);
let Some(manifest_bytes) = get_bytes(&client, &manifest_url).await else {
problems.push(format!(
"{app}: a channel points at {version} but {manifest_url} is missing"
));
continue;
let manifest_bytes = match get_bytes(&client, &manifest_url).await {
Ok(Some(bytes)) => bytes,
Ok(None) => {
problems.push(format!(
"{app}: a channel points at {version} but {manifest_url} is missing"
));
continue;
}
Err(e) => {
problems.push(format!("{app}: cannot reach {manifest_url}: {e:#}"));
continue;
}
};
let manifest: Manifest = match serde_json::from_slice(&manifest_bytes) {
Ok(manifest) => manifest,
Expand All @@ -193,18 +226,38 @@ pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {
}

for artifact in &manifest.artifacts {
if !head_ok(&client, &artifact.url).await {
problems.push(format!(
"{app} {version}: artifact missing: {}",
artifact.url
));
continue;
}
if !options.skip_digest_verification {
// Pointed versions are what clients download; verify the bytes.
let Some(bytes) = get_bytes(&client, &artifact.url).await else {
problems.push(format!("{app} {version}: cannot download {}", artifact.url));
if options.skip_digest_verification {
// Fast mode: presence only.
if !head_present(&client, &artifact.url).await {
problems.push(format!(
"{app} {version}: artifact missing: {}",
artifact.url
));
continue;
}
} else {
// The GET proves presence by itself, so no HEAD first: it was
// a second round trip against the identical URL for every
// artifact of every pointed version.
let bytes = match get_bytes(&client, &artifact.url).await {
Ok(Some(bytes)) => bytes,
Ok(None) => {
problems.push(format!(
"{app} {version}: artifact missing: {}",
artifact.url
));
continue;
}
Err(e) => {
// Distinct from missing, and the reason matters: this
// is the branch that tells an outage from a broken
// publication.
problems.push(format!(
"{app} {version}: cannot reach {}: {e:#}",
artifact.url
));
continue;
}
};
let got = sha256_hex(&bytes);
if got != artifact.sha256 {
Expand All @@ -215,7 +268,7 @@ pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {
}
}
if let ArtifactSignature::OpenpgpDetached { url: sig_url, .. } = &artifact.signature
&& !head_ok(&client, sig_url).await
&& !head_present(&client, sig_url).await
{
problems.push(format!(
"{app} {version}: detached signature missing: {sig_url}"
Expand All @@ -238,3 +291,49 @@ pub async fn run(options: &Options) -> anyhow::Result<HealthcheckResult> {
Err(anyhow!("{result}"))
}
}

#[cfg(test)]
mod tests {
use super::super::http::HttpStatus;

/// The classification `get_bytes` performs, exercised on the error type
/// rather than through the network. A 404 is "absent, healthy"; everything
/// else must stay an error, because reporting an outage as healthy lets the
/// workflow close its own tracking issue while no client can resolve.
fn classifies_as_absent(status: hyper::StatusCode) -> bool {
let err: anyhow::Error = HttpStatus {
url: "https://x/y.json".into(),
status,
}
.into();
matches!(
err.downcast_ref::<HttpStatus>(),
Some(s) if s.status == hyper::StatusCode::NOT_FOUND
)
}

#[test]
fn only_404_means_absent() {
assert!(classifies_as_absent(hyper::StatusCode::NOT_FOUND));
// 403 in particular: this check is credential-less against an
// anonymously readable bucket, so a 403 is a broken read policy, not a
// missing object. Treating it as absent reported a whole store outage
// as "nothing promoted yet; healthy".
for status in [
hyper::StatusCode::FORBIDDEN,
hyper::StatusCode::UNAUTHORIZED,
hyper::StatusCode::INTERNAL_SERVER_ERROR,
hyper::StatusCode::SERVICE_UNAVAILABLE,
hyper::StatusCode::BAD_GATEWAY,
] {
assert!(!classifies_as_absent(status), "{status} read as absent");
}
}

#[test]
fn a_transport_error_is_never_absent() {
// No HttpStatus in the chain at all: DNS, TLS, connection reset.
let err = anyhow::anyhow!("connection reset by peer");
assert!(err.downcast_ref::<HttpStatus>().is_none());
}
}
Loading