fleet icon indicating copy to clipboard operation
fleet copied to clipboard

Refactoring ListHostSoftware

Open ksykulev opened this issue 10 months ago • 6 comments

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • [x] Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes. See Changes files for more information.
  • [x] Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
  • [x] Added/updated automated tests
  • [ ] A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
  • [ ] Manual QA for all new/changed functionality

ksykulev avatar Mar 25 '25 04:03 ksykulev

ListHostSoftware now broken down into a few logical parts.

get installed software (hostInstalledSoftware) get upcoming installs (hostSoftwareInstalls) get upcoming uninstalls (hostSoftwareUninstalls) get vpp installs (hostVPPInstalls) filter pending install/uninstalls by label (filterSoftwareInstallersByLabel) get available software for installs (stmtAvailable) (This is the original available software statement) get information about all those software titles using IN (...ids...) get total count of software to be returned some post processing to adjust to required object to be returned from method pagination

ksykulev avatar Mar 26 '25 14:03 ksykulev

Codecov Report

Attention: Patch coverage is 91.45383% with 87 lines in your changes missing coverage. Please review.

Project coverage is 63.98%. Comparing base (ae92b3e) to head (7582fa2). Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
server/datastore/mysql/software.go 91.45% 56 Missing and 31 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #27490      +/-   ##
==========================================
+ Coverage   63.92%   63.98%   +0.06%     
==========================================
  Files        1775     1779       +4     
  Lines      169916   170453     +537     
  Branches     4880     4951      +71     
==========================================
+ Hits       108618   109064     +446     
- Misses      52736    52796      +60     
- Partials     8562     8593      +31     
Flag Coverage Δ
backend 65.03% <91.45%> (+0.03%) :arrow_up:
frontend 52.44% <ø> (+0.33%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Mar 26 '25 15:03 codecov[bot]

I slight disagree with the failing integration test. https://github.com/fleetdm/fleet/blob/main/server/service/integration_enterprise_test.go#L10701

> [Breakpoint 2] github.com/fleetdm/fleet/v4/server/datastore/mysql.(*Datastore).ListHostSoftware() ./server/datastore/mysql/software.go:2726 (hits goroutine(5261):1 total:1) (PC: 0x10660e7b0)
  2721:			}
  2722:		}
  2723:	
  2724:		var stmtAvailable string
  2725:	
