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 fd0b761 Moved creating authorization Object out of
Master::authorize*Disk.
fd0b761 is described below
commit fd0b7618312aedbbf77aacc2f9b2524b1ef18f15
Author: Andrei Sekretenko <[email protected]>
AuthorDate: Tue Jan 7 12:27:54 2020 -0500
Moved creating authorization Object out of Master::authorize*Disk.
Review: https://reviews.apache.org/r/71862/
---
src/master/authorization.cpp | 59 ++++++++++++++++
src/master/authorization.hpp | 14 ++++
src/master/master.cpp | 164 ++++---------------------------------------
src/master/master.hpp | 43 ------------
4 files changed, 87 insertions(+), 193 deletions(-)
diff --git a/src/master/authorization.cpp b/src/master/authorization.cpp
index 928deaf..6f1231f 100644
--- a/src/master/authorization.cpp
+++ b/src/master/authorization.cpp
@@ -116,6 +116,65 @@ ActionObject ActionObject::shrinkVolume(
}
+Try<ActionObject> ActionObject::createDisk(
+ const Offer::Operation::CreateDisk& createDisk)
+{
+ auto action = ([&createDisk]() -> Option<authorization::Action> {
+ switch (createDisk.target_type()) {
+ case Resource::DiskInfo::Source::MOUNT:
+ return authorization::CREATE_MOUNT_DISK;
+ case Resource::DiskInfo::Source::BLOCK:
+ return authorization::CREATE_BLOCK_DISK;
+ case Resource::DiskInfo::Source::UNKNOWN:
+ case Resource::DiskInfo::Source::PATH:
+ case Resource::DiskInfo::Source::RAW:
+ return None();
+ }
+
+ return None();
+ })();
+
+ if (action.isNone()) {
+ return Error(
+ "Unsupported disk type: " + stringify(createDisk.target_type()));
+ }
+
+ return fromResourceWithLegacyValue(
+ *action, createDisk.source(), getReservationRole(createDisk.source()));
+}
+
+
+Try<ActionObject> ActionObject::destroyDisk(
+ const Offer::Operation::DestroyDisk& destroyDisk)
+{
+ const Resource& resource = destroyDisk.source();
+
+ auto action = ([&resource]() -> Option<authorization::Action> {
+ switch (resource.disk().source().type()) {
+ case Resource::DiskInfo::Source::MOUNT:
+ return authorization::DESTROY_MOUNT_DISK;
+ case Resource::DiskInfo::Source::BLOCK:
+ return authorization::DESTROY_BLOCK_DISK;
+ case Resource::DiskInfo::Source::RAW:
+ return authorization::DESTROY_RAW_DISK;
+ case Resource::DiskInfo::Source::UNKNOWN:
+ case Resource::DiskInfo::Source::PATH:
+ return None();
+ }
+
+ return None();
+ })();
+
+ if (action.isNone()) {
+ return Error(
+ "Unsupported disk type: " +
stringify(resource.disk().source().type()));
+ }
+
+ return fromResourceWithLegacyValue(
+ *action, resource, getReservationRole(resource));
+}
+
+
ostream& operator<<(ostream& stream, const ActionObject& actionObject)
{
const Option<Object>& object = actionObject.object();
diff --git a/src/master/authorization.hpp b/src/master/authorization.hpp
index 3e79f65..0949a0b 100644
--- a/src/master/authorization.hpp
+++ b/src/master/authorization.hpp
@@ -60,6 +60,20 @@ public:
static ActionObject shrinkVolume(
const Offer::Operation::ShrinkVolume& shrink);
+ // Returns Error if disk type is not supported.
+ //
+ // TODO (asekretenko): Change return type to ActionObject after
+ // authorization of invalid operations no longer occurs (see MESOS-10083).
+ static Try<ActionObject> createDisk(
+ const Offer::Operation::CreateDisk& createDisk);
+
+ // Returns Error if disk type is not supported.
+ //
+ // TODO (asekretenko): Change return type to ActionObject after
+ // authorization of invalid operations no longer occurs (see MESOS-10083).
+ static Try<ActionObject> destroyDisk(
+ const Offer::Operation::DestroyDisk& destroyDisk);
+
private:
Action action_;
Option<Object> object_;
diff --git a/src/master/master.cpp b/src/master/master.cpp
index e2f16f4..6ac5476 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -4014,144 +4014,6 @@ Future<bool> Master::authorizeDestroyVolume(
}
-Future<bool> Master::authorizeCreateDisk(
- const Offer::Operation::CreateDisk& createDisk,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- const Resource& resource = createDisk.source();
-
- Option<authorization::Action> action;
- switch (createDisk.target_type()) {
- case Resource::DiskInfo::Source::MOUNT: {
- action = authorization::CREATE_MOUNT_DISK;
- break;
- }
- case Resource::DiskInfo::Source::BLOCK: {
- action = authorization::CREATE_BLOCK_DISK;
- break;
- }
- case Resource::DiskInfo::Source::UNKNOWN:
- case Resource::DiskInfo::Source::PATH:
- case Resource::DiskInfo::Source::RAW: {
- return Failure(
- "Failed to authorize principal '" +
- (principal.isSome() ? stringify(principal.get()) : "ANY") +
- "' to create a " + stringify(createDisk.target_type()) +
- " disk from '" + stringify(resource) + "': Unsupported disk type");
- }
- }
-
- authorization::Request request;
- request.set_action(CHECK_NOTNONE(action));
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- 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`.
- //
- // 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;
-
- request.mutable_object()->set_value(
- Resources::isReserved(resource) ? Resources::reservationRole(resource)
- : "*");
-
- LOG(INFO) << "Authorizing principal '"
- << (principal.isSome() ? stringify(principal.get()) : "ANY")
- << "' to create a " << createDisk.target_type() << " disk from '"
- << createDisk.source() << "'";
-
- return authorizer.get()->authorized(request);
-}
-
-
-Future<bool> Master::authorizeDestroyDisk(
- const Offer::Operation::DestroyDisk& destroyDisk,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- const Resource& resource = destroyDisk.source();
-
- Option<authorization::Action> action;
- switch (resource.disk().source().type()) {
- case Resource::DiskInfo::Source::MOUNT: {
- action = authorization::DESTROY_MOUNT_DISK;
- break;
- }
- case Resource::DiskInfo::Source::BLOCK: {
- action = authorization::DESTROY_BLOCK_DISK;
- break;
- }
- case Resource::DiskInfo::Source::RAW: {
- action = authorization::DESTROY_RAW_DISK;
- break;
- }
- case Resource::DiskInfo::Source::UNKNOWN:
- case Resource::DiskInfo::Source::PATH: {
- return Failure(
- "Failed to authorize principal '" +
- (principal.isSome() ? stringify(principal.get()) : "ANY") +
- "' to destroy disk '" + stringify(resource) +
- "': Unsupported disk type");
- }
- }
-
- authorization::Request request;
- request.set_action(CHECK_NOTNONE(action));
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- 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`.
- //
- // 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;
-
- request.mutable_object()->set_value(
- Resources::isReserved(resource) ? Resources::reservationRole(resource)
- : "*");
-
- LOG(INFO) << "Authorizing principal '"
- << (principal.isSome() ? stringify(principal.get()) : "ANY")
- << "' to destroy disk '" << destroyDisk.source() << "'";
-
- return authorizer.get()->authorized(request);
-}
-
-
Future<bool> Master::authorizeSlave(
const SlaveInfo& slaveInfo,
const Option<Principal>& principal)
@@ -4854,25 +4716,27 @@ void Master::accept(
}
case Offer::Operation::CREATE_DISK: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
+ Try<ActionObject> actionObject =
+ ActionObject::createDisk(operation.create_disk());
- futures.push_back(
- authorizeCreateDisk(
- operation.create_disk(), principal));
+ if (actionObject.isError()) {
+ futures.push_back(Failure(actionObject.error()));
+ } else {
+ futures.push_back(authorize(principal, std::move(*actionObject)));
+ }
break;
}
case Offer::Operation::DESTROY_DISK: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
+ Try<ActionObject> actionObject =
+ ActionObject::destroyDisk(operation.destroy_disk());
- futures.push_back(
- authorizeDestroyDisk(
- operation.destroy_disk(), principal));
+ if (actionObject.isError()) {
+ futures.push_back(Failure(actionObject.error()));
+ } else {
+ futures.push_back(authorize(principal, std::move(*actionObject)));
+ }
break;
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index efc3c62..0dd7b44 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -848,49 +848,6 @@ protected:
const Offer::Operation::Destroy& destroy,
const Option<process::http::authentication::Principal>& principal);
-
- /**
- * Authorizes a `CREATE_DISK` operation.
- *
- * Returns whether the `CREATE_DISK` operation is authorized with the
- * provided principal. This function is used for authorization of operations
- * originating from frameworks. Note that operations may be validated AFTER
- * authorization, so it's possible that the operation could be malformed.
- *
- * @param createDisk The `CREATE_DISK` 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> authorizeCreateDisk(
- const Offer::Operation::CreateDisk& createDisk,
- const Option<process::http::authentication::Principal>& principal);
-
-
- /**
- * Authorizes a `DESTROY_DISK` operation.
- *
- * Returns whether the `DESTROY_DISK` operation is authorized with the
- * provided principal. This function is used for authorization of operations
- * originating from frameworks. Note that operations may be validated AFTER
- * authorization, so it's possible that the operation could be malformed.
- *
- * @param destroyDisk The `DESTROY_DISK` 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> authorizeDestroyDisk(
- const Offer::Operation::DestroyDisk& destroyDisk,
- const Option<process::http::authentication::Principal>& principal);
-
-
// Determine if a new executor needs to be launched.
bool isLaunchExecutor (
const ExecutorID& executorId,
