Migrate configuration format from YAML to TOML #775
Conversation
WalkthroughThe PR migrates configuration from YAML to TOML (adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
| [[policy_configurations.jwtauth_v010.keymanagers]] | ||
| name = "mock-jwks" | ||
| issuer = "http://mock-jwks:8080/token" | ||
|
|
||
| [policy_configurations.jwtauth_v010.keymanagers.jwks.remote] | ||
| uri = "http://mock-jwks:8080/jwks" | ||
|
|
||
| [[policy_configurations.jwtauth_v010.keymanagers]] | ||
| name = "wrong-issuer-km" | ||
| issuer = "http://expected-issuer.com" | ||
|
|
||
| [policy_configurations.jwtauth_v010.keymanagers.jwks.remote] | ||
| uri = "http://mock-jwks:8080/jwks" |
There was a problem hiding this comment.
TOML array syntax issue for nested configuration.
The second [[policy_configurations.jwtauth_v010.keymanagers]] entry (lines 189-194) has its jwks.remote section defined with the same path as the first entry. In TOML, this won't correctly associate the jwks.remote with the second array element. The section at line 193 will overwrite or conflict with the one at line 186.
For inline nested objects in array elements, use inline table syntax or restructure the configuration.
Proposed fix
[[policy_configurations.jwtauth_v010.keymanagers]]
name = "wrong-issuer-km"
issuer = "http://expected-issuer.com"
-
-[policy_configurations.jwtauth_v010.keymanagers.jwks.remote]
-uri = "http://mock-jwks:8080/jwks"
+jwks = { remote = { uri = "http://mock-jwks:8080/jwks" } }📝 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.
| [[policy_configurations.jwtauth_v010.keymanagers]] | |
| name = "mock-jwks" | |
| issuer = "http://mock-jwks:8080/token" | |
| [policy_configurations.jwtauth_v010.keymanagers.jwks.remote] | |
| uri = "http://mock-jwks:8080/jwks" | |
| [[policy_configurations.jwtauth_v010.keymanagers]] | |
| name = "wrong-issuer-km" | |
| issuer = "http://expected-issuer.com" | |
| [policy_configurations.jwtauth_v010.keymanagers.jwks.remote] | |
| uri = "http://mock-jwks:8080/jwks" | |
| [[policy_configurations.jwtauth_v010.keymanagers]] | |
| name = "mock-jwks" | |
| issuer = "http://mock-jwks:8080/token" | |
| [policy_configurations.jwtauth_v010.keymanagers.jwks.remote] | |
| uri = "http://mock-jwks:8080/jwks" | |
| [[policy_configurations.jwtauth_v010.keymanagers]] | |
| name = "wrong-issuer-km" | |
| issuer = "http://expected-issuer.com" | |
| jwks = { remote = { uri = "http://mock-jwks:8080/jwks" } } |
🤖 Prompt for AI Agents
In `@gateway/it/test-config.toml` around lines 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.
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.
| ```toml | ||
| [policy_engine] | ||
| service_name = "policy-engine-prod-us-east-1" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the policy_engine section in the config file
rg -n '^\[policy_engine\]' -A10 gateway/configs/config.tomlRepository: wso2/api-platform
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if the file exists
ls -la gateway/configs/config.toml 2>/dev/null || echo "File not found at that path"
# Search for policy_engine in all config-related files
rg -l 'policy_engine' gateway/configs/ 2>/dev/null || echo "No matches in gateway/configs"
# Search for policy_engine anywhere in the codebase
rg -l 'policy_engine' --type toml --type yaml --type yml 2>/dev/null | head -20
# Find all config.toml files
fd 'config.toml' gateway/Repository: wso2/api-platform
Length of output: 221
🏁 Script executed:
#!/bin/bash
# Search for policy_engine section in the config file
rg -n 'policy_engine' gateway/configs/config.toml -B2 -A10Repository: wso2/api-platform
Length of output: 1904
🏁 Script executed:
#!/bin/bash
# Search for service_name in the entire config file
rg -n 'service_name' gateway/configs/config.toml
# Check for a standalone [policy_engine] section (not subsections)
rg -n '^\[policy_engine\]$' gateway/configs/config.tomlRepository: wso2/api-platform
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Show the complete policy_engine sections from config file to understand actual structure
rg -n '^\[policy_engine' gateway/configs/config.toml -A3Repository: wso2/api-platform
Length of output: 728
Remove the invalid service_name example and replace with correct policy_engine configuration.
The service_name key does not exist under [policy_engine] in gateway/configs/config.toml. The actual configuration uses nested sections like [policy_engine.server], [policy_engine.admin], [policy_engine.xds], etc., with keys such as extproc_port, enabled, and server_address. Update the documentation to show valid policy_engine configuration.
🤖 Prompt for AI Agents
In `@docs/gateway/observability/tracing.md` around lines 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.
| // TracingConfig - Only fill defaults if the whole section wasn't configured | ||
| // We check Endpoint as a marker that the section exists | ||
| if cfg.TracingConfig.Endpoint == "" { | ||
| cfg.TracingConfig = defaults.TracingConfig | ||
| } else { | ||
| // Partially fill in defaults for unset fields within tracing | ||
| if cfg.TracingConfig.ServiceVersion == "" { | ||
| cfg.TracingConfig.ServiceVersion = defaults.TracingConfig.ServiceVersion | ||
| } | ||
| if cfg.TracingConfig.BatchTimeout == 0 { | ||
| cfg.TracingConfig.BatchTimeout = defaults.TracingConfig.BatchTimeout | ||
| } | ||
| if cfg.TracingConfig.MaxExportBatchSize == 0 { | ||
| cfg.TracingConfig.MaxExportBatchSize = defaults.TracingConfig.MaxExportBatchSize | ||
| } | ||
| if cfg.TracingConfig.SamplingRate == 0 { | ||
| cfg.TracingConfig.SamplingRate = defaults.TracingConfig.SamplingRate | ||
| } | ||
| } |
There was a problem hiding this comment.
TracingConfig defaults logic may silently override user intent.
If a user sets tracing.enabled = true without specifying endpoint, the entire section gets replaced with defaults (including Enabled: false). This could silently disable tracing when the user's intent was to enable it.
Consider checking cfg.TracingConfig.Enabled as a marker that the section was configured, or preserve the Enabled field before replacing:
♻️ Suggested fix
// TracingConfig - Only fill defaults if the whole section wasn't configured
- // We check Endpoint as a marker that the section exists
- if cfg.TracingConfig.Endpoint == "" {
+ // We check if the section was configured by examining multiple fields
+ tracingConfigured := cfg.TracingConfig.Endpoint != "" || cfg.TracingConfig.Enabled
+ if !tracingConfigured {
cfg.TracingConfig = defaults.TracingConfig
} else {
+ // Fill Endpoint default if not set but section is configured
+ if cfg.TracingConfig.Endpoint == "" {
+ cfg.TracingConfig.Endpoint = defaults.TracingConfig.Endpoint
+ }
// Partially fill in defaults for unset fields within tracing📝 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.
| // TracingConfig - Only fill defaults if the whole section wasn't configured | |
| // We check Endpoint as a marker that the section exists | |
| if cfg.TracingConfig.Endpoint == "" { | |
| cfg.TracingConfig = defaults.TracingConfig | |
| } else { | |
| // Partially fill in defaults for unset fields within tracing | |
| if cfg.TracingConfig.ServiceVersion == "" { | |
| cfg.TracingConfig.ServiceVersion = defaults.TracingConfig.ServiceVersion | |
| } | |
| if cfg.TracingConfig.BatchTimeout == 0 { | |
| cfg.TracingConfig.BatchTimeout = defaults.TracingConfig.BatchTimeout | |
| } | |
| if cfg.TracingConfig.MaxExportBatchSize == 0 { | |
| cfg.TracingConfig.MaxExportBatchSize = defaults.TracingConfig.MaxExportBatchSize | |
| } | |
| if cfg.TracingConfig.SamplingRate == 0 { | |
| cfg.TracingConfig.SamplingRate = defaults.TracingConfig.SamplingRate | |
| } | |
| } | |
| // TracingConfig - Only fill defaults if the whole section wasn't configured | |
| // We check if the section was configured by examining multiple fields | |
| tracingConfigured := cfg.TracingConfig.Endpoint != "" || cfg.TracingConfig.Enabled | |
| if !tracingConfigured { | |
| cfg.TracingConfig = defaults.TracingConfig | |
| } else { | |
| // Fill Endpoint default if not set but section is configured | |
| if cfg.TracingConfig.Endpoint == "" { | |
| cfg.TracingConfig.Endpoint = defaults.TracingConfig.Endpoint | |
| } | |
| // Partially fill in defaults for unset fields within tracing | |
| if cfg.TracingConfig.ServiceVersion == "" { | |
| cfg.TracingConfig.ServiceVersion = defaults.TracingConfig.ServiceVersion | |
| } | |
| if cfg.TracingConfig.BatchTimeout == 0 { | |
| cfg.TracingConfig.BatchTimeout = defaults.TracingConfig.BatchTimeout | |
| } | |
| if cfg.TracingConfig.MaxExportBatchSize == 0 { | |
| cfg.TracingConfig.MaxExportBatchSize = defaults.TracingConfig.MaxExportBatchSize | |
| } | |
| if cfg.TracingConfig.SamplingRate == 0 { | |
| cfg.TracingConfig.SamplingRate = defaults.TracingConfig.SamplingRate | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@gateway/policy-engine/internal/config/config.go` around lines 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.
| 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
2 participants
Purpose
Fix comments on #774
Summary by CodeRabbit
Documentation
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.