Add custom headers in s3 #4400

bekadavis9 · 2023-10-04T21:37:22Z

Allow users to input custom headers using a new config option, vfs.s3.custom_headers.* with format config["vfs.s3.custom_headers.key_name"] = "header_value"

TYPE: FEATURE
DESC: Support custom headers on s3 requests

shortcut-integration · 2023-10-04T21:37:47Z

This pull request has been linked to Shortcut Story #31648: Capability to add headers to cURL requests for S3.

bekadavis9 · 2023-10-04T21:39:05Z

Functionality was validated locally, with findings here

eric-hughes-tiledb · 2023-10-04T22:14:02Z

Functionality was validated locally, with findings here

We should not expect anyone to find this issue or a gist in order to be able to replicate the test. All the relevant files should be checked in somewhere, probably in a test directory of some sort.

teo-tsirpanis

Mostly good, thanks! Left some small comments.

tiledb/api/c_api/config/config_api_external.h

tiledb/sm/filesystem/s3.cc

ihnorton

Was there a reason to go with the subclass here rather than PutObject::SetAdditionalCustomHeaderValue discussed previously?

(also, as mentioned elsewhere: this should have a test harness; an approach was previously discussed, and outlined in the original ticket, which should take a small amount of python using the built-in HTTP server)

ihnorton · 2023-10-06T03:20:17Z

Also, we should specifically make sure this works for setting a custom User-Agent (additional use-case).

eric-hughes-tiledb · 2023-10-06T12:18:47Z

reason to go with the subclass here

It's gives a designated point of construction and makes C.41 compliance easier. Even though it's not done in the version I looked at, it provide a single place to parse and validate configuration values.

teo-tsirpanis · 2023-10-06T12:32:27Z

Was there a reason to go with the subclass

It was my idea, after being asked offline. I hadn't noticed that PutObjectRequest derives from AmazonWebServiceRequest and there's an easier way.

One other advantage of the subclass is that the custom headers automatically apply to all kinds of S3 requests.

ihnorton · 2023-10-06T14:20:38Z

One other advantage of the subclass is that the custom headers automatically apply to all kinds of S3 requests.

👍 Thanks

davisp

Looks pretty good. Once you fix the syntax error and remove the unneeded config entry changes this will be good to go.

test/src/unit-capi-config.cc

tiledb/api/c_api/config/config_api_external.h

tiledb/sm/config/config.cc

tiledb/sm/cpp_api/config.h

tiledb/sm/filesystem/s3.h

teo-tsirpanis

Thanks!

teo-tsirpanis · 2023-10-11T16:41:28Z

@bekadavis9 can you update the title and description of the PR to account for the new config option and not mention curl?

davisp

+1

@Shelnutt2

