volume rm require exact name #24027

zackattackz · 2024-09-21T05:21:23Z

This commit reverts the behavior of allowing volume removal by partial name in favor of requiring the exact name. It does this by favoring usage of GetVolume over LookupVolume

In the past, issue #3891 was raised to allow partial name matching on podman volume rm, which was fixed by PR #3896. However, it has been determined in issue #23943 that the pros of allowing removal by partial name (less keys to type) are outweighed by the cons (possibility of unintentionally deleting a volume you didn't mean to). So the behavior will be reverted.

Since PR #3896, there has also been PR #4832 and PR #6736, which extended the partial name removal behavior to API endpoints. This commit also makes these endpoints require exact names.

Closes #23943

Does this PR introduce a user-facing change?

Fixed volume removal by partial name in both the CLI and API. The full volume name is now required.

openshift-ci · 2024-09-21T05:21:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zackattackz
Once this PR has been reviewed and has the lgtm label, please assign ashley-cui for approval. For more information see the Kubernetes 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:

OWNERS

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

packit-as-a-service · 2024-09-21T05:21:32Z

We were not able to find or create Copr project packit/containers-podman-24027 specified in the config with the following error:

Packit received HTTP 500 Internal Server Error from Copr Service. Check the Copr status page: https://copr.fedorainfracloud.org/status/stats/, or ask for help in Fedora Build System matrix channel: https://matrix.to/#/#buildsys:fedoraproject.org.

Unless the HTTP status code above is >= 500, please check your configuration for:

typos in owner and project name (groups need to be prefixed with @)
whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
whether the project itself exists (Packit creates projects only in its own namespace)
whether Packit is allowed to build in your Copr project
whether your Copr project/group is not private

zackattackz · 2024-09-21T05:24:25Z

Tested with test-apiv2 as well for the API changes (30-volumes.at) locally and it failed before changes and passes after.

zackattackz · 2024-09-21T05:25:37Z

Regarding anonymous volumes, removal of them exhibited the same behavior as names and ids are treated the same, and no special conditions look to be in place for removal of anonymous volumes.

zackattackz · 2024-09-21T05:26:26Z

Also worth noting, no other rm commands I looked at shared the same original behavior of removing on partial name matching (container rm, rmi, pod rm)

zackattackz · 2024-09-21T05:33:01Z

Amended with test in pkg/bindings/test/volumes_test.go

zackattackz · 2024-09-21T05:48:32Z

Amended because codespell doesn't like "fo"

rhatdan · 2024-09-21T11:41:10Z

Thanks @zackattackz
LGTM, although you need to cleanup tests.

zackattackz · 2024-09-21T16:07:56Z

No problem @rhatdan, thanks for reviewing.
Was pretty simple but nice way to poke around the code a bit.

I amended to fix two failing tests that the stdout result of them wasn't matching what was expected. It's weird though because shouldn't the "bogus volume" test have been failing prior to my commit? It already had the wrong expected stdout in there. Just wasn't sure if this indicated I've screwed something up somehow. I couldn't get the e2e tests running on my machine to check it before my commit.

Also, I did add the release note bc I wasn't sure if this would be a breaking change or not. It is a tightening of the API so I would think it would be, but I'll leave that decision to y'all.

zackattackz · 2024-09-22T00:15:09Z

There seems to be multiple different possible stderr messages volume rm can emit in the e2e volume_rm test. Either "Error: volume with name {name} not found: no such volume" or "Error: no such volume".

Again, I'm not sure how it was passing before, unless I caused this. So to fix it I amended the test to just check for non clean exit instead of checking specific error message.

zackattackz · 2024-09-22T00:29:01Z

Ah ok so after looking more into it basically the new error messages that are coming up are due to GetVolume's implementations in boltdb and sqlite returning different error messages than the LookupVolume counterparts.

So I could check the backend like is done in other tests (here for example) and change the expected error message based on that. Or I could keep the current version that doesn't check specifically. Whichever one the reviewer prefers.

edsantiago

Please find a way to check error messages.

Reason: we've had far too many instances of something like:

    foo = ...podman("command", "OOPSTYPOMISSPELLINGORSOMETHINGLIKETHAT", "...")
    Expect(failure)

...and it does fail with "syntax error" or "unknown command" or something completely different but it's still a failure so CI passes.

Thanks for understanding.

edsantiago · 2024-09-22T00:28:03Z

test/e2e/volume_rm_test.go

