Re: Review Request 58778: Supported GCE container registry.

[Gilbert Song]

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


Ship it!




LGTM! Ship it!

- Gilbert Song


On May 15, 2017, 5:50 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 15, 2017, 5:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 4. JFrog SaaS
> 5. Local Nexus registry 3.3.1
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Gilbert Song]

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




src/uri/fetchers/docker.cpp
Line 660 (original), 715 (patched)


I like the `__fetchBlob()` change. More readable in code.


- Gilbert Song


On May 15, 2017, 5:50 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 15, 2017, 5:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 4. JFrog SaaS
> 5. Local Nexus registry 3.3.1
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

(Updated May 16, 2017, 12:50 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs (updated)
-

  src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 


Diff: https://reviews.apache.org/r/58778/diff/5/

Changes: https://reviews.apache.org/r/58778/diff/4-5/


Testing
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR
4. JFrog SaaS
5. Local Nexus registry 3.3.1


Thanks,

Chun-Hung Hsiao

Re: Review Request 58778: Supported GCE container registry.

[Jie Yu]

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


Fix it, then Ship it!





src/uri/fetchers/docker.cpp
Lines 449 (patched)


We have getAuthHeaderBasic and now we have getBasicAuthHeader. This is 
really confusing.

Let's use the same name `getAuthHeaderBasic` here (just different overload).



src/uri/fetchers/docker.cpp
Lines 693 (patched)


This is pretty confusing. Can we rename this parameter `basicAuthHeaders`?


- Jie Yu


On May 15, 2017, 5:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 15, 2017, 5:39 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 4. JFrog SaaS
> 5. Local Nexus registry 3.3.1
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58778]

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

- Mesos Reviewbot


On May 15, 2017, 5:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 15, 2017, 5:39 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 4. JFrog SaaS
> 5. Local Nexus registry 3.3.1
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

(Updated May 15, 2017, 5:39 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Addressed Gilbert's comments.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs (updated)
-

  src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 


Diff: https://reviews.apache.org/r/58778/diff/4/

Changes: https://reviews.apache.org/r/58778/diff/3-4/


Testing
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR
4. JFrog SaaS
5. Local Nexus registry 3.3.1


Thanks,

Chun-Hung Hsiao

Re: Review Request 58778: Supported GCE container registry.

[Gilbert Song]

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



This patch looks good. Need to take aother look at the `fetchBlob()` logic once 
I wake up. Will make another pass. Thanks!


src/uri/fetchers/docker.cpp
Lines 653 (patched)


s/furute/futrue/g



src/uri/fetchers/docker.cpp
Lines 748 (patched)


Seems like a pre-request of calling `getAuthHeader()` is calling 
`getAuthHeaderBasic()` first. I am fine with this change (a hard dependency), 
but should we rename this variable (as well as the one in `_fetch()` to be 
`basicAuthHeaders`?

It might be less confusing since we have another `getAuthHeaders()` helper.


- Gilbert Song


On May 12, 2017, 5:45 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 12, 2017, 5:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 4. JFrog SaaS
> 5. Local Nexus registry 3.3.1
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58778]

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

- Mesos Reviewbot


On May 13, 2017, 12:45 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 13, 2017, 12:45 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 4. JFrog SaaS
> 5. Local Nexus registry 3.3.1
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

(Updated May 13, 2017, 12:45 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Updated the logic so it is easier to support image secrets. In the future, we 
would like to do the following things:
1. Support image secrets: `fetch()` would receive one more `data` argument for 
secret credentials, and it would merge the default docker config and the secret 
credentials and pass the combined `auths` into `getAuthHeaderBasic()`.
2. Currently the `Basic` credential is constructed in `fetch()` and thus if the 
token is expired when downloading the blobs there's no way to get the `Basic` 
cerdential again. I'd like to refactor this part so the `Basic` credential 
could be integrated into the `userinfo` field in the new `URI` standard to 
avoid this problem, and the whole docker config should be processed in the 
docker registry puller instead of the fetcher plugin.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs (updated)
-

  src/uri/fetchers/docker.cpp dbfc1b2f2918ccaf90fa31496a0792f585489397 


Diff: https://reviews.apache.org/r/58778/diff/2/

Changes: https://reviews.apache.org/r/58778/diff/1-2/


Testing
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR
4. JFrog SaaS
5. Local Nexus registry 3.3.1


Thanks,

Chun-Hung Hsiao

Re: Review Request 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

(Updated May 12, 2017, 4:25 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Will rebase on the image-secret patches.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs
-

  src/uri/fetchers/docker.cpp 44169bf5f22f0ffd9fad7bb3b8f7d2a4989c6415 


Diff: https://reviews.apache.org/r/58778/diff/1/


Testing
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR
4. JFrog SaaS
5. Local Nexus registry 3.3.1


Thanks,

Chun-Hung Hsiao

Re: Review Request 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

(Updated May 1, 2017, 11:47 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

More manual tests.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs
-

  src/uri/fetchers/docker.cpp 44169bf5f22f0ffd9fad7bb3b8f7d2a4989c6415 


Diff: https://reviews.apache.org/r/58778/diff/1/


Testing (updated)
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR
4. JFrog SaaS
5. Local Nexus registry 3.3.1


Thanks,

Chun-Hung Hsiao

Re: Review Request 58778: Supported GCE container registry.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58753, 58725, 58778]

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

- Mesos Reviewbot


On May 1, 2017, 7:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 1, 2017, 7:24 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 44169bf5f22f0ffd9fad7bb3b8f7d2a4989c6415 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>