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 support for socketless image copy #63

Merged
merged 5 commits into from
May 20, 2022
Merged

Conversation

hassenius
Copy link
Contributor

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
Copy link
Collaborator

jpreese commented Apr 6, 2022

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.

# SINKER_VERSION is set during the release process
ARG SINKER_VERSION=0.0.0
RUN go build -ldflags="-X 'github.com/plexsystems/sinker/internal/commands.sinkerVersion=${SINKER_VERSION}'"
RUN go build -tags 'containers_image_openpgp' -ldflags="-X 'github.com/plexsystems/sinker/internal/commands.sinkerVersion=${SINKER_VERSION}'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpreese fyi, this will affect the build pipeline too.
As per https://github.com/containers/image/blob/main/README.md#supported-build-tags

Use a Golang-only OpenPGP implementation for signature verification instead of the default cgo/gpgme-based implementation; the primary downside is that creating new signatures with the Golang-only implementation is not supported.

Which should not affect us, as I don't think generating signatures would be outside the problem domain sinker addresses.

@hassenius
Copy link
Contributor Author

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

@jpreese
Copy link
Collaborator

jpreese commented Apr 7, 2022

@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
Copy link
Contributor Author

You mean to name the command sync instead of copy?

@jpreese
Copy link
Collaborator

jpreese commented Apr 14, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

jpreese commented Apr 15, 2022

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
Copy link
Contributor Author

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 hassenius changed the title WIP: add support for socketless image copy add support for socketless image copy Apr 26, 2022
@hassenius
Copy link
Contributor Author

Anything further needed from me @jpreese ?

@jpreese
Copy link
Collaborator

jpreese commented May 17, 2022

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
Copy link
Contributor Author

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

1 similar comment
@hassenius
Copy link
Contributor Author

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 jpreese merged commit 31de589 into plexsystems:main May 20, 2022
@jpreese
Copy link
Collaborator

jpreese commented May 20, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants