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

client: Fix failed to pull ubuntu image from docker.io #67

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

arronwy
Copy link
Contributor

@arronwy arronwy commented Mar 13, 2023

Manifests list feature in oci-distribution is already implemented, default image in docker.io like busybox use IMAGE_MANIFEST_LIST_MEDIA_TYPE, but ubuntu image use OCI_IMAGE_INDEX_MEDIA_TYPE.

Fixes: #27

Copy link
Contributor

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. I've left a comment about the tests

src/client.rs Outdated
@@ -2375,6 +2377,20 @@ mod test {
assert_eq!(manifest.config.media_type, manifest::WASM_CONFIG_MEDIA_TYPE);
}

#[tokio::test]
async fn test_pull_docker_ubuntu() {
let reference = Reference::try_from(DOCKER_IO_UBUNTU).expect("failed to parse reference");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this test could report false negatives because of connectivity issues or rate limiting done by Docker Hub.

We are already starting a local instance of distribution during the execution of the unit tests. I think we should change this test to leverage this local instance, push something that uses a OCI_IMAGE_INDEX_MEDIA_TYPE inside of it and then try to pull it.

This would make the tests more robust

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agree, we also have many rate limits issue during use docker hub.

Will fix, thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found local registry: https://github.com/distribution/distribution don't support application/vnd.oci.image.index.v1+json even it is defined in : https://github.com/opencontainers/image-spec/blob/main/media-types.md#compatibility-matrix
it will have following error when we try to push the manifest:

ERRO[1363] response completed with error   err.code="manifest invalid" 
err.detail="mediaType in manifest list should be 'application/vnd.docker.distribution.manifest.list.v2+json' not 'application/vnd.oci.image.index.v1+json'" err.message="manifest invalid" go.version=go1.20 http.request.contenttype=application/vnd.docker.distribution.manifest.list.v2+json 
http.request.host="localhost:5005" http.request.id=331dff94-3942-47be-803a-c0c18c1b1e8d 
http.request.method=PUT http.request.remoteaddr="127.0.0.1:39880" http.request.uri=/v2/ubuntu/manifests/latest http.request.useragent= http.response.contenttype=application/json http.response.duration="126.053µs" http.response.status=400 http.response.written=82 instance.id=2b9ba786-7076-4a8a-a5f9-b87a7f720225 service=registry vars.name=ubuntu vars.reference=latest version=v2.7.0-2130-ge5d58108

Copy link
Contributor Author

Choose a reason for hiding this comment

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

submitted the registry issue in: distribution/distribution#3863

Manifests list feature in oci-distribution is already
implemented, default image in docker.io like busybox
use IMAGE_MANIFEST_LIST_MEDIA_TYPE, but ubuntu image
use OCI_IMAGE_INDEX_MEDIA_TYPE.

Fixes: oras-project#27

Signed-off-by: Wang, Arron <[email protected]>
@flavio
Copy link
Contributor

flavio commented Mar 17, 2023

@arronwy Given I don't know how much time it will take for distribution to fix this issue, and given how easy the fix is on our side I propose the following approach:

  • We create an issue to keep track of the missing unit test
  • We merge this fix without any unit test

What do you think about that?

@arronwy
Copy link
Contributor Author

arronwy commented Mar 17, 2023

@arronwy Given I don't know how much time it will take for distribution to fix this issue, and given how easy the fix is on our side I propose the following approach:

  • We create an issue to keep track of the missing unit test
  • We merge this fix without any unit test

What do you think about that?

Agree, thanks a lot!

@arronwy
Copy link
Contributor Author

arronwy commented Mar 17, 2023

I have updated this PR, and create an issue to track uni tests: #68

@flavio flavio merged commit f44124c into oras-project:main Mar 17, 2023
@arronwy arronwy deleted the ubuntu branch March 18, 2023 02:51
arronwy added a commit to confidential-containers/guest-components that referenced this pull request Mar 21, 2023
Latest oci-distribution have fixed faile to pull `ubuntu`
image from docker.io: oras-project/rust-oci-client#67

Restrict sigstore-rs to use the same `oci-distribuiton` with `image-rs`
to fix the build issue.

Signed-off-by: Wang, Arron <[email protected]>
@flavio flavio mentioned this pull request Nov 24, 2023
flavio added a commit to flavio/rust-oci-client that referenced this pull request Nov 24, 2023
What's Changed:
* client: Fix failed to pull ubuntu image from docker.io by @arronwy in oras-project#67
* Update rstest requirement from 0.16.0 to 0.17.0 by @dependabot in oras-project#70
* Bump actions/checkout from 3.3.0 to 3.4.0 by @dependabot in oras-project#69
* Bump actions/checkout from 3.4.0 to 3.5.0 by @dependabot in oras-project#73
* Change the visibility of `OciEnvelop.errors` to `pub` by @linyinfeng in oras-project#71
* Bump actions/checkout from 3.5.0 to 3.5.2 by @dependabot in oras-project#74
* make pushes concurrent and limit pull concurrency by @dicej in oras-project#72
* Update default `docker.io` registry by @radu-matei in oras-project#81
* Fix: Expose HTTP errors when pulling layers by @jsoverson in oras-project#82
* client: Return Stream for async_pull_blob API by @arronwy in oras-project#83
* chore(deps): Bump actions/checkout from 3.5.2 to 3.5.3 by @dependabot in oras-project#85
* fix(CI): address warnings of cargo deny by @flavio in oras-project#84
* consider ARCH as ppc64le when rust arch is powerpc64 by @Amulyam24 in oras-project#86
* chore(deps): Update rstest requirement from 0.17.0 to 0.18.1 by @dependabot in oras-project#88
* add optional artifactType to image manifest for oci v1.1 by @devigned in oras-project#89
* Higher-ranked lifetime error workaround by @linyinfeng in oras-project#90
* chore(deps): Bump actions/checkout from 3.5.3 to 3.6.0 by @dependabot in oras-project#91
* chore(deps): Bump actions/checkout from 3.6.0 to 4.0.0 by @dependabot in oras-project#92
* chore(deps): Bump actions/checkout from 4.0.0 to 4.1.0 by @dependabot in oras-project#94
* Improved Push Logic by @rbbl-dev in oras-project#93
* Update the configfile to match the oci v1 spec by @lswith in oras-project#77
* Conversion between a Config and a ConfigFile by @lswith in oras-project#76
* Enhance client to push blobs, mount blobs, and push raw manifests by @aochagavia in oras-project#66
* fix: bring back workaround for rustc by @flavio in oras-project#95
* implemented method to list tags by @rbbl-dev in oras-project#80
* feat(config.rs): add OS type for WASI Preview 1 by @vdice in oras-project#99
* chore(deps): Bump actions/checkout from 4.1.0 to 4.1.1 by @dependabot in oras-project#100
* chore(deps): Update testcontainers requirement from 0.14 to 0.15 by @dependabot in oras-project#96
* feat: return auth token from Client::auth by @mattarnoatibm in oras-project#105

Signed-off-by: Flavio Castelli <[email protected]>
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.

Support manifest list
2 participants