Repository: kolide/launcher

Issue:
Update how we merge data in CollectAndSetEnrollmentDetails
Noticed this log recently -- note that `OsqueryVersion` goes from `5.16.0` to empty:

>"msg":"updating enrollment details","old_details":"&{OSVersion: OSBuildID: OSPlatform:windows Hostname: HardwareVendor: HardwareModel: HardwareSerial: OsqueryVersion:5.16.0 LauncherHardwareKey: LauncherHardwareKeySource: LauncherLocalKey: LauncherVersion:1.18.3 OSName: OSPlatformLike:windows GOOS:windows GOARCH:amd64 HardwareUUID:}","new_details":"{OSVersion: OSBuildID: OSPlatform:windows Hostname: HardwareVendor: HardwareModel: HardwareSerial: OsqueryVersion: LauncherHardwareKey: LauncherHardwareKeySource: LauncherLocalKey: LauncherVersion:1.18.3 OSName: OSPlatformLike:windows GOOS:windows GOARCH:amd64 HardwareUUID:}"

I think this happens only when we fail to get enrollment details from osquery.

Discussion:
@RebeccaMahany: I realized since filing this that calling `GetEnrollmentDetails` will then re-set the osquery version, so while this log is confusing, I don't think it actually effectively results in wiping the osquery version!

We discussed and will probably want to add some logic to merge old and new enrollment details rather than overwriting -- perhaps something simple, where we always go with the fields from the new details unless they are nil/empty and overwriting a non-nil/non-empty field.

Solution patch:
diff --git a/ee/agent/knapsack/knapsack.go b/ee/agent/knapsack/knapsack.go
index 400fc179..737fcdc6 100644
--- a/ee/agent/knapsack/knapsack.go
+++ b/ee/agent/knapsack/knapsack.go
@@ -248,13 +248,88 @@ func (k *knapsack) CurrentEnrollmentStatus() (types.EnrollmentStatus, error) {
 	return types.Enrolled, nil
 }
 
