From b08ed8e8f061079a07f3f4af929e8a0222d2fb3e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 11:48:42 +0200 Subject: [PATCH 01/12] WIP spike: Store cluster domain in Client --- .../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 | 27 +-- .../src/utils/cluster_domain.rs | 155 +++--------------- 6 files changed, 38 insertions(+), 156 deletions(-) delete mode 100644 crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf delete mode 100644 crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes-multiple.resolv.conf delete mode 100644 crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes.resolv.conf delete mode 100644 crates/stackable-operator/fixtures/cluster_domain/pass/openshift.resolv.conf diff --git a/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf b/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf deleted file mode 100644 index f16e34dc2..000000000 --- a/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf +++ /dev/null @@ -1,2 +0,0 @@ -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 deleted file mode 100644 index 578cb8e4f..000000000 --- a/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes-multiple.resolv.conf +++ /dev/null @@ -1,4 +0,0 @@ -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 deleted file mode 100644 index 9c210febf..000000000 --- a/crates/stackable-operator/fixtures/cluster_domain/pass/kubernetes.resolv.conf +++ /dev/null @@ -1,3 +0,0 @@ -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 deleted file mode 100644 index 305b7f795..000000000 --- a/crates/stackable-operator/fixtures/cluster_domain/pass/openshift.resolv.conf +++ /dev/null @@ -1,3 +0,0 @@ -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 fec3e3501..f488bb82c 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -1,5 +1,6 @@ +use crate::commons::networking::DomainName; use crate::kvp::LabelSelectorExt; -use crate::utils::cluster_domain::{self, retrieve_cluster_domain, KUBERNETES_CLUSTER_DOMAIN}; +use crate::utils::cluster_domain::{self, retrieve_cluster_domain}; use either::Either; use futures::StreamExt; @@ -93,6 +94,7 @@ pub struct Client { delete_params: DeleteParams, /// Default namespace as defined in the kubeconfig this client has been created from. pub default_namespace: String, + pub kubernetes_cluster_domain: DomainName, } impl Client { @@ -100,6 +102,7 @@ impl Client { client: KubeClient, field_manager: Option, default_namespace: String, + kubernetes_cluster_domain: DomainName, ) -> Self { Client { client, @@ -113,6 +116,7 @@ impl Client { }, delete_params: DeleteParams::default(), default_namespace, + kubernetes_cluster_domain, } } @@ -627,19 +631,20 @@ where } pub async fn initialize_operator(field_manager: Option) -> Result { - let _ = KUBERNETES_CLUSTER_DOMAIN - .set(retrieve_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) .context(InferKubeConfigSnafu)?; let default_namespace = kubeconfig.default_namespace.clone(); let client = kube::Client::try_from(kubeconfig).context(CreateKubeClientSnafu)?; - Ok(Client::new(client, field_manager, default_namespace)) + let cluster_domain = retrieve_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?; + + Ok(Client::new( + client, + field_manager, + default_namespace, + cluster_domain, + )) } #[cfg(test)] @@ -657,7 +662,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created() { - let client = super::create_client(None) + let client = super::initialize_operator(None) .await .expect("KUBECONFIG variable must be configured."); @@ -735,7 +740,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created_timeout() { - let client = super::create_client(None) + let client = super::initialize_operator(None) .await .expect("KUBECONFIG variable must be configured."); @@ -755,7 +760,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_list_with_label_selector() { - let client = super::create_client(None) + let client = super::initialize_operator(None) .await .expect("KUBECONFIG variable must be configured."); diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs index 4a0b9bd4d..ce4eac696 100644 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ b/crates/stackable-operator/src/utils/cluster_domain.rs @@ -1,4 +1,4 @@ -use std::{env, path::PathBuf, str::FromStr, sync::OnceLock}; +use std::{env, str::FromStr}; use snafu::{ResultExt, Snafu}; use tracing::instrument; @@ -6,61 +6,26 @@ use tracing::instrument; use crate::commons::networking::DomainName; const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; -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"; #[derive(Debug, Snafu)] pub enum Error { - #[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"))] ParseDomainName { source: crate::validation::Errors, cluster_domain: String, }, - - #[snafu(display(r#"unable to find "search" entry"#))] - NoSearchEntry, - - #[snafu(display(r#"unable to find unambiguous domain in "search" entry"#))] - AmbiguousDomainEntries, } /// Tries to retrieve the Kubernetes cluster domain. /// -/// 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`. -/// -/// 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::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"); -/// -/// tracing::info!(%kubernetes_cluster_domain, "Found cluster domain"); -/// ``` -/// -/// ## See -/// -/// - -/// - -pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); - +/// Return `KUBERNETES_CLUSTER_DOMAIN` if set, otherwise default to +/// [`KUBERNETES_CLUSTER_DOMAIN_DEFAULT`]. #[instrument] pub(crate) fn retrieve_cluster_domain() -> Result { - // 1. Read KUBERNETES_CLUSTER_DOMAIN env var tracing::debug!("Trying to determine the Kubernetes cluster domain..."); - match env::var(KUBERNETES_CLUSTER_DOMAIN_ENV) { + Ok(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 })?; @@ -68,120 +33,44 @@ pub(crate) fn retrieve_cluster_domain() -> Result { %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::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, - "Using Kubernetes cluster domain from {RESOLVE_CONF_FILE_PATH:?} file" - ); - - Ok(cluster_domain) + cluster_domain } - Err(_) => { + _ => { 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" + "Using default Kubernetes cluster domain as {KUBERNETES_CLUSTER_DOMAIN_ENV:?} environment variable is not set" ); - - Ok(cluster_domain) + cluster_domain } - } -} - -#[instrument] -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()) - .find(|&l| l.starts_with("search")) - .map(|l| l.trim_start_matches("search").trim()) - else { - return NoSearchEntrySnafu.fail(); - }; - - let Some(shortest_entry) = last_search_entry - .split_ascii_whitespace() - .min_by_key(|item| item.len()) - 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. - Ok(shortest_entry.to_owned()) + }) } #[cfg(test)] mod tests { - use std::path::PathBuf; - use super::*; - use rstest::rstest; + + #[test] + fn default_kubernetes_cluster_domain_value() { + assert_eq!( + retrieve_cluster_domain().unwrap().to_string(), + "cluster.local" + ); + } #[test] fn use_different_kubernetes_cluster_domain_value() { - let cluster_domain = "my-cluster.local".to_string(); + let cluster_domain = "my-cluster.local"; - // set different domain via env var + // Set custom cluster domain via env var unsafe { - env::set_var(KUBERNETES_CLUSTER_DOMAIN_ENV, &cluster_domain); + env::set_var(KUBERNETES_CLUSTER_DOMAIN_ENV, cluster_domain); } - // initialize the lock - let _ = KUBERNETES_CLUSTER_DOMAIN.set(retrieve_cluster_domain().unwrap()); - assert_eq!( - cluster_domain, - KUBERNETES_CLUSTER_DOMAIN.get().unwrap().to_string() + retrieve_cluster_domain().unwrap().to_string(), + cluster_domain ); } - - #[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 - ); - } - - #[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 62b1bad939d5736444d8ec1ca9596308b2f0b7c7 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 13:51:26 +0200 Subject: [PATCH 02/12] Store cluster domain in separate KubernetesClusterInfo struct --- crates/stackable-operator/src/cli.rs | 16 +++- crates/stackable-operator/src/client.rs | 89 +++++++++++-------- .../src/utils/cluster_domain.rs | 76 ---------------- .../src/utils/cluster_info.rs | 34 +++++++ crates/stackable-operator/src/utils/mod.rs | 2 +- 5 files changed, 102 insertions(+), 115 deletions(-) delete mode 100644 crates/stackable-operator/src/utils/cluster_domain.rs create mode 100644 crates/stackable-operator/src/utils/cluster_info.rs diff --git a/crates/stackable-operator/src/cli.rs b/crates/stackable-operator/src/cli.rs index 022502564..b4ca3ad55 100644 --- a/crates/stackable-operator/src/cli.rs +++ b/crates/stackable-operator/src/cli.rs @@ -115,7 +115,7 @@ use clap::Args; use product_config::ProductConfigManager; use snafu::{ResultExt, Snafu}; -use crate::{logging::TracingTarget, namespace::WatchNamespace}; +use crate::{commons::networking::DomainName, logging::TracingTarget, namespace::WatchNamespace}; pub const AUTHOR: &str = "Stackable GmbH - info@stackable.tech"; @@ -179,7 +179,8 @@ pub enum Command { /// common: ProductOperatorRun { /// product_config: ProductConfigPath::from("bar".as_ref()), /// watch_namespace: WatchNamespace::One("foobar".to_string()), -/// tracing_target: TracingTarget::None +/// tracing_target: TracingTarget::None, +/// kubernetes_cluster_domain: None, /// }, /// })); /// ``` @@ -205,12 +206,20 @@ pub struct ProductOperatorRun { /// Provides the path to a product-config file #[arg(long, short = 'p', value_name = "FILE", default_value = "", env)] pub product_config: ProductConfigPath, + /// Provides a specific namespace to watch (instead of watching all namespaces) #[arg(long, env, default_value = "")] pub watch_namespace: WatchNamespace, + /// Tracing log collector system #[arg(long, env, default_value_t, value_enum)] pub tracing_target: TracingTarget, + + /// Kubernetes cluster domain, usually this is `cluster.local`. + // We are not using a default value here, as operators will probably do an more advanced + // auto-detection of the cluster domain in case it is not specified in the future. + #[arg(long, env)] + pub kubernetes_cluster_domain: Option, } /// A path to a [`ProductConfigManager`] spec file @@ -384,6 +393,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, + kubernetes_cluster_domain: None, } ); @@ -395,6 +405,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::All, tracing_target: TracingTarget::None, + kubernetes_cluster_domain: None, } ); @@ -407,6 +418,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, + kubernetes_cluster_domain: None, } ); } diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index f488bb82c..5e7fd4ff8 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -1,24 +1,28 @@ -use crate::commons::networking::DomainName; -use crate::kvp::LabelSelectorExt; -use crate::utils::cluster_domain::{self, retrieve_cluster_domain}; +use std::{ + convert::TryFrom, + fmt::{Debug, Display}, +}; use either::Either; use futures::StreamExt; -use k8s_openapi::apimachinery::pkg::apis::meta::v1::LabelSelector; -use k8s_openapi::{ClusterResourceScope, NamespaceResourceScope}; -use kube::api::{DeleteParams, ListParams, Patch, PatchParams, PostParams, Resource, ResourceExt}; -use kube::client::Client as KubeClient; -use kube::core::Status; -use kube::runtime::wait::delete::delete_and_finalize; -use kube::runtime::{watcher, WatchStreamExt}; -use kube::{Api, Config}; -use serde::de::DeserializeOwned; -use serde::Serialize; +use k8s_openapi::{ + apimachinery::pkg::apis::meta::v1::LabelSelector, ClusterResourceScope, NamespaceResourceScope, +}; +use kube::{ + api::{DeleteParams, ListParams, Patch, PatchParams, PostParams, Resource, ResourceExt}, + client::Client as KubeClient, + core::Status, + runtime::{wait::delete::delete_and_finalize, watcher, WatchStreamExt}, + Api, Config, +}; +use serde::{de::DeserializeOwned, Serialize}; use snafu::{OptionExt, ResultExt, Snafu}; -use std::convert::TryFrom; -use std::fmt::{Debug, Display}; use tracing::trace; +use crate::{ + cli::ProductOperatorRun, kvp::LabelSelectorExt, utils::cluster_info::KubernetesClusterInfo, +}; + pub type Result = std::result::Result; #[derive(Debug, Snafu)] @@ -79,9 +83,6 @@ 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. @@ -94,7 +95,8 @@ pub struct Client { delete_params: DeleteParams, /// Default namespace as defined in the kubeconfig this client has been created from. pub default_namespace: String, - pub kubernetes_cluster_domain: DomainName, + + pub kubernetes_cluster_info: KubernetesClusterInfo, } impl Client { @@ -102,7 +104,7 @@ impl Client { client: KubeClient, field_manager: Option, default_namespace: String, - kubernetes_cluster_domain: DomainName, + kubernetes_cluster_info: KubernetesClusterInfo, ) -> Self { Client { client, @@ -116,7 +118,7 @@ impl Client { }, delete_params: DeleteParams::default(), default_namespace, - kubernetes_cluster_domain, + kubernetes_cluster_info, } } @@ -515,15 +517,18 @@ impl Client { /// /// ```no_run /// use std::time::Duration; + /// use clap::Parser; /// use tokio::time::error::Elapsed; /// use kube::runtime::watcher; /// use k8s_openapi::api::core::v1::Pod; - /// use stackable_operator::client::{Client, initialize_operator}; + /// use stackable_operator::{cli::ProductOperatorRun, client::{Client, initialize_operator}}; /// /// #[tokio::main] /// async fn main(){ /// - /// let client: Client = initialize_operator(None).await.expect("Unable to construct client."); + /// // Parse CLI arguments with Opts::parse() instead + /// let cli_opts = ProductOperatorRun::parse_from(["run"]); + /// let client: Client = initialize_operator(&cli_opts, None).await.expect("Unable to construct client."); /// let watcher_config: watcher::Config = /// watcher::Config::default().fields(&format!("metadata.name=nonexistent-pod")); /// @@ -630,39 +635,49 @@ where } } -pub async fn initialize_operator(field_manager: Option) -> Result { +pub async fn initialize_operator( + cli_opts: &ProductOperatorRun, + field_manager: Option, +) -> Result { let kubeconfig: Config = kube::Config::infer() .await .map_err(kube::Error::InferConfig) .context(InferKubeConfigSnafu)?; let default_namespace = kubeconfig.default_namespace.clone(); let client = kube::Client::try_from(kubeconfig).context(CreateKubeClientSnafu)?; - let cluster_domain = retrieve_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?; + let cluster_info = KubernetesClusterInfo::new(cli_opts); Ok(Client::new( client, field_manager, default_namespace, - cluster_domain, + cluster_info, )) } #[cfg(test)] mod tests { + use std::{collections::BTreeMap, time::Duration}; + + use clap::Parser; use futures::StreamExt; - use k8s_openapi::api::core::v1::{Container, Pod, PodSpec}; - use k8s_openapi::apimachinery::pkg::apis::meta::v1::LabelSelector; - use kube::api::{ObjectMeta, PostParams, ResourceExt}; - use kube::runtime::watcher; - use kube::runtime::watcher::Event; - use std::collections::BTreeMap; - use std::time::Duration; + use k8s_openapi::{ + api::core::v1::{Container, Pod, PodSpec}, + apimachinery::pkg::apis::meta::v1::LabelSelector, + }; + use kube::{ + api::{ObjectMeta, PostParams, ResourceExt}, + runtime::watcher::{self, Event}, + }; use tokio::time::error::Elapsed; + use crate::cli::ProductOperatorRun; + #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created() { - let client = super::initialize_operator(None) + let cli_opts = ProductOperatorRun::parse_from(["run"]); + let client = super::initialize_operator(&cli_opts, None) .await .expect("KUBECONFIG variable must be configured."); @@ -740,7 +755,8 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created_timeout() { - let client = super::initialize_operator(None) + let cli_opts = ProductOperatorRun::parse_from(["run"]); + let client = super::initialize_operator(&cli_opts, None) .await .expect("KUBECONFIG variable must be configured."); @@ -760,7 +776,8 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_list_with_label_selector() { - let client = super::initialize_operator(None) + let cli_opts = ProductOperatorRun::parse_from(["run"]); + let client = super::initialize_operator(&cli_opts, None) .await .expect("KUBECONFIG variable must be configured."); diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs deleted file mode 100644 index ce4eac696..000000000 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ /dev/null @@ -1,76 +0,0 @@ -use std::{env, str::FromStr}; - -use snafu::{ResultExt, Snafu}; -use tracing::instrument; - -use crate::commons::networking::DomainName; - -const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; -const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; - -#[derive(Debug, Snafu)] -pub enum Error { - #[snafu(display("failed to parse {cluster_domain:?} as domain name"))] - ParseDomainName { - source: crate::validation::Errors, - cluster_domain: String, - }, -} - -/// Tries to retrieve the Kubernetes cluster domain. -/// -/// Return `KUBERNETES_CLUSTER_DOMAIN` if set, otherwise default to -/// [`KUBERNETES_CLUSTER_DOMAIN_DEFAULT`]. -#[instrument] -pub(crate) fn retrieve_cluster_domain() -> Result { - tracing::debug!("Trying to determine the Kubernetes cluster domain..."); - - Ok(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, - "Using Kubernetes cluster domain from {KUBERNETES_CLUSTER_DOMAIN_ENV:?} environment variable" - ); - cluster_domain - } - _ => { - let cluster_domain = DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT) - .expect("KUBERNETES_CLUSTER_DOMAIN_DEFAULT constant must a valid domain"); - tracing::info!( - %cluster_domain, - "Using default Kubernetes cluster domain as {KUBERNETES_CLUSTER_DOMAIN_ENV:?} environment variable is not set" - ); - cluster_domain - } - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn default_kubernetes_cluster_domain_value() { - assert_eq!( - retrieve_cluster_domain().unwrap().to_string(), - "cluster.local" - ); - } - - #[test] - fn use_different_kubernetes_cluster_domain_value() { - let cluster_domain = "my-cluster.local"; - - // Set custom cluster domain via env var - unsafe { - env::set_var(KUBERNETES_CLUSTER_DOMAIN_ENV, cluster_domain); - } - - assert_eq!( - retrieve_cluster_domain().unwrap().to_string(), - cluster_domain - ); - } -} diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs new file mode 100644 index 000000000..fda21c05d --- /dev/null +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -0,0 +1,34 @@ +use std::str::FromStr; + +use crate::{cli::ProductOperatorRun, commons::networking::DomainName}; + +const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; + +#[derive(Debug, Clone)] +pub struct KubernetesClusterInfo { + pub cluster_domain: DomainName, +} + +impl KubernetesClusterInfo { + pub fn new(cli_opts: &ProductOperatorRun) -> Self { + let cluster_domain = match &cli_opts.kubernetes_cluster_domain { + Some(cluster_domain) => { + tracing::info!(%cluster_domain, "Using configured Kubernetes cluster domain"); + + cluster_domain.clone() + } + None => { + // TODO(sbernauer): Do some sort of advanced auto-detection, see https://github.com/stackabletech/issues/issues/436. + // There have been attempts of parsing the `/etc/resolv.conf`, but they have been + // reverted. Please read on the linked Issue for details. + let cluster_domain = DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT) + .expect("KUBERNETES_CLUSTER_DOMAIN_DEFAULT constant must a valid domain"); + tracing::info!(%cluster_domain, "Defaulting Kubernetes cluster domain as it has not been configured"); + + cluster_domain + } + }; + + Self { cluster_domain } + } +} diff --git a/crates/stackable-operator/src/utils/mod.rs b/crates/stackable-operator/src/utils/mod.rs index d6d0021b2..e400072d5 100644 --- a/crates/stackable-operator/src/utils/mod.rs +++ b/crates/stackable-operator/src/utils/mod.rs @@ -1,5 +1,5 @@ pub mod bash; -pub mod cluster_domain; +pub mod cluster_info; pub mod crds; pub mod logging; mod option; From da4df864f7a223e0a184e8133e9e24c59dfe1c77 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 13:56:32 +0200 Subject: [PATCH 03/12] Remove src/utils/cluster_domain.rs --- .../src/utils/cluster_domain.rs | 190 ------------------ 1 file changed, 190 deletions(-) delete mode 100644 crates/stackable-operator/src/utils/cluster_domain.rs diff --git a/crates/stackable-operator/src/utils/cluster_domain.rs b/crates/stackable-operator/src/utils/cluster_domain.rs deleted file mode 100644 index c690369f6..000000000 --- a/crates/stackable-operator/src/utils/cluster_domain.rs +++ /dev/null @@ -1,190 +0,0 @@ -use std::{env, path::PathBuf, str::FromStr, sync::OnceLock}; - -use snafu::{OptionExt, ResultExt, Snafu}; -use tracing::instrument; - -use crate::commons::networking::DomainName; - -const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; -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"; - -#[derive(Debug, Snafu)] -pub enum Error { - #[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"))] - ParseDomainName { - source: crate::validation::Errors, - cluster_domain: String, - }, - - #[snafu(display(r#"unable to find "search" entry"#))] - NoSearchEntry, - - #[snafu(display( - r#"unable to find the Kubernetes service domain, which needs to start with "svc.""# - ))] - FindKubernetesServiceDomain, -} - -/// Tries to retrieve the Kubernetes cluster domain. -/// -/// 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`. -/// -/// 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::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"); -/// -/// tracing::info!(%kubernetes_cluster_domain, "Found cluster domain"); -/// ``` -/// -/// ## See -/// -/// - -/// - -pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock = OnceLock::new(); - -#[instrument] -pub(crate) fn retrieve_cluster_domain() -> Result { - // 1. Read KUBERNETES_CLUSTER_DOMAIN env var - 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, - "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::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, - "Using Kubernetes cluster domain from {RESOLVE_CONF_FILE_PATH:?} file" - ); - - Ok(cluster_domain) - } - Err(_) => { - 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: 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 last search directive is relevant. - // See `man 5 resolv.conf` - let last_search_entry = content - .lines() - .rev() - .map(|entry| entry.trim()) - .find(|&entry| entry.starts_with("search")) - .map(|entry| entry.trim_start_matches("search").trim()) - .context(NoSearchEntrySnafu)?; - - // We only care about entries starting with "svc." to limit the entries to the ones used by - // Kubernetes for Services. - let shortest_entry = last_search_entry - .split_ascii_whitespace() - // Normally there should only be one such entry, but we take the first on in any case. - .find(|&entry| entry.starts_with("svc.")) - // Strip the "svc." prefix to get only the cluster domain. - .map(|entry| entry.trim_start_matches("svc.").trim_end()) - .context(FindKubernetesServiceDomainSnafu)?; - - // 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()) -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use super::*; - use rstest::rstest; - - #[test] - 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, &cluster_domain); - } - - // initialize the lock - let _ = KUBERNETES_CLUSTER_DOMAIN.set(retrieve_cluster_domain().unwrap()); - - assert_eq!( - cluster_domain, - KUBERNETES_CLUSTER_DOMAIN.get().unwrap().to_string() - ); - } - - #[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 - ); - } - - #[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 4869e79fbc19b315681a75f872cfca01b1ced6b6 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 14:10:27 +0200 Subject: [PATCH 04/12] Dont pass all CLI args --- crates/stackable-operator/src/client.rs | 26 +++++++------------ .../src/utils/cluster_info.rs | 6 ++--- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index 5e7fd4ff8..3c5e66e15 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -20,7 +20,8 @@ use snafu::{OptionExt, ResultExt, Snafu}; use tracing::trace; use crate::{ - cli::ProductOperatorRun, kvp::LabelSelectorExt, utils::cluster_info::KubernetesClusterInfo, + commons::networking::DomainName, kvp::LabelSelectorExt, + utils::cluster_info::KubernetesClusterInfo, }; pub type Result = std::result::Result; @@ -517,18 +518,15 @@ impl Client { /// /// ```no_run /// use std::time::Duration; - /// use clap::Parser; /// use tokio::time::error::Elapsed; /// use kube::runtime::watcher; /// use k8s_openapi::api::core::v1::Pod; - /// use stackable_operator::{cli::ProductOperatorRun, client::{Client, initialize_operator}}; + /// use stackable_operator::client::{Client, initialize_operator}; /// /// #[tokio::main] /// async fn main(){ /// - /// // Parse CLI arguments with Opts::parse() instead - /// let cli_opts = ProductOperatorRun::parse_from(["run"]); - /// let client: Client = initialize_operator(&cli_opts, None).await.expect("Unable to construct client."); + /// let client: Client = initialize_operator(&None, None).await.expect("Unable to construct client."); /// let watcher_config: watcher::Config = /// watcher::Config::default().fields(&format!("metadata.name=nonexistent-pod")); /// @@ -636,7 +634,7 @@ where } pub async fn initialize_operator( - cli_opts: &ProductOperatorRun, + cli_kubernetes_cluster_domain: &Option, field_manager: Option, ) -> Result { let kubeconfig: Config = kube::Config::infer() @@ -645,7 +643,7 @@ pub async fn initialize_operator( .context(InferKubeConfigSnafu)?; let default_namespace = kubeconfig.default_namespace.clone(); let client = kube::Client::try_from(kubeconfig).context(CreateKubeClientSnafu)?; - let cluster_info = KubernetesClusterInfo::new(cli_opts); + let cluster_info = KubernetesClusterInfo::new(cli_kubernetes_cluster_domain); Ok(Client::new( client, @@ -659,7 +657,6 @@ pub async fn initialize_operator( mod tests { use std::{collections::BTreeMap, time::Duration}; - use clap::Parser; use futures::StreamExt; use k8s_openapi::{ api::core::v1::{Container, Pod, PodSpec}, @@ -671,13 +668,10 @@ mod tests { }; use tokio::time::error::Elapsed; - use crate::cli::ProductOperatorRun; - #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created() { - let cli_opts = ProductOperatorRun::parse_from(["run"]); - let client = super::initialize_operator(&cli_opts, None) + let client = super::initialize_operator(&None, None) .await .expect("KUBECONFIG variable must be configured."); @@ -755,8 +749,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created_timeout() { - let cli_opts = ProductOperatorRun::parse_from(["run"]); - let client = super::initialize_operator(&cli_opts, None) + let client = super::initialize_operator(&None, None) .await .expect("KUBECONFIG variable must be configured."); @@ -776,8 +769,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_list_with_label_selector() { - let cli_opts = ProductOperatorRun::parse_from(["run"]); - let client = super::initialize_operator(&cli_opts, None) + let client = super::initialize_operator(&None, None) .await .expect("KUBECONFIG variable must be configured."); diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index fda21c05d..98095c4fe 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -1,6 +1,6 @@ use std::str::FromStr; -use crate::{cli::ProductOperatorRun, commons::networking::DomainName}; +use crate::commons::networking::DomainName; const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; @@ -10,8 +10,8 @@ pub struct KubernetesClusterInfo { } impl KubernetesClusterInfo { - pub fn new(cli_opts: &ProductOperatorRun) -> Self { - let cluster_domain = match &cli_opts.kubernetes_cluster_domain { + pub fn new(cli_kubernetes_cluster_domain: &Option) -> Self { + let cluster_domain = match cli_kubernetes_cluster_domain { Some(cluster_domain) => { tracing::info!(%cluster_domain, "Using configured Kubernetes cluster domain"); From 574e03076cd10735767546621cc831667123a7f3 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 14:15:44 +0200 Subject: [PATCH 05/12] changelog --- crates/stackable-operator/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index f9ff6cc81..1104bc379 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,12 +4,19 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- Don't parse `/etc/resolv.conf` to auto-detect the Kubernetes cluster domain in case it is not explicitly configured. + Instead the operator will default to `cluster.local`. We revert this now after some concerns where raised, we will + create a follow-up decision instead addressing how we will continue with this ([#896]). + ### Fixed - Fix Kubernetes cluster domain parsing from resolv.conf, e.g. on AWS EKS. We now only consider Kubernetes services domains instead of all domains (which could include non-Kubernetes domains) ([#895]). [#895]: https://github.com/stackabletech/operator-rs/pull/895 +[#896]: https://github.com/stackabletech/operator-rs/pull/896 ## [0.79.0] - 2024-10-18 From d6db64f5242fd38590b202f8b8898c422e994b94 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 14:21:12 +0200 Subject: [PATCH 06/12] typo --- crates/stackable-operator/src/utils/cluster_info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index 98095c4fe..9fa5c675d 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -20,7 +20,7 @@ impl KubernetesClusterInfo { None => { // TODO(sbernauer): Do some sort of advanced auto-detection, see https://github.com/stackabletech/issues/issues/436. // There have been attempts of parsing the `/etc/resolv.conf`, but they have been - // reverted. Please read on the linked Issue for details. + // reverted. Please read on the linked issue for details. let cluster_domain = DomainName::from_str(KUBERNETES_CLUSTER_DOMAIN_DEFAULT) .expect("KUBERNETES_CLUSTER_DOMAIN_DEFAULT constant must a valid domain"); tracing::info!(%cluster_domain, "Defaulting Kubernetes cluster domain as it has not been configured"); From ce51bc5940cd63bc4047afc3955b73a80030156a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 14:58:11 +0200 Subject: [PATCH 07/12] Move CLI arg into KubernetesClusterInfoCliOpts struct --- crates/stackable-operator/src/cli.rs | 36 +++++++++++------ crates/stackable-operator/src/client.rs | 39 ++++++++++++++----- .../src/utils/cluster_info.rs | 13 ++++++- 3 files changed, 64 insertions(+), 24 deletions(-) diff --git a/crates/stackable-operator/src/cli.rs b/crates/stackable-operator/src/cli.rs index b4ca3ad55..1a5b7e6ba 100644 --- a/crates/stackable-operator/src/cli.rs +++ b/crates/stackable-operator/src/cli.rs @@ -115,7 +115,10 @@ use clap::Args; use product_config::ProductConfigManager; use snafu::{ResultExt, Snafu}; -use crate::{commons::networking::DomainName, logging::TracingTarget, namespace::WatchNamespace}; +use crate::{ + logging::TracingTarget, namespace::WatchNamespace, + utils::cluster_info::KubernetesClusterInfoCliOpts, +}; pub const AUTHOR: &str = "Stackable GmbH - info@stackable.tech"; @@ -171,8 +174,12 @@ pub enum Command { /// common: ProductOperatorRun, /// } /// use clap::Parser; -/// use stackable_operator::logging::TracingTarget; -/// use stackable_operator::namespace::WatchNamespace; +/// use stackable_operator::{ +/// logging::TracingTarget, +/// namespace::WatchNamespace, +/// utils::cluster_info::KubernetesClusterInfoCliOpts +/// }; +/// /// let opts = Command::::parse_from(["foobar-operator", "run", "--name", "foo", "--product-config", "bar", "--watch-namespace", "foobar"]); /// assert_eq!(opts, Command::Run(Run { /// name: "foo".to_string(), @@ -180,7 +187,9 @@ pub enum Command { /// product_config: ProductConfigPath::from("bar".as_ref()), /// watch_namespace: WatchNamespace::One("foobar".to_string()), /// tracing_target: TracingTarget::None, -/// kubernetes_cluster_domain: None, +/// cluster_info_opts: KubernetesClusterInfoCliOpts { +/// kubernetes_cluster_domain: None +/// } /// }, /// })); /// ``` @@ -215,11 +224,8 @@ pub struct ProductOperatorRun { #[arg(long, env, default_value_t, value_enum)] pub tracing_target: TracingTarget, - /// Kubernetes cluster domain, usually this is `cluster.local`. - // We are not using a default value here, as operators will probably do an more advanced - // auto-detection of the cluster domain in case it is not specified in the future. - #[arg(long, env)] - pub kubernetes_cluster_domain: Option, + #[command(flatten)] + pub cluster_info_opts: KubernetesClusterInfoCliOpts, } /// A path to a [`ProductConfigManager`] spec file @@ -393,7 +399,9 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, - kubernetes_cluster_domain: None, + cluster_info_opts: KubernetesClusterInfoCliOpts { + kubernetes_cluster_domain: None + } } ); @@ -405,7 +413,9 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::All, tracing_target: TracingTarget::None, - kubernetes_cluster_domain: None, + cluster_info_opts: KubernetesClusterInfoCliOpts { + kubernetes_cluster_domain: None + } } ); @@ -418,7 +428,9 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, - kubernetes_cluster_domain: None, + cluster_info_opts: KubernetesClusterInfoCliOpts { + kubernetes_cluster_domain: None + } } ); } diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index 3c5e66e15..7ccbf5d02 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -20,8 +20,8 @@ use snafu::{OptionExt, ResultExt, Snafu}; use tracing::trace; use crate::{ - commons::networking::DomainName, kvp::LabelSelectorExt, - utils::cluster_info::KubernetesClusterInfo, + kvp::LabelSelectorExt, + utils::cluster_info::{KubernetesClusterInfo, KubernetesClusterInfoCliOpts}, }; pub type Result = std::result::Result; @@ -521,12 +521,20 @@ impl Client { /// use tokio::time::error::Elapsed; /// use kube::runtime::watcher; /// use k8s_openapi::api::core::v1::Pod; - /// use stackable_operator::client::{Client, initialize_operator}; + /// use stackable_operator::{ + /// client::{Client, initialize_operator}, + /// utils::cluster_info::KubernetesClusterInfoCliOpts + /// }; /// /// #[tokio::main] - /// async fn main(){ + /// async fn main() { /// - /// let client: Client = initialize_operator(&None, None).await.expect("Unable to construct client."); + /// let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + /// kubernetes_cluster_domain: None, + /// }; + /// let client = initialize_operator(None, &cluster_info_cli_opts) + /// .await + /// .expect("Unable to construct client."); /// let watcher_config: watcher::Config = /// watcher::Config::default().fields(&format!("metadata.name=nonexistent-pod")); /// @@ -634,8 +642,8 @@ where } pub async fn initialize_operator( - cli_kubernetes_cluster_domain: &Option, field_manager: Option, + cluster_info_cli_opts: &KubernetesClusterInfoCliOpts, ) -> Result { let kubeconfig: Config = kube::Config::infer() .await @@ -643,7 +651,7 @@ pub async fn initialize_operator( .context(InferKubeConfigSnafu)?; let default_namespace = kubeconfig.default_namespace.clone(); let client = kube::Client::try_from(kubeconfig).context(CreateKubeClientSnafu)?; - let cluster_info = KubernetesClusterInfo::new(cli_kubernetes_cluster_domain); + let cluster_info = KubernetesClusterInfo::new(cluster_info_cli_opts); Ok(Client::new( client, @@ -668,10 +676,15 @@ mod tests { }; use tokio::time::error::Elapsed; + use crate::utils::cluster_info::KubernetesClusterInfoCliOpts; + #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created() { - let client = super::initialize_operator(&None, None) + let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + kubernetes_cluster_domain: None, + }; + let client = super::initialize_operator(None, &cluster_info_cli_opts) .await .expect("KUBECONFIG variable must be configured."); @@ -749,7 +762,10 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created_timeout() { - let client = super::initialize_operator(&None, None) + let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + kubernetes_cluster_domain: None, + }; + let client = super::initialize_operator(None, &cluster_info_cli_opts) .await .expect("KUBECONFIG variable must be configured."); @@ -769,7 +785,10 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_list_with_label_selector() { - let client = super::initialize_operator(&None, None) + let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + kubernetes_cluster_domain: None, + }; + let client = super::initialize_operator(None, &cluster_info_cli_opts) .await .expect("KUBECONFIG variable must be configured."); diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index 9fa5c675d..a011bef57 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -9,9 +9,18 @@ pub struct KubernetesClusterInfo { pub cluster_domain: DomainName, } +#[derive(clap::Parser, Debug, PartialEq, Eq)] +pub struct KubernetesClusterInfoCliOpts { + /// Kubernetes cluster domain, usually this is `cluster.local`. + // We are not using a default value here, as operators will probably do an more advanced + // auto-detection of the cluster domain in case it is not specified in the future. + #[arg(long, env)] + pub kubernetes_cluster_domain: Option, +} + impl KubernetesClusterInfo { - pub fn new(cli_kubernetes_cluster_domain: &Option) -> Self { - let cluster_domain = match cli_kubernetes_cluster_domain { + pub fn new(cluster_info_cli_opts: &KubernetesClusterInfoCliOpts) -> Self { + let cluster_domain = match &cluster_info_cli_opts.kubernetes_cluster_domain { Some(cluster_domain) => { tracing::info!(%cluster_domain, "Using configured Kubernetes cluster domain"); From c0b365077fe2e6c138b45051b93f02364dc9ebaa Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 15:18:13 +0200 Subject: [PATCH 08/12] KubernetesClusterInfoCliOpts -> KubernetesClusterInfoOpts --- crates/stackable-operator/src/cli.rs | 14 +++++++------- crates/stackable-operator/src/client.rs | 16 ++++++++-------- .../stackable-operator/src/utils/cluster_info.rs | 4 ++-- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/crates/stackable-operator/src/cli.rs b/crates/stackable-operator/src/cli.rs index 1a5b7e6ba..a51d96b0c 100644 --- a/crates/stackable-operator/src/cli.rs +++ b/crates/stackable-operator/src/cli.rs @@ -117,7 +117,7 @@ use snafu::{ResultExt, Snafu}; use crate::{ logging::TracingTarget, namespace::WatchNamespace, - utils::cluster_info::KubernetesClusterInfoCliOpts, + utils::cluster_info::KubernetesClusterInfoOpts, }; pub const AUTHOR: &str = "Stackable GmbH - info@stackable.tech"; @@ -177,7 +177,7 @@ pub enum Command { /// use stackable_operator::{ /// logging::TracingTarget, /// namespace::WatchNamespace, -/// utils::cluster_info::KubernetesClusterInfoCliOpts +/// utils::cluster_info::KubernetesClusterInfoOpts /// }; /// /// let opts = Command::::parse_from(["foobar-operator", "run", "--name", "foo", "--product-config", "bar", "--watch-namespace", "foobar"]); @@ -187,7 +187,7 @@ pub enum Command { /// product_config: ProductConfigPath::from("bar".as_ref()), /// watch_namespace: WatchNamespace::One("foobar".to_string()), /// tracing_target: TracingTarget::None, -/// cluster_info_opts: KubernetesClusterInfoCliOpts { +/// cluster_info_opts: KubernetesClusterInfoOpts { /// kubernetes_cluster_domain: None /// } /// }, @@ -225,7 +225,7 @@ pub struct ProductOperatorRun { pub tracing_target: TracingTarget, #[command(flatten)] - pub cluster_info_opts: KubernetesClusterInfoCliOpts, + pub cluster_info_opts: KubernetesClusterInfoOpts, } /// A path to a [`ProductConfigManager`] spec file @@ -399,7 +399,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, - cluster_info_opts: KubernetesClusterInfoCliOpts { + cluster_info_opts: KubernetesClusterInfoOpts { kubernetes_cluster_domain: None } } @@ -413,7 +413,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::All, tracing_target: TracingTarget::None, - cluster_info_opts: KubernetesClusterInfoCliOpts { + cluster_info_opts: KubernetesClusterInfoOpts { kubernetes_cluster_domain: None } } @@ -428,7 +428,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, - cluster_info_opts: KubernetesClusterInfoCliOpts { + cluster_info_opts: KubernetesClusterInfoOpts { kubernetes_cluster_domain: None } } diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index 7ccbf5d02..a59ebfaa5 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -21,7 +21,7 @@ use tracing::trace; use crate::{ kvp::LabelSelectorExt, - utils::cluster_info::{KubernetesClusterInfo, KubernetesClusterInfoCliOpts}, + utils::cluster_info::{KubernetesClusterInfo, KubernetesClusterInfoOpts}, }; pub type Result = std::result::Result; @@ -523,13 +523,13 @@ impl Client { /// use k8s_openapi::api::core::v1::Pod; /// use stackable_operator::{ /// client::{Client, initialize_operator}, - /// utils::cluster_info::KubernetesClusterInfoCliOpts + /// utils::cluster_info::KubernetesClusterInfoOpts /// }; /// /// #[tokio::main] /// async fn main() { /// - /// let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + /// let cluster_info_cli_opts = KubernetesClusterInfoOpts { /// kubernetes_cluster_domain: None, /// }; /// let client = initialize_operator(None, &cluster_info_cli_opts) @@ -643,7 +643,7 @@ where pub async fn initialize_operator( field_manager: Option, - cluster_info_cli_opts: &KubernetesClusterInfoCliOpts, + cluster_info_cli_opts: &KubernetesClusterInfoOpts, ) -> Result { let kubeconfig: Config = kube::Config::infer() .await @@ -676,12 +676,12 @@ mod tests { }; use tokio::time::error::Elapsed; - use crate::utils::cluster_info::KubernetesClusterInfoCliOpts; + use crate::utils::cluster_info::KubernetesClusterInfoOpts; #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created() { - let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + let cluster_info_cli_opts = KubernetesClusterInfoOpts { kubernetes_cluster_domain: None, }; let client = super::initialize_operator(None, &cluster_info_cli_opts) @@ -762,7 +762,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created_timeout() { - let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + let cluster_info_cli_opts = KubernetesClusterInfoOpts { kubernetes_cluster_domain: None, }; let client = super::initialize_operator(None, &cluster_info_cli_opts) @@ -785,7 +785,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_list_with_label_selector() { - let cluster_info_cli_opts = KubernetesClusterInfoCliOpts { + let cluster_info_cli_opts = KubernetesClusterInfoOpts { kubernetes_cluster_domain: None, }; let client = super::initialize_operator(None, &cluster_info_cli_opts) diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index a011bef57..f17400971 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -10,7 +10,7 @@ pub struct KubernetesClusterInfo { } #[derive(clap::Parser, Debug, PartialEq, Eq)] -pub struct KubernetesClusterInfoCliOpts { +pub struct KubernetesClusterInfoOpts { /// Kubernetes cluster domain, usually this is `cluster.local`. // We are not using a default value here, as operators will probably do an more advanced // auto-detection of the cluster domain in case it is not specified in the future. @@ -19,7 +19,7 @@ pub struct KubernetesClusterInfoCliOpts { } impl KubernetesClusterInfo { - pub fn new(cluster_info_cli_opts: &KubernetesClusterInfoCliOpts) -> Self { + pub fn new(cluster_info_cli_opts: &KubernetesClusterInfoOpts) -> Self { let cluster_domain = match &cluster_info_cli_opts.kubernetes_cluster_domain { Some(cluster_domain) => { tracing::info!(%cluster_domain, "Using configured Kubernetes cluster domain"); From 9c36cdaf5c6b74d8923bf40ecad794751bffc22e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 15:22:04 +0200 Subject: [PATCH 09/12] Derive Default for KubernetesClusterInfoOpts --- crates/stackable-operator/src/cli.rs | 17 +++--------- crates/stackable-operator/src/client.rs | 27 ++++--------------- .../src/utils/cluster_info.rs | 2 +- 3 files changed, 10 insertions(+), 36 deletions(-) diff --git a/crates/stackable-operator/src/cli.rs b/crates/stackable-operator/src/cli.rs index a51d96b0c..8bfeb26c9 100644 --- a/crates/stackable-operator/src/cli.rs +++ b/crates/stackable-operator/src/cli.rs @@ -177,7 +177,6 @@ pub enum Command { /// use stackable_operator::{ /// logging::TracingTarget, /// namespace::WatchNamespace, -/// utils::cluster_info::KubernetesClusterInfoOpts /// }; /// /// let opts = Command::::parse_from(["foobar-operator", "run", "--name", "foo", "--product-config", "bar", "--watch-namespace", "foobar"]); @@ -187,9 +186,7 @@ pub enum Command { /// product_config: ProductConfigPath::from("bar".as_ref()), /// watch_namespace: WatchNamespace::One("foobar".to_string()), /// tracing_target: TracingTarget::None, -/// cluster_info_opts: KubernetesClusterInfoOpts { -/// kubernetes_cluster_domain: None -/// } +/// cluster_info_opts: Default::default(), /// }, /// })); /// ``` @@ -399,9 +396,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, - cluster_info_opts: KubernetesClusterInfoOpts { - kubernetes_cluster_domain: None - } + cluster_info_opts: Default::default(), } ); @@ -413,9 +408,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::All, tracing_target: TracingTarget::None, - cluster_info_opts: KubernetesClusterInfoOpts { - kubernetes_cluster_domain: None - } + cluster_info_opts: Default::default(), } ); @@ -428,9 +421,7 @@ mod tests { product_config: ProductConfigPath::from("bar".as_ref()), watch_namespace: WatchNamespace::One("foo".to_string()), tracing_target: TracingTarget::None, - cluster_info_opts: KubernetesClusterInfoOpts { - kubernetes_cluster_domain: None - } + cluster_info_opts: Default::default(), } ); } diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index a59ebfaa5..5d0fdb3bc 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -521,18 +521,12 @@ impl Client { /// use tokio::time::error::Elapsed; /// use kube::runtime::watcher; /// use k8s_openapi::api::core::v1::Pod; - /// use stackable_operator::{ - /// client::{Client, initialize_operator}, - /// utils::cluster_info::KubernetesClusterInfoOpts - /// }; + /// use stackable_operator::client::{Client, initialize_operator}; /// /// #[tokio::main] /// async fn main() { /// - /// let cluster_info_cli_opts = KubernetesClusterInfoOpts { - /// kubernetes_cluster_domain: None, - /// }; - /// let client = initialize_operator(None, &cluster_info_cli_opts) + /// let client = initialize_operator(None, &Default::default()) /// .await /// .expect("Unable to construct client."); /// let watcher_config: watcher::Config = @@ -676,15 +670,10 @@ mod tests { }; use tokio::time::error::Elapsed; - use crate::utils::cluster_info::KubernetesClusterInfoOpts; - #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created() { - let cluster_info_cli_opts = KubernetesClusterInfoOpts { - kubernetes_cluster_domain: None, - }; - let client = super::initialize_operator(None, &cluster_info_cli_opts) + let client = super::initialize_operator(None, &Default::default()) .await .expect("KUBECONFIG variable must be configured."); @@ -762,10 +751,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_wait_created_timeout() { - let cluster_info_cli_opts = KubernetesClusterInfoOpts { - kubernetes_cluster_domain: None, - }; - let client = super::initialize_operator(None, &cluster_info_cli_opts) + let client = super::initialize_operator(None, &Default::default()) .await .expect("KUBECONFIG variable must be configured."); @@ -785,10 +771,7 @@ mod tests { #[tokio::test] #[ignore = "Tests depending on Kubernetes are not ran by default"] async fn k8s_test_list_with_label_selector() { - let cluster_info_cli_opts = KubernetesClusterInfoOpts { - kubernetes_cluster_domain: None, - }; - let client = super::initialize_operator(None, &cluster_info_cli_opts) + let client = super::initialize_operator(None, &Default::default()) .await .expect("KUBECONFIG variable must be configured."); diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index f17400971..05d364ec1 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -9,7 +9,7 @@ pub struct KubernetesClusterInfo { pub cluster_domain: DomainName, } -#[derive(clap::Parser, Debug, PartialEq, Eq)] +#[derive(clap::Parser, Debug, Default, PartialEq, Eq)] pub struct KubernetesClusterInfoOpts { /// Kubernetes cluster domain, usually this is `cluster.local`. // We are not using a default value here, as operators will probably do an more advanced From 6fd8779c6f9a654166743d734ad4e2986345fb24 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 15:24:06 +0200 Subject: [PATCH 10/12] Fix cli leftovers --- crates/stackable-operator/src/client.rs | 4 ++-- crates/stackable-operator/src/utils/cluster_info.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/stackable-operator/src/client.rs b/crates/stackable-operator/src/client.rs index 5d0fdb3bc..de2b5f870 100644 --- a/crates/stackable-operator/src/client.rs +++ b/crates/stackable-operator/src/client.rs @@ -637,7 +637,7 @@ where pub async fn initialize_operator( field_manager: Option, - cluster_info_cli_opts: &KubernetesClusterInfoOpts, + cluster_info_opts: &KubernetesClusterInfoOpts, ) -> Result { let kubeconfig: Config = kube::Config::infer() .await @@ -645,7 +645,7 @@ pub async fn initialize_operator( .context(InferKubeConfigSnafu)?; let default_namespace = kubeconfig.default_namespace.clone(); let client = kube::Client::try_from(kubeconfig).context(CreateKubeClientSnafu)?; - let cluster_info = KubernetesClusterInfo::new(cluster_info_cli_opts); + let cluster_info = KubernetesClusterInfo::new(cluster_info_opts); Ok(Client::new( client, diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index 05d364ec1..d7a153e68 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -19,8 +19,8 @@ pub struct KubernetesClusterInfoOpts { } impl KubernetesClusterInfo { - pub fn new(cluster_info_cli_opts: &KubernetesClusterInfoOpts) -> Self { - let cluster_domain = match &cluster_info_cli_opts.kubernetes_cluster_domain { + pub fn new(cluster_info_opts: &KubernetesClusterInfoOpts) -> Self { + let cluster_domain = match &cluster_info_opts.kubernetes_cluster_domain { Some(cluster_domain) => { tracing::info!(%cluster_domain, "Using configured Kubernetes cluster domain"); From 7ca2f5d3bff95d6e1224302388ed69fb9ef53f0b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 15:53:29 +0200 Subject: [PATCH 11/12] Add rust docs --- crates/stackable-operator/src/utils/cluster_info.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/stackable-operator/src/utils/cluster_info.rs b/crates/stackable-operator/src/utils/cluster_info.rs index d7a153e68..d31275668 100644 --- a/crates/stackable-operator/src/utils/cluster_info.rs +++ b/crates/stackable-operator/src/utils/cluster_info.rs @@ -4,8 +4,10 @@ use crate::commons::networking::DomainName; const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local"; +/// Some information that we know about the Kubernetes cluster. #[derive(Debug, Clone)] pub struct KubernetesClusterInfo { + /// The Kubernetes cluster domain, typically `cluster.local`. pub cluster_domain: DomainName, } From e371742c6c4170ddfeb93af242d420562188848a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 22 Oct 2024 16:43:51 +0200 Subject: [PATCH 12/12] Mark as breaking in changelog --- 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 1104bc379..323f02ee2 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Changed -- Don't parse `/etc/resolv.conf` to auto-detect the Kubernetes cluster domain in case it is not explicitly configured. +- BREAKING: Don't parse `/etc/resolv.conf` to auto-detect the Kubernetes cluster domain in case it is not explicitly configured. Instead the operator will default to `cluster.local`. We revert this now after some concerns where raised, we will create a follow-up decision instead addressing how we will continue with this ([#896]).