chore(core): decompose config loading #2639
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ntrevino-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors the application's configuration loading system. The primary goal is to decompose the monolithic configuration loading into a more modular and extensible architecture. This is achieved by introducing a new Loader interface and providing distinct implementations for loading configuration from environment variables, configuration files, and default settings. The main LoadConfig function now accepts an ordered list of these loaders, allowing for clear precedence and a more robust configuration pipeline. This change improves maintainability and flexibility for future configuration requirements.
Highlights
- Modular Configuration Loading: The core configuration loading logic has been refactored to support a modular, ordered approach. Instead of a single
LoadConfigfunction handling all concerns, the system now uses a slice ofLoaderinterfaces, allowing different sources (environment variables, config files, default settings) to contribute to the final configuration in a defined precedence. - New Loader Implementations: Three new
Loaderimplementations have been introduced:ConfigFileLoaderfor file-based configuration,EnvironmentValueLoaderfor environment variables, andDefaultSettingsLoaderfor built-in default values. This separation of concerns makes the configuration system more extensible and easier to understand. - Updated Core Configuration Function: The
LoadConfigfunction inservice/pkg/config/config.gohas been updated to accept a list ofLoaderinterfaces. It now orchestrates the loading process, applying defaults and validating the configuration at each step. This change centralizes the loading logic while delegating source-specific details to individual loaders. - Explicit Configuration Loader Ordering: The application's startup process in
service/pkg/server/start.gonow explicitly defines and uses an ordered list of configuration loaders. This ensures that configuration values are applied in a predictable sequence (e.g., environment variables override file settings, which override defaults). - Configuration Tag Naming Consistency: Several
mapstructureandjsontags in configuration structs (e.g.,service/internal/auth/config.go,service/pkg/db/db.go) have been updated for consistency, primarily changing from snake_case to camelCase for better alignment with Go's idiomatic naming conventions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Config's old ways gone, New loaders now take their place, Order brings clear path.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration loading mechanism to be more modular and extensible by introducing a Loader interface. This is a significant improvement to the configuration management.
However, there are a few critical issues that need to be addressed:
- The config hot-reloading feature appears to be broken by this refactoring.
- There is a bug in
service/pkg/server/start.gothat leads to an incorrect allow-list for environment variables. - Error handling in
cmdpackages now usespanicinstead of returning errors, which is incorrect for the function signatures.
Additionally, this PR introduces breaking changes by renaming configuration keys from snake_case to camelCase (e.g., cache_refresh_interval to cacheRefreshInterval). This should be clearly documented for users.
I've left detailed comments on these issues and some other suggestions for improving code quality and maintainability.
| l.viper.OnConfigChange(func(e fsnotify.Event) { | ||
| slog.DebugContext(ctx, "environment config file changed", slog.String("file", e.Name)) | ||
|
|
||
| // First reload and validate the config | ||
| if err := l.Load(*cfg); err != nil { | ||
| slog.ErrorContext(ctx, "error reloading environment config", slog.Any("error", err)) | ||
| return | ||
| } |
There was a problem hiding this comment.
The config hot-reloading mechanism appears to be broken with this change. The OnConfigChange handler calls l.Load(*cfg), but the Load method only updates the loader's internal Viper instance by calling l.viper.ReadInConfig(). It does not update the main cfg object that was passed to Watch. Consequently, when onChange is called, it operates on the stale configuration. The previous implementation correctly unmarshalled the new configuration into the cfg object, and this functionality needs to be restored for hot-reloading to work.
| allowedEnvOverrides := make([]string, len(defaultKVs)) | ||
| for defaultKeys := range defaultKVs { | ||
| allowedEnvOverrides = append(allowedEnvOverrides, defaultKeys) | ||
| } |
There was a problem hiding this comment.
There is a bug in the initialization of the allowedEnvOverrides slice. make([]string, len(defaultKVs)) creates a slice of length len(defaultKVs) pre-filled with empty strings. Appending to this slice in the loop adds new elements after these empty strings, resulting in an incorrect allow-list that contains both empty strings and the actual keys. The slice should be initialized with a capacity but zero length.
| allowedEnvOverrides := make([]string, len(defaultKVs)) | |
| for defaultKeys := range defaultKVs { | |
| allowedEnvOverrides = append(allowedEnvOverrides, defaultKeys) | |
| } | |
| allowedEnvOverrides := make([]string, 0, len(defaultKVs)) | |
| for defaultKeys := range defaultKVs { | |
| allowedEnvOverrides = append(allowedEnvOverrides, defaultKeys) | |
| } |
| envLoader, err := config.NewEnvironmentValueLoader(configKey, nil) | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not load config: %w", err)) | ||
| } | ||
| configFileLoader, err := config.NewConfigFileLoader(configKey, configFile) | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not load config: %w", err)) | ||
| } | ||
| conf, err := config.LoadConfig(cmd.Context(), []config.Loader{ | ||
| envLoader, | ||
| configFileLoader, | ||
| }) | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not load config: %w", err)) | ||
| } |
There was a problem hiding this comment.
This function is declared to return an error, but it panics on failure, which violates its contract. It should return errors instead. The error messages could also be more specific to aid debugging.
Additionally, this block of code for loading configuration is duplicated in service/cmd/policy.go (lines 38-52) and service/cmd/provisionFixtures.go (lines 41-55). Consider refactoring this logic into a shared helper function to improve maintainability.
| envLoader, err := config.NewEnvironmentValueLoader(configKey, nil) | |
| if err != nil { | |
| panic(fmt.Errorf("could not load config: %w", err)) | |
| } | |
| configFileLoader, err := config.NewConfigFileLoader(configKey, configFile) | |
| if err != nil { | |
| panic(fmt.Errorf("could not load config: %w", err)) | |
| } | |
| conf, err := config.LoadConfig(cmd.Context(), []config.Loader{ | |
| envLoader, | |
| configFileLoader, | |
| }) | |
| if err != nil { | |
| panic(fmt.Errorf("could not load config: %w", err)) | |
| } | |
| envLoader, err := config.NewEnvironmentValueLoader(configKey, nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create environment value loader: %w", err) | |
| } | |
| configFileLoader, err := config.NewConfigFileLoader(configKey, configFile) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create config file loader: %w", err) | |
| } | |
| conf, err := config.LoadConfig(cmd.Context(), []config.Loader{ | |
| envLoader, | |
| configFileLoader, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not load config: %w", err) | |
| } |
| mostRecentConfig := &Config{} | ||
| err = orderedViper.Unmarshal(mostRecentConfig) | ||
| if err != nil { | ||
| return nil, errors.Join(err, ErrUnmarshallingConfig) | ||
| } | ||
| err = validator.New().Struct(mostRecentConfig) | ||
| if err != nil { | ||
| return nil, errors.Join(err, ErrUnmarshallingConfig) | ||
| } |
There was a problem hiding this comment.
This block unmarshals and validates the configuration on each iteration of the loader loop. This is inefficient and seems redundant, as a final unmarshal and validation is performed at the end of the function. The mostRecentConfig variable is also not used by any of the loader.Load implementations. This block can be safely removed to improve clarity and performance.
| // Validate config | ||
| //validate := validator.New() | ||
| //if err := validate.Struct(cfg); err != nil { | ||
| // return errors.Join(err, ErrUnmarshallingConfig) | ||
| //} |
There was a problem hiding this comment.
This commented-out validation logic should be removed to keep the code clean.
| if allowList != nil || len(allowList) != 0 { | ||
| allowListMap = make(map[string]struct{}) | ||
| } |
There was a problem hiding this comment.
This condition can be simplified. A nil slice has a length of 0, so checking len(allowList) > 0 is sufficient.
| if allowList != nil || len(allowList) != 0 { | |
| allowListMap = make(map[string]struct{}) | |
| } | |
| if len(allowList) > 0 { | |
| allowListMap = make(map[string]struct{}) | |
| } |
| // Get fetches a particular config value by dot-delimited key | ||
| Get(key string) (any, error) | ||
| // Load is called to load the configuration from its source before being used | ||
| Load(mostRecentConfig Config) error |
There was a problem hiding this comment.
The mostRecentConfig parameter is unused in all current implementations of this interface (ConfigFileLoader, EnvironmentValueLoader, DefaultSettingsLoader). Consider changing the signature to Load() error to simplify the interface and its implementations.
| Load(mostRecentConfig Config) error | |
| Load() error |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
Error Summary
TDF3 Benchmark Results:
Error Summary:
NANOTDF Benchmark Results:
Error Summary:
|
Proposed Changes
Checklist
Testing Instructions