----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49082/#review139234 -----------------------------------------------------------
Looks pretty good to me. Just some questions about the lambda parameters, and some nits on other copied code. You can decide if you want to change the same things in the original master state/test code in this patch, another patch, or not at all. src/slave/http.cpp (line 127) <https://reviews.apache.org/r/49082/#comment204377> Can we get a struct-level comment here like you have for FrameworkWriter? src/slave/http.cpp (line 150) <https://reviews.apache.org/r/49082/#comment204375> Is it safe to pass `this` through here? Could you instead just pass immutable copies of `executor_->launchedTasks` and `framework_->info` and `taskApprover_`? src/slave/http.cpp (line 160) <https://reviews.apache.org/r/49082/#comment204376> Ditto on `this` here and elsewhere src/slave/http.cpp (line 859) <https://reviews.apache.org/r/49082/#comment204379> How many things from the current context do we really need to pull into this lambda? Is it safe to pull them all in? src/slave/http.cpp (line 862) <https://reviews.apache.org/r/49082/#comment204380> `this` here refers to the slave? I guess we're deferred back onto the slave, so accessing its variables is safe? (And these should all be read-only operations) src/slave/http.cpp (lines 917 - 930) <https://reviews.apache.org/r/49082/#comment204381> I wouldn't have expected this block to be indented in so far. Imagine you had managed to fit `writer->field("frameworks", [...](JSON::ArrayWriter* writer) {` all on one line: then `foreachvalue (...` would only be indented in 2 spaces from `writer->field(...` src/slave/http.cpp (lines 938 - 954) <https://reviews.apache.org/r/49082/#comment204382> Ditto on the indent here src/tests/mesos.hpp (line 216) <https://reviews.apache.org/r/49082/#comment204383> Remove the extra blank line src/tests/mesos.hpp (lines 217 - 218) <https://reviews.apache.org/r/49082/#comment204384> We usually put the constructor with additional parameters after the constructor with fewer parameters, so this should go last, after `detector authorizer, and flags` src/tests/slave_authorization_tests.cpp (lines 174 - 175) <https://reviews.apache.org/r/49082/#comment204387> I don't think these are really duplicated so many times that they need to be brought up to the top as constants here. "state" is rather obvious, and shorter than its variable name replacement, and it's only used twice. "user" is really only needed once, and its two uses are within a few lines of each other, so there's no need to pull the variable all the way up here. src/tests/slave_authorization_tests.cpp (lines 236 - 241) <https://reviews.apache.org/r/49082/#comment204385> You're not really "starting the framework" with user bar, you're just registering the framework and setting the default user that its tasks/executors will run as, if there is no task/executor-level override. As such, specifying "user=bar" in the ExecutorInfo here is unnecessary, since the framework default is already "bar". (s/framwork/framework/) src/tests/slave_authorization_tests.cpp (line 250) <https://reviews.apache.org/r/49082/#comment204386> s/this->// src/tests/slave_authorization_tests.cpp (line 315) <https://reviews.apache.org/r/49082/#comment204388> This seems redundant with the `EXPECT_EQ(1u, frameworks.values.size());` check below. Remove this one. src/tests/slave_authorization_tests.cpp (lines 321 - 323) <https://reviews.apache.org/r/49082/#comment204389> This check is also redundant, and should be done after you assert that it `is<JSON::Array>` anyway - Adam B On June 23, 2016, 3:01 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49082/ > ----------------------------------------------------------- > > (Updated June 23, 2016, 3:01 a.m.) > > > Review request for mesos, Adam B, Joerg Schad, and Till Toenshoff. > > > Repository: mesos > > > Description > ------- > > While information about frameworks, executors and tasks were well > protected in the master, this information was not protected in the > agents, which enabled unauthorized users to verify the data by getting > the agent `/state` endpoint. This was particularly pressing since the > Mesos UI would work as a proxy for agents endpoints so even being > behind a firewall would not had been enough. > > > Diffs > ----- > > src/common/http.hpp 588a1eb18e2665e71cc4361835d873652c4e100d > src/common/http.cpp 4b35736ef53baeb8eeefe5aadee3964905469e6d > src/master/http.cpp 70f084b84db90fde20c05d2354be190f28e72996 > src/slave/http.cpp 22b5930a7af336a749af4b6b50254f60433ecea5 > src/tests/mesos.hpp b9de9b6dfe0e62df91d5b82006b799345b49fd3c > src/tests/mesos.cpp 15aaaa7979f227083580afc1349373f3e3364c65 > src/tests/slave_authorization_tests.cpp > 98fde4914a5fddda4401d366e75322458bc9104c > > Diff: https://reviews.apache.org/r/49082/diff/ > > > Testing > ------- > > `make check`, some manual testing in the Mesos UI and the following script: > > ```sh > #! /usr/bin/env bash > > rm -rf /tmp/mesos/* > > cat <<EOF > /tmp/credentials.txt > foo bar > baz bar > EOF > > cat <<EOF > /tmp/acls.json > { > "permissive": false, > "access_mesos_logs" : [ > { > "principals" : { "values" : ["foo"] }, > "logs" : { "type" : "ANY" } > } > ], > "access_sandboxes" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "view_tasks" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "view_executors" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "view_frameworks" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "register_frameworks" : [ > { > "principals" : { "values" : ["foo"] }, > "roles" : { "values" : ["test"] } > } > ], > "run_tasks" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "get_endpoints" : [ > { > "principals" : { "values" : ["foo"] }, > "paths" : { "type" : "ANY" } > } > ] > } > EOF > > ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \ > --authenticate_http \ > --log_dir=/tmp/mesos/logs/master \ > --http_credentials=file:///tmp/credentials.txt \ > --acls=file:///tmp/acls.json & > ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \ > --master=127.0.0.1:5050 \ > --authenticate_http \ > --http_credentials=file:///tmp/credentials.txt \ > --acls=file:///tmp/acls.json & > ./src/mesos-execute \ > --command='while true; do echo "Hello world"; sleep 3; done' \ > --role=test \ > --master=127.0.0.1:5050 \ > --name=echoer \ > --principal=foo & > > # This should show complete information about frameworks, > # executors and tasks. > http GET http://127.0.0.1:5051/state -a foo:bar > > # This should show partial elements of the state but none > # related to the running framework. > http GET http://127.0.0.1:5051/state -a baz:bar > > # This should yield a 401 Unauthorized response. > http GET http://127.0.0.1:5051/state -a bar:bar > ``` > > > Thanks, > > Alexander Rojas > >
