Allow prepending access log handler for WASM filter #35910
Closed
Conversation
This file contains 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
sunaydagli
requested review from
wbpcode,
tyxia,
mattklein123 and
yanavlasov
as code owners
|
Hi @sunaydagli, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
Hi, thanks for the contribution and please check my previous comment #35595 (comment) |
|
@sunaydagli |
fix #32737 --------- Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: sunaydagli <[email protected]>
This adds testing of doing retries post HTTP/3 handshake, and resetting brokenness on networ kchange. Risk Level: n/a (adds a stat to tracking) Testing: yes Docs Changes: no Release Notes: no --------- Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: sunaydagli <[email protected]>
using the CI diskspace hack Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: sunaydagli <[email protected]>
Commit Message: dynamic_modules: adds initial object loading logic Additional Description: This is the very first commit of the dynamic loading feature discussed among community members. This is the effort to upstream the playground repository https://github.com/mathetake/envoy-dynamic-modules as an Envoy core extension. Series of commits will follow this little by little. #2053, #24230, #32502 Risk Level: N/A (not compiled into the final build yet) Testing: unit Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Takeshi Yoneda <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
…35618) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
…#35571) Commit Message: [cache filter] Bug fix, abort various functions on filter destruction Additional Description: While testing the aborted thundering herd mitigation PR, one thing that was revealed was several possible situations where the filter onDestroy could be called (most commonly by downstream disconnect) and then the filter could be "resurrected" by a change of state back to a state other than destroyed, if the destruction had occurred during a callback. This could lead to a variety of issues, including potentially crash bugs, if the non-destroyed state leads into doing something using member variables that were invalidated during onDestroy. Risk Level: Very small; change is to a WIP filter, and only impacts behavior when the filter state is 'destroyed'. Testing: Yes. Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a --------- Signed-off-by: Raven Black <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: sunaydagli <[email protected]>
…#35601) Risk Level: low Testing: unit tests Release Notes: inline Fixes #35372 Signed-off-by: Ali Beyad <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Similar to the `getaddrinfo` implementation, this PR adds the c-ares human readable status in the DNS response details. Risk Level: low Testing: unit test Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a --------- Signed-off-by: Fredy Wijaya <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: sunaydagli <[email protected]>
…p async exporter (#35572) only the response code is used for logging purpose. Risk Level: low Testing: unit test Signed-off-by: Boteng Yao <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Fix #35736 Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
…ild rule (#35870) This can be useful for passing properties, such as `tags`, etc, for example tests that need to use external connection, IPv4/IPv6, etc. Risk Level: low (tests only) Testing: CI Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Fredy Wijaya <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Commit Message: update the `approximate_monotonic_time_` after polling instead of before polling. The Envoy FileEventImpl doesn't timeout the polling when there is no I/O. Updating `approximate_monotonic_time_` before polling could cause it to be lagged behind for a substantial period. (`approximateMonotonicTime()` is only used by `EnvoyQuicClock`.) This is done by registering a new `LibeventScheduler` callback with `evwatch_check_new()` to do the update right before LibeventScheduler::onCheckForStats(). Additional Description: fix existing potential TSAN failure in a few integration tests. Risk Level: low, it should at most made no difference if not improving QUIC performance. Testing: new unit test Docs Changes: N/A Release Notes: Yes Platform Specific Features: N/A Runtime guard: envoy.restart_features.fix_dispatcher_approximate_now --------- Signed-off-by: Dan Zhang <[email protected]> Co-authored-by: Dan Zhang <[email protected]> Signed-off-by: sunaydagli <[email protected]>
<!-- !!!ATTENTION!!! If you are fixing *any* crash or *any* potential security issue, *do not* open a pull request in this repo. Please report the issue via emailing [email protected] where the issue will be triaged appropriately. Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) --> Commit Message: Invoke ORCA Load Report callbacks from `Router::Filter`. - Add `LoadBalancerContext::setOrcaLoadReportCb(OrcaLoadReportCb callback)` method. - Allow LoadBalancer to set the callback to be invoked when ORCA load report is received. Risk Level: Low Release Notes: N/A Platform Specific Features: N/A #34777 --------- Signed-off-by: Misha Efimov <[email protected]> Signed-off-by: sunaydagli <[email protected]>
This PR updates the test by using a local test server instead of making an external network request, which can make the test flaky. Risk Level: low (test only) Testing: unit test Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Fredy Wijaya <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Commit Message: fmtlib no longer supports implicit conversion from enum to int and as such all call sites needed to be modified to explicitly convert enum values to int. --------- Signed-off-by: Yan Avlasov <[email protected]> Signed-off-by: sunaydagli <[email protected]>
…e downstream (#34461) Commit Message: Allow HTTP/2 (and HTTP/3) upstream servers to half close the stream before the downstream. This enables bidirectional gRPC streams where server completes streaming before the client. Behavior of HTTP/1 or TCP proxy upstream servers is unchanged and the stream is reset if the upstream server completes response before the downstream. The stream is also reset if the upstream server responds with an error status before the downstream. This behavior is disabled by default and can be enabled by setting the ``envoy.reloadable_features.allow_multiplexed_upstream_half_close`` runtime key to true. Change details: Presently there are two places where the stream was reset when upstream half-closed. 1. In the router filter's `Filter::onUpstreamComplete` method, which covers HTTP upstreams. 2. In the filter manager's `FilterManager::commonEncodePrefix` which covers local reply's by filters and TCP upstreams. When the ``envoy.reloadable_features.allow_multiplexed_upstream_half_close`` is enabled the router filter no longer forces reset in the `Filter::onUpstreamComplete` and allows fully independent half closes. To preserve existing half close behavior of HTTP/1 protocol the force reset is added in the H/1 codec's `ClientConnectionImpl::onMessageCompleteBase()` method. When the ``envoy.reloadable_features.allow_multiplexed_upstream_half_close`` is enabled the filter manager closes stream after both decoder and encoder filter chain complete, except in two cases: 1. local reply - behaves the same. 2. error (non 1xx or 2xx) response from the server. This case is handled in the `FilterManager::checkAndCloseStreamIfFullyClosed()` method. The `state_.decoder_filter_chain_complete_` is added to track completion of the decoder filter chain. To preserver behavior for TCP upstream the force reset is moved into the `TcpUpstream::onUpstreamData` method. Risk Level: High (flag protected, disabled by default) Testing: Unit Tests Docs Changes: No Release Notes: Yes Platform Specific Features: N/A Runtime guard: envoy.reloadable_features.allow_multiplexed_upstream_half_close Fixes #30149 --------- Signed-off-by: Yan Avlasov <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Fixes #33014 Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Fredy Wijaya <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Adds a configuration to set the number of retries in `getaddrinfo` resolver. If the `num_retries` is set, the resolver will retry up to `num_retries` when `getaddrinfo` returns `EAI_AGAIN`. If `num_retries` is not set, the resolver will retry indefinitely until it succeeds or the DNS times out to keep the behavior the same as the current behavior. Risk Level: low (a new feature) Testing: unit tests Docs Changes: inline Release Notes: inline Platform Specific Features: n/a Runtime guard: `envoy.reloadable_features.getaddrinfo_num_retries` --------- Signed-off-by: Fredy Wijaya <[email protected]> Signed-off-by: sunaydagli <[email protected]>
…5876) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
… handled by util method. (#35868) Signed-off-by: blake-snyder <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Created by Envoy dependency bot for @phlax Fix #35878 Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Co-authored-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
Created by Envoy dependency bot for @phlax Fix #35879 Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Co-authored-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
Created by Envoy dependency bot for @phlax Fix #35880 Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Co-authored-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: sunaydagli <[email protected]> Signed-off-by: sunaydagli <[email protected]>
Signed-off-by: sunaydagli <[email protected]> Signed-off-by: sunaydagli <[email protected]>
sunaydagli
requested review from
RyanTheOptimist,
alyssawilk,
abeyad,
fredyw,
nezdolik,
ravenblackx,
zuercher,
jmarantz,
kyessenov,
soulxu and
adisuissa
as code owners
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Commit Message: Allow prepending access log handler for WASM filter
Additional Description:
Risk Level: Low
Testing: Integration
Docs Changes:
Release Notes:
Platform Specific Features:
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, so @spacewander and any other, would welcome feedback or approval on this PR.