Add support for pushing/pulling to insecure registries#1870
Add support for pushing/pulling to insecure registries#1870balopat merged 4 commits intoGoogleContainerTools:masterfrom
Conversation
nkubala
requested review from
balopat,
dgageot,
priyawadhwa and
r2d4
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1870 +/- ##
==========================================
+ Coverage 52.08% 52.21% +0.12%
==========================================
Files 179 180 +1
Lines 7921 7956 +35
==========================================
+ Hits 4126 4154 +28
Misses 3413 3413
- Partials 382 389 +7
Continue to review full report at Codecov.
|
balopat
left a comment
balopat
left a comment
There was a problem hiding this comment.
I have one problem with this - insecureRegistries is passed into a lot of places, impacting the API of a lot of objects - I think we should sit down and see if we can create a design where less methods are impacted - maybe wire it through other objects.
| originalDockerClient := newDockerCilent | ||
| newDockerCilent = func() (docker.LocalDaemon, error) { | ||
| return docker.NewLocalDaemon(test.api, nil), nil | ||
| originalDockerClient := newDockerClient |
There was a problem hiding this comment.
you can make this simpler by
- Calling the defer function using the original stored value and then setting the value
defer func( f func (map[string]bool) (docker.LocalDaemon, error)) { newDockerClinet = f }(newDockerClient)
newDockerClient = func (map[string]bool) (docker.LocalDaemon, error) {
return docker.NewLocalDaemon(test.api, nil, test.insecureRegistries), nil
}
That ways, the originalDockerClient value is already captured in the function call.
Along the lines that @dgageot suggested here
| if isInsecure(ref.Context().Registry.Name(), insecureRegistries) { | ||
| ref, err = getInsecureRegistryImpl(identifier) | ||
| if err != nil { | ||
| logrus.Warnf("error getting insecure registry: %s\nremote references may not be retrieved", err.Error()) |
There was a problem hiding this comment.
Sorry, i am not aware of how the go container registry works.
In case we see an error when parsing an insecure registry, we still try to fetch the image? Shd we not fail instead of warning?
Or is there a case where you can still retrieve the insecure image.
#failFast.
There was a problem hiding this comment.
mmm yeah it's unlikely that retrieving the image will work if this fails, but I don't see the harm in trying. remote.Image should be a pretty inexpensive call, and if that fails skaffold will fail.
| shouldErr: true, | ||
| }, | ||
| { | ||
| name: "secure image provided in insecure registries list", |
There was a problem hiding this comment.
I think this case is testing if user incorrectly adds a secure registry to insecure list by using a hint "test.insecure".
Right now, we do not validate the insecureRegistry list the code will still call getInsecureRegistryImpl which is correct.
Shd we not test this? or add a todo to ring this test back in when we have some validation?
There was a problem hiding this comment.
not sure I understand what you're saying here. if you're asking about validation of the insecure registry list (as in, each specified registry is actually "insecure"), it's not really possible to validate this without pinging each individual registry before making the remote.Image call. if any registries are passed by the user as "insecure", the list will be populated and getInsecureRegistryImpl will be called (which is part of what this test is testing). does that answer your question?
| return nil, errors.Wrap(err, "parsing initial ref") | ||
| } | ||
|
|
||
| if isInsecure(ref.Context().Registry.Name(), insecureRegistries) { |
There was a problem hiding this comment.
As far as I understand, the insecureRegistries is only required here.
Would an package init which retrieves the insecure registries from the config file be a valid approach? That would have the advantage to not scatter the configuration over large portions of the codebase.
On second thoughts: init is probably a nightmare for testing...
There was a problem hiding this comment.
yeah we're thinking along the lines of creating a RunContext that could be passed down from the runner rather.
|
#1885 was merged - this needs rebase/rewrite to use |
nkubala
force-pushed
the
insecure-registry
branch
from
c704f98 to
17d7bcc
Compare
|
Added some commits as we reviewed it together + opened #1943 for a redesign of the image management. |
6 participants
Fixed version of #1824
Now uses new
name.Insecureoption passed toname.ParseReferenceto ensureHTTPprotocol is used for insecure registries on the go-containerregistry side.