- Add PoolSize to LDAPConfig (default 10)
- Add Audience to LDAPConfig to align with OIDC validation
- Update initialization and ValidateToken to use new fields
- Move common logic for session claims and credential generation to prepareSTSCredentials
- Update handleAssumeRole and handleAssumeRoleWithLDAPIdentity to use the helper
- Remove stale comments referencing outdated line numbers
- Consistent with AWS, include the account ID in the assumed-role ARN
- Use the configured account ID from STS service if available, otherwise default to '111122223333'
- Apply to both AssumeRole and AssumeRoleWithLDAPIdentity handlers
- Also update .gitignore to ignore IAM test environment files
- Set Subject, Issuer, Audience, IssuedAt, and ExpiresAt to satisfy the interface
- Use time.Time for timestamps as required by TokenClaims struct
- Default to 1 hour TTL for LDAP tokens
- Add atomic 'closed' flag to connection pool to prevent racing on Close()
- Rebind authenticated user connections back to service account before returning to pool
- Close connections on error instead of returning potentially corrupted state to pool
The GitHub Actions workflow calls 'make setup' which was not creating
iam_config.json, causing the server to start without IAM integration
enabled (iamIntegration = nil), resulting in NotImplemented errors.
Now 'make setup' copies iam_config.local.json to iam_config.json if
it doesn't exist, ensuring IAM is properly configured in CI.
Updated s3_iam_distributed_test.go to use 'admin-user' and 'read-user'
which exist in the standard Keycloak setup provided by setup_keycloak.sh.
This resolves 'unknown test user' errors in distributed integration tests.
Following the change to make TokenGenerator public in STSService,
this commit updates the test files to reference the correct public field name.
This resolves compilation errors in the IAM STS test suite.
CRITICAL FIX:
- Make TokenGenerator public in STSService (was private tokenGenerator)
- Update all references from Config.TokenGenerator to TokenGenerator
- Remove TokenGenerator from STSConfig (it belongs in STSService)
This fixes the "NotImplemented" errors in distributed and Keycloak tests.
The issue was that Round 5 changes tried to access Config.TokenGenerator
which didn't exist - TokenGenerator is a field in STSService, not STSConfig.
The TokenGenerator is properly initialized in STSService.Initialize() and
is now accessible for JWT token generation in AssumeRole handlers.
PERFORMANCE IMPROVEMENT:
- Add connection pool to LDAPProvider (default size: 10 connections)
- Reuse LDAP connections across authentication requests
- Prevent file descriptor exhaustion under high load
IMPLEMENTATION:
- connectionPool struct with channel-based connection management
- getConnection(): retrieves from pool or creates new connection
- returnConnection(): returns healthy connections to pool
- createConnection(): establishes new LDAP connection with TLS support
- Close(): cleanup method to close all pooled connections
- Connection health checking (IsClosing()) before reuse
BENEFITS:
- Reduced connection overhead (no TCP handshake per request)
- Better resource utilization under load
- Prevents "too many open files" errors
- Non-blocking pool operations (creates new conn if pool empty)
- LDAP Provider Hardening:
- Prevent re-initialization
- Enforce single user match in `GetUserInfo` (was explicit only in Authenticate)
- Ensure connection closure if StartTLS fails
- STS Handlers:
- Add robust provider detection using type assertion
- **Security**: Implement authorization check (`VerifyActionPermission`) after LDAP authentication
- Routing:
- Update tests to reflect that STS actions are handled by STS handler, not generic IAM
- Improve `LDAPProvider` connection handling:
- Use `net.Dialer` with configured timeout for connection establishment
- Enforce TLS 1.2+ (`MinVersion: tls.VersionTLS12`) for both LDAPS and StartTLS
- Fix build error in `s3api_sts.go` (format verb for ErrorCode)
- Implement `validateLDAPConfig` in `ProviderFactory`
- Improve `LDAPProvider.Initialize`:
- Support `connectionTimeout` parsing (string/int/float) from config map
- Warn if `BindDN` is present but `BindPassword` is empty
- Improve `LDAPProvider.GetUserInfo`:
- Add fallback to `searchUserGroups` if `memberOf` returns no groups (consistent with Authenticate)
This commit addresses all critical security issues from PR review:
Security Fixes:
- Use crypto/rand for cryptographically secure credential generation
instead of time.Now().UnixNano() (fixes predictable credentials)
- Add sts:AssumeRole permission check via VerifyActionPermission to
prevent unauthorized role assumption
- Generate proper session tokens using crypto/rand instead of
placeholder strings
Code Quality Improvements:
- Refactor DurationSeconds parsing into reusable parseDurationSeconds()
helper function used by all three STS handlers
- Create generateSecureCredentials() helper for consistent and secure
temporary credential generation
- Fix iamMatcher to check query string as fallback when Action not
found in form data
LDAP Provider Implementation:
- Add go-ldap/ldap/v3 dependency
- Create LDAPProvider implementing IdentityProvider interface with
full LDAP authentication support (connect, bind, search, groups)
- Update ProviderFactory to create real LDAP providers
- Wire LDAP provider into AssumeRoleWithLDAPIdentity handler
Test Infrastructure:
- Add LDAP user creation verification step in setup_all_tests.sh
Update the IAM handler matcher to check for STS actions (AssumeRole,
AssumeRoleWithWebIdentity, AssumeRoleWithLDAPIdentity) and exclude them
from IAM handler processing. This allows STS requests to be handled by
the STS fallback handler even when they include AWS SigV4 authentication.
The matcher now parses the form data to check the Action parameter and
returns false for STS actions, ensuring they are routed to the correct
handler.
Note: This is a work-in-progress fix. Tests are still showing some
routing issues that need further investigation.
- Add s3_sts_assume_role_test.go with comprehensive tests for AssumeRole:
* Parameter validation (missing RoleArn, RoleSessionName, invalid duration)
* AWS SigV4 authentication with valid/invalid credentials
* Temporary credential generation and usage
- Add s3_sts_ldap_test.go with tests for AssumeRoleWithLDAPIdentity:
* Parameter validation (missing LDAP credentials, RoleArn)
* LDAP authentication scenarios (valid/invalid credentials)
* Integration with LDAP server (when configured)
- Update Makefile with new test targets:
* test-sts: run all STS tests
* test-sts-assume-role: run AssumeRole tests only
* test-sts-ldap: run LDAP STS tests only
* test-sts-suite: run tests with full service lifecycle
- Enhance setup_all_tests.sh:
* Add OpenLDAP container setup for LDAP testing
* Create test LDAP users (testuser, ldapadmin)
* Set LDAP environment variables for tests
* Update cleanup to remove LDAP container
- Fix setup_keycloak.sh:
* Enable verbose error logging for realm creation
* Improve error diagnostics
Tests use fail-fast approach (t.Fatal) when server not configured,
ensuring clear feedback when infrastructure is missing.
* Fix chown Input/output error on large file sets (Fixes#7911)
Implemented retry logic for MySQL/MariaDB backend to handle transient errors like deadlocks and timeouts.
* Fix syntax error: missing closing brace
* Refactor: Use %w for error wrapping and errors.As for extraction
* Fix: Disable retry logic inside transactions
There is a mistmatch in the conditionals for the definition and mounting of the `config-users` volume in the filer's template.
Volume definition:
```
{{- if and .Values.filer.s3.enabled .Values.filer.s3.enableAuth }}
```
Mount:
```
{{- if .Values.filer.s3.enableAuth }}
```
This leads to an invalid specification in the case where s3 is disabled but the enableAuth value is set to true, as it tries to mount in an undefined volume. I've fixed it here by adding the extra check to the latter conditional.
Fixes#7990
The issue was that the Charset constant used for generating secret keys
included the '/' character, which is URL-unsafe. When secret keys
containing '/' were used in HTTP requests, they would be URL-encoded,
causing a mismatch during signature verification.
Changes:
- Removed '/' from the Charset constant in weed/iam/constants.go
- Added TestGenerateSecretAccessKey_URLSafe to verify generated keys
don't contain URL-unsafe characters like '/' or '+'
This ensures all newly generated secret keys are URL-safe and will
work correctly with S3 authentication. Existing keys continue to work.
Previously, `weed mini` logic duplicated the credential loading process
by creating a temporary IAM config file from environment variables.
`auth_credentials.go` also had fallback logic to load these variables.
This change:
1. Updates `auth_credentials.go` to *always* check for and merge
AWS environment variable credentials (`AWS_ACCESS_KEY_ID`, etc.)
into the identity list. This ensures they are available regardless
of whether other configurations (static file or filer) are loaded.
2. Removes the redundant file creation logic from `weed/command/mini.go`.
3. Updates `weed mini` user messages to accurately reflect that
credentials are loaded from environment variables in-memory.
This results in a cleaner implementation where `weed/s3api` manages
all credential loading logic, and `weed mini` simply relies on it.
Refactored `NewIdentityAccessManagementWithStore` to remove mutual
exclusivity between static (file-based) and dynamic (filer-based)
configuration loading.
Previously, if a static config configuration was present (including the
legacy `IamConfig` option used by `weed mini`), it prevented loading
users from the filer.
Now, the system loads the static configuration first (if present), and
then *always* attempts to merge in the dynamic configuration from the
filer. This ensures that:
1. Static users (e.g. from `weed mini` env vars or `-s3.config`) are loaded and protected.
2. Dynamic users (e.g. created via Admin UI and stored in Filer) are also loaded and available.
`weed mini` sets the `-s3.iam.config` flag instead of `-s3.config`,
which populates `S3ApiServerOption.IamConfig`.
Previously, `NewIdentityAccessManagementWithStore` only checked
`option.Config`. This caused `weed mini` generated credentials (written
to a temp file passed via IamConfig) to be ignored, breaking S3 access
in mini mode even when environment variables were provided.
This change ensures we try to load the configuration from `IamConfig`
if `Config` is empty, restoring functionality for `weed mini`.
Fixed regression where AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
environment variables were not being loaded as fallback credentials.
The issue was that configLoaded was set to true when filer call
succeeded, even if it returned an empty configuration. This blocked
the environment variable fallback logic.
Now only set configLoaded = true when we actually have loaded
identities, allowing env vars to work correctly in mini mode and
other scenarios where filer config is empty.
* fix(s3api): ensure static config file takes precedence over dynamic updates
When a static S3 configuration file is provided, avoid overwriting
the configuration from dynamic filer updates. This ensures the
documented "Highest Priority" for the configuration file is respected.
* refactor(s3api): implement merge-based static config with immutable identities
Static identities from config file are now immutable and protected from
dynamic updates. Dynamic identities (from admin panel) can be added and
updated without affecting static entries.
- Track identity names loaded from static config file
- Implement merge logic that preserves static identities
- Allow dynamic identities to be added or updated
- Remove blanket block on config file updates
* fix: address PR review comments for static config merge logic
Critical Bugs:
- Fix existingIdx always-false condition causing duplicate identities
- Fix race condition in static config initialization (move useStaticConfig inside mutex)
Security & Robustness:
- Add nil identity check in VerifyActionPermission to fail closed
- Mask access keys in STS validation logs to avoid exposing credentials
- Add nil guard for s3a.iam in subscription handler
Test Improvements:
- Add authCalled tracking to MockIAMIntegration for explicit verification
- Lower log level for static config messages to reduce noise
* fix: prevent duplicates and race conditions in merge logic
Data Integrity:
- Prevent service account credential duplicates on repeated merges
- Clean up stale accessKeyIdent entries when replacing identities
- Check existing credentials before appending
Concurrency Safety:
- Add synchronization to IsStaticConfig method
Test Improvements:
- Add mux route vars for proper GetBucketAndObject extraction
- Add STS session token header to trigger correct auth path
* Fix STS authorization in streaming/chunked uploads
During streaming/chunked uploads (SigV4 streaming), authorization happens
twice:
1. Initial authorization in authRequestWithAuthType() - works correctly
2. Second authorization in verifyV4Signature() - was failing for STS
The issue was that verifyV4Signature() only used identity.canDo() for
permission checks, which always denies STS identities (they have empty
Actions). This bypassed IAM authorization completely.
This commit makes verifyV4Signature() IAM-aware by adding the same
fallback logic used in authRequestWithAuthType():
- Traditional identities (with Actions) use legacy canDo() check
- STS/JWT identities (empty Actions) fall back to IAM authorization
Fixes: https://github.com/seaweedfs/seaweedfs/pull/7986#issuecomment-3723196038
* Add comprehensive unit tests for STS authorization in streaming uploads
Created test suite to verify that verifyV4Signature properly handles STS
identities by falling back to IAM authorization when shouldCheckPermissions
is true.
Tests cover:
- STS identities with IAM integration (allow and deny cases)
- STS identities without IAM integration (should deny)
- Traditional identities with Actions (canDo check)
- Permission check bypass when shouldCheckPermissions=false
- Specific streaming upload scenario from bug report
- Action determination based on HTTP method
All tests pass successfully.
* Refactor authorization logic to avoid duplication
Centralized the authorization logic into IdentityAccessManagement.VerifyActionPermission.
Updated auth_signature_v4.go and auth_credentials.go to use this new helper.
Updated tests to clarify that they mirror the centralized logic.
* Refactor tests to use VerifyActionPermission directly
Introduced IAMIntegration interface to facilitate mocking of internal IAM integration logic.
Updated IdentityAccessManagement to use the interface.
Updated tests to directy call VerifyActionPermission using a mocked IAM integration, eliminating duplicated logic in tests.
* fix(s3api): ensure static config file takes precedence and refactor tests
- Track if configuration was loaded from a static file using `useStaticConfig`.
- Ignore filer-based IAM updates when a static configuration is in use to respect "Highest Priority" rule.
- Refactor `TestVerifyV4SignatureWithSTSIdentity` to use `VerifyActionPermission` directly.
- Fix typed nil interface panic in authorization test.
* Fix: Add delimiter support to ListObjectVersions with proper truncation
- Implemented delimiter support to group keys into CommonPrefixes
- Fixed critical truncation bug: now merges versions and common prefixes into single sorted list before truncation
- Ensures total items never exceed MaxKeys (prevents infinite pagination loops)
- Properly sets NextKeyMarker and NextVersionIdMarker for pagination
- Added integration tests in test/s3/versioning/s3_versioning_delimiter_test.go
- Verified behavior matches S3 API specification
* Fix: Add delimiter support to ListObjectVersions with proper truncation
- Implemented delimiter support to group keys into CommonPrefixes
- Fixed critical truncation bug: now merges versions and common prefixes before truncation
- Added safety guard for maxKeys=0 to prevent panics
- Condensed verbose comments for better readability
- Added robust Go integration tests with nil checks for AWS SDK pointers
- Verified behavior matches S3 API specification
- Resolved compilation error in integration tests
- Refined pagination comments and ensured exclusive KeyMarker behavior
- Refactored listObjectVersions into helper methods for better maintainability
* chore: execute goimports to format the code
Signed-off-by: promalert <promalert@outlook.com>
* goimports -w .
---------
Signed-off-by: promalert <promalert@outlook.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>
* Fix STS identity authorization by populating PolicyNames (#7985)
This commit fixes GitHub issue #7985 where STS-assumed identities
received empty identity.Actions, causing all S3 operations to be denied
even when the role had valid IAM policies attached.
Changes:
1. Populate PolicyNames field from sessionInfo.Policies in
validateSTSSessionToken() to enable IAM-based authorization for
STS identities
2. Fix bucket+objectKey path construction in canDo() method to include
proper slash separator between bucket and object key
3. Add comprehensive test suite to validate the fix and prevent
regression
The fix ensures that STS-assumed identities are properly authorized
through the IAM path when iamIntegration is available, allowing roles
with valid IAM policies to perform S3 operations as expected.
* Update STS identity tests to be more rigorous and use actual implementation path
* Fix regression in canDo() path concatenation
The previous fix blindly added a slash separator, which caused double
slashes when objectKey already started with a slash (common in existing
tests and some code paths). This broke TestCanDo and
TestObjectLevelListPermissions.
This commit updates the logic to only add the slash separator if
objectKey is not empty and does not already start with a slash.
This fixes the regressions while maintaining the fix for issue #7985.
* Refactor STS identity tests: extract helpers and simplify redundant logic
- Extracted setupTestSTSService and newTestIdentity helper functions
- Removed redundant if-else verification blocks that were already covered by assertions
- Cleaned up test cases to improve maintainability as suggested in code review.
* Add canDo() verification to STS identity tests
Address code review suggestion: verify that identities with empty
Actions correctly return false for canDo() checks, which confirms the
behavior that forces authorization to fall back to the IAM path.
* Simplify TestCanDoPathConstruction variable names
Rename expectedPath to fullPath and simplify logging/assertion logic
based on code review feedback.
* Refactor path construction and logging in canDo()
- Compute fullPath early and use it for logging to prevent double slashes
- Update TestCanDoPathConstruction to use robust path verification
- Add test case for objectKey with leading slash to ensure correct handling
* Implement Policy Attachment support for Object Store Users
- Added policy_names field to iam.proto and regenerated protos.
- Updated S3 API and IAM integration to support direct policy evaluation for users.
- Enhanced Admin UI to allow attaching policies to users via modals.
- Renamed 'policies' to 'policy_names' to clarify that it stores identifiers.
- Fixed syntax error in user_management.go.
* Fix policy dropdown not populating
The API returns {policies: [...]} but JavaScript was treating response as direct array.
Updated loadPolicies() to correctly access data.policies property.
* Add null safety checks for policy dropdowns
Added checks to prevent "undefined" errors when:
- Policy select elements don't exist
- Policy dropdowns haven't loaded yet
- User is being edited before policies are loaded
* Fix policy dropdown by using correct JSON field name
JSON response has lowercase 'name' field but JavaScript was accessing 'Name'.
Changed policy.Name to policy.name to match the IAMPolicy JSON structure.
* Fix policy names not being saved on user update
Changed condition from len(req.PolicyNames) > 0 to req.PolicyNames != nil
to ensure policy names are always updated when present in the request,
even if it's an empty array (to allow clearing policies).
* Add debug logging for policy names update flow
Added console.log in frontend and glog in backend to trace
policy_names data through the update process.
* Temporarily disable auto-reload for debugging
Commented out window.location.reload() so console logs are visible
when updating a user.
* Add detailed debug logging and alert for policy selection
Added console.log for each step and an alert to show policy_names value
to help diagnose why it's not being included in the request.
* Regenerate templ files for object_store_users
Ran templ generate to ensure _templ.go files are up to date with
the latest .templ changes including debug logging.
* Remove debug logging and restore normal functionality
Cleaned up temporary debug code (console.log and alert statements)
and re-enabled automatic page reload after user update.
* Add step-by-step alert debugging for policy update
Added 5 alert checkpoints to trace policy data through the update flow:
1. Check if policiesSelect element exists
2. Show selected policy values
3. Show userData.policy_names
4. Show full request body
5. Confirm server response
Temporarily disabled auto-reload to see alerts.
* Add version check alert on page load
Added alert on DOMContentLoaded to verify new JavaScript is being executed
and not cached by the browser.
* Compile templates using make
Ran make to compile all template files and install the weed binary.
* Add button click detection and make handleUpdateUser global
- Added inline alert on button click to verify click is detected
- Made handleUpdateUser a window-level function to ensure it's accessible
- Added alert at start of handleUpdateUser function
* Fix handleUpdateUser scope issue - remove duplicate definition
Removed duplicate function definition that was inside DOMContentLoaded.
Now handleUpdateUser is defined only once in global scope (line 383)
making it accessible when button onclick fires.
* Remove all duplicate handleUpdateUser definitions
Now handleUpdateUser is defined only once at the very top of the script
block (line 352), before DOMContentLoaded, ensuring it's available when
the button onclick fires.
* Add function existence check and error catching
Added alerts to check if handleUpdateUser is defined and wrapped
the function call in try-catch to capture any JavaScript errors.
Also added console.log statements to verify function definition.
* Simplify handleUpdateUser to non-async for testing
Removed async/await and added early return to test if function
can be called at all. This will help identify if async is causing
the issue.
* Add cache-control headers to prevent browser caching
Added no-cache headers to ShowObjectStoreUsers handler to prevent
aggressive browser caching of inline JavaScript in the HTML page.
* Fix syntax error - make handleUpdateUser async
Changed function back to async to fix 'await is only valid in async functions' error.
The cache-control headers are working - browser is now loading new code.
* Update version check to v3 to verify cache busting
Changed version alert to 'v3 - WITH EARLY RETURN' to confirm
the new code with early return statement is being loaded.
* Remove all debug code - clean implementation
Removed all alerts, console.logs, and test code.
Implemented clean policy update functionality with proper error handling.
* Add ETag header for cache-busting and update walkthrough
* Fix policy pre-selection in Edit User modal
- Updated admin.js editUser function to pre-select policies
- Root cause: duplicate editUser in admin.js overwrote inline version
- Added policy pre-selection logic to match inline template
- Verified working in browser: policies now pre-select correctly
* Fix policy persistence in handleUpdateUser
- Added policy_names field to userData payload in handleUpdateUser
- Policies were being lost because handleUpdateUser only sent email and actions
- Now collects selected policies from editPolicies dropdown
- Verified working: policies persist correctly across updates
* Fix XSS vulnerability in access keys display
- Escape HTML in access key display using escapeHtml utility
- Replace inline onclick handlers with data attributes
- Add event delegation for delete access key buttons
- Prevents script injection via malicious access key values
* Fix additional XSS vulnerabilities in user details display
- Escape HTML in actions badges (line 626)
- Escape HTML in policy_names badges (line 636)
- Prevents script injection via malicious action or policy names
* Fix XSS vulnerability in loadPolicies function
- Replace innerHTML string concatenation with DOM API
- Use createElement and textContent for safe policy name insertion
- Prevents script injection via malicious policy names
- Apply same pattern to both create and edit select elements
* Remove debug logging from UpdateObjectStoreUser
- Removed glog.V(0) debug statements
- Clean up temporary debugging code before production
* Remove duplicate handleUpdateUser function
- Removed inline handleUpdateUser that duplicated admin.js logic
- Removed debug console.log statement
- admin.js version is now the single source of truth
- Eliminates maintenance burden of keeping two versions in sync
* Refine user management and address code review feedback
- Preserve PolicyNames in UpdateUserPolicies
- Allow clearing actions in UpdateObjectStoreUser by checking for nil
- Remove version comment from object_store_users.templ
- Refactor loadPolicies for DRYness using cloneNode while keeping DOM API security
* IAM Authorization for Static Access Keys
* verified XSS Fixes in Templates
* fix div
* Enable writeback_cache and async_dio FUSE options
Fixes#7978
- Update mount_std.go to use EnableWriteback and EnableAsyncDio from go-fuse
- Add go.mod replace directive to use local go-fuse with capability support
- Remove temporary workaround that disabled these options
This enables proper FUSE kernel capability negotiation for writeback cache
and async direct I/O, improving performance for small writes and concurrent
direct I/O operations.
* Address PR review comments
- Remove redundant nil checks for writebackCache and asyncDio flags
- Update go.mod replace directive to use seaweedfs/go-fuse fork instead of local path
* Add TODO comment for go.mod replace directive
The replace directive must use a local path until seaweedfs/go-fuse#1 is merged.
After merge, this should be updated to use the proper version.
* Use seaweedfs/go-fuse v2.9.0 instead of local repository
Replace local path with seaweedfs/go-fuse v2.9.0 fork which includes
the writeback_cache and async_dio capability support.
* Use github.com/seaweedfs/go-fuse/v2 directly without replace directive
- Updated all imports to use github.com/seaweedfs/go-fuse/v2
- Removed replace directive from go.mod
- Using seaweedfs/go-fuse v2.0.0-20260106181308-87f90219ce09 which includes:
* writeback_cache and async_dio support
* Corrected module path
* Update to seaweedfs/go-fuse v2.9.1
Use v2.9.1 tag which includes the corrected module path
(github.com/seaweedfs/go-fuse/v2) along with writeback_cache
and async_dio support.
* opt: reduce ShardsInfo memory usage with bitmap and sorted slice
- Replace map[ShardId]*ShardInfo with sorted []ShardInfo slice
- Add ShardBits (uint32) bitmap for O(1) existence checks
- Use binary search for O(log n) lookups by shard ID
- Maintain sorted order for efficient iteration
- Add comprehensive unit tests and benchmarks
Memory savings:
- Map overhead: ~48 bytes per entry eliminated
- Pointers: 8 bytes per entry eliminated
- Total: ~56 bytes per shard saved
Performance improvements:
- Has(): O(1) using bitmap
- Size(): O(log n) using binary search (was O(1), acceptable tradeoff)
- Count(): O(1) using popcount on bitmap
- Iteration: Faster due to cache locality
* refactor: add methods to ShardBits type
- Add Has(), Set(), Clear(), and Count() methods to ShardBits
- Simplify ShardsInfo methods by using ShardBits methods
- Improves code readability and encapsulation
* opt: use ShardBits directly in ShardsCountFromVolumeEcShardInformationMessage
Avoid creating a full ShardsInfo object just to count shards.
Directly cast vi.EcIndexBits to ShardBits and use Count() method.
* opt: use strings.Builder in ShardsInfo.String() for efficiency
* refactor: change AsSlice to return []ShardInfo (values instead of pointers)
This completes the memory optimization by avoiding unnecessary pointer slices and potential allocations.
* refactor: rename ShardsCountFromVolumeEcShardInformationMessage to GetShardCount
* fix: prevent deadlock in Add and Subtract methods
Copy shards data from 'other' before releasing its lock to avoid
potential deadlock when a.Add(b) and b.Add(a) are called concurrently.
The previous implementation held other's lock while calling si.Set/Delete,
which acquires si's lock. This could deadlock if two goroutines tried to
add/subtract each other concurrently.
* opt: avoid unnecessary locking in constructor functions
ShardsInfoFromVolume and ShardsInfoFromVolumeEcShardInformationMessage
now build shards slice and bitmap directly without calling Set(), which
acquires a lock on every call. Since the object is local and not yet
shared, locking is unnecessary and adds overhead.
This improves performance during object construction.
* fix: rename 'copy' variable to avoid shadowing built-in function
The variable name 'copy' in TestShardsInfo_Copy shadowed the built-in
copy() function, which is confusing and bad practice. Renamed to 'siCopy'.
* opt: use math/bits.OnesCount32 and reorganize types
1. Replace manual popcount loop with math/bits.OnesCount32 for better
performance and idiomatic Go code
2. Move ShardSize type definition to ec_shards_info.go for better code
organization since it's primarily used there
* refactor: Set() now accepts ShardInfo for future extensibility
Changed Set(id ShardId, size ShardSize) to Set(shard ShardInfo) to
support future additions to ShardInfo without changing the API.
This makes the code more extensible as new fields can be added to
ShardInfo (e.g., checksum, location, etc.) without breaking the Set API.
* refactor: move ShardInfo and ShardSize to separate file
Created ec_shard_info.go to hold the basic shard types (ShardInfo and
ShardSize) for better code organization and separation of concerns.
* refactor: add ShardInfo constructor and helper functions
Added NewShardInfo() constructor and IsValid() method to better
encapsulate ShardInfo creation and validation. Updated code to use
the constructor for cleaner, more maintainable code.
* fix: update remaining Set() calls to use NewShardInfo constructor
Fixed compilation errors in storage and shell packages where Set() calls
were not updated to use the new NewShardInfo() constructor.
* fix: remove unreachable code in filer backup commands
Removed unreachable return statements after infinite loops in
filer_backup.go and filer_meta_backup.go to fix compilation errors.
* fix: rename 'new' variable to avoid shadowing built-in
Renamed 'new' to 'result' in MinusParityShards, Plus, and Minus methods
to avoid shadowing Go's built-in new() function.
* fix: update remaining test files to use NewShardInfo constructor
Fixed Set() calls in command_volume_list_test.go and
ec_rebalance_slots_test.go to use NewShardInfo() constructor.
Chris Lu
Chris Lu
Sheya Bernstein
Nicholas Boyd Isacsson
promalert