You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2016/02/26 20:43:16 UTC

[1/6] mesos git commit: Changed object of `ReserveResources` ACL to `roles`.

Repository: mesos
Updated Branches:
  refs/heads/master 437680300 -> 895012058


Changed object of `ReserveResources` ACL to `roles`.

This solves a problem in which any principal could reserve resources for
any role using the '/reserve' operator endpoint. A new test,
`ReserveOperationValidationTest.DisallowReserveForStarRole`, was added.

Review: https://reviews.apache.org/r/43776/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/f52bce71
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/f52bce71
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/f52bce71

Branch: refs/heads/master
Commit: f52bce71de833dd2fed1fa772c61f24b683ef452
Parents: 4376803
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:42:14 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:42:14 2016 -0800

----------------------------------------------------------------------
 include/mesos/authorizer/authorizer.proto |  7 +++--
 src/authorizer/local/authorizer.cpp       |  4 +--
 src/master/master.cpp                     | 13 ++++++---
 src/master/validation.cpp                 |  6 -----
 src/tests/authorization_tests.cpp         | 37 +++++++++++---------------
 src/tests/master_validation_tests.cpp     | 34 ++++++++++++-----------
 src/tests/reservation_endpoints_tests.cpp | 14 +++++-----
 src/tests/reservation_tests.cpp           | 17 ++++++------
 8 files changed, 66 insertions(+), 66 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/include/mesos/authorizer/authorizer.proto
----------------------------------------------------------------------
diff --git a/include/mesos/authorizer/authorizer.proto b/include/mesos/authorizer/authorizer.proto
index 226441f..c9fc237 100644
--- a/include/mesos/authorizer/authorizer.proto
+++ b/include/mesos/authorizer/authorizer.proto
@@ -79,14 +79,13 @@ message ACL {
     required Entity framework_principals = 2;
   }
 
-  // Specifies which Resources a principal can reserve.
+  // Specifies which roles a principal can reserve resources for.
   message ReserveResources {
     // Subjects: Framework principal or Operator username.
     required Entity principals = 1;
 
-    // Objects: The kind of Resources that can be reserved.
-    // NOTE: Currently the only valid values are ANY and NONE.
-    required Entity resources = 2;
+    // Objects: The principal(s) can reserve resources for these roles.
+    required Entity roles = 2;
   }
 
   // Specifies which principals can unreserve which principals'

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/authorizer/local/authorizer.cpp
----------------------------------------------------------------------
diff --git a/src/authorizer/local/authorizer.cpp b/src/authorizer/local/authorizer.cpp
index 9557bbd..ebb0ec8 100644
--- a/src/authorizer/local/authorizer.cpp
+++ b/src/authorizer/local/authorizer.cpp
@@ -108,10 +108,10 @@ public:
     foreach (const ACL::ReserveResources& acl, acls.reserve_resources()) {
       // ACL matches if both subjects and objects match.
       if (matches(request.principals(), acl.principals()) &&
-          matches(request.resources(), acl.resources())) {
+          matches(request.roles(), acl.roles())) {
         // ACL is allowed if both subjects and objects are allowed.
         return allows(request.principals(), acl.principals()) &&
-               allows(request.resources(), acl.resources());
+               allows(request.roles(), acl.roles());
       }
     }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 8d6d3c6..5c81ce3 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -2855,9 +2855,16 @@ Future<bool> Master::authorizeReserveResources(
     request.mutable_principals()->set_type(ACL::Entity::ANY);
   }
 
-  // TODO(mpark): Determine what kinds of constraints we may want to
-  // enforce on resources. Currently, we simply use ANY.
-  request.mutable_resources()->set_type(ACL::Entity::ANY);
+  // The operation will be authorized if the principal is allowed to make
+  // reservations for all roles included in `reserve.resources`.
+  // Add an element to `request.roles` for each unique role in the resources.
+  hashset<string> roles;
+  foreach (const Resource& resource, reserve.resources()) {
+    if (!roles.contains(resource.role())) {
+      request.mutable_roles()->add_values(resource.role());
+      roles.insert(resource.role());
+    }
+  }
 
   LOG(INFO) << "Authorizing principal '"
             << (principal.isSome() ? principal.get() : "ANY")

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index b0cc7f7..ec26981 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -666,12 +666,6 @@ Option<Error> validate(
           "Resource " + stringify(resource) + " is not dynamically reserved");
     }
 
-    if (role.isSome() && resource.role() != role.get()) {
-      return Error(
-          "The reserved resource's role '" + resource.role() +
-          "' does not match the framework's role '" + role.get() + "'");
-    }
-
     if (principal.isSome()) {
       if (!resource.reservation().has_principal()) {
         return Error(

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/tests/authorization_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/authorization_tests.cpp b/src/tests/authorization_tests.cpp
index 9d046e8..27061f1 100644
--- a/src/tests/authorization_tests.cpp
+++ b/src/tests/authorization_tests.cpp
@@ -405,31 +405,26 @@ TYPED_TEST(AuthorizationTest, PrincipalNotOfferedAnyRoleRestrictive)
 }
 
 
-// This tests the authorization of ACLs used for the dynamic reservation
-// of resources.
-//
-// NOTE: at this time, principals can only be authorized to reserve
-// ANY or NONE.  However, this test exercises the full capabilities of
-// ACL authorization, so specific resource types are tested as well.
+// Tests the authorization of ACLs used for dynamic reservation of resources.
 TYPED_TEST(AuthorizationTest, Reserve)
 {
   ACLs acls;
 
-  // "foo" and "bar" principals can reserve any resources.
+  // Principals "foo" and "bar" can reserve resources for any role.
   mesos::ACL::ReserveResources* acl1 = acls.add_reserve_resources();
   acl1->mutable_principals()->add_values("foo");
   acl1->mutable_principals()->add_values("bar");
-  acl1->mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  acl1->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
-  // "baz" principal can reserve memory.
+  // Principal "baz" can only reserve resources for the "ads" role.
   mesos::ACL::ReserveResources* acl2 = acls.add_reserve_resources();
   acl2->mutable_principals()->add_values("baz");
-  acl2->mutable_resources()->add_values("mem");
+  acl2->mutable_roles()->add_values("ads");
 
   // No other principals can reserve resources.
   mesos::ACL::ReserveResources* acl3 = acls.add_reserve_resources();
   acl3->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
-  acl3->mutable_resources()->set_type(mesos::ACL::Entity::NONE);
+  acl3->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
 
   // Create an `Authorizer` with the ACLs.
   Try<Authorizer*> create = TypeParam::create();
@@ -439,43 +434,43 @@ TYPED_TEST(AuthorizationTest, Reserve)
   Try<Nothing> initialized = authorizer.get()->initialize(acls);
   ASSERT_SOME(initialized);
 
-  // Principals "foo" and "bar" can reserve any resources,
+  // Principals "foo" and "bar" can reserve resources for any role,
   // so requests 1 and 2 will pass.
   mesos::ACL::ReserveResources request1;
   request1.mutable_principals()->add_values("foo");
   request1.mutable_principals()->add_values("bar");
-  request1.mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  request1.mutable_roles()->set_type(mesos::ACL::Entity::ANY);
   AWAIT_EXPECT_TRUE(authorizer.get()->authorize(request1));
 
   mesos::ACL::ReserveResources request2;
   request2.mutable_principals()->add_values("foo");
   request2.mutable_principals()->add_values("bar");
-  request2.mutable_resources()->add_values("disk");
+  request2.mutable_roles()->add_values("awesome_role");
   AWAIT_EXPECT_TRUE(authorizer.get()->authorize(request2));
 
-  // Principal "baz" can reserve memory, so this will pass.
+  // Principal "baz" can only reserve resources for the "ads" role, so request 3
+  // will pass, but requests 4 and 5 will fail.
   mesos::ACL::ReserveResources request3;
   request3.mutable_principals()->add_values("baz");
-  request3.mutable_resources()->add_values("mem");
+  request3.mutable_roles()->add_values("ads");
   AWAIT_EXPECT_TRUE(authorizer.get()->authorize(request3));
 
-  // Principal "baz" can only reserve memory, so requests 4 and 5 will fail.
   mesos::ACL::ReserveResources request4;
   request4.mutable_principals()->add_values("baz");
-  request4.mutable_resources()->add_values("disk");
+  request4.mutable_roles()->add_values("awesome_role");
   AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request4));
 
   mesos::ACL::ReserveResources request5;
   request5.mutable_principals()->add_values("baz");
-  request5.mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  request5.mutable_roles()->set_type(mesos::ACL::Entity::ANY);
   AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request5));
 
   // Principal "zelda" is not mentioned in the ACLs of the Authorizer, so it
   // will be caught by the final ACL, which provides a default case that denies