=>2726:		if (opts.OnlyAvailableForInstall && !opts.VulnerableOnly) || (opts.IncludeAvailableForInstall && !opts.VulnerableOnly) {
  2727:			namedArgs["vpp_apps_platforms"] = fleet.VPPAppsPlatforms
  2728:			if fleet.IsLinux(host.Platform) {
  2729:				namedArgs["host_compatible_platforms"] = fleet.HostLinuxOSs
  2730:			} else {
  2731:				namedArgs["host_compatible_platforms"] = []string{host.FleetPlatform()}
(dlv) p opts
github.com/fleetdm/fleet/v4/server/fleet.HostSoftwareTitleListOptions {
	ListOptions: github.com/fleetdm/fleet/v4/server/fleet.ListOptions {Page: 0, PerPage: 0, OrderKey: "name", OrderDirection: OrderAscending (0), MatchQuery: "", After: "", IncludeMetadata: true, TestSecondaryOrderKey: "", TestSecondaryOrderDirection: OrderAscending (0)},
	SelfServiceOnly: false,
	IncludeAvailableForInstall: true,
	OnlyAvailableForInstall: true,
	VulnerableOnly: false,
	KnownExploit: false,
	MinimumCVSS: 0,
	MaximumCVSS: 0,
	IsMDMEnrolled: false,}

It states that there are supposed to be two software installers returned as "available"

OnlyAvailableForInstall: true,

The database tables

>> dumping table software_titles:
[id name source browser bundle_identifier additional_identifier unique_identifier]
1	bar	apps		NULL	NULL	bar	
2	foo	chrome_extensions		NULL	NULL	foo	
4	ruby	deb_packages		NULL	NULL	ruby	
5	DummyApp.app	apps		com.example.dummy	0	com.example.dummy	
<< dumping table software_titles completed
>> dumping table host_software:
[host_id software_id last_opened_at]
1	1	NULL	
1	2	NULL	
1	3	NULL	
1	4	NULL	
<< dumping table host_software completed
>> dumping table software_installers:
[id team_id global_or_team_id title_id filename version platform pre_install_query install_script_content_id post_install_script_content_id storage_id uploaded_at self_service user_id user_name user_email url package_ids extension uninstall_script_content_id updated_at fleet_maintained_app_id install_during_setup]
1	NULL	0	4	ruby.deb	1:2.5.1	linux		1	NULL	df06d9ce9e2090d9cb2e8cd1f4d7754a803dc452bf93e3204e3acd3b95508628	2025-03-27T16:04:53.076302Z	1	1	Test Name [email protected]	[email protected]		ruby	deb	2	2025-03-27T16:04:53.08412Z	NULL	0	
2	NULL	0	5	dummy_installer.pkg	1.0.0	darwin		1	NULL	7f679541ccfdb56094ca76117fd7cf75071c9d8f43bfd2a6c0871077734ca7c8	2025-03-27T16:04:53.652803Z	0	1	Test Name [email protected]	[email protected]		com.example.dummy	pkg	3	2025-03-27T16:04:53.652803Z	NULL	0	
<< dumping table software_installers completed

The software installers table is a little hard to read, but the important information is dummy_installer.pkg has a global_or_team_id of 0 and a platform of darwin

DummyApp.app is marked as installed on the host, however, the particular software_installer record platform does not match the compatible platforms on the host.

(dlv) p host.Platform
"linux"
(dlv) p namedArgs["host_compatible_platforms"]
interface {}([]string) [
	"linux",
	"ubuntu",
	"debian",
	"rhel",
	"centos",
	"sles",
	"kali",
	"gentoo",
	"amzn",
	"pop",
	"arch",
	"linuxmint",
	"void",
	"nixos",
	"endeavouros",
	"manjaro",
	"opensuse-leap",
	"opensuse-tumbleweed",
	"tuxedo",
]

So while the software matches, the installer is not compatible.

If we filtered for IncludeAvailableForInstall vs OnlyAvailableForInstall, I would expect to see DummyApp.app

ksykulev avatar Mar 27 '25 16:03 ksykulev

@ksykulev you're right about platform filtering on available for install. Catch there is that pulls on a thread that we probably don't have test coverage on. Also, that test doesn't have realistic input data, as:

  1. Host OS is never linux (it'll be some distro) so we'd need to make sure our platform filtering for installers correctly mapped a Linux-flavored host (e.g. ubuntu) to a linux package (getting RPM vs. DEB right based on platform is out of scope).
  2. Linux hosts don't have apps in inventory. We can actually decide which tables to pull info from based on host OS; windows/linux-flavored would get only installers, ios/ipados would get only VPP apps, and darwin would get both.

So we probably need to revise this test from linux to ubuntu and drop out apps + bundle ID logic from this one, then add a darwin-specific test where the host OS is actually darwin (plus maybe another for iOS or iPadOS if we don't already have those covered by lower-level tests).

The cose has coincidentally worked on real hosts probably because installed software etc. has filtered by source, but we need to make sure things are more explicit since, to your point, that test doesn't make sense the way it's built.

The above only addresses the test comment; I haven't reviewed everything else but plan to today.

iansltx avatar Apr 05 '25 18:04 iansltx

@ksykulev BTW careful about force-pushing at this point (the force-push for uncommenting tests was fine, fortunately) since GitHub doesn't do well tracking line-level comments across force-pushes, so stuff has a habit of getting lost.

iansltx avatar Apr 06 '25 22:04 iansltx

@ksykulev Re-request review form me when you're ready (and have merged in main/resolved conflicts). Thx!

iansltx avatar Apr 09 '25 06:04 iansltx