> On March 11, 2016, 2:52 p.m., Alexander Rojas wrote:
> > src/authentication/http/basic_authenticator_factory.cpp, lines 70-71
> > <https://reviews.apache.org/r/44678/diff/2/?file=1295691#file1295691line70>
> >
> >     Wouldn't it be better to just get a `Credentials` object?
> >     
> >     Or at least in the documentation add an example of how exactly a module 
> > protobuf would look like. (see my comment's in the next patch)

My reasoning for doing it this way was to avoid having nested "credentials" 
keys. i.e., if we parse directly to `Credentials` here we end up with 
parameters like:

{
  "key": "credentials",
  "value": {
    "credentials": [
      {
        "principal": "user",
        "secret": "password"
      }
    ]
  }
}

Though, the way I've implemented it, it doesn't exactly mirror the structure of 
the `--credentials` flag due to the structure of the `Parameters` message. I 
think I still prefer the current solution, however. I updated the Doxygen 
comments as you suggested :-)


- Greg


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


On March 11, 2016, 9:39 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44678/
> -----------------------------------------------------------
> 
> (Updated March 11, 2016, 9:39 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4850
>     https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Modified basic HTTP authenticator creator to accept realm.
> 
> To accommodate different authentication realms for the master and agent, the 
> default basic HTTP authenticator needs to accept its authentication realm as 
> a parameter. This patch adds this parameter and modifies the HTTP 
> authentication tests to validate it appropriately.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
>   src/authentication/http/basic_authenticator_factory.cpp 
> 62f851685db3b42c52bbcb7cff3e4f4703004ed7 
>   src/master/master.cpp 249e82ffcef35aa8df3c5b9faef5b9b25d68facc 
>   src/tests/http_authentication_tests.cpp 
> cf2bb762272fa38e04e5c26aef2858300bbd0459 
> 
> Diff: https://reviews.apache.org/r/44678/diff/
> 
> 
> Testing
> -------
> 
> `make check` was used to test on both OSX and CentOS 7.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to