This is an automated email from the ASF dual-hosted git repository. bennoe pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 09c830d87b88d4c2f386cb9ded5931528d6cf144 Author: Benjamin Bannier <[email protected]> AuthorDate: Fri Nov 8 14:19:16 2019 +0100 Added authorization handling for reservations with `source`. This patch adds authorization handling for `RESERVE` operations containing `source` fields. In order to stay backwards-compatible we add a dedicated authorization branch for such operations which under the hood translates each removed reservation to an `UNRESERVE` operation and every added reservation as a `RESERVE` operation where we fall back to existing authorization code for authorization. Review: https://reviews.apache.org/r/71729/ --- src/master/master.cpp | 110 ++++++++++++++----- src/tests/master_authorization_tests.cpp | 177 +++++++++++++++++++++++++++++++ 2 files changed, 259 insertions(+), 28 deletions(-) diff --git a/src/master/master.cpp b/src/master/master.cpp index e7609f3..14b90a5 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -3765,42 +3765,96 @@ Future<bool> Master::authorizeReserveResources( request.mutable_subject()->CopyFrom(subject.get()); } - // The operation will be authorized if the entity is allowed to make - // reservations for all roles included in `reserve.resources`. - // Add an element to `request.roles` for each unique role in the resources. - hashset<string> roles; vector<Future<bool>> authorizations; - foreach (const Resource& resource, reserve.resources()) { - // NOTE: We rely on the master to ensure that the resource is in the - // post-reservation-refinement format. If there is a stack of reservations, - // we perform authorization for the role of the most refined reservation, - // since we only support "pushing" one reservation at a time. That is, all - // of the previous reservations must have already been authorized. - // - // NOTE: If there is no reservation, we authorize the resource with the - // default role '*' for backward compatibility. - CHECK(!resource.has_role()) << resource; - CHECK(!resource.has_reservation()) << resource; - const string role = Resources::isReserved(resource) - ? Resources::reservationRole(resource) : "*"; + // We support `Reserve` operations with either `source` set or unset. If + // `source` is unset (an older API), it is possible to send calls which + // contain also resources whose reservations are unmodified; in that case all + // `Reserve` `resources` will be authorized. In the case where `source` is set + // we follow a narrower contract and e.g., only accept resources whose + // reservations are all modified, and require identical modifications for all + // passed `Resource`s. + // + // This e.g., means that we cannot upgrade calls without `source` to + // calls with `source` and use uniform handling. Instead we branch + // on whether `source` was set to select the authorization handling. + // Each fundamental reservation added in with with-`source` case can + // then be authorized using the historic, wider contract. + if (reserve.source().empty()) { + // The operation will be authorized if the entity is allowed to make + // reservations for all roles included in `reserve.resources`. + // Add an element to `request.roles` for each unique role in the resources. + hashset<string> roles; + + foreach (const Resource& resource, reserve.resources()) { + // NOTE: We rely on the master to ensure that the resource is in the + // post-reservation-refinement format. If there is a stack of + // reservations, we perform authorization for the role of the most refined + // reservation, since we only support "pushing" one reservation at a time. + // That is, all of the previous reservations must have already been + // authorized. + // + // NOTE: If there is no reservation, we authorize the resource with the + // default role '*' for backward compatibility. + CHECK(!resource.has_role()) << resource; + CHECK(!resource.has_reservation()) << resource; - if (!roles.contains(role)) { - roles.insert(role); + const string role = Resources::isReserved(resource) + ? Resources::reservationRole(resource) : "*"; - request.mutable_object()->mutable_resource()->CopyFrom(resource); + if (!roles.contains(role)) { + roles.insert(role); - // We also set the deprecated `object.value` field to support legacy - // authorizers that have not been upgraded to look at `object.resource`. - request.mutable_object()->set_value(role); + request.mutable_object()->mutable_resource()->CopyFrom(resource); - authorizations.push_back(authorizer.get()->authorized(request)); + // We also set the deprecated `object.value` field to support legacy + // authorizers that have not been upgraded to look at `object.resource`. + request.mutable_object()->set_value(role); + + authorizations.push_back(authorizer.get()->authorized(request)); + } } - } - LOG(INFO) << "Authorizing principal '" - << (principal.isSome() ? stringify(principal.get()) : "ANY") - << "' to reserve resources '" << reserve.resources() << "'"; + LOG(INFO) << "Authorizing principal '" + << (principal.isSome() ? stringify(principal.get()) : "ANY") + << "' to reserve resources '" << reserve.resources() << "'"; + } else { + Resources source = reserve.source(); + const Resources target = reserve.resources(); + const Resources ancestor = + Resources::getReservationAncestor(source, target); + + // Authorize `UNRESERVE` operations bringing `source` to `ancestor`. + while (source != ancestor) { + Offer::Operation::Unreserve unreserve; + unreserve.mutable_resources()->CopyFrom(source); + + authorizations.push_back( + authorizeUnreserveResources(unreserve, principal)); + + source = source.popReservation(); + } + + // Authorize `RESERVE` operations bringing `ancestor` to `target`. + const RepeatedPtrField<Resource::ReservationInfo>& targetReservations = + reserve.resources(0).reservations(); + const RepeatedPtrField<Resource::ReservationInfo>& ancestorReservations = + RepeatedPtrField<Resource>(ancestor).begin()->reservations(); + + // Skip reservations common among `source` and `resources`. + auto it = targetReservations.begin(); + std::advance(it, ancestorReservations.size()); + + for (; it != targetReservations.end(); ++it) { + source = source.pushReservation(*it); + + // We do not set `source` here to trigger the previous branch. + Offer::Operation::Reserve reserve; + reserve.mutable_resources()->CopyFrom(source); + + authorizations.push_back(authorizeReserveResources(reserve, principal)); + } + } // NOTE: Empty authorizations are not valid and are checked by a validator. // However under certain circumstances, this method can be called before diff --git a/src/tests/master_authorization_tests.cpp b/src/tests/master_authorization_tests.cpp index 06471aa..bc8155b 100644 --- a/src/tests/master_authorization_tests.cpp +++ b/src/tests/master_authorization_tests.cpp @@ -76,6 +76,8 @@ using mesos::internal::master::allocator::MesosAllocatorProcess; using mesos::internal::slave::Slave; +using mesos::v1::master::Call; + using mesos::master::detector::MasterDetector; using mesos::master::detector::StandaloneMasterDetector; @@ -86,6 +88,7 @@ using process::Owned; using process::PID; using process::Promise; +using process::http::Accepted; using process::http::Forbidden; using process::http::OK; using process::http::Response; @@ -2764,6 +2767,180 @@ TEST_F(MasterAuthorizationTest, AuthorizedToRegisterNoStaticReservations) AWAIT_READY(slaveRegisteredMessage); } +// This test verifies that when reserving resources the correct +// `RESERVE_RESOURCES` or `UNRESERVE_RESOURCES` authorization +// requests are performed. +TEST_F(MasterAuthorizationTest, ReserveResources) +{ + Clock::pause(); + + MockAuthorizer authorizer; + + Try<Owned<cluster::Master>> master = StartMaster(&authorizer); + ASSERT_SOME(master); + + Future<SlaveRegisteredMessage> slaveRegisteredMessage = + FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _); + + Owned<MasterDetector> detector = master.get()->createDetector(); + slave::Flags slaveFlags = CreateSlaveFlags(); + Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags); + ASSERT_SOME(slave); + + Clock::advance(slaveFlags.registration_backoff_factor); + AWAIT_READY(slaveRegisteredMessage); + + const SlaveID& slaveId = slaveRegisteredMessage->slave_id(); + + // Reserve `cpus:0.1` for `foo`. This should trigger exactly one authorization + // request for `RESERVE_RESOURCES`. This is a base case. + { + Future<authorization::Request> request; + EXPECT_CALL(authorizer, authorized(_)) + .WillOnce(DoAll(FutureArg<0>(&request), Return(true))); + + Call call; + call.set_type(Call::RESERVE_RESOURCES); + Call::ReserveResources* reserveResources = call.mutable_reserve_resources(); + + reserveResources->mutable_agent_id()->set_value(slaveId.value()); + + v1::Resource resource = *v1::Resources::parse("cpus", "0.1", "*"); + *reserveResources->add_source() = resource; + + *resource.add_reservations() = + v1::createDynamicReservationInfo("foo", DEFAULT_CREDENTIAL.principal()); + *reserveResources->add_resources() = resource; + + Future<http::Response> response = http::post( + master.get()->pid, + "/api/v1", + createBasicAuthHeaders(DEFAULT_CREDENTIAL), + stringify(JSON::protobuf(call)), + stringify(ContentType::JSON)); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(Accepted().status, response); + + AWAIT_READY(request); + ASSERT_EQ(authorization::Action::RESERVE_RESOURCES, request->action()); + ASSERT_EQ(1, request->object().resource().reservations_size()); + ASSERT_EQ("foo", request->object().resource().reservations(0).role()); + } + + // Reserve `cpus:0.1` for `foo` and refine to `foo/bar`. This should trigger + // two authorization requests, one for the reservation to `foo` and another + // one for the reservation refinement to `foo/bar`. + { + Future<authorization::Request> request1; + Future<authorization::Request> request2; + EXPECT_CALL(authorizer, authorized(_)) + .WillOnce(DoAll(FutureArg<0>(&request1), Return(true))) + .WillOnce(DoAll(FutureArg<0>(&request2), Return(true))); + + Call call; + call.set_type(Call::RESERVE_RESOURCES); + Call::ReserveResources* reserveResources = call.mutable_reserve_resources(); + + reserveResources->mutable_agent_id()->set_value(slaveId.value()); + + v1::Resource resource = *v1::Resources::parse("cpus", "0.1", "*"); + *reserveResources->add_source() = resource; + + *resource.add_reservations() = + v1::createDynamicReservationInfo("foo", DEFAULT_CREDENTIAL.principal()); + *resource.add_reservations() = v1::createDynamicReservationInfo( + "foo/bar", DEFAULT_CREDENTIAL.principal()); + *reserveResources->add_resources() = resource; + + Future<http::Response> response = http::post( + master.get()->pid, + "/api/v1", + createBasicAuthHeaders(DEFAULT_CREDENTIAL), + stringify(JSON::protobuf(call)), + stringify(ContentType::JSON)); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(Accepted().status, response); + + AWAIT_READY(request1); + ASSERT_EQ(authorization::Action::RESERVE_RESOURCES, request1->action()); + ASSERT_EQ(1, request1->object().resource().reservations_size()); + ASSERT_EQ("foo", request1->object().resource().reservations(0).role()); + + AWAIT_READY(request2); + ASSERT_EQ(authorization::Action::RESERVE_RESOURCES, request2->action()); + ASSERT_EQ(2, request2->object().resource().reservations_size()); + ASSERT_EQ("foo", request2->object().resource().reservations(0).role()); + ASSERT_EQ("foo/bar", request2->object().resource().reservations(1).role()); + } + + // Re-reserve the `cpus:0.1` reserved above for `foo` and refined to `foo/bar` + // to `baz` and refined to `baz/bar`. This will trigger two authorization + // requests to unreserved the resource in the `foo` hierarchy, and two + // authorization request for the reservations in the `baz` hierarchy. + { + Future<authorization::Request> request1; + Future<authorization::Request> request2; + Future<authorization::Request> request3; + Future<authorization::Request> request4; + EXPECT_CALL(authorizer, authorized(_)) + .WillOnce(DoAll(FutureArg<0>(&request1), Return(true))) + .WillOnce(DoAll(FutureArg<0>(&request2), Return(true))) + .WillOnce(DoAll(FutureArg<0>(&request3), Return(true))) + .WillOnce(DoAll(FutureArg<0>(&request4), Return(true))); + + Call call; + call.set_type(Call::RESERVE_RESOURCES); + Call::ReserveResources* reserveResources = call.mutable_reserve_resources(); + + reserveResources->mutable_agent_id()->set_value(slaveId.value()); + + v1::Resource source = *v1::Resources::parse("cpus", "0.1", "*"); + *source.add_reservations() = + v1::createDynamicReservationInfo("foo", DEFAULT_CREDENTIAL.principal()); + *source.add_reservations() = v1::createDynamicReservationInfo( + "foo/bar", DEFAULT_CREDENTIAL.principal()); + + v1::Resource resource = *v1::Resources::parse("cpus", "0.1", "*"); + *resource.add_reservations() = + v1::createDynamicReservationInfo("baz", DEFAULT_CREDENTIAL.principal()); + *resource.add_reservations() = v1::createDynamicReservationInfo( + "baz/bar", DEFAULT_CREDENTIAL.principal()); + + *reserveResources->add_resources() = resource; + *reserveResources->add_source() = source; + + Future<http::Response> response = http::post( + master.get()->pid, + "/api/v1", + createBasicAuthHeaders(DEFAULT_CREDENTIAL), + stringify(JSON::protobuf(call)), + stringify(ContentType::JSON)); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(Accepted().status, response); + + AWAIT_READY(request1); + ASSERT_EQ(authorization::Action::UNRESERVE_RESOURCES, request1->action()); + ASSERT_EQ(2, request1->object().resource().reservations_size()); + ASSERT_EQ("foo", request1->object().resource().reservations(0).role()); + ASSERT_EQ("foo/bar", request1->object().resource().reservations(1).role()); + + AWAIT_READY(request2); + ASSERT_EQ(authorization::Action::UNRESERVE_RESOURCES, request2->action()); + ASSERT_EQ(1, request2->object().resource().reservations_size()); + ASSERT_EQ("foo", request1->object().resource().reservations(0).role()); + + AWAIT_READY(request3); + ASSERT_EQ(authorization::Action::RESERVE_RESOURCES, request3->action()); + ASSERT_EQ(1, request3->object().resource().reservations_size()); + ASSERT_EQ("baz", request3->object().resource().reservations(0).role()); + + AWAIT_READY(request4); + ASSERT_EQ(authorization::Action::RESERVE_RESOURCES, request4->action()); + ASSERT_EQ(2, request4->object().resource().reservations_size()); + ASSERT_EQ("baz", request4->object().resource().reservations(0).role()); + ASSERT_EQ("baz/bar", request4->object().resource().reservations(1).role()); + } +} class MasterOperationAuthorizationTest : public MesosTest,
