From 26e43959263fa53cf75883c049b2630ceeaeb5fa Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Mon, 19 May 2025 11:18:28 -0500 Subject: [PATCH] Allow GitOps to clear global settings more easily using overwrite option (#29215) for #28118 # Checklist for submitter - [X] Manual QA for all new/changed functionality ## Details This PR adds an `overwrite` option to the "modify app config" API which, if set, causes the code to replace certain keys in the existing config with keys from the incoming config, without attempting any merge. This is then used by GitOps to allow it to easily clear settings that were otherwise being merged together or ignored entirely due to the PATCH semantics expected for the `fleetctl apply` use case. The new setting is utilized in this first pass for the following settings: * `sso_settings` * `smtp_settings` * `features` * `mdm.end_user_authentication` It could be expanded to several more keys that we currently handle piecemeal in the GitOps code by attempting to send empty values to the server (with varying success). Targeting `mdm.end_user_authentication` vs. all of `mdm` is based on [this bug](https://github.com/fleetdm/fleet/issues/26175) being opened. The concern with doing all of `mdm` would be that anyone who had e.g. VPP set up in their app and hadn't set it up in GitOps would have it wiped out. If we're comfortable with that risk I can update that here and update the warning accordingly. ### More detail **The way this code works _without_ Overwrite mode on** 1. We unmarshall the incoming JSON from GitOps into a fresh AppConfig struct `newAppConfig`. Anything keys not present in the incoming JSON will result in default values being set in `newAppConfig` 2. We unmarshall the incoming JSON from GitOps into the current `appConfig`. This uses an internal merge algorithm where keys not present in the JSON will generally leave the matching keys in `appConfig` untouched. We've been dealing with this by having GitOps find missing keys and explicitly set them to non-nil empty states. When arrays are encountered, they are _merged_, not replaced, which is problematic for the `features.additional_queries` use case and probably others. 3. We piecemeal replace certain data in `appConfig` with data from `newAppConfig`, and save it to the db. **The way this works _with_ Overwrite mode on** Between steps 1 and 2 above, we _copy_ certain keys from `newAppConfig` to `appConfig`. If the incoming JSON didn't have a key, the effect will be that `appConfig` now has default values for that key. For nested arrays like `features.additionalQueries`, the value in `appConfig` will be precisely what the user put in GitOps. ## Testing I tested adding/removing these settings with GitOps manually via `fleetctl gitops`. On the main branch I could reproduce the issue where omitting out these keys in my YAML did not lead to the settings being reset on my instance. With the Features settings, the issue was more granular, with inconsistent behavior when trying to remove individual nested settings. On this branch, the settings are cleared as expected at all levels of granularity. I also added some new automated tests to verify the expected behavior for these keys. All existing tests pass. If accepted this PR would supercede https://github.com/fleetdm/fleet/pull/29180 which approaches the issue from the GitOps side for sso, smtp and mdm. Adapting that approach for `features` would require custom logic to declare nested properties as "cleared". --- changes/28118-clear-gitops-settings | 1 + cmd/fleetctl/fleetctl/gitops_test.go | 192 +++++++++++++++++++++++++++ server/fleet/app.go | 7 + server/service/appconfig.go | 18 ++- server/service/client.go | 3 +- 5 files changed, 216 insertions(+), 5 deletions(-) create mode 100644 changes/28118-clear-gitops-settings diff --git a/changes/28118-clear-gitops-settings b/changes/28118-clear-gitops-settings new file mode 100644 index 0000000000..11cfa8e7e0 --- /dev/null +++ b/changes/28118-clear-gitops-settings @@ -0,0 +1 @@ +- Fixed issue where SSO settings, SMTP settings, Features and MDM end-user authentication settings would not be cleared if they were omitted from YAML files used in a GitOps run. **Warning:** if you have these settings configured via the Fleet web application and you use GitOps to manage your configuration, be sure settings are present in your global YAML settings file before your next GitOps run. diff --git a/cmd/fleetctl/fleetctl/gitops_test.go b/cmd/fleetctl/fleetctl/gitops_test.go index 3e36f36f01..51bf706ee4 100644 --- a/cmd/fleetctl/fleetctl/gitops_test.go +++ b/cmd/fleetctl/fleetctl/gitops_test.go @@ -2,6 +2,7 @@ package fleetctl import ( "context" + "encoding/json" "fmt" "net/http" "net/http/httptest" @@ -2809,3 +2810,194 @@ func TestGitOpsTeamWebhooks(t *testing.T) { require.True(t, team.Config.WebhookSettings.HostStatusWebhook.Enable) require.Equal(t, "http://coolwebhook.biz", team.Config.WebhookSettings.HostStatusWebhook.DestinationURL) } + +func TestGitOpsFeatures(t *testing.T) { + globalFileBasic := createGlobalFileBasic(t, fleetServerURL, orgName) + ds, _, _ := testing_utils.SetupFullGitOpsPremiumServer(t) + + appConfig := fleet.AppConfig{ + Features: fleet.Features{ + EnableHostUsers: true, + EnableSoftwareInventory: true, + AdditionalQueries: ptr.RawMessage(json.RawMessage(`{"query_a": "SELECT 1", "query_b": "SELECT 2"}`)), + DetailQueryOverrides: map[string]*string{ + "detail_query_a": ptr.String("SELECT a"), + "detail_query_b": nil, + }, + }, + } + + globalFileUpdatedFeatures, err := os.CreateTemp(t.TempDir(), "*.yml") + require.NoError(t, err) + _, err = globalFileUpdatedFeatures.WriteString(fmt.Sprintf( + ` +controls: +queries: +policies: +agent_options: +org_settings: + features: + enable_host_users: false + enable_software_inventory: true + additional_queries: + query_a: "SELECT 1" + detail_query_overrides: + detail_query_a: "SELECT it_works" + server_settings: + server_url: %s + org_info: + contact_url: https://example.com/contact + org_logo_url: "" + org_logo_url_light_background: "" + org_name: %s + secrets: [{"secret":"globalSecret"}] +software: +`, fleetServerURL, orgName), + ) + require.NoError(t, err) + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &appConfig, nil + } + + ds.SaveAppConfigFunc = func(ctx context.Context, config *fleet.AppConfig) error { + appConfig = *config + return nil + } + + // Do a GitOps run with updated feature settings. + _, err = RunAppNoChecks([]string{"gitops", "-f", globalFileUpdatedFeatures.Name()}) + require.NoError(t, err) + require.False(t, appConfig.Features.EnableHostUsers) + require.True(t, appConfig.Features.EnableSoftwareInventory) + + // Parse the additional queries into a map. + var additionalQueries map[string]string + err = json.Unmarshal(*appConfig.Features.AdditionalQueries, &additionalQueries) + require.NoError(t, err) + require.Equal(t, 1, len(additionalQueries)) + require.Equal(t, "SELECT 1", additionalQueries["query_a"]) + require.Equal(t, 1, len(appConfig.Features.DetailQueryOverrides)) + require.Equal(t, "SELECT it_works", *appConfig.Features.DetailQueryOverrides["detail_query_a"]) + + // Do a GitOps run with no feature settings. + _, err = RunAppNoChecks([]string{"gitops", "-f", globalFileBasic.Name()}) + require.NoError(t, err) + + require.False(t, appConfig.Features.EnableHostUsers) + require.False(t, appConfig.Features.EnableSoftwareInventory) + require.Nil(t, appConfig.Features.AdditionalQueries) + require.Nil(t, appConfig.Features.DetailQueryOverrides) +} + +func TestGitOpsSSOSettings(t *testing.T) { + globalFileBasic := createGlobalFileBasic(t, fleetServerURL, orgName) + ds, _, _ := testing_utils.SetupFullGitOpsPremiumServer(t) + + appConfig := fleet.AppConfig{ + SSOSettings: &fleet.SSOSettings{ + SSOProviderSettings: fleet.SSOProviderSettings{ + EntityID: "some-entity-id", + IssuerURI: "https://example.com/saml", + Metadata: "some-metadata", + IDPName: "some-idp-name", + }, + IDPImageURL: "https://example.com/logo.png", + EnableSSO: true, + EnableSSOIdPLogin: true, + EnableJITProvisioning: true, + EnableJITRoleSync: true, + }, + } + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &appConfig, nil + } + + ds.SaveAppConfigFunc = func(ctx context.Context, config *fleet.AppConfig) error { + appConfig = *config + return nil + } + + // Do a GitOps run with no sso settings. + _, err := RunAppNoChecks([]string{"gitops", "-f", globalFileBasic.Name()}) + require.NoError(t, err) + + require.Nil(t, appConfig.SSOSettings) +} + +func TestGitOpsSMTPSettings(t *testing.T) { + globalFileBasic := createGlobalFileBasic(t, fleetServerURL, orgName) + ds, _, _ := testing_utils.SetupFullGitOpsPremiumServer(t) + + appConfig := fleet.AppConfig{ + SMTPSettings: &fleet.SMTPSettings{ + SMTPEnabled: true, + SMTPConfigured: true, + SMTPSenderAddress: "http://example.com", + SMTPServer: "server.example.com", + SMTPPort: 587, + SMTPAuthenticationType: "smoooth", + SMTPUserName: "uzer", + SMTPPassword: "pazzword", + SMTPEnableTLS: true, + SMTPAuthenticationMethod: "crunchy", + SMTPDomain: "smtp.example.com", + SMTPVerifySSLCerts: true, + SMTPEnableStartTLS: true, + }, + } + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &appConfig, nil + } + + ds.SaveAppConfigFunc = func(ctx context.Context, config *fleet.AppConfig) error { + appConfig = *config + return nil + } + + // Do a GitOps run with no smtp settings. + _, err := RunAppNoChecks([]string{"gitops", "-f", globalFileBasic.Name()}) + require.NoError(t, err) + + require.Nil(t, appConfig.SMTPSettings) +} + +func TestGitOpsMDMAuthSettings(t *testing.T) { + globalFileBasic := createGlobalFileBasic(t, fleetServerURL, orgName) + ds, _, _ := testing_utils.SetupFullGitOpsPremiumServer(t) + + appConfig := fleet.AppConfig{ + MDM: fleet.MDM{ + EndUserAuthentication: fleet.MDMEndUserAuthentication{ + SSOProviderSettings: fleet.SSOProviderSettings{ + EntityID: "some-entity-id", + IssuerURI: "https://example.com/saml", + Metadata: "some-metadata", + IDPName: "some-idp-name", + }, + }, + }, + } + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &appConfig, nil + } + + ds.SaveAppConfigFunc = func(ctx context.Context, config *fleet.AppConfig) error { + appConfig = *config + return nil + } + + // Do a GitOps run with no mdm end user auth settings. + _, err := RunAppNoChecks([]string{"gitops", "-f", globalFileBasic.Name()}) + require.NoError(t, err) + + require.NotNil(t, appConfig.MDM.EndUserAuthentication) + require.Empty(t, appConfig.MDM.EndUserAuthentication.SSOProviderSettings.EntityID) + require.Empty(t, appConfig.MDM.EndUserAuthentication.SSOProviderSettings.IssuerURI) + require.Empty(t, appConfig.MDM.EndUserAuthentication.SSOProviderSettings.Metadata) + require.Empty(t, appConfig.MDM.EndUserAuthentication.SSOProviderSettings.MetadataURL) + require.Empty(t, appConfig.MDM.EndUserAuthentication.SSOProviderSettings.IDPName) +} diff --git a/server/fleet/app.go b/server/fleet/app.go index fcb8452bb5..92a6ab8ae2 100644 --- a/server/fleet/app.go +++ b/server/fleet/app.go @@ -1216,6 +1216,10 @@ type ApplySpecOptions struct { // NoCache indicates that cached_mysql calls should be bypassed on the server. // This is needed where related data was just updated and we need that latest data from the DB. NoCache bool + // Indicate whether or not the spec should be applied in overwrite mode. + // This means that any missing fields in the spec will be set to their default values. + // GitOps uses this mode. + Overwrite bool } type ApplyTeamSpecOptions struct { @@ -1251,6 +1255,9 @@ func (o *ApplySpecOptions) RawQuery() string { if o.NoCache { query.Set("no_cache", "true") } + if o.Overwrite { + query.Set("overwrite", "true") + } return query.Encode() } diff --git a/server/service/appconfig.go b/server/service/appconfig.go index e3ba31f396..ec315bf88c 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -235,16 +235,18 @@ func (svc *Service) AppConfigObfuscated(ctx context.Context) (*fleet.AppConfig, // ////////////////////////////////////////////////////////////////////////////// type modifyAppConfigRequest struct { - Force bool `json:"-" query:"force,optional"` // if true, bypass strict incoming json validation - DryRun bool `json:"-" query:"dry_run,optional"` // if true, apply validation but do not save changes + Force bool `json:"-" query:"force,optional"` // if true, bypass strict incoming json validation + DryRun bool `json:"-" query:"dry_run,optional"` // if true, apply validation but do not save changes + Overwrite bool `json:"-" query:"overwrite,optional"` // if true, overwrite any existing settings with the incoming ones json.RawMessage } func modifyAppConfigEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (fleet.Errorer, error) { req := request.(*modifyAppConfigRequest) appConfig, err := svc.ModifyAppConfig(ctx, req.RawMessage, fleet.ApplySpecOptions{ - Force: req.Force, - DryRun: req.DryRun, + Force: req.Force, + DryRun: req.DryRun, + Overwrite: req.Overwrite, }) if err != nil { return appConfigResponse{appConfigResponseFields: appConfigResponseFields{Err: err}}, nil @@ -351,6 +353,14 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle } } + // If we're in overwrite mode, clear out any feautures that are not explicitly specified. + if applyOpts.Overwrite { + appConfig.Features = newAppConfig.Features + appConfig.SSOSettings = newAppConfig.SSOSettings + appConfig.SMTPSettings = newAppConfig.SMTPSettings + appConfig.MDM.EndUserAuthentication = newAppConfig.MDM.EndUserAuthentication + } + // We apply the config that is incoming to the old one appConfig.EnableStrictDecoding() if err := json.Unmarshal(p, &appConfig); err != nil { diff --git a/server/service/client.go b/server/service/client.go index 30705c267b..772491491d 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -1958,7 +1958,8 @@ func (c *Client) DoGitOps( // Apply org settings, scripts, enroll secrets, team entities (software, scripts, etc.), and controls. teamIDsByName, teamsSoftwareInstallers, teamsVPPApps, teamsScripts, err := c.ApplyGroup(ctx, true, &group, baseDir, logf, appConfig, fleet.ApplyClientSpecOptions{ ApplySpecOptions: fleet.ApplySpecOptions{ - DryRun: dryRun, + DryRun: dryRun, + Overwrite: true, }, ExpandEnvConfigProfiles: true, }, teamsSoftwareInstallers, teamsVPPApps, teamsScripts)