Skip to content

Commit

Permalink
Service config parse failures should be UNAVAILABLE
Browse files Browse the repository at this point in the history 
INVALID_ARGUMENT is propagated to the data plane if no previous config
is available. INVALID_ARGUMENT is reserved for application use; LBs
should pretty much use UNAVAILABLE exclusively.

While most of the changes are in xds, there do not appear to be likely
xds code paths that would propagate a bad status to the data plane.
Internal policies either don't use parseLoadBalancingPolicyConfig() and
instead have their configuration objects constructed directly or are
constructed transitively through the cluster manager which uses INTERNAL
if there's a child failure. There was a worrisome hole before this
commit for StatusRuntimeExceptions received by the cluster manager, but
the audit didn't find any locations throwing such an exception.
User-selected policies produce a NACK and are protected from the
existing xds client watcher paths. The worst that appears could happen
is the channel could panic (which uses INTERNAL) if a bug let a bad
configuration through.
  • Loading branch information
@ejona86
ejona86 committed Jul 8, 2022
1 parent ac23d33 commit 19ad446
Show file tree
Hide file tree
Showing 13 changed files with 23 additions and 23 deletions.
2 changes: 1 addition & 1 deletion grpclb/src/main/java/io/grpc/grpclb/GrpclbLoadBalancerProvider.java
View file
Expand Up @@ -124,7 +124,7 @@ ConfigOrError parseLoadBalancingConfigPolicyInternal(
}
return ConfigOrError.fromError(
Status
.INVALID_ARGUMENT
.UNAVAILABLE
.withDescription(
"None of " + policiesTried + " specified child policies are available."));
}
Expand Down
2 changes: 1 addition & 1 deletion ...op-testing/src/main/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProvider.java
View file
Expand Up @@ -54,7 +54,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawLoadBalanc
String rpcBehavior = JsonUtil.getString(rawLoadBalancingPolicyConfig, "rpcBehavior");
if (rpcBehavior == null) {
return ConfigOrError.fromError(
Status.INVALID_ARGUMENT.withDescription("no 'rpcBehavior' defined"));
Status.UNAVAILABLE.withDescription("no 'rpcBehavior' defined"));
}
return ConfigOrError.fromConfig(new RpcBehaviorConfig(rpcBehavior));
}
Expand Down
2 changes: 1 addition & 1 deletion rls/src/main/java/io/grpc/rls/RlsLoadBalancerProvider.java
View file
Expand Up @@ -73,7 +73,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawLoadBalanc
new LbPolicyConfiguration(routeLookupConfig, routeLookupChannelServiceConfig, lbPolicy));
} catch (Exception e) {
return ConfigOrError.fromError(
Status.INVALID_ARGUMENT
Status.UNAVAILABLE
.withDescription("can't parse config: " + e.getMessage())
.withCause(e));
}
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java
View file
Expand Up @@ -192,7 +192,7 @@ private void handleClusterDiscovered() {
root.result.lbPolicyConfig());
LoadBalancerProvider lbProvider = lbRegistry.getProvider(unwrappedLbConfig.getPolicyName());
if (lbProvider == null) {
throw NameResolver.ConfigOrError.fromError(Status.INVALID_ARGUMENT.withDescription(
throw NameResolver.ConfigOrError.fromError(Status.UNAVAILABLE.withDescription(
"No provider available for LB: " + unwrappedLbConfig.getPolicyName())).getError()
.asRuntimeException();
}
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/CdsLoadBalancerProvider.java
View file
Expand Up @@ -75,7 +75,7 @@ static ConfigOrError parseLoadBalancingConfigPolicy(Map<String, ?> rawLoadBalanc
return ConfigOrError.fromConfig(new CdsConfig(cluster));
} catch (RuntimeException e) {
return ConfigOrError.fromError(
Status.fromThrowable(e).withDescription(
Status.UNAVAILABLE.withCause(e).withDescription(
"Failed to parse CDS LB config: " + rawLoadBalancingPolicyConfig));
}
}
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancerProvider.java
View file
Expand Up @@ -110,7 +110,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
}
} catch (RuntimeException e) {
return ConfigOrError.fromError(
Status.fromThrowable(e).withDescription(
Status.INTERNAL.withCause(e).withDescription(
"Failed to parse cluster_manager LB config: " + rawConfig));
}
return ConfigOrError.fromConfig(new ClusterManagerConfig(parsedChildPolicies));
Expand Down
6 changes: 3 additions & 3 deletions xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancerProvider.java
View file
Expand Up @@ -67,13 +67,13 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
choiceCount = DEFAULT_CHOICE_COUNT;
}
if (choiceCount < MIN_CHOICE_COUNT) {
return ConfigOrError.fromError(Status.INVALID_ARGUMENT.withDescription(
"Invalid 'choiceCount'"));
return ConfigOrError.fromError(Status.UNAVAILABLE.withDescription(
"Invalid 'choiceCount' in least_request_experimental config"));
}
return ConfigOrError.fromConfig(new LeastRequestConfig(choiceCount));
} catch (RuntimeException e) {
return ConfigOrError.fromError(
Status.fromThrowable(e).withDescription(
Status.UNAVAILABLE.withCause(e).withDescription(
"Failed to parse least_request_experimental LB config: " + rawConfig));
}
}
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java
View file
Expand Up @@ -81,7 +81,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawLoadBalanc
}
if (minRingSize <= 0 || maxRingSize <= 0 || minRingSize > maxRingSize
|| maxRingSize > MAX_RING_SIZE) {
return ConfigOrError.fromError(Status.INVALID_ARGUMENT.withDescription(
return ConfigOrError.fromError(Status.UNAVAILABLE.withDescription(
"Invalid 'mingRingSize'/'maxRingSize'"));
}
return ConfigOrError.fromConfig(new RingHashConfig(minRingSize, maxRingSize));
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/WeightedTargetLoadBalancerProvider.java
View file
Expand Up @@ -117,7 +117,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
return ConfigOrError.fromConfig(new WeightedTargetConfig(parsedChildConfigs));
} catch (RuntimeException e) {
return ConfigOrError.fromError(
Status.fromThrowable(e).withDescription(
Status.INTERNAL.withCause(e).withDescription(
"Failed to parse weighted_target LB config: " + rawConfig));
}
}
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/WrrLocalityLoadBalancerProvider.java
View file
Expand Up @@ -78,7 +78,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
PolicySelection policySelection = (PolicySelection) selectedConfig.getConfig();
return ConfigOrError.fromConfig(new WrrLocalityConfig(policySelection));
} catch (RuntimeException e) {
return ConfigOrError.fromError(Status.fromThrowable(e)
return ConfigOrError.fromError(Status.INTERNAL.withCause(e)
.withDescription("Failed to parse wrr_locality LB config: " + rawConfig));
}
}
Expand Down
8 changes: 4 additions & 4 deletions xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerProviderTest.java
View file
Expand Up @@ -96,9 +96,9 @@ public void parseLoadBalancingConfig_invalid_negativeSize() throws IOException {
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'choiceCount'");
.isEqualTo("Invalid 'choiceCount' in least_request_experimental config");
}

