diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 64dbbfc0c..241631014 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,11 +4,20 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- Add `Hostname` and `KerberosRealmName` types extracted from secret-operator ([#851]). + +### Changed + +- BREAKING: `validation` module now uses typed errors ([#851]). + ### Fixed - Fix the CRD description of `ClientAuthenticationDetails` to not contain internal Rust doc, but a public CRD description ([#846]). [#846]: https://github.com/stackabletech/operator-rs/pull/846 +[#851]: https://github.com/stackabletech/operator-rs/pull/851 ## [0.74.0] - 2024-08-22 diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 5bde690be..1b928d703 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -5,20 +5,21 @@ use k8s_openapi::api::core::v1::{ LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector, SecurityContext, VolumeMount, }; -use snafu::Snafu; +use snafu::{ResultExt as _, Snafu}; use crate::{ - commons::product_image_selection::ResolvedProductImage, validation::is_rfc_1123_label, + commons::product_image_selection::ResolvedProductImage, + validation::{self, is_rfc_1123_label}, }; type Result = std::result::Result; -#[derive(Debug, PartialEq, Snafu)] +#[derive(Debug, Snafu)] pub enum Error { - #[snafu(display("container name {container_name:?} is invalid: {violation:?}"))] + #[snafu(display("container name {container_name:?} is invalid"))] InvalidContainerName { + source: validation::Errors, container_name: String, - violation: String, }, } @@ -276,16 +277,8 @@ impl ContainerBuilder { /// Validates a container name is according to the [RFC 1123](https://www.ietf.org/rfc/rfc1123.txt) standard. /// Returns [Ok] if the name is according to the standard, and [Err] if not. - fn validate_container_name(name: &str) -> Result<()> { - let validation_result = is_rfc_1123_label(name); - - match validation_result { - Ok(_) => Ok(()), - Err(err) => Err(Error::InvalidContainerName { - container_name: name.to_owned(), - violation: err.join(", "), - }), - } + fn validate_container_name(container_name: &str) -> Result<()> { + is_rfc_1123_label(container_name).context(InvalidContainerNameSnafu { container_name }) } } @@ -497,19 +490,20 @@ mod tests { "lengthexceededlengthexceededlengthexceededlengthexceededlengthex"; assert_eq!(long_container_name.len(), 64); // 63 characters is the limit for container names let result = ContainerBuilder::new(long_container_name); - match result { - Ok(_) => { - panic!("Container name exceeding 63 characters should cause an error"); + match result + .err() + .expect("Container name exceeding 63 characters should cause an error") + { + Error::InvalidContainerName { + container_name, + source, + } => { + assert_eq!(container_name, long_container_name); + assert_eq!( + source.to_string(), + "input is 64 bytes long but must be no more than 63" + ) } - Err(error) => match error { - Error::InvalidContainerName { - container_name, - violation, - } => { - assert_eq!(container_name.as_str(), long_container_name); - assert_eq!(violation.as_str(), "must be no more than 63 characters") - } - }, } // One characters shorter name is valid let max_len_container_name: String = long_container_name.chars().skip(1).collect(); @@ -527,11 +521,11 @@ mod tests { assert!(ContainerBuilder::new("name-with-hyphen").is_ok()); assert_container_builder_err( ContainerBuilder::new("ends-with-hyphen-"), - "regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?", + "regex used for validation is \"[a-z0-9]([-a-z0-9]*[a-z0-9])?\"", ); assert_container_builder_err( ContainerBuilder::new("-starts-with-hyphen"), - "regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?", + "regex used for validation is \"[a-z0-9]([-a-z0-9]*[a-z0-9])?\"", ); } @@ -545,7 +539,7 @@ mod tests { assert!(ContainerBuilder::new("name_name").is_err()); assert_container_builder_err( ContainerBuilder::new("name_name"), - "(e.g. 'example-label', or '1-label-1', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + "(e.g. \"example-label\", or \"1-label-1\", regex used for validation is \"[a-z0-9]([-a-z0-9]*[a-z0-9])?\")", ); } @@ -573,19 +567,16 @@ mod tests { result: Result, expected_err_contains: &str, ) { - match result { - Ok(_) => { - panic!("Container name exceeding 63 characters should cause an error"); + match result + .err() + .expect("Container name exceeding 63 characters should cause an error") + { + Error::InvalidContainerName { + container_name: _, + source, + } => { + assert!(dbg!(source.to_string()).contains(dbg!(expected_err_contains))); } - Err(error) => match error { - Error::InvalidContainerName { - container_name: _, - violation, - } => { - println!("{violation}"); - assert!(violation.contains(expected_err_contains)); - } - }, } } } diff --git a/crates/stackable-operator/src/commons/mod.rs b/crates/stackable-operator/src/commons/mod.rs index d85054f4c..7f145616e 100644 --- a/crates/stackable-operator/src/commons/mod.rs +++ b/crates/stackable-operator/src/commons/mod.rs @@ -4,6 +4,7 @@ pub mod affinity; pub mod authentication; pub mod cluster_operation; pub mod listener; +pub mod networking; pub mod opa; pub mod pdb; pub mod product_image_selection; diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs new file mode 100644 index 000000000..7081823b6 --- /dev/null +++ b/crates/stackable-operator/src/commons/networking.rs @@ -0,0 +1,76 @@ +use std::{fmt::Display, ops::Deref}; + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::validation; + +/// A validated hostname type, for use in CRDs. +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] +#[serde(try_from = "String", into = "String")] +pub struct Hostname(#[validate(regex(path = "validation::RFC_1123_SUBDOMAIN_REGEX"))] String); + +impl TryFrom for Hostname { + type Error = validation::Errors; + + fn try_from(value: String) -> Result { + validation::is_rfc_1123_subdomain(&value)?; + Ok(Hostname(value)) + } +} + +impl From for String { + fn from(value: Hostname) -> Self { + value.0 + } +} + +impl Display for Hostname { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl Deref for Hostname { + type Target = str; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +/// A validated kerberos realm name type, for use in CRDs. +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] +#[serde(try_from = "String", into = "String")] +pub struct KerberosRealmName( + #[validate(regex(path = "validation::KERBEROS_REALM_NAME_REGEX"))] String, +); + +impl TryFrom for KerberosRealmName { + type Error = validation::Errors; + + fn try_from(value: String) -> Result { + validation::is_kerberos_realm_name(&value)?; + Ok(KerberosRealmName(value)) + } +} + +impl From for String { + fn from(value: KerberosRealmName) -> Self { + value.0 + } +} + +impl Display for KerberosRealmName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl Deref for KerberosRealmName { + type Target = str; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} diff --git a/crates/stackable-operator/src/validation.rs b/crates/stackable-operator/src/validation.rs index 1c253b157..6cf4dbaaa 100644 --- a/crates/stackable-operator/src/validation.rs +++ b/crates/stackable-operator/src/validation.rs @@ -9,10 +9,11 @@ // This is adapted from Kubernetes. // See apimachinery/pkg/util/validation/validation.go, apimachinery/pkg/api/validation/generic.go and pkg/apis/core/validation/validation.go in the Kubernetes source -use std::sync::LazyLock; +use std::{fmt::Display, sync::LazyLock}; use const_format::concatcp; use regex::Regex; +use snafu::Snafu; const RFC_1123_LABEL_FMT: &str = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"; const RFC_1123_SUBDOMAIN_FMT: &str = @@ -26,121 +27,205 @@ const RFC_1123_SUBDOMAIN_MAX_LENGTH: usize = 253; const RFC_1123_LABEL_MAX_LENGTH: usize = 63; const RFC_1035_LABEL_FMT: &str = "[a-z]([-a-z0-9]*[a-z0-9])?"; -const RFC_1035_LABEL_ERR_MSG: &str = "a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character"; +const RFC_1035_LABEL_ERROR_MSG: &str = "a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character"; // This is a label's max length in DNS (RFC 1035) const RFC_1035_LABEL_MAX_LENGTH: usize = 63; +// Technically Kerberos allows more realm names +// (https://web.mit.edu/kerberos/krb5-1.21/doc/admin/realm_config.html#realm-name), +// however, these are embedded in a lot of configuration files and other strings, +// and will not always be quoted properly. +// +// Hence, restrict them to a reasonable subset. The convention is to use upper-case +// DNS hostnames, so allow all characters used there. +const KERBEROS_REALM_NAME_FMT: &str = "[-.a-zA-Z0-9]+"; +const KERBEROS_REALM_NAME_ERROR_MSG: &str = + "Kerberos realm name must only contain alphanumeric characters, '-', and '.'"; + // Lazily initialized regular expressions -static RFC_1123_SUBDOMAIN_REGEX: LazyLock = LazyLock::new(|| { +pub(crate) static RFC_1123_SUBDOMAIN_REGEX: LazyLock = LazyLock::new(|| { Regex::new(&format!("^{RFC_1123_SUBDOMAIN_FMT}$")) .expect("failed to compile RFC 1123 subdomain regex") }); +static RFC_1123_LABEL_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(&format!("^{RFC_1123_LABEL_FMT}$")).expect("failed to compile RFC 1123 label regex") +}); + static RFC_1035_LABEL_REGEX: LazyLock = LazyLock::new(|| { Regex::new(&format!("^{RFC_1035_LABEL_FMT}$")).expect("failed to compile RFC 1035 label regex") }); -/// Returns a formatted error message for maximum length violations. -fn max_len_error(length: usize) -> String { - format!("must be no more than {length} characters") -} +pub(crate) static KERBEROS_REALM_NAME_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(&format!("^{KERBEROS_REALM_NAME_FMT}$")) + .expect("failed to compile Kerberos realm name regex") +}); -/// Returns a formatted error message for regex violations. -/// -/// # Arguments -/// -/// * `msg` - this is the main error message to return -/// * `fmt` - this is the regular expression that did not match the input -/// * `examples` - are optional well, formed examples that would match the regex -fn regex_error(msg: &str, fmt: &str, examples: &[&str]) -> String { - if examples.is_empty() { - return format!("{msg} (regex used for validation is '{fmt}')"); - } +type Result = std::result::Result; - let mut msg = msg.to_string(); - msg.push_str(" (e.g. "); - for (i, example) in examples.iter().enumerate() { - if i > 0 { - msg.push_str(" or "); +/// A collection of errors discovered during validation. +#[derive(Debug)] +pub struct Errors(Vec); + +impl Display for Errors { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for (i, error) in self.0.iter().enumerate() { + let prefix = match i { + 0 => "", + _ => ", ", + }; + write!(f, "{prefix}{error}")?; } - msg.push('\''); - msg.push_str(example); - msg.push_str("', "); + Ok(()) } +} +impl std::error::Error for Errors {} + +/// A single validation error. +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(transparent)] + Regex { source: RegexError }, - msg.push_str("regex used for validation is '"); - msg.push_str(fmt); - msg.push_str("')"); - msg + #[snafu(display("input is {length} bytes long but must be no more than {max_length}"))] + TooLong { length: usize, max_length: usize }, } -/// Tests for a string that conforms to the definition of a subdomain in DNS (RFC 1123). -pub fn is_rfc_1123_subdomain(value: &str) -> Result<(), Vec> { - let mut errors = vec![]; - if value.len() > RFC_1123_SUBDOMAIN_MAX_LENGTH { - errors.push(max_len_error(RFC_1123_SUBDOMAIN_MAX_LENGTH)) +#[derive(Debug)] +pub struct RegexError { + /// The primary error message. + msg: &'static str, + + /// The regex that the input must match. + regex: &'static str, + + /// Examples of valid inputs (if non-empty). + examples: &'static [&'static str], +} + +impl Display for RegexError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let Self { + msg, + regex, + examples, + } = self; + write!(f, "{msg} (")?; + if !examples.is_empty() { + for (i, example) in examples.iter().enumerate() { + let prefix = match i { + 0 => "e.g.", + _ => "or", + }; + write!(f, "{prefix} {example:?}, ")?; + } + } + write!(f, "regex used for validation is {regex:?})") } +} - if !RFC_1123_SUBDOMAIN_REGEX.is_match(value) { - errors.push(regex_error( - RFC_1123_SUBDOMAIN_ERROR_MSG, - RFC_1123_SUBDOMAIN_FMT, - &["example.com"], - )) +impl std::error::Error for RegexError {} + +/// Returns [`Ok`] if `value`'s length fits within `max_length`. +fn validate_str_length(value: &str, max_length: usize) -> Result<(), Error> { + if value.len() > max_length { + TooLongSnafu { + length: value.len(), + max_length, + } + .fail() + } else { + Ok(()) } +} +/// Returns [`Ok`] if `value` matches `regex`. +fn validate_str_regex( + value: &str, + regex: &'static Regex, + error_msg: &'static str, + examples: &'static [&'static str], +) -> Result<(), Error> { + if regex.is_match(value) { + Ok(()) + } else { + Err(RegexError { + msg: error_msg, + regex: regex + .as_str() + // Clean up start/end-of-line markers + .trim_start_matches('^') + .trim_end_matches('$'), + examples, + } + .into()) + } +} + +/// Returns [`Ok`] if *all* validations are [`Ok`], otherwise returns all errors. +fn validate_all(validations: impl IntoIterator>) -> Result { + let errors = validations + .into_iter() + .filter_map(|res| res.err()) + .collect::>(); if errors.is_empty() { Ok(()) } else { - Err(errors) + Err(Errors(errors)) } } +/// Tests for a string that conforms to the definition of a subdomain in DNS (RFC 1123). +pub fn is_rfc_1123_subdomain(value: &str) -> Result { + validate_all([ + validate_str_length(value, RFC_1123_SUBDOMAIN_MAX_LENGTH), + validate_str_regex( + value, + &RFC_1123_SUBDOMAIN_REGEX, + RFC_1123_SUBDOMAIN_ERROR_MSG, + &["example.com"], + ), + ]) +} + /// Tests for a string that conforms to the definition of a label in DNS (RFC 1123). /// Maximum label length supported by k8s is 63 characters (minimum required). -pub fn is_rfc_1123_label(value: &str) -> Result<(), Vec> { - let mut errors = vec![]; - if value.len() > RFC_1123_LABEL_MAX_LENGTH { - errors.push(max_len_error(RFC_1123_LABEL_MAX_LENGTH)) - } - - // Regex is identical to RFC 1123 subdomain - if !RFC_1123_SUBDOMAIN_REGEX.is_match(value) { - errors.push(regex_error( +pub fn is_rfc_1123_label(value: &str) -> Result { + validate_all([ + validate_str_length(value, RFC_1123_LABEL_MAX_LENGTH), + validate_str_regex( + value, + &RFC_1123_LABEL_REGEX, RFC_1123_LABEL_ERROR_MSG, - RFC_1123_SUBDOMAIN_FMT, &["example-label", "1-label-1"], - )) - } - - if errors.is_empty() { - Ok(()) - } else { - Err(errors) - } + ), + ]) } /// Tests for a string that conforms to the definition of a label in DNS (RFC 1035). -pub fn is_rfc_1035_label(value: &str) -> Result<(), Vec> { - let mut errors = vec![]; - if value.len() > RFC_1035_LABEL_MAX_LENGTH { - errors.push(max_len_error(RFC_1035_LABEL_MAX_LENGTH)) - } - - if !RFC_1035_LABEL_REGEX.is_match(value) { - errors.push(regex_error( - RFC_1035_LABEL_ERR_MSG, - RFC_1035_LABEL_FMT, +pub fn is_rfc_1035_label(value: &str) -> Result { + validate_all([ + validate_str_length(value, RFC_1035_LABEL_MAX_LENGTH), + validate_str_regex( + value, + &RFC_1035_LABEL_REGEX, + RFC_1035_LABEL_ERROR_MSG, &["my-name", "abc-123"], - )) - } + ), + ]) +} - if errors.is_empty() { - Ok(()) - } else { - Err(errors) - } +/// Tests whether a string looks like a reasonable Kerberos realm name. +/// +/// This check is much stricter than krb5's own validation, +pub fn is_kerberos_realm_name(value: &str) -> Result { + validate_all([validate_str_regex( + value, + &KERBEROS_REALM_NAME_REGEX, + KERBEROS_REALM_NAME_ERROR_MSG, + &["EXAMPLE.COM"], + )]) } // mask_trailing_dash replaces the final character of a string with a subdomain safe @@ -161,7 +246,7 @@ fn mask_trailing_dash(mut name: String) -> String { /// /// * `name` - is the name to check for validity /// * `prefix` - indicates whether `name` is just a prefix (ending in a dash, which would otherwise not be legal at the end) -pub fn name_is_dns_subdomain(name: &str, prefix: bool) -> Result<(), Vec> { +pub fn name_is_dns_subdomain(name: &str, prefix: bool) -> Result { let mut name = name.to_string(); if prefix { name = mask_trailing_dash(name); @@ -177,7 +262,7 @@ pub fn name_is_dns_subdomain(name: &str, prefix: bool) -> Result<(), Vec /// /// * `name` - is the name to check for validity /// * `prefix` - indicates whether `name` is just a prefix (ending in a dash, which would otherwise not be legal at the end) -pub fn name_is_dns_label(name: &str, prefix: bool) -> Result<(), Vec> { +pub fn name_is_dns_label(name: &str, prefix: bool) -> Result { let mut name = name.to_string(); if prefix { name = mask_trailing_dash(name); @@ -189,7 +274,7 @@ pub fn name_is_dns_label(name: &str, prefix: bool) -> Result<(), Vec> { /// Validates a namespace name. /// /// See [`name_is_dns_label`] for more information. -pub fn validate_namespace_name(name: &str, prefix: bool) -> Result<(), Vec> { +pub fn validate_namespace_name(name: &str, prefix: bool) -> Result { name_is_dns_label(name, prefix) }