Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| @@ -0,0 +1,47 @@ | |||
| #include "common/quic/client_connection_factory_impl.h" | |||
There was a problem hiding this comment.
Thanks for adding these tests!
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Thanks for the test coverage improvements.
Given that we are removing ASSERTs, I think we should add some nullptr checks to avoid crashes in EnvoyQuicProofVerifier::VerifyCertChain
| auto* quic_socket_factory = | ||
| dynamic_cast<QuicClientTransportSocketFactory*>(&transport_socket_factory); | ||
| ASSERT(quic_socket_factory != nullptr); | ||
| ASSERT(quic_socket_factory->sslCtx() != nullptr); |
There was a problem hiding this comment.
If this ASSERT were to fail the following code would crash. If we remove the ASSERT, we should also fix this code so it doesn't crash.
There was a problem hiding this comment.
This is already removed over here #16462
just hadn't merged it in.
That PR came with tests and error handling for SDS lazy loading of secrets :-)
There was a problem hiding this comment.
nit: Could we add ASSERT(context_ != nullptr) to the EnvoyQuicProofVerifier constructor as a sanity check? I'm fine with it being in this PR or a followup.
| unixSocketPeerCredentials() const override { | ||
| // Unix domain socket is not supported. | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| return absl::nullopt; |
There was a problem hiding this comment.
I'm having trouble finding non-test uses of unixSocketPeerCredentials().
What am I missing? Who depends on this API?
There was a problem hiding this comment.
Not sure. I'd be happy to remove as a follow-up if we think it's doable.
I wonder if it's used internally by whoever @snowp worked for at the time?
|
This has broken four CI pipelines... better to revert for now? |
|
#16597 should fix |
Risk Level: n/a (test only) Testing: yes Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
4 participants
Arguably I should adjust this up but given recent CI failures I'll leave for now.
Risk Level: n/a (test only)
Testing: yes
Docs Changes: n/a
Release Notes: n/a