From 1e539fa43e18c3e4179a47fef573742d163b6a35 Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 5 Apr 2023 16:37:04 +0100 Subject: [PATCH 1/2] admin: skip controller writes for no-op user actions Clients should also avoid issuing these requests to begin with, but in case of bad behavior, let's avoid generating unnecessary writes to the controller log. (cherry picked from commit b88f4a05be5b4e4d1ef23ccea1e5452542af095e) --- src/v/redpanda/admin_server.cc | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/v/redpanda/admin_server.cc b/src/v/redpanda/admin_server.cc index c114d9b1b829..f763b4eb6148 100644 --- a/src/v/redpanda/admin_server.cc +++ b/src/v/redpanda/admin_server.cc @@ -1666,6 +1666,18 @@ static bool match_scram_credential( } } +bool is_no_op_user_write( + security::credential_store& store, + security::credential_user username, + security::scram_credential credential) { + auto user_opt = store.get(username); + if (user_opt.has_value()) { + return user_opt.value() == credential; + } else { + return false; + } +} + ss::future admin_server::create_user_handler(std::unique_ptr req) { auto doc = parse_json_body(*req); @@ -1679,6 +1691,15 @@ admin_server::create_user_handler(std::unique_ptr req) { auto username = security::credential_user(doc["username"].GetString()); + if (is_no_op_user_write( + _controller->get_credential_store().local(), username, credential)) { + vlog( + logger.debug, + "User {} already exists with matching credential", + username); + co_return ss::json::json_return_type(ss::json::json_void()); + } + auto err = co_await _controller->get_security_frontend().local().create_user( username, credential, model::timeout_clock::now() + 5s); @@ -1705,6 +1726,11 @@ ss::future admin_server::delete_user_handler(std::unique_ptr req) { auto user = security::credential_user(req->param["user"]); + if (!_controller->get_credential_store().local().contains(user)) { + vlog(logger.debug, "User '{}' already gone during deletion", user); + co_return ss::json::json_return_type(ss::json::json_void()); + } + auto err = co_await _controller->get_security_frontend().local().delete_user( user, model::timeout_clock::now() + 5s); @@ -1725,6 +1751,15 @@ admin_server::update_user_handler(std::unique_ptr req) { auto credential = parse_scram_credential(doc); + if (is_no_op_user_write( + _controller->get_credential_store().local(), user, credential)) { + vlog( + logger.debug, + "User {} already exists with matching credential", + user); + co_return ss::json::json_return_type(ss::json::json_void()); + } + auto err = co_await _controller->get_security_frontend().local().update_user( user, credential, model::timeout_clock::now() + 5s); From 75a2fd96f4575029cfdc70d2f61f6879fdd750ec Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 5 Apr 2023 16:41:52 +0100 Subject: [PATCH 2/2] admin: redirect user writes to controller leader In order that our checks for no-op writes are correctly ordered, we must execute these on the controller leader. Otherwise we might incorrectly drop a write on another node because its credential store is out of date. (cherry picked from commit f47fb81b2c2768f75b67e3c8c2380aff4216390a) --- src/v/redpanda/admin_server.cc | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/v/redpanda/admin_server.cc b/src/v/redpanda/admin_server.cc index f763b4eb6148..b28e68d6c9d5 100644 --- a/src/v/redpanda/admin_server.cc +++ b/src/v/redpanda/admin_server.cc @@ -1680,6 +1680,12 @@ bool is_no_op_user_write( ss::future admin_server::create_user_handler(std::unique_ptr req) { + if (need_redirect_to_leader(model::controller_ntp, _metadata_cache)) { + // In order that we can do a reliably ordered validation of + // the request (and drop no-op requests), run on controller leader; + throw co_await redirect_to_leader(*req, model::controller_ntp); + } + auto doc = parse_json_body(*req); auto credential = parse_scram_credential(doc); @@ -1724,6 +1730,12 @@ admin_server::create_user_handler(std::unique_ptr req) { ss::future admin_server::delete_user_handler(std::unique_ptr req) { + if (need_redirect_to_leader(model::controller_ntp, _metadata_cache)) { + // In order that we can do a reliably ordered validation of + // the request (and drop no-op requests), run on controller leader; + throw co_await redirect_to_leader(*req, model::controller_ntp); + } + auto user = security::credential_user(req->param["user"]); if (!_controller->get_credential_store().local().contains(user)) { @@ -1745,6 +1757,12 @@ admin_server::delete_user_handler(std::unique_ptr req) { ss::future admin_server::update_user_handler(std::unique_ptr req) { + if (need_redirect_to_leader(model::controller_ntp, _metadata_cache)) { + // In order that we can do a reliably ordered validation of + // the request (and drop no-op requests), run on controller leader; + throw co_await redirect_to_leader(*req, model::controller_ntp); + } + auto user = security::credential_user(req->param["user"]); auto doc = parse_json_body(*req);