xds: listener type validation #11933

shivaspeaks · 2025-03-03T07:24:13Z

Fixes #11737

xds/src/test/java/io/grpc/xds/XdsServerTestHelper.java

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

ejona86

Sorry, I didn't mean to approve

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

danielzhaotongliu · 2025-03-07T00:09:22Z

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

      }
      StatusException statusException = Status.UNAVAILABLE.withDescription(
-          String.format("Listener %s unavailable, xDS node ID: %s", resourceName,
+          String.format("%s listener unavailable, xDS node ID: %s", resourceName,


nit: what would be the reason to switch the order of the error format here? I think Listener %s is slightly more common in the code base. Consistent formatting helps with searching when debugging issues.

ejona86

I found these comments laying around. I don't know why I didn't send it out earlier.

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

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

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

larry-safran · 2025-03-20T09:23:21Z

While it is in the gRFC, why is it really important to enforce a returned address being non-null? This doesn't seem to really provide value as the address was already known for communicating to the xds server in the first place.

ejona86 · 2025-03-24T15:14:03Z

This doesn't seem to really provide value as the address was already known for communicating to the xds server in the first place.

The returned Listener address tells the server what to listen on. We didn't remove it. We just made it so that the control plane can have a trivial job of filling in the correct value.

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java

kannanjgithub

Review comments.

kannanjgithub · 2025-03-25T09:57:00Z

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

-      SocketAddress socketAddress = proto.getAddress().getSocketAddress();
+      socketAddress = proto.getAddress().getSocketAddress();
      address = socketAddress.getAddress();
+      if (address.isEmpty()) {


Add unit test in GrpcXdsClientImplDataTest.

kannanjgithub · 2025-03-25T09:57:18Z

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

        case NAMED_PORT:
-          address = address + ":" + socketAddress.getNamedPort();
-          break;
+          throw new ResourceInvalidException("NAMED_PORT is not supported in gRPC.");


Add unit test in GrpcXdsClientImplDataTest.

kannanjgithub · 2025-03-25T09:59:06Z

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

      // Process Route
      XdsConfig update = updateOrStatus.getValue();
      HttpConnectionManager httpConnectionManager = update.getListener().httpConnectionManager();
+      if (httpConnectionManager == null) {


Add unit test for when the listener update is missing httpConnectionManager.

kannanjgithub · 2025-03-25T10:00:26Z

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

      }
      logger.log(Level.FINEST, "Received Lds update {0}", update);
-      checkNotNull(update.listener(), "update");
+      if (update.listener() == null) {


Add unit test for this case as well.

danielzhaotongliu · 2025-03-26T18:18:35Z

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

      XdsConfig update = updateOrStatus.getValue();
      HttpConnectionManager httpConnectionManager = update.getListener().httpConnectionManager();
+      if (httpConnectionManager == null) {
+        String error = "API Listener: httpConnectionManager does not exist.";


nit: this error string is never reused, thus no need to store it in a local variable.
We can simply rewrite as
logger.log(XdsLogLevel.INFO, "API Listener: httpConnectionManager does not exist.");

danielzhaotongliu · 2025-03-26T19:53:48Z

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

+      if (!ldsAddressHnP.hasPort() || !listenerAddressHnP.hasPort()
+          || ldsAddressHnP.getPort() != listenerAddressHnP.getPort()) {
+        return false;
+      }


Should we have a unit test for this if block?

The tests test hostname mismatch and port mismatch but not missing host or missing port. Like "127.0.0.0" or ":8080"

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java

kannanjgithub · 2025-04-02T11:22:08Z

xds/src/test/java/io/grpc/xds/XdsServerTestHelper.java

        "filter-chain-bar", defaultFilterChainMatch, httpConnectionManager,
        tlsContextForDefaultFilterChain, tlsContextManager);
-    EnvoyServerProtoData.Listener listener = EnvoyServerProtoData.Listener.create(
+    return Listener.create(


Previously was better?

kannanjgithub · 2025-04-02T11:43:33Z

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java

+        Listener.create("listener", "20.3.4.5:1",
+            ImmutableList.copyOf(Collections.singletonList(filterChain)), null, Protocol.TCP));
+    xdsClient.deliverLdsUpdate(listenerUpdate);
+    verify(listener, timeout(10000)).onNotServing(any());


Follow http://go/unit-test-practices#structure

kannanjgithub

Review comments.

kannanjgithub · 2025-04-02T13:50:22Z

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java

        mock(TlsContextManager.class));
    LdsUpdate listenerUpdate = LdsUpdate.forTcpListener(
-        Listener.create("listener", "20.3.4.5:1",
+        Listener.create("listener", "20.3.4.5:",


Does it work if you commit the trailing ':' ?
Does the InetAddress parsing work for an address with just the :8080 missing the hostname part?

Does it work if you commit the trailing ':' ?

For this test case, NO because even if I give same port it will fail at hostname matching, ldsHostname(20.3.4.5) is not same as listenerAddressHostname(10.1.2.3).

Does the InetAddress parsing work for an address with just the :8080 missing the hostname part?

No it doesn't work without hostname. It fails when we convert to InetAddress using InetAddresses.forString("") using empty string as hostname.

kannanjgithub · 2025-04-02T13:50:42Z

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

+      InetAddress listenerIp = InetAddresses.forString(listenerAddressHnP.getHost());
+      InetAddress ldsIp = InetAddresses.forString(ldsAddressHnP.getHost());
      return listenerIp.equals(ldsIp);
    }


Did you get problems with the previous way?

No, there were no problems here but I think if port isn't available or ports are not same then there's no point of parsing HostAndPort into InetAddress

kannanjgithub

Clarification questions

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

kannanjgithub

Question.

danielzhaotongliu

LGTM.

danielzhaotongliu · 2025-04-02T18:45:49Z

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java

  /**
   * Returns the least-specific match-all Filter Chain Match.
   */
-  private static FilterChainMatch createMatch() {


nit: what would be the reason for increasing this visibility of this method (and createTls()), I don't seee them being referenced in other test files? Perhaps I am missing something.

Generally, it is good practice to keep the visibility of members as inaccessible (lowest visibility) as possible.

shivaspeaks added 4 commits February 17, 2025 16:48

XdsNameResolver and XdsServerWrapper checks listener type

4eb625a

XdsNameResolver and XdsServerWrapper check listener type

ad90963

xds listener type validation

01927a6

xds listener type validation

8780c52

shivaspeaks changed the title ~~Listener type validation~~ xds: listener type validation Mar 3, 2025

shivaspeaks requested a review from ejona86 March 3, 2025 15:25

ejona86 approved these changes Mar 5, 2025

View reviewed changes

xds/src/test/java/io/grpc/xds/XdsServerTestHelper.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Outdated Show resolved Hide resolved

ejona86 self-requested a review March 5, 2025 21:20

ejona86 reviewed Mar 5, 2025

View reviewed changes

Address comments

76270a4

shivaspeaks added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 6, 2025

shivaspeaks and others added 2 commits March 6, 2025 20:44

Address comments

da0d5ab

Merge branch 'master' into listener-type-validation

ba8af22

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 6, 2025

shivaspeaks requested a review from ejona86 March 6, 2025 20:52

ejona86 reviewed Mar 6, 2025

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Outdated Show resolved Hide resolved

Pull from master

694c2f1

danielzhaotongliu reviewed Mar 7, 2025

View reviewed changes

shivaspeaks added 3 commits March 7, 2025 14:42

remove wildcard handling

506bbe4

rebase

87aad6f

typo

239f167

shivaspeaks requested review from danielzhaotongliu and ejona86 March 7, 2025 18:34

shivaspeaks requested review from kannanjgithub and larry-safran March 17, 2025 08:41

ejona86 reviewed Mar 17, 2025

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Show resolved Hide resolved

address comments

ca5bb14

shivaspeaks requested a review from ejona86 March 18, 2025 17:03

ejona86 reviewed Mar 18, 2025

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsListenerResource.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsListenerResource.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Show resolved Hide resolved

communicate protocol through EnvoyServerData.Address

cb41652

ejona86 reviewed Mar 19, 2025

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Outdated Show resolved Hide resolved

Final code

6873d59

shivaspeaks requested a review from ejona86 March 19, 2025 18:07

remove unwanted line

b3908f1

ejona86 reviewed Mar 24, 2025

View reviewed changes

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java Outdated Show resolved Hide resolved

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java Show resolved Hide resolved

ejona86 approved these changes Mar 24, 2025

View reviewed changes

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java Show resolved Hide resolved

address comment

4dc1039

kannanjgithub requested changes Mar 25, 2025

View reviewed changes

danielzhaotongliu reviewed Mar 26, 2025

View reviewed changes

Add error cases in unit tests

3a36c2d

shivaspeaks requested a review from kannanjgithub March 28, 2025 11:58

kannanjgithub reviewed Apr 2, 2025

View reviewed changes

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java Show resolved Hide resolved

kannanjgithub reviewed Apr 2, 2025

View reviewed changes

kannanjgithub requested changes Apr 2, 2025

View reviewed changes

Fix unit tests

f82d899

shivaspeaks requested a review from kannanjgithub April 2, 2025 12:40

Add more unit test

eee4ea9

kannanjgithub reviewed Apr 2, 2025

View reviewed changes

kannanjgithub requested changes Apr 2, 2025

View reviewed changes

kannanjgithub reviewed Apr 2, 2025

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java Show resolved Hide resolved

kannanjgithub requested changes Apr 2, 2025

View reviewed changes

kannanjgithub approved these changes Apr 3, 2025

View reviewed changes

shivaspeaks merged commit c8d1e6e into grpc:master Apr 3, 2025
16 checks passed

shivaspeaks deleted the listener-type-validation branch April 3, 2025 05:52

danielzhaotongliu reviewed Apr 3, 2025

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Jul 3, 2025

xds: listener type validation #11933

xds: listener type validation #11933

Uh oh!

Conversation

shivaspeaks commented Mar 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larry-safran commented Mar 20, 2025

Uh oh!

ejona86 commented Mar 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielzhaotongliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees