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 fb97080 Moved creating authz Object out of
Master::authorizeResizeVolume.
fb97080 is described below
commit fb97080ba53e6387779c37b18a42c8375969493c
Author: Andrei Sekretenko <[email protected]>
AuthorDate: Tue Jan 7 12:10:31 2020 -0500
Moved creating authz Object out of Master::authorizeResizeVolume.
Review: https://reviews.apache.org/r/71861/
---
src/master/authorization.cpp | 55 ++++++++++++++++++++++++++++++++++++++
src/master/authorization.hpp | 25 ++++++++++++++++++
src/master/http.cpp | 10 ++++---
src/master/master.cpp | 63 +++-----------------------------------------
src/master/master.hpp | 22 ----------------
5 files changed, 90 insertions(+), 85 deletions(-)
diff --git a/src/master/authorization.cpp b/src/master/authorization.cpp
index 9fde2ad..928deaf 100644
--- a/src/master/authorization.cpp
+++ b/src/master/authorization.cpp
@@ -20,12 +20,26 @@
#include "master/authorization.hpp"
using std::ostream;
+using std::string;
namespace mesos {
namespace authorization {
+ActionObject ActionObject::fromResourceWithLegacyValue(
+ const Action action,
+ const Resource& resource,
+ string value)
+{
+ Object object;
+ *object.mutable_resource() = resource;
+ *object.mutable_value() = std::move(value);
+
+ return ActionObject(action, std::move(object));
+}
+
+
ActionObject ActionObject::taskLaunch(
const TaskInfo& task,
const FrameworkInfo& framework)
@@ -61,6 +75,47 @@ ActionObject ActionObject::frameworkRegistration(
}
+// Returns effective reservation role of resource for the purpose
+// of authorizing an operation; that is, the the role of the most
+// refined reservation if the resource is reserved.
+//
+// NOTE: If the resource is not reserved, the default role '*' is
+// returned; this is needed to handle authorization of invalid
+// operations and could be avoided if we were able to guarantee
+// that authorization Objects for invalid operations are never built
+// (see MESOS-10083).
+//
+// NOTE: If the resource is not in the post-reservation-refinement
+// format, this function will crash the program.
+string getReservationRole(const Resource& resource)
+{
+ CHECK(!resource.has_role()) << resource;
+ CHECK(!resource.has_reservation()) << resource;
+
+ return Resources::isReserved(resource)
+ ? Resources::reservationRole(resource) : "*";
+}
+
+
+ActionObject ActionObject::growVolume(const Offer::Operation::GrowVolume& grow)
+{
+ return fromResourceWithLegacyValue(
+ authorization::RESIZE_VOLUME,
+ grow.volume(),
+ getReservationRole(grow.volume()));
+}
+
+
+ActionObject ActionObject::shrinkVolume(
+ const Offer::Operation::ShrinkVolume& shrink)
+{
+ return fromResourceWithLegacyValue(
+ authorization::RESIZE_VOLUME,
+ shrink.volume(),
+ getReservationRole(shrink.volume()));
+}
+
+
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 54c30f8..3e79f65 100644
--- a/src/master/authorization.hpp
+++ b/src/master/authorization.hpp
@@ -18,6 +18,7 @@
#define __MASTER_AUTHORIZATION_HPP__
#include <ostream>
+#include <string>
#include <stout/option.hpp>
@@ -46,12 +47,36 @@ public:
static ActionObject frameworkRegistration(
const FrameworkInfo& frameworkInfo);
+ // Methods that return action-object pair(s) for authorizing
+ // offer operations other than LAUNCH/LAUNCH_GROUP.
+ //
+ // NOTE: these methods rely on the caller to ensure that operation
+ // resources are in post-reservation-refinement format.
+ // Otherwise, they may crash the program.
+
+ static ActionObject growVolume(
+ const Offer::Operation::GrowVolume& grow);
+
+ static ActionObject shrinkVolume(
+ const Offer::Operation::ShrinkVolume& shrink);
+
private:
Action action_;
Option<Object> object_;
ActionObject(Action action, Option<Object>&& object)
: action_(action), object_(object){};
+
+ // Returns an action-object pair for the case that commonly
+ // occurs when authorizing non-LAUNCH operations: sets `action`,
+ // `object.resource` and `object.value`.
+ //
+ // NOTE: the deprecated `object.value` field is set to support legacy
+ // authorizers that have not been upgraded to look at `object.resource`.
+ static ActionObject fromResourceWithLegacyValue(
+ Action action,
+ const Resource& resource,
+ std::string value_);
};
// Outputs human-readable description of an ActionObject.
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 72587bf..94084dd 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -83,6 +83,7 @@
#include "logging/logging.hpp"
+#include "master/authorization.hpp"
#include "master/machine.hpp"
#include "master/maintenance.hpp"
#include "master/master.hpp"
@@ -137,6 +138,7 @@ using std::tie;
using std::tuple;
using std::vector;
+using mesos::authorization::ActionObject;
using mesos::authorization::createSubject;
using mesos::authorization::DEACTIVATE_AGENT;
using mesos::authorization::DRAIN_AGENT;
@@ -1262,8 +1264,8 @@ Future<Response> Master::Http::growVolume(
stringify(*slave) + ": " + error->message);
}
- return master->authorizeResizeVolume(
- operation.grow_volume().volume(), principal)
+ return master->authorize(
+ principal, ActionObject::growVolume(operation.grow_volume()))
.then(defer(master->self(), [=](bool authorized) -> Future<Response> {
if (!authorized) {
return Forbidden();
@@ -1325,8 +1327,8 @@ Future<Response> Master::Http::shrinkVolume(
stringify(*slave) + ": " + error->message);
}
- return master->authorizeResizeVolume(
- operation.shrink_volume().volume(), principal)
+ return master->authorize(
+ principal, ActionObject::shrinkVolume(operation.shrink_volume()))
.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 ff76831..e2f16f4 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -4014,51 +4014,6 @@ Future<bool> Master::authorizeDestroyVolume(
}
-Future<bool> Master::authorizeResizeVolume(
- const Resource& volume,
- const Option<Principal>& principal)
-{
- if (authorizer.isNone()) {
- return true; // Authorization is disabled.
- }
-
- authorization::Request request;
- request.set_action(authorization::RESIZE_VOLUME);
-
- Option<authorization::Subject> subject = createSubject(principal);
- if (subject.isSome()) {
- request.mutable_subject()->CopyFrom(subject.get());
- }
-
- 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`.
- //
- // 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;
-
- request.mutable_object()->set_value(
- Resources::isReserved(volume) ? Resources::reservationRole(volume) :
"*");
-
- LOG(INFO) << "Authorizing principal '"
- << (principal.isSome() ? stringify(principal.get()) : "ANY")
- << "' to resize volume '" << volume << "'";
-
- return authorizer.get()->authorized(request);
-}
-
-
Future<bool> Master::authorizeCreateDisk(
const Offer::Operation::CreateDisk& createDisk,
const Option<Principal>& principal)
@@ -4885,25 +4840,15 @@ void Master::accept(
}
case Offer::Operation::GROW_VOLUME: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
-
- futures.push_back(
- authorizeResizeVolume(
- operation.grow_volume().volume(), principal));
+ futures.push_back(authorize(
+ principal, ActionObject::growVolume(operation.grow_volume())));
break;
}
case Offer::Operation::SHRINK_VOLUME: {
- Option<Principal> principal = framework->info.has_principal()
- ? Principal(framework->info.principal())
- : Option<Principal>::none();
-
- futures.push_back(
- authorizeResizeVolume(
- operation.shrink_volume().volume(), principal));
+ futures.push_back(authorize(
+ principal, ActionObject::shrinkVolume(operation.shrink_volume())));
break;
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 0fbe3fb..efc3c62 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -848,28 +848,6 @@ protected:
const Offer::Operation::Destroy& destroy,
const Option<process::http::authentication::Principal>& principal);
- /**
- * Authorizes resize of a volume triggered by either `GROW_VOLUME` or
- * `SHRINK_VOLUME` operations.
- *
- * Returns whether the triggering 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 the operation could
be
- * malformed.
- *
- * @param volume The volume being resized.
- * @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> authorizeResizeVolume(
- const Resource& volume,
- const Option<process::http::authentication::Principal>& principal);
-
/**
* Authorizes a `CREATE_DISK` operation.
