Handle multiple software entries with the same bundle ID during renames. (#33479)
- Adjusted logic to support multiple software versions sharing a bundle ID. - Extended tests to validate scenarios involving renamed software across versions. <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #33468 # 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 - [x] Manually QA'd using osquery `--common_software_name_suffix` switch <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Improves handling of apps that share the same bundle ID, ensuring all versions are correctly linked and consistently renamed across hosts. - Reduces duplicate software entries and keeps host associations intact during rename operations. - Delivers more reliable software inventory views with accurate app names derived from bundle IDs. - Tests - Adds comprehensive coverage for scenarios with multiple versions per bundle ID to validate linking and renaming behavior across hosts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -0,0 +1 @@
|
||||
* Fixed edge case when renaming macOS software mapped to multiple checksums.
|
||||
@@ -455,16 +455,22 @@ func (ds *Datastore) applyChangesForNewSoftwareDB(
|
||||
// Check inserted software for renames
|
||||
for _, sw := range r.Inserted {
|
||||
if sw.BundleIdentifier != "" {
|
||||
if updSoftware, needsUpdate := existingBundleIDsToUpdate[sw.BundleIdentifier]; needsUpdate {
|
||||
softwareRenames[sw.ID] = updSoftware.Name
|
||||
if updSoftwareList, needsUpdate := existingBundleIDsToUpdate[sw.BundleIdentifier]; needsUpdate {
|
||||
// Use the first software in the list for the name (they should all have the same name)
|
||||
if len(updSoftwareList) > 0 {
|
||||
softwareRenames[sw.ID] = updSoftwareList[0].Name
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
// Check existing software for renames
|
||||
for _, s := range existingSoftware {
|
||||
if s.BundleIdentifier != nil && *s.BundleIdentifier != "" {
|
||||
if updSoftware, ok := existingBundleIDsToUpdate[*s.BundleIdentifier]; ok {
|
||||
softwareRenames[s.ID] = updSoftware.Name
|
||||
if updSoftwareList, ok := existingBundleIDsToUpdate[*s.BundleIdentifier]; ok {
|
||||
// Use the first software in the list for the name (they should all have the same name)
|
||||
if len(updSoftwareList) > 0 {
|
||||
softwareRenames[s.ID] = updSoftwareList[0].Name
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -671,15 +677,14 @@ func (ds *Datastore) getExistingSoftware(
|
||||
currentSoftware []softwareIDChecksum,
|
||||
incomingChecksumToSoftware map[string]fleet.Software,
|
||||
incomingChecksumToTitle map[string]fleet.SoftwareTitle,
|
||||
existingBundleIDsToUpdate map[string]fleet.Software,
|
||||
existingBundleIDsToUpdate map[string][]fleet.Software,
|
||||
err error,
|
||||
) {
|
||||
// Compute checksums for all incoming software, which we will use for faster retrieval, since checksum is a unique index
|
||||
incomingChecksumToSoftware = make(map[string]fleet.Software, len(current))
|
||||
newSoftware := make(map[string]struct{})
|
||||
bundleIDsToChecksum := make(map[string]string)
|
||||
bundleIDsToNames := make(map[string]string)
|
||||
existingBundleIDsToUpdate = make(map[string]fleet.Software)
|
||||
incomingBundleIDsToNewSoftwareNames := make(map[string]string)
|
||||
existingBundleIDsToUpdate = make(map[string][]fleet.Software)
|
||||
for uniqueName, s := range incoming {
|
||||
_, ok := current[uniqueName]
|
||||
if !ok {
|
||||
@@ -691,8 +696,7 @@ func (ds *Datastore) getExistingSoftware(
|
||||
newSoftware[string(checksum)] = struct{}{}
|
||||
|
||||
if s.BundleIdentifier != "" {
|
||||
bundleIDsToChecksum[s.BundleIdentifier] = string(checksum)
|
||||
bundleIDsToNames[s.BundleIdentifier] = s.Name
|
||||
incomingBundleIDsToNewSoftwareNames[s.BundleIdentifier] = s.Name
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -708,30 +712,29 @@ func (ds *Datastore) getExistingSoftware(
|
||||
if err != nil {
|
||||
return nil, nil, nil, nil, err
|
||||
}
|
||||
for _, s := range currentSoftware {
|
||||
sw, ok := incomingChecksumToSoftware[s.Checksum]
|
||||
|
||||
for _, currentSoftwareItem := range currentSoftware {
|
||||
incomingSoftwareItem, ok := incomingChecksumToSoftware[currentSoftwareItem.Checksum]
|
||||
if !ok {
|
||||
// This should never happen. If it does, we have a bug.
|
||||
return nil, nil, nil, nil, ctxerr.New(
|
||||
ctx, fmt.Sprintf("current software: software not found for checksum %s", hex.EncodeToString([]byte(s.Checksum))),
|
||||
ctx, fmt.Sprintf("current software: software not found for checksum %s", hex.EncodeToString([]byte(currentSoftwareItem.Checksum))),
|
||||
)
|
||||
}
|
||||
if s.BundleIdentifier != nil && s.Source == "apps" {
|
||||
if name, ok := bundleIDsToNames[*s.BundleIdentifier]; ok && name != s.Name {
|
||||
if currentSoftwareItem.BundleIdentifier != nil && currentSoftwareItem.Source == "apps" {
|
||||
if name, ok := incomingBundleIDsToNewSoftwareNames[*currentSoftwareItem.BundleIdentifier]; ok && name != currentSoftwareItem.Name {
|
||||
// Then this is a software whose name has changed, so we should update the name
|
||||
// Copy the incoming software but with the existing software's ID
|
||||
swWithID := sw
|
||||
swWithID.ID = s.ID
|
||||
existingBundleIDsToUpdate[*s.BundleIdentifier] = swWithID
|
||||
swWithID := incomingSoftwareItem
|
||||
swWithID.ID = currentSoftwareItem.ID
|
||||
existingBundleIDsToUpdate[*currentSoftwareItem.BundleIdentifier] = append(existingBundleIDsToUpdate[*currentSoftwareItem.BundleIdentifier], swWithID)
|
||||
|
||||
// Remove from incomingChecksumToSoftware to prevent it being treated as new software
|
||||
if cs, ok := bundleIDsToChecksum[*s.BundleIdentifier]; ok {
|
||||
delete(incomingChecksumToSoftware, cs)
|
||||
}
|
||||
// Delete this checksum to prevent it from being treated as new software
|
||||
delete(incomingChecksumToSoftware, currentSoftwareItem.Checksum)
|
||||
continue
|
||||
}
|
||||
}
|
||||
delete(newSoftware, s.Checksum)
|
||||
delete(newSoftware, currentSoftwareItem.Checksum)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1132,7 +1135,7 @@ func (ds *Datastore) linkExistingBundleIDSoftware(
|
||||
ctx context.Context,
|
||||
tx sqlx.ExtContext,
|
||||
hostID uint,
|
||||
existingBundleIDsToUpdate map[string]fleet.Software,
|
||||
existingBundleIDsToUpdate map[string][]fleet.Software,
|
||||
) ([]fleet.Software, error) {
|
||||
if len(existingBundleIDsToUpdate) == 0 {
|
||||
return nil, nil
|
||||
@@ -1140,12 +1143,14 @@ func (ds *Datastore) linkExistingBundleIDSoftware(
|
||||
|
||||
// Collect all software IDs to verify they still exist
|
||||
softwareIDs := make([]uint, 0, len(existingBundleIDsToUpdate))
|
||||
for _, software := range existingBundleIDsToUpdate {
|
||||
// The software.ID should already be set from getExistingSoftware
|
||||
if software.ID == 0 {
|
||||
return nil, ctxerr.New(ctx, "software ID not set for bundle ID match")
|
||||
for _, softwareList := range existingBundleIDsToUpdate {
|
||||
for _, software := range softwareList {
|
||||
// The software.ID should already be set from getExistingSoftware
|
||||
if software.ID == 0 {
|
||||
return nil, ctxerr.New(ctx, "software ID not set for bundle ID match")
|
||||
}
|
||||
softwareIDs = append(softwareIDs, software.ID)
|
||||
}
|
||||
softwareIDs = append(softwareIDs, software.ID)
|
||||
}
|
||||
|
||||
// Verify software still exists (just like we do in linkSoftwareToHost)
|
||||
@@ -1170,19 +1175,21 @@ func (ds *Datastore) linkExistingBundleIDSoftware(
|
||||
var insertsHostSoftware []any
|
||||
var insertedSoftware []fleet.Software
|
||||
|
||||
for _, software := range existingBundleIDsToUpdate {
|
||||
// Only link if software still exists
|
||||
if _, ok := existingIDSet[software.ID]; ok {
|
||||
insertsHostSoftware = append(insertsHostSoftware, hostID, software.ID, software.LastOpenedAt)
|
||||
insertedSoftware = append(insertedSoftware, software)
|
||||
} else {
|
||||
// Log missing software but continue
|
||||
level.Warn(ds.logger).Log(
|
||||
"msg", "bundle ID software not found after pre-insertion",
|
||||
"software_id", software.ID,
|
||||
"name", software.Name,
|
||||
"bundle_id", software.BundleIdentifier,
|
||||
)
|
||||
for _, softwareList := range existingBundleIDsToUpdate {
|
||||
for _, software := range softwareList {
|
||||
// Only link if software still exists
|
||||
if _, ok := existingIDSet[software.ID]; ok {
|
||||
insertsHostSoftware = append(insertsHostSoftware, hostID, software.ID, software.LastOpenedAt)
|
||||
insertedSoftware = append(insertedSoftware, software)
|
||||
} else {
|
||||
// Log missing software but continue
|
||||
level.Warn(ds.logger).Log(
|
||||
"msg", "bundle ID software not found after pre-insertion",
|
||||
"software_id", software.ID,
|
||||
"name", software.Name,
|
||||
"bundle_id", software.BundleIdentifier,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1286,7 +1293,7 @@ func updateModifiedHostSoftwareDB(
|
||||
hostID uint,
|
||||
currentMap map[string]fleet.Software,
|
||||
incomingMap map[string]fleet.Software,
|
||||
existingBundleIDsToUpdate map[string]fleet.Software,
|
||||
existingBundleIDsToUpdate map[string][]fleet.Software,
|
||||
minLastOpenedAtDiff time.Duration,
|
||||
logger log.Logger,
|
||||
) error {
|
||||
|
||||
@@ -54,6 +54,7 @@ func TestSoftware(t *testing.T) {
|
||||
{"UpdateHostSoftwareSameBundleIDDifferentNames", testUpdateHostSoftwareSameBundleIDDifferentNames},
|
||||
{"UpdateHostSoftwareSameNameDifferentBundleIDs", testUpdateHostSoftwareSameNameDifferentBundleIDs},
|
||||
{"UpdateHostSoftwareMultipleSameBundleID", testUpdateHostSoftwareMultipleSameBundleID},
|
||||
{"UpdateHostSoftwareMultipleChecksumsPerBundleID", testUpdateHostSoftwareMultipleChecksumsPerBundleID},
|
||||
{"UpdateHostSoftwareLongNameTruncation", testUpdateHostSoftwareLongNameTruncation},
|
||||
{"UpdateHostBundleIDRenameOnlyNoNewSoftware", testUpdateHostBundleIDRenameOnlyNoNewSoftware},
|
||||
{"UpdateHostBundleIDRenameWithNewSoftware", testUpdateHostBundleIDRenameWithNewSoftware},
|
||||
@@ -2021,6 +2022,93 @@ func testUpdateHostSoftwareMultipleSameBundleID(t *testing.T, ds *Datastore) {
|
||||
require.Equal(t, "GoLand 2024.app", host2.Software[0].Name, "Host2 should see renamed software")
|
||||
}
|
||||
|
||||
// Test for the bug where multiple software with the same bundle ID causes
|
||||
// "software not found for checksum" errors during bundle ID rename operations.
|
||||
// This test specifically validates that ALL software entries with the same
|
||||
// bundle ID are properly linked to hosts when renaming occurs.
|
||||
func testUpdateHostSoftwareMultipleChecksumsPerBundleID(t *testing.T, ds *Datastore) {
|
||||
ctx := t.Context()
|
||||
|
||||
// Note: Basic multiple versions scenario is already covered in testUpdateHostSoftwareMultipleSameBundleID
|
||||
// This test focuses on the specific bug fix for renamed apps with many versions
|
||||
|
||||
// First, establish the software with host1 - using 10 versions to stress test
|
||||
host1 := test.NewHost(t, ds, "rename-test-host1", "", "rename-key1", "rename-uuid1", time.Now())
|
||||
|
||||
// Create 10 versions to stress test the checksum tracking
|
||||
var initialSoftware []fleet.Software
|
||||
for i := 0; i < 10; i++ {
|
||||
initialSoftware = append(initialSoftware, fleet.Software{
|
||||
Name: "TestApp.app",
|
||||
Version: fmt.Sprintf("1.%d.0", i),
|
||||
Source: "apps",
|
||||
BundleIdentifier: "com.stresstest.app",
|
||||
})
|
||||
}
|
||||
|
||||
_, err := ds.UpdateHostSoftware(ctx, host1.ID, initialSoftware)
|
||||
require.NoError(t, err, "Should handle 10 versions with same bundle ID")
|
||||
|
||||
// Verify all 10 were inserted
|
||||
err = ds.LoadHostSoftware(ctx, host1, false)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, host1.Software, 10, "Host1 should have all 10 versions")
|
||||
|
||||
// Host2 reports the same software but renamed (user renamed the apps)
|
||||
// This triggers the bundle ID rename logic and tests the bug fix
|
||||
host2 := test.NewHost(t, ds, "rename-test-host2", "", "rename-key2", "rename-uuid2", time.Now())
|
||||
|
||||
var renamedSoftware []fleet.Software
|
||||
for i := 0; i < 10; i++ {
|
||||
renamedSoftware = append(renamedSoftware, fleet.Software{
|
||||
Name: "TestApp Renamed.app", // Different name
|
||||
Version: fmt.Sprintf("1.%d.0", i),
|
||||
Source: "apps",
|
||||
BundleIdentifier: "com.stresstest.app",
|
||||
})
|
||||
}
|
||||
|
||||
// This is where the bug would occur - only one software would be linked instead of all 10
|
||||
result, err := ds.UpdateHostSoftware(ctx, host2.ID, renamedSoftware)
|
||||
require.NoError(t, err, "Should handle renamed apps with 10 versions without 'software not found for checksum' error")
|
||||
assert.NotNil(t, result)
|
||||
|
||||
// Verify the rename was processed in the database
|
||||
var dbSoftware []struct {
|
||||
Name string `db:"name"`
|
||||
Version string `db:"version"`
|
||||
NameSource string `db:"name_source"`
|
||||
}
|
||||
err = ds.writer(ctx).SelectContext(ctx, &dbSoftware,
|
||||
`SELECT name, version, name_source FROM software
|
||||
WHERE bundle_identifier = ? ORDER BY version`,
|
||||
"com.stresstest.app")
|
||||
require.NoError(t, err)
|
||||
require.Len(t, dbSoftware, 10, "Should have 10 software entries in database")
|
||||
|
||||
// All should be renamed
|
||||
for _, sw := range dbSoftware {
|
||||
assert.Equal(t, "TestApp Renamed.app", sw.Name, "All software should use the new name")
|
||||
assert.Equal(t, "bundle_4.67", sw.NameSource, "Renamed software should have bundle_4.67 source")
|
||||
}
|
||||
|
||||
// Most importantly, verify that host2 has ALL 10 versions linked (this was the bug)
|
||||
err = ds.LoadHostSoftware(ctx, host2, false)
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, host2.Software, 10, "Host2 should have all 10 versions linked (bug fix verification)")
|
||||
|
||||
// Verify all versions are present
|
||||
versions := make(map[string]bool)
|
||||
for _, sw := range host2.Software {
|
||||
versions[sw.Version] = true
|
||||
assert.Equal(t, "TestApp Renamed.app", sw.Name, "Should see renamed app")
|
||||
}
|
||||
for i := 0; i < 10; i++ {
|
||||
version := fmt.Sprintf("1.%d.0", i)
|
||||
assert.True(t, versions[version], "Should have version %s", version)
|
||||
}
|
||||
}
|
||||
|
||||
// Test edge case: Software with names exceeding maximum length
|
||||
// This validates truncation and handling of long names
|
||||
func testUpdateHostSoftwareLongNameTruncation(t *testing.T, ds *Datastore) {
|
||||
|
||||
Reference in New Issue
Block a user