You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2020/02/03 17:54:46 UTC
Review Request 72079: Added workaround for Docker repositories not
providing scope/service.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72079/
-----------------------------------------------------------
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10092
https://issues.apache.org/jira/browse/MESOS-10092
Repository: mesos
Description
-------
This patch adds a fallback Docker authorization server URI generation
mechanism (see MESOS-10092) for repository servers that provide no
"scope"/"service" params in the "WWW-Authenticate" header of the initial
"401 Unathorized" response.
Diffs
-----
src/uri/fetchers/docker.cpp 75df80bfd59323e06bf563d15a98af244b5b1874
Diff: https://reviews.apache.org/r/72079/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72079: Added workaround for Docker repositories not
providing scope/service.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72079/#review219481
-----------------------------------------------------------
src/uri/fetchers/docker.cpp
Lines 644-646 (patched)
<https://reviews.apache.org/r/72079/#comment307647>
This static method should be moved to the beginning of this file.
src/uri/fetchers/docker.cpp
Line 751 (original), 757 (patched)
<https://reviews.apache.org/r/72079/#comment307648>
Do we really this local variable?
src/uri/fetchers/docker.cpp
Line 969 (original), 976 (patched)
<https://reviews.apache.org/r/72079/#comment307649>
Ditto.
src/uri/fetchers/docker.cpp
Lines 1081-1082 (patched)
<https://reviews.apache.org/r/72079/#comment307650>
Should this be a static method?
src/uri/fetchers/docker.cpp
Lines 1081-1083 (patched)
<https://reviews.apache.org/r/72079/#comment307651>
Since this method will be called with both initial response and root response, I would suggest to just name its parameter as `response` rather than `initialResponse`.
src/uri/fetchers/docker.cpp
Lines 1122 (patched)
<https://reviews.apache.org/r/72079/#comment307659>
If the error is from the line 1089, then here the warning message will be something like:
```
Failed to get WWW-Authenticate header: <detailed error message> from <initialUri>
```
This is a bit odd, it's better to be something like:
```
Failed to get WWW-Authenticate header from <initialUri>: <detailed error message>
```
So maybe we should pass the `initialUri` into `getBearerAuthParam` as another parameter and include it in the error message there?
src/uri/fetchers/docker.cpp
Lines 1145 (patched)
<https://reviews.apache.org/r/72079/#comment307652>
A space between `>` and `{`.
src/uri/fetchers/docker.cpp
Lines 1183 (patched)
<https://reviews.apache.org/r/72079/#comment307653>
Too many spaces for the indent, 4 spaces should be enough.
src/uri/fetchers/docker.cpp
Line 1140 (original), 1186-1187 (patched)
<https://reviews.apache.org/r/72079/#comment307654>
Better to merge these two lines into a single line.
src/uri/fetchers/docker.cpp
Line 1142 (original), 1189 (patched)
<https://reviews.apache.org/r/72079/#comment307656>
Too many spaces for the indent, we need two less.
src/uri/fetchers/docker.cpp
Lines 1144-1145 (original), 1191-1194 (patched)
<https://reviews.apache.org/r/72079/#comment307657>
Why changing these from two lines to four lines?
src/uri/fetchers/docker.cpp
Line 1152 (original), 1201-1202 (patched)
<https://reviews.apache.org/r/72079/#comment307658>
Ditto.
- Qian Zhang
On Feb. 4, 2020, 1:54 a.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72079/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2020, 1:54 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10092
> https://issues.apache.org/jira/browse/MESOS-10092
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a fallback Docker authorization server URI generation
> mechanism (see MESOS-10092) for repository servers that provide no
> "scope"/"service" params in the "WWW-Authenticate" header of the initial
> "401 Unathorized" response.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 75df80bfd59323e06bf563d15a98af244b5b1874
>
>
> Diff: https://reviews.apache.org/r/72079/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72079: Added workaround for Docker repositories not
providing scope/service.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72079/#review219502
-----------------------------------------------------------
Fix it, then Ship it!
src/uri/fetchers/docker.hpp
Lines 52 (patched)
<https://reviews.apache.org/r/72079/#comment307700>
Can we do `bool enableAuthServiceUriFallback = true` instead? Double negative seems a bit odd to me.
src/uri/fetchers/docker.cpp
Line 488 (original), 500 (patched)
<https://reviews.apache.org/r/72079/#comment307701>
Need a space behind `from`.
src/uri/fetchers/docker.cpp
Lines 1151-1152 (patched)
<https://reviews.apache.org/r/72079/#comment307702>
I think we do not need `from initialUri` which should already be included in `authParam.error()`.
- Qian Zhang
On Feb. 4, 2020, 9:24 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72079/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2020, 9:24 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10092
> https://issues.apache.org/jira/browse/MESOS-10092
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a fallback Docker authorization server URI generation
> mechanism (see MESOS-10092) for repository servers that provide no
> "scope"/"service" params in the "WWW-Authenticate" header of the initial
> "401 Unathorized" response.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1
> src/uri/fetchers/docker.cpp 75df80bfd59323e06bf563d15a98af244b5b1874
>
>
> Diff: https://reviews.apache.org/r/72079/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72079: Added workaround for Docker repositories not
providing scope/service.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72079/
-----------------------------------------------------------
(Updated Feb. 4, 2020, 1:24 p.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10092
https://issues.apache.org/jira/browse/MESOS-10092
Repository: mesos
Description
-------
This patch adds a fallback Docker authorization server URI generation
mechanism (see MESOS-10092) for repository servers that provide no
"scope"/"service" params in the "WWW-Authenticate" header of the initial
"401 Unathorized" response.
Diffs (updated)
-----
src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1
src/uri/fetchers/docker.cpp 75df80bfd59323e06bf563d15a98af244b5b1874
Diff: https://reviews.apache.org/r/72079/diff/3/
Changes: https://reviews.apache.org/r/72079/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72079: Added workaround for Docker repositories not
providing scope/service.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72079/
-----------------------------------------------------------
(Updated Feb. 4, 2020, 12:05 p.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Changes
-------
Addressed issues.
Added a flag for disabling the fallback in tests (to be able to make sure that the registry used for testing purposes triggers the fallback logic).
Bugs: MESOS-10092
https://issues.apache.org/jira/browse/MESOS-10092
Repository: mesos
Description
-------
This patch adds a fallback Docker authorization server URI generation
mechanism (see MESOS-10092) for repository servers that provide no
"scope"/"service" params in the "WWW-Authenticate" header of the initial
"401 Unathorized" response.
Diffs (updated)
-----
src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1
src/uri/fetchers/docker.cpp 75df80bfd59323e06bf563d15a98af244b5b1874
Diff: https://reviews.apache.org/r/72079/diff/2/
Changes: https://reviews.apache.org/r/72079/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko