fix(agent): solve permission denied when asserting submit dir exist (#713)

The previous approach with submit_dir.exists() runs as the agent user, which may face permission errors on the directory (like /home/fschuch). It is now verified as the submitter user.

(cherry picked from commit 5f6beb7)

Author: fschuch

changes/agent/713.fixed.md

@@ -0,0 +1 @@
+Solved the permissions denied error the agent user could face at submission time when verifying the submission directory exists and is writable

jobbergate-agent/jobbergate_agent/jobbergate/submit.py

@@ -5,10 +5,16 @@
 import pwd
 from dataclasses import dataclass
 from pathlib import Path
+from subprocess import CompletedProcess
 from tempfile import TemporaryDirectory
 
 from buzz import DoExceptParams, handle_errors_async
-from jobbergate_core.tools.sbatch import InfoHandler, SubmissionHandler, SubprocessHandler, inject_sbatch_params
+from jobbergate_core.tools.sbatch import (
+    InfoHandler,
+    SubmissionHandler,
+    SubprocessHandler,
+    inject_sbatch_params,
+)
 from loguru import logger
 
 from jobbergate_agent.clients.cluster_api import backend_client as jobbergate_api_client
@@ -173,7 +179,7 @@ def __post_init__(self):
         self.uid = pwan.pw_uid
         self.gid = pwan.pw_gid
 
-    def run(self, *args, **kwargs):
+    def run(self, *args, **kwargs) -> CompletedProcess:
         kwargs.update(user=self.uid, group=self.gid, env={})
         # Tests indicate that the change on the working directory precedes the change of user on the subprocess.
         # With that, the user running the agent can face permission denied errors on cwd,
@@ -184,6 +190,24 @@ def run(self, *args, **kwargs):
         return super().run(*args, **kwargs)
 
 
+def validate_submit_dir(submit_dir: Path, subprocess_handler: SubprocessAsUserHandler) -> None:
+    """
+    Validate the submission directory.
+
+    The directory must exist and be writable by the user, so this verification is delegated to the subprocess
+    handler as the user that will run the sbatch command.
+
+    This is needed since `submit_dir.exists()` would run as the agent user, which may face permission errors.
+    """
+    if not submit_dir.is_absolute():
+        raise ValueError("Execution directory must be an absolute path")
+    try:
+        subprocess_handler.run(cmd=("test", "-d", submit_dir.as_posix()))
+        subprocess_handler.run(cmd=("test", "-w", submit_dir.as_posix()))
+    except RuntimeError as e:
+        raise ValueError("Execution directory does not exist or is not writable by the user") from e
+
+
 async def submit_job_script(
     pending_job_submission: PendingJobSubmission,
     user_mapper: SlurmUserMapper,
@@ -221,10 +245,10 @@ async def _reject_handler(params: DoExceptParams) -> None:
         subprocess_handler = SubprocessAsUserHandler(username)
 
     submit_dir = pending_job_submission.execution_directory or SETTINGS.DEFAULT_SLURM_WORK_DIR
-    if not submit_dir.exists() or not submit_dir.is_absolute():
-        message = f"The execution directory must exist and be an absolute path, but got '{submit_dir.as_posix()}'"
-        await mark_as_rejected(pending_job_submission.id, message)
-        raise JobSubmissionError(message)
+    async with handle_errors_async(
+        "Execution directory is invalid", raise_exc_class=JobSubmissionError, do_except=_reject_handler
+    ):
+        validate_submit_dir(submit_dir, subprocess_handler)
 
     sbatch_handler = SubmissionHandler(
         sbatch_path=SETTINGS.SBATCH_PATH,
@@ -277,7 +301,7 @@ async def submit_pending_jobs() -> None:
     for pending_job_submission in pending_job_submissions:
         logger.debug(f"Submitting pending job_submission {pending_job_submission.id}")
         with JobSubmissionError.handle_errors(
-            (f"Failed to submit pending job_submission {pending_job_submission.id}" "...skipping to next pending job"),
+            f"Failed to submit pending job_submission {pending_job_submission.id}...skipping to next pending job",
             do_except=log_error,
             do_else=lambda: logger.debug(f"Finished submitting pending job_submission {pending_job_submission.id}"),
             re_raise=False,

jobbergate-agent/tests/jobbergate/test_submit.py

@@ -14,6 +14,7 @@
 
 from jobbergate_agent.jobbergate.schemas import JobScriptFile, PendingJobSubmission, SlurmJobData
 from jobbergate_agent.jobbergate.submit import (
+    SubprocessAsUserHandler,
     fetch_pending_submissions,
     get_job_script_file,
     mark_as_rejected,
@@ -22,6 +23,7 @@
     retrieve_submission_file,
     submit_job_script,
     submit_pending_jobs,
+    validate_submit_dir,
     write_submission_file,
 )
 from jobbergate_agent.settings import SETTINGS
@@ -403,6 +405,52 @@ async def test_mark_as_rejected__raises_JobbergateApiError_if_the_response_is_no
         assert update_route.called
 
 
+class TestValidateSubmitDir:
+    """
+    Test the ``validate_submit_dir()`` function.
+    """
+
+    subprocess_handler = SubprocessAsUserHandler(username=getpass.getuser())
+
+    def test_validate_submit_dir__success(self, tmp_path):
+        """
+        Test that the function can successfully validate a submission directory.
+        """
+        submit_dir = tmp_path / "submit"
+        submit_dir.mkdir()
+
+        validate_submit_dir(submit_dir, self.subprocess_handler)
+
+    def test_validate_submit_dir__raises_exception_if_not_absolute_path(self):
+        """
+        Test that the function raises an exception if the submission directory is not an absolute path.
+        """
+        submit_dir = Path("./submit")
+
+        with pytest.raises(ValueError, match="must be an absolute path"):
+            validate_submit_dir(submit_dir, self.subprocess_handler)
+
+    def test_validate_submit_dir__raises_exception_if_dir_is_not_writable(self, tmp_path):
+        """
+        Test that the function raises an exception if the submission directory is not writable.
+        """
+        submit_dir = tmp_path / "submit"
+        submit_dir.mkdir()
+        submit_dir.chmod(0o444)
+
+        with pytest.raises(ValueError, match="not writable by the user"):
+            validate_submit_dir(submit_dir, self.subprocess_handler)
+
+    def test_validate_submit_dir__raises_exception_if_dir_does_not_exist(self, tmp_path):
+        """
+        Test that the function raises an exception if the submission directory does not exist.
+        """
+        submit_dir = tmp_path / "submit"
+
+        with pytest.raises(ValueError, match="Execution directory does not exist"):
+            validate_submit_dir(submit_dir, self.subprocess_handler)
+
+
 @pytest.mark.asyncio
 @pytest.mark.usefixtures("mock_access_token")
 @pytest.mark.parametrize("job_submissions_name", ["dummy-job-submission", "really-long-job-submission-name" * 10])
@@ -553,12 +601,12 @@ async def test_submit_job_script__raises_exception_if_execution_dir_does_not_exi
     mocked_sbatch = mock.MagicMock()
     mocker.patch("jobbergate_agent.jobbergate.submit.SubmissionHandler", return_value=mocked_sbatch)
 
-    with pytest.raises(JobSubmissionError, match="The execution directory must exist and be an absolute path"):
+    with pytest.raises(JobSubmissionError, match="Execution directory does not exist or is not writable by the user"):
         await submit_job_script(pending_job_submission, user_mapper)
 
     mock_mark_as_rejected.assert_called_once_with(
         dummy_pending_job_submission_data["id"],
-        RegexArgMatcher(".*The execution directory must exist and be an absolute path.*"),
+        RegexArgMatcher(".*Execution directory does not exist or is not writable by the user.*"),
     )
 
 
@@ -577,12 +625,12 @@ async def test_submit_job_script__raises_exception_if_execution_dir_is_relative(
     mocked_sbatch = mock.MagicMock()
     mocker.patch("jobbergate_agent.jobbergate.submit.SubmissionHandler", return_value=mocked_sbatch)
 
-    with pytest.raises(JobSubmissionError, match="The execution directory must exist and be an absolute path"):
+    with pytest.raises(JobSubmissionError, match="Execution directory must be an absolute path"):
         await submit_job_script(pending_job_submission, user_mapper)
 
     mock_mark_as_rejected.assert_called_once_with(
         dummy_pending_job_submission_data["id"],
-        RegexArgMatcher(".*The execution directory must exist and be an absolute path.*"),
+        RegexArgMatcher(".*Execution directory must be an absolute path.*"),
     )