This is an automated email from the ASF dual-hosted git repository.
tillt pushed a commit to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 35869e5966c59a4047b8e409e7795e38a6b6d5c0
Author: Till Toenshoff <toensh...@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.6.2 **
---
src/master/master.cpp | 54 ++----------------
src/master/master.hpp | 15 +++++
src/master/weights_handler.cpp | 12 +---
src/tests/master_tests.cpp | 122 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 143 insertions(+), 60 deletions(-)
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 3fdc791..3f923a7 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3649,17 +3649,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);
}
@@ -3712,17 +3702,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);
}
@@ -3779,17 +3759,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);
}
@@ -3831,17 +3801,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);
}
@@ -3924,11 +3884,7 @@ Future<bool> Master::authorizeSlave(
authorizeReserveResources(slaveInfo.resources(), principal));
}
- return collect(authorizations)
- .then([](const list<bool>& results)
- -> Future<bool> {
- return std::find(results.begin(), results.end(), false) == results.end();
- });
+ return collectAuthorizations(authorizations);
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 5ec764b..f01b743 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,6 +45,8 @@
#include <mesos/scheduler/scheduler.hpp>
+#include <process/collect.hpp>
+#include <process/future.hpp>
#include <process/limiter.hpp>
#include <process/http.hpp>
#include <process/owned.hpp>
@@ -2204,6 +2207,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 1053652..cd3a919 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 6a3a503..69bcbad 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;
@@ -9754,6 +9755,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 {