-  // access for all other principals. This case will fail.
+  // access for all other principals. This request will fail.
   mesos::ACL::ReserveResources request6;
   request6.mutable_principals()->add_values("zelda");
-  request6.mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  request6.mutable_roles()->add_values("ads");
   AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request6));
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index ab2df22..7db7336 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -217,23 +217,27 @@ TEST_F(ReserveOperationValidationTest, DisallowStarRoleFrameworks)
 }
 
 
-// This test verifies that validation fails if the 'role'
-// specified in the resources of the RESERVE operation does not
-// match the framework's 'role'.
-TEST_F(ReserveOperationValidationTest, NonMatchingRole)
+// This test verifies that validation fails if the framework attempts
+// to reserve for the "*" role.
+TEST_F(ReserveOperationValidationTest, DisallowReserveForStarRole)
 {
-  {
-    // Non-matching role, "role" reserving for "*".
-    Resource resource = Resources::parse("cpus", "8", "*").get();
-    resource.mutable_reservation()->CopyFrom(
-        createReservationInfo("principal"));
+  // Principal "principal" reserving for "*".
+  Resource resource = Resources::parse("cpus", "8", "*").get();
+  resource.mutable_reservation()->CopyFrom(
+      createReservationInfo("principal"));
 
-    Offer::Operation::Reserve reserve;
-    reserve.add_resources()->CopyFrom(resource);
+  Offer::Operation::Reserve reserve;
+  reserve.add_resources()->CopyFrom(resource);
+
+  EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+}
 
-    EXPECT_SOME(operation::validate(reserve, "role", "principal"));
-  }
 
+// This test verifies that validation succeeds even if the 'role'
+// specified in the resources of the RESERVE operation does not
+// match the framework's 'role'.
+TEST_F(ReserveOperationValidationTest, NonMatchingRole)
+{
   {
     // Non-matching role, "*" reserving for "role".
     Resource resource = Resources::parse("cpus", "8", "role").get();
@@ -243,7 +247,7 @@ TEST_F(ReserveOperationValidationTest, NonMatchingRole)
     Offer::Operation::Reserve reserve;
     reserve.add_resources()->CopyFrom(resource);
 
-    EXPECT_SOME(operation::validate(reserve, "*", "principal"));
+    EXPECT_NONE(operation::validate(reserve, "*", "principal"));
   }
 
   {
@@ -255,7 +259,7 @@ TEST_F(ReserveOperationValidationTest, NonMatchingRole)
     Offer::Operation::Reserve reserve;
     reserve.add_resources()->CopyFrom(resource);
 
-    EXPECT_SOME(operation::validate(reserve, "role1", "principal"));
+    EXPECT_NONE(operation::validate(reserve, "role1", "principal"));
   }
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/tests/reservation_endpoints_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/reservation_endpoints_tests.cpp b/src/tests/reservation_endpoints_tests.cpp
index 32b2af4..8c582f7 100644
--- a/src/tests/reservation_endpoints_tests.cpp
+++ b/src/tests/reservation_endpoints_tests.cpp
@@ -964,14 +964,14 @@ TEST_F(ReservationEndpointsTest, GoodReserveAndUnreserveACL)
   TestAllocator<> allocator;
   ACLs acls;
 
-  // This ACL asserts that the DEFAULT_CREDENTIAL's
-  // principal can reserve ANY resources.
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // can reserve resources for any role.
   mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
   reserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  reserve->mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
-  // This ACL asserts that the DEFAULT_CREDENTIAL's
-  // principal can unreserve its own resources.
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // can unreserve its own resources.
   mesos::ACL::UnreserveResources* unreserve = acls.add_unreserve_resources();
   unreserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
   unreserve->mutable_reserver_principals()->add_values(
@@ -1040,10 +1040,10 @@ TEST_F(ReservationEndpointsTest, BadReserveACL)
   ACLs acls;
 
   // This ACL asserts that ANY principal can reserve NONE,
-  // i.e. no principals can reserve anything.
+  // i.e. no principal can reserve resources.
   mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
   reserve->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
-  reserve->mutable_resources()->set_type(mesos::ACL::Entity::NONE);
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
 
   FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
   frameworkInfo.set_role("role");

http://git-wip-us.apache.org/repos/asf/mesos/blob/f52bce71/src/tests/reservation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/reservation_tests.cpp b/src/tests/reservation_tests.cpp
index b8878d5..d7f9de6 100644
--- a/src/tests/reservation_tests.cpp
+++ b/src/tests/reservation_tests.cpp
@@ -1336,12 +1336,13 @@ TEST_F(ReservationTest, GoodACLReserveThenUnreserve)
 {
   ACLs acls;
 
-  // This principal can reserve any resources.
+  // The principal of `DEFAULT_CREDENTIAL` can reserve resources for any role.
   mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
   reserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  reserve->mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
-  // This principal can unreserve its own reserved resources.
+  // The principal of `DEFAULT_CREDENTIAL` can unreserve
+  // its own reserved resources.
   mesos::ACL::UnreserveResources* unreserve = acls.add_unreserve_resources();
   unreserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
   unreserve->mutable_reserver_principals()->add_values(
@@ -1445,7 +1446,7 @@ TEST_F(ReservationTest, BadACLDropReserve)
   // No entity can reserve any resources.
   mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
   reserve->mutable_principals()->set_type(mesos::ACL::Entity::NONE);
-  reserve->mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
   frameworkInfo.set_role("role");
@@ -1528,10 +1529,10 @@ TEST_F(ReservationTest, BadACLDropUnreserve)
 {
   ACLs acls;
 
-  // This principal can reserve any resources.
+  // This principal can reserve resources for any role.
   mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
   reserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  reserve->mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This principal cannot unreserve any resources.
   mesos::ACL::UnreserveResources* unreserve = acls.add_unreserve_resources();
@@ -1655,10 +1656,10 @@ TEST_F(ReservationTest, ACLMultipleOperations)
 
   ACLs acls;
 
-  // This principal can reserve any resources.
+  // This principal can reserve resources for any role.
   mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
   reserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  reserve->mutable_resources()->set_type(mesos::ACL::Entity::ANY);
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This principal cannot unreserve any resources.
   mesos::ACL::UnreserveResources* unreserve = acls.add_unreserve_resources();


[6/6] mesos git commit: Updated docs for reservation, volumes, and authZ.

Posted by ji...@apache.org.
Updated docs for reservation, volumes, and authZ.

This updates the authorization documentation to include the new `roles`
object for the `CreateVolume` and `ReserveResources` ACLs. The docs for
persistent volumes and dynamic reservations are also updated to reflect
the new authorization behavior. A note has been added to `upgrades.md`
detailing the impact of these changes on upgrades.

Review: https://reviews.apache.org/r/43800/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/89501205
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/89501205
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/89501205

Branch: refs/heads/master
Commit: 895012058d306715b7e44890f65801496bd7e86b
Parents: b234d36
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:43:01 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:43:01 2016 -0800

----------------------------------------------------------------------
 docs/authorization.md     | 81 ++++++++++++++++++++++++++++++++----------
 docs/persistent-volume.md | 13 ++++---
 docs/reservation.md       | 20 +++++------
 docs/upgrades.md          |  4 +++
 4 files changed, 85 insertions(+), 33 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/89501205/docs/authorization.md
----------------------------------------------------------------------
diff --git a/docs/authorization.md b/docs/authorization.md
index bbb4f2a..feb0ddd 100644
--- a/docs/authorization.md
+++ b/docs/authorization.md
@@ -36,18 +36,16 @@ The currently supported `Subjects` are:
 
 1. "principals"
 	- Framework principals (used by "register_frameworks", "run_tasks", "reserve", "unreserve", "create_volumes", and "destroy_volumes" actions)
-	- Usernames (used by "teardown_frameworks", "set_quotas", "remove_quotas", "reserve", "unreserve", "create_volumes", and "destroy_volumes" actions)
+	- Operator usernames (used by "teardown_frameworks", "set_quotas", "remove_quotas", "reserve", "unreserve", "create_volumes", and "destroy_volumes" actions)
 
 The currently supported `Objects` are:
 
-1. "roles": Resource [roles](roles.md) that framework can register with (used by "register_frameworks" and "set_quotas" actions)
-2. "users": Unix user to launch the task/executor as (used by "run_tasks" actions)
+1. "roles": Resource [roles](roles.md) that frameworks can register with, [reserve resources](reservation.md) for, or create [persistent volumes](persistent-volume.md) for (used by "register_frameworks", "set_quotas", "reserve_resources", and "create_volumes" actions).
+2. "users": Unix user to launch the task/executor as (used by "run_tasks" actions).
 3. "framework_principals": Framework principals that can be torn down by HTTP POST (used by "teardown_frameworks" actions).
-4. "resources": Resources that can be reserved. Currently the only types considered by the default authorizer are `ANY` and `NONE` (used by "reserves" action).
-5. "reserver_principals": Framework principals whose reserved resources can be unreserved (used by "unreserves" action).
-6. "volume_types": Types of volumes that can be created by a given principal. Currently the only types considered by the default authorizer are `ANY` and `NONE` (used by "create_volumes" action).
-7. "creator_principals": Principals whose persistent volumes can be destroyed (used by "destroy_volumes" action).
-8. "quota_principals": Principals that set the quota to be removed (used by "remove_quotas" action)
+4. "reserver_principals": Framework principals whose reserved resources can be unreserved (used by "unreserves" action).
+5. "creator_principals": Principals whose persistent volumes can be destroyed (used by "destroy_volumes" action).
+6. "quota_principals": Principals that set the quota to be removed (used by "remove_quotas" action).
 
 > NOTE: Both `Subjects` and `Objects` can be either an array of strings or one of the special values `ANY` or `NONE`.
 
@@ -214,7 +212,7 @@ In a real-world organization, principals and roles might be used to represent va
                                  ]
         }
 
-9. The principal `foo` can reserve any resources, and no other principal can reserve resources.
+9. The principal `foo` can reserve resources for any role, and no other principal can reserve resources.
 
         {
           "permissive": false,
@@ -223,14 +221,14 @@ In a real-world organization, principals and roles might be used to represent va
                                    "principals": {
                                      "values": ["foo"]
                                    },
-                                   "resources": {
+                                   "roles": {
                                      "type": "ANY"
                                    }
                                  }
                                ]
         }
 
-10. The principal `foo` cannot reserve any resources, and any other principal (or framework without a principal) can reserve resources.
+10. The principal `foo` cannot reserve resources, and any other principal (or framework without a principal) can reserve resources for any role.
 
         {
           "reserve_resources": [
@@ -238,14 +236,30 @@ In a real-world organization, principals and roles might be used to represent va
                                    "principals": {
                                      "values": ["foo"]
                                    },
-                                   "resources": {
+                                   "roles": {
                                      "type": "NONE"
                                    }
                                  }
                                ]
         }
 
-11. The principal `foo` can unreserve resources reserved by itself and by the principal `bar`. The principal `bar`, however, can only unreserve its own resources. No other principals can unreserve resources.
+11. The principal `foo` can reserve resources only for roles `prod` and `dev`, and no other principal (or framework without a principal) can reserve resources for any role.
+
+        {
+          "permissive": false,
+          "reserve_resources": [
+                                 {
+                                   "principals": {
+                                     "values": ["foo"]
+                                   },
+                                   "roles": {
+                                     "values": ["prod", "dev"]
+                                   }
+                                 }
+                               ]
+        }
+
+12. The principal `foo` can unreserve resources reserved by itself and by the principal `bar`. The principal `bar`, however, can only unreserve its own resources. No other principals can unreserve resources.
 
         {
           "permissive": false,
@@ -269,7 +283,7 @@ In a real-world organization, principals and roles might be used to represent va
                                  ]
         }
 
-12. The principal `foo` can create persistent volumes, and no other principal can create persistent volumes.
+13. The principal `foo` can create persistent volumes for any role, and no other principal can create persistent volumes.
 
         {
           "permissive": false,
@@ -278,14 +292,45 @@ In a real-world organization, principals and roles might be used to represent va
                                 "principals": {
                                   "values": ["foo"]
                                 },
-                                "volume_types": {
+                                "roles": {
                                   "type": "ANY"
                                 }
                               }
                             ]
         }
 
