Copilot commented on code in PR #54:
URL:
https://github.com/apache/skywalking-banyandb-helm/pull/54#discussion_r2893201219
##########
chart/templates/_helpers.tpl:
##########
@@ -88,17 +88,146 @@ Create the name of the service account to use
EtcdEndpoints
*/}}
{{- define "banyandb.etcdEndpoints" -}}
-{{- $endpoints := list }}
-{{- $replicaCount := int .Values.etcd.replicaCount }}
-{{- $releaseName := .Release.Name }}
-{{- $namespace := .Release.Namespace }}
-{{- range $i := until $replicaCount }}
- {{- $endpoint := printf "%s-etcd-%d.%s-etcd-headless.%s:2379" $releaseName
$i $releaseName $namespace }}
- {{- $endpoints = append $endpoints $endpoint }}
-{{- end }}
+{{- $etcdClient := index .Values "etcd-client" | default dict }}
+{{- if $etcdClient.endpoints }}
- name: BYDB_ETCD_ENDPOINTS
- value: "{{- $endpoints | join "," -}}"
+ value: "{{- $etcdClient.endpoints | join "," -}}"
+{{- end }}
+{{- end }}
+
+{{/*
+EtcdEnv - injects all etcd-related env vars (endpoints, auth, TLS)
+Only rendered when schema storage mode is "etcd" or node discovery mode is
"etcd"
+*/}}
+{{- define "banyandb.etcdEnv" -}}
+{{- $schemaMode := (default dict .Values.cluster.schemaStorage).mode | default
"property" }}
+{{- $nodeDiscoveryMode := (default dict .Values.cluster.nodeDiscovery).mode |
default "dns" }}
+{{- if or (eq $schemaMode "etcd") (eq $nodeDiscoveryMode "etcd") }}
+{{- $etcdClient := index .Values "etcd-client" | default dict }}
Review Comment:
When schema storage or node discovery is set to "etcd", this helper
currently just omits BYDB_ETCD_ENDPOINTS if `etcd-client.endpoints` is empty.
Since the chart no longer deploys an embedded etcd, this will result in a
runtime failure/misconfiguration. Consider explicitly failing (or using
`required`) when mode is "etcd" and `etcd-client.endpoints` is empty, so
misconfiguration is caught at template-render time.
```suggestion
{{- $etcdClient := index .Values "etcd-client" | default dict }}
{{- if not $etcdClient.endpoints }}
{{- fail "etcd-client.endpoints must be set when schemaStorage or
nodeDiscovery mode is 'etcd'" }}
{{- end }}
```
##########
chart/templates/cluster_liaison_statefulset.yaml:
##########
@@ -147,28 +147,9 @@ spec:
value: "/etc/tls/{{ .Values.cluster.liaison.tls.httpSecretName
}}/tls.key"
{{- end }}
{{- end }}
- {{- if and .Values.etcd.auth.rbac.create (not
.Values.etcd.auth.rbac.allowNoneAuthentication) }}
- - name: BYDB_ETCD_USERNAME
- value: "root"
- - name: BYDB_ETCD_PASSWORD
- value: {{ .Values.etcd.auth.rbac.rootPassword }}
- {{- end }}
- {{- if and .Values.cluster.liaison.tls
.Values.cluster.liaison.tls.etcdSecretName
.Values.etcd.auth.client.secureTransport }}
- - name: BYDB_ETCD_TLS_CA_FILE
- value: "/etc/tls/{{ .Values.cluster.liaison.tls.etcdSecretName
}}/ca.crt"
- {{- end }}
- {{- if and .Values.cluster.liaison.tls
.Values.cluster.liaison.tls.etcdSecretName
.Values.etcd.auth.client.enableAuthentication }}
- - name: BYDB_ETCD_TLS_CERT_FILE
- value: "/etc/tls/{{ .Values.cluster.liaison.tls.etcdSecretName
}}/tls.crt"
- - name: BYDB_ETCD_TLS_KEY_FILE
- value: "/etc/tls/{{ .Values.cluster.liaison.tls.etcdSecretName
}}/tls.key"
- {{- end }}
- {{- if and (not .Values.etcd.enabled)
.Values.cluster.etcdEndpoints }}
- - name: BYDB_ETCD_ENDPOINTS
- value: "{{- .Values.cluster.etcdEndpoints | join "," -}}"
- {{- else }}
- {{- include "banyandb.etcdEndpoints" . | nindent 12 }}
- {{- end }}
+ {{- include "banyandb.schemaStorageEnv" . | nindent 12 }}
+ {{- include "banyandb.schemaStoragePropertyClientEnv" . | nindent
12 }}
+ {{- include "banyandb.etcdEnv" . | nindent 12 }}
{{- include "banyandb.nodeDiscoveryEnv" (dict "root" .) | nindent
12 }}
Review Comment:
`banyandb.etcdEnv` can emit BYDB_ETCD_TLS_* file paths based on
`etcd-client.auth.tls.*`, but this StatefulSet doesn't mount the
`etcd-client.auth.tls.secretName` secret anywhere. If TLS is enabled, the
container will reference non-existent files under `/etc/tls/<secretName>/...`.
Consider adding a conditional volume + volumeMount for
`etcd-client.auth.tls.secretName` (when tls.enabled && secretName), for every
workload that includes `banyandb.etcdEnv`.
##########
chart/templates/_helpers.tpl:
##########
@@ -180,7 +309,7 @@ Generate node discovery environment variables for a
component
value: {{ $etcdClient.namespace | quote }}
{{- end }}
{{- if $etcdClient.nodeDiscoveryTimeout }}
-- name: BYDB_NODE_DISCOVERY_TIMEOUT
+- name: BYDB_NODE_REGISTRY_TIMEOUT
value: {{ $etcdClient.nodeDiscoveryTimeout | quote }}
Review Comment:
This block is only relevant in etcd mode, but `banyandb.nodeDiscoveryEnv`
currently defaults `$mode` to "etcd" when `cluster.nodeDiscovery.mode` is
missing, while `values.yaml` documents/defaults it to "dns". Consider aligning
the template default with the chart default (dns) to avoid unexpected behavior
when users provide partial overrides of `cluster.nodeDiscovery`.
##########
chart/templates/cluster_ui_deployment.yaml:
##########
@@ -62,28 +62,8 @@ spec:
value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.httpSecretName }}/tls.key"
{{- end }}
{{- end }}
- {{- if and .Values.etcd.auth.rbac.create (not
.Values.etcd.auth.rbac.allowNoneAuthentication) }}
- - name: BYDB_ETCD_USERNAME
- value: "root"
- - name: BYDB_ETCD_PASSWORD
- value: {{ .Values.etcd.auth.rbac.rootPassword }}
- {{- end }}
- {{- if .Values.etcd.auth.client.secureTransport }}
- - name: BYDB_ETCD_TLS_CA_FILE
- value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.etcdSecretName }}/ca.crt"
- {{- end }}
- {{- if .Values.etcd.auth.client.enableAuthentication }}
- - name: BYDB_ETCD_TLS_CERT_FILE
- value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.etcdSecretName }}/tls.crt"
- - name: BYDB_ETCD_TLS_KEY_FILE
- value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.etcdSecretName }}/tls.key"
- {{- end }}
- {{- if and (not .Values.etcd.enabled)
.Values.cluster.etcdEndpoints }}
- - name: BYDB_ETCD_ENDPOINTS
- value: "{{- .Values.cluster.etcdEndpoints | join "," -}}"
- {{- else }}
- {{- include "banyandb.etcdEndpoints" . | nindent 12 }}
- {{- end }}
+ {{- include "banyandb.schemaStorageEnv" . | nindent 12 }}
+ {{- include "banyandb.etcdEnv" . | nindent 12 }}
Review Comment:
`banyandb.etcdEnv` can emit BYDB_ETCD_TLS_* file paths based on
`etcd-client.auth.tls.*`, but the Deployment only mounts secrets from
`cluster.ui.standalone.tls.*`. If `etcd-client.auth.tls.enabled` is used, the
UI container will reference non-existent files under
`/etc/tls/<secretName>/...`. Consider mounting
`etcd-client.auth.tls.secretName` (when enabled) in this Deployment as well, or
documenting that etcd TLS must reuse `cluster.ui.standalone.tls.*` secrets.
```suggestion
{{- include "banyandb.etcdEnv" . | nindent 12 }}
{{- if and .Values.cluster.ui.standalone.tls
.Values.cluster.ui.standalone.tls.httpSecretName
.Values.etcdClient.auth.tls.enabled }}
- name: BYDB_ETCD_TLS
value: "true"
- name: BYDB_ETCD_TLS_CERT_FILE
value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.httpSecretName }}/tls.crt"
- name: BYDB_ETCD_TLS_KEY_FILE
value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.httpSecretName }}/tls.key"
- name: BYDB_ETCD_TLS_CA_FILE
value: "/etc/tls/{{
.Values.cluster.ui.standalone.tls.httpSecretName }}/ca.crt"
{{- end }}
```
##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -174,28 +174,10 @@ spec:
value: "/etc/tls/{{ $roleConfig.tls.grpcSecretName }}/tls.crt"
{{- end }}
{{- end }}
- {{- if and $.Values.etcd.auth.rbac.create (not
$.Values.etcd.auth.rbac.allowNoneAuthentication) }}
- - name: BYDB_ETCD_USERNAME
- value: "root"
- - name: BYDB_ETCD_PASSWORD
- value: "{{ $.Values.etcd.auth.rbac.rootPassword }}"
- {{- end }}
- {{- if $.Values.etcd.auth.client.secureTransport }}
- - name: BYDB_ETCD_TLS_CA_FILE
- value: "/etc/tls/{{ $roleConfig.tls.etcdSecretName }}/ca.crt"
- {{- end }}
- {{- if $.Values.etcd.auth.client.enableAuthentication }}
- - name: BYDB_ETCD_TLS_CERT_FILE
- value: "/etc/tls/{{ $roleConfig.tls.etcdSecretName }}/tls.crt"
- - name: BYDB_ETCD_TLS_KEY_FILE
- value: "/etc/tls/{{ $roleConfig.tls.etcdSecretName }}/tls.key"
- {{- end }}
- {{- if and (not $.Values.etcd.enabled)
$.Values.cluster.etcdEndpoints }}
- - name: BYDB_ETCD_ENDPOINTS
- value: "{{- $.Values.cluster.etcdEndpoints | join "," -}}"
- {{- else }}
- {{- include "banyandb.etcdEndpoints" $ | nindent 12 }}
- {{- end }}
+ {{- include "banyandb.schemaStorageEnv" $ | nindent 12 }}
+ {{- include "banyandb.schemaStoragePropertyServerEnv" $ | nindent
12 }}
+ {{- include "banyandb.schemaStoragePropertyClientEnv" $ | nindent
12 }}
+ {{- include "banyandb.etcdEnv" $ | nindent 12 }}
{{- include "banyandb.nodeDiscoveryEnv" (dict "root" $) | nindent
12 }}
Review Comment:
`banyandb.etcdEnv` can emit BYDB_ETCD_TLS_* file paths based on
`etcd-client.auth.tls.*`, but this StatefulSet doesn't mount the
`etcd-client.auth.tls.secretName` secret anywhere. If TLS is enabled, the
container will reference non-existent files under `/etc/tls/<secretName>/...`.
Consider adding a conditional volume + volumeMount for
`etcd-client.auth.tls.secretName` (when tls.enabled && secretName), for every
workload that includes `banyandb.etcdEnv`.
--
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]
