From ab8e419ac622c6edeeeda1008322dacf52e49060 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Tue, 15 Oct 2024 15:59:12 +0200 Subject: [PATCH 01/28] wip --- crates/stackable-operator/src/utils/mod.rs | 1 + .../src/utils/service_dns_domain.rs | 202 ++++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 crates/stackable-operator/src/utils/service_dns_domain.rs diff --git a/crates/stackable-operator/src/utils/mod.rs b/crates/stackable-operator/src/utils/mod.rs index b9b1e1632..0d9262f98 100644 --- a/crates/stackable-operator/src/utils/mod.rs +++ b/crates/stackable-operator/src/utils/mod.rs @@ -2,6 +2,7 @@ pub mod bash; pub mod crds; pub mod logging; mod option; +pub mod service_dns_domain; mod url; #[deprecated( diff --git a/crates/stackable-operator/src/utils/service_dns_domain.rs b/crates/stackable-operator/src/utils/service_dns_domain.rs new file mode 100644 index 000000000..92067c450 --- /dev/null +++ b/crates/stackable-operator/src/utils/service_dns_domain.rs @@ -0,0 +1,202 @@ +use snafu::{ResultExt, Snafu}; +use std::{ + env, + io::{self, BufRead}, + path::Path, + sync::LazyLock, +}; + +use crate::commons::networking::DomainName; + +const KUBERNETES_SERVICE_DNS_DOMAIN: &str = "KUBERNETES_SERVICE_DNS_DOMAIN"; +const KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT: &str = "cluster.local"; + +const KUBERNETES_SERVICE_HOST: &str = "KUBERNETES_SERVICE_HOST"; + +const RESOLVE_CONF_FILE_PATH: &str = "/etc/resolv.conf"; + +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(display("Env var '{name}' does not exist."))] + EnvVarNotFound { source: env::VarError, name: String }, + + #[snafu(display("Could not find '{resolve_conf_file_path}'."))] + ResolvConfNotFound { + source: io::Error, + resolve_conf_file_path: String, + }, + + #[snafu(display("Could not find any 'search' entry in '{resolve_conf_file_path}'."))] + SearchEntryNotFound { resolve_conf_file_path: String }, +} + +/// This is the primary entry point to retrieve the Kubernetes service DNS domain. +/// +/// Implements the logic decided in +/// +/// 1. Check if KUBERNETES_SERVICE_DNS_DOMAIN is set -> return if set +/// 2. Check if KUBERNETES_SERVICE_HOST is set to determine if we run in a Kubernetes / Openshift cluster +/// 2.1 If set continue and parse the `resolv.conf` +/// 2.2 If not set default to `cluster.local` +/// 3. Read and parse the `resolv.conf`. +/// +/// NOTE: +/// The whole code has many many unwraps and expects. +/// Since this code will be evaluated once and is crucial for +/// successful cluster deployments, we actually want to *crash* +/// the operator if we cannot find a proper value for the service dns domain. +/// +/// # Usage +/// +/// ``` +/// let kubernetes_service_dns_domain = *SERVICE_DNS_DOMAIN; +/// ``` +/// +pub static SERVICE_DNS_DOMAIN: LazyLock = LazyLock::new(|| { + // 1. Read KUBERNETES_SERVICE_DNS_DOMAIN env var + tracing::info!("Trying to determine the Kubernetes service DNS domain..."); + match read_env_var(KUBERNETES_SERVICE_DNS_DOMAIN) { + Ok(service_dns_domain) => return service_dns_domain.try_into().unwrap(), + Err(_) => { + tracing::info!("The env var '{KUBERNETES_SERVICE_DNS_DOMAIN}' is not set."); + } + }; + + // 2. If no env var is set, check if we run in a clusterized (Kubernetes/Openshift) enviroment + // by checking if KUBERNETES_SERVICE_HOST is set: If not default to 'cluster.local'. + tracing::info!("Trying to determine the runtime environment..."); + if read_env_var(KUBERNETES_SERVICE_HOST).is_err() { + tracing::info!("The env var '{KUBERNETES_SERVICE_HOST}' is not set. This means we do not run in Kubernetes / Openshift. Defaulting DNS domain to '{KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT}'."); + // The unwrap is safe here and should never fail + return KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT + .to_string() + .try_into() + .unwrap(); + } + + // 3. Read and parse 'resolv.conf'. We are looking for the last "search" entry and filter for the shortest + // element in that search line + let resolve_conf_lines = read_file_from_path(RESOLVE_CONF_FILE_PATH) + .context(ResolvConfNotFoundSnafu { + resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), + }).unwrap(); + + parse_resolve_config(resolve_conf_lines).try_into().unwrap() +}); + +fn parse_resolve_config(resolv_conf: Vec) -> String { + tracing::debug!( + "Start parsing '{RESOLVE_CONF_FILE_PATH}' to retrieve the Kubernetes service DNS domain..." + ); + + // The unwraps/expects here are to hard abort if this fails. + // This will crash the operator at the start which is desired in that case. + let last_search_entry = find_last_search_entry(&resolv_conf) + .expect("No 'search' entries found in '{RESOLVE_CONF_FILE_PATH}'. Aborting..."); + let last_search_entry_content = parse_search_line(&last_search_entry) + .expect("No 'search' entry found in {last_search_entry}. Aborting..."); + let shortest_search_entry = find_shortest_entry(last_search_entry_content) + .expect("No valid entries found in the line '{last_search_entry_content}'. Aborting..."); + + shortest_search_entry.to_string() +} + +fn read_env_var(name: &str) -> Result { + env::var(name).context(EnvVarNotFoundSnafu { name }) +} + +// Function to read the contents of a file and return all lines as Vec +fn read_file_from_path(resolv_conf_file_path: &str) -> Result, io::Error> { + let file = std::fs::File::open(Path::new(resolv_conf_file_path))?; + let reader = io::BufReader::new(file); + + reader.lines().collect() +} + +// Function to find the last search entry in the lines +fn find_last_search_entry(lines: &[String]) -> Option { + lines + .iter() + .rev() // Start from the end to find the last occurrence + .find(|line| line.trim().starts_with("search")) + .cloned() // Convert the reference to a cloned String +} + +// Function to remove the "search" keyword and return the remaining entries +fn parse_search_line(search_line: &str) -> Option<&str> { + search_line.trim().strip_prefix("search") +} + +// Function to find the shortest entry in the parsed search line +fn find_shortest_entry(search_content: &str) -> Option<&str> { + search_content + .split_whitespace() + .min_by_key(|entry| entry.len()) +} + +#[cfg(test)] +mod tests { + use super::*; + + const KUBERNETES_RESOLV_CONF: &str = r#""" + search sble-operators.svc.cluster.local svc.cluster.local cluster.local + nameserver 10.243.21.53 + options ndots:5 + """#; + + const OPENSHIFT_RESOLV_CONF: &str = r#""" + search openshift-service-ca-operator.svc.cluster.local svc.cluster.local cluster.local cmx.repl-openshift.build + nameserver 172.30.0.10 + options ndots:5 + """#; + + const KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES: &str = r#""" + search baz svc.foo.bar foo.bar + search sble-operators.svc.cluster.local svc.cluster.local cluster.local + nameserver 10.243.21.53 + options ndots:5 + """#; + + const KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES: &str = r#""" + nameserver 10.243.21.53 + options ndots:5 + """#; + + fn read_file_from_string(contents: &str) -> Vec { + // Split the string by lines and collect into a Vec + contents.lines().map(|line| line.to_string()).collect() + } + + #[test] + fn use_different_kubernetes_service_dns_domain_value() { + let service_dns_domain = "my-cluster.local".to_string(); + unsafe { + env::set_var(KUBERNETES_SERVICE_DNS_DOMAIN, &service_dns_domain); + } + assert_eq!(*SERVICE_DNS_DOMAIN, service_dns_domain.try_into().unwrap()); + } + + #[test] + fn parse_resolv_conf_success() { + let correct_resolv_configs = vec![ + KUBERNETES_RESOLV_CONF, + OPENSHIFT_RESOLV_CONF, + KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES, + ]; + + for resolv_conf in correct_resolv_configs { + let lines = read_file_from_string(resolv_conf); + let last_search_entry = find_last_search_entry(lines.as_slice()).unwrap(); + let search_entry = parse_search_line(&last_search_entry).unwrap(); + let service_dns_domain = find_shortest_entry(search_entry).unwrap(); + assert_eq!(service_dns_domain, "cluster.local") + } + } + + #[test] + fn parse_resolv_conf_error_no_search_entry() { + let lines = read_file_from_string(KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES); + let last_search_entry = find_last_search_entry(lines.as_slice()); + assert_eq!(last_search_entry, None) + } +} From e972aeeca25202700dbeea04bbc60bdd82d79f54 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:12:23 +0200 Subject: [PATCH 02/28] rework - wip --- crates/stackable-operator/src/client.rs | 14 +- .../src/utils/cluster_domain.rs | 247 ++++++++++++++++++ crates/stackable-operator/src/utils/mod.rs | 4 +- .../src/utils/service_dns_domain.rs | 202 -------------- 4 files changed, 263 insertions(+), 204 deletions(-) create mode 100644 crates/stackable-operator/src/utils/cluster_domain.rs delete mode 100644 crates/stackable-operator/src/utils/service_dns_domain.rs diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index 235c3e2eb..9b175306d 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -1,4 +1,7 @@ use crate::kvp::LabelSelectorExt; +use crate::utils::cluster_domain::{ + self, resolve_kubernetes_cluster_domain, KUBERNETES_CLUSTER_DOMAIN, +}; use either::Either; use futures::StreamExt; @@ -77,6 +80,9 @@ pub enum Error { #[snafu(display("unable to create kubernetes client"))] CreateKubeClient { source: kube::Error }, + + #[snafu(display("unable to to resolve kubernetes cluster domain"))] + ResolveKubernetesClusterDomain { source: cluster_domain::Error }, } /// This `Client` can be used to access Kubernetes. @@ -622,7 +628,13 @@ where } } -pub async fn create_client(field_manager: Option) -> Result { +pub async fn initialize_operator(field_manager: Option) -> Result { + let _ = KUBERNETES_CLUSTER_DOMAIN + .set(resolve_kubernetes_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?); + create_client(field_manager).await +} + +async fn create_client(field_manager: Option) -> Result { let kubeconfig: Config = kube::Config::infer() .await .map_err(kube::Error::InferConfig) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs new file mode 100644 index 000000000..24f693a38 --- /dev/null +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -0,0 +1,247 @@ +use std::{ + env, + io::{self, BufRead}, + path::Path, + sync::OnceLock, +}; + +use snafu::{OptionExt, ResultExt, Snafu}; + +use crate::commons::networking::DomainName; + +// Env vars +const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; +const KUBERNETES_SERVICE_HOST_ENV: &str = "KUBERNETES_SERVICE_HOST"; +// Misc +const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; +const RESOLVE_CONF_FILE_PATH: &str = "/etc/resolv.conf"; + +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(display("Env var '{name}' does not exist."))] + EnvVarNotFound { source: env::VarError, name: String }, + + #[snafu(display("Could not find '{resolve_conf_file_path}'."))] + ResolvConfNotFound { + source: io::Error, + resolve_conf_file_path: String, + }, + + #[snafu(display("The provided cluster domain '{cluster_domain}' is not valid."))] + InvalidDomain { + source: crate::validation::Errors, + cluster_domain: String, + }, + + #[snafu(display("No 'search' entries found in '{RESOLVE_CONF_FILE_PATH}'."))] + SearchEntryNotFound { resolve_conf_file_path: String }, + + #[snafu(display("Could not trim search entry in '{search_entry_line}'."))] + TrimSearchEntryFailed { search_entry_line: String }, + + #[snafu(display("Could not find any cluster domain entry in search line."))] + LookupClusterDomainEntryFailed, +} + +/// This is the primary entry point to retrieve the Kubernetes cluster domain. +/// +/// Implements the logic decided in +/// +/// 1. Check if KUBERNETES_CLUSTER_DOMAIN is set -> return if set +/// 2. Check if KUBERNETES_SERVICE_HOST is set to determine if we run in a Kubernetes / Openshift cluster +/// 2.1 If set continue and parse the `resolv.conf` +/// 2.2 If not set default to `cluster.local` +/// 3. Read and parse the `resolv.conf`. +/// +/// # Context +/// +/// This variable is initialized in [`stackable_operator::client::initialize_operator`], which is called +/// in the main function. It can be used as suggested below. +/// +/// # Usage +/// +/// ``` +/// use stackable_operator::utils::KUBERNETES_CLUSTER_DOMAIN; +/// +/// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get().expect("Could not resolve the Kubernetes cluster domain!"); +/// tracing::info!("Found cluster domain: {kubernetes_cluster_domain}"); +/// ``` +/// +pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); + +pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { + // 1. Read KUBERNETES_CLUSTER_DOMAIN env var + tracing::info!("Trying to determine the Kubernetes cluster domain..."); + match read_env_var(KUBERNETES_CLUSTER_DOMAIN_ENV) { + Ok(cluster_domain) => { + return Ok(cluster_domain + .clone() + .try_into() + .context(InvalidDomainSnafu { cluster_domain })?) + } + Err(_) => { + tracing::info!("The env var '{KUBERNETES_CLUSTER_DOMAIN_ENV}' is not set."); + } + }; + + // 2. If no env var is set, check if we run in a clusterized (Kubernetes/Openshift) enviroment + // by checking if KUBERNETES_SERVICE_HOST is set: If not default to 'cluster.local'. + tracing::info!("Trying to determine the operator runtime environment..."); + if read_env_var(KUBERNETES_SERVICE_HOST_ENV).is_err() { + tracing::info!("The env var '{KUBERNETES_SERVICE_HOST_ENV}' is not set. This means we do not run in Kubernetes / Openshift. Defaulting cluster domain to '{KUBERNETES_CLUSTER_DOMAIN_DEFAULT}'."); + return KUBERNETES_CLUSTER_DOMAIN_DEFAULT + .to_string() + .try_into() + .context(InvalidDomainSnafu { + cluster_domain: KUBERNETES_CLUSTER_DOMAIN_DEFAULT.to_string(), + }); + } + + // 3. Read and parse 'resolv.conf'. We are looking for the last "search" entry and filter for the shortest + // element in that search line + let resolve_conf_lines = + read_file_from_path(RESOLVE_CONF_FILE_PATH).context(ResolvConfNotFoundSnafu { + resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), + })?; + + let cluster_domain = parse_resolve_config(resolve_conf_lines)?; + tracing::info!("Using Kubernetes cluster domain: {cluster_domain}"); + + Ok(cluster_domain + .clone() + .try_into() + .context(InvalidDomainSnafu { cluster_domain })?) +} + +/// Extract the Kubernetes cluster domain from the vectorized 'resolv.conf'. +/// This will: +/// 1. Use the last entry containing a 'search' prefix. +/// 2. Strip 'search' from the last entry. +/// 3. Return the shortest itme (e.g. 'cluster.local') in the whitespace seperated list. +fn parse_resolve_config(resolv_conf: Vec) -> Result { + tracing::debug!( + "Start parsing '{RESOLVE_CONF_FILE_PATH}' to retrieve the Kubernetes cluster domain..." + ); + + let last_search_entry = + find_last_search_entry(&resolv_conf).context(SearchEntryNotFoundSnafu { + resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), + })?; + + let last_search_entry_content = + trim_search_line(&last_search_entry).context(TrimSearchEntryFailedSnafu { + search_entry_line: last_search_entry.to_string(), + })?; + + let shortest_search_entry = find_shortest_entry(last_search_entry_content) + .context(LookupClusterDomainEntryFailedSnafu)?; + + Ok(shortest_search_entry.into()) +} + +/// Read an ENV variable +fn read_env_var(name: &str) -> Result { + env::var(name).context(EnvVarNotFoundSnafu { name }) +} + +// Function to read the contents of a file and return all lines as Vec +fn read_file_from_path(resolv_conf_file_path: &str) -> Result, io::Error> { + let file = std::fs::File::open(Path::new(resolv_conf_file_path))?; + let reader = io::BufReader::new(file); + + reader.lines().collect() +} + +/// Search the last entry containing the 'search' prefix. We are only interested in +/// the last line (in case there are multiple entries which would be ignored by external tools). +fn find_last_search_entry(lines: &[String]) -> Option { + lines + .iter() + .rev() // Start from the end to find the last occurrence + .find(|line| line.trim().starts_with("search")) + .cloned() +} + +/// Extract the content of the 'search' line. Basically stripping the 'search' prefix from the line like: +/// 'search sble-operators.svc.cluster.local svc.cluster.local cluster.local' will become +/// 'sble-operators.svc.cluster.local svc.cluster.local cluster.local' +fn trim_search_line(search_line: &str) -> Option<&str> { + search_line.trim().strip_prefix("search") +} + +/// Extract the shortest entry from a whitespace seperated string like: +/// 'sble-operators.svc.cluster.local svc.cluster.local cluster.local' +/// This will be 'cluster.local' here. +fn find_shortest_entry(search_content: &str) -> Option<&str> { + search_content + .split_whitespace() + .min_by_key(|entry| entry.len()) +} + +#[cfg(test)] +mod tests { + use super::*; + + const KUBERNETES_RESOLV_CONF: &str = r#""" + search sble-operators.svc.cluster.local svc.cluster.local cluster.local + nameserver 10.243.21.53 + options ndots:5 + """#; + + const OPENSHIFT_RESOLV_CONF: &str = r#""" + search openshift-service-ca-operator.svc.cluster.local svc.cluster.local cluster.local cmx.repl-openshift.build + nameserver 172.30.0.10 + options ndots:5 + """#; + + const KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES: &str = r#""" + search baz svc.foo.bar foo.bar + search sble-operators.svc.cluster.local svc.cluster.local cluster.local + nameserver 10.243.21.53 + options ndots:5 + """#; + + const KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES: &str = r#""" + nameserver 10.243.21.53 + options ndots:5 + """#; + + // Helper method to read resolv.conf from a string and not from file. + fn read_file_from_string(contents: &str) -> Vec { + // Split the string by lines and collect into a Vec + contents.lines().map(|line| line.to_string()).collect() + } + + #[test] + fn use_different_kubernetes_service_dns_domain_value() { + let service_dns_domain = "my-cluster.local".to_string(); + unsafe { + env::set_var(KUBERNETES_CLUSTER_DOMAIN_ENV, &service_dns_domain); + } + //assert_eq!(KUBERNETES_CLUSTER_DOMAIN.get().expect(""), service_dns_domain.try_into().unwrap()); + } + + #[test] + fn parse_resolv_conf_success() { + let correct_resolv_configs = vec![ + KUBERNETES_RESOLV_CONF, + OPENSHIFT_RESOLV_CONF, + KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES, + ]; + + for resolv_conf in correct_resolv_configs { + let lines = read_file_from_string(resolv_conf); + let last_search_entry = find_last_search_entry(lines.as_slice()).unwrap(); + let search_entry = trim_search_line(&last_search_entry).unwrap(); + let service_dns_domain = find_shortest_entry(search_entry).unwrap(); + //assert_eq!(service_dns_domain, KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT); + } + } + + #[test] + fn parse_resolv_conf_error_no_search_entry() { + let lines = read_file_from_string(KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES); + let last_search_entry = find_last_search_entry(lines.as_slice()); + assert_eq!(last_search_entry, None); + } +} diff --git a/crates/stackable-operator/src/utils/mod.rs b/crates/stackable-operator/src/utils/mod.rs index 0d9262f98..3564f2515 100644 --- a/crates/stackable-operator/src/utils/mod.rs +++ b/crates/stackable-operator/src/utils/mod.rs @@ -1,8 +1,8 @@ pub mod bash; +pub mod cluster_domain; pub mod crds; pub mod logging; mod option; -pub mod service_dns_domain; mod url; #[deprecated( @@ -26,3 +26,5 @@ pub use self::{option::OptionExt, url::UrlExt}; pub(crate) fn format_full_controller_name(operator: &str, controller: &str) -> String { format!("{operator}_{controller}") } + +pub use self::cluster_domain::KUBERNETES_CLUSTER_DOMAIN; diff --git a/crates/stackable-operator/src/utils/service_dns_domain.rs b/crates/stackable-operator/src/utils/service_dns_domain.rs deleted file mode 100644 index 92067c450..000000000 --- a/crates/stackable-operator/src/utils/service_dns_domain.rs +++ /dev/null @@ -1,202 +0,0 @@ -use snafu::{ResultExt, Snafu}; -use std::{ - env, - io::{self, BufRead}, - path::Path, - sync::LazyLock, -}; - -use crate::commons::networking::DomainName; - -const KUBERNETES_SERVICE_DNS_DOMAIN: &str = "KUBERNETES_SERVICE_DNS_DOMAIN"; -const KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT: &str = "cluster.local"; - -const KUBERNETES_SERVICE_HOST: &str = "KUBERNETES_SERVICE_HOST"; - -const RESOLVE_CONF_FILE_PATH: &str = "/etc/resolv.conf"; - -#[derive(Debug, Snafu)] -pub enum Error { - #[snafu(display("Env var '{name}' does not exist."))] - EnvVarNotFound { source: env::VarError, name: String }, - - #[snafu(display("Could not find '{resolve_conf_file_path}'."))] - ResolvConfNotFound { - source: io::Error, - resolve_conf_file_path: String, - }, - - #[snafu(display("Could not find any 'search' entry in '{resolve_conf_file_path}'."))] - SearchEntryNotFound { resolve_conf_file_path: String }, -} - -/// This is the primary entry point to retrieve the Kubernetes service DNS domain. -/// -/// Implements the logic decided in -/// -/// 1. Check if KUBERNETES_SERVICE_DNS_DOMAIN is set -> return if set -/// 2. Check if KUBERNETES_SERVICE_HOST is set to determine if we run in a Kubernetes / Openshift cluster -/// 2.1 If set continue and parse the `resolv.conf` -/// 2.2 If not set default to `cluster.local` -/// 3. Read and parse the `resolv.conf`. -/// -/// NOTE: -/// The whole code has many many unwraps and expects. -/// Since this code will be evaluated once and is crucial for -/// successful cluster deployments, we actually want to *crash* -/// the operator if we cannot find a proper value for the service dns domain. -/// -/// # Usage -/// -/// ``` -/// let kubernetes_service_dns_domain = *SERVICE_DNS_DOMAIN; -/// ``` -/// -pub static SERVICE_DNS_DOMAIN: LazyLock = LazyLock::new(|| { - // 1. Read KUBERNETES_SERVICE_DNS_DOMAIN env var - tracing::info!("Trying to determine the Kubernetes service DNS domain..."); - match read_env_var(KUBERNETES_SERVICE_DNS_DOMAIN) { - Ok(service_dns_domain) => return service_dns_domain.try_into().unwrap(), - Err(_) => { - tracing::info!("The env var '{KUBERNETES_SERVICE_DNS_DOMAIN}' is not set."); - } - }; - - // 2. If no env var is set, check if we run in a clusterized (Kubernetes/Openshift) enviroment - // by checking if KUBERNETES_SERVICE_HOST is set: If not default to 'cluster.local'. - tracing::info!("Trying to determine the runtime environment..."); - if read_env_var(KUBERNETES_SERVICE_HOST).is_err() { - tracing::info!("The env var '{KUBERNETES_SERVICE_HOST}' is not set. This means we do not run in Kubernetes / Openshift. Defaulting DNS domain to '{KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT}'."); - // The unwrap is safe here and should never fail - return KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT - .to_string() - .try_into() - .unwrap(); - } - - // 3. Read and parse 'resolv.conf'. We are looking for the last "search" entry and filter for the shortest - // element in that search line - let resolve_conf_lines = read_file_from_path(RESOLVE_CONF_FILE_PATH) - .context(ResolvConfNotFoundSnafu { - resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), - }).unwrap(); - - parse_resolve_config(resolve_conf_lines).try_into().unwrap() -}); - -fn parse_resolve_config(resolv_conf: Vec) -> String { - tracing::debug!( - "Start parsing '{RESOLVE_CONF_FILE_PATH}' to retrieve the Kubernetes service DNS domain..." - ); - - // The unwraps/expects here are to hard abort if this fails. - // This will crash the operator at the start which is desired in that case. - let last_search_entry = find_last_search_entry(&resolv_conf) - .expect("No 'search' entries found in '{RESOLVE_CONF_FILE_PATH}'. Aborting..."); - let last_search_entry_content = parse_search_line(&last_search_entry) - .expect("No 'search' entry found in {last_search_entry}. Aborting..."); - let shortest_search_entry = find_shortest_entry(last_search_entry_content) - .expect("No valid entries found in the line '{last_search_entry_content}'. Aborting..."); - - shortest_search_entry.to_string() -} - -fn read_env_var(name: &str) -> Result { - env::var(name).context(EnvVarNotFoundSnafu { name }) -} - -// Function to read the contents of a file and return all lines as Vec -fn read_file_from_path(resolv_conf_file_path: &str) -> Result, io::Error> { - let file = std::fs::File::open(Path::new(resolv_conf_file_path))?; - let reader = io::BufReader::new(file); - - reader.lines().collect() -} - -// Function to find the last search entry in the lines -fn find_last_search_entry(lines: &[String]) -> Option { - lines - .iter() - .rev() // Start from the end to find the last occurrence - .find(|line| line.trim().starts_with("search")) - .cloned() // Convert the reference to a cloned String -} - -// Function to remove the "search" keyword and return the remaining entries -fn parse_search_line(search_line: &str) -> Option<&str> { - search_line.trim().strip_prefix("search") -} - -// Function to find the shortest entry in the parsed search line -fn find_shortest_entry(search_content: &str) -> Option<&str> { - search_content - .split_whitespace() - .min_by_key(|entry| entry.len()) -} - -#[cfg(test)] -mod tests { - use super::*; - - const KUBERNETES_RESOLV_CONF: &str = r#""" - search sble-operators.svc.cluster.local svc.cluster.local cluster.local - nameserver 10.243.21.53 - options ndots:5 - """#; - - const OPENSHIFT_RESOLV_CONF: &str = r#""" - search openshift-service-ca-operator.svc.cluster.local svc.cluster.local cluster.local cmx.repl-openshift.build - nameserver 172.30.0.10 - options ndots:5 - """#; - - const KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES: &str = r#""" - search baz svc.foo.bar foo.bar - search sble-operators.svc.cluster.local svc.cluster.local cluster.local - nameserver 10.243.21.53 - options ndots:5 - """#; - - const KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES: &str = r#""" - nameserver 10.243.21.53 - options ndots:5 - """#; - - fn read_file_from_string(contents: &str) -> Vec { - // Split the string by lines and collect into a Vec - contents.lines().map(|line| line.to_string()).collect() - } - - #[test] - fn use_different_kubernetes_service_dns_domain_value() { - let service_dns_domain = "my-cluster.local".to_string(); - unsafe { - env::set_var(KUBERNETES_SERVICE_DNS_DOMAIN, &service_dns_domain); - } - assert_eq!(*SERVICE_DNS_DOMAIN, service_dns_domain.try_into().unwrap()); - } - - #[test] - fn parse_resolv_conf_success() { - let correct_resolv_configs = vec![ - KUBERNETES_RESOLV_CONF, - OPENSHIFT_RESOLV_CONF, - KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES, - ]; - - for resolv_conf in correct_resolv_configs { - let lines = read_file_from_string(resolv_conf); - let last_search_entry = find_last_search_entry(lines.as_slice()).unwrap(); - let search_entry = parse_search_line(&last_search_entry).unwrap(); - let service_dns_domain = find_shortest_entry(search_entry).unwrap(); - assert_eq!(service_dns_domain, "cluster.local") - } - } - - #[test] - fn parse_resolv_conf_error_no_search_entry() { - let lines = read_file_from_string(KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES); - let last_search_entry = find_last_search_entry(lines.as_slice()); - assert_eq!(last_search_entry, None) - } -} From 96ff314f27a05755a660e14a50819fe607538fb0 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:16:43 +0200 Subject: [PATCH 03/28] docs & clippy fix --- crates/stackable-operator/src/utils/cluster_domain.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 24f693a38..53318868b 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -55,7 +55,7 @@ pub enum Error { /// /// # Context /// -/// This variable is initialized in [`stackable_operator::client::initialize_operator`], which is called +/// This variable is initialized in [`crate::client::initialize_operator`], which is called /// in the main function. It can be used as suggested below. /// /// # Usage @@ -74,10 +74,10 @@ pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { tracing::info!("Trying to determine the Kubernetes cluster domain..."); match read_env_var(KUBERNETES_CLUSTER_DOMAIN_ENV) { Ok(cluster_domain) => { - return Ok(cluster_domain + return cluster_domain .clone() .try_into() - .context(InvalidDomainSnafu { cluster_domain })?) + .context(InvalidDomainSnafu { cluster_domain }) } Err(_) => { tracing::info!("The env var '{KUBERNETES_CLUSTER_DOMAIN_ENV}' is not set."); @@ -107,10 +107,10 @@ pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { let cluster_domain = parse_resolve_config(resolve_conf_lines)?; tracing::info!("Using Kubernetes cluster domain: {cluster_domain}"); - Ok(cluster_domain + cluster_domain .clone() .try_into() - .context(InvalidDomainSnafu { cluster_domain })?) + .context(InvalidDomainSnafu { cluster_domain }) } /// Extract the Kubernetes cluster domain from the vectorized 'resolv.conf'. From c699b9c2f24e5cd2de963fc26c111ac9c2e55ae2 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:24:47 +0200 Subject: [PATCH 04/28] fix tests --- .../src/utils/cluster_domain.rs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 53318868b..088a10cc9 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -213,12 +213,21 @@ mod tests { } #[test] - fn use_different_kubernetes_service_dns_domain_value() { - let service_dns_domain = "my-cluster.local".to_string(); + fn use_different_kubernetes_cluster_domain_value() { + let cluster_domain = "my-cluster.local".to_string(); + + // set different domain via env var unsafe { - env::set_var(KUBERNETES_CLUSTER_DOMAIN_ENV, &service_dns_domain); + env::set_var(KUBERNETES_CLUSTER_DOMAIN_ENV, &cluster_domain); } - //assert_eq!(KUBERNETES_CLUSTER_DOMAIN.get().expect(""), service_dns_domain.try_into().unwrap()); + + // initialize the lock + let _ = KUBERNETES_CLUSTER_DOMAIN.set(resolve_kubernetes_cluster_domain().unwrap()); + + assert_eq!( + cluster_domain, + KUBERNETES_CLUSTER_DOMAIN.get().unwrap().to_string() + ); } #[test] @@ -233,8 +242,8 @@ mod tests { let lines = read_file_from_string(resolv_conf); let last_search_entry = find_last_search_entry(lines.as_slice()).unwrap(); let search_entry = trim_search_line(&last_search_entry).unwrap(); - let service_dns_domain = find_shortest_entry(search_entry).unwrap(); - //assert_eq!(service_dns_domain, KUBERNETES_SERVICE_DNS_DOMAIN_DEFAULT); + let cluster_domain = find_shortest_entry(search_entry).unwrap(); + assert_eq!(cluster_domain, KUBERNETES_CLUSTER_DOMAIN_DEFAULT); } } From 68f9d6aa0209b1669e3f16078fd128201f700016 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:31:32 +0200 Subject: [PATCH 05/28] adapted changelog --- crates/stackable-operator/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 7404e2561..013c2e223 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -7,6 +7,8 @@ All notable changes to this project will be documented in this file. ### Added - Re-export the `YamlSchema` trait and the `stackable-shared` crate as the `shared` module ([#883]). +- The cluster domain (default: `cluster.local`) can now be configured via the ENV variable + `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the `/etc/resolve.conf` ([#893]). ### Changed @@ -20,6 +22,7 @@ All notable changes to this project will be documented in this file. use it as a `String`. [#883]: https://github.com/stackabletech/operator-rs/pull/883 +[#893]: https://github.com/stackabletech/operator-rs/pull/893 ## [0.78.0] - 2024-09-30 From dd2d67220d42d520a38587bc38a97ae7f5cde4f8 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:35:49 +0200 Subject: [PATCH 06/28] fix changelog entry --- crates/stackable-operator/CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 013c2e223..b59f940a5 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -7,8 +7,9 @@ All notable changes to this project will be documented in this file. ### Added - Re-export the `YamlSchema` trait and the `stackable-shared` crate as the `shared` module ([#883]). -- The cluster domain (default: `cluster.local`) can now be configured via the ENV variable - `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the `/etc/resolve.conf` ([#893]). +- The cluster domain (default: `cluster.local`) can now be configured in the individual operators + via the ENV variable `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the + `/etc/resolve.conf` ([#893]). ### Changed From 084e1d23d3d96ae8ee5b5ec717222df86b53d318 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:37:58 +0200 Subject: [PATCH 07/28] add missing log message --- crates/stackable-operator/src/utils/cluster_domain.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 088a10cc9..4cac58650 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -74,10 +74,11 @@ pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { tracing::info!("Trying to determine the Kubernetes cluster domain..."); match read_env_var(KUBERNETES_CLUSTER_DOMAIN_ENV) { Ok(cluster_domain) => { + tracing::info!("Using Kubernetes cluster domain: {cluster_domain}"); return cluster_domain .clone() .try_into() - .context(InvalidDomainSnafu { cluster_domain }) + .context(InvalidDomainSnafu { cluster_domain }); } Err(_) => { tracing::info!("The env var '{KUBERNETES_CLUSTER_DOMAIN_ENV}' is not set."); From 06b5aac0f921c39d80ea5b875bf4016eb0bb741a Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 14:42:31 +0200 Subject: [PATCH 08/28] improve logging messages --- crates/stackable-operator/src/utils/cluster_domain.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 4cac58650..e9dafad2f 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -74,7 +74,7 @@ pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { tracing::info!("Trying to determine the Kubernetes cluster domain..."); match read_env_var(KUBERNETES_CLUSTER_DOMAIN_ENV) { Ok(cluster_domain) => { - tracing::info!("Using Kubernetes cluster domain: {cluster_domain}"); + tracing::info!("Using Kubernetes cluster domain: '{cluster_domain}'"); return cluster_domain .clone() .try_into() @@ -100,13 +100,16 @@ pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { // 3. Read and parse 'resolv.conf'. We are looking for the last "search" entry and filter for the shortest // element in that search line + tracing::info!( + "Running in clusterized environment. Attempting to parse '{RESOLVE_CONF_FILE_PATH}' ..." + ); let resolve_conf_lines = read_file_from_path(RESOLVE_CONF_FILE_PATH).context(ResolvConfNotFoundSnafu { resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), })?; let cluster_domain = parse_resolve_config(resolve_conf_lines)?; - tracing::info!("Using Kubernetes cluster domain: {cluster_domain}"); + tracing::info!("Using Kubernetes cluster domain: '{cluster_domain}'"); cluster_domain .clone() From e053479a2fe011eed64a6c646283ec0c056fca4d Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 15:46:35 +0200 Subject: [PATCH 09/28] mark changelog entry as breaking --- crates/stackable-operator/CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 589ded02b..3b2ff3838 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -8,9 +8,10 @@ All notable changes to this project will be documented in this file. - Re-export the `YamlSchema` trait and the `stackable-shared` crate as the `shared` module ([#883]). - BREAKING: Added `preferredAddressType` field to ListenerClass CRD ([#885]). -- The cluster domain (default: `cluster.local`) can now be configured in the individual operators +- BREAKING: The cluster domain (default: `cluster.local`) can now be configured in the individual operators via the ENV variable `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the - `/etc/resolve.conf` ([#893]). + `/etc/resolve.conf`. This requires using `initialze_operator` instead of `create_client` in the `main.rs` + of the individual operators ([#893]). ### Changed From 3d5c01e8a187d77ffa48329829b2bca918d225a9 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 15:56:55 +0200 Subject: [PATCH 10/28] fix whitespaces --- crates/stackable-operator/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 3b2ff3838..aae16f76c 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -8,9 +8,9 @@ All notable changes to this project will be documented in this file. - Re-export the `YamlSchema` trait and the `stackable-shared` crate as the `shared` module ([#883]). - BREAKING: Added `preferredAddressType` field to ListenerClass CRD ([#885]). -- BREAKING: The cluster domain (default: `cluster.local`) can now be configured in the individual operators - via the ENV variable `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the - `/etc/resolve.conf`. This requires using `initialze_operator` instead of `create_client` in the `main.rs` +- BREAKING: The cluster domain (default: `cluster.local`) can now be configured in the individual operators + via the ENV variable `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the + `/etc/resolve.conf`. This requires using `initialze_operator` instead of `create_client` in the `main.rs` of the individual operators ([#893]). ### Changed From 53d8c9f7f8f276868d1ff81f9f70d78366136217 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Wed, 16 Oct 2024 16:17:04 +0200 Subject: [PATCH 11/28] fix tests --- crates/stackable-operator/src/client.rs | 4 ++-- .../stackable-operator/src/utils/cluster_domain.rs | 14 +++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index 9b175306d..bdf4e2f42 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -516,12 +516,12 @@ impl Client { /// use tokio::time::error::Elapsed; /// use kube::runtime::watcher; /// use k8s_openapi::api::core::v1::Pod; - /// use stackable_operator::client::{Client, create_client}; + /// use stackable_operator::client::{Client, initialize_operator}; /// /// #[tokio::main] /// async fn main(){ /// - /// let client: Client = create_client(None).await.expect("Unable to construct client."); + /// let client: Client = initialize_operator(None).await.expect("Unable to construct client."); /// let watcher_config: watcher::Config = /// watcher::Config::default().fields(&format!("metadata.name=nonexistent-pod")); /// diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index e9dafad2f..bf00e2b66 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -60,13 +60,17 @@ pub enum Error { /// /// # Usage /// -/// ``` +/// ```no_run +/// use stackable_operator::client::{Client, initialize_operator}; /// use stackable_operator::utils::KUBERNETES_CLUSTER_DOMAIN; /// -/// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get().expect("Could not resolve the Kubernetes cluster domain!"); -/// tracing::info!("Found cluster domain: {kubernetes_cluster_domain}"); +/// #[tokio::main] +/// async fn main(){ +/// let client: Client = initialize_operator(None).await.expect("Unable to construct client."); +/// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get().expect("Could not resolve the Kubernetes cluster domain!"); +/// tracing::info!("Found cluster domain: {kubernetes_cluster_domain}"); +/// } /// ``` -/// pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { @@ -101,7 +105,7 @@ pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { // 3. Read and parse 'resolv.conf'. We are looking for the last "search" entry and filter for the shortest // element in that search line tracing::info!( - "Running in clusterized environment. Attempting to parse '{RESOLVE_CONF_FILE_PATH}' ..." + "Running in clusterized environment. Attempting to parse '{RESOLVE_CONF_FILE_PATH}'..." ); let resolve_conf_lines = read_file_from_path(RESOLVE_CONF_FILE_PATH).context(ResolvConfNotFoundSnafu { From 32e41d8fb5c5cddc49d4ab4d47a4c9cccfff0106 Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 16 Oct 2024 16:52:50 +0200 Subject: [PATCH 12/28] refactor: Adjust cluster domain parsing code There is still some bits and pieces we want to change, mostly regarding the error type / error handling. This commit also moves the input fixtures for testing the resolv parser into dedicated files and utilizies rstest's #[files] attribute to generate tests based on these files. Co-authored-by: Nick Larsen --- .pre-commit-config.yaml | 2 +- .../cluster_domain/fail/invalid.resolv.conf | 2 + .../pass/kubernetes-multiple.resolv.conf | 4 + .../pass/kubernetes.resolv.conf | 3 + .../cluster_domain/pass/openshift.resolv.conf | 3 + crates/stackable-operator/src/client.rs | 6 +- .../src/utils/cluster_domain.rs | 238 ++++++------------ 7 files changed, 87 insertions(+), 171 deletions(-) create mode 100644 crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf create mode 100644 crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes-multiple.resolv.conf create mode 100644 crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes.resolv.conf create mode 100644 crates/stackable-operator/fixtures/cluster_domain/pass/openshift.resolv.conf diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 01da4702d..691eb8d85 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -51,5 +51,5 @@ repos: name: .scripts/verify-crate-versions language: system entry: .scripts/verify_crate_versions.sh - stages: [commit, merge-commit, manual] + stages: [pre-commit, pre-merge-commit, manual] pass_filenames: false diff --git a/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf b/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf new file mode 100644 index 000000000..f16e34dc2 --- /dev/null +++ b/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf @@ -0,0 +1,2 @@ +nameserver 10.243.21.53 +options ndots:5 diff --git a/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes-multiple.resolv.conf b/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes-multiple.resolv.conf new file mode 100644 index 000000000..578cb8e4f --- /dev/null +++ b/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes-multiple.resolv.conf @@ -0,0 +1,4 @@ +search baz svc.foo.bar foo.bar +search sble-operators.svc.cluster.local svc.cluster.local cluster.local +nameserver 10.243.21.53 +options ndots:5 diff --git a/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes.resolv.conf b/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes.resolv.conf new file mode 100644 index 000000000..9c210febf --- /dev/null +++ b/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes.resolv.conf @@ -0,0 +1,3 @@ +search sble-operators.svc.cluster.local svc.cluster.local cluster.local +nameserver 10.243.21.53 +options ndots:5 diff --git a/crates/stackable-operator/fixtures/cluster_domain/pass/openshift.resolv.conf b/crates/stackable-operator/fixtures/cluster_domain/pass/openshift.resolv.conf new file mode 100644 index 000000000..305b7f795 --- /dev/null +++ b/crates/stackable-operator/fixtures/cluster_domain/pass/openshift.resolv.conf @@ -0,0 +1,3 @@ +search openshift-service-ca-operator.svc.cluster.local svc.cluster.local cluster.local cmx.repl-openshift.build +nameserver 172.30.0.10 +options ndots:5 diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index bdf4e2f42..fec3e3501 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -1,7 +1,5 @@ use crate::kvp::LabelSelectorExt; -use crate::utils::cluster_domain::{ - self, resolve_kubernetes_cluster_domain, KUBERNETES_CLUSTER_DOMAIN, -}; +use crate::utils::cluster_domain::{self, retrieve_cluster_domain, KUBERNETES_CLUSTER_DOMAIN}; use either::Either; use futures::StreamExt; @@ -630,7 +628,7 @@ where pub async fn initialize_operator(field_manager: Option) -> Result { let _ = KUBERNETES_CLUSTER_DOMAIN - .set(resolve_kubernetes_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?); + .set(retrieve_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?); create_client(field_manager).await } diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index bf00e2b66..5c022969c 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -1,40 +1,30 @@ -use std::{ - env, - io::{self, BufRead}, - path::Path, - sync::OnceLock, -}; +use std::{env, path::Path, str::FromStr, sync::OnceLock}; use snafu::{OptionExt, ResultExt, Snafu}; use crate::commons::networking::DomainName; -// Env vars const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; const KUBERNETES_SERVICE_HOST_ENV: &str = "KUBERNETES_SERVICE_HOST"; -// Misc + const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; const RESOLVE_CONF_FILE_PATH: &str = "/etc/resolv.conf"; +// TODO (@Techassi): Do we even need this many variants? Can we get rid of a bunch of variants and +// fall back to defaults instead? Also trace the errors #[derive(Debug, Snafu)] pub enum Error { - #[snafu(display("Env var '{name}' does not exist."))] - EnvVarNotFound { source: env::VarError, name: String }, - - #[snafu(display("Could not find '{resolve_conf_file_path}'."))] - ResolvConfNotFound { - source: io::Error, - resolve_conf_file_path: String, - }, + #[snafu(display("failed to read resolv.conf"))] + ReadResolvConfFile { source: std::io::Error }, - #[snafu(display("The provided cluster domain '{cluster_domain}' is not valid."))] - InvalidDomain { + #[snafu(display("failed to parse {cluster_domain:?} as cluster domain"))] + ParseDomainName { source: crate::validation::Errors, cluster_domain: String, }, - #[snafu(display("No 'search' entries found in '{RESOLVE_CONF_FILE_PATH}'."))] - SearchEntryNotFound { resolve_conf_file_path: String }, + #[snafu(display("No 'search' entries found in"))] + SearchEntryNotFound, #[snafu(display("Could not trim search entry in '{search_entry_line}'."))] TrimSearchEntryFailed { search_entry_line: String }, @@ -73,152 +63,75 @@ pub enum Error { /// ``` pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); -pub(crate) fn resolve_kubernetes_cluster_domain() -> Result { +pub(crate) fn retrieve_cluster_domain() -> Result { // 1. Read KUBERNETES_CLUSTER_DOMAIN env var tracing::info!("Trying to determine the Kubernetes cluster domain..."); - match read_env_var(KUBERNETES_CLUSTER_DOMAIN_ENV) { - Ok(cluster_domain) => { + + match env::var(KUBERNETES_CLUSTER_DOMAIN_ENV) { + Ok(cluster_domain) if !cluster_domain.is_empty() => { tracing::info!("Using Kubernetes cluster domain: '{cluster_domain}'"); - return cluster_domain - .clone() - .try_into() - .context(InvalidDomainSnafu { cluster_domain }); + return DomainName::from_str(&cluster_domain) + .context(ParseDomainNameSnafu { cluster_domain }); } - Err(_) => { - tracing::info!("The env var '{KUBERNETES_CLUSTER_DOMAIN_ENV}' is not set."); + _ => { + tracing::info!("The env var '{KUBERNETES_CLUSTER_DOMAIN_ENV}' is not set or empty"); } }; - // 2. If no env var is set, check if we run in a clusterized (Kubernetes/Openshift) enviroment + // 2. If no env var is set, check if we run in a clustered (Kubernetes/Openshift) environment // by checking if KUBERNETES_SERVICE_HOST is set: If not default to 'cluster.local'. tracing::info!("Trying to determine the operator runtime environment..."); - if read_env_var(KUBERNETES_SERVICE_HOST_ENV).is_err() { - tracing::info!("The env var '{KUBERNETES_SERVICE_HOST_ENV}' is not set. This means we do not run in Kubernetes / Openshift. Defaulting cluster domain to '{KUBERNETES_CLUSTER_DOMAIN_DEFAULT}'."); - return KUBERNETES_CLUSTER_DOMAIN_DEFAULT - .to_string() - .try_into() - .context(InvalidDomainSnafu { - cluster_domain: KUBERNETES_CLUSTER_DOMAIN_DEFAULT.to_string(), - }); - } - // 3. Read and parse 'resolv.conf'. We are looking for the last "search" entry and filter for the shortest - // element in that search line - tracing::info!( - "Running in clusterized environment. Attempting to parse '{RESOLVE_CONF_FILE_PATH}'..." - ); - let resolve_conf_lines = - read_file_from_path(RESOLVE_CONF_FILE_PATH).context(ResolvConfNotFoundSnafu { - resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), - })?; + match env::var(KUBERNETES_SERVICE_HOST_ENV) { + Ok(_) => { + let cluster_domain = retrieve_cluster_domain_from_resolv_conf(RESOLVE_CONF_FILE_PATH)?; - let cluster_domain = parse_resolve_config(resolve_conf_lines)?; - tracing::info!("Using Kubernetes cluster domain: '{cluster_domain}'"); + tracing::info!( + cluster_domain, + "Using Kubernetes cluster domain from {RESOLVE_CONF_FILE_PATH} file" + ); - cluster_domain - .clone() - .try_into() - .context(InvalidDomainSnafu { cluster_domain }) + DomainName::from_str(&cluster_domain).context(ParseDomainNameSnafu { cluster_domain }) + } + Err(_) => { + tracing::info!( + cluster_domain = KUBERNETES_CLUSTER_DOMAIN_DEFAULT, + "Using default Kubernetes cluster domain" + ); + Ok(DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT).expect("stuff")) + } + } } -/// Extract the Kubernetes cluster domain from the vectorized 'resolv.conf'. -/// This will: -/// 1. Use the last entry containing a 'search' prefix. -/// 2. Strip 'search' from the last entry. -/// 3. Return the shortest itme (e.g. 'cluster.local') in the whitespace seperated list. -fn parse_resolve_config(resolv_conf: Vec) -> Result { - tracing::debug!( - "Start parsing '{RESOLVE_CONF_FILE_PATH}' to retrieve the Kubernetes cluster domain..." - ); - - let last_search_entry = - find_last_search_entry(&resolv_conf).context(SearchEntryNotFoundSnafu { - resolve_conf_file_path: RESOLVE_CONF_FILE_PATH.to_string(), - })?; - - let last_search_entry_content = - trim_search_line(&last_search_entry).context(TrimSearchEntryFailedSnafu { - search_entry_line: last_search_entry.to_string(), - })?; - - let shortest_search_entry = find_shortest_entry(last_search_entry_content) +fn retrieve_cluster_domain_from_resolv_conf

