logging: new mode -l passthrough #11390

giuseppe · 2021-09-01T13:11:23Z

it allows to pass the current std streams down to the container.

conmon support: containers/conmon#289

[NO TESTS NEEDED] it needs a new conmon.

Signed-off-by: Giuseppe Scrivano [email protected]

openshift-ci · 2021-09-01T13:11:39Z

[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:

~~OWNERS~~ [giuseppe]

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

giuseppe · 2021-09-01T13:17:22Z

@vrothberg what do you think?

This should improve integration with systemd (but some commands won't work)

TomSweeneyRedHat · 2021-09-01T15:12:52Z

Changes LGTM
but tests are an ugly red.

mheon · 2021-09-01T15:38:37Z

cmd/podman/containers/run.go

@@ -200,7 +200,7 @@ func run(cmd *cobra.Command, args []string) error {
 		return err
 	}

-	if runOpts.Detach {
+	if runOpts.Detach && cliVals.LogDriver != define.PassthroughLogging {


Shouldn't these conflict? It doesn't seem particularly sane to log to the terminal's STDOUT and STDERR when the process is no longer attached to the controlling terminal.

the fds are passed down to the container that can still use them.

I think the only sane usage of this option is within a systemd service.

Using a tty won't be allowed by conmon as it opens to security issues: https://github.com/containers/conmon/pull/289/files#diff-26c58c117a24670c396f3e8e3e86cd6a6f134b3f02f84f8c894213c16c542d4cR165-R166

Can we get documentation to that effect? I was very confused by what this option was for.

Also, I think conflicting with --tty makes a lot of sense - we won't have split stderr/stdout, and won't have raw mode set so the output will likely be garbage.

I like Matt's suggestion

I added the error also to Podman so we don't rely on conmon checking for it

cmd/podman/containers/create.go

giuseppe · 2021-09-02T07:40:08Z

converted to draft. Let's wait for the conmon change to be accepted first

alexlarsson · 2021-09-03T09:05:38Z

Ok, I played with this by launching podman with the passthrough log driver from a systemd service, and it works just like I expected. For example, it properly picks up the syslog identifiers set by the systemd unit, and the individual journal entries are tagged with the correct pid, cmdline and exe (i.e. not everything looks like it came from conmon).

The one issue I have is that the systemd journal connection has a system_u:system_r:init_t:s0 selinux context and the container is running in system_u:system_r:container_runtime_t:s0, so I'm getting AVC denied when writing to the journal breaking selinux enforcing mode. I wonder what is the best way to handle this. @rhatdan opinions?

alexlarsson · 2021-09-03T09:06:53Z

These are the AVCs i get:

   type=AVC msg=audit(1630657057.309:47775): avc:  denied  { ioctl } for  pid=1312362 comm="bash" path="socket:[7069215]" dev="sockfs" ino=7069215 ioctlcmd=0x540f scontext=system_u:system_r:container_t:s0:c139,c179 tcontext=system_u:system_r:init_t:s0 tclass=unix_stream_socket permissive=1
   type=AVC msg=audit(1630657057.319:47776): avc:  denied  { getattr } for  pid=1312362 comm="autobus.py" path="socket:[7069215]" dev="sockfs" ino=7069215 scontext=system_u:system_r:container_t:s0:c139,c179 tcontext=system_u:system_r:init_t:s0 tclass=unix_stream_socket permissive=1

rhatdan · 2021-09-04T13:26:13Z

This should be fixed in container-selinux.

rhatdan · 2021-09-04T13:53:46Z

Looks like this is allowed in current container-selinux.

container-selinux-2.167

$ audit2allow -i /tmp/t

#============= container_t ==============

#!!!! This avc is allowed in the current policy
allow container_t init_t:unix_stream_socket getattr;

alexlarsson · 2021-09-06T07:57:56Z

Ok. I'm only on container-selinux-2.164.2-1.fc34 atm.

rhatdan · 2021-09-07T20:41:14Z

@alexlarsson Have you updated to the latest container-selinux to the latest version to make sure it works.

cmd/podman/common/completion.go

it allows to pass the current std streams down to the container. conmon support: containers/conmon#289 [NO TESTS NEEDED] it needs a new conmon. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2021-09-29T15:02:48Z

this is ready

Luap99 · 2021-09-29T15:05:09Z

Can you add a test?

giuseppe · 2021-09-29T15:09:41Z

the issue is that it needs a new conmon which is not available in the CI

rhatdan · 2021-09-29T17:27:34Z

Lets get this in, and then add tests when the new conmon is ready. Then the people who need this IE @alexlarsson can start playing with it now, and see if there are any issues.
LGTM

TomSweeneyRedHat · 2021-09-29T19:31:45Z

LGTM

TomSweeneyRedHat · 2021-09-29T19:31:56Z

/lgtm

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2021

mheon reviewed Sep 1, 2021

View reviewed changes

Luap99 reviewed Sep 2, 2021

View reviewed changes

cmd/podman/containers/create.go Outdated Show resolved Hide resolved

giuseppe force-pushed the logging-passthrough branch from fda5a63 to 37929c6 Compare September 2, 2021 07:39

giuseppe marked this pull request as draft September 2, 2021 07:39

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2021

giuseppe force-pushed the logging-passthrough branch 3 times, most recently from df7aae1 to bead1cd Compare September 2, 2021 09:45

giuseppe force-pushed the logging-passthrough branch from bead1cd to 401583c Compare September 9, 2021 12:58

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2021

giuseppe force-pushed the logging-passthrough branch from 401583c to d9f1d3d Compare September 20, 2021 09:38

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2021

Luap99 reviewed Sep 20, 2021

View reviewed changes

cmd/podman/common/completion.go Outdated Show resolved Hide resolved

giuseppe force-pushed the logging-passthrough branch from d9f1d3d to 887d8a3 Compare September 20, 2021 10:06

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2021

logging: new mode -l passthrough

3ce98a5

it allows to pass the current std streams down to the container. conmon support: containers/conmon#289 [NO TESTS NEEDED] it needs a new conmon. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the logging-passthrough branch from 887d8a3 to 3ce98a5 Compare September 27, 2021 10:13

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2021

giuseppe marked this pull request as ready for review September 29, 2021 15:02

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2021

openshift-ci bot assigned TomSweeneyRedHat Sep 29, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2021

openshift-merge-robot merged commit b187dfe into containers:main Sep 29, 2021

alexlarsson mentioned this pull request Sep 8, 2022

Better settings for running podman containers under systemd #15686

Closed

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 Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logging: new mode -l passthrough #11390

logging: new mode -l passthrough #11390

giuseppe commented Sep 1, 2021

openshift-ci bot commented Sep 1, 2021

giuseppe commented Sep 1, 2021 •

edited

Loading

TomSweeneyRedHat commented Sep 1, 2021

mheon Sep 1, 2021

giuseppe Sep 1, 2021

mheon Sep 1, 2021

vrothberg Sep 2, 2021

giuseppe Sep 2, 2021

giuseppe commented Sep 2, 2021

alexlarsson commented Sep 3, 2021

alexlarsson commented Sep 3, 2021 •

edited

Loading

rhatdan commented Sep 4, 2021

rhatdan commented Sep 4, 2021

alexlarsson commented Sep 6, 2021

rhatdan commented Sep 7, 2021

giuseppe commented Sep 29, 2021

Luap99 commented Sep 29, 2021

giuseppe commented Sep 29, 2021

rhatdan commented Sep 29, 2021

TomSweeneyRedHat commented Sep 29, 2021

TomSweeneyRedHat commented Sep 29, 2021

logging: new mode -l passthrough #11390

logging: new mode -l passthrough #11390

Conversation

giuseppe commented Sep 1, 2021

openshift-ci bot commented Sep 1, 2021

giuseppe commented Sep 1, 2021 • edited Loading

TomSweeneyRedHat commented Sep 1, 2021

mheon Sep 1, 2021

Choose a reason for hiding this comment

giuseppe Sep 1, 2021

Choose a reason for hiding this comment

mheon Sep 1, 2021

Choose a reason for hiding this comment

vrothberg Sep 2, 2021

Choose a reason for hiding this comment

giuseppe Sep 2, 2021

Choose a reason for hiding this comment

giuseppe commented Sep 2, 2021

alexlarsson commented Sep 3, 2021

alexlarsson commented Sep 3, 2021 • edited Loading

rhatdan commented Sep 4, 2021

rhatdan commented Sep 4, 2021

alexlarsson commented Sep 6, 2021

rhatdan commented Sep 7, 2021

giuseppe commented Sep 29, 2021

Luap99 commented Sep 29, 2021

giuseppe commented Sep 29, 2021

rhatdan commented Sep 29, 2021

TomSweeneyRedHat commented Sep 29, 2021

TomSweeneyRedHat commented Sep 29, 2021

giuseppe commented Sep 1, 2021 •

edited

Loading

alexlarsson commented Sep 3, 2021 •

edited

Loading