XdsNameResolver and XdsServerWrapper don't check listener type

[ejona86]

Our resource parsing supports two listener types (I'm not confident enough validation is happening here):

grpc-java/xds/src/main/java/io/grpc/xds/XdsListenerResource.java

Lines 110 to 114 in 210f9c0

	if (listener.hasApiListener()) {
	return processClientSideListener(listener, args);
	} else {
	return processServerSideListener(listener, args);
	}

But XdsNameResolver and XdsServerWrapper don't check that they got the right type. No changes were made to XdsNameResolver when non-API listener type was allowed, so I doubt the logic exists but I'm just not seeing it.

I also don't see XdsServerWrapper call address(), so it appears it is not follow gRFC A36.

The XdsServer must be "not serving" if the address does not match.

[ejona86]

Looks like the client receiving a socket listener will throw a NullPointerException, because httpConnectionManager() is null and instead listener() is set:

grpc-java/xds/src/main/java/io/grpc/xds/XdsNameResolver.java

Lines 661 to 663 in e8ff6da

	HttpConnectionManager httpConnectionManager = update.httpConnectionManager();
	List<VirtualHost> virtualHosts = httpConnectionManager.virtualHosts();
	String rdsName = httpConnectionManager.rdsName();

[shivaspeaks]

Looks like the client receiving a socket listener will throw a NullPointerException

But SocketListener is a non-api server side listener and it will never be used in XdsNameResolver! Isn't it so? XdsListenerResouce validates a check for non-api listener type.
Oh so you mean this is not guaranteed/enforced here in XdsNameResolver! Then we need to do a validation in XdsNameResolver and in XdsServerWrapper.

[ejona86]

Yes, it isn't guaranteed. We expect a failure but we shouldn't trigger a NullPointerException.

[shivaspeaks]

For the second part of this issue that it does not seem to follow gRFC A36. A simple conditional check with listenerAddress seems good enough. I believe we don't want to throw Exception from here, just marking it as not serving and logging the warning. Something like this in onChanged(LdsUpdate):

String ldsAddress = update.listener().address();
      if (!listenerAddress.equals(ldsAddress)) {
        logger.warning(
            String.format(
                "Listener address mismatch: expected %s, but got %s. Transitioning to NOT_SERVING.",
                listenerAddress, ldsAddress));
        listener.onNotServing(
            Status.UNKNOWN.withDescription(
                String.format(
                    "Listener address mismatch: expected %s, but got %s.",
                    listenerAddress, ldsAddress)).asRuntimeException());
      }

[ejona86]

In general, it is really rare to pass an error along and log a warning. There can be some times in our server code because passing the error along is only ever seen by metrics. But here, the default listener logs the error so you'd be double-logging. Don't log directly. I'll note you also need to make sure the server is not serving. It looks like handleConfigNotFound() would be the method to look at for how to do that.