(path: P) -> Result +where + P: AsRef, +{ + let content = std::fs::read_to_string(path).context(ReadResolvConfFileSnafu)?; + + let last = content + .lines() + .map(|l| l.trim()) + .filter(|l| l.starts_with("search")) + .map(|l| l.trim_start_matches("search")) + .last() + .context(SearchEntryNotFoundSnafu)?; + + let shortest_entry = last + .split_ascii_whitespace() + .min_by_key(|item| item.len()) .context(LookupClusterDomainEntryFailedSnafu)?; - Ok(shortest_search_entry.into()) -} - -/// Read an ENV variable -fn read_env_var(name: &str) -> Result { - env::var(name).context(EnvVarNotFoundSnafu { name }) -} - -// Function to read the contents of a file and return all lines as Vec -fn read_file_from_path(resolv_conf_file_path: &str) -> Result, io::Error> { - let file = std::fs::File::open(Path::new(resolv_conf_file_path))?; - let reader = io::BufReader::new(file); - - reader.lines().collect() -} - -/// Search the last entry containing the 'search' prefix. We are only interested in -/// the last line (in case there are multiple entries which would be ignored by external tools). -fn find_last_search_entry(lines: &[String]) -> Option { - lines - .iter() - .rev() // Start from the end to find the last occurrence - .find(|line| line.trim().starts_with("search")) - .cloned() -} - -/// Extract the content of the 'search' line. Basically stripping the 'search' prefix from the line like: -/// 'search sble-operators.svc.cluster.local svc.cluster.local cluster.local' will become -/// 'sble-operators.svc.cluster.local svc.cluster.local cluster.local' -fn trim_search_line(search_line: &str) -> Option<&str> { - search_line.trim().strip_prefix("search") -} - -/// Extract the shortest entry from a whitespace seperated string like: -/// 'sble-operators.svc.cluster.local svc.cluster.local cluster.local' -/// This will be 'cluster.local' here. -fn find_shortest_entry(search_content: &str) -> Option<&str> { - search_content - .split_whitespace() - .min_by_key(|entry| entry.len()) + // NOTE (@Techassi): This is really sad and bothers me more than I would like to admit + Ok(shortest_entry.to_owned()) } #[cfg(test)] mod tests { - use super::*; - - const KUBERNETES_RESOLV_CONF: &str = r#""" - search sble-operators.svc.cluster.local svc.cluster.local cluster.local - nameserver 10.243.21.53 - options ndots:5 - """#; - - const OPENSHIFT_RESOLV_CONF: &str = r#""" - search openshift-service-ca-operator.svc.cluster.local svc.cluster.local cluster.local cmx.repl-openshift.build - nameserver 172.30.0.10 - options ndots:5 - """#; - - const KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES: &str = r#""" - search baz svc.foo.bar foo.bar - search sble-operators.svc.cluster.local svc.cluster.local cluster.local - nameserver 10.243.21.53 - options ndots:5 - """#; + use std::path::PathBuf; - const KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES: &str = r#""" - nameserver 10.243.21.53 - options ndots:5 - """#; - - // Helper method to read resolv.conf from a string and not from file. - fn read_file_from_string(contents: &str) -> Vec { - // Split the string by lines and collect into a Vec - contents.lines().map(|line| line.to_string()).collect() - } + use super::*; + use rstest::rstest; #[test] fn use_different_kubernetes_cluster_domain_value() { @@ -230,7 +143,7 @@ mod tests { } // initialize the lock - let _ = KUBERNETES_CLUSTER_DOMAIN.set(resolve_kubernetes_cluster_domain().unwrap()); + let _ = KUBERNETES_CLUSTER_DOMAIN.set(retrieve_cluster_domain().unwrap()); assert_eq!( cluster_domain, @@ -238,27 +151,20 @@ mod tests { ); } - #[test] - fn parse_resolv_conf_success() { - let correct_resolv_configs = vec![ - KUBERNETES_RESOLV_CONF, - OPENSHIFT_RESOLV_CONF, - KUBERNETES_RESOLV_CONF_MULTIPLE_SEARCH_ENTRIES, - ]; - - for resolv_conf in correct_resolv_configs { - let lines = read_file_from_string(resolv_conf); - let last_search_entry = find_last_search_entry(lines.as_slice()).unwrap(); - let search_entry = trim_search_line(&last_search_entry).unwrap(); - let cluster_domain = find_shortest_entry(search_entry).unwrap(); - assert_eq!(cluster_domain, KUBERNETES_CLUSTER_DOMAIN_DEFAULT); - } + #[rstest] + fn parse_resolv_conf_pass( + #[files("fixtures/cluster_domain/pass/*.resolv.conf")] path: PathBuf, + ) { + assert_eq!( + retrieve_cluster_domain_from_resolv_conf(path).unwrap(), + KUBERNETES_CLUSTER_DOMAIN_DEFAULT + ); } - #[test] - fn parse_resolv_conf_error_no_search_entry() { - let lines = read_file_from_string(KUBERNETES_RESOLV_CONF_MISSING_SEARCH_ENTRIES); - let last_search_entry = find_last_search_entry(lines.as_slice()); - assert_eq!(last_search_entry, None); + #[rstest] + fn parse_resolv_conf_fail( + #[files("fixtures/cluster_domain/fail/*.resolv.conf")] path: PathBuf, + ) { + assert!(retrieve_cluster_domain_from_resolv_conf(path).is_err()); } } From d3bd713db4b58142bc3d2b0d7d92c59c5328d2c0 Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 17 Oct 2024 08:30:44 +0200 Subject: [PATCH 13/28] chore: Adjust error variants --- .../src/utils/cluster_domain.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 5c022969c..79df5074e 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -17,20 +17,17 @@ pub enum Error { #[snafu(display("failed to read resolv.conf"))] ReadResolvConfFile { source: std::io::Error }, - #[snafu(display("failed to parse {cluster_domain:?} as cluster domain"))] + #[snafu(display("failed to parse {cluster_domain:?} as domain name"))] ParseDomainName { source: crate::validation::Errors, cluster_domain: String, }, - #[snafu(display("No 'search' entries found in"))] - SearchEntryNotFound, + #[snafu(display("unable to find \"search\" entry"))] + NoSearchEntry, - #[snafu(display("Could not trim search entry in '{search_entry_line}'."))] - TrimSearchEntryFailed { search_entry_line: String }, - - #[snafu(display("Could not find any cluster domain entry in search line."))] - LookupClusterDomainEntryFailed, + #[snafu(display("unable to find unambiguous domain in \"search\" entry"))] + AmbiguousDomainEntries, } /// This is the primary entry point to retrieve the Kubernetes cluster domain. @@ -115,12 +112,12 @@ where .filter(|l| l.starts_with("search")) .map(|l| l.trim_start_matches("search")) .last() - .context(SearchEntryNotFoundSnafu)?; + .context(NoSearchEntrySnafu)?; let shortest_entry = last .split_ascii_whitespace() .min_by_key(|item| item.len()) - .context(LookupClusterDomainEntryFailedSnafu)?; + .context(AmbiguousDomainEntriesSnafu)?; // NOTE (@Techassi): This is really sad and bothers me more than I would like to admit Ok(shortest_entry.to_owned()) From 5da95d747790f0c49faa65e0c6661011b0b5d622 Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 17 Oct 2024 08:33:00 +0200 Subject: [PATCH 14/28] chore: Update dev comment --- crates/stackable-operator/src/utils/cluster_domain.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 79df5074e..7c6b5f901 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -119,7 +119,9 @@ where .min_by_key(|item| item.len()) .context(AmbiguousDomainEntriesSnafu)?; - // NOTE (@Techassi): This is really sad and bothers me more than I would like to admit + // NOTE (@Techassi): This is really sad and bothers me more than I would like to admit. This + // clone could be removed by using the code directly in the calling function. But that would + // remove the possibility to easily test the parsing. Ok(shortest_entry.to_owned()) } From c4b22b56838e48eaa480cde1668bc68c5149193c Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 17 Oct 2024 08:43:08 +0200 Subject: [PATCH 15/28] chore: Adjust doc comment --- .../src/utils/cluster_domain.rs | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 7c6b5f901..bf8c7c4a5 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -30,34 +30,30 @@ pub enum Error { AmbiguousDomainEntries, } -/// This is the primary entry point to retrieve the Kubernetes cluster domain. +/// Tries to retrieve the Kubernetes cluster domain. /// -/// Implements the logic decided in +/// 1. Return `KUBERNETES_CLUSTER_DOMAIN` if set, otherwise +/// 2. Return the cluster domain parsed from the `/etc/resolv.conf` file if `KUBERNETES_SERVICE_HOST` +/// is set, otherwise fall back to `cluster.local`. cluster. /// -/// 1. Check if KUBERNETES_CLUSTER_DOMAIN is set -> return if set -/// 2. Check if KUBERNETES_SERVICE_HOST is set to determine if we run in a Kubernetes / Openshift cluster -/// 2.1 If set continue and parse the `resolv.conf` -/// 2.2 If not set default to `cluster.local` -/// 3. Read and parse the `resolv.conf`. +/// This variable is initialized in [`crate::client::initialize_operator`], which is called in the +/// main function. It can be used as suggested below. /// -/// # Context +/// ## Usage /// -/// This variable is initialized in [`crate::client::initialize_operator`], which is called -/// in the main function. It can be used as suggested below. -/// -/// # Usage -/// -/// ```no_run -/// use stackable_operator::client::{Client, initialize_operator}; +/// ``` /// use stackable_operator::utils::KUBERNETES_CLUSTER_DOMAIN; /// -/// #[tokio::main] -/// async fn main(){ -/// let client: Client = initialize_operator(None).await.expect("Unable to construct client."); -/// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get().expect("Could not resolve the Kubernetes cluster domain!"); -/// tracing::info!("Found cluster domain: {kubernetes_cluster_domain}"); -/// } +/// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get() +/// .expect("KUBERNETES_CLUSTER_DOMAIN must first be set by calling initialize_operator"); +/// +/// tracing::info!(%kubernetes_cluster_domain, "Found cluster domain"); /// ``` +/// +/// ## See +/// +/// - +/// - pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); pub(crate) fn retrieve_cluster_domain() -> Result { From ab93d4ec632572678778929a3137dc95379cdf12 Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 17 Oct 2024 08:43:24 +0200 Subject: [PATCH 16/28] chore: Adjust changelog --- crates/stackable-operator/CHANGELOG.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 4c5a22612..c29a39da9 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -8,23 +8,26 @@ All notable changes to this project will be documented in this file. - Re-export the `YamlSchema` trait and the `stackable-shared` crate as the `shared` module ([#883]). - BREAKING: Added `preferredAddressType` field to ListenerClass CRD ([#885]). -- BREAKING: The cluster domain (default: `cluster.local`) can now be configured in the individual operators - via the ENV variable `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing the - `/etc/resolve.conf`. This requires using `initialze_operator` instead of `create_client` in the `main.rs` - of the individual operators ([#893]). +- BREAKING: The cluster domain (default: `cluster.local`) can now be configured in the individual + operators via the ENV variable `KUBERNETES_CLUSTER_DOMAIN` or resolved automatically by parsing + the `/etc/resolve.conf` file. This requires using `initialize_operator` instead of `create_client` + in the `main.rs` of the individual operators ([#893]). ### Changed - BREAKING: The `CustomResourceExt` trait is now re-exported from the `stackable-shared` crate. The trait functions use the same parameters but return a different error type ([#883]). -- BREAKING: `KeyValuePairs` (as well as `Labels`/`Annotations` via it) is now backed by a `BTreeMap` rather than a `BTreeSet` ([#888]). +- BREAKING: `KeyValuePairs` (as well as `Labels`/`Annotations` via it) is now backed by a `BTreeMap` + rather than a `BTreeSet` ([#888]). - The `Deref` impl now returns a `BTreeMap` instead. - `iter()` now clones the values. ### Fixed -- BREAKING: `KeyValuePairs::insert` (as well as `Labels::`/`Annotations::` via it) now overwrites the old value if the key already exists ([#888]). - - Previously, `iter()` would return *both* values in lexicographical order (causing further conversions like `Into` to prefer the maximum value). +- BREAKING: `KeyValuePairs::insert` (as well as `Labels::`/`Annotations::` via it) now overwrites + the old value if the key already exists. Previously, `iter()` would return *both* values in + lexicographical order (causing further conversions like `Into` to prefer the maximum + value) ([#888]). ### Removed From abf10f9e738993ca20462d19f27f21fe80dce654 Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 17 Oct 2024 08:55:07 +0200 Subject: [PATCH 17/28] chore: Add no_run to example --- crates/stackable-operator/src/utils/cluster_domain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index bf8c7c4a5..c5d8f1566 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -41,7 +41,7 @@ pub enum Error { /// /// ## Usage /// -/// ``` +/// ```no_run /// use stackable_operator::utils::KUBERNETES_CLUSTER_DOMAIN; /// /// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get() From cc570fecb92c62a222b7c412f511c91893dd0c9d Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 09:52:01 +0200 Subject: [PATCH 18/28] Update crates/stackable-operator/src/utils/cluster_domain.rs Co-authored-by: Techassi --- crates/stackable-operator/src/utils/cluster_domain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index c5d8f1566..f86a6f384 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -62,7 +62,7 @@ pub(crate) fn retrieve_cluster_domain() -> Result { match env::var(KUBERNETES_CLUSTER_DOMAIN_ENV) { Ok(cluster_domain) if !cluster_domain.is_empty() => { - tracing::info!("Using Kubernetes cluster domain: '{cluster_domain}'"); + tracing::info!(cluster_domain, "Kubernetes cluster domain set by environment variable"); return DomainName::from_str(&cluster_domain) .context(ParseDomainNameSnafu { cluster_domain }); } From 954bbbc3a65c64e11d68f82b9186b286be6920ab Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 10:01:02 +0200 Subject: [PATCH 19/28] improve tracing messages --- crates/stackable-operator/src/utils/cluster_domain.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index f86a6f384..5c878e201 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -62,12 +62,17 @@ pub(crate) fn retrieve_cluster_domain() -> Result { match env::var(KUBERNETES_CLUSTER_DOMAIN_ENV) { Ok(cluster_domain) if !cluster_domain.is_empty() => { - tracing::info!(cluster_domain, "Kubernetes cluster domain set by environment variable"); + tracing::info!( + cluster_domain, + "Kubernetes cluster domain set by environment variable" + ); return DomainName::from_str(&cluster_domain) .context(ParseDomainNameSnafu { cluster_domain }); } _ => { - tracing::info!("The env var '{KUBERNETES_CLUSTER_DOMAIN_ENV}' is not set or empty"); + tracing::info!( + "The environment variable \"{KUBERNETES_CLUSTER_DOMAIN_ENV}\" is not set or empty" + ); } }; From db8a7c94852ed99fdfcc189792e8bdb8fa13409f Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 10:07:05 +0200 Subject: [PATCH 20/28] add note --- crates/stackable-operator/src/utils/cluster_domain.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 5c878e201..0dd3cc1ee 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -123,6 +123,8 @@ where // NOTE (@Techassi): This is really sad and bothers me more than I would like to admit. This // clone could be removed by using the code directly in the calling function. But that would // remove the possibility to easily test the parsing. + // NOTE (@maltesander): This is only executed once at the start of the operator, so we can + // can live with that performance reduction :-) Ok(shortest_entry.to_owned()) } From da290fa0d28a85ad65fbd01f5c87878a05ea985d Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 10:10:01 +0200 Subject: [PATCH 21/28] replace expect with error handling --- crates/stackable-operator/src/utils/cluster_domain.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 0dd3cc1ee..1882294ac 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -2,7 +2,7 @@ use std::{env, path::Path, str::FromStr, sync::OnceLock}; use snafu::{OptionExt, ResultExt, Snafu}; -use crate::commons::networking::DomainName; +use crate::{commons::networking::DomainName, utils::cluster_domain}; const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; const KUBERNETES_SERVICE_HOST_ENV: &str = "KUBERNETES_SERVICE_HOST"; @@ -92,11 +92,10 @@ pub(crate) fn retrieve_cluster_domain() -> Result { DomainName::from_str(&cluster_domain).context(ParseDomainNameSnafu { cluster_domain }) } Err(_) => { - tracing::info!( - cluster_domain = KUBERNETES_CLUSTER_DOMAIN_DEFAULT, - "Using default Kubernetes cluster domain" - ); - Ok(DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT).expect("stuff")) + let cluster_domain = KUBERNETES_CLUSTER_DOMAIN_DEFAULT; + tracing::info!(cluster_domain, "Using default Kubernetes cluster domain"); + DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT) + .context(ParseDomainNameSnafu { cluster_domain }) } } } From 59ca78bc436c920cc97cb76829975e7b2bca7b0c Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 10:10:49 +0200 Subject: [PATCH 22/28] clippy --- crates/stackable-operator/src/utils/cluster_domain.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 1882294ac..2f7d5f047 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -2,7 +2,7 @@ use std::{env, path::Path, str::FromStr, sync::OnceLock}; use snafu::{OptionExt, ResultExt, Snafu}; -use crate::{commons::networking::DomainName, utils::cluster_domain}; +use crate::commons::networking::DomainName; const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; const KUBERNETES_SERVICE_HOST_ENV: &str = "KUBERNETES_SERVICE_HOST"; @@ -94,8 +94,7 @@ pub(crate) fn retrieve_cluster_domain() -> Result { Err(_) => { let cluster_domain = KUBERNETES_CLUSTER_DOMAIN_DEFAULT; tracing::info!(cluster_domain, "Using default Kubernetes cluster domain"); - DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT) - .context(ParseDomainNameSnafu { cluster_domain }) + DomainName::from_str(cluster_domain).context(ParseDomainNameSnafu { cluster_domain }) } } } From 23697ae639170512b34b4aca000beebb8eec9733 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 17 Oct 2024 13:43:14 +0200 Subject: [PATCH 23/28] refactor: Adjust tracing --- .../src/commons/networking.rs | 8 ++++ .../src/utils/cluster_domain.rs | 47 ++++++++++++------- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs index 19e58c2eb..18feeb074 100644 --- a/crates/stackable-operator/src/commons/networking.rs +++ b/crates/stackable-operator/src/commons/networking.rs @@ -30,6 +30,14 @@ impl TryFrom for DomainName { } } +impl TryFrom<&str> for DomainName { + type Error = validation::Errors; + + fn try_from(value: &str) -> Result { + value.parse() + } +} + impl From for String { fn from(value: DomainName) -> Self { value.0 diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 2f7d5f047..9a3ffde9c 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -1,6 +1,7 @@ use std::{env, path::Path, str::FromStr, sync::OnceLock}; use snafu::{OptionExt, ResultExt, Snafu}; +use tracing::instrument; use crate::commons::networking::DomainName; @@ -56,52 +57,64 @@ pub enum Error { /// - pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); +#[instrument] pub(crate) fn retrieve_cluster_domain() -> Result { // 1. Read KUBERNETES_CLUSTER_DOMAIN env var - tracing::info!("Trying to determine the Kubernetes cluster domain..."); + tracing::debug!("Trying to determine the Kubernetes cluster domain..."); match env::var(KUBERNETES_CLUSTER_DOMAIN_ENV) { Ok(cluster_domain) if !cluster_domain.is_empty() => { + let cluster_domain = DomainName::from_str(&cluster_domain) + .context(ParseDomainNameSnafu { cluster_domain })?; tracing::info!( - cluster_domain, - "Kubernetes cluster domain set by environment variable" - ); - return DomainName::from_str(&cluster_domain) - .context(ParseDomainNameSnafu { cluster_domain }); - } - _ => { - tracing::info!( - "The environment variable \"{KUBERNETES_CLUSTER_DOMAIN_ENV}\" is not set or empty" + %cluster_domain, + "Using Kubernetes cluster domain from {KUBERNETES_CLUSTER_DOMAIN_ENV} environment variable" ); + return Ok(cluster_domain); } + _ => {} }; // 2. If no env var is set, check if we run in a clustered (Kubernetes/Openshift) environment // by checking if KUBERNETES_SERVICE_HOST is set: If not default to 'cluster.local'. - tracing::info!("Trying to determine the operator runtime environment..."); + tracing::debug!( + "Trying to determine the operator runtime environment as environment variable \ + \"{KUBERNETES_CLUSTER_DOMAIN_ENV}\" is not set" + ); match env::var(KUBERNETES_SERVICE_HOST_ENV) { Ok(_) => { let cluster_domain = retrieve_cluster_domain_from_resolv_conf(RESOLVE_CONF_FILE_PATH)?; + let cluster_domain = DomainName::from_str(&cluster_domain) + .context(ParseDomainNameSnafu { cluster_domain })?; tracing::info!( - cluster_domain, + %cluster_domain, "Using Kubernetes cluster domain from {RESOLVE_CONF_FILE_PATH} file" ); - DomainName::from_str(&cluster_domain).context(ParseDomainNameSnafu { cluster_domain }) + Ok(cluster_domain) } Err(_) => { - let cluster_domain = KUBERNETES_CLUSTER_DOMAIN_DEFAULT; - tracing::info!(cluster_domain, "Using default Kubernetes cluster domain"); - DomainName::from_str(cluster_domain).context(ParseDomainNameSnafu { cluster_domain }) + let cluster_domain = DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT).context( + ParseDomainNameSnafu { + cluster_domain: KUBERNETES_CLUSTER_DOMAIN_DEFAULT, + }, + )?; + + tracing::info!( + %cluster_domain, + "Could not determine Kubernetes cluster domain as the operator is not running within Kubernetes, assuming default Kubernetes cluster domain" + ); + Ok(cluster_domain) } } } +#[instrument] fn retrieve_cluster_domain_from_resolv_conf

