Update operator for embedded IAM in S3 server #167
Conversation
This change updates the operator to support the new embedded IAM architecture where IAM API is built into the S3 server (on the same port as S3). Changes: - Update FilerSpec.IAM to control embedded IAM (default: true when S3 enabled) - Remove incorrect -iam/-iam.port flags from filer startup (these don't exist) - Add -iam=false flag when embedded IAM is explicitly disabled - Deprecate standalone IAM StatefulSet with warning - Update sample manifests with new embedded IAM examples - Update IAM_SUPPORT.md documentation This follows the pattern used by MinIO and Ceph RGW where IAM is served on the same endpoint as S3.
|
Warning Rate limit exceeded@chrislusf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (15)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request consolidates IAM support architecture by promoting embedded IAM as the default when S3 is enabled, deprecating standalone IAM, and updating configuration, type definitions, and controller logic to reflect this shift. Documentation, sample configurations, and startup logic are aligned accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Summary of ChangesHello @chrislusf, 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 refactors how Identity and Access Management (IAM) is handled within the SeaweedFS Operator. It transitions from a standalone IAM service to an embedded model where the IAM API runs directly within the S3 server. This change streamlines deployments, reduces resource overhead, and aligns the operator with common industry patterns seen in systems like MinIO and Ceph RGW. The update includes necessary API changes, controller adjustments, and comprehensive documentation to guide users through the new recommended architecture and the deprecation of the standalone IAM service. Highlights
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 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 counter productive. You can react with 👍 and 👎 on @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. 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
|
![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 is a great pull request that significantly simplifies IAM deployment by embedding it into the S3 server, following the pattern of other object storage systems like MinIO. The changes are comprehensive, consistently applied across API definitions, controller logic, sample manifests, and documentation.
The deprecation of the standalone IAM service is handled very well, with clear notices in the API types, sample files, and a helpful warning log for users who are still using the old configuration. The updated documentation, including the new architecture diagrams and migration guide, is excellent and will be very valuable for users.
I have one minor suggestion for the documentation to replace a version placeholder.
I also noticed that the test files were not updated as part of this PR. The changes to the controller logic, especially in buildFilerStartupScript, are not covered by new or updated tests. Additionally, existing tests for IAM configuration might now be outdated due to the new default behavior of filer.iam. I strongly recommend adding tests for the new embedded IAM logic and updating existing tests to reflect these changes to ensure correctness and prevent future regressions.
Overall, the changes look solid and well-implemented, but would be strengthened by accompanying tests.
IAM_SUPPORT.md
Outdated
| ## Overview | ||
|
|
||
| The SeaweedFS Operator now supports deploying IAM services in two ways: | ||
| Starting from SeaweedFS version X.X.X, the IAM API is now **embedded in the S3 server by default**. This follows the pattern used by MinIO and Ceph RGW, providing a simpler deployment model where both S3 and IAM APIs are available on the same port. |
There was a problem hiding this comment.
The documentation refers to a placeholder version X.X.X. Please update this to the actual SeaweedFS version where this change was introduced, or use a more generic phrasing like 'a recent version of SeaweedFS'. This will help users understand the version requirements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
IAM_SUPPORT.md (1)
179-191: Consider adding language specifiers to diagram code blocks.The ASCII diagram code blocks should include a language specifier to satisfy markdown linting rules. Consider adding
textas the language:-``` +```text ┌─────────────────────────────────────────────┐ │ Filer Pod │ ...Apply the same change to the standalone IAM diagram at lines 195-203. Also applies to: 195-203 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 01017805956d6e64da158ca51feae7d63e84d9f8 and feeaefddf6bf70f97d2acab0c7c3af306b6518aa. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `IAM_SUPPORT.md` (4 hunks) * `api/v1/seaweed_types.go` (2 hunks) * `config/samples/seaweed_v1_seaweed_with_iam_embedded.yaml` (2 hunks) * `config/samples/seaweed_v1_seaweed_with_iam_standalone.yaml` (2 hunks) * `internal/controller/controller_filer_statefulset.go` (2 hunks) * `internal/controller/controller_iam.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>IAM_SUPPORT.md</summary> 179-179: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 195-195: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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). (8)</summary> * GitHub Check: test on k8s latest version * GitHub Check: test on k8s previous version * GitHub Check: test on k8s penultimate version * GitHub Check: Resource Validation Test * GitHub Check: test-e2e on k8s latest version * GitHub Check: test-e2e on k8s previous version * GitHub Check: Build Check * GitHub Check: pre-commit </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>internal/controller/controller_iam.go (1)</summary><blockquote> `23-27`: **LGTM! Clear and actionable deprecation warning.** The deprecation warning effectively communicates the architectural shift to embedded IAM and provides clear guidance for users to migrate their configurations. </blockquote></details> <details> <summary>config/samples/seaweed_v1_seaweed_with_iam_embedded.yaml (1)</summary><blockquote> `23-42`: **LGTM! Sample configuration correctly demonstrates embedded IAM.** The updated sample clearly shows that IAM is enabled by default when S3 is enabled, and the comments provide helpful context about the port consolidation and optional nature of the `iam` field. </blockquote></details> <details> <summary>api/v1/seaweed_types.go (2)</summary><blockquote> `239-264`: **LGTM! Clear and comprehensive deprecation notices.** The deprecation notes on `IAMSpec` and its fields are well-documented and provide clear guidance on the recommended alternative (embedded IAM). This will help users understand the transition path. --- `231-236`: **The backward compatibility concern is documented in IAM_SUPPORT.md.** The `+kubebuilder:default:=true` annotation will enable IAM by default for new deployments, which is intentional and explicitly documented in the migration guide. The documentation provides clear before/after examples showing how to migrate from standalone IAM to embedded IAM, and advises users to explicitly set `filer.iam: false` if they need to disable embedded IAM when upgrading. No additional documentation changes are needed. </blockquote></details> <details> <summary>internal/controller/controller_filer_statefulset.go (2)</summary><blockquote> `25-31`: **LGTM! Startup script correctly implements embedded IAM.** The logic properly handles the default-enabled embedded IAM by only adding the `-iam=false` flag when explicitly disabled. The comments clearly explain the behavior. --- `52-59`: **LGTM! Port consolidation correctly implemented.** The removal of the separate IAM port and the addition of explanatory comments correctly reflect that IAM now uses the same port as S3 (8333) when embedded. </blockquote></details> <details> <summary>config/samples/seaweed_v1_seaweed_with_iam_standalone.yaml (1)</summary><blockquote> `1-50`: **LGTM! Deprecated standalone IAM sample is well-documented.** The sample configuration correctly demonstrates backward-compatible standalone IAM usage while prominently displaying deprecation notices. The explicit `iam: false` on line 36 correctly disables embedded IAM when using standalone mode. </blockquote></details> <details> <summary>IAM_SUPPORT.md (1)</summary><blockquote> `1-237`: **LGTM! Comprehensive and well-structured documentation.** The documentation effectively covers: - The architectural shift to embedded IAM - Clear configuration examples for both embedded and standalone modes - Migration guidance from standalone to embedded IAM - Service discovery information - Troubleshooting tips This will help users understand and adopt the new embedded IAM approach. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Standalone IAM has been removed in favor of embedded IAM in the S3 server. When S3 is enabled, IAM API is automatically available on the same port (8333). Removed: - IAMSpec type from API - SeaweedSpec.IAM field - IAM controller files (controller_iam*.go) - IAM-related test files - BaseIAMSpec() method - getIAMPort() helper - Standalone IAM sample manifest Kept: - FilerSpec.IAM field to control embedded IAM (default: true) - Updated filer service to note S3 port serves IAM too This simplifies deployment by removing the option to run standalone IAM, following the pattern used by MinIO and Ceph RGW.
Summary
This PR updates the SeaweedFS operator to support the new embedded IAM architecture introduced in seaweedfs/seaweedfs#7740.
Background
IAM API is now embedded in the S3 server by default (on the same port as S3), following the pattern used by MinIO and Ceph RGW. This simplifies deployment by running both S3 and IAM on a single port.
Changes
API Types (
api/v1/seaweed_types.go)FilerSpec.IAMto control embedded IAM (default: true when S3 enabled)IAMSpec(standalone IAM is deprecated)Controller (
internal/controller/)-iamand-iam.portflags from filer startup script-iam=falseflag when embedded IAM is explicitly disabledSample Manifests (
config/samples/)seaweed_v1_seaweed_with_iam_embedded.yamlfor new architectureseaweed_v1_seaweed_with_iam_standalone.yamlwith deprecation noticeDocumentation (
IAM_SUPPORT.md)Migration Guide
Before (deprecated):
After (recommended):
Related PRs
Summary by CodeRabbit
Documentation
Configuration Changes
✏️ Tip: You can customize this high-level summary in your review settings.