fixing #19028 by moving chown to use sudo

[plockaby]

Without this change, using user impersonation requires that Airflow run as root or it won't work at all.

closes: #19028
related: #15947

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

• Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[ashb]

Suggested change

	subprocess.call(['sudo', 'chown', self.run_as_user, self._error_file.name], close_fds=True)
	subprocess.check_call(['sudo', 'chown', self.run_as_user, self._error_file.name], close_fds=True)

I think

[plockaby]

I agree that you're probably right it should be check_call. I simply stole the the subprocess call that runs a chown a few statements above so maybe they should both change?

[ashb]

Yeah -- could you change them both?

[ashb]

Refactor to a single sudo chown call (hmm, sorry. I was trying github.dev. This didn't do what I expected it to)

[ashb]

We can make it a single sudo chwon call like this:

diff --git a/airflow/task/task_runner/base_task_runner.py b/airflow/task/task_runner/base_task_runner.py
--- airflow/task/task_runner/base_task_runner.py
+++ airflow/task/task_runner/base_task_runner.py
@@ -56,8 +56,11 @@
                 self.run_as_user = conf.get('core', 'default_impersonation')
             except AirflowConfigException:
                 self.run_as_user = None
 
+
+        self._error_file = NamedTemporaryFile(delete=True)
+
         # Add sudo commands to change user if we need to. Needed to handle SubDagOperator
         # case using a SequentialExecutor.
         self.log.debug("Planning to run as the %s user", self.run_as_user)
         if self.run_as_user and (self.run_as_user != getuser()):
@@ -67,9 +70,9 @@
             # might not be able to run the cmds to get credentials
             cfg_path = tmp_configuration_copy(chmod=0o600, include_env=True, include_cmds=True)
 
             # Give ownership of file to user; only they can read and write
-            subprocess.call(['sudo', 'chown', self.run_as_user, cfg_path], close_fds=True)
+            subprocess.check_call(['sudo', 'chown', self.run_as_user, cfg_path, self._error_file.name], close_fds=True)
 
             # propagate PYTHONPATH environment variable
             pythonpath_value = os.environ.get(PYTHONPATH_VAR, '')
             popen_prepend = ['sudo', '-E', '-H', '-u', self.run_as_user]
@@ -83,11 +86,8 @@
             # variables then we don't need to include those in the config copy
             # - the runner can read/execute those values as it needs
             cfg_path = tmp_configuration_copy(chmod=0o600, include_env=False, include_cmds=False)
 
-        self._error_file = NamedTemporaryFile(delete=True)
-        if self.run_as_user:
-            subprocess.call(['sudo', 'chown', self.run_as_user, self._error_file.name], close_fds=True)
 
         self._cfg_path = cfg_path
         self._command = (
             popen_prepend

[plockaby]

I answered your message and stepped out to a meeting and then you fixed my whole PR. 😆

Looks good to me! Thanks for your help!

[uranusjr]

One downside to this change is it requires sudo to exist (it is not guaranteed), which is unnecessary if the Airflow’s runtime user does have permission to chown. Maybe it’s a better idea to use pwm first, and fall back to sudo only on PermissionError? It should also be worthwhile to catch FileNotFoundError from the subprocess call (which would mean sudo does not exist) and emit a more friendly error (or just carry on silently).

[plockaby]

You're right, @uranusjr , that there are improvements that could be made to this. My first priority is addressing the regression quickly by getting it back to the previous functional state. Then perhaps future enhancements or improvements could be proposed or implemented.

[plockaby]

One thing that I do notice when I run my patch on my own system is this error:

[2021-12-08 06:31:05,703: ERROR/ForkPoolWorker-31] Failed to execute task [Errno 1] Operation not permitted: '/tmp/tmp9g_3zc5j'.
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/airflow/executors/celery_executor.py", line 121, in _execute_in_fork
    args.func(args)
  File "/usr/local/lib/python3.9/site-packages/airflow/cli/cli_parser.py", line 48, in command
    return func(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/airflow/utils/cli.py", line 92, in wrapper
    return f(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/airflow/cli/commands/task_command.py", line 292, in task_run
    _run_task_by_selected_method(args, dag, ti)
  File "/usr/local/lib/python3.9/site-packages/airflow/cli/commands/task_command.py", line 105, in _run_task_by_selected_method
    _run_task_by_local_task_job(args, ti)
  File "/usr/local/lib/python3.9/site-packages/airflow/cli/commands/task_command.py", line 163, in _run_task_by_local_task_job
    run_job.run()
  File "/usr/local/lib/python3.9/site-packages/airflow/jobs/base_job.py", line 245, in run
    self._execute()
  File "/usr/local/lib/python3.9/site-packages/airflow/jobs/local_task_job.py", line 148, in _execute
    self.on_kill()
  File "/usr/local/lib/python3.9/site-packages/airflow/jobs/local_task_job.py", line 174, in on_kill
    self.task_runner.on_finish()
  File "/usr/local/lib/python3.9/site-packages/airflow/task/task_runner/base_task_runner.py", line 183, in on_finish
    self._error_file.close()
  File "/usr/local/lib/python3.9/tempfile.py", line 504, in close
    self._closer.close()
  File "/usr/local/lib/python3.9/tempfile.py", line 441, in close
    unlink(self.name)
PermissionError: [Errno 1] Operation not permitted: '/tmp/tmp9g_3zc5j'

But it doesn't impact the failure state (the task still succeeds) and it was very likely happening before I got here, too. It means that files in /tmp get left hanging out after the task finishes. So we might need to add a chmod here, before the chown, so that the file is readable.

[uranusjr]

The problem is that you’re prioritising your use case over others, and potentially breaking other people’s environments to make your environment work correctly. That’s not how a regression should be addressed.

[plockaby]

I mean the docs are very clear that when using impersonation you have to configure sudo and even tell you how to configure sudo. I'm trying to touch as little code as possible here and not introduce new features while doing so.

[plockaby]

Oh and I may add that the use case you put forth -- people using impersonation without sudo configured or installed -- does not exist. The current code uses a combination of sudo and calls to os which only works for people who run airflow as root. This change is to standardize on just sudo, which is what it was before 2.2.0 and before #15947 broke it for everyone not running airflow as root.

[potiuk]

Yep. I agree with @plockaby - we clearly expect sudo to be available https://airflow.apache.org/docs/apache-airflow/stable/security/workload.html?highlight=impersonation (BTW. This page is very badly named)

We might improve it in the future as separate PR and remove the requirement, but lets's fix what is broken currently first.

[github-actions]

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

[boring-cyborg]

Awesome work, congrats on your first merged pull request!