Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-18 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Dec. 18, 2015, 10:10 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Rebased.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838 
  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-18 Thread Till Toenshoff 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review87
---

Ship it!


Ship It!

- Till Toenshoff


On Dec. 18, 2015, 11:43 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 18, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Bernd 
> Mathiske, Joris Van Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-17 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Dec. 17, 2015, 10:26 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Addressed issues.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-17 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review110930
---

Ship it!



src/master/quota_handler.cpp (line 348)


Shouldn't this comment be above line 327?


- Joerg Schad


On Dec. 17, 2015, 9:26 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 17, 2015, 9:26 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-17 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Dec. 17, 2015, 12:16 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Addressed issues.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-15 Thread Greg Mann 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review110525
---

Ship it!



src/master/quota_handler.cpp (lines 330 - 333)


Do you think `principal = credential.isSome() ? 
credential.get().principal() : None();` would be more concise here?


- Greg Mann


On Dec. 15, 2015, 11:07 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 15, 2015, 11:07 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-15 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Dec. 15, 2015, 12:07 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Rebased.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-11 Thread Greg Mann 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review110048
---


Hey Jan, would you mind rebasing this chain? I just got a "patch does not 
apply" when applying this third one. I was hoping to compile and exercise the 
tests :-)

Also, I noticed that the tests are associated with a different JIRA and haven't 
been added to the 'mesos' group on here yet... I understand the desire to keep 
things moving, but it would be nice to verify this new authorization 
functionality with passing tests before merging. I can have a look at the tests 
and help review those as well.

- Greg Mann


On Dec. 7, 2015, 2:34 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 7, 2015, 2:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-07 Thread Jan Schlicht 


> On Dec. 2, 2015, 4:47 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 236-240
> > 
> >
> > Can we re-write it using a lambda? This way you do not need to inject 
> > `authorized` into the continuation.
> > 
> > ```
> > authorize(quotaInfo)
> >   .then(defer(master->self(), [=](bool authorized) -> Future {
> > if (!authorized) {
> >   return Unauthorized("Mesos master");
> > }
> > 
> > _set(quotaInfo);
> >   }));
> > ```
> 
> Jan Schlicht wrote:
> Sure, but the rest of the function already has a similar lamdba 
> continuation (i.e. will have after rebasing). This will result in a lambda 
> inside a lambda which is too much nesting IMHO. Therefore I would prefer to 
> keep the continuation.

Oh, sorry, I skipped your example code. Thought that I ought to put everything 
into a lambda and remove the continuation completly. Yes, using the lambda this 
way makes much more sense, will implement it that way.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review108645
---


On Dec. 7, 2015, 2:03 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 7, 2015, 2:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-07 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Dec. 7, 2015, 3:34 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Added lambda to simplify continuation.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-07 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Dec. 7, 2015, 2:03 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Rebased, refactored & issues addressed: Instead of authorizing using 
`QuotaInfo` fields, `principal` and `role` are now used directly.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-07 Thread Jan Schlicht 


> On Dec. 2, 2015, 4:47 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 236-240
> > 
> >
> > Can we re-write it using a lambda? This way you do not need to inject 
> > `authorized` into the continuation.
> > 
> > ```
> > authorize(quotaInfo)
> >   .then(defer(master->self(), [=](bool authorized) -> Future {
> > if (!authorized) {
> >   return Unauthorized("Mesos master");
> > }
> > 
> > _set(quotaInfo);
> >   }));
> > ```

Sure, but the rest of the function already has a similar lamdba continuation 
(i.e. will have after rebasing). This will result in a lambda inside a lambda 
which is too much nesting IMHO. Therefore I would prefer to keep the 
continuation.


> On Dec. 2, 2015, 4:47 p.m., Alexander Rukletsov wrote:
> > include/mesos/quota/quota.proto, lines 43-45
> > 
> >
> > Do you think `principal` should be required? Does it mean quota cannot 
> > be requested without providing `Authorization` header? I think you do so in 
> > `authorize()` below.
> > 
> > I think it's also fine to prohibit setting quota without prinicipal, 
> > but let's be consitent and explicit about it.

Actually, after refactoring `principal` can be removed here. It will be needed 
when the `remove quota` is implemented and will be implemented there.


> On Dec. 2, 2015, 4:47 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 94-96
> > 
> >
> > I think this may be buggy: `principal` is required, however we do not 
> > ensure it is present here, means we may create a protbuf message, that we 
> > cannot deserialize.

The `principal` field will be removed and implemented later for `remove quota`.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review108645
---


On Dec. 7, 2015, 2:03 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 7, 2015, 2:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-07 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review109194
---

Ship it!


Ship It!

- Alexander Rukletsov


On Dec. 7, 2015, 2:34 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 7, 2015, 2:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-12-02 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review108645
---



include/mesos/quota/quota.proto (lines 43 - 45)


Do you think `principal` should be required? Does it mean quota cannot be 
requested without providing `Authorization` header? I think you do so in 
`authorize()` below.

I think it's also fine to prohibit setting quota without prinicipal, but 
let's be consitent and explicit about it.



src/master/master.hpp (lines 899 - 900)


Do you need the whole `QuotaInfo` or optional `principal` and role would 
suffice?



src/master/quota_handler.cpp (line 82)


Do we need `Credential` here or principal would suffice?



src/master/quota_handler.cpp (lines 94 - 96)


I think this may be buggy: `principal` is required, however we do not 
ensure it is present here, means we may create a protbuf message, that we 
cannot deserialize.



src/master/quota_handler.cpp (lines 236 - 240)


Can we re-write it using a lambda? This way you do not need to inject 
`authorized` into the continuation.

