Re: Review Request 45952: Implemented support for passing agent default docker config.

[Jie Yu]

> On June 12, 2016, 6:08 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 647-649
> > 
> >
> > I don't get this part. This is not what you're doing here, right? I 
> > think a request to authorization server will return an 401 and we will  set 
> > auth header and send the request again. Putting the comments here is 
> > confusing. I would simply add a TODO here saying that here we assume the 
> > auth is basic, and we simply include the auth header while sending the 
> > request. It'll be ignored if auth is not required.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will  set auth header and send the request again.
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will set auth header and send the request again.
> 
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> >> Putting the comments here is confusing.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.
> 
> Jie Yu wrote:
> Regarding using an incorrect toke, are you sure? I want to confirm this. 
> It sounds unintuitive to me.
> 
> Gilbert Song wrote:
> Yeah, I remember it behaves this way which also sounds weird to me. Just 
> re-confirmed by commandline:
> 
> # no basic auth header attached to authentication server, 401 is from 
> registry because of incorrect token:
> ```
> vagrant@vagrant-ubuntu-trusty-64:~$ curl -s -S -L -D - 
> 'https://auth.docker.io/token?service=registry.docker.io&scope=repository:gilbertsong/inky:pull'
> HTTP/1.1 200 OK
> Content-Type: application/json
> Date: Sun, 12 Jun 2016 19:17:30 GMT
> Content-Length: 1358
> Strict-Transport-Security: max-age=31536000
> 
> 
> {"token":"eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCIsIng1YyI6WyJNSUlDTHpDQ0FkU2dBd0lCQWdJQkFEQUtCZ2dxaGtqT1BRUURBakJHTVVRd1FnWURWUVFERXp0Uk5Gb3pPa2RYTjBrNldGUlFSRHBJVFRSUk9rOVVWRmc2TmtGRlF6cFNUVE5ET2tGU01rTTZUMFkzTnpwQ1ZrVkJPa2xHUlVrNlExazFTekFlRncweE5qQTFNekV5TXpVNE5UZGFGdzB4TnpBMU16RXlNelU0TlRkYU1FWXhSREJDQmdOVkJBTVRPMUV6UzFRNlFqSkpNenBhUjFoT09qSlhXRTA2UTBWWFF6cFVNMHhPT2tvMlYxWTZNbGsyVHpwWlFWbEpPbGhQVTBRNlZFUlJTVG8wVWtwRE1Ga3dFd1lIS29aSXpqMENBUVlJS29aSXpqMERBUWNEUWdBRVo0NkVLV3VKSXhxOThuUC9GWEU3U3VyOXlkZ3c3K2FkcndxeGlxN004VHFUa0N0dzBQZm1SS2VLdExwaXNTRFU4LzZseWZ3QUFwZWh6SHdtWmxZR2dxT0JzakNCcnpBT0JnTlZIUThCQWY4RUJBTUNCNEF3RHdZRFZSMGxCQWd3QmdZRVZSMGxBREJFQmdOVkhRNEVQUVE3VVROTFZEcENNa2t6T2xwSFdFNDZNbGRZVFRwRFJWZERPbFF6VEU0NlNqWlhWam95V1RaUE9sbEJXVWs2V0U5VFJEcFVSRkZKT2pSU1NrTXdSZ1lEVlIwakJEOHdQWUE3VVRSYU16cEhWemRKT2xoVVVFUTZTRTAwVVRwUFZGUllPalpCUlVNNlVrMHpRenBCVWpKRE9rOUdOemM2UWxaRlFUcEpSa1ZKT2tOWk5Vc3dDZ1lJS29aSXpqMEVBd0lEU1FBd1JnSWhBTzYxSWloN1FUcHNTMFFIYUNwTDFZTWNMMnZXZlNydlhHbHpSRDEwN2
 
NRUEFpRUFtZXduelNYRHplRGxqcDc4T1NsTFFzbnROYWM5eHRyYW0xU0kxY0ZXQ2tJPSJdfQ.eyJhY2Nlc3MiOltdLCJhdWQiOiJyZWdpc3RyeS5kb2NrZXIuaW8iLCJleHAiOjE0NjU3NTkzNTAsImlhdCI6MTQ2NTc1OTA1MCwiaXNzIjoiYXV0aC5kb2NrZXIuaW8iLCJqdGkiOiJ4MmlfMkxmOEhoUU9IT1VRajV0SiIsIm5iZiI6MTQ2NTc1OTA1MCwic3ViIjoiIn0.WiGorARqU93j6hCXOgXvx_lt9QyCD59oGOLbmbZox1Cqq44rV13hLeqCCtemaWS3gpwtZ12z9exogJNgRRryfA"}
> 
> vagrant@vagrant-ubuntu-trusty-64:~$ curl -s -S -L -D - -H 'Authorization: 
> Bearer 
> eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCIsIng1YyI6WyJNSUlDTHpDQ0FkU2dBd0lCQWdJQkFEQUtCZ2dxaGtqT1BRUURBakJHTVVRd1FnWURWUVFERXp0Uk5Gb3pPa2RYTjBrNldGUlFSRHBJVFRSUk9rOVVWRmc2TmtGRlF6cFNUVE5ET2tGU01rTTZUMFkzTnpwQ1ZrVkJPa2xHUlVrNlExazFTekFlRncweE5qQTFNekV5TXpVNE5UZGFGdzB4TnpBMU16RXlNelU0TlRkYU1FWXhSREJDQmdOVkJBTVRPMUV6UzFRNlFqSkpNenBhUjFoT09qSlhXRTA2UTBWWFF6cFVNMHhPT2tvMlYxWTZNbGsyVHpwWlFWbEpPbGhQVTBRNlZFUlJTVG8wVWtwRE1Ga3dFd1lIS29aSXpqMENBUVlJS29aSXpqMERBUWNEUWdBRVo0NkVLV3VKSXhxOThuUC9GWEU3U3VyOXlkZ3c3K2FkcndxeGlxN004VHFUa0N0dzBQZm1SS2VLdExwaXNTRFU4LzZseWZ3QUFwZWh6SHdtWmxZR2dxT0JzakNCcnpBT0JnTlZIUThCQWY4RUJBTUNCNEF3RHdZRFZSMGxCQWd3QmdZRVZSMGxBREJFQmdOVkhRNEVQUVE3VVROTFZEcENNa2t6T2xwSFdFNDZNbGRZVFRwRFJWZERPbFF6VEU0NlNqWlhWam95V1RaUE9sbEJXVWs2V0U5VFJEcFVSRkZKT2pSU1NrTXdSZ1lEVlIwakJEOHdQWUE3VVRSYU16cEhWemRKT2xo

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

> On June 12, 2016, 11:08 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 647-649
> > 
> >
> > I don't get this part. This is not what you're doing here, right? I 
> > think a request to authorization server will return an 401 and we will  set 
> > auth header and send the request again. Putting the comments here is 
> > confusing. I would simply add a TODO here saying that here we assume the 
> > auth is basic, and we simply include the auth header while sending the 
> > request. It'll be ignored if auth is not required.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will  set auth header and send the request again.
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will set auth header and send the request again.
> 
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> >> Putting the comments here is confusing.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.
> 
> Jie Yu wrote:
> Regarding using an incorrect toke, are you sure? I want to confirm this. 
> It sounds unintuitive to me.

Yeah, I remember it behaves this way which also sounds weird to me. Just 
re-confirmed by commandline:

# no basic auth header attached to authentication server, 401 is from registry 
because of incorrect token:
```
vagrant@vagrant-ubuntu-trusty-64:~$ curl -s -S -L -D - 
'https://auth.docker.io/token?service=registry.docker.io&scope=repository:gilbertsong/inky:pull'
HTTP/1.1 200 OK
Content-Type: application/json
Date: Sun, 12 Jun 2016 19:17:30 GMT
Content-Length: 1358
Strict-Transport-Security: max-age=31536000

