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

layer tree: handle more errors #1522

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Jun 26, 2023

Commit 97961a6 took a first stab at making operating on the layer
tree more resilient by detecting unknown-image errors. Testing in
containers/podman/pull/18980 revealed that we also need to detect
unknown-layer and -size errors as well.

Move the errors checks into a convenience function and update the
relevant call sites to facilitate future changes. Export the
function since Podman needs the very same checks when operating
on images, for instance, when looking up image labels.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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

The pull request process is described 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

@vrothberg
Copy link
Member Author

/hold

don't merge, still WIP

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.

Should

handleError := func(err error) error {
be updated to use this new function instated? This one also catches storage.ErrNotAnImage

@giuseppe giuseppe self-requested a review June 26, 2023 09:57
@vrothberg
Copy link
Member Author

@Luap99 great idea

@vrothberg
Copy link
Member Author

/hold cancel

@vrothberg
Copy link
Member Author

Good to go from my end. containers/podman#18980 is green.

@Luap99 @giuseppe PTAL

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.

LGTM

libimage/layer_tree.go Outdated Show resolved Hide resolved
Commit 97961a6 took a first stab at making operating on the layer
tree more resilient by detecting unknown-image errors.  Testing in
containers/podman/pull/18980 revealed that we also need to detect
unknown-layer and -size errors as well.

Move the errors checks into a convenience function and update the
relevant call sites to facilitate future changes.  Export the
function since Podman needs the very same checks when operating
on images, for instance, when looking up image labels.

Signed-off-by: Valentin Rothberg <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Jun 26, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 26, 2023
@openshift-merge-robot openshift-merge-robot merged commit 370c898 into containers:main Jun 26, 2023
@vrothberg vrothberg deleted the bz-2216700 branch June 26, 2023 12:01
vrothberg added a commit to vrothberg/libpod that referenced this pull request Jun 26, 2023
Handle more TOCTOUs operating on listed images.  Also pull in
containers/common/pull/1520 and containers/common/pull/1522 which do the
same on the internal layer tree.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2216700
Signed-off-by: Valentin Rothberg <[email protected]>
ashley-cui pushed a commit to ashley-cui/podman that referenced this pull request Jul 13, 2023
Handle more TOCTOUs operating on listed images.  Also pull in
containers/common/pull/1520 and containers/common/pull/1522 which do the
same on the internal layer tree.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2216700
Signed-off-by: Valentin Rothberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants