From 402cf048ed2f6cf65046dd4147705b88ef29727b Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Wed, 20 Jan 2021 14:49:11 +0100 Subject: [PATCH 1/3] Crash fixed when no tag is provided for the container image --- .gitignore | 1 + Cargo.lock | 31 ++++++++ Cargo.toml | 5 ++ src/provider/mod.rs | 119 +++++++++++++++++++++++++---- src/provider/repository/package.rs | 46 +++++++++-- 5 files changed, 180 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index e69de29..2f7896d 100644 --- a/.gitignore +++ b/.gitignore @@ -0,0 +1 @@ +target/ diff --git a/Cargo.lock b/Cargo.lock index acd6832..dd2f8d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1040,6 +1040,15 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "indoc" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e5a75aeaaef0ce18b58056d306c27b07436fbb34b8816c53094b76dd81803136" +dependencies = [ + "unindent", +] + [[package]] name = "inotify" version = "0.8.3" @@ -2248,6 +2257,19 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "rstest" +version = "0.6.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dec448bc157977efdc0a71369cf923915b0c4806b1b2449c3fb011071d6f7c38" +dependencies = [ + "cfg-if 0.1.10", + "proc-macro2", + "quote", + "rustc_version", + "syn", +] + [[package]] name = "rust-argon2" version = "0.8.2" @@ -2552,6 +2574,7 @@ dependencies = [ "env_logger", "flate2", "handlebars", + "indoc", "k8s-openapi", "kube", "kube-derive", @@ -2560,9 +2583,11 @@ dependencies = [ "oci-distribution", "pnet", "reqwest", + "rstest", "serde", "serde_derive", "serde_json", + "serde_yaml", "stackable_config", "tar", "thiserror", @@ -3361,6 +3386,12 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f7fe0bb3479651439c9112f72b6c505038574c9fbb575ed1bf3b797fa39dd564" +[[package]] +name = "unindent" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f14ee04d9415b52b3aeab06258a3f07093182b88ba0f9b8d203f211a7a7d41c7" + [[package]] name = "untrusted" version = "0.7.1" diff --git a/Cargo.toml b/Cargo.toml index 9ea69a1..243a11d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,3 +30,8 @@ thiserror = "1.0" url = "2.2" pnet = "0.26.0" stackable_config = { git = "https://github.com/stackabletech/common.git", branch = "main" } + +[dev-dependencies] +indoc = "1.0" +rstest = "0.6" +serde_yaml = "0.8" diff --git a/src/provider/mod.rs b/src/provider/mod.rs index d6b042f..858254d 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -3,6 +3,7 @@ use std::fs; use std::path::PathBuf; use std::process::Child; +use anyhow::anyhow; use k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::CustomResourceDefinition; use kube::{Api, Client}; use kubelet::backoff::ExponentialBackoffStrategy; @@ -87,23 +88,23 @@ impl StackableProvider { } fn get_package(pod: &Pod) -> Result { - let containers = pod.containers(); - return if containers.len().ne(&1) { - let e = PodValidationError { - msg: String::from("Size of containers list in PodSpec has to be exactly 1"), - }; - Err(e) + if let Some((container, [])) = pod.containers().split_first() { + container + .image() + .and_then(|maybe_ref| maybe_ref.ok_or(anyhow!("Image is required."))) + .and_then(Package::try_from) + .map_err(|err| PodValidationError { + msg: format!( + r#"Unable to get package reference from pod "{}": {}"#, + &pod.name(), + &err + ), + }) } else { - // List has exactly one value, try to parse this - if let Ok(Some(reference)) = containers[0].image() { - Package::try_from(reference) - } else { - let e = PodValidationError { - msg: format!("Unable to get package reference from pod: {}", &pod.name()), - }; - Err(e) - } - }; + Err(PodValidationError { + msg: String::from("Only one container is supported in the PodSpec."), + }) + } } async fn check_crds(&self) -> Result, StackableError> { @@ -206,3 +207,89 @@ impl Provider for StackableProvider { Ok(()) } } + +#[cfg(test)] +mod test { + use super::*; + use indoc::indoc; + use rstest::rstest; + + #[test] + fn try_to_get_package_from_complete_configuration() { + let pod = parse_pod_from_yaml(indoc! {" + apiVersion: v1 + kind: Pod + metadata: + name: test + spec: + containers: + - name: kafka + image: kafka:2.7 + "}); + + let maybe_package = StackableProvider::get_package(&pod); + + if let Ok(package) = maybe_package { + assert_eq!("kafka", package.product); + assert_eq!("2.7", package.version); + } else { + panic!("Package expected but got {:?}", maybe_package); + } + } + + #[rstest(pod_config, expected_err, + case(indoc! {" + apiVersion: v1 + kind: Pod + metadata: + name: test + spec: + containers: + - name: kafka + image: kafka:2.7 + - name: zookeeper + image: zookeeper:3.6.2 + "}, + "Only one container is supported in the PodSpec." + ), + case(indoc! {" + apiVersion: v1 + kind: Pod + metadata: + name: test + spec: + containers: + - name: kafka + "}, + r#"Unable to get package reference from pod "test": Image is required."# + ), + case(indoc! {" + apiVersion: v1 + kind: Pod + metadata: + name: test + spec: + containers: + - name: kafka + image: kafka + "}, + r#"Unable to get package reference from pod "test": Tag is required."# + ), + )] + fn try_to_get_package_from_insufficient_configuration(pod_config: &str, expected_err: &str) { + let pod = parse_pod_from_yaml(pod_config); + + let maybe_package = StackableProvider::get_package(&pod); + + if let Err(PodValidationError { msg }) = maybe_package { + assert_eq!(expected_err, msg); + } else { + panic!("PodValidationError expected but got {:?}", maybe_package); + } + } + + fn parse_pod_from_yaml(pod_config: &str) -> Pod { + let kube_pod: k8s_openapi::api::core::v1::Pod = serde_yaml::from_str(pod_config).unwrap(); + Pod::from(kube_pod) + } +} diff --git a/src/provider/repository/package.rs b/src/provider/repository/package.rs index c18d6cb..dcaf87c 100644 --- a/src/provider/repository/package.rs +++ b/src/provider/repository/package.rs @@ -1,11 +1,10 @@ use std::convert::TryFrom; use std::fmt; +use anyhow::{anyhow, Result}; use oci_distribution::Reference; use serde::{Deserialize, Serialize}; -use crate::provider::error::StackableError; - #[derive(Serialize, Deserialize, Debug, Clone)] pub struct Package { pub product: String, @@ -31,15 +30,18 @@ impl Package { } impl TryFrom for Package { - type Error = StackableError; + type Error = anyhow::Error; // Converts from an oci reference to a package representation // The oci tag (anything after the \":\" in the string) is used as // version by this code and needs to be present - fn try_from(value: Reference) -> Result { + fn try_from(value: Reference) -> Result { + let repository = value.repository(); + let tag = value.tag().ok_or(anyhow!("Tag is required."))?; + Ok(Package { - product: String::from(value.repository()), - version: String::from(value.tag().unwrap()), + product: String::from(repository), + version: String::from(tag), }) } } @@ -49,3 +51,35 @@ impl fmt::Display for Package { write!(f, "{}:{}", self.product, self.version) } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn try_from_complete_reference() { + let reference = Reference::try_from("kafka:2.7").expect("Reference cannot be parsed."); + + let maybe_package = Package::try_from(reference); + + if let Ok(package) = maybe_package { + assert_eq!("kafka", package.product); + assert_eq!("2.7", package.version); + } else { + panic!("Package expected but got {:?}", maybe_package); + } + } + + #[test] + fn try_from_reference_without_tag() { + let reference = Reference::try_from("kafka").expect("Reference cannot be parsed."); + + let maybe_package = Package::try_from(reference); + + if let Err(error) = maybe_package { + assert_eq!("Tag is required.", error.to_string()); + } else { + panic!("Error expected but got {:?}", maybe_package); + } + } +} From 6387fc984ea7c102c89fea82d5c9354e9406f6af Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 22 Jan 2021 13:20:08 +0100 Subject: [PATCH 2/3] Clippy warnings fixed --- src/provider/mod.rs | 2 +- src/provider/repository/package.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 858254d..8592253 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -91,7 +91,7 @@ impl StackableProvider { if let Some((container, [])) = pod.containers().split_first() { container .image() - .and_then(|maybe_ref| maybe_ref.ok_or(anyhow!("Image is required."))) + .and_then(|maybe_ref| maybe_ref.ok_or_else(|| anyhow!("Image is required."))) .and_then(Package::try_from) .map_err(|err| PodValidationError { msg: format!( diff --git a/src/provider/repository/package.rs b/src/provider/repository/package.rs index dcaf87c..f9e0b46 100644 --- a/src/provider/repository/package.rs +++ b/src/provider/repository/package.rs @@ -37,7 +37,7 @@ impl TryFrom for Package { // version by this code and needs to be present fn try_from(value: Reference) -> Result { let repository = value.repository(); - let tag = value.tag().ok_or(anyhow!("Tag is required."))?; + let tag = value.tag().ok_or_else(|| anyhow!("Tag is required."))?; Ok(Package { product: String::from(repository), From 2ba6621b5b7d38d2e85b371b0b7fb92d0e5aace4 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 22 Jan 2021 13:26:59 +0100 Subject: [PATCH 3/3] Error message adapted according to the guidelines --- src/provider/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 8592253..2071740 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -95,7 +95,7 @@ impl StackableProvider { .and_then(Package::try_from) .map_err(|err| PodValidationError { msg: format!( - r#"Unable to get package reference from pod "{}": {}"#, + "Unable to get package reference from pod [{}]: {}", &pod.name(), &err ), @@ -261,7 +261,7 @@ mod test { containers: - name: kafka "}, - r#"Unable to get package reference from pod "test": Image is required."# + "Unable to get package reference from pod [test]: Image is required." ), case(indoc! {" apiVersion: v1 @@ -273,7 +273,7 @@ mod test { - name: kafka image: kafka "}, - r#"Unable to get package reference from pod "test": Tag is required."# + "Unable to get package reference from pod [test]: Tag is required." ), )] fn try_to_get_package_from_insufficient_configuration(pod_config: &str, expected_err: &str) {