wip: adress feedback on pr

labrenbe · labrenbe · commit 25745d11b798 · 2025-11-06T11:53:18.000+01:00
diff --git a/deploy/helm/opensearch-operator/crds/crds.yaml b/deploy/helm/opensearch-operator/crds/crds.yaml
@@ -31,21 +31,37 @@ spec:
                 clusterConfig:
                   default:
                     tls:
-                      secretClass: null
+                      restSecretClass: tls
+                      transportSecretClass: tls
                   description: Configuration that applies to all roles and role groups
                   properties:
                     tls:
+                      default:
+                        restSecretClass: tls
+                        transportSecretClass: tls
+                      description: TLS configuration options for the REST API and internal communication (transport).
                       properties:
-                        secretClass:
+                        restSecretClass:
+                          default: tls
                           description: |-
-                            Affects client connections and internal transport connections.
+                            Only affects client connections to the REST API.
                             This setting controls:
                             - If TLS encryption is used at all
                             - Which cert the servers should use to authenticate themselves against the client
-                          maxLength: 223
+                          maxLength: 253
                           minLength: 1
                           nullable: true
                           type: string
+                        transportSecretClass:
+                          default: tls
+                          description: |-
+                            Only affects internal communication (transport). Used for mutual verification between OpenSearch nodes.
+                            This setting controls:
+                            - Which cert the servers should use to authenticate themselves against other servers
+                            - Which ca.crt to use when validating the other server
+                          maxLength: 253
+                          minLength: 1
+                          type: string
                       type: object
                     vectorAggregatorConfigMapName:
                       description: |-
@@ -57,8 +73,6 @@ spec:
                       minLength: 1
                       nullable: true
                       type: string
-                  required:
-                    - tls
                   type: object
                 clusterOperation:
                   default:
diff --git a/rust/operator-binary/src/controller/build/node_config.rs b/rust/operator-binary/src/controller/build/node_config.rs
@@ -97,7 +97,7 @@ pub const CONFIG_OPTION_PLUGINS_SECURITY_SSL_TRANSPORT_PEMTRUSTEDCAS_FILEPATH: &
 pub struct NodeConfig {
     cluster: ValidatedCluster,
     role_group_config: OpenSearchRoleGroupConfig,
-    discovery_service_name: ServiceName,
+    pub discovery_service_name: ServiceName,
 }
 
 // Most functions are public because their configuration values could also be used in environment
