ccl/storageccl: support HTTP_PROXY env vars #34067
Conversation
|
This change is |
|
We have existing users complaining about the lack of proxy support -- do we want to consider this for a backport? It is fixing a user-visible defect (and $ users at that) and is pretty tightly contained change. I think it is low-risk too? |
|
Roland reported that the proxy settings were already working for S3, although I'm not sure how. I think it makes sense to make a full test to ensure we don't regress and to figure out if it already worked, and if so, why. I'll rework this PR with a test. |
|
Added a pretty annoying test. I verified that it fails without the accompanying change. This leads me to believe that the S3 client has its own proxy stuff which is why the customer reported it working, but the test needed the change to pass since the tests uses HTTP. I do think this warrants a backport in either case. |
|
I like this change. Thank you for making it! Is there still a chance to merge this and backport? |
|
Yes. @dt can you review the test? |
|
Although this is kind of annoying because the httpproxy package would have to be vendored just for this test. Not sure if it's worth it. |
|
still lgtm -- I'm fine with or without the test. I'd say vendoring the lib isn't unreasonable, though doing it again for the backport seems like it's really getting into diminishing returns. |
HTTP and S3 storage both used a wrapped http.Client, but it didn't copy and modify http.DefaultTransport, instead making one from scratch. This meant that the normally supported settings like proxy and timeouts were zero'd. Fixes #32803 Release note (enterprise change): Support standard HTTP proxy environment variables in HTTP and S3 storage.
|
bors r+ |
|
Thanks. I cut the test. But I was glad to write it to verify things worked as expected. |
34067: ccl/storageccl: support HTTP_PROXY env vars r=mjibson a=mjibson HTTP and S3 storage both used a wrapped http.Client, but it didn't copy and modify http.DefaultTransport, instead making one from scratch. This meant that the normally supported settings like proxy and timeouts were zero'd. Fixes #32803 Release note (enterprise change): Support standard HTTP proxy environment variables in HTTP and S3 storage. Co-authored-by: Matt Jibson <[email protected]>
Build succeeded |
HTTP and S3 storage both used a wrapped http.Client, but it didn't copy
and modify http.DefaultTransport, instead making one from scratch. This
meant that the normally supported settings like proxy and timeouts
were zero'd.
Fixes #32803
Release note (enterprise change): Support standard HTTP proxy environment
variables in HTTP and S3 storage.