-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Storage] Proxy support for storage-blob #2928
[Storage] Proxy support for storage-blob #2928
Conversation
ef555d1
to
43db3b8
Compare
|
||
// Use sharedKeyCredential, tokenCredential or anonymousCredential to create a pipeline | ||
const pipeline = StorageClient.newPipeline(sharedKeyCredential, { | ||
proxyOptions: { proxySettings: "http://localhost:3128" } |
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.
If https
is used instead of http
, the test fails with this error.
tunneling socket could not be established, cause=write EPROTO 6156:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:openssl\ssl\record\ssl3_record.c:252:
Needs more investigation.
cc: @bterlson
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.
@HarshaNalluru It's a known bug of axios 0.18.0 when enable proxy over https.
ms-rest-js @kpajdzik did some workaround in this change (Azure/ms-rest-js#322). But storage SDK may not update to use the ms-rest-js version with the fix, please have a check. And if necessary, you can update ms-rest-js version
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.
It's likely an issue in the underlying axios library.
Harsha already verified that http
works fine. Maybe we can merge this PR and track the external https
issue separately.
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.
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.
Are we sure this is an Axios problem and not the proxy server not supporting HTTPS? HTTPS on proxies is hard to set up as you need a trusted root certificate to man-in-the-middle the connection without generating certificate validation errors.
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.
I ran a squid container on my azure VM and tested that the proxy server is working using request
library
var request = require('request');
request({
'url':'https://docs.microsoft.com/en-us/rest/api/securitycenter/',
'method': "GET",
'proxy':'http://<server-ip>:3128'
},function (error, response, body) {
if (!error && response.statusCode == 200) {
console.log(body);
} else {
console.log("Error!!!" + error);
}
})
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.
The proxy there is HTTP, however, so you're talking HTTP to the proxy and the proxy is talking HTTPS to the endpoint. Try with https
for proxy.
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.
ah, proxy with https
gave the same error
Error: tunneling socket could not be established, cause=write EPROTO 41488:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:c:\ws\deps\openssl\openssl\ssl\record\ssl3_record.c:252:
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.
Created a new issue - #3465
|
||
// Use sharedKeyCredential, tokenCredential or anonymousCredential to create a pipeline | ||
const pipeline = StorageClient.newPipeline(sharedKeyCredential, { | ||
proxyOptions: { proxySettings: "http://localhost:3128" } |
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.
proxyOptions.proxySettings seems strange to me. Can we do proxyOptions.url instead?
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.
Done
|
||
// Use sharedKeyCredential, tokenCredential or anonymousCredential to create a pipeline | ||
const pipeline = StorageClient.newPipeline(sharedKeyCredential, { | ||
proxyOptions: { proxySettings: "http://localhost:3128" } |
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.
@HarshaNalluru It's a known bug of axios 0.18.0 when enable proxy over https.
ms-rest-js @kpajdzik did some workaround in this change (Azure/ms-rest-js#322). But storage SDK may not update to use the ms-rest-js version with the fix, please have a check. And if necessary, you can update ms-rest-js version
|
||
// Use sharedKeyCredential, tokenCredential or anonymousCredential to create a pipeline | ||
const pipeline = StorageClient.newPipeline(sharedKeyCredential, { | ||
proxyOptions: { url: "http://localhost:3128" } |
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.
Let's rename this option to proxy
since it's already in an option type, Options seems redundant. Otherwise LGTM!
92c299a
to
20ec78a
Compare
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.
LGTM
No description provided.