Skip to content

feat: add support for loading multiple config files and staleness check #908

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.

Sign up for GitHub

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

Open
Elia-Renzoni wants to merge 3 commits into
base: main
Choose a base branch
from
+206 −13
Open

feat: add support for loading multiple config files and staleness check #908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb3c6a4
feat: add support for loading multiple config files and staleness check
Elia-Renzoni Dec 29, 2025
e2c4f38
fix: linter issues
Elia-Renzoni Dec 29, 2025
67f34b1
Merge branch 'main' into config-493
Elia-Renzoni Aug 4, 2026
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
93 changes: 83 additions & 10 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import (
"fmt"
"io/fs"
"os"
"sync"
"time"
Expand Down Expand Up @@ -102,7 +103,9 @@
ServerName string `yaml:"server_name,omitempty"`

configFilePath string
configFilesPath []string
configLastModified time.Time
configFilesLastMod []time.Time
configRateLimitTime time.Time
stalenessCheckLock sync.Mutex
}
Expand All @@ -124,7 +127,9 @@
},
Logging: lo.New(),
Main: &MainConfig{
ServerName: hn,
ServerName: hn,
configFilesPath: make([]string, 0),
configFilesLastMod: make([]time.Time, 0),
},
MgmtConfig: mgmt.New(),
Metrics: mo.New(),
Expand All @@ -143,6 +148,18 @@
}
}

func (c *Config) loadConfigs(flags *Flags) error {
ok, err := c.isDir(flags)
if err != nil {
return err
}

if !ok {
return c.loadFile(flags)
}
return c.loadAndMergeFiles(flags)
}

// loadFile loads application configuration from a YAML-formatted file.
func (c *Config) loadFile(flags *Flags) error {
b, err := os.ReadFile(flags.ConfigPath)
Expand All @@ -154,10 +171,49 @@
return err
}
c.Main.configFilePath = flags.ConfigPath
c.Main.configLastModified = c.CheckFileLastModified()
c.Main.configLastModified = c.CheckFileLastModified("")
return nil
}

// loadAndMergeFiles loads application configuration from multiple YAML files
func (c *Config) loadAndMergeFiles(flags *Flags) error {
files, err := fs.Glob(os.DirFS(flags.ConfigPath), "*.yaml")

@houyuwushang houyuwushang Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only matches .yaml. A directory containing trickster.yml loads no files and eventually returns no valid backends configured, although the PR describes loading all YAML files. Is .yml intentionally unsupported? If not, this should enumerate both extensions and return a specific error when no matching files are present.

@Elia-Renzoni Elia-Renzoni Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. This is indeed a bug and not intentional. I will push a commit as soon as possible to support both extensions and return a specific error when no matching files are found.

if err != nil {
return err
}

for _, file := range files {
path := flags.ConfigPath + "/" + file
data, err := os.ReadFile(path)
if err != nil {
return err
}

if err = c.loadYAMLConfig(string(data)); err != nil {

@houyuwushang houyuwushang Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated unmarshalling keeps distinct map keys, but a later file replaces an entire same-named backend. In a local case where the second fragment only overrode origin_url, the loaded backend lost its provider. Since the PR says files are merged in order, this needs an explicit contract: deep-merge fields, reject duplicate resource names, or document that a repeated resource must be complete.

@Elia-Renzoni Elia-Renzoni Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting this. You are absolutely right I verified the code and confirmed the behavior you described.

The loadAndMergeFiles function in pkg/config/config.go loads the YAML files sequentially using yaml.Unmarshal(&c), which completely overwrites the configuration structure on each iteration instead of performing a true merge. This means that if the same backend is defined across multiple files with partial fields, the fields from the earlier files are lost.

I agree that we need an explicit contract and among the options you suggested I propose implementing an explicit rejection.

return err
}

c.Main.configFilesPath = append(
c.Main.configFilesPath,
path,
)
c.Main.configFilesLastMod = append(
c.Main.configFilesLastMod,
c.CheckFileLastModified(path),
)
}
return nil
}

func (c *Config) isDir(flags *Flags) (bool, error) {
finfo, err := os.Stat(flags.ConfigPath)
if err != nil {
return false, err
}

return finfo.IsDir(), nil
}

