> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote: > > src/master/master.cpp, line 3348 > > <https://reviews.apache.org/r/40255/diff/3/?file=1156233#file1156233line3348> > > > > You are following the pattern here, but are we sure that the framework > > has the principal? I also do not see any tests with frameworks without > > principals (nor in "reservation_tests.cpp"). It looks like an unsuccessful > > authorization for a framework without a principal can kill the master. > > Greg Mann wrote: > I added tests without a principal, but this code shouldn't lead to a > crash of the master. `principal` is an optional field in `FrameworkInfo`, > which means that if it isn't supplied, it will be initialized with the > default value: an empty string. So if the framework has no principal, this > will result in the logging output: "Authorization of principal '' to create > persistent volumes failed", which seems OK to me. I'm going to drop this for > now, but feel free to re-open if I'm missing something or if you disagree for > another reason.
You are right, my bad. > On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote: > > src/tests/persistent_volume_tests.cpp, lines 717-719 > > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line717> > > > > Could you please add a test with a framework without a principal? > > > > In the same vein, do you think it makes sense to create a ticket for > > the same case for dynamic reservatons (even though we require the principal > > for now)? > > Greg Mann wrote: > Excellent idea, I've added two tests to this patch for cases with no > principal and created a ticket for RESERVE/UNRESERVE: > https://issues.apache.org/jira/browse/MESOS-4195 Thanks! I've noticed we usually do not test cases like "authn is off, authz is on, framework has a principal", "authn is off, authz if off, framework has no principal", though, I would say, are real-world scenarios (for test clusters only I hope : ) ). > On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote: > > src/tests/persistent_volume_tests.cpp, line 790 > > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line790> > > > > Do you think it makes sense to extract "role1" into a constant? > > Greg Mann wrote: > Since this role name appears throughout the file, I think I'd rather > follow these up with a patch that makes this change for all of the persistent > volume tests. What do you think? That's fine. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40255/#review110658 ----------------------------------------------------------- On Dec. 18, 2015, 9:37 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40255/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2015, 9:37 a.m.) > > > Review request for mesos, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-3065 > https://issues.apache.org/jira/browse/MESOS-3065 > > > Repository: mesos > > > Description > ------- > > Added framework authorization for persistent volumes. > > > Diffs > ----- > > src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5 > src/tests/persistent_volume_tests.cpp > 01b3c13751a5558d5f06edb8f650c8644dc54486 > > Diff: https://reviews.apache.org/r/40255/diff/ > > > Testing > ------- > > This is the fifth in a chain of 7 patches. New tests were added to > `persistent_volume_tests.cpp` in order to test a framework attempting both > successful and failed authorizations for `CREATE` and `DESTROY` offer > operations. `make check` was used to test after all patches were applied. > > > Thanks, > > Greg Mann > >
