Add dry-run mode to skopeo-sync

[twexler]

I found this useful when testing skopeo-sync, instead of actually copying to the destination, skopeo-sync will output a message like

time="2021-09-24T14:06:25-04:00" level=info msg="Would have copied image, but `--dry` is specified" from="docker://registry-1.docker.io/prom/prometheus:v2.9.0" to="docker://my-cool-docker-repo.local/registry-1.docker.io/prom/prometheus:v2.9.0"

Fixes #1175.

[rhatdan]

@mtrmac @vrothberg PTAL

[TomSweeneyRedHat]

@twexler thanks for bringing this forward. I think this is indeed useful, but I think I'd prefer it if the code went a bit further and touched the registry and perhaps the destination to see if it was available and if the auths were OK. A bigger effort for sure.

[twexler]

@TomSweeneyRedHat Gotcha. I'll take a look at this again tomorrow, see if I can fit that into this PR with minimal headache...

[vrothberg]

@twexler thanks for bringing this forward. I think this is indeed useful, but I think I'd prefer it if the code went a bit further and touched the registry and perhaps the destination to see if it was available and if the auths were OK. A bigger effort for sure.

I am not sure that will work. Let's assume we want to sync an image to registry.com/repo/image:tag. The presence/credential checks would only work if registry.com/repo/image:tag is already present. I do not know of a reliable way to figure out if an image exists or not since most registries will return an auth error if an image doesn't exist:

~ $ podman pull quay.io/example312123/foo:latest
Trying to pull quay.io/example312123/foo:latest...
Error: initializing source docker://quay.io/example312123/foo:latest: reading manifest latest in quay.io/example312123/foo: unauthorized: access to the requested resource is not authorized

So I don't think that such checks should be performed in a dry run.

[twexler]

@vrothberg You could hit the API Version Check endpoint on the registry to confirm it's a registry and potentially authn/z would play into that, but you're right: If that API isn't authenticated on the specific registry implementation, then we're only verifying that the defined repository is probably a registry which is a worthwhile check, I think.

[vrothberg]

In the current form I find the logs too redundant:

INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:ubuntu" to="dir:/tmp/test/busybox:ubuntu"
INFO[0001] Copying image ref 173/179                     from="docker://busybox:ubuntu-12.04" to="dir:/tmp/test/busybox:ubuntu-12.04"
INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:ubuntu-12.04" to="dir:/tmp/test/busybox:ubuntu-12.04"
INFO[0001] Copying image ref 174/179                     from="docker://busybox:ubuntu-14.04" to="dir:/tmp/test/busybox:ubuntu-14.04"
INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:ubuntu-14.04" to="dir:/tmp/test/busybox:ubuntu-14.04"
INFO[0001] Copying image ref 175/179                     from="docker://busybox:uclibc" to="dir:/tmp/test/busybox:uclibc"
INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:uclibc" to="dir:/tmp/test/busybox:uclibc"
INFO[0001] Copying image ref 176/179                     from="docker://busybox:unstable" to="dir:/tmp/test/busybox:unstable"
INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:unstable" to="dir:/tmp/test/busybox:unstable"
INFO[0001] Copying image ref 177/179                     from="docker://busybox:unstable-glibc" to="dir:/tmp/test/busybox:unstable-glibc"
INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:unstable-glibc" to="dir:/tmp/test/busybox:unstable-glibc"
INFO[0001] Copying image ref 178/179                     from="docker://busybox:unstable-musl" to="dir:/tmp/test/busybox:unstable-musl"
INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:unstable-musl" to="dir:/tmp/test/busybox:unstable-musl"
INFO[0001] Copying image ref 179/179                     from="docker://busybox:unstable-uclibc" to="dir:/tmp/test/busybox:unstable-uclibc"

I think that the Copying image ref 173/179 ... logs shouldn't be printed for --dry.

@rhatdan PTAL

[vrothberg]

Other than that, LGTM.

[mtrmac]

In the current form I find the logs too redundant:

INFO[0001] Would have copied image, but `--dry` is specified  from="docker://busybox:unstable-musl" to="dir:/tmp/test/busybox:unstable-musl"
INFO[0001] Copying image ref 179/179                     from="docker://busybox:unstable-uclibc" to="dir:/tmp/test/busybox:unstable-uclibc"

I think that the Copying image ref 173/179 ... logs shouldn't be printed for --dry.

Also I don’t think every line needs to repeat that “--dry is specified”. Maybe just replace the Copying image ref message with Would try copying image ref in the dry-run mode?

[mtrmac]

@vrothberg You could hit the API Version Check endpoint on the registry to confirm it's a registry and potentially authn/z would play into that, but you're right: If that API isn't authenticated on the specific registry implementation, then we're only verifying that the defined repository is probably a registry which is a worthwhile check, I think.

Yeah, I don’t think we currently know how to do this. IIRC on some registries, the “get token for $username+$password accessing $scope” endpoint actually does fail if the credentials are valid but not allowed to use $scope (e.g. write to a specific repo), but on others this succeeds (and accessing the “version check” would work just fine) and only fails when actually trying to do an operation (i.e. actually trying write something to that repo). So we don’t know whether credentials for writing are valid until we actually try to write something, which is very incompatible with a “dry-run” concept.

Having a mode that tests the source enumeration / allows validation of a YAML with regexes and the like, is still quite valuable without testing credentials.

[twexler]

Having a mode that tests the source enumeration / allows validation of a YAML with regexes and the like, is still quite valuable without testing credentials.

This mode does test this, as it compiles the images to copy before any of the new code comes into play.

[mtrmac]

Yup, I just wanted to say that I don’t think not testing credentials is a reason to reject or delay accepting this PR.

[mtrmac]

ACK overall, but I’ll let others to try this out and review in detail, for this week.

[twexler]

I'll squash these commits once code review is complete.

[mtrmac]

@vrothberg @rhatdan opinions on a sync --dry-run now ending with “Synced 0 images from $nonzero sources” per #1459 (comment) ?

(Feel free to tell me it’s not worth worrying about.)

[vrothberg]

@vrothberg @rhatdan opinions on a sync --dry-run now ending with “Synced 0 images from $nonzero sources” per #1459 (comment) ?

I would expect Skopeo to tell me how many images "would" have been synced.

[mtrmac]

Looks good, some last nits. Also, please rebase the PR.

[mtrmac]

Something like

Suggested change

	logMsg := fmt.Sprintf("Synced %d images from %d sources", imagesNumber, len(srcRepoList))
	if opts.dryRun {
	logMsg = fmt.Sprintf("Would have %s", logMsg)
	}
	if !opts.dryRun {
	logrus.Infof("Synced %d images from %d sources", imagesNumber, len(srcRepoList))
	} else {
	logrus.Infof("Would have synced %d images from %d sources", imagesNumber, len(srcRepoList))
	}

so that the capitalization is correct, please.

[mtrmac]

Please add the flag to completions/bash/skopeo as well.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[jackivanov]

Any progress on this?

[mfriedenhagen]

Hi @twexler, it would be nice if you could rebase and integrate the remarks :-)

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[mtrmac]

Being continued in #1608, for the record.