-
Notifications
You must be signed in to change notification settings - Fork 527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid trying https for insecure registries #1002
Conversation
…nsecure Signed-off-by: pangshaoqiang <pangsq9413@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #1002 +/- ##
==========================================
- Coverage 75.18% 75.04% -0.14%
==========================================
Files 107 107
Lines 5061 5077 +16
==========================================
+ Hits 3805 3810 +5
- Misses 711 721 +10
- Partials 545 546 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
pkg/v1/remote/transport/ping.go
Outdated
// if the registry matches our localhost heuristic. | ||
var schemes []string | ||
if reg.IsInsecure() { | ||
schemes = []string{"http"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me thinks we should fall back to https in this case, still, just to avoid breaking something that worked before, but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I list all conditions below to figure out how this patch will make a difference to the usage.
No. | Registry Protocols | Insecure Flag | Match localhost pattern | Protocol Used Before | Protocol Used After |
---|---|---|---|---|---|
1 | HTTPS+HTTP | ✖ | ✖ | HTTPS | HTTPS |
2 | HTTPS+HTTP | ✖ | ✔ | HTTPS | HTTPS |
3 | HTTPS+HTTP | ✔ | ✖ | HTTPS | HTTP |
4 | HTTPS+HTTP | ✔ | ✔ | HTTPS | HTTP |
5 | HTTPS-only | ✖ | ✖ | HTTPS | HTTPS |
6 | HTTPS-only | ✖ | ✔ | HTTPS | HTTPS |
7 | HTTPS-only | ✔ | ✖ | HTTPS | failed |
8 | HTTPS-only | ✔ | ✔ | HTTPS | failed |
9 | HTTP-only | ✖ | ✖ | failed | failed |
10 | HTTP-only | ✖ | ✔ | HTTP | HTTP |
11 | HTTP-only | ✔ | ✖ | HTTP | HTTP |
12 | HTTP-only | ✔ | ✔ | HTTP | HTTP |
The conditions on which performances will be different are No.7/No.8. On these conditions, users just need to unset the insecure flag to make it work. Considering that the insecure flag is default False
, these conditions may not happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the insecure flag is default False, these conditions may not happen.
My concern is that this option might be set "globally" for a given consumer. If you look at kaniko, they have a bunch of flags that are booleans to enable insecure behavior.
You can imagine someone with a Dockerfile that pulls FROM two different registries: their secure production registry, and an insecure registry running on their network.
If we don't have a fallback to https, pulling from their secure registry would just fail.
Arguably, any decent web server will redirect from http to https, so this might not be a problem in practice, but I wanted to bring it up as a potentially breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agreed that maybe some kaniko users enbale the insecure
flag and have secure/insecure registries in one Dockerfile. On that condition, the pulling from thesecure registries would fail. However, in kaniko, users can also use the insecure-registry
to config which registries are insecure and the others will still use HTTPS for pulling and pushing.
In my opinion, it is determined that if users set insecure
, then we will try HTTP
at first. And whether or not we fallback to HTTPS is another consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fallback to HTTPS after HTTP is failed
is reasonable for this issue.
Signed-off-by: pangshaoqiang <pangsq9413@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This reverts commit 9e56ddd.
Imporve the fallback logic in
ping
. Not tryhttps
if the registry is intentionally set to insecure so that we can avoid long delays on http-only registry (which may directly drop the packet to port 443 rather than answer with a RST).Fixes GoogleContainerTools/kaniko#970
It still keep #279 solved.