xds: Hold parsed service config in CdsUpdate

This avoids re-parsing the config within CdsLB, as the providers could
have changed and the config may no longer be valid.

Many usages of ServiceConfigUtil.unwrapLoadBalancingConfig() were
replaced with public API, which should be less brittle to internal
changes. Similarly, config.equals() was added for LBs least_request,
ring_hash, wrr to use more public APIs in testing. But I've gone out of
my way to avoid using equals for XdsClient change detection, by
preserving the original "JSON" config.

This fixes a bug in WRR config parsing which prevented it from parsing
errorUtilizationPenalty as it assumed it would be a Float, not a Double
like our parser actually generates and our API requires.
JsonUtil.getNumberAsFloat() was added specifically for WRR and has never
worked as JSON Numbers will always be Doubles.

In XdsClusterResource.CdsUpdate, the LB-specific fields like minRingSize
were already not used at all, so this commit deletes them as it seems a
relevant cleanup.

Fixes grpc#12733

Author: ejona86

core/src/main/java/io/grpc/internal/JsonUtil.java

@@ -121,33 +121,6 @@ public static Double getNumberAsDouble(Map<String, ?> obj, String key) {
         String.format("value '%s' for key '%s' in '%s' is not a number", value, key, obj));
   }
 
-  /**
-   * Gets a number from an object for the given key.  If the key is not present, this returns null.
-   * If the value does not represent a float, throws an exception.
-   */
-  @Nullable
-  public static Float getNumberAsFloat(Map<String, ?> obj, String key) {
-    assert key != null;
-    if (!obj.containsKey(key)) {
-      return null;
-    }
-    Object value = obj.get(key);
-    if (value instanceof Float) {
-      return (Float) value;
-    }
-    if (value instanceof String) {
-      try {
-        return Float.parseFloat((String) value);
-      } catch (NumberFormatException e) {
-        throw new IllegalArgumentException(
-            String.format("string value '%s' for key '%s' cannot be parsed as a float", value,
-                key));
-      }
-    }
-    throw new IllegalArgumentException(
-        String.format("value %s for key '%s' is not a float", value, key));
-  }
-
   /**
    * Gets a number from an object for the given key, casted to an integer.  If the key is not
    * present, this returns null.  If the value does not represent an integer, throws an exception.

core/src/test/java/io/grpc/internal/JsonUtilTest.java

@@ -50,7 +50,6 @@ public void getNumber() {
     assertThat(JsonUtil.getNumberAsLong(map, "key_number_1")).isEqualTo(1L);
 
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_2.0")).isEqualTo(2D);
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_2.0")).isEqualTo(2F);
     try {
       JsonUtil.getNumberAsInteger(map, "key_string_2.0");
       fail("expecting to throw but did not");
@@ -69,10 +68,8 @@ public void getNumber() {
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_3")).isEqualTo(3D);
     assertThat(JsonUtil.getNumberAsInteger(map, "key_string_3")).isEqualTo(3);
     assertThat(JsonUtil.getNumberAsLong(map, "key_string_3")).isEqualTo(3L);
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_3")).isEqualTo(3F);
 
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_nan")).isNaN();
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_nan")).isNaN();
     try {
       JsonUtil.getNumberAsInteger(map, "key_string_nan");
       fail("expecting to throw but did not");
@@ -123,41 +120,18 @@ public void getNumber() {
       assertThat(e).hasMessageThat().isEqualTo(
           "value 'six' for key 'key_string_six' is not a long integer");
     }
-    try {
-      JsonUtil.getNumberAsFloat(map, "key_string_six");
-      fail("expecting to throw but did not");
-    } catch (RuntimeException e) {
-      assertThat(e).hasMessageThat().isEqualTo(
-          "string value 'six' for key 'key_string_six' cannot be parsed as a float");
-    }
-
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_number_7")).isEqualTo(7F);
 
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_infinity")).isPositiveInfinity();
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_minus_infinity")).isNegativeInfinity();
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_exponent")).isEqualTo(2.998e8D);
 
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_infinity")).isPositiveInfinity();
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_minus_infinity")).isNegativeInfinity();
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_exponent")).isEqualTo(2.998e8F);
-
     assertThat(JsonUtil.getNumberAsDouble(map, "key_string_minus_zero")).isZero();
     assertThat(JsonUtil.getNumberAsInteger(map, "key_string_minus_zero")).isEqualTo(0);
     assertThat(JsonUtil.getNumberAsLong(map, "key_string_minus_zero")).isEqualTo(0L);
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_string_minus_zero")).isZero();
 
     assertThat(JsonUtil.getNumberAsDouble(map, "key_nonexistent")).isNull();
     assertThat(JsonUtil.getNumberAsInteger(map, "key_nonexistent")).isNull();
     assertThat(JsonUtil.getNumberAsLong(map, "key_nonexistent")).isNull();
-    assertThat(JsonUtil.getNumberAsFloat(map, "key_nonexistent")).isNull();
-
-    try {
-      JsonUtil.getNumberAsFloat(map, "key_string_boolean");
-      fail("expecting to throw but did not");
-    } catch (RuntimeException e) {
-      assertThat(e).hasMessageThat().isEqualTo(
-          "value true for key 'key_string_boolean' is not a float");
-    }
   }
 
   @Test

xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java

@@ -32,7 +32,6 @@
 import io.grpc.LoadBalancer;
 import io.grpc.LoadBalancerProvider;
 import io.grpc.LoadBalancerRegistry;
-import io.grpc.NameResolver;
 import io.grpc.Status;
 import io.grpc.StatusOr;
 import io.grpc.internal.GrpcUtil;
@@ -126,24 +125,12 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
     }
     XdsClusterConfig clusterConfig = clusterConfigOr.getValue();
 
-    NameResolver.ConfigOrError configOrError;
     if (clusterConfig.getChildren() instanceof EndpointConfig) {
-      // The LB policy config is provided in service_config.proto/JSON format.
-      configOrError =
-              GracefulSwitchLoadBalancer.parseLoadBalancingPolicyConfig(
-                      Arrays.asList(clusterConfig.getClusterResource().lbPolicyConfig()),
-                      lbRegistry);
-      if (configOrError.getError() != null) {
-        // Should be impossible, because XdsClusterResource validated this
-        return fail(Status.INTERNAL.withDescription(
-                errorPrefix() + "Unable to parse the LB config: " + configOrError.getError()));
-      }
-
       StatusOr<EdsUpdate> edsUpdate = getEdsUpdate(xdsConfig, clusterName);
       StatusOr<ClusterResolutionResult> statusOrResult = clusterState.edsUpdateToResult(
           clusterName,
           clusterConfig.getClusterResource(),
-          configOrError.getConfig(),
+          clusterConfig.getClusterResource().lbPolicyConfig(),
           edsUpdate);
       if (!statusOrResult.hasValue()) {
         Status status = Status.UNAVAILABLE

xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancer.java

@@ -299,6 +299,20 @@ static final class LeastRequestConfig {
       this.choiceCount = Math.min(choiceCount, MAX_CHOICE_COUNT);
     }
 
+    @Override
+    public boolean equals(Object o) {
+      if (!(o instanceof LeastRequestConfig)) {
+        return false;
+      }
+      LeastRequestConfig that = (LeastRequestConfig) o;
+      return this.choiceCount == that.choiceCount;
+    }
+
+    @Override
+    public int hashCode() {
+      return choiceCount;
+    }
+
     @Override
     public String toString() {
       return MoreObjects.toStringHelper(this)

xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java

@@ -155,7 +155,7 @@ class LoadBalancerConfigFactory {
       configBuilder.put(WEIGHT_UPDATE_PERIOD, weightUpdatePeriod);
     }
     if (errorUtilizationPenalty != null) {
-      configBuilder.put(ERROR_UTILIZATION_PENALTY, errorUtilizationPenalty);
+      configBuilder.put(ERROR_UTILIZATION_PENALTY, errorUtilizationPenalty.doubleValue());
     }
     if (metricNamesForComputingUtilization != null
         && !metricNamesForComputingUtilization.isEmpty()) {

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

@@ -828,6 +828,19 @@ public int hashCode() {
           parsedMetricNamesForComputingUtilization);
     }
 
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this)
+          .add("blackoutPeriodNanos", blackoutPeriodNanos)
+          .add("weightExpirationPeriodNanos", weightExpirationPeriodNanos)
+          .add("enableOobLoadReport", enableOobLoadReport)
+          .add("oobReportingPeriodNanos", oobReportingPeriodNanos)
+          .add("weightUpdatePeriodNanos", weightUpdatePeriodNanos)
+          .add("errorUtilizationPenalty", errorUtilizationPenalty)
+          .add("parsedMetricNamesForComputingUtilization", parsedMetricNamesForComputingUtilization)
+          .toString();
+    }
+
     static final class Builder {
       long blackoutPeriodNanos = 10_000_000_000L; // 10s
       long weightExpirationPeriodNanos = 180_000_000_000L; // 3min

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java

@@ -79,7 +79,8 @@ private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map<String, ?> rawC
     Long oobReportingPeriodNanos = JsonUtil.getStringAsDuration(rawConfig, "oobReportingPeriod");
     Boolean enableOobLoadReport = JsonUtil.getBoolean(rawConfig, "enableOobLoadReport");
     Long weightUpdatePeriodNanos = JsonUtil.getStringAsDuration(rawConfig, "weightUpdatePeriod");
-    Float errorUtilizationPenalty = JsonUtil.getNumberAsFloat(rawConfig, "errorUtilizationPenalty");
+    Double errorUtilizationPenalty =
+        JsonUtil.getNumberAsDouble(rawConfig, "errorUtilizationPenalty");
     List<String> metricNamesForComputingUtilization = JsonUtil.getListOfStrings(rawConfig,
         "metricNamesForComputingUtilization");
 
@@ -104,7 +105,7 @@ private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map<String, ?> rawC
       }
     }
     if (errorUtilizationPenalty != null) {
-      configBuilder.setErrorUtilizationPenalty(errorUtilizationPenalty);
+      configBuilder.setErrorUtilizationPenalty(errorUtilizationPenalty.floatValue());
     }
     if (metricNamesForComputingUtilization != null
         && GrpcUtil.getFlag("GRPC_EXPERIMENTAL_WRR_CUSTOM_METRICS", false)) {

xds/src/main/java/io/grpc/xds/XdsClusterResource.java

@@ -43,8 +43,7 @@
 import io.grpc.LoadBalancerRegistry;
 import io.grpc.NameResolver;
 import io.grpc.internal.GrpcUtil;
-import io.grpc.internal.ServiceConfigUtil;
-import io.grpc.internal.ServiceConfigUtil.LbConfig;
+import io.grpc.util.GracefulSwitchLoadBalancer;
 import io.grpc.xds.EnvoyServerProtoData.OutlierDetection;
 import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext;
 import io.grpc.xds.XdsClusterResource.CdsUpdate;
@@ -165,18 +164,16 @@ static CdsUpdate processCluster(Cluster cluster,
     ImmutableMap<String, ?> lbPolicyConfig = LoadBalancerConfigFactory.newConfig(cluster,
         enableLeastRequest);
 
-    // Validate the LB config by trying to parse it with the corresponding LB provider.
-    LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(lbPolicyConfig);
-    NameResolver.ConfigOrError configOrError = loadBalancerRegistry.getProvider(
-        lbConfig.getPolicyName()).parseLoadBalancingPolicyConfig(
-        lbConfig.getRawConfigValue());
+    NameResolver.ConfigOrError configOrError
+        = GracefulSwitchLoadBalancer.parseLoadBalancingPolicyConfig(
+            ImmutableList.of(lbPolicyConfig), loadBalancerRegistry);
     if (configOrError.getError() != null) {
       throw new ResourceInvalidException(
           "Failed to parse lb config for cluster '" + cluster.getName() + "': "
           + configOrError.getError());
     }
 
-    updateBuilder.lbPolicyConfig(lbPolicyConfig);
+    updateBuilder.lbPolicyConfig(configOrError.getConfig(), lbPolicyConfig);
     updateBuilder.filterMetadata(
         ImmutableMap.copyOf(cluster.getMetadata().getFilterMetadataMap()));
 
@@ -582,16 +579,22 @@ abstract static class CdsUpdate implements ResourceUpdate {
 
     abstract ClusterType clusterType();
 
-    abstract ImmutableMap<String, ?> lbPolicyConfig();
-
-    // Only valid if lbPolicy is "ring_hash_experimental".
-    abstract long minRingSize();
+    /** Graceful switch configuration. */
+    Object lbPolicyConfig() {
+      return internalLbPolicyConfig().getValue();
+    }
 