```
authorize(quotaInfo)
  .then(defer(master->self(), [=](bool authorized) -> Future {
if (!authorized) {
  return Unauthorized("Mesos master");
}

_set(quotaInfo);
  }));
```



src/master/quota_handler.cpp (lines 333 - 334)


Blank line?



src/master/quota_handler.cpp (lines 338 - 339)


Blank line?



src/master/quota_handler.cpp (lines 339 - 340)


Let's wrap variable and function names in backticks.

Also, how about re-wording it a bit for brevity? Something like 
"`quotaInfo` is already validated, role is quaranteed to be present".


- Alexander Rukletsov


On Nov. 20, 2015, 10:03 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 20, 2015, 10:03 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-20 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Nov. 20, 2015, 10:55 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
Toenshoff.


Changes
---

Authorize using the authentification crendentials. Authorization won't work 
without authentification, but this is in line with the current other 
authorization implementations.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-19 Thread Jan Schlicht 


> On Nov. 16, 2015, 3:37 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 188
> > 
> >
> > Should this be part of the validation below?
> 
> Jan Schlicht wrote:
> I'd rather put it into `createQuotaInfo` as it's not validating but 
> setting properties of `create`.
> 
> Joerg Schad wrote:
> Question was whether we should validate that the principal is set in 
> validation...

Well, the principal is optional and should be set if the operator wants to use 
authorization. In case authorization is active and no principal has been set, 
the authorization should fail. So my opinion is that we don't need to validate 
here as the validation is implicitly done in `authorize`.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review106662
---


On Nov. 19, 2015, 11:55 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 19, 2015, 11:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-19 Thread Joerg Schad 


> On Nov. 16, 2015, 2:37 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 188
> > 
> >
> > Should this be part of the validation below?
> 
> Jan Schlicht wrote:
> I'd rather put it into `createQuotaInfo` as it's not validating but 
> setting properties of `create`.

Question was whether we should validate that the principal is set in 
validation...


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review106662
---


On Nov. 19, 2015, 10:55 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 19, 2015, 10:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-19 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Nov. 19, 2015, 11:55 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
Toenshoff.


Changes
---

Set principal from HTTP body. Authentication might be disabled, using the HTTP 
auth data might not work in that case.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-19 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review107170
---



src/master/quota_handler.cpp (lines 201 - 204)


This is to handle the case where authorization is enabled but 
authentication is disabled. I'm not sure if we could consider this an edge 
case. Having both enabled would make the code much simpler (see revision 3).

FYI: The HTTP "Shutdown Framework" message authorizes only when 
authentication is enabled as well.


- Jan Schlicht


On Nov. 19, 2015, 11:55 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 19, 2015, 11:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-17 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Nov. 17, 2015, 2:14 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
Toenshoff.


Changes
---

Authorize quota request before validating satisfiability.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-16 Thread Jan Schlicht 


> On Nov. 16, 2015, 3:37 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 315
> > 
> >
> > VLog?

I'm trying to be consistent with other logs of this kind (e.g. in 
`master.cpp`), which are all `LOG(INFO)`.


> On Nov. 16, 2015, 3:37 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 188
> > 
> >
> > Should this be part of the validation below?

I'd rather put it into `createQuotaInfo` as it's not validating but setting 
properties of `create`.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review106662
---


On Nov. 16, 2015, 2:53 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 16, 2015, 2:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-16 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

(Updated Nov. 16, 2015, 4:50 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
Toenshoff.


Changes
---

Fix issues.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota_handler.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-16 Thread Jan Schlicht 


> On Nov. 16, 2015, 3:37 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 239
> > 
> >
> > I see that this is consistent witht the rest of the code, but in my 
> > opinion the realm should be `Mesos`. If it turns out that this is intended, 
> > feel free to drop...

Also my opinion! But I'm trying to be consistent with the other `Unauthorized` 
return values, that all use `Mesos master` as realm. Let's create a ticket and 
fix it independently from this RR.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review106662
---


On Nov. 16, 2015, 2:53 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 16, 2015, 2:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-16 Thread Jan Schlicht 


> On Nov. 16, 2015, 3:37 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 239
> > 
> >
> > I see that this is consistent witht the rest of the code, but in my 
> > opinion the realm should be `Mesos`. If it turns out that this is intended, 
> > feel free to drop...
> 
> Jan Schlicht wrote:
> Also my opinion! But I'm trying to be consistent with the other 
> `Unauthorized` return values, that all use `Mesos master` as realm. Let's 
> create a ticket and fix it independently from this RR.

A [JIRA ticket](https://issues.apache.org/jira/browse/MESOS-3933) has been 
created.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review106662
---


On Nov. 16, 2015, 2:53 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 16, 2015, 2:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-16 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/#review106662
---



src/master/master.hpp (line 910)


Shouldn't this be autherize (similar to authenticate() and the other calls)?



src/master/master.hpp (line 913)


s/request/set ?



src/master/quota_handler.cpp (line 188)


Should this be part of the validation below?



src/master/quota_handler.cpp (line 239)


I see that this is consistent witht the rest of the code, but in my opinion 
the realm should be `Mesos`. If it turns out that this is intended, feel free 
to drop...



src/master/quota_handler.cpp (line 314)


VLog?



src/master/quota_handler.cpp (line 324)


validateQuotaInfo would have failed


- Joerg Schad


On Nov. 16, 2015, 1:53 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 16, 2015, 1:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

2015-11-16 Thread Jan Schlicht 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40347/
---

Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Till 
Toenshoff.


Bugs: MESOS-3862
https://issues.apache.org/jira/browse/MESOS-3862


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota_handler.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/40347/diff/


Testing
---

make check


Thanks,

Jan Schlicht