From b2e6162e51107b60b002b1add0e1439d83a6456c Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Wed, 8 Apr 2026 17:03:08 -0500 Subject: [PATCH] Fix issue with GitOps incorrectly wiping policy stats (#43282) **Related issue:** Resolves #43273 # 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](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] Added/updated automated tests - Added new test for this case (policies without software automation being pushed by two different users), verified it fails on main and passes on this branch - [X] QA'd all new/changed functionality manually - [X] Verified that changing `webhooks_and_tickets_enabled` on a policy AND running gitops as another user doesn't wipe stats - [X] Verified that changing `query` on a policy and running gitops does wipe stats - [X] Verified that changing `query` on a policy and running gitops does wipe stats ## Summary by CodeRabbit * **Bug Fixes** * Fixed an issue where policy stats were incorrectly reset during GitOps policy updates. Policy statistics now remain accurate when policies are re-applied without modification to installation or script configurations. --- 43273-fix-policy-stats-wipe | 1 + server/datastore/mysql/policies.go | 18 ++---- server/datastore/mysql/policies_test.go | 85 +++++++++++++++++++++++++ server/ptr/ptr.go | 12 ++++ 4 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 43273-fix-policy-stats-wipe diff --git a/43273-fix-policy-stats-wipe b/43273-fix-policy-stats-wipe new file mode 100644 index 0000000000..51f403c7a1 --- /dev/null +++ b/43273-fix-policy-stats-wipe @@ -0,0 +1 @@ +- Fixed an issue where policy stats may be wiped incorrectly after a GitOps run. diff --git a/server/datastore/mysql/policies.go b/server/datastore/mysql/policies.go index 705a012bc7..67b1bea07b 100644 --- a/server/datastore/mysql/policies.go +++ b/server/datastore/mysql/policies.go @@ -1519,29 +1519,19 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs removePolicyStats bool ) if insertOnDuplicateDidInsertOrUpdate(res) { - // Figure out if the query, platform, software installer, or VPP app changed. - var softwareInstallerID *uint - if spec.SoftwareTitleID != nil { - softwareInstallerID = softwareInstallerIDs[teamID][*spec.SoftwareTitleID] - } + // Figure out if the query, platform, software installer, VPP app, or script changed. if prev, ok := teamIDToPoliciesByName[teamID][spec.Name]; ok { switch { case prev.Query != spec.Query: shouldRemoveAllPolicyMemberships = true removePolicyStats = true - case teamID != nil && - ((prev.SoftwareInstallerID == nil && spec.SoftwareTitleID != nil) || - (prev.SoftwareInstallerID != nil && softwareInstallerID != nil && *prev.SoftwareInstallerID != *softwareInstallerID)): + case teamID != nil && softwareInstallerID != nil && !ptr.Equal(prev.SoftwareInstallerID, softwareInstallerID): shouldRemoveAllPolicyMemberships = true removePolicyStats = true - case teamID != nil && - ((prev.VPPAppsTeamsID == nil && spec.SoftwareTitleID != nil) || - (prev.VPPAppsTeamsID != nil && vppAppsTeamsID != nil && *prev.VPPAppsTeamsID != *vppAppsTeamsID)): + case teamID != nil && vppAppsTeamsID != nil && !ptr.Equal(prev.VPPAppsTeamsID, vppAppsTeamsID): shouldRemoveAllPolicyMemberships = true removePolicyStats = true - case teamID != nil && - ((prev.ScriptID == nil && spec.ScriptID != nil) || - (prev.ScriptID != nil && spec.ScriptID != nil && *prev.ScriptID != *spec.ScriptID)): + case teamID != nil && scriptID != nil && !ptr.Equal(prev.ScriptID, scriptID): shouldRemoveAllPolicyMemberships = true removePolicyStats = true case prev.Platforms != spec.Platform: diff --git a/server/datastore/mysql/policies_test.go b/server/datastore/mysql/policies_test.go index f6a5b58a0e..46cc3b2541 100644 --- a/server/datastore/mysql/policies_test.go +++ b/server/datastore/mysql/policies_test.go @@ -93,6 +93,7 @@ func TestPolicies(t *testing.T) { {"BatchedPolicyMembershipCleanupOnPolicyUpdate", testBatchedPolicyMembershipCleanupOnPolicyUpdate}, {"ApplyPolicySpecsNeedsFullMembershipCleanupFlag", testApplyPolicySpecsNeedsFullMembershipCleanupFlag}, {"CleanupPolicyMembershipCrashRecovery", testCleanupPolicyMembershipCrashRecovery}, + {"ApplyPolicySpecNoSpuriousStatsReset", testApplyPolicySpecNoSpuriousStatsReset}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -7948,3 +7949,87 @@ func testTeamPolicyAutomationFilter(t *testing.T, ds *Datastore) { require.NoError(t, err) assert.Equal(t, 3, mergedCount) } + +// testApplyPolicySpecNoSpuriousStatsReset verifies that re-applying a team +// policy via ApplyPolicySpecs does not spuriously reset policy_stats when the +// only change is author_id (e.g. a different user runs GitOps) and the spec +// has SoftwareTitleID set to ptr.Uint(0) (as the GitOps client does for all +// team policies to "unset" the installer field). +func testApplyPolicySpecNoSpuriousStatsReset(t *testing.T, ds *Datastore) { + ctx := t.Context() + + user1 := test.NewUser(t, ds, "StatsUser1", "statsuser1@example.com", true) + user2 := test.NewUser(t, ds, "StatsUser2", "statsuser2@example.com", true) + team, err := ds.NewTeam(ctx, &fleet.Team{Name: t.Name()}) + require.NoError(t, err) + + // Create a team policy (no software installer) as user1. + require.NoError(t, ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{ + { + Name: "stats-test-policy", + Query: "SELECT 1;", + Team: team.Name, + Platform: "darwin", + Type: fleet.PolicyTypeDynamic, + }, + })) + + // Create a host on the team and record a failing result. + hostID := fmt.Sprintf("%s-host", strings.ReplaceAll(t.Name(), "/", "_")) + h, err := ds.NewHost(ctx, &fleet.Host{ + OsqueryHostID: &hostID, + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now(), + NodeKey: &hostID, + UUID: hostID, + Hostname: hostID, + Platform: "darwin", + TeamID: &team.ID, + }) + require.NoError(t, err) + + // Get the policy to find its ID. + policies, _, err := ds.ListTeamPolicies(ctx, team.ID, fleet.ListOptions{}, fleet.ListOptions{}, "") + require.NoError(t, err) + require.Len(t, policies, 1) + pol := policies[0] + + // Record a failing result for the host. + err = ds.RecordPolicyQueryExecutions(ctx, h, map[uint]*bool{pol.ID: new(false)}, time.Now(), false, nil) + require.NoError(t, err) + + // Update aggregate counts. + err = ds.UpdateHostPolicyCounts(ctx) + require.NoError(t, err) + + // Verify the policy has a failing host count of 1. + policies, _, err = ds.ListTeamPolicies(ctx, team.ID, fleet.ListOptions{}, fleet.ListOptions{}, "") + require.NoError(t, err) + require.Len(t, policies, 1) + require.Equal(t, uint(1), policies[0].FailingHostCount) + + // Now re-apply the same policy as a DIFFERENT user (simulating GitOps run + // by a different API token), with SoftwareTitleID=ptr.Uint(0) as the GitOps + // client sets for all team policies. This changes author_id in the DB, which + // causes insertOnDuplicateDidInsertOrUpdate to return true. Before the fix, + // the comparison logic would see SoftwareTitleID!=nil and incorrectly + // conclude the software installer changed, resetting stats. + require.NoError(t, ds.ApplyPolicySpecs(ctx, user2.ID, []*fleet.PolicySpec{ + { + Name: "stats-test-policy", + Query: "SELECT 1;", + Team: team.Name, + Platform: "darwin", + SoftwareTitleID: new(uint), // ptr to 0, as GitOps does + Type: fleet.PolicyTypeDynamic, + }, + })) + + // Verify that policy stats were NOT reset. + policies, _, err = ds.ListTeamPolicies(ctx, team.ID, fleet.ListOptions{}, fleet.ListOptions{}, "") + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Equal(t, uint(1), policies[0].FailingHostCount, "policy stats should not have been reset") +} diff --git a/server/ptr/ptr.go b/server/ptr/ptr.go index ecc564c9f1..6baaa38a72 100644 --- a/server/ptr/ptr.go +++ b/server/ptr/ptr.go @@ -91,6 +91,18 @@ func T[T any](x T) *T { return &x } +// Equal returns true if both pointers are nil, or both are non-nil and +// point to equal values. +func Equal[T comparable](a, b *T) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + // ValOrZero returns the value of x if x is not nil, and the zero value // for T otherwise. func ValOrZero[T any](x *T) T {