plexsystems / sinker

A tool to sync images from one container registry to another
MIT License
609 stars 53 forks source link

add support for socketless image copy #63

Closed hassenius closed 2 years ago

hassenius commented 2 years ago

This proposal adds support for github.com/containers/image/v5 (addesses #47) which enables copying of images from source to target without requiring access to docker socket.

This is necessary for us to enable use of sinker in a containerised pipeline without docker-in-docker enabled.

I also started an outline of how to address #62 via boolean --all-variants and --override-os/--override-arch

It currently works (without docker running on a mac)

make build
./sinker -m imagetest.yaml push      
INFO[0000] Finding images that need to be pushed ...    
Error: push: image exists: get all images: list images: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Usage:
<snip>

./sinker -m imagetest.yaml copy
INFO[0000] Finding images that need to be copied ...    
INFO[0001] Copying image quay.io/coreos/prometheus-operator:v0.41.1 to docker-<snip>/containerimagetests/coreos/prometheus-operator:v0.41.1 
Error: copy: copy image: choosing an image from manifest list docker://quay.io/coreos/prometheus-operator:v0.41.1: no image found in manifest list for architecture amd64, variant "", OS darwin
Usage:
  sinker copy [flags]

./sinker -m imagetest.yaml copy --override-os=linux 
INFO[0000] Finding images that need to be copied ...    
INFO[0001] Copying image quay.io/coreos/prometheus-operator:v0.41.1 to docker-<snip>/containerimagetests/coreos/prometheus-operator:v0.41.1 
INFO[0021] All images have been copied!

I would like as a next step add the option to direct this, as a cleaner part of command line and image manifest.

For example could be something like

target:
  host: mycompany.com
  repository: myrepo
sources:
- repository: coreos/prometheus-operator
  host: quay.io
  tag: v0.40.0
  architecture: amd64
  os: linux
- repository: coreos/prometheus-operator
  host: quay.io
  tag: v0.40.0
  variants: all

Or something like that..

Please let me know your thoughts, and I'll should be able to wrap it up quite quickly...

jpreese commented 2 years ago

Thanks for your work on this @hassenius ❤️ , I think all of this makes sense. Running without the Daemon has been something desired for quite some time.

hassenius commented 2 years ago

@jpreese how do you feel about the separate copy command, and the new switches "force", "override-arch", "override-os", "all-variants"?

jpreese commented 2 years ago

@jpreese how do you feel about the separate copy command, and the new switches "force", "override-arch", "override-os", "all-variants"?

How would copy differ from sync?

hassenius commented 2 years ago

You mean to name the command sync instead of copy?

jpreese commented 2 years ago

Yeah, instead of introducing a new command, just have the current sync command use your work.

It just wasn't clear to me if you're also proposing behavioral differences.

hassenius commented 2 years ago

Not sure which sync command you refer to @jpreese ?

Available Commands:
  check       Check for newer images
  completion  Generate the autocompletion script for the specified shell
  create      Create a new a manifest
  help        Help about any command
  list        List the images found in the manifest
  pull        Pull the images in the manifest
  push        Push the images in the manifest to the target repository
  update      Update an existing manifest
  version     The version of sinker

I think the new command is quite fundamentally different from from the pull/push implementations, since it doesn't use the image store managed by the docker daemon, so I think it makes sense to keep it separate from the current push/pull commands.

jpreese commented 2 years ago

I agree that the mechanics differ quite a bit between the current commands and the only you're proposing, but the semantics feel the same. That is, whatever is defined in manifest.yaml gets added to the remote registry. The need to pull down the image to the daemon and then push is just a technical limitation of the current implementation.

You're deeper into the woods than I am on this, so if you feel they are different enough to warrant more commands and it fits your flow better--I am for it. Just wanted to make sure we've considered everything.

hassenius commented 2 years ago

So, this is leveraging what's implemented in https://github.com/containers/image/blob/main/copy/copy.go, and the code is mainly just setting up what's needed for that. I think the main difference being that the focus on the implementation in copy.go is to ensure an identical copy, including manifest digests, in pull/push it's not. That's a subtle difference that can be a breaking change for some folks.

I wouldn't be too keen on pulling apart the implementation in copy.go to fit it into existing pull/push commands, but I guess it would be possible to replace the existing pull/push with just this implementation as push (probably removing pull since it doesn't make much sense anymore).

I think from the intended usage pattern of sinker this would be fine, but it could be breaking if anyone does anything outside that, like for example pull, modify, push (i.e. add labels/annotations, whatever) -- which I think technically would be supported with the current implementation?

hassenius commented 2 years ago

Anything further needed from me @jpreese ?

jpreese commented 2 years ago

Shouldn't be, @hassenius! I'm at KubeCon EU at the moment so activity is spotty. But I will take a look at the PR as soon as I can ❤️

hassenius commented 2 years ago

Shouldn't be, @hassenius! I'm at KubeCon EU at the moment so activity is spotty. But I will take a look at the PR as soon as I can ❤️

Me too, we should meet for a coffee

hassenius commented 2 years ago

Shouldn't be, @hassenius! I'm at KubeCon EU at the moment so activity is spotty. But I will take a look at the PR as soon as I can ❤️

Me too, we should meet for a coffee

jpreese commented 2 years ago

Spoke at KubeCon -- this is already being used in production, so has been battle tested. Code itself looks OK to me. Will do a couple cleanups in a followup PR.