Change self-service categories GitOps to not require dedicated key (#47439)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves # # Checklist for submitter If some of the following don't apply, delete the relevant line. - [ ] 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. - Not needed - [ ] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Batch software installer and app-association endpoints now return the list of referenced self-service categories. * Category fields support an “omit when unset” JSON behavior so omitted vs empty categories are distinguishable. * **Bug Fixes** * Improved category validation (trim + case-insensitive dedupe) and GitOps reconciliation to remove unused categories. * **Chores** * GitOps schema simplified: no separate top-level self_service_categories; categories are defined inline with packages. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Jonathan Katz
+6
-52
@@ -310,10 +310,9 @@ func (spec SoftwarePackage) HydrateToPackageLevel(packageLevel fleet.SoftwarePac
|
||||
}
|
||||
|
||||
type Software struct {
|
||||
Packages []SoftwarePackage `json:"packages"`
|
||||
AppStoreApps []fleet.TeamSpecAppStoreApp `json:"app_store_apps"`
|
||||
FleetMaintainedApps []fleet.MaintainedAppSpec `json:"fleet_maintained_apps"`
|
||||
SelfServiceCategories optjson.Slice[string] `json:"self_service_categories"`
|
||||
Packages []SoftwarePackage `json:"packages"`
|
||||
AppStoreApps []fleet.TeamSpecAppStoreApp `json:"app_store_apps"`
|
||||
FleetMaintainedApps []fleet.MaintainedAppSpec `json:"fleet_maintained_apps"`
|
||||
}
|
||||
|
||||
// GitOpsMDM extends fleet.MDM with gitops-only fields that are not part of the server type.
|
||||
@@ -378,10 +377,9 @@ type GitOps struct {
|
||||
}
|
||||
|
||||
type GitOpsSoftware struct {
|
||||
Packages []*fleet.SoftwarePackageSpec
|
||||
AppStoreApps []*fleet.TeamSpecAppStoreApp
|
||||
FleetMaintainedApps []*fleet.MaintainedAppSpec
|
||||
SelfServiceCategories optjson.Slice[string]
|
||||
Packages []*fleet.SoftwarePackageSpec
|
||||
AppStoreApps []*fleet.TeamSpecAppStoreApp
|
||||
FleetMaintainedApps []*fleet.MaintainedAppSpec
|
||||
}
|
||||
|
||||
type Logf func(format string, a ...interface{})
|
||||
@@ -1942,47 +1940,6 @@ func parseSoftware(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
multiError = multierror.Append(multiError, validateRawKeys(softwareRaw, reflect.TypeFor[Software](), filePath, []string{"software"})...)
|
||||
}
|
||||
|
||||
// validate self service categories
|
||||
if software.SelfServiceCategories.Set {
|
||||
declared := software.SelfServiceCategories.Value
|
||||
var seen []string
|
||||
|
||||
for i, name := range declared {
|
||||
declared[i] = strings.TrimSpace(name)
|
||||
|
||||
if err := (fleet.SoftwareCategory{Name: declared[i]}).Validate(); err != nil {
|
||||
multiError = multierror.Append(multiError, fmt.Errorf("self_service_categories: %w", err))
|
||||
continue
|
||||
}
|
||||
|
||||
// Doesn't catch utf8mb4_unicode_ci collation collisions (e.g. "🔐 Security" vs "🛡 Security") in dry runs.
|
||||
if slices.ContainsFunc(seen, func(s string) bool { return strings.EqualFold(s, declared[i]) }) {
|
||||
multiError = multierror.Append(multiError,
|
||||
fmt.Errorf("self_service_categories: duplicate category %q", declared[i]))
|
||||
continue
|
||||
}
|
||||
|
||||
seen = append(seen, declared[i])
|
||||
}
|
||||
|
||||
result.Software.SelfServiceCategories = optjson.SetSlice(declared)
|
||||
}
|
||||
|
||||
validateCategoryReferences := func(categories []string) {
|
||||
if !result.Software.SelfServiceCategories.Set {
|
||||
return
|
||||
}
|
||||
|
||||
for _, name := range categories {
|
||||
if !slices.ContainsFunc(result.Software.SelfServiceCategories.Value, func(d string) bool {
|
||||
return fleet.SoftwareCategoryReferenceMatches(name, d)
|
||||
}) {
|
||||
multiError = multierror.Append(multiError,
|
||||
fmt.Errorf("category %q is not in software.self_service_categories", name))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for _, item := range software.AppStoreApps {
|
||||
if item.AppStoreID == "" {
|
||||
multiError = multierror.Append(multiError, errors.New("software app store id required"))
|
||||
@@ -2008,7 +1965,6 @@ func parseSoftware(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
|
||||
item = item.ResolvePaths(baseDir)
|
||||
|
||||
validateCategoryReferences(item.Categories)
|
||||
result.Software.AppStoreApps = append(result.Software.AppStoreApps, &item)
|
||||
}
|
||||
for _, maintainedAppSpec := range software.FleetMaintainedApps {
|
||||
@@ -2050,7 +2006,6 @@ func parseSoftware(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
}
|
||||
}
|
||||
|
||||
validateCategoryReferences(maintainedAppSpec.Categories)
|
||||
result.Software.FleetMaintainedApps = append(result.Software.FleetMaintainedApps, &maintainedAppSpec)
|
||||
}
|
||||
for _, teamLevelPackage := range software.Packages {
|
||||
@@ -2222,7 +2177,6 @@ func parseSoftware(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
continue
|
||||
}
|
||||
|
||||
validateCategoryReferences(softwarePackageSpec.Categories)
|
||||
result.Software.Packages = append(result.Software.Packages, softwarePackageSpec)
|
||||
}
|
||||
}
|
||||
|
||||
+31
-148
@@ -222,7 +222,7 @@ func TestValidGitOpsYaml(t *testing.T) {
|
||||
if strings.Contains(pkg.URL, "MicrosoftTeams") {
|
||||
assert.Equal(t, "testdata/lib/uninstall.sh", pkg.UninstallScript.Path)
|
||||
assert.Contains(t, pkg.LabelsIncludeAny, "a")
|
||||
assert.Contains(t, pkg.Categories, "Communication")
|
||||
assert.Contains(t, pkg.Categories.Value, "Communication")
|
||||
assert.Empty(t, pkg.LabelsExcludeAny)
|
||||
assert.Empty(t, pkg.LabelsIncludeAll)
|
||||
} else {
|
||||
@@ -236,14 +236,14 @@ func TestValidGitOpsYaml(t *testing.T) {
|
||||
for _, fma := range gitops.Software.FleetMaintainedApps {
|
||||
switch fma.Slug {
|
||||
case "slack/darwin":
|
||||
require.ElementsMatch(t, fma.Categories, []string{"Productivity", "Communication"})
|
||||
require.ElementsMatch(t, fma.Categories.Value, []string{"Productivity", "Communication"})
|
||||
require.Equal(t, "4.47.65", fma.Version)
|
||||
require.Empty(t, fma.PreInstallQuery)
|
||||
require.Empty(t, fma.PostInstallScript)
|
||||
require.Empty(t, fma.InstallScript)
|
||||
require.Empty(t, fma.UninstallScript)
|
||||
case "box-drive/windows":
|
||||
require.ElementsMatch(t, fma.Categories, []string{"Productivity", "Developer tools"})
|
||||
require.ElementsMatch(t, fma.Categories.Value, []string{"Productivity", "Developer tools"})
|
||||
require.Empty(t, fma.Version)
|
||||
require.NotEmpty(t, fma.PreInstallQuery)
|
||||
require.NotEmpty(t, fma.PostInstallScript)
|
||||
@@ -4067,7 +4067,7 @@ software:
|
||||
require.NoError(t, err)
|
||||
require.Len(t, result.Software.Packages, 1)
|
||||
assert.True(t, strings.HasSuffix(result.Software.Packages[0].InstallScript.Path, "install-app.sh"))
|
||||
assert.Equal(t, []string{"Utilities"}, result.Software.Packages[0].Categories)
|
||||
assert.Equal(t, []string{"Utilities"}, result.Software.Packages[0].Categories.Value)
|
||||
assert.True(t, result.Software.Packages[0].SelfService)
|
||||
assert.Empty(t, result.Software.Packages[0].URL)
|
||||
assert.Empty(t, result.Software.Packages[0].SHA256)
|
||||
@@ -4168,7 +4168,7 @@ software:
|
||||
require.NoError(t, err)
|
||||
require.Len(t, result.Software.Packages, 1)
|
||||
pkg := result.Software.Packages[0]
|
||||
assert.Equal(t, []string{"Browsers", "Productivity"}, pkg.Categories)
|
||||
assert.Equal(t, []string{"Browsers", "Productivity"}, pkg.Categories.Value)
|
||||
assert.True(t, pkg.SelfService)
|
||||
assert.True(t, pkg.InstallDuringSetup.Value)
|
||||
assert.Equal(t, []string{"include_label"}, pkg.LabelsIncludeAny)
|
||||
@@ -4470,163 +4470,46 @@ name: TestTeam
|
||||
})
|
||||
}
|
||||
|
||||
func TestGitOpsSelfServiceCategoriesPresence(t *testing.T) {
|
||||
func TestGitOpsFMACategoriesPresence(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
t.Run("key omitted leaves Present false", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
parse := func(t *testing.T, categoriesYAML string) optjson.Slice[string] {
|
||||
config := getTeamConfig(nil)
|
||||
config += "software:\n packages: []\n"
|
||||
config += "software:\n fleet_maintained_apps:\n - slug: 1password/darwin\n" + categoriesYAML
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
gitops, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.NoError(t, err)
|
||||
assert.False(t, gitops.Software.SelfServiceCategories.Set)
|
||||
assert.Empty(t, gitops.Software.SelfServiceCategories.Value)
|
||||
require.Len(t, gitops.Software.FleetMaintainedApps, 1)
|
||||
return gitops.Software.FleetMaintainedApps[0].Categories
|
||||
}
|
||||
|
||||
t.Run("omitted key is unset", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
cats := parse(t, "")
|
||||
assert.False(t, cats.Set)
|
||||
})
|
||||
|
||||
t.Run("empty list sets Present true", func(t *testing.T) {
|
||||
t.Run("categories: (null) is set but not valid", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += "software:\n self_service_categories: []\n"
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
gitops, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.NoError(t, err)
|
||||
assert.True(t, gitops.Software.SelfServiceCategories.Set)
|
||||
assert.Empty(t, gitops.Software.SelfServiceCategories.Value)
|
||||
cats := parse(t, " categories:\n")
|
||||
assert.True(t, cats.Set)
|
||||
assert.False(t, cats.Valid)
|
||||
assert.Empty(t, cats.Value)
|
||||
})
|
||||
|
||||
t.Run("populated list sets Present true and preserves names verbatim", func(t *testing.T) {
|
||||
t.Run("categories: [] is set and valid", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- "🌎 Browsers"
|
||||
- "Productivity"
|
||||
- "💼 Engineering"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
gitops, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.NoError(t, err)
|
||||
assert.True(t, gitops.Software.SelfServiceCategories.Set)
|
||||
assert.Equal(t, []string{"🌎 Browsers", "Productivity", "💼 Engineering"}, gitops.Software.SelfServiceCategories.Value)
|
||||
cats := parse(t, " categories: []\n")
|
||||
assert.True(t, cats.Set)
|
||||
assert.True(t, cats.Valid)
|
||||
assert.Empty(t, cats.Value)
|
||||
})
|
||||
|
||||
t.Run("duplicate name in payload fails at parse", func(t *testing.T) {
|
||||
t.Run("categories with a value is set with the value", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- "🔐 Security"
|
||||
- "🔐 Security"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "duplicate")
|
||||
assert.Contains(t, err.Error(), "🔐 Security")
|
||||
})
|
||||
|
||||
t.Run("package referencing undeclared category fails at parse", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- "Allowed"
|
||||
packages:
|
||||
- url: https://example.com/installer.pkg
|
||||
hash_sha256: "0000000000000000000000000000000000000000000000000000000000000000"
|
||||
categories:
|
||||
- "Forbidden"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `"Forbidden"`)
|
||||
assert.Contains(t, err.Error(), "self_service_categories")
|
||||
})
|
||||
|
||||
t.Run("app_store_apps referencing undeclared category fails at parse", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- "Allowed"
|
||||
app_store_apps:
|
||||
- app_store_id: "12345"
|
||||
categories:
|
||||
- "Forbidden"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `"Forbidden"`)
|
||||
assert.Contains(t, err.Error(), "self_service_categories")
|
||||
})
|
||||
|
||||
t.Run("fleet_maintained_apps referencing undeclared category fails at parse", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- "Allowed"
|
||||
fleet_maintained_apps:
|
||||
- slug: 1password/darwin
|
||||
categories:
|
||||
- "Forbidden"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `"Forbidden"`)
|
||||
assert.Contains(t, err.Error(), "self_service_categories")
|
||||
})
|
||||
|
||||
t.Run("validation rejects empty, whitespace-only, and over-length names", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += fmt.Sprintf(`software:
|
||||
self_service_categories:
|
||||
- ""
|
||||
- " "
|
||||
- %q
|
||||
`, strings.Repeat("x", 256))
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "name is required")
|
||||
assert.Contains(t, err.Error(), "must be at most 255")
|
||||
})
|
||||
|
||||
t.Run("names are trimmed and case-only duplicates are caught", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- " Productivity "
|
||||
- "PRODUCTIVITY"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "duplicate")
|
||||
// The second entry collides with the trimmed first.
|
||||
assert.Contains(t, err.Error(), "PRODUCTIVITY")
|
||||
})
|
||||
|
||||
t.Run("legacy plain reference matches declared emoji form", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig(nil)
|
||||
config += `software:
|
||||
self_service_categories:
|
||||
- "💻 Productivity"
|
||||
packages:
|
||||
- url: https://example.com/installer.pkg
|
||||
hash_sha256: "0000000000000000000000000000000000000000000000000000000000000000"
|
||||
categories:
|
||||
- "Productivity"
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.NoError(t, err)
|
||||
cats := parse(t, " categories:\n - somevalue\n")
|
||||
assert.True(t, cats.Set)
|
||||
assert.True(t, cats.Valid)
|
||||
assert.Equal(t, []string{"somevalue"}, cats.Value)
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user