diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java index 55440fde8e..928e8e8bf8 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java @@ -27,6 +27,7 @@ import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; import com.google.common.base.MoreObjects; import com.google.common.collect.Sets; +import com.google.common.primitives.UnsignedInteger; import io.grpc.Attributes; import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; @@ -84,11 +85,10 @@ final class RingHashLoadBalancer extends LoadBalancer { public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { logger.log(XdsLogLevel.DEBUG, "Received resolution result: {0}", resolvedAddresses); List addrList = resolvedAddresses.getAddresses(); - if (addrList.isEmpty()) { - handleNameResolutionError(Status.UNAVAILABLE.withDescription("Ring hash lb error: EDS " - + "resolution was successful, but returned server addresses are empty.")); + if (!validateAddrList(addrList)) { return false; } + Map latestAddrs = stripAttrs(addrList); Set removedAddrs = Sets.newHashSet(Sets.difference(subchannels.keySet(), latestAddrs.keySet())); @@ -166,6 +166,47 @@ final class RingHashLoadBalancer extends LoadBalancer { return true; } + private boolean validateAddrList(List addrList) { + if (addrList.isEmpty()) { + handleNameResolutionError(Status.UNAVAILABLE.withDescription("Ring hash lb error: EDS " + + "resolution was successful, but returned server addresses are empty.")); + return false; + } + + long totalWeight = 0; + for (EquivalentAddressGroup eag : addrList) { + Long weight = eag.getAttributes().get(InternalXdsAttributes.ATTR_SERVER_WEIGHT); + + if (weight == null) { + weight = 1L; + } + + if (weight < 0) { + handleNameResolutionError(Status.UNAVAILABLE.withDescription( + String.format("Ring hash lb error: EDS resolution was successful, but returned a " + + "negative weight for %s.", stripAttrs(eag)))); + return false; + } + if (weight > UnsignedInteger.MAX_VALUE.longValue()) { + handleNameResolutionError(Status.UNAVAILABLE.withDescription( + String.format("Ring hash lb error: EDS resolution was successful, but returned a weight" + + " too large to fit in an unsigned int for %s.", stripAttrs(eag)))); + return false; + } + totalWeight += weight; + } + + if (totalWeight > UnsignedInteger.MAX_VALUE.longValue()) { + handleNameResolutionError(Status.UNAVAILABLE.withDescription( + String.format( + "Ring hash lb error: EDS resolution was successful, but returned a sum of weights too" + + " large to fit in an unsigned int (%d).", totalWeight))); + return false; + } + + return true; + } + private static List buildRing( Map serverWeights, long totalWeight, double scale) { List ring = new ArrayList<>(); diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java index aae297d1ac..a4435625b0 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java @@ -36,6 +36,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import com.google.common.collect.Iterables; +import com.google.common.primitives.UnsignedInteger; import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.ConnectivityStateInfo; @@ -961,6 +962,46 @@ public class RingHashLoadBalancerTest { .requestConnection(); } + @Test + public void largeWeights() { + RingHashConfig config = new RingHashConfig(10000, 100000); // large ring + List servers = + createWeightedServerAddrs(Integer.MAX_VALUE, 10, 100); // MAX:10:100 + boolean addressesAccepted = loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(servers).setLoadBalancingPolicyConfig(config).build()); + assertThat(addressesAccepted).isTrue(); + + // Try value between max signed and max unsigned int + servers = createWeightedServerAddrs(Integer.MAX_VALUE + 100L, 100); // (MAX+100):100 + addressesAccepted = loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(servers).setLoadBalancingPolicyConfig(config).build()); + assertThat(addressesAccepted).isTrue(); + + // Try a negative value + servers = createWeightedServerAddrs(10, -20, 100); // 10:-20:100 + addressesAccepted = loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(servers).setLoadBalancingPolicyConfig(config).build()); + assertThat(addressesAccepted).isFalse(); + + // Try an individual value larger than max unsigned int + long maxUnsigned = UnsignedInteger.MAX_VALUE.longValue(); + servers = createWeightedServerAddrs(maxUnsigned + 10, 10, 100); // uMAX+10:10:100 + addressesAccepted = loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(servers).setLoadBalancingPolicyConfig(config).build()); + assertThat(addressesAccepted).isFalse(); + + // Try a sum of values larger than max unsigned int + servers = createWeightedServerAddrs(Integer.MAX_VALUE, Integer.MAX_VALUE, 100); // MAX:MAX:100 + addressesAccepted = loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(servers).setLoadBalancingPolicyConfig(config).build()); + assertThat(addressesAccepted).isFalse(); + } + @Test public void hostSelectionProportionalToWeights() { RingHashConfig config = new RingHashConfig(10000, 100000); // large ring