From 02a4cb5c69f56c278fb3286397f9640e8b3d1336 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 10 May 2023 11:04:08 -0700 Subject: [PATCH] Fix throwing LBProvider.parseLoadBalancingConfig() impls LoadBalancers in general should never throw, but parseLoadBalancingConfig() in particular has a return value to communicate the error. Throwing can be a bit unpredictable, but at its most trivial form causes a channel panic. There's no reason to throw explicitly and calls to JsonUtil have to be protected by a try-catch because it can throw. --- .../internal/PickFirstLoadBalancerProvider.java | 13 ++++++++++--- .../util/OutlierDetectionLoadBalancerProvider.java | 10 ++++++++++ .../grpc/xds/ClusterImplLoadBalancerProvider.java | 4 +++- .../xds/ClusterResolverLoadBalancerProvider.java | 4 +++- .../io/grpc/xds/PriorityLoadBalancerProvider.java | 4 +++- .../io/grpc/xds/RingHashLoadBalancerProvider.java | 11 +++++++++++ .../xds/WeightedRoundRobinLoadBalancerProvider.java | 11 +++++++++++ 7 files changed, 51 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java index 35d96cfdcc..e3e955ce32 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java @@ -22,6 +22,7 @@ import io.grpc.LoadBalancer; import io.grpc.LoadBalancerProvider; import io.grpc.NameResolver; import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; import io.grpc.internal.PickFirstLoadBalancer.PickFirstLoadBalancerConfig; import java.util.Map; @@ -62,9 +63,15 @@ public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider { public ConfigOrError parseLoadBalancingPolicyConfig( Map rawLoadBalancingPolicyConfig) { if (enablePickFirstConfig) { - return ConfigOrError.fromConfig( - new PickFirstLoadBalancerConfig(JsonUtil.getBoolean(rawLoadBalancingPolicyConfig, - SHUFFLE_ADDRESS_LIST_KEY))); + try { + return ConfigOrError.fromConfig( + new PickFirstLoadBalancerConfig(JsonUtil.getBoolean(rawLoadBalancingPolicyConfig, + SHUFFLE_ADDRESS_LIST_KEY))); + } catch (RuntimeException e) { + return ConfigOrError.fromError( + Status.UNAVAILABLE.withCause(e).withDescription( + "Failed parsing configuration for " + getPolicyName())); + } } else { return ConfigOrError.fromConfig(NO_CONFIG); } diff --git a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancerProvider.java b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancerProvider.java index e52c741465..5d8233eb8a 100644 --- a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancerProvider.java @@ -59,6 +59,16 @@ public final class OutlierDetectionLoadBalancerProvider extends LoadBalancerProv @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { + try { + return parseLoadBalancingPolicyConfigInternal(rawConfig); + } catch (RuntimeException e) { + return ConfigOrError.fromError( + Status.UNAVAILABLE.withCause(e).withDescription( + "Failed parsing configuration for " + getPolicyName())); + } + } + + private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawConfig) { // Common configuration. Long intervalNanos = JsonUtil.getStringAsDuration(rawConfig, "interval"); Long baseEjectionTimeNanos = JsonUtil.getStringAsDuration(rawConfig, "baseEjectionTime"); diff --git a/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancerProvider.java index 11e649474b..575c04ac48 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancerProvider.java @@ -25,6 +25,7 @@ import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.xds.Bootstrapper.ServerInfo; import io.grpc.xds.Endpoints.DropOverload; @@ -60,7 +61,8 @@ public final class ClusterImplLoadBalancerProvider extends LoadBalancerProvider @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalancingPolicyConfig) { - throw new UnsupportedOperationException("not supported as top-level LB policy"); + return ConfigOrError.fromError( + Status.INTERNAL.withDescription(getPolicyName() + " cannot be used from service config")); } @Override diff --git a/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancerProvider.java index 38da1f465c..d587572c9c 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancerProvider.java @@ -24,6 +24,7 @@ import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancerProvider; import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.xds.Bootstrapper.ServerInfo; import io.grpc.xds.EnvoyServerProtoData.OutlierDetection; @@ -58,7 +59,8 @@ public final class ClusterResolverLoadBalancerProvider extends LoadBalancerProvi @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalancingPolicyConfig) { - throw new UnsupportedOperationException("not supported as top-level LB policy"); + return ConfigOrError.fromError( + Status.INTERNAL.withDescription(getPolicyName() + " cannot be used from service config")); } @Override diff --git a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancerProvider.java index df35361bed..2bc561268f 100644 --- a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancerProvider.java @@ -25,6 +25,7 @@ import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancerProvider; import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import java.util.Collections; import java.util.HashSet; @@ -57,7 +58,8 @@ public final class PriorityLoadBalancerProvider extends LoadBalancerProvider { @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { - throw new UnsupportedOperationException(); + return ConfigOrError.fromError( + Status.INTERNAL.withDescription(getPolicyName() + " cannot be used from service config")); } static final class PriorityLbConfig { diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java index 5bba0dc9b0..dad7938456 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java @@ -68,6 +68,17 @@ public final class RingHashLoadBalancerProvider extends LoadBalancerProvider { @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalancingPolicyConfig) { + try { + return parseLoadBalancingPolicyConfigInternal(rawLoadBalancingPolicyConfig); + } catch (RuntimeException e) { + return ConfigOrError.fromError( + Status.UNAVAILABLE.withCause(e).withDescription( + "Failed parsing configuration for " + getPolicyName())); + } + } + + private ConfigOrError parseLoadBalancingPolicyConfigInternal( + Map rawLoadBalancingPolicyConfig) { Long minRingSize = JsonUtil.getNumberAsLong(rawLoadBalancingPolicyConfig, "minRingSize"); Long maxRingSize = JsonUtil.getNumberAsLong(rawLoadBalancingPolicyConfig, "maxRingSize"); long maxRingSizeCap = RingHashOptions.getRingSizeCap(); diff --git a/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java index f4468b79e9..ceaa4d7e97 100644 --- a/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java @@ -24,6 +24,7 @@ import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancerProvider; import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; import io.grpc.internal.JsonUtil; import io.grpc.xds.WeightedRoundRobinLoadBalancer.WeightedRoundRobinLoadBalancerConfig; import java.util.Map; @@ -62,6 +63,16 @@ public final class WeightedRoundRobinLoadBalancerProvider extends LoadBalancerPr @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { + try { + return parseLoadBalancingPolicyConfigInternal(rawConfig); + } catch (RuntimeException e) { + return ConfigOrError.fromError( + Status.UNAVAILABLE.withCause(e).withDescription( + "Failed parsing configuration for " + getPolicyName())); + } + } + + private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawConfig) { Long blackoutPeriodNanos = JsonUtil.getStringAsDuration(rawConfig, "blackoutPeriod"); Long weightExpirationPeriodNanos = JsonUtil.getStringAsDuration(rawConfig, "weightExpirationPeriod");