search

buildpacks / imgutil

Helpful utilities for working with images
Apache License 2.0
24 stars 41 forks source link

Add insecureSkipVerify in other missing functions #219

Closed dlion closed 11 months ago

dlion commented 11 months ago

@jabrown85 has spotted some points in the imgutil library where the insecureSkipVerify flag should be necessary.

This PR is addressing that.

Thank you @jabrown85!

dlion commented 11 months ago

So just to be aligned, shall we add remote.WithRegistrySetting("myotherregistry.com", true) loops (as we have done in lifecycle) in those functions as well? Did I understand correctly?

jabrown85 commented 11 months ago

From the outside I would expect to be able to do this and have the remote.WithTransport use the correct reg.insecure value. 

    remoteImage, err := remote.NewImage("myregistry.domain.com/repo/app-image",
        authn.DefaultKeychain,
        remote.FromBaseImage("myother-insecure-registry.com/repo/superbase"),
        remote.WithRegistrySetting("myregistry.domain.com", true),
        remote.WithRegistrySetting("myother-insecure-registry.com", true),
    )

I see map[string]registrySetting already in getRegistry - If we update remote.WithRegistrySetting to not override the entire map it may just work. Everything reads ok to me.

jabrown85 commented 11 months ago

@natalieparellano I will leave for you to review as well.

dlion commented 11 months ago

@jabrown85 now I got it! Thanks for clarifying. Yes you are right, the actual implementation reset the map every time, preventing having multiple registries, and the test we currently have test just one registry. 😅 I'll update this PR fixing the map initialization and extending the test with the check for multiple registries.

Thank you again for spotting it 😁