Migrate configuration format from YAML to TOML#775
Migrate configuration format from YAML to TOML#775renuka-fernando merged 18 commits intowso2:mainfrom
Conversation
WalkthroughThis PR migrates configuration from YAML to TOML, enforces required -config flags, introduces typed config structures and an APIP_GW_ env var prefix, renames logging format option "console"→"text", updates manifests/helm/docker to use TOML and APIP_GW_, and bumps versions to 0.5.0-SNAPSHOT. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
renuka-fernando
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
renuka-fernando
changed the title
renuka-fernando
force-pushed
the
correlation
branch
from
0a5d7e8 to
cc3fa5a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/ai-gateway/analytics/moesif-analytics.md (2)
25-26: Fix step numbering typo (“Sept” → “Step”).User-facing instructions show “Sept 2/3”; should be “Step 2/3.”
🔧 Proposed fix
-> - Sept 2: Follow the onboarding wizard. -> - Sept 3: During the sign up process, you will receive a Collector Application ID for your configured application. Copy this value and keep it saved. +> - Step 2: Follow the onboarding wizard. +> - Step 3: During the sign up process, you will receive a Collector Application ID for your configured application. Copy this value and keep it saved.
69-69: Fix typo in “private key” label.“privare key” should be “private key.”
🔧 Proposed fix
-| `private_key_path` | string | No | - | Path to the privare key used for securing ALS communication when transport-level encryption or authentication is enabled. | +| `private_key_path` | string | No | - | Path to the private key used for securing ALS communication when transport-level encryption or authentication is enabled. |docs/ai-gateway/observability/tracing.md (1)
80-80: Minor text inconsistency with TOML syntax.The text mentions "set
enabled: false" using YAML-style colon syntax, but since the configuration is now TOML, it should referenceenabled = falsefor consistency.Suggested fix
-**Note:** If tracing is enabled in the configuration but the OTLP collector is not running, components will log warnings about failed trace exports. To completely disable tracing, set `enabled: false` in the configuration. +**Note:** If tracing is enabled in the configuration but the OTLP collector is not running, components will log warnings about failed trace exports. To completely disable tracing, set `enabled = false` in the configuration.docs/gateway/observability/metrics.md (1)
892-898: Use TOML key/value syntax in the troubleshooting note.
enabled: trueis YAML; in TOML it should beenabled = true.♻️ Proposed fix
-Ensure `enabled: true`. +Ensure `enabled = true`.docs/gateway/observability/tracing.md (1)
80-81: Use TOML syntax in the prose examples.Replace
enabled: falseandenabled: truewithenabled = falseandenabled = trueto match the actual TOML configuration format.The documentation incorrectly uses YAML-style colon syntax at lines 80-81 and 730-736, while the actual
config.tomluses TOML-style equals syntax.
🤖 Fix all issues with AI agents
In `@docs/gateway/observability/tracing.md`:
- Around line 41-51: The disablement guidance references a non-existent section
`[policy_engine.tracing]`; update the disable example to use the actual config
key `[tracing]` (matching the schema and the enable example) by removing any
`[policy_engine.tracing]` entries and replacing them with `[tracing]` so both
enable/disable examples consistently use the same `tracing` section and fields
(e.g., `enabled`, `endpoint`, `service_version`, `batch_timeout`,
`max_export_batch_size`, `sampling_rate`).
In `@gateway/configs/config.toml`:
- Around line 227-236: The shutdown_timeout field under
[analytics.access_logs_service] is a time.Duration and should be written as a
duration string (e.g. "600s") instead of the integer 600; update
shutdown_timeout to a quoted duration string like "600s" while leaving integer
nanosecond fields buffer_flush_interval and grpc_request_timeout under
[analytics.grpc_access_logs] unchanged.
In `@gateway/gateway-controller/pkg/config/config.go`:
- Line 26: The koanf unmarshal call using k.Unmarshal() does not supply a
DecodeHook so TOML duration strings (e.g., for ShutdownTimeout,
ReconnectInitial, ReconnectMax, PollingInterval, BatchTimeout) fail to decode
into time.Duration; replace the k.Unmarshal call (the one that populates cfg)
with k.UnmarshalWithConf using koanf.UnmarshalConf and a
mapstructure.DecoderConfig that sets TagName:"koanf", WeaklyTypedInput:true,
Result:cfg and DecodeHook:mapstructure.StringToTimeDurationHookFunc(); also add
the import for github.com/go-viper/mapstructure/v2 to enable
mapstructure.StringToTimeDurationHookFunc().
In `@gateway/it/test-config.toml`:
- Around line 182-194: The second policy_configurations.jwtauth_v010.keymanagers
entry (name "wrong-issuer-km") defines jwks.remote using the same TOML section
path as the first element, causing a collision; fix by embedding the jwks.remote
object inside that array element using inline table syntax (e.g. set jwks = {
remote = { uri = "http://mock-jwks:8080/jwks" } } for the keymanager with name
"wrong-issuer-km") so each policy_configurations.jwtauth_v010.keymanagers
element (mock-jwks and wrong-issuer-km) carries its own jwks.remote config and
avoids shared/overwritten sections.
In `@gateway/policy-engine/internal/config/config.go`:
- Line 26: The TOML duration fields in the config structs (ConnectTimeout,
RequestTimeout, InitialReconnectDelay, MaxReconnectDelay, BatchTimeout,
ShutdownTimeout) are currently unmarshaled by Load() expecting integer
nanoseconds, which will break if users supply Go-style strings like "10s";
update Load() to add a mapstructure DecodeHook that converts string duration
values to time.Duration (parsing via time.ParseDuration) before assignment, and
ensure the DecodeHook is registered with k.Unmarshal (or the koanf/decoder) so
strings like "10s" are accepted, or alternatively add a clear comment/example
near the config struct definitions and Load() documentation stating that
durations must be integer nanoseconds if you choose not to add the DecodeHook.
🧹 Nitpick comments (9)
.vscode/launch.json (1)
53-57: Minor formatting inconsistency.There's an extra space before the comma on line 54. While valid JSON, it's inconsistent with the other configurations.
🔧 Suggested fix
- "-config", "${workspaceFolder}/gateway/configs/config.toml" , + "-config", "${workspaceFolder}/gateway/configs/config.toml",docs/gateway/policies/advanced-ratelimit.md (1)
403-416: Consider aligning parameter names between tables and examples.The parameter tables (lines 243-266) document keys in camelCase (e.g.,
keyPrefix,failureMode,includeXRateLimit), but the TOML examples use all-lowercase (e.g.,keyprefix,failuremode,includexratelimit).Based on learnings, the policy engine validates configuration paths case-insensitively, so both will work. However, aligning the examples with the documented table format would improve documentation consistency and reduce potential confusion.
docs/ai-gateway/observability/tracing.md (1)
389-393: Remaining YAML syntax should be converted to TOML.These gateway configuration snippets for alternative backends (Tempo, Datadog, Istio, Linkerd) still use YAML syntax. Since the gateway configuration has migrated to TOML, these should be updated for consistency.
Example fix for Tempo configuration
-Configure gateway to send directly to Tempo: -```yaml -tracing: - endpoint: tempo:4317 -``` +Configure gateway to send directly to Tempo: +```toml +[tracing] +endpoint = "tempo:4317" +```Similar updates needed for lines 467-471 (Datadog), 513-517 (Istio), and 523-527 (Linkerd).
gateway/gateway-controller/cmd/controller/main.go (1)
45-45: Default config path update looks good; remember to rebuild gateway images.Line 45 aligns with the TOML migration. Per gateway guidelines, rebuild images via
cd gateway && make build-local.gateway/docker-compose.yaml (1)
37-60: Remember to rebuild gateway images after this config path change.Run
cd gateway && make build-localso local images reflect the new config.toml paths. As per coding guidelines, ...gateway/configs/config.toml (4)
127-131: Security: Default credentials should include a prominent warning.Hardcoded
admin/admincredentials withpassword_hashed = falsepose a risk if users deploy without changing them. Consider adding a comment warning that these must be changed for production, or use placeholder values that will fail authentication until configured.[[gateway_controller.auth.basic.users]] +# WARNING: Change these credentials before deploying to production! username = "admin" -password = "admin" +password = "CHANGE_ME" password_hashed = false roles = ["admin"]
184-186: Consider aligning log level with gateway_controller.The policy engine defaults to
"debug"while gateway_controller uses"info". Debug level is typically too verbose for production defaults and may impact performance. Consider setting both to"info"for consistency.[policy_engine.logging] -level = "debug" +level = "info" format = "json"
247-254: High default sampling rate may impact production performance.
sampling_rate = 1.0means 100% of requests will be traced when tracing is enabled. This can cause significant overhead in production. Consider a lower default (e.g.,0.1for 10%) with a comment explaining the trade-off.[tracing] enabled = false endpoint = "otel-collector:4317" insecure = true service_version = "1.0.0" batch_timeout = "1s" max_export_batch_size = 512 -sampling_rate = 1.0 +# Sampling rate: 1.0 = 100%, 0.1 = 10%. Lower values recommended for production. +sampling_rate = 0.1
196-207: Naming convention inconsistency (may be intentional).The
policy_configurationssection uses camelCase (maxEntries,cleanupInterval,includeXRateLimit) while the rest of the file uses snake_case. Based on learnings, the policy engine validates paths case-insensitively, so this works functionally. If this matches existing policy definition conventions, it's fine to keep; otherwise, consider aligning with the file's snake_case convention.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/ai-gateway/observability/tracing.md (1)
80-80: Use TOML syntax in prose to avoid confusion.These lines still show YAML-style
enabled: .... For TOML, the doc should sayenabled = false/enabled = true.✏️ Suggested doc fix
-**Note:** If tracing is enabled in the configuration but the OTLP collector is not running, components will log warnings about failed trace exports. To completely disable tracing, set `enabled: false` in the configuration. +**Note:** If tracing is enabled in the configuration but the OTLP collector is not running, components will log warnings about failed trace exports. To completely disable tracing, set `enabled = false` in the configuration.-Ensure `enabled: true`. +Ensure `enabled = true`.Also applies to: 735-735
docs/ai-gateway/llm/semantic-caching.md (1)
48-49: Clarify override guidance for systemParameters.
Current text says system parameters can be overridden from API artifact params, which conflicts with the repo guidance that systemParameters must be configured exclusively in the gateway config file and referenced viawso2/defaultValuepaths. Please align this sentence with that rule to avoid misconfiguration. Based on learnings, ...docs/gateway/observability/metrics.md (1)
892-898: Fix YAML-style wording in TOML troubleshooting note.
The note below the command still referencesenabled: true, which is YAML syntax.💡 Suggested doc tweak
-Ensure `enabled: true`. +Ensure `enabled = true`.
🤖 Fix all issues with AI agents
In `@docs/ai-gateway/llm/guardrails/semantic-prompt-guard.md`:
- Around line 54-67: Remove the unused embedding_provider_dimension entry from
the example config.toml in the Semantic Prompt Guard docs: the Semantic Prompt
Guard policy only needs embedding_provider, embedding_provider_endpoint,
embedding_provider_model, and embedding_provider_api_key, so delete the
embedding_provider_dimension line from the config example and update the
surrounding explanatory text to not mention a dimension parameter (references:
embedding_provider_dimension, Semantic Prompt Guard policy).
In `@docs/ai-gateway/llm/semantic-caching.md`:
- Around line 79-94: The docs currently show flat config keys (e.g.,
embedding_provider, vector_db_provider_*) but must be moved under the namespaced
section policy_configurations.semantic_cache_v010 (i.e., wrap those keys inside
[policy_configurations.semantic_cache_v010]); also update any policy definition
default references (wso2/defaultValue) to the full path form such as
${config.policy_configurations.semantic_cache_v010.embedding_provider} instead
of ${config.embedding_provider}, and similarly replace other ${config.<key>}
uses with ${config.policy_configurations.semantic_cache_v010.<key>} for
vector_db_provider_*, embedding_provider_*, etc.
In `@docs/gateway/policies/advanced-ratelimit.md`:
- Around line 422-442: The TOML example under
[policy_configurations.ratelimit_v010.redis] and
[policy_configurations.ratelimit_v010.headers] uses lowercase keys that mismatch
the documented camelCase parameters; update the TOML keys to use the camelCase
names used by the config parser: change keyprefix → keyPrefix, failuremode →
failureMode, connectiontimeout → connectionTimeout, readtimeout → readTimeout,
writetimeout → writeTimeout, includexratelimit → includeXRateLimit, includeietf
→ includeIETF, and includeretryafter → includeRetryAfter so the example matches
the tables and parser.
- Around line 403-416: Update the TOML example under
[policy_configurations.ratelimit_v010] so the keys use the camelCase names
expected by the parser and docs: in the
[policy_configurations.ratelimit_v010.memory] section rename maxentries ->
maxEntries and cleanupinterval -> cleanupInterval; in the
[policy_configurations.ratelimit_v010.headers] section rename includexratelimit
-> includeXRateLimit, includeietf -> includeIETF, and includeretryafter ->
includeRetryAfter; ensure the modified keys align with the Go implementation and
the documentation tables for policy_configurations.ratelimit_v010.
♻️ Duplicate comments (4)
docs/gateway/observability/tracing.md (1)
820-826: Remove[policy_engine.tracing]from disablement guidance.The config schema uses
[tracing]exclusively. The enable section correctly shows[tracing], but this disable section includes[policy_engine.tracing]which does not exist in the configuration schema. Update to use only[tracing]for consistency.Suggested fix
-```toml -[policy_engine.tracing] -enabled = false - -[tracing] -enabled = false -``` +```toml +[tracing] +enabled = false +```gateway/policy-engine/internal/config/config.go (1)
236-238: AddDecodeHookto parse TOML string durations intotime.Durationfields.The config structs contain multiple
time.Durationfields (ConnectTimeout,RequestTimeout,InitialReconnectDelay,MaxReconnectDelay,BatchTimeout,ShutdownTimeout). With koanf v2.3.0 and TOML parsing,k.Unmarshal()at line 236 will fail to decode string durations (e.g.,"10s") withoutmapstructure.StringToTimeDurationHookFunc().Proposed fix using UnmarshalWithConf
+ "github.com/go-viper/mapstructure/v2"// Unmarshal into Config struct - if err := k.Unmarshal("", cfg); err != nil { + if err := k.UnmarshalWithConf("", cfg, koanf.UnmarshalConf{ + DecoderConfig: &mapstructure.DecoderConfig{ + TagName: "koanf", + WeaklyTypedInput: true, + DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + Result: cfg, + }, + }); err != nil { return nil, fmt.Errorf("failed to unmarshal config: %w", err) }gateway/gateway-controller/pkg/config/config.go (1)
372-374: AddDecodeHookto unmarshal TOML string durations intotime.Durationfields.The config structs contain multiple
time.Durationfields (ShutdownTimeout,BatchTimeout,ReconnectInitial,ReconnectMax,PollingInterval). With koanf v2.3.0 and TOML parsing,k.Unmarshal()at line 372 will fail to decode string durations (e.g.,"15s") withoutmapstructure.StringToTimeDurationHookFunc().Proposed fix using UnmarshalWithConf
+ "github.com/go-viper/mapstructure/v2"// Unmarshal into Config struct - if err := k.Unmarshal("", cfg); err != nil { + if err := k.UnmarshalWithConf("", cfg, koanf.UnmarshalConf{ + DecoderConfig: &mapstructure.DecoderConfig{ + TagName: "koanf", + WeaklyTypedInput: true, + DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + Result: cfg, + }, + }); err != nil { return nil, fmt.Errorf("failed to unmarshal config: %w", err) }gateway/it/test-config.toml (1)
182-194: Duplicate: nested table collides acrosskeymanagersentries.This repeats the earlier concern:
[policy_configurations.jwtauth_v010.keymanagers.jwks.remote]declared after each[[...keymanagers]]can collide or attach only to the most recent element in TOML. Consider inline tables per element or explicit nested tables scoped to each array entry.TOML array-of-tables nested table behavior: how does [a.b.c] associate with [[a.b]] elements?
🧹 Nitpick comments (1)
gateway/gateway-controller/cmd/controller/main.go (1)
45-46: Rebuild gateway images after default config change.
Please runcd gateway && make build-localso local images reflect the new TOML default. As per coding guidelines, rebuild is required after gateway component changes.
| @@ -0,0 +1,254 @@ | |||
| # Gateway Controller and Policy Engine Configuration | |||
There was a problem hiding this comment.
Shall we keep only necessary values in here and move all others to template.toml as a documentation purpose.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/gateway/observability/tracing.md`:
- Around line 199-201: Replace the invalid example that sets a top-level
policy_engine.service_name with a valid policy_engine configuration: remove the
`service_name` key and instead show nested tables such as
`[policy_engine.server]`, `[policy_engine.admin]`, `[policy_engine.xds]` and
include real keys like `extproc_port`, `enabled`, and `server_address` (e.g.,
`[policy_engine.server] extproc_port = <port>`, `[policy_engine.admin] enabled =
true`, `[policy_engine.xds] server_address = "<addr>"`) so the docs match the
actual config structure used by the policy_engine code paths.
In `@gateway/policy-engine/internal/config/config.go`:
- Around line 343-361: The current logic treats an empty
cfg.TracingConfig.Endpoint as "section missing" and replaces cfg.TracingConfig
with defaults.TracingConfig which can overwrite a user-set Enabled flag;
preserve user intent by using Enabled as the presence marker or by saving
cfg.TracingConfig.Enabled before assigning defaults and restoring it afterward.
Update the block around TracingConfig handling (refs: cfg.TracingConfig,
defaults.TracingConfig, Enabled) so that if Endpoint is empty you either check
cfg.TracingConfig.Enabled to decide whether to apply defaults, or copy the
existing Enabled value, assign defaults, and then restore the preserved Enabled
boolean so a user who set tracing.enabled=true won’t be silently disabled.
| @@ -369,7 +369,7 @@ systemParameters: | |||
| type: string | |||
| description: Prefix for all Redis keys to avoid conflicts | |||
| default: "ratelimit:v1:" | |||
| "wso2/defaultValue": "${config.policy_configurations.ratelimit_v010.redis.keyprefix}" | |||
| "wso2/defaultValue": "${config.policy_configurations.ratelimit_v010.redis.key_prefix}" | |||
There was a problem hiding this comment.
We need to remove policies from here as well and add this changes to controller repo
- Resolved conflicts in go.work.sum, gateway/configs/config.yaml, gateway/it/test-config.yaml - Upstream attempted to revert to GATEWAY_ prefix, but our APIP_GW_ changes are preserved - New upstream features include lazy resources, analytics header filter, benchmark tests - Config files use YAML format in upstream but our code still supports TOML
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/gateway/policies/advanced-ratelimit.md (1)
236-266: Update documentation tables to use snake_case parameter names.The parameter tables show camelCase names (
keyPrefix,failureMode,connectionTimeout,readTimeout,writeTimeout,maxEntries,cleanupInterval,includeXRateLimit,includeIETF,includeRetryAfter), but the TOML examples (lines 403-416, 422-442) use snake_case (key_prefix,failure_mode,connection_timeout,read_timeout,write_timeout,max_entries,cleanup_interval,include_x_rate_limit,include_ietf,include_retry_after). Since the PR migrates configuration fields to snake_case, the tables must be updated to match the actual TOML format to prevent user configuration errors.📋 Required table updates
Redis Configuration table (lines 236-247):
keyPrefix→key_prefixfailureMode→failure_modeconnectionTimeout→connection_timeoutreadTimeout→read_timeoutwriteTimeout→write_timeoutMemory Configuration table (lines 254-256):
maxEntries→max_entriescleanupInterval→cleanup_intervalHeaders Configuration table (lines 263-266):
includeXRateLimit→include_x_rate_limitincludeIETF→include_ietfincludeRetryAfter→include_retry_aftergateway/gateway-controller/default-policies/advanced-ratelimit-v0.1.1.yaml (1)
368-399: Redis backend configuration section missing from config.toml.The YAML policy references
config.policy_configurations.ratelimit_v010.redis.*keys (key_prefix, failure_mode, connection_timeout, read_timeout, write_timeout, host, port, username, password, db), but the redis section is not defined ingateway/configs/config.toml. When backend is set to "redis", these config overrides will fail silently and hardcoded defaults will apply. Add the missing[policy_configurations.ratelimit_v010.redis]section to the config file with the required fields.The memory and headers sections are correctly configured with snake_case keys.
Also applies to: 406-422, 429-451
kubernetes/gateway-operator/internal/controller/resources/api-platform-gateway-k8s-manifests.yaml (1)
124-127: Update K8s Deployment to use TOML config with required-configcommand flag.The K8s manifest is out of sync with docker-compose.yaml. The auto-generated manifest mounts a YAML config but the application requires:
- A
-configcommand-line flag (currently missing; app exits if not provided)- A TOML-formatted config file (the manifest contains YAML)
Align with docker-compose.yaml by adding the command field to pass
-config /app/config/config.tomland update the ConfigMap to use TOML format instead of YAML.gateway/gateway-controller/pkg/config/config.go (1)
360-362: Mapcontrol_plane_urltocontrolplane.host.
control_plane_urlcurrently maps togateway_controller.controlplane.url, but the struct tag ishost, so this env override won’t apply.🛠 Proposed fix
- case "control_plane_url": - return "gateway_controller.controlplane.url" + case "control_plane_url": + return "gateway_controller.controlplane.host"gateway/it/docker-compose.test.yaml (1)
61-64:depends_on.condition: service_healthyrequires a healthcheck on gateway-controller.The policy-engine service depends on gateway-controller with
condition: service_healthy(lines 61-64), but gateway-controller has no healthcheck defined. Docker Compose will fail to start without a healthcheck on the dependency. Either add a healthcheck to gateway-controller or change to a simpledepends_onlist.🛠 Suggested fix (start-order only)
- depends_on: - gateway-controller: - condition: service_healthy + depends_on: + - gateway-controller
🧹 Nitpick comments (6)
docs/gateway/mcp/policies/mcp-authentication.md (2)
20-20: Clarify where user parameters are defined.Line 20 says “configuration yaml,” but these params are set in the MCP API definition YAML (as shown below). Suggest rewording to avoid confusion.
📝 Suggested edit
-- **User Parameters**: Configured per MCP proxy in the configuration yaml. +- **User Parameters**: Configured per MCP proxy in the MCP API definition YAML.
53-53: Fix reference to key manager path.The
issuersdescription mentionssystem.keymanagers, but the system parameter is underpolicy_configurations.jwtauth_v010.keymanagers. Align the wording with the actual TOML path to prevent misconfiguration.📝 Suggested edit
-| `issuers` | array | No | - | List of issuer names (referencing entries in `system.keymanagers`). This list is sent as `authorization_servers` in the protected resource metadata response. If omitted, all configured key managers are used. | +| `issuers` | array | No | - | List of issuer names (referencing entries in `policy_configurations.jwtauth_v010.keymanagers`). This list is sent as `authorization_servers` in the protected resource metadata response. If omitted, all configured key managers are used. |gateway/gateway-builder/cmd/builder/main.go (1)
177-200: Consider logging a warning when BUILD_DATE parsing fails.When
BUILD_DATEis set but fails to parse, the code silently falls back to the current time. This could mask configuration issues and make debugging harder. A warning log would help operators identify malformed date values.♻️ Suggested improvement
// Build date can come from environment or use current timestamp buildDateStr := os.Getenv("BUILD_DATE") var buildTimestamp time.Time if buildDateStr != "" { // Try to parse the build date parsedTime, err := time.Parse(time.RFC3339, buildDateStr) if err == nil { buildTimestamp = parsedTime } else { + slog.Warn("Failed to parse BUILD_DATE, using current time", + "build_date", buildDateStr, + "expected_format", time.RFC3339, + "error", err) buildTimestamp = time.Now().UTC() } } else { buildTimestamp = time.Now().UTC() }gateway/policy-engine/Dockerfile (1)
25-28: Add a default value forBUILD_DATEfor consistency.
VERSIONandGIT_COMMITboth have default values, butBUILD_DATEdoes not. If the build argument is not provided, this will result in an empty string being propagated to the build process.♻️ Suggested fix
# Build arguments for version information ARG VERSION=0.0.1-SNAPSHOT ARG GIT_COMMIT=unknown -ARG BUILD_DATE +ARG BUILD_DATE=unknowngateway/gateway-controller/default-policies/basic-ratelimit-v0.1.1.yaml (1)
102-156: Reminder: rebuild gateway images after gateway YAML changes.Per repo guidelines for gateway components, please rebuild with
cd gateway && make build-localto validate the updated default policy bundle.As per coding guidelines: “
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local”.gateway/gateway-controller/default-policies/advanced-ratelimit-v0.1.1.yaml (1)
368-372: Rebuild gateway images after this YAML change.Please run
cd gateway && make build-localto validate gateway artifacts for this update.As per coding guidelines, "gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local."
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/pkg/config/config.go (1)
357-385:control_plane_urlmaps to a non-existent config key.
ControlPlaneConfigexposeskoanf:"host"but the env mapping returnsgateway_controller.controlplane.url, soAPIP_GW_CONTROL_PLANE_URLwon’t populate the struct. This breaks env overrides for control plane host/URL.🔧 Proposed fix
- case "control_plane_url": - return "gateway_controller.controlplane.url" + case "control_plane_url": + return "gateway_controller.controlplane.host"- // For other GATEWAY_ prefixed vars, use standard mapping (underscore to dot) + // For other APIP_GW_ prefixed vars, use standard mapping (underscore to dot)
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/config/config.go`:
- Around line 352-355: The code attempts to Load a config file unconditionally
using k.Load(file.Provider(configPath), ...) even though the function comment
says "Load config file if path is provided"; add a guard before the k.Load call
to check if configPath == "" and skip calling k.Load when empty (i.e., only call
k.Load when configPath != ""), leaving the rest of the function unchanged;
reference k.Load and configPath to locate the change.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/config/config.go (1)
19-39: Remember to rebuild gateway Docker images after these gateway changes.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
| // Load config file if path is provided | ||
| if err := k.Load(file.Provider(configPath), yaml.Parser()); err != nil { | ||
| if err := k.Load(file.Provider(configPath), toml.Parser()); err != nil { | ||
| return nil, fmt.Errorf("failed to load config file: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check where LoadConfig is called
rg -n "LoadConfig\(" -g '*.go' | head -20Repository: wso2/api-platform
Length of output: 1009
🏁 Script executed:
# Also let's look at the actual LoadConfig function to understand its contract
rg -B5 -A20 "func.*LoadConfig" gateway/gateway-controller/pkg/config/config.goRepository: wso2/api-platform
Length of output: 977
🏁 Script executed:
# Let's also check how configPath is obtained/set in the callers
rg -B10 "LoadConfig\(" gateway/gateway-controller/pkg/config/config.goRepository: wso2/api-platform
Length of output: 551
🏁 Script executed:
cat -n gateway/gateway-controller/cmd/controller/main.go | head -70Repository: wso2/api-platform
Length of output: 3079
🏁 Script executed:
# Also check what happens with file.Provider with empty path - search for koanf documentation or error handling
rg -A5 "file\.Provider" gateway/gateway-controller/pkg/config/config.goRepository: wso2/api-platform
Length of output: 316
🏁 Script executed:
# Let's check if there's any validation of configPath elsewhere
rg -B3 -A3 "configPath" gateway/gateway-controller/cmd/controller/main.goRepository: wso2/api-platform
Length of output: 1009
🏁 Script executed:
# Check the kubernetes-gateway-operator call site
cat -n kubernetes/gateway-operator/cmd/main.go | head -100 | tail -30Repository: wso2/api-platform
Length of output: 1354
🏁 Script executed:
# Get more context around that LoadConfig call in kubernetes-gateway-operator
rg -B10 -A5 "LoadConfig" kubernetes/gateway-operator/cmd/main.goRepository: wso2/api-platform
Length of output: 621
🏁 Script executed:
# Let's find where configPath is defined in kubernetes-gateway-operator/cmd/main.go
rg -B5 "configPath" kubernetes/gateway-operator/cmd/main.go | head -40Repository: wso2/api-platform
Length of output: 675
🏁 Script executed:
# Also check the full context of the file
head -60 kubernetes/gateway-operator/cmd/main.goRepository: wso2/api-platform
Length of output: 1923
Add guard against empty configPath to match function intent.
The function's docstring states "Load config file if path is provided," but the code lacks a guard to skip file loading when configPath is empty. This mismatch could confuse future callers and allow silent failures when empty paths are passed. While current call sites prevent this, the function should be self-documenting. Add a check to skip file loading when configPath is empty, or update the docstring to clarify that a valid path is required.
Suggested fix
- if err := k.Load(file.Provider(configPath), toml.Parser()); err != nil {
- return nil, fmt.Errorf("failed to load config file: %w", err)
- }
+ if strings.TrimSpace(configPath) != "" {
+ if err := k.Load(file.Provider(configPath), toml.Parser()); err != nil {
+ return nil, fmt.Errorf("failed to load config file: %w", err)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Load config file if path is provided | |
| if err := k.Load(file.Provider(configPath), yaml.Parser()); err != nil { | |
| if err := k.Load(file.Provider(configPath), toml.Parser()); err != nil { | |
| return nil, fmt.Errorf("failed to load config file: %w", err) | |
| } | |
| // Load config file if path is provided | |
| if strings.TrimSpace(configPath) != "" { | |
| if err := k.Load(file.Provider(configPath), toml.Parser()); err != nil { | |
| return nil, fmt.Errorf("failed to load config file: %w", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/config/config.go` around lines 352 - 355, The
code attempts to Load a config file unconditionally using
k.Load(file.Provider(configPath), ...) even though the function comment says
"Load config file if path is provided"; add a guard before the k.Load call to
check if configPath == "" and skip calling k.Load when empty (i.e., only call
k.Load when configPath != ""), leaving the rest of the function unchanged;
reference k.Load and configPath to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml`:
- Around line 105-112: The TOML array-of-tables header is incorrectly placed
before the loop causing duplicate keys when multiple users exist; move the
`[[gateway_controller.auth.basic.users]]` header inside the `range` loop so each
iteration emits its own array-table entry: locate the block referencing
`$gc.auth.basic.users` and the `range $gc.auth.basic.users` loop and ensure
`[[gateway_controller.auth.basic.users]]` is emitted at the start of each loop
iteration (inside the `range`) so each user produces a separate TOML table with
`username`, `password`, and `roles`.
- Around line 167-178: The TOML array-of-tables header [[analytics.publishers]]
is emitted once before the range, which breaks when multiple publishers exist;
move the header inside the loop that iterates
.Values.gateway.config.analytics.publishers so each item starts with
[[analytics.publishers]] and then emits type and optional
[analytics.publishers.config] for that item (use the existing range and config
handling inside the loop), ensuring the header appears for every publisher
entry.
🧹 Nitpick comments (1)
kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml (1)
210-218: Use type-aware value rendering in the policy_configurations section to preserve TOML types.The template currently quotes all values with
{{ $v | quote }}, which converts policy configuration values (numbers, booleans, duration strings) to TOML string literals. This differs from the repository's TOML format, where values likemax_entries = 10000andinclude_x_rate_limit = trueare unquoted. While the system may handle string values acceptably through type-flexible processing, proper TOML type preservation ensures values are parsed with their intended types.♻️ Example using type checking
{{- range $k, $v := $value }} {{- if kindIs "bool" $v }} {{ $k }} = {{ $v }} {{- else if kindIs "float64" $v }} {{ $k }} = {{ $v }} {{- else if kindIs "int64" $v }} {{ $k }} = {{ $v }} {{- else }} {{ $k }} = {{ $v | quote }} {{- end }} {{- end }}
renuka-fernando
force-pushed
the
correlation
branch
from
3d233ac to
9128476
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kubernetes/helm/gateway-helm-chart/templates/gateway/controller/deployment.yaml (1)
73-95: Align XDS_SERVER_HOST naming with APIP_GW_ convention or move to config.toml.*The controller still reads
XDS_SERVER_HOST(ingateway/gateway-controller/pkg/xds/translator.go:1737), but it doesn't follow the newAPIP_GW_*naming pattern used elsewhere. Either rename it toAPIP_GW_XDS_SERVER_HOSTfor consistency or migrate it intoconfig.tomlalongside other configuration to avoid future drift.
🤖 Fix all issues with AI agents
In `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml`:
- Around line 210-216: The template currently always quotes policy configuration
values (in the block iterating .Values.gateway.config.policy_configurations
using {{ range $k, $v }} and emitting {{ $k }} = {{ $v | quote }}), which forces
booleans and numbers to strings; change that to emit unquoted literals for
booleans and numerics and only quote others by replacing the unconditional quote
with a conditional: detect booleans via regexMatch `^(true|false)$`
(case-insensitive if needed) and numerics via regexMatch
`^-?[0-9]+(\.[0-9]+)?$`, output {{ $v }} directly for those matches, otherwise
output {{ $v | quote }} so TOML preserves native number/boolean types. Ensure
you update the code inside the same policy_configurations range that references
$k and $v.
d360c64 to
febd5ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml`:
- Around line 54-59: The template treats $gc.router.access_logs.json_fields as
an array but values.yaml defines it as a map; update the gateway-config.yaml
template so json_fields is rendered as a TOML table (inline or standard) by
iterating over the map keys and values instead of using an index $i and quoting
the whole $field. Specifically, replace the current array-style block that
references $gc.router.access_logs.json_fields with logic that emits either an
inline table like json_fields = { key = "value", ... } or a multi-line table
with each key = "value", iterating over the map entries and quoting values
appropriately so the produced TOML is valid.
febd5ba to
332bc6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml`:
- Around line 210-219: The current template for
.Values.gateway.config.policy_configurations uses regexMatch to guess types and
only flattens two levels, which loses numeric strings and fails to render nested
maps/arrays as TOML tables/array-tables; replace the regex-based branch with a
recursive, type-aware partial (e.g., define a template partial renderToml or
renderPolicyValue) that uses Sprig type/kind checks (kindIs/typeOf) to: 1) emit
primitives (bool/number/string) correctly (strings quoted, numbers unquoted)
based on actual type rather than regex, 2) recurse into maps to emit section
headers like [policy_configurations.<parent>.<child>] and iterate keys, and 3)
handle slices/arrays by emitting TOML array primitives or array-of-tables for
slices of maps using [[policy_configurations.<parent>.<arrayKey>]] and recursing
into each element; then call this partial from the existing range over
.Values.gateway.config.policy_configurations instead of the current regexMatch
block so nested structures (keymanagers, memory, headers, allowedalgorithms,
etc.) render as proper TOML.
🧹 Nitpick comments (1)
kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml (1)
167-175: Handle type preservation for analytics publisher config values.Line 174 quotes all publisher config values, converting booleans and numbers to strings. This is inconsistent with how other TOML numeric/boolean values are handled elsewhere in the same template (e.g.,
buffer_size_bytes,als_server_port,als_plain_textare unquoted). If users need to pass numeric or boolean values to publisher configs, they should not be coerced to strings.♻️ Suggested fix
{{- range $key, $value := .config }} - {{ $key }} = {{ $value | quote }} + {{- if kindIs "string" $value }} + {{ $key }} = {{ $value | quote }} + {{- else }} + {{ $key }} = {{ $value }} + {{- end }} {{- end }}
renuka-fernando
force-pushed
the
correlation
branch
from
332bc6b to
7e87ce4
Compare
renuka-fernando
force-pushed
the
correlation
branch
from
bab55e9 to
00e48ff
Compare
3 participants
Purpose
Fix #634
Fix comments on #774
Summary by CodeRabbit
Configuration Updates
Documentation
Observability
Version Bump
✏️ Tip: You can customize this high-level summary in your review settings.