From c943e0665a7b71620c042f0b723a9b7f22020846 Mon Sep 17 00:00:00 2001 From: Roberto Dip Date: Wed, 24 Aug 2022 18:40:09 -0300 Subject: [PATCH] add a mechanism to deprecate AppConfig settings w/ backwards-compat (#7353) Related to https://github.com/fleetdm/fleet/issues/7312, the motivation behind these changes is to introduce a way to deprecate configurations in `AppConfig`, while still preserving backwards compatibility. From the Epic: > NOTE: `host_settings` is now replaced by `features`. We should still support `host_settings` as an alias to `features` for backwards compatibility, but in our communications, we should use and recommend features as the canonical way forward. --- server/fleet/app.go | 72 +++++++++++++++++++++++++++++++++++++ server/service/appconfig.go | 48 ++++++++++++++++++------- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/server/fleet/app.go b/server/fleet/app.go index b990838f9..58faf7044 100644 --- a/server/fleet/app.go +++ b/server/fleet/app.go @@ -1,8 +1,11 @@ package fleet import ( + "bytes" "encoding/json" + "errors" "fmt" + "io" "time" "github.com/fleetdm/fleet/v4/server/config" @@ -106,6 +109,9 @@ type VulnerabilitySettings struct { } // AppConfig holds server configuration that can be changed via the API. +// +// Note: management of deprecated fields is done on JSON-marshalling and uses +// the legacyConfig struct to list them. type AppConfig struct { OrgInfo OrgInfo `json:"org_info"` ServerSettings ServerSettings `json:"server_settings"` @@ -125,6 +131,13 @@ type AppConfig struct { WebhookSettings WebhookSettings `json:"webhook_settings"` Integrations Integrations `json:"integrations"` + + strictDecoding bool +} + +// legacyConfig holds settings that have been replaced, superceded or +// deprecated by other AppConfig settings. +type legacyConfig struct { } // EnrichedAppConfig contains the AppConfig along with additional fleet @@ -132,13 +145,34 @@ type AppConfig struct { // "GET /api/latest/fleet/config" API endpoint (and fleetctl get config). type EnrichedAppConfig struct { AppConfig + enrichedAppConfigFields +} +// enrichedAppConfigFields are grouped separately to aid with JSON unmarshaling +type enrichedAppConfigFields struct { UpdateInterval *UpdateIntervalConfig `json:"update_interval,omitempty"` Vulnerabilities *VulnerabilitiesConfig `json:"vulnerabilities,omitempty"` License *LicenseInfo `json:"license,omitempty"` Logging *Logging `json:"logging,omitempty"` } +// UnmarshalJSON implements the json.Unmarshaler interface to make sure we serialize +// both AppConfig and enrichedAppConfigFields properly: +// +// - If this function is not defined, AppConfig.UnmarshalJSON gets promoted and +// will be called instead. +// - If we try to unmarshal everything in one go, AppConfig.UnmarshalJSON doesn't get +// called. +func (e *EnrichedAppConfig) UnmarshalJSON(data []byte) error { + if err := json.Unmarshal(data, &e.AppConfig); err != nil { + return err + } + if err := json.Unmarshal(data, &e.enrichedAppConfigFields); err != nil { + return err + } + return nil +} + type Duration struct { time.Duration } @@ -239,6 +273,44 @@ func (c *AppConfig) ApplyDefaults() { c.WebhookSettings.Interval.Duration = 24 * time.Hour } +// EnableStrictDecoding enables strict decoding of the AppConfig struct. +func (c *AppConfig) EnableStrictDecoding() { c.strictDecoding = true } + +// UnmarshalJSON implements the json.Unmarshaler interface. +func (c *AppConfig) UnmarshalJSON(b []byte) error { + // Define a new type, this is to prevent infinite recursion when + // unmarshalling the AppConfig struct. + type cfgStructUnmarshal AppConfig + compatConfig := struct { + *legacyConfig + *cfgStructUnmarshal + }{ + &legacyConfig{}, + (*cfgStructUnmarshal)(c), + } + + decoder := json.NewDecoder(bytes.NewReader(b)) + if c.strictDecoding { + decoder.DisallowUnknownFields() + } + if err := decoder.Decode(&compatConfig); err != nil { + return err + } + if _, err := decoder.Token(); err != io.EOF { + return errors.New("unexpected extra tokens found in config") + } + + // TODO(roperzh): define and assign legacy settings to new fields. This has + // the drawback of legacy fields taking precedence over new fields if both + // are defined. Eg: + // + // if compatConfig.legacyConfig.HostSettings != nil { + // c.Features = *compatConfig.legacyConfig.HostSettings + // } + + return nil +} + // OrgInfo contains general info about the organization using Fleet. type OrgInfo struct { OrgName string `json:"org_name"` diff --git a/server/service/appconfig.go b/server/service/appconfig.go index c1be3befb..0b7b60081 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -26,7 +26,11 @@ import ( type appConfigResponse struct { fleet.AppConfig + appConfigResponseFields +} +// appConfigResponseFields are grouped separately to aid with JSON unmarshaling +type appConfigResponseFields struct { UpdateInterval *fleet.UpdateIntervalConfig `json:"update_interval"` Vulnerabilities *fleet.VulnerabilitiesConfig `json:"vulnerabilities"` @@ -40,6 +44,23 @@ type appConfigResponse struct { Err error `json:"error,omitempty"` } +// UnmarshalJSON implements the json.Unmarshaler interface to make sure we serialize +// both AppConfig and appConfigResponseFields properly: +// +// - If this function is not defined, AppConfig.UnmarshalJSON gets promoted and +// will be called instead. +// - If we try to unmarshal everything in one go, AppConfig.UnmarshalJSON doesn't get +// called. +func (r *appConfigResponse) UnmarshalJSON(data []byte) error { + if err := json.Unmarshal(data, &r.AppConfig); err != nil { + return err + } + if err := json.Unmarshal(data, &r.appConfigResponseFields); err != nil { + return err + } + return nil +} + func (r appConfigResponse) error() error { return r.Err } func getAppConfigEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (interface{}, error) { @@ -105,11 +126,13 @@ func getAppConfigEndpoint(ctx context.Context, request interface{}, svc fleet.Se WebhookSettings: config.WebhookSettings, Integrations: config.Integrations, }, - UpdateInterval: updateIntervalConfig, - Vulnerabilities: vulnConfig, - License: license, - Logging: loggingConfig, - SandboxEnabled: svc.SandboxEnabled(), + appConfigResponseFields: appConfigResponseFields{ + UpdateInterval: updateIntervalConfig, + Vulnerabilities: vulnConfig, + License: license, + Logging: loggingConfig, + SandboxEnabled: svc.SandboxEnabled(), + }, } return response, nil } @@ -157,7 +180,7 @@ func modifyAppConfigEndpoint(ctx context.Context, request interface{}, svc fleet req := request.(*modifyAppConfigRequest) config, err := svc.ModifyAppConfig(ctx, req.RawMessage) if err != nil { - return appConfigResponse{Err: err}, nil + return appConfigResponse{appConfigResponseFields: appConfigResponseFields{Err: err}}, nil } license, err := svc.License(ctx) if err != nil { @@ -169,8 +192,10 @@ func modifyAppConfigEndpoint(ctx context.Context, request interface{}, svc fleet } response := appConfigResponse{ AppConfig: *config, - License: license, - Logging: loggingConfig, + appConfigResponseFields: appConfigResponseFields{ + License: license, + Logging: loggingConfig, + }, } if response.SMTPSettings.SMTPPassword != "" { @@ -223,7 +248,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte) (*fleet.AppCo invalid := &fleet.InvalidArgumentError{} var newAppConfig fleet.AppConfig if err := json.Unmarshal(p, &newAppConfig); err != nil { - return nil, ctxerr.Wrap(ctx, err) + return nil, ctxerr.Wrap(ctx, &badRequestError{message: err.Error()}) } if newAppConfig.FleetDesktop.TransparencyURL != "" { @@ -243,9 +268,8 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte) (*fleet.AppCo } // We apply the config that is incoming to the old one - decoder := json.NewDecoder(bytes.NewReader(p)) - decoder.DisallowUnknownFields() - if err := decoder.Decode(&appConfig); err != nil { + appConfig.EnableStrictDecoding() + if err := json.Unmarshal(p, &appConfig); err != nil { return nil, ctxerr.Wrap(ctx, &badRequestError{message: err.Error()}) }