[SC-49141](https://app.shortcut.com/tiledb-inc/story/49141/add-support-for-custom-rest-headers-via-config) Similar to S3 custom header support (#4400) this PR adds support for custom headers in REST request. This allows users to set header via a config parameter instead of the current `tildb_context_set_tag`. The context set tag does not work in all cases because its not always possible to use the same context, i.e SOMA and VCF python APIs can't pass a TileDB-Py ctx to their respective C++ code. Big thanks to @Shelnutt2 for implementing this. I added a test and documentation. --- TYPE: CONFIG DESC: Add `rest.custom_headers.*` config option to set custom headers on REST requests. --------- Co-authored-by: Seth Shelnutt <[email protected]> Co-authored-by: KiterLuc <[email protected]> Co-authored-by: eric-hughes-tiledb <[email protected]> Co-authored-by: Luc Rancourt <[email protected]>

@Shelnutt2

[SC-49141](https://app.shortcut.com/tiledb-inc/story/49141/add-support-for-custom-rest-headers-via-config) Similar to S3 custom header support (#4400) this PR adds support for custom headers in REST request. This allows users to set header via a config parameter instead of the current `tildb_context_set_tag`. The context set tag does not work in all cases because its not always possible to use the same context, i.e SOMA and VCF python APIs can't pass a TileDB-Py ctx to their respective C++ code. Big thanks to @Shelnutt2 for implementing this. I added a test and documentation. --- TYPE: CONFIG DESC: Add `rest.custom_headers.*` config option to set custom headers on REST requests. --------- Co-authored-by: Seth Shelnutt <[email protected]> Co-authored-by: KiterLuc <[email protected]> Co-authored-by: eric-hughes-tiledb <[email protected]> Co-authored-by: Luc Rancourt <[email protected]>

@Shelnutt2

[SC-49141](https://app.shortcut.com/tiledb-inc/story/49141/add-support-for-custom-rest-headers-via-config) Similar to S3 custom header support (#4400) this PR adds support for custom headers in REST request. This allows users to set header via a config parameter instead of the current `tildb_context_set_tag`. The context set tag does not work in all cases because its not always possible to use the same context, i.e SOMA and VCF python APIs can't pass a TileDB-Py ctx to their respective C++ code. Big thanks to @Shelnutt2 for implementing this. I added a test and documentation. --- TYPE: CONFIG DESC: Add `rest.custom_headers.*` config option to set custom headers on REST requests. --------- Co-authored-by: Seth Shelnutt <[email protected]> Co-authored-by: KiterLuc <[email protected]> Co-authored-by: eric-hughes-tiledb <[email protected]> Co-authored-by: Luc Rancourt <[email protected]>

bekadavis9 requested review from davisp, ihnorton and teo-tsirpanis October 4, 2023 21:39

teo-tsirpanis requested changes Oct 4, 2023

View reviewed changes

tiledb/api/c_api/config/config_api_external.h Outdated Show resolved Hide resolved

tiledb/sm/filesystem/s3.cc Outdated Show resolved Hide resolved

KiterLuc added the pending-2.18 label Oct 5, 2023

ihnorton reviewed Oct 6, 2023

View reviewed changes

bkmartinjr mentioned this pull request Oct 6, 2023

analytics: identify census builder H5AD download requests chanzuckerberg/cellxgene-census#257

Closed

2 tasks

bekadavis9 added 2 commits October 10, 2023 09:59

Add custom curl headers in s3

c500537

Allow users to add multiple custom headers

ef0f10f

davisp requested changes Oct 10, 2023

View reviewed changes

Rebase on dev, address comments

8d46801

bekadavis9 force-pushed the rd/curl_headers branch from 794b555 to 8d46801 Compare October 10, 2023 17:16

bekadavis9 requested review from davisp and teo-tsirpanis October 11, 2023 15:03

teo-tsirpanis approved these changes Oct 11, 2023

View reviewed changes

KiterLuc force-pushed the rd/curl_headers branch from dfe0725 to 8d46801 Compare October 11, 2023 16:20

S3ClientConfiguration -> ClientConfiguration

af7ac74

davisp approved these changes Oct 11, 2023

View reviewed changes

Fix CI?

11cc5fe

bekadavis9 changed the title ~~Add custom curl headers in s3~~ Add custom headers in s3 Oct 11, 2023

KiterLuc merged commit 0dc6650 into dev Oct 12, 2023
54 checks passed

KiterLuc deleted the rd/curl_headers branch October 12, 2023 06:02

bkmartinjr mentioned this pull request Feb 21, 2024

[misc] user-agent tagging chanzuckerberg/cellxgene-census#997

Closed

5 tasks

teo-tsirpanis mentioned this pull request Jun 19, 2024

Add support for custom headers for REST via config. #5104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom headers in s3 #4400

Add custom headers in s3 #4400

bekadavis9 commented Oct 4, 2023 •

edited

Loading

shortcut-integration bot commented Oct 4, 2023

bekadavis9 commented Oct 4, 2023

eric-hughes-tiledb commented Oct 4, 2023

teo-tsirpanis left a comment

ihnorton left a comment

ihnorton commented Oct 6, 2023

eric-hughes-tiledb commented Oct 6, 2023 •

edited

Loading

teo-tsirpanis commented Oct 6, 2023

ihnorton commented Oct 6, 2023

davisp left a comment

teo-tsirpanis left a comment

teo-tsirpanis commented Oct 11, 2023

davisp left a comment

Add custom headers in s3 #4400

Add custom headers in s3 #4400

Conversation

bekadavis9 commented Oct 4, 2023 • edited Loading

shortcut-integration bot commented Oct 4, 2023

bekadavis9 commented Oct 4, 2023

eric-hughes-tiledb commented Oct 4, 2023

teo-tsirpanis left a comment

Choose a reason for hiding this comment

ihnorton left a comment

Choose a reason for hiding this comment

ihnorton commented Oct 6, 2023

eric-hughes-tiledb commented Oct 6, 2023 • edited Loading

teo-tsirpanis commented Oct 6, 2023

ihnorton commented Oct 6, 2023

davisp left a comment

Choose a reason for hiding this comment

teo-tsirpanis left a comment

Choose a reason for hiding this comment

teo-tsirpanis commented Oct 11, 2023

davisp left a comment

Choose a reason for hiding this comment

bekadavis9 commented Oct 4, 2023 •

edited

Loading

eric-hughes-tiledb commented Oct 6, 2023 •

edited

Loading