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

Update ensureAlpnAndH2Enabled to reflect NPN protocol #8836

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Spikhalskiy
Copy link

Currently #ensureAlpnAndH2Enabled is functionally correct, but the method name and error messages make it look like only ALPN is allowed by this method and it doesn't allow NPN protocol, which is not the case.
This method likely wasn't updated when NPN was added in addition to NPN and now the naming and the error messages are misleading for the reader.

This PR updates the method name and error messages to reflect that there are currently two Protocol Negotiators

@ejona86
Copy link
Member

ejona86 commented Feb 7, 2022

How did you notice this? Are you using NPN or were surprised that methods that said "ALPN" might do NPN instead?

Our code initially relied quite a bit on NPN. The Netty API supported both ALPN and NPN and I think the naming "application protocol negotiation" was just close enough to ALPN that alpn was used. Generally speaking, we have made little distinction between the two protocols and which one was actually being used was a deeper implementation detail. (This is partly due to how ALPN was created and how the implementation APIs evolved.) We'd also not use the term "protocol negotiation," as we have already have ProtocolNegotiator that handles the handshake for connections.

NPN though, has been basically dead for many years. Jetty NPN only existed for Java 7, which was only useful to people configuring their SslContext manually because the'd need to disable GCM to have anything resembling a mediocre throughput. And tcnative NPN only existed for those using their operating-system-provided openssl, not the normally-encouraged boringssl-static. It was maybe a half decade ago that we worked to ensure 99% of our users would be using ALPN. We could consider dropping NPN completely at this point, although probably won't because it takes virtually no maintenance.

@Spikhalskiy
Copy link
Author

Spikhalskiy commented Feb 8, 2022

Mainly when I was reading gRPC SSL related implementations, I saw a call to ensureAlpnAndH2Enabled and spent quite a bit of time trying to understand why there may be Alpn only at that place and why it's not allowed to be NPN and how it all works with NPN, looks like it should just throw. But after digging into ensureAlpnAndH2Enabled, I realized that the actual meaning of the check is "any negotiator is set" and it makes sense after that. So, I updated the naming in a way that this confusion for the reader doesn't happen.

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.

None yet

2 participants