use hash func that generates deterministic result for protobuf by quanjielin · Pull Request #5814 · envoyproxy/envoy

quanjielin · 2019-02-02T01:31:52Z

This PR uses hash func that generates deterministic result for proto.

Background -
When integrate envoy SDS with Istio, we found envoy sends out multiple requests for same sdsconfig(details in #5744);

After some debugging, we found the issue describes in protocolbuffers/protobuf#5668.

quanjielin · 2019-02-02T02:26:54Z

/cc @qiwzhang @lizan PTAL.

mattklein123

Thanks for the investigation and fix. Can we craft a failing test for both of these call sites? Thank you!

/wait

mattklein123 · 2019-02-03T01:21:41Z

source/common/router/rds_impl.cc

I think it's probably OK to use an 8-byte hash for this (we do similar things elsewhere for collision detection). Assuming people agree, can you remove the TODO above? (The alternative would be to use deterministic serialization to string much like MessageUtil::hash does.)

lizan · 2019-02-05T10:23:10Z

source/common/secret/secret_manager_impl.h

You can just use uint64_t for map_key type and change type elsewhere, no need convert to string.

since resourcename is also needed to construct mapkey, are you suggesting using something like MessageUtil::hash(sds_config_source+config_name) ?

Ah sorry I meant rds_impl

quanjielin · 2019-02-08T00:25:16Z

/cc @PiotrSikora

Signed-off-by: Quanjie Lin <quanlin@google.com>

PiotrSikora · 2019-02-08T01:52:26Z

source/common/router/rds_impl.cc

 RdsRouteConfigSubscription::RdsRouteConfigSubscription(
    const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
-    const std::string& manager_identifier, Server::Configuration::FactoryContext& factory_context,
+    const uint64_t& manager_identifier, Server::Configuration::FactoryContext& factory_context,


I think you need to drop & here (and in the header file).

Signed-off-by: Quanjie Lin <quanlin@google.com>

lizan · 2019-02-08T02:05:07Z

how about tests?

PiotrSikora · 2019-02-08T02:11:59Z

@lizan what test(s) would you like to see here? Showing that SerializeAsString() is nondeterministic and MessageUtil::hash() is? Do we have examples of such tests for other xDS services? Thanks!

htuch

Yeah, I think this is good without tests. Have we grepped for other uses of SerializeAsString? Ideally squash them all (doesn't have to be this PR).

htuch · 2019-02-08T04:26:26Z

To clarify, good without tests because I imagine we're talking about protobuf internal implementation details. If you happen to have a repeatable test case that can easily show this then we should add it, but I'm suspecting not.

PiotrSikora · 2019-02-08T04:52:03Z

@quanjielin could you also fix source/common/config/config_provider_impl.h?

mattklein123 · 2019-02-08T05:19:32Z

Why are we ok without tests here? It seems like it shouldn't be very difficult to craft a case that reproduces this? Or am I missing the problem?

htuch · 2019-02-08T05:24:34Z

@mattklein123 you're asking to come up with two situations in which SerializeAsString() produces different results in the existing code base; maybe it's as simple as two back-to-back calls, but maybe it involves some stateful internals of protobuf library. If it's the former, seems totally reasonable to add a test, but the latter will have less value due to its tight coupling with a single implementation.

mattklein123 · 2019-02-08T05:28:48Z

Ok fair enough. Please open up a tech debt issue to remove all use of serializeasstring as well as a format check to block it's use at the very least? (A stronger version of @htuch previous comment). Thank you.

PiotrSikora · 2019-02-08T05:41:01Z

source/common/config/config_provider_impl.h

Nit: fix format (extra spaces).

Signed-off-by: Quanjie Lin<quanlin@google.com> Signed-off-by: Quanjie Lin <quanlin@google.com>

PiotrSikora · 2019-02-08T05:48:25Z

(I've asked @quanjielin to force-push, due to missed DCO in previous commit)

PiotrSikora

LGTM, with small nit. Thanks for the quick turn-around!

PiotrSikora · 2019-02-08T05:52:24Z

source/common/config/config_provider_impl.h

  using ConfigProviderSet = std::unordered_set<ConfigProvider*>;
  using ConfigProviderMap = std::unordered_map<ConfigProviderInstanceType,
                                               std::unique_ptr<ConfigProviderSet>, EnumClassHash>;
+


Nit: new extra line.

lizan · 2019-02-08T06:36:05Z

source/common/secret/secret_manager_impl.h

                 Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
-      const std::string map_key = sds_config_source.SerializeAsString() + config_name;
+      const std::string map_key =
+          std::to_string(MessageUtil::hash(sds_config_source)) + config_name;


Will a number started config_name potentially conflicts with other hashes? i.e. a config hash 123 with config name 0-abc conflicts with a config hash 12 with config name 30-abc, this happens much easier than hash conflict.

Potentially, yes, though I'm not sure how well do we guard against that in rest of the codebase.

@quanjielin could you add a separator? e.g.

const std::string map_key = absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name);

Thanks!

Yeah that will be good enough

htuch

LGTM, but please add a check like https://github.com/envoyproxy/envoy/blob/master/tools/check_format.py#L393 for SerializeAsString and some test like https://github.com/envoyproxy/envoy/blob/master/tools/check_format_test_helper.py#L194.

Signed-off-by: Quanjie Lin <quanlin@google.com>

htuch · 2019-02-08T17:35:17Z

@quanjielin also, can you avoid force pushing from now on? Please DCO as appropriate to avoid this. It makes it hard to understand the diff between reviews.

lizan · 2019-02-08T17:36:41Z

LGTM, but please add a check like https://github.com/envoyproxy/envoy/blob/master/tools/check_format.py#L393 for SerializeAsString and some test like https://github.com/envoyproxy/envoy/blob/master/tools/check_format_test_helper.py#L194.

Not sure if we really need that, some use cases doesn't need deterministic serialization (e.g. gRPC codec, TAP writing to file.)

htuch · 2019-02-08T17:37:52Z

@lizan these an be whitelisted at a file-level. I rather play it safe with this one as we have been burned.

Signed-off-by: Quanjie Lin <quanlin@google.com>

htuch

Thanks!

…proxy#5814) This PR uses hash func that generates deterministic result for proto. Background - When integrate envoy SDS with Istio, we found envoy sends out multiple requests for same sdsconfig(details in envoyproxy#5744); After some debugging, we found the issue describes in [protocolbuffers/protobuf#5668](protocolbuffers/protobuf#5668). Signed-off-by: Quanjie Lin <quanlin@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

quanjielin force-pushed the quanlinhash0201 branch 2 times, most recently from 5eddb10 to 2d018ec Compare February 2, 2019 01:42

mattklein123 assigned mattklein123 and lizan Feb 3, 2019

mattklein123 requested changes Feb 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 3, 2019

lizan reviewed Feb 5, 2019

View reviewed changes

quanjielin added 2 commits February 7, 2019 17:43

use hashfunc that generates deterministic for protobuf

3299ceb

Signed-off-by: Quanjie Lin <quanlin@google.com>

format

b7957ec

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin force-pushed the quanlinhash0201 branch from 2d018ec to 8bf5b0d Compare February 8, 2019 01:44

repokitteh-read-only bot removed the waiting label Feb 8, 2019

use uint64_t as rds manager_identifier

cffd4e9

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin force-pushed the quanlinhash0201 branch from 8bf5b0d to cffd4e9 Compare February 8, 2019 01:46

cleanup

3396c2d

Signed-off-by: Quanjie Lin <quanlin@google.com>

PiotrSikora reviewed Feb 8, 2019

View reviewed changes

address comments

ed28a13

Signed-off-by: Quanjie Lin <quanlin@google.com>

htuch previously approved these changes Feb 8, 2019

View reviewed changes

PiotrSikora previously approved these changes Feb 8, 2019

View reviewed changes

quanjielin dismissed stale reviews from PiotrSikora and htuch via 0535bba February 8, 2019 05:38

PiotrSikora reviewed Feb 8, 2019

View reviewed changes

source/common/config/config_provider_impl.h Outdated

Copy link

Contributor

PiotrSikora Feb 8, 2019

Choose a reason for hiding this comment

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

Nit: fix format (extra spaces).

update config provider

1d79741

Signed-off-by: Quanjie Lin<quanlin@google.com> Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin force-pushed the quanlinhash0201 branch from 0535bba to 1d79741 Compare February 8, 2019 05:45

PiotrSikora previously approved these changes Feb 8, 2019

View reviewed changes

lizan reviewed Feb 8, 2019

View reviewed changes

htuch reviewed Feb 8, 2019

View reviewed changes

quanjielin dismissed PiotrSikora’s stale review via a32f0f6 February 8, 2019 16:52

update constructing sds mapkey

7ce7966

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin force-pushed the quanlinhash0201 branch from a32f0f6 to 7ce7966 Compare February 8, 2019 16:55

quanjielin added 3 commits February 8, 2019 11:04

update check_format

077ba0d

Signed-off-by: Quanjie Lin <quanlin@google.com>

format

8a7bb58

Signed-off-by: Quanjie Lin <quanlin@google.com>

format again

eb10a74

Signed-off-by: Quanjie Lin <quanlin@google.com>

htuch approved these changes Feb 8, 2019

View reviewed changes

htuch merged commit 925810d into envoyproxy:master Feb 8, 2019

quanjielin deleted the quanlinhash0201 branch February 9, 2019 02:41

lizan mentioned this pull request Feb 14, 2019

SDS/RDS sends two DiscoveryRequest with same config resource name #5744

Closed

Conversation

quanjielin commented Feb 2, 2019

Uh oh!

quanjielin commented Feb 2, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quanjielin commented Feb 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Feb 8, 2019

Uh oh!

PiotrSikora commented Feb 8, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 8, 2019

Uh oh!

PiotrSikora commented Feb 8, 2019

Uh oh!

mattklein123 commented Feb 8, 2019

Uh oh!

htuch commented Feb 8, 2019

Uh oh!

mattklein123 commented Feb 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Feb 8, 2019

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 8, 2019

Uh oh!

lizan commented Feb 8, 2019

Uh oh!

htuch commented Feb 8, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants