Skip to content
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

Delegate checking of remote image existence to the daemon #1145

Merged

Conversation

aemengo
Copy link
Contributor

@aemengo aemengo commented Apr 26, 2021

Summary

When Docker Daemon is available, defer to Docker Daemon when pulling remote images.
The motivation is that the Docker Daemon already handles more edge cases that we do.

The biggest difference to the end user is the error message bubbled from the Daemon, in the case of config.PullPolicy == Always

Output

Before

./pack builder create my-builder --config config.toml
ERROR: invalid run image config: failed to fetch image: connect to repo store 'mdavis-l:5000/cnbs/sample-stack-run:bionic': Get "https://mdavis-l:5000/v2/": http: server gave HTTP response to HTTPS client

After

./pack builder create my-builder --config config.toml
Trying to pull repository mdavis-l:5000/cnbs/sample-stack-run ... 
bionic: Pulling from mdavis-l:5000/cnbs/sample-stack-run
Digest: sha256:0e6d2966062c26f0a0660c89c5bd1dba7e1fa019e6d68ef5c3694eafde1ab805
...

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #771

@aemengo aemengo requested a review from a team as a code owner April 26, 2021 18:15
@github-actions github-actions bot added this to the 0.19.0 milestone Apr 26, 2021
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Apr 26, 2021
@aemengo aemengo force-pushed the fix-unable-to-pull-insecure-registry branch 2 times, most recently from f7628bf to 0c911b7 Compare April 27, 2021 21:11
@jromero
Copy link
Member

jromero commented May 10, 2021

@aemengo this looks like a great fix. Any progress on getting the tests to pass?

@jromero jromero changed the title Modify fetcher Delegate checking of remote image existence to the daemon May 10, 2021
@aemengo aemengo force-pushed the fix-unable-to-pull-insecure-registry branch 2 times, most recently from f62f64e to 6313d36 Compare May 10, 2021 22:16
@github-actions github-actions bot added the type/chore Issue that requests non-user facing changes. label May 10, 2021
When Docker Daemon is available, defer to Docker Daemon when pulling remote images.
The motivation is that the Docker Daemon already handles more edge cases that we do.

Signed-off-by: Anthony Emengo <aemengo@vmware.com>
@aemengo aemengo force-pushed the fix-unable-to-pull-insecure-registry branch from 6313d36 to cf1156c Compare May 11, 2021 14:36
@github-actions github-actions bot removed the type/chore Issue that requests non-user facing changes. label May 11, 2021
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #1145 (43cc52b) into main (216a4d1) will increase coverage by 1.13%.
The diff coverage is 89.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
+ Coverage   79.74%   80.86%   +1.13%     
==========================================
  Files         136      136              
  Lines        8294     8296       +2     
==========================================
+ Hits         6613     6708      +95     
+ Misses       1254     1160      -94     
- Partials      427      428       +1     
Flag Coverage Δ
os_linux 80.41% <87.50%> (-0.02%) ⬇️
os_macos 77.96% <0.00%> (-0.01%) ⬇️
os_windows 80.78% <89.29%> (+1.13%) ⬆️
unit 80.46% <87.50%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@aemengo aemengo force-pushed the fix-unable-to-pull-insecure-registry branch from cf1156c to 0423545 Compare May 11, 2021 16:34
@aemengo
Copy link
Contributor Author

aemengo commented May 11, 2021

@jromero @natalieparellano I'm really sorry for the delay on this. The PR should now be ready to look at.

Copy link
Contributor

@dwillist dwillist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome 💯

Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic! Two small comments that can be fixed in the future, but overall this is 🏅 for simplifying the code and solving the bug in one.

Comment on lines +10 to 14
"github.com/buildpacks/imgutil/remote"

"github.com/buildpacks/imgutil"
"github.com/buildpacks/imgutil/local"
"github.com/buildpacks/imgutil/remote"
"github.com/buildpacks/lifecycle/auth"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Javier mentioned #1187 (comment), there should be 3 import groups, I think.

Comment on lines +48 to +49
img, err := f.fetchDaemonImage(name)
return img, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
img, err := f.fetchDaemonImage(name)
return img, err
return f.fetchDaemonImage(name)

@dfreilich dfreilich merged commit eb72b8a into buildpacks:main May 19, 2021
@aemengo aemengo deleted the fix-unable-to-pull-insecure-registry branch May 19, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot pull from an insecure registry when referred to by hostname
5 participants