grpc-js: default service config (`grpc.service_config`) is not used when the first resolver result has an invalid service config

packages/grpc-js/src/resolving-load-balancer.tsを読み、特に290–301行目のservice configの選択と、217–226行目のデフォルト設定の初期化を確認してください。コンストラクターが、明示的に指定されたgrpc.service_configと空のデフォルト設定をどのように区別するかを確認し、その後、issueに記載された再現テストを実行してください。無効なresolver configが明示的なデフォルトにフォールバックするのは、それ以前に有効な設定が存在しない場合に限られ、デフォルトがない場合は引き続きresolution failureが報告される状態になれば完了です。

Problem description

Location (grpc/grpc-node @ default branch ca7f83f):

• packages/grpc-js/src/resolving-load-balancer.ts:290-301 — service config selection
• packages/grpc-js/src/resolving-load-balancer.ts:217-226 — defaultServiceConfig is always filled
• packages/grpc-js/src/resolver-dns.ts:268-275 — example source of a service-config error

if (serviceConfig === null) {
  workingServiceConfig = this.defaultServiceConfig;
} else if (serviceConfig.ok) {
  workingServiceConfig = serviceConfig.value;
} else {
  if (this.previousServiceConfig !== null) {
    workingServiceConfig = this.previousServiceConfig;
  } else {
    resultAccepted = false;
    this.handleResolutionFailure(serviceConfig.error);
  }
}

Problem

When a channel has an explicit default service config (grpc.service_config channel option) and the resolver's first result contains valid addresses but an invalid service config, ResolvingLoadBalancer reports a resolution failure instead of falling back to the default service config.

gRFC A21 (Service Config Error Handling) says:

If a default service config is provided, the client should fallback on the default service config (in case of an invalid config with no valid service config received earlier). On the other hand, if there is no default service config provided as a channel argument, the client should fail to start and keep waiting for a valid service config.

So the expected behavior differs only in the case "invalid config, no previous valid config, explicit default configured". The other cases already match A21 and should stay as they are:

• a previously accepted valid config keeps taking priority over the default;
• with no grpc.service_config option, the result should still be treated as a resolution failure.

This is separate from #3116, where resolver errors were explained to be expected transient errors. This report is not about those. It concerns only a service config that fails validation while the addresses are usable, such as the DNS resolver's service-config error when TXT parsing fails. That is the kind of error #1510 (comment by @murgatroid99) said serviceConfigError should be limited to. Handling of address errors and transient lookup failures would not change.

Reproduction steps

Reproduction

1. Check out commit 6741e13.
2. Add the test below as packages/grpc-js/test/test-default-service-config.ts. It registers a resolver that returns one valid endpoint plus an error StatusOr<ServiceConfig>, and creates a ResolvingLoadBalancer with a valid grpc.service_config default. This exercises the balancer directly; I did not verify it with a live RPC.
3. Run npm test (gulp test) in packages/grpc-js.

import * as assert from 'assert';
import { ResolvingLoadBalancer } from '../src/resolving-load-balancer';
import {
  registerResolver,
  ResolverListener,
  Resolver,
  ResolverConstructor,
} from '../src/resolver';
import { ChannelControlHelper } from '../src/load-balancer';
import { Picker } from '../src/picker';
import { ConnectivityState } from '../src/connectivity-state';
import {
  statusOrFromValue,
  statusOrFromError,
} from '../src/call-interface';
import { ServiceConfig } from '../src/service-config';
import { Endpoint, subchannelAddressToString } from '../src/subchannel-address';
import { GrpcUri } from '../src/uri-parser';
import { Status } from '../src/constants';
import { MockSubchannel } from './common';
import { ChannelOptions } from '../src/channel-options';

// Ensure all resolvers and load balancers are registered
import '../src/index';

/**
 * A resolver that returns a valid endpoint list but an invalid/error
 * service config. This simulates a resolver that encounters an error
 * while parsing or fetching the service config.
 */
class InvalidServiceConfigResolver implements Resolver {
  constructor(
    _target: GrpcUri,
    private listener: ResolverListener,
    _channelOptions: ChannelOptions
  ) {}

  updateResolution() {
    setImmediate(() => {
      this.listener(
        statusOrFromValue<Endpoint[]>([
          { addresses: [{ host: 'localhost', port: 50051 }] },
        ]),
        {},
        statusOrFromError<ServiceConfig>({
          code: Status.UNAVAILABLE,
          details: 'Invalid service config from resolver',
        }),
        ''
      );
    });
  }

  destroy() {}

  static getDefaultAuthority(target: GrpcUri): string {
    return target.path;
  }
}

