we stumbled across a rather nasty bug recently and I would like 1) to share the story and 2) think about a good long-term solution. Heads up: A fix is currently available for Foreman nightly in core, only. Plugins may still be affected.
Context
It started with a colleague accidentally deleting a set of hosts in the wrong location, when we started to have a closer look at the ‘All Hosts’ page and its behavior.
What happened? The person chose one location with a set of test hosts. After all tests were done, he selected all the hosts using the ‘Select All’ button. Then he went to another tab, changed the location there to test something else, went back to the tab with the hosts he wanted to delete and hit the button.
Result: The test hosts remained while the hosts of the other location were all gone.
Why? The back end tracks the current user context and adjusts it if you make a change in your browser, across multiple tabs. So if you change the location in one tab, this is what is currently being tracked. The front end bulk actions allow to send search queries to the back end. Static search queries explicitly pass the host ids. Dynamic search queries pass a search string that is evaluated on the server side. This happens if you enter a search string in the search bar (e.g. hostgroup = alma10) or if you click ‘Select All’ (which results in an empty search string). This search string is everything that is currently passed from front end to the back end. There is nothing that guarantees that the command will be executed within the correct user context, e.g. no information about location/organization is sent to the back end.
Scope of error: This error does not only occur for the ‘Delete’ bulk action but can occur for any of the bulk actions on the new ‘All Hosts’ page that talks to the API directly, so remote jobs are NOT affected. It happens if you use a search query or the ‘Select All’ option (dynamic search queries), NOT if you select individual hosts. This also affects plugins such as foreman_puppet and foreman_snapshot_management (and any that send dynamic search queries and do not pass along taxonomies from UI).
Comparison of different proposals for a solution
We came up with different solutions to the problem which are summarized and compared in the below table.
pass along taxonomies from UI to back end to make sure that the search query is not performed in the wrong scope
easy to implement, light-weight
needs implementation across plugins, no guard for dynamic search queries
2. Client-side context guard
(comparable to github PR refresh button) If you enter the page, it will check if user, location or organization have changed in the background. If yes, the UI will ask you to refresh the page and you are in line with your current user session again.
(most likely) light-weight, no need to pass org/loc; no need for extra implementations on plugin side; throws error before I submit the bulk action
implementation complexity/performance impact is unknown, no guard for dynamic search queries
A hash is calculated when the ‘All Hosts’ page is loaded that takes into account location, organization, user and hosts information. When a bulk action is triggered, this hash is sent to the back end and checked against a newly calculated hash. If the hashes do not match, the bulk action will not be performed but returns an error.
no need to pass org/loc necessary, guards dynamic search queries
the guard needs a careful design, especially for large systems with many users, to prevent side-effects when multiple user interact with one Foreman instance at the same time; failure appears only once the bulk action is submitted
4. Host validation before bulk action submission
In the old UI, you had a preview of all hosts that are effected of the bulk action. We could add something similar or a wizard-like view.
in line with old ‘All Hosts’ page, safe in terms of dynamic search queries - hosts can be verified before applying the bulk action → dynamic search queries are transformed into static ones
complex for dynamic search queries, large implementation overhead, probably not feasible for a large number of hosts
Open questions
Do we want/need more control over dynamic search queries for bulk actions?
If not, do we want to implement a more generic solution than the current one, e.g. a client-side guard that would catch changes in the user session?
While number 2 sounds like the most user-friendly one, I also see the complexity there. One advantage I see there should be no scaling issue.
With number 3 taking the hosts into account for the hash calculation will this scale even with big numbers of hosts? I have the numbers in mind which were given in New Job Invocations Page in 3.18.1 - Unusable due to performance and want to avoid similar negative feedback, even if I am quite sure none of our customers will reach this numbers.
The old page did not use API requests but performed all bulk actions based on controller data, so it has direct access to the objects it performs an action on. In addition, it has a preview of the hosts that the action is performed on. So it would show you all the hosts that are affected by the bulk action. If the user context changes in the background, you would see a wrong list of hosts.
In the table it says it guards dynamic search queries, but in my understanding the computed hash only takes into account org / loc / user / and all readable hosts. It would not change depending on search query. I think it would be more ideal if the current search query could be taken into account.
Regarding, only using the current search: this would mean, that the “current search” term always needs to be send to the backend - it need to calc the digest and the UI need to re-use it. For every change of the search, this need to happen. Therefore, I would not choose this because of performance and because such a request+response can also break.
Yeah, and the ‘guard’ if you want to call it that way, was a preview of the hosts that the operation is performed on. The old page shows a list of all the hosts (with host group, location and organization) and you need actively confirm it before the bulk action is performed. So it would show the wrong hosts if something changed in the background.
The main difference implementation-wise is that the old page directly operates on the @hosts object from the back end, so there no passing back and forth passing data via API calls. If I remember correctly, the new page only ‘knows’ the paginated hosts it receives and the total number of hosts. So if you have a search query or ‘Select All’ option, you first need to talk to the back end and get all the information if you wanted to show a confirmation page similar to the old ‘All Hosts’ page. @jeremylenz please correct me if I am wrong.
The current implementation of 3) checks if there are any changes to org/loc/user/hosts in the back end and prevents you from executing a bulk action if the status quo has changed between loading the page and submitting the bulk action. This is what I mean by ‘guard’.
This is correct. We don’t want the frontend to download full info about all your thousands of hosts, so we have a compromise where the set of “what hosts are selected?” is not always stored in full in the browser; rather, it is described by your scoped search query. This by nature is dynamic, and has resulted in plenty of interesting problems like this
