add a mechanism to deprecate AppConfig settings w/ backwards-compat #7353

roperzh · 2022-08-23T12:23:43Z

Related to #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.

I will actually use this in a follow-up PR I have ready, but that's practically all noise from doing s/host_settings/features and I thought this deserves a conversation.

Checklist for submitter

If some of the following don't apply, delete the relevant line.

Added/updated tests
Manual QA for all new/changed functionality

server/fleet/app.go

mna

The general approach looks good to me, I also like your alternative suggestion to avoid unmarshaling twice but it might cause additional issues when it comes to marshaling (i.e. it would require a custom marshaler to ignore the legacy fields, right? Could be worth it if everything else works fine though).

server/fleet/app.go

mna · 2022-08-23T12:54:55Z

server/fleet/app.go

+	}
+
+	// TODO(roperzh): define and assign legacy settings to new fields. E.g:
+	// c.Features = legacy.HostSettings


The legacy approach makes sense to me.

server/fleet/app.go

chiiph · 2022-08-23T14:05:47Z

Will we have tests for this in the future PR? At this point without any deprecation, the regular appconfig tests should cover this, correct?

roperzh · 2022-08-23T14:33:40Z

Will we have tests for this in the future PR? At this point without any deprecation, the regular appconfig tests should cover this, correct?

that's right, in this PR the goal is to make all existent tests pass, the next PR introduces features in place of host_settings and includes testing specifically related to backwards-compatibility

codecov-commenter · 2022-08-23T20:04:43Z

Codecov Report

Merging #7353 (268f921) into main (7fb109e) will decrease coverage by 0.01%.
The diff coverage is 25.92%.

@@            Coverage Diff             @@
##             main    #7353      +/-   ##
==========================================
- Coverage   60.55%   60.54%   -0.02%     
==========================================
  Files         409      409              
  Lines       38864    38919      +55     
==========================================
+ Hits        23536    23564      +28     
- Misses      13019    13044      +25     
- Partials     2309     2311       +2

Impacted Files	Coverage Δ
server/fleet/app.go	`0.00% <0.00%> (ø)`
server/service/appconfig.go	`61.79% <63.63%> (+0.36%)`	⬆️
cmd/fleet/cron.go	`28.96% <0.00%> (-0.60%)`	⬇️
server/fleet/activities.go	`0.00% <0.00%> (ø)`
server/datastore/mysql/software.go	`81.97% <0.00%> (+1.78%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mna

Left some minor nits (mostly typos), the general idea LGTM!

server/fleet/app.go

mna · 2022-08-24T15:51:02Z

server/fleet/app.go

+// 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.


server/service/appconfig.go

mna · 2022-08-24T15:56:51Z

server/service/appconfig.go

@@ -39,6 +43,23 @@ type appConfigResponse struct {
 	Err error `json:"error,omitempty"`


Could Err still be a field on appConfigResponse? I feel like this is where it should belong, but there may be technical reasons not to put it there with the custom marshaling. Nothing critical if that's not possible.

totally possible, but AFAIK, we will have to do another unmarshalling pass:

if err := json.Unmarshal(data, &r.Err); err != nil { return err }

what do you think?

Yeah, I was gonna say outside of tests we only ever marshal that struct (since it's the response payload), but looks like it is used in service/client_appconfig.go for fleetctl. Might not be worth the trouble.

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

server/fleet/app.go

michalnicp · 2022-08-24T15:56:14Z

server/fleet/app.go

+	}
+
+	// TODO(roperzh): define and assign legacy settings to new fields. This has
+	// the drawback of legacy fields taking precedence over new fields if both


We don't have to do it this way. We could assign the legacy fields if and only if the new fields are empty.

that's an excellent point, will adjust the related PR that actually uses this method.

@michalnicp we would have to make the new fields pointers for that to work, right? I wonder if it's worth the hassle

If we make sure that when the appconfig is saved again, legacy fields are never saved (because anyway their values got transferred to the new fields), then having both new and legacy should never happen, and so this approach would be fine as-is? I believe this is what would happen with this implementation, correct me if I'm wrong (i.e. legacyConfig is not part of AppConfig and so would never get marshalled+saved).

that is correct, and I think I should make the comment in the code cleaner, but what I mean is that if an user specifies both a legacy config and a new config when applying a config, the legacy wins:

~/fleet $ cat /tmp/cfg.yml apiVersion: v1 kind: config spec: features: additional_queries: - "this is new" host_settings: additional_queries: - "this is old" ~/fleet $ ./build/fleetctl apply -f /tmp/cfg.yml [+] applied fleet config ~/fleet $ ./build/fleetctl get config --json | jq '.spec.features' { "enable_host_users": false, "enable_software_inventory": false, "additional_queries": [ "this is old" ] }

Oh I see. There's this ticket we have that is coming up soon-ish: #5222

It would make a lot of sense to reject configs with legacy fields when we implement this, and that would alleviate the concern here. Of course there's still the case where the user force-applies a config, so it might still make sense to avoid the legacy-wins-over-new.

thanks for the context on that, given that:

if you only apply a legacy field, we're good

if you only apply a new field, we're good

if you apply both fields at the same time, legacy wins

the validation could be then that you can't specify both a legacy field + a new field at the same time. If despite all of that, you specify --force, then I feel OK with this behavior as it's such an edge case.

Any reason why we should allow a legacy field at all, even if the new is not set? Keeping in mind that it's the fleet server instance that would do the validation (i.e. not fleetctl), so if the fleet instance knows that a field is legacy, it's because it knows about the new field, so there's no good reason to accept the legacy one

If despite all of that, you specify --force, then I feel OK with this behavior as it's such an edge case.

Yeah I'd agree with that too.

No reason on my end! this epic specified to support both host_settings (legacy) and features (new) keys, but I guess that if there's an error message when you specify host_settings we're good!

Only downside I see is that users will have to update all their configs as soon as the validation feature is implemented. Maybe we could allow both for a while? The whole deprecation/removal process definitely needs some discussion outside of both issues (this and #5222) I think

Definitely, thanks for the feedback and the example scenario, I'll make sure to discuss this with Noah when I start work on the validation part.

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

Related to #7312, this makes use of the changes introduced in #7353 to rename host_settings to features while keeping backwards compatibility.

add a mechanism to deprecate AppConfig settings w/ backwards-compat

843a4e0

roperzh requested a review from a team as a code owner August 23, 2022 12:23

typo

f842e65

roperzh temporarily deployed to Docker Hub August 23, 2022 12:24 Inactive

roperzh commented Aug 23, 2022

View reviewed changes

server/fleet/app.go Outdated Show resolved Hide resolved

mna reviewed Aug 23, 2022

View reviewed changes

better naming and handling of trailing JSON data

3b1c853

roperzh temporarily deployed to Docker Hub August 23, 2022 13:42 Inactive

improve clarity around serialization

b56c6a9

roperzh had a problem deploying to Docker Hub August 23, 2022 19:45 Failure

remove leftover

9752c0c

roperzh temporarily deployed to Docker Hub August 23, 2022 19:50 Inactive

roperzh mentioned this pull request Aug 23, 2022

deprecate host_settings in favor of features #7358

Merged

4 tasks

manually deserialize structs

58e470b

roperzh temporarily deployed to Docker Hub August 24, 2022 15:32 Inactive

add comments

200c36f

roperzh temporarily deployed to Docker Hub August 24, 2022 15:38 Inactive

roperzh requested a review from a team August 24, 2022 15:38

mna reviewed Aug 24, 2022

View reviewed changes

Update server/fleet/app.go

cce3d70

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

michalnicp reviewed Aug 24, 2022

View reviewed changes

roperzh temporarily deployed to Docker Hub August 24, 2022 16:02 Inactive

Update server/fleet/app.go

1f28417

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

roperzh temporarily deployed to Docker Hub August 24, 2022 16:02 Inactive

Roberto Dip and others added 3 commits August 24, 2022 13:02

Update server/fleet/app.go

d959ca4

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

Update server/service/appconfig.go

1bb020a

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

Update server/service/appconfig.go

2291e7d

Co-authored-by: Martin Angers <martin.n.angers@gmail.com>

roperzh temporarily deployed to Docker Hub August 24, 2022 16:03 Inactive

remove redundant tag

268f921

roperzh temporarily deployed to Docker Hub August 24, 2022 16:05 Inactive

mna approved these changes Aug 24, 2022

View reviewed changes

mna mentioned this pull request Aug 24, 2022

Add validation for config and teams YAML documents #5222

Closed

8 tasks

roperzh merged commit c943e06 into main Aug 24, 2022

roperzh deleted the 7312-config-changes branch August 24, 2022 21:40

roperzh pushed a commit that referenced this pull request Aug 25, 2022

deprecate host_settings in favor of features (#7358)

9a1d206

Related to #7312, this makes use of the changes introduced in #7353 to rename host_settings to features while keeping backwards compatibility.

roperzh mentioned this pull request Aug 29, 2022

Implement controlled rollout of features based on teams #7312

Closed

3 tasks

roperzh mentioned this pull request Jun 6, 2024

fleetd_logs table #19489

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a mechanism to deprecate AppConfig settings w/ backwards-compat #7353

add a mechanism to deprecate AppConfig settings w/ backwards-compat #7353

roperzh commented Aug 23, 2022

mna left a comment

mna Aug 23, 2022

chiiph commented Aug 23, 2022

roperzh commented Aug 23, 2022

codecov-commenter commented Aug 23, 2022 •

edited

Loading

mna left a comment

mna Aug 24, 2022

mna Aug 24, 2022

roperzh Aug 24, 2022

mna Aug 24, 2022

michalnicp Aug 24, 2022

roperzh Aug 24, 2022

roperzh Aug 24, 2022

mna Aug 24, 2022

roperzh Aug 24, 2022

mna Aug 24, 2022

roperzh Aug 24, 2022

mna Aug 24, 2022

roperzh Aug 24, 2022 •

edited

Loading

mna Aug 24, 2022

		@@ -39,6 +43,23 @@ type appConfigResponse struct {
		Err error `json:"error,omitempty"`

add a mechanism to deprecate AppConfig settings w/ backwards-compat #7353

add a mechanism to deprecate AppConfig settings w/ backwards-compat #7353

Conversation

roperzh commented Aug 23, 2022

Checklist for submitter

mna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiiph commented Aug 23, 2022

roperzh commented Aug 23, 2022

codecov-commenter commented Aug 23, 2022 • edited Loading

Codecov Report

mna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roperzh Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 23, 2022 •

edited

Loading

roperzh Aug 24, 2022 •

edited

Loading