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.

Sign up for GitHub

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

PS-9581 fix: ALTER INSTANCE RELOAD TLS results in a very long execution (WIP) #5524

Draft
wants to merge 1 commit into
base: 8.0
Choose a base branch
from
Draft

PS-9581 fix: ALTER INSTANCE RELOAD TLS results in a very long execution (WIP) #5524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 17 additions & 21 deletions sql/auth/sql_authentication.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1623,11 +1623,11 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, const char *data,
protocol->add_client_capability(CAN_CLIENT_COMPRESS);

bool have_ssl = false;
if (current_thd->is_admin_connection() && g_admin_ssl_configured == true) {
Lock_and_access_ssl_acceptor_context context(mysql_admin);
have_ssl = context.have_ssl();
} else {
Lock_and_access_ssl_acceptor_context context(mysql_main);
{
Lock_and_access_ssl_acceptor_context_data context(
current_thd->is_admin_connection() && g_admin_ssl_configured
? mysql_admin
: mysql_main);
have_ssl = context.have_ssl();
}

Expand Down Expand Up @@ -2892,24 +2892,20 @@ static size_t parse_client_handshake_packet(THD *thd, MPVIO_EXT *mpvio,
We need to make sure that reference count for
SSL context is kept till the end of function
*/
std::unique_ptr<Lock_and_access_ssl_acceptor_context> context;
if (thd->is_admin_connection() && g_admin_ssl_configured == true)
context =
std::make_unique<Lock_and_access_ssl_acceptor_context>(mysql_admin);
else
context =
std::make_unique<Lock_and_access_ssl_acceptor_context>(mysql_main);
/* Do the SSL layering. */
if (!context.get()->have_ssl()) return packet_error;
DBUG_PRINT("info", ("IO layer change in progress..."));
if (sslaccept(*(context.get()), protocol->get_vio(),
protocol->get_net()->read_timeout, &errptr)) {
DBUG_PRINT("error", ("Failed to accept new SSL connection"));
return packet_error;
{
Lock_and_access_ssl_acceptor_context_data context(
thd->is_admin_connection() && g_admin_ssl_configured ? mysql_admin
: mysql_main);
/* Do the SSL layering. */
if (!context.have_ssl()) return packet_error;
DBUG_PRINT("info", ("IO layer change in progress..."));
if (sslaccept(context.get_vio_ssl_fd(), protocol->get_vio(),
protocol->get_net()->read_timeout, &errptr)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion int -> bool

Suggested change
protocol->get_net()->read_timeout, &errptr)) {
protocol->get_net()->read_timeout, &errptr) != 0) {

DBUG_PRINT("error", ("Failed to accept new SSL connection"));
return packet_error;
}
}

context.reset();

DBUG_PRINT("info", ("Reading user information over SSL layer"));
int rc = protocol->read_packet();
pkt_len = protocol->get_packet_length();
Expand Down
6 changes: 3 additions & 3 deletions sql/mysqld.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5732,7 +5732,7 @@ static int init_ssl() {
}

static int init_ssl_communication() {
if (TLS_channel::singleton_init(&mysql_main, mysql_main_channel, opt_use_ssl,
if (TLS_channel::singleton_init(mysql_main, mysql_main_channel, opt_use_ssl,
&server_main_callback, opt_initialize))
return 1;

Expand All @@ -5750,13 +5750,13 @@ static int init_ssl_communication() {
: false;

Ssl_init_callback_server_admin server_admin_callback;
if (TLS_channel::singleton_init(&mysql_admin, mysql_admin_channel,
if (TLS_channel::singleton_init(mysql_admin, mysql_admin_channel,
initialize_admin_tls, &server_admin_callback,
opt_initialize))
return 1;

if (initialize_admin_tls && !g_admin_ssl_configured) {
Lock_and_access_ssl_acceptor_context context(mysql_main);
Lock_and_access_ssl_acceptor_context_data context(mysql_main);
if (context.have_ssl())
LogErr(SYSTEM_LEVEL, ER_TLS_CONFIGURATION_REUSED,
mysql_admin_channel.c_str(), mysql_main_channel.c_str());
Expand Down
5 changes: 1 addition & 4 deletions sql/ssl_acceptor_context_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@
#include "sql/ssl_init_callback.h" /* Ssl_init_callback */
#include "violite.h" /* st_VioSSLFd, enum_ssl_init_error */

class Ssl_acceptor_context_container;
class TLS_channel;
class Lock_and_access_ssl_acceptor_context;

/**
Properties exposed by Ssl Acceptor context
Expand Down Expand Up @@ -213,9 +211,8 @@ class Ssl_acceptor_context_data final {
bool current_tls_session_cache_mode_;

/* F.R.I.E.N.D.S. */
friend class Ssl_acceptor_context_container;
friend class TLS_channel;
friend class Lock_and_access_ssl_acceptor_context;
friend class Lock_and_access_ssl_acceptor_context_data;
};

#endif // SSL_ACCEPTOR_CONTEXT_DATA_INCLUDED
4 changes: 2 additions & 2 deletions sql/ssl_acceptor_context_iterator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ class Ssl_acceptor_context_iterator {
public:
using Ssl_acceptor_context_iterator_data_container =
std::vector<Ssl_acceptor_context_iterator_data>;
Ssl_acceptor_context_iterator(Ssl_acceptor_context_container *context_type) {
Lock_and_access_ssl_acceptor_context context(context_type);
Ssl_acceptor_context_iterator(const Ssl_acceptor_context_data_ptr &data) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ google-explicit-constructor ⚠️
single-argument constructors must be marked explicit to avoid unintentional implicit conversions

Suggested change
Ssl_acceptor_context_iterator(const Ssl_acceptor_context_data_ptr &data) {
explicit Ssl_acceptor_context_iterator(const Ssl_acceptor_context_data_ptr &data) {

Lock_and_access_ssl_acceptor_context_data context(data);
const std::string channel_name = context.channel_name();
Ssl_acceptor_context_iterator_data first_one(
channel_name, "Enabled", context.have_ssl() ? "Yes" : "No");
Expand Down
105 changes: 38 additions & 67 deletions sql/ssl_acceptor_context_operator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,13 @@

#include "sql/ssl_acceptor_context_operator.h"

Ssl_acceptor_context_container *mysql_main;
Ssl_acceptor_context_container *mysql_admin;
Ssl_acceptor_context_data_ptr mysql_main;
Ssl_acceptor_context_data_ptr mysql_admin;

Ssl_acceptor_context_container::Ssl_acceptor_context_container(
Ssl_acceptor_context_data *data)
: lock_(nullptr) {
lock_ = new Ssl_acceptor_context_data_lock(data);
}

Ssl_acceptor_context_container ::~Ssl_acceptor_context_container() {
if (lock_ != nullptr) delete lock_;
lock_ = nullptr;
}

void Ssl_acceptor_context_container::switch_data(
Ssl_acceptor_context_data *new_data) {
if (lock_ != nullptr) lock_->write_wait_and_delete(new_data);
}

bool TLS_channel::singleton_init(Ssl_acceptor_context_container **out,
std::string channel, bool use_ssl_arg,
bool TLS_channel::singleton_init(Ssl_acceptor_context_data_ptr &out,
const std::string &channel, bool use_ssl_arg,
Ssl_init_callback *callbacks, bool db_init) {
if (out == nullptr || callbacks == nullptr) return true;
*out = nullptr;
if (callbacks == nullptr) return true;
/*
No need to take the ssl_ctx_lock lock here since it's being called
from singleton_init().
Expand All @@ -66,76 +49,64 @@ bool TLS_channel::singleton_init(Ssl_acceptor_context_container **out,

We don't hush the option since it would indicate a failure
in auto-generation, bad key material explicitly specified or
auto-generation disabled explcitly while SSL is still on.
auto-generation disabled explicitly while SSL is still on.
*/
Ssl_acceptor_context_data *news =
new Ssl_acceptor_context_data(channel, use_ssl_arg, callbacks);
Ssl_acceptor_context_container *new_container =
new Ssl_acceptor_context_container(news);
if (news == nullptr || new_container == nullptr) {
LogErr(WARNING_LEVEL, ER_SSL_LIBRARY_ERROR,
"Error initializing the SSL context system structure");
if (new_container) delete new_container;
return true;
}
// TODO: check if splitting control block / data block for this
// shared_ptr makes any difference
const auto new_data = std::make_shared<const Ssl_acceptor_context_data>(
channel, use_ssl_arg, callbacks);

if (news->have_ssl() && callbacks->warn_self_signed_ca()) {
/* This would delete Ssl_acceptor_context_data too */
delete new_container;
if (new_data->have_ssl() && callbacks->warn_self_signed_ca()) {
return true;
}

if (!db_init && news->have_ssl())
if (!db_init && new_data->have_ssl())
LogErr(SYSTEM_LEVEL, ER_TLS_CONFIGURED_FOR_CHANNEL, channel.c_str());

*out = new_container;
std::atomic_store(&out, new_data);
return false;
}

void TLS_channel::singleton_deinit(Ssl_acceptor_context_container *container) {
if (container == nullptr) return;
delete container;
void TLS_channel::singleton_deinit(Ssl_acceptor_context_data_ptr &data) {
Ssl_acceptor_context_data_ptr empty;
std::atomic_store(&data, empty);
}

void TLS_channel::singleton_flush(Ssl_acceptor_context_container *container,
std::string channel,
void TLS_channel::singleton_flush(Ssl_acceptor_context_data_ptr &data,
const std::string &channel,
Ssl_init_callback *callbacks,
enum enum_ssl_init_error *error, bool force) {
if (container == nullptr) return;
Ssl_acceptor_context_data *news =
new Ssl_acceptor_context_data(channel, true, callbacks, false, error);
// TODO: check if splitting control block / data block for this
// shared_ptr makes any difference
const auto new_data = std::make_shared<const Ssl_acceptor_context_data>(
channel, true, callbacks, false, error);
if (*error != SSL_INITERR_NOERROR && !force) {
delete news;
return;
}
(void)container->switch_data(news);
return;
std::atomic_store(&data, new_data);
}

std::string Lock_and_access_ssl_acceptor_context::show_property(
Ssl_acceptor_context_property_type property_type) {
const Ssl_acceptor_context_data *data = read_lock_;
return (data != nullptr ? data->show_property(property_type) : std::string{});
std::string Lock_and_access_ssl_acceptor_context_data::show_property(
Ssl_acceptor_context_property_type property_type) const {
return (data_ ? data_->show_property(property_type) : std::string{});
}

std::string Lock_and_access_ssl_acceptor_context::channel_name() {
const Ssl_acceptor_context_data *data = read_lock_;
return (data != nullptr ? data->channel_name() : std::string{});
std::string Lock_and_access_ssl_acceptor_context_data::channel_name() const {
return (data_ ? data_->channel_name() : std::string{});
}

bool Lock_and_access_ssl_acceptor_context::have_ssl() {
const Ssl_acceptor_context_data *data = read_lock_;
return (data != nullptr ? data->have_ssl() : false);
bool Lock_and_access_ssl_acceptor_context_data::have_ssl() const {
return (data_ ? data_->have_ssl() : false);
}

bool have_ssl() {
if (mysql_main != nullptr) {
Lock_and_access_ssl_acceptor_context context(mysql_main);
if (context.have_ssl()) return true;
{
Lock_and_access_ssl_acceptor_context_data context(mysql_main);
if (context.get() != nullptr && context.have_ssl()) return true;
}
if (mysql_admin != nullptr) {
Lock_and_access_ssl_acceptor_context context(mysql_admin);
if (context.have_ssl()) return true;
{
Lock_and_access_ssl_acceptor_context_data context(mysql_admin);
if (context.get() != nullptr && context.have_ssl()) return true;
}
return false;
}
Expand All @@ -145,7 +116,7 @@ static int show_long_status(SHOW_VAR *var, char *buff,
Ssl_acceptor_context_property_type property_type) {
std::string property;
if (mysql_main != nullptr) {
Lock_and_access_ssl_acceptor_context main(mysql_main);
Lock_and_access_ssl_acceptor_context_data main(mysql_main);
property = main.show_property(property_type);
}
var->type = SHOW_LONG;
Expand All @@ -159,7 +130,7 @@ static int show_char_status(SHOW_VAR *var, char *buff,
Ssl_acceptor_context_property_type property_type) {
std::string property;
if (mysql_main != nullptr) {
Lock_and_access_ssl_acceptor_context main(mysql_main);
Lock_and_access_ssl_acceptor_context_data main(mysql_main);
property = main.show_property(property_type);
}
var->type = SHOW_CHAR;
Expand Down
Loading