> On 二月 18, 2016, 6:57 a.m., Guangya Liu wrote: > > src/master/validation.cpp, lines 692-711 > > <https://reviews.apache.org/r/43639/diff/1/?file=1253091#file1253091line692> > > > > Can you please add some test cases to cover those scenarios? > > > > 1) authentication is enabled and reservation does not have principal. > > 2) authentication is enabled and reservation pricipal does not same as > > authentication principal. > > 3) authentication is disabled but reservation has principal. > > Guangya Liu wrote: > Seems 2) was covered by `ReservationEndpointsTest, NonMatchingPrincipal`, > but 1) and 3) was not covered for now. > > Greg Mann wrote: > Good call, I added a test case to the `ReservationTest` tests for #3. > > Due to the way authentication works, I'm not sure that test case #1 is > necessary. When authentication is enabled, HTTP endpoints require a principal > and frameworks must supply a principal to register. The best we could do to > test #1 is to have a request with an authenticated principal but no principal > in `ReservationInfo`, which is really just testing that a request with > non-matching principals will fail. The non-matching principal case is covered > by `ReservationEndpointsTest.NonMatchingPrincipal`. What do you think?
fair enough, thanks Greg. - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43639/#review119589 ----------------------------------------------------------- On 二月 23, 2016, 12:16 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43639/ > ----------------------------------------------------------- > > (Updated 二月 23, 2016, 12:16 a.m.) > > > Review request for mesos, Michael Park and Neil Conway. > > > Bugs: MESOS-3940 > https://issues.apache.org/jira/browse/MESOS-3940 > > > Repository: mesos > > > Description > ------- > > Allowed dynamic reservation without a principal. > > The `ReservationInfo.principal` field has been migrated to `optional`, which > means we can now allow dynamic reservation and unreservation without a > principal. This allows the use of the `/reserve` and `/unreserve` HTTP > endpoints when HTTP authentication is disabled. > > Note that we still require that frameworks/operators set the > `ReservationInfo.principal` field to match their own principal, if present. > It may be desirable to remove this requirement; this improvement is tracked > in MESOS-4696. > > > Diffs > ----- > > src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca > src/tests/master_validation_tests.cpp > 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 > src/tests/reservation_endpoints_tests.cpp > afe81b1d38a1b3a82583720f26482ddcde8f5e85 > src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 > > Diff: https://reviews.apache.org/r/43639/diff/ > > > Testing > ------- > > A new test case was added to `ReservationTest.NoAuthentication`. > > `make check` was used to test on OSX, both with and without SSL enabled. > > Also manually reserved/unreserved resources using curl, with a command like > this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d > resources='[ { "name": "cpus", "type": "SCALAR", "scalar": { "value": 3 }, > "role": "ads", "reservation": { } } ]' -X POST > http://127.0.0.1:5050/master/reserve` > > Inspecting `/master/state` before & after these operations confirmed that the > reserve/unreserve operations were successful. > > > Thanks, > > Greg Mann > >