-13. The principal `foo` can destroy volumes created by itself and by the principal `bar`. The principal `bar`, however, can only destroy its own volumes. No other principals can destroy volumes.
+14. The principal `foo` cannot create persistent volumes for any role, and any other principal can create persistent volumes for any role.
+
+        {
+          "create_volumes": [
+                              {
+                                "principals": {
+                                  "values": ["foo"]
+                                },
+                                "roles": {
+                                  "type": "NONE"
+                                }
+                              }
+                            ]
+        }
+
+15. The principal `foo` can create persistent volumes only for roles `prod` and `dev`, and no other principal can create persistent volumes for any role.
+
+        {
+          "permissive": false,
+          "create_volumes": [
+                              {
+                                "principals": {
+                                  "values": ["foo"]
+                                },
+                                "roles": {
+                                  "values": ["prod", "dev"]
+                                }
+                              }
+                            ]
+        }
+
+16. The principal `foo` can destroy volumes created by itself and by the principal `bar`. The principal `bar`, however, can only destroy its own volumes. No other principals can destroy volumes.
 
         {
           "permissive": false,
@@ -309,7 +354,7 @@ In a real-world organization, principals and roles might be used to represent va
                              ]
         }
 
-14. The principal `ops` can set quota for any role. The principal `foo`, however, can only set quota for `foo-role`. No other principals can set quota.
+17. The principal `ops` can set quota for any role. The principal `foo`, however, can only set quota for `foo-role`. No other principals can set quota.
 
         {
           "permissive": false,
@@ -333,7 +378,7 @@ In a real-world organization, principals and roles might be used to represent va
                         ]
         }
 
