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.

Sign up for GitHub

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

Jump to bottom

libpod: skip DBUS_SESSION_BUS_ADDRESS in conmon #20425

Merged
merged 1 commit into from
Oct 21, 2023
Merged

libpod: skip DBUS_SESSION_BUS_ADDRESS in conmon #20425

merged 1 commit into from
Oct 21, 2023

Conversation

giuseppe
Copy link
Member

commit 7ade972 introduced the change that caused an issue in crun since it forces the root user session instead of the system one when DBUS_SESSION_BUS_ADDRESS is set.

I am addressing it in crun, but for the time being, let's also not pass the variable down to conmon since the assumption is that when running as root the containers must be created on the system bus.

[NO NEW TESTS NEEDED]

the crun PR: containers/crun#1328

Does this PR introduce a user-facing change?

None

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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 Oct 20, 2023
@giuseppe giuseppe mentioned this pull request Oct 20, 2023
Closed
@giuseppe
Copy link
Member Author

@vrothberg PTAL

vrothberg
vrothberg reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Code LGTM
Could you extend the test (7ade972#diff-78c7444aee8aa212e7101395b618015dec2d5d1eab06fb7de2a8ecdf7418cde4) to make sure we won't regress again?

@giuseppe giuseppe force-pushed the podman-do-not-leak-DBUS_SESSION_BUS_ADDRESS-into-conmon branch from b5f95b6 to 6cb3081 Compare October 20, 2023 11:26
@giuseppe
Copy link
Member Author

Code LGTM Could you extend the test (7ade972#diff-78c7444aee8aa212e7101395b618015dec2d5d1eab06fb7de2a8ecdf7418cde4) to make sure we won't regress again?

sure, added a test

vrothberg
vrothberg reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

test/system/030-run.bats Outdated Show resolved Hide resolved
Luap99
Luap99 reviewed Oct 20, 2023
View reviewed changes
libpod/oci_conmon_common.go Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the podman-do-not-leak-DBUS_SESSION_BUS_ADDRESS-into-conmon branch from 6cb3081 to c1896d0 Compare October 20, 2023 11:33
@rhatdan
Copy link
Member

rhatdan commented Oct 20, 2023

/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 Oct 20, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2023
@giuseppe giuseppe force-pushed the podman-do-not-leak-DBUS_SESSION_BUS_ADDRESS-into-conmon branch from c1896d0 to 09ca548 Compare October 20, 2023 12:58
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2023
vrothberg
vrothberg reviewed Oct 20, 2023
View reviewed changes
test/system/030-run.bats Outdated
@@ -1298,10 +1298,18 @@ search | $IMAGE |
run_podman run -d -q --syslog $IMAGE sleep infinity
cid="$output"

# The CIRRUS_CHANGE_TITLE env variable should not affect the tests below:
unset CIRRUS_CHANGE_TITLE
Copy link
Member

Choose a reason for hiding this comment

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

@giuseppe how did this variable affect the test? I'm trying to understand it but failed to see a connection

Copy link
Member Author

Choose a reason for hiding this comment

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

it contained the title of the PR "libpod: skip DBUS_SESSION_BUS_ADDRESS in conmon" so DBUS_SESSION_BUS_ADDRESS was still found in the environment variables

Copy link
Member

Choose a reason for hiding this comment

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

:-D that's hilarious in a way. Thanks, makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

if I need to repush, I'll also change the check for DBUS_SESSION_BUS_ADDRESS= instead of DBUS_SESSION_BUS_ADDRESS

Copy link
Member Author

Choose a reason for hiding this comment

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

it passes now

@giuseppe
libpod: skip DBUS_SESSION_BUS_ADDRESS in conmon
03947ab 
commit 7ade972 introduced the change
that caused an issue in crun since it forces the root user session
instead of the system one when DBUS_SESSION_BUS_ADDRESS is set.

I am addressing it in crun, but for the time being, let's also not
pass the variable down to conmon since the assumption is that when
running as root the containers must be created on the system bus.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the podman-do-not-leak-DBUS_SESSION_BUS_ADDRESS-into-conmon branch from 09ca548 to 03947ab Compare October 20, 2023 14:06
@packit-as-a-service
Copy link

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

@Luap99
Copy link
Member

Luap99 commented Oct 20, 2023

/lgtm
/hold

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2023
edsantiago
edsantiago reviewed Oct 21, 2023
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

LGTM but will defer final slash-lgtm to someone who better understands the logic here

test/system/030-run.bats
@@ -1301,7 +1301,12 @@ search | $IMAGE |
run_podman container inspect $cid --format "{{ .State.ConmonPid }}"
conmon_pid="$output"
is "$(< /proc/$conmon_pid/cmdline)" ".*--exit-command-arg--syslog.*" "conmon's exit-command has --syslog set"
assert "$(< /proc/$conmon_pid/environ)" =~ "BATS_TEST_TMPDIR" "entire env is passed down to conmon (incl. BATS variables)"
conmon_env="$(< /proc/$conmon_pid/environ)"
Copy link
Member

Choose a reason for hiding this comment

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

On a future iteration this might be nicer: conmon_env=$(sed -z -e 's/=.*/\n/g' </proc/$conmon_pid/environ) which gives you one envariable per line. No need for repush though.

test/system/030-run.bats
assert "$(< /proc/$conmon_pid/environ)" =~ "BATS_TEST_TMPDIR" "entire env is passed down to conmon (incl. BATS variables)"
conmon_env="$(< /proc/$conmon_pid/environ)"
assert "$conmon_env" =~ "BATS_TEST_TMPDIR" "entire env is passed down to conmon (incl. BATS variables)"
assert "$conmon_env" !~ "NOTIFY_SOCKET=" "NOTIFY_SOCKET is not included (incl. BATS variables)"
Copy link
Member

Choose a reason for hiding this comment

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

the incl part makes no sense on this line, nor 1308, but again I won't insist on a repush.

@rhatdan
Copy link
Member

rhatdan commented Oct 21, 2023

/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 Oct 21, 2023
@openshift-ci openshift-ci bot merged commit 19c870d into containers:main Oct 21, 2023
98 checks passed
@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 Jan 20, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg left review comments

@edsantiago edsantiago edsantiago left review comments

Assignees

@rhatdan rhatdan

@Luap99 Luap99

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@giuseppe @rhatdan @Luap99 @edsantiago @vrothberg