affo commented on code in PR #2700:
URL: https://github.com/apache/fluss/pull/2700#discussion_r2989278228


##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -193,6 +193,15 @@ The following table lists the configurable parameters of 
the Fluss chart, and th
 | `security.internal.sasl.plain.username` | Internal listener PLAIN username | 
`""` |
 | `security.internal.sasl.plain.password` | Internal listener PLAIN password | 
`""` |
 
+#### ZooKeeper SASL Parameters

Review Comment:
   I think this is going in-between a previous section breaking it 🤔 



##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -314,6 +323,23 @@ security:
         password: internal-password
 ```
 
+### Enabling ZooKeeper SASL Authentication
+
+You can enable ZooKeeper ensemble SASL authentication, with the following 
values in the Fluss Helm chart:
+
+```yaml
+security:
+  zookeeper:
+    sasl:
+      enabled: true
+      username: fluss-zk-user
+      password: fluss-zk-password
+```
+
+If the username or password is left empty, the chart automatically generates 
credentials based on the Helm release name. It is recommended to set these 
explicitly in production.

Review Comment:
   we should not generate those credentials, but simply fail.
   Fluss does not manage ZK.



##########
helm/tests/security_test.yaml:
##########
@@ -282,3 +282,161 @@ tests:
       - matchRegex:
           path: spec.template.spec.containers[0].command[2]
           pattern: '>> \$FLUSS_HOME/conf/server\.yaml'
+

Review Comment:
   Should we add tests to check that the zookeper SASL is not enabled when it 
is not enabled?



##########
helm/templates/_security.tpl:
##########
@@ -118,39 +118,84 @@ Usage:
 {{- end -}}
 
 {{/*
-Returns the default internal SASL username based on the release name.
+Validates that ZooKeeper SASL loginModuleClass is not empty when ZK SASL is 
enabled.
+Returns an error message if invalid, empty string otherwise.
+Usage:
+  include "fluss.security.sasl.validateZookeeperLoginModuleClass" .
+*/}}
+{{- define "fluss.security.sasl.validateZookeeperLoginModuleClass" -}}
+{{- if and .Values.security.zookeeper.sasl.enabled (not 
.Values.security.zookeeper.sasl.loginModuleClass) -}}
+  {{- print "security.zookeeper.sasl.loginModuleClass must not be empty when 
security.zookeeper.sasl.enabled is true" -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Returns a default SASL username based on a given prefix and the release name.
 Usage:
-  include "fluss.security.sasl.plain.internal.defaultUsername" .
+  include "fluss.security.sasl.defaultUsername" (dict "prefix" 
"fluss-internal" "Release" .Release)
 */}}
-{{- define "fluss.security.sasl.plain.internal.defaultUsername" -}}
-{{- printf "fluss-internal-user-%s" .Release.Name -}}
+{{- define "fluss.security.sasl.defaultUsername" -}}
+{{- printf "%s-user-%s" .prefix .Release.Name -}}
 {{- end -}}
 
 {{/*
-Returns the default internal SASL password based on the release name (sha256 
hashed).
+Returns a default SASL password based on a given prefix and the release name 
(sha256 hashed).
 Usage:
-  include "fluss.security.sasl.plain.internal.defaultPassword" .
+  include "fluss.security.sasl.defaultPassword" (dict "prefix" 
"fluss-internal" "Release" .Release)
 */}}
-{{- define "fluss.security.sasl.plain.internal.defaultPassword" -}}
-{{- printf "fluss-internal-password-%s" .Release.Name | sha256sum -}}
+{{- define "fluss.security.sasl.defaultPassword" -}}
+{{- printf "%s-password-%s" .prefix .Release.Name | sha256sum -}}
 {{- end -}}
 
 {{/*
-Returns the resolved internal SASL username (user-provided or auto-generated 
default).
+Returns the resolved internal SASL username.
+It generates internal username if user provided is empty.
 Usage:
   include "fluss.security.sasl.plain.internal.username" .
 */}}
 {{- define "fluss.security.sasl.plain.internal.username" -}}
-{{- .Values.security.internal.sasl.plain.username | default (include 
"fluss.security.sasl.plain.internal.defaultUsername" .) -}}
+{{- .Values.security.internal.sasl.plain.username | default (include 
"fluss.security.sasl.defaultUsername" (dict "prefix" "fluss-internal" "Release" 
.Release)) -}}
 {{- end -}}
 
 {{/*
-Returns the resolved internal SASL password (user-provided or auto-generated 
default).
+Returns the resolved internal SASL password.
+It generates internal password if user provided is empty.
 Usage:
   include "fluss.security.sasl.plain.internal.password" .
 */}}
 {{- define "fluss.security.sasl.plain.internal.password" -}}
-{{- .Values.security.internal.sasl.plain.password | default (include 
"fluss.security.sasl.plain.internal.defaultPassword" .) -}}
+{{- .Values.security.internal.sasl.plain.password | default (include 
"fluss.security.sasl.defaultPassword" (dict "prefix" "fluss-internal" "Release" 
.Release)) -}}
+{{- end -}}
+
+{{/*
+Returns the resolved ZooKeeper SASL username.
+It generates Zookeeper username if user provided is empty.
+Usage:
+  include "fluss.security.zookeeper.sasl.username" .
+*/}}
+{{- define "fluss.security.zookeeper.sasl.username" -}}
+{{- .Values.security.zookeeper.sasl.username | default (include 
"fluss.security.sasl.defaultUsername" (dict "prefix" "fluss-zookeeper" 
"Release" .Release)) -}}

Review Comment:
   no need for username/password generation here.
   We are not the owner of the ZK server



##########
helm/templates/_security.tpl:
##########
@@ -118,39 +118,84 @@ Usage:
 {{- end -}}
 
 {{/*
-Returns the default internal SASL username based on the release name.
+Validates that ZooKeeper SASL loginModuleClass is not empty when ZK SASL is 
enabled.
+Returns an error message if invalid, empty string otherwise.
+Usage:
+  include "fluss.security.sasl.validateZookeeperLoginModuleClass" .
+*/}}
+{{- define "fluss.security.sasl.validateZookeeperLoginModuleClass" -}}
+{{- if and .Values.security.zookeeper.sasl.enabled (not 
.Values.security.zookeeper.sasl.loginModuleClass) -}}
+  {{- print "security.zookeeper.sasl.loginModuleClass must not be empty when 
security.zookeeper.sasl.enabled is true" -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Returns a default SASL username based on a given prefix and the release name.
 Usage:
-  include "fluss.security.sasl.plain.internal.defaultUsername" .
+  include "fluss.security.sasl.defaultUsername" (dict "prefix" 
"fluss-internal" "Release" .Release)
 */}}
-{{- define "fluss.security.sasl.plain.internal.defaultUsername" -}}
-{{- printf "fluss-internal-user-%s" .Release.Name -}}
+{{- define "fluss.security.sasl.defaultUsername" -}}
+{{- printf "%s-user-%s" .prefix .Release.Name -}}
 {{- end -}}
 
 {{/*
-Returns the default internal SASL password based on the release name (sha256 
hashed).
+Returns a default SASL password based on a given prefix and the release name 
(sha256 hashed).
 Usage:
-  include "fluss.security.sasl.plain.internal.defaultPassword" .
+  include "fluss.security.sasl.defaultPassword" (dict "prefix" 
"fluss-internal" "Release" .Release)
 */}}
-{{- define "fluss.security.sasl.plain.internal.defaultPassword" -}}
-{{- printf "fluss-internal-password-%s" .Release.Name | sha256sum -}}
+{{- define "fluss.security.sasl.defaultPassword" -}}
+{{- printf "%s-password-%s" .prefix .Release.Name | sha256sum -}}
 {{- end -}}
 
 {{/*
-Returns the resolved internal SASL username (user-provided or auto-generated 
default).
+Returns the resolved internal SASL username.
+It generates internal username if user provided is empty.
 Usage:
   include "fluss.security.sasl.plain.internal.username" .
 */}}
 {{- define "fluss.security.sasl.plain.internal.username" -}}
-{{- .Values.security.internal.sasl.plain.username | default (include 
"fluss.security.sasl.plain.internal.defaultUsername" .) -}}
+{{- .Values.security.internal.sasl.plain.username | default (include 
"fluss.security.sasl.defaultUsername" (dict "prefix" "fluss-internal" "Release" 
.Release)) -}}
 {{- end -}}
 
 {{/*
-Returns the resolved internal SASL password (user-provided or auto-generated 
default).
+Returns the resolved internal SASL password.
+It generates internal password if user provided is empty.
 Usage:
   include "fluss.security.sasl.plain.internal.password" .
 */}}
 {{- define "fluss.security.sasl.plain.internal.password" -}}
-{{- .Values.security.internal.sasl.plain.password | default (include 
"fluss.security.sasl.plain.internal.defaultPassword" .) -}}
+{{- .Values.security.internal.sasl.plain.password | default (include 
"fluss.security.sasl.defaultPassword" (dict "prefix" "fluss-internal" "Release" 
.Release)) -}}
+{{- end -}}
+
+{{/*
+Returns the resolved ZooKeeper SASL username.
+It generates Zookeeper username if user provided is empty.
+Usage:
+  include "fluss.security.zookeeper.sasl.username" .
+*/}}
+{{- define "fluss.security.zookeeper.sasl.username" -}}
+{{- .Values.security.zookeeper.sasl.username | default (include 
"fluss.security.sasl.defaultUsername" (dict "prefix" "fluss-zookeeper" 
"Release" .Release)) -}}
+{{- end -}}
+
+{{/*
+Returns the resolved ZooKeeper SASL password.
+It generates Zookeeper password if user provided is empty.
+Usage:
+  include "fluss.security.zookeeper.sasl.password" .
+*/}}
+{{- define "fluss.security.zookeeper.sasl.password" -}}
+{{- .Values.security.zookeeper.sasl.password | default (include 
"fluss.security.sasl.defaultPassword" (dict "prefix" "fluss-zookeeper" 
"Release" .Release)) -}}

Review Comment:
   no need for username/password generation here.
   We are not the owner of the ZK server



##########
helm/templates/sts-tablet.yaml:
##########
@@ -115,7 +115,7 @@ spec:
         - name: data
           emptyDir: {}
         {{- end }}
-        {{- if (include "fluss.security.sasl.plain.enabled" .) }}
+        {{- if (include "fluss.security.jaas.required" .) }}
         - name: sasl-config
           secret:
             secretName: {{ include "fluss.security.sasl.configName" . }}

Review Comment:
   should this be rename to `fluss.security.jaas.configName`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to