You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ti...@apache.org on 2018/11/20 19:16:33 UTC
[mesos] 01/02: Added collectAuthorizations helper to master.hpp.
This is an automated email from the ASF dual-hosted git repository.
tillt pushed a commit to branch 1.5.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 0ef48eb254a69ab9d492cbec03736fc88bb8012a
Author: Till Toenshoff <to...@me.com>
AuthorDate: Tue Nov 20 14:45:50 2018 +0100
Added collectAuthorizations helper to master.hpp.
Adds the helper function 'collectAuthorizations' to master.hpp. This
function allows for a simple way to collect authorization futures and
only if all supplied futures result in an approved authorization will
the returned future return true.
All identified areas that were formally triggering MESOS-9317 are
being updated to make use of this new helper.
A helper function has been chosen and preferred over copying this
pattern into the areas that needed a fix to allow for an efficient and
complete test coverage.
Additionally we are adding a test validating that new helper.
Review: https://reviews.apache.org/r/69369/
** Backported for 1.5.x **
---
src/master/master.cpp | 48 ++--------------
src/master/master.hpp | 17 +++++-
src/master/weights_handler.cpp | 12 +---
src/tests/master_tests.cpp | 122 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 143 insertions(+), 56 deletions(-)
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 5fcdd63..0229d1b 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3706,17 +3706,7 @@ Future<bool> Master::authorizeReserveResources(
return authorizer.get()->authorized(request);
}
- return await(authorizations)
- .then([](const list<Future<bool>>& authorizations)
- -> Future<bool> {
- // Compute a disjunction.
- foreach (const Future<bool>& authorization, authorizations) {
- if (!authorization.get()) {
- return false;
- }
- }
- return true;
- });
+ return collectAuthorizations(authorizations);
}
@@ -3769,17 +3759,7 @@ Future<bool> Master::authorizeUnreserveResources(
return authorizer.get()->authorized(request);
}
- return await(authorizations)
- .then([](const list<Future<bool>>& authorizations)
- -> Future<bool> {
- // Compute a disjunction.
- foreach (const Future<bool>& authorization, authorizations) {
- if (!authorization.get()) {
- return false;
- }
- }
- return true;
- });
+ return collectAuthorizations(authorizations);
}
@@ -3836,17 +3816,7 @@ Future<bool> Master::authorizeCreateVolume(
return authorizer.get()->authorized(request);
}
- return await(authorizations)
- .then([](const list<Future<bool>>& authorizations)
- -> Future<bool> {
- // Compute a disjunction.
- foreach (const Future<bool>& authorization, authorizations) {
- if (!authorization.get()) {
- return false;
- }
- }
- return true;
- });
+ return collectAuthorizations(authorizations);
}
@@ -3888,17 +3858,7 @@ Future<bool> Master::authorizeDestroyVolume(
return authorizer.get()->authorized(request);
}
- return await(authorizations)
- .then([](const list<Future<bool>>& authorizations)
- -> Future<bool> {
- // Compute a disjunction.
- foreach (const Future<bool>& authorization, authorizations) {
- if (!authorization.get()) {
- return false;
- }
- }
- return true;
- });
+ return collectAuthorizations(authorizations);
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 43676bd..c1db276 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -19,6 +19,7 @@
#include <stdint.h>
+#include <algorithm>
#include <list>
#include <memory>
#include <set>
@@ -44,8 +45,10 @@
#include <mesos/scheduler/scheduler.hpp>
-#include <process/limiter.hpp>
+#include <process/collect.hpp>
+#include <process/future.hpp>
#include <process/http.hpp>
+#include <process/limiter.hpp>
#include <process/owned.hpp>
#include <process/process.hpp>
#include <process/protobuf.hpp>
@@ -2164,6 +2167,18 @@ private:
};
+// Collects authorization results. Any discarded or failed future
+// results in a failure; any false future results in false.
+inline process::Future<bool> collectAuthorizations(
+ const std::list<process::Future<bool>>& authorizations)
+{
+ return process::collect(authorizations)
+ .then([](const std::list<bool>& results) -> process::Future<bool> {
+ return std::find(results.begin(), results.end(), false) == results.end();
+ });
+}
+
+
inline std::ostream& operator<<(
std::ostream& stream,
const Framework& framework);
diff --git a/src/master/weights_handler.cpp b/src/master/weights_handler.cpp
index 59104a8..ce07271 100644
--- a/src/master/weights_handler.cpp
+++ b/src/master/weights_handler.cpp
@@ -346,17 +346,7 @@ Future<bool> Master::WeightsHandler::authorizeUpdateWeights(
return master->authorizer.get()->authorized(request);
}
- return await(authorizations)
- .then([](const list<Future<bool>>& authorizations)
- -> Future<bool> {
- // Compute a disjunction.
- foreach (const Future<bool>& authorization, authorizations) {
- if (!authorization.get()) {
- return false;
- }
- }
- return true;
- });
+ return collectAuthorizations(authorizations);
}
diff --git a/src/tests/master_tests.cpp b/src/tests/master_tests.cpp
index dcd9088..9c46e0c 100644
--- a/src/tests/master_tests.cpp
+++ b/src/tests/master_tests.cpp
@@ -105,6 +105,7 @@ using process::PID;
using process::Promise;
using process::http::Accepted;
+using process::http::InternalServerError;
using process::http::OK;
using process::http::Response;
using process::http::Unauthorized;
@@ -9333,6 +9334,127 @@ TEST_P(MasterTestPrePostReservationRefinement, CreateAndDestroyVolumesV1)
AWAIT_EXPECT_RESPONSE_STATUS_EQ(Accepted().status, v1DestroyVolumesResponse);
}
+
+// This test validates that an authorization error when requesting
+// volume creation does result in an internal server error.
+// See MESOS-9317.
+TEST_F(MasterTest, CreateVolumesV1AuthorizationFailure)
+{
+ MockAuthorizer authorizer;
+ Try<Owned<cluster::Master>> master = StartMaster(&authorizer);
+ ASSERT_SOME(master);
+
+ // For capturing the `SlaveID` so we can use it in the create/destroy
+ // volumes API call.
+ Future<SlaveRegisteredMessage> slaveRegisteredMessage =
+ FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
+
+ Owned<MasterDetector> detector = master.get()->createDetector();
+
+ slave::Flags slaveFlags = CreateSlaveFlags();
+ // Do static reservation so we can create persistent volumes from it.
+ slaveFlags.resources = "disk(role1):1024";
+
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+
+ ASSERT_SOME(slave);
+
+ AWAIT_READY(slaveRegisteredMessage);
+ SlaveID slaveId = slaveRegisteredMessage->slave_id();
+
+ // Create the persistent volume.
+ v1::master::Call v1CreateVolumesCall;
+ v1CreateVolumesCall.set_type(v1::master::Call::CREATE_VOLUMES);
+ v1::master::Call_CreateVolumes* createVolumes =
+ v1CreateVolumesCall.mutable_create_volumes();
+
+ Resources volume = createPersistentVolume(
+ Megabytes(64),
+ "role1",
+ "id1",
+ "path1",
+ None(),
+ None(),
+ DEFAULT_CREDENTIAL.principal());
+
+ createVolumes->mutable_agent_id()->CopyFrom(evolve(slaveId));
+ createVolumes->mutable_volumes()->CopyFrom(v1::Resources(evolve(volume)));
+
+ ContentType contentType = ContentType::PROTOBUF;
+
+ Promise<bool> promise;
+ EXPECT_CALL(authorizer, authorized(_))
+ .WillOnce(Return(promise.future()));
+
+ Future<Response> response = process::http::post(
+ master.get()->pid,
+ "api/v1",
+ createBasicAuthHeaders(DEFAULT_CREDENTIAL),
+ serialize(contentType, v1CreateVolumesCall),
+ stringify(contentType));
+
+ promise.fail("Authorizer failure");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(InternalServerError().status, response);
+}
+
+
+// Test for the authorization collect helper.
+TEST_F(MasterTest, CollectAuthorizations)
+{
+ {
+ Promise<bool> promise1;
+ Promise<bool> promise2;
+
+ Future<bool> result =
+ master::collectAuthorizations({promise1.future(), promise2.future()});
+
+ promise1.set(true);
+ promise2.set(false);
+
+ AWAIT_EXPECT_FALSE(result);
+ }
+
+ {
+ Promise<bool> promise1;
+ Promise<bool> promise2;
+
+ Future<bool> result =
+ master::collectAuthorizations({promise1.future(), promise2.future()});
+
+ promise1.set(true);
+ promise2.fail("Authorization failure");
+
+ AWAIT_EXPECT_FAILED(result);
+ }
+
+ {
+ Promise<bool> promise1;
+ Promise<bool> promise2;
+
+ Future<bool> result =
+ master::collectAuthorizations({promise1.future(), promise2.future()});
+
+ promise1.set(true);
+ promise2.discard();
+
+ AWAIT_EXPECT_FAILED(result);
+ }
+
+ {
+ Promise<bool> promise1;
+ Promise<bool> promise2;
+
+ Future<bool> result =
+ master::collectAuthorizations({promise1.future(), promise2.future()});
+
+ promise1.set(true);
+ promise2.set(true);
+
+ AWAIT_EXPECT_TRUE(result);
+ }
+}
+
} // namespace tests {
} // namespace internal {
} // namespace mesos {