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

Fix image filters parsing #21260

Merged
merged 4 commits into from
Jan 28, 2024
Merged

Conversation

umohnani8
Copy link
Member

@umohnani8 umohnani8 commented Jan 15, 2024

Fix the image filter parsing in the common libraries
to follow an AND logic for all filters passed in ensuring
compatibility with Docker behavior.
Also fix the filter parsing on the tunnel side so that we grab
all the filters given by the user and not only the last filter
in the list.
Add tests for the fixes.

Fixes #18412

Does this PR introduce a user-facing change?

Fix image filter parsing to follow AND logic.
Fix image filter parsing on the tunnel endpoint to pick up all the filters given by the user and not only the last one in the list.

Copy link
Contributor

openshift-ci bot commented Jan 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2024
@umohnani8
Copy link
Member Author

Need to wait on containers/common#1800 before merging

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2024
Pull in updates made to the filters code for
images. Filters now perform an AND operation
except for th reference filter which does an
OR operation for positive case but an AND operation
for negative cases.

Signed-off-by: Urvashi Mohnani <[email protected]>
Fix the image filter parsing in the common libraries
to follow an AND logic for all filters passed in ensuring
compatibility with Docker behavior.
Also fix the filter parsing on the tunnel side so that we grab
all the filters given by the user and not only the last filter
in the list.
Add tests for the fixes.

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

This is an improvement, but I can’t see how it could fix https://github.com/containers/podman/pull/21359/checks?check_run_id=20847280131 . In there we have a spurious match, not a spurious mismatch.

pkg/autoupdate/autoupdate.go Outdated Show resolved Hide resolved
Since images can have multiple digests, it is better
to compare the image ID as that will definitely change
on an update and each image can only have one ID.

Signed-off-by: Urvashi Mohnani <[email protected]>
is "$output" "$expected"
run_podman container inspect $randomname --format "{{.ImageDigest}}"
assert "$output" =~ "$expectedDigests"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also allow a match against the repo name part of RepoDigests. That’s not quite clean, but it might be sufficient given the test description.

Either way, let’s see if this version works :)

Given that we can have multiple image digests,
fix the inspect test to check whether the digest
given matches one of the digests of the image.

Signed-off-by: Urvashi Mohnani <[email protected]>
@umohnani8
Copy link
Member Author

@mheon @ashley-cui tests are happy now with the c/common bump.

We just need bloat approval, PTAL

@ashley-cui
Copy link
Member

Excellent, thanks! LGTM

@mheon mheon added the bloat_approved Approve a PR in which binary file size grows by over 50k label Jan 26, 2024
@mheon
Copy link
Member

mheon commented Jan 26, 2024

Adding bloat approved.

@TomSweeneyRedHat
Copy link
Member

Changes LGTM
but a couple tests still seem to be causing hiccups

@TomSweeneyRedHat
Copy link
Member

@umohnani8 ^^

@umohnani8
Copy link
Member Author

I think the machine test failures are expected. All other tests are green now.

@mheon
Copy link
Member

mheon commented Jan 26, 2024

WSL and HyperV should be passing

@rhatdan
Copy link
Member

rhatdan commented Jan 26, 2024

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2024
@ashley-cui
Copy link
Member

I can reproduce the win failure locally, taking a look....

@ashley-cui
Copy link
Member

ashley-cui commented Jan 26, 2024

Looks like the error comes from this pr, on line 297. This is called by the windows client while reading containers.conf

@giuseppe, should getRootlessDirInfo() be reinstated, or is there something else we can do?

@rhatdan
Copy link
Member

rhatdan commented Jan 28, 2024

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d7bf138 into containers:main Jan 28, 2024
89 of 92 checks passed
@ashley-cui
Copy link
Member

I don't think this should have merged, the windows failure is real. c/common is broken right now because of c/storage.

@giuseppe
Copy link
Member

@giuseppe, should getRootlessDirInfo() be reinstated, or is there something else we can do?

can we fix it in c/storage? What is the expected result on windows?

@ashley-cui
Copy link
Member

We need to be able to get the rootlessruntimedir in so that we can read containers.conf on the client side for windows. homedir.GetDataHome() is unsupported on windows, it was working before since getRootlessDirInfo() had a fallthrough, where it called getdatahome(), and if that errored, it used the homedir instead. I think putting that fallback logic back should fix the issue.

@ashley-cui
Copy link
Member

We either fix it in c/storage there, or c/common here, and not use the function from c/storage. containers/common@ff1039a

@mheon
Copy link
Member

mheon commented Jan 29, 2024

Fixing in c/storage seems safer, we don't know what other places will try and call it

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 29, 2024

(OTOH the goal of that effort was to consolidate the heuristics/fallbacks to be the same in all parts of the binary. That’s why it was moved all the way down to c/storage. I didn’t look into details, just to make sure this is considered.)

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 29, 2024

Looks like the error comes from this pr, on line 297. This is called by the windows client while reading containers.conf

Do I undestand correctly that we are trying to compute c/storage options when setting up a podman-remote client on Windows? That seems conceptually completely unnecessary, can all of that be avoided? (And, in the extreme, compiled out?) I guess there is an abstraction boundary somewhere so that we don’t know/care that this is a remote client only… maybe worth at least filing a Podman tracking issue?

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Apr 29, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bloat_approved Approve a PR in which binary file size grows by over 50k lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[spike] remote filters
8 participants