From 2b28ba2ede568ecff8a9508409d8495cc185766d Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 1 Mar 2024 14:21:38 +0100 Subject: [PATCH 1/7] Support listing endpoints of Listeners in "stacklet list" command --- .../stackable-cockpit/src/platform/service.rs | 48 +++++++++++++++++-- .../src/platform/stacklet/mod.rs | 2 +- .../stackable-cockpit/src/utils/k8s/client.rs | 26 +++++++++- rust/stackablectl/CHANGELOG.md | 5 ++ 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/rust/stackable-cockpit/src/platform/service.rs b/rust/stackable-cockpit/src/platform/service.rs index cda0d481..0d1dc95a 100644 --- a/rust/stackable-cockpit/src/platform/service.rs +++ b/rust/stackable-cockpit/src/platform/service.rs @@ -45,15 +45,53 @@ pub async fn get_endpoints( object_name: &str, object_namespace: &str, ) -> Result, Error> { - let service_list_params = + let mut endpoints = IndexMap::new(); + let list_params = ListParams::from_product(product_name, Some(object_name), k8s::ProductLabel::Name); - let services = kube_client - .list_services(Some(object_namespace), &service_list_params) + let listeners = kube_client + .list_listeners(Some(object_namespace), &list_params) .await .context(KubeClientFetchSnafu)?; - let mut endpoints = IndexMap::new(); + for listener in &listeners { + let Some(listener_status) = &listener.status else { + continue; + }; + for address in listener_status.ingress_addresses.iter().flatten() { + for port in &address.ports { + // Listener name usually has pattern listener-simple-hdfs-namenode-default-0 or + // simple-hdfs-datanode-default-0-listener, so we can strip everything before the first occurrence of + // the stacklet name (simple-hdfs in this case). + // This truncation is *not* ideal, however we only have implemented listener-operator for HDFS so far. + let listener_name = listener.name_any(); + let Some((_, display_name)) = listener_name.split_once(object_name) else { + continue; + }; + let text = format!( + "{display_name}-{port_name}", + display_name = display_name.trim_start_matches('-'), + port_name = port.0 + ); + let endpoint_url = endpoint_url(&address.address, *port.1, port.0); + endpoints.insert(text, endpoint_url); + } + } + } + + // Ideally we use listener-operator everywhere, afterwards we can remove the whole k8s Services fetching. + // Currently the Services created by listener-op are missing the recommended labels, so early exit in case we find + // Listeners is not required. However, once we add the recommended labels to the k8s Services, we would have + // duplicated entries (one from the Listener and one from the Service). Because of this we don't look at the + // Services in case we found Listeners! + if !listeners.items.is_empty() { + return Ok(endpoints); + } + + let services = kube_client + .list_services(Some(object_namespace), &list_params) + .await + .context(KubeClientFetchSnafu)?; for service in services { match get_endpoint_urls(kube_client, &service, object_name).await { @@ -268,7 +306,7 @@ async fn get_node_name_ip_mapping( } fn endpoint_url(endpoint_host: &str, endpoint_port: i32, port_name: &str) -> String { - // TODO: Consolidate web-ui port names in operators based on decision in arch meeting from 2022/08/10 + // TODO: Consolidate web-ui port names in operators based on decision in arch meeting from 2022-08-10 // For Superset: https://github.com/stackabletech/superset-operator/issues/248 // For Airflow: https://github.com/stackabletech/airflow-operator/issues/146 // As we still support older operator versions we need to also include the "old" way of naming diff --git a/rust/stackable-cockpit/src/platform/stacklet/mod.rs b/rust/stackable-cockpit/src/platform/stacklet/mod.rs index eaaacf98..fbd8dcb3 100644 --- a/rust/stackable-cockpit/src/platform/stacklet/mod.rs +++ b/rust/stackable-cockpit/src/platform/stacklet/mod.rs @@ -126,7 +126,7 @@ async fn list_stackable_stacklets( Some(obj) => obj, None => { info!( - "Failed to list services because the gvk {product_gvk:?} can not be resolved" + "Failed to list stacklets because the gvk {product_gvk:?} can not be resolved" ); continue; } diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index 32218843..12df47b6 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -12,7 +12,7 @@ use kube::{ }; use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; -use stackable_operator::kvp::Labels; +use stackable_operator::{commons::listener::Listener, kvp::Labels}; use crate::{ platform::{cluster, credentials::Credentials}, @@ -188,7 +188,7 @@ impl Client { )) } - /// Lists [`Service`]s by matching labels. The services can be matched by + /// Lists [`Service`]s by matching labels. The Services can be matched by /// the product labels. [`ListParamsExt`] provides a utility function to /// create [`ListParams`] based on a product name and optional instance /// name. @@ -210,6 +210,28 @@ impl Client { Ok(services) } + /// Lists [`Listener`]s by matching labels. The Listeners can be matched by + /// the product labels. [`ListParamsExt`] provides a utility function to + /// create [`ListParams`] based on a product name and optional instance + /// name. + pub async fn list_listeners( + &self, + namespace: Option<&str>, + list_params: &ListParams, + ) -> ListResult { + let listener_api: Api = match namespace { + Some(namespace) => Api::namespaced(self.client.clone(), namespace), + None => Api::all(self.client.clone()), + }; + + let listeners = listener_api + .list(list_params) + .await + .context(KubeClientFetchSnafu)?; + + Ok(listeners) + } + /// Retrieves user credentials consisting of username and password from a /// secret identified by `secret_name` inside the `secret_namespace`. If /// either one of the values is missing, [`Ok(None)`] is returned. An error diff --git a/rust/stackablectl/CHANGELOG.md b/rust/stackablectl/CHANGELOG.md index fa744333..38b5e2aa 100644 --- a/rust/stackablectl/CHANGELOG.md +++ b/rust/stackablectl/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- Support listing endpoints of Listeners in `stackablectl stacklet list`. + Currently only HDFS is using listener-op, so we can only test that so far ([#XXX]). + ### Changed - Operators are now installed in parallel when installing a release ([#202]). From f191ec4d904a6c336d023af076abf59a8cadb67c Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 1 Mar 2024 14:24:16 +0100 Subject: [PATCH 2/7] changelog --- rust/stackablectl/CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rust/stackablectl/CHANGELOG.md b/rust/stackablectl/CHANGELOG.md index 38b5e2aa..b50ab681 100644 --- a/rust/stackablectl/CHANGELOG.md +++ b/rust/stackablectl/CHANGELOG.md @@ -6,8 +6,8 @@ All notable changes to this project will be documented in this file. ### Added -- Support listing endpoints of Listeners in `stackablectl stacklet list`. - Currently only HDFS is using listener-op, so we can only test that so far ([#XXX]). +- Support listing endpoints of Listeners in `stackablectl stacklet list` command. + Currently only HDFS is using listener-op, so we can only test that so far ([#213]). ### Changed @@ -19,6 +19,7 @@ All notable changes to this project will be documented in this file. [#181]: https://github.com/stackabletech/stackable-cockpit/pull/181 [#202]: https://github.com/stackabletech/stackable-cockpit/pull/202 +[#213]: https://github.com/stackabletech/stackable-cockpit/pull/213 ## [23.11.3] - 2024-01-03 From 763e23ef64161b87c4919924688412cde539ba12 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 1 Mar 2024 14:26:56 +0100 Subject: [PATCH 3/7] improve docs --- .../stackable-cockpit/src/platform/service.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/rust/stackable-cockpit/src/platform/service.rs b/rust/stackable-cockpit/src/platform/service.rs index 0d1dc95a..9aba9b0b 100644 --- a/rust/stackable-cockpit/src/platform/service.rs +++ b/rust/stackable-cockpit/src/platform/service.rs @@ -60,10 +60,11 @@ pub async fn get_endpoints( }; for address in listener_status.ingress_addresses.iter().flatten() { for port in &address.ports { - // Listener name usually has pattern listener-simple-hdfs-namenode-default-0 or - // simple-hdfs-datanode-default-0-listener, so we can strip everything before the first occurrence of - // the stacklet name (simple-hdfs in this case). - // This truncation is *not* ideal, however we only have implemented listener-operator for HDFS so far. + // Listener names usually have the pattern "listener-simple-hdfs-namenode-default-0" or + // "simple-hdfs-datanode-default-0-listener", so we can strip everything before the first occurrence of + // the stacklet name ("simple-hdfs" in this case). + // This truncation is *not* ideal, however we only have implemented listener-operator for HDFS so far, + // so better to have support for that than nothing :) let listener_name = listener.name_any(); let Some((_, display_name)) = listener_name.split_once(object_name) else { continue; @@ -79,11 +80,11 @@ pub async fn get_endpoints( } } - // Ideally we use listener-operator everywhere, afterwards we can remove the whole k8s Services fetching. - // Currently the Services created by listener-op are missing the recommended labels, so early exit in case we find - // Listeners is not required. However, once we add the recommended labels to the k8s Services, we would have - // duplicated entries (one from the Listener and one from the Service). Because of this we don't look at the - // Services in case we found Listeners! + // Ideally we use listener-operator everywhere, afterwards we can remove the whole k8s Services handling following. + // Currently the Services created by listener-op are missing the recommended labels, so this early exit in case we + // find Listeners is currently not required. However, once we add the recommended labels to the k8s Services, we + // would have duplicated entries (one from the Listener and one from the Service). Because of this we don't look at + // the Services in case we found Listeners! if !listeners.items.is_empty() { return Ok(endpoints); } From b7d4c9d68440ffe210f902966af82805c698cfda Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 1 Mar 2024 15:05:20 +0100 Subject: [PATCH 4/7] doc comment --- rust/stackable-cockpit/src/platform/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/stackable-cockpit/src/platform/service.rs b/rust/stackable-cockpit/src/platform/service.rs index 9aba9b0b..45c544d8 100644 --- a/rust/stackable-cockpit/src/platform/service.rs +++ b/rust/stackable-cockpit/src/platform/service.rs @@ -62,7 +62,7 @@ pub async fn get_endpoints( for port in &address.ports { // Listener names usually have the pattern "listener-simple-hdfs-namenode-default-0" or // "simple-hdfs-datanode-default-0-listener", so we can strip everything before the first occurrence of - // the stacklet name ("simple-hdfs" in this case). + // the stacklet name ("simple-hdfs" in this case). After that it actually get's pretty hard. // This truncation is *not* ideal, however we only have implemented listener-operator for HDFS so far, // so better to have support for that than nothing :) let listener_name = listener.name_any(); From 4c7b9df80fe02feb37a85c480c15c9890eeb69f6 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 7 Mar 2024 15:13:22 +0100 Subject: [PATCH 5/7] Move endpoints declaration down --- rust/stackable-cockpit/src/platform/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/stackable-cockpit/src/platform/service.rs b/rust/stackable-cockpit/src/platform/service.rs index 45c544d8..5bc89c10 100644 --- a/rust/stackable-cockpit/src/platform/service.rs +++ b/rust/stackable-cockpit/src/platform/service.rs @@ -45,7 +45,6 @@ pub async fn get_endpoints( object_name: &str, object_namespace: &str, ) -> Result, Error> { - let mut endpoints = IndexMap::new(); let list_params = ListParams::from_product(product_name, Some(object_name), k8s::ProductLabel::Name); @@ -54,6 +53,7 @@ pub async fn get_endpoints( .await .context(KubeClientFetchSnafu)?; + let mut endpoints = IndexMap::new(); for listener in &listeners { let Some(listener_status) = &listener.status else { continue; From 6250fae717cda28e6551dc9bf6f249dc028b1f1e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 7 Mar 2024 15:14:33 +0100 Subject: [PATCH 6/7] Update rust/stackable-cockpit/src/platform/service.rs Co-authored-by: Techassi --- rust/stackable-cockpit/src/platform/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/stackable-cockpit/src/platform/service.rs b/rust/stackable-cockpit/src/platform/service.rs index 5bc89c10..706b75ba 100644 --- a/rust/stackable-cockpit/src/platform/service.rs +++ b/rust/stackable-cockpit/src/platform/service.rs @@ -80,7 +80,7 @@ pub async fn get_endpoints( } } - // Ideally we use listener-operator everywhere, afterwards we can remove the whole k8s Services handling following. + // Ideally we use listener-operator everywhere, afterwards we can remove the whole k8s Services handling below. // Currently the Services created by listener-op are missing the recommended labels, so this early exit in case we // find Listeners is currently not required. However, once we add the recommended labels to the k8s Services, we // would have duplicated entries (one from the Listener and one from the Service). Because of this we don't look at From 1a11e416af22f888a451b19276666b691fbf9be4 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 11 Mar 2024 10:13:16 +0100 Subject: [PATCH 7/7] Move stuff into display_name_for_listener_name function --- .../stackable-cockpit/src/platform/service.rs | 55 ++++++++++++++----- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/rust/stackable-cockpit/src/platform/service.rs b/rust/stackable-cockpit/src/platform/service.rs index 706b75ba..491d20c9 100644 --- a/rust/stackable-cockpit/src/platform/service.rs +++ b/rust/stackable-cockpit/src/platform/service.rs @@ -55,25 +55,17 @@ pub async fn get_endpoints( let mut endpoints = IndexMap::new(); for listener in &listeners { + let Some(display_name) = display_name_for_listener_name(&listener.name_any(), object_name) + else { + continue; + }; let Some(listener_status) = &listener.status else { continue; }; + for address in listener_status.ingress_addresses.iter().flatten() { for port in &address.ports { - // Listener names usually have the pattern "listener-simple-hdfs-namenode-default-0" or - // "simple-hdfs-datanode-default-0-listener", so we can strip everything before the first occurrence of - // the stacklet name ("simple-hdfs" in this case). After that it actually get's pretty hard. - // This truncation is *not* ideal, however we only have implemented listener-operator for HDFS so far, - // so better to have support for that than nothing :) - let listener_name = listener.name_any(); - let Some((_, display_name)) = listener_name.split_once(object_name) else { - continue; - }; - let text = format!( - "{display_name}-{port_name}", - display_name = display_name.trim_start_matches('-'), - port_name = port.0 - ); + let text = format!("{display_name}-{port_name}", port_name = port.0); let endpoint_url = endpoint_url(&address.address, *port.1, port.0); endpoints.insert(text, endpoint_url); } @@ -324,3 +316,38 @@ fn endpoint_url(endpoint_host: &str, endpoint_port: i32, port_name: &str) -> Str format!("{endpoint_host}:{endpoint_port}") } } + +/// Listener names usually have the pattern `listener-simple-hdfs-namenode-default-0` or +/// `simple-hdfs-datanode-default-0-listener`, so we can strip everything before the first occurrence of +/// the stacklet name (`simple-hdfs` in this case). After that it actually get's pretty hard. +/// This truncation is *not* ideal, however we only have implemented listener-operator for HDFS so far, +/// so better to have support for that than nothing :) +fn display_name_for_listener_name(listener_name: &str, object_name: &str) -> Option { + let Some((_, display_name)) = listener_name.split_once(object_name) else { + return None; + }; + Some(display_name.trim_start_matches('-').to_owned()) +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + #[rstest] + // These are all the listener names implemented so far (only HDFS is using listener-operator). In the future more + // test-case should be added. + #[case("listener-simple-hdfs-namenode-default-0", "simple-hdfs", Some("namenode-default-0".to_string()))] + #[case("listener-simple-hdfs-namenode-default-1", "simple-hdfs", Some("namenode-default-1".to_string()))] + // FIXME: Come up with a more clever strategy to remove the `-listener` suffix. I would prefer to wait until we + // actually have more products using listener-op to not accidentally strip to much. + #[case("simple-hdfs-datanode-default-0-listener", "simple-hdfs", Some("datanode-default-0-listener".to_string()))] + fn test_display_name_for_listener_name( + #[case] listener_name: &str, + #[case] object_name: &str, + #[case] expected: Option, + ) { + let output = display_name_for_listener_name(listener_name, object_name); + assert_eq!(output, expected); + } +}