affo commented on code in PR #2506:
URL: https://github.com/apache/fluss/pull/2506#discussion_r2781380714
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate JAAS configuration for SASL
+*/}}
+{{- define "fluss.sasl.jaasConfig" -}}
+{{- if .Values.sasl.jaasConfig }}
+{{- .Values.sasl.jaasConfig -}}
+{{- else }}
+FlussServer {
+ org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required
+ {{- range .Values.sasl.users }}
+ user_{{ .username }}="{{ .password }}"
+ {{- end }};
+};
+{{- end }}
+{{- end }}
+
+{{/*
+Return true if SASL is configured in any of the listener protocols
+*/}}
+{{- define "fluss.isSaslEnabled" -}}
+{{- $ctx := . -}}
+{{- $res := "" -}}
+{{- $keys := keys .Values.listeners | sortAlpha -}}
+{{- range $keys }}
+ {{- $id := . -}}
+ {{- $l := index $ctx.Values.listeners $id -}}
+ {{- if regexFind "SASL" (upper $l.protocol) -}}
+ {{- $res = "true" -}}
+ {{- end -}}
+{{- end -}}
+{{- if $res -}}
+{{- true -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Generate ID:SECURITY list for listener protocols
+*/}}
+{{- define "fluss.listeners.protocolMap" -}}
Review Comment:
```suggestion
{{- define "fluss.listeners.securityProtocolMap" -}}
```
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate JAAS configuration for SASL
+*/}}
+{{- define "fluss.sasl.jaasConfig" -}}
+{{- if .Values.sasl.jaasConfig }}
Review Comment:
I don't see this value in the values.
I would remove this if and the value, why offering the possibility to
hardcode?
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate JAAS configuration for SASL
+*/}}
+{{- define "fluss.sasl.jaasConfig" -}}
+{{- if .Values.sasl.jaasConfig }}
+{{- .Values.sasl.jaasConfig -}}
+{{- else }}
+FlussServer {
Review Comment:
I like this approach, however, you would need to set
```
export FLUSS_ENV_JAVA_OPTS="-Djava.security.auth.login.config=..."
```
to make this work.
However, there is a caveat, this is a JVM-global setting, hence JAAS would
be configured globally.
This would entail also Zookeeper 🤝
Once adding authentication towards Zookeeper, this should be kept in mind as
well.
I also think this can directly be moved to the secret.
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
Review Comment:
Better to move this templating part to a separate `_sasl.tpl` file
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate JAAS configuration for SASL
+*/}}
+{{- define "fluss.sasl.jaasConfig" -}}
+{{- if .Values.sasl.jaasConfig }}
+{{- .Values.sasl.jaasConfig -}}
+{{- else }}
+FlussServer {
+ org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required
+ {{- range .Values.sasl.users }}
+ user_{{ .username }}="{{ .password }}"
+ {{- end }};
+};
+{{- end }}
+{{- end }}
+
+{{/*
+Return true if SASL is configured in any of the listener protocols
+*/}}
+{{- define "fluss.isSaslEnabled" -}}
Review Comment:
```suggestion
{{- define "fluss.sasl.enabled" -}}
```
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate JAAS configuration for SASL
+*/}}
+{{- define "fluss.sasl.jaasConfig" -}}
+{{- if .Values.sasl.jaasConfig }}
+{{- .Values.sasl.jaasConfig -}}
+{{- else }}
+FlussServer {
+ org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required
+ {{- range .Values.sasl.users }}
+ user_{{ .username }}="{{ .password }}"
+ {{- end }};
+};
+{{- end }}
+{{- end }}
+
+{{/*
+Return true if SASL is configured in any of the listener protocols
+*/}}
+{{- define "fluss.isSaslEnabled" -}}
+{{- $ctx := . -}}
+{{- $res := "" -}}
+{{- $keys := keys .Values.listeners | sortAlpha -}}
+{{- range $keys }}
+ {{- $id := . -}}
+ {{- $l := index $ctx.Values.listeners $id -}}
+ {{- if regexFind "SASL" (upper $l.protocol) -}}
+ {{- $res = "true" -}}
+ {{- end -}}
+{{- end -}}
+{{- if $res -}}
+{{- true -}}
+{{- end -}}
+{{- end -}}
Review Comment:
```suggestion
{{- $enabled := false -}}
{{- range $id, $l := .Values.listeners -}}
{{- if and (not $enabled) (regexFind "SASL" (upper $l.protocol)) -}}
{{- $enabled = true -}}
{{- end -}}
{{- end -}}
{{- $enabled -}}
```
I think this would reach the same goal with less overhead 👍
##########
.github/workflows/helm-chart.yaml:
##########
@@ -0,0 +1,59 @@
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+name: "Helm Chart"
+
+permissions:
+ contents: read
+
+on:
+ pull_request:
+ branches: [main, release-*, ci-*]
+ paths:
+ - 'helm/**'
+ - '.github/workflows/helm-chart.yaml'
+ push:
+ branches: [main, release-*, ci-*]
+ paths:
+ - 'helm/**'
+ - '.github/workflows/helm-chart.yaml'
+
+concurrency:
+ group: ${{ github.workflow }}-${{ github.event_name }}-${{
github.event.number || github.run_id }}
+ cancel-in-progress: true
+
+jobs:
+ test-helm-chart:
+ name: "Helm Lint and Unittest"
+ runs-on: ubuntu-latest
+ steps:
+ - name: "Checkout code"
+ uses: actions/checkout@v6
+
+ - name: "Setup Helm"
+ uses: azure/setup-helm@v4
+
+ - name: "Lint Helm chart"
+ run: helm lint ./helm
+
+ - name: "Run helm-unittest"
Review Comment:
It seems there is an action already for this:
https://github.com/marketplace/actions/helm-unit-tests.
You can also avoid the `setup` step using that one, first running the tests,
and then lint 🤝
##########
helm/values.yaml:
##########
@@ -85,3 +94,12 @@ serviceAccount:
# Additional annotations to apply to the ServiceAccount.
# These can be useful, for example, to support integrations like workload
identity.
annotations: {}
+
+## Fluss SASL configurations for authentication.
+## These are required if SASL listeners are configured.
+sasl:
+ mechanism: PLAIN
+ # List of client users for authentication
+ users:
+ - username: admin
+ password: password
Review Comment:
maybe good to scope this under a root `security` key.
As after SASL other protocols will probably be supported.
Also, I think the mechanism cannot be set from the outside, as every
mechanism has different inputs.
```suggestion
security:
sasl_plain:
# List of client users for authentication
users:
- username: admin
password: password
```
##########
helm/templates/_helpers.tpl:
##########
@@ -63,4 +63,54 @@ Selector labels
{{- define "fluss.selectorLabels" -}}
app.kubernetes.io/name: {{ include "fluss.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate JAAS configuration for SASL
+*/}}
+{{- define "fluss.sasl.jaasConfig" -}}
+{{- if .Values.sasl.jaasConfig }}
+{{- .Values.sasl.jaasConfig -}}
+{{- else }}
+FlussServer {
+ org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required
+ {{- range .Values.sasl.users }}
+ user_{{ .username }}="{{ .password }}"
+ {{- end }};
+};
+{{- end }}
+{{- end }}
+
+{{/*
+Return true if SASL is configured in any of the listener protocols
+*/}}
+{{- define "fluss.isSaslEnabled" -}}
+{{- $ctx := . -}}
+{{- $res := "" -}}
+{{- $keys := keys .Values.listeners | sortAlpha -}}
+{{- range $keys }}
+ {{- $id := . -}}
+ {{- $l := index $ctx.Values.listeners $id -}}
+ {{- if regexFind "SASL" (upper $l.protocol) -}}
+ {{- $res = "true" -}}
+ {{- end -}}
+{{- end -}}
+{{- if $res -}}
+{{- true -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Generate ID:SECURITY list for listener protocols
+*/}}
+{{- define "fluss.listeners.protocolMap" -}}
+{{- $ctx := . -}}
+{{- $parts := list -}}
+{{- $keys := keys .Values.listeners | sortAlpha -}}
+{{- range $keys }}
+ {{- $id := . -}}
+ {{- $l := index $ctx.Values.listeners $id -}}
+ {{- $parts = append $parts (printf "%s:%s" (upper $id) (upper $l.protocol))
-}}
+{{- end -}}
+{{- join "," $parts -}}
+{{- end }}
Review Comment:
This looks a bit too much as of now, as you cannot effectively template
listeners, but you can for the security protocol map.
I would hardcode this one similarly to listeners.
We can follow up with another PR to template both listeners and protocol map
at the same time 🤝
##########
.github/workflows/helm-chart.yaml:
##########
@@ -0,0 +1,59 @@
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+name: "Helm Chart"
+
+permissions:
+ contents: read
+
+on:
+ pull_request:
+ branches: [main, release-*, ci-*]
Review Comment:
```suggestion
```
##########
helm/templates/sts-coordinator.yaml:
##########
@@ -77,6 +77,26 @@ spec:
echo "" >> $FLUSS_HOME/conf/server.yaml && \
echo "bind.listeners: ${BIND_LISTENERS}" >>
$FLUSS_HOME/conf/server.yaml && \
echo "advertised.listeners: ${ADVERTISED_LISTENERS}" >>
$FLUSS_HOME/conf/server.yaml && \
+ echo "security.protocol.map: {{ include
"fluss.listeners.protocolMap" . }}" >> $FLUSS_HOME/conf/server.yaml && \
+
+ {{- if (include "fluss.isSaslEnabled" .) }}
+ {{- $saslUsers := default (list) .Values.sasl.users -}}
+ {{- if eq (len $saslUsers) 0 -}}
+ {{- fail "sasl.users must contain at least one user when SASL is
enabled in listeners" -}}
+ {{- end }}
+ {{- $jaasUsers := list -}}
+ {{- range $saslUsers }}
+ {{- $jaasUsers = append $jaasUsers (printf "user_%s=\\\"%s\\\""
.username .password) -}}
+ {{- end }}
+ {{- $clientUser := first $saslUsers -}}
+ echo "security.sasl.enabled.mechanisms: {{
.Values.sasl.mechanism }}" >> $FLUSS_HOME/conf/server.yaml && \
+ echo "security.sasl.plain.jaas.config:
org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required {{ join " "
$jaasUsers }};" >> $FLUSS_HOME/conf/server.yaml && \
Review Comment:
I don't understand why having the Jaas secret mounted but also adding this
complexity here.
It looks to me as a repetition 🤔
##########
helm/templates/secret-sasl.yaml:
##########
@@ -0,0 +1,29 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+{{- if (include "fluss.isSaslEnabled" .) }}
+apiVersion: v1
+kind: Secret
+metadata:
+ name: {{ include "fluss.fullname" . }}-sasl-jaas-config
+ labels:
+ {{- include "fluss.labels" . | nindent 4 }}
+type: Opaque
+data:
+ jaas.conf: {{ include "fluss.sasl.jaasConfig" . | b64enc | quote }}
+{{- end -}}
Review Comment:
I would add this file as generic, JVM-global, JAAS config.
hence, I would rename to secret-jaas-config.yaml.
```suggestion
{{- if (include "fluss.sasl.enabled" .) }}
apiVersion: v1
kind: Secret
metadata:
name: {{ include "fluss.fullname" . }}-jaas-config
labels:
{{- include "fluss.labels" . | nindent 4 }}
type: Opaque
stringData:
jaas.conf: |
FlussServer {
org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required
{{- range .Values.sasl.users }}
user_{{ .username }}="{{ .password }}"
{{- end }};
};
FlussClient {
org.apache.fluss.security.auth.sasl.plain.PlainLoginModule required
username="{{ }}"
password="{{ }}";
};
# Space to add Zookeeper authentication below in the future
{{- end -}}
```
--
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]
