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.

Sign up for GitHub

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

Jump to bottom

Fixes: Distribution API #17726 #25378

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixes: Distribution API #17726 #25378

wants to merge 1 commit into from

Conversation

kanlac
Copy link

@kanlac kanlac commented Feb 21, 2025 •
edited
Loading

Does this PR introduce a user-facing change?

Added support for /distribution API

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 21, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 21, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kanlac
Once this PR has been reviewed and has the lgtm label, please assign ashley-cui 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:

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

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Feb 21, 2025
Luap99
Luap99 requested changes Feb 24, 2025
View reviewed changes
Copy link
Member

@Luap99 Luap99 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 PR, we should avoid using that code considering that all our image handling is done in containers/image so we should use that.
The github.com/distribution/reference dependency causes way to much bloat (binary size increase)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2025
@kanlac kanlac force-pushed the main branch 3 times, most recently from b125e3d to 90ba130 Compare February 28, 2025 03:49
@kanlac
Copy link
Author

kanlac commented Feb 28, 2025

I used the image API to handle the query, keeping things lightweight with no new deps. Please take a look—if it looks good, I’ll add support for different manifest types next. @Luap99

@kanlac kanlac marked this pull request as ready for review February 28, 2025 03:52
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2025
Luap99
Luap99 reviewed Feb 28, 2025
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks, that looks much better. I don't know the c/image API well so @mtrmac might having a look here?

@Luap99
Copy link
Member

Luap99 commented Feb 28, 2025

Oh and please add Fixes #17726 to your commit message

mtrmac
mtrmac reviewed Feb 28, 2025
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Just a very brief skim for now, I didn’t check what the API is expected to do.

@kanlac kanlac force-pushed the main branch 4 times, most recently from cf24ec6 to 3ed7716 Compare March 3, 2025 01:38
mtrmac
mtrmac reviewed Mar 3, 2025
View reviewed changes
pkg/api/handlers/compat/distribution.go

namedRef, imgRef, err := parseImageReference(name)
if err != nil {
utils.Error(w, http.StatusUnauthorized, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is StatusUnauthorized correct for a format error?

@kanlac kanlac force-pushed the main branch 3 times, most recently from 45251c5 to 22cdd39 Compare March 7, 2025 00:43
mtrmac
mtrmac reviewed Mar 7, 2025
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

The c/image API usage looks generally fine now.

(I didn’t check what the API is expected to return.)

pkg/api/handlers/compat/distribution.go Outdated
Comment on lines 126 to 129
switch manType {
case ocispec.MediaTypeImageManifest, manifest.DockerV2Schema2MediaType:
config, err := img.OCIConfig(ctx)
if err != nil {
return nil, fmt.Errorf("error getting OCI config: %w", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for the restrictive manType, excluding schema1?

OTOH this will fail for OCI artifacts. Should it fail? I really don’t know. If not, check for NonImageArtifactError.

@kanlac kanlac requested a review from Luap99 March 8, 2025 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac left review comments

@Luap99 Luap99 Awaiting requested review from Luap99

Assignees
No one assigned
Labels
kind/api-change Change to remote API; merits scrutiny release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kanlac @Luap99 @mtrmac @openshift-merge-robot