Repository navigation
fix: hide the job template's log and marker files from the v1 artifact listing - #163
matthewberry wants to merge 2 commits into
Conversation
…t listing
LOG_ARTIFACTS listed only the novostoic names (output.log, error.log, success, fail)
and chemscraper's errors.txt. It missed the names the job template and the CLEAN,
CRISPR-Copies and Mutagenesis commands write: `finished`, `log` and `error`. So
GET /v1/jobs/{id}/artifacts listed `log`, which holds the tool's console output and
traceback, without ?include=logs.
Add the three names, and record next to the set where each name is written.
| # their output to `log` and touch `error` on failure; | ||
| # - the novostoic commands (chart/values.yaml) write `output.log`, `error.log`, | ||
| # `success` and `fail`. | ||
| LOG_ARTIFACTS = {'output.log', 'error.log', 'errors.txt', 'success', 'fail', 'log', 'error', 'finished'} |
There was a problem hiding this comment.
Hiding these from the listing doesn't protect them. get_artifact (line ~501) serves any path under {job_id}/out/ with no LOG_ARTIFACTS check, so GET /v1/jobs/{id}/artifacts/log still returns the full tee'd traceback, and anyone can list with ?include=logs anyway. This predates the PR, but the stated reason here (logs can include internal paths and configuration) isn't addressed, only discoverability is. If that concern is real, the gate belongs in get_artifact, or the logs shouldn't live under out/.
There was a problem hiding this comment.
Agreed, and not changed here. The comment above LOG_ARTIFACTS now says it only filters the default listing and is not access control (27a852a). Gating get_artifact on the same set would add nothing while ?include=logs is open to anyone holding the job id. A real gate needs auth on the artifact routes, or moving logs out of out/, and both are bigger than this PR. Happy to file that as an issue.
| # their output to `log` and touch `error` on failure; | ||
| # - the novostoic commands (chart/values.yaml) write `output.log`, `error.log`, | ||
| # `success` and `fail`. | ||
| LOG_ARTIFACTS = {'output.log', 'error.log', 'errors.txt', 'success', 'fail', 'log', 'error', 'finished'} |
There was a problem hiding this comment.
The listing filter (line ~468) matches short.split('/')[-1], the file name at any depth. With generic names like log, error and finished in the set, a real result in a subfolder (e.g. run1/log from Mutagenesis, which writes its whole tree to $JOB_OUTPUT_DIR) would silently disappear from the default listing. Every writer listed above writes these at the top level only, so short in LOG_ARTIFACTS would cover them without the collision.
There was a problem hiding this comment.
Fixed in 27a852a: the filter now matches short in LOG_ARTIFACTS, the path relative to out/. I checked that every writer found by git grep -nE 'OUTPUT_DIR[}]?/' -- app chart puts these files at the top level, so nothing that was hidden before becomes visible. The new test_a_result_in_a_subfolder_with_a_log_name_stays_listed fails with basename matching restored.
| # their output to `log` and touch `error` on failure; | ||
| # - the novostoic commands (chart/values.yaml) write `output.log`, `error.log`, | ||
| # `success` and `fail`. | ||
| LOG_ARTIFACTS = {'output.log', 'error.log', 'errors.txt', 'success', 'fail', 'log', 'error', 'finished'} |
There was a problem hiding this comment.
Minor: errors.txt never matches here. Its only writer (chemscraper_service.py:297) uploads to {job_id}/errors.txt, outside the {job_id}/out/ prefix this endpoint lists. It's also not mentioned in the writer list above.
There was a problem hiding this comment.
Confirmed: chemscraper_service.py:297 uploads to {jobId}/errors.txt, outside the {job_id}/out/ prefix. Removed it in 27a852a. That changes no behavior, since it could never match.
| # chose to print, which can include internal paths and configuration. | ||
| LOG_ARTIFACTS = {'output.log', 'error.log', 'errors.txt', 'success', 'fail'} | ||
| # | ||
| # Keep this in step with every writer of a log or marker into $JOB_OUTPUT_DIR: |
There was a problem hiding this comment.
This list isn't complete: chart/novostoic.job.yaml:100 (the somn job) also does touch "${JOB_OUTPUT_DIR}/finished". (Also, the template's path is app/services/templates/job.tpl.yaml.) Nothing ties these string literals to their writers, so it would help to note the search that produced the list, e.g. git grep -nE 'OUTPUT_DIR[}]?/' -- app chart. Better still would be a shared constant, or a dedicated logs subfolder, so the next tool that writes a new marker doesn't leak it into the default listing.
There was a problem hiding this comment.
Fixed in 27a852a: added chart/novostoic.job.yaml (a rendered somn Job; nothing in the repo references it, but it does touch finished), corrected the template path to app/services/templates/job.tpl.yaml, and put the git grep command next to the list.
I did not add a shared constant: most writers are shell strings in chart/values.yaml and job.tpl.yaml, which a Python constant can't reach. A dedicated logs subfolder would fix it properly, but it changes every tool's command and the paths clients already read, so it should be its own change.
| assert [i["name"] for i in default["items"]] == ["output.json"] | ||
| assert with_logs["count"] == 4 | ||
|
|
||
| def test_job_template_log_and_markers_are_hidden_unless_requested(self, client): |
There was a problem hiding this comment.
This duplicates test_logs_are_hidden_unless_requested line for line apart from the file names. Consider one pytest.mark.parametrize test over the name tuples instead.
There was a problem hiding this comment.
Done in 27a852a: one test_logs_are_hidden_unless_requested, parametrized over the novostoic names and the job-template names.
| # The job template touches `finished`; CLEAN, CRISPR-Copies and Mutagenesis tee | ||
| # to `log` and touch `error` on failure. `log` holds the tool's traceback. | ||
| job_id = _submit(client).json()["job_id"] | ||
| for name in ("output.json", "log", "error", "finished"): |
There was a problem hiding this comment.
These go into the novostoic-optstoic bucket at the top level only, so the test doesn't cover the CLEAN/CRISPR-Copies/Mutagenesis job types or the nested-name case (e.g. sub/log being hidden). A case asserting that a nested log stays visible would pin down the matching behavior.
There was a problem hiding this comment.
Added the nested case in 27a852a: run1/log and run1/error stay listed, and a top-level log is hidden. I kept the single bucket: the filter never looks at the job type, so repeating the test per bucket would test the fake MinIO, not the filter. The test comment now says so.
- Match LOG_ARTIFACTS against the path relative to out/, not the file name at any
depth. Every log and marker writer puts its file at the top level, and with generic
names like `log` and `error` in the set, a result in a subfolder could be hidden.
- Drop `errors.txt`: its only writer (chemscraper_service.py) uploads it to
{job_id}/errors.txt, outside the out/ prefix this endpoint lists, so it never matched.
- Fix the template path in the writer list, add chart/novostoic.job.yaml, and record
the search that produced the list.
- Say in the comment that the set only filters the default listing and is not access
control.
- Fold the two hidden-name tests into one parametrized test, and add a test that a
subfolder file named `log` stays listed.
Problem
GET /v1/jobs/{id}/artifactshides log and status-marker files unless?include=logsis passed. Its docstring says why: their content "is whatever the container printed, which can carry internal paths and configuration, and anyone holding a job id can read them."The names it hides (
LOG_ARTIFACTS,app/routers/v1/jobs.py) wereoutput.log,error.log,errors.txt,successandfail. Those are novostoic's and chemscraper's. The set missed the names most jobs actually write:finished: touched byjob.tpl.yamlafter every successful job command;log: the CLEAN, CRISPR-Copies and Mutagenesis commandsteethe tool's output (including any traceback) to it;error: touched by the same three commands when the tool fails.So a default listing of one of those jobs included
log.Change
log,errorandfinishedtoLOG_ARTIFACTS.out/, not the file name at any depth. Every writer puts these files at the top level, so a result in a subfolder that happens to be namedlogorerrorstays listed.errors.txt. Its only writer (chemscraper_service.py:297) uploads it to{job_id}/errors.txt, outside theout/prefix this endpoint lists, so it never matched.$JOB_OUTPUT_DIRhas somewhere to add it.How I found the set:
git grep -nE 'OUTPUT_DIR[}]?/' -- app chart. Apart from the names above, that finds onlypi.out(the demo job's output),step1.out/step2.out(commented out) andcache(deleted after the run).chart/novostoic.job.yaml, a rendered somn Job that nothing references, also touchesfinished. Logs a tool image writes from inside its own code don't show up in this repo, so I couldn't enumerate those.What this does not change
GET /v1/jobs/{id}/artifacts/logstill returns the file, and anyone can pass?include=logs. That's how the endpoint already worked for the novostoic names, and the comment onLOG_ARTIFACTSnow says so. A real gate needs auth on the artifact routes or logs moved out ofout/, which is a separate change.Context
#161 makes failed CLEAN runs upload
loganderrorto MinIO, so this matters more once it lands. This PR doesn't depend on it.Testing
In
tests/test_v1_api.py:test_logs_are_hidden_unless_requestedis now parametrized over the novostoic names and the job-template names (log,error,finished). In each case the default listing shows onlyoutput.json, and?include=logsshows everything. The job-template case fails without the change.test_a_result_in_a_subfolder_with_a_log_name_stays_listed:run1/logandrun1/errorstay listed while a top-levellogis hidden. It fails with basename matching restored.Dockerfile.testimage: 625 passed.