(path: P) -> Result where - P: AsRef, + P: std::fmt::Debug + AsRef, { let content = std::fs::read_to_string(path).context(ReadResolvConfFileSnafu)?; From 816b9ef2b35a355c8c1081edde38e4338acc0618 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 13:46:51 +0200 Subject: [PATCH 24/28] Apply suggestions from code review Co-authored-by: Sebastian Bernauer --- crates/stackable-operator/src/utils/cluster_domain.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 9a3ffde9c..c3719e51f 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -15,7 +15,7 @@ const RESOLVE_CONF_FILE_PATH: &str = "/etc/resolv.conf"; // fall back to defaults instead? Also trace the errors #[derive(Debug, Snafu)] pub enum Error { - #[snafu(display("failed to read resolv.conf"))] + #[snafu(display("failed to read resolv config from {RESOLVE_CONF_FILE_PATH}"))] ReadResolvConfFile { source: std::io::Error }, #[snafu(display("failed to parse {cluster_domain:?} as domain name"))] @@ -35,7 +35,7 @@ pub enum Error { /// /// 1. Return `KUBERNETES_CLUSTER_DOMAIN` if set, otherwise /// 2. Return the cluster domain parsed from the `/etc/resolv.conf` file if `KUBERNETES_SERVICE_HOST` -/// is set, otherwise fall back to `cluster.local`. cluster. +/// is set, otherwise fall back to `cluster.local`. /// /// This variable is initialized in [`crate::client::initialize_operator`], which is called in the /// main function. It can be used as suggested below. From c90cee85172c14aa57bf880e6c09c23c154d7f4c Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 17 Oct 2024 13:50:01 +0200 Subject: [PATCH 25/28] Remove re-export --- crates/stackable-operator/src/utils/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/stackable-operator/src/utils/mod.rs b/crates/stackable-operator/src/utils/mod.rs index 3564f2515..d6d0021b2 100644 --- a/crates/stackable-operator/src/utils/mod.rs +++ b/crates/stackable-operator/src/utils/mod.rs @@ -26,5 +26,3 @@ pub use self::{option::OptionExt, url::UrlExt}; pub(crate) fn format_full_controller_name(operator: &str, controller: &str) -> String { format!("{operator}_{controller}") } - -pub use self::cluster_domain::KUBERNETES_CLUSTER_DOMAIN; From d0ab02048338fcb56edb1166d7d4aaadf998a149 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Thu, 17 Oct 2024 13:57:04 +0200 Subject: [PATCH 26/28] fix docs --- crates/stackable-operator/src/utils/cluster_domain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index c3719e51f..bdfbc5fed 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -43,7 +43,7 @@ pub enum Error { /// ## Usage /// /// ```no_run -/// use stackable_operator::utils::KUBERNETES_CLUSTER_DOMAIN; +/// use stackable_operator::utils::cluster_domain::KUBERNETES_CLUSTER_DOMAIN; /// /// let kubernetes_cluster_domain = KUBERNETES_CLUSTER_DOMAIN.get() /// .expect("KUBERNETES_CLUSTER_DOMAIN must first be set by calling initialize_operator"); From c6865684dababfdb643dfbf2a5d062aa38514822 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 17 Oct 2024 15:41:51 +0200 Subject: [PATCH 27/28] chore: Apply suggestions from code review Co-authored-by: Malte Sander Co-authored-by: Techassi --- .../src/utils/cluster_domain.rs | 60 ++++++++++--------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index bdfbc5fed..353b8b004 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -1,6 +1,6 @@ -use std::{env, path::Path, str::FromStr, sync::OnceLock}; +use std::{env, path::PathBuf, str::FromStr, sync::OnceLock}; -use snafu::{OptionExt, ResultExt, Snafu}; +use snafu::{ResultExt, Snafu}; use tracing::instrument; use crate::commons::networking::DomainName; @@ -11,11 +11,9 @@ const KUBERNETES_SERVICE_HOST_ENV: &str = "KUBERNETES_SERVICE_HOST"; const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; const RESOLVE_CONF_FILE_PATH: &str = "/etc/resolv.conf"; -// TODO (@Techassi): Do we even need this many variants? Can we get rid of a bunch of variants and -// fall back to defaults instead? Also trace the errors #[derive(Debug, Snafu)] pub enum Error { - #[snafu(display("failed to read resolv config from {RESOLVE_CONF_FILE_PATH}"))] + #[snafu(display("failed to read resolv config from {RESOLVE_CONF_FILE_PATH:?}"))] ReadResolvConfFile { source: std::io::Error }, #[snafu(display("failed to parse {cluster_domain:?} as domain name"))] @@ -24,10 +22,10 @@ pub enum Error { cluster_domain: String, }, - #[snafu(display("unable to find \"search\" entry"))] + #[snafu(display(r#"unable to find "search" entry"#))] NoSearchEntry, - #[snafu(display("unable to find unambiguous domain in \"search\" entry"))] + #[snafu(display(r#"unable to find unambiguous domain in "search" entry"#))] AmbiguousDomainEntries, } @@ -90,52 +88,58 @@ pub(crate) fn retrieve_cluster_domain() -> Result { tracing::info!( %cluster_domain, - "Using Kubernetes cluster domain from {RESOLVE_CONF_FILE_PATH} file" + "Using Kubernetes cluster domain from {RESOLVE_CONF_FILE_PATH:?} file" ); Ok(cluster_domain) } Err(_) => { - let cluster_domain = DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT).context( - ParseDomainNameSnafu { - cluster_domain: KUBERNETES_CLUSTER_DOMAIN_DEFAULT, - }, - )?; + let cluster_domain = DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT) + .expect("KUBERNETES_CLUSTER_DOMAIN_DEFAULT constant must a valid domain"); tracing::info!( %cluster_domain, "Could not determine Kubernetes cluster domain as the operator is not running within Kubernetes, assuming default Kubernetes cluster domain" ); + Ok(cluster_domain) } } } #[instrument] -fn retrieve_cluster_domain_from_resolv_conf

(path: P) -> Result -where - P: std::fmt::Debug + AsRef, -{ - let content = std::fs::read_to_string(path).context(ReadResolvConfFileSnafu)?; - - let last = content +fn retrieve_cluster_domain_from_resolv_conf( + path: impl Into + std::fmt::Debug, +) -> Result { + let path = path.into(); + let content = std::fs::read_to_string(&path) + .inspect_err(|error| { + tracing::error!(%error, path = %path.display(), "Cannot read resolv conf"); + }) + .context(ReadResolvConfFileSnafu)?; + + // If there are multiple search directives, only the search + // man 5 resolv.conf + let Some(last_search_entry) = content .lines() + .rev() .map(|l| l.trim()) - .filter(|l| l.starts_with("search")) - .map(|l| l.trim_start_matches("search")) - .last() - .context(NoSearchEntrySnafu)?; + .find(|&l| l.starts_with("search")) + .map(|l| l.trim_start_matches("search").trim()) + else { + return NoSearchEntrySnafu.fail(); + }; - let shortest_entry = last + let Some(shortest_entry) = last_search_entry .split_ascii_whitespace() .min_by_key(|item| item.len()) - .context(AmbiguousDomainEntriesSnafu)?; + else { + return AmbiguousDomainEntriesSnafu.fail(); + }; // NOTE (@Techassi): This is really sad and bothers me more than I would like to admit. This // clone could be removed by using the code directly in the calling function. But that would // remove the possibility to easily test the parsing. - // NOTE (@maltesander): This is only executed once at the start of the operator, so we can - // can live with that performance reduction :-) Ok(shortest_entry.to_owned()) } From e9afa5aa7a869d098d060e41e52c937444bad786 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 17 Oct 2024 15:45:53 +0200 Subject: [PATCH 28/28] chore: Apply suggestions from code review Co-authored-by: Malte Sander Co-authored-by: Techassi --- crates/stackable-operator/src/utils/cluster_domain.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 353b8b004..4a0b9bd4d 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -66,7 +66,7 @@ pub(crate) fn retrieve_cluster_domain() -> Result { .context(ParseDomainNameSnafu { cluster_domain })?; tracing::info!( %cluster_domain, - "Using Kubernetes cluster domain from {KUBERNETES_CLUSTER_DOMAIN_ENV} environment variable" + "Using Kubernetes cluster domain from {KUBERNETES_CLUSTER_DOMAIN_ENV:?} environment variable" ); return Ok(cluster_domain); } @@ -77,7 +77,7 @@ pub(crate) fn retrieve_cluster_domain() -> Result { // by checking if KUBERNETES_SERVICE_HOST is set: If not default to 'cluster.local'. tracing::debug!( "Trying to determine the operator runtime environment as environment variable \ - \"{KUBERNETES_CLUSTER_DOMAIN_ENV}\" is not set" + {KUBERNETES_CLUSTER_DOMAIN_ENV:?} is not set" ); match env::var(KUBERNETES_SERVICE_HOST_ENV) {