Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid polluting config file when "save" (#25395) #25406

Merged
merged 3 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions cmd/web.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,15 @@ func setPort(port string) error {
defaultLocalURL += ":" + setting.HTTPPort + "/"

// Save LOCAL_ROOT_URL if port changed
setting.CfgProvider.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
if err := setting.CfgProvider.Save(); err != nil {
return fmt.Errorf("Failed to save config file: %v", err)
rootCfg := setting.CfgProvider
saveCfg, err := rootCfg.PrepareSaving()
if err != nil {
return fmt.Errorf("failed to save config file: %v", err)
}
rootCfg.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
saveCfg.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
if err = saveCfg.Save(); err != nil {
return fmt.Errorf("failed to save config file: %v", err)
}
}
return nil
Expand Down
38 changes: 35 additions & 3 deletions modules/setting/config_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package setting

import (
"errors"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -51,11 +52,17 @@ type ConfigProvider interface {
GetSection(name string) (ConfigSection, error)
Save() error
SaveTo(filename string) error

DisableSaving()
PrepareSaving() (ConfigProvider, error)
}

type iniConfigProvider struct {
opts *Options
ini *ini.File
opts *Options
ini *ini.File

disableSaving bool

newFile bool // whether the file has not existed previously
}

Expand Down Expand Up @@ -191,7 +198,7 @@ type Options struct {
// NewConfigProviderFromFile load configuration from file.
// NOTE: do not print any log except error.
func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) {
cfg := ini.Empty()
cfg := ini.Empty(ini.LoadOptions{KeyValueDelimiterOnWrite: " = "})
newFile := true

if opts.CustomConf != "" {
Expand Down Expand Up @@ -252,8 +259,13 @@ func (p *iniConfigProvider) GetSection(name string) (ConfigSection, error) {
return &iniConfigSection{sec: sec}, nil
}

var errDisableSaving = errors.New("this config can't be saved, developers should prepare a new config to save")

// Save saves the content into file
func (p *iniConfigProvider) Save() error {
if p.disableSaving {
return errDisableSaving
}
filename := p.opts.CustomConf
if filename == "" {
if !p.opts.AllowEmpty {
Expand Down Expand Up @@ -285,9 +297,29 @@ func (p *iniConfigProvider) Save() error {
}

func (p *iniConfigProvider) SaveTo(filename string) error {
if p.disableSaving {
return errDisableSaving
}
return p.ini.SaveTo(filename)
}

// DisableSaving disables the saving function, use PrepareSaving to get clear config options.
func (p *iniConfigProvider) DisableSaving() {
p.disableSaving = true
}

// PrepareSaving loads the ini from file again to get clear config options.
// Otherwise, the "MustXxx" calls would have polluted the current config provider,
// it makes the "Save" outputs a lot of garbage options
// After the INI package gets refactored, no "MustXxx" pollution, this workaround can be dropped.
func (p *iniConfigProvider) PrepareSaving() (ConfigProvider, error) {
cfgFile := p.opts.CustomConf
if cfgFile == "" {
return nil, errors.New("no config file to save")
}
return NewConfigProviderFromFile(p.opts)
}

func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting any) {
if err := rootCfg.Section(sectionName).MapTo(setting); err != nil {
log.Fatal("Failed to map %s settings: %v", sectionName, err)
Expand Down
30 changes: 27 additions & 3 deletions modules/setting/config_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ func TestNewConfigProviderFromFile(t *testing.T) {

bs, err := os.ReadFile(testFile)
assert.NoError(t, err)
assert.Equal(t, "[foo]\nk1=a\n", string(bs))
assert.Equal(t, "[foo]\nk1 = a\n", string(bs))

bs, err = os.ReadFile(testFile1)
assert.NoError(t, err)
assert.Equal(t, "[foo]\nk1=a\nk2=b\n", string(bs))
assert.Equal(t, "[foo]\nk1 = a\nk2 = b\n", string(bs))

// load existing file and save
cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
Expand All @@ -99,7 +99,7 @@ func TestNewConfigProviderFromFile(t *testing.T) {
assert.NoError(t, cfg.Save())
bs, err = os.ReadFile(testFile)
assert.NoError(t, err)
assert.Equal(t, "[foo]\nk1=a\n\n[bar]\nk1=b\n", string(bs))
assert.Equal(t, "[foo]\nk1 = a\n\n[bar]\nk1 = b\n", string(bs))
}

func TestNewConfigProviderForLocale(t *testing.T) {
Expand All @@ -119,3 +119,27 @@ func TestNewConfigProviderForLocale(t *testing.T) {
assert.Equal(t, "foo", cfg.Section("").Key("k1").String())
assert.Equal(t, "xxx", cfg.Section("").Key("k2").String())
}

func TestDisableSaving(t *testing.T) {
testFile := t.TempDir() + "/test.ini"
_ = os.WriteFile(testFile, []byte("k1=a\nk2=b"), 0o644)
cfg, err := NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
assert.NoError(t, err)

cfg.DisableSaving()
err = cfg.Save()
assert.ErrorIs(t, err, errDisableSaving)

saveCfg, err := cfg.PrepareSaving()
assert.NoError(t, err)

saveCfg.Section("").Key("k1").MustString("x")
saveCfg.Section("").Key("k2").SetValue("y")
saveCfg.Section("").Key("k3").SetValue("z")
err = saveCfg.Save()
assert.NoError(t, err)

bs, err := os.ReadFile(testFile)
assert.NoError(t, err)
assert.Equal(t, "k1 = a\nk2 = y\nk3 = z\n", string(bs))
}
13 changes: 9 additions & 4 deletions modules/setting/lfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,18 @@ func loadLFSFrom(rootCfg ConfigProvider) error {
if err != nil || n != 32 {
LFS.JWTSecretBase64, err = generate.NewJwtSecretBase64()
if err != nil {
return fmt.Errorf("Error generating JWT Secret for custom config: %v", err)
return fmt.Errorf("error generating JWT Secret for custom config: %v", err)
}

// Save secret
sec.Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64)
if err := rootCfg.Save(); err != nil {
return fmt.Errorf("Error saving JWT Secret for custom config: %v", err)
saveCfg, err := rootCfg.PrepareSaving()
if err != nil {
return fmt.Errorf("error saving JWT Secret for custom config: %v", err)
}
rootCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64)
saveCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64)
if err := saveCfg.Save(); err != nil {
return fmt.Errorf("error saving JWT Secret for custom config: %v", err)
}
}

Expand Down
7 changes: 6 additions & 1 deletion modules/setting/oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,13 @@ func loadOAuth2From(rootCfg ConfigProvider) {
}

secretBase64 := base64.RawURLEncoding.EncodeToString(key)
saveCfg, err := rootCfg.PrepareSaving()
if err != nil {
log.Fatal("save oauth2.JWT_SECRET failed: %v", err)
}
rootCfg.Section("oauth2").Key("JWT_SECRET").SetValue(secretBase64)
if err := rootCfg.Save(); err != nil {
saveCfg.Section("oauth2").Key("JWT_SECRET").SetValue(secretBase64)
if err := saveCfg.Save(); err != nil {
log.Fatal("save oauth2.JWT_SECRET failed: %v", err)
}
}
Expand Down
7 changes: 6 additions & 1 deletion modules/setting/security.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,13 @@ func generateSaveInternalToken(rootCfg ConfigProvider) {
}

InternalToken = token
saveCfg, err := rootCfg.PrepareSaving()
if err != nil {
log.Fatal("Error saving internal token: %v", err)
}
rootCfg.Section("security").Key("INTERNAL_TOKEN").SetValue(token)
if err := rootCfg.Save(); err != nil {
saveCfg.Section("security").Key("INTERNAL_TOKEN").SetValue(token)
if err = saveCfg.Save(); err != nil {
log.Fatal("Error saving internal token: %v", err)
}
}
Expand Down
3 changes: 2 additions & 1 deletion modules/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ func Init(opts *Options) {
}
var err error
CfgProvider, err = NewConfigProviderFromFile(opts)
CfgProvider.DisableSaving() // do not allow saving the CfgProvider into file, it will be polluted by the "MustXxx" calls
if err != nil {
log.Fatal("newConfigProviderFromFile[%v]: %v", opts, err)
}
Expand All @@ -214,7 +215,7 @@ func Init(opts *Options) {

// loadCommonSettingsFrom loads common configurations from a configuration provider.
func loadCommonSettingsFrom(cfg ConfigProvider) error {
// WARNNING: don't change the sequence except you know what you are doing.
// WARNING: don't change the sequence except you know what you are doing.
loadRunModeFrom(cfg)
loadLogGlobalFrom(cfg)
loadServerFrom(cfg)
Expand Down