You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/08/01 05:58:39 UTC
Re: Review Request 61171: Enabled filtering of the 'GET_AGENTS' v1
API call.
-----------------------------------------------------------
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
>
>
Re: Review Request 61171: Enabled filtering of the 'GET_AGENTS' v1
API call.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Aug. 1, 2017, 7:58 a.m., Greg Mann wrote:
> > src/common/http.cpp
> > Lines 984-1009 (patched)
> > <https://reviews.apache.org/r/61171/diff/4/?file=1785908#file1785908line984>
> >
> > 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?)
I verify this after running all the different tests, the canonical form may include the role `*` in `Resource.role` in the recovered agents. Other than that, I added the required check.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61171/#review181829
-----------------------------------------------------------
On July 31, 2017, 3: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, 3: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
>
>