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 b4c705f Moved creating authorization Object out of
Master::authorize*Volume.
b4c705f is described below
commit b4c705fb884b23e7a016b873aca6369a26eaa32a
Author: Andrei Sekretenko <[email protected]>
AuthorDate: Tue Jan 7 12:56:53 2020 -0500
Moved creating authorization Object out of Master::authorize*Volume.
Review: https://reviews.apache.org/r/71863/
---
src/master/authorization.cpp | 58 +++++++++++++++-
src/master/authorization.hpp | 7 ++
src/master/http.cpp | 6 +-
src/master/master.cpp | 153 +++++++++----------------------------------
src/master/master.hpp | 48 ++------------
5 files changed, 105 insertions(+), 167 deletions(-)
diff --git a/src/master/authorization.cpp b/src/master/authorization.cpp
index 6f1231f..96f6a1c 100644
--- a/src/master/authorization.cpp
+++ b/src/master/authorization.cpp
@@ -21,7 +21,7 @@
using std::ostream;
using std::string;
-
+using std::vector;
namespace mesos {
namespace authorization {
@@ -97,6 +97,62 @@ string getReservationRole(const Resource& resource)
}
+vector<ActionObject> ActionObject::createVolume(
+ const Offer::Operation::Create& create)
+{
+ vector<ActionObject> result;
+
+ // The operation will be authorized if the entity is allowed
+ // to create volumes for all roles included in `create.volumes`.
+
+ // Add an object for each unique role in the volumes.
+ hashset<string> roles;
+
+ foreach (const Resource& volume, create.volumes()) {
+ string role = getReservationRole(volume);
+
+ if (!roles.contains(role)) {
+ roles.insert(role);
+ result.push_back(fromResourceWithLegacyValue(
+ authorization::CREATE_VOLUME, volume, role));
+ }
+ }
+
+ if (result.empty()) {
+ result.push_back(ActionObject(authorization::CREATE_VOLUME, None()));
+ }
+
+ return result;
+}
+
+
+vector<ActionObject> ActionObject::destroyVolume(
+ const Offer::Operation::Destroy& destroy)
+{
+ vector<ActionObject> result;
+
+ // NOTE: As this code can be called for an invalid operation, we create
+ // action-object pairs only for resources that are persistent volumes and
+ // skip all the others, relying on the caller to perform validation after
+ // authorization (see MESOS-10083).
+ foreach (const Resource& volume, destroy.volumes()) {
+ // TODO(asekretenko): Replace with CHECK after MESOS-10083 is fixed.
+ if (volume.has_disk() && volume.disk().has_persistence()) {
+ result.push_back(fromResourceWithLegacyValue(
+ authorization::DESTROY_VOLUME,
+ volume,
+ volume.disk().persistence().principal()));
+ }
+ }
+
+ if (result.empty()) {
+ result.push_back(ActionObject(authorization::DESTROY_VOLUME, None()));
+ }
+
+ return result;
+}
+
+
ActionObject ActionObject::growVolume(const Offer::Operation::GrowVolume& grow)
{
return fromResourceWithLegacyValue(
diff --git a/src/master/authorization.hpp b/src/master/authorization.hpp
index 0949a0b..d2b96ef 100644
--- a/src/master/authorization.hpp
+++ b/src/master/authorization.hpp
@@ -19,6 +19,7 @@
#include <ostream>
#include <string>
+#include <vector>
#include <stout/option.hpp>
@@ -54,6 +55,12 @@ public:
// resources are in post-reservation-refinement format.
// Otherwise, they may crash the program.
+ static std::vector<ActionObject> createVolume(
+ const Offer::Operation::Create& create);
+
+ static std::vector<ActionObject> destroyVolume(
+ const Offer::Operation::Destroy& destroy);
+
static ActionObject growVolume(
const Offer::Operation::GrowVolume& grow);
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 94084dd..93ec096 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1010,7 +1010,8 @@ Future<Response> Master::Http::_createVolumes(
error->message);
}
- return master->authorizeCreateVolume(operation.create(), principal)
+ return master->authorize(
+ principal, ActionObject::createVolume(operation.create()))
.then(defer(master->self(), [=](bool authorized) -> Future<Response> {
if (!authorized) {
return Forbidden();
@@ -1178,7 +1179,8 @@ Future<Response> Master::Http::_destroyVolumes(
return BadRequest("Invalid DESTROY operation: " + error->message);
}
- return master->authorizeDestroyVolume(operation.destroy(), principal)
+ return master->authorize(
+ principal, ActionObject::destroyVolume(operation.destroy()))
.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 6ac5476..d3c2889 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3727,6 +3727,33 @@ Future<bool> Master::authorize(
}
+Future<bool> Master::authorize(
+ const Option<Principal>& principal,
+ vector<ActionObject>&& actionObjects)
+{
+ // NOTE: In some cases (example: RESERVE with empty resources or with source
+ // identical to target) there is no need to authorize any action-object pair
+ // (and no meaningful ActionObject can be composed anyway). Here, we treat
+ // authorization as PASSED in these cases and expect these cases to be
+ // handled by validation afterwards.
+ //
+ // Some of these cases are invalid; ideally, they should be filtered by
+ // validation before being fed into ActionObject-composing code (see
+ // MESOS-10083).
+ if (actionObjects.empty()) {
+ return true;
+ }
+
+ vector<Future<bool>> authorizations;
+ authorizations.reserve(actionObjects.size());
+ for (ActionObject& actionObject: actionObjects) {
+ authorizations.push_back(authorize(principal, std::move(actionObject)));
+ }
+
+ return authorization::collectAuthorizations(authorizations);
+}
+
+
Future<bool> Master::authorizeReserveResources(
const Offer::Operation::Reserve& reserve,
const Option<Principal>& principal)
@@ -3906,114 +3933,6 @@ Future<bool> Master::authorizeUnreserveResources(
}
-Future<bool> Master::authorizeCreateVolume(
- const Offer::Operation::Create& create,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- authorization::Request request;
- request.set_action(authorization::CREATE_VOLUME);
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- // The operation will be authorized if the entity is allowed to create
- // volumes for all roles included in `create.volumes`.
- // Add an element to `request.roles` for each unique role in the volumes.
- hashset<string> roles;
- vector<Future<bool>> authorizations;
- foreach (const Resource& volume, create.volumes()) {
- // 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: Since authorization happens __before__ validation, we must check
- // here that this resource has a reservation. If not, the error will be
- // caught during validation, but we still authorize the resource with the
- // default role '*' for backward compatibility.
- CHECK(!volume.has_role()) << volume;
- CHECK(!volume.has_reservation()) << volume;
-
- const string role =
- Resources::isReserved(volume) ? Resources::reservationRole(volume) : "*";
-
- if (!roles.contains(role)) {
- roles.insert(role);
-
- request.mutable_object()->mutable_resource()->CopyFrom(volume);
-
- // 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 create volumes '" << create.volumes() << "'";
-
- if (authorizations.empty()) {
- return authorizer.get()->authorized(request);
- }
-
- return authorization::collectAuthorizations(authorizations);
-}
-
-
-Future<bool> Master::authorizeDestroyVolume(
- const Offer::Operation::Destroy& destroy,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- authorization::Request request;
- request.set_action(authorization::DESTROY_VOLUME);
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- vector<Future<bool>> authorizations;
- foreach (const Resource& volume, destroy.volumes()) {
- // NOTE: Since authorization happens __before__ validation, we must check
- // here that this resource is a persistent volume. If not, the error will
be
- // caught during validation.
- if (volume.has_disk() && volume.disk().has_persistence()) {
- request.mutable_object()->mutable_resource()->CopyFrom(volume);
-
- // 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(
- volume.disk().persistence().principal());
-
- authorizations.push_back(authorizer.get()->authorized(request));
- }
- }
-
- LOG(INFO) << "Authorizing principal '"
- << (principal.isSome() ? stringify(principal.get()) : "ANY")
- << "' to destroy volumes '" << destroy.volumes() << "'";
-
- if (authorizations.empty()) {
- return authorizer.get()->authorized(request);
- }
-
- return authorization::collectAuthorizations(authorizations);
-}
-
-
Future<bool> Master::authorizeSlave(
const SlaveInfo& slaveInfo,
const Option<Principal>& principal)
@@ -4677,26 +4596,16 @@ void Master::accept(
// The CREATE operation allows the creation of a persistent volume.
case Offer::Operation::CREATE: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
-
- futures.push_back(
- authorizeCreateVolume(
- operation.create(), principal));
+ futures.push_back(authorize(
+ principal, ActionObject::createVolume(operation.create())));
break;
}
// The DESTROY operation allows the destruction of a persistent volume.
case Offer::Operation::DESTROY: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
-
- futures.push_back(
- authorizeDestroyVolume(
- operation.destroy(), principal));
+ futures.push_back(authorize(
+ principal, ActionObject::destroyVolume(operation.destroy())));
break;
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 0dd7b44..be1a0dd 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -750,6 +750,12 @@ protected:
const Option<process::http::authentication::Principal>& principal,
authorization::ActionObject&& actionObject);
+ // Overload of authorize() for cases which require multiple action-object
+ // pairs to be authorized simultaneously.
+ process::Future<bool> authorize(
+ const Option<process::http::authentication::Principal>& principal,
+ std::vector<authorization::ActionObject>&& actionObjects);
+
// TODO(asekretenko): get rid of action-specific authorizeSomething()
methods.
// Returns whether the framework is authorized.
@@ -806,48 +812,6 @@ protected:
const Offer::Operation::Unreserve& unreserve,
const Option<process::http::authentication::Principal>& principal);
- /**
- * Authorizes a `CREATE` operation.
- *
- * Returns whether the Create 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 `create` could be
- * malformed.
- *
- * @param create The `CREATE` 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> authorizeCreateVolume(
- const Offer::Operation::Create& create,
- const Option<process::http::authentication::Principal>& principal);
-
- /**
- * Authorizes a `DESTROY` operation.
- *
- * Returns whether the Destroy 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 `destroy` could be
- * malformed.
- *
- * @param destroy The `DESTROY` 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> authorizeDestroyVolume(
- const Offer::Operation::Destroy& destroy,
- const Option<process::http::authentication::Principal>& principal);
-
// Determine if a new executor needs to be launched.
bool isLaunchExecutor (
const ExecutorID& executorId,
