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/deploy/helm/druid-operator/templates/clusterrole-operator.yaml b/deploy/helm/druid-operator/templates/clusterrole-operator.yaml index 58b53a69..ce6f8fb3 100644 --- a/deploy/helm/druid-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/druid-operator/templates/clusterrole-operator.yaml @@ -13,46 +13,28 @@ rules: - nodes/proxy verbs: - get - # Manage core workload resources created per DruidCluster. - # All resources are applied via Server-Side Apply (create + patch) and tracked for - # orphan cleanup (list + delete). + # Manage core workload resources created per DruidCluster: ConfigMaps and Services, + # the shared internal authentication Secret (cookie passphrase and internal client + # password; no orphan cleanup — Kubernetes GC via owner reference instead), and the + # ServiceAccount providing workload pod identity. All are applied via Server-Side + # Apply (create + patch), tracked for orphan cleanup where applicable (list + + # delete) and watched by the controller (`.owns()` lists before it watches). - apiGroups: - "" resources: - configmaps - - services - verbs: - - create - - delete - - get - - list - - patch - - watch - # Shared internal authentication secret (cookie passphrase and internal client password). - # Orphan cleanup not needed (instead, Kubernetes GC via owner reference). - - apiGroups: - - "" - resources: - secrets - verbs: - - create - - delete - - get - - patch - # ServiceAccount created per DruidCluster for workload pod identity. - # Applied via SSA and tracked for orphan cleanup. Not watched by the controller. - - apiGroups: - - "" - resources: - serviceaccounts + - services verbs: - create - delete - 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 +45,7 @@ rules: - get - list - patch + - watch # Required to bind the product ClusterRole to the per-cluster ServiceAccount. - apiGroups: - rbac.authorization.k8s.io @@ -85,8 +68,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 +79,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/src/controller.rs b/rust/operator-binary/src/controller.rs index c6c2bad6..f5d9a509 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,54 @@ mod test { "role group {tested_rolegroup_name}" ); } + + /// The client points at a closed port, so any API call would fail the reconciliation: an `Ok` + /// proves that a cluster being deleted returns before the reconciler touches the Kubernetes + /// API, and because the spec is invalid, before the [`DeserializeGuard`] is unwrapped. + #[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: {} +"#, + ) + .expect("YAML parses; the invalid spec is captured inside the DeserializeGuard"); + + let action = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("current-thread tokio runtime") + .block_on(async { + let ctx = Arc::new(Ctx { + client: Client::new( + KubeClient::try_from(Config::new( + "http://127.0.0.1:1".parse().expect("valid static URI"), + )) + .expect("client from static config"), + 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(), + }, + }); + + reconcile_druid(Arc::new(druid), ctx).await + }) + .expect("a deleted cluster reconciles without any API call"); + + assert_eq!(action, Action::await_change()); + } } diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index f3f8c0ef..3fc26082 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), + 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), + 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(), ) .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..ac5a3a8a --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/70-delete-owned-resources.yaml @@ -0,0 +1,73 @@ +--- +# Every resource the operator applies carries an ownerReference and a `.owns()` watch +# (main.rs): deleting it must trigger a reconcile of the DruidCluster that re-applies it, +# proving the `.owns()` routing and the ClusterRole `watch` verbs end to end. +# `.watches()` registrations can't be tested this way: the operator never recreates +# what it didn't apply. +# +# Resources are discovered by label (ClusterResources::add enforces the labels on +# everything the operator applies), so new resources and kinds are covered +# automatically. Labels over-match on derived objects, so each match must also carry +# a controller ownerReference pointing at the DruidCluster; kinds that can never pass that +# gate are excluded up front. Recreation is proven by UID change, and a floor guard +# catches a selector that silently matches nothing. +# +# The shared internal Secret is swept first: any later deletion would re-emit it via +# its reconcile, masking a missing `.owns(Secret)`; the discovery loop excludes +# secrets so it is not swept twice. +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +metadata: + name: delete-owned-resources +timeout: 300 +commands: + - script: | + set -eu + + delete_and_await_recreation() { + resource=$1 + old_uid=$(kubectl get -n "$NAMESPACE" "$resource" -o jsonpath='{.metadata.uid}') + kubectl delete -n "$NAMESPACE" "$resource" --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" "$resource" -o jsonpath='{.metadata.uid}' 2>/dev/null || true) + if [ -n "$new_uid" ] && [ "$new_uid" != "$old_uid" ]; then + return 0 + fi + sleep 1 + done + echo "$resource was not recreated (old uid: $old_uid, current: '${new_uid:-}')" >&2 + return 1 + } + + selector="app.kubernetes.io/instance=derby-druid,app.kubernetes.io/managed-by=druid.stackable.tech_druidcluster" + excluded="^(pods|persistentvolumeclaims|endpoints|events|secrets)$|^endpointslices\.|^controllerrevisions\.|^events\." + + deleted=0 + + sweep_kind() { + kind=$1 + for resource in $(kubectl get -n "$NAMESPACE" "$kind" -l "$selector" -o name 2>/dev/null); do + owner=$(kubectl get -n "$NAMESPACE" "$resource" -o jsonpath='{.metadata.ownerReferences[?(@.controller==true)].kind}/{.metadata.ownerReferences[?(@.controller==true)].name}' 2>/dev/null || true) + if [ "$owner" != "DruidCluster/derby-druid" ]; then + echo "skipping $resource: controller owner is '${owner:-none}', not the DruidCluster" + continue + fi + delete_and_await_recreation "$resource" + deleted=$((deleted + 1)) + done + } + + sweep_kind secrets + + for kind in $(kubectl api-resources --verbs=list --namespaced -o name | grep -Ev "$excluded" | sort); do + sweep_kind "$kind" + done + + # Guard against the sweep silently matching nothing (wrong selector, renamed + # labels): the fixture is known to produce well over this many owned resources. + if [ "$deleted" -lt 15 ]; then + echo "only $deleted labelled resources were swept - the label selector is broken" >&2 + exit 1 + fi