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

Fix redis transaction when traffic is mirrored #28149

Closed
wants to merge 201 commits into from

Conversation

asheryerm
Copy link
Contributor

@asheryerm asheryerm commented Jun 27, 2023

Commit Message: Fix redis transaction when traffic is mirrored

Additional Description:

When envoy is configured to mirror Redis traffic, using transactions (MULTI) would not work well, possibly leading to a crash. This PR fixes this issue by making the following changes:

  1. I have defined MULTI, EXEC, and DISCARD as write commands, so they will be mirrored even when read commands are excluded from mirroring.
  2. A dedicated connection for the transaction is opened for each redis server that receives mirrored commands, the same way a dedicated connection is opened to the main redis server.

This PR fixes the following issue: #26342

Risk Level: Low
Testing: Integration tests and local tests. Reproduced the problem by using the same configuration used in the open issue.
Docs Changes: None
Release Notes: None
Platform Specific Features: None
[Optional Runtime guard:]
[Optional Fixes #Issue] #26342
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@asheryerm asheryerm changed the title Fix redis transaction when traffic is mirrored (#26342 Fix redis transaction when traffic is mirrored (#26342) Jun 27, 2023
@asheryerm asheryerm changed the title Fix redis transaction when traffic is mirrored (#26342) Fix redis transaction when traffic is mirrored Jun 27, 2023
Network::ConnectionCallbacks* connection_cb_;
uint32_t current_client_idx{0};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing underscore on member variables.

Would be nice to have some commentary on this too, since it's a bit of a weird mutable value. Is there definitely no way this can change while operations are in flight? Is this whole class effectively single-threaded in everything it does?

I'm not familiar with this code, but I would expect

mirror_policy->upstream()->makeRequest(key, ConnPool::RespVariant(incoming_request),
                                               null_pool_callbacks, transaction);

to be something where stuff happens asynchronously, such that the loop that makes this call for each mirror instance might be doing something racy once this gets out of the test environment.

Copy link
Contributor Author

@asheryerm asheryerm Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requests themselves are always sent in the same order, this can be seen in the code. First you send the request to the main server:

  transaction.current_client_idx_ = 0;
  auto handler = route->upstream()->makeRequest(key, ConnPool::RespVariant(incoming_request),
                                                callbacks, transaction);

And then to each of the mirror servers in the order in which they appear in the vector route->mirrorPolicies() at startup:

  if (handler) {
    for (auto& mirror_policy : route->mirrorPolicies()) {
      transaction.current_client_idx++;
      if (mirror_policy->shouldMirror(command)) {
        mirror_policy->upstream()->makeRequest(key, ConnPool::RespVariant(incoming_request),
                                               null_pool_callbacks, transaction);
      }
    }
  }

The replies may be received out of order but that's ok because each reply is mapped to the proper client connection. For mirrored connections we ignore the replies in any case. This is done by providing null_pool_callbacks as the callbacks parameter. So only the reply to the main connection is not discarded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the underscore + a comment on the variable name

ravenblackx
ravenblackx previously approved these changes Jun 29, 2023
@ravenblackx
Copy link
Contributor

Got a DCO problem now because the commit wasn't signed.

asheryerm and others added 21 commits July 5, 2023 10:33
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…nvoyproxy#27278)

Commit Message: application_logs: add bootstrap option to write logs in JSON format
Additional Description: Adds an option in bootstrap config to write application logs in JSON format, while supporting all the log-format flags as defined in the CLI --log-format option. Related to envoyproxy#25959 - this is the first step in the implementation for supporting custom JSON properties, while printing the application logs output in JSON format.
Risk Level: Low (all new code paths are only enabled by config option)
Testing: Unit tests
Docs Changes: API, Application logs docs
Release Notes: None
Platform Specific Features: None
Signed-off-by: ohadvano <ohadvano@gmail.com>
Signed-off-by: ohadvano <49730675+ohadvano@users.noreply.github.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Commit Message: [QUIC only] Log retransmitted packets and bytes.
Additional Description: Adds to the deferred logging implementation from envoyproxy#23648 and implements the existing OnPacketRetransmitted function.

Risk Level: Low
Testing: Integration test
Docs Changes: N/A
Release Notes: added

Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: ohadvano <49730675+ohadvano@users.noreply.github.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…xy#27707)

the no-YAML build is easy to break and we've move this off the constrained machines.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
* [balsa] Disallow extended ASCII in header names.

This is for parity with http-parser. Note that BalsaParser already
disallows control characters, space, and certain separator characters
[1, 2], but not characters above 127.

[1]
https://github.com/google/quiche/blob/ef9e527e14440794cc18813ef079ebdb3650f8ad/quiche/balsa/header_properties.h#L21-L30
[2]
https://github.com/google/quiche/blob/ef9e527e14440794cc18813ef079ebdb3650f8ad/quiche/balsa/balsa_frame.cc#L414

Signed-off-by: Bence Béky <bnc@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
With PR envoyproxy#25818, xds attributes (like upstream cluster metadata) are provided via streamInfo and are available to CEL library. This PR enables them and tests upstream cluster metadata specifically.

Signed-off-by: asheryer <asheryer@amazon.com>
Commit Message: remove all usage of quic::kRVCM
Additional Description: This QUIC connection option no longer does anything and is no longer needed after some recent (~1 month ago) changes in QUICHE. Now, QUICHE is removing the definition of quic::kRVCM. Removing all usage from Envoy to unblock QUICHE rolls to Envoy.
Risk Level: Low
Testing: Covered by existing unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Eric Orth <ericorth@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Commit Message: Implement hidden functionality of gauges to stats_request
Additional Description:
Risk Level: Low
Testing: Added three tests to stats_requests_test

Signed-off-by: AlanDiaz <diazalan@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
* Add a histogram for the tls inspector to capture the amount of data the filter parsed.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
… in tests (envoyproxy#27642)

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Commit Message: Renders histograms in the stats HTML view. Screenshot:
<img width="849" alt="Screenshot 2023-05-29 at 11 39 15 PM" src="https://github.com/envoyproxy/envoy/assets/1942589/eeb650f1-0f6a-42e2-a03d-a6a4b22cb9fa">

This is the next phase in envoyproxy#26472 -- remaining phases are further refactors, as well as sorting histograms by change-count for active-html mode, and make web navigation work better.

This includes unit tests for the new histogram rendering, as well as a mechanism to render histograms.js on its own.

Additional Description:
Risk Level: low
Testing: //test/...,  test/integration/admin_html/web_test.sh for browser-based tests on FF, and manually ran on Chrome as well.
Docs Changes: n/a -- the histograms were previously rendered as text with format=html; now they are rendered graphically, but detail about this is not in the docs currently.
Release Notes:
Platform Specific Features:

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
…voyproxy#27754)

Risk Level: low
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…ension (envoyproxy#27593)

* upstream: update interface of TypedLoadBalancerFactory to support hierarchical lb

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* subset lb: split child lb creation out to separated creator

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* subset lb: complete the development of lb

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* complete test of subset

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* fix docs

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* resolve conflict after merge main

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* fix type url in extension file

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* remove unnecessary comment

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* add more test for coverage

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

---------

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…roxy#27811)

The new build option simply compiles out all try/catch code, while leaving in the exceptions. This can not yet be successfully used as it turns up fno-exceptions which chokes on throw statements. This is by design as if we compiled out throw as well, config failures would be fatal instead of gracefully handled.

Risk Level: low
Testing: manual testing
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy#27412

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
Enable oghttp2 and UHV in HTTP/2 flood test

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…alking (envoyproxy#27838)

build(deps): bump elasticsearch in /examples/skywalking

Bumps elasticsearch from 8.7.1 to 8.8.0.

---
updated-dependencies:
- dependency-name: elasticsearch
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: asheryer <asheryer@amazon.com>
simplyatul and others added 9 commits July 5, 2023 10:33
This PR seeks to improve this issue: envoyproxy#1963

Signed-off-by: Atul Thosar <atulthosar@gmail.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…yproxy#28055)

* maglev: fix maglev stability by sorting host_weights beforehand

Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
Signed-off-by: asheryer <asheryer@amazon.com>
…OG (envoyproxy#28117)

Signed-off-by: ohadvano <ohadvano@gmail.com>
Signed-off-by: asheryer <asheryer@amazon.com>
* http: Add metric for active outbound frames

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>
Signed-off-by: asheryer <asheryer@amazon.com>
…y#28096)

Signed-off-by: Robert Femmer <robert.femmer@x41-dsec.de>
Signed-off-by: asheryer <asheryer@amazon.com>
…nvoyproxy#28054)

One can specify a MetadataKey with a path selector to pick up a host
from the dynamic metadata of the request or downstream. Selected
value can either be a string or a list with at least a single
element of string type. Request metadata is considered first.

Signed-off-by: Andrii Chabykin <chabster@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: asheryer <asheryer@amazon.com>
@asheryerm
Copy link
Contributor Author

Unfortunately Signing off previous commits is complicated since I have a merge commit. Closing this PR and opening a new PR: #28244

@asheryerm
Copy link
Contributor Author

Opened a new PR with commits signed properly: #28244

@asheryerm asheryerm closed this Jul 5, 2023
mattklein123 pushed a commit that referenced this pull request Jul 7, 2023
) (#28244)

Signed-off-by: asheryer <asheryer@amazon.com>
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
…oyproxy#28149) (envoyproxy#28244)

Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
sumitd2 pushed a commit to sumitd2/envoy that referenced this pull request Jul 12, 2023
…oyproxy#28149) (envoyproxy#28244)

Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Sumit Dubey <sumit.dubey2@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.