From 851be762f8ec2d91390e94a13f83423d0994e034 Mon Sep 17 00:00:00 2001
From: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Date: Wed, 26 Apr 2023 10:47:33 +0200
Subject: [PATCH 1/5] Add support for 2.4.17, 2.5.3 and 2.5.3-hadoop3

---
 CHANGELOG.md                                        |  4 ++++
 docs/modules/hbase/partials/supported-versions.adoc |  3 +++
 tests/test-definition.yaml                          | 11 +++++------
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 511a4e1e..5cbbc772 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Added
+
+- Support for HBase `2.4.17`, `2.3.3` and `2.5.3-hadoop3` ([#XXX]).
+
 ### Changed
 
 - Operator-rs: `0.40.2` -> `0.41.0` ([#349]).
diff --git a/docs/modules/hbase/partials/supported-versions.adoc b/docs/modules/hbase/partials/supported-versions.adoc
index 91756f34..e9085c48 100644
--- a/docs/modules/hbase/partials/supported-versions.adoc
+++ b/docs/modules/hbase/partials/supported-versions.adoc
@@ -7,3 +7,6 @@
 - 2.4.9
 - 2.4.11
 - 2.4.12
+- 2.4.17
+- 2.5.3
+- 2.5.3-hadoop3
diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml
index 67088230..a4263198 100644
--- a/tests/test-definition.yaml
+++ b/tests/test-definition.yaml
@@ -2,17 +2,16 @@
 dimensions:
   - name: hbase
     values:
-      - 2.4.8-stackable0.0.0-dev
-      - 2.4.9-stackable0.0.0-dev
-      - 2.4.11-stackable0.0.0-dev
-      - 2.4.12-stackable0.0.0-dev
+      - 2.4.17-stackable0.0.0-dev
+      - 2.5.3-stackable0.0.0-dev
+      - 2.5.3-hadoop3-stackable0.0.0-dev
   - name: hbase-latest
     values:
-      - 2.4.12-stackable0.0.0-dev
+      - 2.5.3-hadoop3-stackable0.0.0-dev
   - name: hdfs
     values:
       - 3.2.2-stackable0.0.0-dev
-      - 3.3.3-stackable0.0.0-dev
+      - 3.3.4-stackable0.0.0-dev
   - name: hdfs-latest
     values:
       - 3.3.4-stackable0.0.0-dev

From 7fc187b3671f28ac47a617f2e6e351b96c3a80fb Mon Sep 17 00:00:00 2001
From: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Date: Wed, 26 Apr 2023 10:53:04 +0200
Subject: [PATCH 2/5] changelog

---
 CHANGELOG.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5cbbc772..112c6e3e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 ### Added
 
-- Support for HBase `2.4.17`, `2.3.3` and `2.5.3-hadoop3` ([#XXX]).
+- Support for HBase `2.4.17`, `2.3.3` and `2.5.3-hadoop3` ([#353]).
 
 ### Changed
 
@@ -14,6 +14,7 @@
 
 [#349]: https://github.com/stackabletech/hbase-operator/pull/349
 [#351]: https://github.com/stackabletech/hbase-operator/pull/351
+[#353]: https://github.com/stackabletech/hbase-operator/pull/353
 
 ## [23.4.0] - 2023-04-17
 

From d031f8f8ab0dbf706c8d3747da0e0e7955f61827 Mon Sep 17 00:00:00 2001
From: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Date: Thu, 27 Apr 2023 08:41:09 +0200
Subject: [PATCH 3/5] Add test on config merging

---
 rust/crd/src/lib.rs | 68 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs
index 6ead072b..e3ec91f6 100644
--- a/rust/crd/src/lib.rs
+++ b/rust/crd/src/lib.rs
@@ -461,3 +461,71 @@ impl HbaseCluster {
         fragment::validate(conf_rolegroup).context(FragmentValidationFailureSnafu)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_config_merging() {
+        let input = r#"
+        apiVersion: hbase.stackable.tech/v1alpha1
+        kind: HbaseCluster
+        metadata:
+          name: simple-hbase
+        spec:
+          image:
+            productVersion: 2.4.12
+            stackableVersion: "23.4"
+          clusterConfig:
+            hdfsConfigMapName: simple-hdfs
+            zookeeperConfigMapName: simple-znode
+          masters:
+            config:
+              logging:
+                enableVectorAgent: true
+            roleGroups:
+              default:
+                replicas: 1
+              disable-logging:
+                replicas: 1
+                config:
+                  logging:
+                    enableVectorAgent: false
+          regionServers:
+            roleGroups:
+              default:
+                replicas: 1
+          restServers:
+            roleGroups:
+              default:
+                replicas: 1
+        "#;
+        let hbase: HbaseCluster = serde_yaml::from_str(input).expect("illegal test input");
+        let master_default_config = hbase
+            .merged_config(
+                &HbaseRole::Master,
+                "default",
+                &hbase.spec.cluster_config.hdfs_config_map_name,
+            )
+            .unwrap();
+        let master_disable_logging_config = hbase
+            .merged_config(
+                &HbaseRole::Master,
+                "disable-logging",
+                &hbase.spec.cluster_config.hdfs_config_map_name,
+            )
+            .unwrap();
+        let region_server_default_config = hbase
+            .merged_config(
+                &HbaseRole::RegionServer,
+                "default",
+                &hbase.spec.cluster_config.hdfs_config_map_name,
+            )
+            .unwrap();
+
+        assert!(master_default_config.logging.enable_vector_agent);
+        assert!(!master_disable_logging_config.logging.enable_vector_agent);
+        assert!(!region_server_default_config.logging.enable_vector_agent);
+    }
+}

From bad3461f60ca96917471f3ccf895e5608a257191 Mon Sep 17 00:00:00 2001
From: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Date: Thu, 27 Apr 2023 13:49:22 +0200
Subject: [PATCH 4/5] Fix logging in 2.5.x by using log4j2

---
 rust/operator-binary/src/hbase_controller.rs |  7 +-
 rust/operator-binary/src/product_logging.rs  | 86 ++++++++++++++++++--
 2 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/rust/operator-binary/src/hbase_controller.rs b/rust/operator-binary/src/hbase_controller.rs
index 6b13de00..382038c2 100644
--- a/rust/operator-binary/src/hbase_controller.rs
+++ b/rust/operator-binary/src/hbase_controller.rs
@@ -2,9 +2,7 @@
 
 use crate::{
     discovery::build_discovery_configmap,
-    product_logging::{
-        extend_role_group_config_map, resolve_vector_aggregator_address, LOG4J_CONFIG_FILE,
-    },
+    product_logging::{extend_role_group_config_map, resolve_vector_aggregator_address},
     OPERATOR_NAME,
 };
 
@@ -490,6 +488,7 @@ fn build_rolegroup_config_map(
         .add_data(HBASE_ENV_SH, write_hbase_env_sh(hbase_env_config.iter()));
 
     extend_role_group_config_map(
+        &resolved_product_image.product_version,
         rolegroup,
         vector_aggregator_address,
         &config.logging,
@@ -649,7 +648,7 @@ fn build_rolegroup_statefulset(
                 HDFS_DISCOVERY_TMP_DIR, CONFIG_DIR_NAME
             ),
             format!("cp {}/* {}", HBASE_CONFIG_TMP_DIR, CONFIG_DIR_NAME),
-            format!("cp {HBASE_LOG_CONFIG_TMP_DIR}/{LOG4J_CONFIG_FILE} {CONFIG_DIR_NAME}",),
+            format!("cp {HBASE_LOG_CONFIG_TMP_DIR}/log4j*.properties {CONFIG_DIR_NAME}"),
             format!(
                 "bin/hbase {} start",
                 match role {
diff --git a/rust/operator-binary/src/product_logging.rs b/rust/operator-binary/src/product_logging.rs
index 7f73a49f..58c71c30 100644
--- a/rust/operator-binary/src/product_logging.rs
+++ b/rust/operator-binary/src/product_logging.rs
@@ -7,7 +7,9 @@ use stackable_operator::{
     kube::ResourceExt,
     product_logging::{
         self,
-        spec::{ContainerLogConfig, ContainerLogConfigChoice, Logging},
+        spec::{
+            AutomaticContainerLogConfig, ContainerLogConfig, ContainerLogConfigChoice, Logging,
+        },
     },
     role_utils::RoleGroupRef,
 };
@@ -38,8 +40,63 @@ type Result<T, E = Error> = std::result::Result<T, E>;
 
 const VECTOR_AGGREGATOR_CM_ENTRY: &str = "ADDRESS";
 const CONSOLE_CONVERSION_PATTERN: &str = "%d{ISO8601} %-5p [%t] %c{2}: %.1000m%n";
-const HBASE_LOG_FILE: &str = "hbase.log4j.xml";
-pub const LOG4J_CONFIG_FILE: &str = "log4j.properties";
+
+// TODO: Move to operator-rs when other products switch from log4j1 to log4j2.
+#[derive(Debug, PartialEq, Eq)]
+pub enum Log4JVersion {
+    Log4J1,
+    Log4J2,
+}
+
+impl Log4JVersion {
+    fn config_file_name(&self) -> &str {
+        match &self {
+            Log4JVersion::Log4J1 => "log4j.properties",
+            Log4JVersion::Log4J2 => "log4j2.properties",
+        }
+    }
+
+    fn log_file_name(&self) -> &str {
+        match &self {
+            Log4JVersion::Log4J1 => "hbase.log4j.xml",
+            Log4JVersion::Log4J2 => "hbase.log4j2.xml",
+        }
+    }
+
+    fn create_log_config(
+        &self,
+        log_dir: &str,
+        max_size_in_mib: u32,
+        console_conversion_pattern: &str,
+        config: &AutomaticContainerLogConfig,
+    ) -> String {
+        match &self {
+            Log4JVersion::Log4J1 => product_logging::framework::create_log4j_config(
+                log_dir,
+                self.log_file_name(),
+                max_size_in_mib,
+                console_conversion_pattern,
+                config,
+            ),
+            Log4JVersion::Log4J2 => product_logging::framework::create_log4j2_config(
+                log_dir,
+                self.log_file_name(),
+                max_size_in_mib,
+                console_conversion_pattern,
+                config,
+            ),
+        }
+    }
+}
+
+fn calculate_log4j_version(product_version: &str) -> Log4JVersion {
+    // The first version we support is 2.4.x, so we don't need to care about versions below that
+    if product_version.starts_with("2.4.") {
+        Log4JVersion::Log4J1
+    } else {
+        Log4JVersion::Log4J2
+    }
+}
 
 /// Return the address of the Vector aggregator if the corresponding ConfigMap name is given in the
 /// cluster spec
@@ -78,6 +135,7 @@ pub async fn resolve_vector_aggregator_address(
 
 /// Extend the role group ConfigMap with logging and Vector configurations
 pub fn extend_role_group_config_map(
+    product_version: &str,
     rolegroup: &RoleGroupRef<HbaseCluster>,
     vector_aggregator_address: Option<&str>,
     logging: &Logging<Container>,
@@ -87,11 +145,11 @@ pub fn extend_role_group_config_map(
         choice: Some(ContainerLogConfigChoice::Automatic(log_config)),
     }) = logging.containers.get(&Container::Hbase)
     {
+        let log4j_version = calculate_log4j_version(product_version);
         cm_builder.add_data(
-            LOG4J_CONFIG_FILE,
-            product_logging::framework::create_log4j_config(
+            log4j_version.config_file_name(),
+            log4j_version.create_log_config(
                 &format!("{STACKABLE_LOG_DIR}/hbase"),
-                HBASE_LOG_FILE,
                 MAX_HBASE_LOG_FILES_SIZE_IN_MIB,
                 CONSOLE_CONVERSION_PATTERN,
                 log_config,
@@ -121,3 +179,19 @@ pub fn extend_role_group_config_map(
 
     Ok(())
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_calculate_log4j_version() {
+        assert_eq!(calculate_log4j_version("2.4.0"), Log4JVersion::Log4J1);
+        assert_eq!(calculate_log4j_version("2.4.17"), Log4JVersion::Log4J1);
+        assert_eq!(calculate_log4j_version("2.5.0"), Log4JVersion::Log4J2);
+        assert_eq!(calculate_log4j_version("2.5.1"), Log4JVersion::Log4J2);
+        assert_eq!(calculate_log4j_version("2.6.0"), Log4JVersion::Log4J2);
+        assert_eq!(calculate_log4j_version("3.0.0"), Log4JVersion::Log4J2);
+        assert_eq!(calculate_log4j_version("42.43.44"), Log4JVersion::Log4J2);
+    }
+}

From 64767b991d74865472b7154b4a576071a601d20c Mon Sep 17 00:00:00 2001
From: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Date: Tue, 2 May 2023 09:07:44 +0200
Subject: [PATCH 5/5] typo

---
 CHANGELOG.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e4892408..38b0a4ae 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 ### Added
 
-- Support for HBase `2.4.17`, `2.3.3` and `2.5.3-hadoop3` ([#353]).
+- Support for HBase `2.4.17`, `2.5.3` and `2.5.3-hadoop3` ([#353]).
 
 ### Changed