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 c1e716054d8dead61074c8619ffa7b33f3064152 Author: Andrei Sekretenko <asekrete...@apache.org> AuthorDate: Mon Oct 12 22:06:51 2020 +0200 Added validation that offer constraints are set only for existing roles. This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls validate that the framework does not specify offer constraints for roles to which it is not going to be subscribed. Review: https://reviews.apache.org/r/72956 --- src/master/master.cpp | 8 ++- src/master/validation.cpp | 19 +++++++ src/master/validation.hpp | 4 ++ src/tests/master/update_framework_tests.cpp | 87 +++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/master/master.cpp b/src/master/master.cpp index 531b971..164720a 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -2619,7 +2619,13 @@ static Try<allocator::FrameworkOptions> createAllocatorFrameworkOptions( return *error; } - // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). + error = validation::framework::validateOfferConstraintsRoles( + validFrameworkRoles, offerConstraints); + + if (error.isSome()) { + return *error; + } + Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( filterOptions, std::move(offerConstraints)); diff --git a/src/master/validation.cpp b/src/master/validation.cpp index aafffd5..6bdab54 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -59,6 +59,8 @@ using std::vector; using google::protobuf::RepeatedPtrField; +using mesos::scheduler::OfferConstraints; + namespace mesos { namespace internal { namespace master { @@ -697,6 +699,23 @@ Option<Error> validateSuppressedRoles( } +Option<Error> validateOfferConstraintsRoles( + const set<string>& validFrameworkRoles, + const OfferConstraints& offerConstraints) +{ + for (const auto& pair : offerConstraints.role_constraints()) { + const string& role = pair.first; + if (validFrameworkRoles.count(role) < 1) { + return Error( + "Offer constraints specify `role_constraints` for a role '" + role + + "'not contained in the set of roles"); + } + } + + return None(); +} + + } // namespace framework { diff --git a/src/master/validation.hpp b/src/master/validation.hpp index 6239492..17652e3 100644 --- a/src/master/validation.hpp +++ b/src/master/validation.hpp @@ -134,6 +134,10 @@ Option<Error> validateSuppressedRoles( const std::set<std::string>& validFrameworkRoles, const std::set<std::string>& suppressedRoles); +Option<Error> validateOfferConstraintsRoles( + const std::set<std::string>& validFrameworkRoles, + const scheduler::OfferConstraints& offerConstraints); + } // namespace framework { diff --git a/src/tests/master/update_framework_tests.cpp b/src/tests/master/update_framework_tests.cpp index 3f86573..ce5a51c 100644 --- a/src/tests/master/update_framework_tests.cpp +++ b/src/tests/master/update_framework_tests.cpp @@ -981,6 +981,93 @@ TEST_F(UpdateFrameworkTest, OfferConstraints) } +// This test ensures that an UPDATE_FRAMEWORK call trying to set offer +// constraints for a role to which the framework will not be subscribed +// fails validation and is rejected as a whole. +TEST_F(UpdateFrameworkTest, OfferConstraintsForUnsubscribedRole) +{ + using ::mesos::v1::scheduler::OfferConstraints; + + const Try<JSON::Object> constraintsJson = JSON::parse<JSON::Object>( + R"~( + { + "role_constraints": { + ")~" + DEFAULT_FRAMEWORK_INFO.roles(0) + R"~(": { + "groups": [{ + "attribute_constraints": [{ + "selector": {"attribute_name": "foo"}, + "predicate": {"exists": {}} + }] + }] + } + } + })~"); + + ASSERT_SOME(constraintsJson); + + const Try<OfferConstraints> constraints = + ::protobuf::parse<OfferConstraints>(*constraintsJson); + + ASSERT_SOME(constraints); + + Try<Owned<cluster::Master>> master = StartMaster(CreateMasterFlags()); + ASSERT_SOME(master); + + auto scheduler = std::make_shared<MockHTTPScheduler>(); + + Future<Nothing> connected; + EXPECT_CALL(*scheduler, connected(_)) + .WillOnce(Invoke([constraints](Mesos* mesos) { + Call call; + call.set_type(Call::SUBSCRIBE); + *call.mutable_subscribe()->mutable_framework_info() = + DEFAULT_FRAMEWORK_INFO; + + *call.mutable_subscribe()->mutable_offer_constraints() = *constraints; + + mesos->send(call); + })); + + EXPECT_CALL(*scheduler, heartbeat(_)) + .WillRepeatedly(Return()); // Ignore heartbeats. + + Future<Event::Subscribed> subscribed; + + EXPECT_CALL(*scheduler, subscribed(_, _)) + .WillOnce(FutureArg<1>(&subscribed)); + + TestMesos mesos(master->get()->pid, ContentType::PROTOBUF, scheduler); + + AWAIT_READY(subscribed); + + // Try to change framework's role but specify constraints for the old role. + FrameworkInfo frameworkInfoWithNewRole = DEFAULT_FRAMEWORK_INFO; + frameworkInfoWithNewRole.clear_roles(); + frameworkInfoWithNewRole.add_roles(DEFAULT_FRAMEWORK_INFO.roles(0) + "_new"); + *frameworkInfoWithNewRole.mutable_id() = subscribed->framework_id(); + + Future<APIResult> result = + callUpdateFramework(&mesos, frameworkInfoWithNewRole, {}, *constraints); + + AWAIT_READY(result); + EXPECT_EQ(result->status_code(), 400u); + EXPECT_TRUE(strings::contains(result->error(), "Offer constraints")); + + Future<v1::master::Response::GetFrameworks> frameworks = + getFrameworks(master->get()->pid); + + AWAIT_READY(frameworks); + + ASSERT_EQ(frameworks->frameworks_size(), 1); + + // The framework info should have remained the same, despite + // `updatedFrameworkInfo` on its own not containing any invalid updates. + FrameworkInfo expected = DEFAULT_FRAMEWORK_INFO; + *expected.mutable_id() = subscribed->framework_id(); + EXPECT_NONE(::mesos::v1::typeutils::diff( + frameworks->frameworks(0).framework_info(), expected)); +} + } // namespace scheduler { } // namespace v1 {