-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
quic: start re-enabling and working on quic with openssl 3.5 #59249
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
Conversation
Review requested:
|
@nodejs/quic @nodejs/build @codebytere ... just a heads up since this will impact electron builds using boring... there's some work that'll need to be done to support boring here but it's possible. Specifically, see the edits to ngtcp2.gyp ... and more specifically, there's a comment in there about enabling the boringssl adapter that is provided by ngtcp2. @richardlau ... it's worth taking a look at the configure.py changes here. If at all possible, I could use some help with supporting openssl shared library builds on this. |
eab4848
to
907adb7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
907adb7
to
3ae9b2a
Compare
@jasnell (I'm out of office this week, so this is just quick thoughts after glancing at the changes.) For a shared library build, I'm not sure
We can see, for example, https://ci.nodejs.org/job/node-test-commit-linux-containered/51946/nodes=ubuntu2204_sharedlibs_openssl35_x64/consoleFull, that
If ngtcp2 is only needed for quic, maybe we don't need to version check at all for shared library openssl and assume whoever is building is responsible for linking to an appropriate OpenSSL version if quic support is enabled? If they are using a pre-3.5 version of OpenSSL then it's their responsibility to either not configure |
I was hoping to be able to do away with these compile guards and based it entirely on whether the version of openssl or boringssl that is used as the appropriate APIs available.. simply because it gives one less knob we have to turn when building. If we cannot reliably detect this, however, then yeah, falling back on these and just requiring whomever is building to provide the right thing would work. |
Start working on re-enabling QUIC support with the availability of OpenSSL 3.5. This will be a multi-step process. Signed-off-by: James M Snell <[email protected]>
Signed-off-by: James M Snell <[email protected]>
Signed-off-by: James M Snell <[email protected]>
Since we need to be able to use the openssl adapter provided by the ngtcp2 library, and because that adapter does not include any compile guards to ensure that OpenSSL 3.5 is being used and that the APIs are actually available, we need to add a compile time check for the openssl version in order to conditionally include the adapter to avoid build errors when using a shared openssl library that is not OpenSSL 3.5. Signed-off-by: James M Snell <[email protected]>
3ae9b2a
to
ce92086
Compare
This comment was marked as outdated.
This comment was marked as outdated.
warn(f'Could not recognize `gas`: {gas_ret}') | ||
return '0.0' | ||
|
||
def get_openssl_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.
Just as a heads up to reviewers... while I'm generally not very bullish on AI generated code, I did use copilot/claude to generate this particular function for me. I went through it and the impl appeared reasonable but it's absolutely worth reviewing in detail to make sure it is correct.
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.
Looking at the CI run, it does look like the check here is still failing for the shared-openssl cases but the build still proceeds since the value is set to 0. This has the side effect of disabling quic in those builds so it's not fatal. Still, would be ideal to figure out a version of this that works. Going to keep iterating but if anyone in @nodejs/build has any suggestions I'd appreciate it :-) .. the key issue is that it's not able to see the include path to find the opensslv.h header in the CI builds here. It looks like our builders aren't using the --shared-openssl-includes
option to set that.
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.
Brilliant! Thank you @richardlau !
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
RSLGTM
Start working on re-enabling QUIC support with the availability of OpenSSL 3.5. This will be a multi-step process. Signed-off-by: James M Snell <[email protected]> PR-URL: #59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: #59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: #59249 Reviewed-By: Matteo Collina <[email protected]>
Since we need to be able to use the openssl adapter provided by the ngtcp2 library, and because that adapter does not include any compile guards to ensure that OpenSSL 3.5 is being used and that the APIs are actually available, we need to add a compile time check for the openssl version in order to conditionally include the adapter to avoid build errors when using a shared openssl library that is not OpenSSL 3.5. Signed-off-by: James M Snell <[email protected]> PR-URL: #59249 Reviewed-By: Matteo Collina <[email protected]>
Landed in 99f5931...99c80e3 |
Hello sorry I am a bit lost, |
Not quite yet. I need to get the implementation updated to use the alternative APIs provided by openssl 3.5, get the build working again, and then finish working on the javascript side of the implementation. Given my current availability it's still likely to be at least a few weeks but my goal is to have it ready to go in time for the 25.0.0 release in October. |
Is this going to include a streams implementation or webtransport only? |
Start working on re-enabling QUIC support with the availability of OpenSSL 3.5. This will be a multi-step process. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Since we need to be able to use the openssl adapter provided by the ngtcp2 library, and because that adapter does not include any compile guards to ensure that OpenSSL 3.5 is being used and that the APIs are actually available, we need to add a compile time check for the openssl version in order to conditionally include the adapter to avoid build errors when using a shared openssl library that is not OpenSSL 3.5. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Start working on re-enabling QUIC support with the availability of OpenSSL 3.5. This will be a multi-step process. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Since we need to be able to use the openssl adapter provided by the ngtcp2 library, and because that adapter does not include any compile guards to ensure that OpenSSL 3.5 is being used and that the APIs are actually available, we need to add a compile time check for the openssl version in order to conditionally include the adapter to avoid build errors when using a shared openssl library that is not OpenSSL 3.5. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Start working on re-enabling QUIC support with the availability of OpenSSL 3.5. This will be a multi-step process. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Since we need to be able to use the openssl adapter provided by the ngtcp2 library, and because that adapter does not include any compile guards to ensure that OpenSSL 3.5 is being used and that the APIs are actually available, we need to add a compile time check for the openssl version in order to conditionally include the adapter to avoid build errors when using a shared openssl library that is not OpenSSL 3.5. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Start working on re-enabling QUIC support with the availability of OpenSSL 3.5. This will be a multi-step process.
There are some functional changes here but this is mostly set up to start enabling building the quic mechanisms by default if openssl 3.5.1 or higher is present. Otherwise, all the QUIC stuff will be disabled.
Note that this PR does not fully switch over. The move to the new guard will be incremental, after which the original compile guard will be removed.
Updates the ngtcp2 and nghttp3 dependencies also.
This cannot land until #59234 lands