refactor: append trailing dot only to cluster domain

Author: dervoeti

crates/stackable-operator/src/commons/authentication/oidc.rs

@@ -323,7 +323,7 @@ mod test {
 
         assert_eq!(
             oidc.endpoint_url().unwrap().as_str(),
-            "http://my.keycloak.server.:12345/my-root-path"
+            "http://my.keycloak.server:12345/my-root-path"
         );
     }
 
@@ -345,7 +345,7 @@ mod test {
 
         assert_eq!(
             oidc.endpoint_url().unwrap().as_str(),
-            "https://my.keycloak.server./"
+            "https://my.keycloak.server/"
         );
     }
 
@@ -369,13 +369,13 @@ mod test {
     }
 
     #[rstest]
-    #[case("/", "http://my.keycloak.server.:1234/")]
-    #[case("/realms/sdp", "http://my.keycloak.server.:1234/realms/sdp")]
-    #[case("/realms/sdp/", "http://my.keycloak.server.:1234/realms/sdp")]
-    #[case("/realms/sdp//////", "http://my.keycloak.server.:1234/realms/sdp")]
+    #[case("/", "http://my.keycloak.server:1234/")]
+    #[case("/realms/sdp", "http://my.keycloak.server:1234/realms/sdp")]
+    #[case("/realms/sdp/", "http://my.keycloak.server:1234/realms/sdp")]
+    #[case("/realms/sdp//////", "http://my.keycloak.server:1234/realms/sdp")]
     #[case(
         "/realms/my/realm/with/slashes//////",
-        "http://my.keycloak.server.:1234/realms/my/realm/with/slashes"
+        "http://my.keycloak.server:1234/realms/my/realm/with/slashes"
     )]
     fn root_path_endpoint_url(#[case] root_path: String, #[case] expected_endpoint_url: &str) {
         let oidc = serde_yaml::from_str::<AuthenticationProvider>(&format!(
@@ -393,22 +393,22 @@ mod test {
     }
 
     #[rstest]
-    #[case("/", "https://my.keycloak.server./.well-known/openid-configuration")]
+    #[case("/", "https://my.keycloak.server/.well-known/openid-configuration")]
     #[case(
         "/realms/sdp",
-        "https://my.keycloak.server./realms/sdp/.well-known/openid-configuration"
+        "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration"
     )]
     #[case(
         "/realms/sdp/",
-        "https://my.keycloak.server./realms/sdp/.well-known/openid-configuration"
+        "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration"
     )]
     #[case(
         "/realms/sdp//////",
-        "https://my.keycloak.server./realms/sdp/.well-known/openid-configuration"
+        "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration"
     )]
     #[case(
         "/realms/my/realm/with/slashes//////",
-        "https://my.keycloak.server./realms/my/realm/with/slashes/.well-known/openid-configuration"
+        "https://my.keycloak.server/realms/my/realm/with/slashes/.well-known/openid-configuration"
     )]
     fn root_path_well_known_url(#[case] root_path: String, #[case] expected_well_known_url: &str) {
         let oidc = serde_yaml::from_str::<AuthenticationProvider>(&format!(

crates/stackable-operator/src/commons/networking.rs

@@ -18,10 +18,8 @@ impl FromStr for DomainName {
 
     fn from_str(value: &str) -> Result<Self, Self::Err> {
         // Remove optional trailing dot before validation, logic taken from https://github.com/kubernetes/kubernetes/blob/30de989fb57fb5921a7ae3e3203cf7ecac9cf3f0/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L100
-        let value = value.strip_suffix('.').unwrap_or(value);
-        validation::is_rfc_1123_subdomain(value)?;
-        // Append trailing dot to make the domain name a FQDN to improve DNS lookup performance, see https://github.com/stackabletech/issues/issues/656
-        Ok(DomainName(format!("{}.", value)))
+        validation::is_rfc_1123_subdomain(value.strip_suffix('.').unwrap_or(value))?;
+        Ok(DomainName(value.to_owned()))
     }
 }
 
@@ -185,22 +183,17 @@ mod tests {
     use rstest::rstest;
 
     #[rstest]
-    #[case("foo", "foo.")]
-    #[case("foo.", "foo.")]
-    #[case("foo.bar", "foo.bar.")]
-    #[case("foo.bar.", "foo.bar.")]
-    fn test_domain_name_and_host_name_parsing_success(
-        #[case] domain_name: String,
-        #[case] expected_domain_name: &str,
-    ) {
+    #[case("foo")]
+    #[case("foo.bar")]
+    fn test_domain_name_and_host_name_parsing_success(#[case] domain_name: String) {
         let parsed_domain_name: DomainName =
             domain_name.parse().expect("domain name can not be parsed");
         // Every domain name is also a valid host name
         let parsed_host_name: HostName = domain_name.parse().expect("host name can not be parsed");
 
         // Also test the round-trip
-        assert_eq!(parsed_domain_name.to_string(), expected_domain_name);
-        assert_eq!(parsed_host_name.to_string(), expected_domain_name);
+        assert_eq!(parsed_domain_name.to_string(), domain_name);
+        assert_eq!(parsed_host_name.to_string(), domain_name);
     }
 
     #[rstest]
@@ -214,8 +207,8 @@ mod tests {
     }
 
     #[rstest]
-    #[case("foo.", "foo.")]
-    #[case("foo.bar.", "foo.bar.")]
+    #[case("foo", "foo")]
+    #[case("foo.bar", "foo.bar")]
     #[case("1.2.3.4", "1.2.3.4")]
     #[case("fe80::1", "[fe80::1]")]
     fn test_host_name_parsing_success(#[case] host: &str, #[case] expected_url_host: &str) {

crates/stackable-operator/src/commons/s3/helpers.rs

@@ -204,7 +204,7 @@ mod tests {
         };
         let (volumes, mounts) = s3.volumes_and_mounts().unwrap();
 
-        assert_eq!(s3.endpoint().unwrap(), Url::parse("http://minio.").unwrap());
+        assert_eq!(s3.endpoint().unwrap(), Url::parse("http://minio").unwrap());
         assert_eq!(volumes, vec![]);
         assert_eq!(mounts, vec![]);
     }
@@ -231,7 +231,7 @@ mod tests {
 
         assert_eq!(
             s3.endpoint().unwrap(),
-            Url::parse("https://s3-eu-central-2.ionoscloud.com.").unwrap()
+            Url::parse("https://s3-eu-central-2.ionoscloud.com").unwrap()
         );
         assert_eq!(volumes.len(), 1);
         let volume = volumes.remove(0);
@@ -283,7 +283,7 @@ mod tests {
 
         assert_eq!(
             s3.endpoint().unwrap(),
-            Url::parse("https://minio.:1234").unwrap()
+            Url::parse("https://minio:1234").unwrap()
         );
         assert_eq!(volumes, vec![]);
         assert_eq!(mounts, vec![]);

crates/stackable-operator/src/utils/cluster_info.rs

@@ -2,7 +2,7 @@ use std::str::FromStr;
 
 use crate::commons::networking::DomainName;
 
-const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local";
+const KUBERNETES_CLUSTER_DOMAIN_DEFAULT: &str = "cluster.local.";
 
 /// Some information that we know about the Kubernetes cluster.
 #[derive(Debug, Clone)]
@@ -24,16 +24,19 @@ impl KubernetesClusterInfo {
     pub fn new(cluster_info_opts: &KubernetesClusterInfoOpts) -> Self {
         let cluster_domain = match &cluster_info_opts.kubernetes_cluster_domain {
             Some(cluster_domain) => {
+                // Append trailing dot to make the domain name a FQDN to improve DNS lookup performance, see https://github.com/stackabletech/issues/issues/656
+                let cluster_domain = DomainName::from_str(&format!("{}.", cluster_domain))
+                    .expect("cluster_domain should already have been validated");
                 tracing::info!(%cluster_domain, "Using configured Kubernetes cluster domain");
 
-                cluster_domain.clone()
+                cluster_domain
             }
             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");
+                    .expect("KUBERNETES_CLUSTER_DOMAIN_DEFAULT constant must be a valid domain");
                 tracing::info!(%cluster_domain, "Defaulting Kubernetes cluster domain as it has not been configured");
 
                 cluster_domain