describe('Default Service Config', () => {
  before(() => {
    registerResolver(
      'test-invalid-sc',
      InvalidServiceConfigResolver as unknown as ResolverConstructor
    );
  });

  it('should use default service config when resolver returns invalid service config', function (done) {
    this.timeout(5000);

    const defaultServiceConfig: ServiceConfig = {
      loadBalancingConfig: [],
      methodConfig: [
        {
          name: [{ service: 'TestService', method: 'TestMethod' }],
          waitForReady: true,
        },
      ],
    };

    let subchannels: MockSubchannel[] = [];
    const channelControlHelper: ChannelControlHelper = {
      createSubchannel: (subchannelAddress, subchannelArgs) => {
        const subchannel = new MockSubchannel(
          subchannelAddressToString(subchannelAddress)
        );
        subchannels.push(subchannel);
        return subchannel;
      },
      addChannelzChild: () => {},
      removeChannelzChild: () => {},
      requestReresolution: () => {},
      updateState: (
        state: ConnectivityState,
        picker: Picker,
        errorMessage: string | null
      ) => {},
    };

    const rlb = new ResolvingLoadBalancer(
      { scheme: 'test-invalid-sc', authority: undefined, path: 'test-target' },
      channelControlHelper,
      {
        'grpc.service_config': JSON.stringify(defaultServiceConfig),
      },
      (serviceConfig: ServiceConfig, configSelector) => {
        // onSuccessfulResolution: The default service config should be used.
        try {
          assert.strictEqual(
            serviceConfig.methodConfig.length,
            1,
            'Default service config should have one method config entry'
          );
          assert.strictEqual(
            serviceConfig.methodConfig[0].waitForReady,
            true,
            'waitForReady should be true from default service config'
          );
        } finally {
          rlb.destroy();
        }
        done();
      },
      status => {
        // onFailedResolution: This should NOT be called when a default
        // service config is available.
        rlb.destroy();
        done(
          new Error(
            'Resolution failed, but should have fallen back to default service config. ' +
              `Status: ${status.code} - ${status.details}`
          )
        );
      }
    );

    rlb.exitIdle();
  });
});

Observed:

  1) Default Service Config should use default service config when resolver returns invalid service config:
     Error: Resolution failed, but should have fallen back to default service config. Status: 14 - Invalid service config from resolver
      at ResolvingLoadBalancer.onFailedResolution (packages/grpc-js/test/test-default-service-config.ts:147:11)
      at ResolvingLoadBalancer.handleResolutionFailure (packages/grpc-js/src/resolving-load-balancer.ts:371:12)
      at ResolvingLoadBalancer.handleResolverResult (packages/grpc-js/src/resolving-load-balancer.ts:299:14)

The rest of the suite passed in the same run (505 passing, 9 pending, 1 failing).

Environment

• OS: Linux x64 (Debian 11)
• Node v20.19.6
• Package: @grpc/grpc-js 1.13.4 at commit 6741e13f210f119d4d3da37691dd01055541663b
• I compared current master ca7f83f by blob hash: resolving-load-balancer.ts and resolver-dns.ts are identical to the tested commit. I did not re-run the test on that commit.

Additional context

Already implemented in grpc/grpc-go

PR: https://github.com/grpc/grpc-go/pull/2686 — "service config: default service config" (merged 2019-04-03T17:50:29Z, merge commit ea5e6da287d). It added default service config support to grpc-go, including this fallback: the default is applied when the resolver returns no config or an invalid one, and only if no service config has been applied yet. A21 remains the basis for this report; this PR shows another gRPC implementation handling the same case, and I am only citing the PR, not the current grpc-go code.

How grpc-go fixed it:

--- a/clientconn.go
+++ b/clientconn.go
@@ -457,6 +465,24 @@ func (cc *ClientConn) waitForResolvedAddrs(ctx context.Context) error {
 	}
 }
 
+// gRPC should resort to default service config when:
+// * resolver service config is disabled
+// * or, resolver does not return a service config or returns an invalid one.
+func (cc *ClientConn) fallbackToDefaultServiceConfig(sc string) bool {
+	if cc.dopts.disableServiceConfig {
+		return true
+	}
+	// The logic below is temporary, will be removed once we change the resolver.State ServiceConfig field type.
+	// Right now, we assume that empty service config string means resolver does not return a config.
+	if sc == "" {
+		return true
+	}
+	// TODO: the logic below is temporary. Once we finish the logic to validate service config
+	// in resolver, we will replace the logic below.
+	_, err := parseServiceConfig(sc)
+	return err != nil
+}
+
 func (cc *ClientConn) updateResolverState(s resolver.State) error {
 	cc.mu.Lock()
 	defer cc.mu.Unlock()
...
@@ -467,29 +493,26 @@ func (cc *ClientConn) updateResolverState(s resolver.State) error {
-	if !cc.dopts.disableServiceConfig && cc.scRaw != s.ServiceConfig {
-		// New service config; apply it.
+	if cc.fallbackToDefaultServiceConfig(s.ServiceConfig) {
+		if cc.dopts.defaultServiceConfig != nil && cc.sc == nil {
+			cc.applyServiceConfig(cc.dopts.defaultServiceConfig)
+		}
+	} else {

Suggested fix

Use the default service config in the !serviceConfig.ok && previousServiceConfig === null branch when grpc.service_config was set, as A21 specifies and as grpc-go does. The check reads the channel option because the constructor always fills defaultServiceConfig; without the option the result is still a resolution failure:

diff --git a/packages/grpc-js/src/resolving-load-balancer.ts b/packages/grpc-js/src/resolving-load-balancer.ts
--- a/packages/grpc-js/src/resolving-load-balancer.ts
+++ b/packages/grpc-js/src/resolving-load-balancer.ts
@@ -294,6 +294,8 @@ export class ResolvingLoadBalancer implements LoadBalancer {
     } else {
       if (this.previousServiceConfig !== null) {
         workingServiceConfig = this.previousServiceConfig;
+      } else if (this.channelOptions['grpc.service_config']) {
+        workingServiceConfig = this.defaultServiceConfig;
       } else {
         resultAccepted = false;
         this.handleResolutionFailure(serviceConfig.error);

Verified on 6741e13 (Node.js v22.23.4, mocha 10.8.2): the test above fails without this patch and passes with it; no other test results change.

I can submit a PR for this.

Related: I searched existing issues and PRs ("default service config" invalid, "service config" resolver invalid fallback, previousServiceConfig). The only related item is #1510 (closed, 2020), which was about TXT lookup failures being reported as service-config errors; it was fixed in the DNS resolver and did not change this fallback branch.