refactor: Split into Deployment and DaemonSet

Techassi · Techassi · commit 7339bee1e3f1 · 2025-10-20T11:35:05.000+02:00
The splits the deployment of the secret-operator as a whole into two parts: - The controller is deployed via a Deployment which ensures only a single instance of the secret-operator in controller mode is running in a Kubernetes cluster. This can potentially lead to perfomance issues and as such should be monitored going forward. - The CSI server is deployed via a DaemonSet (unchanged) as this server is needed on every node to provision requested secret volumes. This refactor is introduced in preparation for #634, in which only a single instance of the CRD conversion webhook must exist as otherwise TLS certificate verification will fail with multiple available certificates.
diff --git a/Tiltfile b/Tiltfile
@@ -26,6 +26,7 @@ if os.path.exists('result'):
 # oci.stackable.tech/sandbox/opa-operator:7y19m3d8clwxlv34v5q2x4p7v536s00g instead of
 # oci.stackable.tech/sandbox/opa-operator:0.0.0-dev (which does not exist)
 k8s_kind('Deployment', image_json_path='{.spec.template.metadata.annotations.internal\\.stackable\\.tech/image}')
+k8s_kind('DaemonSet', image_json_path='{.spec.template.metadata.annotations.internal\\.stackable\\.tech/image}')
 
 # Exclude stale CRDs from Helm chart, and apply the rest
 helm_crds, helm_non_crds = filter_yaml(
@@ -34,7 +35,7 @@ helm_crds, helm_non_crds = filter_yaml(
       name=operator_name,
       namespace="stackable-operators",
       set=[
-         'image.repository=' + registry + '/' + operator_name,
+         'secretOperator.image.repository=' + registry + '/' + operator_name,
       ],
    ),
    api_version = "^apiextensions\\.k8s\\.io/.*$",
diff --git a/deploy/helm/secret-operator/templates/controller-deployment.yaml b/deploy/helm/secret-operator/templates/controller-deployment.yaml
@@ -0,0 +1,96 @@
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: {{ include "operator.fullname" . }}-controller
+  labels:
+    {{- include "operator.labels" . | nindent 4 }}
+spec:
+  selector:
+    matchLabels:
+      {{- include "operator.selectorLabels" . | nindent 6 }}
+  template:
+    metadata:
+      annotations:
+        internal.stackable.tech/image: "{{ .Values.secretOperator.image.repository }}:{{ .Values.secretOperator.image.tag | default .Chart.AppVersion }}"
+        {{- with .Values.podAnnotations }}
+          {{- toYaml . | nindent 8 }}
+        {{- end }}
+      labels:
+        {{- include "operator.selectorLabels" . | nindent 8 }}
+    spec:
+      {{- with .Values.imagePullSecrets }}
+      imagePullSecrets:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      # NOTE (@Techassi): Does it maybe make sense to have two different service accounts?
+      serviceAccountName: {{ include "operator.fullname" . }}-serviceaccount
+      securityContext:
+        {{- toYaml .Values.podSecurityContext | nindent 8 }}
+      containers:
+        - name: {{ include "operator.appname" . }}-controller
+          securityContext:
+            {{- toYaml .Values.secretOperator.securityContext | nindent 12 }}
+          image: "{{ .Values.secretOperator.image.repository }}:{{ .Values.secretOperator.image.tag | default .Chart.AppVersion }}"
+          imagePullPolicy: {{ .Values.secretOperator.image.pullPolicy }}
+          resources:
+            {{ .Values.secretOperator.resources | toYaml | nindent 12 }}
+          # The arguments passed to the command being run in the container. The final command will
+          # look like `secret-operator run controller [OPTIONS]`. The controller needs to only run
+          # once in a Kubernetes cluster and as such is deployed as a Deployment.
+          args:
+            - run
+            - controller
+          env:
+            # The following env vars are passed as clap (think CLI) arguments to the operator.
+            # They are picked up by clap using the structs defied in the operator.
+            # (which is turn pulls in https://github.com/stackabletech/operator-rs/blob/main/crates/stackable-operator/src/cli.rs)
+            # You can read there about the expected values and purposes.
+
+            # Sometimes products need to know the operator image, e.g. the opa-bundle-builder OPA
+            # sidecar uses the operator image.
+            - name: OPERATOR_IMAGE
+              # Tilt can use annotations as image paths, but not env variables
+              valueFrom:
+                fieldRef:
+                  fieldPath: metadata.annotations['internal.stackable.tech/image']
+
+            # Namespace the operator Pod is running in, e.g. used to construct the conversion
+            # webhook endpoint.
+            - name: OPERATOR_NAMESPACE
+              valueFrom:
+                fieldRef:
+                  fieldPath: metadata.namespace
+
+            # The name of the Kubernetes Service that point to the operator Pod, e.g. used to
+            # construct the conversion webhook endpoint.
+            - name: OPERATOR_SERVICE_NAME
+              value: {{ include "operator.fullname" . }}
+
+            # Operators need to know the node name they are running on, to e.g. discover the
+            # Kubernetes domain name from the kubelet API.
+            - name: KUBERNETES_NODE_NAME
+              valueFrom:
+                fieldRef:
+                  fieldPath: spec.nodeName
+
+            {{- if .Values.kubernetesClusterDomain }}
+            - name: KUBERNETES_CLUSTER_DOMAIN
+              value: {{ .Values.kubernetesClusterDomain | quote }}
+            {{- end }}
+            {{- include "telemetry.envVars" . | nindent 12 }}
+      {{- with .Values.nodeSelector }}
+      nodeSelector:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      {{- with .Values.affinity }}
+      affinity:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      {{- with .Values.tolerations }}
+      tolerations:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      {{- with .Values.priorityClassName }}
+      priorityClassName: {{ . }}
+      {{- end }}
diff --git a/deploy/helm/secret-operator/templates/csi-server-daemonset.yaml b/deploy/helm/secret-operator/templates/csi-server-daemonset.yaml
@@ -2,7 +2,7 @@
 apiVersion: apps/v1
 kind: DaemonSet
 metadata:
-  name: {{ include "operator.fullname" . }}-daemonset
+  name: {{ include "operator.fullname" . }}-csi-server
   labels:
     {{- include "operator.labels" . | nindent 4 }}
 spec:
@@ -11,28 +11,36 @@ spec:
       {{- include "operator.selectorLabels" . | nindent 6 }}
   template:
     metadata:
-      {{- with .Values.podAnnotations }}
       annotations:
-        {{- toYaml . | nindent 8 }}
-      {{- end }}
+        internal.stackable.tech/image: "{{ .Values.secretOperator.image.repository }}:{{ .Values.secretOperator.image.tag | default .Chart.AppVersion }}"
+        {{- with .Values.podAnnotations }}
+          {{- toYaml . | nindent 8 }}
+        {{- end }}
       labels:
         {{- include "operator.selectorLabels" . | nindent 8 }}
     spec:
       {{- with .Values.image.pullSecrets }}
       imagePullSecrets:
         {{- toYaml . | nindent 8 }}
       {{- end }}
+      # NOTE (@Techassi): Does it maybe make sense to have two different service accounts?
       serviceAccountName: {{ include "operator.fullname" . }}-serviceaccount
       securityContext:
         {{- toYaml .Values.podSecurityContext | nindent 8 }}
       containers:
-        - name: {{ include "operator.appname" . }}
+        - name: {{ include "operator.appname" . }}-csi-server
           securityContext:
             {{- toYaml .Values.secretOperator.securityContext | nindent 12 }}
           image: "{{ .Values.secretOperator.image.repository }}:{{ .Values.secretOperator.image.tag | default .Chart.AppVersion }}"
           imagePullPolicy: {{ .Values.secretOperator.image.pullPolicy }}
           resources:
             {{ .Values.secretOperator.resources | toYaml | nindent 12 }}
+          # The arguments passed to the command being run in the container. The final command will
+          # look like `secret-operator run csi-server [OPTIONS]`. The CSI server needs to run on
+          # every Kubernetes cluster node and as such is deployed as a DaemonSet.
+          args:
+            - run
+            - csi-server
           env:
             # The following env vars are passed as clap (think CLI) arguments to the operator.
             # They are picked up by clap using the structs defied in the operator.
diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs
@@ -2,7 +2,7 @@
 // This will need changes in our and upstream error types.
 #![allow(clippy::result_large_err)]
 
-use std::{os::unix::prelude::FileTypeExt, path::PathBuf, pin::pin};
+use std::{os::unix::prelude::FileTypeExt, path::PathBuf};
 
 use anyhow::Context;
 use clap::Parser;
@@ -16,7 +16,9 @@ use grpc::csi::v1::{
 };
 use stackable_operator::{
     YamlSchema,
-    cli::{CommonOptions, ProductOperatorRun},
+    cli::{Command, CommonOptions, ProductOperatorRun},
+    client::Client,
+    namespace::WatchNamespace,
     shared::yaml::SerializeOptions,
     telemetry::Tracing,
 };
@@ -40,14 +42,33 @@ pub const OPERATOR_NAME: &str = "secrets.stackable.tech";
 
 #[derive(clap::Parser)]
 #[clap(author, version)]
-struct Opts {
+struct Cli {
     #[clap(subcommand)]
-    cmd: stackable_operator::cli::Command<SecretOperatorRun>,
+    cmd: Command<SecretOperatorRun>,
 }
 
 #[derive(clap::Parser)]
 struct SecretOperatorRun {
-    #[clap(long, env)]
+    /// The run mode in which this operator should run in.
+    #[command(subcommand)]
+    mode: RunMode,
+
+    #[clap(flatten)]
+    common: ProductOperatorRun,
+}
+
+#[derive(Debug, clap::Subcommand)]
+enum RunMode {
+    /// Run the CSI server, one per Kubernetes cluster node.
+    CsiServer(CsiServerArguments),
+
+    /// Run the controller, one per Kubernetes cluster.
+    Controller,
+}
+
+#[derive(Debug, clap::Args)]
+struct CsiServerArguments {
+    #[arg(long, env)]
     csi_endpoint: PathBuf,
 
     /// Unprivileged mode disables any features that require running secret-operator in a privileged container.
@@ -56,11 +77,8 @@ struct SecretOperatorRun {
     /// - Secret volumes will be stored on disk, rather than in a ramdisk
     ///
     /// Unprivileged mode is EXPERIMENTAL and heavily discouraged, since it increases the risk of leaking secrets.
-    #[clap(long, env)]
+    #[arg(long, env)]
     privileged: bool,
-
-    #[clap(flatten)]
-    common: ProductOperatorRun,
 }
 
 mod built_info {
@@ -69,29 +87,28 @@ mod built_info {
 
 #[tokio::main]
 async fn main() -> anyhow::Result<()> {
-    let opts = Opts::parse();
-    match opts.cmd {
-        stackable_operator::cli::Command::Crd => {
+    let cli = Cli::parse();
+
+    match cli.cmd {
+        Command::Crd => {
             SecretClass::merged_crd(crd::SecretClassVersion::V1Alpha1)?
                 .print_yaml_schema(built_info::PKG_VERSION, SerializeOptions::default())?;
             TrustStore::merged_crd(crd::TrustStoreVersion::V1Alpha1)?
                 .print_yaml_schema(built_info::PKG_VERSION, SerializeOptions::default())?;
         }
-        stackable_operator::cli::Command::Run(SecretOperatorRun {
-            csi_endpoint,
-            privileged,
-            common:
-                ProductOperatorRun {
-                    common:
-                        CommonOptions {
-                            telemetry,
-                            cluster_info,
-                        },
-                    product_config: _,
-                    watch_namespace,
-                    operator_environment: _,
-                },
-        }) => {
+        Command::Run(SecretOperatorRun { common, mode }) => {
+            let ProductOperatorRun {
+                operator_environment: _,
+                product_config: _,
+                watch_namespace,
+                common,
+            } = common;
+
+            let CommonOptions {
+                telemetry,
+                cluster_info,
+            } = common;
+
             // NOTE (@NickLarsenNZ): Before stackable-telemetry was used:
             // - The console log level was set by `SECRET_PROVISIONER_LOG`, and is now `CONSOLE_LOG` (when using Tracing::pre_configured).
             // - The file log level was set by `SECRET_PROVISIONER_LOG`, and is now set via `FILE_LOG` (when using Tracing::pre_configured).
@@ -113,46 +130,70 @@ async fn main() -> anyhow::Result<()> {
                 &cluster_info,
             )
             .await?;
-            if csi_endpoint
-                .symlink_metadata()
-                .is_ok_and(|meta| meta.file_type().is_socket())
-            {
-                let _ = std::fs::remove_file(&csi_endpoint);
-            }
-            let mut sigterm = signal(SignalKind::terminate())?;
-            let csi_server = pin!(
-                Server::builder()
-                    .add_service(
-                        tonic_reflection::server::Builder::configure()
-                            .include_reflection_service(true)
-                            .register_encoded_file_descriptor_set(grpc::FILE_DESCRIPTOR_SET_BYTES)
-                            .build_v1()?,
-                    )
-                    .add_service(IdentityServer::new(SecretProvisionerIdentity))
-                    .add_service(ControllerServer::new(SecretProvisionerController {
-                        client: client.clone(),
-                    }))
-                    .add_service(NodeServer::new(SecretProvisionerNode {
-                        client: client.clone(),
-                        node_name: cluster_info.kubernetes_node_name.to_owned(),
+
+            match mode {
+                RunMode::CsiServer(CsiServerArguments {
+                    csi_endpoint,
+                    privileged,
+                }) => {
+                    run_csi_server(
+                        csi_endpoint,
+                        cluster_info.kubernetes_node_name,
                         privileged,
-                    }))
-                    .serve_with_incoming_shutdown(
-                        UnixListenerStream::new(
-                            uds_bind_private(csi_endpoint)
-                                .context("failed to bind CSI listener")?,
-                        )
-                        .map_ok(TonicUnixStream),
-                        sigterm.recv().map(|_| ()),
+                        client,
                     )
-            );
-            let truststore_controller =
-                pin!(truststore_controller::start(&client, &watch_namespace).map(Ok));
-            futures::future::select(csi_server, truststore_controller)
-                .await
-                .factor_first()
-                .0?;
+                    .await?
+                }
+                RunMode::Controller => run_controller(watch_namespace, client).await,
+            }
         }
     }
+
     Ok(())
 }
+
+async fn run_csi_server(
+    csi_endpoint: PathBuf,
+    node_name: String,
+    privileged: bool,
+    client: Client,
+) -> anyhow::Result<()> {
+    if csi_endpoint
+        .symlink_metadata()
+        .is_ok_and(|meta| meta.file_type().is_socket())
+    {
+        let _ = std::fs::remove_file(&csi_endpoint);
+    }
+
+    let mut sigterm = signal(SignalKind::terminate())?;
+
+    let csi_server = Server::builder()
+        .add_service(
+            tonic_reflection::server::Builder::configure()
+                .include_reflection_service(true)
+                .register_encoded_file_descriptor_set(grpc::FILE_DESCRIPTOR_SET_BYTES)
+                .build_v1()?,
+        )
+        .add_service(IdentityServer::new(SecretProvisionerIdentity))
+        .add_service(ControllerServer::new(SecretProvisionerController {
+            client: client.clone(),
+        }))
+        .add_service(NodeServer::new(SecretProvisionerNode {
+            privileged,
+            node_name,
+            client,
+        }))
+        .serve_with_incoming_shutdown(
+            UnixListenerStream::new(
+                uds_bind_private(csi_endpoint).context("failed to bind CSI listener")?,
+            )
+            .map_ok(TonicUnixStream),
+            sigterm.recv().map(|_| ()),
+        );
+
+    csi_server.await.context("failed to run the CSI server")
+}
+
+async fn run_controller(watch_namespace: WatchNamespace, client: Client) {
+    truststore_controller::start(client, &watch_namespace).await
+}
diff --git a/rust/operator-binary/src/truststore_controller.rs b/rust/operator-binary/src/truststore_controller.rs
