-
Notifications
You must be signed in to change notification settings - Fork 533
implement ContentServerDataStore for azure blob storage too #1234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
607aafc
c0f70f3
e7d6c40
922e53b
874f79e
7607b93
5a7b497
c089bc2
25280a5
ba65513
e5dbeb1
ea551c2
1825452
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,9 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "sort" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/Azure/azure-sdk-for-go/sdk/azcore" | ||
|
|
@@ -68,6 +70,8 @@ type AzBlob interface { | |
| Upload(ctx context.Context, body io.ReadSeeker) error | ||
| // Download returns a readcloser to download the contents of the blob | ||
| Download(ctx context.Context) (io.ReadCloser, error) | ||
| // Serves the contents of the blob directly handling special HTTP headers like Range, if set | ||
| ServeContent(ctx context.Context, w http.ResponseWriter, r *http.Request) error | ||
| // Get the offset of the blob and its indexes | ||
| GetOffset(ctx context.Context) (int64, error) | ||
| // Commit the uploaded blocks to the BlockBlob | ||
|
|
@@ -199,6 +203,64 @@ func (blockBlob *BlockBlob) Download(ctx context.Context) (io.ReadCloser, error) | |
| return resp.Body, nil | ||
| } | ||
|
|
||
| // Serve content respecting range header | ||
| func (blockBlob *BlockBlob) ServeContent(ctx context.Context, w http.ResponseWriter, r *http.Request) error { | ||
| var downloadOptions, err = ParseDownloadOptions(r) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| result, err := blockBlob.BlobClient.DownloadStream(ctx, downloadOptions) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer result.Body.Close() | ||
|
|
||
| statusCode := http.StatusOK | ||
| if result.ContentRange != nil { | ||
| // Use 206 Partial Content for range requests | ||
| statusCode = http.StatusPartialContent | ||
| } else if result.ContentLength != nil && *result.ContentLength == 0 { | ||
| statusCode = http.StatusNoContent | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When ETag is matched the response should be } else if result.ContentLength != nil && *result.ContentLength == 0 {
if result.ErrorCode != nil && *result.ErrorCode == string(bloberror.ConditionNotMet) &&
downloadOptions != nil && downloadOptions.AccessConditions != nil &&
downloadOptions.AccessConditions.ModifiedAccessConditions != nil && downloadOptions.AccessConditions.ModifiedAccessConditions.IfNoneMatch != nil {
// If the client sent an If-None-Match header and we get an X-Ms-Error-Code "ConditionNotMet", return 304 Not Modified
statusCode = http.StatusNotModified
} else {
statusCode = http.StatusNoContent
}
} |
||
| } | ||
|
|
||
| // Add Accept-Ranges,Content-*, Cache-Control, ETag, Expires, Last-Modified headers if present in azure response | ||
| if result.AcceptRanges != nil { | ||
| w.Header().Set("Accept-Ranges", *result.AcceptRanges) | ||
| } | ||
| if result.ContentDisposition != nil { | ||
| w.Header().Set("Content-Disposition", *result.ContentDisposition) | ||
| } | ||
| if result.ContentEncoding != nil { | ||
| w.Header().Set("Content-Encoding", *result.ContentEncoding) | ||
| } | ||
| if result.ContentLanguage != nil { | ||
| w.Header().Set("Content-Language", *result.ContentLanguage) | ||
| } | ||
| if result.ContentLength != nil { | ||
| w.Header().Set("Content-Length", strconv.FormatInt(*result.ContentLength, 10)) | ||
| } | ||
| if result.ContentRange != nil { | ||
| w.Header().Set("Content-Range", *result.ContentRange) | ||
| } | ||
| if result.ContentType != nil { | ||
| w.Header().Set("Content-Type", *result.ContentType) | ||
| } | ||
| if result.CacheControl != nil { | ||
| w.Header().Set("Cache-Control", *result.CacheControl) | ||
| } | ||
| if result.ETag != nil && *result.ETag != "" { | ||
| w.Header().Set("ETag", string(*result.ETag)) | ||
| } | ||
| if result.LastModified != nil { | ||
| w.Header().Set("Last-Modified", result.LastModified.Format(http.TimeFormat)) | ||
| } | ||
|
|
||
| w.WriteHeader(statusCode) | ||
Acconut marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| _, err = io.Copy(w, result.Body) | ||
| return err | ||
| } | ||
|
|
||
| func (blockBlob *BlockBlob) GetOffset(ctx context.Context) (int64, error) { | ||
| // Get the offset of the file from azure storage | ||
| // For the blob, show each block (ID and size) that is a committed part of it. | ||
|
|
@@ -260,6 +322,11 @@ func (infoBlob *InfoBlob) Download(ctx context.Context) (io.ReadCloser, error) { | |
| return resp.Body, nil | ||
| } | ||
|
|
||
| // ServeContent is not needed for infoBlob | ||
| func (infoBlob *InfoBlob) ServeContent(ctx context.Context, w http.ResponseWriter, r *http.Request) error { | ||
| return errors.New("azurestore: ServeContent is not implemented for InfoBlob") | ||
| } | ||
|
|
||
| // infoBlob does not utilise offset, so just return 0, nil | ||
| func (infoBlob *InfoBlob) GetOffset(ctx context.Context) (int64, error) { | ||
| return 0, nil | ||
|
|
@@ -316,3 +383,47 @@ func checkForNotFoundError(err error) error { | |
| } | ||
| return err | ||
| } | ||
|
|
||
| // parse the Range, If-Match, If-None-Match, If-Unmodified-Since, If-Modified-Since headers if present | ||
| func ParseDownloadOptions(r *http.Request) (*azblob.DownloadStreamOptions, error) { | ||
| input := azblob.DownloadStreamOptions{AccessConditions: &azblob.AccessConditions{}} | ||
|
|
||
| if val := r.Header.Get("Range"); val != "" { | ||
| // zero value count indicates from the offset to the resource's end, suffix-length is not required | ||
| input.Range = azblob.HTTPRange{Offset: 0, Count: 0} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does Azure support fetching multiple ranges in a single request? I.e. using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello, thank you for getting back :) Regarding multiple range in a single request, that is not supported. see also https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/storage/[email protected]/internal/exported#HTTPRange
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, I see. Can you please add a comment mentioning that Azure doesn't support multiple ranges and thus azurestore falls back to fetching the full resource, just like s3store does?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Great, thanks you! |
||
| bytesEnd := 0 | ||
| if _, err := fmt.Sscanf(val, "bytes=%d-%d", &input.Range.Offset, &bytesEnd); err != nil { | ||
| if _, err := fmt.Sscanf(val, "bytes=%d-", &input.Range.Offset); err != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to be handling requests where only the last bytes are requested using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that isn't supported either. how should this be handled? throw an error/map to a different httprange? It would be nice, if you could pull it across the finish line, I'm willing to do the testing :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see that the Azure SDK doesn't support this natively: https://github.com/Azure/azure-sdk-for-go/blob/f29a1d52c4e6894a536b0da18ac4399692e02c4c/sdk/storage/azblob/internal/exported/exported.go#L23 However, I think the corresponding I don't want to drag this on too long, but if there is an easy way to support fetching trailing bytes, I think it would be great to have it :) |
||
| return nil, err | ||
| } | ||
| } | ||
| if bytesEnd != 0 { | ||
| input.Range.Count = int64(bytesEnd) - input.Range.Offset + 1 | ||
| } | ||
| } | ||
| if val := r.Header.Get("If-Match"); val != "" { | ||
| etagIfMatch := azcore.ETag(val) | ||
| input.AccessConditions.ModifiedAccessConditions.IfMatch = &etagIfMatch | ||
| } | ||
| if val := r.Header.Get("If-None-Match"); val != "" { | ||
| etagIfNoneMatch := azcore.ETag(val) | ||
| input.AccessConditions.ModifiedAccessConditions.IfNoneMatch = &etagIfNoneMatch | ||
| } | ||
| if val := r.Header.Get("If-Modified-Since"); val != "" { | ||
| t, err := http.ParseTime(val) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| input.AccessConditions.ModifiedAccessConditions.IfModifiedSince = &t | ||
|
|
||
| } | ||
| if val := r.Header.Get("If-Unmodified-Since"); val != "" { | ||
| t, err := http.ParseTime(val) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| input.AccessConditions.ModifiedAccessConditions.IfUnmodifiedSince = &t | ||
| } | ||
|
|
||
| return &input, nil | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've done some tests using this branch, so first I'd like to thank you for your work :).
The only issue we found is that when a client does a request with a range above the total content length he'll receive an 500 error response instead of 416 because of the error being returned here.
I think it would be better to return the 416 response here, including the
content-rangeheader (example value:bytes */64978)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http.StatusPreconditionFailedin case ofIf-None-Matchheader is used.http.StatusNotFound, though that should normally not be hit sinceGetInfoinunroute_handlerwill fail before reaching ServerContent. File might no longer exist later on though.So updated condition would be: