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

Add registry mirror support for pack build #1210

Merged
merged 3 commits into from
Jul 11, 2021

Conversation

aemengo
Copy link
Contributor

@aemengo aemengo commented Jun 16, 2021

Summary

Add support for a registry mirroring during pack build command. The use-case being enterprise environments where public registry access is denied.

If pack's config.toml is configured like:

[registry-mirrors]
  "index.docker.io" = "nexus.mycompany.local/mirror"

Then requests from registriespaketobuildpacks/builder will be turned into nexus.mycompany.local/mirror/paketobuildpacks/builder

This PR also adds the config subcommands to manipulate these entries to config.toml.

Output

Before

$ pack build aemengo/hello
full: Pulling from paketobuildpacks/builder
Digest: sha256:2917564c2de52debd1df70d11c014b2708c8974e6e853804958825be84cfb859
Status: Image is up to date for paketobuildpacks/builder:full
...

After

$ pack build aemengo/hello
full: Pulling from mirror/paketobuildpacks/builder
Digest: sha256:2917564c2de52debd1df70d11c014b2708c8974e6e853804958825be84cfb859
Status: Image is up to date for nexus.mycompany.local/mirror/dockerhub-mirror/paketobuildpacks/builder:full
...

Caveats

  • The user entered image-tag is not modified. Not sure if this is desired behavior or not. 🤷🏾‍♂️

Documentation

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

Related

Resolves #821

@aemengo aemengo requested a review from a team as a code owner June 16, 2021 21:27
@aemengo aemengo marked this pull request as draft June 16, 2021 21:27
@github-actions github-actions bot added this to the 0.20.0 milestone Jun 16, 2021
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jun 16, 2021
@aemengo aemengo changed the title wip Add registry mirror support for pack build Jun 16, 2021
@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #1210 (95ed976) into main (9df4e6b) will increase coverage by 0.05%.
The diff coverage is 84.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1210      +/-   ##
==========================================
+ Coverage   80.96%   81.00%   +0.05%     
==========================================
  Files         136      138       +2     
  Lines        8316     8435     +119     
==========================================
+ Hits         6732     6832     +100     
- Misses       1158     1172      +14     
- Partials      426      431       +5     
Flag Coverage Δ
os_linux 80.67% <87.78%> (+0.18%) ⬆️
os_macos 78.13% <83.34%> (+0.10%) ⬆️
os_windows 80.92% <84.56%> (+0.05%) ⬆️
unit 80.67% <87.78%> (+0.10%) ⬆️

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

@aemengo aemengo force-pushed the registry-mirror-support branch 4 times, most recently from bccf755 to 7d38c24 Compare June 24, 2021 18:44
Comment on lines 60 to 69
name, err := pname.TranslateRegistry(name, f.registryMirrors)
if err != nil {
return nil, err
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just acknowledging that this line isn't tested. 😕

@aemengo aemengo marked this pull request as ready for review June 24, 2021 19:52
@jromero
Copy link
Member

jromero commented Jun 29, 2021

The implemented proxy per host or base url is interesting.

[registry-mirrors]
  "index.docker.io" = "nexus.mycompany.local/mirror"
  "gcr.io/some/path" = "nexus.mycompany.local/mirror"

I'm not sure where that requirement came from (I see it in the example config comment here) but mirrors are more commonly applied globally to any and all dependencies. I'd want to validate that this feature request does need a per host/base url type configuration.

  1. If it does, we'll need a wildcard proxy so it works across any and all images.

    [registry-mirrors]
      "*" = "nexus.mycompany.local/mirror"
  2. If there isn't a current requirement for per host/base url, I would remove it to reduce complexity.

Reference:


EDIT: Based on my comment and additional research I've been doing I think it's worth having the per host/base url mirrors BUT we should implement the wildcard option.

@aemengo
Copy link
Contributor Author

aemengo commented Jun 29, 2021

@jromero This flexibility for per host/base url came from my experience using harbor as a proxy cache.

At the very least, clients using the proxy cache feature of Harbor cannot set a global mirror.

@jromero
Copy link
Member

jromero commented Jun 29, 2021

TL;DR: we want both per base urls and wildcard (any).

That makes sense although it does appear to be a harbor specific limitation.

FWIW, the global mirror I was referring to is declared at the client side (pack in this case). It basically says, "for any image, I want you to go look it up at this registry" (ie. Harbor or Artifactory), then it's up to configuration in the registry to look up that image in configured remote (delegated) registries. In Artifactory's case you can setup multiple registries it can delegate/proxy so as a client you want "any" support so that it's up to the Artifactory to do the look up without having to manually map all base urls at the client side.

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 is really super for my current use case, where I want to ensure all of our requests are proxied to Artifactory. Really happy to see this work @aemengo; superb as always.

One very confusing aspect of it for me, in UA, was the fetcher doesn't immediately say that it's pulling from the mirror. I cancelled it a number of times, because I didn't think I had set my mirror appropriately, until I got to the Status line and saw that it did work appropriately. If you could make the image mirror explicit from the beginning, that would make the UX for me significantly better.

$ out/pack build test-go -B paketobuildpacks/builder:tiny -p ../paketo-samples/go/mod
tiny: Pulling from paketobuildpacks/builder
b734f5039f3d: Pull complete
...
Digest: sha256:e6e5fcd7c7094ed28f45b98e1528303f0f3304696e6bd737537c838cdd8f463f
Status: Downloaded newer image for <Artifactory-mirror>/paketobuildpacks/builder:tiny
tiny-cnb: Pulling from paketobuildpacks/run
4fb75c639a6b: Pull complete
...
Digest: sha256:b643257f93f44aa4e77c1c746fea14f419e29a2123829c7a228bc7f1b633fe2a
Status: Downloaded newer image for <Artifactory Mirror>/paketobuildpacks/run:tiny-cnb
...

If we could change the Pulling from line to also reflect the registries mirror, that would be a slam-dunk from my end

internal/commands/config_registry_mirrors.go Outdated Show resolved Hide resolved
internal/commands/config_registry_mirrors.go Outdated Show resolved Hide resolved
internal/commands/config_registry_mirrors.go Outdated Show resolved Hide resolved
@dfreilich dfreilich mentioned this pull request Jul 4, 2021
2 tasks
Also adds a new config subcommand for registry-mirrors

Signed-off-by: Anthony Emengo <[email protected]>
* Support wildcard registry mirror
* Add registry-mirror alias

Signed-off-by: Anthony Emengo <[email protected]>
@aemengo
Copy link
Contributor Author

aemengo commented Jul 6, 2021

@dfreilich

If we could change the Pulling from line to also reflect the registries mirror, that would be a slam-dunk from my end

It looks like we're not writing that line, it's being feed to us by the docker API. I don't know how you still feel about replacing it. To me, it's a bit unsavory.

Maybe a big yellow text WARNING: using mirror <Artifactory-mirror> is better?


EDIT: see 95ed9769

$ pack build
Using mirror <MY-MIRROR>/dockerhub-mirror/paketobuildpacks/builder:full for index.docker.io/paketobuildpacks/builder:full
full: Pulling from dockerhub-mirror/paketobuildpacks/builder
Digest: sha256:7190dbb8177139fbfcdf9445f03420aaaa94832d8b3084e1a34870b11cf0d102
Status: Image is up to date for <MY-MIRROR>/dockerhub-mirror/paketobuildpacks/builder:full
Using mirror <MY-MIRROR>/dockerhub-mirror/paketobuildpacks/run:full-cnb for index.docker.io/paketobuildpacks/run:full-cnb
full-cnb: Pulling from dockerhub-mirror/paketobuildpacks/run
Digest: sha256:65972587451d7a0b6a375f9e39573c4272add20ce1c6a82c7d195e9e915e2796
Status: Image is up to date for <MY-MIRROR>/dockerhub-mirror/paketobuildpacks/run:full-cnb
Using mirror <MY-MIRROR>/dockerhub-mirror/paketobuildpacks/run:full-cnb for index.docker.io/paketobuildpacks/run:full-cnb
===> DETECTING
3 of 6 buildpacks participating
paketo-buildpacks/ca-certificates 2.3.2
paketo-buildpacks/go-dist         0.5.1
paketo-buildpacks/go-build        0.3.4
...

Signed-off-by: Anthony Emengo <[email protected]>
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.

Beautiful work. Thanks!

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.

Support typical enterprise proxy and internal mirrors setup
3 participants