From 9611bb87b7054038b61b2c6a672edb774dfae573 Mon Sep 17 00:00:00 2001 From: Sharon Katz <121527325+sharon-fdm@users.noreply.github.com> Date: Wed, 6 May 2026 13:08:27 -0400 Subject: [PATCH] Improve error message when referencing a bad label for a configuration profile (#44839) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #39739 ## Local reproduction Reproduced the bug locally by running `fleetctl gitops --dry-run` against a local Fleet server with a GitOps config that references a nonexistent label on a configuration profile. **Setup:** 1. Started a local Fleet server (`fleet serve` against Docker MySQL/Redis on `https://localhost:8080`, fresh database). 2. Created a minimal `.mobileconfig` profile (`test-profile.mobileconfig`). 3. Created a `default.yml` that references it under `controls.macos_settings.custom_settings` with `labels_include_all: ["this-label-does-not-exist"]`. **Reproduction:** ```bash fleetctl gitops -f /tmp/repro-39739/default.yml --dry-run ``` **Result (before fix):** ``` [!] Unknown label 'this-label-does-not-exist' is referenced by MDM Profile '/tmp/repro-39739/profiles/test-profile.mobileconfig' Error: Please create the missing labels, or update your settings to not refer to these labels. ``` Two problems visible: - Says "MDM Profile" — internal jargon, not the user-facing term - Shows the full absolute path — noisy and unhelpful --- ## Code changes **Summary:** Two lines changed in `cmd/fleetctl/fleetctl/gitops.go` inside the `getLabelUsage()` function. Both fix how configuration profile label errors are displayed to the user during `fleetctl gitops` runs. ### `cmd/fleetctl/fleetctl/gitops.go` **Line 851 — "multiple label keys" error message:** Changed `"MDM profile"` → `"configuration profile"` and wrapped `setting.Path` in `filepath.Base()` so the error shows just the filename instead of the full absolute path. ```diff - err := fmt.Errorf("MDM profile '%s' has multiple label keys; ...", setting.Path) + err := fmt.Errorf("configuration profile '%s' has multiple label keys; ...", filepath.Base(setting.Path)) ``` **Line 869 — label usage tracking entry:** Changed the type string from `"MDM Profile"` → `"configuration profile"` and the identifier from the full `setting.Path` to `filepath.Base(setting.Path)`. This feeds into the error message on line 458: `[!] Unknown label '' is referenced by ''` ```diff - updateLabelUsage(labels, setting.Path, "MDM Profile", result) + updateLabelUsage(labels, filepath.Base(setting.Path), "configuration profile", result) ``` **After fix:** ``` [!] Unknown label 'this-label-does-not-exist' is referenced by configuration profile 'test-profile.mobileconfig' ``` --- ## Testing ### Manual testing 1. Started a local Fleet server (fresh DB, `fleet serve` on `https://localhost:8080`). 2. Created a minimal `.mobileconfig` profile and a `default.yml` GitOps config that references it with `labels_include_all: ["this-label-does-not-exist"]`. 3. Built `fleetctl` from the **unfixed** code (`git stash`) and ran `fleetctl gitops -f default.yml --dry-run`. Confirmed the old error message: ``` [!] Unknown label 'this-label-does-not-exist' is referenced by MDM Profile '/tmp/repro-39739/profiles/test-profile.mobileconfig' ``` 4. Built `fleetctl` from the **fixed** code and ran the same command. Confirmed the new error message: ``` [!] Unknown label 'this-label-does-not-exist' is referenced by configuration profile 'test-profile.mobileconfig' ``` ### Unit tests added New file: `cmd/fleetctl/fleetctl/gitops_label_usage_test.go` — two tests that exercise `getLabelUsage()` directly (no Redis/MySQL needed): - **`TestGetLabelUsageProfilePathShortened`**: Creates a `GitOps` config with a macOS profile using a full absolute path and a nonexistent label. Asserts the label usage entry has the basename (not the full path) and the type is `"configuration profile"` (not `"MDM Profile"`). - **`TestGetLabelUsageMultipleLabelKeysError`**: Creates a config with both `labels_include_all` and `labels_include_any` on the same profile. Asserts the error contains `"configuration profile"` and the short filename, and does **not** contain the directory path. Both tests were verified to **fail on unfixed code** and **pass on the fix** via a `git stash` round-trip. ## Summary by CodeRabbit * **Bug Fixes** * Enhanced error messages for MDM configuration profile label validation to display concise filenames instead of full file paths, improving user experience. * **Refactor** * Updated internal label usage tracking to use configuration profile base filenames for consistency and clarity. * **Tests** * Added test coverage for configuration profile path shortening and error message validation in label key scenarios. --- cmd/fleetctl/fleetctl/gitops.go | 4 +- .../fleetctl/gitops_label_usage_test.go | 70 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 cmd/fleetctl/fleetctl/gitops_label_usage_test.go diff --git a/cmd/fleetctl/fleetctl/gitops.go b/cmd/fleetctl/fleetctl/gitops.go index a08934edf4..d70d34f87a 100644 --- a/cmd/fleetctl/fleetctl/gitops.go +++ b/cmd/fleetctl/fleetctl/gitops.go @@ -851,7 +851,7 @@ func getLabelUsage(config *spec.GitOps) (map[string][]LabelUsage, error) { if osSettings, ok := getCustomSettings(osSettingName); ok { for _, setting := range osSettings { var labels []string - err := fmt.Errorf("MDM profile '%s' has multiple label keys; please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`.", setting.Path) + err := fmt.Errorf("configuration profile '%s' has multiple label keys; please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`.", filepath.Base(setting.Path)) if len(setting.LabelsIncludeAny) > 0 { labels = setting.LabelsIncludeAny @@ -869,7 +869,7 @@ func getLabelUsage(config *spec.GitOps) (map[string][]LabelUsage, error) { labels = setting.LabelsExcludeAny } - updateLabelUsage(labels, setting.Path, "MDM Profile", result) + updateLabelUsage(labels, filepath.Base(setting.Path), "configuration profile", result) } } } diff --git a/cmd/fleetctl/fleetctl/gitops_label_usage_test.go b/cmd/fleetctl/fleetctl/gitops_label_usage_test.go new file mode 100644 index 0000000000..27c9a7954d --- /dev/null +++ b/cmd/fleetctl/fleetctl/gitops_label_usage_test.go @@ -0,0 +1,70 @@ +package fleetctl + +import ( + "testing" + + "github.com/fleetdm/fleet/v4/pkg/spec" + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetLabelUsageProfilePathShortened(t *testing.T) { + // Simulate what happens when spec parsing resolves a relative path to absolute. + absPath := "/home/runner/work/Detroit-GitOps-Workshop/Detroit-GitOps-Workshop/lib/macos/configuration-profiles/disable-bluetooth-file-sharing.mobileconfig" + + config := &spec.GitOps{ + Controls: spec.GitOpsControls{ + MacOSSettings: &fleet.MacOSSettings{ + CustomSettings: []fleet.MDMProfileSpec{ + { + Path: absPath, + LabelsIncludeAll: []string{"nonexistent-label"}, + }, + }, + }, + }, + } + + usage, err := getLabelUsage(config) + require.NoError(t, err) + + // The label "nonexistent-label" should be in the usage map. + entries, ok := usage["nonexistent-label"] + require.True(t, ok, "expected label to be in usage map") + require.Len(t, entries, 1) + + // The Name should be the base filename, not the full absolute path. + assert.Equal(t, "disable-bluetooth-file-sharing.mobileconfig", entries[0].Name, + "profile path should be shortened to just the filename") + + // The Type should be "configuration profile", not "MDM Profile". + assert.Equal(t, "configuration profile", entries[0].Type, + "type should say 'configuration profile' not 'MDM Profile'") +} + +func TestGetLabelUsageMultipleLabelKeysError(t *testing.T) { + absPath := "/absolute/path/to/profile.mobileconfig" + + config := &spec.GitOps{ + Controls: spec.GitOpsControls{ + MacOSSettings: &fleet.MacOSSettings{ + CustomSettings: []fleet.MDMProfileSpec{ + { + Path: absPath, + LabelsIncludeAll: []string{"label-a"}, + LabelsIncludeAny: []string{"label-b"}, + }, + }, + }, + }, + } + + _, err := getLabelUsage(config) + require.Error(t, err) + + // Error should use "configuration profile" and the short filename. + assert.Contains(t, err.Error(), "configuration profile") + assert.Contains(t, err.Error(), "profile.mobileconfig") + assert.NotContains(t, err.Error(), "/absolute/path/to/") +}