Add Access Logging support#2297
Draft
DDH13 wants to merge 3 commits into
Draft
Conversation
DDH13
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds analytics header and payload capture settings to configuration and policy schemas, propagates those settings into system policies, and updates analytics runtime handling to serialize headers, truncate payloads, and skip ignored paths. It also adds a log analytics publisher that writes JSON events to stdout with configurable header masking, plus related tests. The main config also adds new JWT and opaque token policy configuration blocks. Sequence Diagram(s)sequenceDiagram
participant ControllerConfig
participant SystemPolicies
participant AnalyticsPolicy
participant RuntimeAnalytics
ControllerConfig->>SystemPolicies: inject send_request_headers, send_response_headers, max_payload_size
SystemPolicies->>AnalyticsPolicy: set request_headers and response_headers metadata
AnalyticsPolicy->>RuntimeAnalytics: process analytics event
RuntimeAnalytics->>RuntimeAnalytics: skip ignored paths or continue capture
sequenceDiagram
participant AnalyticsRuntime
participant LogPublisher
participant Stdout
AnalyticsRuntime->>LogPublisher: register log publisher
LogPublisher->>LogPublisher: mask configured headers
LogPublisher->>Stdout: write JSON line
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
gateway/system-policies/analytics/analytics_headers_test.go (1)
10-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct policy-method coverage for header metadata.
These tests validate the helpers, but they do not exercise
OnRequestHeadersorOnResponseHeaders. A small request-phase and response-phase test that assertsAnalyticsMetadatacontainsrequest_headers/response_headerswhen the params are enabled would catch key-name or wiring regressions in the new behavior.🤖 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/system-policies/analytics/analytics_headers_test.go` around lines 10 - 59, Add direct coverage for the policy entry points, since the current tests only hit getHeaderFlags and serializeHeaders. Add small tests around OnRequestHeaders and OnResponseHeaders that enable send_request_headers/send_response_headers, then assert the resulting AnalyticsMetadata contains request_headers and response_headers with the expected serialized values. Use the existing helpers and policy type names to locate the wiring, and make sure the assertions catch key-name or metadata-plumbing regressions.
🤖 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/utils/system_policies.go`:
- Around line 202-208: The analytics policy defaults in system_policies.go are
missing the legacy allow_payloads compatibility path, so older configs no longer
map correctly through the effectiveDefaults/mergeParameters flow. Update the
analytics branch in the policy-building logic to translate
cfg.Analytics.AllowPayloads into the body-related defaults before
mergeParameters runs, or inject the legacy allow_payloads parameter there so it
still controls send_request_body/send_response_body alongside the new header
flags. Keep the fix scoped to the analytics policy handling in the code path
that builds effectiveDefaults for sysPol.Name.
---
Nitpick comments:
In `@gateway/system-policies/analytics/analytics_headers_test.go`:
- Around line 10-59: Add direct coverage for the policy entry points, since the
current tests only hit getHeaderFlags and serializeHeaders. Add small tests
around OnRequestHeaders and OnResponseHeaders that enable
send_request_headers/send_response_headers, then assert the resulting
AnalyticsMetadata contains request_headers and response_headers with the
expected serialized values. Use the existing helpers and policy type names to
locate the wiring, and make sure the assertions catch key-name or
metadata-plumbing regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba9cbd48-6ce2-43f1-82db-aa5a9bec3263
📒 Files selected for processing (12)
gateway/configs/config-template.tomlgateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/utils/system_policies.gogateway/gateway-controller/pkg/utils/system_policies_test.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/log.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/log_test.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/system-policies/analytics/analytics.gogateway/system-policies/analytics/analytics_headers_test.gogateway/system-policies/analytics/policy-definition.yaml
Comment on lines
+202
to
+208
| // For the analytics system policy, propagate the payload and header | ||
| // capture flags from runtime config. | ||
| if sysPol.Name == constants.ANALYTICS_SYSTEM_POLICY_NAME { | ||
| effectiveDefaults["send_request_body"] = cfg.Analytics.SendRequestBody | ||
| effectiveDefaults["send_response_body"] = cfg.Analytics.SendResponseBody | ||
| effectiveDefaults["send_request_headers"] = cfg.Analytics.SendRequestHeaders | ||
| effectiveDefaults["send_response_headers"] = cfg.Analytics.SendResponseHeaders |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the documented allow_payloads fallback here.
This block now propagates the new header flags, but it still never carries cfg.Analytics.AllowPayloads. That breaks the deprecated compatibility path described in config/schema because the analytics policy only sees the directional body flags from this params map. Please translate AllowPayloads into the body flags here (or inject the legacy parameter before mergeParameters) so older configs keep the same behavior.
Suggested fix
// For the analytics system policy, propagate the payload and header
// capture flags from runtime config.
if sysPol.Name == constants.ANALYTICS_SYSTEM_POLICY_NAME {
- effectiveDefaults["send_request_body"] = cfg.Analytics.SendRequestBody
- effectiveDefaults["send_response_body"] = cfg.Analytics.SendResponseBody
+ if cfg.Analytics.AllowPayloads && !cfg.Analytics.SendRequestBody && !cfg.Analytics.SendResponseBody {
+ effectiveDefaults["send_request_body"] = true
+ effectiveDefaults["send_response_body"] = true
+ } else {
+ effectiveDefaults["send_request_body"] = cfg.Analytics.SendRequestBody
+ effectiveDefaults["send_response_body"] = cfg.Analytics.SendResponseBody
+ }
+ effectiveDefaults["allow_payloads"] = cfg.Analytics.AllowPayloads
effectiveDefaults["send_request_headers"] = cfg.Analytics.SendRequestHeaders
effectiveDefaults["send_response_headers"] = cfg.Analytics.SendResponseHeaders
}Based on learnings, gateway/gateway-controller/pkg/utils/system_policies.go should keep allow_payloads runtime-controlled in effectiveDefaults before mergeParameters.
📝 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.
Suggested change
| // For the analytics system policy, propagate the payload and header | |
| // capture flags from runtime config. | |
| if sysPol.Name == constants.ANALYTICS_SYSTEM_POLICY_NAME { | |
| effectiveDefaults["send_request_body"] = cfg.Analytics.SendRequestBody | |
| effectiveDefaults["send_response_body"] = cfg.Analytics.SendResponseBody | |
| effectiveDefaults["send_request_headers"] = cfg.Analytics.SendRequestHeaders | |
| effectiveDefaults["send_response_headers"] = cfg.Analytics.SendResponseHeaders | |
| // For the analytics system policy, propagate the payload and header | |
| // capture flags from runtime config. | |
| if sysPol.Name == constants.ANALYTICS_SYSTEM_POLICY_NAME { | |
| if cfg.Analytics.AllowPayloads && !cfg.Analytics.SendRequestBody && !cfg.Analytics.SendResponseBody { | |
| effectiveDefaults["send_request_body"] = true | |
| effectiveDefaults["send_response_body"] = true | |
| } else { | |
| effectiveDefaults["send_request_body"] = cfg.Analytics.SendRequestBody | |
| effectiveDefaults["send_response_body"] = cfg.Analytics.SendResponseBody | |
| } | |
| effectiveDefaults["allow_payloads"] = cfg.Analytics.AllowPayloads | |
| effectiveDefaults["send_request_headers"] = cfg.Analytics.SendRequestHeaders | |
| effectiveDefaults["send_response_headers"] = cfg.Analytics.SendResponseHeaders | |
| } |
🤖 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/utils/system_policies.go` around lines 202 -
208, The analytics policy defaults in system_policies.go are missing the legacy
allow_payloads compatibility path, so older configs no longer map correctly
through the effectiveDefaults/mergeParameters flow. Update the analytics branch
in the policy-building logic to translate cfg.Analytics.AllowPayloads into the
body-related defaults before mergeParameters runs, or inject the legacy
allow_payloads parameter there so it still controls
send_request_body/send_response_body alongside the new header flags. Keep the
fix scoped to the analytics policy handling in the code path that builds
effectiveDefaults for sysPol.Name.
Source: Learnings
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/configs/config.toml`:
- Around line 78-108: The policy_configurations.backendjwt_v1.signingkey inline
private key is checked in and should be removed from the shared config. Replace
the inline secret with a non-sensitive placeholder or file reference, and make
sure the real key is loaded from deployment-specific secret storage instead of
being embedded in config.toml.
- Around line 179-181: The introspection config is checking in real provider
credentials instead of placeholders. Update the relevant entries in the config
section containing uri, clientId, and clientSecret to use non-sensitive
placeholder values or external secret references, and keep the concrete
credentials out of the repository. Make sure the same config keys remain in
place so the introspection setup still works with injected secrets.
- Around line 157-176: The opaque token introspection providers use inconsistent
token pattern key names, so make both entries in
opaquetokenauth_v1.introspectionproviders use the same exact key as the existing
local_app configuration. Update the asgardeo_app provider’s token pattern field
to match tokenPattern so the matcher is recognized consistently by the config
parser.
In `@gateway/gateway-runtime/policy-engine/internal/config/config.go`:
- Around line 66-69: The runtime AnalyticsConfig schema is missing the new
analytics settings, so add send_request_headers, send_response_headers, and
max_payload_size alongside IgnoredPathPrefixes in the config struct used by
koanf. Update AnalyticsConfig and any related config mapping so
file-config/runtime-only loading preserves the same header capture and payload
truncation fields exposed by config-template.toml, config.toml, and the
controller config.
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 496-498: The streaming analytics path still buffers the full
response in analyticsStreamAccKey even when max_payload_size is set, so update
the accumulation logic in analytics.go to cap memory use during stream
collection. Adjust the response-body handling in the streaming aggregation path
that feeds accumulateResponsePayload/getPayloadFlags so it keeps only a bounded
buffer (or stops appending once the configured limit is reached), while still
allowing response_payload to be truncated for emission.
- Around line 977-999: The getMaxPayloadSize helper currently returns negative
values unchanged even though its contract says non-positive values should be
treated as 0. Update getMaxPayloadSize in analytics.go so every parsed branch
(int, int64, float64, string) normalizes any value <= 0 to 0 before returning,
and keep the fallback behavior for unset or invalid inputs unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4d929f1-7abf-49a6-acbf-cf8af0edd27b
📒 Files selected for processing (13)
gateway/configs/config-template.tomlgateway/configs/config.tomlgateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/utils/system_policies.gogateway/gateway-controller/pkg/utils/system_policies_test.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/log.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/log_test.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/system-policies/analytics/analytics.gogateway/system-policies/analytics/analytics_headers_test.gogateway/system-policies/analytics/policy-definition.yaml
💤 Files with no reviewable changes (1)
- gateway/gateway-runtime/policy-engine/internal/analytics/publishers/log_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- gateway/gateway-controller/pkg/utils/system_policies.go
- gateway/system-policies/analytics/policy-definition.yaml
- gateway/gateway-controller/pkg/config/config.go
Comment on lines
+78
to
+108
| [policy_configurations.backendjwt_v1.signingkey] | ||
| inline = """ | ||
| -----BEGIN PRIVATE KEY----- | ||
| MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCpilDV57t6zg7H | ||
| fJKOVdhLKR3DHpDO0q0+chermJLdNjH1O2mq6gPsWIWwaDp2G1pM+0Iu1vAOV+5r | ||
| YEJc3S2MyJlws9WQTCTChDtC8d4dwL6JuWG5lxMV2tpJfjVaKhx7aBHBRLZdRhPb | ||
| JKu08PjrwDbMdVDlM5BwWGMeaPelbSe5KBNExjEILX/GPLJXH0lLPDDLQCuYvHQ4 | ||
| 3aIRG53AwEqbr5MbOGQ9qFiLmwOgHYE0ppE2uXFj+s4ehSsJLpIcQSE/kcVNqu/i | ||
| 9Pw5VyPc0PBh5S95wO9jltt+n5nj2p5W156XWCbIOb64dbM/G7Sdui3OFlbUnEgf | ||
| lv++ylpZAgMBAAECggEADc52wMSrTtfi+jnZ0X+KKqzBgLdHgaBEfXZbGR1GsH8O | ||
| WHMrKsD96sxWrsP+JsoZc3VusVPNns1oXBQts1RLtlBLtpLejG62+6Q7EzuRJAkj | ||
| OyH+pFd6NWhqKFv/ykTtptsVvxBybWwch9cXqJeLHd+8VrAjE1c8pvl7sOlJaY40 | ||
| 4AIAo119jRrvBDKZb9zNoBMFAjHBLUciqV7FWnECP2Z20HzWE6CvQ3Glp9KXa/Vd | ||
| 3l74XeugRtkHzZJDKrQOsT1tGTBNcUlG1BDxmykpNQGtYzgmQURmAOLmXpMUAMbV | ||
| 3aJlDm51CP13QzhqrPGhcz0TICb8JZ/hI6lGmvpxRQKBgQDsLaPFf/9yCpf/oogx | ||
| JG2k66wYhA/83v7zrq2SfO+RWq+tOV54xoUiGc7UJVmgcxPF4YXj3ciqhOaWt1Sh | ||
| 47OUWVWLpnPjwaFjCc0NirvzPjYZcqOgpOyg8/bpQEyr9+mIB4nDToe/PaaLk/fN | ||
| IKuDxZXDdQM4KEg410rmcbEzkwKBgQC3xPBwPUNqlocOf9H5VVCIjuxhITWuUPcd | ||
| CC0l69aiUvCcSHq2DrpQkmr/E5is+IY25n11OzOJmEPRAzkiZxasWNwG3eHYEbwg | ||
| PAbmF0gLoQI80k5j7cnuumj2usbizN3q4IkonZhbSculFEtYvidbox0PGpF/ohww | ||
| f2Q2pRFF4wKBgHurZl9Lp+oCbBca92+sIJHEKkhoFqsV5bCaHh9ftl6JewYczUA2 | ||
| y43Qr8HckPL4bzkl32MzRhksIEZ5VyKyRd1/bdEDZxmy0Zw3jJzzsXkscU2f/thm | ||
| o9WgCgGXfs5psWpecB/J5kEYujXKVG6gFc+ZTdOcWQQ166N/8KN8kwprAoGAO1QD | ||
| Y9kDeNfcBlxRx7M5arzpp3n3QK22+dJ38PQwuPsZ3vqvUzNyBy0eCfejpMQfYH4N | ||
| JtnUC6nyyvnFakvpz/5oGndrwDwJhkaa25HMmqSCemxTDoPoW4Am/vDR4MrPIlMX | ||
| NNucImyyqDrjEGVjg3wnl4a2ToRhoW6fRd0ItI0CgYBV5Xulb5e44HCQvznKhloe | ||
| Kie6EvbJrDion3udphv2gY69G+fh4eIJr7vtQZ+MP7Oyl0I20y6LeEvSKoGvg0AA | ||
| knCmk/s1op+H/gojHTKLFVRd3Q2llOpX1VCiPD80PTvmcKR3ZWbUJGUfD5TXNyWc | ||
| wbJ3Ujx/o5t1QEzMNZWLZg== | ||
| -----END PRIVATE KEY----- | ||
| """ |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Replace the inline signing key with a non-sensitive reference.
A checked-in private key is not a safe default for a shared config file. Keep only a path or placeholder here and load the real key from deployment-specific secret storage.
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 80-107: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 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/configs/config.toml` around lines 78 - 108, The
policy_configurations.backendjwt_v1.signingkey inline private key is checked in
and should be removed from the shared config. Replace the inline secret with a
non-sensitive placeholder or file reference, and make sure the real key is
loaded from deployment-specific secret storage instead of being embedded in
config.toml.
Source: Linters/SAST tools
Comment on lines
+157
to
+176
| [[policy_configurations.opaquetokenauth_v1.introspectionproviders]] | ||
| name = "local_app" | ||
| issuer = "http://host.docker.internal:3000/oauth2/token" | ||
| tokenPattern = "opaque.*" | ||
|
|
||
| [policy_configurations.opaquetokenauth_v1.introspectionproviders.introspection] | ||
| uri = "http://host.docker.internal:3000/oauth2/introspect" | ||
| # Client credentials the gateway uses to authenticate to the introspection | ||
| # endpoint (RFC 7662 requires the protected resource to authenticate). Replace | ||
| # with a client authorized to introspect tokens on this Asgardeo tenant. | ||
| clientId = "test-client" | ||
| clientSecret = "test-secret" | ||
| authStyle = "basic" | ||
| tokenTypeHint = "access_token" | ||
| skipTlsVerify = false | ||
|
|
||
| [[policy_configurations.opaquetokenauth_v1.introspectionproviders]] | ||
| name = "asgardeo_app" | ||
| issuer = "https://api.asgardeo.io/t/dinethwso2/oauth2/token" | ||
| tokenpattern = "fd" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one token pattern key name in both providers.
The first provider uses tokenPattern, but the second uses tokenpattern. In configuration maps that casing difference changes the key name, so the second provider can miss its matcher.
🤖 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/configs/config.toml` around lines 157 - 176, The opaque token
introspection providers use inconsistent token pattern key names, so make both
entries in opaquetokenauth_v1.introspectionproviders use the same exact key as
the existing local_app configuration. Update the asgardeo_app provider’s token
pattern field to match tokenPattern so the matcher is recognized consistently by
the config parser.
Comment on lines
+179
to
+181
| uri = "https://api.asgardeo.io/t/dinethwso2/oauth2/introspect" | ||
| clientId = "PfHXs0tbGmfp7lvjyrnO6o8ViCIa" | ||
| clientSecret = "NsvBsvF9FfcTvpolXLjYJtW_FYzVnFhVBA7QAeIdr_ka" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Replace the hard-coded introspection credentials.
This config now includes concrete provider credentials. Use placeholders or external secret references instead of checked-in values.
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 181-181: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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/configs/config.toml` around lines 179 - 181, The introspection config
is checking in real provider credentials instead of placeholders. Update the
relevant entries in the config section containing uri, clientId, and
clientSecret to use non-sensitive placeholder values or external secret
references, and keep the concrete credentials out of the repository. Make sure
the same config keys remain in place so the introspection setup still works with
injected secrets.
Source: Linters/SAST tools
Comment on lines
+66
to
+69
| // IgnoredPathPrefixes suppresses analytics events (and therefore all | ||
| // publisher output, including the stdout/log publisher) for requests whose | ||
| // original path starts with any of these prefixes, e.g. health-check probes. | ||
| IgnoredPathPrefixes []string `koanf:"ignored_path_prefixes"` |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the missing runtime analytics fields to the schema.
config-template.toml, config.toml, and the controller config now expose send_request_headers, send_response_headers, and max_payload_size, but the runtime AnalyticsConfig only adds ignored_path_prefixes. In file-config/runtime-only flows those settings will be dropped, so header capture and payload truncation will not stay aligned across layers.
Suggested change
type AnalyticsConfig struct {
Enabled bool `koanf:"enabled"`
EnabledPublishers []string `koanf:"enabled_publishers"`
Publishers AnalyticsPublishersConfig `koanf:"publishers"`
GRPCEventServerCfg map[string]interface{} `koanf:"grpc_event_server"`
AccessLogsServiceCfg AccessLogsServiceConfig `koanf:"access_logs_service"`
AllowPayloads bool `koanf:"allow_payloads"`
SendRequestBody bool `koanf:"send_request_body"`
SendResponseBody bool `koanf:"send_response_body"`
+ SendRequestHeaders bool `koanf:"send_request_headers"`
+ SendResponseHeaders bool `koanf:"send_response_headers"`
+ MaxPayloadSize int `koanf:"max_payload_size"`
IgnoredPathPrefixes []string `koanf:"ignored_path_prefixes"`
} AllowPayloads: false,
SendRequestBody: false,
SendResponseBody: false,
+ SendRequestHeaders: false,
+ SendResponseHeaders: false,
+ MaxPayloadSize: 0,
IgnoredPathPrefixes: []string{},Also applies to: 398-401
🤖 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-runtime/policy-engine/internal/config/config.go` around lines
66 - 69, The runtime AnalyticsConfig schema is missing the new analytics
settings, so add send_request_headers, send_response_headers, and
max_payload_size alongside IgnoredPathPrefixes in the config struct used by
koanf. Update AnalyticsConfig and any related config mapping so
file-config/runtime-only loading preserves the same header capture and payload
truncation fields exposed by config-template.toml, config.toml, and the
controller config.
Comment on lines
+977
to
+999
| // getMaxPayloadSize reads the max_payload_size policy parameter (bytes). It | ||
| // returns 0 when unset or non-positive, meaning "no limit". | ||
| func getMaxPayloadSize(params map[string]interface{}) int { | ||
| if params == nil { | ||
| return 0 | ||
| } | ||
| raw, ok := params["max_payload_size"] | ||
| if !ok { | ||
| return 0 | ||
| } | ||
| switch v := raw.(type) { | ||
| case int: | ||
| return v | ||
| case int64: | ||
| return int(v) | ||
| case float64: | ||
| return int(v) | ||
| case string: | ||
| if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil { | ||
| return n | ||
| } | ||
| } | ||
| return 0 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize non-positive max_payload_size values to 0.
The comment says this helper returns 0 for unset or non-positive values, but negative numbers are currently returned unchanged. Current callers happen to treat <=0 as unlimited, yet the helper contract is now inconsistent and untested for negative inputs.
Proposed fix
func getMaxPayloadSize(params map[string]interface{}) int {
if params == nil {
return 0
}
raw, ok := params["max_payload_size"]
if !ok {
return 0
}
+ normalize := func(n int) int {
+ if n <= 0 {
+ return 0
+ }
+ return n
+ }
switch v := raw.(type) {
case int:
- return v
+ return normalize(v)
case int64:
- return int(v)
+ return normalize(int(v))
case float64:
- return int(v)
+ return normalize(int(v))
case string:
if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil {
- return n
+ return normalize(n)
}
}
return 0
}📝 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.
Suggested change
| // getMaxPayloadSize reads the max_payload_size policy parameter (bytes). It | |
| // returns 0 when unset or non-positive, meaning "no limit". | |
| func getMaxPayloadSize(params map[string]interface{}) int { | |
| if params == nil { | |
| return 0 | |
| } | |
| raw, ok := params["max_payload_size"] | |
| if !ok { | |
| return 0 | |
| } | |
| switch v := raw.(type) { | |
| case int: | |
| return v | |
| case int64: | |
| return int(v) | |
| case float64: | |
| return int(v) | |
| case string: | |
| if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil { | |
| return n | |
| } | |
| } | |
| return 0 | |
| // getMaxPayloadSize reads the max_payload_size policy parameter (bytes). It | |
| // returns 0 when unset or non-positive, meaning "no limit". | |
| func getMaxPayloadSize(params map[string]interface{}) int { | |
| if params == nil { | |
| return 0 | |
| } | |
| raw, ok := params["max_payload_size"] | |
| if !ok { | |
| return 0 | |
| } | |
| normalize := func(n int) int { | |
| if n <= 0 { | |
| return 0 | |
| } | |
| return n | |
| } | |
| switch v := raw.(type) { | |
| case int: | |
| return normalize(v) | |
| case int64: | |
| return normalize(int(v)) | |
| case float64: | |
| return normalize(int(v)) | |
| case string: | |
| if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil { | |
| return normalize(n) | |
| } | |
| } | |
| return 0 | |
| } |
🤖 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/system-policies/analytics/analytics.go` around lines 977 - 999, The
getMaxPayloadSize helper currently returns negative values unchanged even though
its contract says non-positive values should be treated as 0. Update
getMaxPayloadSize in analytics.go so every parsed branch (int, int64, float64,
string) normalizes any value <= 0 to 0 before returning, and keep the fallback
behavior for unset or invalid inputs unchanged.
Contributor
Author
|
@coderabbitai pause |
Contributor
✅ Action performedReviews paused. |
DDH13
force-pushed
the
main.traffic-logging
branch
from
6469102 to
18ab16b
Compare
DDH13
changed the title
DDH13
force-pushed
the
main.traffic-logging
branch
from
21e9396 to
1b52ca4
Compare
- Introduced configuration options for enabling header capture in analytics events. - Added a new Log publisher to output analytics events to stdout. - Updated analytics system policy to handle request and response headers. - Enhanced tests to validate header capture functionality. Enhance analytics configuration with max payload size and ignored path prefixes Remove pretty Refactor analytics and traffic logging configuration to use a unified collector - Migrate analytics configuration to a new collector structure, consolidating settings for analytics and traffic logging. - Update tests to reflect changes in configuration structure and ensure compatibility with the new collector. - Deprecate old analytics settings and provide migration paths for existing configurations. - Adjust access log service server initialization to utilize the new collector settings. - Ensure that both analytics and traffic logging consumers require the collector to be enabled for proper functionality. Refactor access log configuration to unify ALS settings under [collector.als] - Updated CollectorConfig and AnalyticsConfig to replace deprecated grpc_event_server and access_logs_service fields with als. - Adjusted validation and migration logic to reflect new configuration structure. - Modified error messages to reference the new [collector.als] section. - Enhanced traffic logging functionality by introducing TrafficLog and TrafficLogDirective types, allowing for per-API logging configurations. - Implemented tests to ensure correct behavior of traffic logging, including handling of excluded headers and field selection. - Updated access logger server initialization to utilize new configuration keys. Add auto activate for collector Refactor traffic logging configuration to support max payload size and ignored path prefixes Refactor latency calculations and enhance Latencies struct with new duration fields
- Updated the collector configuration to remove the explicit `enabled` flag, making the collector implicitly active whenever a consumer (analytics or traffic logging) is enabled. - Adjusted related comments and documentation to reflect the new behavior. - Modified tests to ensure they validate the collector's state based on consumer configurations rather than an explicit flag. - Removed deprecated path ignoring functionality from traffic logging configuration. - Enhanced logging to include custom properties in traffic log events.
DDH13
force-pushed
the
main.traffic-logging
branch
from
c5fc51f to
ba5e6f0
Compare
…ed headers and caching traffic log directives
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.
Related to #2222
This pull request introduces a major refactor of the configuration structure for analytics and logging in the gateway. The primary change is the introduction of a new
[collector]section, which centralizes all data-capture settings previously scattered under[analytics]and related sections. This makes the configuration clearer, enforces correct dependencies between analytics/logging consumers and the underlying capture pipeline, and provides a migration path for deprecated settings. Backward compatibility is maintained via migration logic and deprecation warnings. The ALS (Access Log Service) transport tuning is also moved under[collector].Key changes include:
Configuration Structure Refactor:
Introduced a new
[collector]section in config files (config.toml,config-template.toml) to centralize all data collection settings, including payload/header capture, path exclusion, and ALS transport tuning. Analytics and traffic logging are now explicit consumers of the collector, and enabling them requires[collector].enabled = true. [1]], [2]], [3]])Updated the
Configstruct in Go to add aCollectorConfigtype, which holds all collector-related settings, and deprecated direct body/header capture flags underAnalyticsConfig. ALS transport config is now underCollectorConfig. [1]], [2]])Migration and Validation Logic:
Added logic in Go to migrate deprecated
[analytics]settings (body/header capture and ALS transport) onto the new[collector]fields during validation, with appropriate warnings. This ensures backward compatibility for existing configs. [1]], [2]])Enforced that analytics and traffic logging consumers cannot be enabled unless the collector is enabled, preventing misconfiguration. ([gateway/gateway-controller/pkg/config/config.goL1700-R1840])
Defaults and Test Updates:
Documentation and Deprecation:
These changes significantly improve the clarity, maintainability, and correctness of analytics and logging configuration in the gateway.