@@ -54,7 +54,7 @@ var _ = Describe("Podman volume rm", func() {
 It("podman volume remove bogus", func() {
 session := podmanTest.Podman([]string{"volume", "rm", "bogus"})
 session.WaitWithDefaultTimeout()
- Expect(session).Should(ExitWithError(1, `no volume with name "bogus" found: no such volume`))
+ Expect(session).ShouldNot(ExitCleanly())


Hi! Thank you for your contribution, this is much-needed work, but we absolutely require error-message checks. This is not negotiable.

Hey thank you for checking this out. I figured out why they were different, see my latest comment. I can check the specific database backend of the test runner to change the expected result

edsantiago · 2024-09-22T00:28:17Z

test/e2e/volume_rm_test.go

- expect = `more than one result for volume name "myv": volume already exists`
- }
- Expect(session).To(ExitWithError(125, expect))
+ Expect(session).ShouldNot(ExitCleanly())


This commit reverts the behavior of allowing volume removal by partial name in favor of requiring the exact name. It does this by favoring usage of `GetVolume` over `LookupVolume` In the past, issue containers#3891 was raised to allow partial name matching on `podman volume rm`, which was fixed by PR containers#3896. However, it has been determined in issue containers#23943 that the pros of allowing removal by partial name (less keys to type) are outweighed by the cons (possibility of unintentionally deleting a volume you didn't mean to). So the behavior will be reverted. Since PR containers#3896, there has also been PR containers#4832 and PR containers#6736, which extended the partial name removal behavior to API endpoints. This commit also makes these endpoints require exact names. Closes containers#23943 Signed-off-by: Zachary Hanham <[email protected]>

zackattackz · 2024-09-22T15:36:57Z

test/e2e/volume_rm_test.go


 session = podmanTest.Podman([]string{"volume", "ls"})
 session.WaitWithDefaultTimeout()
 Expect(session).Should(ExitCleanly())
- Expect(len(session.OutputToStringArray())).To(BeNumerically(">=", 2))
+ output := session.OutputToStringArray()
+ expectedLs := []string{"DRIVER VOLUME NAME", fmt.Sprintf("local %s", defaultVolName)}


Should I use volume ls -q here to make checking more succinct?

zackattackz · 2024-09-22T15:40:41Z

test/system/160-volumes.bats

@@ -556,7 +556,11 @@ EOF

 @test "podman volume rm --force bogus" {
 run_podman 1 volume rm bogus
- is "$output" "Error: no volume with name \"bogus\" found: no such volume" "Should print error"
+ expectedoutput="Error: no such volume"
+ if [[ "$CI_DESIRED_DATABASE" = "boltdb" ]]; then


Saw this env var used in some other test/system/*.bats files, wasn't sure if it was OK to use it here?

zackattackz · 2024-09-22T15:42:31Z

OK tests are green now. Amended to conditionally check db backend in tests to get expected outpuit. Also amended to add and fix a test in test/system/160-volumes.bats

Luap99

blocking for now as we must verify how docker actually behaves.
Structurally it seems odd to me to only change this for volume rm but not volume inspect or other commands. This inconsistency worries me.
And reading the original issue it seems we have at least one user who prefers the current behavior so we must make a clear decision if we want to "break" this without major release.

Code wise looks good

Luap99 · 2024-09-23T11:41:39Z

test/e2e/volume_rm_test.go


 session = podmanTest.Podman([]string{"volume", "ls"})
 session.WaitWithDefaultTimeout()
 Expect(session).Should(ExitCleanly())
- Expect(len(session.OutputToStringArray())).To(BeNumerically(">=", 2))
+ output := session.OutputToStringArray()
+ expectedLs := []string{"DRIVER VOLUME NAME", fmt.Sprintf("local %s", defaultVolName)}


zackattackz · 2024-09-23T12:46:53Z

Hi @Luap99, thank you for reviewing.

blocking for now as we must verify how docker actually behaves

Sorry forgot to look more into this. From a cursory look at docker volume rm it doesn't seem to allow partial names. But I'll have to look more deeply to see if partial names are a feature anywhere else.

Structurally it seems odd to me to only change this for volume rm but not volume inspect or other commands. This inconsistency worries me

Yes this is a good point, a short look into docker volume inspect it doesn't allow partial names either. (and others)

I think it may be worth considering also removing partial naming from anywhere else too (assuming that docker doesn't have it). The reason is that you are never "certain" that a command you run that expects a partial name actually operated on the correct item unless you are also certain of any of these:

Other ambiguously named items also currently exist (will error out, so it is "safe").
No ambiguously named items will ever exist in your use case. Or if they do, at least two with conflicting ones always exist at a given time (doubtful to me in many scenarios).
You run podman volume exists {name} beforehand and it exits 0. This command expects an exact name, so you could use it to verify that the name you want to operate on is an exact match. This however runs risk of TOCTOU, unless there is some kind of locking method you could use that I'm not aware of. (this is all to emulate exact naming semantics anyways, so exact naming would be much simpler to have to begin with).

If you are not certain of one of the above, you always run the risk of the situation that the item you're expecting to operate on doesn't actually exist, and exactly one matching ambiguously named item does exist, which will be operated on instead.

In that sense, operations that use partial names (anything that uses a LookupX function, such as LookupVolume) are not safe at all to use in workloads with ambiguously named items that are constantly being created/removed.

zackattackz · 2024-09-25T21:44:19Z

To verify how docker behaves:

Calling docker volume rm eventually calls into this Remove method, which calls Get and passes the name.
Where Get calls getVolume
Which you can see here searches for exact match on the name.

All this points to the fact that there is no partial name lookup happening in docker.
If we are to extend this PR to other commands like inspect, etc, I could look into each individually to be sure. But also there are a lot of commands to be looked into 😅

Luap99 · 2024-09-27T12:21:00Z

All this points to the fact that there is no partial name lookup happening in docker.
If we are to extend this PR to other commands like inspect, etc, I could look into each individually to be sure. But also there are a lot of commands to be looked into 😅

Well there isn't a need to test all commands, our logic currently is in LookupVolume() (partial lookup) and GetVolume() (full match). So if docker does full match everywhere we can remove LookupVolume() and use GetVolume() everywhere.
However this is really big into the breaking change territory so I am not sure if we should make such a change right now. It may be best to defer this to the next major version if we really want this as there is always the risk this breaks someone

zackattackz · 2024-09-27T14:31:02Z

Well there isn't a need to test all commands

I meant more so that I'd need to also verify that other docker commands don't use partial names, I only looked into image remove. It probably wouldn't be too bad to figure out though.

I am not sure if we should make such a change right now

Yes that does make sense to defer to a later version. Is there a development branch I should re-target into then? Or where else should I follow to keep up with progress on the next major version, so that I could re-submit this PR when the time is right?

Thank you

openshift-ci bot added the release-note label Sep 21, 2024

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Sep 21, 2024

zackattackz force-pushed the main branch from 94c81fd to 9448163 Compare September 21, 2024 05:32

zackattackz mentioned this pull request Sep 21, 2024

"podman volume rm" behaviour #23943

Open

zackattackz force-pushed the main branch from 9448163 to fdbecec Compare September 21, 2024 05:48

zackattackz force-pushed the main branch 2 times, most recently from 9d385e2 to 68d4318 Compare September 21, 2024 16:00

zackattackz force-pushed the main branch 2 times, most recently from ee8f0f7 to e5af790 Compare September 22, 2024 00:12

edsantiago requested changes Sep 22, 2024

View reviewed changes

zackattackz force-pushed the main branch 3 times, most recently from af5997d to 8bb8f6a Compare September 22, 2024 02:45

zackattackz force-pushed the main branch from 8bb8f6a to 9ae856d Compare September 22, 2024 03:22

zackattackz commented Sep 22, 2024

View reviewed changes

Luap99 requested changes Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

volume rm require exact name #24027

volume rm require exact name #24027

zackattackz commented Sep 21, 2024

openshift-ci bot commented Sep 21, 2024

packit-as-a-service bot commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

rhatdan commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 22, 2024 •

edited

Loading

zackattackz commented Sep 22, 2024

edsantiago left a comment

edsantiago Sep 22, 2024

zackattackz Sep 22, 2024

edsantiago Sep 22, 2024

zackattackz Sep 22, 2024

Luap99 Sep 23, 2024

zackattackz Sep 22, 2024

zackattackz commented Sep 22, 2024

Luap99 left a comment

Luap99 Sep 23, 2024

zackattackz commented Sep 23, 2024 •

edited

Loading

zackattackz commented Sep 25, 2024

Luap99 commented Sep 27, 2024

zackattackz commented Sep 27, 2024

volume rm require exact name #24027

Are you sure you want to change the base?

volume rm require exact name #24027

Conversation

zackattackz commented Sep 21, 2024

Does this PR introduce a user-facing change?

openshift-ci bot commented Sep 21, 2024

packit-as-a-service bot commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 21, 2024

rhatdan commented Sep 21, 2024

zackattackz commented Sep 21, 2024

zackattackz commented Sep 22, 2024 • edited Loading

zackattackz commented Sep 22, 2024

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Sep 22, 2024

Choose a reason for hiding this comment

zackattackz Sep 22, 2024

Choose a reason for hiding this comment

edsantiago Sep 22, 2024

Choose a reason for hiding this comment

zackattackz Sep 22, 2024

Choose a reason for hiding this comment

Luap99 Sep 23, 2024

Choose a reason for hiding this comment

zackattackz Sep 22, 2024

Choose a reason for hiding this comment

zackattackz commented Sep 22, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Sep 23, 2024

Choose a reason for hiding this comment

zackattackz commented Sep 23, 2024 • edited Loading

zackattackz commented Sep 25, 2024

Luap99 commented Sep 27, 2024

zackattackz commented Sep 27, 2024

zackattackz commented Sep 22, 2024 •

edited

Loading

zackattackz commented Sep 23, 2024 •

edited

Loading