-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
tls: use custom instead of socket based bio #12911
Conversation
Signed-off-by: Florin Coras <fcoras@cisco.com>
Please mention issue #12899 in the PR description. I need to look at it more closely but it seems more or less right. It may be worth adding a separate header for BIO_new_io_handle |
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.
+1 to a separate header for BIO, can we get for coverage for https://storage.googleapis.com/envoy-pr/12911/coverage/source/extensions/transport_sockets/tls/io_handle_bio.cc.gcov.html ?
|
||
namespace { | ||
|
||
static inline Envoy::Network::IoHandle* bio_io_handle(BIO* bio) { |
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.
static doesn't do anything in unnamed namespace :)
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.
Thanks for that! Wanted to maintain the "typical" BIO formatting but even clang_tidy is complaining now. Is there any macro/comment that we could use to disable clang_tidy for this file? Do we want that or should I rename everything?
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.
I'm fine with keeping the naming, but not static
. You can add NOLINT or NOLINTNEXTLINE for each method.
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.
Thanks and done!
@@ -30,8 +30,14 @@ envoy_cc_extension( | |||
|
|||
envoy_cc_library( | |||
name = "ssl_socket_lib", | |||
srcs = ["ssl_socket.cc"], | |||
hdrs = ["ssl_socket.h"], | |||
srcs = [ |
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.
Can io_handle_bio.{cc,h} in a separate envoy_cc_library?
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.
Done!
Signed-off-by: Florin Coras <fcoras@cisco.com>
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.
Thanks for the cleanup. One less need for direct access to the fd.
And sorry for the delay in review, it has been a busy week.
// socket BIO which relies on access to a file descriptor. | ||
BIO* bio = BIO_new_socket(callbacks_->ioHandle().fdDoNotUse(), 0); | ||
// Use custom BIO that reads from/writes to IoHandle | ||
BIO* bio = BIO_new_io_handle(&callbacks_->ioHandle()); |
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.
Consider protecting with a runtime feature to allow for fast rollback if we find some problem despite it being relatively simple and as far I can tell, 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.
Okay, let me add that. But, since I'm not familiar with them, will switching be possible at runtime or does the code have to be recompiled with the feature moved from enabled to disabled list?
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.
My understanding is that it is controllable via reloadable config. The default is to use the new code path. Since this call only happens once per connection, during initialization of the transport socket object, it is safe for the runtime feature to change during runtime.
case BIO_C_GET_FD: | ||
ret = -1; | ||
*reinterpret_cast<void**>(ptr) = b->ptr; | ||
break; |
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.
Do we really need to implement BIO_C_SET_FD and BIO_C_GET_FD?
It may be good to add ASSERT(false) for cases that we do not expect to ever happen at the proxy. This includes: BIO_CTRL_RESET, BIO_C_FILE_SEEK, BIO_C_FILE_TELL, BIO_CTRL_INFO, BIO_C_GET_FD, BIO_C_SET_FD:
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.
I've used the socket bio as an example (since we're faking one) and those are the only control messages it implements. BIO_C_SET_FD is the one I've used for setting the ptr but I guess we could change that.
What would you prefer, keep following the socket model or implement things differently?
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.
Your constructor could set b->ptr directly instead of using BIO_C_SET_FD.
I think your implementation of iohandle BIOs is a great improvement, I'm just working my way through some nits.
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.
Ow. Yes we can, but we need to initialize all the other fields (shutdown, init, num). Should I do that explicitly in the constructor and then ASSERT(false) on BIO_C_SET_FD and BIO_C_GET_FD?
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.
Yes, we'ld need to also initiate other relevant fields like init also in the contructor.
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.
Perfect! Done!
const BIO_METHOD* BIO_s_io_handle(void); | ||
|
||
// NOLINTNEXTLINE(readability-identifier-naming) | ||
BIO* BIO_new_io_handle(Envoy::Network::IoHandle* io_handle); |
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.
Would it be possible to add some comments for this factory method?
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.
Done!
namespace Tls { | ||
|
||
// NOLINTNEXTLINE(readability-identifier-naming) | ||
const BIO_METHOD* BIO_s_io_handle(void); |
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.
This method seems to only be used in the cc file, should we remove it from the header?
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.
Good point. I exposed it for testing but ended up not using it.
auto result = bio_io_handle(b)->writev(&slice, 1); | ||
BIO_clear_retry_flags(b); | ||
if (!result.ok()) { | ||
if (result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again) { |
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.
We may also want to set retry in the case of Api::IoError::IoErrorCode::Interrupt.
Although it seems that writes returning Interrupt will result in Envoy closing the socket. I don't know how often a EINTR return would happen in practice.
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.
Interesting. I guess we could treat EINTR as EAGAIN here.
ret = bio_->method->ctrl(bio_, BIO_CTRL_GET_CLOSE, 0, nullptr); | ||
EXPECT_EQ(ret, 1); | ||
|
||
// avoid BIO_free assert in destructor |
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.
The bio_->init = 1 and bio_->shutdown = 1 further down seem to undo the work done by this BIO_CTRL_SET_CLOSE call.
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.
Removed this test. It was crashing only prior to adding the last one.
b->shutdown = int(num); | ||
break; | ||
case BIO_CTRL_FLUSH: | ||
ret = 1; |
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.
test for this case.
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.
I didn't add a test for this because it's exercised by the rest of the boringssl infra in other tests, but let me add a simple one for completion.
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.
Looks great, thanks for the improvements to the socket APIs.
Signed-off-by: Florin Coras <fcoras@cisco.com>
Thank you for the review and help with this! N.B. Pushed a fix for clang-tidy's complaints regarding the use of nullptr |
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.
Awesome, thanks, one quick high level comment.
/wait
@@ -81,6 +81,7 @@ constexpr const char* runtime_features[] = { | |||
"envoy.reloadable_features.preserve_upstream_date", | |||
"envoy.reloadable_features.stop_faking_paths", | |||
"envoy.reloadable_features.strict_1xx_and_204_response_headers", | |||
"envoy.reloadable_features.tls_use_io_handle_bio", |
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.
This should be documented somewhere. Can you add a release note for this and also update the PR description to mention it? This will help us track it for removal later.
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.
Done! Let me know if I missed something because this is my first time doing release notes updates.
Signed-off-by: Florin Coras <fcoras@cisco.com>
Signed-off-by: Florin Coras <fcoras@cisco.com>
Signed-off-by: Florin Coras <fcoras@cisco.com>
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.
Nice!
Signed-off-by: Florin Coras fcoras@cisco.com
Solves #12899
Risk Level: Medium
Testing: unit tests
Docs Changes: n/a
Release Notes: tls: switched from using socket BIOs to using custom BIOs that know how to interact with IoHandles. The feature can be disabled by setting runtime feature
envoy.reloadable_features.tls_use_io_handle_bio
to false.