@@ -123,9 +123,7 @@ impl NodeConfig {
     pub fn opensearch_config(&self) -> serde_json::Map<String, Value> {
         let mut config = self.static_opensearch_config();
 
-        if self.cluster.cluster_config.tls.secret_class.is_some() {
-            config.append(&mut self.tls_config());
-        }
+        config.append(&mut self.tls_config());
 
         for (setting, value) in self
             .role_group_config
@@ -176,41 +174,49 @@ impl NodeConfig {
     pub fn tls_config(&self) -> serde_json::Map<String, Value> {
         let mut config = serde_json::Map::new();
 
-        // TLS config for HTTP port
-        config.insert(
-            CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_ENABLED.to_owned(),
-            json!("true".to_string()),
-        );
-        config.insert(
-            CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_PEMCERT_FILEPATH.to_owned(),
-            json!("${OPENSEARCH_PATH_CONF}/tls/tls.crt".to_string()),
-        );
-        config.insert(
-            CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_PEMKEY_FILEPATH.to_owned(),
-            json!("${OPENSEARCH_PATH_CONF}/tls/tls.key".to_string()),
-        );
-        config.insert(
-            CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_PEMTRUSTEDCAS_FILEPATH.to_owned(),
-            json!("${OPENSEARCH_PATH_CONF}/tls/ca.crt".to_string()),
-        );
-        // TLS config for TRANSPORT port
+        // TLS config for TRANSPORT port which is always enabled.
         config.insert(
             CONFIG_OPTION_PLUGINS_SECURITY_SSL_TRANSPORT_ENABLED.to_owned(),
             json!("true".to_string()),
         );
         config.insert(
             CONFIG_OPTION_PLUGINS_SECURITY_SSL_TRANSPORT_PEMCERT_FILEPATH.to_owned(),
-            json!("${OPENSEARCH_PATH_CONF}/tls/tls.crt".to_string()),
+            json!("${OPENSEARCH_PATH_CONF}/tls/transport/tls.crt".to_string()),
         );
         config.insert(
             CONFIG_OPTION_PLUGINS_SECURITY_SSL_TRANSPORT_PEMKEY_FILEPATH.to_owned(),
-            json!("${OPENSEARCH_PATH_CONF}/tls/tls.key".to_string()),
+            json!("${OPENSEARCH_PATH_CONF}/tls/transport/tls.key".to_string()),
         );
         config.insert(
             CONFIG_OPTION_PLUGINS_SECURITY_SSL_TRANSPORT_PEMTRUSTEDCAS_FILEPATH.to_owned(),
-            json!("${OPENSEARCH_PATH_CONF}/tls/ca.crt".to_string()),
+            json!("${OPENSEARCH_PATH_CONF}/tls/transport/ca.crt".to_string()),
         );
 
+        // TLS config for HTTP port which is optional.
+        if self.cluster.cluster_config.tls.rest_secret_class.is_some() {
+            config.insert(
+                CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_ENABLED.to_owned(),
+                json!("true".to_string()),
+            );
+            config.insert(
+                CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_PEMCERT_FILEPATH.to_owned(),
+                json!("${OPENSEARCH_PATH_CONF}/tls/rest/tls.crt".to_string()),
+            );
+            config.insert(
+                CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_PEMKEY_FILEPATH.to_owned(),
+                json!("${OPENSEARCH_PATH_CONF}/tls/rest/tls.key".to_string()),
+            );
+            config.insert(
+                CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_PEMTRUSTEDCAS_FILEPATH.to_owned(),
+                json!("${OPENSEARCH_PATH_CONF}/tls/rest/ca.crt".to_string()),
+            );
+        } else {
+            config.insert(
+                CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_ENABLED.to_owned(),
+                json!("false".to_string()),
+            );
+        }
+
         config
     }
 
diff --git a/rust/operator-binary/src/controller/build/role_group_builder.rs b/rust/operator-binary/src/controller/build/role_group_builder.rs
@@ -72,7 +72,8 @@ constant!(DATA_VOLUME_NAME: VolumeName = "data");
 constant!(LISTENER_VOLUME_NAME: PersistentVolumeClaimName = "listener");
 const LISTENER_VOLUME_DIR: &str = "/stackable/listener";
 
-constant!(TLS_VOLUME_NAME: VolumeName = "tls");
+constant!(TLS_REST_VOLUME_NAME: VolumeName = "tls-rest");
+constant!(TLS_TRANSPORT_VOLUME_NAME: VolumeName = "tls-transport");
 
 constant!(LOG_VOLUME_NAME: VolumeName = "log");
 const LOG_VOLUME_DIR: &str = "/stackable/log";
@@ -215,10 +216,7 @@ impl<'a> RoleGroupBuilder<'a> {
     /// Builds the [`PodTemplateSpec`] for the role-group [`StatefulSet`]
     fn build_pod_template(&self) -> PodTemplateSpec {
         let mut node_role_labels = Labels::new();
-        let service_scopes = vec![
-            self.resource_names.cluster_name.to_string(),
-            self.resource_names.headless_service_name().to_string(),
-        ];
+        let service_scopes = vec![self.node_config.discovery_service_name.clone()];
 
         for node_role in self.role_group_config.config.node_roles.iter() {
             node_role_labels.insert(Self::build_node_role_label(node_role));
@@ -293,12 +291,21 @@ impl<'a> RoleGroupBuilder<'a> {
                 }),
                 ..Volume::default()
             },
+            build_tls_volume(
+                &TLS_TRANSPORT_VOLUME_NAME.to_string(),
+                &self.cluster.cluster_config.tls.transport_secret_class,
+                service_scopes.clone(),
+                SecretFormat::TlsPem,
+                &self.role_group_config.config.requested_secret_lifetime,
+                Some(&LISTENER_VOLUME_NAME.to_string()),
+            ),
         ];
 
-        if let Some(tls_secret_class_name) = &self.cluster.cluster_config.tls.secret_class {
+        if let Some(tls_rest_secret_class_name) = &self.cluster.cluster_config.tls.rest_secret_class
+        {
             volumes.push(build_tls_volume(
-                &TLS_VOLUME_NAME.to_string(),
-                tls_secret_class_name,
+                &TLS_REST_VOLUME_NAME.to_string(),
+                tls_rest_secret_class_name,
                 service_scopes,
                 SecretFormat::TlsPem,
                 &self.role_group_config.config.requested_secret_lifetime,
@@ -457,12 +464,17 @@ impl<'a> RoleGroupBuilder<'a> {
                 name: LOG_VOLUME_NAME.to_string(),
                 ..VolumeMount::default()
             },
+            VolumeMount {
+                mount_path: format!("{opensearch_path_conf}/tls/transport"),
+                name: TLS_TRANSPORT_VOLUME_NAME.to_string(),
+                ..VolumeMount::default()
+            },
         ];
 
-        if self.cluster.cluster_config.tls.secret_class.is_some() {
+        if self.cluster.cluster_config.tls.rest_secret_class.is_some() {
             volume_mounts.push(VolumeMount {
-                mount_path: format!("{opensearch_path_conf}/tls"),
-                name: TLS_VOLUME_NAME.to_string(),
+                mount_path: format!("{opensearch_path_conf}/tls/rest"),
+                name: TLS_REST_VOLUME_NAME.to_string(),
                 ..VolumeMount::default()
             })
         }
diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs
@@ -74,9 +74,6 @@ pub enum Error {
         source: crate::framework::product_logging::framework::Error,
     },
 
-    #[snafu(display("failed to set tls secret class"))]
-    ParseTlsSecretClassName { source: crate::framework::Error },
-
     #[snafu(display("fragment validation failure"))]
     ValidateOpenSearchConfig {
         source: stackable_operator::config::fragment::ValidationError,
@@ -282,7 +279,7 @@ mod tests {
         },
         framework::{
             ClusterName, ConfigMapName, ControllerName, ListenerClassName, NamespaceName,
-            OperatorName, ProductName, ProductVersion, RoleGroupName, TlsSecretClassName,
+            OperatorName, ProductName, ProductVersion, RoleGroupName, SecretClassName,
             builder::pod::container::{EnvVarName, EnvVarSet},
             product_logging::framework::{
                 ValidatedContainerLogConfigChoice, VectorContainerLogConfig,
@@ -311,7 +308,8 @@ mod tests {
                 uuid!("e6ac237d-a6d4-43a1-8135-f36506110912"),
                 OpenSearchClusterConfig {
                     tls: OpenSearchTls {
-                        secret_class: Some(TlsSecretClassName::from_str_unsafe("tls"))
+                        rest_secret_class: Some(SecretClassName::from_str_unsafe("tls")),
+                        transport_secret_class: SecretClassName::from_str_unsafe("tls")
                     },
                     vector_aggregator_config_map_name: Some(ConfigMapName::from_str_unsafe(
                         "vector-aggregator"
@@ -678,7 +676,8 @@ mod tests {
                     .expect("should be a valid ProductImage structure"),
                 cluster_config: v1alpha1::OpenSearchClusterConfig {
                     tls: OpenSearchTls {
-                        secret_class: Some(TlsSecretClassName::from_str_unsafe("tls")),
+                        rest_secret_class: Some(SecretClassName::from_str_unsafe("tls")),
+                        transport_secret_class: SecretClassName::from_str_unsafe("tls"),
                     },
                     vector_aggregator_config_map_name: Some(ConfigMapName::from_str_unsafe(
                         "vector-aggregator",
diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs
@@ -30,11 +30,12 @@ use crate::{
     constant,
     framework::{
         ClusterName, ConfigMapName, ContainerName, ListenerClassName, NameIsValidLabelValue,
-        ProductName, RoleName, TlsSecretClassName, role_utils::GenericProductSpecificCommonConfig,
+        ProductName, RoleName, SecretClassName, role_utils::GenericProductSpecificCommonConfig,
     },
 };
 
 constant!(DEFAULT_LISTENER_CLASS: ListenerClassName = "cluster-internal");
+constant!(TLS_DEFAULT_SECRET_CLASS: SecretClassName = "tls");
 
 #[versioned(
     version(name = "v1alpha1"),
@@ -80,6 +81,8 @@ pub mod versioned {
     #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
     #[serde(rename_all = "camelCase")]
     pub struct OpenSearchClusterConfig {
+        /// TLS configuration options for the REST API and internal communication (transport).
+        #[serde(default)]
         pub tls: OpenSearchTls,
         /// Name of the Vector aggregator [discovery ConfigMap](DOCS_BASE_URL_PLACEHOLDER/concepts/service_discovery).
         /// It must contain the key `ADDRESS` with the address of the Vector aggregator.
@@ -89,14 +92,24 @@ pub mod versioned {
         pub vector_aggregator_config_map_name: Option<ConfigMapName>,
     }
 
-    #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
+    #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
     #[serde(rename_all = "camelCase")]
     pub struct OpenSearchTls {
-        /// Affects client connections and internal transport connections.
+        /// Only affects client connections to the REST API.
         /// This setting controls:
         /// - If TLS encryption is used at all
         /// - Which cert the servers should use to authenticate themselves against the client
-        pub secret_class: Option<TlsSecretClassName>,
+        #[serde(
+            default = "rest_secret_class_default",
+            skip_serializing_if = "Option::is_none"
+        )]
+        pub rest_secret_class: Option<SecretClassName>,
+        /// Only affects internal communication (transport). Used for mutual verification between OpenSearch nodes.
+        /// This setting controls:
+        /// - Which cert the servers should use to authenticate themselves against other servers
+        /// - Which ca.crt to use when validating the other server
+        #[serde(default = "transport_secret_class_default")]
+        pub transport_secret_class: SecretClassName,
     }
 
     // The possible node roles are by default the built-in roles and the search role, see
@@ -320,6 +333,23 @@ impl v1alpha1::OpenSearchConfig {
     }
 }
 
+impl Default for v1alpha1::OpenSearchTls {
+    fn default() -> Self {
+        v1alpha1::OpenSearchTls {
+            rest_secret_class: rest_secret_class_default(),
+            transport_secret_class: transport_secret_class_default(),
+        }
+    }
+}
+
+fn rest_secret_class_default() -> Option<SecretClassName> {
+    Some(TLS_DEFAULT_SECRET_CLASS.to_owned())
+}
+
+fn transport_secret_class_default() -> SecretClassName {
+    TLS_DEFAULT_SECRET_CLASS.to_owned()
+}
+
 #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)]
 pub struct NodeRoles(pub Vec<v1alpha1::NodeRole>);
 
diff --git a/rust/operator-binary/src/framework.rs b/rust/operator-binary/src/framework.rs
@@ -79,9 +79,6 @@ pub enum Error {
 /// Duplicates the private constant [`stackable-operator::kvp::label::value::LABEL_VALUE_MAX_LEN`]
 pub const MAX_LABEL_VALUE_LENGTH: usize = 63;
 
-/// Maximum length of annotation values
-pub const MAX_ANNOTATION_LENGTH: usize = 253;
-
 /// Has a non-empty name
 ///
 /// Useful as an object reference; Should not be used to create an object because the name could
@@ -516,11 +513,12 @@ attributed_string_type! {
     is_valid_label_value
 }
 attributed_string_type! {
-    TlsSecretClassName,
+    SecretClassName,
     "The TLS SecretClass name",
     "tls",
-    // The secret class name is used in an annotation on the tls volume. To make sure the
-    (max_length = MAX_ANNOTATION_LENGTH - 30)
+    // The secret class name is used in an annotation on the tls volume.
+    (max_length = RFC_1123_SUBDOMAIN_MAX_LENGTH),
+    is_rfc_1123_dns_subdomain_name
 }
 #[cfg(test)]
 mod tests {
diff --git a/rust/operator-binary/src/framework/builder/volume.rs b/rust/operator-binary/src/framework/builder/volume.rs
@@ -4,12 +4,12 @@ use stackable_operator::{
     shared::time::Duration,
 };
 
-use crate::framework::TlsSecretClassName;
+use crate::framework::{SecretClassName, ServiceName};
 
 pub fn build_tls_volume(
     volume_name: &String,
-    tls_secret_class_name: &TlsSecretClassName,
-    service_scopes: impl IntoIterator<Item = impl AsRef<str>>,
+    tls_secret_class_name: &SecretClassName,
+    service_scopes: Vec<ServiceName>,
     secret_format: SecretFormat,
     requested_secret_lifetime: &Duration,
     listener_scope: Option<&str>,
@@ -18,7 +18,7 @@ pub fn build_tls_volume(
         SecretOperatorVolumeSourceBuilder::new(tls_secret_class_name);
 
     for scope in service_scopes {
-        secret_volume_source_builder.with_service_scope(scope.as_ref());
+        secret_volume_source_builder.with_service_scope(scope);
     }
     if let Some(listener_scope) = listener_scope {
         secret_volume_source_builder.with_listener_volume_scope(listener_scope);
diff --git a/tests/templates/kuttl/logging/20-install-opensearch.yaml.j2 b/tests/templates/kuttl/logging/20-install-opensearch.yaml.j2
