feat: support repeatable, required -config flag across config loaders#2877
Open
renuka-fernando wants to merge 7 commits into
Open
feat: support repeatable, required -config flag across config loaders#2877renuka-fernando wants to merge 7 commits into
renuka-fernando wants to merge 7 commits into
Conversation
…g flag Make the gateway-controller -config flag repeatable so config can be layered as a base file plus thin overlays instead of one monolithic file. - main.go: back -config with a repeatable flag.Var; require at least one - config.go: LoadConfig now variadic, merging files in order with last-wins precedence via koanf StrictMerge (maps deep-merge, arrays replace); interpolation still runs once after all files are merged - add tests for deep-merge, array-replace, strict type-mismatch, interpolation-after-merge, order precedence, and single-file regression Closes wso2#2865 Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
…nfig flag Extend the multi-config-file support from the gateway-controller (wso2#2865) to the gateway-runtime policy-engine, so config can be layered as a base file plus thin overlays instead of one monolithic file. - main.go: back -config with a repeatable flag.Var; require at least one - config.go: Load now variadic, merging files in order with last-wins precedence via koanf StrictMerge (maps deep-merge, arrays replace); drop the optional-file branch so the loader fails closed instead of silently running on defaults; interpolation still runs once after merge - add tests for deep-merge, array-replace, strict type-mismatch, interpolation-after-merge, and order precedence; update the empty-path test to assert the new fail-closed contract Closes wso2#2865 Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Adopt the layered multi-config-file pattern (wso2#2874) so config can be a base file plus thin overlays, and unify the flag behavior with the gateway components (wso2#2865): required, repeatable, no default path. - cmd/main.go: back -config with a repeatable flag.Var; require at least one file (fail fast instead of silently booting on defaults); correct the misleading "env vars take priority" help text - config.go: LoadConfig now variadic, merging files in order with last-wins precedence via koanf StrictMerge (maps deep-merge, arrays replace) before the platform_api Cut + interpolation; configFilePath singleton becomes a slice; add SetConfigPaths - platform/options.go: add WithConfigPaths alongside WithConfigPath - add tests for deep-merge, array-replace, strict type-mismatch, interpolation-after-merge, and order precedence The embeddable LoadConfig/platform façade still permits zero files (env + defaults via WithConfig); only the platform-api binary requires -config. Both deployment paths (all-in-one compose, platform-api Helm chart) already pass -config, so no runtime breakage. BREAKING CHANGE: the platform-api binary now requires at least one -config flag. Running it with no -config (previously booted on env + built-in defaults) exits non-zero. Deployments already pass -config; local `go run ./cmd/main.go` invocations must now add -config <file>. Closes wso2#2874 Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
…uired Adopt the layered multi-config-file pattern (wso2#2873) so config can be a base file plus thin overlays, and unify the flag behavior with the gateway components (wso2#2865): required, repeatable, no default path. - main.go: back -config with a repeatable flag.Var; require at least one - config.go: Load now variadic with last-wins merge; drop the DefaultConfigFile fallback so a missing/absent file fails fast instead of silently applying built-in defaults - settings.go: loadConfigKoanf loops the files with a koanf StrictMerge instance (maps deep-merge, arrays replace); a missing explicitly-passed file is now a hard error, not a no-op - Dockerfile: ENTRYPOINT passes -config /etc/ai-workspace/config.toml explicitly (the container previously relied on the default path; the Helm chart inherits this via the image ENTRYPOINT) - add tests for deep-merge, strict type-mismatch, interpolation-after- merge, order precedence, and the no-files/missing-file fail-closed paths BREAKING CHANGE: the ai-workspace BFF now requires at least one -config flag and no longer falls back to /etc/ai-workspace/config.toml. The image ENTRYPOINT and `make bff-run` already pass -config, so running the published image or the standard dev target is unaffected; only a bare `bff` invocation that relied on the default path must now pass -config. Closes wso2#2873 Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Update the platform-bootstrap implementation spec for the repeatable, required -config flag (wso2#2874): - fix the stale "parses environment variables" description — config comes from the layered -config file(s) plus {{ env }}/{{ file }} tokens, not an env-var provider - the verification step now runs go run ./cmd/main.go with -config and notes the binary exits non-zero when no -config is given Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
renuka-fernando
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Induwara04,
Krishanx92,
PasanT9,
Piumal1999,
RakhithaRR,
Tharsanan1,
Thushani-Jayasekera,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
lasanthaS,
malinthaprasan,
pubudu538,
senthuran16,
tgtshanika,
tharikaGitHub,
tharindu1st and
thivindu
as code owners
July 26, 2026 11:33
Contributor
📝 WalkthroughWalkthroughThe change adds repeatable, required ChangesLayered configuration support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Application
participant ConfigLoader
participant Koanf
CLI->>Application: Parse repeated -config paths
Application->>ConfigLoader: Load(paths...)
ConfigLoader->>Koanf: Load files in argument order
Koanf-->>ConfigLoader: Merged configuration
ConfigLoader-->>Application: Interpolate and unmarshal config
Application-->>CLI: Start or report configuration error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/config/config_multifile_test.go (1)
106-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest interpolation timing with an overridden token. These tests pass whether interpolation occurs before or after merging because the overlay changes an unrelated field. Make the base define a token that would fail if evaluated, then override that exact key with a literal in the overlay and assert loading succeeds with the literal.
gateway/gateway-controller/pkg/config/config_multifile_test.go#L106-L127: add the timing-distinguishing override case.gateway/gateway-runtime/policy-engine/internal/config/config_multifile_test.go#L105-L126: add the equivalent policy-engine case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/config/config_multifile_test.go` around lines 106 - 127, Update TestLoadConfig_MultiFile_InterpolationAfterMerge in gateway/gateway-controller/pkg/config/config_multifile_test.go:106-127 so the base defines an invalid or failing interpolation token, the overlay overrides that exact key with a literal, and the test asserts loading succeeds with the literal value. Apply the equivalent timing-distinguishing change to the corresponding policy-engine test in gateway/gateway-runtime/policy-engine/internal/config/config_multifile_test.go:105-126.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/gateway-controller/pkg/config/config.go`:
- Around line 715-718: Validate each config path in the loaders at
gateway/gateway-controller/pkg/config/config.go:715-718 and
gateway/gateway-runtime/policy-engine/internal/config/config.go:369-371 before
calling file.Provider: reject null bytes and URL-encoded traversal, resolve and
clean the absolute path, then require strict containment under the
server-controlled config root using a separator-suffixed prefix check. Pass only
the validated path to file.Provider and preserve the existing load error
behavior.
In `@platform-api/config/config.go`:
- Around line 443-446: Update the config-path handling in LoadConfig to return
an error when any configPath is empty instead of continuing and loading
defaults; preserve normal processing for non-empty paths. Add a regression test
covering LoadConfig("") that asserts the expected error.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/config/config_multifile_test.go`:
- Around line 106-127: Update TestLoadConfig_MultiFile_InterpolationAfterMerge
in gateway/gateway-controller/pkg/config/config_multifile_test.go:106-127 so the
base defines an invalid or failing interpolation token, the overlay overrides
that exact key with a literal, and the test asserts loading succeeds with the
literal value. Apply the equivalent timing-distinguishing change to the
corresponding policy-engine test in
gateway/gateway-runtime/policy-engine/internal/config/config_multifile_test.go:105-126.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cfe529e2-5292-4207-9d40-309f6e7c458d
📒 Files selected for processing (17)
gateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/config_multifile_test.gogateway/gateway-runtime/policy-engine/cmd/policy-engine/main.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/gateway-runtime/policy-engine/internal/config/config_multifile_test.gogateway/gateway-runtime/policy-engine/internal/config/config_test.goplatform-api/cmd/main.goplatform-api/config/config.goplatform-api/config/config_multifile_test.goplatform-api/platform/options.goplatform-api/spec/impls/platform-bootstrap.mdportals/ai-workspace/Dockerfileportals/ai-workspace/bff/internal/config/config.goportals/ai-workspace/bff/internal/config/config_multifile_test.goportals/ai-workspace/bff/internal/config/settings.goportals/ai-workspace/bff/main.go
renuka-fernando
added a commit
to renuka-fernando/api-platform
that referenced
this pull request
Jul 26, 2026
An explicit empty-string config path (e.g. the binary invoked with
`-config=`) was skipped by the load loop, so it fell through to zero
effective files. Return a clear "config path must not be empty" error
at the path check instead — matching the other loaders, which already
reject "" via file.Provider — rather than surfacing a confusing
downstream validation error (or silently booting on defaults if those
were ever valid). A zero-arg call remains the embeddable defaults path.
Adds a regression test asserting LoadConfig("") fails with the explicit
empty-path error. Addresses a CodeRabbit review finding on wso2#2877.
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
- platform-api: reject an explicit empty-string -config path with a clear error instead of skipping it (matches the other loaders); add a regression test. Addresses a CodeRabbit review finding on wso2#2877. - ai-workspace: pin the docker-compose image to :1.0.0-rc-SNAPSHOT to match the built VERSION and the sibling platform-api SNAPSHOT tag, so the pr-check stack uses the freshly built image instead of pulling the unpublished :1.0.0-rc tag (manifest unknown). Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
renuka-fernando
force-pushed
the
multi-conf-file
branch
from
July 26, 2026 15:31
41439cb to
5334b3a
Compare
…s files
StrictMerge compared raw parsed types at merge time, before interpolation.
Since an {{ env }}/{{ file }} token is a string until resolved, a numeric or
bool field set natively in one file and overridden by a token in an overlay
was wrongly rejected as a type mismatch — a legitimate, common overlay
pattern. Drop StrictMerge in all four loaders (gateway-controller,
policy-engine, platform-api, ai-workspace-bff) and rely on the weakly-typed
unmarshal plus Validate, which still fail loudly on genuinely invalid values.
- config loaders: use koanf.New instead of NewWithConf(StrictMerge); update
the doc/inline comments explaining why
- tests: rename ...StrictMergeTypeMismatchFails to ...TypeMismatchFails
(now caught at unmarshal), and add per-component regressions —
NumericOverriddenByEnvToken and EnvTokenResolvingToNonNumberFails
Deep-merge (maps), array-replace, last-wins, and fail-closed interpolation
are unchanged; only the pre-interpolation type-strictness is removed.
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/config/config_multifile_test.go (1)
71-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a scalar overriding an array-typed field across files.
ArrayReplaceNotAppendonly proves array-vs-array replacement. Removing koanfStrictMergemeans a cross-file override where a later file sets a scalar for a field typed as a slice (e.g.admin_server.allowed_ips) is no longer rejected at merge time — mapstructure'sWeaklyTypedInputsilently converts a single value into a one-element slice at unmarshal instead. Worth a regression test asserting this is the intended behavior (rather than an oversight), especially since several array-typed fields (e.g.AllowedIPs) are security-relevant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/config/config_multifile_test.go` around lines 71 - 87, The multi-file configuration tests lack coverage for a scalar overriding an array-typed field. Add a regression test alongside TestLoadConfig_MultiFile_ArrayReplaceNotAppend that loads an initial array value and a later scalar value for a slice field such as admin_server.allowed_ips, then assert LoadConfig succeeds and mapstructure conversion produces the expected one-element slice.portals/ai-workspace/bff/internal/config/config_multifile_test.go (1)
1-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing array-replace-not-append regression test for this component.
Gateway-controller, policy-engine, and platform-api each have a
..._ArrayReplaceNotAppendtest proving a later file's array replaces (rather than appends to) an earlier file's array. This suite has no equivalent, even though the same "arrays replace" merge contract applies here. Worth adding for parity and to guard the documented merge semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/bff/internal/config/config_multifile_test.go` around lines 1 - 30, Add an AI Workspace regression test named with the existing ArrayReplaceNotAppend convention in config_multifile_test.go. Configure two input files containing the same array-valued key with different elements, merge them through the component’s normal config-loading path, and assert the result contains only the later file’s array values rather than an appended combination.gateway/gateway-controller/pkg/config/config.go (1)
704-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNear-identical multi-file load logic duplicated across four packages. The "deliberately not
StrictMerge" rationale comment, the sequential file-load loop, and the pre/post-interpolation wiring are copy-pasted almost verbatim across gateway-controller, policy-engine, platform-api, and the AI Workspace BFF — despite all four already sharing theconfiginterpolatepackage for token expansion. A shared loader helper (e.g.LoadFiles(paths []string, parser koanf.Parser) (*koanf.Koanf, error)) would reduce the risk of the four copies drifting apart on a future fix or hardening change.
gateway/gateway-controller/pkg/config/config.go#L704-L753: extract the load-loop/koanf-setup portion into a shared helper and delegate here.gateway/gateway-runtime/policy-engine/internal/config/config.go#L358-L415: delegate to the same shared helper instead of duplicating the loop and comment.platform-api/config/config.go#L436-L535: delegate to the same shared helper (keeping the platform-api-specificCutstep local to this file).portals/ai-workspace/bff/internal/config/settings.go#L72-L125: delegate to the same shared helper (keeping the ai-workspace-specificCutstep local to this file).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/config/config.go` around lines 704 - 753, Extract the shared koanf setup, sequential file-loading loop, and interpolation wiring from gateway/gateway-controller/pkg/config/config.go#L704-L753, gateway/gateway-runtime/policy-engine/internal/config/config.go#L358-L415, platform-api/config/config.go#L436-L535, and portals/ai-workspace/bff/internal/config/settings.go#L72-L125 into a common helper such as LoadFiles(paths []string, parser koanf.Parser). Update all four config loaders to delegate to it, retaining platform-api’s and ai-workspace’s local Cut steps while removing duplicated merge rationale and load-loop logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/gateway-controller/pkg/config/config_multifile_test.go`:
- Around line 71-87: The multi-file configuration tests lack coverage for a
scalar overriding an array-typed field. Add a regression test alongside
TestLoadConfig_MultiFile_ArrayReplaceNotAppend that loads an initial array value
and a later scalar value for a slice field such as admin_server.allowed_ips,
then assert LoadConfig succeeds and mapstructure conversion produces the
expected one-element slice.
In `@gateway/gateway-controller/pkg/config/config.go`:
- Around line 704-753: Extract the shared koanf setup, sequential file-loading
loop, and interpolation wiring from
gateway/gateway-controller/pkg/config/config.go#L704-L753,
gateway/gateway-runtime/policy-engine/internal/config/config.go#L358-L415,
platform-api/config/config.go#L436-L535, and
portals/ai-workspace/bff/internal/config/settings.go#L72-L125 into a common
helper such as LoadFiles(paths []string, parser koanf.Parser). Update all four
config loaders to delegate to it, retaining platform-api’s and ai-workspace’s
local Cut steps while removing duplicated merge rationale and load-loop logic.
In `@portals/ai-workspace/bff/internal/config/config_multifile_test.go`:
- Around line 1-30: Add an AI Workspace regression test named with the existing
ArrayReplaceNotAppend convention in config_multifile_test.go. Configure two
input files containing the same array-valued key with different elements, merge
them through the component’s normal config-loading path, and assert the result
contains only the later file’s array values rather than an appended combination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b427725a-d96c-4fae-9d78-f765ab494bf7
📒 Files selected for processing (10)
gateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/config_multifile_test.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/gateway-runtime/policy-engine/internal/config/config_multifile_test.goplatform-api/config/config.goplatform-api/config/config_multifile_test.goportals/ai-workspace/bff/internal/config/config.goportals/ai-workspace/bff/internal/config/config_multifile_test.goportals/ai-workspace/bff/internal/config/settings.goportals/ai-workspace/docker-compose.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Unify configuration loading across the platform's Go components behind a repeatable, required
-configflag, so config can be layered as a complete base file plus thin environment/deployment overlays instead of a single monolithic file.Resolves #2865. Related #2873, #2874 (follow-ups for the AI Workspace BFF and Platform API, both included here) and #2876 (Developer Portal, tracked separately).
Goals
-configfiles, merged in order with last-wins precedence (maps deep-merge, arrays replace) via koanfStrictMerge, so a type-mismatched override across files fails loudly.{{ env }}/{{ file }}interpolation runs once, after all files are merged, so a token in the base can be resolved by a later overlay.-config(or a passed file that does not exist/parse) fails fast rather than silently booting on built-in defaults — consistent with the repo's fail-closed startup posture (GO-AUTH-011). There is no env-var provider; env reaches config only through explicit{{ env }}tokens, which is exactly why a missing config file must fail fast.Approach
c6b343a8): repeatableflag.Var;LoadConfigvariadic with StrictMerge last-wins loop.073cb77b): same flag change;Loadvariadic; dropped the loader's optional-file branch so it fails closed instead of silently running on defaults.28d3466f): repeatable + required flag;LoadConfigvariadic;configFilePathsingleton became a slice withSetConfigPaths; added aWithConfigPathsfaçade option. The embeddable library API/platformfaçade still permits zero files (env + defaults viaWithConfig); only the binary requires-config. Corrected the misleading "env vars take priority" flag help. Breaking (flag was optional).443c1595): repeatable + required flag;Load/loadConfigKoanfvariadic with StrictMerge; dropped theDefaultConfigFilefallback and the missing-file no-op; updated the image ENTRYPOINT to pass-configexplicitly (the Helm chart inherits it). Breaking (had a default path).0a76a8da): updated the platform-api bootstrap spec.Both real deployment paths for each affected binary already pass
-config(all-in-one compose, the platform-api and BFF Helm charts / image ENTRYPOINTs), so there is no runtime breakage.User stories
As an operator, I want to keep a shared base config and apply thin per-environment overlays, so I don't duplicate a full monolithic config per deployment.
Documentation
Updated
platform-api/spec/impls/platform-bootstrap.md. The-configflag help text documents the repeatable + last-wins behavior in each binary. Developer Portal docs are out of scope (tracked in #2876).Automation tests
config_multifile_test.goin gateway-controller, policy-engine, platform-api, and ai-workspace/bff — covering deep-merge/last-wins, array-replace, StrictMerge type-mismatch, interpolation-after-merge, order precedence, and the fail-closed no-files/missing-file paths. Existing single-path callers keep working via the variadic signatures. Fullconfigsuites,go vet, and builds pass for each module.Security checks
Samples
N/A
Related PRs
N/A
Test environment
Go 1.26 (module toolchains), macOS (darwin).
go build,go vet, andgo test ./.../config/...for each affected module.Checklist