Skip to content

chore NET-1404: remove deprecated package request from autocert-* #2996

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

Merged
merged 21 commits into from
Feb 12, 2025

Conversation

harbu
Copy link
Contributor

@harbu harbu commented Jan 29, 2025

Summary

Package request has been deprecated since 2020. Removed its usage from sub-packages autocertifier-server and autocertifier-client and replaced with with native https module's request functionality.

Also made test packages/autocertifier-server/test/integration/RestServer-real-http-client.test.ts exercise the new HTTP requesting utility in autocertifier-client to gain more test coverage confidence.

I tested the certificate fetching process by hand by creating a new cloud instance and installing this branch there. My node was successfully able to get a subdomain and cert assigned to itself by the production autocertifier-server.

Future improvements

Use built-in node.js fetch instead of https. However, see the discussion of this PR as to how this needs to be approached.

Checklist before requesting a review

  • Is this a breaking change? If it is, be clear in summary.
  • Read through code myself one more time.
  • Make sure any and all TODO comments left behind are meant to be left in.
  • Has reasonable passing test coverage?
  • Updated changelog if applicable.
  • Updated documentation if applicable.

@harbu harbu changed the title Net 1404 remove old module request chore NET-1404: remove deprecated package request from autocert-* Jan 29, 2025
@harbu
Copy link
Contributor Author

harbu commented Jan 30, 2025

There are no tests in the autocertifier-client sub-package. How can I check that my changes work? In particular I removed the allowance of self-signed certs in the autocertifier-client, should the ability to allow self-signed certs be kept in? Ping @juslesan @ptesavol

@juslesan
Copy link
Contributor

juslesan commented Jan 30, 2025

There are no tests in the autocertifier-client sub-package. How can I check that my changes work?

There is a test in the autocertifier-server that tests that the integration between the client and server works. It should check that the basic functionality is okay.

In particular I removed the allowance of self-signed certs in the autocertifier-client, should the ability to allow self-signed certs be kept in?

This is something that should be checked by running the client against the production server. I do believe there is a proper TLS certificate there so this should not be required.

@harbu harbu force-pushed the NET-1404-remove-old-module-request branch from f67c6f2 to 2ce4156 Compare January 30, 2025 16:17
@harbu harbu marked this pull request as ready for review February 3, 2025 10:27
@github-actions github-actions bot added the docs label Feb 3, 2025
@github-actions github-actions bot removed the docs label Feb 3, 2025
@harbu harbu requested a review from teogeb February 3, 2025 16:25
@harbu harbu requested review from ptesavol, teogeb and juslesan and removed request for teogeb February 3, 2025 16:25
Copy link
Collaborator

@ptesavol ptesavol left a comment

Choose a reason for hiding this comment

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

Good simplification indeed, many lines of code less. However, one thing to discuss is whether we should move to using fetch already. Fetch will be available in the global object the same way as in browsers starting from NodeJS 21, and we could fall back to node-fetch package on older NodeJS versions. You can see see this post about fetch in node: https://blog.logrocket.com/fetch-api-node-js/

@harbu
Copy link
Contributor Author

harbu commented Feb 11, 2025

Good simplification indeed, many lines of code less. However, one thing to discuss is whether we should move to using fetch already. Fetch will be available in the global object the same way as in browsers starting from NodeJS 21, and we could fall back to node-fetch package on older NodeJS versions. You can see see this post about fetch in node: https://blog.logrocket.com/fetch-api-node-js/

I had a go at using built-in node.js fetch to accomplish this first, however there seems to be no way of passing a option to allow for self-signed certificates or to disable certificate validation. Apparently to achieve this you need to install a package undici and I'm not sure it is worth it in this case (see https://github.com/orgs/nodejs/discussions/44038#discussioncomment-5701073)

@harbu harbu merged commit c520534 into main Feb 12, 2025
24 checks passed
@harbu harbu deleted the NET-1404-remove-old-module-request branch February 12, 2025 14:48
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.

3 participants