{"token":"eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCIsIng1YyI6WyJNSUlDTHpDQ0FkU2dBd0lCQWdJQkFEQUtCZ2dxaGtqT1BRUURBakJHTVVRd1FnWURWUVFERXp0Uk5Gb3pPa2RYTjBrNldGUlFSRHBJVFRSUk9rOVVWRmc2TmtGRlF6cFNUVE5ET2tGU01rTTZUMFkzTnpwQ1ZrVkJPa2xHUlVrNlExazFTekFlRncweE5qQTFNekV5TXpVNE5UZGFGdzB4TnpBMU16RXlNelU0TlRkYU1FWXhSREJDQmdOVkJBTVRPMUV6UzFRNlFqSkpNenBhUjFoT09qSlhXRTA2UTBWWFF6cFVNMHhPT2tvMlYxWTZNbGsyVHpwWlFWbEpPbGhQVTBRNlZFUlJTVG8wVWtwRE1Ga3dFd1lIS29aSXpqMENBUVlJS29aSXpqMERBUWNEUWdBRVo0NkVLV3VKSXhxOThuUC9GWEU3U3VyOXlkZ3c3K2FkcndxeGlxN004VHFUa0N0dzBQZm1SS2VLdExwaXNTRFU4LzZseWZ3QUFwZWh6SHdtWmxZR2dxT0JzakNCcnpBT0JnTlZIUThCQWY4RUJBTUNCNEF3RHdZRFZSMGxCQWd3QmdZRVZSMGxBREJFQmdOVkhRNEVQUVE3VVROTFZEcENNa2t6T2xwSFdFNDZNbGRZVFRwRFJWZERPbFF6VEU0NlNqWlhWam95V1RaUE9sbEJXVWs2V0U5VFJEcFVSRkZKT2pSU1NrTXdSZ1lEVlIwakJEOHdQWUE3VVRSYU16cEhWemRKT2xoVVVFUTZTRTAwVVRwUFZGUllPalpCUlVNNlVrMHpRenBCVWpKRE9rOUdOemM2UWxaRlFUcEpSa1ZKT2tOWk5Vc3dDZ1lJS29aSXpqMEVBd0lEU1FBd1JnSWhBTzYxSWloN1FUcHNTMFFIYUNwTDFZTWNMMnZXZlNydlhHbHpSRDEwN2NRUEFp
 
RUFtZXduelNYRHplRGxqcDc4T1NsTFFzbnROYWM5eHRyYW0xU0kxY0ZXQ2tJPSJdfQ.eyJhY2Nlc3MiOltdLCJhdWQiOiJyZWdpc3RyeS5kb2NrZXIuaW8iLCJleHAiOjE0NjU3NTkzNTAsImlhdCI6MTQ2NTc1OTA1MCwiaXNzIjoiYXV0aC5kb2NrZXIuaW8iLCJqdGkiOiJ4MmlfMkxmOEhoUU9IT1VRajV0SiIsIm5iZiI6MTQ2NTc1OTA1MCwic3ViIjoiIn0.WiGorARqU93j6hCXOgXvx_lt9QyCD59oGOLbmbZox1Cqq44rV13hLeqCCtemaWS3gpwtZ12z9exogJNgRRryfA"}

vagrant@vagrant-ubuntu-trusty-64:~$ curl -s -S -L -D - -H 'Authorization: 
Bearer 
eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCIsIng1YyI6WyJNSUlDTHpDQ0FkU2dBd0lCQWdJQkFEQUtCZ2dxaGtqT1BRUURBakJHTVVRd1FnWURWUVFERXp0Uk5Gb3pPa2RYTjBrNldGUlFSRHBJVFRSUk9rOVVWRmc2TmtGRlF6cFNUVE5ET2tGU01rTTZUMFkzTnpwQ1ZrVkJPa2xHUlVrNlExazFTekFlRncweE5qQTFNekV5TXpVNE5UZGFGdzB4TnpBMU16RXlNelU0TlRkYU1FWXhSREJDQmdOVkJBTVRPMUV6UzFRNlFqSkpNenBhUjFoT09qSlhXRTA2UTBWWFF6cFVNMHhPT2tvMlYxWTZNbGsyVHpwWlFWbEpPbGhQVTBRNlZFUlJTVG8wVWtwRE1Ga3dFd1lIS29aSXpqMENBUVlJS29aSXpqMERBUWNEUWdBRVo0NkVLV3VKSXhxOThuUC9GWEU3U3VyOXlkZ3c3K2FkcndxeGlxN004VHFUa0N0dzBQZm1SS2VLdExwaXNTRFU4LzZseWZ3QUFwZWh6SHdtWmxZR2dxT0JzakNCcnpBT0JnTlZIUThCQWY4RUJBTUNCNEF3RHdZRFZSMGxCQWd3QmdZRVZSMGxBREJFQmdOVkhRNEVQUVE3VVROTFZEcENNa2t6T2xwSFdFNDZNbGRZVFRwRFJWZERPbFF6VEU0NlNqWlhWam95V1RaUE9sbEJXVWs2V0U5VFJEcFVSRkZKT2pSU1NrTXdSZ1lEVlIwakJEOHdQWUE3VVRSYU16cEhWemRKT2xoVVVFUTZTRTAwVVRwUFZGUllPalpCUlVNNlVrMHpRenBCVWpKRE9rOUdOemM2UWxaRlFUcEpSa1ZKT2tOWk5Vc3dDZ1lJS29aSXpqMEVBd0lEU
 
1FBd1JnS

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

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

(Updated June 12, 2016, 12:11 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented support for passing agent default docker config.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
cd5849bb9cdd12f2240885a0eae90569d2a9502e 
  src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub. Both cases are 
tested:
1. --docker_registry=private_registry
2. private_registry/repo


Thanks,

Gilbert Song

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Jie Yu]

> On June 12, 2016, 6:08 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 647-649
> > 
> >
> > I don't get this part. This is not what you're doing here, right? I 
> > think a request to authorization server will return an 401 and we will  set 
> > auth header and send the request again. Putting the comments here is 
> > confusing. I would simply add a TODO here saying that here we assume the 
> > auth is basic, and we simply include the auth header while sending the 
> > request. It'll be ignored if auth is not required.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will  set auth header and send the request again.
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will set auth header and send the request again.
> 
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> >> Putting the comments here is confusing.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.

Regarding using an incorrect toke, are you sure? I want to confirm this. It 
sounds unintuitive to me.


- Jie


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


On June 11, 2016, 12:42 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated June 11, 2016, 12:42 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> cd5849bb9cdd12f2240885a0eae90569d2a9502e 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

> On June 12, 2016, 11:08 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 647-649
> > 
> >
> > I don't get this part. This is not what you're doing here, right? I 
> > think a request to authorization server will return an 401 and we will  set 
> > auth header and send the request again. Putting the comments here is 
> > confusing. I would simply add a TODO here saying that here we assume the 
> > auth is basic, and we simply include the auth header while sending the 
> > request. It'll be ignored if auth is not required.
> 
> Gilbert Song wrote:
> Step 3 ~ 7 is what I haven't done here.
> 
> >> I think a request to authorization server will return an 401 and we 
> will  set auth header and send the request again.
> The auth server would always return a token, but the token is incorrect 
> (not accessible) if no auth attached or the basic auth is incorrect. We could 
> only know that once the wrong token sent to registry and get an 401. Then we 
> will attach basic auth to request another token again.
> 
> Yeah, a little confusing here. Just want to make it in detail, otherwise 
> people may not understand what this TODO exactly is.

Step 3 ~ 7 is what I haven't done here.

>> I think a request to authorization server will return an 401 and we will set 
>> auth header and send the request again.

The auth server would always return a token, but the token is incorrect (not 
accessible) if no auth attached or the basic auth is incorrect. We could only 
know that once the wrong token sent to registry and get an 401. Then we will 
attach basic auth to request another token again.

>> Putting the comments here is confusing.

Yeah, a little confusing here. Just want to make it in detail, otherwise people 
may not understand what this TODO exactly is.


- Gilbert


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


On June 10, 2016, 5:42 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated June 10, 2016, 5:42 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> cd5849bb9cdd12f2240885a0eae90569d2a9502e 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

> On June 12, 2016, 11:08 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 647-649
> > 
> >
> > I don't get this part. This is not what you're doing here, right? I 
> > think a request to authorization server will return an 401 and we will  set 
> > auth header and send the request again. Putting the comments here is 
> > confusing. I would simply add a TODO here saying that here we assume the 
> > auth is basic, and we simply include the auth header while sending the 
> > request. It'll be ignored if auth is not required.

Step 3 ~ 7 is what I haven't done here.

>> I think a request to authorization server will return an 401 and we will  
>> set auth header and send the request again.
The auth server would always return a token, but the token is incorrect (not 
accessible) if no auth attached or the basic auth is incorrect. We could only 
know that once the wrong token sent to registry and get an 401. Then we will 
attach basic auth to request another token again.

Yeah, a little confusing here. Just want to make it in detail, otherwise people 
may not understand what this TODO exactly is.


- Gilbert


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


On June 10, 2016, 5:42 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated June 10, 2016, 5:42 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> cd5849bb9cdd12f2240885a0eae90569d2a9502e 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Jie Yu]

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


