podman stop --filter id=abc : 'abc' should match only at start

[edsantiago]

An interesting new flake today, took me much too long to understand:

# podman-remote [options] start --all
452f149c83e56a519a8ad550daf49ecb9afee438449b6816ed935be96d6834e4
88d4b8a4acc0a0269dff81c7e687008e46e9a2ac5e14cb3884789d76bcbc020b
76bcb4b0b1cd7af97ada76bef9e803a74e5fe9e0a6ecd55d8dcd4e864fe3e72b

# podman-remote [options] stop -a --filter id=76bcb
88d4b8a4acc0a0269dff81c7e687008e46e9a2ac5e14cb3884789d76bcbc020b
76bcb4b0b1cd7af97ada76bef9e803a74e5fe9e0a6ecd55d8dcd4e864fe3e72b

...test fails because it expected output to be 76bcb4b0b1cd7af97ada76bef9e803a74e5fe9e0a6ecd55d8dcd4e864fe3e72b. Can you see why the test failed? How about now?

...--filter id=76bcb
88d4b8a4acc0a0269dff81c7e687008e46e9a2ac5e14cb3884789d76bcbc020b
76bcb4b0b1cd7af97ada76bef9e803a74e5fe9e0a6ecd55d8dcd4e864fe3e72b

Assumption: nobody is ever, ever, ever going to call --filter id=SOMETHING-IN-THE-MIDDLE.

Proposal: make --filter id=NNN anchor at the beginning of the id.

[rhatdan]

The code calls:
return util.StringMatchRegexSlice(c.ID(), filterValues)
I believe, so we would need to see what Docker does

Your test could be changed to "^76bcb.*" I think.

[rhatdan]

Playing around with docker, it looks like it does not use regex at all.

[rhatdan]

