From 4c93cbebc9e267a4f2a307f06a4dc797ce21ff1b Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 08:28:34 -0500 Subject: [PATCH 01/15] Check that all TODOs are addressed --- server/datastore/mysql/labels.go | 2 +- server/datastore/mysql/software_installers.go | 8 ++++---- server/datastore/mysql/vpp.go | 7 +++++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index e7fd8a7bd2b7..e8901fd16411 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -695,7 +695,7 @@ func (ds *Datastore) applyHostLabelFilters(ctx context.Context, filter fleet.Tea return "", nil, ctxerr.Wrap(ctx, err, "get software installer metadata by team and title id") default: - // TODO(uniq): prior code was joining on installer id but based on how list options are parsed [1] it seems like this should be the title id + // TODO(Sarah): prior code was joining on installer id but based on how list options are parsed [1] it seems like this should be the title id // [1] https://github.com/fleetdm/fleet/blob/8aecae4d853829cb6e7f828099a4f0953643cf18/server/datastore/mysql/hosts.go#L1088-L1089 installerJoin, installerParams, err := ds.softwareInstallerJoin(*opt.SoftwareTitleIDFilter, *opt.SoftwareStatusFilter) if err != nil { diff --git a/server/datastore/mysql/software_installers.go b/server/datastore/mysql/software_installers.go index 06be873e1ec1..c003654b4a4d 100644 --- a/server/datastore/mysql/software_installers.go +++ b/server/datastore/mysql/software_installers.go @@ -1089,7 +1089,7 @@ WHERE func (ds *Datastore) GetSummaryHostSoftwareInstalls(ctx context.Context, installerID uint) (*fleet.SoftwareInstallerStatusSummary, error) { var dest fleet.SoftwareInstallerStatusSummary - // TODO(uniq): AFAICT we don't have uniqueness for host_id + title_id in upcoming or + // TODO(Sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or // past activities. In the past the max(id) approach was "good enough" as a proxy for the most // recent activity since we didn't really need to worry about the order of activities. // Moving to a time-based approach would be more accurate but would require a more complex and @@ -1207,7 +1207,7 @@ WHERE status = fleet.SoftwareInstallFailed // TODO: When VPP supports uninstall this should become STATUS IN ('failed_install', 'failed_uninstall') } - // TODO(uniq): AFAICT we don't have uniqueness for host_id + title_id in upcoming or + // TODO(Sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or // past activities. In the past the max(id) approach was "good enough" as a proxy for the most // recent activity since we didn't really need to worry about the order of activities. // Moving to a time-based approach would be more accurate but would require a more complex and @@ -1278,7 +1278,7 @@ WHERE statusFilter = "hsi.status IN (:installFailed, :uninstallFailed)" } - // TODO(uniq): AFAICT we don't have uniqueness for host_id + title_id in upcoming or + // TODO(Sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or // past activities. In the past the max(id) approach was "good enough" as a proxy for the most // recent activity since we didn't really need to worry about the order of activities. // Moving to a time-based approach would be more accurate but would require a more complex and @@ -1308,7 +1308,7 @@ WHERE "status": status, "installFailed": fleet.SoftwareInstallFailed, "uninstallFailed": fleet.SoftwareUninstallFailed, - // TODO(uniq): prior code was joining based on installer id but based on how list options are parsed [1] it seems like this should be the title id + // TODO(Sarah): prior code was joining based on installer id but based on how list options are parsed [1] it seems like this should be the title id // [1] https://github.com/fleetdm/fleet/blob/8aecae4d853829cb6e7f828099a4f0953643cf18/server/datastore/mysql/hosts.go#L1088-L1089 "title_id": titleID, }) diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index e78ea7e0e5b5..a5681238113a 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -118,8 +118,9 @@ func (ds *Datastore) GetSummaryHostVPPAppInstalls(ctx context.Context, teamID *u // not handling it as part of the unified queue work. // TODO(uniq): refactor vppHostStatusNamedQuery to use the same logic as below + // re: ^ : seems like the logic is the same except for pending/null? - // TODO(uniq): AFAICT we don't have uniqueness for host_id + title_id in upcoming or + // TODO(Sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or // past activities. In the past the max(id) approach was "good enough" as a proxy for the most // recent activity since we didn't really need to worry about the order of activities. // Moving to a time-based approach would be more accurate but would require a more complex and @@ -180,6 +181,8 @@ SELECT WHEN ncr.status = :mdm_status_error OR ncr.status = :mdm_status_format_error THEN :software_status_failed ELSE + -- TODO should that count as pending install (should be covered by the upcoming + -- case, but seems more correct to report as pending it is found in past) NULL -- either pending or not installed via VPP App END AS status FROM @@ -706,7 +709,7 @@ func (ds *Datastore) GetVPPAppMetadataByAdamIDPlatformTeamID(ctx context.Context vat.self_service, va.title_id, va.platform, - va.created_at, + va.created_at, va.updated_at, vat.id FROM vpp_apps va From c33652ddd7b335664ce2bb67c3c382f60a52f44a Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 09:08:06 -0500 Subject: [PATCH 02/15] Fix orbit-related test failure --- server/service/orbit_test.go | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/server/service/orbit_test.go b/server/service/orbit_test.go index 6ebe2cc6ebf3..f5310ece55f4 100644 --- a/server/service/orbit_test.go +++ b/server/service/orbit_test.go @@ -42,10 +42,10 @@ func TestGetOrbitConfigLinuxEscrow(t *testing.T) { ds.TeamAgentOptionsFunc = func(ctx context.Context, id uint) (*json.RawMessage, error) { return ptr.RawMessage(json.RawMessage(`{}`)), nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { + ds.ListReadyToExecuteScriptsForHostFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { return nil, nil } - ds.ListPendingSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { + ds.ListReadyToExecuteSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { return nil, nil } ds.IsHostConnectedToFleetMDMFunc = func(ctx context.Context, host *fleet.Host) (bool, error) { @@ -104,10 +104,10 @@ func TestGetOrbitConfigLinuxEscrow(t *testing.T) { ds.TeamAgentOptionsFunc = func(ctx context.Context, id uint) (*json.RawMessage, error) { return ptr.RawMessage(json.RawMessage(`{}`)), nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { + ds.ListReadyToExecuteScriptsForHostFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { return nil, nil } - ds.ListPendingSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { + ds.ListReadyToExecuteSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { return nil, nil } ds.IsHostConnectedToFleetMDMFunc = func(ctx context.Context, host *fleet.Host) (bool, error) { @@ -283,10 +283,10 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.GetHostOperatingSystemFunc = func(ctx context.Context, hostID uint) (*fleet.OperatingSystem, error) { return os, nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { + ds.ListReadyToExecuteScriptsForHostFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { return nil, nil } - ds.ListPendingSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { + ds.ListReadyToExecuteSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { return nil, nil } ds.IsHostConnectedToFleetMDMFunc = func(ctx context.Context, host *fleet.Host) (bool, error) { @@ -352,7 +352,7 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.GetHostOperatingSystemFunc = func(ctx context.Context, hostID uint) (*fleet.OperatingSystem, error) { return os, nil } - ds.ListPendingSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { + ds.ListReadyToExecuteSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { return nil, nil } team := fleet.Team{ID: 1} @@ -364,7 +364,7 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.TeamAgentOptionsFunc = func(ctx context.Context, id uint) (*json.RawMessage, error) { return ptr.RawMessage(json.RawMessage(`{}`)), nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { + ds.ListReadyToExecuteScriptsForHostFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { return nil, nil } ds.IsHostConnectedToFleetMDMFunc = func(ctx context.Context, host *fleet.Host) (bool, error) { @@ -437,7 +437,7 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { return appCfg, nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { + ds.ListReadyToExecuteScriptsForHostFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { return nil, nil } @@ -452,7 +452,7 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.TeamAgentOptionsFunc = func(ctx context.Context, id uint) (*json.RawMessage, error) { return ptr.RawMessage(json.RawMessage(`{}`)), nil } - ds.ListPendingSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { + ds.ListReadyToExecuteSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { return nil, nil } ds.GetHostMDMFunc = func(ctx context.Context, hostID uint) (*fleet.HostMDM, error) { @@ -528,10 +528,10 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.TeamAgentOptionsFunc = func(ctx context.Context, id uint) (*json.RawMessage, error) { return ptr.RawMessage(json.RawMessage(`{}`)), nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { + ds.ListReadyToExecuteScriptsForHostFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { return nil, nil } - ds.ListPendingSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { + ds.ListReadyToExecuteSoftwareInstallsFunc = func(ctx context.Context, hostID uint) ([]string, error) { return nil, nil } ds.IsHostConnectedToFleetMDMFunc = func(ctx context.Context, host *fleet.Host) (bool, error) { @@ -555,9 +555,6 @@ func TestGetOrbitConfigNudge(t *testing.T) { ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { return appCfg, nil } - ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hostID uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) { - return nil, nil - } ds.GetHostOperatingSystemFunc = func(ctx context.Context, hostID uint) (*fleet.OperatingSystem, error) { return os, nil } From ca633085f75dd3861ceb5ffe964fd92acc8a135d Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 10:42:38 -0500 Subject: [PATCH 03/15] Fix policy automation failing test --- server/datastore/mysql/activities.go | 12 +++++++++-- server/service/integration_mdm_test.go | 30 +++++++++++++++++++------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/server/datastore/mysql/activities.go b/server/datastore/mysql/activities.go index 99742750fdb2..d5ed4a6d99d0 100644 --- a/server/datastore/mysql/activities.go +++ b/server/datastore/mysql/activities.go @@ -58,7 +58,15 @@ func (ds *Datastore) NewActivity( cols = append(cols, "user_email") } - if vppAct, ok := activity.(fleet.ActivityInstalledAppStoreApp); ok { + vppPtrAct, okPtr := activity.(*fleet.ActivityInstalledAppStoreApp) + vppAct, ok := activity.(fleet.ActivityInstalledAppStoreApp) + if okPtr || ok { + hostID := vppAct.HostID + cmdUUID := vppAct.CommandUUID + if okPtr { + cmdUUID = vppPtrAct.CommandUUID + hostID = vppPtrAct.HostID + } // NOTE: ideally this would be called in the same transaction as storing // the nanomdm command results, but the current design doesn't allow for // that with the nano store being a distinct entity to our datastore (we @@ -76,7 +84,7 @@ func (ds *Datastore) NewActivity( // gets created only when the MDM command status is in a final state // (success or failure), which is exactly when we want to activate the next // activity. - if _, err := ds.activateNextUpcomingActivity(ctx, ds.writer(ctx), vppAct.HostID, vppAct.CommandUUID); err != nil { + if _, err := ds.activateNextUpcomingActivity(ctx, ds.writer(ctx), hostID, cmdUUID); err != nil { return ctxerr.Wrap(ctx, err, "activate next activity from VPP app install") } } diff --git a/server/service/integration_mdm_test.go b/server/service/integration_mdm_test.go index 0c161712ed4c..9eaf8fd5511d 100644 --- a/server/service/integration_mdm_test.go +++ b/server/service/integration_mdm_test.go @@ -12220,7 +12220,8 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { fmt.Sprint(team.ID), "software_title_id", fmt.Sprint(macOSTitleID)) require.Equal(t, 0, countResp.Count) - // MDM host failing policy should queue install + // MDM host failing policy should queue install (adam ID 1 for macOS) and it will be "activated" + // in the queue as there is nothing pending for that host. s.DoJSONWithoutAuth("POST", "/api/osquery/distributed/write", genDistributedReqWithPolicyResults( mdmHost, map[uint]*bool{ @@ -12231,7 +12232,9 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { fmt.Sprint(team.ID), "software_title_id", fmt.Sprint(macOSTitleID)) require.Equal(t, 1, countResp.Count) - // App is out of scope for mdmHost2, so we do not enqueue an install. + // App is out of scope for mdmHost2, so we do not enqueue a software install + // but we do enqueue a script (will be "activated" in the queue as there is nothing + // pending for that host). s.DoJSONWithoutAuth("POST", "/api/osquery/distributed/write", genDistributedReqWithPolicyResults( mdmHost2, map[uint]*bool{ @@ -12247,11 +12250,14 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { require.Equal(t, getHostResp.Host.ID, mdmHost2.ID) require.NotNil(t, getHostResp.Host.Policies) require.Len(t, *getHostResp.Host.Policies, 4) + policySeen := false for _, p := range *getHostResp.Host.Policies { if p.Name == policy3Team1.Name { require.Empty(t, p.Response) + policySeen = true } } + require.True(t, policySeen) // Validate that orbit got a notif (and get the exec ID for the script) var orbitResp orbitGetConfigResponse @@ -12297,7 +12303,7 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { ) // Update the app to exclude any with l2. We should not enqueue an install here because mdmHost2 - // has l2. + // has l2 (but it will re-enqueue the script for execution, immediately "activated") updateAppReq := &updateAppStoreAppRequest{TeamID: &team.ID, SelfService: false, LabelsExcludeAny: []string{l2.Name}} s.Do("PATCH", fmt.Sprintf("/api/latest/fleet/software/titles/%d/app_store_app", macOSTitleID), updateAppReq, http.StatusOK) @@ -12316,11 +12322,14 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { require.Equal(t, getHostResp.Host.ID, mdmHost2.ID) require.NotNil(t, getHostResp.Host.Policies) require.Len(t, *getHostResp.Host.Policies, 4) + policySeen = false for _, p := range *getHostResp.Host.Policies { if p.Name == policy3Team1.Name { require.Empty(t, p.Response) + policySeen = true } } + require.True(t, policySeen) s.DoJSON("POST", "/api/fleet/orbit/config", json.RawMessage(fmt.Sprintf(`{"orbit_node_key": %q}`, *mdmHost2.OrbitNodeKey)), @@ -12361,7 +12370,8 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { 0, ) - // Update the app to include any with l1. We should now enqueue an install as the app is in scope. + // Update the app to include any with l1. We should now enqueue an install as the app is in scope + // (in addition to the script execution, which will be the only one "activated"). updateAppReq = &updateAppStoreAppRequest{TeamID: &team.ID, SelfService: false, LabelsIncludeAny: []string{l1.Name, l2.Name}} s.Do("PATCH", fmt.Sprintf("/api/latest/fleet/software/titles/%d/app_store_app", macOSTitleID), updateAppReq, http.StatusOK) @@ -12379,11 +12389,14 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { require.Equal(t, getHostResp.Host.ID, mdmHost2.ID) require.NotNil(t, getHostResp.Host.Policies) require.Len(t, *getHostResp.Host.Policies, 4) + policySeen = false for _, p := range *getHostResp.Host.Policies { if p.Name == policy3Team1.Name { require.Equal(t, "fail", p.Response) + policySeen = true } } + require.True(t, policySeen) // MDM host failing policy should not queue another install while install is pending s.DoJSONWithoutAuth("POST", "/api/osquery/distributed/write", genDistributedReqWithPolicyResults( @@ -12396,9 +12409,9 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { var countPendingInstalls uint mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { return sqlx.GetContext(ctx, q, &countPendingInstalls, `SELECT COUNT(*) - FROM host_vpp_software_installs hvsi - JOIN nano_view_queue nvq ON nvq.command_uuid = hvsi.command_uuid AND nvq.status IS NULL - WHERE hvsi.host_id = ?`, mdmHost.ID) + FROM upcoming_activities + WHERE activity_type = 'vpp_app_install' + AND host_id = ?`, mdmHost.ID) }) require.Equal(t, uint(1), countPendingInstalls) @@ -12479,6 +12492,7 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { string(*hostActivitiesResp.Activities[0].Details), ) + // setting a script result will activate the vpp install s.DoJSON("POST", "/api/fleet/orbit/scripts/result", json.RawMessage(fmt.Sprintf(`{"orbit_node_key": %q, "execution_id": %q, "exit_code": 0, "output": "ok"}`, *mdmHost2.OrbitNodeKey, scriptExecID)), http.StatusOK, &orbitPostScriptResp) @@ -12492,7 +12506,7 @@ func (s *integrationMDMTestSuite) TestVPPAppPolicyAutomation() { 0, ) - // Process mdmHost2's installation + // Process mdmHost2's vpp installation cmd, err = mdmDevice2.Idle() require.NoError(t, err) require.NoError(t, plist.Unmarshal(cmd.Raw, &fullCmd)) From ab39c53da258fa936563020c91cffc792a3061e8 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 11:19:37 -0500 Subject: [PATCH 04/15] Fix integration test for list host upcoming activities --- server/service/integration_core_test.go | 31 ++++++++++--------- server/service/integration_enterprise_test.go | 1 + 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index a469c2d872fd..c9d75f1b4ff9 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -12218,16 +12218,19 @@ func (s *integrationTestSuite) TestListHostUpcomingActivities() { h1Foo, err := s.ds.InsertSoftwareInstallRequest(ctx, host1.ID, s1Meta.InstallerID, fleet.HostSoftwareInstallOptions{}) require.NoError(t, err) - // force an order to the activities - endTime := mysql.SetOrderedCreatedAtTimestamps(t, s.ds, time.Now(), "host_script_results", "execution_id", h1A, h1B) - endTime = mysql.SetOrderedCreatedAtTimestamps(t, s.ds, endTime, "host_software_installs", "execution_id", h1Foo) - mysql.SetOrderedCreatedAtTimestamps(t, s.ds, endTime, "host_script_results", "execution_id", h1C, h1D, h1E) + // force an order to the activities (h1A must be first as it was automatically activated) + mysql.SetOrderedCreatedAtTimestamps(t, s.ds, time.Now(), "upcoming_activities", "execution_id", h1A, h1B, h1Foo, h1C, h1D, h1E) // modify the timestamp h1A and h1B to simulate an script that has been - // pending for a long time (h1A is a sync request, so it will be ignored for - // upcoming activities) + // pending for a long time (h1A is a sync script but that doesn't change + // anything anymore, sync scripts are part of the queue like any other: + // https://github.com/fleetdm/fleet/issues/22866#issuecomment-2575961141) mysql.ExecAdhocSQL(t, s.ds, func(tx sqlx.ExtContext) error { - _, err := tx.ExecContext(ctx, "UPDATE host_script_results SET created_at = ? WHERE execution_id IN (?, ?)", time.Now().Add(-24*time.Hour), h1A, h1B) + _, err := tx.ExecContext(ctx, "UPDATE host_script_results SET created_at = ? WHERE execution_id = ?", time.Now().Add(-24*time.Hour), h1A) + if err != nil { + return err + } + _, err = tx.ExecContext(ctx, "UPDATE host_script_results SET created_at = ? WHERE execution_id = ?", time.Now().Add(-23*time.Hour), h1B) return err }) @@ -12237,22 +12240,22 @@ func (s *integrationTestSuite) TestListHostUpcomingActivities() { wantMeta *fleet.PaginationMetadata }{ { - wantExecs: []string{h1B, h1Foo, h1C, h1D, h1E}, + wantExecs: []string{h1A, h1B, h1Foo, h1C, h1D, h1E}, wantMeta: &fleet.PaginationMetadata{HasNextResults: false, HasPreviousResults: false}, }, { queries: []string{"per_page", "2"}, - wantExecs: []string{h1B, h1Foo}, + wantExecs: []string{h1A, h1B}, wantMeta: &fleet.PaginationMetadata{HasNextResults: true, HasPreviousResults: false}, }, { queries: []string{"per_page", "2", "page", "1"}, - wantExecs: []string{h1C, h1D}, + wantExecs: []string{h1Foo, h1C}, wantMeta: &fleet.PaginationMetadata{HasNextResults: true, HasPreviousResults: true}, }, { queries: []string{"per_page", "2", "page", "2"}, - wantExecs: []string{h1E}, + wantExecs: []string{h1D, h1E}, wantMeta: &fleet.PaginationMetadata{HasNextResults: false, HasPreviousResults: true}, }, { @@ -12262,12 +12265,12 @@ func (s *integrationTestSuite) TestListHostUpcomingActivities() { }, { queries: []string{"per_page", "3"}, - wantExecs: []string{h1B, h1Foo, h1C}, + wantExecs: []string{h1A, h1B, h1Foo}, wantMeta: &fleet.PaginationMetadata{HasNextResults: true, HasPreviousResults: false}, }, { queries: []string{"per_page", "3", "page", "1"}, - wantExecs: []string{h1D, h1E}, + wantExecs: []string{h1C, h1D, h1E}, wantMeta: &fleet.PaginationMetadata{HasNextResults: false, HasPreviousResults: true}, }, { @@ -12282,7 +12285,7 @@ func (s *integrationTestSuite) TestListHostUpcomingActivities() { queryArgs := c.queries s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d/activities/upcoming", host1.ID), nil, http.StatusOK, &listResp, queryArgs...) - require.Equal(t, uint(5), listResp.Count) + require.Equal(t, uint(6), listResp.Count) require.Equal(t, len(c.wantExecs), len(listResp.Activities)) require.Equal(t, c.wantMeta, listResp.Meta) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 822cba9e51e4..84f97b7bac55 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -7472,6 +7472,7 @@ func (s *integrationEnterpriseTestSuite) TestHostScriptDetails() { }) require.NoError(t, err) + // TODO(uniq): use datastore methods insertResults := func(t *testing.T, hostID uint, script *fleet.Script, createdAt time.Time, execID string, exitCode *int64) { stmt := ` INSERT INTO From b4601938dfbef75c504a333b3431614d6c0dec7a Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 11:33:11 -0500 Subject: [PATCH 05/15] Fix test host software result integration test --- server/service/integration_enterprise_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 84f97b7bac55..56322ed4515b 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -12839,7 +12839,7 @@ func (s *integrationEnterpriseTestSuite) TestHostSoftwareInstallResult() { latestInstallUUID := func() string { var id string mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(ctx, q, &id, `SELECT execution_id FROM host_software_installs ORDER BY id DESC LIMIT 1`) + return sqlx.GetContext(ctx, q, &id, `SELECT execution_id FROM upcoming_activities ORDER BY id DESC LIMIT 1`) }) return id } From e2d37514b0c60825e1f6292fa80ac21f17a5b6e2 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 11:54:00 -0500 Subject: [PATCH 06/15] Start debugging another test --- server/service/integration_enterprise_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 56322ed4515b..66788dde5f02 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -12343,7 +12343,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { s.Do("DELETE", fmt.Sprintf("/api/latest/fleet/software/titles/%d/available_for_install", pkgTitleID), nil, http.StatusNoContent, "team_id", fmt.Sprintf("%d", *teamID)) - // install script request succeeds + // install software request succeeds resp = installSoftwareResponse{} s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/software/%d/install", h.ID, titleID), nil, http.StatusAccepted, &resp) @@ -12545,6 +12545,12 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { assert.Nil(t, getHostSoftwareResp.Software[0].SoftwarePackage.LastInstall) assert.Empty(t, getHostSoftwareResp.Software[0].InstalledVersions, "Installed versions should now not exist") + fmt.Println(">>>> h2.ID: ", h2.ID) + mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + mysql.DumpTable(t, q, "upcoming_activities") + return nil + }) + // Mark original install successful s.Do("POST", "/api/fleet/orbit/software_install/result", json.RawMessage(fmt.Sprintf(`{ "orbit_node_key": %q, From 5332ee61867fa080f2cef506126c4c4c3a35061f Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 14:35:42 -0500 Subject: [PATCH 07/15] Fix datastore test --- server/datastore/mysql/scripts.go | 27 ++++++++++--------- server/datastore/mysql/scripts_test.go | 4 +-- server/service/integration_enterprise_test.go | 17 +++++------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/server/datastore/mysql/scripts.go b/server/datastore/mysql/scripts.go index 76eac2a408ad..29e156c78aa4 100644 --- a/server/datastore/mysql/scripts.go +++ b/server/datastore/mysql/scripts.go @@ -276,11 +276,13 @@ func (ds *Datastore) ListReadyToExecuteScriptsForHost(ctx context.Context, hostI func (ds *Datastore) listUpcomingHostScriptExecutions(ctx context.Context, hostID uint, onlyShowInternal, onlyReadyToExecute bool) ([]*fleet.HostScriptResult, error) { extraWhere := "" if onlyShowInternal { - extraWhere = " AND ua.payload->'$.is_internal' = 1" + // software_uninstalls are implicitly internal + extraWhere = " AND COALESCE(ua.payload->'$.is_internal', 1) = 1" } if onlyReadyToExecute { extraWhere += " AND ua.activated_at IS NOT NULL" } + // this selects software uninstalls too as they run as scripts listStmt := fmt.Sprintf(` SELECT id, @@ -299,13 +301,14 @@ func (ds *Datastore) listUpcomingHostScriptExecutions(ctx context.Context, hostI IF(ua.activated_at IS NULL, 0, 1) AS topmost FROM upcoming_activities ua - INNER JOIN script_upcoming_activities sua + -- left join because software_uninstall has no script join + LEFT JOIN script_upcoming_activities sua ON ua.id = sua.upcoming_activity_id WHERE ua.host_id = ? AND - ua.activity_type = 'script' AND + ua.activity_type IN ('script', 'software_uninstall') AND ( - ua.payload->'$.sync_request' = 0 OR + COALESCE(ua.payload->'$.sync_request', 0) = 0 OR ua.created_at >= DATE_SUB(NOW(), INTERVAL ? SECOND) ) %s @@ -911,7 +914,7 @@ WHERE INNER JOIN script_upcoming_activities sua ON upcoming_activities.id = sua.upcoming_activity_id WHERE - upcoming_activities.activity_type = 'script' + upcoming_activities.activity_type = 'script' AND (upcoming_activities.payload->'$.sync_request' = 0 OR upcoming_activities.created_at >= NOW() - INTERVAL ? SECOND) AND sua.script_id IN (SELECT id FROM scripts WHERE global_or_team_id = ?)` @@ -932,13 +935,13 @@ WHERE exit_code IS NULL AND (sync_request = 0 OR created_at >= NOW() - INTERVAL ? SECOND) AND script_id IN (SELECT id FROM scripts WHERE global_or_team_id = ? AND name NOT IN (?))` - const clearPendingExecutionsNotInListUA = `DELETE FROM upcoming_activities - USING + const clearPendingExecutionsNotInListUA = `DELETE FROM upcoming_activities + USING upcoming_activities INNER JOIN script_upcoming_activities sua ON upcoming_activities.id = sua.upcoming_activity_id WHERE - upcoming_activities.activity_type = 'script' + upcoming_activities.activity_type = 'script' AND (upcoming_activities.payload->'$.sync_request' = 0 OR upcoming_activities.created_at >= NOW() - INTERVAL ? SECOND) AND sua.script_id IN (SELECT id FROM scripts WHERE global_or_team_id = ? AND name NOT IN (?))` @@ -957,13 +960,13 @@ ON DUPLICATE KEY UPDATE exit_code IS NULL AND (sync_request = 0 OR created_at >= NOW() - INTERVAL ? SECOND) AND script_id = ? AND script_content_id != ?` - const clearPendingExecutionsWithObsoleteScriptUA = `DELETE FROM upcoming_activities - USING - upcoming_activities + const clearPendingExecutionsWithObsoleteScriptUA = `DELETE FROM upcoming_activities + USING + upcoming_activities INNER JOIN script_upcoming_activities sua ON upcoming_activities.id = sua.upcoming_activity_id WHERE - upcoming_activities.activity_type = 'script' + upcoming_activities.activity_type = 'script' AND (upcoming_activities.payload->'$.sync_request' = 0 OR upcoming_activities.created_at >= NOW() - INTERVAL ? SECOND) AND sua.script_id = ? AND sua.script_content_id != ?` diff --git a/server/datastore/mysql/scripts_test.go b/server/datastore/mysql/scripts_test.go index 09dff717b717..7d318d7651b6 100644 --- a/server/datastore/mysql/scripts_test.go +++ b/server/datastore/mysql/scripts_test.go @@ -232,8 +232,8 @@ func testHostScriptResult(t *testing.T, ds *Datastore) { // modify the upcoming script to be a sync script that has // been pending for a long time ExecAdhocSQL(t, ds, func(tx sqlx.ExtContext) error { - _, err := tx.ExecContext(ctx, "UPDATE upcoming_activities SET created_at = ?, payload = JSON_SET(payload, '$.sync_request', true) WHERE id = ?", - time.Now().Add(-24*time.Hour), createdScript3.ID) + _, err := tx.ExecContext(ctx, "UPDATE upcoming_activities SET created_at = ?, payload = JSON_SET(payload, '$.sync_request', ?) WHERE id = ?", + time.Now().Add(-24*time.Hour), true, createdScript3.ID) return err }) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 66788dde5f02..9417972bd9c9 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -12524,7 +12524,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { assert.NotNil(t, getHostSoftwareResp.Software[0].SoftwarePackage.LastInstall) assert.NotEmpty(t, getHostSoftwareResp.Software[0].InstalledVersions, "Installed versions should exist") - // Remove the installed app by not returning it + // Remove the installed app of h2 by not returning it distributedReq = submitDistributedQueryResultsRequestShim{ NodeKey: *h2.NodeKey, Results: map[string]json.RawMessage{ @@ -12545,13 +12545,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { assert.Nil(t, getHostSoftwareResp.Software[0].SoftwarePackage.LastInstall) assert.Empty(t, getHostSoftwareResp.Software[0].InstalledVersions, "Installed versions should now not exist") - fmt.Println(">>>> h2.ID: ", h2.ID) - mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - mysql.DumpTable(t, q, "upcoming_activities") - return nil - }) - - // Mark original install successful + // Mark original h install successful s.Do("POST", "/api/fleet/orbit/software_install/result", json.RawMessage(fmt.Sprintf(`{ "orbit_node_key": %q, "install_uuid": %q, @@ -12560,7 +12554,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { "install_script_output": "ok" }`, *h.OrbitNodeKey, installUUID)), http.StatusNoContent) - // simulate a lock/unlock; this creates the host_mdm_actions table, which reproduces #25144 + // simulate a lock/unlock on h; this creates the host_mdm_actions table, which reproduces #25144 s.Do("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/lock", h.ID), nil, http.StatusOK) status, err := s.ds.GetHostLockWipeStatus(context.Background(), h) require.NoError(t, err) @@ -12575,7 +12569,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { json.RawMessage(fmt.Sprintf(`{"orbit_node_key": %q, "execution_id": %q, "exit_code": 0, "output": "ok"}`, *h.OrbitNodeKey, status.UnlockScript.ExecutionID)), http.StatusOK, &orbitScriptResp) - // Do uninstall + // Do uninstall on h s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/software/%d/uninstall", h.ID, titleID), nil, http.StatusAccepted, &resp) s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d/software", h.ID), nil, http.StatusOK, &getHostSoftwareResp) require.Len(t, getHostSoftwareResp.Software, 1) @@ -12610,12 +12604,13 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() { s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/software/%d/install", h.ID, titleID), nil, http.StatusBadRequest, &resp) s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/software/%d/uninstall", h.ID, titleID), nil, http.StatusBadRequest, &resp) - // expect uninstall script to be pending + // expect uninstall script (uninstall software works via a script) to be pending var orbitResp orbitGetConfigResponse s.DoJSON("POST", "/api/fleet/orbit/config", json.RawMessage(fmt.Sprintf(`{"orbit_node_key": %q}`, *h.OrbitNodeKey)), http.StatusOK, &orbitResp) require.Len(t, orbitResp.Notifications.PendingScriptExecutionIDs, 1) + require.Equal(t, uninstallExecutionID, orbitResp.Notifications.PendingScriptExecutionIDs[0]) // Host sends successful uninstall result var orbitPostScriptResp orbitPostScriptResultResponse From e58e30a0c73ee37b1120b4bb5945aa12e275b9a7 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 14:44:30 -0500 Subject: [PATCH 08/15] Modify the sync script error message --- server/fleet/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/fleet/errors.go b/server/fleet/errors.go index 8dc616bdb54a..5e6e024f2282 100644 --- a/server/fleet/errors.go +++ b/server/fleet/errors.go @@ -613,7 +613,7 @@ const ( RunScriptHostOfflineErrMsg = "Script can't run on offline host." RunScriptForbiddenErrMsg = "You don't have the right permissions in Fleet to run the script." RunScriptAlreadyRunningErrMsg = "A script is already running on this host. Please wait about 5 minutes to let it finish." - RunScriptHostTimeoutErrMsg = "Fleet didn't hear back from the host in under 5 minutes (timeout for live scripts). Fleet doesn't know if the script ran because it didn't receive the result. Please try again." + RunScriptHostTimeoutErrMsg = "Fleet didn't hear back from the host in under 5 minutes (timeout for live scripts). Fleet doesn't know if the script ran because it didn't receive the result. Go to Fleet and check Host details > Activities to see script results." RunScriptScriptsDisabledGloballyErrMsg = "Running scripts is disabled in organization settings." RunScriptDisabledErrMsg = "Scripts are disabled for this host. To run scripts, deploy the fleetd agent with scripts enabled." RunScriptsOrbitDisabledErrMsg = "Couldn't run script. To run a script, deploy the fleetd agent with --enable-scripts." From e33ca416d676d87362b028bcb0c076ff4af772ec Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 15:02:51 -0500 Subject: [PATCH 09/15] All test suite now passes! --- server/service/integration_core_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index c9d75f1b4ff9..d40207ae8e87 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -12226,11 +12226,11 @@ func (s *integrationTestSuite) TestListHostUpcomingActivities() { // anything anymore, sync scripts are part of the queue like any other: // https://github.com/fleetdm/fleet/issues/22866#issuecomment-2575961141) mysql.ExecAdhocSQL(t, s.ds, func(tx sqlx.ExtContext) error { - _, err := tx.ExecContext(ctx, "UPDATE host_script_results SET created_at = ? WHERE execution_id = ?", time.Now().Add(-24*time.Hour), h1A) + _, err := tx.ExecContext(ctx, "UPDATE upcoming_activities SET created_at = ? WHERE execution_id = ?", time.Now().Add(-24*time.Hour), h1A) if err != nil { return err } - _, err = tx.ExecContext(ctx, "UPDATE host_script_results SET created_at = ? WHERE execution_id = ?", time.Now().Add(-23*time.Hour), h1B) + _, err = tx.ExecContext(ctx, "UPDATE upcoming_activities SET created_at = ? WHERE execution_id = ?", time.Now().Add(-23*time.Hour), h1B) return err }) From 7472383cda9b95ad8b783a48e1be9f9862e05a56 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 15:15:00 -0500 Subject: [PATCH 10/15] Remove most uses of host_software_installs in tests --- server/service/integration_enterprise_test.go | 31 +++---------------- server/service/integration_install_test.go | 15 ++++++--- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 9417972bd9c9..459eed482286 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -11038,10 +11038,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerUploadDownloadAndD checkDownloadResponse(t, r, payload.Filename) // Get execution ID, normally comes from orbit config - var installUUID string - mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(context.Background(), q, &installUUID, "SELECT execution_id FROM host_software_installs WHERE host_id = ? AND install_script_exit_code IS NULL", hostInTeam.ID) - }) + installUUID := getLatestSoftwareInstallExecID(t, s.ds, hostInTeam.ID) // Installation complete, host no longer has access to software s.Do("POST", "/api/fleet/orbit/software_install/result", orbitPostSoftwareInstallResultRequest{ @@ -11130,10 +11127,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerUploadDownloadAndD checkDownloadResponse(t, r, payload.Filename) // Get execution ID, normally comes from orbit config - var installUUID string - mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(context.Background(), q, &installUUID, "SELECT execution_id FROM host_software_installs WHERE host_id = ? AND install_script_exit_code IS NULL", hostInTeam.ID) - }) + installUUID := getLatestSoftwareInstallExecID(t, s.ds, hostInTeam.ID) // Installation complete, host no longer has access to software s.Do("POST", "/api/fleet/orbit/software_install/result", orbitPostSoftwareInstallResultRequest{ @@ -16053,7 +16047,6 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallersWithoutBundleIden } func (s *integrationEnterpriseTestSuite) TestSoftwareUploadRPM() { - ctx := context.Background() t := s.T() // Fedora and RHEL have hosts.platform = 'rhel'. @@ -16070,19 +16063,11 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareUploadRPM() { s.uploadSoftwareInstaller(t, payload, http.StatusOK, "") titleID := getSoftwareTitleID(t, s.ds, payload.Title, "rpm_packages") - latestInstallUUID := func() string { - var id string - mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(ctx, q, &id, `SELECT execution_id FROM host_software_installs ORDER BY id DESC LIMIT 1`) - }) - return id - } - // Send a request to the host to install the RPM package. var installSoftwareResp installSoftwareResponse beforeInstallRequest := time.Now() s.DoJSON("POST", fmt.Sprintf("/api/v1/fleet/hosts/%d/software/%d/install", host.ID, titleID), nil, http.StatusAccepted, &installSoftwareResp) - installUUID := latestInstallUUID() + installUUID := getLatestSoftwareInstallExecID(t, s.ds, host.ID) // Simulate host installing the RPM package. beforeInstallResult := time.Now() @@ -16652,18 +16637,10 @@ func (s *integrationEnterpriseTestSuite) TestListHostSoftwareWithLabelScoping() s.uploadSoftwareInstaller(t, payload, http.StatusOK, "") titleID := getSoftwareTitleID(t, s.ds, payload.Title, "deb_packages") - latestInstallUUID := func() string { - var id string - mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(ctx, q, &id, `SELECT execution_id FROM host_software_installs ORDER BY id DESC LIMIT 1`) - }) - return id - } - // create install request for the software and record a successful result resp := installSoftwareResponse{} s.DoJSON("POST", fmt.Sprintf("/api/v1/fleet/hosts/%d/software/%d/install", host.ID, titleID), nil, http.StatusAccepted, &resp) - installUUID := latestInstallUUID() + installUUID := getLatestSoftwareInstallExecID(t, s.ds, host.ID) s.Do("POST", "/api/fleet/orbit/software_install/result", json.RawMessage(fmt.Sprintf(`{ diff --git a/server/service/integration_install_test.go b/server/service/integration_install_test.go index 734218b87acb..14d8ab10ad43 100644 --- a/server/service/integration_install_test.go +++ b/server/service/integration_install_test.go @@ -152,11 +152,7 @@ func (s *integrationInstallTestSuite) TestSoftwareInstallerSignedURL() { http.StatusAccepted) // Get the InstallerUUID - var installUUID string - mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(context.Background(), q, &installUUID, - "SELECT execution_id FROM host_software_installs WHERE host_id = ?", hostInTeam.ID) - }) + installUUID := getLatestSoftwareInstallExecID(t, s.ds, hostInTeam.ID) // Fetch installer details var orbitSoftwareResp orbitGetSoftwareInstallResponse @@ -210,3 +206,12 @@ func (s *integrationInstallTestSuite) TestSoftwareInstallerSignedURL() { require.Equal(t, filename, orbitSoftwareResp.SoftwareInstallerURL.Filename) } + +func getLatestSoftwareInstallExecID(t *testing.T, ds *mysql.Datastore, hostID uint) string { + var installUUID string + mysql.ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(context.Background(), q, &installUUID, + "SELECT execution_id FROM host_software_installs WHERE host_id = ? ORDER BY id desc", hostID) + }) + return installUUID +} From 7ca3c6a619325df3078d4f23a3388228b566fc6b Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 16:01:00 -0500 Subject: [PATCH 11/15] Fix sync script pending behavior --- server/datastore/mysql/scripts.go | 9 ++------- server/datastore/mysql/scripts_test.go | 8 +++++--- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/server/datastore/mysql/scripts.go b/server/datastore/mysql/scripts.go index 29e156c78aa4..4066f12d342f 100644 --- a/server/datastore/mysql/scripts.go +++ b/server/datastore/mysql/scripts.go @@ -306,17 +306,12 @@ func (ds *Datastore) listUpcomingHostScriptExecutions(ctx context.Context, hostI ON ua.id = sua.upcoming_activity_id WHERE ua.host_id = ? AND - ua.activity_type IN ('script', 'software_uninstall') AND - ( - COALESCE(ua.payload->'$.sync_request', 0) = 0 OR - ua.created_at >= DATE_SUB(NOW(), INTERVAL ? SECOND) - ) + ua.activity_type IN ('script', 'software_uninstall') %s ORDER BY topmost DESC, priority DESC, created_at ASC) t`, extraWhere) var results []*fleet.HostScriptResult - seconds := int(constants.MaxServerWaitTime.Seconds()) - if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, listStmt, hostID, seconds); err != nil { + if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, listStmt, hostID); err != nil { return nil, ctxerr.Wrap(ctx, err, "list pending host script executions") } return results, nil diff --git a/server/datastore/mysql/scripts_test.go b/server/datastore/mysql/scripts_test.go index 7d318d7651b6..16e2d744120c 100644 --- a/server/datastore/mysql/scripts_test.go +++ b/server/datastore/mysql/scripts_test.go @@ -230,17 +230,19 @@ func testHostScriptResult(t *testing.T, ds *Datastore) { require.Equal(t, createdScript3.ID, pending[0].ID) // modify the upcoming script to be a sync script that has - // been pending for a long time + // been pending for a long time doesn't change result + // https://github.com/fleetdm/fleet/issues/22866#issuecomment-2575961141 ExecAdhocSQL(t, ds, func(tx sqlx.ExtContext) error { _, err := tx.ExecContext(ctx, "UPDATE upcoming_activities SET created_at = ?, payload = JSON_SET(payload, '$.sync_request', ?) WHERE id = ?", time.Now().Add(-24*time.Hour), true, createdScript3.ID) return err }) - // the script is not pending anymore + // the script is still pending pending, err = ds.ListPendingHostScriptExecutions(ctx, 1, false) require.NoError(t, err) - require.Len(t, pending, 0) + require.Len(t, pending, 1) + require.Equal(t, createdScript3.ExecutionID, pending[0].ExecutionID) // check that scripts with large unsigned error codes get // converted to signed error codes From b294ac97a9fa9e1a2fec87ebdf935517d4039bd0 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 4 Feb 2025 16:16:45 -0500 Subject: [PATCH 12/15] Fix a build failure after merging main --- server/datastore/mysql/software_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/datastore/mysql/software_test.go b/server/datastore/mysql/software_test.go index bb15b3cf0bfc..5596981db460 100644 --- a/server/datastore/mysql/software_test.go +++ b/server/datastore/mysql/software_test.go @@ -6000,7 +6000,7 @@ func testDeletedInstalledSoftware(t *testing.T, ds *Datastore) { ValidatedLabels: &fleet.LabelIdentsWithScope{}, }) require.NoError(t, err) - _, err = ds.InsertSoftwareInstallRequest(ctx, host1.ID, installerID, false, nil) + _, err = ds.InsertSoftwareInstallRequest(ctx, host1.ID, installerID, fleet.HostSoftwareInstallOptions{}) require.NoError(t, err) ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { From 135e4c94878e77241d5ad3f91d1aacecd3e1a9c1 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Wed, 5 Feb 2025 08:29:19 -0500 Subject: [PATCH 13/15] Fix some comments --- server/datastore/mysql/vpp.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index a5681238113a..bb3753d3b11d 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -117,10 +117,10 @@ func (ds *Datastore) GetSummaryHostVPPAppInstalls(ctx context.Context, teamID *u // Currently there is no host_deleted_at in host_vpp_software_installs, so // not handling it as part of the unified queue work. - // TODO(uniq): refactor vppHostStatusNamedQuery to use the same logic as below + // TODO(sarah): refactor vppHostStatusNamedQuery to use the same logic as below // re: ^ : seems like the logic is the same except for pending/null? - // TODO(Sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or + // TODO(sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or // past activities. In the past the max(id) approach was "good enough" as a proxy for the most // recent activity since we didn't really need to worry about the order of activities. // Moving to a time-based approach would be more accurate but would require a more complex and @@ -181,8 +181,6 @@ SELECT WHEN ncr.status = :mdm_status_error OR ncr.status = :mdm_status_format_error THEN :software_status_failed ELSE - -- TODO should that count as pending install (should be covered by the upcoming - -- case, but seems more correct to report as pending it is found in past) NULL -- either pending or not installed via VPP App END AS status FROM From 663bf5562e30c21ec760acda0a4746ab340d83ed Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Wed, 5 Feb 2025 08:32:54 -0500 Subject: [PATCH 14/15] Fix comment --- server/datastore/mysql/vpp.go | 1 - 1 file changed, 1 deletion(-) diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index bb3753d3b11d..1c40f3de5c7d 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -118,7 +118,6 @@ func (ds *Datastore) GetSummaryHostVPPAppInstalls(ctx context.Context, teamID *u // not handling it as part of the unified queue work. // TODO(sarah): refactor vppHostStatusNamedQuery to use the same logic as below - // re: ^ : seems like the logic is the same except for pending/null? // TODO(sarah): AFAICT we don't have uniqueness for host_id + title_id in upcoming or // past activities. In the past the max(id) approach was "good enough" as a proxy for the most From c85816bf2d3921ebf38cf2aacaffcb26292a21de Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Wed, 5 Feb 2025 08:38:39 -0500 Subject: [PATCH 15/15] Fix comment --- server/service/integration_enterprise_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 459eed482286..a6aeb57d4e4b 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -7472,7 +7472,8 @@ func (s *integrationEnterpriseTestSuite) TestHostScriptDetails() { }) require.NoError(t, err) - // TODO(uniq): use datastore methods + // it's fine to write directly to host_script_results here because we're not testing the execution + // only the script details insertResults := func(t *testing.T, hostID uint, script *fleet.Script, createdAt time.Time, execID string, exitCode *int64) { stmt := ` INSERT INTO