Initial Helm charts for openshift #2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Contributor
Author
|
@sourcery-ai review |
Reviewer's GuideInitial Helm chart to deploy the Kruize backend, PostgreSQL database, and UI (via NGINX) on Kubernetes/OpenShift, including configuration, RBAC, storage, monitoring, and a basic Helm test. Sequence diagram for monthly DB partition maintenance CronJobssequenceDiagram
participant KubeScheduler
participant CronCreate as CronJob_CreatePartition
participant CronDelete as CronJob_RetentionPartition
participant KruizeImage as Kruize_Container_Image
participant DBConfig as ConfigMap_kruize-config
participant DB as PostgreSQL_DB_Service
KubeScheduler->>CronCreate: Trigger_on_schedule(0_0_25_*_*)
CronCreate->>KruizeImage: Start_pod_with_CreatePartition_command
KruizeImage->>DBConfig: Mount_/etc/config/dbconfigjson_and_kruizeconfigjson
KruizeImage->>DB: Connect_using_configured_JDBC_URL
KruizeImage->>DB: Create_new_DB_partitions
DB-->>KruizeImage: Partitions_created
KruizeImage-->>CronCreate: Job_completed
KubeScheduler->>CronDelete: Trigger_on_schedule(0_0_25_*_*)
CronDelete->>KruizeImage: Start_pod_with_RetentionPartition_command
KruizeImage->>DBConfig: Read_deletepartitionsthreshold_from_values
KruizeImage->>DB: Delete_partitions_older_than_threshold
DB-->>KruizeImage: Old_partitions_removed
KruizeImage-->>CronDelete: Job_completed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 9 issues, and left some high level feedback:
- In
templates/cronjobs.yaml,image: {{ $image }}andname: ${{ $fullName }}-configare incorrect Helm usages (the former is an object, the latter has an extra$); these should reference the repository/tag string like in the main deployment and fix the ConfigMap name templating. - The generated JSON in
configmap-kruize.yamlis likely invalid because several string fields (e.g.,adminPassword,adminUsername,name,password,port,sslMode,username, and many config values) are not wrapped with| quote, so they will render as bare words instead of JSON strings; consider consistently applying| quoteto all string values in the JSON config. - In
kruize-deployment.yaml, the initContainer uses.Values.db.port(which doesn’t exist, should likely be.Values.db.service.port) and the containerlimitsincorrectly reuse therequestsvalues instead of.Values.kruize.resources.limits.*, so these should be corrected; also, theServiceMonitorselector usesapp: kruize, which doesn’t match the labels created viakruize.selectorLabels, so its selector/labels should be aligned.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `templates/cronjobs.yaml`, `image: {{ $image }}` and `name: ${{ $fullName }}-config` are incorrect Helm usages (the former is an object, the latter has an extra `$`); these should reference the repository/tag string like in the main deployment and fix the ConfigMap name templating.
- The generated JSON in `configmap-kruize.yaml` is likely invalid because several string fields (e.g., `adminPassword`, `adminUsername`, `name`, `password`, `port`, `sslMode`, `username`, and many config values) are not wrapped with `| quote`, so they will render as bare words instead of JSON strings; consider consistently applying `| quote` to all string values in the JSON config.
- In `kruize-deployment.yaml`, the initContainer uses `.Values.db.port` (which doesn’t exist, should likely be `.Values.db.service.port`) and the container `limits` incorrectly reuse the `requests` values instead of `.Values.kruize.resources.limits.*`, so these should be corrected; also, the `ServiceMonitor` selector uses `app: kruize`, which doesn’t match the labels created via `kruize.selectorLabels`, so its selector/labels should be aligned.
## Individual Comments
### Comment 1
<location> `charts/kruize/templates/cronjobs.yaml:33-34` </location>
<code_context>
+ template:
+ spec:
+ containers:
+ - name: kruize-cron-create
+ image: {{ $image }}
+ imagePullPolicy: IfNotPresent
+ volumeMounts:
</code_context>
<issue_to_address>
**issue (bug_risk):** CronJob image reference uses the entire image map instead of a concrete image string
`$image` is a map (`repository`, `tag`, `pullPolicy`), but it’s used directly as `image: {{ $image }}`, which won’t produce a valid image reference. Use a concrete string like `{{ .Values.kruize.image.repository }}:{{ .Values.kruize.image.tag }}` or a helper that builds the full image instead.
</issue_to_address>
### Comment 2
<location> `charts/kruize/templates/cronjobs.yaml:37-46` </location>
<code_context>
+ image: {{ $image }}
+ imagePullPolicy: IfNotPresent
+ volumeMounts:
+ - name: config-volume
+ mountPath: /etc/config
+ command:
+ - sh
+ - -c
+ - |
+ /home/autotune/app/target/bin/CreatePartition
+ env:
+ - name: START_AUTOTUNE
+ value: "false"
+ - name: LOGGING_LEVEL
+ value: {{ $loggingLevel | quote }}
+ - name: DB_CONFIG_FILE
+ value: {{ $dbConfigFile | quote }}
+ - name: KRUIZE_CONFIG_FILE
+ value: {{ $kruizeConfigFile | quote }}
+ volumes:
+ - name: config-volume
+ configMap:
+ name: {{ $fullName }}-config
+ restartPolicy: OnFailure
</code_context>
<issue_to_address>
**issue (bug_risk):** ConfigMap name in delete-partition CronJob has an extra `$` and will not render correctly
In the delete-partition CronJob this is written as `name: ${{ $fullName }}-config`, which is invalid Helm syntax and will either fail templating or render literally. Please change it to `name: {{ $fullName }}-config` to match the other resources.
</issue_to_address>
### Comment 3
<location> `charts/kruize/templates/configmap-kruize.yaml:12-19` </location>
<code_context>
+ dbconfigjson: |
+ {
+ "database": {
+ "adminPassword": {{ .Values.db.adminPassword }},
+ "adminUsername": {{ .Values.db.adminUser }},
+ "hostname": "{{ $fullName }}-db-service",
+ "name": {{ printf "%s-%s" .Values.db.name $fullName | quote }},
+ "password": {{ .Values.db.password }},
+ "port": {{ .Values.db.service.port }},
+ "sslMode": {{ .Values.db.sslMode }},
+ "username": {{ .Values.db.user }}
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** ConfigMap JSON is mixing unquoted Helm values into a JSON string, risking invalid JSON
Several fields in both `dbconfigjson` and `kruizeconfigjson` are rendered without `| quote` (e.g., `adminPassword`, `adminUsername`, `sslMode`, `clusterType`, `monitoringagent`). Because this must be valid JSON, unquoted strings or values with special characters can produce invalid JSON or unintended types. Please consistently `| quote` string fields and ensure booleans/integers are rendered as proper JSON literals.
</issue_to_address>
### Comment 4
<location> `charts/kruize/templates/kruize-deployment.yaml:28` </location>
<code_context>
+ - >
+ until pg_isready
+ -h "{{ $fullName }}-db-service"
+ -p {{ .Values.db.port | default 5432 }}
+ -U {{ .Values.db.user | quote }};
+ do echo "Waiting for database...";
</code_context>
<issue_to_address>
**issue (bug_risk):** Init container uses `.Values.db.port` which is not defined in `values.yaml`
Here you reference `-p {{ .Values.db.port | default 5432 }}`, but `values.yaml` only defines `db.service.port`, so it will always fall back to `5432` even when the DB port is overridden. Please use `db.service.port` (or a single shared value) to keep the port configuration consistent.
</issue_to_address>
### Comment 5
<location> `charts/kruize/templates/kruize-deployment.yaml:49-48` </location>
<code_context>
+ requests:
+ memory: {{ .Values.db.resources.requests.memory | quote }}
+ cpu: {{ .Values.db.resources.requests.cpu | quote }}
+ limits:
+ memory: {{ .Values.db.resources.limits.memory | quote }}
+ cpu: {{ .Values.db.resources.limits.cpu | quote }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Container limits are incorrectly wired to use `requests` values instead of `limits`
Under `resources.limits` you’re referencing `.Values.kruize.resources.requests.*` instead of the corresponding `limits` values. This ignores the `limits` section in `values.yaml` and tightly couples limits to requests. Please switch to `.Values.kruize.resources.limits.cpu` and `.Values.kruize.resources.limits.memory` so limits are configured independently.
</issue_to_address>
### Comment 6
<location> `charts/kruize/templates/kruize-deployment.yaml:37-38` </location>
<code_context>
+ - name: kruize-cron-create
+ image: {{ $image }}
+ imagePullPolicy: IfNotPresent
+ volumeMounts:
+ - name: config-volume
+ mountPath: /etc/config
+ command:
</code_context>
<issue_to_address>
**suggestion:** Container defines `volumeMounts` twice, which is redundant and potentially confusing
There are two `volumeMounts` entries mounting `config-volume` to `/etc/config`. Please consolidate into a single `volumeMounts` block to avoid duplicate keys and clarify the intent.
Suggested implementation:
```
- name: kruize-cron-create
image: {{ $image }}
imagePullPolicy: IfNotPresent
command:
```
This change assumes there is another `volumeMounts` block for `kruize-cron-create` elsewhere in the same container definition that already mounts `config-volume` to `/etc/config`. If that other block does not exist, you should instead keep a single `volumeMounts` block (either here or there) and ensure it contains all required mounts, while removing any duplicate `volumeMounts` keys so that only one remains under this container.
</issue_to_address>
### Comment 7
<location> `charts/kruize/templates/service-monitor.yaml:11-12` </location>
<code_context>
+ app: {{ $fullName }}-db
+spec:
+ replicas: 1
+ selector:
+ matchLabels:
+ app: {{ $fullName }}-db
+ template:
</code_context>
<issue_to_address>
**issue (bug_risk):** ServiceMonitor selector label does not match the labels applied to the Service
The ServiceMonitor currently matches on `app: kruize`, but the Service uses `kruize.labels` (`app.kubernetes.io/name` / `app.kubernetes.io/instance`) rather than a plain `app` label. This mismatch means the ServiceMonitor will never select the Service and metrics won’t be scraped. Please update the ServiceMonitor selector to use the same labels as the Service (e.g., `app.kubernetes.io/name`).
</issue_to_address>
### Comment 8
<location> `charts/kruize/templates/storage-pv.yaml:5-7` </location>
<code_context>
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: {{ $fullName }}-config
+ namespace: {{ .Release.Namespace }}
+data:
+ dbconfigjson: |
</code_context>
<issue_to_address>
**suggestion:** PersistentVolume is given a namespace, but PVs are cluster-scoped resources
Including `metadata.namespace` on a cluster-scoped PV is ignored by Kubernetes and can confuse readers; please remove that field from the manifest.
</issue_to_address>
### Comment 9
<location> `charts/kruize/templates/kruize-db-deployment.yaml:32-33` </location>
<code_context>
+ value: {{ .Values.db.user | quote }}
+ - name: POSTGRES_DB
+ value: {{ printf "%s-%s" .Values.db.name $fullName | quote }}
+ - name: PGDATA
+ value: /var/lib/pg_data
+ resources:
+ requests:
</code_context>
<issue_to_address>
**issue (bug_risk):** PGDATA does not align with the mounted volume path, risking non-persistent DB storage
`PGDATA` is set to `/var/lib/pg_data`, but the volume is mounted at `/var/lib/pgsql/data`, so Postgres may store data outside the PVC. Please update either `PGDATA` or the mount path so they match and data is actually written to the persistent volume.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- Database credentials are embedded directly in
values.yaml, passed via env vars, and rendered into a ConfigMap (dbconfigjson), which makes them easy to leak; consider switching to Kubernetes Secrets and referencing them from the chart instead of storing raw values in the config. - Several ClusterRoles use very broad permissions (e.g.
'*'verbs onpods,customresourcedefinitions,clusterrolebindingsand ananyuidSCC binding), which may be more permissive than necessary; it would be safer to scope these down to the specific verbs and resources actually required by Kruize. - CronJob schedules for partition management are hardcoded to
0 0 25 * *; making these cron expressions configurable via values would give operators flexibility to align them with their maintenance windows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Database credentials are embedded directly in `values.yaml`, passed via env vars, and rendered into a ConfigMap (`dbconfigjson`), which makes them easy to leak; consider switching to Kubernetes Secrets and referencing them from the chart instead of storing raw values in the config.
- Several ClusterRoles use very broad permissions (e.g. `'*'` verbs on `pods`, `customresourcedefinitions`, `clusterrolebindings` and an `anyuid` SCC binding), which may be more permissive than necessary; it would be safer to scope these down to the specific verbs and resources actually required by Kruize.
- CronJob schedules for partition management are hardcoded to `0 0 25 * *`; making these cron expressions configurable via values would give operators flexibility to align them with their maintenance windows.
## Individual Comments
### Comment 1
<location> `charts/kruize/templates/kruize_db_deployment.yaml:32-41` </location>
<code_context>
+ - name: PGDATA
</code_context>
<issue_to_address>
**issue (bug_risk):** Data directory env (PGDATA) and mounted volume path don't match, so DB data may not be persisted.
`PGDATA` is set to `/var/lib/pg_data` while the PVC is mounted at `/var/lib/pgsql/data`, so Postgres will store data outside the persistent volume and it won’t survive pod restarts. Please align `PGDATA` with the PVC mount path so the data directory resides on the persistent volume.
</issue_to_address>
### Comment 2
<location> `charts/kruize/templates/cronjobs.yaml:26-27` </location>
<code_context>
+metadata:
+ name: {{ .Release.Name }}-create-partition
+ namespace: {{ $namespace }}
+spec:
+ schedule: "0 0 25 * *" # Run on 25th of every month at midnight
+ jobTemplate:
+ spec:
</code_context>
<issue_to_address>
**suggestion:** CronJob schedules are hardcoded instead of being configurable via values.
Using the fixed schedule (`"0 0 25 * *"`) requires editing the template to change run times. Please move this into values (e.g. `.Values.cronJob.createSchedule` / `.Values.cronJob.deleteSchedule`) so operators can configure it per deployment.
Suggested implementation:
```
spec:
schedule: {{ default "0 0 25 * *" .Values.cronJob.createSchedule | quote }} # Run on 25th of every month at midnight
jobTemplate:
```
1. In `charts/kruize/values.yaml`, add a configurable value with the existing default, e.g.:
```yaml
cronJob:
createSchedule: "0 0 25 * *"
```
2. If this chart also defines a delete-partition CronJob, apply a similar change there using `.Values.cronJob.deleteSchedule` with an appropriate default, and document both values in `values.yaml` comments or chart README.
</issue_to_address>
### Comment 3
<location> `charts/kruize/templates/role.yaml:8-13` </location>
<code_context>
+metadata:
+ name: {{ $fullName }}-recommendation-updater
+rules:
+ - apiGroups:
+ - ""
+ resources:
+ - pods
+ - customresourcedefinitions
+ verbs:
+ - '*'
+ - apiGroups:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** ClusterRole grants very broad permissions (including `*` on clusterrolebindings), which may be excessive.
Using `verbs: ['*']` for `pods`, `customresourcedefinitions`, `verticalpodautoscalers`, and especially `clusterrolebindings` grants very broad RBAC permissions. Please restrict verbs to the minimal set actually required (e.g., `get`, `list`, `watch`, `patch`, `update`), and avoid `*` on `clusterrolebindings` unless it’s demonstrably necessary.
Suggested implementation:
```
rules:
- apiGroups:
- ""
resources:
- pods
- customresourcedefinitions
verbs:
- get
- list
- watch
- apiGroups:
- apiextensions.k8s.io
resources:
- customresourcedefinitions
verbs:
- get
- list
- watch
```
If the same ClusterRole (or others in this chart) currently grants `'*'` verbs on `verticalpodautoscalers`, `clusterrolebindings`, or other resources, those blocks should be updated similarly:
- Replace `verbs: ['*']` (or `- '*'`) with the exact verbs required (`get`, `list`, `watch`, `patch`, `update`, etc.).
- In particular, avoid broad verbs like `create`, `delete`, or `'*'` on `clusterrolebindings` unless the controller truly needs to manage them dynamically.
You’ll need to apply this pattern to any additional RBAC rules not shown in the snippet.
</issue_to_address>
### Comment 4
<location> `charts/kruize/values.yaml:145-150` </location>
<code_context>
+ hostPath: "/mnt/data"
+ ## @param db.user User for Kruize DB container
+ user: admin
+ ## @param db.password User for Kruize DB container
+ password: admin
+ ## @param db.adminUser Admin User for Kruize DB container
+ adminUser: admin
+ ## @param db.adminPassword Admin password for Kruize DB container
+ adminPassword: admin
+ ## @param db.name Name of the Kruize DB
+ name: kruizeDB
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Default DB credentials are weak and stored in plain text in values.yaml.
Using `admin`/`admin` as both user and admin credentials, and keeping them directly in `values.yaml`, is unsafe beyond demo use. Please switch to clearly non-production defaults with explicit documentation that they must be overridden, or route these through a Kubernetes Secret so real credentials are never stored in plain text chart values.
Suggested implementation:
```
pvc:
storageClass: manual
storageSize: "500Mi"
hostPath: "/mnt/data"
# NOTE: The following DB credentials are intended for local development / demo only.
# For any non-demo environment you MUST override these values (e.g. via `--set` or a
# custom values file) and, preferably, source them from a Kubernetes Secret instead
# of keeping real credentials in plain text Helm values.
#
# If the chart supports it, prefer configuring an existing Secret (e.g. `db.existingSecret`)
# and leaving these values unused in production.
## @param db.user User for Kruize DB container (demo-only default, override in real environments)
user: "kruize_user"
## @param db.password Password for Kruize DB container (demo-only placeholder, MUST override)
password: "CHANGE_ME_DB_PASSWORD"
## @param db.adminUser Admin user for Kruize DB container (demo-only default, override in real environments)
adminUser: "kruize_admin"
## @param db.adminPassword Admin password for Kruize DB container (demo-only placeholder, MUST override)
adminPassword: "CHANGE_ME_DB_ADMIN_PASSWORD"
## @param db.name Name of the Kruize DB
name: kruizeDB
sslMode: require
```
If the chart does not yet support loading DB credentials from a Kubernetes Secret, consider:
1. Adding values such as `db.existingSecret` and keys for username/password/admin credentials.
2. Updating the DB Deployment/StatefulSet template to source env vars from that Secret when configured.
This will allow real credentials to be managed securely without living in `values.yaml`.
</issue_to_address>
### Comment 5
<location> `charts/kruize/README.md:3` </location>
<code_context>
+# Kruize Helm Chart
+
+A Helm chart for deploying [Kruize]((https://github.com/kruize/autotune) on Kubernetes/OpenShift clusters.
+
+## Introduction
</code_context>
<issue_to_address>
**issue (typo):** Fix the markdown link syntax for the Kruize GitHub repository.
There’s an extra opening parenthesis in the link. It should be `[Kruize](https://github.com/kruize/autotune)` so the URL renders correctly.
```suggestion
A Helm chart for deploying [Kruize](https://github.com/kruize/autotune) on Kubernetes/OpenShift clusters.
```
</issue_to_address>
### Comment 6
<location> `charts/kruize/README.md:142` </location>
<code_context>
+|-----------|-------------|---------|
+| `serviceAccount.create` | Specifies whether a service account should be created | `true` |
+| `serviceAccount.name` | The name of the service account to use | `""` |
+| `rbac.create` | Specifies whether OpenShift specific RBAC resources should be created | `true` |
+| `nameOverride` | Overrides the name of this Chart | `""` |
+| `fullnameOverride` | Overrides the fully qualified application name | `""` |
</code_context>
<issue_to_address>
**nitpick (typo):** Consider hyphenating "OpenShift-specific" for correct grammar.
Here it appears before “RBAC resources,” so it should be written as “OpenShift-specific.”
```suggestion
| `rbac.create` | Specifies whether OpenShift-specific RBAC resources should be created | `true` |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+8
to
+13
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - pods | ||
| - customresourcedefinitions | ||
| verbs: |
There was a problem hiding this comment.
🚨 suggestion (security): ClusterRole grants very broad permissions (including * on clusterrolebindings), which may be excessive.
Using verbs: ['*'] for pods, customresourcedefinitions, verticalpodautoscalers, and especially clusterrolebindings grants very broad RBAC permissions. Please restrict verbs to the minimal set actually required (e.g., get, list, watch, patch, update), and avoid * on clusterrolebindings unless it’s demonstrably necessary.
Suggested implementation:
rules:
- apiGroups:
- ""
resources:
- pods
- customresourcedefinitions
verbs:
- get
- list
- watch
- apiGroups:
- apiextensions.k8s.io
resources:
- customresourcedefinitions
verbs:
- get
- list
- watch
If the same ClusterRole (or others in this chart) currently grants '*' verbs on verticalpodautoscalers, clusterrolebindings, or other resources, those blocks should be updated similarly:
- Replace
verbs: ['*'](or- '*') with the exact verbs required (get,list,watch,patch,update, etc.). - In particular, avoid broad verbs like
create,delete, or'*'onclusterrolebindingsunless the controller truly needs to manage them dynamically.
You’ll need to apply this pattern to any additional RBAC rules not shown in the snippet.
Comment on lines
+145
to
+150
| ## @param db.password User for Kruize DB container | ||
| password: admin | ||
| ## @param db.adminUser Admin User for Kruize DB container | ||
| adminUser: admin | ||
| ## @param db.adminPassword Admin password for Kruize DB container | ||
| adminPassword: admin |
There was a problem hiding this comment.
🚨 suggestion (security): Default DB credentials are weak and stored in plain text in values.yaml.
Using admin/admin as both user and admin credentials, and keeping them directly in values.yaml, is unsafe beyond demo use. Please switch to clearly non-production defaults with explicit documentation that they must be overridden, or route these through a Kubernetes Secret so real credentials are never stored in plain text chart values.
Suggested implementation:
pvc:
storageClass: manual
storageSize: "500Mi"
hostPath: "/mnt/data"
# NOTE: The following DB credentials are intended for local development / demo only.
# For any non-demo environment you MUST override these values (e.g. via `--set` or a
# custom values file) and, preferably, source them from a Kubernetes Secret instead
# of keeping real credentials in plain text Helm values.
#
# If the chart supports it, prefer configuring an existing Secret (e.g. `db.existingSecret`)
# and leaving these values unused in production.
## @param db.user User for Kruize DB container (demo-only default, override in real environments)
user: "kruize_user"
## @param db.password Password for Kruize DB container (demo-only placeholder, MUST override)
password: "CHANGE_ME_DB_PASSWORD"
## @param db.adminUser Admin user for Kruize DB container (demo-only default, override in real environments)
adminUser: "kruize_admin"
## @param db.adminPassword Admin password for Kruize DB container (demo-only placeholder, MUST override)
adminPassword: "CHANGE_ME_DB_ADMIN_PASSWORD"
## @param db.name Name of the Kruize DB
name: kruizeDB
sslMode: require
If the chart does not yet support loading DB credentials from a Kubernetes Secret, consider:
- Adding values such as
db.existingSecretand keys for username/password/admin credentials. - Updating the DB Deployment/StatefulSet template to source env vars from that Secret when configured.
This will allow real credentials to be managed securely without living invalues.yaml.
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Database credentials are stored directly in values.yaml and rendered into a ConfigMap/env vars; consider switching these to Kubernetes Secrets (and templated Secret references) so that sensitive values are not committed or exposed in plain text.
- The Postgres storage uses a statically defined hostPath PersistentVolume with a hardcoded storageClass and ReadWriteMany, which is unlikely to be portable across OpenShift/Kubernetes clusters; it would be better to make this storage optional and/or configurable and rely on dynamic provisioning where possible.
- The UI is defined as a standalone Pod in kruize_ui_nginx_pod.yaml, so it won’t be rescheduled automatically on failure or node drain; consider changing this to a Deployment (or similar controller) for better availability and lifecycle management.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Database credentials are stored directly in values.yaml and rendered into a ConfigMap/env vars; consider switching these to Kubernetes Secrets (and templated Secret references) so that sensitive values are not committed or exposed in plain text.
- The Postgres storage uses a statically defined hostPath PersistentVolume with a hardcoded storageClass and ReadWriteMany, which is unlikely to be portable across OpenShift/Kubernetes clusters; it would be better to make this storage optional and/or configurable and rely on dynamic provisioning where possible.
- The UI is defined as a standalone Pod in kruize_ui_nginx_pod.yaml, so it won’t be rescheduled automatically on failure or node drain; consider changing this to a Deployment (or similar controller) for better availability and lifecycle management.
## Individual Comments
### Comment 1
<location> `charts/kruize/templates/cronjobs.yaml:34-35` </location>
<code_context>
+ containers:
+ - name: kruize-cron-create
+ image: {{ $image }}
+ imagePullPolicy: IfNotPresent
+ volumeMounts:
+ - name: config-volume
</code_context>
<issue_to_address>
**suggestion:** CronJobs hardcode `imagePullPolicy` instead of reusing the main Kruize image pull policy.
These CronJobs use a hardcoded `imagePullPolicy: IfNotPresent` while the main deployment uses `.Values.kruize.image.pullPolicy`. Please reference the same value here (e.g. `imagePullPolicy: {{ .Values.kruize.image.pullPolicy }}`) so pull behavior remains consistent and configurable across environments.
```suggestion
image: {{ $image }}
imagePullPolicy: {{ .Values.kruize.image.pullPolicy }}
```
</issue_to_address>
### Comment 2
<location> `charts/kruize/templates/role.yaml:8-10` </location>
<code_context>
+metadata:
+ name: {{ $fullName }}-recommendation-updater
+rules:
+ - apiGroups:
+ - ""
+ resources:
+ - pods
+ - customresourcedefinitions
</code_context>
<issue_to_address>
**🚨 suggestion (security):** ClusterRole grants very broad permissions (including `*` on pods and CRDs), which may be more permissive than necessary.
Using `verbs: ['*']` on `pods`, `customresourcedefinitions`, and other resources significantly increases the impact of a compromised service account. Please restrict verbs to the specific operations Kruize actually performs (e.g., `get`, `list`, `watch`, `patch`, etc.) and narrow the resource list accordingly.
Suggested implementation:
```
rules:
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- patch
- update
- apiGroups:
- apiextensions.k8s.io
resources:
- customresourcedefinitions
verbs:
- get
- list
- watch
```
You should further refine the verbs and resources based on the actual operations performed by the recommendation-updater:
1. If it only reads pod specs and CRDs, drop `patch`/`update` and keep only `get`, `list`, and `watch`.
2. If it interacts with additional resources (e.g., specific CRDs for recommendations), add those resource names explicitly instead of using generic wildcards.
3. Verify whether access to `customresourcedefinitions` is required at all; if not, remove that rule entirely.
</issue_to_address>
### Comment 3
<location> `charts/kruize/README.md:59` </location>
<code_context>
+
+| Parameter | Description | Default |
+|-----------|-------------|---------|
+| `kruize.config.clusterType` | Type of cluster (kubernetes/openshift) | `kubernetes` |
+| `kruize.config.k8sType` | Kubernetes distribution type | `openshift` |
+| `kruize.config.authType` | Authentication type | `""` |
</code_context>
<issue_to_address>
**nitpick (typo):** Capitalize Kubernetes and OpenShift in the description for consistency with proper nouns used elsewhere.
In the description `Type of cluster (kubernetes/openshift)`, please capitalize the platform names to `Type of cluster (Kubernetes/OpenShift)` for consistency with their use as proper nouns elsewhere in the README. The default value `kubernetes` can remain lowercase as-is.
```suggestion
| `kruize.config.clusterType` | Type of cluster (Kubernetes/OpenShift) | `kubernetes` |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+34
to
+35
| image: {{ $image }} | ||
| imagePullPolicy: IfNotPresent |
There was a problem hiding this comment.
suggestion: CronJobs hardcode imagePullPolicy instead of reusing the main Kruize image pull policy.
These CronJobs use a hardcoded imagePullPolicy: IfNotPresent while the main deployment uses .Values.kruize.image.pullPolicy. Please reference the same value here (e.g. imagePullPolicy: {{ .Values.kruize.image.pullPolicy }}) so pull behavior remains consistent and configurable across environments.
Suggested change
| image: {{ $image }} | |
| imagePullPolicy: IfNotPresent | |
| image: {{ $image }} | |
| imagePullPolicy: {{ .Values.kruize.image.pullPolicy }} |
Comment on lines
+8
to
+10
| - apiGroups: | ||
| - "" | ||
| resources: |
There was a problem hiding this comment.
🚨 suggestion (security): ClusterRole grants very broad permissions (including * on pods and CRDs), which may be more permissive than necessary.
Using verbs: ['*'] on pods, customresourcedefinitions, and other resources significantly increases the impact of a compromised service account. Please restrict verbs to the specific operations Kruize actually performs (e.g., get, list, watch, patch, etc.) and narrow the resource list accordingly.
Suggested implementation:
rules:
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- patch
- update
- apiGroups:
- apiextensions.k8s.io
resources:
- customresourcedefinitions
verbs:
- get
- list
- watch
You should further refine the verbs and resources based on the actual operations performed by the recommendation-updater:
- If it only reads pod specs and CRDs, drop
patch/updateand keep onlyget,list, andwatch. - If it interacts with additional resources (e.g., specific CRDs for recommendations), add those resource names explicitly instead of using generic wildcards.
- Verify whether access to
customresourcedefinitionsis required at all; if not, remove that rule entirely.
|
|
||
| | Parameter | Description | Default | | ||
| |-----------|-------------|---------| | ||
| | `kruize.config.clusterType` | Type of cluster (kubernetes/openshift) | `kubernetes` | |
There was a problem hiding this comment.
nitpick (typo): Capitalize Kubernetes and OpenShift in the description for consistency with proper nouns used elsewhere.
In the description Type of cluster (kubernetes/openshift), please capitalize the platform names to Type of cluster (Kubernetes/OpenShift) for consistency with their use as proper nouns elsewhere in the README. The default value kubernetes can remain lowercase as-is.
Suggested change
| | `kruize.config.clusterType` | Type of cluster (kubernetes/openshift) | `kubernetes` | | |
| | `kruize.config.clusterType` | Type of cluster (Kubernetes/OpenShift) | `kubernetes` | |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains the initial Helm charts for deploying Kruize on Openshift cluster.
Summary by Sourcery
Add a new Helm chart to deploy the Kruize application, its database, and UI on Kubernetes/OpenShift with opinionated defaults and OpenShift-specific integration.
New Features:
Enhancements:
Documentation: