mirror of
https://github.com/fleetdm/fleet
synced 2026-04-21 13:37:30 +00:00
Make most GitOps top-level optional (#41138)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #41012 # Details This PR makes it allowable to leave out almost all top-level keys from GitOps files. The only required keys are _either_ `name:` (for a fleet settings file) or `org_settings:` (for a global settings file). Omitting a key is identical to supplying it with no value. # 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 Updated the "missing all global keys test", and added some new tests to verify that omitting the key was the same as supplying it with an empty value. - [X] QA'd all new/changed functionality manually 1. Ran `fleetctl generate-gitops` to get a clean set of GitOps yml files 2. Removed all removable keys from default.yml and ran `fleetctl gitops` 3. Ran `fleetctl generate-gitops` again into a different dir 4. Ran `fleetctl gitops` with the original files to get back to original state 5. Cleared out all now-removable keys and replaced them with empty value (e.g. `reports:` with nothing under it) 6. Ran `fleetctl generate-gitops` again into a third dir 7. Compared the files from the second and third generate-gitops runs to verify that omitting the key had the same result as supplying it with an empty value 8. Did the above steps with a fleet (i.e. non-global) .yml file. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * GitOps files now support omitting top-level configuration keys instead of requiring them to be explicitly set to empty values. * org_settings is now required when team name is not specified. * **Tests** * Added integration tests validating behavior when omitting top-level keys in global and team-level GitOps configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Scott Gress
GitHub
parent
49b7db18fa
commit
9c4d5ce97e
5 changed files with 291 additions and 19 deletions
1
changes/41012-allow-omitting-top-level-gitops-keys
Normal file
1
changes/41012-allow-omitting-top-level-gitops-keys
Normal file
|
|
@ -0,0 +1 @@
|
|||
- Most top-level keys can now be omitted from GitOps files in place of supplying them with an empty value.
|
||||
|
|
@ -201,7 +201,7 @@ org_settings:
|
|||
require.NoError(t, err)
|
||||
_, err = RunAppNoChecks([]string{"gitops", "-f", badFile.Name()})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "errors occurred")
|
||||
assert.Contains(t, err.Error(), "'org_settings' is required")
|
||||
|
||||
// DoGitOps error
|
||||
t.Setenv("ORG_NAME", "")
|
||||
|
|
@ -209,7 +209,7 @@ org_settings:
|
|||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "organization name must be present")
|
||||
|
||||
// Missing controls.
|
||||
// Missing controls is now allowed (controls is optional).
|
||||
tmpFile2, err := os.CreateTemp(t.TempDir(), "*.yml")
|
||||
require.NoError(t, err)
|
||||
_, err = tmpFile2.WriteString(
|
||||
|
|
@ -229,8 +229,8 @@ org_settings:
|
|||
)
|
||||
require.NoError(t, err)
|
||||
_, err = RunAppNoChecks([]string{"gitops", "-f", tmpFile2.Name()})
|
||||
require.Error(t, err)
|
||||
assert.Equal(t, `'controls' must be set on global config`, err.Error())
|
||||
require.NoError(t, err)
|
||||
savedAppConfig = &fleet.AppConfig{}
|
||||
|
||||
// Dry run
|
||||
t.Setenv("ORG_NAME", orgName)
|
||||
|
|
@ -2262,7 +2262,7 @@ software:
|
|||
assert.True(t, strings.Contains(err.Error(), "calendar events are not supported on policies included in `no-team.yml`: \"Foobar\""), err.Error())
|
||||
})
|
||||
|
||||
t.Run("global and no-team.yml DO NOT define controls -- should fail", func(t *testing.T) {
|
||||
t.Run("global and no-team.yml DO NOT define controls -- controls is now optional", func(t *testing.T) {
|
||||
globalFileWithoutControlsAndSoftwareKeys := createGlobalFileWithoutControlsAndSoftwareKeys(t, fleetServerURL, orgName)
|
||||
|
||||
noTeamFilePathWithoutControls := filepath.Join(t.TempDir(), "no-team.yml")
|
||||
|
|
@ -2275,20 +2275,16 @@ software:
|
|||
`)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Dry run, controls should be defined somewhere, either in no-team.yml or global.
|
||||
_, err = RunAppNoChecks([]string{
|
||||
// Dry run, controls is now optional so this should succeed.
|
||||
_ = RunAppForTest(t, []string{
|
||||
"gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFileBasic.Name(), "-f",
|
||||
noTeamFileWithoutControls.Name(), "--dry-run",
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.True(t, strings.Contains(err.Error(), "'controls' must be set on global config or no-team.yml"))
|
||||
// Real run
|
||||
_, err = RunAppNoChecks([]string{
|
||||
_ = RunAppForTest(t, []string{
|
||||
"gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFileBasic.Name(), "-f",
|
||||
noTeamFileWithoutControls.Name(),
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.True(t, strings.Contains(err.Error(), "'controls' must be set on global config or no-team.yml"))
|
||||
})
|
||||
|
||||
t.Run("controls only defined in no-team.yml", func(t *testing.T) {
|
||||
|
|
|
|||
|
|
@ -3753,3 +3753,249 @@ settings:
|
|||
require.NotNil(t, tmPols[0].SoftwareInstallerID)
|
||||
require.Equal(t, installer.InstallerID, *tmPols[0].SoftwareInstallerID)
|
||||
}
|
||||
|
||||
// TestOmittedTopLevelKeysGlobal verifies that omitting top-level keys from a global
|
||||
// gitops file clears the corresponding settings (e.g. policies, agent_options).
|
||||
func (s *enterpriseIntegrationGitopsTestSuite) TestOmittedTopLevelKeysGlobal() {
|
||||
t := s.T()
|
||||
ctx := context.Background()
|
||||
|
||||
user := s.createGitOpsUser(t)
|
||||
fleetctlConfig := s.createFleetctlConfig(t, user)
|
||||
t.Setenv("FLEET_URL", s.Server.URL)
|
||||
|
||||
// Step 1: Apply a full global config with policies, agent_options, controls, and reports.
|
||||
const fullGlobalConfig = `
|
||||
org_settings:
|
||||
server_settings:
|
||||
server_url: $FLEET_URL
|
||||
org_info:
|
||||
org_name: Fleet
|
||||
secrets:
|
||||
- secret: boofar
|
||||
agent_options:
|
||||
config:
|
||||
options:
|
||||
pack_delimiter: /
|
||||
controls:
|
||||
enable_disk_encryption: true
|
||||
policies:
|
||||
- name: Test Global Policy
|
||||
query: SELECT 1;
|
||||
reports:
|
||||
- name: Test Global Report
|
||||
query: SELECT 1;
|
||||
automations_enabled: false
|
||||
labels:
|
||||
- name: Test Global Label
|
||||
label_membership_type: dynamic
|
||||
query: SELECT 1
|
||||
`
|
||||
fullFile, err := os.CreateTemp(t.TempDir(), "*.yml")
|
||||
require.NoError(t, err)
|
||||
_, err = fullFile.WriteString(fullGlobalConfig)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, fullFile.Close())
|
||||
|
||||
s.assertRealRunOutput(t, fleetctl.RunAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", fullFile.Name()}))
|
||||
|
||||
// Verify policy, agent_options, controls, and reports were applied.
|
||||
policies, err := s.DS.ListGlobalPolicies(ctx, fleet.ListOptions{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, policies, 1)
|
||||
require.Equal(t, "Test Global Policy", policies[0].Name)
|
||||
|
||||
appCfg, err := s.DS.AppConfig(ctx)
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, appCfg.AgentOptions)
|
||||
require.Contains(t, string(*appCfg.AgentOptions), "pack_delimiter")
|
||||
require.True(t, appCfg.MDM.EnableDiskEncryption.Value)
|
||||
|
||||
queries, _, _, _, err := s.DS.ListQueries(ctx, fleet.ListQueryOptions{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, queries, 1)
|
||||
require.Equal(t, "Test Global Report", queries[0].Name)
|
||||
|
||||
globalSecrets, err := s.DS.GetEnrollSecrets(ctx, nil)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, globalSecrets, 1)
|
||||
require.Equal(t, "boofar", globalSecrets[0].Secret)
|
||||
|
||||
labels, err := s.DS.LabelsByName(ctx, []string{"Test Global Label"}, fleet.TeamFilter{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, labels, 1)
|
||||
|
||||
// Step 2: Apply a minimal global config that omits policies, agent_options, controls, reports, labels.
|
||||
const minimalGlobalConfig = `
|
||||
org_settings:
|
||||
server_settings:
|
||||
server_url: $FLEET_URL
|
||||
org_info:
|
||||
org_name: Fleet
|
||||
`
|
||||
minimalFile, err := os.CreateTemp(t.TempDir(), "*.yml")
|
||||
require.NoError(t, err)
|
||||
_, err = minimalFile.WriteString(minimalGlobalConfig)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, minimalFile.Close())
|
||||
|
||||
s.assertRealRunOutput(t, fleetctl.RunAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", minimalFile.Name()}))
|
||||
|
||||
// Verify policies were cleared.
|
||||
policies, err = s.DS.ListGlobalPolicies(ctx, fleet.ListOptions{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, policies, 0)
|
||||
|
||||
appCfg, err = s.DS.AppConfig(ctx)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify agent_options were cleared (set to null).
|
||||
require.Nil(t, appCfg.AgentOptions)
|
||||
|
||||
// Verify controls were cleared (disk encryption reverts to false).
|
||||
require.False(t, appCfg.MDM.EnableDiskEncryption.Value)
|
||||
|
||||
// Verify reports were cleared.
|
||||
queries, _, _, _, err = s.DS.ListQueries(ctx, fleet.ListQueryOptions{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, queries, 0)
|
||||
|
||||
// Verify secrets are unchanged.
|
||||
globalSecrets, err = s.DS.GetEnrollSecrets(ctx, nil)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, globalSecrets, 1)
|
||||
require.Equal(t, "boofar", globalSecrets[0].Secret)
|
||||
|
||||
// Verify labels are unchanged.
|
||||
labels, err = s.DS.LabelsByName(ctx, []string{"Test Global Label"}, fleet.TeamFilter{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, labels, 1)
|
||||
}
|
||||
|
||||
// TestOmittedTopLevelKeysFleet verifies that omitting top-level keys from a fleet
|
||||
// gitops file clears the corresponding settings (e.g. policies, agent_options, settings).
|
||||
func (s *enterpriseIntegrationGitopsTestSuite) TestOmittedTopLevelKeysFleet() {
|
||||
t := s.T()
|
||||
ctx := context.Background()
|
||||
|
||||
user := s.createGitOpsUser(t)
|
||||
fleetctlConfig := s.createFleetctlConfig(t, user)
|
||||
t.Setenv("FLEET_URL", s.Server.URL)
|
||||
testing_utils.StartSoftwareInstallerServer(t)
|
||||
|
||||
fleetName := "Test Omitted Keys " + uuid.NewString()
|
||||
|
||||
// Step 1: Apply a full fleet config with policies, agent_options, controls, features, reports, and software.
|
||||
fullFleetConfig := fmt.Sprintf(`
|
||||
name: %s
|
||||
settings:
|
||||
secrets:
|
||||
- secret: foobar
|
||||
features:
|
||||
enable_host_users: false
|
||||
agent_options:
|
||||
config:
|
||||
options:
|
||||
pack_delimiter: /
|
||||
controls:
|
||||
enable_disk_encryption: true
|
||||
policies:
|
||||
- name: Test Fleet Policy
|
||||
query: SELECT 1;
|
||||
reports:
|
||||
- name: Test Fleet Report
|
||||
query: SELECT 1;
|
||||
automations_enabled: false
|
||||
software:
|
||||
packages:
|
||||
- url: ${SOFTWARE_INSTALLER_URL}/ruby.deb
|
||||
`, fleetName)
|
||||
|
||||
fullFleetFile, err := os.CreateTemp(t.TempDir(), "*.yml")
|
||||
require.NoError(t, err)
|
||||
_, err = fullFleetFile.WriteString(fullFleetConfig)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, fullFleetFile.Close())
|
||||
|
||||
s.assertRealRunOutput(t, fleetctl.RunAppForTest(t, []string{
|
||||
"gitops", "--config", fleetctlConfig.Name(), "-f", fullFleetFile.Name(),
|
||||
}))
|
||||
|
||||
// Verify policy, agent_options, controls, features, and reports were applied.
|
||||
fl, err := s.DS.TeamByName(ctx, fleetName)
|
||||
require.NoError(t, err)
|
||||
|
||||
flPols, err := s.DS.ListMergedTeamPolicies(ctx, fl.ID, fleet.ListOptions{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, flPols, 1)
|
||||
require.Equal(t, "Test Fleet Policy", flPols[0].Name)
|
||||
|
||||
require.NotNil(t, fl.Config.AgentOptions)
|
||||
require.Contains(t, string(*fl.Config.AgentOptions), "pack_delimiter")
|
||||
require.True(t, fl.Config.MDM.EnableDiskEncryption)
|
||||
require.False(t, fl.Config.Features.EnableHostUsers)
|
||||
|
||||
flQueries, _, _, _, err := s.DS.ListQueries(ctx, fleet.ListQueryOptions{TeamID: &fl.ID})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, flQueries, 1)
|
||||
require.Equal(t, "Test Fleet Report", flQueries[0].Name)
|
||||
|
||||
flSecrets, err := s.DS.GetEnrollSecrets(ctx, &fl.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, flSecrets, 1)
|
||||
require.Equal(t, "foobar", flSecrets[0].Secret)
|
||||
|
||||
titles, _, _, err := s.DS.ListSoftwareTitles(ctx, fleet.SoftwareTitleListOptions{AvailableForInstall: true, TeamID: &fl.ID},
|
||||
fleet.TeamFilter{User: test.UserAdmin})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, titles, 1)
|
||||
|
||||
// Step 2: Apply a minimal fleet config that omits policies, agent_options, controls, reports, software, settings.
|
||||
minimalFleetConfig := fmt.Sprintf(`
|
||||
name: %s
|
||||
`, fleetName)
|
||||
|
||||
minimalFleetFile, err := os.CreateTemp(t.TempDir(), "*.yml")
|
||||
require.NoError(t, err)
|
||||
_, err = minimalFleetFile.WriteString(minimalFleetConfig)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, minimalFleetFile.Close())
|
||||
|
||||
s.assertRealRunOutput(t, fleetctl.RunAppForTest(t, []string{
|
||||
"gitops", "--config", fleetctlConfig.Name(), "-f", minimalFleetFile.Name(),
|
||||
}))
|
||||
|
||||
// Verify policies were cleared.
|
||||
flPols, err = s.DS.ListMergedTeamPolicies(ctx, fl.ID, fleet.ListOptions{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, flPols, 0)
|
||||
|
||||
fl, err = s.DS.TeamByName(ctx, fleetName)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify agent_options were cleared (set to null).
|
||||
require.Nil(t, fl.Config.AgentOptions)
|
||||
|
||||
// Verify controls were cleared (disk encryption reverts to false).
|
||||
require.False(t, fl.Config.MDM.EnableDiskEncryption)
|
||||
|
||||
// Verify features reverted to defaults (enable_host_users defaults to true).
|
||||
require.True(t, fl.Config.Features.EnableHostUsers)
|
||||
|
||||
// Verify reports were cleared.
|
||||
flQueries, _, _, _, err = s.DS.ListQueries(ctx, fleet.ListQueryOptions{TeamID: &fl.ID})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, flQueries, 0)
|
||||
|
||||
// Verify secrets are unchanged.
|
||||
flSecrets, err = s.DS.GetEnrollSecrets(ctx, &fl.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, flSecrets, 1)
|
||||
require.Equal(t, "foobar", flSecrets[0].Secret)
|
||||
|
||||
// Verify software was cleared.
|
||||
titles, _, _, err = s.DS.ListSoftwareTitles(ctx, fleet.SoftwareTitleListOptions{AvailableForInstall: true, TeamID: &fl.ID},
|
||||
fleet.TeamFilter{User: test.UserAdmin})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, titles, 0)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -342,6 +342,11 @@ func GitOpsFromFile(filePath, baseDir string, appConfig *fleet.EnrichedAppConfig
|
|||
if err := json.Unmarshal(updatedBytes, &top); err != nil {
|
||||
return nil, fmt.Errorf("failed to unmarshal file %s: %w", filePath, err)
|
||||
}
|
||||
// This should never happen since we don't support empty yaml files,
|
||||
// but adding for defensive purposes.
|
||||
if top == nil {
|
||||
top = make(map[string]json.RawMessage)
|
||||
}
|
||||
|
||||
var multiError *multierror.Error
|
||||
result := &GitOps{}
|
||||
|
|
@ -391,14 +396,32 @@ func GitOpsFromFile(filePath, baseDir string, appConfig *fleet.EnrichedAppConfig
|
|||
multiError = parseNoTeamSettings(settingsRaw, result, filePath, multiError)
|
||||
}
|
||||
default:
|
||||
// Allow omitting settings key for teams, clearing all team settings as a result.
|
||||
if !settingsOk {
|
||||
multiError = multierror.Append(multiError, errors.New("'settings' is required when 'name' is provided"))
|
||||
} else {
|
||||
multiError = parseTeamSettings(settingsRaw, result, baseDir, filePath, multiError)
|
||||
settingsRaw = json.RawMessage("null")
|
||||
}
|
||||
multiError = parseTeamSettings(settingsRaw, result, baseDir, filePath, multiError)
|
||||
}
|
||||
default:
|
||||
multiError = multierror.Append(multiError, errors.New("either 'org_settings' or 'name' and 'settings' is required"))
|
||||
multiError = multierror.Append(multiError, errors.New("if `name` is not provided, 'org_settings' is required"))
|
||||
}
|
||||
|
||||
for _, topKey := range topKeys {
|
||||
// "name" is handled later with special logic based on the filename.
|
||||
// "labels" is a special case where omitting is a no-op, rather than a directive to clear settings.
|
||||
// settings keys were handled above.
|
||||
if topKey == "name" || topKey == "labels" || topKey == "settings" || topKey == "org_settings" {
|
||||
continue
|
||||
}
|
||||
// "agent_options" and "reports" are not supported in no-team/unassigned files.
|
||||
if result.IsNoTeam() && (topKey == "agent_options" || topKey == "reports") {
|
||||
continue
|
||||
}
|
||||
// Default top keys to null if not present.
|
||||
// This will clear the settings as if the key was provided with an empty value.
|
||||
if _, ok := top[topKey]; !ok {
|
||||
top[topKey] = json.RawMessage("null")
|
||||
}
|
||||
}
|
||||
|
||||
// Get the labels. If `labels:` is specified but no labels are listed, this will
|
||||
|
|
|
|||
|
|
@ -606,10 +606,10 @@ func TestInvalidGitOpsYaml(t *testing.T) {
|
|||
_, err = gitOpsFromString(t, config)
|
||||
assert.ErrorContains(t, err, "must have a 'secret' key")
|
||||
|
||||
// Missing settings (formerly settings).
|
||||
// Missing settings is now allowed (defaults to null, clearing team settings).
|
||||
config = getConfig([]string{"settings"})
|
||||
_, err = gitOpsFromString(t, config)
|
||||
assert.ErrorContains(t, err, "'settings' is required when 'name' is provided")
|
||||
assert.NoError(t, err)
|
||||
|
||||
// settings is now allowed on "no-team.yml" for webhook settings
|
||||
config = getConfig([]string{"name", "settings"}) // Exclude settings with secrets
|
||||
|
|
@ -913,17 +913,22 @@ func TestTopLevelGitOpsValidation(t *testing.T) {
|
|||
shouldPass: true,
|
||||
isTeam: true,
|
||||
},
|
||||
"missing_all": {
|
||||
// Top-level keys besides "name" and "org_settings" are now optional.
|
||||
// A file must have either "name" (team) or "org_settings" (global).
|
||||
"missing_all_global": {
|
||||
optsToExclude: []string{"controls", "reports", "policies", "agent_options", "org_settings"},
|
||||
},
|
||||
"missing_reports": {
|
||||
optsToExclude: []string{"reports"},
|
||||
shouldPass: true,
|
||||
},
|
||||
"missing_policies": {
|
||||
optsToExclude: []string{"policies"},
|
||||
shouldPass: true,
|
||||
},
|
||||
"missing_agent_options": {
|
||||
optsToExclude: []string{"agent_options"},
|
||||
shouldPass: true,
|
||||
},
|
||||
"missing_org_settings": {
|
||||
optsToExclude: []string{"org_settings"},
|
||||
|
|
@ -934,6 +939,7 @@ func TestTopLevelGitOpsValidation(t *testing.T) {
|
|||
},
|
||||
"missing_settings": {
|
||||
optsToExclude: []string{"settings"},
|
||||
shouldPass: true,
|
||||
isTeam: true,
|
||||
},
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue