Improve logging with run_as_user to avoid "double" logging/plain-text-over-stdout
#51934
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
Done. |
ashb
force-pushed
the
user-impersonation-log-handling
branch
from
0247226 to
c445203
Compare
…xt-over-stdout When we are running normally (without impersonation) the supervisor sets up a new socketpair for logging before forking, and then the task procees configures structlog in the forked process to send logs over that socket. This all works as forking a process gives the new process a copy of all open file descriptors. However sudo by default will close all open file descriptors other than stdin, stdout and stderr, so our logs socket (sockets, and files, are all file descriptors). We could ask people to change their `sudoers` config file to add the [`closefrom_overide`][1] and invoke `sudo -C <logfd>` however many people either might not have access to do this, or might not feel comfortable in making this change. There is however another option to us: On both unix and windows there is the ability to pass _open_ file descriptors (which remember, sockets are file descriptors) between two processes! So what this PR does is introduce a new Request and Response pair, and customize the send+receive code to send a new FD (since we've already closed the child end for normal start up before we knew the task was actually going to run as another user, and we can't get it back, so we just open another) that is configured to receive and handle JSON logs. [1]: https://linux.die.net/man/5/sudoers#:~:text=on%20by%20default.-,closefrom_override,is%20off%20by%20default.,-compress_io'%20If%20set
ashb
force-pushed
the
user-impersonation-log-handling
branch
from
c445203 to
a9eccd3
Compare
shahar1
approved these changes
Jun 19, 2025
RoyLee1224
pushed a commit
to RoyLee1224/airflow
that referenced
this pull request
Jun 21, 2025
…xt-over-stdout (apache#51934) When we are running normally (without impersonation) the supervisor sets up a new socketpair for logging before forking, and then the task procees configures structlog in the forked process to send logs over that socket. This all works as forking a process gives the new process a copy of all open file descriptors. However sudo by default will close all open file descriptors other than stdin, stdout and stderr, so our logs socket (sockets, and files, are all file descriptors). We could ask people to change their `sudoers` config file to add the [`closefrom_overide`][1] and invoke `sudo -C <logfd>` however many people either might not have access to do this, or might not feel comfortable in making this change. There is however another option to us: On both unix and windows there is the ability to pass _open_ file descriptors (which remember, sockets are file descriptors) between two processes! So what this PR does is introduce a new Request and Response pair, and customize the send+receive code to send a new FD (since we've already closed the child end for normal start up before we knew the task was actually going to run as another user, and we can't get it back, so we just open another) that is configured to receive and handle JSON logs. [1]: https://linux.die.net/man/5/sudoers#:~:text=on%20by%20default.-,closefrom_override,is%20off%20by%20default.,-compress_io'%20If%20set
kaxil
pushed a commit
that referenced
this pull request
Jul 2, 2025
…xt-over-stdout (#51934) When we are running normally (without impersonation) the supervisor sets up a new socketpair for logging before forking, and then the task procees configures structlog in the forked process to send logs over that socket. This all works as forking a process gives the new process a copy of all open file descriptors. However sudo by default will close all open file descriptors other than stdin, stdout and stderr, so our logs socket (sockets, and files, are all file descriptors). We could ask people to change their `sudoers` config file to add the [`closefrom_overide`][1] and invoke `sudo -C <logfd>` however many people either might not have access to do this, or might not feel comfortable in making this change. There is however another option to us: On both unix and windows there is the ability to pass _open_ file descriptors (which remember, sockets are file descriptors) between two processes! So what this PR does is introduce a new Request and Response pair, and customize the send+receive code to send a new FD (since we've already closed the child end for normal start up before we knew the task was actually going to run as another user, and we can't get it back, so we just open another) that is configured to receive and handle JSON logs. [1]: https://linux.die.net/man/5/sudoers#:~:text=on%20by%20default.-,closefrom_override,is%20off%20by%20default.,-compress_io'%20If%20set (cherry picked from commit 348b292)
This was referenced Jul 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we are running normally (without impersonation) the supervisor sets up a new socketpair for logging before forking, and then the task procees configures structlog in the forked process to send logs over that socket. This all works as forking a process gives the new process a copy of all open file descriptors.
However sudo by default will close all open file descriptors other than stdin, stdout and stderr, so our logs socket (sockets, and files, are all file descriptors).
We could ask people to change their
sudoersconfig file to add theclosefrom_overideand invokesudo -C <logfd>however many people either might not have access to do this, or might not feel comfortable in making this change.There is however another option to us: On both unix and windows there is the ability to pass open file descriptors (which remember, sockets are file descriptors) between two processes!
So what this PR does is introduce a new Request and Response pair, and customize the send+receive code to send a new FD (since we've already closed the child end for normal start up before we knew the task was actually going to run as another user, and we can't get it back, so we just open another) that is configured to receive and handle JSON logs.
Relates to #51780
Enough with the words, lets see what it does.
Logging from a task
Before
Note the double timestamp and level etc (one formatted nicely by the UI, the other in the log message etc)
After
The two highlighted sections are the same source in each task run
Unhandled exception
Now on to the real "ick". An unhandled exception in an operator/task:
Before
Ick. Not helped by rich at all here
After