Fix image filters parsing

[umohnani8]

Fix the libpodFilters parsing to ensure that
we get a list of filters in the form of key=value.
This matches what the docker endpoint does for filter
parsing as well.

When multiple filters are given, only return objects
that match all the filters given by the user.
This matches Docker behavior.

[umohnani8]

Podman PR containers/podman#21260

[umohnani8]

@rhatdan @mtrmac @vrothberg PTAL

[mtrmac]

Just a quick skim of the PR, I didn’t now compare with Docker.

[mtrmac]

Just a quick skim of the PR, I didn’t now compare with Docker.

From a quick spot check, this is not nearly as simple:
https://github.com/moby/moby/blob/3eba4216e085be3b5c62c2e10317183485d006d7/daemon/images/image_list.go#L131 (and some other fields) are an implicit AND, but https://github.com/moby/moby/blob/3eba4216e085be3b5c62c2e10317183485d006d7/daemon/images/image_list.go#L162 are an implicit OR.

IOW something somewhere (compileImageFilters?) needs to collect the values for each key (or only for the special cases?) and turn them into filters with equivalent operation.

[umohnani8]

@mtrmac by implicit OR, do you mean the pattern match being done for the reference filter? Podman does that as well, we match reference and id on pattern as well.
From what I saw while testing with Docker, when multiple filters are given the image must match all of them before making the cut. Some filters do a pattern match, while others do a strict equality match. reference and id are the ones I have seen doing a pattern match.

[mtrmac]

I read that loop to mean that (reference=foo reference=bar) matches if either one matches; it doesn’t need to match both.

Also see the reference examples in https://docs.docker.com/engine/reference/commandline/images/ or https://docs.docker.com/config/filter/ .

[umohnani8]

From what I see and understand from the docs, looks like only reference does an OR check while the others do an AND. We can add a special case for reference, wdyt?

[mtrmac]

If you mean a special case for reference in compileImageFilters; I agree. Doing that in applyFilters seems rather unclean … restructuring the compilation to work one key at a time would make that special case in applyFilters unnecessary.

• The “compilation” is what should coalesce all values for the same key, and interpret that. It should return just a []filterFunc, or, with a more extreme restructuring, just a filterFunc (which does the ANDing itself, in a lambda).
• In some cases that means that it could just do one check instead of several (if there were several since values, for example); we don’t have to do that, it’s extra work to benefit cases that probably never happen.
• That would be more similar to how non-image filters work; compare how various Podman callers of PrepareFilters consume the values.
• So, ListImagesOptions.Filters should be a map[string][]string, not just []string; that would avoid unnecessary conversion to the current []string format
• … uh, really, why are we carrying data around in a stringly-typed format at all? Some variant of the “compile” step could happen very early in the handling of a request, and then ListImagesOptions.Filters could be a filterImageFunc, a single function pointer with the result of that compilation.

Admittedly the further down that list we go, the larger and more invasive the rewrite becomes… but restructuring compileImageFilters to work not one key=value pair at a time, but one key at a time, seems pretty doable.

[mtrmac]

Hum, reference != … is currently accepted. So the user can say (reference!=a, reference=b, reference=c, reference!=d) and I don’t know what that means. (Docker seems not to have `reference!=', but I didn’t check too carefully.)

[umohnani8]

@mtrmac I fixed up the filterReference function as we discussed and added tests - PTAL.

[mtrmac]

Thanks!

Semantics LGTM. Aesthetically I’d prefer some further simplification but it’s not really blocking if we were in a time pressure — feel free to merge as is.

[mtrmac]

1. This can outright return false now; code below doesn’t need to worry about unwanted…Matches and the unwantedMatched variable can be dropped.

[mtrmac]

2. Then it might be a tiny bit more readable to have

if len(wanted…) != 0 {
  for … wanted {
    if matches { return true }
  }
  return false // wanted a match, but nothing found
}
return true // all requirements satisfied. 

[mtrmac]

… on second thought, the “all requirements satisfied” comment might be misleading (it somewhat suggests that there isn’t an early return true in there). So this might not be worth it.

[umohnani8]

@mtrmac can you please add the approve and lgtm labels

[mtrmac]

/lgtm

[mtrmac]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, 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:

• OWNERS [mtrmac]

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