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

quic: remove experimental quic #37067

Closed
wants to merge 5 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 25, 2021

The OpenSSL OMC has not yet committed to landing the updated QUIC APIs and has indicated that they will not even look at it until OpenSSL 3.1. With OpenSSL 3.0 beta currently delayed with no clear idea of when it will actually land, the initial QUIC support landed in core has now just become a maintenance burden with no clear idea of when we'd ever be capable of delivering it. This PR, therefore, removes the QUIC support and reverts the patched in modifications to openssl. I will be investigating a userland alternative that does not depend on the built-in openssl bindings.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jan 25, 2021
@jasnell jasnell changed the title Revert "deps: update patch and docs for openssl update" quic: remove quic Jan 25, 2021
@jasnell jasnell changed the title quic: remove quic quic: remove experimental quic Jan 25, 2021
@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

This is unfortunate. Is there any chance BoringSSL might provide QUIC support before OpenSSL does?

@jasnell
Copy link
Member Author

jasnell commented Jan 25, 2021

BoringSSL already does. Up to this point, we've been using Akamai's port of the BoringSSL APIs to OpenSSL 1.1.1. However, because we cannot use BoringSSL in supported releases because of the lack of an adequate LTS policy we cannot depend on it.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@tniessen
Copy link
Member

However, because we cannot use BoringSSL in supported releases because of the lack of an adequate LTS policy we cannot depend on it.

Yes, that's always been a problem, but we've still applied numerous patches to allow linking Node.js against BoringSSL. Do you see any way of supporting QUIC only when linking against BoringSSL, e.g., within Electron? Or is it too much of a maintenance burden?

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen
Copy link
Member

However, because we cannot use BoringSSL in supported releases because of the lack of an adequate LTS policy we cannot depend on it.

Yes, that's always been a problem, but we've still applied numerous patches to allow linking Node.js against BoringSSL. Do you see any way of supporting QUIC only when linking against BoringSSL, e.g., within Electron? Or is it too much of a maintenance burden?

Maybe we keep a boringssl deps only for quic part like https://github.com/cloudflare/quiche did.

@jasnell
Copy link
Member Author

jasnell commented Jan 26, 2021

Yes, that's always been a problem, but we've still applied numerous patches to allow linking Node.js against BoringSSL. Do you see any way of supporting QUIC only when linking against BoringSSL, e.g., within Electron? Or is it too much of a maintenance burden?

That's entirely up to the @nodejs/tsc as whole. We apply patches to support BoringSSL but that's a far cry from shipping supported features that only work with BoringSSL when we do not ship any officially supported releases that use BoringSSL.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM and 😞

@tniessen
Copy link
Member

We apply patches to support BoringSSL but that's a far cry from shipping supported features that only work with BoringSSL when we do not ship any officially supported releases that use BoringSSL.

Sadly, that's true. I am not advocating for officially supporting BoringSSL, I don't think that's reasonable as long as Google doesn't provide LTS support for BoringSSL Also, I had a few discussions with @codebytere about BoringSSL/Electron in the past and IIRC, some Node.js crypto features are unavailable in Electron because BoringSSL does not provide them.

Still, it's sad to not see this progressing in OpenSSL.

@jasnell
Copy link
Member Author

jasnell commented Jan 26, 2021

If we're able to see progress made on the openssl side... even just a realistic time frame when we can expect movement towards official support, then we'd have something better to go on. But as it is now, all of this code just ends up being an open ended maintenance burden. Given the amount of work I've done on this, there's literally no one that is more disappointed by this PR than I am.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 27, 2021
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

jasnell added a commit that referenced this pull request Jan 30, 2021
This reverts commit 548790a.

PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
jasnell added a commit that referenced this pull request Jan 30, 2021
This reverts commit b0d5bfe.

PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
jasnell added a commit that referenced this pull request Jan 30, 2021
This reverts commit 06c5b53.

PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
jasnell added a commit that referenced this pull request Jan 30, 2021
PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
jasnell added a commit that referenced this pull request Jan 30, 2021
PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Jan 30, 2021

Landed in 255d633...290ecb3 ....

Ugh this is painful.

@jasnell jasnell closed this Jan 30, 2021
targos pushed a commit that referenced this pull request Feb 2, 2021
This reverts commit 548790a.

PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Feb 2, 2021
This reverts commit b0d5bfe.

PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Feb 2, 2021
This reverts commit 06c5b53.

PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Feb 2, 2021
PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Feb 2, 2021
PR-URL: #37067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Feb 2, 2021
@pimterry pimterry mentioned this pull request Apr 30, 2021
RaisinTen added a commit to RaisinTen/node that referenced this pull request Sep 12, 2022
These were added in nodejs#32379 and were
supposed to get removed in nodejs#37067.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants