diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f2f9c82..839c88e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ All notable changes to this project will be documented in this file. - Fix a longstanding problem of including empty `categories`, `shortNames` and `additionalPrinterColumns` in the CRDs, which could cause problems with GitOps tools (e.g. ArgoCD) reporting a diff in the custom resources. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#860]). +- The operator now watches all resources that it creates and early-exits the reconcile action when the + cluster is marked for deletion ([#867]). [#841]: https://github.com/stackabletech/druid-operator/pull/841 [#846]: https://github.com/stackabletech/druid-operator/pull/846 @@ -38,6 +40,7 @@ All notable changes to this project will be documented in this file. [#856]: https://github.com/stackabletech/druid-operator/pull/856 [#860]: https://github.com/stackabletech/druid-operator/pull/860 [#865]: https://github.com/stackabletech/druid-operator/pull/865 +[#867]: https://github.com/stackabletech/druid-operator/pull/867 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.lock b/Cargo.lock index 72f964c9..73848f89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3239,6 +3239,7 @@ dependencies = [ "const_format", "fnv", "futures 0.3.34", + "http", "indoc", "openssl", "pin-project", diff --git a/Cargo.nix b/Cargo.nix index e3dea6df..5f7e044a 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -10666,6 +10666,10 @@ rec { } ]; devDependencies = [ + { + name = "http"; + packageId = "http"; + } { name = "rstest"; packageId = "rstest"; diff --git a/Cargo.toml b/Cargo.toml index aea3394a..89690f00 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ clap = "4.6" const_format = "0.2" fnv = "1.0" futures = { version = "0.3", features = ["compat"] } +http = "1.3" indoc = "2.0" openssl = "0.10" pin-project = "1.1" diff --git a/deploy/helm/druid-operator/templates/clusterrole-operator.yaml b/deploy/helm/druid-operator/templates/clusterrole-operator.yaml index 58b53a69..375a31ab 100644 --- a/deploy/helm/druid-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/druid-operator/templates/clusterrole-operator.yaml @@ -29,7 +29,8 @@ rules: - patch - watch # Shared internal authentication secret (cookie passphrase and internal client password). - # Orphan cleanup not needed (instead, Kubernetes GC via owner reference). + # Orphan cleanup not needed (instead, Kubernetes GC via owner reference). Watched by the + # controller, which needs `list` as well: `.owns()` uses lists before it watches. - apiGroups: - "" resources: @@ -38,9 +39,11 @@ rules: - create - delete - get + - list - patch + - watch # ServiceAccount created per DruidCluster for workload pod identity. - # Applied via SSA and tracked for orphan cleanup. Not watched by the controller. + # Applied via SSA and tracked for orphan cleanup and watched by the controller. - apiGroups: - "" resources: @@ -51,8 +54,9 @@ rules: - get - list - patch + - watch # RoleBinding created per DruidCluster to bind the product ClusterRole to the workload - # ServiceAccount. Applied via SSA and tracked for orphan cleanup. Not watched by the controller. + # ServiceAccount. Applied via SSA and tracked for orphan cleanup and watched by the controller. - apiGroups: - rbac.authorization.k8s.io resources: @@ -63,6 +67,7 @@ rules: - get - list - patch + - watch # Required to bind the product ClusterRole to the per-cluster ServiceAccount. - apiGroups: - rbac.authorization.k8s.io @@ -85,8 +90,7 @@ rules: - list - patch - watch - # PodDisruptionBudget created per role. Applied via SSA and tracked for orphan cleanup. - # Not watched by the controller. + # PodDisruptionBudget created per role. Applied via SSA and tracked for orphan cleanup and watched by the controller. - apiGroups: - policy resources: @@ -97,6 +101,7 @@ rules: - get - list - patch + - watch # Required for maintaining the CRDs within the operator (including the conversion webhook info). # Also for the startup condition check before the controller can run. - apiGroups: diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index ebd1b76a..e583fb6f 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -32,6 +32,7 @@ uuid.workspace = true built.workspace = true [dev-dependencies] +http.workspace = true rstest.workspace = true serde_yaml.workspace = true stackable-operator = { workspace = true, features = ["test-support"] } diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index c6c2bad6..28855bb5 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -17,6 +17,7 @@ use stackable_operator::{ rbac::v1::RoleBinding, }, kube::{ + Resource, core::{DeserializeGuard, error_boundary}, runtime::controller::Action, }, @@ -115,6 +116,11 @@ pub async fn reconcile_druid( ctx: Arc, ) -> Result { tracing::info!("Starting reconcile"); + + if druid.meta().deletion_timestamp.is_some() { + return Ok(Action::await_change()); + } + let druid = druid .0 .as_ref() @@ -173,9 +179,15 @@ mod test { use std::str::FromStr; use rstest::*; - use stackable_operator::v2::types::operator::RoleGroupName; + use stackable_operator::{ + client::Client, + commons::networking::DomainName, + kube::{Client as KubeClient, Config, runtime::controller::Action}, + utils::cluster_info::KubernetesClusterInfo, + v2::types::operator::RoleGroupName, + }; - use super::{CONTROLLER_NAME, OPERATOR_NAME, PRODUCT_NAME}; + use super::{CONTROLLER_NAME, OPERATOR_NAME, PRODUCT_NAME, *}; use crate::{ controller::build::{ properties::ConfigFileName, resource::config_map::build_rolegroup_config_map, @@ -254,4 +266,142 @@ mod test { "role group {tested_rolegroup_name}" ); } + + /// A [`Ctx`] whose client points at a closed port. Any API call made through it fails the + /// reconciliation, so an `Ok` result proves the reconciler returned before touching the + /// Kubernetes API. + fn unreachable_ctx() -> Arc { + let config = Config::new( + "http://127.0.0.1:1" + .parse::() + .expect("valid static URI"), + ); + let kube_client = KubeClient::try_from(config).expect("client from static config"); + + Arc::new(Ctx { + client: Client::new( + kube_client, + None, + "default".to_owned(), + KubernetesClusterInfo { + cluster_domain: DomainName::from_str("cluster.local") + .expect("valid cluster domain"), + }, + ), + operator_environment: OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_owned(), + operator_service_name: "druid-operator".to_owned(), + image_repository: "oci.stackable.tech/sdp".to_owned(), + }, + }) + } + + fn reconcile(druid: DeserializeGuard) -> Result { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("current-thread tokio runtime") + .block_on(async { reconcile_druid(Arc::new(druid), unreachable_ctx()).await }) + } + + #[test] + fn reconcile_exits_early_for_deleted_cluster() { + let druid = serde_yaml::from_str( + r#" +apiVersion: druid.stackable.tech/v1alpha1 +kind: DruidCluster +metadata: + name: druid + namespace: default + deletionTimestamp: "2026-08-14T12:00:00Z" +spec: + image: + productVersion: 3.2.2 +"#, + ) + .expect("valid cluster YAML"); + + let action = reconcile(druid).expect("a deleted cluster reconciles without any API call"); + + assert_eq!(action, Action::await_change()); + } + + #[test] + fn reconcile_exits_early_for_deleted_cluster_with_invalid_spec() { + let druid = serde_yaml::from_str( + r#" +apiVersion: druid.stackable.tech/v1alpha1 +kind: DruidCluster +metadata: + name: druid + namespace: default + deletionTimestamp: "2026-08-14T12:00:00Z" +spec: {} +"#, + ) + .expect("YAML parses; the invalid spec is captured inside the DeserializeGuard"); + + let action = + reconcile(druid).expect("a deleted cluster reconciles even when its spec is invalid"); + + assert_eq!(action, Action::await_change()); + } + + #[test] + fn reconcile_proceeds_for_live_cluster() { + // Without a deletion timestamp the reconciler must not exit early. + // `validate` resolves the uid, so the fixture needs one. The probe for + // "reached the API" is then the random Secret creation rather than the + // dereference step: dereference only contacts the API when for + // optional objects, whereas the random Secrets are always created. + let druid = serde_yaml::from_str( + r#" +apiVersion: druid.stackable.tech/v1alpha1 +kind: DruidCluster +metadata: + name: druid + namespace: default + uid: 12345678-1234-1234-1234-123456789012 +spec: + image: + productVersion: 3.2.2 + clusterConfig: + deepStorage: + hdfs: + configMapName: druid-hdfs + directory: /druid + metadataDatabase: + derby: {} + zookeeperConfigMapName: druid-znode + brokers: + roleGroups: + default: + replicas: 1 + coordinators: + roleGroups: + default: + replicas: 1 + historicals: + roleGroups: + default: + replicas: 1 + middleManagers: + roleGroups: + default: + replicas: 1 + routers: + roleGroups: + default: + replicas: 1 +"#, + ) + .expect("valid cluster YAML"); + + let result = reconcile(druid); + + assert!( + matches!(result, Err(Error::Dereference { .. })), + "a live cluster must reach the API but when dereferencing against the unreachable test server: {result:?}" + ); + } } diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index f3f8c0ef..cdd7de4a 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -15,7 +15,9 @@ use stackable_operator::{ eos::EndOfSupportChecker, k8s_openapi::api::{ apps::v1::StatefulSet, - core::v1::{ConfigMap, Service}, + core::v1::{ConfigMap, Secret, Service, ServiceAccount}, + policy::v1::PodDisruptionBudget, + rbac::v1::RoleBinding, }, kube::{ CustomResourceExt, ResourceExt, @@ -125,19 +127,35 @@ async fn main() -> anyhow::Result<()> { let config_map_store = druid_controller.store(); let druid_controller = druid_controller .owns( - watch_namespace.get_api::(&client), + watch_namespace.get_api::(&client), watcher::Config::default(), ) .owns( - watch_namespace.get_api::(&client), + watch_namespace.get_api::(&client), watcher::Config::default(), ) .owns( - watch_namespace.get_api::(&client), + watch_namespace.get_api::(&client), watcher::Config::default(), ) .owns( - watch_namespace.get_api::(&client), + watch_namespace.get_api::(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::(&client), watcher::Config::default(), ) .watches( diff --git a/tests/templates/kuttl/cluster-operation/70-assert.yaml b/tests/templates/kuttl/cluster-operation/70-assert.yaml new file mode 100644 index 00000000..ee4e3756 --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/70-assert.yaml @@ -0,0 +1,296 @@ +--- +# The recreated StatefulSets must bring the cluster back to ready, and the recreated +# objects must carry an owner reference back to the DruidCluster so that garbage +# collection still works for them. +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +metadata: + name: recreate-owned-resources +timeout: 600 +commands: + - script: kubectl -n $NAMESPACE wait --for=condition=available druidclusters.druid.stackable.tech/derby-druid --timeout 601s +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: derby-druid-broker-default + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: derby-druid-coordinator-default + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: derby-druid-historical-default + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: derby-druid-middlemanager-default + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: derby-druid-router-default + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: v1 +kind: Secret +metadata: + name: derby-druid-shared-internal-secret + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: derby-druid-serviceaccount + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: derby-druid-rolebinding + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: derby-druid-broker + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: derby-druid-coordinator + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: derby-druid-historical + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: derby-druid-middlemanager + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: derby-druid-router + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + name: derby-druid-broker + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + name: derby-druid-coordinator + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + name: derby-druid-router + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: derby-druid + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-broker-default-headless + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-broker-default-metrics + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-coordinator-default-headless + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-coordinator-default-metrics + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-historical-default-headless + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-historical-default-metrics + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-middlemanager-default-headless + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-middlemanager-default-metrics + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-router-default-headless + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid +--- +apiVersion: v1 +kind: Service +metadata: + name: derby-druid-router-default-metrics + ownerReferences: + - apiVersion: druid.stackable.tech/v1alpha1 + controller: true + kind: DruidCluster + name: derby-druid diff --git a/tests/templates/kuttl/cluster-operation/70-delete-owned-resources.yaml b/tests/templates/kuttl/cluster-operation/70-delete-owned-resources.yaml new file mode 100644 index 00000000..0d9ff163 --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/70-delete-owned-resources.yaml @@ -0,0 +1,72 @@ +# Every resource the operator applies carries an ownerReference and a `.owns()` watch +# (main.rs); deleting it must trigger a reconcile of the DruidCluster, which re-applies +# it. This step checks that chain — the ClusterRole's `watch` verbs plus the `.owns()` +# routing — and lives here because this test already cycles the pods. The `.watches()` +# registration (ConfigMaps referenced but not owned: ZooKeeper, OPA, HDFS) can't be +# tested by deletion: the operator never recreates what it didn't apply. +# +# Recreation is proven by UID change: mere existence could pass without any deletion. +# TestStep commands run exactly once (no kuttl retry loop), so the polling stays quiet. +# +# The ConfigMap deleted here is the discovery one, named after the cluster. +# +# The shared internal Secret is deleted first, before anything else, on purpose: every other +# deletion below triggers a reconcile that re-emits it, so testing it later would pass whether +# or not `.owns(Secret)` is registered. Going first, the only thing that can bring it back is +# its own watch. +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +metadata: + name: delete-owned-resources +timeout: 300 +commands: + - script: | + set -eu + + delete_and_await_recreation() { + kind=$1 + name=$2 + old_uid=$(kubectl get -n "$NAMESPACE" "$kind" "$name" -o jsonpath='{.metadata.uid}') + kubectl delete -n "$NAMESPACE" "$kind" "$name" --wait=false + # Recreation is a single reconcile away, so this normally succeeds on the + # first iteration; 30s is a generous upper bound well below the step timeout. + for _ in $(seq 1 30); do + new_uid=$(kubectl get -n "$NAMESPACE" "$kind" "$name" -o jsonpath='{.metadata.uid}' 2>/dev/null || true) + if [ -n "$new_uid" ] && [ "$new_uid" != "$old_uid" ]; then + return 0 + fi + sleep 1 + done + echo "$kind/$name was not recreated (old uid: $old_uid, current: '${new_uid:-}')" >&2 + return 1 + } + + delete_and_await_recreation secret derby-druid-shared-internal-secret + delete_and_await_recreation serviceaccount derby-druid-serviceaccount + delete_and_await_recreation rolebinding derby-druid-rolebinding + delete_and_await_recreation poddisruptionbudget derby-druid-broker + delete_and_await_recreation poddisruptionbudget derby-druid-coordinator + delete_and_await_recreation poddisruptionbudget derby-druid-historical + delete_and_await_recreation poddisruptionbudget derby-druid-middlemanager + delete_and_await_recreation poddisruptionbudget derby-druid-router + # Group Listeners exist for the externally reachable roles only. + delete_and_await_recreation listener derby-druid-broker + delete_and_await_recreation listener derby-druid-coordinator + delete_and_await_recreation listener derby-druid-router + delete_and_await_recreation configmap derby-druid + delete_and_await_recreation service derby-druid-broker-default-headless + delete_and_await_recreation service derby-druid-broker-default-metrics + delete_and_await_recreation service derby-druid-coordinator-default-headless + delete_and_await_recreation service derby-druid-coordinator-default-metrics + delete_and_await_recreation service derby-druid-historical-default-headless + delete_and_await_recreation service derby-druid-historical-default-metrics + delete_and_await_recreation service derby-druid-middlemanager-default-headless + delete_and_await_recreation service derby-druid-middlemanager-default-metrics + delete_and_await_recreation service derby-druid-router-default-headless + delete_and_await_recreation service derby-druid-router-default-metrics + delete_and_await_recreation statefulset derby-druid-broker-default + delete_and_await_recreation statefulset derby-druid-coordinator-default + delete_and_await_recreation statefulset derby-druid-historical-default + delete_and_await_recreation statefulset derby-druid-middlemanager-default + delete_and_await_recreation statefulset derby-druid-router-default