-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cylc clean - initial implementation #3961
Conversation
And prevent symlink attack
- Ensure passed "reg" is not a path that points to the cylc-run dir or higher - Ensure symlinks to be deleted have expected form i.e. `something/cylc-run/reg/log` etc
19d062d
to
6413632
Compare
Should the docs update come later as part of the 7-to-8 guide? |
cylc/flow/suite_files.py
Outdated
if os.path.isdir(target_top_parent): | ||
remove_dir(target_top_parent) | ||
run_dir_top_parent = get_suite_run_dir(Path(reg).parts[0]) | ||
remove_dir(run_dir_top_parent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, we don't want to remove the entire hierarchy in the process of removing something a the bottom of the hierarchy.
For example here are two flows, a/b/c
and a/d
:
$ tree a
a
|-- b
| `-- c
| `-- suite.rc
`-- d
`-- suite.rc
If removing one flow, the other should remain:
$ cylc clean a/b/c
$ tree a
a
`-- d
`-- suite.rc
Though we can remove any empty dirs between the REG and ~/cylc-run
:
$ cylc clean a/d
$ tree a
Aside: We can extend this at some point to discover flows in a hierarchy using the suite detecting "scan" logic already implemented:
$ tree a
a
|-- b
| `-- c
| `-- suite.rc
`-- d
`-- suite.rc
$ cylc clean a
$ tree a -r
8d1ec14
to
7c6b385
Compare
""" | ||
_validate_reg(reg) | ||
reg = os.path.normpath(reg) | ||
if reg.startswith('.'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW the suite name validator has you covered here:
cylc-flow/cylc/flow/unicode_rules.py
Line 190 in 80bb050
not_starts_with(r'\.', r'\-'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is I'm checking after normalising the path, because you could have cylc clean foo/../..
which normalises to cylc clean ..
, i.e. rm $HOME
. However, I guess I could change the order to
reg = os.path.normpath(reg)
_validate_reg(reg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should accept path manipulations, though I'm unaware of a simple method to block them (testing for ..
doesn't work for all cases).
Probs good enough to ensure that the flow exists within the cylc-run dir:
reg = Path(reg)
reg.resolve()
try:
reg.relative_to(run_dir_path)
except ValueError:
raise Exception('Flow not found')
Anyway, not a problem for this PR.
raise WorkflowFilesError( | ||
f'Invalid Cylc symlink directory {path} -> {target}\n' | ||
f'Target is not a directory') | ||
expected_end = str(Path('cylc-run', reg, name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str(Path(
😆
sed -i '$d' "${TEST_NAME}.stdout" | ||
# Note: backticks need to be escaped in the heredoc | ||
cmp_ok "${TEST_NAME}.stdout" << __TREE__ | ||
${TEST_DIR}/sym-cycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice human readable test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read code, checked this branch out and run tests. I have one comment that needs addressing, other than that I'm pretty sure it is good to go!
${TEST_DIR}/sym-log | ||
\`-- cylc-run | ||
\`-- cylctb-${CYLC_TEST_TIME_INIT} | ||
|-- f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests will fail when running tests using the functional
directory name, rather than the f
one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed.
@@ -62,23 +67,23 @@ cmp_ok "${TEST_NAME}.stdout" << __TREE__ | |||
${TEST_DIR}/sym-cycle | |||
\`-- cylc-run | |||
\`-- cylctb-${CYLC_TEST_TIME_INIT} | |||
\`-- f | |||
\`-- ${FUNCTIONAL_DIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realised that if another test gets added to this directory (i.e. 01-<name>.t
) then this test will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried copying another test file into tests/functional/cylc-clean/01-something.t
and 00-basic.t
still works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If 01-something creates a workflow and the two tests are run together then 01-something will change the output of tree
based on the order in which the tests are run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried making a copy of 00-basic.t
called 01-foo.t
that has a slightly different expected tree (no symlink work
dir or leave-me-alone
dir) and both pass? Doesn't purge
take care of removing the old stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, but only once the test has passed but tests run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully b35b10b prevents that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with these changes. Thanks @MetRonnie!
These changes partially address #3887
Implements a new command
Note: Cleaning remote directories will come in a later PR.
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.