-15. The principal `ops` can remove quota which was set by any principal. The principal `foo`, however, can only remove quota which was set by itself. No other principals can remove quota.
+18. The principal `ops` can remove quota which was set by any principal. The principal `foo`, however, can only remove quota which was set by itself. No other principals can remove quota.
 
         {
           "permissive": false,

http://git-wip-us.apache.org/repos/asf/mesos/blob/89501205/docs/persistent-volume.md
----------------------------------------------------------------------
diff --git a/docs/persistent-volume.md b/docs/persistent-volume.md
index 2a794a5..f772405 100644
--- a/docs/persistent-volume.md
+++ b/docs/persistent-volume.md
@@ -29,11 +29,14 @@ Persistent volumes can also be created on isolated and auxiliary disks by
 reserving [multiple disk resources](multiple-disk.md).
 
 Persistent volumes can be created by __operators__ and authorized
-__frameworks__. We require a `principal` from the operator or framework in order
-to authenticate/authorize the operations. Permissions are specified via the
-existing ACL mechanism. To use authorization with reserve, unreserve, create,
-and destroy operations, the Mesos master must be configured with the desired
-ACLs. For more information, see the
+__frameworks__. By default, frameworks and operators can create volumes for any
+role and destroy any persistent volumes. [Authorization](authorization.md)
+allows this behavior to be limited so that volumes can only be created for
+particular roles and only particular volumes can be destroyed. For these
+operations to be authorized, the framework or operator should provide a
+`principal` to identify itself. To use authorization with reserve, unreserve,
+create, and destroy operations, the Mesos master must be configured with the
+appropriate ACLs. For more information, see the
 [authorization documentation](authorization.md).
 
 * `Offer::Operation::Create` and `Offer::Operation::Destroy` messages are

http://git-wip-us.apache.org/repos/asf/mesos/blob/89501205/docs/reservation.md
----------------------------------------------------------------------
diff --git a/docs/reservation.md b/docs/reservation.md
index b98ebe6..cbf0a08 100644
--- a/docs/reservation.md
+++ b/docs/reservation.md
@@ -45,20 +45,20 @@ That is, statically reserved resources cannot be reserved for another role nor
 be unreserved. Dynamic reservation enables operators and authorized frameworks
 to reserve and unreserve resources after slave-startup.
 
-We require a `principal` from the operator or framework in order to
-authenticate/authorize the operations. Permissions are specified via the
-existing ACL mechanism. To use authorization with reserve/unreserve operations,
-the Mesos master must be configured with the desired ACLs. For more information,
-see the [authorization documentation](authorization.md).
+By default, frameworks and operators can reserve resources for any role, and can
+unreserve any dynamically reserved resources. [Authorization](authorization.md)
+allows this behavior to be limited so that only particular roles can be reserved
+for, and only particular resources can be unreserved. For these operations to be
+authorized, the framework or operator should provide a `principal` to identify
+itself. To use authorization with reserve/unreserve operations, the Mesos master
+must be configured with the appropriate ACLs. For more information, see the
+[authorization documentation](authorization.md).
 
 * `Offer::Operation::Reserve` and `Offer::Operation::Unreserve` messages are
   available for __frameworks__ to send back via the `acceptOffers` API as a
-  response to a resource offer. Each framework may only reserve resources for
-  its own role.
+  response to a resource offer.
 * `/reserve` and `/unreserve` HTTP endpoints allow __operators__ to manage
-  dynamic reservations through the master. Operators may currently reserve
-  resources for any role, although this
-  [will change](https://issues.apache.org/jira/browse/MESOS-4591).
+  dynamic reservations through the master.
 
 In the following sections, we will walk through examples of each of the
 interfaces described above.

http://git-wip-us.apache.org/repos/asf/mesos/blob/89501205/docs/upgrades.md
----------------------------------------------------------------------
diff --git a/docs/upgrades.md b/docs/upgrades.md
index 4f30d72..21faea8 100644
--- a/docs/upgrades.md
+++ b/docs/upgrades.md
@@ -6,6 +6,10 @@ layout: documentation
 
 This document serves as a guide for users who wish to upgrade an existing Mesos cluster. Some versions require particular upgrade techniques when upgrading a running cluster. Some upgrades will have incompatible changes.
 
+## Upgrading from 0.27.x to 0.28.x ##
+
+* Mesos 0.28 changes the definitions of two ACLs used for authorization. The objects of the `ReserveResources` and `CreateVolume` ACLs have been changed to `roles`. In both cases, principals can now be authorized to perform these operations for particular roles. This means that by default, a framework or operator can reserve resources/create volumes for any role. To restrict this behavior, [ACLs can be added](authorization.md) to the master which authorize principals to reserve resources/create volumes for specified roles only. Previously, frameworks could only reserve resources for their own role; this behavior can be preserved by configuring the `ReserveResources` ACLs such that the framework's principal is only authorized to reserve for the framework's role. **NOTE** This renders existing `ReserveResources` and `CreateVolume` ACL definitions obsolete; if you are authorizing these operations, your ACL definitions should be updated.
+
 ## Upgrading from 0.26.x to 0.27.x ##
 
 * Mesos 0.27 introduces the concept of _implicit roles_. In previous releases, configuring roles required specifying a static whitelist of valid role names on master startup (via the `--roles` flag). In Mesos 0.27, if `--roles` is omitted, _any_ role name can be used; controlling which principals are allowed to register as which roles should be done using [ACLs](authorization.md). The role whitelist functionality is still supported but is deprecated.


[5/6] mesos git commit: Added '/reserve' tests with multiple roles.

Posted by ji...@apache.org.
Added '/reserve' tests with multiple roles.

Operators may reserve resources for multiple roles in the same
operation; this patch adds tests to confirm correct behavior of
authorization in this case. The tests
`ReservationEndpointsTest.GoodReserveACLMultipleRoles` and
`ReservationEndpointsTest.BadReserveACLMultipleRoles` were added.

Review: https://reviews.apache.org/r/43779/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b234d364
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b234d364
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b234d364

Branch: refs/heads/master
Commit: b234d3641e65fdd4f6ec845d1015bd3f50184914
Parents: 27ce052
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:42:55 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:42:55 2016 -0800

----------------------------------------------------------------------
 src/tests/reservation_endpoints_tests.cpp | 124 +++++++++++++++++++++++++
 1 file changed, 124 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/b234d364/src/tests/reservation_endpoints_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/reservation_endpoints_tests.cpp b/src/tests/reservation_endpoints_tests.cpp
index 8c582f7..f3a1438 100644
--- a/src/tests/reservation_endpoints_tests.cpp
+++ b/src/tests/reservation_endpoints_tests.cpp
@@ -1032,6 +1032,63 @@ TEST_F(ReservationEndpointsTest, GoodReserveAndUnreserveACL)
 }
 
 
+// This tests that correct setup of `ReserveResources` ACLs allows the operator
+// to perform reserve operations for multiple roles successfully.
+TEST_F(ReservationEndpointsTest, GoodReserveACLMultipleRoles)
+{
+  TestAllocator<> allocator;
+  ACLs acls;
+
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // can reserve resources for any role.
+  mesos::ACL::ReserveResources* reserve = acls.add_reserve_resources();
+  reserve->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
+  reserve->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
+
+  // Create a master.
+  master::Flags masterFlags = CreateMasterFlags();
+  masterFlags.acls = acls;
+
+  EXPECT_CALL(allocator, initialize(_, _, _, _));
+
+  Try<PID<Master>> master = StartMaster(&allocator, masterFlags);
+  ASSERT_SOME(master);
+
+  // Create a slave.
+  slave::Flags slaveFlags = CreateSlaveFlags();
+  slaveFlags.resources = "cpus:2;mem:1024";
+
+  Future<SlaveID> slaveId;
+  EXPECT_CALL(allocator, addSlave(_, _, _, _, _))
+    .WillOnce(DoAll(InvokeAddSlave(&allocator),
+                    FutureArg<0>(&slaveId)));
+
+  Try<PID<Slave>> slave = StartSlave(slaveFlags);
+  ASSERT_SOME(slave);
+
+  Resources unreserved = Resources::parse("cpus:1;mem:512").get();
+  Resources dynamicallyReserved1 = unreserved.flatten(
+      "jedi_master",
+      createReservationInfo(DEFAULT_CREDENTIAL.principal()));
+  Resources dynamicallyReserved2 = unreserved.flatten(
+      "sith_lord",
+      createReservationInfo(DEFAULT_CREDENTIAL.principal()));
+  Resources dynamicallyReservedMultipleRoles =
+    dynamicallyReserved1 + dynamicallyReserved2;
+
+  // Reserve the resources.
+  Future<Response> response = process::http::post(
+      master.get(),
+      "reserve",
+      createBasicAuthHeaders(DEFAULT_CREDENTIAL),
+      createRequestBody(slaveId.get(), dynamicallyReservedMultipleRoles));
+
+  AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
+
+  Shutdown();
+}
+
+
 // This tests that an incorrect set-up of Reserve ACL disallows the
 // operator from performing reserve operations.
 TEST_F(ReservationEndpointsTest, BadReserveACL)
@@ -1161,6 +1218,73 @@ TEST_F(ReservationEndpointsTest, BadUnreserveACL)
 }
 
 
