chore: Merge branch 'main' into feat/stackable-versioned-mod-support

Techassi · Techassi · commit 31378ff94bec · 2024-10-18T09:52:14.000+02:00
diff --git 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/Cargo.lock b/Cargo.lock
diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md
@@ -4,23 +4,32 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+## [0.79.0] - 2024-10-18
+
 ### Added
 
 - 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` 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<BTreeMap>` 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<BTreeMap>` to prefer the maximum
+  value) ([#888]).
 
 ### Removed
 
@@ -31,6 +40,7 @@ All notable changes to this project will be documented in this file.
 [#883]: https://github.com/stackabletech/operator-rs/pull/883
 [#885]: https://github.com/stackabletech/operator-rs/pull/885
 [#888]: https://github.com/stackabletech/operator-rs/pull/888
+[#893]: https://github.com/stackabletech/operator-rs/pull/893
 
 ## [0.78.0] - 2024-09-30
 
diff --git a/crates/stackable-operator/Cargo.toml b/crates/stackable-operator/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "stackable-operator"
 description = "Stackable Operator Framework"
-version = "0.78.0"
+version = "0.79.0"
 authors.workspace = true
 license.workspace = true
 edition.workspace = true
diff --git a/crates/stackable-operator/fixtures/cluster_domain/fail/invalid.resolv.conf 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
@@ -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
@@ -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
@@ -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
@@ -1,4 +1,5 @@
 use crate::kvp::LabelSelectorExt;
+use crate::utils::cluster_domain::{self, retrieve_cluster_domain, KUBERNETES_CLUSTER_DOMAIN};
 
 use either::Either;
 use futures::StreamExt;
@@ -77,6 +78,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.
@@ -510,12 +514,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"));
     ///
@@ -622,7 +626,13 @@ where
     }
 }
 
-pub async fn create_client(field_manager: Option<String>) -> Result<Client> {
+pub async fn initialize_operator(field_manager: Option<String>) -> Result<Client> {
+    let _ = KUBERNETES_CLUSTER_DOMAIN
+        .set(retrieve_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?);
+    create_client(field_manager).await
+}
+
+async fn create_client(field_manager: Option<String>) -> Result<Client> {
     let kubeconfig: Config = kube::Config::infer()
         .await
         .map_err(kube::Error::InferConfig)
diff --git a/crates/stackable-operator/src/commons/networking.rs b/crates/stackable-operator/src/commons/networking.rs
@@ -30,6 +30,14 @@ impl TryFrom<String> for DomainName {
     }
 }
 
+impl TryFrom<&str> for DomainName {
+    type Error = validation::Errors;
+
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        value.parse()
+    }
+}
+
 impl From<DomainName> 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
@@ -0,0 +1,187 @@
+use std::{env, path::PathBuf, str::FromStr, sync::OnceLock};
+
+use snafu::{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 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
+///
+/// - <https://github.com/stackabletech/issues/issues/436>
+/// - <https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/>
+pub static KUBERNETES_CLUSTER_DOMAIN: OnceLock<DomainName> = OnceLock::new();
+
+#[instrument]
+pub(crate) fn retrieve_cluster_domain() -> Result<DomainName, Error> {
+    // 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<PathBuf> + std::fmt::Debug,
+) -> Result<String, Error> {
+    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 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());
+    }
+}
diff --git a/crates/stackable-operator/src/utils/mod.rs b/crates/stackable-operator/src/utils/mod.rs
@@ -1,4 +1,5 @@
 pub mod bash;
+pub mod cluster_domain;
 pub mod crds;
 pub mod logging;
 mod option;

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+nameserver 10.243.21.53`
	`2`	`+options ndots:5`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+search sble-operators.svc.cluster.local svc.cluster.local cluster.local`
	`2`	`+nameserver 10.243.21.53`
	`3`	`+options ndots:5`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+search openshift-service-ca-operator.svc.cluster.local svc.cluster.local cluster.local cmx.repl-openshift.build`
	`2`	`+nameserver 172.30.0.10`
	`3`	`+options ndots:5`
Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,14 @@ impl TryFrom<String> for DomainName {`
`30`	`30`	`}`
`31`	`31`	`}`
`32`	`32`
	`33`	`+impl TryFrom<&str> for DomainName {`
	`34`	`+ type Error = validation::Errors;`
	`35`	`+`
	`36`	`+ fn try_from(value: &str) -> Result<Self, Self::Error> {`
	`37`	`+ value.parse()`
	`38`	`+ }`
	`39`	`+}`
	`40`	`+`
`33`	`41`	`impl From<DomainName> for String {`
`34`	`42`	`fn from(value: DomainName) -> Self {`
`35`	`43`	`value.0`