mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-08-19 05:36:58 +00:00
address comments
This commit is contained in:
chrislu
@@ -104,9 +104,9 @@ func TestS3IAMDistributedTests(t *testing.T) {
|
||||
|
||||
t.Run("distributed_concurrent_operations", func(t *testing.T) {
|
||||
// Test concurrent operations across distributed instances
|
||||
// Reduced concurrency (3x2=6 total ops) for CI stability
|
||||
const numGoroutines = 3
|
||||
const numOperationsPerGoroutine = 2
|
||||
// Increased concurrency to effectively detect race conditions and system stability
|
||||
const numGoroutines = 10 // More goroutines to stress test concurrency
|
||||
const numOperationsPerGoroutine = 5 // More operations per goroutine for better coverage
|
||||
|
||||
var wg sync.WaitGroup
|
||||
errors := make(chan error, numGoroutines*numOperationsPerGoroutine)
|
||||
@@ -215,25 +215,31 @@ func TestS3IAMDistributedTests(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// STRICT CONCURRENCY TESTING: Use appropriate thresholds for the operation count
|
||||
// For totalOperations=6, error thresholds need to be adjusted to avoid unreachable conditions
|
||||
|
||||
// RIGOROUS CONCURRENCY TESTING: Use strict thresholds for effective race condition detection
|
||||
// For totalOperations=50, we can set much more realistic and strict error thresholds
|
||||
|
||||
// Serious errors (race conditions, deadlocks) should be zero for reliable testing
|
||||
maxSeriousErrors := 0 // Allow 0 serious errors max - any serious error indicates system issues
|
||||
maxSeriousErrors := 0 // Zero tolerance for serious errors - any indicates system issues
|
||||
if len(seriousErrors) > maxSeriousErrors {
|
||||
t.Errorf("❌ %d serious error(s) detected, indicating potential concurrency bugs. First error: %v",
|
||||
len(seriousErrors), seriousErrors[0])
|
||||
}
|
||||
|
||||
// For total errors, allow more flexibility with small operation counts
|
||||
maxTotalErrors := 2 // Allow 2 total errors max (33.3%) for small test size
|
||||
if len(errorList) > maxTotalErrors {
|
||||
t.Errorf("❌ Too many total errors: %d (%.1f%%) - max allowed: %d. This suggests system instability under concurrent load.",
|
||||
len(errorList), errorRate*100, maxTotalErrors)
|
||||
|
||||
// For total errors, use strict thresholds that can actually detect system instability
|
||||
maxTotalErrorsStrict := 2 // Allow max 2 total errors (4% rate) - very strict
|
||||
maxTotalErrorsRelaxed := 5 // Allow max 5 total errors (10% rate) - fallback for CI environments
|
||||
|
||||
if len(errorList) > maxTotalErrorsRelaxed {
|
||||
t.Errorf("❌ Too many total errors: %d (%.1f%%) - exceeds relaxed threshold of %d (%.1f%%). System is unstable under concurrent load.",
|
||||
len(errorList), errorRate*100, maxTotalErrorsRelaxed, float64(maxTotalErrorsRelaxed)/float64(totalOperations)*100)
|
||||
} else if len(errorList) > maxTotalErrorsStrict {
|
||||
t.Logf("⚠️ Concurrent operations completed with %d errors (%.1f%%) - exceeds strict threshold but within relaxed limits. Consider investigating.",
|
||||
len(errorList), errorRate*100)
|
||||
} else if len(errorList) > 0 {
|
||||
t.Logf("⚠️ Concurrent operations completed with %d errors (%.1f%%) - within acceptable range for testing", len(errorList), errorRate*100)
|
||||
t.Logf("✅ Concurrent operations completed with %d errors (%.1f%%) - within strict thresholds, excellent stability!",
|
||||
len(errorList), errorRate*100)
|
||||
} else {
|
||||
t.Logf("✅ All concurrent operations completed successfully - excellent concurrency handling!")
|
||||
t.Logf("🎉 All %d concurrent operations completed successfully - perfect concurrency handling!", totalOperations)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -301,12 +301,12 @@ func (e *PolicyEngine) Evaluate(ctx context.Context, filerAddress string, evalCt
|
||||
// statementMatches checks if a statement matches the evaluation context
|
||||
func (e *PolicyEngine) statementMatches(statement *Statement, evalCtx *EvaluationContext) bool {
|
||||
// Check action match
|
||||
if !e.matchesActions(statement.Action, evalCtx.Action) {
|
||||
if !e.matchesActions(statement.Action, evalCtx.Action, evalCtx) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Check resource match
|
||||
if !e.matchesResources(statement.Resource, evalCtx.Resource) {
|
||||
if !e.matchesResources(statement.Resource, evalCtx.Resource, evalCtx) {
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -319,9 +319,9 @@ func (e *PolicyEngine) statementMatches(statement *Statement, evalCtx *Evaluatio
|
||||
}
|
||||
|
||||
// matchesActions checks if any action in the list matches the requested action
|
||||
func (e *PolicyEngine) matchesActions(actions []string, requestedAction string) bool {
|
||||
func (e *PolicyEngine) matchesActions(actions []string, requestedAction string, evalCtx *EvaluationContext) bool {
|
||||
for _, action := range actions {
|
||||
if matchAction(action, requestedAction) {
|
||||
if awsIAMMatch(action, requestedAction, evalCtx) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
@@ -329,9 +329,9 @@ func (e *PolicyEngine) matchesActions(actions []string, requestedAction string)
|
||||
}
|
||||
|
||||
// matchesResources checks if any resource in the list matches the requested resource
|
||||
func (e *PolicyEngine) matchesResources(resources []string, requestedResource string) bool {
|
||||
func (e *PolicyEngine) matchesResources(resources []string, requestedResource string, evalCtx *EvaluationContext) bool {
|
||||
for _, resource := range resources {
|
||||
if matchResource(resource, requestedResource) {
|
||||
if awsIAMMatch(resource, requestedResource, evalCtx) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,191 @@
|
||||
package policy
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// TestPolicyVariableMatchingInActionsAndResources tests that Actions and Resources
|
||||
// now support policy variables like ${aws:username} just like string conditions do
|
||||
func TestPolicyVariableMatchingInActionsAndResources(t *testing.T) {
|
||||
engine := NewPolicyEngine()
|
||||
config := &PolicyEngineConfig{
|
||||
DefaultEffect: "Deny",
|
||||
StoreType: "memory",
|
||||
}
|
||||
|
||||
err := engine.Initialize(config)
|
||||
require.NoError(t, err)
|
||||
|
||||
ctx := context.Background()
|
||||
filerAddress := ""
|
||||
|
||||
// Create a policy that uses policy variables in Action and Resource fields
|
||||
policyDoc := &PolicyDocument{
|
||||
Version: "2012-10-17",
|
||||
Statement: []Statement{
|
||||
{
|
||||
Sid: "AllowUserSpecificActions",
|
||||
Effect: "Allow",
|
||||
Action: []string{
|
||||
"s3:Get*", // Regular wildcard
|
||||
"s3:${aws:principaltype}*", // Policy variable in action
|
||||
},
|
||||
Resource: []string{
|
||||
"arn:aws:s3:::user-${aws:username}/*", // Policy variable in resource
|
||||
"arn:aws:s3:::shared/${saml:username}/*", // Different policy variable
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
err = engine.AddPolicy(filerAddress, "user-specific-policy", policyDoc)
|
||||
require.NoError(t, err)
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
principal string
|
||||
action string
|
||||
resource string
|
||||
requestContext map[string]interface{}
|
||||
expectedEffect Effect
|
||||
description string
|
||||
}{
|
||||
{
|
||||
name: "policy_variable_in_action_matches",
|
||||
principal: "test-user",
|
||||
action: "s3:AssumedRole", // Should match s3:${aws:principaltype}* when principaltype=AssumedRole
|
||||
resource: "arn:aws:s3:::user-testuser/file.txt",
|
||||
requestContext: map[string]interface{}{
|
||||
"aws:username": "testuser",
|
||||
"aws:principaltype": "AssumedRole",
|
||||
},
|
||||
expectedEffect: EffectAllow,
|
||||
description: "Action with policy variable should match when variable is expanded",
|
||||
},
|
||||
{
|
||||
name: "policy_variable_in_resource_matches",
|
||||
principal: "alice",
|
||||
action: "s3:GetObject",
|
||||
resource: "arn:aws:s3:::user-alice/document.pdf", // Should match user-${aws:username}/*
|
||||
requestContext: map[string]interface{}{
|
||||
"aws:username": "alice",
|
||||
},
|
||||
expectedEffect: EffectAllow,
|
||||
description: "Resource with policy variable should match when variable is expanded",
|
||||
},
|
||||
{
|
||||
name: "saml_username_variable_in_resource",
|
||||
principal: "bob",
|
||||
action: "s3:GetObject",
|
||||
resource: "arn:aws:s3:::shared/bob/data.json", // Should match shared/${saml:username}/*
|
||||
requestContext: map[string]interface{}{
|
||||
"saml:username": "bob",
|
||||
},
|
||||
expectedEffect: EffectAllow,
|
||||
description: "SAML username variable should be expanded in resource patterns",
|
||||
},
|
||||
{
|
||||
name: "policy_variable_no_match_wrong_user",
|
||||
principal: "charlie",
|
||||
action: "s3:GetObject",
|
||||
resource: "arn:aws:s3:::user-alice/file.txt", // charlie trying to access alice's files
|
||||
requestContext: map[string]interface{}{
|
||||
"aws:username": "charlie",
|
||||
},
|
||||
expectedEffect: EffectDeny,
|
||||
description: "Policy variable should prevent access when username doesn't match",
|
||||
},
|
||||
{
|
||||
name: "missing_policy_variable_context",
|
||||
principal: "dave",
|
||||
action: "s3:GetObject",
|
||||
resource: "arn:aws:s3:::user-dave/file.txt",
|
||||
requestContext: map[string]interface{}{
|
||||
// Missing aws:username context
|
||||
},
|
||||
expectedEffect: EffectDeny,
|
||||
description: "Missing policy variable context should result in no match",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
evalCtx := &EvaluationContext{
|
||||
Principal: tt.principal,
|
||||
Action: tt.action,
|
||||
Resource: tt.resource,
|
||||
RequestContext: tt.requestContext,
|
||||
}
|
||||
|
||||
result, err := engine.Evaluate(ctx, filerAddress, evalCtx, []string{"user-specific-policy"})
|
||||
require.NoError(t, err, "Policy evaluation should not error")
|
||||
|
||||
assert.Equal(t, tt.expectedEffect, result.Effect,
|
||||
"Test %s: %s. Expected %s but got %s",
|
||||
tt.name, tt.description, tt.expectedEffect, result.Effect)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestActionResourceConsistencyWithStringConditions verifies that Actions, Resources,
|
||||
// and string conditions all use the same AWS IAM-compliant matching logic
|
||||
func TestActionResourceConsistencyWithStringConditions(t *testing.T) {
|
||||
engine := NewPolicyEngine()
|
||||
config := &PolicyEngineConfig{
|
||||
DefaultEffect: "Deny",
|
||||
StoreType: "memory",
|
||||
}
|
||||
|
||||
err := engine.Initialize(config)
|
||||
require.NoError(t, err)
|
||||
|
||||
ctx := context.Background()
|
||||
filerAddress := ""
|
||||
|
||||
// Policy that uses case-insensitive matching in all three areas
|
||||
policyDoc := &PolicyDocument{
|
||||
Version: "2012-10-17",
|
||||
Statement: []Statement{
|
||||
{
|
||||
Sid: "CaseInsensitiveMatching",
|
||||
Effect: "Allow",
|
||||
Action: []string{"S3:GET*"}, // Uppercase action pattern
|
||||
Resource: []string{"arn:aws:s3:::TEST-BUCKET/*"}, // Uppercase resource pattern
|
||||
Condition: map[string]map[string]interface{}{
|
||||
"StringLike": {
|
||||
"s3:RequestedRegion": "US-*", // Uppercase condition pattern
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
err = engine.AddPolicy(filerAddress, "case-insensitive-policy", policyDoc)
|
||||
require.NoError(t, err)
|
||||
|
||||
evalCtx := &EvaluationContext{
|
||||
Principal: "test-user",
|
||||
Action: "s3:getobject", // lowercase action
|
||||
Resource: "arn:aws:s3:::test-bucket/file.txt", // lowercase resource
|
||||
RequestContext: map[string]interface{}{
|
||||
"s3:RequestedRegion": "us-east-1", // lowercase condition value
|
||||
},
|
||||
}
|
||||
|
||||
result, err := engine.Evaluate(ctx, filerAddress, evalCtx, []string{"case-insensitive-policy"})
|
||||
require.NoError(t, err)
|
||||
|
||||
// All should match due to case-insensitive AWS IAM-compliant matching
|
||||
assert.Equal(t, EffectAllow, result.Effect,
|
||||
"Actions, Resources, and Conditions should all use case-insensitive AWS IAM matching")
|
||||
|
||||
// Verify that matching statements were found
|
||||
assert.Len(t, result.MatchingStatements, 1,
|
||||
"Should have exactly one matching statement")
|
||||
assert.Equal(t, "Allow", string(result.MatchingStatements[0].Effect),
|
||||
"Matching statement should have Allow effect")
|
||||
}
|
||||
Reference in New Issue
Block a user