Repository navigation
fix(seaweedfs): point S3 consumers at the reachable S3 service - #3119
Conversation
The s3-service-name.patch renamed the S3 Service to <name>-s3 (seaweedfs-s3), but several consumers still resolved it through componentName -> <fullname>-s3. name and fullname only coincide when the release name equals the chart name; the shipped release is seaweedfs-system, so they pointed at seaweedfs-system-s3 while the real service stayed seaweedfs-s3: - the S3 ingress backend and the iceberg ingress backend, which left ingress-nginx returning 503 on the external S3 endpoint. The COSI provisioner in-cluster ENDPOINT was wrong on the same axis and also on the port. Both the s3-gateway branch and the filer-S3 branch rendered a portless URL that defaults to 443, while the s3 service only listens on the S3 port (8333); the filer branch additionally targeted the filer service, which serves no S3. Both in-cluster branches now resolve seaweedfs-s3 on the configured S3 port. The default path (s3.ingress.enabled=true) is repaired by the ingress fix; the COSI in-cluster paths are latent until an operator disables the ingress. Resolve every consumer through the same name helper the Service uses, and pin the contract with unittests rendered under the production release name so the mismatch cannot recur. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
added
the
kind/backport
github-actions
Bot
added
area/storage
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 addresses a service discovery issue where several SeaweedFS components were attempting to connect to a non-existent S3 service name. By aligning these consumers with the standardized 'seaweedfs-s3' service, the changes resolve 503 errors on ingress endpoints and enable proper connectivity for the COSI provisioner. The fix is supported by new automated tests to prevent future regressions. 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
|
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughFixes a regression where S3-related Helm consumers (S3 ingress, iceberg ingress, COSI deployment) resolved service names via ChangesSeaweedFS S3 service name consumer fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the SeaweedFS package by adding a new patch s3-service-name-consumers.patch to the Makefile and introducing a comprehensive test suite in s3_service_name_consumers_test.yaml. The tests verify that various S3 consumers (such as S3 ingress, iceberg ingress, and COSI in-cluster endpoints) correctly resolve the renamed S3 service (seaweedfs-s3) rather than falling back to an incorrect fullname-derived service name. No review comments were provided, so there is no additional feedback.
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.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — completes the seaweedfs-s3 Service rename by aligning the three remaining consumers that still resolved the non-existent <fullname>-s3 name. Correct for every deployment mode and pinned by passing unit tests.
Business context: a prior patch renamed the S3 Service from componentName-based (seaweedfs-system-s3) to name-based (seaweedfs-s3), but the S3 ingress, iceberg ingress, and COSI endpoint still pointed at the old name — 503 on the external S3 endpoint and a COSI provisioner that can't reach S3 in-cluster.
Verified
- Root cause —
s3-service.yamlnames the Service viaseaweedfs.name(seaweedfs-s3), whileseaweedfs.componentNameis fullname-based (seaweedfs-system-s3). Since the shipped release isseaweedfs-system, name ≠ fullname and the old consumers resolved a Service that does not exist. - Coverage — the only in-chart consumers that resolve the S3 Service by name are the three this PR touches.
SEAWEEDFS_FILERand the all-in-one ingress branch correctly remain on fullname-based names, because those Services are themselves fullname-based. - Ports —
s3-service.yamlexposess3.portwhens3.enabled, elsefiler.s3.port; the COSI branches now mirror that exactly. The old URLs were portless (defaulted to 443). - TLS scheme —
s3-tls-main-port.patchapplies TLS to the main S3 port underenableSecurity(set in the package overlay), sohttps://…:8333is correct. - Filer-S3-only mode —
seaweedfs-s3is created ons3.enabled OR filer.s3.enabled, so the endpoint resolves in filer mode too. - Tests — the suite passes (6/6) and the package has a wired
test:target, so the pin runs in CI.
Non-blocking follow-ups
- The description says the COSI filer branch "targeted the filer Service, which serves no S3." The filer Service does expose an
swfs-s3port (filer.s3.port) whenfiler.s3.enabled; the real defect in that branch was the missing port (the URL defaulted to 443), not the absence of an S3 listener. The fix is correct either way — just worth tightening the rationale. - The same
<name>-s3string is built three different ways across the files ({{ template "seaweedfs.name" . }}-s3,printf "%s-s3" (include "seaweedfs.name" .), and inline in the COSIprintf). A smallseaweedfs.s3ServiceNamehelper would centralize it and structurally prevent the exact drift this PR fixes; the test pin is an acceptable alternative.
|
Successfully created backport PR for |
myasnikovdaniil
added
the
kind/backport-previous
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.5
git worktree add -d .worktree/backport-3119-to-release-1.5 origin/release-1.5
cd .worktree/backport-3119-to-release-1.5
git switch --create backport-3119-to-release-1.5
git cherry-pick -x 8a14de9abfee64589c4911a567cd8aff9520487d |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.4
git worktree add -d .worktree/backport-3119-to-release-1.4 origin/release-1.4
cd .worktree/backport-3119-to-release-1.4
git switch --create backport-3119-to-release-1.4
git cherry-pick -x 8a14de9abfee64589c4911a567cd8aff9520487d |
|
The
So the external-S3 503 this PR fixes doesn't occur on For the record, the earlier "Backport failed for |
|
Confirming this in the wild on v1.5.1 (Talos, Symptom / evidence:
Confirmed it's exactly the mismatch this PR fixes —
Timeline for anyone else landing here: |
…3155) ## What this PR does The Automatic Backport workflow passed `conflict_resolution: draft_commit_conflicts` as a **top-level** input to `korthout/backport-action`, but the action only reads `conflict_resolution` from inside its `experimental` JSON input. Passed top-level it was silently ignored — the action run logs warn `Unexpected input(s) 'conflict_resolution'` — so it fell back to the `experimental` default of `fail`. On a cherry-pick conflict the action then created **no** pull request and only left a "Backport failed" comment on the source PR (this bit #3121 and #3119). This moves the key into the `experimental` input, so a conflicting backport instead opens a **draft PR with the first conflict committed** for maintainers to resolve in place. No version bump: the pinned `v3.2.1` already supports `draft_commit_conflicts` — the bug was purely the input placement. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Backport pull requests now handle conflicts more reliably and open as draft PRs when conflicts are detected. * Improved configuration handling so the expected conflict behavior is applied consistently during backport automation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
What this PR does
The S3 Service is named with the release-independent
seaweedfs.namehelper (alwaysseaweedfs-s3), but several in-chart consumers still resolved it throughcomponentName(<fullname>-s3). Since the shipped release isseaweedfs-system, those consumers pointed atseaweedfs-system-s3— a Service that does not exist — so ingress-nginx returns 503 on the external S3 endpoint and the COSI provisioner cannot reach S3 in-cluster.This aligns every in-chart S3 consumer with the actual Service:
seaweedfs-s3(fixes the 503 on the defaults3.ingress.enabled: truepath).ENDPOINT(both the s3-gateway and filer-S3 branches) →seaweedfs-s3on the configured S3 port. These were also portless (defaulting to 443 against an S3 that listens on 8333), and the filer branch additionally targeted the filer Service, which serves no S3.The all-in-one ingress branch is intentionally left on
componentName "all-in-one"(a different Service). Changes are captured as a downstream patch (s3-service-name-consumers.patch) wired intomake update, and pinned by helm-unittest rendered under the production release name so the mismatch cannot recur.Release note
Summary by CodeRabbit
Bug Fixes
Tests