Repository navigation
fix(platform): stop handing tenants a root S3 the root tenant does not run - #5097
europrinter (yankawai) wants to merge 1 commit into
Conversation
…t run cozystack-basics wrote seaweedfs: tenant-root into the root values unconditionally, so every tenant without its own SeaweedFS inherited tenant-root as its S3 even when Tenant/root runs none. A Bucket there got a BucketClaim for a bucket class that never appears, and Harbor got its registry bucket the same way. seaweedfs.rootEnabled records that choice the way monitoring.rootEnabled does for monitoring, and the bucket and harbor charts refuse to render without S3 up the tenant chain rather than wait on a class that does not exist. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>
github-actions
Bot
added
area/platform
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Configure and propagate root availability packages/core/platform/values.yaml, packages/core/platform/values-isp-*.yaml, packages/core/platform/templates/apps.yaml, packages/core/platform/tests/apps_seaweedfs_enabled_wiring_test.yaml |
Adds seaweedfs.rootEnabled, defaulting to true, and sets it to false in ISP slim configurations. The platform template writes seaweedfs-enabled using this setting. Tests cover the default, disabled, and null configuration cases. |
Conditionally expose SeaweedFS to tenants packages/system/cozystack-basics/templates/cozystack-values-secret.yaml, packages/system/cozystack-basics/tests/cozystack-values-secret_test.yaml |
The shared values Secret includes _namespace.seaweedfs when _cluster.seaweedfs-enabled is missing or true, and omits it when false. Tests cover these cases and check that monitoring remains present when SeaweedFS is omitted. |
Require an available bucket backend packages/apps/bucket/templates/bucketclaim.yaml, packages/apps/bucket/tests/s3_backend_test.yaml, packages/apps/harbor/templates/bucket.yaml, packages/apps/harbor/tests/s3_backend_test.yaml |
Bucket and Harbor templates fail rendering when the tenant’s SeaweedFS value is empty or unset. Tests check rendering with a configured bucket class and the error for a missing backend. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant PlatformValues
participant AppsTemplate
participant BasicsSecret
participant BucketTemplates
PlatformValues->>AppsTemplate: Set seaweedfs.rootEnabled
AppsTemplate->>BasicsSecret: Provide seaweedfs-enabled
BasicsSecret->>BucketTemplates: Include _namespace.seaweedfs when enabled
BucketTemplates->>BucketTemplates: Fail rendering if the value is absent
Possibly related issues
- Issue 5092: The platform setting is added to shared values and copied into a Secret, which is related to the issue’s reported upgrade behavior.
Possibly related PRs
- cozystack/cozystack#5096: This PR addresses the bucket-claim provisioning path, while
#5096changes workload readiness when a bucket claim is not provisioned.
Suggested labels: kind/bug, size/L, area/storage
Suggested reviewers: lexfrei, kvaps
Merge Risk: 
5a646
-
The configuration now rejects Bucket and Harbor deployments without an available SeaweedFS backend. No actionable merge-blocking risk was established.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main behavior change: the platform stops giving tenants a root S3 endpoint that the root tenant does not run. The changeset adds seaweedfs.rootEnabled, gates cozystack-basics' seaw… Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main behavior change: the platform stops giving tenants a root S3 endpoint that the root tenant does not run. The changeset adds seaweedfs.rootEnabled, gates cozystack-basics' seaweedfs: tenant-root output on it, and makes the Bucket and Harbor charts fail when no S3 is configured up the tenant chain. The title is specific, follows the conventional commit style, and fits within a reasonable length. It is a bit long at 74 characters and omits the new failing-render behavior for Bucket and Harbor, but it captures the primary intent.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (12 skipped: 12 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
europrinter (@yankawai) The gate is right; two things before it can merge.
-
Conflict with main in
platform/templates/apps.yaml,platform/values.yaml,cozystack-basics/templates/cozystack-values-secret.yamland its test: main now gates_namespace.ingresson_cluster.ingress-enabledthe same way. Keep both gates, and update the comment and the test names that say etcd, ingress and monitoring are unconditional: after the merge only etcd and monitoring are. I didn't merge it myself because those sentences need your wording. -
Release note: slim variants now set
seaweedfs.rootEnabled: false, butvalues-isp-slim.yamlallows opting SeaweedFS in onTenant/root. Such a cluster loses_namespace.seaweedfson upgrade and its Buckets and Harbors fail the next render. Add that slim clusters running root SeaweedFS must setseaweedfs.rootEnabled: true.
What this PR does
cozystack-basicswritesseaweedfs: tenant-rootinto thecozystack-valuesSecret of the root tenant unconditionally, so every tenant without its own SeaweedFS inheritstenant-rootas its S3, even whenTenant/roothasspec.seaweedfs: false. On a v1.6.5 cluster whose root tenant runs no SeaweedFS, a tenant created a Bucket, its BucketClaim asked for bucket classtenant-root, and the objectstorage controller loggedbucketclasses.objectstorage.k8s.io "tenant-root" not found8796 times over three days while the app stayed Ready. #5096 makes that state visible in WorkloadsReady, this PR removes its cause.The new platform value
seaweedfs.rootEnabledreaches cozystack-basics as_cluster.seaweedfs-enabled, the same waymonitoring.rootEnabledreaches it as_cluster.monitoring-enabledfor #3181, and_namespace.seaweedfs: tenant-rootis written only when it is true. The defaulttruekeeps today's behaviour. The slim variants, which do not install SeaweedFS for the root tenant, set it tofalsenext tomonitoring.rootEnabled. Without S3 up the tenant chain the bucket and harbor charts now fail to render with a message that names the problem, instead of creating a claim for a class that never appears. A lookup of the live Tenant is not used, for the reasonmonitoring-external-services.yamlgives for the monitoring gate.An existing Bucket or Harbor in a tenant without S3 up its chain fails its next upgrade with that message once the flag is false. Its BucketClaim was never provisioned, so nothing that worked stops working.
A new
apps_seaweedfs_enabled_wiring_test.yamlin the platform chart pins the flag in_clusterfor the default,falseand null cases.cozystack-values-secret_test.yamlkeepsetcd,ingressandmonitoringunconditional and checksseaweedfsfor an absent,"true"and"false"flag. News3_backend_test.yamlsuites in bucket and harbor check the class and the refusal. With helm-unittest 1.1.2 the platform passes 288 of 288, cozystack-basics 69 of 69, bucket 13 of 13 and harbor 49 of 49. Against the templates on main the new cases fail, 3 in the platform, 1 in cozystack-basics and 1 each in bucket and harbor, and the rest pass. The harbor suites that render every template already set_namespace.seaweedfs, and the full e2e setsspec.seaweedfs: trueon the root Tenant and keeps the default. I have not run the change on a live cluster.Screenshots
Not a UI change.
Downstream repositories
packages/core/platform/values.yamlgainsseaweedfs.rootEnabled, which the trigger map routes toplatform-package.mdon the website. The variants page lists what the slim variants need for SeaweedFS, so it gets the same note as monitoring. No app schema, values default or other contract changes.Release note
Summary by CodeRabbit