Add debugging for manifests when uploading to destinations.

[robbmanes]

Add optional logging for printing outgoing manifests. This can greatly help in debugging anything that might alter a manifest either on wire or in the use case I just had, verifying a problem in local storage (verifying the outgoing manifests without having to watch pure HTTP calls, etc).

[mtrmac]

Thanks!

There already is

image/docker/docker_image_dest.go

Line 522 in 8d501c7

err := fmt.Errorf("uploading manifest %s to %s: %w", tagOrDigest, d.ref.ref.Name(), rawErr)

, on failure. Is that not sufficient?

On success, it should be possible to read the uploaded manifest from the registry — due to the existence of digest references, the registry does not really have the option to modify the uploaded data.

(My basic concern is that this is fairly noisy… and potentially we might worry about escaping the data, e.g. if it included newlines.)

[Luap99]

One option could be to use the trace level not debug to log this message. We don't make a lot of use of that in podman but I used that myself to trigger trace logging in netavark which dumps netlink message which was useful to me a few times. (podman --log-level trace ...)
That said it looks like skopeo only offers the debug level and not a per level selection like podman or buildah so if c/image starts logging with trace level skopeo would need to support that as well.

If escaping is a concern it could log with %q instead of %s.

[robbmanes]

In our case here, the specific scenario we were debugging was not a failure scenario; skopeo/podman was uploading layers with a negative size value (-1), which is a different issue altogether I'm filing shortly, but because it didn't fail to upload we couldn't prove sans HTTP dump of the payload what the manifest coming from the client was, or if any of the myriad of proxies/data inspection security devices were interfering with the data on-wire (TLS/SSL termination and data inspection network device which ultimately led to being exonerated by this debug line, as we could see clearly the manifest being pushed was wrong).

And as @Luap99 mentioned (and was exactly right about), this was with skopeo so I initially did Tracef but quickly found out skopeo didn't support anything but Debugf, so I did that instead. I agree it should be trace-level data if we updated skopeo to include such a thing though.

Also agreed with escaping the data if we prefer; I originally did %+v but it just wasn't necessary, and strings worked, but I'm perfectly happy to squash in a %q if you prefer. Thanks!

[robbmanes]

Cast as string and formatting with %q is squashed in.

[robbmanes]

This really should be Trace, it's very verbose but still greatly helpful. Tried my hand at adding --log-level to skopeo in containers/skopeo#2514 if we want to wait on that, and then I can squash this into a Tracef.

[mtrmac]

Good idea, using the trace level makes sense to me. (Also #201 proposes to use trace level, so that would be consistent.)

[mtrmac]

I think this would be better handled by logging before the PutManifest call in copyMultipleImages; that should record the same data, and avoid logging the same manifest twice in single-architecture copy situations.

[jankaluza]

Hi, and thank you for your contribution!

We’ve recently migrated this repository into a new monorepo: containers/container-libs along with other repositories

As part of this migration, this repository is no longer accepting new Pull-Requests and therefore this Pull-Request is being closed.

Thank you very much for your contribution. We would appreciate your continued help in migrating this PR to the new container-libs repository. Please let us know if you are facing any issues.

You can read more about the migration and the reasoning behind it in our blog post: Upcoming migration of three containers repositories to monorepo.

Thanks again for your work and for supporting the containers ecosystem!