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

use rootless netns from c/common #20772

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Nov 24, 2023

Use the new rootlessnetns logic from c/common, drop the podman code
here and make use of the new much simpler API.

ref: containers/common#1761

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Nov 24, 2023
Copy link
Contributor

openshift-ci bot commented Nov 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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 Nov 24, 2023
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@Luap99 Luap99 force-pushed the rootlessnetns branch 4 times, most recently from f8ccfe3 to c5e30c5 Compare November 27, 2023 14:10
@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

@Luap99 Still valid?

@Luap99
Copy link
Member Author

Luap99 commented Dec 6, 2023

@dfr Could you try if these changes still work on freebsd? I had to move a bunch of cgroups definitions around and I am not sure if I might broke something on freebsd.

@Luap99 Luap99 changed the title WIP: use rootless netns from c/common use rootless netns from c/common Dec 6, 2023
@Luap99 Luap99 marked this pull request as ready for review December 6, 2023 14:52
@openshift-ci openshift-ci bot added release-note-none and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Dec 6, 2023
@Luap99
Copy link
Member Author

Luap99 commented Dec 6, 2023

@baude @mheon @rhatdan PTAL

@dfr
Copy link
Contributor

dfr commented Dec 7, 2023

@dfr Could you try if these changes still work on freebsd? I had to move a bunch of cgroups definitions around and I am not sure if I might broke something on freebsd.

SetupRootless causes things like podman run to fail. Perhaps the call site could be conditional on uid!=0 or SetupRootless could return nil. Otherwise everything seems to work well.

@Luap99
Copy link
Member Author

Luap99 commented Dec 7, 2023

SetupRootless causes things like podman run to fail. Perhaps the call site could be conditional on uid!=0 or SetupRootless could return nil. Otherwise everything seems to work well.

Thanks for checking, should I just make it return nil for freebsd then? There is only linux specific code in there.

@dfr
Copy link
Contributor

dfr commented Dec 7, 2023

SetupRootless causes things like podman run to fail. Perhaps the call site could be conditional on uid!=0 or SetupRootless could return nil. Otherwise everything seems to work well.

Thanks for checking, should I just make it return nil for freebsd then? There is only linux specific code in there.

I'm just reading the linux version now. I wondered if it might be better to return nil only if !rootless.IsRootless() which would give a better error for attempts to run as non root. If I change SetupRootless to return nil and run as non-root, I get this:

$ ./bin/podman --log-level=debug run -ti --rm registry.lab.rabson.org/freebsd13.2-minimal sh
INFO[0000] ./bin/podman filtering at log level debug
DEBU[0000] Called run.PersistentPreRunE(./bin/podman --log-level=debug run -ti --rm registry.lab.rabson.org/freebsd13.2-minimal sh)
DEBU[0000] Using conmon: "/usr/local/bin/conmon"
Error: stat /var/db/containers/storage/libpod/bolt_state.db: permission denied
DEBU[0000] Shutting down engines

Edit: I tried returning nil if !rootless.isRootless() and I still get the confusing 'permission denied' for non-root and working as expected for root.

@dfr
Copy link
Contributor

dfr commented Dec 7, 2023

Edit: I tried returning nil if !rootless.isRootless() and I still get the confusing 'permission denied' for non-root and working as expected for root.

The 'permission denied' happens during the call to registry.NewImageEngine():

