Repository navigation
xds: Validate ring_hash requestHashHeader when parsing the config - #13161
Open
hidingmyname wants to merge 1 commit into
Open
hidingmyname wants to merge 1 commit into
hidingmyname wants to merge 1 commit into
Conversation
gRFC A76 requires the config to be rejected if request_hash_header is not a valid header name or ends in -bin. The provider accepted any string, and an invalid name only failed later in RingHashLoadBalancer.acceptResolvedAddresses(), where Metadata.Key.of() throws inside the SynchronizationContext and panics the channel. Validate the name in RingHashLoadBalancerProvider with the same Metadata.Key rules and return an UNAVAILABLE config error if it is invalid. The stored name is now the lower-cased form used by Metadata.Key. Fixes grpc#13151
This branch has not been deployed
This file contains hidden or 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
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.
Fixes #13151
gRFC A76 says the config must be rejected if
request_hash_headeris not a valid header name or is a-binheader.RingHashLoadBalancerProvideraccepted any string, so a bad name only failed later inRingHashLoadBalancer.acceptResolvedAddresses().Metadata.Key.of()then threw inside theSynchronizationContextand panicked the channel. This only applies withGRPC_EXPERIMENTAL_RING_HASH_SET_REQUEST_HASH_KEYenabled.The provider now validates the name with
Metadata.Keyand returns anUNAVAILABLEconfig error if it is invalid, like the existingminRingSize/maxRingSizecheck. The config now stores the lower-cased name thatMetadata.Keyuses.Added three tests to
RingHashLoadBalancerProviderTest: a-binname, invalid characters and a mixed-case name. All three fail without the change. I did not add an end-to-end channel test for the panic, as it needs a customNameResolver; I can add one if you want it.In one full
:grpc-xds:testrun (4489 tests),FakeControlPlaneXdsIntegrationTest.changeClusterForRoute[enableRfc3986UrisParam=true]failed. It doesn't use ring_hash. Rerunning that class on this branch failed once and then passed 16 of 16, so it looks flaky.