You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Quinn Leng <qu...@gmail.com> on 2017/06/15 17:01:33 UTC
Review Request 60107: Add class definition for ObjectFilter.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
Bugs: MESOS-7630
https://issues.apache.org/jira/browse/MESOS-7630
Repository: mesos
Description
-------
Add class definition for ObjectFilter.
Write class definition for ObjectFilter, AuthorizeFilter,
which is the superclass for all Filters related with
authorization.
Add asynchronize construction method for AuthorizeFilter,
since it will incorporate the construction of ObjectApprover,
which is constructed asynchronously
Diffs
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
Diff: https://reviews.apache.org/r/60107/diff/1/
Testing
-------
Passed "make check"
No specific test case related
Thanks,
Quinn Leng
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/#review178678
-----------------------------------------------------------
Patch looks great!
Reviews applied: [60107]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On June 20, 2017, 12:30 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> -----------------------------------------------------------
>
> (Updated June 20, 2017, 12:30 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add class definition for ObjectFilter.
>
>
> Diffs
> -----
>
> src/common/http.hpp 48f7c4a9c35371544c92bb1712e1e354feb20987
> src/common/http.cpp 98899a789610add924a7d2c0af943230cf914d2d
>
>
> Diff: https://reviews.apache.org/r/60107/diff/4/
>
>
> Testing
> -------
>
> Passed "make check"
> No specific test case related
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/#review178668
-----------------------------------------------------------
Also, I don't know if calling it a filter is the right thing to do. A filter should recieve a data set and remove things from it. This abstraction can only verify one element at a time, therefore it is not a filter. Acceptor would be a better name for it.
- Alexander Rojas
On June 20, 2017, 2:30 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> -----------------------------------------------------------
>
> (Updated June 20, 2017, 2:30 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add class definition for ObjectFilter.
>
>
> Diffs
> -----
>
> src/common/http.hpp 48f7c4a9c35371544c92bb1712e1e354feb20987
> src/common/http.cpp 98899a789610add924a7d2c0af943230cf914d2d
>
>
> Diff: https://reviews.apache.org/r/60107/diff/4/
>
>
> Testing
> -------
>
> Passed "make check"
> No specific test case related
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/
-----------------------------------------------------------
(Updated June 20, 2017, 12:30 a.m.)
Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
Bugs: MESOS-7630
https://issues.apache.org/jira/browse/MESOS-7630
Repository: mesos
Description
-------
Add class definition for ObjectFilter.
Diffs (updated)
-----
src/common/http.hpp 48f7c4a9c35371544c92bb1712e1e354feb20987
src/common/http.cpp 98899a789610add924a7d2c0af943230cf914d2d
Diff: https://reviews.apache.org/r/60107/diff/4/
Changes: https://reviews.apache.org/r/60107/diff/3-4/
Testing
-------
Passed "make check"
No specific test case related
Thanks,
Quinn Leng
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/
-----------------------------------------------------------
(Updated June 16, 2017, 12:35 a.m.)
Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
Bugs: MESOS-7630
https://issues.apache.org/jira/browse/MESOS-7630
Repository: mesos
Description
-------
Add class definition for ObjectFilter.
Diffs (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
Diff: https://reviews.apache.org/r/60107/diff/3/
Changes: https://reviews.apache.org/r/60107/diff/2-3/
Testing
-------
Passed "make check"
No specific test case related
Thanks,
Quinn Leng
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/#review178068
-----------------------------------------------------------
src/common/http.hpp
Lines 168-170 (patched)
<https://reviews.apache.org/r/60107/#comment251869>
Maybe something like:
"Used to filter which objects will be returned when building responses for requests to the operator APIs."
src/common/http.hpp
Lines 178 (patched)
<https://reviews.apache.org/r/60107/#comment251870>
End all comments with a period.
src/common/http.hpp
Lines 198 (patched)
<https://reviews.apache.org/r/60107/#comment251867>
Indent two more spaces.
src/common/http.cpp
Lines 1136 (patched)
<https://reviews.apache.org/r/60107/#comment251871>
This will fail if `authorizer.isNone()` is true. Should do:
```
if (authorizer.isNone()) {
return AcceptingFilter();
} else {
// construct/return AuthorizeTaskInfoFilter here...
}
```
This will allow us to factor out all of the checks for `authorizer.isSome()` in the handler code, which is nice.
src/common/http.cpp
Lines 1139-1140 (patched)
<https://reviews.apache.org/r/60107/#comment251866>
Indent two more spaces.
- Greg Mann
On June 15, 2017, 11:57 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> -----------------------------------------------------------
>
> (Updated June 15, 2017, 11:57 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add class definition for ObjectFilter.
>
>
> Diffs
> -----
>
> src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
> src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
>
>
> Diff: https://reviews.apache.org/r/60107/diff/2/
>
>
> Testing
> -------
>
> Passed "make check"
> No specific test case related
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/
-----------------------------------------------------------
(Updated June 15, 2017, 11:57 p.m.)
Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
Bugs: MESOS-7630
https://issues.apache.org/jira/browse/MESOS-7630
Repository: mesos
Description (updated)
-------
Add class definition for ObjectFilter.
Diffs (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
Diff: https://reviews.apache.org/r/60107/diff/2/
Changes: https://reviews.apache.org/r/60107/diff/1-2/
Testing
-------
Passed "make check"
No specific test case related
Thanks,
Quinn Leng
Re: Review Request 60107: Add class definition for ObjectFilter.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/#review178049
-----------------------------------------------------------
src/common/http.hpp
Lines 178-180 (patched)
<https://reviews.apache.org/r/60107/#comment251832>
Maybe something simpler like:
"Parent class for authorization-based filters."
src/common/http.hpp
Lines 184-185 (patched)
<https://reviews.apache.org/r/60107/#comment251833>
Let's make this private or protected.
src/common/http.hpp
Lines 185 (patched)
<https://reviews.apache.org/r/60107/#comment251841>
Indent two more spaces.
src/common/http.hpp
Lines 195-196 (patched)
<https://reviews.apache.org/r/60107/#comment251835>
Let's make this private or protected.
src/common/http.hpp
Lines 196 (patched)
<https://reviews.apache.org/r/60107/#comment251842>
Indent two more spaces.
src/common/http.hpp
Lines 198-200 (patched)
<https://reviews.apache.org/r/60107/#comment251836>
Let's try to move this declaration into the parent class.
src/common/http.cpp
Lines 1133-1134 (patched)
<https://reviews.apache.org/r/60107/#comment251839>
Would prefer:
```
const Option<authorization::Subject> subject =
authorization::createSubject(principal);
```
src/common/http.cpp
Lines 1136-1138 (patched)
<https://reviews.apache.org/r/60107/#comment251840>
Would prefer:
```
return authorizer.get()->getObjectApprover(
subject, authorization::VIEW_TASK)
.then([=](const Owned<ObjectApprover>& approver) {
```
src/common/http.cpp
Lines 1139 (patched)
<https://reviews.apache.org/r/60107/#comment251838>
Actually, we should avoid using `new` here:
```
return AuthorizeTaskInfoFilter(approver);
```
- Greg Mann
On June 15, 2017, 5:01 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> -----------------------------------------------------------
>
> (Updated June 15, 2017, 5:01 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add class definition for ObjectFilter.
>
> Write class definition for ObjectFilter, AuthorizeFilter,
> which is the superclass for all Filters related with
> authorization.
>
> Add asynchronize construction method for AuthorizeFilter,
> since it will incorporate the construction of ObjectApprover,
> which is constructed asynchronously
>
>
> Diffs
> -----
>
> src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
> src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
>
>
> Diff: https://reviews.apache.org/r/60107/diff/1/
>
>
> Testing
> -------
>
> Passed "make check"
> No specific test case related
>
>
> Thanks,
>
> Quinn Leng
>
>