Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Helm chart for Cloud Native PostgreSQL (CNPG) and integrates it into the onechart dependency list. The review identifies several critical issues, including hardcoded sensitive credentials, an invalid image tag, and non-idempotent password generation that would cause unintended rotations during upgrades. Additionally, the feedback suggests cleaning up unused variables, unreachable template logic, and correcting placeholder metadata in the Chart.yaml file.
| type: {{ .Values.postgresAuth.type | default "kubernetes.io/basic-auth" }} | ||
| stringData: | ||
| username: {{ .Values.postgresAuth.username | quote }} | ||
| password: {{ .Values.postgresAuth.password | quote }} |
There was a problem hiding this comment.
To prevent deployments with weak or empty default passwords, it's crucial to enforce that a password is provided by the user. Using the required function will cause the Helm release to fail if the password is not set, ensuring a secure configuration.
password: {{ required "A password is required for .Values.postgresAuth.password" .Values.postgresAuth.password | quote }}| enabled: true | ||
|
|
||
| cluster: | ||
| imageName: ghcr.io/cloudnative-pg/postgresql:16.8-13-bullseye |
There was a problem hiding this comment.
The image tag 16.8-13-bullseye for ghcr.io/cloudnative-pg/postgresql appears to be invalid. The standard tags for this image are typically just the PostgreSQL version number (e.g., 16.2). An incorrect image tag will prevent pods from starting with an ImagePullBackOff error. Please use a valid image tag.
imageName: ghcr.io/cloudnative-pg/postgresql:16.2
charts/cnpg/values.yaml
Outdated
| postgresAuth: | ||
| create: true | ||
| username: cnpg | ||
| password: P@ssw0rd123 |
There was a problem hiding this comment.
Hardcoding a default password is a critical security vulnerability. This value should be removed, and the chart should require the user to provide a password. I've added a separate comment in templates/secret-postgres.yaml to enforce this with a required check.
password: ""
charts/cnpg/values.yaml
Outdated
| accessKeyValue: accesskey | ||
| secretKeyValue: secretkey |
There was a problem hiding this comment.
Default credentials (accesskey and secretkey) are hardcoded in the values file. This is a critical security risk. These should be removed to force the user to provide their own credentials. The template templates/secret-s3.yaml already correctly uses the required function, so this change will enforce that credentials are provided.
accessKeyValue: ""
secretKeyValue: ""| type: {{ default "kubernetes.io/basic-auth" }} | ||
| stringData: | ||
| username: postgres | ||
| password: {{ randAlphaNum 8 | quote }} |
There was a problem hiding this comment.
Using randAlphaNum here will generate a new password on every helm upgrade. This non-idempotent behavior will cause the postgres superuser password to be rotated with every deployment, which can lead to service disruptions for any applications that rely on this password. Additionally, a password length of 8 characters is insufficient for production environments.
For a more robust solution, consider generating the password only on initial installation and keeping it stable for subsequent upgrades. A common pattern is to use the lookup function to check if the secret already exists.
As a minimal improvement, please increase the password length.
password: {{ randAlphaNum 32 | quote }}
charts/cnpg/Chart.yaml
Outdated
| @@ -0,0 +1,8 @@ | |||
| apiVersion: v1 | |||
| description: CNPG (China Postgres) | |||
There was a problem hiding this comment.
The description "CNPG (China Postgres)" is likely a misunderstanding. CNPG stands for "Cloud Native PostgreSQL". Using an incorrect name can be misleading for users of this chart.
description: A Helm chart for Cloud Native PostgreSQL (CNPG)| - name: Developer | ||
| email: dev@opsta.in.th |
There was a problem hiding this comment.
The maintainer information appears to be using placeholder values. Please update this section with the actual maintainer's details for proper ownership and contact purposes.
- name: <Your Name>
email: <your-email@example.com>| @@ -0,0 +1,69 @@ | |||
| {{- if .Values.enabled }} | |||
| #{{- $cnpg := .Values.cnpg -}} | |||
There was a problem hiding this comment.
The variable $cnpg is defined here but is not used anywhere in the template. This unused variable should be removed to improve code clarity and maintainability.
| {{- if and $backup.enabled (not $s3SecretName) -}} | ||
| {{- fail "backup.enabled=true but S3 secret name resolved empty (unexpected)" -}} | ||
| {{ end }} |
There was a problem hiding this comment.
This conditional check is currently unreachable. The variable $s3SecretName is always assigned a value from printf, so (not $s3SecretName) will never evaluate to true. This block of code serves no purpose and should be removed.
|
Please use cnpg-chart repo instead. |
2 participants
Add Cloud Native Postgres Template