Need care with hashing protobufs

[kyessenov]

It appears envoy hashes protobuf config structures to determine whether a config has changed. This is a problem since configs are placed into typed_configs as Any, and protobuf specifically does not guarantee deterministic serialization. The main non-determinism comes from map ordering.

If the control plane repeatedly serializes and pushes same configs without change, then envoy might incorrectly assume a change, and drain/reload listeners causing a significant CPU spike and drained connections.

For golang, the solution is to force the control plane to serialize maps in a stable manner. C++ has a global SetDefaultSerializationDeterministic. I assume the same applies to python and java, but that needs to be checked with protobuf libraries for those languages.

[kyessenov]

/cc @snowp

[lizan]

FYI protocolbuffers/protobuf#5731

[mattklein123]

@lizan @htuch I think we need a holistic approach to solving this problem as this keeps coming up in various forms. Any thoughts here? Do we need someone to take a look at this and think about a general solution?

[mandarjog]

Map ordering is the prominent reason for non determinism. However there are many other reasons why binary encoding of two messages is not the same even though messages are the same.for example You can encode fields out of order, varInt encodings allow for zero extensions.

I think for an api to be clear we could have a “sha” field as a peer of any. That way the onus is on the caller to supply it. At present it is a hidden contract of the api.

[htuch]

@mandarjog arguably that just punts the problem to the management server. It's in a position to compute the SHA for sure, but it's going to be a lot of tedious work if it can't rely on the proto library for this. With resource level versioning in incremental xDS (CC @fredlas), this might be improved, since the management server now provides a per-resource "version" at each update, that can be used to avoid diffing.

[JimmyCYJ]

FYI, when Istio Polot generates SDS config, we need to marshal FileBasedMetadataConfig and place it in a field(TypedConfig) in GrpcService_GoogleGrpc_CallCredentials. FileBasedMetadataConfig does not support deterministic config, and forcing to marshal it deterministically will get empty string. So a workaround I use is to marshal it once, store the marshaled string in a map. Other generation of SDS config just gets a copy of the marshaled string, instead of marshaling FileBasedMetadataConfig.

[lizan]

Short term fixed by #8231. Long term discussion: #8301.

[incfly]

/cc @incfly