More likelye HasPrefix(

[vrothberg]

Slightly related: remote filters are quite broken (see #18412)

[rhatdan]

Well if we change to match Docker behavior we have a breaking change. I think it best to fix the test and just let it be.

[vrothberg]

Well if we change to match Docker behavior we have a breaking change. I think it best to fix the test and just let it be.

It's broken one way or another, but backwards compat is certainly a challenge.

[rhatdan]

Well broken, if you don't give it enough specificity.

[edsantiago]

Going back to my initial report... let's say I have these two containers:

f00.....    etc etc
...f00...   something else

Then let's say I type podman stop -a --filter id=f00 (which, granted, I really should type as podman stop f00).

I humbly submit that stopping both containers violates POLA. I realize it's impossible to know if anyone has ever intentionally done --filter id=SOMETHING-IN-THE-MIDDLE, but I'd bet it's unlikely: that's just not how humans work.

name=SOMETHING-IN-THE-MIDDLE, I can see that being important to keep: I could easily name containers foo_action, bar_action, baz_action and want to stop all action. But ID???

[vrothberg]

I agree, Ed. Short IDs have always been prefixes not sub-strings.

[Luap99]

I guess I take the blame for the regex part: 4f427a8
However if you look there it was already Contains() before and not HasPrefix(). So this goes back a very long time.

I rather not break podman users.

Interestingly enough if you use docker ps --filter id=XXX docker matches the prefix but as soon as there is more than one match ps will return nothing. So if we want to be compatible there we have to follow that.

[root@fedora ~]# docker ps -a
CONTAINER ID   IMAGE     COMMAND                  CREATED       STATUS                     PORTS     NAMES
251d6b1c410e   alpine    "top"                    4 weeks ago   Exited (143) 4 weeks ago             brave_herschel
14912c3d505c   nginx     "/docker-entrypoint.…"   5 weeks ago   Exited (255) 4 weeks ago             funny_dirac
5dd9db23142c   alpine    "ping 1.1.1.1"           5 weeks ago   Exited (0) 5 weeks ago               crazy_snyder
0ee1a7046c27   alpine    "ip a"                   5 weeks ago   Exited (0) 5 weeks ago               upbeat_ritchie
0340e48050f0   alpine    "ip a"                   5 weeks ago   Exited (0) 5 weeks ago               reverent_boyd
66d8f33c5b78   alpine    "top"                    6 weeks ago   Exited (143) 6 weeks ago             practical_gauss
[root@fedora ~]# docker ps -a --filter id=0
CONTAINER ID   IMAGE     COMMAND   CREATED   STATUS    PORTS     NAMES
[root@fedora ~]# docker ps -a --filter id=6
CONTAINER ID   IMAGE     COMMAND   CREATED       STATUS                     PORTS     NAMES
66d8f33c5b78   alpine    "top"     6 weeks ago   Exited (143) 6 weeks ago             practical_gauss

[vrothberg]

I rather not break podman users.

That's a tough one. I see it as a bug but if users would depend on this very behavior - which we for sure can never know - they'd break.

[edsantiago]

if users would depend on this very behavior - which we for sure can never know - they'd break.

I humbly submit that we do not care. This is a serious POLA violation, and the future cost of keeping the current behavior is higher than the cost of breaking anyone who does match-the-middle-of-a-SHA.

[rhatdan]

I am leaning towards @edsantiago here. Users could accidentally do bad things like
podman rm --filter
podman stop --filter
podman start --filter

[Luap99]

I disagree, the docs say the id filter accepts regex. We never know if users rely on that.
We should always care about not breaking users! If your script would stop working all of the sudden you will not like it. Sure it may not make much sense and is a compatibility bug with docker but it is what is is.

This affects all --filter id= on containers, pods, networks and maybe stuff that I am missing where we have ids. (Volumes have no id).

[rhatdan]

Using regex on a semi random string, makes little sense. You would need to search all images ids for a pattern and then match on it. which eliminates the need for a filter

I agree on regex for meaningful patterns, I don't see how it makes sense for random patterns.

Another way of saying this would be there is nothing regular in an id, to use a regular expression on.

[edsantiago]

"accepts regex", to me, means "ACCEPTS", as in, "if I see a dot, asterisk, bracket, or other character suggesting a regular expression is desired, I will interpret it as such". "Accepts" does not, to my mind, automatically mean "defaults to".

IOW id=.*abc will work as a normal human would expect, but id=abc will not mean "abc is a regex".

Seriously, this is very important to me. There is no human being alive who has a script that does "podman ps -q | select some random middle characters from the CID | podman ... --filter". If there is such a person or script, there is something very wrong with their brain.

OTOH this will bite us: a human being will some day type, or write a script, that filters on id=FIRST-FEW-CHARACTERS-OF-CID, and those characters will match in the middle of another container, purely by chance, and a container will be killed that the user did not want to be killed.

Yeah, what Dan said.

[Luap99]

Sure I get your point but what is POLA here?
Assume you have two container ids starting with abcd, should id=abcd match both, none or error out?
docker ps matches none (there is no --filter for docker start/stop)

[edsantiago]

Good question. I would expect podman to match both. If I, as a human, see two CIDs in the output, I would immediately see that both begin with abcd, and I would think, well, that was stupid of me to use a short substring, and I would learn from it. (Compare to seeing two CIDs, and taking ten minutes for my stupid mammal brain to figure out that the second CID matches because it has abcd in the middle, I would learn nothing from that, and I would be very angry at having my time wasted). But maybe my reaction is not neurotypical?

[rhatdan]

I am surprised that @edsantiago even found the conflict. Current behaviour is very difficult to figure out what happened.

podman rm --filter=a

Could remove my objects with the current design.

[rhatdan]

I am not even sure we are consistant on our id filters.

// filterID creates an image-ID filter for matching the specified value.
func filterID(value string) filterFunc {
	return func(img *Image) (bool, error) {
		return img.ID() == value, nil
	}
}

Image id filter looks like you need a complete match.

[edsantiago]

Flake just triggered again.

Yes, it's easy to amend the test to use an anchored regex, but that is the wrong thing to do.

[Luap99]

I think a compromise would be to use prefix match if input is a valid ID (e.g. hex chars only). And then only use regex when is contains non hex chars. This would only break someone if they do a match in the middle but as you point out this isn't very likely.

[edsantiago]

Wait - we're talking --filter id=. How can that be valid with non-hex characters?

[Luap99]

id=^abc.*$ is a regex so it will work.

[edsantiago]

duh, sorry. I shouldn't do drive-by comments on my days off.

Can anyone see any use case for regex matching? it can't possibly be automated. IMHO it's not worth the effort of adding regex logic when HasPrefix() is the simple and desired solution.

[Luap99]

Can anyone see any use case for regex matching? it can't possibly be automated. IMHO it's not worth the effort of adding regex logic when HasPrefix() is the simple and desired solution.

I agree in principle that it is not really useful. However it is around for a long time and even documented so changing it could break people. While there is little reason to do it it doesn't mean nobody does it. People do weird things all the time, e.g. me adding regex for ids in the first place. And a valid use case may be people who run into an issue like you and then worked around it by doing ^id... instead of reporting it upstream.

[rhatdan]

Sounds like a plan. Check the id first for any non hex characters, if yes then use regex check, if no do HasPrefix()