-    // Only valid if lbPolicy is "ring_hash_experimental".
-    abstract long maxRingSize();
+    /**
+     * Use {@link #lbPolicyConfig()} instead. This avoids using the LB policy configs' equals() when
+     * XdsClient squelches config updates that are identical to the current value. LB policies are
+     * not required to implement equals for their configs. Instead, {link
+     * #internalLbPolicyConfigJson()} is used to detect changes.
+     */
+    abstract IgnoreEquals<Object> internalLbPolicyConfig();
 
-    // Only valid if lbPolicy is "least_request_experimental".
-    abstract int choiceCount();
+    /** Use {@code lbPolicyConfig} instead. */
+    @Nullable
+    abstract ImmutableMap<String, ?> internalLbPolicyConfigJson();
 
     // Alternative resource name to be used in EDS requests.
     /// Only valid for EDS cluster.
@@ -640,9 +643,6 @@ abstract static class CdsUpdate implements ResourceUpdate {
     private static Builder newBuilder(String clusterName) {
       return new AutoValue_XdsClusterResource_CdsUpdate.Builder()
           .clusterName(clusterName)
-          .minRingSize(0)
-          .maxRingSize(0)
-          .choiceCount(0)
           .filterMetadata(ImmutableMap.of())
           .parsedMetadata(ImmutableMap.of())
           .isHttp11ProxyAvailable(false)
@@ -703,10 +703,7 @@ public final String toString() {
       return MoreObjects.toStringHelper(this)
           .add("clusterName", clusterName())
           .add("clusterType", clusterType())
-          .add("lbPolicyConfig", lbPolicyConfig())
-          .add("minRingSize", minRingSize())
-          .add("maxRingSize", maxRingSize())
-          .add("choiceCount", choiceCount())
+          .add("lbPolicyConfigJson", internalLbPolicyConfigJson())
           .add("edsServiceName", edsServiceName())
           .add("dnsHostName", dnsHostName())
           .add("lrsServerInfo", lrsServerInfo())
@@ -725,31 +722,31 @@ abstract static class Builder {
       // Private, use one of the static factory methods instead.
       protected abstract Builder clusterType(ClusterType clusterType);
 
-      protected abstract Builder lbPolicyConfig(ImmutableMap<String, ?> lbPolicyConfig);
-
-      Builder roundRobinLbPolicy() {
-        return this.lbPolicyConfig(ImmutableMap.of("round_robin", ImmutableMap.of()));
-      }
-
-      Builder ringHashLbPolicy(Long minRingSize, Long maxRingSize) {
-        return this.lbPolicyConfig(ImmutableMap.of("ring_hash_experimental",
-            ImmutableMap.of("minRingSize", minRingSize.doubleValue(), "maxRingSize",
-                maxRingSize.doubleValue())));
-      }
-
-      Builder leastRequestLbPolicy(Integer choiceCount) {
-        return this.lbPolicyConfig(ImmutableMap.of("least_request_experimental",
-            ImmutableMap.of("choiceCount", choiceCount.doubleValue())));
+      /**
+       * The config to use, and the JSON representation that produced that config. The JSON
+       * representation is only used to detect if the configuration changed, since LB policies don't
+       * have to implement equals() for their parsed configs.
+       */
+      protected Builder lbPolicyConfig(
+          Object gracefulSwitchConfig, ImmutableMap<String, ?> jsonConfig) {
+        return internalLbPolicyConfig(new IgnoreEquals<>(gracefulSwitchConfig))
+            .internalLbPolicyConfigJson(jsonConfig);
+      }
+
+      protected Builder lbPolicyConfigJsonForTesting(ImmutableMap<String, ?> jsonConfig) {
+        NameResolver.ConfigOrError result =
+            GracefulSwitchLoadBalancer.parseLoadBalancingPolicyConfig(
+                ImmutableList.of(jsonConfig));
+        if (result.getError() != null) {
+          throw new IllegalArgumentException(
+              "Bad JSON config: " + result.getError() + " json: " + jsonConfig);
+        }
+        return lbPolicyConfig(result.getConfig(), jsonConfig);
       }
 
-      // Private, use leastRequestLbPolicy(int).
-      protected abstract Builder choiceCount(int choiceCount);
-
-      // Private, use ringHashLbPolicy(long, long).
-      protected abstract Builder minRingSize(long minRingSize);
+      protected abstract Builder internalLbPolicyConfig(IgnoreEquals<Object> gracefulSwitchConfig);
 
-      // Private, use ringHashLbPolicy(long, long).
-      protected abstract Builder maxRingSize(long maxRingSize);
+      protected abstract Builder internalLbPolicyConfigJson(ImmutableMap<String, ?> jsonConfig);
 
       // Private, use CdsUpdate.forEds() instead.
       protected abstract Builder edsServiceName(String edsServiceName);
@@ -783,4 +780,35 @@ protected abstract Builder backendMetricPropagation(
       abstract CdsUpdate build();
     }
   }
+
+  /** Always equal to this same type. */
+  static final class IgnoreEquals<V> {
+    private final V value;
+
+    IgnoreEquals(V value) {
+      this.value = value;
+    }
+
+    public V getValue() {
+      return value;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (!(o instanceof IgnoreEquals)) {
+        return false;
+      }
+      return true;
+    }
+
+    @Override
+    public int hashCode() {
+      return 0;
+    }
+
+    @Override
+    public String toString() {
+      return "IgnoreEquals{" + value + "}";
+    }
+  }
 }