Skip to content
Open
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
127 changes: 114 additions & 13 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package config

import (
"fmt"
"io/fs"
"os"
"sync"
"time"
Expand All @@ -37,7 +38,7 @@ import (
auth "github.com/trickstercache/trickster/v2/pkg/proxy/authenticator/options"
"github.com/trickstercache/trickster/v2/pkg/proxy/request/rewriter"
rwopts "github.com/trickstercache/trickster/v2/pkg/proxy/request/rewriter/options"
"gopkg.in/yaml.v2"
"go.yaml.in/yaml/v2"
)

const defaultResourceName = "default"
Expand Down Expand Up @@ -92,7 +93,9 @@ type MainConfig struct {
ServerName string `yaml:"server_name,omitempty"`

configFilePath string
configFilesPath []string
configLastModified time.Time
configFilesLastMod []time.Time
configRateLimitTime time.Time
stalenessCheckLock sync.Mutex
}
Expand All @@ -114,7 +117,9 @@ func NewConfig() *Config {
},
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 @@ -132,6 +137,18 @@ func NewConfig() *Config {
}
}

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 @@ -143,10 +160,49 @@ func (c *Config) loadFile(flags *Flags) error {
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")

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.

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 {

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.

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 {
err := yaml.Unmarshal([]byte(yml), &c)
Expand All @@ -172,15 +228,24 @@ func (c *Config) loadYAMLConfig(yml string) error {
}

// 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 {

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 @@ -228,7 +293,9 @@ func (c *Config) Clone() *Config {
nc.MgmtConfig = c.MgmtConfig.Clone()

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 @@ -287,8 +354,10 @@ func (c *Config) IsStale() bool {
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))) {

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 @@ -297,7 +366,20 @@ func (c *Config) IsStale() bool {
}

c.Main.configRateLimitTime = time.Now().Add(c.MgmtConfig.ReloadRateLimit)
t := c.CheckFileLastModified()
if len(c.Main.configFilesPath) > 0 {
for index, file := range c.Main.configFilesPath {
t := c.CheckFileLastModified(file)
if t.IsZero() {
continue
}
if !t.Equal(c.Main.configFilesLastMod[index]) {
return true
}
}
return false
}

t := c.CheckFileLastModified("")
if t.IsZero() {
return false
}
Expand All @@ -309,15 +391,31 @@ func (c *Config) IsStale() bool {
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
}
if c.MgmtConfig == nil {
c.MgmtConfig = mgmt.New()
}
c.Main.configRateLimitTime = time.Now().Add(c.MgmtConfig.ReloadRateLimit)
t := c.CheckFileLastModified()

if len(c.Main.configFilesPath) > 0 {
for index, file := range c.Main.configFilesPath {
t := c.CheckFileLastModified(file)
if t.IsZero() {

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 walks configFilesPath captured during the initial load. A newly added YAML file is never examined, while a removed file returns a zero timestamp and is skipped here. daemon.Hup calls this method before loading again, so both changes make an explicit reload report "not stale" and leave the running config unchanged. I reproduced both cases after rebasing onto current main. Could this compare a fresh directory snapshot (names and modification times) with the loaded snapshot instead?

continue
}
if !t.Equal(c.Main.configFilesLastMod[index]) {
c.Main.configFilesLastMod[index] = t
return true
}
}
return false
}

t := c.CheckFileLastModified("")
if t.IsZero() {
return false
}
Expand Down Expand Up @@ -353,7 +451,10 @@ func (c *Config) String() string {
// 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

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
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,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 @@ -310,6 +310,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
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