You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by al...@apache.org on 2016/04/27 17:29:35 UTC
[2/2] mesos git commit: Added authorization to agents'
`/monitor/statistics` endpoints.
Added authorization to agents' `/monitor/statistics` endpoints.
Review: https://reviews.apache.org/r/46319/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/fbbcc6af
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/fbbcc6af
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/fbbcc6af
Branch: refs/heads/master
Commit: fbbcc6af67fe146bb1a12ead11ee96861280247b
Parents: 721a414
Author: Benjamin Bannier <be...@mesosphere.io>
Authored: Wed Apr 27 16:53:46 2016 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Wed Apr 27 17:28:27 2016 +0200
----------------------------------------------------------------------
src/slave/http.cpp | 70 +++++++++++------
src/slave/slave.hpp | 9 ++-
src/tests/slave_authorization_tests.cpp | 112 +++++++++++++++++++++++++++
3 files changed, 165 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/fbbcc6af/src/slave/http.cpp
----------------------------------------------------------------------
diff --git a/src/slave/http.cpp b/src/slave/http.cpp
index 1196c93..9b55886 100644
--- a/src/slave/http.cpp
+++ b/src/slave/http.cpp
@@ -32,8 +32,9 @@
#include <process/collect.hpp>
#include <process/help.hpp>
-#include <process/owned.hpp>
+#include <process/http.hpp>
#include <process/limiter.hpp>
+#include <process/owned.hpp>
#include <process/metrics/metrics.hpp>
@@ -621,40 +622,59 @@ string Slave::Http::STATISTICS_HELP()
Future<Response> Slave::Http::statistics(
const Request& request,
- const Option<string>& /* principal */) const
+ const Option<string>& principal) const
{
- return statisticsLimiter->acquire()
- .then(defer(slave->self(), &Slave::usage))
- .then([=](const Future<ResourceUsage>& usage) -> Future<Response> {
- JSON::Array result;
-
- foreach (const ResourceUsage::Executor& executor,
- usage.get().executors()) {
- if (executor.has_statistics()) {
- const ExecutorInfo info = executor.executor_info();
-
- JSON::Object entry;
- entry.values["framework_id"] = info.framework_id().value();
- entry.values["executor_id"] = info.executor_id().value();
- entry.values["executor_name"] = info.name();
- entry.values["source"] = info.source();
- entry.values["statistics"] = JSON::protobuf(executor.statistics());
+ const PID<Slave> pid = slave->self();
+ Shared<RateLimiter> limiter = statisticsLimiter;
- result.values.push_back(entry);
- }
- }
+ return authorizeEndpoint(request, principal)
+ .then(defer(
+ pid,
+ [pid, limiter, request](bool authorized) -> Future<Response> {
+ if (!authorized) {
+ return Forbidden();
+ }
- return process::http::OK(result, request.url.query.get("jsonp"));
- })
+ return limiter->acquire()
+ .then(defer(pid, &Slave::usage))
+ .then(defer(pid, [request](const ResourceUsage& usage) {
+ return _statistics(usage, request);
+ }));
+ }))
.repair([](const Future<Response>& future) {
- LOG(WARNING) << "Could not collect resource usage: "
+ LOG(WARNING) << "Could not collect statistics: "
<< (future.isFailed() ? future.failure() : "discarded");
- return process::http::InternalServerError();
+ return InternalServerError();
});
}
+Response Slave::Http::_statistics(
+ const ResourceUsage& usage,
+ const Request& request)
+{
+ JSON::Array result;
+
+ foreach (const ResourceUsage::Executor& executor, usage.executors()) {
+ if (executor.has_statistics()) {
+ const ExecutorInfo info = executor.executor_info();
+
+ JSON::Object entry;
+ entry.values["framework_id"] = info.framework_id().value();
+ entry.values["executor_id"] = info.executor_id().value();
+ entry.values["executor_name"] = info.name();
+ entry.values["source"] = info.source();
+ entry.values["statistics"] = JSON::protobuf(executor.statistics());
+
+ result.values.push_back(entry);
+ }
+ }
+
+ return OK(result, request.url.query.get("jsonp"));
+}
+
+
string Slave::Http::CONTAINERS_HELP()
{
return HELP(
http://git-wip-us.apache.org/repos/asf/mesos/blob/fbbcc6af/src/slave/slave.hpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp
index 4b8bb07..b724380 100644
--- a/src/slave/slave.hpp
+++ b/src/slave/slave.hpp
@@ -454,7 +454,7 @@ private:
// /slave/monitor/statistics.json
process::Future<process::http::Response> statistics(
const process::http::Request& request,
- const Option<std::string>& /* principal */) const;
+ const Option<std::string>& principal) const;
// /slave/containers
process::Future<process::http::Response> containers(
@@ -481,6 +481,13 @@ private:
const process::http::Request& request,
const Option<std::string>& principal) const;
+
+ // Make continuation for `statistics` `static` as it might
+ // execute when the invoking `Http` is already destructed.
+ static process::http::Response _statistics(
+ const ResourceUsage& usage,
+ const process::http::Request& request);
+
Slave* slave;
// Used to rate limit the statistics endpoint.
http://git-wip-us.apache.org/repos/asf/mesos/blob/fbbcc6af/src/tests/slave_authorization_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/slave_authorization_tests.cpp b/src/tests/slave_authorization_tests.cpp
index 61e94c8..fc3aef7 100644
--- a/src/tests/slave_authorization_tests.cpp
+++ b/src/tests/slave_authorization_tests.cpp
@@ -16,6 +16,8 @@
#include <string>
+#include <gmock/gmock.h>
+
#include <gtest/gtest.h>
#include <mesos/authorizer/authorizer.hpp>
@@ -51,6 +53,8 @@ using process::http::Response;
using std::string;
+using testing::DoAll;
+
namespace mesos {
namespace internal {
namespace tests {
@@ -157,6 +161,114 @@ TYPED_TEST(SlaveAuthorizationTest, AuthorizeFlagsEndpointWithoutPrincipal)
<< response.get().body;
}
+
+// Parameterized fixture for agent-specific authorization tests. The
+// path of the tested endpoint is passed as the only parameter.
+class SlaveEndpointTest:
+ public MesosTest,
+ public ::testing::WithParamInterface<string> {};
+
+
+// The tests are parameterized by the endpoint being queried.
+//
+// TODO(bbannier): Once agent endpoint handlers use more than just
+// `GET_ENDPOINT_WITH_PATH`, we should consider parameterizing
+// `SlaveEndpointTest` by the authorization action as well.
+INSTANTIATE_TEST_CASE_P(
+ Endpoint,
+ SlaveEndpointTest,
+ ::testing::Values(
+ "monitor/statistics", "monitor/statistics.json", "flags"));
+
+
+// Tests that an agent endpoint handler form
+// correct queries against the authorizer.
+TEST_P(SlaveEndpointTest, AuthorizedRequest)
+{
+ const string endpoint = GetParam();
+
+ StandaloneMasterDetector detector;
+
+ MockAuthorizer mockAuthorizer;
+
+ Try<Owned<cluster::Slave>> agent = StartSlave(&detector, &mockAuthorizer);
+ ASSERT_SOME(agent);
+
+ Future<authorization::Request> request;
+ EXPECT_CALL(mockAuthorizer, authorized(_))
+ .WillOnce(DoAll(FutureArg<0>(&request),
+ Return(true)));
+
+ Future<Response> response = http::get(
+ agent.get()->pid,
+ endpoint,
+ None(),
+ createBasicAuthHeaders(DEFAULT_CREDENTIAL));
+
+ AWAIT_READY(request);
+
+ EXPECT_EQ(DEFAULT_CREDENTIAL.principal(), request.get().subject().value());
+
+ // TODO(bbannier): Once agent endpoint handlers use more than just
+ // `GET_ENDPOINT_WITH_PATH` we should factor out the request method
+ // and expected authorization action and parameterize
+ // `SlaveEndpointTest` on that as well in addition to the endpoint.
+ EXPECT_EQ(authorization::GET_ENDPOINT_WITH_PATH, request.get().action());
+
+ EXPECT_EQ("/" + endpoint, request.get().object().value());
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response)
+ << response.get().body;
+}
+
+
+// Tests that unauthorized requests for an agent endpoint are properly rejected.
+TEST_P(SlaveEndpointTest, UnauthorizedRequest)
+{
+ const string endpoint = GetParam();
+
+ StandaloneMasterDetector detector;
+
+ MockAuthorizer mockAuthorizer;
+
+ Try<Owned<cluster::Slave>> agent = StartSlave(&detector, &mockAuthorizer);
+ ASSERT_SOME(agent);
+
+ EXPECT_CALL(mockAuthorizer, authorized(_))
+ .WillOnce(Return(false));
+
+ Future<Response> response = http::get(
+ agent.get()->pid,
+ endpoint,
+ None(),
+ createBasicAuthHeaders(DEFAULT_CREDENTIAL));
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response)
+ << response.get().body;
+}
+
+
+// Tests that requests for an agent endpoint
+// always succeed if the authorizer is absent.
+TEST_P(SlaveEndpointTest, NoAuthorizer)
+{
+ const string endpoint = GetParam();
+
+ StandaloneMasterDetector detector;
+
+ Try<Owned<cluster::Slave>> agent = StartSlave(&detector, CreateSlaveFlags());
+ ASSERT_SOME(agent);
+
+ Future<Response> response = http::get(
+ agent.get()->pid,
+ endpoint,
+ None(),
+ createBasicAuthHeaders(DEFAULT_CREDENTIAL));
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response)
+ << response.get().body;
+}
+
} // namespace tests {
} // namespace internal {
} // namespace mesos {