----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61171/#review181829 -----------------------------------------------------------
src/common/http.hpp Lines 274-276 (patched) <https://reviews.apache.org/r/61171/#comment257623> Could you add a comment above this declaration, explaining what resource format this function accepts? src/common/http.cpp Lines 984-1009 (patched) <https://reviews.apache.org/r/61171/#comment257622> I left another comment regarding the resource format we use for authorization. If we switch to authorize using only the canonical post-reservation-refinement resource format, then I think it should be sufficient to authorize just the roles in `Resource.reservations`. Perhaps we should also add a CHECK to ensure that `!resource.has_role()` and `!resource.has_reservation()`? (Question: are all resources in the canonical format, including the `SlaveInfo` in `recovered` agents?) src/common/http.cpp Lines 988-990 (patched) <https://reviews.apache.org/r/61171/#comment257668> As written, when authorizing resources in the ENDPOINT format, this condition will incorrectly call into the acceptor when `resource.role() == "*"`; this actually represents an _unreserved_ resource, so we should not call into the acceptor. If we switch to authorizing only the canonical resource format, this code could be removed. src/common/protobuf_utils.cpp Lines 900-904 (original), 910-915 (patched) <https://reviews.apache.org/r/61171/#comment257593> We discussed this in chat, but leaving a comment here for posterity. Shouldn't we be authorizing using the internal, canonical resource representation, rather than converting the resource format first? src/master/http.cpp Lines 2525-2527 (patched) <https://reviews.apache.org/r/61171/#comment257615> Indented too far. src/tests/api_tests.cpp Lines 1638 (patched) <https://reviews.apache.org/r/61171/#comment257659> s/tests/test/ src/tests/api_tests.cpp Lines 1719-1737 (patched) <https://reviews.apache.org/r/61171/#comment257660> While this code ensures that all reservations specify the correct role, it doesn't assert that the expected reservations do appear. src/tests/api_tests.cpp Lines 1722 (patched) <https://reviews.apache.org/r/61171/#comment257664> Note that this should fail for unreserved resources, since in that case `role == "*"` in the ENDPOINT resource format. src/tests/api_tests.cpp Lines 1725-1727 (patched) <https://reviews.apache.org/r/61171/#comment257666> Do we expect any AllocationInfo in this response? src/tests/api_tests.cpp Lines 1771 (patched) <https://reviews.apache.org/r/61171/#comment257662> Since the default value of `role` is `*`, this should be: ``` if (resource.has_role()) { EXPECT_EQ(resource.role(), "*"); } ``` src/tests/api_tests.cpp Lines 1773-1775 (patched) <https://reviews.apache.org/r/61171/#comment257667> Do we expect any AllocationInfo in this response? src/tests/api_tests.cpp Lines 1781-1784 (patched) <https://reviews.apache.org/r/61171/#comment257663> Since the `ReservationInfo`s in the `reservations` field should _always_ have their `role` field set, you can probably just do: ``` EXPECT_EQ(resource.reservations_size(), 0); ``` - Greg Mann On July 31, 2017, 1:50 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61171/ > ----------------------------------------------------------- > > (Updated July 31, 2017, 1:50 p.m.) > > > Review request for mesos, Adam B, Greg Mann, Quinn Leng, and Till Toenshoff. > > > Bugs: MESOS-7416 > https://issues.apache.org/jira/browse/MESOS-7416 > > > Repository: mesos > > > Description > ------- > > Enables filtering of the results of calls to the 'GET_AGENTS' v1 > API. It filters the contents of different resources entries based > on the 'VIEW_ROLE' permissions of the principal doing the request > based on resource roles, allocation roles and reservations. > > > Diffs > ----- > > src/common/http.hpp ba8dda18a02f51d1a28e719f06ee4b51573dfbec > src/common/http.cpp dfd5f335e8a3745d047d4f9f5e8c821b2c22da5a > src/common/protobuf_utils.hpp 80d2edd452f3ffa38c40f9a21f8489799065c401 > src/common/protobuf_utils.cpp 49d3a229925f4aa107e3e5f762936c16318aeadb > src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 > src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 > src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b > > > Diff: https://reviews.apache.org/r/61171/diff/4/ > > > Testing > ------- > > ```shell > make check > ``` > > Manual test: > > ```shell > mkdir -p /tmp/mesos/master > mkdir -p /tmp/mesos/agent > > # Create credentials > cat <<EOF > /tmp/mesos/credentials.txt > hal-9000 dave > glados potato > skynet connor > EOF > > # Create ACLs > cat <<EOF > /tmp/mesos/acls.json > { > "permissive": true, > "view_roles" : [ > { > "principals" : { "type" : "ANY" }, > "roles" : { "values" : ["*"] } > }, > { > "principals" : { "values" : ["hal-9000"] }, > "roles" : { "values" : ["space-odyssey"] } > }, > { > "principals" : { "values" : ["hal-9000"] }, > "roles" : { "type" : "NONE" } > }, > { > "principals" : { "values" : ["glados"] }, > "roles" : { "values" : ["portal"] } > }, > { > "principals" : { "values" : ["glados"] }, > "roles" : { "type" : "NONE" } > }, > { > "principals" : { "values" : ["skynet"] }, > "roles" : { "values" : ["terminator"] } > }, > { > "principals" : { "values" : ["skynet"] }, > "roles" : { "type" : "NONE" } > } > ] > } > EOF > > # Launch Master with some predefined roles. > ./bin/mesos-master.sh \ > --work_dir=/tmp/mesos/master \ > --log_dir=/tmp/mesos/master/log \ > --authenticate_http \ > --credentials=/tmp/mesos/credentials.txt \ > --authenticate_http_frameworks \ > --http_framework_authenticators=basic \ > --http_authenticators=basic \ > --authenticate_http_readonly \ > --acls=/tmp/mesos/acls.json \ > --roles="space-odyssey,portal,terminator" & > > # Launch Agent with static reservations for all roles. > sudo ./bin/mesos-agent.sh \ > --master=127.0.0.1:5050 \ > --work_dir=/tmp/mesos/agent \ > --authenticate_http_readwrite \ > --http_authenticators=basic \ > --http_credentials=/tmp/mesos/credentials.txt \ > --acls=/tmp/mesos/acls.json \ > > --resources='cpus(space-odyssey):2;cpus(portal):2;cpus(*):4;mem(space-odyssey):250;mem(portal):250;mem(*):10360;ports(space-odyssey):[31000-32000];ports(portal):[32001-33000];ports(*):[33001-35000];disk(space-odyssey):250;disk(portal):250;disk(*):1000' > & > > # Launch test framework. > ./src/mesos-execute \ > --master=127.0.0.1:5050 \ > --command='while true; do echo "Hello World"; sleep 5; done;' \ > --resources="cpus:1;mem:128;disk:32;ports:[31002-31003]" \ > --role=space-odyssey \ > --name=hello-discovery \ > --principal=hal-9000 \ > --secret=dave & > > # Create a dynamic reservation. > cat > /tmp/resources.json <<EOM > [ > { > "name": "cpus", > "type": "SCALAR", > "scalar": { "value": 2 }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > }, > { > "name": "mem", > "type": "SCALAR", > "scalar": { "value": 250 }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > }, > { > "name": "disk", > "type": "SCALAR", > "scalar": { "value": 250 }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > }, > { > "name": "ports", > "type": "RANGES", > "ranges": { > "range": [ > { > "begin": 33001, > "end": 34000 > } > ] > }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > } > ] > EOM > > http \ > -a skynet:connor \ > -f POST \ > 127.0.0.1:5050/master/reserve \ > slaveId=${SLAVE_ID} \ > resources=@/tmp/resources.json > > > # Create some quota. > cat > /tmp/quota.json <<EOM > { > "role": "portal", > "guarantee": [ > { > "name": "cpus", > "type": "SCALAR", > "scalar": { "value": 2 } > }, > { > "name": "mem", > "type": "SCALAR", > "scalar": { "value": 250 } > }, > { > "name": "disk", > "type": "SCALAR", > "scalar": { "value": 250 } > } > ] > } > EOM > > http \ > -a glados:potato \ > POST \ > 127.0.0.1:5050/master/quota \ > @/tmp/quota.json > > > # Query the master with all users and check > # that only the information of his role is > # available. > http -a glados:potato -v -j -f POST 127.0.0.1:5050/api/v1 type=GET_AGENTS > > http -a skynet:connor -v -j -f POST 127.0.0.1:5050/api/v1 type=GET_AGENTS > > http -a hal-9000:dave -v -j -f POST 127.0.0.1:5050/api/v1 type=GET_AGENTS > ``` > > > Thanks, > > Alexander Rojas > >
