Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Greg Mann]

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

(Updated April 13, 2017, 6:04 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
Kone.


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


Repository: mesos


Description
---

This patch changes the `claims` member of the authentication
`Principal` struct from a `std::map` to a `hashmap`, so that
we can make use of the `contains()` helper during authorization.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
272d92117547512328c09dfc04c6ca4bf7b6b937 


Diff: https://reviews.apache.org/r/58251/diff/2/

Changes: https://reviews.apache.org/r/58251/diff/1-2/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On April 7, 2017, 5:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58251/
> ---
> 
> (Updated April 7, 2017, 5:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `claims` member of the authentication
> `Principal` struct from a `std::map` to a `hashmap`, so that
> we can make use of the `contains()` helper during authorization.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/58251/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On April 7, 2017, 3:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58251/
> ---
> 
> (Updated April 7, 2017, 3:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `claims` member of the authentication
> `Principal` struct from a `std::map` to a `hashmap`, so that
> we can make use of the `contains()` helper during authorization.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/58251/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Greg Mann]

> On April 7, 2017, 7:47 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp
> > Line 69 (original), 69 (patched)
> > 
> >
> > My only concern with using either a `std::map` or a `hashmap` is that 
> > it forces clients to have only one entry of each claim which forces us to 
> > find creative ways around this. How about using `std::multi_map`, 
> > `std::multi_hashmap` or `hashmap`?

I think our decision here should be determined by what we think is the most 
appropriate API for authenticators/authorizers. It could make sense to allow 
multiple occurrences of the same key; for example, to allow a single 
`Principal` to contain claims associating it with multiple usernames. If we 
decide to go that way, we could use our `multihashmap` class.

I would suggest that we merge this change to unblock the current patch chain, 
and consider independently the option of moving to a `multihashmap`. I think it 
may be a good change to make, and best to do it before 1.3 ships if we're going 
to.


- Greg


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


On April 7, 2017, 3:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58251/
> ---
> 
> (Updated April 7, 2017, 3:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `claims` member of the authentication
> `Principal` struct from a `std::map` to a `hashmap`, so that
> we can make use of the `contains()` helper during authorization.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/58251/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Alexander Rojas]

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




3rdparty/libprocess/include/process/authenticator.hpp
Line 69 (original), 69 (patched)


My only concern with using either a `std::map` or a `hashmap` is that it 
forces clients to have only one entry of each claim which forces us to find 
creative ways around this. How about using `std::multi_map`, 
`std::multi_hashmap` or `hashmap`?


- Alexander Rojas


On April 7, 2017, 5:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58251/
> ---
> 
> (Updated April 7, 2017, 5:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `claims` member of the authentication
> `Principal` struct from a `std::map` to a `hashmap`, so that
> we can make use of the `contains()` helper during authorization.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/58251/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>