Fix it, then Ship it!





src/uri/fetchers/docker.cpp (lines 645 - 647)


I don't get this part. This is not what you're doing here, right? I think a 
request to authorization server will return an 401 and we will  set auth header 
and send the request again. Putting the comments here is confusing. I would 
simply add a TODO here saying that here we assume the auth is basic, and we 
simply include the auth header while sending the request. It'll be ignored if 
auth is not required.



src/uri/fetchers/docker.cpp (line 664)


This check is redundant, isn't it? I would just remove it.



src/uri/fetchers/docker.cpp (line 693)


I would kill this line.


- Jie Yu


On June 11, 2016, 12:42 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated June 11, 2016, 12:42 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> cd5849bb9cdd12f2240885a0eae90569d2a9502e 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

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

(Updated June 10, 2016, 5:42 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented support for passing agent default docker config.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
cd5849bb9cdd12f2240885a0eae90569d2a9502e 
  src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub. Both cases are 
tested:
1. --docker_registry=private_registry
2. private_registry/repo


Thanks,

Gilbert Song

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Jie Yu]

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




src/uri/fetchers/docker.cpp (line 281)


We typically use leading underscore for function parameters and the non 
leading underscore version for the field members.



src/uri/fetchers/docker.cpp (line 319)


`s/_cachedAuths/auths/`

