golang: add OnStreamComplete callback to mutate final metadata

[spacewander]

As discussed from #35595 (comment), the AccessLogHandler::log is not designed for mutating the StreamInfo. It's recommended to use OnStreamComplete to do the final metadata management.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #35742 was opened by spacewander.

see: more, trace.

[spacewander]

CC @doujiang24

[sunaydagli]

I wanted to confirm whether or not this fixes #30859 as AccessLogger will access dynamicMetadata after WASM writes to it?

[spacewander]

I wanted to confirm whether or not this fixes #30859 as AccessLogger will access dynamicMetadata after WASM writes to it?

No, this change is only about the Golang filter. If we want to support this in Wasm, we need to add OnStreamComplete callback in Wasm too.

[adisuissa]

Assigning @doujiang24 as codeowner
/assign @doujiang24

[adisuissa]

/assign-from @envoyproxy/envoy-maintainers

[repokitteh-read-only]

@envoyproxy/envoy-maintainers assignee is @RyanTheOptimist

🐱

Caused by: a #35742 (comment) was created by @adisuissa.

see: more, trace.

[doujiang24]

Nice work, lgtm~

[sunaydagli]

I wanted to confirm whether or not this fixes #30859 as AccessLogger will access dynamicMetadata after WASM writes to it?

No, this change is only about the Golang filter. If we want to support this in Wasm, we need to add OnStreamComplete callback in Wasm too.

@spacewander I am unsure if the onStreamComplete callback is already added in Wasm, as per envoy/http/filter.h and envoy/source/common/http/filter_manager.h. In this case, would moving the onStreamComplete call to after the destroyFilters() (which uses onDestroy) call in envoy/source/common/http/conn_manager_impl.cc provide the desired change? Thanks for the help!

[spacewander]

I wanted to confirm whether or not this fixes #30859 as AccessLogger will access dynamicMetadata after WASM writes to it?

No, this change is only about the Golang filter. If we want to support this in Wasm, we need to add OnStreamComplete callback in Wasm too.

@spacewander I am unsure if the onStreamComplete callback is already added in Wasm, as per envoy/http/filter.h and envoy/source/common/http/filter_manager.h. In this case, would moving the onStreamComplete call to after the destroyFilters() (which uses onDestroy) call in envoy/source/common/http/conn_manager_impl.cc provide the desired change? Thanks for the help!

No, the wasm filter doesn't override the onStreamComplete so it's empty for Wasm.
Another idea is that you might create a wasm access logger before the real logger: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/access_loggers/wasm/v3/wasm.proto. I haven't tested it yet.