mirror of
https://github.com/fleetdm/fleet
synced 2026-05-19 23:18:51 +00:00
26e4395926
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". |
||
|---|---|---|
| .. | ||
| archtest | ||
| authz | ||
| bindata | ||
| config | ||
| contexts | ||
| cron | ||
| datastore | ||
| errorstore | ||
| fleet | ||
| goose | ||
| health | ||
| launcher | ||
| live_query | ||
| logging | ||
| mdm | ||
| mock | ||
| policies | ||
| ptr | ||
| pubsub | ||
| service | ||
| shellquote | ||
| sso | ||
| test | ||
| version | ||
| vulnerabilities | ||
| webhooks | ||
| websocket | ||
| worker | ||
| utils.go | ||
| utils_test.go | ||