You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/02/01 17:51:15 UTC
Re: Review Request 54537: Support 'Basic' auth docker registry on
Unified Containerizer.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54537/#review163759
-----------------------------------------------------------
src/uri/fetchers/docker.cpp (line 468)
<https://reviews.apache.org/r/54537/#comment235251>
s/challenge/header/
src/uri/fetchers/docker.cpp (lines 468 - 512)
<https://reviews.apache.org/r/54537/#comment235330>
In fact, a second thought on this. Should we rename `getAuthToken` to `getAuthHeader` and put all the logic of generating the auth header in that function? In that way, you don't need to implement the same logic multiple times.
src/uri/fetchers/docker.cpp (line 473)
<https://reviews.apache.org/r/54537/#comment235252>
s/challenge/header
src/uri/fetchers/docker.cpp (line 475)
<https://reviews.apache.org/r/54537/#comment235253>
s/challenge/header/
src/uri/fetchers/docker.cpp (lines 481 - 491)
<https://reviews.apache.org/r/54537/#comment235329>
I saw this several places. Is there a way to factor out this into a helper:
```
Option<string> getAuthFromConfig(const URI& uri);
```
- Jie Yu
On Jan. 31, 2017, 9:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54537/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 9:10 a.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
>
>
> Bugs: MESOS-6758
> https://issues.apache.org/jira/browse/MESOS-6758
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch implements the support for 'Basic' docker registry
> authorization. It is tested by a local authenticated private
> registry using 'localhost:443/alpine' docker image.
> Please note that the AWS ECS uses Basic authorization but it
> does not work yet due to the redirect issue MESOS-5172.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 5dd7b91a5302067ce150bd632a05eccaf424a8a8
>
> Diff: https://reviews.apache.org/r/54537/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested with local authenticated registry. Please follow the steps below:
>
> 1. Start a local private registry and push an image to it.
> ```
> docker run -d -p 443:5000 --restart=always --name registry \
> -v `pwd`/auth:/auth \
> -e "REGISTRY_AUTH=htpasswd" \
> -e "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm" \
> -e REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd \
> -v `pwd`/certs:/certs \
> -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/localhost.crt \
> -e REGISTRY_HTTP_TLS_KEY=/certs/localhost.key \
> registry:2
> ```
> (*Note: need to generate TLS certificate file and key first)
>
> Then, push an image to the registry.
> ```
> docker push localhost:443/alpine
> ```
>
> 2. Use `mesos-execute` to test the `localhost:443/alpine` image.
> (*Note: need to configure the curl using the curl's default RC file), e.g., in `~/.curlrc` file:
> cacert = "/path/to/cacert.pem"
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54537: Support 'Basic' auth docker registry on
Unified Containerizer.
Posted by Gilbert Song <so...@gmail.com>.
> On Feb. 1, 2017, 9:51 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 470-514
> > <https://reviews.apache.org/r/54537/diff/3/?file=1619966#file1619966line470>
> >
> > In fact, a second thought on this. Should we rename `getAuthToken` to `getAuthHeader` and put all the logic of generating the auth header in that function? In that way, you don't need to implement the same logic multiple times.
Great idea. It simplify the logic.
> On Feb. 1, 2017, 9:51 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 483-493
> > <https://reviews.apache.org/r/54537/diff/3/?file=1619966#file1619966line483>
> >
> > I saw this several places. Is there a way to factor out this into a helper:
> > ```
> > Option<string> getAuthFromConfig(const URI& uri);
> > ```
Not doing that for now since it only exist in `getAuthHeader()` and we need to do differently for `Basic` auth and `Bearer` auth.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54537/#review163759
-----------------------------------------------------------
On Feb. 1, 2017, 4:42 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54537/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 4:42 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
>
>
> Bugs: MESOS-6758
> https://issues.apache.org/jira/browse/MESOS-6758
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch implements the support for 'Basic' docker registry
> authorization. It is tested by a local authenticated private
> registry using 'localhost:443/alpine' docker image.
> Please note that the AWS ECS uses Basic authorization but it
> does not work yet due to the redirect issue MESOS-5172.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 5dd7b91a5302067ce150bd632a05eccaf424a8a8
>
> Diff: https://reviews.apache.org/r/54537/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested with local authenticated registry. Please follow the steps below:
>
> 1. Start a local private registry and push an image to it.
> ```
> docker run -d -p 443:5000 --restart=always --name registry \
> -v `pwd`/auth:/auth \
> -e "REGISTRY_AUTH=htpasswd" \
> -e "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm" \
> -e REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd \
> -v `pwd`/certs:/certs \
> -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/localhost.crt \
> -e REGISTRY_HTTP_TLS_KEY=/certs/localhost.key \
> registry:2
> ```
> (*Note: need to generate TLS certificate file and key first)
>
> Then, push an image to the registry.
> ```
> docker push localhost:443/alpine
> ```
>
> 2. Use `mesos-execute` to test the `localhost:443/alpine` image.
> (*Note: need to configure the curl using the curl's default RC file), e.g., in `~/.curlrc` file:
> cacert = "/path/to/cacert.pem"
>
>
> Thanks,
>
> Gilbert Song
>
>