Also, why optional? what's the difference between None() and an empty 
hashmap? I would suggest that we use hashmap here (without optional).



src/uri/fetchers/docker.cpp (line 576)


I would rename this to uri, and rename the existing uri to authServerUri.

ALso, please add a comment about why you need to pass in the uri here.



src/uri/fetchers/docker.cpp (line 635)


Ideally, we should do that after getting the 401 response. Can you add a 
TODO here?

Also, i think from docker 1.11, it support OAuth2, so you probably should 
add another TODO here.

I think we should think about how are we going to support both. You should 
mention that in the TODO.



src/uri/fetchers/docker.cpp (lines 644 - 647)


Can you figure out how exactly docker does that? It's better to understand 
their logic, rather than treat it as a black box?


- Jie Yu


On May 25, 2016, 1:33 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated May 25, 2016, 1:33 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

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

(Updated May 24, 2016, 6:33 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented support for passing agent default docker config.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
eeec94326a4fd67675df10e0b6a32267e555fa96 
  src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub. Both cases are 
tested:
1. --docker_registry=private_registry
2. private_registry/repo


Thanks,

Gilbert Song

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Guangya Liu]

> On 四月 11, 2016, 7:37 a.m., Guangya Liu wrote:
> > src/uri/fetchers/docker.cpp, line 636
> > 
> >
> > Add a log here to identify that the fetcher is now using auth to fetch 
> > the image?
> 
> Guangya Liu wrote:
> Adding the log in `getCredential` may be better, post some comments in 
> https://reviews.apache.org/r/45949/ , please check. Thanks.
> 
> Gilbert Song wrote:
> Thanks for the comments, Guangya! Seems like we dont have any log in 
> docker fetcher.

