OCPBUGS-50660: use destination client on v1 conversion #1975

ricardomaraschini · 2025-02-13T16:33:19Z

Note for the reviewer of this: please check the comment section in the linked OCPBUGS-50660, there are a lot of things there, it is quite impossible to reproduce everything here in the description

I suspect we always knew that our manifest v2 implementation was somewhat failing (for the oc image append that is). I say that because when we fail to upload a v2 we attempt to convert it back to v1 and push it again.
Here is where we do such a thing (if we fail to push the v2 due to Invalid Manifest).

Here is where things go sideways: Docker Hub simply accepts the v2 we produce but Quay doesn't, so for the latter we gonna attempt the conversion to v1 and that should work but it doesn't.

We didn't know why it fails because we masked the actual error (the actual error is "convertErr" but we return "err"). With a verbose 2 we can see what actually happens: when we try to push the v1 after the conversion we fail with "permission denied". This code has been around for quite a while and we only now see it failing.

These operations of conversion to v1 should go away and we should produce compatible v2 manifests, I guess. In the meantime if we replace the blob service used during the conversion we ensure that we have enough permissions to read the blobs.

NOTE that v1 has longer been deprecated and Docker will stop supporting it on its next version:

$ docker pull quay.io/repo/v1:latest
latest: Pulling from repo/v1:latest
[DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest version 2, schema 1 support is disabled by default and will be removed in an upcoming release. Suggest the author of quay.io/repo/v1:latest to upgrade the image to the OCI Format or Docker Image manifest v2, schema 2. More information at https://docs.docker.com/go/deprecated-image-specs/
$

I suspect we always knew that our manifest v2 implementation was somewhat failing (for the `oc image append` that is). I say that because when we fail to upload a v2 we attempt to convert it back to v1 and push it again. [Here](https://github.com/openshift/oc/blob/master/pkg/cli/image/manifest/manifest.go#L593-L594) is where we do such a thing (if we [fail to push the v2 due to Invalid Manifest](https://github.com/openshift/oc/blob/master/pkg/cli/image/manifest/manifest.go#L580-L583)). Here is where things go sideways: Docker Hub simply accepts the v2 we produce but Quay doesn't, so for the latter we gonna attempt the conversion to v1 and that should work but it doesn't. We didn't know why it fails because [we masked the actual error](https://github.com/openshift/oc/blob/master/pkg/cli/image/manifest/manifest.go#L594-L602) (the actual error is "convertErr" but we return "err"). With a verbose 2 we can see what actually happens: when we try to push the v1 after the conversion we fail with "permission denied". This code has been around for quite a while and we only now see it failing. These operations of conversion to v1 should go away and we should produce compatible v2 manifests, I guess. In the meantime if we replace the blob service used during the conversion we ensure that we have enough permissions to read the blobs. **NOTE** that v1 has longer been deprecated and Docker will stop supporting it on its next version: ``` $ docker pull quay.io/rmarasch/v1:latest latest: Pulling from rmarasch/tests-y [DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest version 2, schema 1 support is disabled by default and will be removed in an upcoming release. Suggest the author of quay.io/rmarasch/tests-y:latest to upgrade the image to the OCI Format or Docker Image manifest v2, schema 2. More information at https://docs.docker.com/go/deprecated-image-specs/ $ ```

openshift-ci-robot · 2025-02-13T16:33:26Z

@ricardomaraschini: This pull request references Jira Issue OCPBUGS-50660, which is invalid:

expected the bug to target the "4.19.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Note for the reviewer of this: please check the comment section in the linked OCPBUGS-50660, there are a lot of things there, it is quite impossible to reproduce everything here in the description

I suspect we always knew that our manifest v2 implementation was somewhat failing (for the oc image append that is). I say that because when we fail to upload a v2 we attempt to convert it back to v1 and push it again.
Here is where we do such a thing (if we fail to push the v2 due to Invalid Manifest).

Here is where things go sideways: Docker Hub simply accepts the v2 we produce but Quay doesn't, so for the latter we gonna attempt the conversion to v1 and that should work but it doesn't.

We didn't know why it fails because we masked the actual error (the actual error is "convertErr" but we return "err"). With a verbose 2 we can see what actually happens: when we try to push the v1 after the conversion we fail with "permission denied". This code has been around for quite a while and we only now see it failing.

These operations of conversion to v1 should go away and we should produce compatible v2 manifests, I guess. In the meantime if we replace the blob service used during the conversion we ensure that we have enough permissions to read the blobs.

NOTE that v1 has longer been deprecated and Docker will stop supporting it on its next version:
$ docker pull quay.io/repo/v1:latest
latest: Pulling from repo/v1:latest
[DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest
version 2, schema 1 support is disabled by default and will be removed
in an upcoming release. Suggest the author of
quay.io/repo/v1:latest to upgrade the image to the OCI Format
or Docker Image manifest v2, schema 2. More information at
https://docs.docker.com/go/deprecated-image-specs/
$

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-02-13T16:33:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ricardomaraschini
Once this PR has been reviewed and has the lgtm label, please assign atiratree for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ardaguclu · 2025-02-13T17:57:58Z

pkg/cli/image/append/append.go

@@ -559,7 +559,7 @@ func (o *AppendImageOptions) append(ctx context.Context, createdAt *time.Time,
 	if skipTagging {
 		tag = ""
 	}
-	toDigest, err := imagemanifest.PutManifestInCompatibleSchema(ctx, manifest, tag, toManifests, toRepo.Named(), fromRepo.Blobs(ctx), configJSON)
+	toDigest, err := imagemanifest.PutManifestInCompatibleSchema(ctx, manifest, tag, toManifests, toRepo.Named(), toRepo.Blobs(ctx), configJSON)


I appreciate such in depth analysis and thank you. What issue does this change fix?

As far as I understand, this fixes permission error that blocking the functionality of v2 schema. Thanks to this fix, there won't be fall back to v1 conversion.

But this is not related to OCI media type support, right?

The logic we currently have is: if a push of a v2 manifest fails (because it is invalid) we convert it to v1 and retry (this hasn't changed). The retry isn't working because of permissions though.

After looking at the code it seems like we were using the client for the "source registry" (fromRepo) instead of the client for the "target registry" (toRepo). i.e. the client for the "source registry" does not contain credentials for the "target registry".

I am happy to chat about it. Please let me know.

ardaguclu · 2025-02-14T05:14:20Z

/test e2e-agnostic-ovn-cmd

openshift-ci · 2025-02-14T07:27:21Z

@ricardomaraschini: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`ba860bb`	link	false	`/test security`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Feb 13, 2025

openshift-ci bot requested review from ardaguclu and atiratree February 13, 2025 16:33

ardaguclu reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-50660: use destination client on v1 conversion #1975

OCPBUGS-50660: use destination client on v1 conversion #1975

ricardomaraschini commented Feb 13, 2025 •

edited

Loading

openshift-ci-robot commented Feb 13, 2025

openshift-ci bot commented Feb 13, 2025

ardaguclu Feb 13, 2025

ardaguclu Feb 13, 2025

ricardomaraschini Feb 13, 2025 •

edited

Loading

ardaguclu commented Feb 14, 2025

openshift-ci bot commented Feb 14, 2025

OCPBUGS-50660: use destination client on v1 conversion #1975

Are you sure you want to change the base?

OCPBUGS-50660: use destination client on v1 conversion #1975

Conversation

ricardomaraschini commented Feb 13, 2025 • edited Loading

openshift-ci-robot commented Feb 13, 2025

openshift-ci bot commented Feb 13, 2025

ardaguclu Feb 13, 2025

Choose a reason for hiding this comment

ardaguclu Feb 13, 2025

Choose a reason for hiding this comment

ricardomaraschini Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

ardaguclu commented Feb 14, 2025

openshift-ci bot commented Feb 14, 2025

ricardomaraschini commented Feb 13, 2025 •

edited

Loading

ricardomaraschini Feb 13, 2025 •

edited

Loading