feat(cli)!: bundle submit - remove "--yes" and add "--trust" and "--upload" #427
Conversation
…pload" Signed-off-by: Morgan Epp <[email protected]>
|
| help="Skip any confirmation prompts", | ||
| help="Indicates that files will be uploaded to S3 if needed", | ||
| ) | ||
| @click.option( |
There was a problem hiding this comment.
@mwiebe explicitly said we shouldn't just have a --trust option but to add an allow list of paths.
Thoughts ?
There was a problem hiding this comment.
Keeping the --yes option to mean accept default prompts seems like a good idea to me. What's our rationale for removing it?
There was a problem hiding this comment.
It doesn't behave that way I or customers expect. Removal may be part of a bigger change than in this one, but this change does propose changed behaviour by switching to --upload (something like that).
I do think we should have an option shared amongst all (or most, can't think of exception atm) options that indicate you're operating in an session that is not interactive and thus, anything that would prompt or is not answered would cause a failure of the command. --yes is currently the way to get that, but that's not great
| @click.option( | ||
| "--trust", | ||
| is_flag=True, | ||
| help="Indicates the job bundle is trusted to not perform malicious activities", |
There was a problem hiding this comment.
I think we can do better than an option like this. The question is about where to upload data from for jobs, not about which job bundles are ok or not.
|
What I recommend as our first step to making this experience right is to fix the bug #407 by adding a list of permitted upload globs where it's relevant, and adding the job bundle directory and all paths provided from the CLI options to start. That would be a backwards-compatible change that needs no interface modification. |
What was the problem/requirement? (What/Why)
Starting this PR for discussion around the concept of trusting a job bundle and the UX we present around it.
Right now the deadline client lib is only concerned with job bundles uploading files a user might NOT want uploaded. This currently means that a user will be prompted if files are outside of storage profile locations returned by
GetStorageProfileForQueue.Fixing #407 would allow the client library to not prompt the user if the only files being uploaded are from within the job bundle. However this is all just treating the symptom of submitting untrusted bundles, rather than getting a user to confirm and understand what it means to submit a job bundle written by someone else.
Due to all of this, we've effectively blocked users from running
deadline bundle submitas a scriptable/non-interactive command without piping "yes" into it. This especially becomes problematic for workflows that embed the usage of the deadline cli to perform the submission after they've generated a bundle. If they've just generated a job bundle in their workflow, they should not be prompted about the job. They've specifically set out to create this job, we're just getting in the way.If the workflow that generated the job bundle is malicious, then it can also just move whatever it wants to upload into the job bundle directory/storage profile locations and bypass these filepath checks. So if you trust the workflow to generate the bundle, you inherently trust the job bundle.
What was the solution? (How)
To start unpacking this, we really want the ability to signify that we trust a job bundle, as trusting a path to be uploaded is just 1 part of the greater problem of trusting bundles. Future changes will most likely involve configuring paths to not warn about.
So for
deadline bundle submitwe split out the existing confusing prompt into 2 smaller prompts :( so that users can answer them individually. The first one is about trusting the bundle, and the second one is about confirmation around uploading files. These two prompts have their own flags,--trustand--uploadthat allow users of the cli to answer both, some, or neither.This change intentionally does not add
--trusttodeadline bundle gui-submit, since that function needs quite a bit of refactoring to pass in all the options to make it have parity withdeadline bundle submit.I've removed the
--yescommand becauseauto-accepting defaultsis useless for actually making it through these prompts. Maybe it should stay in for now, but I believe there's a bigger topic for discussion about indicating that you cannot answer prompts, ie you want to use the cli non-interactively, and fail if you do not have an answer for everything.What is the impact of this change?
Users can now script their usage of
deadline bundle submit(minus concurrency) to submit jobs.How was this change tested?
hatch run test?Yes
hatch run integ:test? SeeDEVELOPMENT.mdon "Run integration tests"Yes
Was this change documented?
Self-documenting
Is this a breaking change?
You better believe it!
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.