// loadYAMLConfig loads application configuration from a YAML-formatted byte slice.
func (c *Config) loadYAMLConfig(yml string) error {
if err := c.detectListenerSections(yml); err != nil {
Expand Down Expand Up @@ -186,15 +242,24 @@
}

// CheckFileLastModified returns the last modified date of the running config file, if present
func (c *Config) CheckFileLastModified() time.Time {
if c.Main == nil || c.Main.configFilePath == "" {
func (c *Config) CheckFileLastModified(confFile string) time.Time {

@houyuwushang houyuwushang Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes an exported v2 method from CheckFileLastModified() to CheckFileLastModified(string), which breaks external callers at compile time. Could the path-taking helper stay private while retaining the existing no-argument method for compatibility?

if c.Main == nil {
return time.Time{}
}
file, err := os.Stat(c.Main.configFilePath)

path := confFile
if path == "" {
path = c.Main.configFilePath
if path == "" {
return time.Time{}
}
}

fInfo, err := os.Stat(path)
if err != nil {
return time.Time{}
}
return file.ModTime()
return fInfo.ModTime()
}

// Process converts various raw config options into internal data structures
Expand Down Expand Up @@ -252,7 +317,9 @@
nc.legacyMgmtUsed = c.legacyMgmtUsed

nc.Main.configFilePath = c.Main.configFilePath
nc.Main.configFilesPath = c.Main.configFilesPath
nc.Main.configLastModified = c.Main.configLastModified
nc.Main.configFilesLastMod = c.Main.configFilesLastMod
nc.Main.configRateLimitTime = c.Main.configRateLimitTime

nc.Metrics.ListenAddress = c.Metrics.ListenAddress
Expand Down Expand Up @@ -311,8 +378,10 @@
c.Main.stalenessCheckLock.Lock()
defer c.Main.stalenessCheckLock.Unlock()

if c.Main == nil || c.Main.configFilePath == "" ||
time.Now().Before(c.Main.configRateLimitTime) {
if c.Main == nil ||
(len(c.Main.configFilesPath) == 0 &&
(c.Main.configFilePath == "" ||
time.Now().Before(c.Main.configRateLimitTime))) {

@houyuwushang houyuwushang Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For directory-backed configs, len(c.Main.configFilesPath) == 0 is false, so the rate-limit check is never evaluated. After an existing file changes, IsStale() scans the files and returns true on every call during the rate-limit window. The rate-limit condition should apply to both single-file and directory modes.

return false
}

Expand All @@ -321,7 +390,7 @@
}

c.Main.configRateLimitTime = time.Now().Add(time.Duration(c.MgmtConfig.ReloadRateLimit))
t := c.CheckFileLastModified()

Check failure on line 393 in pkg/config/config.go

View workflow job for this annotation

GitHub Actions / build (1.26) 

not enough arguments in call to c.CheckFileLastModified

Check failure on line 393 in pkg/config/config.go

View workflow job for this annotation

GitHub Actions / build (1.26) 

not enough arguments in call to c.CheckFileLastModified

Check failure on line 393 in pkg/config/config.go

View workflow job for this annotation

GitHub Actions / lint 

not enough arguments in call to c.CheckFileLastModified
if t.IsZero() {
return false
}
Expand All @@ -333,7 +402,8 @@
func (c *Config) CheckAndMarkReloadInProgress() bool {
c.Main.stalenessCheckLock.Lock()
defer c.Main.stalenessCheckLock.Unlock()
if c.Main == nil || c.Main.configFilePath == "" ||
if c.Main == nil ||
(c.Main.configFilePath == "" && len(c.Main.configFilesPath) == 0) ||
time.Now().Before(c.Main.configRateLimitTime) {
return false
}
Expand All @@ -341,7 +411,7 @@
c.MgmtConfig = mgmt.New()
}
c.Main.configRateLimitTime = time.Now().Add(time.Duration(c.MgmtConfig.ReloadRateLimit))
t := c.CheckFileLastModified()

Check failure on line 414 in pkg/config/config.go

View workflow job for this annotation

GitHub Actions / build (1.26) 

not enough arguments in call to c.CheckFileLastModified

Check failure on line 414 in pkg/config/config.go

View workflow job for this annotation

GitHub Actions / build (1.26) 

not enough arguments in call to c.CheckFileLastModified

Check failure on line 414 in pkg/config/config.go

View workflow job for this annotation

GitHub Actions / lint 

not enough arguments in call to c.CheckFileLastModified
if t.IsZero() {
return false
}
Expand Down Expand Up @@ -381,7 +451,10 @@
// ConfigFilePath returns the file path from which this configuration is based
func (c *Config) ConfigFilePath() string {
if c.Main != nil {
return c.Main.configFilePath
if len(c.Main.configFilesPath) == 0 {
return c.Main.configFilePath
}
return c.Flags.ConfigPath

@houyuwushang houyuwushang Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clone() copies configFilesPath but does not copy Flags, so cfg.Clone().ConfigFilePath() panics for every directory-backed config at this dereference. I reproduced that on current main. Storing the directory path in MainConfig and deep-copying the directory snapshot would avoid making this accessor depend on runtime flags.

}
return ""
}
123 changes: 121 additions & 2 deletions pkg/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,12 @@ func TestCloneBackendOptions(t *testing.T) {
func TestCheckFileLastModified(t *testing.T) {
c := NewConfig()

if !c.CheckFileLastModified().IsZero() {
if !c.CheckFileLastModified("").IsZero() {
t.Error("expected zero time")
}

c.Main.configFilePath = "\t\n"
if !c.CheckFileLastModified().IsZero() {
if !c.CheckFileLastModified("").IsZero() {
t.Error("expected zero time")
}
}
Expand Down Expand Up @@ -466,6 +466,125 @@ func TestConfig_defaulting(t *testing.T) {
}
}


const (
dataTrk1 = `
frontend:
listen_port: 8480

caches:
default:
provider: memory

backends:
default:
provider: prometheus
origin_url: http://prometheus:9090
cache_name: default
is_default: true
`

dataTrk2 = `
backends:
prometheus_backend:
provider: prometheus
origin_url: http://prometheus:9090
cache_name: mycache
is_default: true

caches:
mycache:
provider: memory
`
)

func TestLoadAndMergeFiles(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(dir + "/trickster1.yaml", []byte(dataTrk1), 0666)
if err != nil {
t.Error(err)
}
err = os.WriteFile(dir + "/trickster2.yaml", []byte(dataTrk2), 0666)
if err != nil {
t.Error(err)
}

c := NewConfig()

args := []string{"-config", dir}
sargs := make([]string, 0, len(args))
for _, v := range args {
if !strings.HasPrefix(v, "-test.") {
sargs = append(sargs, v)
}
}
flags, err := parseFlags(sargs)
if err != nil {
t.Error(err)
}

err = c.loadAndMergeFiles(flags)
if err != nil {
t.Error(err)
}

opt1, ok1 := c.Backends["default"]
opt2, ok2 := c.Backends["prometheus_backend"]

switch {
case !ok1, !ok2:
t.Error("expected true result when performing backend lookup")
case opt1.Provider != "prometheus", opt2.Provider != "prometheus":
t.Error("expected prometheus as provider")
case opt1.OriginURL != "http://prometheus:9090", opt2.OriginURL != "http://prometheus:9090":
t.Error("expected http://prometheus:9090 as origin url")
}

cOpt1, cOk1 := c.Caches["default"]
cOpt2, cOk2 := c.Caches["mycache"]

switch {
case !cOk1, !cOk2:
t.Error("expected true result when performing cache lookup")
case cOpt1.Provider != "memory", cOpt2.Provider != "memory":
t.Error("expected memory as cache provider")
}
}

func TestIsStaleWithMultipleFiles(t *testing.T) {
_, tml := emptyTestConfig()
dir := t.TempDir()
err := os.WriteFile(dir + "/trickster1.yaml", []byte(dataTrk1), 0666)
if err != nil {
t.Error(err)
}

err = os.WriteFile(dir + "/trickster2.yaml", []byte(dataTrk2), 0666)
if err != nil {
t.Error(err)
}

configs, err := Load([]string{"-config", dir})
configs.MgmtConfig.ReloadRateLimit = 0
if err != nil {
t.Error(err)
}

if configs.IsStale() {
t.Error("expected non-stale configs")
}

time.Sleep(400 * time.Millisecond)
err = os.WriteFile(dir + "/trickster1.yaml", []byte(tml), 0666)
if err != nil {
t.Error(err)
}

if !configs.IsStale() {
t.Error("expected stale configs")
}
}

// remove any values that are non-deterministic
func clean(c *Config) {
c.Main.ServerName = "trickster-test"
Expand Down
3 changes: 2 additions & 1 deletion pkg/config/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ func Load(args []string) (*Config, error) {
c.Flags = flags
return c, nil
}
if err := c.loadFile(flags); err != nil && flags.customPath {

if err := c.loadConfigs(flags); err != nil && flags.customPath {
// a user-provided path couldn't be loaded. return the error for the application to handle
return nil, err
}
Expand Down
Loading