Fix SSHRemoteJobOperator cleanup failing with custom remote_base_dir - #69885
Conversation
|
Heads-up: #70091 merged a few minutes ago and has put this into conflict — apologies, I reviewed them in sequence and the overlap only became visible once the first landed. Both PRs modify The resolution should be mechanical — keep your signature change and On the fix itself, which I did review: the diagnosis is right. No review comments beyond the rebase — happy to look again once it's green. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
0095a4f to
ea0a187
Compare
potiuk
left a comment
There was a problem hiding this comment.
This is the right fix, and of the three PRs opened for this bug it has the strongest tests — it's the only one that pins the negative case on both POSIX and Windows, i.e. that a job dir outside the custom base is still rejected. On a guard function the interesting behaviour is what it refuses, so that matters.
Using (base_dir or DEFAULT) rather than an is None check is also the better failure mode: an empty base dir falls back to the safe default instead of producing a prefix of / that almost any absolute path satisfies. Not reachable today, since _validate_base_dir rejects empty at execute time, but it is the right way round for a security check.
#70091 landed on main while this was open and rewrote the same POSIX command builders to use shlex.quote, which put this branch into conflict. That was merged underneath you, so I have resolved it rather than sending it back:
build_posix_cleanup_commandnow takes both changes —_validate_job_dir(job_dir, "posix", base_dir)together withshlex.quote(job_dir).- Your four
test_remote_job.pytests and theTestPosixPathQuotingclass from #70091 are both kept; they were adjacent additions rather than a genuine conflict. test_cleanup_with_custom_remote_base_dirassertedrm -rf '/tmp-data/...'with hand-written quotes.shlex.quoteleaves a path containing no shell metacharacters unquoted, so that assertion needed updating to the unquoted form.
I checked the merged result keeps both properties rather than just compiling:
custom base accepted : rm -rf /data/jobs/job_1
outside base rejected: Invalid job directory '/tmp/airflow-ssh-jobs/j'...
injection quoted : rm -rf '/tmp/airflow-ssh-jobs/x'"'"'; touch /tmp/pwned; :'"'"''
default base works : rm -rf /tmp/airflow-ssh-jobs/j
58 tests pass in the ssh unit suite.
Two other PRs were opened for this same bug — #70788 and #70802 — and I am closing both in favour of this one.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
The cleanup command builders validated the job directory against the hardcoded default base directory, so any job started with a custom remote_base_dir ran to completion and then failed its cleanup step with "Invalid job directory ... Expected path under '/tmp/airflow-ssh-jobs'". Thread the operator's configured remote_base_dir through build_posix_cleanup_command / build_windows_cleanup_command into _validate_job_dir, falling back to the OS default when unset, so the safety check still constrains the rm target to the directory the job was actually created under. Signed-off-by: Harsh Gupta <harshgupta93198@gmail.com>
ea0a187 to
d617cd4
Compare
Fixes
SSHRemoteJobOperatorfailing at the cleanup step whenever a customremote_base_diris used.The operator accepts (and validates) a custom
remote_base_dirat construction time and creates the job directory under it, butbuild_posix_cleanup_command/build_windows_cleanup_commandvalidated the job directory against the hardcoded default base directory. Result: the remote job runs to completion, then the task fails withFix: thread the operator's configured
remote_base_dirthrough the cleanup builders into_validate_job_dir, falling back to the OS-specific default when unset. The safety property is preserved — therm -rftarget must still live under the base directory the job was actually created under (which was itself validated by_validate_base_dirat operator construction). Both the sync and deferrable (execute_complete) cleanup paths go through the fixed_cleanup_remote_job.Tests: new unit tests for custom-base-dir acceptance and rejection (posix + windows), plus an operator-level regression test reproducing the exact failure from the issue — verified it fails on
mainand passes with this change. Full ssh provider unit suite: 49 passed.closes: #69813
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.