Browse Source

Fix hardcoded http scheme and add panic recovery

Address CodeRabbit review #3512114811:

1. **Major: Fix hardcoded http:// scheme in Location URLs**
   - Location URLs always used http:// regardless of client connection
   - HTTPS clients got http:// URLs (incorrect)
   - Fixed: Detect scheme from request
   - Check X-Forwarded-Proto header (for proxies) first
   - Check r.TLS != nil for direct HTTPS
   - Fallback to http for plain connections
   - Applied to all 4 CompleteMultipartUploadResult locations

2. **Major: Add panic recovery to discovery goroutine**
   - Long-running background goroutine could crash entire process
   - Panic in refreshFilerList would terminate program
   - Fixed: Add defer recover() with error logging
   - Goroutine failures now logged, not fatal

3. **Note: Close() idempotency already implemented**
   - Review flagged as duplicate issue
   - Already fixed in commit 3d7a65c7e
   - sync.Once (closeDiscoveryOnce) prevents double-close panic
   - Safe to call Close() multiple times

Changes:
- filer_multipart.go:
  - Add getRequestScheme() helper function
  - Update all 4 Location URLs to use dynamic scheme
  - Format: scheme://host/bucket/key (was: http://...)

- filer_client.go:
  - Add panic recovery to discoverFilers()
  - Log panics instead of crashing

Benefits:
- Correct scheme (https/http) in Location URLs
- Works behind proxies (X-Forwarded-Proto)
- No process crashes from discovery failures
- Production-hardened background goroutine
- Proper AWS S3 API compliance
pull/7550/head
Chris Lu 1 week ago
parent
commit
4070d62367
  1. 24
      weed/s3api/filer_multipart.go
  2. 6
      weed/wdclient/filer_client.go

24
weed/s3api/filer_multipart.go

@ -43,6 +43,20 @@ type InitiateMultipartUploadResult struct {
s3.CreateMultipartUploadOutput
}
// getRequestScheme determines the URL scheme (http or https) from the request
// Checks X-Forwarded-Proto header first (for proxies), then TLS state
func getRequestScheme(r *http.Request) string {
// Check X-Forwarded-Proto header for proxied requests
if proto := r.Header.Get("X-Forwarded-Proto"); proto != "" {
return proto
}
// Check if connection is TLS
if r.TLS != nil {
return "https"
}
return "http"
}
func (s3a *S3ApiServer) createMultipartUpload(r *http.Request, input *s3.CreateMultipartUploadInput) (output *InitiateMultipartUploadResult, code s3err.ErrorCode) {
glog.V(2).Infof("createMultipartUpload input %v", input)
@ -185,9 +199,9 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
if entry, _ := s3a.getEntry(dirName, entryName); entry != nil && entry.Extended != nil {
if uploadId, ok := entry.Extended[s3_constants.SeaweedFSUploadId]; ok && *input.UploadId == string(uploadId) {
// Location uses the S3 endpoint that the client connected to
// Format: http://s3-endpoint/bucket/object (following AWS S3 API)
// Format: scheme://s3-endpoint/bucket/object (following AWS S3 API)
return &CompleteMultipartUploadResult{
Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Bucket: input.Bucket,
ETag: aws.String("\"" + filer.ETagChunks(entry.GetChunks()) + "\""),
Key: objectKey(input.Key),
@ -404,7 +418,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
// The latest version information is tracked in the .versions directory metadata
output = &CompleteMultipartUploadResult{
Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Bucket: input.Bucket,
ETag: aws.String("\"" + filer.ETagChunks(finalParts) + "\""),
Key: objectKey(input.Key),
@ -457,7 +471,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
// Note: Suspended versioning should NOT return VersionId field according to AWS S3 spec
output = &CompleteMultipartUploadResult{
Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Bucket: input.Bucket,
ETag: aws.String("\"" + filer.ETagChunks(finalParts) + "\""),
Key: objectKey(input.Key),
@ -514,7 +528,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
// For non-versioned buckets, return response without VersionId
output = &CompleteMultipartUploadResult{
Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
Bucket: input.Bucket,
ETag: aws.String("\"" + filer.ETagChunks(finalParts) + "\""),
Key: objectKey(input.Key),

6
weed/wdclient/filer_client.go

@ -218,6 +218,12 @@ func (fc *FilerClient) Close() {
// discoverFilers periodically queries the master to discover filers in the same group
// and updates the filer list. This runs in a background goroutine.
func (fc *FilerClient) discoverFilers() {
defer func() {
if r := recover(); r != nil {
glog.Errorf("FilerClient: panic in filer discovery goroutine for group '%s': %v", fc.filerGroup, r)
}
}()
// Do an initial discovery
fc.refreshFilerList()

Loading…
Cancel
Save