This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new ec2d1cb Moved creating authorization Object out of
Master::authorize*reserve.
ec2d1cb is described below
commit ec2d1cb1f291b6ffa4e814151ad4a82a0daa950a
Author: Andrei Sekretenko <[email protected]>
AuthorDate: Tue Jan 7 13:42:17 2020 -0500
Moved creating authorization Object out of Master::authorize*reserve.
NOTE: This patch also partially addresses MESOS-9562 issue
by modifying the logic of authorizing UNRESERVE operations.
Now, if some reservations have no principal, both authorization of
UNRESERVE_RESOURCES for ANY object and authorizations for unreserving
all reservations that have principals will be requested.
Review: https://reviews.apache.org/r/71864/
---
src/master/authorization.cpp | 122 ++++++++++++++++++++++++++
src/master/authorization.hpp | 24 ++++++
src/master/http.cpp | 6 +-
src/master/master.cpp | 199 ++-----------------------------------------
src/master/master.hpp | 43 ----------
5 files changed, 155 insertions(+), 239 deletions(-)
diff --git a/src/master/authorization.cpp b/src/master/authorization.cpp
index 96f6a1c..d97288a 100644
--- a/src/master/authorization.cpp
+++ b/src/master/authorization.cpp
@@ -23,6 +23,8 @@ using std::ostream;
using std::string;
using std::vector;
+using google::protobuf::RepeatedPtrField;
+
namespace mesos {
namespace authorization {
@@ -97,6 +99,126 @@ string getReservationRole(const Resource& resource)
}
+void ActionObject::pushUnreserveActionObjects(
+ const Resources& resources,
+ vector<ActionObject>* result)
+{
+ bool hasReservationsWithoutPrincipal = false;
+
+ for (const Resource& resource : resources) {
+ // NOTE: We rely on the master to ensure that the resource is in the
+ // post-reservation-refinement format.
+ CHECK(!resource.has_role()) << resource;
+ CHECK(!resource.has_reservation()) << resource;
+
+ if (!resource.reservations().empty() &&
+ resource.reservations().rbegin()->has_principal()) {
+ result->push_back(fromResourceWithLegacyValue(
+ authorization::UNRESERVE_RESOURCES,
+ resource,
+ resource.reservations().rbegin()->principal()));
+ } else {
+ hasReservationsWithoutPrincipal = true;
+ }
+ }
+
+ if (hasReservationsWithoutPrincipal) {
+ result->push_back(ActionObject(authorization::UNRESERVE_RESOURCES,
None()));
+ }
+}
+
+
+vector<ActionObject>
+ActionObject::unreserve(const Offer::Operation::Unreserve& unreserve)
+{
+ vector<ActionObject> result;
+ pushUnreserveActionObjects(unreserve.resources(), &result);
+ // NOTE: In some cases, such as UNRESERVE with no resources
+ // (which is invalid) or resources reserved without a principal
+ // by an old version of Mesos, an empty vector will be returned.
+ return result;
+}
+
+
+vector<ActionObject> ActionObject::reserve(
+ const Offer::Operation::Reserve& reserve)
+{
+ std::vector<ActionObject> result;
+ auto pushReserveActionObjects =
+ [&result](const Resources& resources) mutable {
+ // The operation will be authorized if the entity is allowed to make
+ // reservations for all unique roles included in `resources`.
+ //
+ // TODO(asekretenko): This approach assumes that resources with identical
+ // reservation roles are equivalent from the authorizers' point of view;
+ // in future this assumption might become incorrect. As there is no
reason
+ // other than performance to leave only resources with unique roles, this
+ // filtration can probably be avoided after implementing synchronous
+ // authorization (see MESOS-10056).
+ hashset<string> roles;
+
+ foreach (const Resource& resource, resources) {
+ const string role = getReservationRole(resource);
+
+ if (!roles.contains(role)) {
+ roles.insert(role);
+ result.push_back(fromResourceWithLegacyValue(
+ authorization::RESERVE_RESOURCES, resource, role));
+ }
+ }
+ };
+
+ // 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())
+ {
+ pushReserveActionObjects(reserve.resources());
+ return result;
+ }
+
+ Resources source = reserve.source();
+ const Resources target = reserve.resources();
+ const Resources ancestor =
+ Resources::getReservationAncestor(source, target);
+
+ // We request UNRESERVE_RESOURCES to bring `source` to `ancestor`.
+ while (source != ancestor) {
+ pushUnreserveActionObjects(source, &result);
+ source = source.popReservation();
+ }
+
+ // We request RESERVE_RESOURCES to bring `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);
+ pushReserveActionObjects(source);
+ }
+
+ // NOTE: In some cases, such as RESERVE with source identical to target,
+ // an empty vector will be returned.
+ return result;
+}
+
+
vector<ActionObject> ActionObject::createVolume(
const Offer::Operation::Create& create)
{
diff --git a/src/master/authorization.hpp b/src/master/authorization.hpp
index d2b96ef..58bf214 100644
--- a/src/master/authorization.hpp
+++ b/src/master/authorization.hpp
@@ -55,6 +55,12 @@ public:
// resources are in post-reservation-refinement format.
// Otherwise, they may crash the program.
+ static std::vector<ActionObject> unreserve(
+ const Offer::Operation::Unreserve& unreserve);
+
+ static std::vector<ActionObject> reserve(
+ const Offer::Operation::Reserve& reserve);
+
static std::vector<ActionObject> createVolume(
const Offer::Operation::Create& create);
@@ -98,6 +104,24 @@ private:
Action action,
const Resource& resource,
std::string value_);
+
+ // Helper method for `reserve()`/`unreserve()`.
+ //
+ // For each resource reserved with a principal, pushes into `result` an
+ // ActionObject needed for unreserving it. If there are resources reserved
+ // without a principal, an UNRESERVE_RESOURCE for ANY object is also added
+ // (see MESOS-9562).
+ //
+ // NOTE: Since the UNRESERVE operation only "pops" one reservation off the
+ // stack of reservations, only principals of the most refined reservations
+ // (i.e. ones that will be unreserved) are used.
+ //
+ // NOTE: Currently, validation of RESERVE operations prevents creating
+ // reservation without a principal, so they should not exist in new
+ // clusters.
+ static void pushUnreserveActionObjects(
+ const Resources& resources,
+ std::vector<ActionObject>* result);
};
// Outputs human-readable description of an ActionObject.
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 93ec096..16dbed3 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -2827,7 +2827,8 @@ Future<Response> Master::Http::_reserve(
error->message);
}
- return master->authorizeReserveResources(operation.reserve(), principal)
+ return master->authorize(
+ principal, ActionObject::reserve(operation.reserve()))
.then(defer(master->self(), [=](bool authorized) -> Future<Response> {
if (!authorized) {
return Forbidden();
@@ -5578,7 +5579,8 @@ Future<Response> Master::Http::_unreserve(
return BadRequest("Invalid UNRESERVE operation: " + error->message);
}
- return master->authorizeUnreserveResources(operation.unreserve(), principal)
+ return master->authorize(
+ principal, ActionObject::unreserve(operation.unreserve()))
.then(defer(master->self(), [=](bool authorized) -> Future<Response> {
if (!authorized) {
return Forbidden();
diff --git a/src/master/master.cpp b/src/master/master.cpp
index d3c2889..c9ae0cd 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3754,185 +3754,6 @@ Future<bool> Master::authorize(
}
-Future<bool> Master::authorizeReserveResources(
- const Offer::Operation::Reserve& reserve,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- authorization::Request request;
- request.set_action(authorization::RESERVE_RESOURCES);
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- vector<Future<bool>> authorizations;
-
- // 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;
-
- const string role = Resources::isReserved(resource)
- ? Resources::reservationRole(resource) : "*";
-
- if (!roles.contains(role)) {
- roles.insert(role);
-
- request.mutable_object()->mutable_resource()->CopyFrom(resource);
-
- // 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() << "'";
- } 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
- // the validation occur and the case must be considered non erroneous.
- // TODO(arojas): Consider ensuring that `validate()` is called before
- // `authorizeReserveResources` so a `CHECK(!roles.empty())` can be added.
- if (authorizations.empty()) {
- return authorizer.get()->authorized(request);
- }
-
- return authorization::collectAuthorizations(authorizations);
-}
-
-
-Future<bool> Master::authorizeUnreserveResources(
- const Offer::Operation::Unreserve& unreserve,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- authorization::Request request;
- request.set_action(authorization::UNRESERVE_RESOURCES);
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- vector<Future<bool>> authorizations;
- foreach (const Resource& resource, unreserve.resources()) {
- // NOTE: We rely on the master to ensure that the resource is in the
- // post-reservation-refinement format. Since the UNRESERVE operation only
- // "pops" one reservation off the stack of reservations, we perform
- // authorization for the principal of the most refined reservation, which
- // will be unreserved (i.e., popped off the stack).
- //
- // NOTE: Since authorization happens __before__ validation, we must check
- // here that this resource has a reservation. If not, the error will be
- // caught during validation.
- CHECK(!resource.has_role()) << resource;
- CHECK(!resource.has_reservation()) << resource;
-
- Option<string> principal;
- if (!resource.reservations().empty() &&
- resource.reservations().rbegin()->has_principal()) {
- principal = resource.reservations().rbegin()->principal();
- }
-
- if (principal.isSome()) {
- request.mutable_object()->mutable_resource()->CopyFrom(resource);
-
- // 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(principal.get());
-
- authorizations.push_back(authorizer.get()->authorized(request));
- }
- }
-
- LOG(INFO) << "Authorizing principal '"
- << (principal.isSome() ? stringify(principal.get()) : "ANY")
- << "' to unreserve resources '" << unreserve.resources() << "'";
-
- if (authorizations.empty()) {
- return authorizer.get()->authorized(request);
- }
-
- return authorization::collectAuthorizations(authorizations);
-}
-
-
Future<bool> Master::authorizeSlave(
const SlaveInfo& slaveInfo,
const Option<Principal>& principal)
@@ -3974,7 +3795,7 @@ Future<bool> Master::authorizeSlave(
Offer::Operation::Reserve reserve;
reserve.mutable_resources()->CopyFrom(slaveInfo.resources());
authorizations.push_back(
- authorizeReserveResources(reserve, principal));
+ authorize(principal, ActionObject::reserve(reserve)));
}
return authorization::collectAuthorizations(authorizations);
@@ -4570,26 +4391,16 @@ void Master::accept(
// The RESERVE operation allows a principal to reserve resources.
case Offer::Operation::RESERVE: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
-
- futures.push_back(
- authorizeReserveResources(
- operation.reserve(), principal));
+ futures.push_back(authorize(
+ principal, ActionObject::reserve(operation.reserve())));
break;
}
// The UNRESERVE operation allows a principal to unreserve resources.
case Offer::Operation::UNRESERVE: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
-
- futures.push_back(
- authorizeUnreserveResources(
- operation.unreserve(), principal));
+ futures.push_back(authorize(
+ principal, ActionObject::unreserve(operation.unreserve())));
break;
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index be1a0dd..0ac3332 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -769,49 +769,6 @@ protected:
const SlaveInfo& slaveInfo,
const Option<process::http::authentication::Principal>& principal);
-
- /**
- * Authorizes a `RESERVE` operation.
- *
- * Returns whether the Reserve operation is authorized with the
- * provided principal. This function is used for authorization of
- * operations originating from both frameworks and operators. Note
- * that operations may be validated AFTER authorization, so it's
- * possible that `reserve` could be malformed.
- *
- * @param reserve The `RESERVE` operation to be performed.
- * @param principal An `Option` containing the principal attempting
- * this operation.
- *
- * @return A `Future` containing a boolean value representing the
- * success or failure of this authorization. A failed `Future`
- * implies that validation of the operation did not succeed.
- */
- process::Future<bool> authorizeReserveResources(
- const Offer::Operation::Reserve& reserve,
- const Option<process::http::authentication::Principal>& principal);
-
- /**
- * Authorizes an `UNRESERVE` operation.
- *
- * Returns whether the Unreserve operation is authorized with the
- * provided principal. This function is used for authorization of
- * operations originating both from frameworks and operators. Note
- * that operations may be validated AFTER authorization, so it's
- * possible that `unreserve` could be malformed.
- *
- * @param unreserve The `UNRESERVE` operation to be performed.
- * @param principal An `Option` containing the principal attempting
- * this operation.
- *
- * @return A `Future` containing a boolean value representing the
- * success or failure of this authorization. A failed `Future`
- * implies that validation of the operation did not succeed.
- */
- process::Future<bool> authorizeUnreserveResources(
- const Offer::Operation::Unreserve& unreserve,
- const Option<process::http::authentication::Principal>& principal);
-
// Determine if a new executor needs to be launched.
bool isLaunchExecutor (
const ExecutorID& executorId,