+// Tests that reserve operations will fail if multiple roles are included in a
+// request, while the principal attempting the reservation is not authorized to
+// reserve for one of them.
+TEST_F(ReservationEndpointsTest, BadReserveACLMultipleRoles)
+{
+  const string AUTHORIZED_ROLE = "panda";
+  const string UNAUTHORIZED_ROLE = "giraffe";
+
+  TestAllocator<> allocator;
+  ACLs acls;
+
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // can reserve resources for `AUTHORIZED_ROLE`.
+  mesos::ACL::ReserveResources* reserve1 = acls.add_reserve_resources();
+  reserve1->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
+  reserve1->mutable_roles()->add_values(AUTHORIZED_ROLE);
+
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // cannot reserve resources for any other role.
+  mesos::ACL::ReserveResources* reserve2 = acls.add_reserve_resources();
+  reserve2->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
+  reserve2->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
+
+  // Create a master.
+  master::Flags masterFlags = CreateMasterFlags();
+  masterFlags.acls = acls;
+
+  EXPECT_CALL(allocator, initialize(_, _, _, _));
+
+  Try<PID<Master>> master = StartMaster(&allocator, masterFlags);
+  ASSERT_SOME(master);
+
+  // Create a slave.
+  slave::Flags slaveFlags = CreateSlaveFlags();
+  slaveFlags.resources = "cpus:2;mem:1024";
+
+  Future<SlaveID> slaveId;
+  EXPECT_CALL(allocator, addSlave(_, _, _, _, _))
+    .WillOnce(DoAll(InvokeAddSlave(&allocator),
+                    FutureArg<0>(&slaveId)));
+
+  Try<PID<Slave>> slave = StartSlave(slaveFlags);
+  ASSERT_SOME(slave);
+
+  Resources unreserved = Resources::parse("cpus:1;mem:512").get();
+  Resources dynamicallyReserved1 = unreserved.flatten(
+      AUTHORIZED_ROLE,
+      createReservationInfo(DEFAULT_CREDENTIAL.principal()));
+  Resources dynamicallyReserved2 = unreserved.flatten(
+      UNAUTHORIZED_ROLE,
+      createReservationInfo(DEFAULT_CREDENTIAL.principal()));
+  Resources dynamicallyReservedMultipleRoles =
+    dynamicallyReserved1 + dynamicallyReserved2;
+
+  // Reserve the resources.
+  Future<Response> response = process::http::post(
+      master.get(),
+      "reserve",
+      createBasicAuthHeaders(DEFAULT_CREDENTIAL),
+      createRequestBody(slaveId.get(), dynamicallyReservedMultipleRoles));
+
+  AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response);
+
+  Shutdown();
+}
+
+
 // This tests that an attempt to reserve with no 'slaveId' results in a
 // 'BadRequest' HTTP error.
 TEST_F(ReservationEndpointsTest, NoSlaveId)


[2/6] mesos git commit: Removed unnecessary parameter from validation function.

Posted by ji...@apache.org.
Removed unnecessary parameter from validation function.

Now that Reserve operations are authorized for particular roles, it is
unnecessary to pass the framework's role into the validation function
for Reserve operations. The function previously ensured that a framework
could only reserve resources for its own role, but this check has been
removed. Since this check has been removed, the test
`ReserveOperationValidationTest.NonMatchingRole` is no longer needed and
has also been removed.

Review: https://reviews.apache.org/r/43777/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/082ab0ee
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/082ab0ee
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/082ab0ee

Branch: refs/heads/master
Commit: 082ab0ee9d86ad29c2d19ac103395436612ff379
Parents: f52bce7
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:42:26 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:42:26 2016 -0800

----------------------------------------------------------------------
 src/master/http.cpp                   |  2 +-
 src/master/master.cpp                 |  2 +-
 src/master/validation.cpp             |  1 -
 src/master/validation.hpp             |  1 -
 src/tests/master_validation_tests.cpp | 51 ++++++------------------------
 5 files changed, 12 insertions(+), 45 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/http.cpp
----------------------------------------------------------------------
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 950206b..f3ce1aa 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1204,7 +1204,7 @@ Future<Response> Master::Http::reserve(
   operation.mutable_reserve()->mutable_resources()->CopyFrom(resources);
 
   Option<Error> error = validation::operation::validate(
-      operation.reserve(), None(), principal);
+      operation.reserve(), principal);
 
   if (error.isSome()) {
     return BadRequest("Invalid RESERVE operation: " + error.get().message);

http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 5c81ce3..5522a44 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3353,7 +3353,7 @@ void Master::_accept(
 
         // Make sure this reserve operation is valid.
         Option<Error> error = validation::operation::validate(
-            operation.reserve(), framework->info.role(), principal);
+            operation.reserve(), principal);
 
         if (error.isSome()) {
           drop(framework, operation, error.get().message);

http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index ec26981..820a9fa 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -652,7 +652,6 @@ namespace operation {
 
 Option<Error> validate(
     const Offer::Operation::Reserve& reserve,
-    const Option<string>& role,
     const Option<string>& principal)
 {
   Option<Error> error = resource::validate(reserve.resources());

http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/validation.hpp
----------------------------------------------------------------------
diff --git a/src/master/validation.hpp b/src/master/validation.hpp
index 6ec883e..29dbdf1 100644
--- a/src/master/validation.hpp
+++ b/src/master/validation.hpp
@@ -105,7 +105,6 @@ namespace operation {
 // Validates the RESERVE operation.
 Option<Error> validate(
     const Offer::Operation::Reserve& reserve,
-    const Option<std::string>& role,
     const Option<std::string>& principal);
 
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index 7db7336..c9bc38c 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -197,7 +197,7 @@ TEST_F(ReserveOperationValidationTest, MatchingRole)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_NONE(operation::validate(reserve, "role", "principal"));
+  EXPECT_NONE(operation::validate(reserve, "principal"));
 }
 
 
@@ -213,7 +213,7 @@ TEST_F(ReserveOperationValidationTest, DisallowStarRoleFrameworks)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_SOME(operation::validate(reserve, "*", "principal"));
+  EXPECT_SOME(operation::validate(reserve, "principal"));
 }
 
 
@@ -229,38 +229,7 @@ TEST_F(ReserveOperationValidationTest, DisallowReserveForStarRole)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_SOME(operation::validate(reserve, "role", "principal"));
-}
-
-
-// This test verifies that validation succeeds even if the 'role'
-// specified in the resources of the RESERVE operation does not
-// match the framework's 'role'.
-TEST_F(ReserveOperationValidationTest, NonMatchingRole)
-{
-  {
-    // Non-matching role, "*" reserving for "role".
-    Resource resource = Resources::parse("cpus", "8", "role").get();
-    resource.mutable_reservation()->CopyFrom(
-        createReservationInfo("principal"));
-
-    Offer::Operation::Reserve reserve;
-    reserve.add_resources()->CopyFrom(resource);
-
-    EXPECT_NONE(operation::validate(reserve, "*", "principal"));
-  }
-
-  {
-    // Non-matching role, "role1" reserving for "role2".
-    Resource resource = Resources::parse("cpus", "8", "role2").get();
-    resource.mutable_reservation()->CopyFrom(
-        createReservationInfo("principal"));
-
-    Offer::Operation::Reserve reserve;
-    reserve.add_resources()->CopyFrom(resource);
-
-    EXPECT_NONE(operation::validate(reserve, "role1", "principal"));
-  }
+  EXPECT_SOME(operation::validate(reserve, "principal"));
 }
 
 
@@ -274,7 +243,7 @@ TEST_F(ReserveOperationValidationTest, MatchingPrincipal)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_NONE(operation::validate(reserve, "role", "principal"));
+  EXPECT_NONE(operation::validate(reserve, "principal"));
 }
 
 
@@ -289,7 +258,7 @@ TEST_F(ReserveOperationValidationTest, NonMatchingPrincipal)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_SOME(operation::validate(reserve, "role", "principal1"));
+  EXPECT_SOME(operation::validate(reserve, "principal1"));
 }
 
 
@@ -303,7 +272,7 @@ TEST_F(ReserveOperationValidationTest, FrameworkMissingPrincipal)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_SOME(operation::validate(reserve, "role", None()));
+  EXPECT_SOME(operation::validate(reserve, None()));
 }
 
 
@@ -319,7 +288,7 @@ TEST_F(ReserveOperationValidationTest, ReservationInfoMissingPrincipal)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(resource);
 
-  EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+  EXPECT_SOME(operation::validate(reserve, "principal"));
 }
 
 
@@ -332,7 +301,7 @@ TEST_F(ReserveOperationValidationTest, StaticReservation)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(staticallyReserved);
 
-  EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+  EXPECT_SOME(operation::validate(reserve, "principal"));
 }
 
 
@@ -346,7 +315,7 @@ TEST_F(ReserveOperationValidationTest, NoPersistentVolumes)
   Offer::Operation::Reserve reserve;
   reserve.add_resources()->CopyFrom(reserved);
 
-  EXPECT_NONE(operation::validate(reserve, "role", "principal"));
+  EXPECT_NONE(operation::validate(reserve, "principal"));
 }
 
 
@@ -364,7 +333,7 @@ TEST_F(ReserveOperationValidationTest, PersistentVolumes)
   reserve.add_resources()->CopyFrom(reserved);
   reserve.add_resources()->CopyFrom(volume);
 
-  EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+  EXPECT_SOME(operation::validate(reserve, "principal"));
 }
 
 


[4/6] mesos git commit: Added '/create-volumes' tests with multiple roles.

Posted by ji...@apache.org.
Added '/create-volumes' tests with multiple roles.

Operators may create volumes for multiple roles in the same operation;
this patch adds tests to confirm correct behavior of authorization in
this case. The tests
`ReservationEndpointsTest.GoodReserveACLMultipleRoles` and
`ReservationEndpointsTest.BadReserveACLMultipleRoles` were added.

Review: https://reviews.apache.org/r/43778/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/27ce0529
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/27ce0529
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/27ce0529

Branch: refs/heads/master
Commit: 27ce052995da5b885ba1a811cccb8b256e5da3bd
Parents: 5dc9f56
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:42:48 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:42:48 2016 -0800

----------------------------------------------------------------------
 src/tests/persistent_volume_endpoints_tests.cpp | 138 +++++++++++++++++++
 1 file changed, 138 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/27ce0529/src/tests/persistent_volume_endpoints_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/persistent_volume_endpoints_tests.cpp b/src/tests/persistent_volume_endpoints_tests.cpp
index 270bb1a..08b9102 100644
--- a/src/tests/persistent_volume_endpoints_tests.cpp
+++ b/src/tests/persistent_volume_endpoints_tests.cpp
@@ -831,6 +831,72 @@ TEST_F(PersistentVolumeEndpointsTest, GoodCreateAndDestroyACL)
 }
 
 
+// Tests that correct setup of `CreateVolume` ACLs allows an operator to perform
+// volume creation operations successfully when volumes for multiple roles are
+// included in the request.
+TEST_F(PersistentVolumeEndpointsTest, GoodCreateACLMultipleRoles)
+{
+  const string AUTHORIZED_ROLE_1 = "potato_head";
+  const string AUTHORIZED_ROLE_2 = "gumby";
+
+  TestAllocator<> allocator;
+  ACLs acls;
+
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // can create volumes for any role.
+  mesos::ACL::CreateVolume* create = acls.add_create_volumes();
+  create->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
+  create->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
+
+  // Create a master.
+  master::Flags masterFlags = CreateMasterFlags();
+  masterFlags.acls = acls;
+
+  EXPECT_CALL(allocator, initialize(_, _, _, _));
+
+  Try<PID<Master>> master = StartMaster(&allocator, masterFlags);
+  ASSERT_SOME(master);
+
+  // Create a slave. Disk resources are statically reserved to allow the
+  // creation of a persistent volume.
+  slave::Flags slaveFlags = CreateSlaveFlags();
+  slaveFlags.resources =
+    "cpus:1;mem:512;disk(" + AUTHORIZED_ROLE_1 +"):1024;disk(" +
+    AUTHORIZED_ROLE_2 + "):1024";
+
+  Future<SlaveID> slaveId;
+  EXPECT_CALL(allocator, addSlave(_, _, _, _, _))
+    .WillOnce(DoAll(InvokeAddSlave(&allocator), FutureArg<0>(&slaveId)));
+
+  Try<PID<Slave>> slave = StartSlave(slaveFlags);
+  ASSERT_SOME(slave);
+
+  Resources volume1 = createPersistentVolume(
+      Megabytes(64),
+      AUTHORIZED_ROLE_1,
+      "id1",
+      "path1");
+
+  Resources volume2 = createPersistentVolume(
+      Megabytes(64),
+      AUTHORIZED_ROLE_2,
+      "id2",
+      "path2");
+
+  Resources volumesMultipleRoles = volume1 + volume2;
+
+  Future<Response> response = process::http::post(
+      master.get(),
+      "create-volumes",
+      createBasicAuthHeaders(DEFAULT_CREDENTIAL),
+      createRequestBody(slaveId.get(), "volumes", volumesMultipleRoles));
+
+  AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
+
+  Shutdown();
+}
+
+
 // This tests that an ACL prohibiting the creation of a persistent volume by a
 // principal will lead to a properly failed request.
 TEST_F(PersistentVolumeEndpointsTest, BadCreateAndDestroyACL)
@@ -949,6 +1015,78 @@ TEST_F(PersistentVolumeEndpointsTest, BadCreateAndDestroyACL)
 }
 
 
+// Tests that a request to create volumes will fail if volumes for multiple
+// roles are included in the request and the operator is not authorized to
+// create volumes for one of them.
+TEST_F(PersistentVolumeEndpointsTest, BadCreateACLMultipleRoles)
+{
+  const string AUTHORIZED_ROLE = "potato_head";
+  const string UNAUTHORIZED_ROLE = "gumby";
+
+  TestAllocator<> allocator;
+  ACLs acls;
+
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // can create volumes for `AUTHORIZED_ROLE`.
+  mesos::ACL::CreateVolume* create1 = acls.add_create_volumes();
+  create1->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
+  create1->mutable_roles()->add_values(AUTHORIZED_ROLE);
+
+  // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
+  // cannot create volumes for any other role.
+  mesos::ACL::CreateVolume* create2 = acls.add_create_volumes();
+  create2->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
+  create2->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
+
+  // Create a master.
+  master::Flags masterFlags = CreateMasterFlags();
+  masterFlags.acls = acls;
+
+  EXPECT_CALL(allocator, initialize(_, _, _, _));
+
+  Try<PID<Master>> master = StartMaster(&allocator, masterFlags);
+  ASSERT_SOME(master);
+
+  // Create a slave. Disk resources are statically reserved to allow the
+  // creation of a persistent volume.
+  slave::Flags slaveFlags = CreateSlaveFlags();
+  slaveFlags.resources =
+    "cpus:1;mem:512;disk(" + AUTHORIZED_ROLE +"):1024;disk(" +
+    UNAUTHORIZED_ROLE + "):1024";
+
+  Future<SlaveID> slaveId;
+  EXPECT_CALL(allocator, addSlave(_, _, _, _, _))
+    .WillOnce(DoAll(InvokeAddSlave(&allocator), FutureArg<0>(&slaveId)));
+
+  Try<PID<Slave>> slave = StartSlave(slaveFlags);
+  ASSERT_SOME(slave);
+
+  Resources volume1 = createPersistentVolume(
+      Megabytes(64),
+      AUTHORIZED_ROLE,
+      "id1",
+      "path1");
+
+  Resources volume2 = createPersistentVolume(
+      Megabytes(64),
+      UNAUTHORIZED_ROLE,
+      "id2",
+      "path2");
+
+  Resources volumesMultipleRoles = volume1 + volume2;
+
+  Future<Response> response = process::http::post(
+      master.get(),
+      "create-volumes",
+      createBasicAuthHeaders(DEFAULT_CREDENTIAL),
+      createRequestBody(slaveId.get(), "volumes", volumesMultipleRoles));
+
+  AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response);
+
+  Shutdown();
+}
+
+
 // This tests that a request containing a credential which is not listed in the
 // master for authentication will not succeed, even if a good authorization ACL
 // is provided.


