Add duplicate patch policy check to GitOps (#48896)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #46193 Adds a client-side check for duplicate patch policies, similar to the existing policy name and label duplicate checks. # 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. - [ ] 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 Adding two patch policies for the same fma slug results in this error: ``` Error: 1 error occurred: * Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": "google-chrome/darwin". ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added validation to GitOps application checks to prevent multiple patch policies from targeting the same app slug. * Improved error reporting when patch policy slugs are duplicated or missing from the configured app list. * **Bug Fixes** * Prevented duplicate patch policies from being silently accepted, reducing the risk of one policy overwriting another. * Existing valid combinations, such as different patch slugs or certain mixed policy types, continue to work as expected. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -0,0 +1 @@
|
||||
- Added a check for duplicate patch policies when applying GitOps.
|
||||
@@ -1797,6 +1797,7 @@ func parsePolicies(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
}
|
||||
}
|
||||
// Make sure team name is correct, and do additional validation
|
||||
var patchSlugs []string
|
||||
for _, item := range result.Policies {
|
||||
if item.Name == "" {
|
||||
multiError = multierror.Append(multiError, errors.New("policy name is required for each policy"))
|
||||
@@ -1820,6 +1821,9 @@ func parsePolicies(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
),
|
||||
)
|
||||
}
|
||||
if item.FleetMaintainedAppSlug != "" {
|
||||
patchSlugs = append(patchSlugs, item.FleetMaintainedAppSlug)
|
||||
}
|
||||
}
|
||||
if result.TeamName != nil {
|
||||
item.Team = *result.TeamName
|
||||
@@ -1838,6 +1842,9 @@ func parsePolicies(top map[string]json.RawMessage, result *GitOps, baseDir strin
|
||||
if len(duplicates) > 0 {
|
||||
multiError = multierror.Append(multiError, fmt.Errorf("duplicate policy names: %v", duplicates))
|
||||
}
|
||||
for _, slug := range getDuplicateNames(patchSlugs, func(s string) string { return s }) {
|
||||
multiError = multierror.Append(multiError, fmt.Errorf(`Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": %q.`, slug))
|
||||
}
|
||||
return multiError
|
||||
}
|
||||
|
||||
|
||||
@@ -4863,3 +4863,153 @@ func TestGitOpsFMACategoriesPresence(t *testing.T) {
|
||||
assert.Equal(t, []string{"somevalue"}, cats.Value)
|
||||
})
|
||||
}
|
||||
|
||||
func TestDuplicatePatchPolicySlug(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Every slug referenced by a patch policy must be declared under software.fleet_maintained_apps.
|
||||
fmaSoftware := `
|
||||
software:
|
||||
fleet_maintained_apps:
|
||||
- slug: google-chrome/darwin
|
||||
- slug: 1password/darwin
|
||||
- slug: firefox/darwin
|
||||
`
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
policies string
|
||||
// wantErrs empty means the config must apply cleanly.
|
||||
wantErrs []string
|
||||
}{
|
||||
{
|
||||
// Before this check the second patch policy silently overwrote the first.
|
||||
name: "two patch policies with the same slug",
|
||||
policies: `
|
||||
policies:
|
||||
- name: Chrome up to date
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Chrome up to date again
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
`,
|
||||
wantErrs: []string{`Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": "google-chrome/darwin".`},
|
||||
},
|
||||
{
|
||||
// Each duplicated slug gets its own error, driven by the slug in the config.
|
||||
name: "two slugs each duplicated report one error per slug",
|
||||
policies: `
|
||||
policies:
|
||||
- name: Chrome up to date
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Chrome up to date again
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: 1Password up to date
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: 1password/darwin
|
||||
- name: 1Password up to date again
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: 1password/darwin
|
||||
`,
|
||||
wantErrs: []string{
|
||||
`Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": "google-chrome/darwin".`,
|
||||
`Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": "1password/darwin".`,
|
||||
},
|
||||
},
|
||||
{
|
||||
// A slug used by three patch policies is still reported a single time.
|
||||
name: "slug used three times is reported once",
|
||||
policies: `
|
||||
policies:
|
||||
- name: Chrome A
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Chrome B
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Chrome C
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
`,
|
||||
wantErrs: []string{`Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": "google-chrome/darwin".`},
|
||||
},
|
||||
{
|
||||
// Duplicate names and duplicate patch slug surface together.
|
||||
name: "duplicate names and duplicate patch slug both reported",
|
||||
policies: `
|
||||
policies:
|
||||
- name: Same name
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Same name
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
`,
|
||||
wantErrs: []string{
|
||||
"duplicate policy names",
|
||||
`Couldn't add multiple policies with type "patch" for "fleet_maintained_app_slug": "google-chrome/darwin".`,
|
||||
},
|
||||
},
|
||||
{
|
||||
// A dynamic install_software policy and a patch policy may share a slug.
|
||||
name: "dynamic install_software and patch with the same slug is allowed",
|
||||
policies: `
|
||||
policies:
|
||||
- name: Chrome installed
|
||||
platform: darwin
|
||||
query: SELECT 1 FROM apps WHERE bundle_identifier = 'com.google.Chrome';
|
||||
install_software:
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Chrome up to date
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
`,
|
||||
},
|
||||
{
|
||||
name: "two patch policies with different slugs is allowed",
|
||||
policies: `
|
||||
policies:
|
||||
- name: Chrome up to date
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: google-chrome/darwin
|
||||
- name: Firefox up to date
|
||||
type: patch
|
||||
platform: darwin
|
||||
fleet_maintained_app_slug: firefox/darwin
|
||||
`,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig([]string{"policies"}) + fmaSoftware + tc.policies
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
if len(tc.wantErrs) == 0 {
|
||||
require.NoError(t, err)
|
||||
return
|
||||
}
|
||||
require.Error(t, err)
|
||||
for _, want := range tc.wantErrs {
|
||||
assert.ErrorContains(t, err, want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user