Skip to content

Conversation

@code-asher
Copy link
Member

@code-asher code-asher commented Jan 3, 2022

For coder/code-server#4672

I tested it with our extension (which downloads the Coder CLI) and confirmed it works. Once I finish my test extension PR I will add a test for the proxy there as well.

@code-asher code-asher requested a review from jsjoeio January 3, 2022 20:53
Copy link

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Looks great! No concerns just one clarifying question.

* @author coder
*/
export async function monkeyPatchHttpAgent(): Promise<void> {
if (process.env.http_proxy || process.env.https_proxy || process.env.HTTP_PROXY || process.env.HTTPS_PROXY) {
Copy link

Choose a reason for hiding this comment

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

Do we need to add no_proxy as an option?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, no_proxy is for specifying exceptions; the proxy agent module will check no_proxy when making a request to see whether it should send through the proxy or not.

If http_proxy and friends are not set then no_proxy has no effect.

Copy link

Choose a reason for hiding this comment

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

Oh! I see now. Thanks for clarifying!

@code-asher
Copy link
Member Author

Uh...apparently this already works. I removed my changes to confirm but everything works exactly the same. I cannot seem to find how they are doing this so I am not sure if I am just doing something dumb like not properly recompiling.

@jsjoeio
Copy link

jsjoeio commented Jan 3, 2022

Uh...apparently this already works. I removed my changes to confirm but everything works exactly the same. I cannot seem to find how they are doing this so I am not sure if I am just doing something dumb like not properly recompiling.

Lol out of curiosity, how are you testing this?

@code-asher
Copy link
Member Author

i am running https://github.com/coder/vscode-coder since it makes a download request to fetch the Coder CLI.

Then I run http_proxy=http://localhost:3128 yarn watch. The download fails saying it cannot connect to the proxy. Then I start the proxy (I am using squid) and it works.

@code-asher
Copy link
Member Author

code-asher commented Jan 3, 2022

I confirmed it works in the latest artifact built by CI...I still have not located where this happens but I guess for now we are good to move on.

no_proxy does not seem to work but I tested previous versions and trying to use a proxy causes an infinite loop so...it was already broken. Maybe we can patch support in for no_proxy but we have to find where this happens first. In any case the current state is an improvement over infinite loops. 😛

@code-asher code-asher closed this Jan 3, 2022
@code-asher code-asher deleted the http-proxy branch January 3, 2022 22:20
@jsjoeio
Copy link

jsjoeio commented Jan 3, 2022

Nice thorough testing! Well, I'll leave that issue in code-server open and move it to the next version milestone then!

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.

2 participants