Skip to content

implement ContentServerDataStore for azure blob storage too#1234

Open
xelat09 wants to merge 13 commits into
tus:mainfrom
xelat09:main
Open

implement ContentServerDataStore for azure blob storage too#1234
xelat09 wants to merge 13 commits into
tus:mainfrom
xelat09:main

Conversation

@xelat09

@xelat09 xelat09 commented Jan 5, 2025

Copy link
Copy Markdown
Contributor

Hi & Happy new year

Implement ContentServerDataStore for azure blob storage too, tests with azurite+azure blob are ok.
Incomplete range requests (Range: bytes=100- ) are not supported, hope that's ok

Regards

@Acconut Acconut left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this PR, it's greatly appreciated! I had a brief first look and left a few comments. Let me know what you think!

Comment thread pkg/azurestore/azureservice.go Outdated
Comment thread pkg/azurestore/azureservice.go
Comment thread pkg/azurestore/azureservice.go Outdated
@xelat09 xelat09 reopened this Jan 12, 2025
@xelat09

xelat09 commented Jan 12, 2025

Copy link
Copy Markdown
Contributor Author

yeah, as suggested, azure supports all those headers too. please have a look, i gave it a new try. tests with azurite do work..

do you see anything else missing?

@Acconut Acconut left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the quick updates! The handler is the request headers looks much better yet. Could you also have a look at the failing tests at https://github.com/tus/tusd/actions/runs/12736636713/job/35496766413?pr=1234? Talking about tests, it would be great to have some tests for ServeContent in azurestore as well. For inspiration you can use s3store's tests: https://github.com/tus/tusd/blob/main/pkg/s3store/serve_content_test.go

Comment thread pkg/azurestore/azureservice.go Outdated
Comment thread pkg/azurestore/azureservice.go Outdated
Comment thread pkg/azurestore/azureservice.go Outdated
@xelat09 xelat09 requested a review from Acconut January 23, 2025 12:57

@Acconut Acconut left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there, I just had another look over this PR. Apologies for missing it earlier. The PR seems mostly finished. I think the handling of Range header could be improved a bit and then there is still this comment: https://github.com/tus/tusd/pull/1234/files#r1905384839

Let me know if you are still interested in completing this PR. If not, I can consider pulling it across the finish line.


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}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 Range: <unit>=<range-start>-<range-end>, …, <range-startN>-<range-endN> (from https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Range). S3 doesn't so the s3store doesn't support it. We can do the same here, but I was just wondering.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thank you for getting back :)
Response headers are forwarded now.
Regarding Range Header handling, what is missing, give me some hints?

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/azblob@v1.6.1/internal/exported#HTTPRange

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/azblob@v1.6.1/internal/exported#HTTPRange

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response headers are forwarded now.

Great, thanks you!

input.Range = azblob.HTTPRange{Offset: 0, Count: 0}
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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 Range: <unit>=-<suffix-length>. Is that supported by Azure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 AzUpload might have the total size of the object already in its info object. So we could calculate the corresponding offset based on the size as offset = size - suffix-length (and maybe -1 depending on how the bytes are counted, not sure).

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 :)

}
result, err := blockBlob.BlobClient.DownloadStream(ctx, downloadOptions)
if err != nil {
return err

Copy link
Copy Markdown
Contributor

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-range header (example value: bytes */64978)

	if err != nil {
		// copy header "Content-Range", "X-Ms-Error-Code", "Date", "X-Ms-Request-Id"; Body empty; StatusCode+Status if present in error response
		var azureError *azcore.ResponseError
		if errors.As(err, &azureError) {
			if azureError.StatusCode == http.StatusRequestedRangeNotSatisfiable {
				if azureError.RawResponse != nil {
					if val := azureError.RawResponse.Header.Get("Content-Range"); val != "" {
						w.Header().Set("Content-Range", val)
					}
					if val := azureError.RawResponse.Header.Get("X-Ms-Error-Code"); val != "" {
						w.Header().Set("X-Ms-Error-Code", val)
					}
					if val := azureError.RawResponse.Header.Get("X-Ms-Request-Id"); val != "" {
						w.Header().Set("X-Ms-Request-Id", val)
					}
					if val := azureError.RawResponse.Header.Get("Date"); val != "" {
						w.Header().Set("Date", val)
					}
				}
				w.WriteHeader(azureError.StatusCode)
				return nil
			}
		}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Also missing http.StatusPreconditionFailed in case of If-None-Match header is used.
  • Might want to include http.StatusNotFound, though that should normally not be hit since GetInfo in unroute_handler will fail before reaching ServerContent. File might no longer exist later on though.

So updated condition would be:

// ...
			if http.StatusRequestedRangeNotSatisfiable == azureError.StatusCode ||
				http.StatusPreconditionFailed == azureError.StatusCode ||
				http.StatusNotFound == azureError.StatusCode {
// ...

// Use 206 Partial Content for range requests
statusCode = http.StatusPartialContent
} else if result.ContentLength != nil && *result.ContentLength == 0 {
statusCode = http.StatusNoContent

@quality-leftovers quality-leftovers Jan 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When ETag is matched the response should be 304 StatusNotModified. The following allows doing so use the returned error header. Sadly the SDK does NOT expose the status code / actual typed error.

   } 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
		}
   }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to result.ContentLength == nil, required for http.StatusNotModified to work with Azurite

@Acconut

Acconut commented Feb 27, 2026

Copy link
Copy Markdown
Member

@quality-leftovers Thank you for helping out with this! Would you be willing to open a new PR with your changes included? Then we can iterate there quickly.

Once merging, we would obviously make sure to also pay tribute to @xelat09, who started the implementation in the first place. Thank you as well for that, @xelat09!

@quality-leftovers

Copy link
Copy Markdown
Contributor

@quality-leftovers Thank you for helping out with this! Would you be willing to open a new PR with your changes included? Then we can iterate there quickly.

Once merging, we would obviously make sure to also pay tribute to @xelat09, who started the implementation in the first place. Thank you as well for that, @xelat09!

Sure. If @xelat09 cannot continue his work I'll try to pick it up.

Should I

  • create a branch based on the head of this PR (xelat09:main)
  • create a new branch based on tusd:main and add a single commit with the original changes and then do the changes on top of that
  • create a single commit with all changes
  • ... ?

@Acconut

Acconut commented Feb 27, 2026

Copy link
Copy Markdown
Member

create a branch based on the head of this PR (xelat09:main)

Great, this sounds like the best approach. We squash PRs when merging, so the commit history is not too relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants