Skip to content
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

Add support for aborting scans to the API #176

Merged
1 commit merged into from
Mar 30, 2023
Merged

Add support for aborting scans to the API #176

1 commit merged into from
Mar 30, 2023

Conversation

chrisgacsal
Copy link
Contributor

@chrisgacsal chrisgacsal commented Mar 22, 2023

Description

This change is the first step towards implementing the functionality to allow cancelling ongoing scans:

  • allow marking Scan state as Aborting indicating that the Scan needs to be cancelled and cleaned up. Orchestrator is responsible for watching Scans which requested to be aborted and setting the stateReason to Aborted.

    State transition:

    Pending > Discovered > InProgress > Done
                                      > Failed
                         > Aborted > Failed
            > Aborted > Failed
  • allow marking state of a TargetScan to ABORTED to allow the cli to detect and act upon cancellation events (e.g. stopping scanners, reporting partial results, etc).

Type of Change

[ ] Bug Fix
[x] New Feature
[ ] Breaking Change
[ ] Refactor
[ ] Documentation
[ ] Other (please describe)

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@chrisgacsal chrisgacsal requested a review from a team as a code owner March 22, 2023 08:14
@ghost
Copy link

ghost commented Mar 22, 2023

Hi @chrisgacsal, thanks for the thoughts here. I think we're going along the same lines, I hadn't written much about the abort case in https://cisco-eti.atlassian.net/wiki/spaces/PA/pages/8192001/Persistent+Scan+Orchestrator but my expectation is that it would behave exactly as documented for the timeout scenario, except the trigger would be the user.

  • User patch Scan status as Aborting
  • Scan reconcile loop moves Scan from Aborting to Failed, Reason: "Aborted"
  • TargetScan timeout reconcile loop notices that the TargetScan's parent Scan has moved to Failed (for any reason), and moves the TargetScan to failed
  • Scanner CLI itself notices that the TargetScan is in Failed and no longer updates the TargetScan.

With that in mind I don't think we need a separate ABORTED state for the TargetScan. If we want the metadata for why the TargetScan failed, we could perhaps follow the same design as we had for Scan's with a state and reason. So TargetScan would have reasons like:

  • Aborted
  • Scan Failed
  • TimedOut

If the TargetScan gets canceled because the parent Scan has timed out or was aborted, then the TargetScan reason could be Aborted, but if the individual TargetScan times out then it would be TimedOut. Or we could have an extra reason "ParentTimedOut" then it would be really informative.

@chrisgacsal chrisgacsal reopened this Mar 22, 2023
@chrisgacsal
Copy link
Contributor Author

@sambetts-cisco I have requested access to Confluent, so I'll get back to you after I had a chance to review the document from above.

@chrisgacsal chrisgacsal changed the title feat(api): add support for aborting scans Add support for aborting scans to the API Mar 27, 2023
@chrisgacsal chrisgacsal requested a review from a user March 27, 2023 07:26
@chrisgacsal
Copy link
Contributor Author

@sambetts-cisco I have updated the PR according to what we discussed last week.

ghost
ghost previously approved these changes Mar 29, 2023
* allow marking Scan state as `Aborted` indicating that the Scan needs
  to be cancelled and cleaned up. Orchestrator is responsible for
  watching Scans which requested to be aborted and setting the
  `stateReason` to `Aborted`.

  State transition:

    Pending > Discovered > InProgress > Done
                                      > Failed
                         > Aborted > Failed
            > Aborted > Failed

* allow marking `state` of a TargetScan to `ABORTED` to allow the `cli`
  to detect and act upon cancellation events (e.g. stopping scanners,
  reporting partial results, etc).
@ghost ghost merged commit 87b0e73 into main Mar 30, 2023
@ghost ghost deleted the stop-scan branch March 30, 2023 12:53
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant