ssl: moving openssl context manager to contrib #32748
Conversation
alyssawilk
force-pushed
the
manager
branch
7 times, most recently
from
f481f4a to
e97cff4
Compare
Signed-off-by: Alyssa Wilk <[email protected]>
|
/retest |
Signed-off-by: Alyssa Wilk <[email protected]>
|
/retest |
| Ssl::ContextManagerFactory* ssl_factory = | ||
| Registry::FactoryRegistry<Ssl::ContextManagerFactory>::getFactory("ssl_context_manager"); | ||
| if (!ssl_factory) { | ||
| throwEnvoyExceptionOrPanic("No SSL factory compiled"); |
There was a problem hiding this comment.
Isn't this a regression? Before this change, it was possible (I think?) to compile Envoy with no TLS stack at all, and it would mostly work. With this change, it makes it throw an error on startup.
I don't know if anyone actually uses Envoy with no TLS stack or not.
There was a problem hiding this comment.
so if you want to compile Enovy with no TLS stack, you compile in the contrib stub extension instead. It now registers as the basic ssl_context_manager
There was a problem hiding this comment.
I think I'm missing some context for why this change is being made. It's a tiny chunk of code. What's the advantage of moving it into contrib, vs leaving it where it is?
There was a problem hiding this comment.
- generally not needed and should have been a contrib thing to start with but really 2) I've been working to make Envoy Mobile (and hopefully then core Envoy) exception free and this is just a big bundle of unnecessary exceptions.
There was a problem hiding this comment.
I dislike the approach because you can build an envoy binary that cannot start; that should be a compile-time error.
However, I experimented a bit with no-ssl builds of envoy this morning. I tried bazel build --config=clang -- //:envoy --define boringssl=disabled --//bazel:http3=False --//source/extensions/transport_sockets/tls:enabled=false. What I got was a build with no tls transport socket, envoy --version says 065710fbe7f453d7f8ba44871330af33b0a0d978/1.30.0-dev/Modified/DEBUG/no-ssl, but nm clearly shows all of the boringssl symbols compiled in. I have no idea what in the build system caused that, but it seems clear that nobody is using a no-ssl build of envoy. @phlax are you aware of anyone building Envoy without ssl support?
If it can't even be built without ssl, can we make this not-an-extension, and just have the server use the one and only ssl context manager statically?
There was a problem hiding this comment.
My other finding is that the methods that throw (create context) are only called from the tls transport socket, and from the quic transport socket (which depends at compiletime on the tls transport socket). So if you compile out the tls transport socket, the code that throws will never be called.
So I wonder if the interfaces can be refactored so that the fallback code doesn't need to implement that function at all. Really the only part that needs to be there is returning information about next cert to expire and similar, and that can already return an empty optional.
There was a problem hiding this comment.
if it's simply not working I'm fine just removing it. opened another PR up for that just in case they want to retcon at some point
|
force pushing reverting the main merge as the PR was rolled back |
Pull request was closed
#32748 was an attempt to move no-boring code to contrib. Per investigation at that time, the noboring build was already broken, so cleaning up that code entirely. If needed, #32748 can be reinstated instead. This also cleans up some unneeded DNS code. --------- Signed-off-by: Alyssa Wilk <[email protected]>
Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176