From 317205ef41e7e6e6b1c387bb8ecca9ba6dd065b1 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 09:52:06 +0200 Subject: [PATCH 1/3] add missing watches/rbac permissions and add tests for reconcile early-exit --- Cargo.lock | 1 + Cargo.nix | 6 ++ Cargo.toml | 1 + .../templates/clusterrole-operator.yaml | 24 ++--- rust/operator-binary/Cargo.toml | 3 + rust/operator-binary/src/main.rs | 17 +++- rust/operator-binary/src/opa_controller.rs | 74 +++++++++++++++ .../kuttl/cluster-operation/50-assert.yaml | 91 +++++++++++++++++++ .../50-delete-owned-resources.yaml | 61 +++++++++++++ 9 files changed, 258 insertions(+), 20 deletions(-) create mode 100644 tests/templates/kuttl/cluster-operation/50-assert.yaml create mode 100644 tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml diff --git a/Cargo.lock b/Cargo.lock index 4e10b19c..4d971d79 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3634,6 +3634,7 @@ dependencies = [ "semver", "serde", "serde_json", + "serde_yaml", "snafu 0.9.2", "stackable-operator", "strum", diff --git a/Cargo.nix b/Cargo.nix index 86e4fc63..6ed742a4 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -12027,6 +12027,12 @@ rec { features = [ "chrono" "git2" ]; } ]; + devDependencies = [ + { + name = "serde_yaml"; + packageId = "serde_yaml"; + } + ]; }; "stackable-opa-regorule-library" = rec { diff --git a/Cargo.toml b/Cargo.toml index dc88e33a..9ed3f6fe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ rustls-pki-types = "1.15" semver = "1.0" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +serde_yaml = "0.9" snafu = "0.9" strum = { version = "0.28", features = ["derive"] } tar = "0.4" diff --git a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml index 5805576d..68fea7a6 100644 --- a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml @@ -15,13 +15,15 @@ rules: - nodes/proxy verbs: - get - # Manage core workload resources created per OpaCluster. - # All resources are applied via Server-Side Apply (create + patch) and tracked for - # orphan cleanup (list + delete). + # Manage core workload resources created per OpaCluster (the ServiceAccount provides + # workload pod identity). All resources are applied via Server-Side Apply + # (create + patch), tracked for orphan cleanup (list + delete) and watched by the + # controller. - apiGroups: - "" resources: - configmaps + - serviceaccounts - services verbs: - create @@ -30,20 +32,9 @@ rules: - list - patch - watch - # ServiceAccount created per OpaCluster for workload pod identity. - # Applied via SSA and tracked for orphan cleanup. - - apiGroups: - - "" - resources: - - serviceaccounts - verbs: - - create - - delete - - get - - list - - patch # RoleBinding created per OpaCluster to bind the product ClusterRole to the workload - # ServiceAccount. Applied via SSA and tracked for orphan cleanup. + # ServiceAccount. Applied via SSA and tracked for orphan cleanup and watched by the + # controller. - apiGroups: - rbac.authorization.k8s.io resources: @@ -54,6 +45,7 @@ rules: - get - list - patch + - watch # Required to bind the product ClusterRole to the per-cluster ServiceAccount. - apiGroups: - rbac.authorization.k8s.io diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index 86d854db..af3a50ef 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -29,3 +29,6 @@ tracing.workspace = true [build-dependencies] built.workspace = true + +[dev-dependencies] +serde_yaml.workspace = true diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index f87b75c1..3e268c4f 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -13,7 +13,8 @@ use stackable_operator::{ eos::EndOfSupportChecker, k8s_openapi::api::{ apps::v1::DaemonSet, - core::v1::{ConfigMap, Service}, + core::v1::{ConfigMap, Service, ServiceAccount}, + rbac::v1::RoleBinding, }, kube::{ CustomResourceExt as _, @@ -148,15 +149,23 @@ async fn main() -> anyhow::Result<()> { let controller = 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), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::(&client), watcher::Config::default(), ) .graceful_shutdown_on(sigterm_watcher.handle()) diff --git a/rust/operator-binary/src/opa_controller.rs b/rust/operator-binary/src/opa_controller.rs index f46d4beb..0ad30c13 100644 --- a/rust/operator-binary/src/opa_controller.rs +++ b/rust/operator-binary/src/opa_controller.rs @@ -14,6 +14,7 @@ use stackable_operator::{ cluster_resources::ClusterResourceApplyStrategy, constant, kube::{ + Resource, core::{DeserializeGuard, error_boundary}, runtime::controller::Action, }, @@ -88,6 +89,11 @@ pub async fn reconcile_opa( ctx: Arc, ) -> Result { tracing::info!("Starting reconcile"); + + if opa.meta().deletion_timestamp.is_some() { + return Ok(Action::await_change()); + } + let opa = opa .0 .as_ref() @@ -139,6 +145,14 @@ pub fn error_policy( #[cfg(test)] mod tests { + use std::str::FromStr; + + use stackable_operator::{ + client::Client, + commons::networking::DomainName, + kube::{Client as KubeClient, Config}, + }; + use super::*; #[test] @@ -148,4 +162,64 @@ mod tests { let _ = *OPERATOR_NAME; let _ = *CONTROLLER_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() { + // Building the kube client initialises rustls. kube enables its `ring` backend, + // but the direct `rustls` dependency also enables the default `aws-lc-rs` + // backend - with two candidates rustls refuses to auto-select one and panics. + // Install one explicitly, exactly as main() does. + let _ = rustls::crypto::ring::default_provider().install_default(); + + let opa = serde_yaml::from_str( + r#" +apiVersion: opa.stackable.tech/v1alpha2 +kind: OpaCluster +metadata: + name: opa + 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 cluster_info = KubernetesClusterInfo { + cluster_domain: DomainName::from_str("cluster.local") + .expect("valid cluster domain"), + }; + 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(), + cluster_info.clone(), + ), + opa_bundle_builder_image: "opa-bundle-builder".to_owned(), + user_info_fetcher_image: "user-info-fetcher".to_owned(), + cluster_info, + operator_environment: OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_owned(), + operator_service_name: "opa-operator".to_owned(), + image_repository: "oci.stackable.tech/sdp".to_owned(), + }, + }); + + reconcile_opa(Arc::new(opa), ctx).await + }) + .expect("a deleted cluster reconciles without any API call"); + + assert_eq!(action, Action::await_change()); + } } diff --git a/tests/templates/kuttl/cluster-operation/50-assert.yaml b/tests/templates/kuttl/cluster-operation/50-assert.yaml new file mode 100644 index 00000000..55fd90d2 --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/50-assert.yaml @@ -0,0 +1,91 @@ +--- +# The recreated DaemonSet must bring the cluster back to ready, and the recreated +# objects must carry an owner reference back to the OpaCluster 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 opaclusters.opa.stackable.tech/test-opa --timeout 601s +--- +apiVersion: apps/v1 +kind: DaemonSet +metadata: + name: test-opa-server-default + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: test-opa-serviceaccount + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: test-opa-rolebinding + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-opa + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-opa-server-default + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: v1 +kind: Service +metadata: + name: test-opa-server + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: v1 +kind: Service +metadata: + name: test-opa-server-default-headless + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa +--- +apiVersion: v1 +kind: Service +metadata: + name: test-opa-server-default-metrics + ownerReferences: + - apiVersion: opa.stackable.tech/v1alpha2 + controller: true + kind: OpaCluster + name: test-opa diff --git a/tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml b/tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml new file mode 100644 index 00000000..41cc7ad0 --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml @@ -0,0 +1,61 @@ +--- +# Every resource the operator applies carries an ownerReference and a `.owns()` watch +# (main.rs): deleting it must trigger a reconcile of the OpaCluster 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 OpaCluster; 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. +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=test-opa,app.kubernetes.io/managed-by=opa.stackable.tech_opacluster" + excluded="^(pods|persistentvolumeclaims|endpoints|events)$|^endpointslices\.|^controllerrevisions\.|^events\." + + deleted=0 + for kind in $(kubectl api-resources --verbs=list --namespaced -o name | grep -Ev "$excluded" | sort); do + 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" != "OpaCluster/test-opa" ]; then + echo "skipping $resource: controller owner is '${owner:-none}', not the OpaCluster" + continue + fi + delete_and_await_recreation "$resource" + deleted=$((deleted + 1)) + done + 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 6 ]; then + echo "only $deleted labelled resources were swept - the label selector is broken" >&2 + exit 1 + fi From 44175aedc2fc2408e9807f60e559e79df67abc30 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 09:54:32 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc9a33b1..cbdf768d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ All notable changes to this project will be documented in this file. the role-level Service (`-server`) and the discovery ConfigMap lose `app.kubernetes.io/role-group`, and the RBAC ServiceAccount and RoleBinding lose both labels (previously `none`). Anything selecting on these label values must be adjusted. All resources can be updated in place; no manual deletion is required ([#880]). +- The operator now watches all resources that it creates and early-exits the reconcile action when the + cluster is marked for deletion ([#882]). ### Fixed @@ -36,6 +38,7 @@ All notable changes to this project will be documented in this file. [#871]: https://github.com/stackabletech/opa-operator/pull/871 [#872]: https://github.com/stackabletech/opa-operator/pull/872 [#880]: https://github.com/stackabletech/opa-operator/pull/880 +[#882]: https://github.com/stackabletech/opa-operator/pull/882 ## [26.7.0] - 2026-07-21 From 06eb1282f141deeeabeb31f2314a92d49acd2a2f Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 10:25:13 +0200 Subject: [PATCH 3/3] use DeserializeGuard consistently --- rust/operator-binary/src/main.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index 3e268c4f..aa7e1ba2 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -149,23 +149,23 @@ async fn main() -> anyhow::Result<()> { let controller = 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), + watch_namespace.get_api::>(&client), watcher::Config::default(), ) .graceful_shutdown_on(sigterm_watcher.handle())