From c4aa3f3048d6767c6ff02db7688f87bffe1afbbb Mon Sep 17 00:00:00 2001 From: "Sean P. Kelly" Date: Fri, 6 Sep 2024 17:37:13 +0000 Subject: [PATCH] twoliter: force vendor lookups to deal with overrides This funnels vendor lookups into receiving an enum, which forces callers handling an image's vendor to explicitly handle overrides. --- twoliter/src/lock/image.rs | 181 ++++++++++-------- twoliter/src/lock/mod.rs | 2 +- twoliter/src/project.rs | 150 ++++++++++++--- twoliter/src/test/cargo_make.rs | 8 +- .../data/override/Twoliter-override-1.toml | 15 ++ .../src/test/data/override/Twoliter.override | 3 + 6 files changed, 251 insertions(+), 108 deletions(-) create mode 100644 twoliter/src/test/data/override/Twoliter-override-1.toml create mode 100644 twoliter/src/test/data/override/Twoliter.override diff --git a/twoliter/src/lock/image.rs b/twoliter/src/lock/image.rs index 0b3e6dcd7..e243e25be 100644 --- a/twoliter/src/lock/image.rs +++ b/twoliter/src/lock/image.rs @@ -3,7 +3,10 @@ use super::views::ManifestListView; use super::Override; use crate::common::fs::create_dir_all; use crate::docker::ImageUri; -use crate::project::{Image, Project, ValidIdentifier}; +use crate::project::{ + ArtifactVendor, Image, OverriddenVendor, Project, ValidIdentifier, VendedArtifact, + VerbatimVendor, +}; use anyhow::{bail, Context, Result}; use base64::Engine; use futures::{pin_mut, stream, StreamExt, TryStreamExt}; @@ -20,11 +23,11 @@ use tracing::{debug, error, info, instrument}; #[derive(Debug, Clone, Eq, Ord, PartialOrd, Serialize, Deserialize)] pub(crate) struct LockedImage { /// The name of the dependency - pub name: String, + pub name: ValidIdentifier, /// The version of the dependency pub version: Version, /// The vendor this dependency came from - pub vendor: String, + pub vendor: ValidIdentifier, /// The resolved image uri of the dependency pub source: String, /// The digest of the image @@ -46,6 +49,20 @@ impl Display for LockedImage { } } +impl VendedArtifact for LockedImage { + fn artifact_name(&self) -> &ValidIdentifier { + &self.name + } + + fn vendor_name(&self) -> &ValidIdentifier { + &self.vendor + } + + fn version(&self) -> &Version { + &self.version + } +} + #[derive(Deserialize, Debug, Clone)] pub(crate) struct ImageMetadata { /// The name of the kit @@ -138,6 +155,22 @@ pub struct VerbatimImage { vendor: String, } +impl VerbatimImage { + fn from_vendor(artifact: &V, vendor: VerbatimVendor) -> Self { + let vendor_name = &vendor.vendor_name; + let vendor = vendor.vendor; + + VerbatimImage { + vendor: vendor_name.as_ref().to_string(), + uri: ImageUri { + registry: Some(vendor.registry.clone()), + repo: artifact.artifact_name().to_string(), + tag: format!("v{}", artifact.version()), + }, + } + } +} + impl ImageResolverImpl for VerbatimImage { fn name(&self) -> String { self.uri.repo.clone() @@ -167,6 +200,22 @@ pub struct OverriddenImage { override_: Override, } +impl OverriddenImage { + fn from_vendor(artifact: &V, vendor: OverriddenVendor) -> Self { + let original_vendor = vendor.original_vendor; + + OverriddenImage { + base_uri: ImageUri { + registry: Some(original_vendor.registry.clone()), + repo: artifact.artifact_name().to_string(), + tag: format!("v{}", artifact.version()), + }, + vendor: vendor.original_vendor_name.as_ref().to_string(), + override_: vendor.override_.clone(), + } + } +} + impl ImageResolverImpl for OverriddenImage { fn name(&self) -> String { self.base_uri.repo.clone() @@ -209,88 +258,60 @@ pub struct ImageResolver { impl ImageResolver { pub(crate) fn from_image(image: &Image, project: &Project) -> Result { - let vendor_name = image.vendor.0.as_str(); - let vendor = project.vendor().get(&image.vendor).context(format!( - "vendor '{}' is not specified in Twoliter.toml", - image.vendor - ))?; - let override_ = project - .overrides() - .get(&image.vendor.to_string()) - .and_then(|x| x.get(&image.name.to_string())); - if let Some(override_) = override_.as_ref() { - debug!( - ?override_, - "Found override for image '{}' with vendor '{}'", image.name, image.vendor - ); - } + let vendor = project.vendor_for(image).with_context(|| { + format!( + "failed to find vendor for image with name '{}' and vendor '{}'", + image.name, image.vendor, + ) + })?; + + let image_resolver_impl = match vendor { + ArtifactVendor::Verbatim(vendor) => { + Box::new(VerbatimImage::from_vendor(image, vendor)) as Box + } + + ArtifactVendor::Overridden(vendor) => { + debug!( + vendor_override = ?vendor, + "Found override for image '{}' with vendor '{}'", image.name, image.vendor + ); + Box::new(OverriddenImage::from_vendor(image, vendor)) as Box + } + }; + Ok(Self { - image_resolver_impl: if let Some(override_) = override_ { - Box::new(OverriddenImage { - base_uri: ImageUri { - registry: Some(vendor.registry.clone()), - repo: image.name.to_string(), - tag: format!("v{}", image.version), - }, - vendor: vendor_name.to_string(), - override_: override_.clone(), - }) - } else { - Box::new(VerbatimImage { - vendor: vendor_name.to_string(), - uri: ImageUri { - registry: Some(vendor.registry.clone()), - repo: image.name.to_string(), - tag: format!("v{}", image.version), - }, - }) - }, + image_resolver_impl, skip_metadata_retrieval: false, }) } pub(crate) fn from_locked_image(locked_image: &LockedImage, project: &Project) -> Result { - let vendor_name = &locked_image.vendor; - let vendor = project - .vendor() - .get(&ValidIdentifier(vendor_name.clone())) - .context(format!( - "failed to find vendor for kit with name '{}' and vendor '{}'", - locked_image.name, locked_image.vendor - ))?; - let override_ = project - .overrides() - .get(&locked_image.vendor) - .and_then(|x| x.get(&locked_image.name)); - if let Some(override_) = override_.as_ref() { - debug!( - ?override_, - "Found override for image '{}' with vendor '{}'", - locked_image.name, - locked_image.vendor - ); - } + let vendor = project.vendor_for(locked_image).with_context(|| { + format!( + "failed to find vendor for image with name '{}' and vendor '{}'", + locked_image.name, locked_image.vendor, + ) + })?; + + let image_resolver_impl = match vendor { + ArtifactVendor::Verbatim(vendor) => { + Box::new(VerbatimImage::from_vendor(locked_image, vendor)) + as Box + } + ArtifactVendor::Overridden(vendor) => { + debug!( + vendor_override = ?vendor, + "Found override for image '{}' with vendor '{}'", + locked_image.name, + locked_image.vendor + ); + Box::new(OverriddenImage::from_vendor(locked_image, vendor)) + as Box + } + }; + Ok(Self { - image_resolver_impl: if let Some(override_) = override_ { - Box::new(OverriddenImage { - base_uri: ImageUri { - registry: Some(vendor.registry.clone()), - repo: locked_image.name.to_string(), - tag: format!("v{}", locked_image.version), - }, - vendor: vendor_name.to_string(), - override_: override_.clone(), - }) - } else { - Box::new(VerbatimImage { - vendor: vendor_name.to_string(), - uri: ImageUri { - registry: Some(vendor.registry.clone()), - repo: locked_image.name.to_string(), - tag: format!("v{}", locked_image.version), - }, - }) - }, + image_resolver_impl, skip_metadata_retrieval: false, }) } @@ -338,9 +359,9 @@ impl ImageResolver { .context("no registry found for image")?; let locked_image = LockedImage { - name: self.image_resolver_impl.name(), + name: self.image_resolver_impl.name().parse()?, version: self.image_resolver_impl.version()?, - vendor: self.image_resolver_impl.vendor(), + vendor: self.image_resolver_impl.vendor().parse()?, // The source is the image uri without the tag, which is the digest source: self.image_resolver_impl.source(), digest: self.calculate_digest(image_tool).await?, diff --git a/twoliter/src/lock/mod.rs b/twoliter/src/lock/mod.rs index afea27bc4..b61a2f67b 100644 --- a/twoliter/src/lock/mod.rs +++ b/twoliter/src/lock/mod.rs @@ -276,7 +276,7 @@ impl Lock { continue; } ensure!( - project.vendor().get(&image.vendor).is_some(), + project.vendor_for(image).is_some(), "vendor '{}' is not specified in Twoliter.toml", image.vendor ); diff --git a/twoliter/src/project.rs b/twoliter/src/project.rs index 824937179..afdc8102c 100644 --- a/twoliter/src/project.rs +++ b/twoliter/src/project.rs @@ -15,6 +15,7 @@ use std::ffi::OsStr; use std::fmt::{Display, Formatter}; use std::hash::Hash; use std::path::{Path, PathBuf}; +use std::str::FromStr; use toml::Table; use tracing::{debug, info, instrument, trace, warn}; @@ -115,10 +116,6 @@ impl Project { self.filepath.clone() } - pub(crate) fn overrides(&self) -> &BTreeMap> { - &self.overrides - } - pub(crate) fn project_dir(&self) -> PathBuf { self.project_dir.clone() } @@ -139,8 +136,28 @@ impl Project { self.release_version.as_str() } - pub(crate) fn vendor(&self) -> &BTreeMap { - &self.vendor + pub(crate) fn vendor_for<'proj, 'arti, V: VendedArtifact>( + &'proj self, + artifact: &'arti V, + ) -> Option> { + let artifact_name = artifact.artifact_name(); + let vendor_name = artifact.vendor_name(); + let vendor = self.vendor.get(vendor_name)?; + + self.overrides + .get(vendor_name.as_ref()) + .and_then(|vendor_overrides| vendor_overrides.get(artifact_name.as_ref())) + .map(|override_| { + ArtifactVendor::Overridden(OverriddenVendor { + original_vendor_name: vendor_name, + original_vendor: vendor, + override_, + }) + }) + .or(Some(ArtifactVendor::Verbatim(VerbatimVendor { + vendor_name, + vendor, + }))) } pub(crate) fn kits(&self) -> Vec { @@ -223,6 +240,37 @@ pub(crate) struct Vendor { pub registry: String, } +/// `ArtifactVendor` represents a vendor associated with an image artifact used in a project. +#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd)] +pub(crate) enum ArtifactVendor<'proj, 'arti> { + /// The project only knows of the given vendor as it is written in Twoliter.toml + Verbatim(VerbatimVendor<'proj, 'arti>), + /// The project has an override expressed in Twoliter.override + Overridden(OverriddenVendor<'proj, 'arti>), +} + +#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd)] +pub(crate) struct VerbatimVendor<'proj, 'arti> { + vendor_name: &'arti ValidIdentifier, + vendor: &'proj Vendor, +} + +#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd)] +pub(crate) struct OverriddenVendor<'proj, 'arti> { + original_vendor_name: &'arti ValidIdentifier, + original_vendor: &'proj Vendor, + override_: &'proj Override, +} + +/// An artifact/vendor name combination used to identify an artifact resolved by Twoliter. +/// +/// This is intended for use in [`Project::vendor_for`] lookups. +pub(crate) trait VendedArtifact: std::fmt::Debug { + fn artifact_name(&self) -> &ValidIdentifier; + fn vendor_name(&self) -> &ValidIdentifier; + fn version(&self) -> &Version; +} + #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] pub(crate) struct ValidIdentifier(pub(crate) String); @@ -235,29 +283,35 @@ impl Serialize for ValidIdentifier { } } +impl FromStr for ValidIdentifier { + type Err = anyhow::Error; + + fn from_str(input: &str) -> Result { + ensure!( + !input.is_empty(), + "cannot define an identifier as an empty string", + ); + + // Check if the input contains any invalid characters + for c in input.chars() { + ensure!( + is_valid_id_char(c), + "invalid character '{}' found in identifier name", + c + ); + } + + Ok(Self(input.to_string())) + } +} + impl<'de> Deserialize<'de> for ValidIdentifier { fn deserialize(deserializer: D) -> std::result::Result where D: Deserializer<'de>, { let input = String::deserialize(deserializer)?; - // Check if the input is empty - if input.is_empty() { - return Err(D::Error::custom( - "cannot define an identifier as an empty string", - )); - } - - // Check if the input contains any invalid characters - for c in input.chars() { - if !is_valid_id_char(c) { - return Err(D::Error::custom(format!( - "invalid character '{}' found in identifier name", - c - ))); - } - } - Ok(Self(input.clone())) + input.parse().map_err(D::Error::custom) } } @@ -297,6 +351,20 @@ impl Display for Image { } } +impl VendedArtifact for Image { + fn artifact_name(&self) -> &ValidIdentifier { + &self.name + } + + fn vendor_name(&self) -> &ValidIdentifier { + &self.vendor + } + + fn version(&self) -> &Version { + &self.version + } +} + /// This is used to `Deserialize` a project, then run validation code before returning a valid /// [`Project`]. This is necessary both because there is no post-deserialization serde hook for /// validation and, even if there was, we need to know the project directory path in order to check @@ -516,6 +584,42 @@ mod test { ); } + #[tokio::test] + async fn test_verbatim_sdk() { + let path = data_dir().join("Twoliter-1.toml"); + let project = Project::load(path).await.unwrap(); + + let sdk = project.sdk.as_ref().unwrap(); + + let vendor = project.vendor_for(sdk).unwrap(); + + assert!(matches!(vendor, ArtifactVendor::Verbatim(_))); + } + + #[tokio::test] + async fn test_overridden_sdk() { + let path = data_dir().join("override/Twoliter-override-1.toml"); + let project = Project::load(path).await.unwrap(); + + let sdk = project.sdk.as_ref().unwrap(); + + let vendor = project.vendor_for(sdk).unwrap(); + + assert_eq!( + vendor, + ArtifactVendor::Overridden(OverriddenVendor { + original_vendor_name: &sdk.vendor, + original_vendor: &Vendor { + registry: "a.com/b".parse().unwrap(), + }, + override_: &Override { + name: Some("my-overridden-sdk".parse().unwrap()), + registry: Some("c.com/d".parse().unwrap()), + }, + }) + ); + } + #[tokio::test] async fn test_vendor_specifications() { let project = UnvalidatedProject { diff --git a/twoliter/src/test/cargo_make.rs b/twoliter/src/test/cargo_make.rs index 41ffcf11b..849fd87ae 100644 --- a/twoliter/src/test/cargo_make.rs +++ b/twoliter/src/test/cargo_make.rs @@ -13,15 +13,15 @@ async fn test_cargo_make() { let project = Project::load(path).await.unwrap(); let version = Version::new(1, 2, 3); let vendor_id = ValidIdentifier("my-vendor".into()); - let vendor = project.vendor().get(&vendor_id).unwrap(); + let registry = "a.com/b"; let lock = Lock { schema_version: project.schema_version(), kit: Vec::new(), sdk: LockedImage { - name: "my-bottlerocket-sdk".to_string(), + name: "my-bottlerocket-sdk".parse().unwrap(), version, - vendor: "my-vendor".to_string(), - source: format!("{}/{}:v{}", vendor.registry, "my-bottlerocket-sdk", "1.2.3"), + vendor: "my-vendor".parse().unwrap(), + source: format!("{}/{}:v{}", registry, "my-bottlerocket-sdk", "1.2.3"), digest: "abc".to_string(), }, }; diff --git a/twoliter/src/test/data/override/Twoliter-override-1.toml b/twoliter/src/test/data/override/Twoliter-override-1.toml new file mode 100644 index 000000000..6900b9e2a --- /dev/null +++ b/twoliter/src/test/data/override/Twoliter-override-1.toml @@ -0,0 +1,15 @@ +schema-version = 1 +release-version = "1.0.0" + +[sdk] +name = "my-bottlerocket-sdk" +version = "1.2.3" +vendor = "my-vendor" + +[vendor.my-vendor] +registry = "a.com/b" + +[[kit]] +name = "my-core-kit" +version = "1.2.3" +vendor = "my-vendor" diff --git a/twoliter/src/test/data/override/Twoliter.override b/twoliter/src/test/data/override/Twoliter.override new file mode 100644 index 000000000..79c77edd3 --- /dev/null +++ b/twoliter/src/test/data/override/Twoliter.override @@ -0,0 +1,3 @@ +[my-vendor.my-bottlerocket-sdk] +name = "my-overridden-sdk" +registry = "c.com/d"