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

Upgrade https-proxy-agent ^5.0.1 to ^7.0.2 #679

Closed

Conversation

jportner
Copy link

@jportner jportner commented Oct 27, 2023

Description

Resolves #491.

This is a follow-on to #641. That PR fixed socket keepAlive for direct connections. However, keepAlive currently does not work on proxied connections, because https-proxy-agent 5.x closes connections prematurely. (The maintainer of that package added support for keepAlive in version 6.0.0).

This PR upgrades https-proxy-agent to ^7.0.2, so keepAlive will work for proxied connections.

Checklist

  • Format code according to the existing code style (run npm run lint:check -- CHANGED_FILES and fix problems in changed code)
  • Create tests which fail without the change (if possible)
  • Make all tests (unit and integration) pass (npm run test:unit and npm run test:integration)
  • Extend the README / documentation and ensure is properly displayed (if necessary)
  • Provide JIRA issue id (if possible) or GitHub issue id in commit message

Testing

I tested that the upgraded package still works by running a local proxy and using these environment variables to run the "testProxy" system test:

export SNOWFLAKE_TEST_USER="xxx"
export SNOWFLAKE_TEST_PASSWORD="xxx"
export SNOWFLAKE_TEST_ACCOUNT="xxx"
export SNOWFLAKE_TEST_WAREHOUSE="xxx"
export SNOWFLAKE_TEST_DATABASE="xxx"
export SNOWFLAKE_TEST_SCHEMA="xxx"
export SNOWFLAKE_TEST_PROXY_HOST="localhost"
export SNOWFLAKE_TEST_PROXY_PORT="xxx"
export SNOWFLAKE_TEST_PROXY_PROTOCOL="http"
export SNOWFLAKE_TEST_PROXY_USER="xxx"
export SNOWFLAKE_TEST_PROXY_PASSWORD="xxx"
npm run test:single -- system_test/testProxy.js

Output:

> [email protected] test:single
> mocha -timeout 180000 --full-trace system_test/testProxy.js



  testProxy
    ✔ testConnectionWithProxy (649ms)
    ✔ testSimpleSelectWithProxy (1609ms)


  2 passing (2s)

@jportner jportner force-pushed the upgrade-https-proxy-agent branch from bfd9430 to f3a54cd Compare October 27, 2023 21:05
@jportner jportner marked this pull request as ready for review October 27, 2023 21:08
@jportner jportner requested a review from a team as a code owner October 27, 2023 21:08
@jportner
Copy link
Author

jportner commented Oct 27, 2023

Hi @sfc-gh-pmotacki

I'm following up on your comment on my other PR: #496 (comment)
Since that PR is closed, I am unable to reply to you there.

I submitted this PR to upgrade the https-proxy-agent package, would you be able to review it?

See the description:

This is a follow-on to #641. That PR fixed socket keepAlive for direct connections. However, keepAlive currently does not work on proxied connections, because https-proxy-agent 5.x closes connections prematurely. (The maintainer of that package added support for keepAlive in version 6.0.0).

@sfc-gh-pmotacki
Copy link
Collaborator

Hi @jportner
Thank you for your contribution. It is great news that using new http-proxy-agent version we can remove custom implementation of http agent functionality. I've run your PR and tests were passed. I need to manually test all types of authentications with proxy to confirm we can marge this to master.

@jportner
Copy link
Author

jportner commented Nov 7, 2023

Thank you! Let me know if I can help at all.

@kiru42
Copy link

kiru42 commented Nov 27, 2023

Hello @jportner, @sfc-gh-pmotacki ,

Thank you for the great work.

Any update on this PR? Let me know if I can help you on this PR.

@sfc-gh-pmotacki
Copy link
Collaborator

This is still on my side. I am trying to arrange time to complete the planned work and confirm that the changes are ready for merging.

@sfc-gh-pmotacki
Copy link
Collaborator

@jportner One more time thank you for you contribution. After tests I added some small fixes and finally I merged your change in pr 689.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
@sfc-gh-pmotacki
Copy link
Collaborator

Changes was merged in PR #689

@jportner jportner deleted the upgrade-https-proxy-agent branch December 5, 2023 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable keepAlive for proxied connections
3 participants