You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Adam B <ad...@mesosphere.io> on 2014/04/14 07:02:03 UTC
Review Request 20301: Moved reading of credentials file into sasl/common.hpp
for reuse.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description
-------
Moved reading of credentials file out of master.cpp and into sasl/common.hpp for reuse by slave authentication.
Diffs
-----
src/master/master.cpp 3c3c989
src/sasl/common.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/20301/diff/
Testing
-------
make check
Thanks,
Adam B
Re: Review Request 20301: Moved reading of credentials file into
sasl/common.hpp for reuse.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 16, 2014, 1:49 a.m., Vinod Kone wrote:
> > src/sasl/common.hpp, line 32
> > <https://reviews.apache.org/r/20301/diff/1/?file=555776#file555776line32>
> >
> > put this under credentials namespace.
>
> Adam B wrote:
> Do you want it in mesos::internal::sasl::credentials or in mesos::internal::credentials?
> It really has nothing sasl-specific in it, so I'm leaning towards the latter, in which case should I move it to src/credentials/credentials.hpp?
sgtm.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40487
-----------------------------------------------------------
On April 14, 2014, 5:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 14, 2014, 5:01 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Moved reading of credentials file out of master.cpp and into sasl/common.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3c3c989
> src/sasl/common.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Moved reading of credentials file into
sasl/common.hpp for reuse.
Posted by Adam B <ad...@mesosphere.io>.
> On April 15, 2014, 6:49 p.m., Vinod Kone wrote:
> > src/sasl/common.hpp, line 32
> > <https://reviews.apache.org/r/20301/diff/1/?file=555776#file555776line32>
> >
> > put this under credentials namespace.
Do you want it in mesos::internal::sasl::credentials or in mesos::internal::credentials?
It really has nothing sasl-specific in it, so I'm leaning towards the latter, in which case should I move it to src/credentials/credentials.hpp?
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40487
-----------------------------------------------------------
On April 13, 2014, 10:01 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 10:01 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Moved reading of credentials file out of master.cpp and into sasl/common.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3c3c989
> src/sasl/common.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Moved reading of credentials file into
sasl/common.hpp for reuse.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40487
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/20301/#comment73520>
I think there are too many nested if else's here making it complicated to understand. Can you think of a better refactor that is easy to follow?
src/master/master.cpp
<https://reviews.apache.org/r/20301/#comment73514>
We need to print this even when --credentials is not specified.
src/sasl/common.hpp
<https://reviews.apache.org/r/20301/#comment73517>
put this under credentials namespace.
src/sasl/common.hpp
<https://reviews.apache.org/r/20301/#comment73519>
with the name space change you can
s/readCredentials/read/
- Vinod Kone
On April 14, 2014, 5:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 14, 2014, 5:01 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Moved reading of credentials file out of master.cpp and into sasl/common.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3c3c989
> src/sasl/common.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Refactored reading credentials out of master.cpp
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/
-----------------------------------------------------------
(Updated April 17, 2014, 1:56 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Added credentials.hpp to Makefile.am, and responded to other review feedback.
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description
-------
Refactored reading credentials file out of master.cpp and into credentials/credentials.hpp for reuse by slave authentication.
Diffs (updated)
-----
src/Makefile.am 560b4c7
src/credentials/credentials.hpp PRE-CREATION
src/master/master.cpp 0335b34
Diff: https://reviews.apache.org/r/20301/diff/
Testing
-------
make check
Thanks,
Adam B
Re: Review Request 20301: Refactored reading credentials out of master.cpp
Posted by Adam B <ad...@mesosphere.io>.
> On April 17, 2014, 10:39 a.m., Vinod Kone wrote:
> > you also need to update the Makefile.
Right. Done.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40668
-----------------------------------------------------------
On April 17, 2014, 1:56 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 1:56 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Refactored reading credentials file out of master.cpp and into credentials/credentials.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/credentials/credentials.hpp PRE-CREATION
> src/master/master.cpp 0335b34
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Refactored reading credentials out of master.cpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40668
-----------------------------------------------------------
you also need to update the Makefile.
- Vinod Kone
On April 17, 2014, 8:56 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 8:56 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Refactored reading credentials file out of master.cpp and into credentials/credentials.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/credentials/credentials.hpp PRE-CREATION
> src/master/master.cpp 0335b34
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Refactored reading credentials out of master.cpp
Posted by Adam B <ad...@mesosphere.io>.
> On April 17, 2014, 10:34 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 298
> > <https://reviews.apache.org/r/20301/diff/2/?file=561568#file561568line298>
> >
> > Log?
No longer doing validation here, just logging authentication state (changed comment to "Log authentication state." in case you like that better).
With the addition of the --authenticate_slaves flag, we would need to EXIT(1) << "Authentication requires a credential file" if flags.credentials.isNone() and either authenticate flag is set.
To simplify the code, this section now only logs the authentication state, and then we'll load credentials if flags.credentials.isSome(), else if either authenticate flag is set then we exit with the "requires credentials file" message.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40665
-----------------------------------------------------------
On April 17, 2014, 1:56 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 1:56 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Refactored reading credentials file out of master.cpp and into credentials/credentials.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/credentials/credentials.hpp PRE-CREATION
> src/master/master.cpp 0335b34
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Refactored reading credentials out of master.cpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/#review40665
-----------------------------------------------------------
Ship it!
src/credentials/credentials.hpp
<https://reviews.apache.org/r/20301/#comment73723>
This flag seems odd. Im assuming this is because of the difference between master and slave flag. Let the callers (master/slave) specify the flag when they do EXIT.
src/master/master.cpp
<https://reviews.apache.org/r/20301/#comment73724>
Log?
- Vinod Kone
On April 17, 2014, 8:56 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20301/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 8:56 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-804
> https://issues.apache.org/jira/browse/MESOS-804
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Refactored reading credentials file out of master.cpp and into credentials/credentials.hpp for reuse by slave authentication.
>
>
> Diffs
> -----
>
> src/credentials/credentials.hpp PRE-CREATION
> src/master/master.cpp 0335b34
>
> Diff: https://reviews.apache.org/r/20301/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 20301: Refactored reading credentials out of master.cpp
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20301/
-----------------------------------------------------------
(Updated April 17, 2014, 1:56 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Reorganized master's conditional logic for logging authentication flags and reading credentials, for less nesting and better readability.
Moved sasl/common.hpp to credentials/credentials.hpp and put it in the mesos::internal::credentials namespace.
Summary (updated)
-----------------
Refactored reading credentials out of master.cpp
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description (updated)
-------
Refactored reading credentials file out of master.cpp and into credentials/credentials.hpp for reuse by slave authentication.
Diffs (updated)
-----
src/credentials/credentials.hpp PRE-CREATION
src/master/master.cpp 0335b34
Diff: https://reviews.apache.org/r/20301/diff/
Testing
-------
make check
Thanks,
Adam B