Soft-delete entries for host script results so the details are still available in activities

[mna]

Checklist for submitter

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.
• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
• Added/updated tests
• If database migrations are included, checked table schema to confirm autoupdate
• Manual QA for all new/changed functionality

[mna]

Note that if the host was deleted, the only authorization done is "list hosts" above. Should there be a different authz for that case?

[roperzh]

I tried to come up with something else but I couldn't, the other alternative is to keep some host information around.

We never merged this, and if you remember: but at some point we tried to solve a similar problem, (but for users that were deleted), we were considering having a table like deleted_user_data that kept information like this.

Something like that would be more tricky for hosts because the table is so huge that there will be the temptation to slowly move all the fields.

[mna]

Note that deleting a software installer will still result in not being able to view the activity details. This PR only handles the host's soft-deletion (to keep scope reasonable and mostly in-line with estimation).

[roperzh]

based on your comment I wonder if we should scope software installers in a completely different ticket.

asking because I think we also need to update places like the one below to ignore software installers with a NOT NULL deleted_at, is that correct?

fleet/server/datastore/mysql/software_installers.go

Lines 340 to 381 in 4b82ff6

	func (ds *Datastore) GetSummaryHostSoftwareInstalls(ctx context.Context, installerID uint) (*fleet.SoftwareInstallerStatusSummary, error) {
	var dest fleet.SoftwareInstallerStatusSummary
	stmt := fmt.Sprintf(`
	SELECT
	COALESCE(SUM( IF(status = :software_status_pending, 1, 0)), 0) AS pending,
	COALESCE(SUM( IF(status = :software_status_failed, 1, 0)), 0) AS failed,
	COALESCE(SUM( IF(status = :software_status_installed, 1, 0)), 0) AS installed
	FROM (
	SELECT
	software_installer_id,
	%s
	FROM
	host_software_installs hsi
	WHERE
	software_installer_id = :installer_id
	AND id IN(
	SELECT
	max(id) -- ensure we use only the most recently created install attempt for each host
	FROM host_software_installs
	WHERE
	software_installer_id = :installer_id
	GROUP BY
	host_id)) s`, softwareInstallerHostStatusNamedQuery("hsi", "status"))
	query, args, err := sqlx.Named(stmt, map[string]interface{}{
	"installer_id": installerID,
	"software_status_pending": fleet.SoftwareInstallerPending,
	"software_status_failed": fleet.SoftwareInstallerFailed,
	"software_status_installed": fleet.SoftwareInstallerInstalled,
	})
	if err != nil {
	return nil, ctxerr.Wrap(ctx, err, "get summary host software installs: named query")
	}
	err = sqlx.GetContext(ctx, ds.reader(ctx), &dest, query, args...)
	if err != nil {
	return nil, ctxerr.Wrap(ctx, err, "get summary host software install status")
	}
	return &dest, nil
	}

(I'm not sure if similar considerations also applies to scripts)

[roperzh]

I tried to come up with something else but I couldn't, the other alternative is to keep some host information around.

We never merged this, and if you remember: but at some point we tried to solve a similar problem, (but for users that were deleted), we were considering having a table like deleted_user_data that kept information like this.

Something like that would be more tricky for hosts because the table is so huge that there will be the temptation to slowly move all the fields.

[mna]

@roperzh

I think we also need to update places like the one below to ignore software installers with a NOT NULL deleted_at, is that correct?

Oh that's a great catch, that's correct and I'm not sure if we have something similar for scripts, I'll check. Wondering if we're opening up a can of worms with that fix (or that approach to the fix), given how error-prone it will be to make queries that target those tables and don't explicitly join with hosts...

[mna]

Ok what I think I'll do is scope it back down to scripts only, as @roperzh suggested. Not only do software installers involve other queries, but the installer activities will still fail if the installer is removed, whereas the scripts work in all cases (removing the named script doesn't remove the host script results entry, it just removes the relationship with the saved script).

[mna]

@roperzh

(I'm not sure if similar considerations also applies to scripts)

I did a search for all uses of host_script_results, I couldn't see any that would be wrong due to the soft-deletion (e.g. aggregations that would consider deleted execution requests).

[roperzh]

✨

[georgekarrv]

#17387