(dlv) bt
 0  0x0000000002a5d399 in github.com/containers/podman/v4/libpod.getDBState
    at ./libpod/runtime.go:320
 1  0x0000000002a5e0f0 in github.com/containers/podman/v4/libpod.makeRuntime
    at ./libpod/runtime.go:379
 2  0x0000000002a5bdff in github.com/containers/podman/v4/libpod.newRuntimeFromConfig
    at ./libpod/runtime.go:228
 3  0x0000000002a5b485 in github.com/containers/podman/v4/libpod.NewRuntime
    at ./libpod/runtime.go:171
 4  0x0000000002c8b14c in github.com/containers/podman/v4/pkg/domain/infra.getRuntime
    at ./pkg/domain/infra/runtime_libpod.go:270
 5  0x0000000002c88d93 in github.com/containers/podman/v4/pkg/domain/infra.GetRuntime.func1
    at ./pkg/domain/infra/runtime_libpod.go:82
 6  0x00000000012cae98 in sync.(*Once).doSlow
    at /usr/local/go120/src/sync/once.go:74
 7  0x00000000012cad25 in sync.(*Once).Do
    at /usr/local/go120/src/sync/once.go:65
 8  0x0000000002c88c29 in github.com/containers/podman/v4/pkg/domain/infra.GetRuntime
    at ./pkg/domain/infra/runtime_libpod.go:81
 9  0x0000000002c8caa8 in github.com/containers/podman/v4/pkg/domain/infra.NewLibpodImageRuntime
    at ./pkg/domain/infra/runtime_proxy.go:25
10  0x0000000002c870ee in github.com/containers/podman/v4/pkg/domain/infra.NewImageEngine
    at ./pkg/domain/infra/runtime_abi.go:34
11  0x0000000002c8eaf6 in github.com/containers/podman/v4/cmd/podman/registry.NewImageEngine
    at ./cmd/podman/registry/registry.go:50
12  0x0000000002fc2a8a in main.persistentPreRunE
    at ./cmd/podman/root.go:306
13  0x0000000001b798f7 in github.com/spf13/cobra.(*Command).execute
    at ./vendor/github.com/spf13/cobra/command.go:954
14  0x0000000001b7ad3b in github.com/spf13/cobra.(*Command).ExecuteC
    at ./vendor/github.com/spf13/cobra/command.go:1115
15  0x0000000001b7a1d8 in github.com/spf13/cobra.(*Command).Execute
    at ./vendor/github.com/spf13/cobra/command.go:1039
16  0x0000000001b7a136 in github.com/spf13/cobra.(*Command).ExecuteContext
    at ./vendor/github.com/spf13/cobra/command.go:1032
17  0x0000000002fc0616 in main.Execute
    at ./cmd/podman/root.go:115
18  0x0000000002fbf765 in main.main
    at ./cmd/podman/main.go:60
19  0x0000000001284a13 in runtime.main
    at /usr/local/go120/src/runtime/proc.go:250
20  0x00000000012b9461 in runtime.goexit
    at /usr/local/go120/src/runtime/asm_amd64.s:1598
(dlv) p err
error(*io/fs.PathError) *{
	Op: "stat",
	Path: "/var/db/containers/storage/libpod/bolt_state.db",
	Err: error(syscall.Errno) EACCES (13),}

Use the new rootlessnetns logic from c/common, drop the podman code
here and make use of the new much simpler API.

ref: containers/common#1761

[NO NEW TESTS NEEDED]

Signed-off-by: Paul Holzinger <[email protected]>
There is no point in calling into cgroup specific code as freebsd does
not support cgroups.

Signed-off-by: Paul Holzinger <[email protected]>
So that we do not cause compile error on freebsd.

Signed-off-by: Paul Holzinger <[email protected]>
So that we do not cause compile errors on freebsd.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
Copy link
Member Author

Luap99 commented Dec 7, 2023

Thanks, I changed it to return nil then. I leave it to you to return a better rootless error for freebsd.

@dfr
Copy link
Contributor

dfr commented Dec 7, 2023

Thanks, I changed it to return nil then. I leave it to you to return a better rootless error for freebsd.

Ok, I can do that. I just built your latest version and it works just fine when run as root on freebsd.

@Luap99
Copy link
Member Author

Luap99 commented Dec 8, 2023

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented Dec 8, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 44a9371 into containers:main Dec 8, 2023
85 of 94 checks passed
@Luap99 Luap99 deleted the rootlessnetns branch December 8, 2023 14:30
@dfr
Copy link
Contributor

dfr commented Dec 9, 2023

It turns out that the confusing error from podman run when run as non-root was a minor regression caused by a change in c/storage and was unrelated to this PR.

@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 Mar 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 9, 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. 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-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants