Copilot commented on code in PR #54:
URL:
https://github.com/apache/skywalking-banyandb-helm/pull/54#discussion_r2785640941
##########
test/e2e/values.dns.registry.yaml:
##########
@@ -24,7 +24,8 @@ image:
cluster:
enabled: true
- etcdEndpoints: []
+ schemaStorage:
+ mode: "property"
Review Comment:
The property schema storage config keys here (`property.cron` and
`property.syncInterval`) don’t match the chart values/docs
(`cluster.schemaStorage.property.serverRepairCron` and
`cluster.schemaStorage.property.clientSyncInterval`). As a result these
settings will be ignored during the DNS registry e2e run.
```suggestion
mode: "property"
property:
serverRepairCron: "*/1 * * * *"
clientSyncInterval: "10s"
```
##########
chart/templates/_helpers.tpl:
##########
@@ -88,16 +88,73 @@ 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 := (.Values.cluster.schemaStorage).mode | default "property" }}
+{{- $nodeDiscoveryMode := ((.Values.cluster.nodeDiscovery).mode) | default
"dns" }}
Review Comment:
`(.Values.cluster.schemaStorage).mode` and
`(.Values.cluster.nodeDiscovery).mode` will throw a template error if
`cluster.schemaStorage` or `cluster.nodeDiscovery` is unset (e.g., upgrading
from older values files). Wrap these lookups with `default dict` before
dereferencing `.mode` to keep the chart backward-compatible.
```suggestion
{{- $schemaMode := (default dict .Values.cluster.schemaStorage).mode |
default "property" }}
{{- $nodeDiscoveryMode := (default dict .Values.cluster.nodeDiscovery).mode
| default "dns" }}
```
##########
chart/templates/_helpers.tpl:
##########
@@ -88,16 +88,73 @@ 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 := (.Values.cluster.schemaStorage).mode | default "property" }}
+{{- $nodeDiscoveryMode := ((.Values.cluster.nodeDiscovery).mode) | default
"dns" }}
+{{- if or (eq $schemaMode "etcd") (eq $nodeDiscoveryMode "etcd") }}
+{{- $etcdClient := index .Values "etcd-client" | default dict }}
+{{- $auth := $etcdClient.auth | default dict }}
+{{- $tls := $auth.tls | default dict }}
+{{- include "banyandb.etcdEndpoints" . }}
+{{- if $auth.username }}
+- name: BYDB_ETCD_USERNAME
+ value: {{ $auth.username | quote }}
+{{- end }}
+{{- if $auth.password }}
+- name: BYDB_ETCD_PASSWORD
+ value: {{ $auth.password | quote }}
+{{- end }}
+{{- if $tls.enabled }}
+{{- if $tls.secretName }}
+- name: BYDB_ETCD_TLS_CA_FILE
+ value: "/etc/tls/{{ $tls.secretName }}/{{ $tls.caFilename | default "ca.crt"
}}"
+- name: BYDB_ETCD_TLS_CERT_FILE
+ value: "/etc/tls/{{ $tls.secretName }}/{{ $tls.certFilename | default
"tls.crt" }}"
+- name: BYDB_ETCD_TLS_KEY_FILE
+ value: "/etc/tls/{{ $tls.secretName }}/{{ $tls.keyFilename | default
"tls.key" }}"
Review Comment:
`etcd-client.auth.tls.secretName` is used to build
`/etc/tls/<secretName>/...` paths, but the chart does not mount a volume for
that secret anywhere (existing mounts are driven by
`cluster.*.tls.etcdSecretName`). As-is, enabling etcd-client TLS will point to
non-existent files unless users also manually wire matching mounts; either
mount the secret based on `etcd-client.auth.tls.secretName` or derive the paths
from the component `tls.etcdSecretName` values.
##########
chart/templates/_helpers.tpl:
##########
@@ -88,16 +88,73 @@ 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 }}
Review Comment:
When schema storage or node discovery mode is `etcd`, the chart currently
renders etcd env vars but will omit `BYDB_ETCD_ENDPOINTS` entirely if
`etcd-client.endpoints` is empty. This produces a misconfigured deployment
without a clear error; consider using `required` (or a Helm fail) to force
endpoints to be set when etcd is in use.
##########
doc/parameters.md:
##########
@@ -28,13 +28,31 @@ The content of this document describes the parameters that
can be configured in
| `auth.credentialsFileKey` | Key name in the Secret that stores the
| `credentials.yaml` |
| `auth.users` | List of users to configure when not using
existingSecret | `[]` |
-### Etcd Client Configuration for Node Discovery
+### Etcd Client Configuration
| Name | Description
| Value |
|------------------------------------|-----------------------------------------|------------|
| `etcd-client.namespace` | Namespace in etcd for node registration
| `banyandb` |
| `etcd-client.nodeDiscoveryTimeout` | Timeout for node discovery
| `2m` |
| `etcd-client.fullSyncInterval` | Interval for full state synchronization
| `30m` |
+| `etcd-client.endpoints` | List of external etcd endpoints
| `[]` |
+
+### Etcd Client Authentication
+
+| Name | Description |
Value |
+|---------------------------------|--------------------------------------|---------|
+| `etcd-client.auth.username` | Username for etcd authentication |
`""` |
+| `etcd-client.auth.password` | Password for etcd authentication |
`""` |
+
+### Etcd Client TLS Configuration
+
+| Name | Description
| Value |
+|-----------------------------------|------------------------------------------|-----------|
+| `etcd-client.auth.tls.enabled` | Enable TLS for etcd client
| `false` |
+| `etcd-client.auth.tls.secretName` | K8s secret name containing TLS certs
| `""` |
+| `etcd-client.auth.tls.caFilename` | CA certificate filename
| `ca.crt` |
+| `etcd-client.auth.tls.certFilename` | Client certificate filename
| `tls.crt` |
+| `etcd-client.auth.tls.keyFilename` | Client key filename
| `tls.key` |
Review Comment:
The docs imply `etcd-client.auth.tls.secretName` is sufficient to enable
etcd client TLS, but current templates don’t mount that secret (mounts are
controlled by `cluster.*.tls.etcdSecretName`). Please align docs with the
actual mounting behavior, or update templates so
`etcd-client.auth.tls.secretName` is mounted automatically.
```suggestion
| Name | Description
|
Value |
|-------------------------------------|----------------------------------------------------------------------------------------------------------------|-----------|
| `etcd-client.auth.tls.enabled` | Enable TLS for etcd client (requires
the TLS Secret to be mounted via `cluster.*.tls.etcdSecretName`) |
`false` |
| `etcd-client.auth.tls.secretName` | Name of the Kubernetes Secret
containing TLS certs; this value alone does not mount the Secret to the pod
| `""` |
| `etcd-client.auth.tls.caFilename` | CA certificate filename
|
`ca.crt` |
| `etcd-client.auth.tls.certFilename` | Client certificate filename
|
`tls.crt` |
| `etcd-client.auth.tls.keyFilename` | Client key filename
|
`tls.key` |
```
--
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]
