Allow overriding ZNode path (#799)

* Add znodePath field to ZookeeperZnode status * Error handling * Docs, update CRD YAML * Changelog * Add znode override test * Documentation * Add missing doc start marker * fix: use module level error * chore: apply clippy suggestion * chore: apply markdown lint suggestion * Update CHANGELOG.md Co-authored-by: Sebastian Bernauer <[email protected]> --------- Co-authored-by: Nick Larsen <[email protected]> Co-authored-by: Sebastian Bernauer <[email protected]>
stackabletech · Jun 20, 2024 · f11c86a · f11c86a
1 parent f6f416e
commit f11c86a
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+### Added
+
+- Allow overriding ZNode path by setting `status.znodePath` ([#799]).
+
 ### Changed
 
 - Bump `built`, `clap`, `rstest`, `stackable-operator` and `strum`
@@ -14,6 +18,7 @@ All notable changes to this project will be documented in this file.
 - Processing of corrupted log events fixed; If errors occur, the error
   messages are added to the log event ([#821]).
 
+[#799]: https://github.com/stackabletech/zookeeper-operator/pull/799
 [#812]: https://github.com/stackabletech/zookeeper-operator/pull/812
 [#821]: https://github.com/stackabletech/zookeeper-operator/pull/821
 

diff --git a/deploy/helm/zookeeper-operator/crds/crds.yaml b/deploy/helm/zookeeper-operator/crds/crds.yaml
@@ -7561,10 +7561,22 @@ spec:
                       type: string
                   type: object
               type: object
+            status:
+              nullable: true
+              properties:
+                znodePath:
+                  description: |-
+                    The absolute ZNode allocated to the ZookeeperZnode. This will typically be set by the operator.
+
+                    This can be set explicitly by an administrator, such as when restoring from a backup.
+                  nullable: true
+                  type: string
+              type: object
           required:
             - spec
           title: ZookeeperZnode
           type: object
       served: true
       storage: true
-      subresources: {}
+      subresources:
+        status: {}
diff --git a/deploy/helm/zookeeper-operator/templates/roles.yaml b/deploy/helm/zookeeper-operator/templates/roles.yaml
@@ -120,6 +120,7 @@ rules:
       - {{ include "operator.name" . }}.stackable.tech
     resources:
       - {{ include "operator.name" . }}clusters/status
+      - {{ include "operator.name" . }}znodes/status
     verbs:
       - patch
 {{ if .Capabilities.APIVersions.Has "security.openshift.io/v1" }}

diff --git a/docs/modules/zookeeper/pages/usage_guide/isolating_clients_with_znodes.adoc b/docs/modules/zookeeper/pages/usage_guide/isolating_clients_with_znodes.adoc
@@ -100,3 +100,20 @@ The Stackable Operators for Kafka and Druid use the discovery ConfigMaps to conn
 == What's next
 
 You can find out more about the discovery ConfigMap xref:discovery.adoc[] and the xref:znodes.adoc[] in the concepts documentation.
+
+== Restoring from backups
+
+For security reasons, a unique ZNode path is generated every time the same ZookeeperZnode object is recreated, even if it has the same name.
+
+If a ZookeeperZnode needs to be associated with an existing ZNode path, the field `status.znodePath` can be set to
+the desired path. Note that since this is a subfield of `status`, it must explicitly be updated on the `status` subresource,
+and requires RBAC permissions to replace the `zookeeperznodes/status` resource. For example:
+
+[source,bash]
+----
+kubectl get zookeeperznode/test-znode -o json -n $NAMESPACE \
+        | jq '.status.znodePath = "/znode-override"' \
+        | kubectl replace -f- --subresource=status
+----
+
+NOTE: The auto-generated ZNode will still be kept, and should be cleaned up by an administrator.
diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs
@@ -466,16 +466,14 @@ impl ZookeeperCluster {
             "1.", "2.", "3.0.", "3.1.", "3.2.", "3.3.", "3.4.", "3.5.", "3.6.", "3.7.",
         ];
 
-        let framework = if zookeeper_versions_with_log4j
+        if zookeeper_versions_with_log4j
             .into_iter()
             .any(|prefix| version.starts_with(prefix))
         {
             LoggingFramework::LOG4J
         } else {
             LoggingFramework::LOGBACK
-        };
-
-        framework
+        }
     }
 
     /// The name of the role-level load-balanced Kubernetes `Service`
@@ -695,6 +693,7 @@ impl ZookeeperPodRef {
     plural = "zookeeperznodes",
     shortname = "zno",
     shortname = "znode",
+    status = "ZookeeperZnodeStatus",
     namespaced,
     crates(
         kube_core = "stackable_operator::kube::core",
@@ -709,6 +708,15 @@ pub struct ZookeeperZnodeSpec {
     pub cluster_ref: ClusterRef<ZookeeperCluster>,
 }
 
+#[derive(Clone, Default, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ZookeeperZnodeStatus {
+    /// The absolute ZNode allocated to the ZookeeperZnode. This will typically be set by the operator.
+    ///
+    /// This can be set explicitly by an administrator, such as when restoring from a backup.
+    pub znode_path: Option<String>,
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

diff --git a/rust/operator-binary/src/znode_controller.rs b/rust/operator-binary/src/znode_controller.rs
@@ -1,7 +1,7 @@
 //! Reconciles state for ZooKeeper znodes between Kubernetes [`ZookeeperZnode`] objects and the ZooKeeper cluster
 //!
 //! See [`ZookeeperZnode`] for more details.
-use std::{convert::Infallible, sync::Arc};
+use std::{borrow::Cow, convert::Infallible, sync::Arc};
 
 use snafu::{OptionExt, ResultExt, Snafu};
 use stackable_operator::{
@@ -19,9 +19,11 @@ use stackable_operator::{
     time::Duration,
 };
 use stackable_zookeeper_crd::{
-    security::ZookeeperSecurity, ZookeeperCluster, ZookeeperZnode, DOCKER_IMAGE_BASE_NAME,
+    security::ZookeeperSecurity, ZookeeperCluster, ZookeeperZnode, ZookeeperZnodeStatus,
+    DOCKER_IMAGE_BASE_NAME,
 };
 use strum::{EnumDiscriminants, IntoStaticStr};
+use tracing::{debug, info};
 
 use crate::{
     discovery::{self, build_discovery_configmaps},
@@ -92,6 +94,11 @@ pub enum Error {
         cm: ObjectRef<ConfigMap>,
     },
 
+    #[snafu(display("failed to update status"))]
+    ApplyStatus {
+        source: stackable_operator::client::Error,
+    },
+
     #[snafu(display("error managing finalizer"))]
     Finalizer {
         source: finalizer::Error<Infallible>,
@@ -148,6 +155,7 @@ impl ReconcilerError for Error {
             Error::EnsureZnodeMissing { zk, .. } => Some(zk.clone().erase()),
             Error::BuildDiscoveryConfigMap { source: _ } => None,
             Error::ApplyDiscoveryConfigMap { cm, .. } => Some(cm.clone().erase()),
+            Error::ApplyStatus { .. } => None,
             Error::Finalizer { source: _ } => None,
             Error::DeleteOrphans { source: _ } => None,
             Error::ObjectHasNoNamespace => None,
@@ -174,14 +182,40 @@ pub async fn reconcile_znode(
     let client = &ctx.client;
 
     let zk = find_zk_of_znode(client, &znode).await;
-    // Use the uid (managed by k8s itself) rather than the object name, to ensure that malicious users can't trick the controller
-    // into letting them take over a znode owned by someone else
-    let znode_path = format!("/znode-{}", uid);
+    let mut default_status_updates: Option<ZookeeperZnodeStatus> = None;
+    // Store the znode path in the status rather than the object itself, to ensure that only K8s administrators can override it
+    let znode_path = match znode.status.as_ref().and_then(|s| s.znode_path.as_deref()) {
+        Some(znode_path) => {
+            debug!(znode.path = znode_path, "Using configured znode path");
+            Cow::Borrowed(znode_path)
+        }
+        None => {
+            // Default to the uid (managed by k8s itself) rather than the object name, to ensure that malicious users can't trick the controller
+            // into letting them take over a znode owned by someone else
+            let znode_path = format!("/znode-{}", uid);
+            info!(
+                znode.path = znode_path,
+                "No znode path set, setting to default"
+            );
+            default_status_updates
+                .get_or_insert_with(Default::default)
+                .znode_path = Some(znode_path.clone());
+            Cow::Owned(znode_path)
+        }
+    };
+
+    if let Some(status) = default_status_updates {
+        info!("Writing default configuration to status");
+        ctx.client
+            .merge_patch_status(&*znode, &status)
+            .await
+            .context(ApplyStatusSnafu)?;
+    }
 
     finalizer(
         &client.get_api::<ZookeeperZnode>(&ns),
         &format!("{OPERATOR_NAME}/znode"),
-        znode,
+        znode.clone(),
         |ev| async {
             match ev {
                 finalizer::Event::Apply(znode) => {

diff --git a/tests/templates/kuttl/znode/01-assert.yaml b/tests/templates/kuttl/znode/01-assert.yaml
@@ -0,0 +1,10 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-znode
+data:
+  ZOOKEEPER_CHROOT: /znode-override
diff --git a/tests/templates/kuttl/znode/01-set-znode-override.yaml b/tests/templates/kuttl/znode/01-set-znode-override.yaml
@@ -0,0 +1,5 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: kubectl get zookeeperznode/test-znode -o json -n $NAMESPACE | jq '.status.znodePath = "/znode-override"' | kubectl replace -f- --subresource=status