fleetdm / fleet

Open-source platform for IT, security, and infrastructure teams. (Linux, macOS, Chrome, Windows, cloud, data center)
https://fleetdm.com
Other
3.15k stars 431 forks source link

Scripts don't get dequeued when they are edited or deleted #21888

Open noahtalerman opened 2 months ago

noahtalerman commented 2 months ago

Expected Behavior

If an admin deletes/edits a script they expect it not to run.

Currently, the only way to edit a script is via YAML files (GitOps).

Actual Behavior

The script stays as pending.

Steps to reproduce

  1. Create a script
  2. Queue it for a host
  3. Delete (or edit via GitOps) the script

To Fix

When a script's content is edited, a side effect should be to clear any pending script runs referencing that script. Same with deletion of the script (which has a UI-facing endpoint already). Right now the scope of edits is the batch script edit endpoint used by GitOps, but between GitOps edit and UI deletion there's reason enough to encapsulate this in the data store so adding the side effect on script edit is trivial once we implement that endpoint.

Update the copy in the Delete script modal on the Controls > Scripts page:

"This will cancel any pending script execution for ."

"If the script is current running on a host it will still complete, but results won’t appear in Fleet."

"You cannot undo this action."

Test Plan

This covers:

  1. Add two scripts via GitOps. One should take a decent amount of time to run.
  2. Run both scripts successfully on a host.
  3. Run the longer-running script in the foreground (via fleetctl), and queue up the other for async execution.
  4. Take the host offline, then edit the longer-running script in GitOps and apply. The longer-running script pending execution should be deleted (should no longer be visible in pending host actions). The other script should still be pending. The successful script runs should still exist.
  5. Bring the host online, then run the longer-running script in the foreground again, and queue up the other for async execution.
  6. Take the host offline, then delete the longer-running script in GitOps and apply. The longer-running script pending execution should be deleted. The other script should still be pending. The successful script runs should still exist.
  7. Delete the other script in GitOps and apply. Remaining pending script executions should be removed.
  8. Add the two scripts back via the UI. Run the longer-running script in the foreground (via fleetctl), and queue up the other for async execution.
  9. Take the host offline, then delete the longer-running script. The longer-runnings cript pending execution should be deleted. The other script should still be pending. The successful script runs should still exist.
  10. Delete the other script. The remaining pending script run should disappear. The successful script runs should still exist.
noahtalerman commented 2 months ago

Canceling pending script runs when a script is deleted is the expected behavior.

The copy in the UI tells the IT admin that scripts will be canceled. See the screenshot in this bug: #21889

The current plan is to update the copy ASAP to say something like "pending scripts will still run."

In a later iteration, as part of this request, we'll follow up and update the behavior and copy to something like "pending scripts will be canceled."

cc @nonpunctual @georgekarrv @lukeheath

iansltx commented 1 month ago

Chatted with @noahtalerman earlier as I noticed this is adjacent to #17129, and it feels more bug-ish.

There is a trivial UI component to this to update wording introduced in #21889.

noahtalerman commented 1 month ago

There is a trivial UI component to this to update wording introduced in https://github.com/fleetdm/fleet/issues/21889.

@iansltx great call out. I think we can switch back to the old copy:

Screenshot 2024-09-06 at 4 00 29 PM

I added this^ screenshot to the issue description so we don't forget.

iansltx commented 1 month ago

Hey team! Please add your planning poker estimate with Zenhub @mostlikelee @lucasmrod @getvictor