Copilot commented on code in PR #369:
URL:
https://github.com/apache/incubator-devlake-helm-chart/pull/369#discussion_r3185497589
##########
charts/devlake/templates/validate.yaml:
##########
@@ -1,21 +1,43 @@
-{{- if and .Values.lake.encryptionSecret.autoCreateSecret (not
.Values.lake.encryptionSecret.secret) }}
+{{- if and (not .Values.externalSecrets.enabled)
.Values.lake.encryptionSecret.autoCreateSecret (not
.Values.lake.encryptionSecret.secret) }}
{{- fail
"Helm test requires lake.encryptionSecret.secret.\n\n - If you're upgrading
from DevLake v0.17.x or earlier versions, please get the encryption secret by
copying the ENCODE_KEY value from /app/config/.env of the lake pod (e.g.
devlake-lake-0);\n - If upgrading from v0.18.0+, get the original secret in
k8s secret and decode it\n - If new installation, get the encryption secret
via command `openssl rand -base64 2000 | tr -dc 'A-Z' | fold -w 128 | head -n
1`.\n\nFor more information, please check
https://github.com/apache/incubator-devlake-helm-chart";
}}
{{- end }}
-{{- if and .Values.ui.basicAuth.enabled .Values.ui.basicAuth.autoCreateSecret
(or (not .Values.ui.basicAuth.user) (not .Values.ui.basicAuth.password)) }}
+{{- if and (not .Values.externalSecrets.enabled) .Values.ui.basicAuth.enabled
.Values.ui.basicAuth.autoCreateSecret (or (not .Values.ui.basicAuth.user) (not
.Values.ui.basicAuth.password)) }}
{{- fail "Helm test requires ui.basicAuth.user and ui.basicAuth.password" }}
{{- end }}
-{{- if and (eq .Values.option.database "mysql")
.Values.option.autoCreateSecret (or (not .Values.mysql.username) (not
.Values.mysql.password) (not .Values.mysql.database)) }}
-{{- fail "Helm test requires mysql.username, mysql.password and
mysql.database" }}
+{{- if not (has .Values.database.type (list "mysql" "postgresql")) }}
+{{- fail "database.type must be 'mysql' or 'postgresql'" }}
{{- end }}
-{{- if and .Values.option.autoCreateSecret (eq .Values.option.database
"mysql") (not .Values.mysql.useExternal) (not .Values.mysql.rootPassword) }}
-{{- fail "Helm test requires mysql.rootPassword" }}
+{{- if and (not .Values.externalSecrets.enabled)
.Values.option.autoCreateSecret (or (not .Values.database.username) (not
.Values.database.password) (not .Values.database.database)) }}
+{{- fail "Helm test requires database.username, database.password and
database.database" }}
{{- end }}
-{{- if and (not .Values.grafana.enabled) (not .Values.grafana.external.url) }}
-{{- fail "Helm test requires grafana.enabled to be true or
grafana.external.url to be provided" }}
+{{- if and (not .Values.externalSecrets.enabled)
.Values.option.autoCreateSecret (eq .Values.database.type "mysql") (not
.Values.database.useExternal) (not .Values.database.mysql.rootPassword) }}
+{{- fail "Helm test requires database.mysql.rootPassword when using embedded
MySQL" }}
+{{- end }}
+
+{{- if and (eq .Values.database.type "postgresql") .Values.grafana.enabled }}
+{{- fail "Grafana dashboards currently only support MySQL datasources. Set
grafana.enabled=false when using PostgreSQL, or provide an external Grafana
instance with manually configured PostgreSQL datasources via
grafana.external.url" }}
+{{- end }}
+
+{{- if .Values.lake.hostNetwork }}
+{{- fail "SECURITY VIOLATION: hostNetwork is not allowed. Remove
lake.hostNetwork or use LoadBalancer/NodePort service type for external access.
hostNetwork bypasses NetworkPolicies and exposes the node's network namespace."
}}
Review Comment:
This validation hard-fails when `lake.hostNetwork=true`, but other
templates/NOTES treat `lake.hostNetwork` as merely deprecated (and
`deployments.yaml` still renders `hostNetwork: true`). Either fully
remove/disable `lake.hostNetwork` across the chart (values/docs/templates), or
relax this validation; keeping both paths is inconsistent and leaves
dead/unused template branches.
##########
charts/devlake/templates/_helpers.tpl:
##########
@@ -40,41 +40,51 @@ helm.sh/chart: {{ include "devlake.chart" . }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
-{{- end }}
+{{- end -}}
{{/*
-Selector labels
+Selector labels (returns YAML string)
*/}}
{{- define "devlake.selectorLabels" -}}
app.kubernetes.io/name: {{ include "devlake.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
-{{- end }}
+{{- end -}}
+
+{{/*
+Selector labels as dict (for matchLabels)
+*/}}
+{{- define "devlake.selectorLabelsDict" -}}
+{{- $labels := dict }}
+{{- $_ := set $labels "app.kubernetes.io/name" (include "devlake.name" .) }}
+{{- $_ := set $labels "app.kubernetes.io/instance" .Release.Name }}
+{{- toYaml $labels }}
+{{- end -}}
{{/*
Create the name of the service account to use
*/}}
{{- define "devlake.serviceAccountName" -}}
-{{- if .Values.serviceAccount.create }}
-{{- default (include "devlake.fullname" .) .Values.serviceAccount.name }}
-{{- else }}
-{{- default "default" .Values.serviceAccount.name }}
-{{- end }}
-{{- end }}
+{{- if .Values.serviceAccount.name -}}
+{{- .Values.serviceAccount.name -}}
+{{- else -}}
+{{- include "devlake.fullname" . }}-sa
Review Comment:
`devlake.serviceAccountName` ignores `serviceAccount.create`. If a user sets
`serviceAccount.create=false` and leaves `serviceAccount.name` empty, workloads
will still reference `<fullname>-sa` even though the chart won't create it.
Please incorporate `serviceAccount.create` into this helper (e.g., default to
`default` when not creating) or require a name when `create=false`.
##########
charts/devlake/Chart.yaml:
##########
@@ -28,10 +28,10 @@ keywords:
type: application
# Chart version
-version: 1.0.3-beta10
+version: 2.0.0
# devlake version
-appVersion: v1.0.3-beta10
+appVersion: v1.0.3-beta12
Review Comment:
The PR description says it only splits the database StatefulSet template by
type, but this PR also introduces many additional, user-facing changes (chart
version bump to 2.0.0, new NetworkPolicies, ExternalSecrets support, backup
CronJob, schema validation, new defaults like grafana disabled, etc.). Please
update the PR description (or split into smaller PRs) so reviewers/users can
clearly understand the full scope of the breaking changes.
##########
charts/devlake/templates/networkpolicy-lake.yaml:
##########
@@ -0,0 +1,73 @@
+#
+# 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 .Values.networkPolicy.enabled }}
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+ name: {{ include "devlake.fullname" . }}-lake
+ labels:
+ {{- include "devlake.labels" . | nindent 4 }}
+spec:
+ podSelector:
+ matchLabels:
+ devlakeComponent: lake
+ policyTypes:
+ - Ingress
+ - Egress
+ ingress:
+ - from:
+ - podSelector:
+ matchLabels:
+ devlakeComponent: ui
+ ports:
+ - protocol: TCP
+ port: 8080
+ egress:
+ # Database access
+ - to:
+ - podSelector:
+ matchLabels:
+ devlakeComponent: {{ if eq .Values.database.type "mysql"
}}mysql{{ else }}postgresql{{ end }}
+ ports:
+ - protocol: TCP
+ port: {{ if eq .Values.database.type "mysql" }}3306{{ else }}5432{{
end }}
Review Comment:
This DB egress rule only allows traffic to in-namespace pods selected by
`devlakeComponent: mysql/postgresql`. When `database.useExternal=true`, there
is no matching DB pod, so enabling this NetworkPolicy will block lake ->
external DB connectivity. Please add a conditional egress rule for the external
DB (configurable ipBlock(s) + port) when `database.useExternal=true`.
##########
charts/devlake/templates/backup-cronjob.yaml:
##########
@@ -0,0 +1,100 @@
+#
+# 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 .Values.backup.enabled }}
+apiVersion: batch/v1
+kind: CronJob
+metadata:
+ name: {{ include "devlake.fullname" . }}-backup
+ labels:
+ {{- include "devlake.labels" . | nindent 4 }}
+spec:
+ schedule: "{{ .Values.backup.schedule }}"
+ successfulJobsHistoryLimit: {{ .Values.backup.successfulJobsHistoryLimit }}
+ failedJobsHistoryLimit: {{ .Values.backup.failedJobsHistoryLimit }}
+ concurrencyPolicy: Forbid
+ jobTemplate:
+ spec:
+ backoffLimit: {{ .Values.backup.backoffLimit }}
+ template:
+ metadata:
+ labels:
+ {{- include "devlake.selectorLabels" . | nindent 12 }}
+ app.kubernetes.io/component: backup
+ spec:
+ restartPolicy: OnFailure
+ serviceAccountName: {{ include "devlake.serviceAccountName" . }}
+ securityContext:
+ runAsNonRoot: true
+ runAsUser: 65534
+ fsGroup: 65534
+ seccompProfile:
+ type: RuntimeDefault
+ containers:
+ - name: backup
+ image: "{{ .Values.backup.image.repository }}:{{
.Values.backup.image.tag }}"
Review Comment:
The backup CronJob image is fully user-specified, but the default values set
it to `mysql:8`. When `database.type=postgresql`, the script calls `pg_dump`,
which is not present in the MySQL image and the job will fail. Please
default/select the backup image based on `database.type` (or validate that the
image contains the required dump tool).
##########
charts/devlake/templates/postgresql-migration-job.yaml:
##########
@@ -0,0 +1,144 @@
+{{- if eq .Values.database.type "postgresql" }}
+#
+# 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.
+#
+---
+apiVersion: batch/v1
+kind: Job
+metadata:
+ name: {{ include "devlake.fullname" . }}-postgresql-migration-fix
+ labels:
+ {{- include "devlake.labels" . | nindent 4 }}
+ annotations:
+ "helm.sh/hook": post-install,post-upgrade
+ "helm.sh/hook-weight": "10"
+ "helm.sh/hook-delete-policy": before-hook-creation
+spec:
+ backoffLimit: 5
+ template:
+ metadata:
+ labels:
+ {{- include "devlake.selectorLabels" . | nindent 8 }}
+ devlakeComponent: migration-fixer
+ spec:
+ restartPolicy: OnFailure
+ {{- with .Values.imagePullSecrets }}
+ imagePullSecrets:
+ {{- toYaml . | nindent 8 }}
+ {{- end }}
+ serviceAccountName: {{ include "devlake.serviceAccountName" . }}
+ containers:
+ - name: postgresql-migration-fix
+ image: postgres:15-alpine
Review Comment:
The PostgreSQL migration hook job uses `postgres:15-alpine` while the
embedded PostgreSQL StatefulSet defaults to `postgres:14` (and tests use
`postgres:14`). Mixing major client/server versions is avoidable and can
complicate debugging. Consider using `{{ include "database.image" . }}` (or a
dedicated, configurable client image tag) to keep versions aligned.
##########
charts/devlake/templates/statefulset-postgresql.yaml:
##########
@@ -0,0 +1,197 @@
+#
+# 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.
+#
+---
+# PostgreSQL database statefulset
+{{- if eq .Values.database.type "postgresql" }}
+{{- if not .Values.database.useExternal }}
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+ name: {{ include "devlake.fullname" . }}-postgresql
+ labels:
+ {{- include "devlake.labels" . | nindent 4 }}
+spec:
+ replicas: {{ if gt (int .Values.database.replicaCount) 1 }}1{{ else }}{{
.Values.database.replicaCount }}{{ end }}
+ serviceName: {{ include "devlake.fullname" . }}-postgresql
+ selector:
+ matchLabels:
+ app.kubernetes.io/name: {{ include "devlake.name" . }}
+ app.kubernetes.io/instance: {{ .Release.Name }}
+ template:
+ metadata:
+ labels:
+ app.kubernetes.io/name: {{ include "devlake.name" . }}
+ app.kubernetes.io/instance: {{ .Release.Name }}
+ devlakeComponent: postgresql
+ {{- with .Values.database.extraLabels }}
+ {{- toYaml . | nindent 8 }}
+ {{- end }}
+ annotations:
+ {{- toYaml .Values.database.podAnnotations | nindent 8 }}
+ spec:
+ {{- with .Values.imagePullSecrets }}
+ imagePullSecrets:
+ {{- toYaml . | nindent 8 }}
+ {{- end }}
+ securityContext:
+ runAsNonRoot: {{ .Values.database.securityContext.runAsNonRoot }}
+ runAsUser: {{ include "database.uid" . }}
+ fsGroup: {{ include "database.uid" . }}
+ seccompProfile:
+ type: {{ .Values.database.securityContext.seccompProfile.type }}
+ initContainers:
+ - name: fix-permissions
+ image: {{ include "database.image" . }}
+ command: ['sh', '-c']
+ args:
+ - |
+ chown -R 70:70 /var/lib/postgresql/data
+ chmod 700 /var/lib/postgresql/data
+ volumeMounts:
+ - name: data
+ mountPath: /var/lib/postgresql/data
+ securityContext:
+ runAsUser: 0
+ runAsNonRoot: false
+ {{- with .Values.database.initContainers}}
Review Comment:
The `fix-permissions` initContainer explicitly runs as root (`runAsUser: 0`,
`runAsNonRoot: false`). This will be rejected under Pod Security Standards
"restricted" and contradicts the chart’s stated restricted support. Consider
making this initContainer optional and/or switching to a non-root approach
(fsGroup/fsGroupChangePolicy) so PostgreSQL can run in restricted namespaces.
##########
charts/devlake/templates/backup-cronjob.yaml:
##########
@@ -0,0 +1,100 @@
+#
+# 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 .Values.backup.enabled }}
+apiVersion: batch/v1
+kind: CronJob
+metadata:
+ name: {{ include "devlake.fullname" . }}-backup
+ labels:
+ {{- include "devlake.labels" . | nindent 4 }}
+spec:
+ schedule: "{{ .Values.backup.schedule }}"
+ successfulJobsHistoryLimit: {{ .Values.backup.successfulJobsHistoryLimit }}
+ failedJobsHistoryLimit: {{ .Values.backup.failedJobsHistoryLimit }}
+ concurrencyPolicy: Forbid
+ jobTemplate:
+ spec:
+ backoffLimit: {{ .Values.backup.backoffLimit }}
+ template:
+ metadata:
+ labels:
+ {{- include "devlake.selectorLabels" . | nindent 12 }}
+ app.kubernetes.io/component: backup
+ spec:
+ restartPolicy: OnFailure
+ serviceAccountName: {{ include "devlake.serviceAccountName" . }}
+ securityContext:
+ runAsNonRoot: true
+ runAsUser: 65534
+ fsGroup: 65534
+ seccompProfile:
+ type: RuntimeDefault
+ containers:
+ - name: backup
+ image: "{{ .Values.backup.image.repository }}:{{
.Values.backup.image.tag }}"
+ imagePullPolicy: {{ .Values.backup.image.pullPolicy }}
+ securityContext:
+ readOnlyRootFilesystem: true
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop:
+ - ALL
+ envFrom:
+ - configMapRef:
+ name: {{ include "devlake.db.configmap" . }}
+ - secretRef:
+ name: {{ include "devlake.db.secret" . }}
+ command:
+ - sh
+ - -c
+ - |
+ set -e
+ TIMESTAMP=$(date +%Y%m%d-%H%M%S)
+ BACKUP_FILE="/backup/devlake-${TIMESTAMP}.sql.gz"
+
+ echo "Starting backup at ${TIMESTAMP}..."
+
+ {{- if eq .Values.database.type "mysql" }}
+ mysqldump -h$(MYSQL_SERVER) -P$(MYSQL_PORT) -u$(MYSQL_USER)
-p$(MYSQL_PASSWORD) $(MYSQL_DATABASE) \
+ --single-transaction --quick --lock-tables=false | gzip >
"${BACKUP_FILE}"
+ {{- else if eq .Values.database.type "postgresql" }}
+ PGPASSWORD=$(DB_PASSWORD) pg_dump -h $(DB_SERVER) -p $(DB_PORT)
-U $(DB_USER) $(DB_DATABASE) \
+ --no-owner --no-acl | gzip > "${BACKUP_FILE}"
+ {{- end }}
+
+ echo "Backup completed: ${BACKUP_FILE}"
+ ls -lh "${BACKUP_FILE}"
+
+ # Retention: delete backups older than specified days
+ {{- if .Values.backup.retentionDays }}
+ echo "Cleaning backups older than {{
.Values.backup.retentionDays }} days..."
+ find /backup -name "devlake-*.sql.gz" -type f -mtime +{{
.Values.backup.retentionDays }} -delete
+ {{- end }}
+
+ echo "Backup job finished successfully"
+ volumeMounts:
+ - name: backup
+ mountPath: /backup
+ - name: tmp
+ mountPath: /tmp
+ volumes:
+ - name: backup
+ persistentVolumeClaim:
+ claimName: {{ .Values.backup.pvc.existingClaim | default
(printf "%s-backup" (include "devlake.fullname" .)) }}
Review Comment:
The CronJob always mounts a PVC (defaulting to `<fullname>-backup`), but the
chart does not define a PersistentVolumeClaim resource. If users enable backups
without pre-creating that PVC (or setting `backup.pvc.existingClaim`), the
CronJob will fail to schedule. Either add a PVC template or require an existing
claim via validation/docs.
--
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]
