This is an automated email from the ASF dual-hosted git repository. asekretenko pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 3fcc148d5d28405f4256e4cfa7005786de10b4f9 Author: Andrei Sekretenko <[email protected]> AuthorDate: Mon Oct 12 20:47:01 2020 +0200 Consolidated creation and validation of `allocator::Framework` options. This merges three near-identical pieces of scattered code in SUBSCRIBE and UPDATE_FRAMEWORK execution paths in the Master that validate and construct parts of `allocator::FrameworkOptions` (the set of suppressed roles and the offer constraints filter) into a single function. This is a prerequisite to adding validation of offer constraint roles. Review: https://reviews.apache.org/r/72955 --- src/master/master.cpp | 172 +++++++++++++++++++++++----------------------- src/master/master.hpp | 13 ++-- src/master/validation.cpp | 20 ++++++ src/master/validation.hpp | 4 ++ 4 files changed, 118 insertions(+), 91 deletions(-) diff --git a/src/master/master.cpp b/src/master/master.cpp index 6c0523d..531b971 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -2551,8 +2551,7 @@ void Master::reregisterFramework( Option<Error> Master::validateFramework( - const FrameworkInfo& frameworkInfo, - const google::protobuf::RepeatedPtrField<std::string>& suppressedRoles) const + const FrameworkInfo& frameworkInfo) const { Option<Error> validationError = validation::framework::validate(frameworkInfo); @@ -2583,17 +2582,6 @@ Option<Error> Master::validateFramework( " are not present in the master's --roles"); } - // Ensure each of the suppressed role is contained in the list of roles. - set<string> frameworkRoles = protobuf::framework::getRoles(frameworkInfo); - // The suppressed roles must be contained within the list of all - // roles for the framwork. - foreach (const string& role, suppressedRoles) { - if (!frameworkRoles.count(role)) { - return Error("Suppressed role '" + role + - "' is not contained in the list of roles"); - } - } - // TODO(vinod): Deprecate this in favor of authorization. if (frameworkInfo.user() == "root" && !flags.root_submissions) { return Error("User 'root' is not allowed to run frameworks" @@ -2614,6 +2602,37 @@ Option<Error> Master::validateFramework( } +static Try<allocator::FrameworkOptions> createAllocatorFrameworkOptions( + const set<string>& validFrameworkRoles, + const OfferConstraintsFilter::Options filterOptions, + google::protobuf::RepeatedPtrField<std::string>&& suppressedRoles, + OfferConstraints offerConstraints) +{ + set<string> suppressedRolesSet( + make_move_iterator(suppressedRoles.begin()), + make_move_iterator(suppressedRoles.end())); + + Option<Error> error = validation::framework::validateSuppressedRoles( + validFrameworkRoles, suppressedRolesSet); + + if (error.isSome()) { + return *error; + } + + // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). + Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( + filterOptions, std::move(offerConstraints)); + + if (filter.isError()) { + return Error( + "Offer constraints are not valid: " + std::move(filter.error())); + } + + return allocator::FrameworkOptions{ + std::move(suppressedRolesSet), std::move(*filter)}; +} + + // Returns None if the framework object approvers are ready and the scheduler // trying to SUBSCRIBE is authorized to do so with provided framework info. // Otherwise, returns an error to be sent to the scheduler trying to subscribe. @@ -2665,40 +2684,34 @@ void Master::subscribe( LOG(INFO) << "Received subscription request for" << " HTTP framework '" << frameworkInfo.name() << "'"; - Option<Error> validationError = - validateFramework(frameworkInfo, subscribe.suppressed_roles()); - - allocator::FrameworkOptions allocatorOptions; - - // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). - if (validationError.isNone()) { - Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( - offerConstraintsFilterOptions, - OfferConstraints(subscribe.offer_constraints())); - - if (filter.isError()) { - validationError = Error(std::move(filter.error())); - } else { - allocatorOptions.offerConstraintsFilter = std::move(*filter); - } - } - - if (validationError.isSome()) { + auto refuseSubscription = [&](const string& error) { LOG(INFO) << "Refusing subscription of framework" - << " '" << frameworkInfo.name() << "': " - << validationError->message; + << " '" << frameworkInfo.name() << "': " << error; FrameworkErrorMessage message; - message.set_message(validationError->message); + message.set_message(error); http.send(message); http.close(); + }; + + const Option<Error> validationError = validateFramework(frameworkInfo); + if (validationError.isSome()) { + refuseSubscription(validationError->message); return; } - allocatorOptions.suppressedRoles = set<string>( - make_move_iterator(subscribe.mutable_suppressed_roles()->begin()), - make_move_iterator(subscribe.mutable_suppressed_roles()->end())); + Try<allocator::FrameworkOptions> allocatorOptions = + createAllocatorFrameworkOptions( + protobuf::framework::getRoles(frameworkInfo), + offerConstraintsFilterOptions, + std::move(*subscribe.mutable_suppressed_roles()), + subscribe.offer_constraints()); + + if (allocatorOptions.isError()) { + refuseSubscription(allocatorOptions.error()); + return; + } // Need to disambiguate for the compiler. void (Master::*_subscribe)( @@ -2719,7 +2732,7 @@ void Master::subscribe( std::move(frameworkInfo), std::move(*subscribe.mutable_offer_constraints()), subscribe.force(), - std::move(allocatorOptions), + std::move(*allocatorOptions), lambda::_1)); } @@ -2918,37 +2931,37 @@ void Master::subscribe( return; } - Option<Error> validationError = - validateFramework(frameworkInfo, subscribe.suppressed_roles()); + auto refuseSubscription = [&](const string& error) { + LOG(INFO) << "Refusing subscription of framework" + << " '" << frameworkInfo.name() << "' at " << from << ": " + << error; + + FrameworkErrorMessage message; + message.set_message(error); + send(from, message); + }; + + Option<Error> validationError = validateFramework(frameworkInfo); // Note that re-authentication errors are already handled above. if (validationError.isNone()) { validationError = validateFrameworkAuthentication(frameworkInfo, from); } - allocator::FrameworkOptions allocatorOptions; - - // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). - if (validationError.isNone()) { - Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( - offerConstraintsFilterOptions, - OfferConstraints(subscribe.offer_constraints())); - - if (filter.isError()) { - validationError = Error(std::move(filter.error())); - } else { - allocatorOptions.offerConstraintsFilter = std::move(*filter); - } + if (validationError.isSome()) { + refuseSubscription(validationError->message); + return; } - if (validationError.isSome()) { - LOG(INFO) << "Refusing subscription of framework" - << " '" << frameworkInfo.name() << "' at " << from << ": " - << validationError->message; + Try<allocator::FrameworkOptions> allocatorOptions = + createAllocatorFrameworkOptions( + protobuf::framework::getRoles(frameworkInfo), + offerConstraintsFilterOptions, + std::move(*subscribe.mutable_suppressed_roles()), + subscribe.offer_constraints()); - FrameworkErrorMessage message; - message.set_message(validationError->message); - send(from, message); + if (allocatorOptions.isError()) { + refuseSubscription(allocatorOptions.error()); return; } @@ -2968,10 +2981,6 @@ void Master::subscribe( frameworkInfo.set_principal(authenticated[from]); } - allocatorOptions.suppressedRoles = set<string>( - make_move_iterator(subscribe.mutable_suppressed_roles()->begin()), - make_move_iterator(subscribe.mutable_suppressed_roles()->end())); - // Need to disambiguate for the compiler. void (Master::*_subscribe)( const UPID&, @@ -2991,7 +3000,7 @@ void Master::subscribe( std::move(frameworkInfo), std::move(*subscribe.mutable_offer_constraints()), subscribe.force(), - std::move(allocatorOptions), + std::move(*allocatorOptions), lambda::_1)); } @@ -3262,8 +3271,7 @@ Future<process::http::Response> Master::updateFramework( LOG(INFO) << "Processing UPDATE_FRAMEWORK call for framework " << call.framework_info().id(); - Option<Error> error = - validateFramework(call.framework_info(), call.suppressed_roles()); + Option<Error> error = validateFramework(call.framework_info()); if (error.isSome()) { return process::http::BadRequest( @@ -3281,19 +3289,17 @@ Future<process::http::Response> Master::updateFramework( const bool frameworkInfoChanged = !typeutils::equivalent(framework->info, call.framework_info()); - allocator::FrameworkOptions allocatorOptions; - // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). - Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( - offerConstraintsFilterOptions, - OfferConstraints(call.offer_constraints())); - - if (filter.isError()) { - return process::http::BadRequest( - "'UpdateFramework.offer_constraints' are not valid: " + - filter.error()); - } + Try<allocator::FrameworkOptions> allocatorOptions = + createAllocatorFrameworkOptions( + protobuf::framework::getRoles(call.framework_info()), + offerConstraintsFilterOptions, + std::move(*call.mutable_suppressed_roles()), + call.offer_constraints()); - allocatorOptions.offerConstraintsFilter = std::move(*filter); + if (allocatorOptions.isError()) { + return process::http::BadRequest( + "'UpdateFramework' call is not valid: " + allocatorOptions.error()); + } ActionObject actionObject = ActionObject::frameworkRegistration(call.framework_info()); @@ -3312,15 +3318,11 @@ Future<process::http::Response> Master::updateFramework( "Not authorized to " + stringify(actionObject)); } - allocatorOptions.suppressedRoles = set<string>( - make_move_iterator(call.mutable_suppressed_roles()->begin()), - make_move_iterator(call.mutable_suppressed_roles()->end())); - updateFramework( framework, call.framework_info(), std::move(*call.mutable_offer_constraints()), - std::move(allocatorOptions)); + std::move(*allocatorOptions)); if (frameworkInfoChanged) { // NOTE: Among the framework properties that can be changed by this call diff --git a/src/master/master.hpp b/src/master/master.hpp index 83d8190..c200c32 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -1058,12 +1058,13 @@ private: const std::string role; }; - // Performs validations of the FrameworkInfo and suppressed roles set - // which do not depend on the current state of this framework. - Option<Error> validateFramework( - const FrameworkInfo& frameworkInfo, - const google::protobuf::RepeatedPtrField<std::string>& suppressedRoles) - const; + // Performs stateless and stateful validation of the FrameworkInfo. + // The stateful validation only uses the master flags and whether + // the framework is completed. + + // TODO(asekretenko): Pass in the state explicitly to move this function into + // validation.hpp for unit testability and better separation of concerns. + Option<Error> validateFramework(const FrameworkInfo& frameworkInfo) const; /** * Inner class used to namespace the handling of quota requests. diff --git a/src/master/validation.cpp b/src/master/validation.cpp index feeea8e..aafffd5 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -37,6 +37,7 @@ #include <stout/hashset.hpp> #include <stout/lambda.hpp> #include <stout/none.hpp> +#include <stout/set.hpp> #include <stout/stringify.hpp> #include "checks/checker.hpp" @@ -677,6 +678,25 @@ void preserveImmutableFields( } +Option<Error> validateSuppressedRoles( + const set<string>& validFrameworkRoles, + const set<string>& suppressedRoles) +{ + // Needed to prevent shadowing of the template '::operator-<std::set<T>>' + // by a non-template '::mesos::operator-' + using ::operator-; + + set<string> invalidSuppressedRoles = suppressedRoles - validFrameworkRoles; + if (!invalidSuppressedRoles.empty()) { + return Error( + "Suppressed roles " + stringify(invalidSuppressedRoles) + + " are not contained in the set of roles"); + } + + return None(); +} + + } // namespace framework { diff --git a/src/master/validation.hpp b/src/master/validation.hpp index 7fe8f08..6239492 100644 --- a/src/master/validation.hpp +++ b/src/master/validation.hpp @@ -130,6 +130,10 @@ void preserveImmutableFields( const FrameworkInfo& oldInfo, FrameworkInfo* newInfo); +Option<Error> validateSuppressedRoles( + const std::set<std::string>& validFrameworkRoles, + const std::set<std::string>& suppressedRoles); + } // namespace framework {