[3/6] mesos git commit: Changed object of `CreateVolume` ACL to `roles`.

Posted by ji...@apache.org.
Changed object of `CreateVolume` ACL to `roles`.

This solves a problem in which any principal could create volumes for
any role using the '/create-volumes' operator endpoint.

Review: https://reviews.apache.org/r/43782/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/5dc9f563
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/5dc9f563
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/5dc9f563

Branch: refs/heads/master
Commit: 5dc9f563818c72b35571a9a0a7d62a577769d772
Parents: 082ab0e
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:42:39 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:42:39 2016 -0800

----------------------------------------------------------------------
 include/mesos/authorizer/authorizer.proto       |  8 +--
 src/authorizer/local/authorizer.cpp             |  4 +-
 src/master/master.cpp                           | 13 +++-
 src/tests/authorization_tests.cpp               | 63 +++++++++++++-------
 src/tests/persistent_volume_endpoints_tests.cpp | 18 +++---
 src/tests/persistent_volume_tests.cpp           | 20 +++----
 6 files changed, 74 insertions(+), 52 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/5dc9f563/include/mesos/authorizer/authorizer.proto
----------------------------------------------------------------------
diff --git a/include/mesos/authorizer/authorizer.proto b/include/mesos/authorizer/authorizer.proto
index c9fc237..84d2cb3 100644
--- a/include/mesos/authorizer/authorizer.proto
+++ b/include/mesos/authorizer/authorizer.proto
@@ -98,15 +98,13 @@ message ACL {
     required Entity reserver_principals = 2;
   }
 
-  // Specifies what types of volumes, if any, a principal can create.
+  // Specifies which roles a principal can create volumes for.
   message CreateVolume {
     // Subjects: Framework principal or Operator username.
     required Entity principals = 1;
 
-    // Objects: The principal(s) can create volumes of these types.
-    // NOTE: Currently, the only valid values in the default authorizer
-    // implementation are ANY and NONE.
-    required Entity volume_types = 2;
+    // Objects: The principal(s) can create volumes for these roles.
+    required Entity roles = 2;
   }
 
   // Specifies which principals can destroy volumes

http://git-wip-us.apache.org/repos/asf/mesos/blob/5dc9f563/src/authorizer/local/authorizer.cpp
----------------------------------------------------------------------
diff --git a/src/authorizer/local/authorizer.cpp b/src/authorizer/local/authorizer.cpp
index ebb0ec8..a1486bd 100644
--- a/src/authorizer/local/authorizer.cpp
+++ b/src/authorizer/local/authorizer.cpp
@@ -138,10 +138,10 @@ public:
     foreach (const ACL::CreateVolume& acl, acls.create_volumes()) {
       // ACL matches if both subjects and objects match.
       if (matches(request.principals(), acl.principals()) &&
-          matches(request.volume_types(), acl.volume_types())) {
+          matches(request.roles(), acl.roles())) {
         // ACL is allowed if both subjects and objects are allowed.
         return allows(request.principals(), acl.principals()) &&
-               allows(request.volume_types(), acl.volume_types());
+               allows(request.roles(), acl.roles());
       }
     }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/5dc9f563/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 5522a44..1e64133 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -2926,9 +2926,16 @@ Future<bool> Master::authorizeCreateVolume(
     request.mutable_principals()->set_type(ACL::Entity::ANY);
   }
 
-  // TODO(greggomann): Determine what `volume_types` we may want to
-  // allow/prevent creation of. Currently, we simply use ANY.
-  request.mutable_volume_types()->set_type(ACL::Entity::ANY);
+  // The operation will be authorized if the principal is allowed to create
+  // volumes for all roles included in `create.volumes`.
+  // Add an element to `request.roles` for each unique role in the volumes.
+  hashset<string> roles;
+  foreach (const Resource& volume, create.volumes()) {
+    if (!roles.contains(volume.role())) {
+      request.mutable_roles()->add_values(volume.role());
+      roles.insert(volume.role());
+    }
+  }
 
   LOG(INFO)
     << "Authorizing principal '"

http://git-wip-us.apache.org/repos/asf/mesos/blob/5dc9f563/src/tests/authorization_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/authorization_tests.cpp b/src/tests/authorization_tests.cpp
index 27061f1..2b22970 100644
--- a/src/tests/authorization_tests.cpp
+++ b/src/tests/authorization_tests.cpp
@@ -551,27 +551,30 @@ TYPED_TEST(AuthorizationTest, Unreserve)
 }
 
 
-// This tests the authorization of ACLs used for the creation
-// of persistent volumes.
+// Tests the authorization of ACLs used for the creation of persistent volumes.
 TYPED_TEST(AuthorizationTest, CreateVolume)
 {
   ACLs acls;
 
-  // "foo" and "bar" principals can create any volumes.
+  // Principal "foo" can create volumes for any role.
   mesos::ACL::CreateVolume* acl1 = acls.add_create_volumes();
   acl1->mutable_principals()->add_values("foo");
-  acl1->mutable_principals()->add_values("bar");
-  acl1->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  acl1->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
-  // "baz" principal cannot create volumes.
+  // Principal "bar" can only create volumes for the "panda" role.
   mesos::ACL::CreateVolume* acl2 = acls.add_create_volumes();
-  acl2->mutable_principals()->add_values("baz");
-  acl2->mutable_volume_types()->set_type(mesos::ACL::Entity::NONE);
+  acl2->mutable_principals()->add_values("bar");
+  acl2->mutable_roles()->add_values("panda");
 
-  // No other principals can create volumes.
+  // Principal "baz" cannot create volumes.
   mesos::ACL::CreateVolume* acl3 = acls.add_create_volumes();
-  acl3->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
-  acl3->mutable_volume_types()->set_type(mesos::ACL::Entity::NONE);
+  acl3->mutable_principals()->add_values("baz");
+  acl3->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
+
+  // No other principals can create volumes.
+  mesos::ACL::CreateVolume* acl4 = acls.add_create_volumes();
+  acl4->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
+  acl4->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
 
   // Create an Authorizer with the ACLs.
   Try<Authorizer*> create = TypeParam::create();
@@ -581,26 +584,40 @@ TYPED_TEST(AuthorizationTest, CreateVolume)
   Try<Nothing> initialized = authorizer.get()->initialize(acls);
   ASSERT_SOME(initialized);
 
-  // Principals "foo" and "bar" can create volumes, so this request will pass.
+  // Principal "foo" can create volumes for any role, so this request will pass.
   mesos::ACL::CreateVolume request1;
   request1.mutable_principals()->add_values("foo");
-  request1.mutable_principals()->add_values("bar");
-  request1.mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  request1.mutable_roles()->add_values("awesome_role");
   AWAIT_EXPECT_TRUE(authorizer.get()->authorize(request1));
 
-  // Principal "baz" cannot create volumes, so this request will fail.
+  // Principal "bar" can create volumes for the "panda" role,
+  // so this request will pass.
   mesos::ACL::CreateVolume request2;
-  request2.mutable_principals()->add_values("baz");
-  request2.mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
-  AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request2));
+  request2.mutable_principals()->add_values("bar");
+  request2.mutable_roles()->add_values("panda");
+  AWAIT_EXPECT_TRUE(authorizer.get()->authorize(request2));
 
