chore(seaweedfs): bump SeaweedFS to 4.31 - #2834
Merged
Merged
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the SeaweedFS Helm chart to version 4.31. The changes involve adapting the local package configuration to breaking changes introduced in recent upstream releases, specifically regarding global namespace structure and native support for S3 traffic distribution. The update also includes new unit tests to ensure configuration consistency across various Kubernetes environments. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Contributor
📝 WalkthroughWalkthroughMigrates SeaweedFS Helm chart to seaweedfs.* helpers and global.seaweedfs values; updates templates across components, adds OpenShift profile, new ingresses (including S3 Iceberg/volume), refactors security/cert wiring, implements version-gated trafficDistribution with tests, updates docs/Grafana, and adds a Makefile ChangesSeaweedFS chart migration to seaweedfs. helpers and values*
Sequence Diagram(s)(silent) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds a test target to the SeaweedFS Makefile, transitions from a hardcoded patch to the native chart's trafficDistribution setting, and namespaces global values under global.seaweedfs. It also introduces a unit test suite to verify traffic distribution behavior. However, the tests incorrectly use majorVersion and minorVersion under capabilities, which is unsupported by helm-unittest. The reviewer recommends replacing these with the kubeVersion map to ensure correct Kubernetes version mocking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+18
to
+20
| capabilities: | ||
| majorVersion: 1 | ||
| minorVersion: 33 |
Contributor
There was a problem hiding this comment.
In helm-unittest, the capabilities block does not support majorVersion and minorVersion directly. Instead, you should use the kubeVersion map with version, major, and minor fields. Without this, the Kubernetes version will not be mocked correctly, and the test will run against the default Helm client version.
capabilities:
kubeVersion:
version: 1.33.0
major: "1"
minor: "33"
Contributor
Author
There was a problem hiding this comment.
In helm-unittest 1.0.3 (the version pinned in this repo) capabilities.majorVersion/minorVersion is honored. I verified the suite mocks the version per test: it asserts PreferClose at 1.33, PreferSameZone at 1.35, and the field absent at 1.30, and all three pass — only possible if each test renders at its own version. A control case asserting PreferClose at 1.30 fails with unknown path spec.trafficDistribution, confirming the gate is exercised. Keeping the current form.
Comment on lines
+29
to
+31
| capabilities: | ||
| majorVersion: 1 | ||
| minorVersion: 35 |
Contributor
There was a problem hiding this comment.
In helm-unittest, the capabilities block does not support majorVersion and minorVersion directly. Instead, you should use the kubeVersion map with version, major, and minor fields. Without this, the Kubernetes version will not be mocked correctly, and the test will run against the default Helm client version.
capabilities:
kubeVersion:
version: 1.35.0
major: "1"
minor: "35"
Contributor
Author
There was a problem hiding this comment.
Same conclusion as the related thread on this file: capabilities.majorVersion/minorVersion is honored by helm-unittest 1.0.3, and I verified the kube version is mocked per test (a control asserting the field at 1.30 fails with unknown path spec.trafficDistribution). Keeping the current form.
Comment on lines
+38
to
+40
| capabilities: | ||
| majorVersion: 1 | ||
| minorVersion: 30 |
Contributor
There was a problem hiding this comment.
In helm-unittest, the capabilities block does not support majorVersion and minorVersion directly. Instead, you should use the kubeVersion map with version, major, and minor fields. Without this, the Kubernetes version will not be mocked correctly, and the test will run against the default Helm client version.
capabilities:
kubeVersion:
version: 1.30.0
major: "1"
minor: "30"
Contributor
Author
There was a problem hiding this comment.
Same conclusion as the related thread on this file: capabilities.majorVersion/minorVersion is honored by helm-unittest 1.0.3, and I verified the kube version is mocked per test (a control asserting the field at 1.30 fails with unknown path spec.trafficDistribution). Keeping the current form.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yaml (1)
84-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid emitting duplicate environment variables from global/component maps.
Line 84 computes a merged map, but Lines 103-128 still render two independent loops. If a key is present in both maps, duplicate
env.nameentries are emitted and precedence flips unexpectedly toward global values.Proposed fix
- {{- if .Values.allInOne.extraEnvironmentVars }} - {{- range $key, $value := .Values.allInOne.extraEnvironmentVars }} - {{- if and (ne $key $clusterMasterKey) (ne $key $clusterFilerKey) }} - - name: {{ $key }} - {{- if kindIs "string" $value }} - value: {{ tpl $value $ | quote }} - {{- else }} - valueFrom: - {{ toYaml $value | nindent 16 }} - {{- end }} - {{- end }} - {{- end }} - {{- end }} - {{- if .Values.global.seaweedfs.extraEnvironmentVars }} - {{- range $key, $value := .Values.global.seaweedfs.extraEnvironmentVars }} + {{- $mergedExtraEnvironmentVars := dict }} + {{- include "seaweedfs.mergeExtraEnvironmentVars" (dict "global" .Values.global.seaweedfs "component" .Values.allInOne "target" $mergedExtraEnvironmentVars) }} + {{- range $key := keys $mergedExtraEnvironmentVars | sortAlpha }} + {{- $value := index $mergedExtraEnvironmentVars $key }} {{- if and (ne $key $clusterMasterKey) (ne $key $clusterFilerKey) }} - name: {{ $key }} {{- if kindIs "string" $value }} value: {{ tpl $value $ | quote }} {{- else }} valueFrom: {{ toYaml $value | nindent 16 }} {{- end }} {{- end }} {{- end }} - {{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yaml` around lines 84 - 128, The template currently loops global and component env maps separately causing duplicate env entries; instead iterate over the previously computed merged map $envMerged (which was created as merge (.Values.global.seaweedfs.extraEnvironmentVars | default dict) (.Values.allInOne.extraEnvironmentVars | default dict) so component keys win) and render each entry once while skipping the reserved keys $clusterMasterKey and $clusterFilerKey; inside the single range (range $key, $value := $envMerged) use the same kindIs "string" check to emit value: {{ tpl $value $ | quote }} for strings and valueFrom: {{ toYaml $value | nindent 16 }} for non-strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/seaweedfs/charts/seaweedfs/openshift-values.yaml`:
- Around line 26-32: The global replication keys are defined at the wrong level;
move enableReplication and replicationPlacement under a seaweedfs map so the
chart templates (e.g., master-statefulset which checks
.Values.global.seaweedfs.enableReplication) will read them. Update the YAML to
create .Values.global.seaweedfs.enableReplication and
.Values.global.seaweedfs.replicationPlacement (preserve existing values like
true and "000") so the templates that reference .Values.global.seaweedfs.* pick
them up.
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/shared/post-install-bucket-hook.yaml`:
- Around line 143-152: The template treats boolean false (.versioning as bool)
as a no-op while string "false" maps to "Suspended", causing inconsistent
behavior; update the conditional for kindIs "bool" .versioning in the chart
template to explicitly set $bucketVersioning = "Suspended" when .versioning is
false (mirror the string branch), so both boolean false and string values like
"false"/"suspended"/"disable" result in Suspended and true/enable/true result in
Enabled; modify the block that sets $bucketVersioning (referencing .versioning
and $bucketVersioning) to handle both bool and string cases consistently.
---
Outside diff comments:
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yaml`:
- Around line 84-128: The template currently loops global and component env maps
separately causing duplicate env entries; instead iterate over the previously
computed merged map $envMerged (which was created as merge
(.Values.global.seaweedfs.extraEnvironmentVars | default dict)
(.Values.allInOne.extraEnvironmentVars | default dict) so component keys win)
and render each entry once while skipping the reserved keys $clusterMasterKey
and $clusterFilerKey; inside the single range (range $key, $value := $envMerged)
use the same kindIs "string" check to emit value: {{ tpl $value $ | quote }} for
strings and valueFrom: {{ toYaml $value | nindent 16 }} for non-strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07cb636c-c45a-48a8-a688-526cb689d253
📒 Files selected for processing (68)
packages/system/seaweedfs/Makefilepackages/system/seaweedfs/charts/seaweedfs/Chart.yamlpackages/system/seaweedfs/charts/seaweedfs/README.mdpackages/system/seaweedfs/charts/seaweedfs/dashboards/seaweedfs-grafana-dashboard.jsonpackages/system/seaweedfs/charts/seaweedfs/openshift-values.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-secret.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-pvc.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-service.ymlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/admin-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/ca-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/cert-caissuer.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/cert-issuer.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/client-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/filer-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/master-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/volume-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/worker-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-cluster-role.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-service-account.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-service-client.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-secret.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-secret.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/_compat.tplpackages/system/seaweedfs/charts/seaweedfs/templates/shared/_helpers.tplpackages/system/seaweedfs/charts/seaweedfs/templates/shared/cluster-role.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/notification-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/post-install-bucket-hook.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/seaweedfs-grafana-dashboard.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/secret-seaweedfs-db.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/service-account.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-resize-hook.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/values.yamlpackages/system/seaweedfs/patches/s3-traffic-distribution.patchpackages/system/seaweedfs/tests/s3_traffic_distribution_test.yamlpackages/system/seaweedfs/values.yaml
💤 Files with no reviewable changes (1)
- packages/system/seaweedfs/patches/s3-traffic-distribution.patch
Comment on lines
+26
to
+32
| global: | ||
| enableReplication: true | ||
| # replication type is XYZ: | ||
| # X number of replica in other data centers | ||
| # Y number of replica in other racks in the same data center | ||
| # Z number of replica in other servers in the same rack | ||
| replicationPlacement: "000" # no data replica |
Contributor
There was a problem hiding this comment.
Fix the global namespace structure to match the chart contract.
The templates expect .Values.global.seaweedfs.enableReplication and .Values.global.seaweedfs.replicationPlacement, but this file creates .Values.global.enableReplication directly under global:. The seaweedfs: nesting level is missing, so these replication settings will be silently ignored and the master will fall back to the default behavior.
Based on the master-statefulset template (context snippet 1), which checks {{- if .Values.global.seaweedfs.enableReplication }}.
🔧 Proposed fix
global:
- enableReplication: true
- # replication type is XYZ:
- # X number of replica in other data centers
- # Y number of replica in other racks in the same data center
- # Z number of replica in other servers in the same rack
- replicationPlacement: "000" # no data replica
+ seaweedfs:
+ enableReplication: true
+ # replication type is XYZ:
+ # X number of replica in other data centers
+ # Y number of replica in other racks in the same data center
+ # Z number of replica in other servers in the same rack
+ replicationPlacement: "000" # no data replica🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/seaweedfs/charts/seaweedfs/openshift-values.yaml` around
lines 26 - 32, The global replication keys are defined at the wrong level; move
enableReplication and replicationPlacement under a seaweedfs map so the chart
templates (e.g., master-statefulset which checks
.Values.global.seaweedfs.enableReplication) will read them. Update the YAML to
create .Values.global.seaweedfs.enableReplication and
.Values.global.seaweedfs.replicationPlacement (preserve existing values like
true and "000") so the templates that reference .Values.global.seaweedfs.* pick
them up.
Contributor
Author
There was a problem hiding this comment.
openshift-values.yaml is part of the vendored upstream chart (pulled via make update from k8s/charts/seaweedfs), so I don't hand-edit the vendored copy — chart fixes go upstream. It is also not consumed by this package, and the chart's _compat.tpl shim (included by all templates, with enableReplication/replicationPlacement in its scalar list) backfills these into global.seaweedfs.*, so they aren't actually dropped. Leaving as-is here.
Contributor
There was a problem hiding this comment.
@lexfrei, understood — two separate reasons make this a non-issue:
- Vendored file:
openshift-values.yamlcomes straight from upstreamk8s/charts/seaweedfsviamake updateand must not be hand-edited per cozystack convention. _compat.tplshim: the compat helper (included by every template) copies the flatglobal.enableReplication/global.replicationPlacementscalars intoglobal.seaweedfs.*, so the values are not silently dropped even if someone uses the old layout.
Thanks for the clear explanation — I'll record this for future reviews.
✏️ Learnings added
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2834
File: packages/system/seaweedfs/charts/seaweedfs/openshift-values.yaml:26-32
Timestamp: 2026-06-05T14:00:00.047Z
Learning: In cozystack/cozystack, `packages/system/seaweedfs/charts/seaweedfs/openshift-values.yaml` is vendored from the upstream SeaweedFS chart (`k8s/charts/seaweedfs`) via `make update` and must not be hand-edited. It is not consumed by the cozystack package's own `values.yaml`. The chart's `_compat.tpl` shim (templates/shared/_compat.tpl, included by all templates) copies scalar keys such as `enableReplication` and `replicationPlacement` from `global.*` into `global.seaweedfs.*`, so the flat-namespace layout used in `openshift-values.yaml` is fully supported at runtime. Do not flag the global namespace structure in this vendored file as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2699
File: packages/system/metallb/charts/metallb/charts/frr-k8s/values.schema.json:307-309
Timestamp: 2026-05-20T22:57:05.486Z
Learning: Files under `packages/system/metallb/charts/metallb/charts/frr-k8s/` (including `values.schema.json` and all other templates/values) are vendored directly from the upstream metallb Helm chart and must not be manually edited in cozystack. Bugs found in these files should be reported upstream to the metallb project; fixes arrive via a chart version bump and re-vendoring.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2464
File: packages/system/cilium/charts/cilium/values.yaml.tmpl:1177-1190
Timestamp: 2026-04-23T11:10:34.283Z
Learning: In cozystack/cozystack, the directory `packages/system/cilium/charts/cilium/` contains vendored upstream Cilium Helm chart files regenerated via `make update` (running `make -C packages/system/cilium update`). Cozystack does not modify these vendored files directly. Upstream-sourced defaults (e.g., hardcoded `docker.io/istio/ztunnel` image with `digest: ~` and `useDigest: false` in the ztunnel encryption block of `values.yaml.tmpl`) should not be flagged as issues — they come straight from upstream. Any Cozystack-specific image mirroring or digest-pinning for non-enabled features is handled in the image-mirror post-processing layer, not in the vendored chart templates.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2464
File: packages/system/cilium/charts/cilium/templates/ztunnel/secret.yaml:20-21
Timestamp: 2026-04-23T11:10:34.978Z
Learning: In the cozystack/cozystack repository, the `packages/system/cilium/charts/cilium/` directory contains the vendored upstream Cilium Helm chart, refreshed via `make update` in `packages/system/cilium/Makefile`. Cozystack does not modify these vendored templates directly. Issues found in those templates should be reported upstream at https://github.com/cilium/cilium, not patched locally. Additionally, the ztunnel encryption feature (`encryption.type=ztunnel`) is not enabled in Cozystack's own values files, so the ztunnel templates are not rendered in practice. Do not flag issues in `packages/system/cilium/charts/` as actionable for this repository.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2744
File: packages/system/trust-manager-crds/templates/_helpers.tpl:27-29
Timestamp: 2026-05-26T15:27:50.095Z
Learning: In the cozystack repository, `packages/system/trust-manager-crds/templates/_helpers.tpl` and `packages/system/cert-manager-crds/templates/_helpers.tpl` are vendored verbatim from the upstream `jetstack/trust-manager` and `jetstack/cert-manager` Helm charts respectively, populated by `make update`. Per cozystack convention, vendored chart code is never patched locally — any fixes must land in the upstream charts first. Do not suggest local modifications to these vendored helper templates.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2744
File: packages/system/trust-manager-crds/templates/_helpers.tpl:27-29
Timestamp: 2026-05-26T15:39:27.724Z
Learning: In the cozystack repository, vendored Helm chart templates under `packages/system/trust-manager-crds/templates/` and `packages/system/cert-manager-crds/templates/` are never patched locally — any fixes to the templates must land in the upstream `jetstack/trust-manager` and `jetstack/cert-manager` charts first, then re-vendored via `make update`. Do not suggest local modifications to these vendored helper templates.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2647
File: packages/system/kuberture/charts/kuberture/templates/NOTES.txt:20-20
Timestamp: 2026-05-14T13:52:59.241Z
Learning: In `packages/system/kuberture`, the vendored upstream chart under `charts/kuberture/` is pinned by OCI chart-manifest digest in the `Makefile` (`KUBERTURE_CHART_DIGEST`). Local patches to vendored chart templates are NOT part of the cozystack vendoring contract — changes must be made upstream in `lexfrei/kuberture` and pulled in via a chart-version bump. Avoid suggesting direct edits to files under `packages/system/kuberture/charts/kuberture/`.
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T10:16:39.753Z
Learning: Do NOT edit `/vendor/` directory, auto-generated files (`zz_generated.*.go`), or upstream charts directly
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2699
File: packages/system/metallb/charts/metallb/templates/controller.yaml:74-85
Timestamp: 2026-05-20T22:57:08.692Z
Learning: In the cozystack/cozystack repository, files under `packages/system/metallb/charts/metallb/` (and similar `charts/` subdirectories) are vendored upstream Helm chart templates. Cozystack does not patch vendored templates; bugs in those templates should be reported and fixed upstream. Do not flag issues in vendored chart templates that require patching the vendored file directly.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2073
File: packages/system/kamaji/charts/kamaji/templates/kubeconfiggenerator-deployment.yaml:11-21
Timestamp: 2026-02-19T13:25:34.652Z
Learning: The `packages/system/kamaji/charts/` directory in the cozystack/cozystack repository contains vendored upstream Kamaji Helm charts fetched via `make -C packages/system/kamaji update`. The team does not modify vendored charts directly; patches only go into `images/kamaji/patches/` for the Go binary. Issues with the vendored charts should be reported upstream at https://github.com/clastix/kamaji.
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2699
File: packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml:633-643
Timestamp: 2026-05-20T22:56:39.980Z
Learning: In the cozystack/cozystack repository, vendored upstream Helm chart content under `packages/system/metallb/charts/metallb/**` (and similar vendor paths) must not be patched directly. Any bugs found in vendored charts should be filed upstream (e.g., at metallb/metallb) rather than fixed in-tree. This policy is documented in AGENTS.md and feedback-helm-charts-need-tests.md.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2744
File: packages/system/trust-manager/charts/trust-manager/templates/metrics-servicemonitor.yaml:15-17
Timestamp: 2026-05-26T20:25:16.207Z
Learning: In cozystack/cozystack, the vendored `packages/system/trust-manager/charts/trust-manager/templates/metrics-servicemonitor.yaml` (trust-manager v0.22.1) has a conditional rendering bug: when both `.Values.commonAnnotations` and `.Values.app.metrics.service.servicemonitor.labels` are set, the extra-labels block (rendered with `indent 4` after the `annotations:` block) lands inside the `annotations:` map instead of inside `metadata.labels:`. With only `servicemonitor.labels` set and no `commonAnnotations`, the indent-4 entries happen to merge into `metadata.labels:` correctly. Since cozystack ships without overriding either key by default, the default install is unaffected. The upstream fix was filed as cert-manager/trust-manager#981. Do not flag this in future reviews unless the vendored file has been updated without the fix.
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T10:16:39.753Z
Learning: Applies to packages/core/** : Follow Helm Charts umbrella pattern with vendored upstream charts in `charts/` subdirectory for core platform charts in `packages/core/`
Learnt from: mattia-eleuteri
Repo: cozystack/cozystack PR: 2199
File: packages/apps/tenant/templates/networkpolicy.yaml:222-225
Timestamp: 2026-03-10T16:00:27.642Z
Learning: In this repository, Cilium hostFirewall is enabled, so pods using hostNetwork (e.g., virt-handler) retain their Cilium endpoint identity and are matchable by label-based toEndpoints/matchLabels in CiliumNetworkPolicy. When reviewing YAML network policy manifests (e.g., packages/apps/**/templates/networkpolicy.yaml and similar), do not flag toEndpoints rules that target hostNetwork: true pods as ineffective. Ensure that policy selectors use labels that correctly target these hostNetwork pods and validate that policies reflect intended access while leveraging the retained identity for precise matching.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2699
File: packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/README.md:3-3
Timestamp: 2026-05-20T22:56:49.337Z
Learning: In cozystack/cozystack, treat files vendored from upstream Helm charts as read-only. For any content under `packages/system/*/charts/**`, do not raise code-review issues that would typically apply to edited source (e.g., stale README badges, inaccuracies in generated/vendored chart content). If you find such problems, report/follow up upstream on the original chart’s repository (e.g., `metallb/metallb`) rather than changing the vendored copy in cozystack.
Aleksei Sviridkin (lexfrei)
force-pushed
the
chore/seaweedfs-4.31
branch
from
June 8, 2026 18:21
4a1d6ff to
0b6f2f9
Compare
Re-vendor the SeaweedFS Helm chart from 4.05 to 4.31. Improvements that land with the binary: the PostgreSQL filer store moves to pgx v5 with PgBouncer support and an ON CONFLICT upsert default, HTTPS certificates hot-reload without a pod restart on cert-manager rotation, and more S3/volume Prometheus metrics are exported. The S3 service now exposes a metrics port by default. Adaptations: - Drop s3-traffic-distribution.patch: native s3.trafficDistribution support landed upstream in chart 4.12. The native field is gated to Kubernetes >=1.31 and auto-converts to PreferSameZone on >=1.35, which the hardcoded patch did not do. Set s3.trafficDistribution: PreferClose in values instead. - Migrate global.* to global.seaweedfs.* in values: chart 4.18 namespaced the app-specific globals to avoid polluting the shared global namespace when the chart is used as a subchart. Covers enableSecurity, serviceAccountName, extraEnvironmentVars and monitoring. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
… gating Pin the native s3.trafficDistribution behaviour that replaced the dropped s3-traffic-distribution.patch: PreferClose is kept on Kubernetes 1.33, auto-converts to PreferSameZone on 1.35, and the field is omitted below the 1.31 gate. Adds the helm-unittest harness (test target + suite) the package previously lacked. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
chore/seaweedfs-4.31
branch
from
June 9, 2026 09:53
0b6f2f9 to
ed367f3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yaml (1)
84-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeduplicate
allInOneenv rendering.Lines 103-128 still emit
.Values.allInOne.extraEnvironmentVarsand.Values.global.seaweedfs.extraEnvironmentVarsin two separate passes. If the same key is set in both scopes, this renders duplicateenventries and makes precedence depend on list order instead of a single merged result, unlike the helper-based merge used in the other updated workloads.Based on learnings, files under
packages/system/seaweedfs/charts/seaweedfs/are upstream-managed and should be fixed in the SeaweedFS chart, then re-vendored viamake update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yaml` around lines 84 - 128, The template emits env entries twice causing duplicates; iterate over the already-merged map ($envMerged) instead of separately ranging .Values.allInOne.extraEnvironmentVars and .Values.global.seaweedfs.extraEnvironmentVars so precedence is deterministic. Replace the two range blocks with a single loop over $envMerged (skipping keys $clusterMasterKey and $clusterFilerKey) and preserve the existing handling for string vs valueFrom using kindIs, tpl and toYaml so env entries come from the merged source with correct precedence.Source: Learnings
packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml (1)
47-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
topologySpreadConstraintsreads the wrong values key.The guard checks
cosi.topologySpreadConstraints, but the body renderscosi.topologySpreadConstraint. Once the plural key is configured, this block pulls from a missing value and breaks the COSI Deployment rendering instead of applying the spread rules. Please fix this in the upstream SeaweedFS chart and then re-vendor it here.Based on learnings, vendored SeaweedFS chart templates under
packages/system/seaweedfs/charts/seaweedfs/should be fixed upstream and re-vendored viapackages/system/seaweedfs/Makefile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml` around lines 47 - 50, The guard currently checks .Values.cosi.topologySpreadConstraints but the template body renders .Values.cosi.topologySpreadConstraint (singular), causing missing-value rendering; update the template so the rendered key matches the guard (replace .Values.cosi.topologySpreadConstraint with .Values.cosi.topologySpreadConstraints in the topologySpreadConstraints block), then fix upstream in the SeaweedFS chart and re-vendor the corrected chart into this repo using the vendoring step in packages/system/seaweedfs/Makefile.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml`:
- Line 61: The deployment sets spec.template.spec.serviceAccountName using the
helper {{ include "seaweedfs.componentName" (list . "objectstorage-provisioner")
}} which can differ from the ServiceAccount and RBAC resources that are created
as {{ .Values.global.seaweedfs.serviceAccountName }}-objectstorage-provisioner;
change the Deployment to derive the same name as the SA/RBAC (or change the
SA/RBAC to use the componentName helper) so the serviceAccountName used in the
Deployment matches the subjects[].name in the ClusterRoleBinding and the
metadata.name of the ServiceAccount (ensure you update the reference in
cosi-deployment.yaml to use .Values.global.seaweedfs.serviceAccountName with the
same suffix or consolidate both to use the seaweedfs.componentName helper).
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yaml`:
- Around line 1-14: The template seaweedfs.s3.iceberg.ingress.paths currently
renders pathType unconditionally and elsewhere renders spec.ingressClassName
when .Values.s3.icebergIngress.className is set; both fields are invalid on
pre-1.18 APIs—update the templates to gate emission of pathType and
ingressClassName behind a semverCompare check (use semverCompare ">=1.18-0"
against .Capabilities.KubeVersion.GitVersion) so they are only rendered for
Kubernetes >=1.18, and for older clusters emit the legacy annotation
"kubernetes.io/ingress.class" when .Values.s3.icebergIngress.className is
provided; locate references to seaweedfs.s3.iceberg.ingress.paths,
.Values.s3.icebergIngress.pathType, .Values.s3.icebergIngress.className and the
ingress spec rendering to apply these conditional guards.
---
Outside diff comments:
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yaml`:
- Around line 84-128: The template emits env entries twice causing duplicates;
iterate over the already-merged map ($envMerged) instead of separately ranging
.Values.allInOne.extraEnvironmentVars and
.Values.global.seaweedfs.extraEnvironmentVars so precedence is deterministic.
Replace the two range blocks with a single loop over $envMerged (skipping keys
$clusterMasterKey and $clusterFilerKey) and preserve the existing handling for
string vs valueFrom using kindIs, tpl and toYaml so env entries come from the
merged source with correct precedence.
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml`:
- Around line 47-50: The guard currently checks
.Values.cosi.topologySpreadConstraints but the template body renders
.Values.cosi.topologySpreadConstraint (singular), causing missing-value
rendering; update the template so the rendered key matches the guard (replace
.Values.cosi.topologySpreadConstraint with
.Values.cosi.topologySpreadConstraints in the topologySpreadConstraints block),
then fix upstream in the SeaweedFS chart and re-vendor the corrected chart into
this repo using the vendoring step in packages/system/seaweedfs/Makefile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4329f197-f640-45a7-89ca-584a6ba9ca6f
📒 Files selected for processing (68)
packages/system/seaweedfs/Makefilepackages/system/seaweedfs/charts/seaweedfs/Chart.yamlpackages/system/seaweedfs/charts/seaweedfs/README.mdpackages/system/seaweedfs/charts/seaweedfs/dashboards/seaweedfs-grafana-dashboard.jsonpackages/system/seaweedfs/charts/seaweedfs/openshift-values.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-secret.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-pvc.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-service.ymlpackages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/admin-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/ca-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/cert-caissuer.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/cert-issuer.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/client-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/filer-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/master-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/volume-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cert/worker-cert.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-cluster-role.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-service-account.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-service-client.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/master/master-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-secret.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-secret.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/_compat.tplpackages/system/seaweedfs/charts/seaweedfs/templates/shared/_helpers.tplpackages/system/seaweedfs/charts/seaweedfs/templates/shared/cluster-role.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/notification-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/post-install-bucket-hook.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/seaweedfs-grafana-dashboard.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/secret-seaweedfs-db.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/shared/service-account.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-ingress.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-resize-hook.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-statefulset.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-deployment.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-service.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-servicemonitor.yamlpackages/system/seaweedfs/charts/seaweedfs/values.yamlpackages/system/seaweedfs/patches/s3-traffic-distribution.patchpackages/system/seaweedfs/tests/s3_traffic_distribution_test.yamlpackages/system/seaweedfs/values.yaml
💤 Files with no reviewable changes (1)
- packages/system/seaweedfs/patches/s3-traffic-distribution.patch
✅ Files skipped from review due to trivial changes (3)
- packages/system/seaweedfs/charts/seaweedfs/Chart.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/all-in-one/all-in-one-pvc.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-secret.yaml
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/system/seaweedfs/charts/seaweedfs/templates/worker/worker-service.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-service-client.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-service.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-secret.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/master/master-configmap.yaml
- packages/system/seaweedfs/values.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/shared/notification-configmap.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-secret.yaml
- packages/system/seaweedfs/Makefile
- packages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-service.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/master/master-service.yaml
- packages/system/seaweedfs/charts/seaweedfs/openshift-values.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/shared/secret-seaweedfs-db.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-service.yaml
- packages/system/seaweedfs/tests/s3_traffic_distribution_test.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/filer/filer-ingress.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/shared/_compat.tpl
- packages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-ingress.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/sftp/sftp-service.yaml
- packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml
- packages/system/seaweedfs/charts/seaweedfs/README.md
- packages/system/seaweedfs/charts/seaweedfs/templates/admin/admin-service.yaml
- packages/system/seaweedfs/charts/seaweedfs/dashboards/seaweedfs-grafana-dashboard.json
- packages/system/seaweedfs/charts/seaweedfs/templates/shared/_helpers.tpl
- packages/system/seaweedfs/charts/seaweedfs/values.yaml
| {{- end }} | ||
| enableServiceLinks: false | ||
| serviceAccountName: {{ .Values.global.serviceAccountName }}-objectstorage-provisioner | ||
| serviceAccountName: {{ include "seaweedfs.componentName" (list . "objectstorage-provisioner") }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Helper definition =="
HELPER_FILE="$(fd -p '_helpers.tpl' packages/system/seaweedfs/charts/seaweedfs/templates/shared)"
rg -n -C20 'define "seaweedfs.componentName"' "$HELPER_FILE"
echo
echo "== COSI ServiceAccount/RBAC references =="
rg -n -C2 'objectstorage-provisioner|serviceAccountName' \
packages/system/seaweedfs/charts/seaweedfs/templates/cosi \
packages/system/seaweedfs/charts/seaweedfs/values.yaml \
packages/system/seaweedfs/values.yamlRepository: cozystack/cozystack
Length of output: 12312
Align COSI Deployment serviceAccountName with the ServiceAccount/RBAC names the chart creates
packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml sets spec.template.spec.serviceAccountName to {{ include "seaweedfs.componentName" (list . "objectstorage-provisioner") }}, where seaweedfs.componentName is built from seaweedfs.fullname (release/name overrides), not .Values.global.seaweedfs.serviceAccountName.
But cosi-service-account.yaml creates metadata.name as {{ .Values.global.seaweedfs.serviceAccountName }}-objectstorage-provisioner, and cosi-cluster-role.yaml binds subjects[].name to the same global-derived name. If those differ, the COSI pod can reference a ServiceAccount without the intended ClusterRoleBinding.
Update cosi-deployment.yaml to use the same global-derived ServiceAccount name (or otherwise make SA/RBAC and Deployment derive from the same helper/values).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml`
at line 61, The deployment sets spec.template.spec.serviceAccountName using the
helper {{ include "seaweedfs.componentName" (list . "objectstorage-provisioner")
}} which can differ from the ServiceAccount and RBAC resources that are created
as {{ .Values.global.seaweedfs.serviceAccountName }}-objectstorage-provisioner;
change the Deployment to derive the same name as the SA/RBAC (or change the
SA/RBAC to use the componentName helper) so the serviceAccountName used in the
Deployment matches the subjects[].name in the ClusterRoleBinding and the
metadata.name of the ServiceAccount (ensure you update the reference in
cosi-deployment.yaml to use .Values.global.seaweedfs.serviceAccountName with the
same suffix or consolidate both to use the seaweedfs.componentName helper).
Source: Learnings
Comment on lines
+1
to
+14
| {{- define "seaweedfs.s3.iceberg.ingress.paths" -}} | ||
| paths: | ||
| - path: {{ .Values.s3.icebergIngress.path | quote }} | ||
| pathType: {{ .Values.s3.icebergIngress.pathType | quote }} | ||
| backend: | ||
| {{- if semverCompare ">=1.19-0" .Capabilities.KubeVersion.GitVersion }} | ||
| service: | ||
| name: {{ include "seaweedfs.componentName" (list . "s3") }} | ||
| port: | ||
| number: {{ .Values.s3.icebergPort }} | ||
| {{- else }} | ||
| serviceName: {{ include "seaweedfs.componentName" (list . "s3") }} | ||
| servicePort: {{ .Values.s3.icebergPort }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In which Kubernetes release were the Ingress pathTypeandspec.ingressClassNamefields added, and are they valid onnetworking.k8s.io/v1beta1orextensions/v1beta1 Ingress resources?
💡 Result:
The Ingress pathType and spec.ingressClassName fields were added in Kubernetes 1.18 [1][2][3]. These fields are valid in the networking.k8s.io/v1beta1 API version (as they were introduced in 1.18 to allow for a transition to networking.k8s.io/v1) [1][4]. They are not valid in the older extensions/v1beta1 API version [5]. When using networking.k8s.io/v1beta1 in Kubernetes 1.18+, these fields are supported and allow for the transition from the deprecated kubernetes.io/ingress.class annotation to the IngressClass resource model [1][3][6]. Older versions of Kubernetes that only support extensions/v1beta1 do not recognize these fields, which will cause validation errors if included in those environments [5].
Citations:
- 1: https://kubernetes.website.cncfstack.com/blog/2020/04/02/improvements-to-the-ingress-api-in-kubernetes-1.18/
- 2: https://v1-33.docs.kubernetes.io/blog/2020/06/05/supporting-the-evolving-ingress-specification-in-kubernetes-1.18/
- 3: https://kubernetes.io/docs/concepts/services-networking/ingress/
- 4: Graduating Ingress to V1 kubernetes/kubernetes#88041
- 5: https://support.konghq.com/support/s/article/Error-during-Helm-deployment-ValidationError-Ingress-spec-rules-0-http-paths-0-unknown-field-pathType
- 6: https://kubernetes.github.io/ingress-nginx/user-guide/k8s-122-migration/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yaml"
echo "== $FILE =="
wc -l "$FILE"
nl -ba "$FILE" | sed -n '1,140p'Repository: cozystack/cozystack
Length of output: 293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yaml"
echo "== $FILE =="
wc -l "$FILE"
nl -ba "$FILE" | sed -n '1,140p'Repository: cozystack/cozystack
Length of output: 293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yaml"
echo "== $FILE =="
echo "--- head(1-120) ---"
sed -n '1,120p' "$FILE"
echo "--- cat -n (1-120) ---"
cat -n "$FILE" | sed -n '1,120p'Repository: cozystack/cozystack
Length of output: 5039
Gate Ingress pathType and ingressClassName on Kubernetes >=1.18 (and omit for beta fallbacks).
seaweedfs.s3.iceberg.ingress.pathsrenderspathType:unconditionally (line 4), even when the chart falls back tonetworking.k8s.io/v1beta1/extensions/v1beta1for older Kubernetes—pathTypeis not valid on pre-1.18 APIs.spec.ingressClassNameis rendered whenever.Values.s3.icebergIngress.classNameis set (lines 45-46), but it is not gated by Kubernetes version—ingressClassNameis also not valid pre-1.18. Use the legacykubernetes.io/ingress.classannotation for those clusters and only render these fields for Kubernetes >= 1.18.
Fix should be done upstream in the SeaweedFS chart and re-vendored.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-iceberg-ingress.yaml`
around lines 1 - 14, The template seaweedfs.s3.iceberg.ingress.paths currently
renders pathType unconditionally and elsewhere renders spec.ingressClassName
when .Values.s3.icebergIngress.className is set; both fields are invalid on
pre-1.18 APIs—update the templates to gate emission of pathType and
ingressClassName behind a semverCompare check (use semverCompare ">=1.18-0"
against .Capabilities.KubeVersion.GitVersion) so they are only rendered for
Kubernetes >=1.18, and for older clusters emit the legacy annotation
"kubernetes.io/ingress.class" when .Values.s3.icebergIngress.className is
provided; locate references to seaweedfs.s3.iceberg.ingress.paths,
.Values.s3.icebergIngress.pathType, .Values.s3.icebergIngress.className and the
ingress spec rendering to apply these conditional guards.
Source: Learnings
myasnikovdaniil
approved these changes
Jun 9, 2026
myasnikovdaniil
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The vendored re-vendor is mechanical; the package-layer adaptations are correct, documented, and verified.
Verified locally:
- Native
s3.trafficDistribution: PreferClosereplaces the droppeds3-traffic-distribution.patch.helm templateconfirms the version gating: K8s >=1.35 rendersPreferSameZone(auto-converted),--kube-version 1.33rendersPreferClose, and the field is omitted below the 1.31 gate. - The
global.* -> global.seaweedfs.*migration is complete for all four keys the package sets. Render confirms the cluster env vars reach the pods, the ServiceMonitors render, andsecurity.tomlwith the gRPC mTLS sections renders intact. - The new
s3_traffic_distribution_test.yamlis not vacuous — its three cases (1.30 omitted, 1.33 PreferClose, 1.35 PreferSameZone) produce distinct version-appropriate outcomes and all pass, confirming thecapabilities.majorVersion/minorVersionmock populatesKubeVersion.GitVersion.
helm template renders clean (41 docs, exit 0); helm unittest 3/3. The remaining bot findings (iceberg pathType/ingressClassName pre-1.18; all-in-one env duplication) are upstream-vendored-chart concerns that don't apply here — the platform targets K8s >=1.31 and all-in-one mode is disabled. No blockers. Worth confirming E2E is green before merge given the filer backend moves to pgx v5.
Aleksei Sviridkin (lexfrei)
enabled auto-merge
June 9, 2026 19:57
Andrei Kvapil (kvaps)
approved these changes
Jun 9, 2026
Andrei Kvapil (kvaps)
left a comment
Member
There was a problem hiding this comment.
LGTM. The hand-edited package layer (the 4 non-vendored files) is clean and correct:
global.* → global.seaweedfs.*migration (chart 4.18) is complete and the values verifiably reach the pods (cluster env vars, serviceAccountName, ServiceMonitors, security.toml all render).- Dropping
s3-traffic-distribution.patchin favor of the natives3.trafficDistribution: PreferClose(chart 4.12) is the right move, and the newtests/s3_traffic_distribution_test.yamlpins the version-gating behavior (PreferClose@1.33, auto-PreferSameZone@1.35, omitted@1.30). The gemini bot claim that helm-unittest ignorescapabilities.{major,minor}Versionis wrong — confirmed the suite passes as written. - Version bump is internally consistent (Chart.yaml
4.05/4.0.405 → 4.31/4.31.0, image tag resolves to:4.31).
Info note (not blocking): the vendored upstream chart 4.31 rewrote charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml — it drops the literal <bucketClassName>-lock BucketClass (object-lock COMPLIANCE) and <bucketClassName>-readonly BucketAccessClass, replacing them with a parameterizable cosi.bucketClassParameters that this package leaves unset. Since system/seaweedfs runs with cosi.enabled: true, bucketClassName: "seaweedfs", the rendered set goes from 4 → 2 resources. This does not affect the Bucket API: apps/bucket builds bucketClassName: <_namespace.seaweedfs>-lock (a per-tenant name), served by the untouched extra/seaweedfs COSI — not the literal seaweedfs-lock. So object-lock/readonly buckets keep working. Flagging only because the literal system-COSI classes silently disappear; if nothing consumes them (looks like the case), consider dropping cosi.enabled from system/seaweedfs in a follow-up, otherwise no action needed.
Couldn't verify the pgx-v5 filer-store runtime change without a cluster — worth a maintainer eye if the external seaweedfs-db Postgres interaction matters.
This was referenced Jun 13, 2026
3 tasks
IvanHunters
pushed a commit
that referenced
this pull request
Jun 15, 2026
The 4.31 chart bump (#2834) broke E2E for every PR: tenant-root seaweedfs-system never becomes ready. 1. S3 served plaintext. Until 4.05 the -cert.file/-key.file args were rendered whenever enableSecurity was set, so weed s3 served TLS on the main port (8333); 4.31 nested them under 'if s3.httpsPort' and we ship httpsPort=0, so s3 spoke HTTP while the HTTPS probes, the ingress backend-protocol and the COSI sidecar endpoint all expected HTTPS ('server gave HTTP response to HTTPS client'). Restore the 4.05 behaviour via patch. 2. COSI provisioner ServiceAccount mismatch. cosi-service-account.yaml and the ClusterRoleBinding name the SA <serviceAccountName>-objectstorage-provisioner, but the Deployment referenced componentName (<release>-objectstorage-provisioner). When serviceAccountName != release name (tenant-root-seaweedfs vs seaweedfs-system) the Deployment could not create pods ('serviceaccount not found'). Align the Deployment with the SA + binding. Both fixes are vendored as patches re-applied by 'make update', and covered by helm-unittest. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
IvanHunters
added a commit
that referenced
this pull request
Jun 15, 2026
…lass The seaweedfs 4.31 upstream bump (#2834) rewrote packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml from scratch and silently dropped the supplementary COSI resources that cozystack ships on top of the upstream chart: - BucketClass {name}-lock (object lock, COMPLIANCE mode, 365 days retention) — used by apps that store immutable backups - BucketAccessClass {name} parameters.accessPolicy: readwrite — the explicit grant for the default access class - BucketAccessClass {name}-readonly (accessPolicy: readonly) — used by every Bucket app that declares readonly users apps/bucket still renders BucketAccess resources that reference {name}-readonly, but the class no longer exists, so the COSI controller never picks the BucketAccess up: no finalizer is added, status.accessGranted stays empty, and the bucket E2E test waits 300s and times out on the viewer access. The admin (readwrite) access class still works because it falls back to the default BAC. Restore the three resources so the system chart again matches the shape that packages/extra/seaweedfs/templates/client/cosi-bucket-class.yaml already ships for the client topology (where this never regressed). Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
IvanHunters
added a commit
that referenced
this pull request
Jun 15, 2026
The seaweedfs 4.31 upstream bump (#2834) switched the s3 Service name from "$chartname-s3" (built with the seaweedfs.name helper, evaluates to "seaweedfs-s3") to the seaweedfs.componentName helper, which is release-name based and now evaluates to "$release-s3" — "seaweedfs- system-s3" for the tenant-root deployment. The bats E2E asset hack/e2e-apps/bucket.bats hard-codes "service/seaweedfs-s3" for the port-forward that sets up its S3 client; backup-controller and other in-cluster consumers also referenced the legacy name. After the bump, the legacy Service is gone and the port-forward immediately exits, the test then times out on the 30-second nc poll for localhost:8333. Restore the legacy Service name. Internal references inside the chart (s3-deployment -filer=, etc) keep using componentName for the filer endpoint, so this change only re-exposes the public-facing name without touching the internal topology. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Aleksei Sviridkin (lexfrei)
added a commit
that referenced
this pull request
Jun 15, 2026
#2916) ## What this PR does The SeaweedFS 4.31 chart bump (#2834) broke E2E for **every** PR on `main`: in `tenant-root` the `seaweedfs-system` release never becomes ready, so `install-cozystack` times out waiting on its HelmRelease. Two regressions came in with the new chart; both reproduce with a plain `helm template` of `packages/system/seaweedfs` (no cluster needed) and are fixed here as vendored patches re-applied by `make update`, with helm-unittest coverage. **1. S3 served plaintext while everything expected HTTPS.** Until chart 4.05 the `-cert.file`/`-key.file` args were rendered whenever `enableSecurity` was set, so `weed s3` served TLS on the main port (8333). Chart 4.31 nested those args inside `{{- if .Values.s3.httpsPort }}`, and we ship `httpsPort: 0` — so no TLS args rendered, s3 spoke HTTP, and the HTTPS readiness/liveness probes (plus the ingress `backend-protocol: HTTPS` and the COSI sidecar's `https://` endpoint) all failed with `server gave HTTP response to HTTPS client`. `patches/s3-tls-main-port.patch` moves the cert/key args back out of the `httpsPort` gate, restoring TLS-on-main-port; `-port.https` is still only added when `httpsPort` is set. **2. COSI provisioner ServiceAccount name mismatch.** `cosi-service-account.yaml` and the `ClusterRoleBinding` name the SA `<global.seaweedfs.serviceAccountName>-objectstorage-provisioner`, but the Deployment referenced `componentName` (`<release>-objectstorage-provisioner`). Whenever `serviceAccountName` != release name — our case, `tenant-root-seaweedfs` vs `seaweedfs-system` — the Deployment could not create pods (`serviceaccount "seaweedfs-system-objectstorage-provisioner" not found`). `patches/cosi-provisioner-sa-name.patch` aligns the Deployment with the SA and the binding. (Upstream installs only avoid this because their release name and serviceAccountName usually coincide.) This is `main`-only: the 4.31 chart landed only on `main`, every release branch is still on 4.05, so no `backport` is needed. ### Release note ```release-note fix(seaweedfs): restore S3 TLS on the main port and fix the COSI provisioner ServiceAccount name after the 4.31 chart bump, so tenant SeaweedFS becomes ready again ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed S3 TLS certificate arguments to render correctly when HTTPS port is disabled. * Updated COSI provisioner to use configurable service account naming. * **Tests** * Added regression tests for S3 TLS configuration and COSI service account handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
pull Bot
pushed a commit
to medampudi/cozystack
that referenced
this pull request
Jun 15, 2026
The 4.31 chart bump (cozystack#2834) broke E2E for every PR: tenant-root seaweedfs-system never becomes ready. 1. S3 served plaintext. Until 4.05 the -cert.file/-key.file args were rendered whenever enableSecurity was set, so weed s3 served TLS on the main port (8333); 4.31 nested them under 'if s3.httpsPort' and we ship httpsPort=0, so s3 spoke HTTP while the HTTPS probes, the ingress backend-protocol and the COSI sidecar endpoint all expected HTTPS ('server gave HTTP response to HTTPS client'). Restore the 4.05 behaviour via patch. 2. COSI provisioner ServiceAccount mismatch. cosi-service-account.yaml and the ClusterRoleBinding name the SA <serviceAccountName>-objectstorage-provisioner, but the Deployment referenced componentName (<release>-objectstorage-provisioner). When serviceAccountName != release name (tenant-root-seaweedfs vs seaweedfs-system) the Deployment could not create pods ('serviceaccount not found'). Align the Deployment with the SA + binding. Both fixes are vendored as patches re-applied by 'make update', and covered by helm-unittest. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
4 tasks
myasnikovdaniil
pushed a commit
that referenced
this pull request
Jun 17, 2026
…lass The seaweedfs 4.31 upstream bump (#2834) rewrote packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml from scratch and silently dropped the supplementary COSI resources that cozystack ships on top of the upstream chart: - BucketClass {name}-lock (object lock, COMPLIANCE mode, 365 days retention) — used by apps that store immutable backups - BucketAccessClass {name} parameters.accessPolicy: readwrite — the explicit grant for the default access class - BucketAccessClass {name}-readonly (accessPolicy: readonly) — used by every Bucket app that declares readonly users apps/bucket still renders BucketAccess resources that reference {name}-readonly, but the class no longer exists, so the COSI controller never picks the BucketAccess up: no finalizer is added, status.accessGranted stays empty, and the bucket E2E test waits 300s and times out on the viewer access. The admin (readwrite) access class still works because it falls back to the default BAC. Restore the three resources so the system chart again matches the shape that packages/extra/seaweedfs/templates/client/cosi-bucket-class.yaml already ships for the client topology (where this never regressed). Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
myasnikovdaniil
pushed a commit
that referenced
this pull request
Jun 17, 2026
The seaweedfs 4.31 upstream bump (#2834) switched the s3 Service name from "$chartname-s3" (built with the seaweedfs.name helper, evaluates to "seaweedfs-s3") to the seaweedfs.componentName helper, which is release-name based and now evaluates to "$release-s3" — "seaweedfs- system-s3" for the tenant-root deployment. The bats E2E asset hack/e2e-apps/bucket.bats hard-codes "service/seaweedfs-s3" for the port-forward that sets up its S3 client; backup-controller and other in-cluster consumers also referenced the legacy name. After the bump, the legacy Service is gone and the port-forward immediately exits, the test then times out on the 30-second nc poll for localhost:8333. Restore the legacy Service name. Internal references inside the chart (s3-deployment -filer=, etc) keep using componentName for the filer endpoint, so this change only re-exposes the public-facing name without touching the internal topology. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
This was referenced Jun 17, 2026
1 task
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What this PR does
Re-vendors the SeaweedFS Helm chart from 4.05 (chart 4.0.405) to 4.31 (chart 4.31.0) via
make update, and adapts the package layer to the breaking changes that accumulated across the gap.What lands with the bump: the PostgreSQL filer store (the backend used here) moves to pgx v5 with PgBouncer support and an
ON CONFLICTupsert default; HTTPS certificates hot-reload without a pod restart on cert-manager rotation; more S3 and volume Prometheus metrics are exported, and the S3 Service now exposes a metrics port by default.Package-layer adaptations:
s3-traffic-distribution.patch. Natives3.trafficDistributionsupport landed upstream in chart 4.12. The native field is gated to Kubernetes >= 1.31 and auto-convertsPreferClosetoPreferSameZoneon >= 1.35, which the hardcoded patch did not do. The patch is replaced withs3.trafficDistribution: PreferClosein the package values, plus helm-unittest coverage pinning all three version cases.global.*toglobal.seaweedfs.*. Chart 4.18 namespaced the app-specific globals to avoid polluting the shared global namespace when the chart is used as a subchart. The package values moveenableSecurity,serviceAccountName,extraEnvironmentVarsandmonitoringto the new path instead of relying on the chart's backward-compat shim.Verified with
helm templateandhelm unittest: the cluster env vars reach the pods, the gRPC mTLSsecurity.tomlrenders intact (including the new admin/worker sections from the 4.24/4.26 security tightening), and both retained patches still apply.Screenshots
N/A — no UI changes.
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores