Re: Review Request 43388: Revised authentication documentation.

2016-02-10 Thread Neil Conway 


> On Feb. 10, 2016, 3:04 a.m., Guangya Liu wrote:
> > docs/authentication.md, line 65
> > 
> >
> > The MESOS-2281 is planning to remove the JSON format credential, does 
> > it make sense to remove the `JSON format` here as the `plaintext` is the 
> > recommended format.

I'm inclined to leave this as-is: we talk about both formats elsewhere, e.g., 
in `--help` output and in `configuration.md`. If/when we remove/deprecate the 
JSON format, we should update all those places accordingly.


- Neil


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


On Feb. 10, 2016, 2:06 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 10, 2016, 2:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43388: Revised authentication documentation.

2016-02-10 Thread Adam B 


> On Feb. 9, 2016, 5:28 p.m., Adam B wrote:
> > docs/authentication.md, line 49
> > 
> >
> > We may be deprecating the plaintext format soon

As Guangya points out, it's actually the json format that we'll be deprecating. 
Now I remember that we created the json format so we could distinguish http 
credentials from framework/agent credentials, but that proved unnecessary.


- Adam


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


On Feb. 9, 2016, 6:06 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 9, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Neil Conway 

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

(Updated Feb. 10, 2016, 12:10 a.m.)


Review request for mesos, Adam B, Greg Mann, and Vinod Kone.


Changes
---

Tweak link syntax.


Repository: mesos


Description
---

Revised authentication documentation.


Diffs (updated)
-

  docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 

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


Testing
---

Previewed in site-docker.


Thanks,

Neil Conway

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Greg Mann 

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



It's unfortunate that the docs don't currently maintain a single convention for 
line-wrapping, but they are locally consistent within each file. I would 
recommend picking either wrapping at 80 characters or not at all, and keep it 
consistent throughout this entire file.


docs/authentication.md (line 26)


Perhaps a bit misleading to call the secret an "arbitrary string" here. 
Maybe something like "the secret is used to verify that identity."?



docs/authentication.md (line 43)


Nit: perhaps true/false should be monospace formatted here, rather than 
single-quoted?



docs/authentication.md (line 67)


Not sure if you want to add it in this patch, but it would be useful to 
also have an example of credentials in JSON format.



docs/authentication.md (line 72)


Monospace formatting for the filename, here and below?


- Greg Mann


On Feb. 10, 2016, 12:10 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 10, 2016, 12:10 a.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Neil Conway 

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

Review request for mesos, Adam B, Greg Mann, and Vinod Kone.


Repository: mesos


Description
---

Revised authentication documentation.


Diffs
-

  docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 

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


Testing
---

Previewed in site-docker.


Thanks,

Neil Conway

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Adam B 


> On Feb. 9, 2016, 5:28 p.m., Adam B wrote:
> > Looks good, but I'd include a mention of FrameworkInfo.principal for use 
> > with authorization.

Probably worth linking to 
http://mesos.apache.org/documentation/latest/authorization/ there too


- Adam


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


On Feb. 9, 2016, 4:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 9, 2016, 4:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Adam B 

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


Fix it, then Ship it!




Looks good, but I'd include a mention of FrameworkInfo.principal for use with 
authorization.


docs/authentication.md (line 47)


We may be deprecating the plaintext format soon



docs/authentication.md (lines 61 - 63)


To enable authorization based on the authenticated principal, the framework 
developer should also copy the Credential.principal into 
FrameworkInfo.principal when registering.



docs/authentication.md (line 86)


Step 6.?


- Adam B


On Feb. 9, 2016, 4:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 9, 2016, 4:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Vinod Kone 

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


Fix it, then Ship it!





docs/authentication.md (line 11)


I would just say the same thing that we did with frameworks above

"""
To require that slaves be authenticated in order to register with the 
master.
"""



docs/authentication.md (lines 14 - 19)


Since we modularized the authenticator, there is no strict dependency on 
SASL/CRAM-MD5. So I would reword this to say that the default implementation 
uses SASL/CRAM-MD5.



docs/authentication.md (line 31)


used by the slave to run tasks/executors


- Vinod Kone


On Feb. 10, 2016, 12:10 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 10, 2016, 12:10 a.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Neil Conway 

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

(Updated Feb. 10, 2016, 2:06 a.m.)


Review request for mesos, Adam B, Greg Mann, and Vinod Kone.


Changes
---

Address review comments.


Repository: mesos


Description
---

Revised authentication documentation.


Diffs (updated)
-

  docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 

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


Testing
---

Previewed in site-docker.


Thanks,

Neil Conway

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [43388]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 10, 2016, 2:06 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated Feb. 10, 2016, 2:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43388: Revised authentication documentation.

2016-02-09 Thread Guangya Liu 

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


Fix it, then Ship it!




Ship It!


docs/authentication.md (line 63)


The MESOS-2281 is planning to remove the JSON format credential, does it 
make sense to remove the `JSON format` here as the `plaintext` is the 
recommended format.


- Guangya Liu


On 二月 10, 2016, 2:06 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43388/
> ---
> 
> (Updated 二月 10, 2016, 2:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised authentication documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md b40b09eaceddc8e5498b54b3e8ef7a5dbd7b9dc2 
> 
> Diff: https://reviews.apache.org/r/43388/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>