@Test
Expand All @@ -107,9 +107,9 @@ public void parseLoadBalancingConfig_invalid_tooSmallSize() throws IOException {
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'choiceCount'");
.isEqualTo("Invalid 'choiceCount' in least_request_experimental config");
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions xds/src/test/java/io/grpc/xds/MetadataLoadBalancerProvider.java
View file
Expand Up @@ -45,13 +45,13 @@ public NameResolver.ConfigOrError parseLoadBalancingPolicyConfig(
String metadataKey = JsonUtil.getString(rawLoadBalancingPolicyConfig, "metadataKey");
if (metadataKey == null) {
return NameResolver.ConfigOrError.fromError(
Status.INVALID_ARGUMENT.withDescription("no 'metadataKey' defined"));
Status.UNAVAILABLE.withDescription("no 'metadataKey' defined"));
}

String metadataValue = JsonUtil.getString(rawLoadBalancingPolicyConfig, "metadataValue");
if (metadataValue == null) {
return NameResolver.ConfigOrError.fromError(
Status.INVALID_ARGUMENT.withDescription("no 'metadataValue' defined"));
Status.UNAVAILABLE.withDescription("no 'metadataValue' defined"));
}

return NameResolver.ConfigOrError.fromConfig(
Expand Down
10 changes: 5 additions & 5 deletions xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java
View file
Expand Up @@ -98,7 +98,7 @@ public void parseLoadBalancingConfig_invalid_negativeSize() throws IOException {
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'mingRingSize'/'maxRingSize'");
}
Expand All @@ -109,7 +109,7 @@ public void parseLoadBalancingConfig_invalid_minGreaterThanMax() throws IOExcept
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'mingRingSize'/'maxRingSize'");
}
Expand All @@ -121,7 +121,7 @@ public void parseLoadBalancingConfig_invalid_ringTooLarge() throws IOException {
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'mingRingSize'/'maxRingSize'");
}
Expand All @@ -132,7 +132,7 @@ public void parseLoadBalancingConfig_zeroMinRingSize() throws IOException {
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'mingRingSize'/'maxRingSize'");
}
Expand All @@ -143,7 +143,7 @@ public void parseLoadBalancingConfig_minRingSizeGreaterThanMaxRingSize() throws
ConfigOrError configOrError =
provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig));
assertThat(configOrError.getError()).isNotNull();
assertThat(configOrError.getError().getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE);
assertThat(configOrError.getError().getDescription())
.isEqualTo("Invalid 'mingRingSize'/'maxRingSize'");
}
Expand Down

0 comments on commit 19ad446

Please sign in to comment.