`CompositeGrpcChannelFactory` identifies default inprocess target as supported by factories other than `InProcessGrpcChannelFactory`

[cengdall-blaze]

As part of identifying the appropriate factory to use for a given target, CompositeGrpcChannelFactory finds the first factory for which supports(target) returns true.

@Override
	public ManagedChannel createChannel(final String target, ChannelBuilderOptions options) {
		return this.channelFactories.stream()
			.filter((cf) -> cf.supports(target))
			.findFirst()
			.orElseThrow(
					() -> new IllegalStateException("No grpc channel factory found that supports target : " + target))
			.createChannel(target, options);
	}

When the target is the default channel configured via application properties, target in the above method will evaluate to default. If the default channel address has been prefixed with in-process, the above code incorrectly identifies target as being supported by factories other than InProcessGrpcChannelFactory.

grpc:
    client:
      default-channel:
        address: in-process:test

As a result, the createChannel call ultimately fails with an IllegalArgumentException due to having an invalid DNS name.

Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [java.lang.Object]: Factory method 'getClient' threw exception with message: Invalid DNS name: in-process:test
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.lambda$instantiate$0(SimpleInstantiationStrategy.java:183)
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiateWithFactoryMethod(SimpleInstantiationStrategy.java:72)
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:152)
	at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:653)
	... 39 more
Caused by: java.lang.IllegalArgumentException: Invalid DNS name: in-process:test
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:217)
	at io.grpc.internal.DnsNameResolver.<init>(DnsNameResolver.java:168)
	at io.grpc.internal.DnsNameResolverProvider.newNameResolver(DnsNameResolverProvider.java:64)
	at io.grpc.internal.ManagedChannelImpl.getNameResolver(ManagedChannelImpl.java:682)
	at io.grpc.internal.ManagedChannelImpl.<init>(ManagedChannelImpl.java:605)
	at io.grpc.internal.ManagedChannelImplBuilder.build(ManagedChannelImplBuilder.java:731)
	at io.grpc.ForwardingChannelBuilder2.build(ForwardingChannelBuilder2.java:278)
	at org.springframework.grpc.client.DefaultGrpcChannelFactory.createChannel(DefaultGrpcChannelFactory.java:120)

[dsyer]

I'm sure you are right, but I'm struggling a bit to extract the logic of your argument. It looks on the face of it as though the filter in the composite channel factory exlcudes everything except the inprocess factory. Can you provide a simple sample that demonstrates the problem directly (a public repo in github would be great)?

[cengdall-blaze]

The TLDR is that the filter is getting supplied what I'm guessing is the name of the target channel, which is set to default when using the grpc.client.default-channel.address property. I didn't dig into the other code paths, but presumably when I set an address via grpc.client.<channel>.address the filter is correctly getting supplied the address (eg: in-process:<name>>.

Because the filter is looking for target to be prefixed or not prefixed with in-process:, the value of default passes the filter and the wrong factory is used to try and create the channel. Actual channel creation in DefaultGrpcChannelFactory looks up the address for the given target using this.targets.getTarget(target).

[dsyer]

I think the logic is a bit convoluted internally so it's hard to analyse - we had to do some acrobatics to support in-process channels as the default in tests, but require something explicit when not. So I'm sure the behaviour you describe is possible but I really need a sample to see how it arises in practice.

[cengdall-blaze]

I can try and push something when I find time, but I think it should be possible to reproduce by just setting grpc.client.default-channel.address to an inprocess address. I suspect that's not really something anyone is doing, and even my uncovering of this bug was due to my experimenting with capabilities (I'm in the process of converting from grpc-spring).

[mkleine]

Hi there, we're using the in-process feature in production and have worked around this issue by using this bean definition

  @ConditionalOnProperty(prefix = "spring.grpc.client.inprocess", name = "enabled", havingValue = "true",
          matchIfMissing = true)
  @Bean
  @Order(Ordered.HIGHEST_PRECEDENCE) // Ensure this bean comes first when iterating over the gRPC channel factory beans
  InProcessGrpcChannelFactoryWrapper inProcessGrpcChannelFactoryWrapper(InProcessGrpcChannelFactory inProcessGrpcChannelFactory,
                                                                        GrpcClientProperties grpcClientProperties) {
    return new InProcessGrpcChannelFactoryWrapper(inProcessGrpcChannelFactory, grpcClientProperties);
  }

along with this implementation class:

/**
 * A wrapper for {@link InProcessGrpcChannelFactory} that uses the target from
 * {@link GrpcClientProperties} to determine if it should create an in-process channel.
 */
public class InProcessGrpcChannelFactoryWrapper implements GrpcChannelFactory {

  private final GrpcClientProperties grpcClientProperties;
  private final InProcessGrpcChannelFactory delegate;

  public InProcessGrpcChannelFactoryWrapper(InProcessGrpcChannelFactory delegate,
                                            GrpcClientProperties grpcClientProperties) {
    this.delegate = delegate;
    this.grpcClientProperties = grpcClientProperties;
  }

  @Override
  public boolean supports(String target) {
    return grpcClientProperties.getTarget(target).startsWith("in-process:");
  }

  @Override
  public ManagedChannel createChannel(String target, ChannelBuilderOptions options) {
    return delegate.createChannel(grpcClientProperties.getTarget(target), options);
  }

}