+// SetEnrollmentDetails updates the enrollment details, merging with existing details
 func (k *knapsack) SetEnrollmentDetails(newDetails types.EnrollmentDetails) {
+	// Get current details or empty struct if nil
+	// capture old values for logging
+	var oldDetails types.EnrollmentDetails
+	if enrollmentDetails != nil {
+		oldDetails = *enrollmentDetails
+	}
+
+	// Merge the new details with existing ones
+	mergedDetails := mergeEnrollmentDetails(oldDetails, newDetails)
+
+	// Log old and merged details - simpler approach
 	k.Slogger().Log(context.Background(), slog.LevelDebug,
 		"updating enrollment details",
-		"old_details", fmt.Sprintf("%+v", enrollmentDetails),
-		"new_details", fmt.Sprintf("%+v", newDetails),
+		"old_details", fmt.Sprintf("%+v", oldDetails),
+		"new_details", fmt.Sprintf("%+v", mergedDetails),
 	)
-	enrollmentDetails = &newDetails
+
+	// Update the global enrollment details
+	enrollmentDetails = &mergedDetails
+}
+
+// mergeEnrollmentDetails combines old and new details, only updating non-empty fields
+func mergeEnrollmentDetails(oldDetails, newDetails types.EnrollmentDetails) types.EnrollmentDetails {
+	// Start with existing details
+	mergedDetails := oldDetails
+
+	// Only update fields that are not empty
+	if newDetails.OSPlatform != "" {
+		mergedDetails.OSPlatform = newDetails.OSPlatform
+	}
+	if newDetails.OSPlatformLike != "" {
+		mergedDetails.OSPlatformLike = newDetails.OSPlatformLike
+	}
+	if newDetails.OSVersion != "" {
+		mergedDetails.OSVersion = newDetails.OSVersion
+	}
+	if newDetails.OSBuildID != "" {
+		mergedDetails.OSBuildID = newDetails.OSBuildID
+	}
+	if newDetails.Hostname != "" {
+		mergedDetails.Hostname = newDetails.Hostname
+	}
+	if newDetails.HardwareSerial != "" {
+		mergedDetails.HardwareSerial = newDetails.HardwareSerial
+	}
+	if newDetails.HardwareModel != "" {
+		mergedDetails.HardwareModel = newDetails.HardwareModel
+	}
+	if newDetails.HardwareVendor != "" {
+		mergedDetails.HardwareVendor = newDetails.HardwareVendor
+	}
+	if newDetails.HardwareUUID != "" {
+		mergedDetails.HardwareUUID = newDetails.HardwareUUID
+	}
+	if newDetails.OsqueryVersion != "" {
+		mergedDetails.OsqueryVersion = newDetails.OsqueryVersion
+	}
+	if newDetails.LauncherVersion != "" {
+		mergedDetails.LauncherVersion = newDetails.LauncherVersion
+	}
+	if newDetails.GOOS != "" {
+		mergedDetails.GOOS = newDetails.GOOS
+	}
+	if newDetails.GOARCH != "" {
+		mergedDetails.GOARCH = newDetails.GOARCH
+	}
+	if newDetails.LauncherLocalKey != "" {
+		mergedDetails.LauncherLocalKey = newDetails.LauncherLocalKey
+	}
+	if newDetails.LauncherHardwareKey != "" {
+		mergedDetails.LauncherHardwareKey = newDetails.LauncherHardwareKey
+	}
+	if newDetails.LauncherHardwareKeySource != "" {
+		mergedDetails.LauncherHardwareKeySource = newDetails.LauncherHardwareKeySource
+	}
+	if newDetails.OSName != "" {
+		mergedDetails.OSName = newDetails.OSName
+	}
+
+	return mergedDetails
 }
 
 func (k *knapsack) GetEnrollmentDetails() types.EnrollmentDetails {



Test patch:
diff --git a/ee/agent/knapsack/knapsack_test.go b/ee/agent/knapsack/knapsack_test.go
new file mode 100644
index 00000000..8650d34c
--- /dev/null
+++ b/ee/agent/knapsack/knapsack_test.go
@@ -0,0 +1,163 @@
+package knapsack
+
+import (
+	"testing"
+
+	"github.com/kolide/launcher/ee/agent/types"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMergeEnrollmentDetails(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name     string
+		old      types.EnrollmentDetails
+		new      types.EnrollmentDetails
+		expected types.EnrollmentDetails
+	}{
+		{
+			name: "empty old, populated new",
+			old:  types.EnrollmentDetails{},
+			new: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "test-host",
+				OsqueryVersion: "5.0.1",
+			},
+			expected: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "test-host",
+				OsqueryVersion: "5.0.1",
+			},
+		},
+		{
+			name: "populated old, empty new",
+			old: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "test-host",
+				OsqueryVersion: "5.0.1",
+			},
+			new: types.EnrollmentDetails{},
+			expected: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "test-host",
+				OsqueryVersion: "5.0.1",
+			},
+		},
+		{
+			name: "partial update",
+			old: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "old-host",
+				OsqueryVersion: "5.0.1",
+				HardwareModel:  "MacBookPro16,1",
+			},
+			new: types.EnrollmentDetails{
+				Hostname:       "new-host",
+				OsqueryVersion: "5.0.2",
+			},
+			expected: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "new-host",
+				OsqueryVersion: "5.0.2",
+				HardwareModel:  "MacBookPro16,1",
+			},
+		},
+		{
+			name: "complete update",
+			old: types.EnrollmentDetails{
+				OSPlatform:                "darwin",
+				OSPlatformLike:            "darwin",
+				OSVersion:                 "10.15.7",
+				OSBuildID:                 "19H2",
+				Hostname:                  "old-host",
+				HardwareSerial:            "C02XL0GYJGH5",
+				HardwareModel:             "MacBookPro16,1",
+				HardwareVendor:            "Apple Inc.",
+				HardwareUUID:              "A1B2C3D4-E5F6-7890-ABCD-EF1234567890",
+				OsqueryVersion:            "5.0.1",
+				LauncherVersion:           "0.15.0",
+				GOOS:                      "darwin",
+				GOARCH:                    "amd64",
+				LauncherLocalKey:          "key1",
+				LauncherHardwareKey:       "key2",
+				LauncherHardwareKeySource: "source1",
+				OSName:                    "macOS",
+			},
+			new: types.EnrollmentDetails{
+				OSPlatform:                "darwin",
+				OSPlatformLike:            "darwin",
+				OSVersion:                 "11.0.0",
+				OSBuildID:                 "20A2411",
+				Hostname:                  "new-host",
+				HardwareSerial:            "C02XL0GYJGH6",
+				HardwareModel:             "MacBookPro17,1",
+				HardwareVendor:            "Apple Inc.",
+				HardwareUUID:              "B2C3D4E5-F6G7-8901-BCDE-F01234567891",
+				OsqueryVersion:            "5.0.2",
+				LauncherVersion:           "0.16.0",
+				GOOS:                      "darwin",
+				GOARCH:                    "arm64",
+				LauncherLocalKey:          "key3",
+				LauncherHardwareKey:       "key4",
+				LauncherHardwareKeySource: "source2",
+				OSName:                    "macOS",
+			},
+			expected: types.EnrollmentDetails{
+				OSPlatform:                "darwin",
+				OSPlatformLike:            "darwin",
+				OSVersion:                 "11.0.0",
+				OSBuildID:                 "20A2411",
+				Hostname:                  "new-host",
+				HardwareSerial:            "C02XL0GYJGH6",
+				HardwareModel:             "MacBookPro17,1",
+				HardwareVendor:            "Apple Inc.",
+				HardwareUUID:              "B2C3D4E5-F6G7-8901-BCDE-F01234567891",
+				OsqueryVersion:            "5.0.2",
+				LauncherVersion:           "0.16.0",
+				GOOS:                      "darwin",
+				GOARCH:                    "arm64",
+				LauncherLocalKey:          "key3",
+				LauncherHardwareKey:       "key4",
+				LauncherHardwareKeySource: "source2",
+				OSName:                    "macOS",
+			},
+		},
+		{
+			name: "empty strings don't overwrite",
+			old: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "test-host",
+				OsqueryVersion: "5.0.1",
+			},
+			new: types.EnrollmentDetails{
+				OSPlatform:     "",
+				OSVersion:      "",
+				Hostname:       "new-host",
+				OsqueryVersion: "",
+			},
+			expected: types.EnrollmentDetails{
+				OSPlatform:     "darwin",
+				OSVersion:      "10.15.7",
+				Hostname:       "new-host",
+				OsqueryVersion: "5.0.1",
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			result := mergeEnrollmentDetails(tc.old, tc.new)
+			require.Equal(t, tc.expected, result)
+		})
+	}
+}