From 557845fc42c582ed9ec57ac5acc08a89d4c9a607 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 10 Sep 2024 15:40:51 +0200 Subject: [PATCH 01/26] refactor!: Add Host type. Fix S3 structs to have mandatory fields --- crates/stackable-operator/CHANGELOG.md | 2 + .../src/commons/authentication/ldap.rs | 5 +- .../src/commons/authentication/oidc.rs | 21 ++- .../src/commons/networking.rs | 127 +++++++++++++++++- crates/stackable-operator/src/commons/s3.rs | 87 +++++------- crates/stackable-operator/src/validation.rs | 7 +- 6 files changed, 181 insertions(+), 68 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 0eed91c73..eb7b8ebf0 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file. ### Added - Add `Hostname` and `KerberosRealmName` types extracted from secret-operator ([#851]). +- BREAKING: Add `Host` type and use it within LDAP and OIDC AuthenticationClass as well as S3Connection ([#XXX]). - Add support for listener volume scopes to `SecretOperatorVolumeSourceBuilder` ([#858]). ### Changed @@ -15,6 +16,7 @@ All notable changes to this project will be documented in this file. ### Fixed +- BREAKING: The fields on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory (as they should be) ([#XXX]). - Fix the CRD description of `ClientAuthenticationDetails` to not contain internal Rust doc, but a public CRD description ([#846]). - `StackableAffinity` fields are no longer erroneously marked as required ([#855]). diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs index 35370cce9..ed2af302c 100644 --- a/crates/stackable-operator/src/commons/authentication/ldap.rs +++ b/crates/stackable-operator/src/commons/authentication/ldap.rs @@ -11,6 +11,7 @@ use crate::{ tls::{TlsClientDetails, TlsClientDetailsError}, SECRET_BASE_PATH, }, + networking::Host, secret_class::{SecretClassVolume, SecretClassVolumeError}, }, }; @@ -36,8 +37,8 @@ pub enum Error { )] #[serde(rename_all = "camelCase")] pub struct AuthenticationProvider { - /// Hostname of the LDAP server, for example: `my.ldap.server`. - pub hostname: String, + /// Host of the LDAP server, for example: `my.ldap.server`. + pub hostname: Host, /// Port of the LDAP server. If TLS is used defaults to 636 otherwise to 389. port: Option, diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 3f0c42a22..ac764a100 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -11,7 +11,10 @@ use url::{ParseError, Url}; #[cfg(doc)] use crate::commons::authentication::AuthenticationClass; -use crate::commons::authentication::{tls::TlsClientDetails, SECRET_BASE_PATH}; +use crate::commons::{ + authentication::{tls::TlsClientDetails, SECRET_BASE_PATH}, + networking::Host, +}; pub type Result = std::result::Result; @@ -40,8 +43,8 @@ pub enum Error { )] #[serde(rename_all = "camelCase")] pub struct AuthenticationProvider { - /// Hostname of the identity provider, e.g. `my.keycloak.corp`. - hostname: String, + /// Host of the identity provider, e.g. `my.keycloak.corp`. + hostname: Host, /// Port of the identity provider. If TLS is used defaults to 443, /// otherwise to 80. @@ -90,7 +93,7 @@ fn default_root_path() -> String { impl AuthenticationProvider { pub fn new( - hostname: String, + hostname: Host, port: Option, root_path: String, tls: TlsClientDetails, @@ -113,8 +116,12 @@ impl AuthenticationProvider { /// configuration path, use `url.join()`. This module provides the default /// path at [`DEFAULT_OIDC_WELLKNOWN_PATH`]. pub fn endpoint_url(&self) -> Result { - let mut url = Url::parse(&format!("http://{}:{}", self.hostname, self.port())) - .context(ParseOidcEndpointUrlSnafu)?; + let mut url = Url::parse(&format!( + "http://{}:{}", + self.hostname.as_url_host(), + self.port() + )) + .context(ParseOidcEndpointUrlSnafu)?; if self.tls.uses_tls() { url.set_scheme("https").map_err(|_| { @@ -317,7 +324,7 @@ mod test { fn test_oidc_ipv6_endpoint_url() { let oidc = serde_yaml::from_str::( " - hostname: '[2606:2800:220:1:248:1893:25c8:1946]' + hostname: 2606:2800:220:1:248:1893:25c8:1946 rootPath: my-root-path port: 12345 scopes: [openid] diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 7081823b6..96cbf70fa 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -1,21 +1,31 @@ -use std::{fmt::Display, ops::Deref}; +use std::{fmt::Display, net::IpAddr, ops::Deref, str::FromStr}; 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)] +/// A validated hostname type conforming to RFC 1123, e.g. not an IPv6 address. +#[derive( + Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, JsonSchema, +)] #[serde(try_from = "String", into = "String")] pub struct Hostname(#[validate(regex(path = "validation::RFC_1123_SUBDOMAIN_REGEX"))] String); +impl FromStr for Hostname { + type Err = validation::Errors; + + fn from_str(value: &str) -> Result { + validation::is_rfc_1123_subdomain(&value)?; + Ok(Hostname(value.to_owned())) + } +} + impl TryFrom for Hostname { type Error = validation::Errors; fn try_from(value: String) -> Result { - validation::is_rfc_1123_subdomain(&value)?; - Ok(Hostname(value)) + value.parse() } } @@ -39,6 +49,68 @@ impl Deref for Hostname { } } +/// A validated host (either a [`Hostname`] or IP address) type. +#[derive( + Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, JsonSchema, +)] +#[serde(try_from = "String", into = "String")] +pub enum Host { + IpAddress(IpAddr), + Hostname(Hostname), +} + +impl FromStr for Host { + type Err = validation::Error; + + fn from_str(value: &str) -> Result { + if let Ok(ip) = value.parse::() { + return Ok(Host::IpAddress(ip)); + } + + if let Ok(hostname) = value.parse() { + return Ok(Host::Hostname(hostname)); + }; + + Err(validation::Error::NotAHost {}) + } +} + +impl TryFrom for Host { + type Error = validation::Error; + + fn try_from(value: String) -> Result { + value.parse() + } +} + +impl From for String { + fn from(value: Host) -> Self { + format!("{value}") + } +} + +impl Display for Host { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Host::IpAddress(ip) => write!(f, "{ip}"), + Host::Hostname(hostname) => write!(f, "{hostname}"), + } + } +} + +impl Host { + /// Formats the host in such a way that it can be used in URLs. + pub fn as_url_host(&self) -> String { + match self { + Host::IpAddress(ip) => match ip { + IpAddr::V4(ip) => ip.to_string(), + IpAddr::V6(ip) => format!("[{ip}]"), + }, + Host::Hostname(hostname) => hostname.to_string(), + } + } +} + /// A validated kerberos realm name type, for use in CRDs. #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] #[serde(try_from = "String", into = "String")] @@ -74,3 +146,48 @@ impl Deref for KerberosRealmName { &self.0 } } + +#[cfg(test)] +mod tests { + use super::*; + + use rstest::rstest; + + #[rstest] + #[case("foo")] + #[case("foo.bar")] + // Well this is also a valid hostname I guess + #[case("1.2.3.4")] + fn test_host_and_hostname_parsing_success(#[case] hostname: String) { + let parsed_hostname: Hostname = hostname.parse().expect("hostname can not be parsed"); + // Every host is also a valid hostname + let parsed_host: Host = hostname.parse().expect("host can not be parsed"); + + // Also test the round-trip + assert_eq!(parsed_hostname.to_string(), hostname); + assert_eq!(parsed_host.to_string(), hostname); + } + + #[rstest] + #[case("")] + #[case("foo.bar:1234")] + #[case("fe80::1")] + fn test_hostname_parsing_invalid_input(#[case] hostname: &str) { + assert!(hostname.parse::().is_err()); + } + + #[rstest] + #[case("foo", "foo")] + #[case("foo.bar", "foo.bar")] + #[case("1.2.3.4", "1.2.3.4")] + #[case("fe80::1", "[fe80::1]")] + fn test_host_parsing_success(#[case] host: &str, #[case] expected_url_host: &str) { + // Every host is also a valid hostname + let parsed_host: Host = host.parse().expect("host can not be parsed"); + + // Also test the round-trip + assert_eq!(parsed_host.to_string(), host); + + assert_eq!(parsed_host.as_url_host(), expected_url_host); + } +} diff --git a/crates/stackable-operator/src/commons/s3.rs b/crates/stackable-operator/src/commons/s3.rs index fd2bb8711..c3c35564c 100644 --- a/crates/stackable-operator/src/commons/s3.rs +++ b/crates/stackable-operator/src/commons/s3.rs @@ -11,7 +11,7 @@ use snafu::{ResultExt, Snafu}; use crate::{ client::Client, - commons::{authentication::tls::Tls, secret_class::SecretClassVolume}, + commons::{authentication::tls::Tls, networking::Host, secret_class::SecretClassVolume}, }; type Result = std::result::Result; @@ -35,9 +35,7 @@ pub enum Error { /// S3 bucket specification containing the bucket name and an inlined or referenced connection specification. /// Learn more on the [S3 concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). -#[derive( - Clone, CustomResource, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize, -)] +#[derive(Clone, CustomResource, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] #[kube( group = "s3.stackable.tech", version = "v1alpha1", @@ -53,14 +51,10 @@ pub enum Error { #[serde(rename_all = "camelCase")] pub struct S3BucketSpec { /// The name of the S3 bucket. - // FIXME: Try to remove the Option<>, as this field should be mandatory - #[serde(default, skip_serializing_if = "Option::is_none")] - pub bucket_name: Option, + pub bucket_name: String, /// The definition of an S3 connection, either inline or as a reference. - // FIXME: Try to remove the Option<>, as this field should be mandatory - #[serde(default, skip_serializing_if = "Option::is_none")] - pub connection: Option, + pub connection: S3ConnectionDef, } impl S3BucketSpec { @@ -82,31 +76,26 @@ impl S3BucketSpec { /// Map &self to an [InlinedS3BucketSpec] by obtaining connection spec from the K8S API service if necessary pub async fn inlined(&self, client: &Client, namespace: &str) -> Result { - match self.connection.as_ref() { - Some(connection_def) => Ok(InlinedS3BucketSpec { - connection: Some(connection_def.resolve(client, namespace).await?), - bucket_name: self.bucket_name.clone(), - }), - None => Ok(InlinedS3BucketSpec { - bucket_name: self.bucket_name.clone(), - connection: None, - }), - } + Ok(InlinedS3BucketSpec { + connection: self.connection.resolve(client, namespace).await?, + bucket_name: self.bucket_name.clone(), + }) } } /// Convenience struct with the connection spec inlined. pub struct InlinedS3BucketSpec { - pub bucket_name: Option, - pub connection: Option, + /// Name of the S3 bucket + pub bucket_name: String, + + // docs are on the struct + pub connection: S3ConnectionSpec, } impl InlinedS3BucketSpec { /// Build the endpoint URL from [S3ConnectionSpec::host] and [S3ConnectionSpec::port] and the S3 implementation to use - pub fn endpoint(&self) -> Option { - self.connection - .as_ref() - .and_then(|connection| connection.endpoint()) + pub fn endpoint(&self) -> String { + self.connection.endpoint() } } @@ -163,9 +152,7 @@ impl S3ConnectionDef { /// S3 connection definition as a resource. /// Learn more on the [S3 concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). -#[derive( - CustomResource, Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize, -)] +#[derive(CustomResource, Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] #[kube( group = "s3.stackable.tech", version = "v1alpha1", @@ -180,22 +167,19 @@ impl S3ConnectionDef { )] #[serde(rename_all = "camelCase")] pub struct S3ConnectionSpec { - /// Hostname of the S3 server without any protocol or port. For example: `west1.my-cloud.com`. - // FIXME: Try to remove the Option<>, as this field should be mandatory - #[serde(default, skip_serializing_if = "Option::is_none")] - pub host: Option, + /// Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`. + pub host: Host, /// Port the S3 server listens on. /// If not specified the product will determine the port to use. #[serde(default, skip_serializing_if = "Option::is_none")] pub port: Option, - // FIXME: Try to remove the Option<>, as this field should be mandatory /// Which access style to use. /// Defaults to virtual hosted-style as most of the data products out there. /// Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). - #[serde(default, skip_serializing_if = "Option::is_none")] - pub access_style: Option, + #[serde(default)] + pub access_style: S3AccessStyle, /// If the S3 uses authentication you have to specify you S3 credentials. /// In the most cases a [SecretClass](DOCS_BASE_URL_PLACEHOLDER/secret-operator/secretclass) @@ -226,15 +210,15 @@ impl S3ConnectionSpec { } /// Build the endpoint URL from this connection - pub fn endpoint(&self) -> Option { + pub fn endpoint(&self) -> String { let protocol = match self.tls.as_ref() { Some(_tls) => "https", _ => "http", }; - self.host.as_ref().map(|h| match self.port { - Some(p) => format!("{protocol}://{h}:{p}"), - None => format!("{protocol}://{h}"), - }) + match self.port { + Some(p) => format!("{protocol}://{host}:{p}", host = self.host), + None => format!("{protocol}://{host}", host = self.host), + } } } @@ -245,6 +229,7 @@ impl S3ConnectionSpec { pub enum S3AccessStyle { /// Use path-style access as described in Path, + /// Use as virtual hosted-style access as described in #[default] VirtualHosted, @@ -252,28 +237,24 @@ pub enum S3AccessStyle { #[cfg(test)] mod test { - use std::str; - - use crate::commons::s3::{S3AccessStyle, S3ConnectionDef}; - use crate::commons::s3::{S3BucketSpec, S3ConnectionSpec}; - use crate::yaml; + use super::*; #[test] fn test_ser_inline() { let bucket = S3BucketSpec { - bucket_name: Some("test-bucket-name".to_owned()), - connection: Some(S3ConnectionDef::Inline(S3ConnectionSpec { - host: Some("host".to_owned()), + bucket_name: "test-bucket-name".to_owned(), + connection: S3ConnectionDef::Inline(S3ConnectionSpec { + host: "host".to_string().try_into().unwrap(), port: Some(8080), credentials: None, - access_style: Some(S3AccessStyle::VirtualHosted), + access_style: S3AccessStyle::VirtualHosted, tls: None, - })), + }), }; let mut buf = Vec::new(); - yaml::serialize_to_explicit_document(&mut buf, &bucket).expect("serializable value"); - let actual_yaml = str::from_utf8(&buf).expect("UTF-8 encoded document"); + crate::yaml::serialize_to_explicit_document(&mut buf, &bucket).expect("serializable value"); + let actual_yaml = std::str::from_utf8(&buf).expect("UTF-8 encoded document"); let expected_yaml = "--- bucketName: test-bucket-name diff --git a/crates/stackable-operator/src/validation.rs b/crates/stackable-operator/src/validation.rs index 6cf4dbaaa..fd26205ae 100644 --- a/crates/stackable-operator/src/validation.rs +++ b/crates/stackable-operator/src/validation.rs @@ -23,7 +23,7 @@ const RFC_1123_LABEL_ERROR_MSG: &str = "a lowercase RFC 1123 label must consist // This is a subdomain's max length in DNS (RFC 1123) const RFC_1123_SUBDOMAIN_MAX_LENGTH: usize = 253; -// Minimal length reuquired by RFC 1123 is 63. Up to 255 allowed, unsupported by k8s. +// Minimal length required by RFC 1123 is 63. Up to 255 allowed, unsupported by k8s. const RFC_1123_LABEL_MAX_LENGTH: usize = 63; const RFC_1035_LABEL_FMT: &str = "[a-z]([-a-z0-9]*[a-z0-9])?"; @@ -90,6 +90,11 @@ pub enum Error { #[snafu(display("input is {length} bytes long but must be no more than {max_length}"))] TooLong { length: usize, max_length: usize }, + + #[snafu(display( + "input is not a valid host, which needs to be either a hostname or IP address" + ))] + NotAHost {}, } #[derive(Debug)] From e25c9e0d6bfc58c93af7369a287df63acf76e288 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 10 Sep 2024 16:12:35 +0200 Subject: [PATCH 02/26] TLS verification struct now reside top-level in the `commons` module --- crates/stackable-operator/CHANGELOG.md | 1 + .../src/commons/authentication/ldap.rs | 6 +- .../src/commons/authentication/oidc.rs | 3 +- .../src/commons/authentication/tls.rs | 155 ----------------- crates/stackable-operator/src/commons/mod.rs | 1 + crates/stackable-operator/src/commons/s3.rs | 2 +- .../src/commons/tls_verification.rs | 157 ++++++++++++++++++ 7 files changed, 163 insertions(+), 162 deletions(-) create mode 100644 crates/stackable-operator/src/commons/tls_verification.rs diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index eb7b8ebf0..0cd696316 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -13,6 +13,7 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: `validation` module now uses typed errors ([#851]). +- BREAKING: TLS verification struct now reside top-level in the `commons` module, instead of being placed below `commons::authentication::tls` ([#XXX]). ### Fixed diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs index ed2af302c..1349c9336 100644 --- a/crates/stackable-operator/src/commons/authentication/ldap.rs +++ b/crates/stackable-operator/src/commons/authentication/ldap.rs @@ -7,12 +7,10 @@ use url::{ParseError, Url}; use crate::{ builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder}, commons::{ - authentication::{ - tls::{TlsClientDetails, TlsClientDetailsError}, - SECRET_BASE_PATH, - }, + authentication::SECRET_BASE_PATH, networking::Host, secret_class::{SecretClassVolume, SecretClassVolumeError}, + tls_verification::{TlsClientDetails, TlsClientDetailsError}, }, }; diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index ac764a100..99f1168fb 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -12,8 +12,7 @@ use url::{ParseError, Url}; #[cfg(doc)] use crate::commons::authentication::AuthenticationClass; use crate::commons::{ - authentication::{tls::TlsClientDetails, SECRET_BASE_PATH}, - networking::Host, + authentication::SECRET_BASE_PATH, networking::Host, tls_verification::TlsClientDetails, }; pub type Result = std::result::Result; diff --git a/crates/stackable-operator/src/commons/authentication/tls.rs b/crates/stackable-operator/src/commons/authentication/tls.rs index 5f2e515f3..202e49a10 100644 --- a/crates/stackable-operator/src/commons/authentication/tls.rs +++ b/crates/stackable-operator/src/commons/authentication/tls.rs @@ -1,15 +1,5 @@ -use k8s_openapi::api::core::v1::{Volume, VolumeMount}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use snafu::{ResultExt, Snafu}; - -use crate::{ - builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder}, - commons::{ - authentication::SECRET_BASE_PATH, - secret_class::{SecretClassVolume, SecretClassVolumeError}, - }, -}; #[derive( Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, @@ -22,148 +12,3 @@ pub struct AuthenticationProvider { /// will be used to provision client certificates. pub client_cert_secret_class: Option, } - -#[derive(Debug, PartialEq, Snafu)] -pub enum TlsClientDetailsError { - #[snafu(display("failed to convert secret class volume into named Kubernetes volume"))] - SecretClassVolume { source: SecretClassVolumeError }, -} - -#[derive( - Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, -)] -#[serde(rename_all = "camelCase")] -pub struct TlsClientDetails { - /// Use a TLS connection. If not specified no TLS will be used. - pub tls: Option, -} - -impl TlsClientDetails { - /// This functions adds - /// - /// * The needed volumes to the PodBuilder - /// * The needed volume_mounts to all the ContainerBuilder in the list (e.g. init + main container) - /// - /// This function will handle - /// - /// * Tls secret class used to verify the cert of the LDAP server - pub fn add_volumes_and_mounts( - &self, - pod_builder: &mut PodBuilder, - container_builders: Vec<&mut ContainerBuilder>, - ) -> Result<(), TlsClientDetailsError> { - let (volumes, mounts) = self.volumes_and_mounts()?; - pod_builder.add_volumes(volumes); - - for cb in container_builders { - cb.add_volume_mounts(mounts.clone()); - } - - Ok(()) - } - - /// It is recommended to use [`Self::add_volumes_and_mounts`], this function returns you the - /// volumes and mounts in case you need to add them by yourself. - pub fn volumes_and_mounts( - &self, - ) -> Result<(Vec, Vec), TlsClientDetailsError> { - let mut volumes = Vec::new(); - let mut mounts = Vec::new(); - - if let Some(secret_class) = self.tls_ca_cert_secret_class() { - let volume_name = format!("{secret_class}-ca-cert"); - let secret_class_volume = SecretClassVolume::new(secret_class.clone(), None); - let volume = secret_class_volume - .to_volume(&volume_name) - .context(SecretClassVolumeSnafu)?; - - volumes.push(volume); - mounts.push( - VolumeMountBuilder::new(volume_name, format!("{SECRET_BASE_PATH}/{secret_class}")) - .build(), - ); - } - - Ok((volumes, mounts)) - } - - /// Whether TLS is configured - pub const fn uses_tls(&self) -> bool { - self.tls.is_some() - } - - /// Whether TLS verification is configured. Returns `false` if TLS itself isn't configured - pub fn uses_tls_verification(&self) -> bool { - self.tls - .as_ref() - .map(|tls| tls.verification != TlsVerification::None {}) - .unwrap_or_default() - } - - /// Returns the path of the ca.crt that should be used to verify the LDAP server certificate - /// if TLS verification with a CA cert from a SecretClass is configured. - pub fn tls_ca_cert_mount_path(&self) -> Option { - self.tls_ca_cert_secret_class() - .map(|secret_class| format!("{SECRET_BASE_PATH}/{secret_class}/ca.crt")) - } - - /// Extracts the SecretClass that provides the CA cert used to verify the server certificate. - pub(crate) fn tls_ca_cert_secret_class(&self) -> Option { - if let Some(Tls { - verification: - TlsVerification::Server(TlsServerVerification { - ca_cert: CaCert::SecretClass(secret_class), - }), - }) = &self.tls - { - Some(secret_class.to_owned()) - } else { - None - } - } -} - -#[derive( - Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, -)] -#[serde(rename_all = "camelCase")] -pub struct Tls { - /// The verification method used to verify the certificates of the server and/or the client. - pub verification: TlsVerification, -} - -#[derive( - Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, -)] -#[serde(rename_all = "camelCase")] -pub enum TlsVerification { - /// Use TLS but don't verify certificates. - None {}, - - /// Use TLS and a CA certificate to verify the server. - Server(TlsServerVerification), -} - -#[derive( - Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, -)] -#[serde(rename_all = "camelCase")] -pub struct TlsServerVerification { - /// CA cert to verify the server. - pub ca_cert: CaCert, -} - -#[derive( - Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, -)] -#[serde(rename_all = "camelCase")] -pub enum CaCert { - /// Use TLS and the CA certificates trusted by the common web browsers to verify the server. - /// This can be useful when you e.g. use public AWS S3 or other public available services. - WebPki {}, - - /// Name of the [SecretClass](DOCS_BASE_URL_PLACEHOLDER/secret-operator/secretclass) which will provide the CA certificate. - /// Note that a SecretClass does not need to have a key but can also work with just a CA certificate, - /// so if you got provided with a CA cert but don't have access to the key you can still use this method. - SecretClass(String), -} diff --git a/crates/stackable-operator/src/commons/mod.rs b/crates/stackable-operator/src/commons/mod.rs index 7f145616e..2e61dfe23 100644 --- a/crates/stackable-operator/src/commons/mod.rs +++ b/crates/stackable-operator/src/commons/mod.rs @@ -13,3 +13,4 @@ pub mod resources; pub mod s3; pub mod secret; pub mod secret_class; +pub mod tls_verification; diff --git a/crates/stackable-operator/src/commons/s3.rs b/crates/stackable-operator/src/commons/s3.rs index c3c35564c..ef5d74c7a 100644 --- a/crates/stackable-operator/src/commons/s3.rs +++ b/crates/stackable-operator/src/commons/s3.rs @@ -11,7 +11,7 @@ use snafu::{ResultExt, Snafu}; use crate::{ client::Client, - commons::{authentication::tls::Tls, networking::Host, secret_class::SecretClassVolume}, + commons::{networking::Host, secret_class::SecretClassVolume, tls_verification::Tls}, }; type Result = std::result::Result; diff --git a/crates/stackable-operator/src/commons/tls_verification.rs b/crates/stackable-operator/src/commons/tls_verification.rs new file mode 100644 index 000000000..a29695a95 --- /dev/null +++ b/crates/stackable-operator/src/commons/tls_verification.rs @@ -0,0 +1,157 @@ +use k8s_openapi::api::core::v1::{Volume, VolumeMount}; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use snafu::{ResultExt, Snafu}; + +use crate::{ + builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder}, + commons::{ + authentication::SECRET_BASE_PATH, + secret_class::{SecretClassVolume, SecretClassVolumeError}, + }, +}; + +#[derive(Debug, PartialEq, Snafu)] +pub enum TlsClientDetailsError { + #[snafu(display("failed to convert secret class volume into named Kubernetes volume"))] + SecretClassVolume { source: SecretClassVolumeError }, +} + +#[derive( + Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, +)] +#[serde(rename_all = "camelCase")] +pub struct TlsClientDetails { + /// Use a TLS connection. If not specified no TLS will be used. + pub tls: Option, +} + +impl TlsClientDetails { + /// This functions adds + /// + /// * The needed volumes to the PodBuilder + /// * The needed volume_mounts to all the ContainerBuilder in the list (e.g. init + main container) + /// + /// This function will handle + /// + /// * Tls secret class used to verify the cert of the LDAP server + pub fn add_volumes_and_mounts( + &self, + pod_builder: &mut PodBuilder, + container_builders: Vec<&mut ContainerBuilder>, + ) -> Result<(), TlsClientDetailsError> { + let (volumes, mounts) = self.volumes_and_mounts()?; + pod_builder.add_volumes(volumes); + + for cb in container_builders { + cb.add_volume_mounts(mounts.clone()); + } + + Ok(()) + } + + /// It is recommended to use [`Self::add_volumes_and_mounts`], this function returns you the + /// volumes and mounts in case you need to add them by yourself. + pub fn volumes_and_mounts( + &self, + ) -> Result<(Vec, Vec), TlsClientDetailsError> { + let mut volumes = Vec::new(); + let mut mounts = Vec::new(); + + if let Some(secret_class) = self.tls_ca_cert_secret_class() { + let volume_name = format!("{secret_class}-ca-cert"); + let secret_class_volume = SecretClassVolume::new(secret_class.clone(), None); + let volume = secret_class_volume + .to_volume(&volume_name) + .context(SecretClassVolumeSnafu)?; + + volumes.push(volume); + mounts.push( + VolumeMountBuilder::new(volume_name, format!("{SECRET_BASE_PATH}/{secret_class}")) + .build(), + ); + } + + Ok((volumes, mounts)) + } + + /// Whether TLS is configured + pub const fn uses_tls(&self) -> bool { + self.tls.is_some() + } + + /// Whether TLS verification is configured. Returns `false` if TLS itself isn't configured + pub fn uses_tls_verification(&self) -> bool { + self.tls + .as_ref() + .map(|tls| tls.verification != TlsVerification::None {}) + .unwrap_or_default() + } + + /// Returns the path of the ca.crt that should be used to verify the LDAP server certificate + /// if TLS verification with a CA cert from a SecretClass is configured. + pub fn tls_ca_cert_mount_path(&self) -> Option { + self.tls_ca_cert_secret_class() + .map(|secret_class| format!("{SECRET_BASE_PATH}/{secret_class}/ca.crt")) + } + + /// Extracts the SecretClass that provides the CA cert used to verify the server certificate. + pub(crate) fn tls_ca_cert_secret_class(&self) -> Option { + if let Some(Tls { + verification: + TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::SecretClass(secret_class), + }), + }) = &self.tls + { + Some(secret_class.to_owned()) + } else { + None + } + } +} + +#[derive( + Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, +)] +#[serde(rename_all = "camelCase")] +pub struct Tls { + /// The verification method used to verify the certificates of the server and/or the client. + pub verification: TlsVerification, +} + +#[derive( + Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, +)] +#[serde(rename_all = "camelCase")] +pub enum TlsVerification { + /// Use TLS but don't verify certificates. + None {}, + + /// Use TLS and a CA certificate to verify the server. + Server(TlsServerVerification), +} + +#[derive( + Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, +)] +#[serde(rename_all = "camelCase")] +pub struct TlsServerVerification { + /// CA cert to verify the server. + pub ca_cert: CaCert, +} + +#[derive( + Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize, +)] +#[serde(rename_all = "camelCase")] +pub enum CaCert { + /// Use TLS and the CA certificates trusted by the common web browsers to verify the server. + /// This can be useful when you e.g. use public AWS S3 or other public available services. + WebPki {}, + + /// Name of the [SecretClass](DOCS_BASE_URL_PLACEHOLDER/secret-operator/secretclass) which will provide the CA certificate. + /// Note that a SecretClass does not need to have a key but can also work with just a CA certificate, + /// so if you got provided with a CA cert but don't have access to the key you can still use this method. + SecretClass(String), +} From be75dc21de50aef161887d23916f59f2b9e23f5a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 10 Sep 2024 16:48:31 +0200 Subject: [PATCH 03/26] WIP --- crates/stackable-operator/src/commons/s3.rs | 270 ---------------- .../stackable-operator/src/commons/s3/crd.rs | 88 ++++++ .../src/commons/s3/helpers.rs | 287 ++++++++++++++++++ .../stackable-operator/src/commons/s3/mod.rs | 31 ++ 4 files changed, 406 insertions(+), 270 deletions(-) delete mode 100644 crates/stackable-operator/src/commons/s3.rs create mode 100644 crates/stackable-operator/src/commons/s3/crd.rs create mode 100644 crates/stackable-operator/src/commons/s3/helpers.rs create mode 100644 crates/stackable-operator/src/commons/s3/mod.rs diff --git a/crates/stackable-operator/src/commons/s3.rs b/crates/stackable-operator/src/commons/s3.rs deleted file mode 100644 index ef5d74c7a..000000000 --- a/crates/stackable-operator/src/commons/s3.rs +++ /dev/null @@ -1,270 +0,0 @@ -//! Implementation of the bucket definition as described in -//! the -//! -//! Operator CRDs are expected to use the [S3BucketDef] as an entry point to this module -//! and obtain an [InlinedS3BucketSpec] by calling [`S3BucketDef::resolve`]. -//! -use kube::CustomResource; -use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; -use snafu::{ResultExt, Snafu}; - -use crate::{ - client::Client, - commons::{networking::Host, secret_class::SecretClassVolume, tls_verification::Tls}, -}; - -type Result = std::result::Result; - -#[derive(Debug, Snafu)] -pub enum Error { - #[snafu(display("missing S3Connection {resource_name:?} in namespace {namespace:?}"))] - MissingS3Connection { - source: crate::client::Error, - resource_name: String, - namespace: String, - }, - - #[snafu(display("missing S3Bucket {resource_name:?} in namespace {namespace:?}"))] - MissingS3Bucket { - source: crate::client::Error, - resource_name: String, - namespace: String, - }, -} - -/// S3 bucket specification containing the bucket name and an inlined or referenced connection specification. -/// Learn more on the [S3 concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). -#[derive(Clone, CustomResource, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] -#[kube( - group = "s3.stackable.tech", - version = "v1alpha1", - kind = "S3Bucket", - plural = "s3buckets", - crates( - kube_core = "kube::core", - k8s_openapi = "k8s_openapi", - schemars = "schemars" - ), - namespaced -)] -#[serde(rename_all = "camelCase")] -pub struct S3BucketSpec { - /// The name of the S3 bucket. - pub bucket_name: String, - - /// The definition of an S3 connection, either inline or as a reference. - pub connection: S3ConnectionDef, -} - -impl S3BucketSpec { - /// Convenience function to retrieve the spec of a S3 bucket resource from the K8S API service. - pub async fn get( - resource_name: &str, - client: &Client, - namespace: &str, - ) -> Result { - client - .get::(resource_name, namespace) - .await - .map(|crd| crd.spec) - .context(MissingS3BucketSnafu { - resource_name, - namespace, - }) - } - - /// Map &self to an [InlinedS3BucketSpec] by obtaining connection spec from the K8S API service if necessary - pub async fn inlined(&self, client: &Client, namespace: &str) -> Result { - Ok(InlinedS3BucketSpec { - connection: self.connection.resolve(client, namespace).await?, - bucket_name: self.bucket_name.clone(), - }) - } -} - -/// Convenience struct with the connection spec inlined. -pub struct InlinedS3BucketSpec { - /// Name of the S3 bucket - pub bucket_name: String, - - // docs are on the struct - pub connection: S3ConnectionSpec, -} - -impl InlinedS3BucketSpec { - /// Build the endpoint URL from [S3ConnectionSpec::host] and [S3ConnectionSpec::port] and the S3 implementation to use - pub fn endpoint(&self) -> String { - self.connection.endpoint() - } -} - -/// An S3 bucket definition, it can either be a reference to an explicit S3Bucket object, -/// or it can be an inline definition of a bucket. Read the -/// [S3 resources concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3) -/// to learn more. -#[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub enum S3BucketDef { - /// An inline definition, containing the S3 bucket properties. - Inline(S3BucketSpec), - /// A reference to an S3 bucket object. This is simply the name of the `S3Bucket` - /// resource. - Reference(String), -} - -impl S3BucketDef { - /// Returns an [InlinedS3BucketSpec]. - pub async fn resolve(&self, client: &Client, namespace: &str) -> Result { - match self { - S3BucketDef::Inline(s3_bucket) => s3_bucket.inlined(client, namespace).await, - S3BucketDef::Reference(s3_bucket) => { - S3BucketSpec::get(s3_bucket.as_str(), client, namespace) - .await? - .inlined(client, namespace) - .await - } - } - } -} - -/// Operators are expected to define fields for this type in order to work with S3 connections. -#[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub enum S3ConnectionDef { - /// Inline definition of an S3 connection. - Inline(S3ConnectionSpec), - /// A reference to an S3Connection resource. - Reference(String), -} - -impl S3ConnectionDef { - /// Returns an [S3ConnectionSpec]. - pub async fn resolve(&self, client: &Client, namespace: &str) -> Result { - match self { - S3ConnectionDef::Inline(s3_connection_spec) => Ok(s3_connection_spec.clone()), - S3ConnectionDef::Reference(s3_conn_reference) => { - S3ConnectionSpec::get(s3_conn_reference, client, namespace).await - } - } - } -} - -/// S3 connection definition as a resource. -/// Learn more on the [S3 concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). -#[derive(CustomResource, Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] -#[kube( - group = "s3.stackable.tech", - version = "v1alpha1", - kind = "S3Connection", - plural = "s3connections", - crates( - kube_core = "kube::core", - k8s_openapi = "k8s_openapi", - schemars = "schemars" - ), - namespaced -)] -#[serde(rename_all = "camelCase")] -pub struct S3ConnectionSpec { - /// Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`. - pub host: Host, - - /// Port the S3 server listens on. - /// If not specified the product will determine the port to use. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub port: Option, - - /// Which access style to use. - /// Defaults to virtual hosted-style as most of the data products out there. - /// Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). - #[serde(default)] - pub access_style: S3AccessStyle, - - /// If the S3 uses authentication you have to specify you S3 credentials. - /// In the most cases a [SecretClass](DOCS_BASE_URL_PLACEHOLDER/secret-operator/secretclass) - /// providing `accessKey` and `secretKey` is sufficient. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub credentials: Option, - - /// If you want to use TLS when talking to S3 you can enable TLS encrypted communication with this setting. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub tls: Option, -} - -impl S3ConnectionSpec { - /// Convenience function to retrieve the spec of a S3 connection resource from the K8S API service. - pub async fn get( - resource_name: &str, - client: &Client, - namespace: &str, - ) -> Result { - client - .get::(resource_name, namespace) - .await - .map(|conn| conn.spec) - .context(MissingS3ConnectionSnafu { - resource_name, - namespace, - }) - } - - /// Build the endpoint URL from this connection - pub fn endpoint(&self) -> String { - let protocol = match self.tls.as_ref() { - Some(_tls) => "https", - _ => "http", - }; - match self.port { - Some(p) => format!("{protocol}://{host}:{p}", host = self.host), - None => format!("{protocol}://{host}", host = self.host), - } - } -} - -#[derive( - strum::Display, Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize, -)] -#[strum(serialize_all = "PascalCase")] -pub enum S3AccessStyle { - /// Use path-style access as described in - Path, - - /// Use as virtual hosted-style access as described in - #[default] - VirtualHosted, -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn test_ser_inline() { - let bucket = S3BucketSpec { - bucket_name: "test-bucket-name".to_owned(), - connection: S3ConnectionDef::Inline(S3ConnectionSpec { - host: "host".to_string().try_into().unwrap(), - port: Some(8080), - credentials: None, - access_style: S3AccessStyle::VirtualHosted, - tls: None, - }), - }; - - let mut buf = Vec::new(); - crate::yaml::serialize_to_explicit_document(&mut buf, &bucket).expect("serializable value"); - let actual_yaml = std::str::from_utf8(&buf).expect("UTF-8 encoded document"); - - let expected_yaml = "--- -bucketName: test-bucket-name -connection: - inline: - host: host - port: 8080 - accessStyle: VirtualHosted -"; - - assert_eq!(expected_yaml, actual_yaml) - } -} diff --git a/crates/stackable-operator/src/commons/s3/crd.rs b/crates/stackable-operator/src/commons/s3/crd.rs new file mode 100644 index 000000000..804432085 --- /dev/null +++ b/crates/stackable-operator/src/commons/s3/crd.rs @@ -0,0 +1,88 @@ +use kube::CustomResource; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::commons::{ + networking::Host, secret_class::SecretClassVolume, tls_verification::TlsClientDetails, +}; + +use super::S3ConnectionInlineOrReference; + +/// S3 bucket specification containing the bucket name and an inlined or referenced connection specification. +/// Learn more on the [S3 concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). +#[derive(Clone, CustomResource, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] +#[kube( + group = "s3.stackable.tech", + version = "v1alpha1", + kind = "S3Bucket", + plural = "s3buckets", + crates( + kube_core = "kube::core", + k8s_openapi = "k8s_openapi", + schemars = "schemars" + ), + namespaced +)] +#[serde(rename_all = "camelCase")] +pub struct S3BucketSpec { + /// The name of the S3 bucket. + pub bucket_name: String, + + /// The definition of an S3 connection, either inline or as a reference. + pub connection: S3ConnectionInlineOrReference, +} + +/// S3 connection definition as a resource. +/// Learn more on the [S3 concept documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). +#[derive(CustomResource, Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] +#[kube( + group = "s3.stackable.tech", + version = "v1alpha1", + kind = "S3Connection", + plural = "s3connections", + crates( + kube_core = "kube::core", + k8s_openapi = "k8s_openapi", + schemars = "schemars" + ), + namespaced +)] +#[serde(rename_all = "camelCase")] +pub struct S3ConnectionSpec { + /// Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`. + pub host: Host, + + /// Port the S3 server listens on. + /// If not specified the product will determine the port to use. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub port: Option, + + /// Which access style to use. + /// Defaults to virtual hosted-style as most of the data products out there. + /// Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). + #[serde(default)] + pub access_style: S3AccessStyle, + + /// If the S3 uses authentication you have to specify you S3 credentials. + /// In the most cases a [SecretClass](DOCS_BASE_URL_PLACEHOLDER/secret-operator/secretclass) + /// providing `accessKey` and `secretKey` is sufficient. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub credentials: Option, + + /// Use a TLS connection. If not specified no TLS will be used. + #[serde(flatten)] + pub tls: TlsClientDetails, +} + +#[derive( + strum::Display, Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize, +)] +#[strum(serialize_all = "PascalCase")] +pub enum S3AccessStyle { + /// Use path-style access as described in + Path, + + /// Use as virtual hosted-style access as described in + #[default] + VirtualHosted, +} diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs new file mode 100644 index 000000000..a58b5c297 --- /dev/null +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -0,0 +1,287 @@ +use k8s_openapi::api::core::v1::{Volume, VolumeMount}; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use snafu::ResultExt; +use url::Url; + +use crate::{ + builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder}, + client::Client, + commons::authentication::SECRET_BASE_PATH, +}; + +use super::{ + AddS3CredentialVolumesSnafu, AddS3TlsClientDetailsVolumesSnafu, ParseS3EndpointSnafu, + RetrieveS3ConnectionSnafu, S3Bucket, S3BucketSpec, S3Connection, S3ConnectionSpec, S3Error, + SetS3EndpointSchemeSnafu, +}; + +#[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +// TODO: This probably should be serde(untagged), but this would be a breaking change +pub enum S3ConnectionInlineOrReference { + Inline(S3ConnectionSpec), + Reference(String), +} + +/// Use this type in you operator! +pub type ResolvedS3Connection = S3ConnectionSpec; + +impl S3ConnectionInlineOrReference { + pub async fn resolve( + self, + client: &Client, + namespace: &str, + ) -> Result { + match self { + Self::Inline(inline) => Ok(inline), + Self::Reference(reference) => Ok(client + .get::(&reference, namespace) + .await + .context(RetrieveS3ConnectionSnafu)? + .spec), + } + } +} + +impl ResolvedS3Connection { + /// Build the endpoint URL from this connection + pub fn endpoint(&self) -> Result { + let mut url = Url::parse(&format!( + "http://{}:{}", + self.host.as_url_host(), + self.port() + )) + .context(ParseS3EndpointSnafu)?; + + if self.tls.uses_tls() { + url.set_scheme("https").map_err(|_| { + SetS3EndpointSchemeSnafu { + scheme: "https".to_string(), + endpoint: url.clone(), + } + .build() + })?; + } + + Ok(url) + } + + /// Returns the port to be used, which is either user configured or defaulted based upon TLS usage + pub fn port(&self) -> u16 { + self.port + .unwrap_or(if self.tls.uses_tls() { 443 } else { 80 }) + } + + /// This functions adds + /// + /// * Credentials needed to connect to S3 + /// * Needed TLS volumes + pub fn add_volumes_and_mounts( + &self, + pod_builder: &mut PodBuilder, + container_builders: Vec<&mut ContainerBuilder>, + ) -> Result<(), S3Error> { + let (volumes, mounts) = self.volumes_and_mounts()?; + pod_builder.add_volumes(volumes); + for cb in container_builders { + cb.add_volume_mounts(mounts.clone()); + } + + Ok(()) + } + + /// It is recommended to use [`Self::add_volumes_and_mounts`], this function returns you the + /// volumes and mounts in case you need to add them by yourself. + pub fn volumes_and_mounts(&self) -> Result<(Vec, Vec), S3Error> { + let mut volumes = Vec::new(); + let mut mounts = Vec::new(); + + if let Some(credentials) = &self.credentials { + let secret_class = &credentials.secret_class; + let volume_name = format!("{secret_class}-s3-credentials"); + + volumes.push( + credentials + .to_volume(&volume_name) + .context(AddS3CredentialVolumesSnafu)?, + ); + mounts.push( + VolumeMountBuilder::new(volume_name, format!("{SECRET_BASE_PATH}/{secret_class}")) + .build(), + ); + } + + // Add needed TLS volumes + let (tls_volumes, tls_mounts) = self + .tls + .volumes_and_mounts() + .context(AddS3TlsClientDetailsVolumesSnafu)?; + volumes.extend(tls_volumes); + mounts.extend(tls_mounts); + + Ok((volumes, mounts)) + } + + /// Returns the path of the files containing bind user and password. + /// This will be None if there are no credentials for this LDAP connection. + pub fn credentials_mount_paths(&self) -> Option<(String, String)> { + self.credentials.as_ref().map(|bind_credentials| { + let secret_class = &bind_credentials.secret_class; + ( + format!("{SECRET_BASE_PATH}/{secret_class}/accessKey"), + format!("{SECRET_BASE_PATH}/{secret_class}/secretKey"), + ) + }) + } +} + +#[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +// TODO: This probably should be serde(untagged), but this would be a breaking change +pub enum S3BucketInlineOrReference { + Inline(S3BucketSpec), + Reference(String), +} + +/// Use this struct in your operator. +pub struct ResolvedS3Bucket { + pub bucket_name: String, + pub connection: S3ConnectionSpec, +} + +impl S3BucketInlineOrReference { + pub async fn resolve( + self, + client: &Client, + namespace: &str, + ) -> Result { + match self { + Self::Inline(inline) => Ok(ResolvedS3Bucket { + bucket_name: inline.bucket_name, + connection: inline.connection.resolve(client, namespace).await?, + }), + Self::Reference(reference) => { + let bucket = client + .get::(&reference, namespace) + .await + .context(RetrieveS3ConnectionSnafu)? + .spec; + Ok(ResolvedS3Bucket { + bucket_name: bucket.bucket_name, + connection: bucket.connection.resolve(client, namespace).await?, + }) + } + } + } +} + +#[cfg(test)] +mod test { + use std::collections::BTreeMap; + + use crate::commons::{ + secret_class::SecretClassVolume, + tls_verification::{CaCert, Tls, TlsClientDetails, TlsServerVerification, TlsVerification}, + }; + + use super::*; + + // We cant test the correct resolve, as we can't mock the k8s API. + + #[test] + fn test_http() { + let s3 = ResolvedS3Connection { + host: "minio".parse().unwrap(), + port: None, + access_style: Default::default(), + credentials: None, + tls: TlsClientDetails { tls: None }, + }; + let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); + + assert_eq!(s3.endpoint().unwrap(), Url::parse("http://minio").unwrap()); + assert_eq!(volumes, vec![]); + assert_eq!(mounts, vec![]); + } + + #[test] + fn test_https() { + let s3 = ResolvedS3Connection { + host: "s3-eu-central-2.ionoscloud.com".parse().unwrap(), + port: None, + access_style: Default::default(), + credentials: Some(SecretClassVolume { + secret_class: "ionos-s3-credentials".to_string(), + scope: None, + }), + tls: TlsClientDetails { + tls: Some(Tls { + verification: TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::WebPki {}, + }), + }), + }, + }; + let (mut volumes, mut mounts) = s3.volumes_and_mounts().unwrap(); + + assert_eq!( + s3.endpoint().unwrap(), + Url::parse("https://s3-eu-central-2.ionoscloud.com").unwrap() + ); + assert_eq!(volumes.len(), 1); + let volume = volumes.remove(0); + assert_eq!(mounts.len(), 1); + let mount = mounts.remove(0); + + assert_eq!(&volume.name, "ionos-s3-credentials-s3-credentials"); + assert_eq!( + &volume + .ephemeral + .unwrap() + .volume_claim_template + .unwrap() + .metadata + .unwrap() + .annotations + .unwrap(), + &BTreeMap::from([( + "secrets.stackable.tech/class".to_string(), + "ionos-s3-credentials".to_string() + )]), + ); + + assert_eq!(mount.name, volume.name); + assert_eq!(mount.mount_path, "/stackable/secrets/ionos-s3-credentials"); + assert_eq!( + s3.credentials_mount_paths(), + Some(( + "/stackable/secrets/ionos-s3-credentials/accessKey".to_string(), + "/stackable/secrets/ionos-s3-credentials/secretKey".to_string() + )) + ); + } + + #[test] + fn test_https_without_verification() { + let s3 = ResolvedS3Connection { + host: "minio".parse().unwrap(), + port: Some(1234), + access_style: Default::default(), + credentials: None, + tls: TlsClientDetails { + tls: Some(Tls { + verification: TlsVerification::None {}, + }), + }, + }; + let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); + + assert_eq!( + s3.endpoint().unwrap(), + Url::parse("https://minio:1234").unwrap() + ); + assert_eq!(volumes, vec![]); + assert_eq!(mounts, vec![]); + } +} diff --git a/crates/stackable-operator/src/commons/s3/mod.rs b/crates/stackable-operator/src/commons/s3/mod.rs new file mode 100644 index 000000000..dc6a52183 --- /dev/null +++ b/crates/stackable-operator/src/commons/s3/mod.rs @@ -0,0 +1,31 @@ +mod crd; +mod helpers; + +pub use crd::*; +pub use helpers::*; + +use snafu::Snafu; +use url::Url; + +use super::{secret_class::SecretClassVolumeError, tls_verification::TlsClientDetailsError}; + +#[derive(Debug, Snafu)] +pub enum S3Error { + #[snafu(display("failed to retrieve S3 connection"))] + RetrieveS3Connection { source: crate::client::Error }, + + #[snafu(display("failed to retrieve S3 bucket"))] + RetrieveS3Bucket { source: crate::client::Error }, + + #[snafu(display("failed to parse S3 endpoint"))] + ParseS3Endpoint { source: url::ParseError }, + + #[snafu(display("failed to set S3 endpoint scheme '{scheme}' for endpoint '{endpoint}'"))] + SetS3EndpointScheme { endpoint: Url, scheme: String }, + + #[snafu(display("failed to add S3 credential volumes and volume mounts"))] + AddS3CredentialVolumes { source: SecretClassVolumeError }, + + #[snafu(display("failed to add S3 TLS client details volumes and volume mounts"))] + AddS3TlsClientDetailsVolumes { source: TlsClientDetailsError }, +} From 68dc3170e6f261945330da2cbd641b761ce8cc3b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 11 Sep 2024 14:56:23 +0200 Subject: [PATCH 04/26] fix: Use String for Host JsonSchema --- .../stackable-operator/src/commons/networking.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 96cbf70fa..416c4ad5d 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -50,15 +50,23 @@ impl Deref for Hostname { } /// A validated host (either a [`Hostname`] or IP address) type. -#[derive( - Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, JsonSchema, -)] +#[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] #[serde(try_from = "String", into = "String")] pub enum Host { IpAddress(IpAddr), Hostname(Hostname), } +impl JsonSchema for Host { + fn schema_name() -> String { + "Host".to_owned() + } + + fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema { + String::json_schema(gen) + } +} + impl FromStr for Host { type Err = validation::Error; From 4b761eb50a52c3a9606852c9142309b153c76b22 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 11 Sep 2024 15:44:48 +0200 Subject: [PATCH 05/26] changelog --- crates/stackable-operator/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 0cd696316..29cdd1260 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -13,11 +13,11 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: `validation` module now uses typed errors ([#851]). -- BREAKING: TLS verification struct now reside top-level in the `commons` module, instead of being placed below `commons::authentication::tls` ([#XXX]). +- BREAKING: TLS verification struct now reside in the `commons::tls_verification` module, instead of being placed below `commons::authentication::tls` ([#XXX]). ### Fixed -- BREAKING: The fields on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory (as they should be) ([#XXX]). +- BREAKING: The fields `bucketName`, `connection` and `host` on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory. Previously operators probably errored out in case they where missing, so this should not affect users, but make the errors much clearer ([#XXX]). - Fix the CRD description of `ClientAuthenticationDetails` to not contain internal Rust doc, but a public CRD description ([#846]). - `StackableAffinity` fields are no longer erroneously marked as required ([#855]). From d07bd55bf8724adff0bcd1e733f5afe8c3bbd666 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 11 Sep 2024 15:48:07 +0200 Subject: [PATCH 06/26] changelog --- crates/stackable-operator/CHANGELOG.md | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 29cdd1260..43754a6bc 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -6,18 +6,31 @@ All notable changes to this project will be documented in this file. ### Added +- BREAKING: Add `Host` type and use it within LDAP and OIDC AuthenticationClass as well as S3Connection ([#863]). + +### Changed + +- BREAKING: TLS verification struct now reside in the `commons::tls_verification` module, instead of being placed below `commons::authentication::tls` ([#863]). + +### Fixed + +- BREAKING: The fields `bucketName`, `connection` and `host` on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory. Previously operators probably errored out in case they where missing, so this should not affect users, but make the errors much clearer ([#863]). + +[#863]: https://github.com/stackabletech/operator-rs/pull/863 + +## TODO: Create a release before #863, as it's majorly breaking + +### Added + - Add `Hostname` and `KerberosRealmName` types extracted from secret-operator ([#851]). -- BREAKING: Add `Host` type and use it within LDAP and OIDC AuthenticationClass as well as S3Connection ([#XXX]). - Add support for listener volume scopes to `SecretOperatorVolumeSourceBuilder` ([#858]). ### Changed - BREAKING: `validation` module now uses typed errors ([#851]). -- BREAKING: TLS verification struct now reside in the `commons::tls_verification` module, instead of being placed below `commons::authentication::tls` ([#XXX]). ### Fixed -- BREAKING: The fields `bucketName`, `connection` and `host` on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory. Previously operators probably errored out in case they where missing, so this should not affect users, but make the errors much clearer ([#XXX]). - Fix the CRD description of `ClientAuthenticationDetails` to not contain internal Rust doc, but a public CRD description ([#846]). - `StackableAffinity` fields are no longer erroneously marked as required ([#855]). From d97a6017ecea3ef21aedfa46e5a0e08d6010a7b8 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 09:46:17 +0200 Subject: [PATCH 07/26] Add unique identifier to avoid clashing volumes and mounts --- .../src/commons/s3/helpers.rs | 45 ++++++++++++------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs index a58b5c297..dd862cf37 100644 --- a/crates/stackable-operator/src/commons/s3/helpers.rs +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -79,10 +79,11 @@ impl ResolvedS3Connection { /// * Needed TLS volumes pub fn add_volumes_and_mounts( &self, + unique_identifier: &str, pod_builder: &mut PodBuilder, container_builders: Vec<&mut ContainerBuilder>, ) -> Result<(), S3Error> { - let (volumes, mounts) = self.volumes_and_mounts()?; + let (volumes, mounts) = self.volumes_and_mounts(unique_identifier)?; pod_builder.add_volumes(volumes); for cb in container_builders { cb.add_volume_mounts(mounts.clone()); @@ -93,13 +94,16 @@ impl ResolvedS3Connection { /// It is recommended to use [`Self::add_volumes_and_mounts`], this function returns you the /// volumes and mounts in case you need to add them by yourself. - pub fn volumes_and_mounts(&self) -> Result<(Vec, Vec), S3Error> { + pub fn volumes_and_mounts( + &self, + unique_identifier: &str, + ) -> Result<(Vec, Vec), S3Error> { let mut volumes = Vec::new(); let mut mounts = Vec::new(); if let Some(credentials) = &self.credentials { let secret_class = &credentials.secret_class; - let volume_name = format!("{secret_class}-s3-credentials"); + let volume_name = format!("{secret_class}-s3-credentials-{unique_identifier}"); volumes.push( credentials @@ -107,8 +111,11 @@ impl ResolvedS3Connection { .context(AddS3CredentialVolumesSnafu)?, ); mounts.push( - VolumeMountBuilder::new(volume_name, format!("{SECRET_BASE_PATH}/{secret_class}")) - .build(), + VolumeMountBuilder::new( + volume_name, + format!("{SECRET_BASE_PATH}/{secret_class}-{unique_identifier}"), + ) + .build(), ); } @@ -125,12 +132,12 @@ impl ResolvedS3Connection { /// Returns the path of the files containing bind user and password. /// This will be None if there are no credentials for this LDAP connection. - pub fn credentials_mount_paths(&self) -> Option<(String, String)> { + pub fn credentials_mount_paths(&self, unique_identifier: &str) -> Option<(String, String)> { self.credentials.as_ref().map(|bind_credentials| { let secret_class = &bind_credentials.secret_class; ( - format!("{SECRET_BASE_PATH}/{secret_class}/accessKey"), - format!("{SECRET_BASE_PATH}/{secret_class}/secretKey"), + format!("{SECRET_BASE_PATH}/{secret_class}-{unique_identifier}/accessKey"), + format!("{SECRET_BASE_PATH}/{secret_class}-{unique_identifier}/secretKey"), ) }) } @@ -198,7 +205,7 @@ mod test { credentials: None, tls: TlsClientDetails { tls: None }, }; - let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); + let (volumes, mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); assert_eq!(s3.endpoint().unwrap(), Url::parse("http://minio").unwrap()); assert_eq!(volumes, vec![]); @@ -223,7 +230,7 @@ mod test { }), }, }; - let (mut volumes, mut mounts) = s3.volumes_and_mounts().unwrap(); + let (mut volumes, mut mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); assert_eq!( s3.endpoint().unwrap(), @@ -234,7 +241,10 @@ mod test { assert_eq!(mounts.len(), 1); let mount = mounts.remove(0); - assert_eq!(&volume.name, "ionos-s3-credentials-s3-credentials"); + assert_eq!( + &volume.name, + "ionos-s3-credentials-s3-credentials-lakehouse" + ); assert_eq!( &volume .ephemeral @@ -252,12 +262,15 @@ mod test { ); assert_eq!(mount.name, volume.name); - assert_eq!(mount.mount_path, "/stackable/secrets/ionos-s3-credentials"); assert_eq!( - s3.credentials_mount_paths(), + mount.mount_path, + "/stackable/secrets/ionos-s3-credentials-lakehouse" + ); + assert_eq!( + s3.credentials_mount_paths("lakehouse"), Some(( - "/stackable/secrets/ionos-s3-credentials/accessKey".to_string(), - "/stackable/secrets/ionos-s3-credentials/secretKey".to_string() + "/stackable/secrets/ionos-s3-credentials-lakehouse/accessKey".to_string(), + "/stackable/secrets/ionos-s3-credentials-lakehouse/secretKey".to_string() )) ); } @@ -275,7 +288,7 @@ mod test { }), }, }; - let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); + let (volumes, mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); assert_eq!( s3.endpoint().unwrap(), From 1f271dd217d95e98795124c1f8ea5c77c5a70952 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 09:53:31 +0200 Subject: [PATCH 08/26] move identifier to front --- .../stackable-operator/src/commons/s3/helpers.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs index dd862cf37..fee9d571f 100644 --- a/crates/stackable-operator/src/commons/s3/helpers.rs +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -103,7 +103,7 @@ impl ResolvedS3Connection { if let Some(credentials) = &self.credentials { let secret_class = &credentials.secret_class; - let volume_name = format!("{secret_class}-s3-credentials-{unique_identifier}"); + let volume_name = format!("{unique_identifier}-{secret_class}-s3-credentials"); volumes.push( credentials @@ -113,7 +113,7 @@ impl ResolvedS3Connection { mounts.push( VolumeMountBuilder::new( volume_name, - format!("{SECRET_BASE_PATH}/{secret_class}-{unique_identifier}"), + format!("{SECRET_BASE_PATH}/{unique_identifier}-{secret_class}"), ) .build(), ); @@ -136,8 +136,8 @@ impl ResolvedS3Connection { self.credentials.as_ref().map(|bind_credentials| { let secret_class = &bind_credentials.secret_class; ( - format!("{SECRET_BASE_PATH}/{secret_class}-{unique_identifier}/accessKey"), - format!("{SECRET_BASE_PATH}/{secret_class}-{unique_identifier}/secretKey"), + format!("{SECRET_BASE_PATH}/{unique_identifier}-{secret_class}/accessKey"), + format!("{SECRET_BASE_PATH}/{unique_identifier}-{secret_class}/secretKey"), ) }) } @@ -243,7 +243,7 @@ mod test { assert_eq!( &volume.name, - "ionos-s3-credentials-s3-credentials-lakehouse" + "lakehouse-ionos-s3-credentials-s3-credentials" ); assert_eq!( &volume @@ -264,13 +264,13 @@ mod test { assert_eq!(mount.name, volume.name); assert_eq!( mount.mount_path, - "/stackable/secrets/ionos-s3-credentials-lakehouse" + "/stackable/secrets/lakehouse-ionos-s3-credentials" ); assert_eq!( s3.credentials_mount_paths("lakehouse"), Some(( - "/stackable/secrets/ionos-s3-credentials-lakehouse/accessKey".to_string(), - "/stackable/secrets/ionos-s3-credentials-lakehouse/secretKey".to_string() + "/stackable/secrets/lakehouse-ionos-s3-credentials/accessKey".to_string(), + "/stackable/secrets/lakehouse-ionos-s3-credentials/secretKey".to_string() )) ); } From decfb0566a51df4d37bb092c3a2c53e120162d5e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 10:15:28 +0200 Subject: [PATCH 09/26] clippy --- crates/stackable-operator/src/commons/networking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 416c4ad5d..32b3707bd 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -16,7 +16,7 @@ impl FromStr for Hostname { type Err = validation::Errors; fn from_str(value: &str) -> Result { - validation::is_rfc_1123_subdomain(&value)?; + validation::is_rfc_1123_subdomain(value)?; Ok(Hostname(value.to_owned())) } } From 5969711215147736bac6678c7c0f2f6fa2741195 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 12:08:20 +0200 Subject: [PATCH 10/26] Apply suggestions from code review Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- .../stackable-operator/src/commons/authentication/oidc.rs | 6 +++--- crates/stackable-operator/src/commons/s3/helpers.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 99f1168fb..d25d96018 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -116,9 +116,9 @@ impl AuthenticationProvider { /// path at [`DEFAULT_OIDC_WELLKNOWN_PATH`]. pub fn endpoint_url(&self) -> Result { let mut url = Url::parse(&format!( - "http://{}:{}", - self.hostname.as_url_host(), - self.port() + "http://{host}:{port}", + host = self.hostname.as_url_host(), + port = self.port() )) .context(ParseOidcEndpointUrlSnafu)?; diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs index fee9d571f..9d4f745be 100644 --- a/crates/stackable-operator/src/commons/s3/helpers.rs +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -48,9 +48,9 @@ impl ResolvedS3Connection { /// Build the endpoint URL from this connection pub fn endpoint(&self) -> Result { let mut url = Url::parse(&format!( - "http://{}:{}", - self.host.as_url_host(), - self.port() + "http://{host}:{port}", + host = self.host.as_url_host(), + port = self.port() )) .context(ParseS3EndpointSnafu)?; From 687141c4d7ef19c451120c9a2eed019c9140db5f Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 12:16:23 +0200 Subject: [PATCH 11/26] Remove RetrieveS3Bucket error --- crates/stackable-operator/src/commons/s3/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/stackable-operator/src/commons/s3/mod.rs b/crates/stackable-operator/src/commons/s3/mod.rs index dc6a52183..dfe9779ea 100644 --- a/crates/stackable-operator/src/commons/s3/mod.rs +++ b/crates/stackable-operator/src/commons/s3/mod.rs @@ -14,9 +14,6 @@ pub enum S3Error { #[snafu(display("failed to retrieve S3 connection"))] RetrieveS3Connection { source: crate::client::Error }, - #[snafu(display("failed to retrieve S3 bucket"))] - RetrieveS3Bucket { source: crate::client::Error }, - #[snafu(display("failed to parse S3 endpoint"))] ParseS3Endpoint { source: url::ParseError }, From 72417ff87cd5e74340d7d85caa6034c6e2804584 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 12:22:09 +0200 Subject: [PATCH 12/26] Add some error details --- .../stackable-operator/src/commons/s3/helpers.rs | 14 +++++++++----- crates/stackable-operator/src/commons/s3/mod.rs | 16 +++++++++++----- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs index 9d4f745be..d5a7826e3 100644 --- a/crates/stackable-operator/src/commons/s3/helpers.rs +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -38,7 +38,9 @@ impl S3ConnectionInlineOrReference { Self::Reference(reference) => Ok(client .get::(&reference, namespace) .await - .context(RetrieveS3ConnectionSnafu)? + .context(RetrieveS3ConnectionSnafu { + s3_connection: reference, + })? .spec), } } @@ -47,12 +49,12 @@ impl S3ConnectionInlineOrReference { impl ResolvedS3Connection { /// Build the endpoint URL from this connection pub fn endpoint(&self) -> Result { - let mut url = Url::parse(&format!( + let endpoint = format!( "http://{host}:{port}", host = self.host.as_url_host(), port = self.port() - )) - .context(ParseS3EndpointSnafu)?; + ); + let mut url = Url::parse(&endpoint).context(ParseS3EndpointSnafu { endpoint })?; if self.tls.uses_tls() { url.set_scheme("https").map_err(|_| { @@ -172,7 +174,9 @@ impl S3BucketInlineOrReference { let bucket = client .get::(&reference, namespace) .await - .context(RetrieveS3ConnectionSnafu)? + .context(RetrieveS3ConnectionSnafu { + s3_connection: reference, + })? .spec; Ok(ResolvedS3Bucket { bucket_name: bucket.bucket_name, diff --git a/crates/stackable-operator/src/commons/s3/mod.rs b/crates/stackable-operator/src/commons/s3/mod.rs index dfe9779ea..c7f36c6eb 100644 --- a/crates/stackable-operator/src/commons/s3/mod.rs +++ b/crates/stackable-operator/src/commons/s3/mod.rs @@ -11,11 +11,17 @@ use super::{secret_class::SecretClassVolumeError, tls_verification::TlsClientDet #[derive(Debug, Snafu)] pub enum S3Error { - #[snafu(display("failed to retrieve S3 connection"))] - RetrieveS3Connection { source: crate::client::Error }, - - #[snafu(display("failed to parse S3 endpoint"))] - ParseS3Endpoint { source: url::ParseError }, + #[snafu(display("failed to retrieve S3 connection '{s3_connection}'"))] + RetrieveS3Connection { + source: crate::client::Error, + s3_connection: String, + }, + + #[snafu(display("failed to parse S3 endpoint '{endpoint}'"))] + ParseS3Endpoint { + source: url::ParseError, + endpoint: String, + }, #[snafu(display("failed to set S3 endpoint scheme '{scheme}' for endpoint '{endpoint}'"))] SetS3EndpointScheme { endpoint: Url, scheme: String }, From ad89d3fdbad05beb4d4dcf3cd012dda6fd1fe5ee Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 12:30:03 +0200 Subject: [PATCH 13/26] typo --- crates/stackable-operator/src/commons/networking.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 32b3707bd..54ae0946b 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -168,7 +168,7 @@ mod tests { #[case("1.2.3.4")] fn test_host_and_hostname_parsing_success(#[case] hostname: String) { let parsed_hostname: Hostname = hostname.parse().expect("hostname can not be parsed"); - // Every host is also a valid hostname + // Every hostname is also a valid host let parsed_host: Host = hostname.parse().expect("host can not be parsed"); // Also test the round-trip @@ -190,7 +190,6 @@ mod tests { #[case("1.2.3.4", "1.2.3.4")] #[case("fe80::1", "[fe80::1]")] fn test_host_parsing_success(#[case] host: &str, #[case] expected_url_host: &str) { - // Every host is also a valid hostname let parsed_host: Host = host.parse().expect("host can not be parsed"); // Also test the round-trip From 7dc239408e3a4c3d9b7beaf9a414d5e0c2931f50 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 12 Sep 2024 12:32:31 +0200 Subject: [PATCH 14/26] Add docs for unique_identifier --- crates/stackable-operator/src/commons/s3/helpers.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs index d5a7826e3..c6392dbe3 100644 --- a/crates/stackable-operator/src/commons/s3/helpers.rs +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -79,6 +79,9 @@ impl ResolvedS3Connection { /// /// * Credentials needed to connect to S3 /// * Needed TLS volumes + /// + /// `unique_identifier` needs to be a unique identifier (e.g. in case of trino-operator the name of the catalog), + /// so that multiple mounts of the same SecretClass do not produce clashing volumes and volumeMounts. pub fn add_volumes_and_mounts( &self, unique_identifier: &str, @@ -134,6 +137,9 @@ impl ResolvedS3Connection { /// Returns the path of the files containing bind user and password. /// This will be None if there are no credentials for this LDAP connection. + /// + /// `unique_identifier` needs to be a unique identifier (e.g. in case of trino-operator the name of the catalog), + /// so that multiple mounts of the same SecretClass do not produce clashing volumes and volumeMounts. pub fn credentials_mount_paths(&self, unique_identifier: &str) -> Option<(String, String)> { self.credentials.as_ref().map(|bind_credentials| { let secret_class = &bind_credentials.secret_class; @@ -199,7 +205,6 @@ mod test { use super::*; // We cant test the correct resolve, as we can't mock the k8s API. - #[test] fn test_http() { let s3 = ResolvedS3Connection { From 34d6db89e617771637cb3a65ac62867a3a94cf67 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 16 Sep 2024 11:10:41 +0200 Subject: [PATCH 15/26] Apply suggestions from code review Co-authored-by: Techassi --- crates/stackable-operator/src/commons/authentication/ldap.rs | 2 +- crates/stackable-operator/src/commons/authentication/oidc.rs | 2 +- crates/stackable-operator/src/commons/networking.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs index 1349c9336..8b3545060 100644 --- a/crates/stackable-operator/src/commons/authentication/ldap.rs +++ b/crates/stackable-operator/src/commons/authentication/ldap.rs @@ -35,7 +35,7 @@ pub enum Error { )] #[serde(rename_all = "camelCase")] pub struct AuthenticationProvider { - /// Host of the LDAP server, for example: `my.ldap.server`. + /// Host of the LDAP server, for example: `my.ldap.server` or `127.0.0.1`. pub hostname: Host, /// Port of the LDAP server. If TLS is used defaults to 636 otherwise to 389. diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index d25d96018..4e2684ec8 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -42,7 +42,7 @@ pub enum Error { )] #[serde(rename_all = "camelCase")] pub struct AuthenticationProvider { - /// Host of the identity provider, e.g. `my.keycloak.corp`. + /// Host of the identity provider, e.g. `my.keycloak.corp` or `127.0.0.1`. hostname: Host, /// Port of the identity provider. If TLS is used defaults to 443, diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 54ae0946b..c41f367cd 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -93,7 +93,7 @@ impl TryFrom for Host { impl From for String { fn from(value: Host) -> Self { - format!("{value}") + value.to_string() } } From 92f09b8a454891f2215de0a5e37ff151766b5b76 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 16 Sep 2024 11:11:46 +0200 Subject: [PATCH 16/26] Rename NotAHost -> NotAHost --- crates/stackable-operator/src/commons/networking.rs | 2 +- crates/stackable-operator/src/validation.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index c41f367cd..6b4b27e12 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -79,7 +79,7 @@ impl FromStr for Host { return Ok(Host::Hostname(hostname)); }; - Err(validation::Error::NotAHost {}) + Err(validation::Error::InvalidHost {}) } } diff --git a/crates/stackable-operator/src/validation.rs b/crates/stackable-operator/src/validation.rs index fd26205ae..e6c3bcd08 100644 --- a/crates/stackable-operator/src/validation.rs +++ b/crates/stackable-operator/src/validation.rs @@ -94,7 +94,7 @@ pub enum Error { #[snafu(display( "input is not a valid host, which needs to be either a hostname or IP address" ))] - NotAHost {}, + InvalidHost {}, } #[derive(Debug)] From 4fb9d50d971366778efe4274365619b1692bc425 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 16 Sep 2024 11:17:02 +0200 Subject: [PATCH 17/26] Add invalid host to error message --- crates/stackable-operator/src/commons/networking.rs | 4 +++- crates/stackable-operator/src/validation.rs | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 6b4b27e12..dbdc723d2 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -79,7 +79,9 @@ impl FromStr for Host { return Ok(Host::Hostname(hostname)); }; - Err(validation::Error::InvalidHost {}) + Err(validation::Error::InvalidHost { + host: value.to_owned(), + }) } } diff --git a/crates/stackable-operator/src/validation.rs b/crates/stackable-operator/src/validation.rs index e6c3bcd08..3d337e9e1 100644 --- a/crates/stackable-operator/src/validation.rs +++ b/crates/stackable-operator/src/validation.rs @@ -92,9 +92,9 @@ pub enum Error { TooLong { length: usize, max_length: usize }, #[snafu(display( - "input is not a valid host, which needs to be either a hostname or IP address" + "The input '{host}' is not a valid host, which needs to be either a hostname or IP address" ))] - InvalidHost {}, + InvalidHost { host: String }, } #[derive(Debug)] From acb84d80440db650555233e7b227f2ee48f0e7b6 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 16 Sep 2024 11:27:09 +0200 Subject: [PATCH 18/26] Move to HostParseError --- .../src/commons/networking.rs | 18 ++++++++++++++---- crates/stackable-operator/src/validation.rs | 5 ----- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index dbdc723d2..75b853765 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -2,6 +2,7 @@ use std::{fmt::Display, net::IpAddr, ops::Deref, str::FromStr}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use snafu::Snafu; use crate::validation; @@ -49,6 +50,14 @@ impl Deref for Hostname { } } +#[derive(Debug, Snafu)] +pub enum HostParseError { + #[snafu(display( + "the given host '{host}' is not a valid host, which needs to be either a hostname or IP address" + ))] + InvalidHost { host: String }, +} + /// A validated host (either a [`Hostname`] or IP address) type. #[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] #[serde(try_from = "String", into = "String")] @@ -68,7 +77,7 @@ impl JsonSchema for Host { } impl FromStr for Host { - type Err = validation::Error; + type Err = HostParseError; fn from_str(value: &str) -> Result { if let Ok(ip) = value.parse::() { @@ -79,14 +88,15 @@ impl FromStr for Host { return Ok(Host::Hostname(hostname)); }; - Err(validation::Error::InvalidHost { + InvalidHostSnafu { host: value.to_owned(), - }) + } + .fail() } } impl TryFrom for Host { - type Error = validation::Error; + type Error = HostParseError; fn try_from(value: String) -> Result { value.parse() diff --git a/crates/stackable-operator/src/validation.rs b/crates/stackable-operator/src/validation.rs index 3d337e9e1..8f22e8202 100644 --- a/crates/stackable-operator/src/validation.rs +++ b/crates/stackable-operator/src/validation.rs @@ -90,11 +90,6 @@ pub enum Error { #[snafu(display("input is {length} bytes long but must be no more than {max_length}"))] TooLong { length: usize, max_length: usize }, - - #[snafu(display( - "The input '{host}' is not a valid host, which needs to be either a hostname or IP address" - ))] - InvalidHost { host: String }, } #[derive(Debug)] From 90f927171bcc89cebe77918418555a1ed73ddfee Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 16 Sep 2024 15:45:16 +0200 Subject: [PATCH 19/26] Rename Host -> HostName --- crates/stackable-operator/CHANGELOG.md | 3 +- .../src/commons/authentication/ldap.rs | 4 +- .../src/commons/authentication/oidc.rs | 6 +- .../src/commons/networking.rs | 95 ++++++++++--------- .../stackable-operator/src/commons/s3/crd.rs | 4 +- 5 files changed, 57 insertions(+), 55 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index b2acbdd40..d3db91e98 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -6,11 +6,12 @@ All notable changes to this project will be documented in this file. ### Added -- BREAKING: Add `Host` type and use it within LDAP and OIDC AuthenticationClass as well as S3Connection ([#863]). +- BREAKING: Add `HostName` type and use it within LDAP and OIDC AuthenticationClass as well as S3Connection ([#863]). ### Changed - BREAKING: TLS verification struct now reside in the `commons::tls_verification` module, instead of being placed below `commons::authentication::tls` ([#863]). +- BREAKING: Rename the `Hostname` type to `DomainName` to be consistent with RFC 1123 ([#863]). ### Fixed diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs index 8b3545060..2b43bbd71 100644 --- a/crates/stackable-operator/src/commons/authentication/ldap.rs +++ b/crates/stackable-operator/src/commons/authentication/ldap.rs @@ -8,7 +8,7 @@ use crate::{ builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder}, commons::{ authentication::SECRET_BASE_PATH, - networking::Host, + networking::HostName, secret_class::{SecretClassVolume, SecretClassVolumeError}, tls_verification::{TlsClientDetails, TlsClientDetailsError}, }, @@ -36,7 +36,7 @@ pub enum Error { #[serde(rename_all = "camelCase")] pub struct AuthenticationProvider { /// Host of the LDAP server, for example: `my.ldap.server` or `127.0.0.1`. - pub hostname: Host, + pub hostname: HostName, /// Port of the LDAP server. If TLS is used defaults to 636 otherwise to 389. port: Option, diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 4e2684ec8..d8316758f 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -12,7 +12,7 @@ use url::{ParseError, Url}; #[cfg(doc)] use crate::commons::authentication::AuthenticationClass; use crate::commons::{ - authentication::SECRET_BASE_PATH, networking::Host, tls_verification::TlsClientDetails, + authentication::SECRET_BASE_PATH, networking::HostName, tls_verification::TlsClientDetails, }; pub type Result = std::result::Result; @@ -43,7 +43,7 @@ pub enum Error { #[serde(rename_all = "camelCase")] pub struct AuthenticationProvider { /// Host of the identity provider, e.g. `my.keycloak.corp` or `127.0.0.1`. - hostname: Host, + hostname: HostName, /// Port of the identity provider. If TLS is used defaults to 443, /// otherwise to 80. @@ -92,7 +92,7 @@ fn default_root_path() -> String { impl AuthenticationProvider { pub fn new( - hostname: Host, + hostname: HostName, port: Option, root_path: String, tls: TlsClientDetails, diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 75b853765..1fb97c09d 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -6,23 +6,23 @@ use snafu::Snafu; use crate::validation; -/// A validated hostname type conforming to RFC 1123, e.g. not an IPv6 address. +/// A validated domain name type conforming to RFC 1123, e.g. an IPv4, but not an IPv6 address. #[derive( Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, JsonSchema, )] #[serde(try_from = "String", into = "String")] -pub struct Hostname(#[validate(regex(path = "validation::RFC_1123_SUBDOMAIN_REGEX"))] String); +pub struct DomainName(#[validate(regex(path = "validation::RFC_1123_SUBDOMAIN_REGEX"))] String); -impl FromStr for Hostname { +impl FromStr for DomainName { type Err = validation::Errors; fn from_str(value: &str) -> Result { validation::is_rfc_1123_subdomain(value)?; - Ok(Hostname(value.to_owned())) + Ok(DomainName(value.to_owned())) } } -impl TryFrom for Hostname { +impl TryFrom for DomainName { type Error = validation::Errors; fn try_from(value: String) -> Result { @@ -30,19 +30,19 @@ impl TryFrom for Hostname { } } -impl From for String { - fn from(value: Hostname) -> Self { +impl From for String { + fn from(value: DomainName) -> Self { value.0 } } -impl Display for Hostname { +impl Display for DomainName { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(&self.0) } } -impl Deref for Hostname { +impl Deref for DomainName { type Target = str; fn deref(&self) -> &Self::Target { @@ -51,24 +51,24 @@ impl Deref for Hostname { } #[derive(Debug, Snafu)] -pub enum HostParseError { +pub enum HostNameParseError { #[snafu(display( - "the given host '{host}' is not a valid host, which needs to be either a hostname or IP address" + "the given hostname '{hostname}' is not a valid hostname, which needs to be either a domain name or IP address" ))] - InvalidHost { host: String }, + InvalidHostname { hostname: String }, } -/// A validated host (either a [`Hostname`] or IP address) type. +/// A validated hostname (either a [`DomainName`] or IP address) type. #[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] #[serde(try_from = "String", into = "String")] -pub enum Host { +pub enum HostName { IpAddress(IpAddr), - Hostname(Hostname), + DomainName(DomainName), } -impl JsonSchema for Host { +impl JsonSchema for HostName { fn schema_name() -> String { - "Host".to_owned() + "HostName".to_owned() } fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema { @@ -76,57 +76,57 @@ impl JsonSchema for Host { } } -impl FromStr for Host { - type Err = HostParseError; +impl FromStr for HostName { + type Err = HostNameParseError; fn from_str(value: &str) -> Result { if let Ok(ip) = value.parse::() { - return Ok(Host::IpAddress(ip)); + return Ok(HostName::IpAddress(ip)); } - if let Ok(hostname) = value.parse() { - return Ok(Host::Hostname(hostname)); + if let Ok(domain_name) = value.parse() { + return Ok(HostName::DomainName(domain_name)); }; - InvalidHostSnafu { - host: value.to_owned(), + InvalidHostnameSnafu { + hostname: value.to_owned(), } .fail() } } -impl TryFrom for Host { - type Error = HostParseError; +impl TryFrom for HostName { + type Error = HostNameParseError; fn try_from(value: String) -> Result { value.parse() } } -impl From for String { - fn from(value: Host) -> Self { +impl From for String { + fn from(value: HostName) -> Self { value.to_string() } } -impl Display for Host { +impl Display for HostName { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Host::IpAddress(ip) => write!(f, "{ip}"), - Host::Hostname(hostname) => write!(f, "{hostname}"), + HostName::IpAddress(ip) => write!(f, "{ip}"), + HostName::DomainName(domain_name) => write!(f, "{domain_name}"), } } } -impl Host { +impl HostName { /// Formats the host in such a way that it can be used in URLs. pub fn as_url_host(&self) -> String { match self { - Host::IpAddress(ip) => match ip { + HostName::IpAddress(ip) => match ip { IpAddr::V4(ip) => ip.to_string(), IpAddr::V6(ip) => format!("[{ip}]"), }, - Host::Hostname(hostname) => hostname.to_string(), + HostName::DomainName(domain_name) => domain_name.to_string(), } } } @@ -176,24 +176,25 @@ mod tests { #[rstest] #[case("foo")] #[case("foo.bar")] - // Well this is also a valid hostname I guess + // This is also a valid domain name #[case("1.2.3.4")] - fn test_host_and_hostname_parsing_success(#[case] hostname: String) { - let parsed_hostname: Hostname = hostname.parse().expect("hostname can not be parsed"); - // Every hostname is also a valid host - let parsed_host: Host = hostname.parse().expect("host can not be parsed"); + fn test_domain_name_and_host_name_parsing_success(#[case] domain_name: String) { + let parsed_domain_name: DomainName = + domain_name.parse().expect("domain name can not be parsed"); + // Every domain name is also a valid host name + let parsed_host_name: HostName = domain_name.parse().expect("host name can not be parsed"); // Also test the round-trip - assert_eq!(parsed_hostname.to_string(), hostname); - assert_eq!(parsed_host.to_string(), hostname); + assert_eq!(parsed_domain_name.to_string(), domain_name); + assert_eq!(parsed_host_name.to_string(), domain_name); } #[rstest] #[case("")] #[case("foo.bar:1234")] #[case("fe80::1")] - fn test_hostname_parsing_invalid_input(#[case] hostname: &str) { - assert!(hostname.parse::().is_err()); + fn test_domain_name_parsing_invalid_input(#[case] domain_name: &str) { + assert!(domain_name.parse::().is_err()); } #[rstest] @@ -201,12 +202,12 @@ mod tests { #[case("foo.bar", "foo.bar")] #[case("1.2.3.4", "1.2.3.4")] #[case("fe80::1", "[fe80::1]")] - fn test_host_parsing_success(#[case] host: &str, #[case] expected_url_host: &str) { - let parsed_host: Host = host.parse().expect("host can not be parsed"); + fn test_host_name_parsing_success(#[case] host: &str, #[case] expected_url_host: &str) { + let parsed_host_name: HostName = host.parse().expect("host can not be parsed"); // Also test the round-trip - assert_eq!(parsed_host.to_string(), host); + assert_eq!(parsed_host_name.to_string(), host); - assert_eq!(parsed_host.as_url_host(), expected_url_host); + assert_eq!(parsed_host_name.as_url_host(), expected_url_host); } } diff --git a/crates/stackable-operator/src/commons/s3/crd.rs b/crates/stackable-operator/src/commons/s3/crd.rs index 804432085..6905a396b 100644 --- a/crates/stackable-operator/src/commons/s3/crd.rs +++ b/crates/stackable-operator/src/commons/s3/crd.rs @@ -3,7 +3,7 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use crate::commons::{ - networking::Host, secret_class::SecretClassVolume, tls_verification::TlsClientDetails, + networking::HostName, secret_class::SecretClassVolume, tls_verification::TlsClientDetails, }; use super::S3ConnectionInlineOrReference; @@ -50,7 +50,7 @@ pub struct S3BucketSpec { #[serde(rename_all = "camelCase")] pub struct S3ConnectionSpec { /// Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`. - pub host: Host, + pub host: HostName, /// Port the S3 server listens on. /// If not specified the product will determine the port to use. From 006e37a2b7d7689861f3f429b746912dd8a2a93f Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 18 Sep 2024 12:41:17 +0200 Subject: [PATCH 20/26] Update crates/stackable-operator/src/commons/networking.rs Co-authored-by: Techassi --- crates/stackable-operator/src/commons/networking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 1fb97c09d..598ac6305 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -53,7 +53,7 @@ impl Deref for DomainName { #[derive(Debug, Snafu)] pub enum HostNameParseError { #[snafu(display( - "the given hostname '{hostname}' is not a valid hostname, which needs to be either a domain name or IP address" + "the given hostname {hostname:?} is not a valid hostname, which needs to be either a domain name or IP address" ))] InvalidHostname { hostname: String }, } From 5ba45224cd16866e345e767a0ba075262424202d Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 18 Sep 2024 12:41:36 +0200 Subject: [PATCH 21/26] Update crates/stackable-operator/CHANGELOG.md Co-authored-by: Techassi --- crates/stackable-operator/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index d3db91e98..9236b4e8f 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to this project will be documented in this file. ### Changed -- BREAKING: TLS verification struct now reside in the `commons::tls_verification` module, instead of being placed below `commons::authentication::tls` ([#863]). +- BREAKING: The TLS verification struct now resides in the `commons::tls_verification` module, instead of being placed below `commons::authentication::tls` ([#863]). - BREAKING: Rename the `Hostname` type to `DomainName` to be consistent with RFC 1123 ([#863]). ### Fixed From 152ee94a876647c6b04b70fc6a3add42a6824579 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 18 Sep 2024 12:41:58 +0200 Subject: [PATCH 22/26] Update crates/stackable-operator/CHANGELOG.md Co-authored-by: Techassi --- crates/stackable-operator/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 9236b4e8f..12a7c864a 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -15,7 +15,7 @@ All notable changes to this project will be documented in this file. ### Fixed -- BREAKING: The fields `bucketName`, `connection` and `host` on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory. Previously operators probably errored out in case they where missing, so this should not affect users, but make the errors much clearer ([#863]). +- BREAKING: The fields `bucketName`, `connection` and `host` on `S3BucketSpec`, `InlinedS3BucketSpec` and `S3ConnectionSpec` are now mandatory. Previously operators errored out in case these fields where missing ([#863]). [#863]: https://github.com/stackabletech/operator-rs/pull/863 From 9dcd4458334afdea57fa9230ad1bb1bd08ead6cc Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 19 Sep 2024 10:06:18 +0200 Subject: [PATCH 23/26] changelog --- crates/stackable-operator/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 9a16727d5..83a197b50 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] -## [0.75.0] - 2024-09-19 +## [0.76.0] - 2024-09-19 ### Added @@ -21,7 +21,7 @@ All notable changes to this project will be documented in this file. [#863]: https://github.com/stackabletech/operator-rs/pull/863 -## TODO: Create a release before #863, as it's majorly breaking +## [0.75.0] - 2024-09-19 ### Added From d4e673be01d0287a8e0897374c0a886f2cd6aa48 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 19 Sep 2024 10:09:45 +0200 Subject: [PATCH 24/26] Bump to 0.76.0 --- Cargo.lock | 2 +- crates/stackable-operator/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bfe34f3d7..11a4cc65e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2926,7 +2926,7 @@ dependencies = [ [[package]] name = "stackable-operator" -version = "0.75.0" +version = "0.76.0" dependencies = [ "chrono", "clap", diff --git a/crates/stackable-operator/Cargo.toml b/crates/stackable-operator/Cargo.toml index c0c871155..0aaa82e57 100644 --- a/crates/stackable-operator/Cargo.toml +++ b/crates/stackable-operator/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "stackable-operator" description = "Stackable Operator Framework" -version = "0.75.0" +version = "0.76.0" authors.workspace = true license.workspace = true edition.workspace = true From 9e73900786e6aca1d1bf8f276f9faf645f7b1b1b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 19 Sep 2024 10:42:19 +0200 Subject: [PATCH 25/26] Update crates/stackable-operator/src/commons/networking.rs Co-authored-by: Techassi --- crates/stackable-operator/src/commons/networking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 598ac6305..c0b6a5191 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -58,7 +58,7 @@ pub enum HostNameParseError { InvalidHostname { hostname: String }, } -/// A validated hostname (either a [`DomainName`] or IP address) type. +/// A validated hostname, which is either a [`DomainName`] or [`IpAddr`]. #[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] #[serde(try_from = "String", into = "String")] pub enum HostName { From 87ca3050bda8070d36076706bbea41f2ee008e3d Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 19 Sep 2024 10:54:13 +0200 Subject: [PATCH 26/26] fix domain names --- crates/stackable-operator/src/commons/networking.rs | 6 +++--- crates/stackable-operator/src/validation.rs | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index c0b6a5191..19e58c2eb 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -6,7 +6,7 @@ use snafu::Snafu; use crate::validation; -/// A validated domain name type conforming to RFC 1123, e.g. an IPv4, but not an IPv6 address. +/// A validated domain name type conforming to RFC 1123, so e.g. not an IP addresses #[derive( Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, JsonSchema, )] @@ -176,8 +176,6 @@ mod tests { #[rstest] #[case("foo")] #[case("foo.bar")] - // This is also a valid domain name - #[case("1.2.3.4")] fn test_domain_name_and_host_name_parsing_success(#[case] domain_name: String) { let parsed_domain_name: DomainName = domain_name.parse().expect("domain name can not be parsed"); @@ -192,6 +190,8 @@ mod tests { #[rstest] #[case("")] #[case("foo.bar:1234")] + // FIXME: This should not be an valid domain name! + // #[case("1.2.3.4")] #[case("fe80::1")] fn test_domain_name_parsing_invalid_input(#[case] domain_name: &str) { assert!(domain_name.parse::().is_err()); diff --git a/crates/stackable-operator/src/validation.rs b/crates/stackable-operator/src/validation.rs index 8f22e8202..4472e4aaf 100644 --- a/crates/stackable-operator/src/validation.rs +++ b/crates/stackable-operator/src/validation.rs @@ -15,6 +15,8 @@ use const_format::concatcp; use regex::Regex; use snafu::Snafu; +// FIXME: According to https://www.rfc-editor.org/rfc/rfc1035#section-2.3.1 domain names must start with a letter +// (and not a number). const RFC_1123_LABEL_FMT: &str = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"; const RFC_1123_SUBDOMAIN_FMT: &str = concatcp!(RFC_1123_LABEL_FMT, "(\\.", RFC_1123_LABEL_FMT, ")*");