Skip to content
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

Blobstore clients supports PUT headers #2494

Merged
merged 3 commits into from Feb 12, 2024
Merged

Conversation

mvach
Copy link
Contributor

@mvach mvach commented Feb 5, 2024

What is this change about?

When putting blobs via signed urls to Azure Blobstore, then the mandatory header 'x-ms-blob-type' has to be set to value 'blockblob'. Therefore we add a mechanism to support PUT headers.

We are doing it just the same why like encryption headers can be set for GCS.

@mvach mvach added the discuss label Feb 5, 2024
@mvach
Copy link
Contributor Author

mvach commented Feb 5, 2024

Hi @jpalermo,
we did not test this PR right now but just want to double check if the approach is right from you perspective.
So you feedback is very welcome :-)

When putting blobs via signed urls to Azure Blobstore, then the mandatory
header 'x-ms-blob-type' has to be set to value 'blockblob'.
@mvach mvach force-pushed the blobstore-client-put-headers branch from a6105af to 08d2216 Compare February 7, 2024 12:21
@jpalermo
Copy link
Member

jpalermo commented Feb 8, 2024

I haven't looked at it super closely, but I wonder if we can just combine all the headers in the blobstore client classes and rename signed_url_encryption_header to just signed_url_headers and let the blobstore object determine which headers should be included.

@mvach
Copy link
Contributor Author

mvach commented Feb 8, 2024

Hi @jpalermo,
Thx for the feedback, I fully agree but just wanted to follow the existing implementation, even if I wondered about these outside existences checks.
I'll update the PR accordingly.

@mvach mvach removed the discuss label Feb 8, 2024
@mvach
Copy link
Contributor Author

mvach commented Feb 8, 2024

After incorporating the feedback we started validating the PR in our local test environment.

@mvach mvach force-pushed the blobstore-client-put-headers branch from 198e085 to fdafcbb Compare February 8, 2024 15:47
@mvach
Copy link
Contributor Author

mvach commented Feb 9, 2024

We successfully finished testing this PR on our azure landscape.

@mvach mvach requested review from a team, aramprice and selzoc and removed request for a team February 9, 2024 11:22
@mvach
Copy link
Contributor Author

mvach commented Feb 9, 2024

Hi @jpalermo, hi @aramprice,
a review would be very nice.

@mvach
Copy link
Contributor Author

mvach commented Feb 12, 2024

Hi @aramprice,
if you are fine with the PR, then an approval would be nice.

Copy link
Member

@jpalermo jpalermo left a comment

Choose a reason for hiding this comment

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

👍

@jpalermo jpalermo merged commit 50d628d into main Feb 12, 2024
4 checks passed
@jpalermo jpalermo deleted the blobstore-client-put-headers branch February 12, 2024 17:37
jpalermo pushed a commit that referenced this pull request Feb 15, 2024
Follow up to PR #2494

Signed-off-by: Joseph Palermo <joseph.palermo@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants