Mattermost-server: [MM-14400] Programmatically generate default.json

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Today, config/default.json is hand-generated and deviates slightly from *model.Config.SetDefaults. It is easy to forget to update this file when making changes, and the intentional differences (to distinguish "no config file" from "existing config file") are subtle (and have been perceived as mistakes).

This ticket encompasses a few tasks:

• On build, programmatically generate default.json from an invocation of *model.Config.SetDefaults, ideally by using a go:generate directive. Details up for discussion.
• Compare the generated default.json from the existing version, and get consensus on how to handle the differences. This might involve ignoring the difference, changing *model.Config.SetDefaults to match, or updating the business logic in *model.Config.SetDefaults to pick the right value for the context. For example, DisableLegacyMFA should be true for "new" config files, but false for "existing" config files. Distinguishing "new" from "existing" can probably be done by comparing existing fields in the config with their own defaults (e.g. SiteURL should always be different for an "existing" config file).
• Finally, remove default.json from the repository: it will no longer be versioned as such, but will remain available to the build process for generating the bundle.

[reflog]

I'll take this one

[hanzei]

Thanks @reflog!

[reflog]

Hi!
Two questions:

1. "go:generate" is mentioned, but if I'm not mistaken it doesn't support non-go files, so it cannot generate .json . Am I missing something?
2. I wrote a small piece of code:

func main() {
    config := &model.Config{}
    config.SetDefaults()
    if data, err := json.MarshalIndent(config, "", "  "); err != nil {
        panic(err)
    } else {
        fmt.Printf("%v", string(data))
    }

}

And the resulting JSON is quite different from the current one (default_2.json.gz).
Stuff like: SiteURL, AtRestEncryptKey, PublicLinkSalt, PushNotificationServer, GoogleSettings is either not populated or differs significantly.

What should be the source of truth and how to decide who's right?

[reflog]

Attaching a json diff of the original and the generated config.
diff.json.gz

[lieut-data]

@reflog, with respect to go:generate, the built-in tooling just scans for special comments to run general commands. See https://blog.golang.org/generate for details, but an example from there is to write something like the following in a .go file:

//go:generate goyacc -o gopher.go -p parser gopher.y

And then invoke go generate trigger that execution. You should be able to run arbitrary commands, including perhaps adding a config/default package with a main that spits out the desired JSON to STDOUT. It might get wrapped up in a nice:

//go:generate go run github.com/mattermost/mattermost-server/config/default -o default.json

allowing the CI tooling to invoke via go generate.

I'm not too hung up on wrapping this in go generate myself -- if it's easier to start with a new Makefile target that does effectively this, let's do that instead.

[lieut-data]

@reflog, thanks for the diff -- would you be able to paste a plain-text diff as a comment into this ticket for analysis? The JSON version is cool, but lacks the "old" value to easily compare what's going on.

[reflog]

@lieut-data , thanks, that makes sense! I'll use the //go:generate go run github.com/mattermost/mattermost-server/config/default -o default.json approach .

And here a regular diff:
config.patch.gz

[lieut-data]

Thanks, @reflog -- is the spacing off in the generated default.json from which the diff was created? Looks like it's just reporting a wholesale diff of the two files which makes it hard to parse.

I'd suggest using something like https://stedolan.github.io/jq/ to normalize the format of the two files to be diffed, and then hopefully we can review /that/.

Note too that GitHub supports diff as an inline filetype, so if you paste the changes as a comment directly vs. attaching and wrap with bacticks and a diff code-type designation, it will be super easy for anyone to consume here.

[reflog]

@lieut-data sorry for the messy diff. Here's a clean one:

431c431
<     "SiteURL": "",
---
>     "SiteURL": "http://localhost:8065",
457d456
<     "EnableAPIv3": false,
489d487
<     "ExperimentalEnableAuthenticationTransfer": true,
495a494
>     "ExperimentalEnableAuthenticationTransfer": true,
497d495
<     "EnablePreviewFeatures": true,
498a497
>     "EnablePreviewFeatures": true,
502a502,504
>     "ImageProxyType": "",
>     "ImageProxyURL": "",
>     "ImageProxyOptions": "",
505a508
>     "ExperimentalStrictCSRFEnforcement": false,
508c511
<     "ExperimentalStrictCSRFEnforcement": false
---
>     "DisableBotsWhenOwnerIsDeactivated": true
541,545c544
<     "ExperimentalDefaultChannels": ""
<   },
<   "DisplaySettings": {
<     "CustomUrlSchemes": [],
<     "ExperimentalTimezone": false
---
>     "ExperimentalDefaultChannels": []
564c563
<     "AtRestEncryptKey": "",
---
>     "AtRestEncryptKey": "msjnisasdurjtqnwhk46f7ec7k3aihpa",
573a573
>     "FileFormat": "",
593c593
<     "PublicLinkSalt": "",
---
>     "PublicLinkSalt": "qrr71icsn7xosonsuacdamrsenmwzqqc",
622,623c622,623
<     "SendPushNotifications": true,
<     "PushNotificationServer": "https://push-test.mattermost.com",
---
>     "SendPushNotifications": false,
>     "PushNotificationServer": "",
654c654,656
<     "SupportEmail": "[email protected]"
---
>     "SupportEmail": "[email protected]",
>     "CustomTermsOfServiceEnabled": false,
>     "CustomTermsOfServiceReAcceptancePeriod": 365
682,685c684,687
<     "Scope": "profile email",
<     "AuthEndpoint": "https://accounts.google.com/o/oauth2/v2/auth",
<     "TokenEndpoint": "https://www.googleapis.com/oauth2/v4/token",
<     "UserApiEndpoint": "https://www.googleapis.com/plus/v1/people/me"
---
>     "Scope": "",
>     "AuthEndpoint": "",
>     "TokenEndpoint": "",
>     "UserApiEndpoint": ""
691,694c693,696
<     "Scope": "User.Read",
<     "AuthEndpoint": "https://login.microsoftonline.com/common/oauth2/v2.0/authorize",
<     "TokenEndpoint": "https://login.microsoftonline.com/common/oauth2/v2.0/token",
<     "UserApiEndpoint": "https://graph.microsoft.com/v1.0/me"
---
>     "Scope": "",
>     "AuthEndpoint": "",
>     "TokenEndpoint": "",
>     "UserApiEndpoint": ""
722,724c724,726
<     "LoginButtonColor": "",
<     "LoginButtonBorderColor": "",
<     "LoginButtonTextColor": ""
---
>     "LoginButtonColor": "#0000",
>     "LoginButtonBorderColor": "#2389D7",
>     "LoginButtonTextColor": "#2389D7"
759,761c761,763
<     "LoginButtonColor": "",
<     "LoginButtonBorderColor": "",
<     "LoginButtonTextColor": ""
---
>     "LoginButtonColor": "#34a28b",
>     "LoginButtonBorderColor": "#2389D7",
>     "LoginButtonTextColor": "#ffffff"
789a792
>     "EnableClickToReply": false,
791,792c794
<     "RestrictSystemAdmin": false,
<     "EnableClickToReply": false
---
>     "RestrictSystemAdmin": false
826a829
>     "ExportFormat": "actiance",
829d831
<     "FileLocation": "export",
848a851,854
>   },
>   "DisplaySettings": {
>     "CustomUrlSchemes": [],
>     "ExperimentalTimezone": false

[lieut-data]

@reflog, here's a proposal for generating default.json programmatically:

• SiteURL should default to localhost:8065
• EnableAPIv3 should be removed
• ExperimentalEnableAuthenticationTransfer: no changes needed? seems both are true
• EnablePreviewFeatures: no changes needed? seems both are true
• ImageProxyType should be omitted, as it is deprecated
• ImageProxyUrl should be omitted, as it is deprecated
• ImageProxyOptions should be omitted, as it is deprecated
• ExperimentalStrictCSRFEnforcement: no changes needed? seems both are false
• DisableBotsWhenOwnerIsDeactivated should default to true
• ExperimentalDefaultChannels should default to []
• AtRestEncryptKey should default to "", though we'll need a way to distinguish getting an "empty" defaults vs. a "populated" defaults
• FileFormat should default to ""
• PublicLinkSalt should default to "", though we'll need a way to distinguish getting an "empty" defaults vs. a "populated" defaults
• SupportEmail: no changes needed? seems both are [email protected]
• CustomTermsOfServiceEnabled should default to false
• CustomTermsOfServiceReAcceptancePeriod should default to 365
• Scope, AuthEndpoint, TokenEndpoint, UserApiEndpoint: these should default to their respective values in the existing default.json. It might mean we introduce a new type with a new implementation of setDefaults vs. the generic SSOSettings today.
• LoginButtonColor, LoginButtonBorderColor, LoginButtonTextColor should default to their respective values in setDefaults
• EnableClickToReply: no changes needed? seems both are false
• RestrictSystemAdmin: no changes needed? seems both are false
• ExportFormat should default to the value from setDefaults
• CustomUrlSchemes: no changes needed? seems both are []
• ExperimentalTimezone: no changes needed? seems both are false

That really just leaves the following points to discuss:

• SendPushNotifications
• PushNotificationServer

I'd suggest contacting @hmhealey to discuss the above two further, as well as @DSchalla for the pending security. change to default.json.

[DSchalla]

Hi @reflog,
thanks for your proposal, first of all!
There is right now only a single security change called DisableLegacyMFA which we want to enable by default for new installations, but cannot do that for already existing ones since it's a breaking change. The PR to set that setting to true in the default.json was merged yesterday. So as described in the issue itself:

For example, DisableLegacyMFA should be true for "new" config files, but false for "existing" config files. Distinguishing "new" from "existing" can probably be done by comparing existing fields in the config with their own defaults (e.g. SiteURL should always be different for an "existing" config file).

Otherwise there are right now no special cases for any security settings I'd be aware of. If you got any other questions to specific configuration items, please just ping me here or on our community instance!

[reflog]

@lieut-data , @DSchalla I think I'm a little confused, so if you don't mind, can you provide a clarification:

• The idea of this ticket is to delete default.json from the repo and build it from the defaults model. So far - so good :)
• There are some differences between the model and current default.json and they need to be addressed - this is also clear and you provided plenty information
• What confuses me is the following part of the ticket: e.g. SiteURL should always be different for an "existing" config file. What do you mean by 'existing config file'? Aren't we generating default.json and not config.json? If so - there is no 'existing file' at build time, since there will be no default.json in the repo.

Hope I explained my conundrum ok :)

[lieut-data]

Good questions, @reflog. The current default.json is copied to become the stock config.json in new installations. This makes it easy to start customizing a simple Mattermost install out-of-the box.

Your changes will allow us to generate default.json programmatically -- something we'll probably kick at build time to generate config.json, effectively discarding the very idea of default.json.

Now, the nuance with respect to SiteURL and such is that generating the default configuration comes down to invoking SetDefaults on the current config, but we use SetDefaults to upgrade /existing/ configs too. Effectively, SetDefaults needs to be able to decide whether it's writing out the defaults for a "new" install or an "existing" install, in order to properly flip bits like DisableLegacyMFA in the right direction. One way to do that is to detect if SiteURL is empty or not: an empty value should be indicative of a "new install".

Does that clarify the concern?

[reflog]

Thanks @lieut-data , this helps alot!
If I'm understanding correctly, instead of just calling SetDefaults and dumping the resulting JSON, I should try and do ConfigFromJson from the existing config.json and then do the SetDefaults, while also modifying SetDefaults to take care of the "bit flipping".

Sounds good?

[lieut-data]

@reflog, that's almost right, but I note there's no need to go and read any "existing" configuration explicitly. Here's an example invocation where we use SetDefaults on a just-loaded configuration: https://github.com/mattermost/mattermost-server/blob/master/config/common.go#L112.

On such a config, SetDefaults will go through and make sure none of the fields are nil, effectively upgrading the config to the new code semantics. Your changes, then, would live inside the SetDefaults call chain, but you might do something up front like:

isNewConfig := this.ServiceSettings.SiteURL == SERVICE_SETTINGS_DEFAULT_SITE_URL
// ...
if someField == nil && isNewConfig {
    // flip the bit one way
} else if someField == nil && !isNewConfig {
    // flip the bit the other way
}

The assumption above being that the SiteURL is always modified for a "real installation", making it appropriate to use to distinguish a new installation from a previously configured one.

[reflog]

Super. Thanks for the clarification!

[reflog]

@lieut-data , I've started with the work (please see the WIP PR) and got more questions (sorry)

1. If config.NewFileStore("config.json", false) already returns a Config object that is "upgraded" using SetDefaults - then what is the purpose of the go generate ? Why do we need to output anything on build time if the config will get created (properly, with handling of new/old bit flipping)?
2. Flipping the state of stuff like DisableLegacyMFA is clear, however I am not sure what to do with stuff like: CustomTermsOfServiceReAcceptancePeriod - there seems to be no change needed. If the value is not set, it'll get the defaults. Am I missing something?

Thanks again!

[lieut-data]

Questions are great, @reflog -- don't be afraid to keep clarifying :)

1. The go generate workflow will be used to generate the new default config.json that we ship with the build. Instead of copying the default.json to config.json as the CI server does today, it will instead use your tooling to generate this file. (The problem today is that we manage default.json by hand.)
2. You shouldn't have to do anything to fields whose "new" and "existing" defaults are the same: the SetDefaults will handle that already. The only change we should need is for those few fields where we want to flip the bit one way or another based on whether we're upgrading an existing config or generating a config from scratch (e.g. during the CI build).

I think your draft PR is on the right track: I'll make a few comments there.

[reflog]

@lieut-data - thanks for a great code review and explanations!
I'll start on the changes now.

[hmhealey]

For the push notification settings, I'll figure out what the default should be. I'd assume that it makes sense to have it on by default like it was in the old default.json, but I'll see if it was intentional how it worked before

[hmhealey]

Looks like push notifications should be on by default as of https://github.com/mattermost/mattermost-server/pull/9774, so SendPushNotifications should be true, and PushNotificationServer should be "https://push-test.mattermost.com".