[seaweedfs] Add Client topology #1239
Conversation
WalkthroughThis update introduces a new "Client" topology mode for the SeaweedFS Helm chart, adding related configuration parameters and conditionally generating new Kubernetes resources and templates for this mode. Documentation, schema, and validation logic are updated, and several new manifest templates are added or modified to support the "Client" topology and external filer access. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
participant SeaweedFS Client Provisioner
User->>Helm: Install/upgrade chart with topology: "Client"
Helm->>Kubernetes: Deploys ConfigMap, ServiceAccount, ClusterRole, Deployment, etc. for "Client"
Kubernetes->>SeaweedFS Client Provisioner: Starts objectstorage-provisioner Deployment
SeaweedFS Client Provisioner->>Remote Filer: Connects using remoteEndpoint
sequenceDiagram
participant User
participant Helm
participant Kubernetes
User->>Helm: Install/upgrade chart with topology: "Simple" or "MultiZone"
Helm->>Kubernetes: Deploys SeaweedFS core resources (StatefulSets, Services, etc.)
Note over Kubernetes: Core SeaweedFS components run as usual
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @kvaps, 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 significantly extends the deployment options for SeaweedFS by introducing a new 'Client' topology. This allows Kubernetes clusters to integrate with an existing, externally managed SeaweedFS instance, leveraging the Container Object Storage Interface (COSI) for bucket provisioning. The changes also include improvements to topology management during upgrades and new options for exposing the SeaweedFS filer externally.
Highlights
- New 'Client' Topology: Introduced a new
Clienttopology option for SeaweedFS deployments. When this topology is selected, the Helm chart will not deploy the core SeaweedFS components (master, volume, filer, etc.) within the Kubernetes cluster. Instead, it configures the cluster to connect to an external SeaweedFS instance. - Container Object Storage Interface (COSI) Integration: Added comprehensive support for COSI when using the
Clienttopology. This includes new Kubernetes resources such asBucketClass,BucketAccessClass,ClusterRole,ClusterRoleBinding,ServiceAccount, and aDeploymentfor theseaweedfs-cosi-driverand anobjectstorage-sidecar. This enables Kubernetes applications to provision and manage object storage buckets on the external SeaweedFS instance via COSI. - Enhanced Topology Change Detection: Refactored the preflight checks to prevent switching between any SeaweedFS topologies (Simple, MultiZone, or Client) after the initial deployment. A new ConfigMap is introduced to persistently store the currently deployed topology, improving the robustness of upgrade checks.
- External Filer Access Configuration: Added a new
filer.externalparameter to enable external access to the SeaweedFS filer. This introduces a new Ingress resource and a LoadBalancer Service, conditionally deployed when the topology is notClientandfiler.externalis enabled, allowing external clients to connect to the filer's gRPC service. - Dynamic Resource Deployment: Modified existing templates (e.g.,
dashboard-resourcemap.yaml,vpa.yaml,seaweedfs.yaml) to conditionally deploy resources based on the selectedtopology. For instance, Vertical Pod Autoscalers (VPAs) and certain dashboard RBAC rules are now only applied when the topology is notClient.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Client' topology for the SeaweedFS Helm chart, allowing it to act as a COSI driver for a remote SeaweedFS instance. The changes include new templates for the client-side components, updates to existing templates to conditionally render resources based on the topology, and modifications to the chart's values and schema. My review identified several critical issues, including incorrect resource naming that would cause deployment failures, broken logic for upgrade compatibility, and incorrect service selectors. I've also pointed out some high-severity issues like using a 'latest' image tag and missing validation checks, along with some medium-severity suggestions for code cleanup and documentation consistency.
packages/extra/seaweedfs/ing.yaml
Outdated
| apiVersion: networking.k8s.io/v1 | ||
| kind: Ingress | ||
| metadata: | ||
| annotations: | ||
| acme.cert-manager.io/http01-ingress-class: tenant-root | ||
| cert-manager.io/cluster-issuer: letsencrypt-prod | ||
| nginx.ingress.kubernetes.io/backend-protocol: GRPC | ||
| nginx.ingress.kubernetes.io/proxy-body-size: "0" | ||
| generation: 2 | ||
| labels: | ||
| app.kubernetes.io/component: objectstorage-provisioner-cluster1 | ||
| name: seaweedfs-filer-external | ||
| namespace: tenant-dev | ||
| spec: | ||
| ingressClassName: tenant-root | ||
| rules: | ||
| - host: prod.s3.hikube.cloud | ||
| http: | ||
| paths: | ||
| - backend: | ||
| service: | ||
| name: seaweedfs-objectstorage-provisioner-cluster1 | ||
| port: | ||
| number: 18889 | ||
| path: / | ||
| pathType: ImplementationSpecific | ||
| tls: | ||
| - hosts: | ||
| - prod.s3.hikube.cloud | ||
| secretName: seaweedfs-filer-external-tls |
There was a problem hiding this comment.
This file appears to be a test or example manifest with hardcoded values (e.g., namespace: tenant-dev, host: prod.s3.hikube.cloud). It's not a Helm template and is located at the root of the chart directory, which means it won't be processed by Helm.
If this file was added by mistake, it should be removed to avoid confusion.
| name: client-cert | ||
| readOnly: true | ||
| - name: seaweedfs-cosi-sidecar | ||
| image: ghcr.io/cozystack/cozystack/objectstorage-sidecar |
There was a problem hiding this comment.
The seaweedfs-cosi-sidecar container is using an image ghcr.io/cozystack/cozystack/objectstorage-sidecar without a specific tag. This will default to the latest tag, which is not recommended for production environments as it can lead to unpredictable behavior when the image is updated.
Please specify a fixed version tag for this image to ensure repeatable and stable deployments.
| {{- $ingress := index $myNS.metadata.annotations "namespace.cozystack.io/ingress" }} | ||
| {{- $host := index $myNS.metadata.annotations "namespace.cozystack.io/host" }} |
There was a problem hiding this comment.
The Helm variable $ingress is defined here but it is not used anywhere in this template. It should be removed to improve code clarity and maintainability.
| creationTimestamp: null | ||
| labels: |
There was a problem hiding this comment.
The creationTimestamp: null field is unnecessary in a Helm template. Kubernetes will populate this field upon resource creation. It's best to remove it to keep the templates clean.
packages/extra/seaweedfs/values.yaml
Outdated
| ## @param remoteFilerEndpoint The endpoint of the remote filer GRPC service. Used when `topology` is set to `Client`. | ||
| remoteEndpoint: "1.2.3.4:18888" |
There was a problem hiding this comment.
There's a mismatch between the parameter name in the comment (@param remoteFilerEndpoint) and the actual value key (remoteEndpoint). For consistency and clarity, the comment should be updated to match the key.
## @param remoteEndpoint The endpoint of the remote filer GRPC service. Used when `topology` is set to `Client`.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/extra/seaweedfs/templates/dashboard-resourcemap.yaml (1)
6-31: Role is skipped forClienttopology, but RoleBindings are still rendered – chart will fail to install.When
.Values.topology == "Client"theRoleis omitted (if not (eq ...)),
however bothRoleBindings below are unconditional and will reference a
non-existent Role, yielding:Error: validation failed: error validating "…": must specify roleRefWrap the entire block (Role and bindings) in the same
ifguard.-{{- if not (eq .Values.topology "Client") }} +{{- if not (eq .Values.topology "Client") }} # Role definition … ... # both RoleBindings … +{{- end }}
🧹 Nitpick comments (12)
packages/extra/seaweedfs/Makefile (1)
5-8: Order-of-operations safeguard looks correct – minor edge-case noteInjecting the enum after
readme-generator-for-helmavoids the well-known enum-stripping issue (👍).
If.properties.topologyis later refactored into a nested object, the hard-coded path will silently create a duplicate property instead of failing. Addingselect(has("topology"))(oryq eval '... | select(tag == "!!map")') would give an early signal.No action required now, just flagging the future-proofing angle.
packages/extra/seaweedfs/README.md (3)
1-11: Header still says “Managed NATS Service”Looks like a copy-paste artifact; this README now documents SeaweedFS. Mismatched headers confuse users and automated doc tooling.
-# Managed NATS Service +# SeaweedFS Helm Chart
10-10: Keep default value list alphabetically sortedFor quick visual scanning it helps to list enum values in a predictable order. Since the schema and Makefile inject
"Client"last, the docs should mirror that order or sort alphabetically:Client, MultiZone, Simple.
16-16: Consider clarifying external access scope
filer.external≈ “expose via LoadBalancer”. Stating the actual mechanism (LB + optional Ingress) prevents ambiguity for users deploying on non-cloud clusters whereLoadBalanceris unsupported.packages/extra/seaweedfs/templates/external/cosi-service-account.yaml (1)
1-8: Prepend document delimiter to silence YAML lintersThe leading Helm control line confuses many YAML linters (see YAMLlint error).
A single---before the template block keeps both Helm and linters happy:+--- {{- if eq .Values.topology "Client" }} apiVersion: v1 kind: ServiceAccountpackages/extra/seaweedfs/templates/vpa.yaml (1)
1-1: Same linter issue as aboveAdd
---before the{{- if ... }}to keep YAML validators quiet.+--- {{- if not (eq .Values.topology "Client") }}packages/extra/seaweedfs/templates/svc-external.yaml (2)
1-18: Service selector may miss custom labelsThe selector keys are hard-coded; if users override
podLabelsin values, the Service may not match any pods. Consider merging with.Values.podLabelsor basing on commonapp.kubernetes.io/instance.- selector: - app.kubernetes.io/component: filer - app.kubernetes.io/name: {{ $.Release.Name }} + selector: + app.kubernetes.io/component: filer + app.kubernetes.io/instance: {{ $.Release.Name }}
1-18: Minor linter noteSame as other templates – add an opening
---before YAML content to appease linters.packages/extra/seaweedfs/ing.yaml (1)
1-31: Hard-coded Ingress manifest is environment-specific and contains illegal fields.
Issues
metadata.generationis server-populated; supplying it is refused by the
API.- Namespace, host, service name, port, TLS secret are hard-wired – makes the
chart non-reusable and breaks multi-tenant installs.- File sits outside
templates/, sohelm templatewon’t render values but
still packages a static object.Suggested action
Either turn this into a proper Helm template (parametrised via
values.yaml) or remove it and let users create environment-specific Ingress
separately.Minimal diff to drop the read-only field:
- generation: 2But a full refactor to
templates/svc-external.yaml-style conditional logic is
recommended.packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml (1)
3-4: Order fields asapiVersion→kindfor consistencyMost Kubernetes manifests start with
apiVersionfollowed bykind; aligning with that convention improves readability and diff-friendliness.packages/extra/seaweedfs/templates/seaweedfs.yaml (1)
18-26: Guardlookup-based topology detection against missing resources
lookupreturnsnilon failure; subsequentindex/string ops will panic.
No issue here, but it’s worth noting that if the PVC/STS names ever change,$detectedTopologywill stay"Unknown"silently. Consider surfacing a warning to chart users.packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml (1)
7-48: Indentation is legal but non-idiomatic – trim 2 spacesHelm renders fine, yet over-indented list items break YAML-lint and hurt readability. Two-space indentation below
rules:is the canonical style.Nit only – no functional impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/extra/seaweedfs/Makefile(1 hunks)packages/extra/seaweedfs/README.md(1 hunks)packages/extra/seaweedfs/ing.yaml(1 hunks)packages/extra/seaweedfs/templates/cm.yaml(1 hunks)packages/extra/seaweedfs/templates/dashboard-resourcemap.yaml(3 hunks)packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml(1 hunks)packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml(1 hunks)packages/extra/seaweedfs/templates/external/cosi-deployment.yaml(1 hunks)packages/extra/seaweedfs/templates/external/cosi-service-account.yaml(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(3 hunks)packages/extra/seaweedfs/templates/svc-external.yaml(1 hunks)packages/extra/seaweedfs/templates/vpa.yaml(2 hunks)packages/extra/seaweedfs/values.schema.json(2 hunks)packages/extra/seaweedfs/values.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
packages/extra/seaweedfs/Makefile (1)
Learnt from: NickVolynkin
PR: #1196
File: packages/apps/http-cache/Makefile:24-27
Timestamp: 2025-07-14T16:23:12.803Z
Learning: In the cozystack repository, the readme-generator tool removes enum contents from values.schema.json files during its operation. Therefore, when using readme-generator in Makefiles, any enum values need to be injected back into the schema using yq commands after readme-generator has run, not before.
packages/extra/seaweedfs/templates/vpa.yaml (1)
Learnt from: lllamnyp
PR: #1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.624Z
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.
packages/extra/seaweedfs/values.schema.json (1)
Learnt from: lllamnyp
PR: #1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.624Z
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.
packages/extra/seaweedfs/templates/dashboard-resourcemap.yaml (1)
Learnt from: lllamnyp
PR: #1161
File: packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml:6-12
Timestamp: 2025-07-10T12:14:27.197Z
Learning: Kubernetes RBAC rules with resourceNames work correctly for list/watch verbs. When resourceNames is specified in an RBAC rule, it properly restricts access to only those named resources, even for list and watch operations. Examples: kubectl get resource resourcename -w watches for changes on a single resource, and kubectl get resource --field-selector .metadata.name=resourcename lists a specific resource. The Kubernetes API server correctly distinguishes such requests from their less specific counterparts.
packages/extra/seaweedfs/templates/seaweedfs.yaml (1)
Learnt from: lllamnyp
PR: #1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the {{- directive chomps all leading whitespace including newlines back to the previous content, so {{- toYaml .Values.something | nindent 2 }} will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}).
🧬 Code Graph Analysis (1)
packages/extra/seaweedfs/ing.yaml (1)
internal/controller/tenant_helm_reconciler.go (1)
annotateTenantRootNs(132-158)
🪛 YAMLlint (1.37.1)
packages/extra/seaweedfs/templates/vpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/svc-external.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/cm.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 21-21: wrong indentation: expected 2 but found 4
(indentation)
[warning] 30-30: wrong indentation: expected 2 but found 4
(indentation)
[warning] 38-38: wrong indentation: expected 2 but found 4
(indentation)
[warning] 41-41: wrong indentation: expected 2 but found 4
(indentation)
[warning] 54-54: wrong indentation: expected 0 but found 2
(indentation)
packages/extra/seaweedfs/templates/seaweedfs.yaml
[warning] 20-20: too many spaces after hyphen
(hyphens)
[warning] 22-22: too many spaces after hyphen
(hyphens)
[warning] 23-23: too many spaces after hyphen
(hyphens)
[warning] 24-24: too many spaces after hyphen
(hyphens)
[warning] 25-25: too many spaces after hyphen
(hyphens)
[warning] 26-26: too many spaces after hyphen
(hyphens)
packages/extra/seaweedfs/templates/external/cosi-deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/external/cosi-service-account.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 GitHub Actions: Pre-Commit Checks
packages/extra/seaweedfs/values.schema.json
[error] 1-1: Make generate failed: Missing metadata for key 'remoteEndpoint' and metadata provided for non existing key 'remoteFilerEndpoint'. Metadata errors found.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/extra/seaweedfs/templates/external/cosi-service-account.yaml (1)
5-6: Name collision check
{{ .Release.Name }}-objectstorage-provisioneris also used in the ClusterRoleBinding template; good.
Just verify no existing SA from non-Client topologies shares this name to avoid upgrade conflicts.packages/extra/seaweedfs/templates/vpa.yaml (1)
1-68: Guard clause is sound – double-check defaultChart defaults still set
topology: Simple, so VPA docs/examples remain valid.
No functional concerns here.
| - name: ENDPOINT | ||
| value: https://{{ .Values.host | default (printf "s3.%s" $host) }} | ||
| - name: SEAWEEDFS_FILER | ||
| value: {{ .Values.remoteEndpoint }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ENDPOINT env var may resolve to empty string
.Values.host is not part of the newly-added Client values; if namespace annotations are also missing, the template renders https://. Provide a sane default or fail fast.
- value: https://{{ .Values.host | default (printf "s3.%s" $host) }}
+ value: https://{{ .Values.host | default (printf "s3.%s" (default "example.com" $host)) }}🏁 Script executed:
#!/bin/bash
set -e
echo "===== Template: external/cosi-deployment.yaml (lines 1-200) ====="
sed -n '1,200p' packages/extra/seaweedfs/templates/external/cosi-deployment.yaml
echo
echo "===== Occurrences of \$host in external/cosi-deployment.yaml ====="
rg '\$host' -n packages/extra/seaweedfs/templates/external/cosi-deployment.yaml || true
echo
echo "===== Occurrences of .Values.host in external/cosi-deployment.yaml ====="
rg '\.Values\.host' -n packages/extra/seaweedfs/templates/external/cosi-deployment.yaml || true
echo
echo "===== SeaweedFS values.yaml (lines 1-200) ====="
sed -n '1,200p' packages/extra/seaweedfs/values.yamlLength of output: 5276
Ensure ENDPOINT always includes a valid host or fails fast
Currently, with
- name: ENDPOINT
value: https://{{ .Values.host | default (printf "s3.%s" $host) }}if both .Values.host (defaults to "" in values.yaml) and the namespace annotation namespace.cozystack.io/host are unset, you end up with https://s3. (no real domain). You should either supply a sane default for $host or fail fast. For example, to fallback on "example.com":
- value: https://{{ .Values.host | default (printf "s3.%s" $host) }}
+ value: https://{{ .Values.host | default (printf "s3.%s" (default "example.com" $host)) }}Or add a guard to stop deployment when neither value is provided:
{{- if not (or .Values.host $host) }}
{{- fail "Missing .Values.host or 'namespace.cozystack.io/host' annotation" }}
{{- end }}
…
- name: ENDPOINT
value: https://{{ .Values.host | default (printf "s3.%s" $host) }}
🤖 Prompt for AI Agents
In packages/extra/seaweedfs/templates/external/cosi-deployment.yaml around lines
43 to 46, the ENDPOINT environment variable can resolve to an invalid URL if
both .Values.host and the namespace annotation $host are unset, resulting in a
URL like "https://s3.". To fix this, add a guard before setting ENDPOINT that
checks if either .Values.host or $host is set; if neither is set, use the fail
function to stop the deployment with a clear error message. Alternatively,
provide a sane default for $host such as "example.com" to ensure the URL is
always valid.
packages/extra/seaweedfs/templates/external/cosi-deployment.yaml
Outdated
Show resolved
Hide resolved
| {{- $ingress := index $myNS.metadata.annotations "namespace.cozystack.io/ingress" }} | ||
| {{- $host := index $myNS.metadata.annotations "namespace.cozystack.io/host" }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
index $myNS.metadata.annotations … may panic when annotations are absent
index fails if .metadata.annotations is nil. Wrap with default or dig to avoid hard-to-trace template panics in fresh namespaces.
-{{- $ingress := index $myNS.metadata.annotations "namespace.cozystack.io/ingress" }}
+{{- $ingress := (dig "metadata" "annotations" "namespace.cozystack.io/ingress" "" $myNS) }}🏁 Script executed:
#!/bin/bash
echo "=== myNS Definition Context ==="
rg -n "\$myNS\s*:=" -C3 packages/extra/seaweedfs/templates/seaweedfs.yaml || true
echo
echo "=== index on annotations Context ==="
rg -n "index\s+\$myNS\.metadata\.annotations" -C2 packages/extra/seaweedfs/templates/seaweedfs.yaml || trueLength of output: 353
Avoid template panics on nil annotations
The index function will error if .metadata.annotations is nil in a fresh namespace. Wrap access in default or use dig to safely handle missing annotations.
• File packages/extra/seaweedfs/templates/seaweedfs.yaml, lines 35–36
• Replace direct index calls with a nil-safe lookup:
- {{- $ingress := index $myNS.metadata.annotations "namespace.cozystack.io/ingress" }}
- {{- $host := index $myNS.metadata.annotations "namespace.cozystack.io/host" }}
+ {{- $ingress := (dig "metadata" "annotations" "namespace.cozystack.io/ingress" "" $myNS) }}
+ {{- $host := (dig "metadata" "annotations" "namespace.cozystack.io/host" "" $myNS) }}Alternatively, you can initialize annotations with default:
{{- $anns := default dict $myNS.metadata.annotations }}
{{- $ingress := index $anns "namespace.cozystack.io/ingress" }}
{{- $host := index $anns "namespace.cozystack.io/host" }}🤖 Prompt for AI Agents
In packages/extra/seaweedfs/templates/seaweedfs.yaml at lines 35 to 36, the
current use of the index function on .metadata.annotations can cause template
panics if annotations is nil. To fix this, wrap the annotations access with the
default function to provide an empty dictionary when annotations is nil, then
use index on this safe dictionary. For example, assign annotations to a variable
using default dict, then use index on that variable to safely retrieve the
ingress and host values.
packages/extra/seaweedfs/values.yaml
Outdated
| ## @param filer.external Enable external access to the SeaweedFS filer from outside the cluster. Use this when `topology` is not set to `Client`. | ||
| filer: | ||
| external: false | ||
|
|
||
| ## @param remoteFilerEndpoint The endpoint of the remote filer GRPC service. Used when `topology` is set to `Client`. | ||
| remoteEndpoint: "1.2.3.4:18888" |
There was a problem hiding this comment.
Parameter name drift – remoteEndpoint lacks metadata and the schema, triggering CI failure.
values.yaml introduces remoteEndpoint, while the metadata generator expects
remoteFilerEndpoint, causing:
Make generate failed: Missing metadata for key 'remoteEndpoint' …
Unify the key name across values.yaml, values.schema.json, comments and
templates.
-## @param remoteFilerEndpoint The endpoint of the remote filer GRPC service. Used when `topology` is set to `Client`.
-remoteEndpoint: "1.2.3.4:18888"
+## @param remoteEndpoint The endpoint of the remote filer GRPC service. Used when `topology` is set to `Client`.
+remoteEndpoint: "1.2.3.4:18888"Don’t forget to add the field to values.schema.json.
🤖 Prompt for AI Agents
In packages/extra/seaweedfs/values.yaml around lines 35 to 40, the parameter
name remoteEndpoint is inconsistent with the expected remoteFilerEndpoint,
causing CI failures due to missing metadata. Rename remoteEndpoint to
remoteFilerEndpoint in values.yaml, update all related comments and templates to
use remoteFilerEndpoint, and add the corresponding field with proper metadata to
values.schema.json to ensure schema validation passes.
f23ffda to
1f2691f
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
packages/extra/seaweedfs/templates/svc-external.yaml (1)
15-17: Selector still mismatches pod labels – service will be emptySeaweedFS pods are labelled
app.kubernetes.io/name: seaweedfs; using{{ $.Release.Name }}prevents the Service from selecting the filer pods.- app.kubernetes.io/name: {{ $.Release.Name }} + app.kubernetes.io/name: seaweedfspackages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml (1)
3-9:BucketClassmissingspec:block – will fail CRD validation
driverNameanddeletionPolicymust be nested underspecfor aBucketClass.kind: BucketClass apiVersion: objectstorage.k8s.io/v1alpha1 metadata: name: {{ .Release.Namespace }} -driverName: {{ .Release.Namespace }}.seaweedfs.objectstorage.k8s.io -deletionPolicy: Delete +spec: + driverName: {{ .Release.Namespace }}.seaweedfs.objectstorage.k8s.io + deletionPolicy: Deletepackages/extra/seaweedfs/templates/seaweedfs.yaml (3)
2-7: Still missing placeholder-value guard for.Values.remoteEndpoint
Only checking for an empty string allows the default1.2.3.4:18888(or any other nonsense) to slip through and break the COSI driver at runtime. The pre-flight block should also reject obviously invalid placeholders.
25-29: Topology auto-detection still uses wrong resource names
The PVC and StatefulSet look-ups are missing the-systemsuffix (data1-%s-system-volume-0,%s-system-master) so upgrades from pre-existing installations are still mis-detected, enabling an unintended topology switch.
37-39: Potential template panic on missing namespace annotations
index $myNS.metadata.annotations …panics whenannotationsisnil. Wrap withdig/defaultbefore dereferencing.-{{- $ingress := index $myNS.metadata.annotations "namespace.cozystack.io/ingress" }} -{{- $host := index $myNS.metadata.annotations "namespace.cozystack.io/host" }} +{{- $ingress := (dig "metadata" "annotations" "namespace.cozystack.io/ingress" "" $myNS) }} +{{- $host := (dig "metadata" "annotations" "namespace.cozystack.io/host" "" $myNS) }}packages/extra/seaweedfs/templates/external/cosi-deployment.yaml (3)
3-4: Same annotation panic risk as inseaweedfs.yamlWrap
indexcalls withdig/defaultto avoid nil dereference when the namespace lacks the expected annotations.
43-46:ENDPOINTmay render an invalid URL
If both.Values.hostand the namespace annotation are unset you gethttps://s3.. Fail fast or supply a sane fallback host.{{- if not (or .Values.host $host) }} {{- fail "Set .Values.host or annotate the namespace with 'namespace.cozystack.io/host'" }} {{- end }} - value: https://{{ .Values.host | default (printf "s3.%s" $host) }} + value: https://{{ .Values.host | default (printf "s3.%s" $host) }}
29-30: RemovecreationTimestamp: null
Kubernetes sets this automatically; leaving it clutters the manifest.
🧹 Nitpick comments (1)
packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml (1)
8-27: Scoping RBAC verbs more tightly would be safer
update,delete, andcreateon all bucket-related resources (including status sub-resources) is broad. Consider least-privilege: split status verbs, or dropdeleteif not strictly required by the driver.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/extra/seaweedfs/Makefile(1 hunks)packages/extra/seaweedfs/README.md(1 hunks)packages/extra/seaweedfs/images/objectstorage-sidecar.tag(1 hunks)packages/extra/seaweedfs/templates/cm.yaml(1 hunks)packages/extra/seaweedfs/templates/dashboard-resourcemap.yaml(2 hunks)packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml(1 hunks)packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml(1 hunks)packages/extra/seaweedfs/templates/external/cosi-deployment.yaml(1 hunks)packages/extra/seaweedfs/templates/external/cosi-service-account.yaml(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(3 hunks)packages/extra/seaweedfs/templates/svc-external.yaml(1 hunks)packages/extra/seaweedfs/templates/vpa.yaml(2 hunks)packages/extra/seaweedfs/values.schema.json(2 hunks)packages/extra/seaweedfs/values.yaml(2 hunks)packages/system/objectstorage-controller/Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extra/seaweedfs/images/objectstorage-sidecar.tag
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/extra/seaweedfs/Makefile
- packages/extra/seaweedfs/templates/dashboard-resourcemap.yaml
- packages/extra/seaweedfs/values.yaml
- packages/extra/seaweedfs/README.md
- packages/extra/seaweedfs/templates/cm.yaml
- packages/extra/seaweedfs/values.schema.json
🧰 Additional context used
🧠 Learnings (6)
packages/system/objectstorage-controller/Makefile (2)
Learnt from: NickVolynkin
PR: #1196
File: packages/apps/http-cache/Makefile:24-27
Timestamp: 2025-07-14T16:23:12.803Z
Learning: In the cozystack repository, the readme-generator tool removes enum contents from values.schema.json files during its operation. Therefore, when using readme-generator in Makefiles, any enum values need to be injected back into the schema using yq commands after readme-generator has run, not before.
Learnt from: NickVolynkin
PR: #1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.
packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml (4)
Learnt from: lllamnyp
PR: #1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.354Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.
Learnt from: lllamnyp
PR: #1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.624Z
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: NickVolynkin
PR: #1120
File: packages/apps/ferretdb/README.md:35-37
Timestamp: 2025-07-02T09:58:11.406Z
Learning: In the cozystack repository, the maintainer NickVolynkin prefers to keep realistic-looking example credentials in README documentation rather than using generic placeholders like <ACCESS_KEY>, even though they are just examples and not real secrets.
Learnt from: lllamnyp
PR: #1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is kuberneteses.apps.cozystack.io, not kubernetes.apps.cozystack.io. This is defined in the API schema even though it's not grammatically perfect.
packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml (1)
Learnt from: lllamnyp
PR: #1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.354Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.
packages/extra/seaweedfs/templates/external/cosi-deployment.yaml (1)
Learnt from: lllamnyp
PR: #1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the {{- directive chomps all leading whitespace including newlines back to the previous content, so {{- toYaml .Values.something | nindent 2 }} will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}).
packages/extra/seaweedfs/templates/seaweedfs.yaml (2)
Learnt from: lllamnyp
PR: #1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.624Z
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: lllamnyp
PR: #1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the {{- directive chomps all leading whitespace including newlines back to the previous content, so {{- toYaml .Values.something | nindent 2 }} will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}).
packages/extra/seaweedfs/templates/vpa.yaml (1)
Learnt from: lllamnyp
PR: #1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.624Z
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.
🪛 YAMLlint (1.37.1)
packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/external/cosi-cluster-role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 21-21: wrong indentation: expected 2 but found 4
(indentation)
[warning] 30-30: wrong indentation: expected 2 but found 4
(indentation)
[warning] 38-38: wrong indentation: expected 2 but found 4
(indentation)
[warning] 41-41: wrong indentation: expected 2 but found 4
(indentation)
[warning] 54-54: wrong indentation: expected 0 but found 2
(indentation)
packages/extra/seaweedfs/templates/external/cosi-deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/external/cosi-service-account.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/seaweedfs.yaml
[warning] 6-6: too many spaces after hyphen
(hyphens)
[warning] 23-23: too many spaces after hyphen
(hyphens)
[warning] 25-25: too many spaces after hyphen
(hyphens)
[warning] 26-26: too many spaces after hyphen
(hyphens)
[warning] 27-27: too many spaces after hyphen
(hyphens)
[warning] 28-28: too many spaces after hyphen
(hyphens)
[warning] 29-29: too many spaces after hyphen
(hyphens)
packages/extra/seaweedfs/templates/svc-external.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/seaweedfs/templates/vpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/extra/seaweedfs/templates/external/cosi-service-account.yaml (1)
1-8: No issues found – template looks correctConditional rendering, metadata and the
automountServiceAccountTokenflag are all valid for a ServiceAccount.packages/extra/seaweedfs/templates/vpa.yaml (1)
1-1: YAML-lint false positive – nothing to fixThe leading Helm directive
{{- if … }}confuses YAML-lint but is perfectly valid during template rendering.
| @@ -27,3 +27,4 @@ image-controller image-sidecar: | |||
| IMAGE="$(REGISTRY)/objectstorage-$(TARGET):$(call settag,$(TAG))@$$(yq e '."containerimage.digest"' images/$(TARGET).json -r)" && \ | |||
| yq -i '$(YAML_PATH) = strenv(IMAGE)' $(VALUES_FILE) | |||
| rm -f images/$(TARGET).json | |||
| yq .seaweedfs.cosi.sidecar.image ../seaweedfs/values.yaml > ../../extra/seaweedfs/images/objectstorage-sidecar.tag | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Run-once extraction should be gated to the sidecar build
yq … > objectstorage-sidecar.tag is executed for both image-controller and image-sidecar.
When make image (without -j) runs, the first invocation (controller) writes a stale tag that is only corrected later by the sidecar build. With parallel or incremental builds this becomes racy and may leave an incorrect image reference in objectstorage-sidecar.tag.
- yq .seaweedfs.cosi.sidecar.image ../seaweedfs/values.yaml > ../../extra/seaweedfs/images/objectstorage-sidecar.tag
+ @if [ "$(TARGET)" = "sidecar" ]; then \
+ yq '.seaweedfs.cosi.sidecar.image' ../seaweedfs/values.yaml \
+ > ../../extra/seaweedfs/images/objectstorage-sidecar.tag ; \
+ fi🤖 Prompt for AI Agents
In packages/system/objectstorage-controller/Makefile at line 30, the command
extracting the sidecar image tag using yq is run for both image-controller and
image-sidecar builds, causing race conditions and stale tags. Modify the
Makefile to ensure this extraction command runs only during the sidecar build
target, gating it so it does not execute during the controller build. This
prevents concurrent writes and ensures the tag file is correctly updated only
once per build.
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
Release note
Summary by CodeRabbit
New Features
filer.externalto allow external filer access, andremoteEndpointfor specifying a remote filer service when using "Client" topology.Improvements
Bug Fixes