Replace Equivalent call with Equals in proto comparisons
#32694
Conversation
…q functors. `eq(k1, k2) -> hash(k1) == hash(k2)` must be true for hashtable usage to be valid. Signed-off-by: Yan Avlasov <[email protected]>
| @@ -224,7 +224,7 @@ class MessageUtil { | |||
|
|
|||
| // std::equals_to | |||
| bool operator()(const Protobuf::Message& lhs, const Protobuf::Message& rhs) const { | |||
| return Protobuf::util::MessageDifferencer::Equivalent(lhs, rhs); | |||
| return Protobuf::util::MessageDifferencer::Equals(lhs, rhs); | |||
|
I think golang intentionally randomizes maps which means Go-based control planes might be triggering this condition. |
|
@ravenblackx did some work in this area, IIRC, around hashing of protobuf structures. I feel like it might be helpful to bring him into this also. I thought we devised a way to make the hash on a map be order independent. It looks like the proposed fix makes (a,b) != (b,a). Do we instead want to make them ==, so I guess that means fixing the hasher to be order-independent (which I think @ravenblackx already has code for) rather than fixing the == to be order-dependent? Or am I mixing this up? This is why a testcase would be helpful :) |
|
I think this has a potential to be a breaking change if a control plane doesn't enforce deterministic encoding (and some SDKs do not by default, e.g. Go). With a weaker equality, a new config is going to be applied more often which increases the load on the main to process it and potentially triggers drains that were not triggered before, which can lead to non-graceful connection termination. |
|
#30761 is Raven's PR that I was thinking of, https://github.com/ravenblackx/envoy/blob/fbed541d563741540e923ea694be13708434acda/source/common/protobuf/deterministic_hash.cc#L84 (reflectionMapField) is the hash function for maps. |
|
Looks like I may have been hasty with this change. Let me think through this one a little more. |
Pull request was converted to draft
Signed-off-by: Yan Avlasov <[email protected]>
yanavlasov
changed the title
Equivalent call with Equals in proto comparisons
…oxy#32694) Replace Equivalent call with Equals in proto comparisons. In OSS proto3 these methods have the same behavior, since their difference is only relevant for proto2, in treatment of missing fields with default values. --------- Signed-off-by: Yan Avlasov <[email protected]>
Commit Message:
Replace
Equivalentcall withEqualsin proto comparisons. In OSS proto3 these methods have the same behavior, since their difference is only relevant for proto2, in treatment of missing fields with default values.This only becomes relevant for Google internal builds where proto2 is used. If this change is considered too risky we can abandon it and deal with it internally.
Risk Level: Low.
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A