Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow prepending access log handler for WASM filter #35924

Closed
wants to merge 4 commits into from

Formatting error p2

b391719
Select commit
Loading
Failed to load commit list.
Closed

Allow prepending access log handler for WASM filter #35924

Formatting error p2
b391719
Select commit
Loading
Failed to load commit list.
CI (Envoy) / Mobile/Android skipped Aug 30, 2024 in 0s

Check was skipped

This check was not triggered in this CI run

Details

Request (pr/35924/main@b391719)

sunaydagli @sunaydagli b391719 #35924 merge main@a630749

Allow prepending access log handler for WASM filter

(Duplicate of #35910, mistakenly did not follow DCO and used wrong fixing command)

Commit Message: Allow prepending access log handler for WASM filter
Additional Description:
Risk Level: Low
Testing: Integration
Docs Changes:
Release Notes:
Platform Specific Features:
Runtime guard: envoy.reloadable_features.prepend_access_log_handler
Fixes #30859

This PR is in response to the approaches considered in ##35595 and #35742 in order to solve #30859. The recommended approach in these discussions was to override onStreamComplete callback, which may make sense for most filters. However, the WASM filter is a unique case in which doing this method would be more of a work-around hack as the filter itself does not inherit StreamFilter, and while the common WASM section does, it cannot utilize the onStreamComplete from its individual filters and thus is missing onStreamComplete, instead using onDone as a proxy for it. As a result, implementing onStreamComplete here would lead to onLog being called within the onStreamComplete method instead of the log() method, which doesn't seem to make the most sense for readability.

As such, we propose to continue the initial decided implementation of prepending the access loggers in order to maintain the execution order. This solution does seem to address this specific problem as described in the issue, so that AccessLogger doesn't see the dynamicMetadata until WASM finishes writing to it. This would be a case specific to WASM because of the details described above. Another WASM implementation is dependent on this issue being fixed, would welcome feedback or approval on this PR.

Environment

Request variables

Key Value
ref 6a7b331
sha b391719
pr 35924
base-sha a630749
actor sunaydagli @sunaydagli
message Allow prepending access log handler for WASM filter...
started 1725045156.05389
target-branch main
trusted false
Build image

Container image/s (as used in this CI run)

Key Value
default envoyproxy/envoy-build-ubuntu:f94a38f62220a2b017878b790b6ea98a0f6c5f9c
mobile envoyproxy/envoy-build-ubuntu:mobile-f94a38f62220a2b017878b790b6ea98a0f6c5f9c
Version

Envoy version (as used in this CI run)

Key Value
major 1
minor 32
patch 0
dev true