From 01e730acf3efeb6ff2181d558cf373ded383a365 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 31 Aug 2023 13:35:43 -0400 Subject: [PATCH] [tufaceous-lib] Reject duplicate artifact names (#3990) Fixes #3988, albeit in a quick-and-dirty way. I have a couple concerns; will leave them as PR comments below. --- Cargo.lock | 1 + tufaceous-lib/Cargo.toml | 3 + tufaceous-lib/src/repository.rs | 130 ++++++++++++------ .../tests/integration-tests/command_tests.rs | 4 +- 4 files changed, 96 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ab3af8829..47c342c5e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9032,6 +9032,7 @@ dependencies = [ "hubtools", "itertools 0.11.0", "omicron-common 0.1.0", + "omicron-test-utils", "rand 0.8.5", "ring", "serde", diff --git a/tufaceous-lib/Cargo.toml b/tufaceous-lib/Cargo.toml index f9712f6635..9178b96fbe 100644 --- a/tufaceous-lib/Cargo.toml +++ b/tufaceous-lib/Cargo.toml @@ -32,3 +32,6 @@ toml.workspace = true tough.workspace = true url = "2.4.0" zip.workspace = true + +[dev-dependencies] +omicron-test-utils.workspace = true diff --git a/tufaceous-lib/src/repository.rs b/tufaceous-lib/src/repository.rs index 2207d00ce4..11a6064602 100644 --- a/tufaceous-lib/src/repository.rs +++ b/tufaceous-lib/src/repository.rs @@ -11,7 +11,7 @@ use omicron_common::{ api::external::SemverVersion, update::{Artifact, ArtifactsDocument}, }; -use std::num::NonZeroU64; +use std::{collections::BTreeSet, num::NonZeroU64}; use tough::{ editor::{signed::SignedRole, RepositoryEditor}, schema::{Root, Target}, @@ -202,20 +202,24 @@ pub struct OmicronRepoEditor { editor: RepositoryEditor, repo_path: Utf8PathBuf, artifacts: ArtifactsDocument, - existing_targets: Vec, + + // Set of `TargetName::resolved()` names for every target that existed when + // the repo was opened. We use this to ensure we don't overwrite an existing + // target when adding new artifacts. + existing_target_names: BTreeSet, } impl OmicronRepoEditor { fn new(repo: OmicronRepo) -> Result { let artifacts = repo.read_artifacts()?; - let existing_targets = repo + let existing_target_names = repo .repo .targets() .signed .targets_iter() - .map(|(name, _)| name.to_owned()) - .collect::>(); + .map(|(name, _)| name.resolved().to_string()) + .collect::>(); let editor = RepositoryEditor::from_repo( repo.repo_path @@ -228,7 +232,7 @@ impl OmicronRepoEditor { editor, repo_path: repo.repo_path, artifacts, - existing_targets, + existing_target_names, }) } @@ -252,53 +256,41 @@ impl OmicronRepoEditor { editor, repo_path, artifacts: ArtifactsDocument::empty(system_version), - existing_targets: vec![], + existing_target_names: BTreeSet::new(), }) } /// Adds an artifact to the repository. pub fn add_artifact(&mut self, new_artifact: &AddArtifact) -> Result<()> { - let filename = format!( - "{}-{}.tar.gz", + let target_name = format!( + "{}-{}-{}.tar.gz", + new_artifact.kind(), new_artifact.name(), new_artifact.version(), ); - // if we already have an artifact of this name/version/kind, replace it. - if let Some(artifact) = - self.artifacts.artifacts.iter_mut().find(|artifact| { - artifact.name == new_artifact.name() - && &artifact.version == new_artifact.version() - && artifact.kind == new_artifact.kind().clone() - }) - { - self.editor.remove_target(&artifact.target.as_str().try_into()?)?; - artifact.target = filename.clone(); - } else { - // if we don't, make sure we're not overriding another target. - if self.existing_targets.iter().any(|target_name| { - target_name.raw() == filename - && target_name.resolved() == filename - }) { - bail!( - "a target named {} already exists in the repository", - filename - ); - } - self.artifacts.artifacts.push(Artifact { - name: new_artifact.name().to_owned(), - version: new_artifact.version().to_owned(), - kind: new_artifact.kind().clone(), - target: filename.clone(), - }) + // make sure we're not overwriting an existing target (either one that + // existed when we opened the repo, or one that's been added via this + // method) + if !self.existing_target_names.insert(target_name.clone()) { + bail!( + "a target named {target_name} already exists in the repository", + ); } + self.artifacts.artifacts.push(Artifact { + name: new_artifact.name().to_owned(), + version: new_artifact.version().to_owned(), + kind: new_artifact.kind().clone(), + target: target_name.clone(), + }); + let targets_dir = self.repo_path.join("targets"); - let mut file = TargetWriter::new(&targets_dir, filename.clone())?; - new_artifact - .write_to(&mut file) - .with_context(|| format!("error writing artifact `{filename}"))?; + let mut file = TargetWriter::new(&targets_dir, target_name.clone())?; + new_artifact.write_to(&mut file).with_context(|| { + format!("error writing artifact `{target_name}") + })?; file.finish(&mut self.editor)?; Ok(()) @@ -344,3 +336,61 @@ fn update_versions( editor.timestamp_expires(expiry); Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::ArtifactSource; + use buf_list::BufList; + use camino_tempfile::Utf8TempDir; + use chrono::Days; + use omicron_test_utils::dev::test_setup_log; + + #[test] + fn reject_artifacts_with_the_same_filename() { + let logctx = test_setup_log("reject_artifacts_with_the_same_filename"); + let tempdir = Utf8TempDir::new().unwrap(); + let mut repo = OmicronRepo::initialize( + &logctx.log, + tempdir.path(), + "0.0.0".parse().unwrap(), + vec![Key::generate_ed25519()], + Utc::now() + Days::new(1), + ) + .unwrap() + .into_editor() + .unwrap(); + + // Targets are uniquely identified by their kind/name/version triple; + // trying to add two artifacts with identical triples should fail. + let kind = "test-kind"; + let name = "test-artifact-name"; + let version = "1.0.0"; + + repo.add_artifact(&AddArtifact::new( + kind.parse().unwrap(), + name.to_string(), + version.parse().unwrap(), + ArtifactSource::Memory(BufList::new()), + )) + .unwrap(); + + let err = repo + .add_artifact(&AddArtifact::new( + kind.parse().unwrap(), + name.to_string(), + version.parse().unwrap(), + ArtifactSource::Memory(BufList::new()), + )) + .unwrap_err() + .to_string(); + + assert!(err.contains("a target named")); + assert!(err.contains(kind)); + assert!(err.contains(name)); + assert!(err.contains(version)); + assert!(err.contains("already exists")); + + logctx.cleanup_successful(); + } +} diff --git a/tufaceous/tests/integration-tests/command_tests.rs b/tufaceous/tests/integration-tests/command_tests.rs index b3a79875c0..73c94572eb 100644 --- a/tufaceous/tests/integration-tests/command_tests.rs +++ b/tufaceous/tests/integration-tests/command_tests.rs @@ -73,7 +73,7 @@ fn test_init_and_add() -> Result<()> { "artifact kind" ); assert_eq!( - artifact.target, "omicron-nexus-42.0.0.tar.gz", + artifact.target, "gimlet_sp-omicron-nexus-42.0.0.tar.gz", "artifact target" ); @@ -86,7 +86,7 @@ fn test_init_and_add() -> Result<()> { "artifact kind" ); assert_eq!( - artifact.target, "my-unknown-kind-0.1.0.tar.gz", + artifact.target, "my_unknown_kind-my-unknown-kind-0.1.0.tar.gz", "artifact target" );