From e5f72077ee6a5c5299f28c73a05f9477b7b54540 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 23 Mar 2026 19:33:20 -0700 Subject: [PATCH] fix: resolve CORS cache race condition causing stale 404 responses (#8748) The metadata subscription handler (updateBucketConfigCacheFromEntry) was making a separate RPC call via loadCORSFromBucketContent to load CORS configuration. This created a race window where a slow CreateBucket subscription event could re-cache stale data after PutBucketCors had already cleared the cache, causing subsequent GetBucketCors to return 404 NoSuchCORSConfiguration. Parse CORS directly from the subscription entry's Content field instead of making a separate RPC. Also fix getBucketConfig to parse CORS from the already-fetched entry, eliminating a redundant RPC call. Fix TestCORSCaching to use require.NoError to prevent nil pointer dereference panics when GetBucketCors fails. --- test/s3/cors/s3_cors_test.go | 18 ++++-------- weed/s3api/auth_credentials_subscribe.go | 16 +++++------ weed/s3api/s3api_bucket_config.go | 35 ++++++++++-------------- 3 files changed, 28 insertions(+), 41 deletions(-) diff --git a/test/s3/cors/s3_cors_test.go b/test/s3/cors/s3_cors_test.go index 18a113d99..0155254e1 100644 --- a/test/s3/cors/s3_cors_test.go +++ b/test/s3/cors/s3_cors_test.go @@ -590,17 +590,14 @@ func TestCORSCaching(t *testing.T) { Bucket: aws.String(bucketName), CORSConfiguration: corsConfig1, }) - assert.NoError(t, err, "Should be able to put initial CORS configuration") - - // Wait for metadata subscription to update cache - time.Sleep(50 * time.Millisecond) + require.NoError(t, err, "Should be able to put initial CORS configuration") // Get the configuration getResp1, err := client.GetBucketCors(context.TODO(), &s3.GetBucketCorsInput{ Bucket: aws.String(bucketName), }) - assert.NoError(t, err, "Should be able to get initial CORS configuration") - assert.Len(t, getResp1.CORSRules, 1, "Should have one CORS rule") + require.NoError(t, err, "Should be able to get initial CORS configuration") + require.Len(t, getResp1.CORSRules, 1, "Should have one CORS rule") // Update the configuration corsConfig2 := &types.CORSConfiguration{ @@ -618,17 +615,14 @@ func TestCORSCaching(t *testing.T) { Bucket: aws.String(bucketName), CORSConfiguration: corsConfig2, }) - assert.NoError(t, err, "Should be able to update CORS configuration") - - // Wait for metadata subscription to update cache - time.Sleep(50 * time.Millisecond) + require.NoError(t, err, "Should be able to update CORS configuration") // Get the updated configuration (should reflect the changes) getResp2, err := client.GetBucketCors(context.TODO(), &s3.GetBucketCorsInput{ Bucket: aws.String(bucketName), }) - assert.NoError(t, err, "Should be able to get updated CORS configuration") - assert.Len(t, getResp2.CORSRules, 1, "Should have one CORS rule") + require.NoError(t, err, "Should be able to get updated CORS configuration") + require.Len(t, getResp2.CORSRules, 1, "Should have one CORS rule") rule := getResp2.CORSRules[0] assert.Equal(t, []string{"Content-Type"}, rule.AllowedHeaders, "Should have updated headers") diff --git a/weed/s3api/auth_credentials_subscribe.go b/weed/s3api/auth_credentials_subscribe.go index e0eb15554..60c143d3a 100644 --- a/weed/s3api/auth_credentials_subscribe.go +++ b/weed/s3api/auth_credentials_subscribe.go @@ -1,7 +1,6 @@ package s3api import ( - "errors" "strings" "time" @@ -224,14 +223,13 @@ func (s3a *S3ApiServer) updateBucketConfigCacheFromEntry(entry *filer_pb.Entry) // Sync bucket policy to the policy engine for evaluation s3a.syncBucketPolicyToEngine(bucket, config.BucketPolicy) - // Load CORS configuration from bucket directory content - if corsConfig, err := s3a.loadCORSFromBucketContent(bucket); err != nil { - if !errors.Is(err, filer_pb.ErrNotFound) { - glog.Errorf("updateBucketConfigCacheFromEntry: failed to load CORS configuration for bucket %s: %v", bucket, err) - } - } else { - config.CORS = corsConfig - glog.V(2).Infof("updateBucketConfigCacheFromEntry: loaded CORS config for bucket %s", bucket) + // Parse CORS configuration directly from the subscription entry's Content field. + // This avoids a separate RPC call that could return stale data when racing with + // concurrent metadata updates (e.g., PutBucketCors clearing the cache while this + // handler is still processing an older event). + config.CORS = parseCORSFromEntryContent(entry.Content) + if config.CORS != nil { + glog.V(2).Infof("updateBucketConfigCacheFromEntry: parsed CORS config for bucket %s from entry content", bucket) } // Update timestamp diff --git a/weed/s3api/s3api_bucket_config.go b/weed/s3api/s3api_bucket_config.go index aad6d03f3..185c1adb7 100644 --- a/weed/s3api/s3api_bucket_config.go +++ b/weed/s3api/s3api_bucket_config.go @@ -408,18 +408,9 @@ func (s3a *S3ApiServer) getBucketConfig(bucket string) (*BucketConfig, s3err.Err // Sync bucket policy to the policy engine for evaluation s3a.syncBucketPolicyToEngine(bucket, config.BucketPolicy) - // Load CORS configuration from bucket directory content - if corsConfig, err := s3a.loadCORSFromBucketContent(bucket); err != nil { - if errors.Is(err, filer_pb.ErrNotFound) { - // Missing metadata is not an error; fall back cleanly - glog.V(2).Infof("CORS metadata not found for bucket %s, falling back to default behavior", bucket) - } else { - // Log parsing or validation errors - glog.Errorf("Failed to load CORS configuration for bucket %s: %v", bucket, err) - } - } else { - config.CORS = corsConfig - } + // Parse CORS configuration directly from the entry's Content field. + // This avoids a redundant RPC call since we already have the entry. + config.CORS = parseCORSFromEntryContent(entry.Content) // Cache the result s3a.bucketConfigCache.Set(bucket, config) @@ -588,15 +579,19 @@ func (s3a *S3ApiServer) setBucketOwnership(bucket, ownership string) s3err.Error }) } -// loadCORSFromBucketContent loads CORS configuration from bucket directory content -func (s3a *S3ApiServer) loadCORSFromBucketContent(bucket string) (*cors.CORSConfiguration, error) { - metadata, err := s3a.GetBucketMetadata(bucket) - if err != nil { - return nil, err +// parseCORSFromEntryContent parses CORS configuration directly from an entry's Content field. +// This avoids a separate RPC call when the entry is already available (e.g., from a +// subscription event or a prior getBucketEntry call). +func parseCORSFromEntryContent(content []byte) *cors.CORSConfiguration { + if len(content) == 0 { + return nil } - - // Note: corsConfig can be nil if no CORS configuration is set, which is valid - return metadata.CORS, nil + var protoMetadata s3_pb.BucketMetadata + if err := proto.Unmarshal(content, &protoMetadata); err != nil { + glog.Errorf("parseCORSFromEntryContent: failed to unmarshal protobuf metadata: %v", err) + return nil + } + return corsConfigFromProto(protoMetadata.Cors) } // getCORSConfiguration retrieves CORS configuration with caching