From 06ebba68874c9bb6434380225460bbb8f3dfc255 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Fri, 22 Mar 2024 12:07:05 +0100 Subject: [PATCH 1/5] chore: derive PartialEq on error enums to aid in testing errors in operators --- crates/stackable-certs/src/ca/mod.rs | 4 ++-- crates/stackable-certs/src/keys/ecdsa.rs | 2 +- crates/stackable-certs/src/keys/rsa.rs | 2 +- crates/stackable-certs/src/lib.rs | 2 +- crates/stackable-operator/src/builder/meta.rs | 2 +- crates/stackable-operator/src/builder/pod/mod.rs | 2 +- crates/stackable-operator/src/builder/pod/volume.rs | 4 ++-- .../stackable-operator/src/commons/authentication/ldap.rs | 2 +- .../stackable-operator/src/commons/authentication/oidc.rs | 2 +- crates/stackable-operator/src/commons/authentication/tls.rs | 2 +- crates/stackable-operator/src/commons/secret_class.rs | 2 +- crates/stackable-operator/src/config/fragment.rs | 6 +++--- crates/stackable-operator/src/kvp/mod.rs | 2 +- crates/stackable-operator/src/product_config_utils.rs | 2 +- crates/stackable-operator/src/time/duration.rs | 2 +- crates/stackable-webhook/src/lib.rs | 2 +- crates/stackable-webhook/src/tls.rs | 2 +- 17 files changed, 21 insertions(+), 21 deletions(-) diff --git a/crates/stackable-certs/src/ca/mod.rs b/crates/stackable-certs/src/ca/mod.rs index f06581c0a..e8938136a 100644 --- a/crates/stackable-certs/src/ca/mod.rs +++ b/crates/stackable-certs/src/ca/mod.rs @@ -32,7 +32,7 @@ pub type Result = std::result::Result; /// Defines all error variants which can occur when creating a CA and/or leaf /// certificates. -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("failed to generate RSA signing key"))] GenerateRsaSigningKey { source: rsa::Error }, @@ -70,7 +70,7 @@ pub enum Error { /// Defines all error variants which can occur when loading a CA from a /// Kubernetes [`Secret`]. -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum SecretError where E: std::error::Error + 'static, diff --git a/crates/stackable-certs/src/keys/ecdsa.rs b/crates/stackable-certs/src/keys/ecdsa.rs index 0f016eb0d..167fca3d1 100644 --- a/crates/stackable-certs/src/keys/ecdsa.rs +++ b/crates/stackable-certs/src/keys/ecdsa.rs @@ -10,7 +10,7 @@ use crate::keys::CertificateKeypair; pub type Result = std::result::Result; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(context(false))] SerializeKeyToPem { source: x509_cert::spki::Error }, diff --git a/crates/stackable-certs/src/keys/rsa.rs b/crates/stackable-certs/src/keys/rsa.rs index eceda60f1..ce85d4c20 100644 --- a/crates/stackable-certs/src/keys/rsa.rs +++ b/crates/stackable-certs/src/keys/rsa.rs @@ -13,7 +13,7 @@ const KEY_SIZE: usize = 4096; pub type Result = std::result::Result; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("failed to create RSA key"))] CreateKey { source: rsa::Error }, diff --git a/crates/stackable-certs/src/lib.rs b/crates/stackable-certs/src/lib.rs index 232a8e615..d1efe37fd 100644 --- a/crates/stackable-certs/src/lib.rs +++ b/crates/stackable-certs/src/lib.rs @@ -39,7 +39,7 @@ pub mod keys; /// Error variants which can be encountered when creating a new /// [`CertificatePair`]. -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum CertificatePairError where E: std::error::Error + 'static, diff --git a/crates/stackable-operator/src/builder/meta.rs b/crates/stackable-operator/src/builder/meta.rs index b1d6396e9..a3f8b7c0a 100644 --- a/crates/stackable-operator/src/builder/meta.rs +++ b/crates/stackable-operator/src/builder/meta.rs @@ -9,7 +9,7 @@ use crate::{ }; // NOTE (Techassi): Think about that name -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum ObjectMetaBuilderError { #[snafu(display("failed to set recommended labels"))] RecommendedLabels { source: LabelError }, diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index aa95be505..44fa856ad 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -33,7 +33,7 @@ pub mod resources; pub mod security; pub mod volume; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("termination grace period is too long (got {duration}, maximum allowed is {max})", max = Duration::from_secs(i64::MAX as u64)))] TerminationGracePeriodTooLong { diff --git a/crates/stackable-operator/src/builder/pod/volume.rs b/crates/stackable-operator/src/builder/pod/volume.rs index bf3f6ad73..708d50f91 100644 --- a/crates/stackable-operator/src/builder/pod/volume.rs +++ b/crates/stackable-operator/src/builder/pod/volume.rs @@ -262,7 +262,7 @@ impl VolumeMountBuilder { } } -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum SecretOperatorVolumeSourceBuilderError { #[snafu(display("failed to parse secret operator volume annotation"))] ParseAnnotation { source: AnnotationError }, @@ -414,7 +414,7 @@ impl ListenerReference { // NOTE (Techassi): We might want to think about these names and how long they // are getting. -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum ListenerOperatorVolumeSourceBuilderError { #[snafu(display("failed to convert listener reference into Kubernetes annotation"))] ListenerReferenceAnnotation { source: AnnotationError }, diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs index 8269701ae..f11724207 100644 --- a/crates/stackable-operator/src/commons/authentication/ldap.rs +++ b/crates/stackable-operator/src/commons/authentication/ldap.rs @@ -17,7 +17,7 @@ use crate::{ pub type Result = std::result::Result; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display( "failed to convert bind credentials (secret class volume) into named Kubernetes volume" diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index d5b611612..331759055 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -19,7 +19,7 @@ pub const DEFAULT_OIDC_WELLKNOWN_PATH: &str = ".well-known/openid-configuration" pub const CLIENT_ID_SECRET_KEY: &str = "clientId"; pub const CLIENT_SECRET_SECRET_KEY: &str = "clientSecret"; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("failed to parse OIDC endpoint url"))] ParseOidcEndpointUrl { source: ParseError }, diff --git a/crates/stackable-operator/src/commons/authentication/tls.rs b/crates/stackable-operator/src/commons/authentication/tls.rs index 90c6e9286..c028be9e2 100644 --- a/crates/stackable-operator/src/commons/authentication/tls.rs +++ b/crates/stackable-operator/src/commons/authentication/tls.rs @@ -23,7 +23,7 @@ pub struct AuthenticationProvider { pub client_cert_secret_class: Option, } -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum TlsClientDetailsError { #[snafu(display("failed to convert secret class volume into named Kubernetes volume"))] SecretClassVolume { source: SecretClassVolumeError }, diff --git a/crates/stackable-operator/src/commons/secret_class.rs b/crates/stackable-operator/src/commons/secret_class.rs index acb2e5f5a..0f19f69ea 100644 --- a/crates/stackable-operator/src/commons/secret_class.rs +++ b/crates/stackable-operator/src/commons/secret_class.rs @@ -7,7 +7,7 @@ use crate::builder::{ SecretOperatorVolumeSourceBuilder, SecretOperatorVolumeSourceBuilderError, VolumeBuilder, }; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum SecretClassVolumeError { #[snafu(display("failed to build secret operator volume"))] SecretOperatorVolume { diff --git a/crates/stackable-operator/src/config/fragment.rs b/crates/stackable-operator/src/config/fragment.rs index e09193bc4..c9e053e17 100644 --- a/crates/stackable-operator/src/config/fragment.rs +++ b/crates/stackable-operator/src/config/fragment.rs @@ -58,7 +58,7 @@ impl<'a> Validator<'a> { } } -#[derive(Debug)] +#[derive(Debug, PartialEq)] struct FieldPath { idents: Vec, } @@ -74,7 +74,7 @@ impl Display for FieldPath { } } -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] #[snafu(display("failed to validate {path}"))] /// An error that occurred when validating an object. /// @@ -85,7 +85,7 @@ pub struct ValidationError { problem: ValidationProblem, } /// A problem that was discovered during validation, with no additional context. -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] enum ValidationProblem { #[snafu(display("field is required"))] FieldRequired, diff --git a/crates/stackable-operator/src/kvp/mod.rs b/crates/stackable-operator/src/kvp/mod.rs index c1d6d7493..c37ce71c0 100644 --- a/crates/stackable-operator/src/kvp/mod.rs +++ b/crates/stackable-operator/src/kvp/mod.rs @@ -150,7 +150,7 @@ where } } -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum KeyValuePairsError { #[snafu(display("key/value pair already exists"))] PairAlreadyExists, diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index ff810340f..e3c86a89b 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -11,7 +11,7 @@ use crate::{ role_utils::{CommonConfiguration, Role}, }; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum ConfigError { #[snafu(display("Invalid configuration found: {reason}"))] InvalidConfiguration { reason: String }, diff --git a/crates/stackable-operator/src/time/duration.rs b/crates/stackable-operator/src/time/duration.rs index c8999dc00..16558511f 100644 --- a/crates/stackable-operator/src/time/duration.rs +++ b/crates/stackable-operator/src/time/duration.rs @@ -25,7 +25,7 @@ use schemars::JsonSchema; use snafu::{OptionExt, ResultExt, Snafu}; use strum::IntoEnumIterator; -#[derive(Debug, Snafu, PartialEq)] +#[derive(Debug, PartialEq, Snafu)] #[snafu(module)] pub enum DurationParseError { #[snafu(display("empty input"))] diff --git a/crates/stackable-webhook/src/lib.rs b/crates/stackable-webhook/src/lib.rs index 64d16f0e9..84d5c8aec 100644 --- a/crates/stackable-webhook/src/lib.rs +++ b/crates/stackable-webhook/src/lib.rs @@ -66,7 +66,7 @@ pub(crate) trait StatefulWebhookHandler { fn call(self, req: Req, state: S) -> Res; } -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("failed to create TLS server"))] CreateTlsServer { source: tls::Error }, diff --git a/crates/stackable-webhook/src/tls.rs b/crates/stackable-webhook/src/tls.rs index 3dfed5f48..f647ae9e5 100644 --- a/crates/stackable-webhook/src/tls.rs +++ b/crates/stackable-webhook/src/tls.rs @@ -16,7 +16,7 @@ use tracing::{error, instrument, warn}; pub type Result = std::result::Result; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("failed to construct TLS server config, bad certificate/key"))] InvalidTlsPrivateKey { source: tokio_rustls::rustls::Error }, From c51a06bca792f766da8f1d6c98c73973207c5b43 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Fri, 22 Mar 2024 12:07:28 +0100 Subject: [PATCH 2/5] chore: bump strum patch level --- Cargo.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6e0c9bdbc..9cc27c98b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,9 +27,9 @@ futures-util = "0.3.30" hyper = { version = "1.0.0", features = ["full"] } hyper-util = "0.1.3" json-patch = "1.0.0" -k8s-openapi = { version = "0.21.0", default-features = false, features = ["schemars","v1_28"] } +k8s-openapi = { version = "0.21.0", default-features = false, features = ["schemars", "v1_28"] } # We use rustls instead of openssl for easier portablitly, e.g. so that we can build stackablectl without the need to vendor (build from source) openssl -kube = { version = "0.88.1", default-features = false, features = ["client","jsonpatch","runtime","derive","rustls-tls"] } +kube = { version = "0.88.1", default-features = false, features = ["client", "jsonpatch", "runtime", "derive", "rustls-tls"] } lazy_static = "1.4.0" opentelemetry = "0.21.0" opentelemetry_sdk = { version = "0.21.0", features = ["rt-tokio"] } @@ -51,7 +51,7 @@ sha2 = { version = "0.10.8", features = ["oid"] } signature = "2.2.0" snafu = "0.8.0" stackable-operator-derive = { path = "stackable-operator-derive" } -strum = { version = "0.26.1", features = ["derive"] } +strum = { version = "0.26.2", features = ["derive"] } syn = "2.0.28" tempfile = "3.7.1" thiserror = "1.0.44" From a3067d55ee4596251b90eaba7715f61487e3cbfb Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Mon, 25 Mar 2024 15:03:21 +0100 Subject: [PATCH 3/5] add custom impls for PartialEq where needed. Some are restriced to `#[cfg(test)]` --- crates/stackable-certs/src/ca/mod.rs | 46 ++++++++++++++++++++++++++-- crates/stackable-certs/src/lib.rs | 19 ++++++++++-- crates/stackable-webhook/src/lib.rs | 2 +- crates/stackable-webhook/src/tls.rs | 27 +++++++++++++++- 4 files changed, 88 insertions(+), 6 deletions(-) diff --git a/crates/stackable-certs/src/ca/mod.rs b/crates/stackable-certs/src/ca/mod.rs index e8938136a..b5f33c306 100644 --- a/crates/stackable-certs/src/ca/mod.rs +++ b/crates/stackable-certs/src/ca/mod.rs @@ -32,7 +32,7 @@ pub type Result = std::result::Result; /// Defines all error variants which can occur when creating a CA and/or leaf /// certificates. -#[derive(Debug, PartialEq, Snafu)] +#[derive(Debug, Snafu)] pub enum Error { #[snafu(display("failed to generate RSA signing key"))] GenerateRsaSigningKey { source: rsa::Error }, @@ -68,9 +68,51 @@ pub enum Error { ParseAuthorityKeyIdentifier { source: x509_cert::der::Error }, } +/// Custom implementation of [`std::cmp::PartialEq`] because some inner types +/// don't implement it. +/// +/// Note that this implementation is restritced to testing because there is a +/// variant that is impossible to compare, and will cause a panic if it +/// attemped. +#[cfg(test)] +impl PartialEq for Error { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + ( + Self::CreateCertificateBuilder { source: lhs_source }, + Self::CreateCertificateBuilder { source: rhs_source }, + ) + | ( + Self::AddCertificateExtension { source: lhs_source }, + Self::AddCertificateExtension { source: rhs_source }, + ) + | ( + Self::BuildCertificate { source: lhs_source }, + Self::BuildCertificate { source: rhs_source }, + ) => match (lhs_source, rhs_source) { + (x509_cert::builder::Error::Asn1(lhs), x509_cert::builder::Error::Asn1(rhs)) => { + lhs == rhs + } + ( + x509_cert::builder::Error::PublicKey(lhs), + x509_cert::builder::Error::PublicKey(rhs), + ) => lhs == rhs, + ( + x509_cert::builder::Error::Signature(_), + x509_cert::builder::Error::Signature(_), + ) => panic!( + "it is impossible to compare the opaque Error contained witin signature::error::Error" + ), + _ => false, + }, + (lhs, rhs) => lhs == rhs, + } + } +} + /// Defines all error variants which can occur when loading a CA from a /// Kubernetes [`Secret`]. -#[derive(Debug, PartialEq, Snafu)] +#[derive(Debug, Snafu)] pub enum SecretError where E: std::error::Error + 'static, diff --git a/crates/stackable-certs/src/lib.rs b/crates/stackable-certs/src/lib.rs index d1efe37fd..1db8d78d9 100644 --- a/crates/stackable-certs/src/lib.rs +++ b/crates/stackable-certs/src/lib.rs @@ -39,7 +39,7 @@ pub mod keys; /// Error variants which can be encountered when creating a new /// [`CertificatePair`]. -#[derive(Debug, PartialEq, Snafu)] +#[derive(Debug, Snafu)] pub enum CertificatePairError where E: std::error::Error + 'static, @@ -75,6 +75,21 @@ where ReadFile { source: std::io::Error }, } +/// Custom implementation of [`std::cmp::PartialEq`] because [`std::io::Error`] doesn't implement it, but [`std::io::ErrorKind`] does. +impl PartialEq for CertificatePairError { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Self::WriteFile { source: lhs_source }, Self::WriteFile { source: rhs_source }) => { + lhs_source.kind() == rhs_source.kind() + } + (Self::ReadFile { source: lhs_source }, Self::ReadFile { source: rhs_source }) => { + lhs_source.kind() == rhs_source.kind() + } + (lhs, rhs) => lhs == rhs, + } + } +} + /// Contains the certificate and the signing / embedded key pair. /// /// A [`CertificateAuthority`](crate::ca::CertificateAuthority) uses this struct @@ -155,7 +170,7 @@ pub enum PrivateKeyType { } /// Private and public key encoding, either DER or PEM. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum KeyEncoding { Pem, Der, diff --git a/crates/stackable-webhook/src/lib.rs b/crates/stackable-webhook/src/lib.rs index 84d5c8aec..64d16f0e9 100644 --- a/crates/stackable-webhook/src/lib.rs +++ b/crates/stackable-webhook/src/lib.rs @@ -66,7 +66,7 @@ pub(crate) trait StatefulWebhookHandler { fn call(self, req: Req, state: S) -> Res; } -#[derive(Debug, PartialEq, Snafu)] +#[derive(Debug, Snafu)] pub enum Error { #[snafu(display("failed to create TLS server"))] CreateTlsServer { source: tls::Error }, diff --git a/crates/stackable-webhook/src/tls.rs b/crates/stackable-webhook/src/tls.rs index f647ae9e5..0dec81c82 100644 --- a/crates/stackable-webhook/src/tls.rs +++ b/crates/stackable-webhook/src/tls.rs @@ -16,7 +16,7 @@ use tracing::{error, instrument, warn}; pub type Result = std::result::Result; -#[derive(Debug, PartialEq, Snafu)] +#[derive(Debug, Snafu)] pub enum Error { #[snafu(display("failed to construct TLS server config, bad certificate/key"))] InvalidTlsPrivateKey { source: tokio_rustls::rustls::Error }, @@ -46,6 +46,31 @@ pub enum Error { }, } +/// Custom implementation of [`std::cmp::PartialEq`] because some inner types +/// don't implement it. +/// +/// Note that this implementation is restritced to testing because there are +/// variants that use [`stackable_certs::ca::Error`] which only implements +/// [`PartialEq`] for tests. +#[cfg(test)] +impl PartialEq for Error { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + ( + Self::BindTcpListener { + source: lhs_source, + socket_addr: lhs_socket_addr, + }, + Self::BindTcpListener { + source: rhs_source, + socket_addr: rhs_socket_addr, + }, + ) => lhs_socket_addr == rhs_socket_addr && lhs_source.kind() == rhs_source.kind(), + (lhs, rhs) => lhs == rhs, + } + } +} + /// A server which terminates TLS connections and allows clients to commnunicate /// via HTTPS with the underlying HTTP router. pub struct TlsServer { From ef5d84cd1fe267d5882d025483e5d8016ca505a0 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Mon, 25 Mar 2024 15:10:33 +0100 Subject: [PATCH 4/5] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a0be789e..75feb9921 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file. ## Changed - Remove `resources` key from `DynamicValues` struct ([#734]). +- Implement `PartialEq` for most _Snafu_ Error enums ([#757]). ## Fixed @@ -22,6 +23,7 @@ All notable changes to this project will be documented in this file. [#734]: https://github.com/stackabletech/operator-rs/pull/734 [#735]: https://github.com/stackabletech/operator-rs/pull/735 +[#757]: https://github.com/stackabletech/operator-rs/pull/757 ## [0.64.0] - 2024-01-31 From 736b80105186929f16ec549fdc00a251d67dc085 Mon Sep 17 00:00:00 2001 From: Nick Date: Tue, 26 Mar 2024 10:49:24 +0100 Subject: [PATCH 5/5] Apply suggestions Co-authored-by: Techassi --- crates/stackable-certs/src/ca/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-certs/src/ca/mod.rs b/crates/stackable-certs/src/ca/mod.rs index b5f33c306..2a1debbce 100644 --- a/crates/stackable-certs/src/ca/mod.rs +++ b/crates/stackable-certs/src/ca/mod.rs @@ -72,7 +72,7 @@ pub enum Error { /// don't implement it. /// /// Note that this implementation is restritced to testing because there is a -/// variant that is impossible to compare, and will cause a panic if it +/// variant that is impossible to compare, and will cause a panic if it is /// attemped. #[cfg(test)] impl PartialEq for Error {