From b53f5882f13a5cb3c599804e96304bf5b8407ea6 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 22 Jun 2023 14:26:31 -0700 Subject: [PATCH 1/2] grpc-js: Disallow pick_first as child of outlier_detection --- packages/grpc-js/package.json | 2 +- .../grpc-js/src/load-balancer-outlier-detection.ts | 11 +++++++---- packages/grpc-js/src/resolver-dns.ts | 4 ++-- packages/grpc-js/test/test-outlier-detection.ts | 12 +++++++++++- 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index f0331f1fc..d4b822600 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.8.16", + "version": "1.8.17", "description": "gRPC Library for Node - pure JS implementation", "homepage": "https://grpc.io/", "repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js", diff --git a/packages/grpc-js/src/load-balancer-outlier-detection.ts b/packages/grpc-js/src/load-balancer-outlier-detection.ts index 2f72a9625..885c4feb9 100644 --- a/packages/grpc-js/src/load-balancer-outlier-detection.ts +++ b/packages/grpc-js/src/load-balancer-outlier-detection.ts @@ -113,6 +113,9 @@ export class OutlierDetectionLoadBalancingConfig implements LoadBalancingConfig failurePercentageEjection: Partial | null, private readonly childPolicy: LoadBalancingConfig[] ) { + if (childPolicy[0].getLoadBalancerName() === 'pick_first') { + throw new Error('outlier_detection LB policy cannot have a pick_first child policy'); + } this.intervalMs = intervalMs ?? 10_000; this.baseEjectionTimeMs = baseEjectionTimeMs ?? 30_000; this.maxEjectionTimeMs = maxEjectionTimeMs ?? 300_000; @@ -395,8 +398,8 @@ export class OutlierDetectionLoadBalancer implements LoadBalancer { } private isCountingEnabled(): boolean { - return this.latestConfig !== null && - (this.latestConfig.getSuccessRateEjectionConfig() !== null || + return this.latestConfig !== null && + (this.latestConfig.getSuccessRateEjectionConfig() !== null || this.latestConfig.getFailurePercentageEjectionConfig() !== null); } @@ -496,7 +499,7 @@ export class OutlierDetectionLoadBalancer implements LoadBalancer { if (addressesWithTargetVolume < failurePercentageConfig.minimum_hosts) { return; } - + // Step 2 for (const [address, mapEntry] of this.addressMap.entries()) { // Step 2.i @@ -656,4 +659,4 @@ export function setup() { if (OUTLIER_DETECTION_ENABLED) { registerLoadBalancerType(TYPE_NAME, OutlierDetectionLoadBalancer, OutlierDetectionLoadBalancingConfig); } -} \ No newline at end of file +} diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 355ce2dfd..b20b7a543 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -137,7 +137,7 @@ class DnsResolver implements Resolver { details: `Name resolution failed for target ${uriToString(this.target)}`, metadata: new Metadata(), }; - + const backoffOptions: BackoffOptions = { initialDelay: channelOptions['grpc.initial_reconnect_backoff_ms'], maxDelay: channelOptions['grpc.max_reconnect_backoff_ms'], @@ -276,7 +276,7 @@ class DnsResolver implements Resolver { } catch (err) { this.latestServiceConfigError = { code: Status.UNAVAILABLE, - details: 'Parsing service config failed', + details: `Parsing service config failed with error ${(err as Error).message}`, metadata: new Metadata(), }; } diff --git a/packages/grpc-js/test/test-outlier-detection.ts b/packages/grpc-js/test/test-outlier-detection.ts index c9021e605..d51ccf3fd 100644 --- a/packages/grpc-js/test/test-outlier-detection.ts +++ b/packages/grpc-js/test/test-outlier-detection.ts @@ -360,6 +360,16 @@ describe('Outlier detection config validation', () => { }, /failure_percentage_ejection\.enforcement_percentage parse error: value out of range for percentage/); }); }); + describe('child_policy', () => { + it('Should reject a pick_first child_policy', () => { + const loadBalancingConfig = { + child_policy: [{pick_first: {}}] + }; + assert.throws(() => { + OutlierDetectionLoadBalancingConfig.createFromJson(loadBalancingConfig); + }, /outlier_detection LB policy cannot have a pick_first child policy/); + }); + }); }); describe('Outlier detection', () => { @@ -533,4 +543,4 @@ describe('Outlier detection', () => { }) }); }); -}); \ No newline at end of file +}); From ed70a0b381144b387698f2d57001f5a7bc82cbe9 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 27 Jun 2023 10:11:45 -0700 Subject: [PATCH 2/2] Fix handling of OD policy with no child --- packages/grpc-js/src/load-balancer-outlier-detection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/load-balancer-outlier-detection.ts b/packages/grpc-js/src/load-balancer-outlier-detection.ts index 885c4feb9..ce8668f18 100644 --- a/packages/grpc-js/src/load-balancer-outlier-detection.ts +++ b/packages/grpc-js/src/load-balancer-outlier-detection.ts @@ -113,7 +113,7 @@ export class OutlierDetectionLoadBalancingConfig implements LoadBalancingConfig failurePercentageEjection: Partial | null, private readonly childPolicy: LoadBalancingConfig[] ) { - if (childPolicy[0].getLoadBalancerName() === 'pick_first') { + if (childPolicy.length > 0 && childPolicy[0].getLoadBalancerName() === 'pick_first') { throw new Error('outlier_detection LB policy cannot have a pick_first child policy'); } this.intervalMs = intervalMs ?? 10_000;