It will be very difficult for trouble shooting when I encounter some issue in 
fetcher, is it possible that you can put some log first for your project and we 
can add more log if needed?


- Guangya


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


On 五月 23, 2016, 9:52 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated 五月 23, 2016, 9:52 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

> On April 11, 2016, 12:37 a.m., Guangya Liu wrote:
> > src/uri/fetchers/docker.cpp, lines 323-325
> > 
> >
> > Does there are any document change for this?

Unfortunately, we dont have document for docker fetcher yet.


> On April 11, 2016, 12:37 a.m., Guangya Liu wrote:
> > src/uri/fetchers/docker.cpp, line 636
> > 
> >
> > Add a log here to identify that the fetcher is now using auth to fetch 
> > the image?
> 
> Guangya Liu wrote:
> Adding the log in `getCredential` may be better, post some comments in 
> https://reviews.apache.org/r/45949/ , please check. Thanks.

Thanks for the comments, Guangya! Seems like we dont have any log in docker 
fetcher.


- Gilbert


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


On May 23, 2016, 2:52 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated May 23, 2016, 2:52 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

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

(Updated May 23, 2016, 2:52 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented support for passing agent default docker config.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
eeec94326a4fd67675df10e0b6a32267e555fa96 
  src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub. Both cases are 
tested:
1. --docker_registry=private_registry
2. private_registry/repo


Thanks,

Gilbert Song

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45949, 45950, 45951, 45952]

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 April 8, 2016, 9:50 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated April 8, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Guangya Liu]

> On 四月 11, 2016, 7:37 a.m., Guangya Liu wrote:
> > src/uri/fetchers/docker.cpp, line 636
> > 
> >
> > Add a log here to identify that the fetcher is now using auth to fetch 
> > the image?

Adding the log in `getCredential` may be better, post some comments in 
https://reviews.apache.org/r/45949/ , please check. Thanks.


- Guangya


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


On 四月 8, 2016, 9:50 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated 四月 8, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Guangya Liu]

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




src/uri/fetchers/docker.cpp (lines 323 - 325)


Does there are any document change for this?



src/uri/fetchers/docker.cpp (line 634)


Add a log here to identify that the fetcher is now using auth to fetch the 
image?


- Guangya Liu


On 四月 8, 2016, 9:50 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated 四月 8, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

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

Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented support for passing agent default docker config.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
eeec94326a4fd67675df10e0b6a32267e555fa96 
  src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub. Both cases are 
tested:
1. --docker_registry=private_registry
2. private_registry/repo


Thanks,

Gilbert Song