-  // Principal "zelda" is not mentioned in the ACLs of the Authorizer, so it
-  // will be caught by the final ACL, which provides a default case that denies
-  // access for all other principals. This case will fail.
+  // Principal "bar" cannot create volumes for the "giraffe" role,
+  // so this request will fail.
   mesos::ACL::CreateVolume request3;
-  request3.mutable_principals()->add_values("zelda");
-  request3.mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  request3.mutable_principals()->add_values("bar");
+  request3.mutable_roles()->add_values("giraffe");
   AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request3));
+
+  // Principal "baz" cannot create volumes for any role,
+  // so this request will fail.
+  mesos::ACL::CreateVolume request4;
+  request4.mutable_principals()->add_values("baz");
+  request4.mutable_roles()->add_values("panda");
+  AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request4));
+
+  // Principal "zelda" is not mentioned in the ACLs of the Authorizer, so it
+  // will be caught by the final ACL, which provides a default case that denies
+  // access for all other principals. This request will fail.
+  mesos::ACL::CreateVolume request5;
+  request5.mutable_principals()->add_values("zelda");
+  request5.mutable_roles()->add_values("panda");
+  AWAIT_EXPECT_FALSE(authorizer.get()->authorize(request5));
 }
 
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/5dc9f563/src/tests/persistent_volume_endpoints_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/persistent_volume_endpoints_tests.cpp b/src/tests/persistent_volume_endpoints_tests.cpp
index 6069ca1..270bb1a 100644
--- a/src/tests/persistent_volume_endpoints_tests.cpp
+++ b/src/tests/persistent_volume_endpoints_tests.cpp
@@ -720,10 +720,10 @@ TEST_F(PersistentVolumeEndpointsTest, GoodCreateAndDestroyACL)
   ACLs acls;
 
   // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
-  // can create ANY volumes.
+  // can create volumes for any role.
   mesos::ACL::CreateVolume* create = acls.add_create_volumes();
   create->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  create->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  create->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
   // can destroy volumes that it created.
@@ -846,13 +846,13 @@ TEST_F(PersistentVolumeEndpointsTest, BadCreateAndDestroyACL)
   mesos::ACL::CreateVolume* cannotCreate = acls.add_create_volumes();
   cannotCreate->mutable_principals()->add_values(
       DEFAULT_CREDENTIAL.principal());
-  cannotCreate->mutable_volume_types()->set_type(mesos::ACL::Entity::NONE);
+  cannotCreate->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
 
   // This ACL asserts that the principal of `DEFAULT_CREDENTIAL_2`
-  // can create persistent volumes.
+  // can create persistent volumes for any role.
   mesos::ACL::CreateVolume* canCreate = acls.add_create_volumes();
   canCreate->mutable_principals()->add_values(DEFAULT_CREDENTIAL_2.principal());
-  canCreate->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  canCreate->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
   // cannot destroy persistent volumes.
@@ -967,10 +967,10 @@ TEST_F(PersistentVolumeEndpointsTest, GoodCreateAndDestroyACLBadCredential)
   ACLs acls;
 
   // This ACL asserts that the principal of `failedCredential`
-  // can create persistent volumes.
+  // can create persistent volumes for any role.
   mesos::ACL::CreateVolume* failedCreate = acls.add_create_volumes();
   failedCreate->mutable_principals()->add_values(failedCredential.principal());
-  failedCreate->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  failedCreate->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL asserts that the principal of `failedCredential`
   // can destroy persistent volumes.
@@ -980,10 +980,10 @@ TEST_F(PersistentVolumeEndpointsTest, GoodCreateAndDestroyACLBadCredential)
       mesos::ACL::Entity::ANY);
 
   // This ACL asserts that the principal of `DEFAULT_CREDENTIAL`
-  // can create persistent volumes.
+  // can create persistent volumes for any role.
   mesos::ACL::CreateVolume* canCreate = acls.add_create_volumes();
   canCreate->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  canCreate->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  canCreate->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // Create a master.
   master::Flags masterFlags = CreateMasterFlags();

http://git-wip-us.apache.org/repos/asf/mesos/blob/5dc9f563/src/tests/persistent_volume_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/persistent_volume_tests.cpp b/src/tests/persistent_volume_tests.cpp
index e169e1b..bf19c81 100644
--- a/src/tests/persistent_volume_tests.cpp
+++ b/src/tests/persistent_volume_tests.cpp
@@ -829,10 +829,10 @@ TEST_P(PersistentVolumeTest, GoodACLCreateThenDestroy)
   ACLs acls;
 
   // This ACL declares that the principal of `DEFAULT_CREDENTIAL`
-  // can create any persistent volumes.
+  // can create persistent volumes for any role.
   mesos::ACL::CreateVolume* create = acls.add_create_volumes();
   create->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal());
-  create->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  create->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL declares that the principal of `DEFAULT_CREDENTIAL`
   // can destroy its own persistent volumes.
@@ -973,10 +973,10 @@ TEST_P(PersistentVolumeTest, GoodACLNoPrincipal)
   ACLs acls;
 
   // This ACL declares that any principal (and also frameworks without a
-  // principal) can create persistent volumes.
+  // principal) can create persistent volumes for any role.
   mesos::ACL::CreateVolume* create = acls.add_create_volumes();
   create->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
-  create->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  create->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL declares that any principal (and also frameworks without a
   // principal) can destroy persistent volumes.
@@ -1121,16 +1121,16 @@ TEST_P(PersistentVolumeTest, BadACLNoPrincipal)
   ACLs acls;
 
   // This ACL declares that the principal of `DEFAULT_FRAMEWORK_INFO`
-  // can create persistent volumes.
+  // can create persistent volumes for any role.
   mesos::ACL::CreateVolume* create1 = acls.add_create_volumes();
   create1->mutable_principals()->add_values(DEFAULT_FRAMEWORK_INFO.principal());
-  create1->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  create1->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL declares that any other principals
   // cannot create persistent volumes.
   mesos::ACL::CreateVolume* create2 = acls.add_create_volumes();
   create2->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
-  create2->mutable_volume_types()->set_type(mesos::ACL::Entity::NONE);
+  create2->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
 
   // We use this filter so that resources will not
   // be filtered for 5 seconds (the default).
@@ -1321,16 +1321,16 @@ TEST_P(PersistentVolumeTest, BadACLDropCreateAndDestroy)
   ACLs acls;
 
   // This ACL declares that the principal 'creator-principal'
-  // can create persistent volumes.
+  // can create persistent volumes for any role.
   mesos::ACL::CreateVolume* create1 = acls.add_create_volumes();
   create1->mutable_principals()->add_values("creator-principal");
-  create1->mutable_volume_types()->set_type(mesos::ACL::Entity::ANY);
+  create1->mutable_roles()->set_type(mesos::ACL::Entity::ANY);
 
   // This ACL declares that all other principals
   // cannot create any persistent volumes.
   mesos::ACL::CreateVolume* create = acls.add_create_volumes();
   create->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
-  create->mutable_volume_types()->set_type(mesos::ACL::Entity::NONE);
+  create->mutable_roles()->set_type(mesos::ACL::Entity::NONE);
 
   // We use the filter explicitly here so that the resources will not
   // be filtered for 5 seconds (the default).