Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions xds/src/main/java/io/grpc/xds/XdsServerWrapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -453,14 +453,12 @@ public void onChanged(final LdsUpdate update) {
private boolean ipAddressesMatch(String ldsAddress) {
HostAndPort ldsAddressHnP = HostAndPort.fromString(ldsAddress);
HostAndPort listenerAddressHnP = HostAndPort.fromString(listenerAddress);

InetAddress listenerIp = InetAddresses.forString(listenerAddressHnP.getHost());
InetAddress ldsIp = InetAddresses.forString(ldsAddressHnP.getHost());
if (!ldsAddressHnP.hasPort() || !listenerAddressHnP.hasPort()
|| ldsAddressHnP.getPort() != listenerAddressHnP.getPort()) {
return false;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get problems with the previous way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand Down
45 changes: 41 additions & 4 deletions xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,36 @@ public void run() {
}

@Test
public void onChanged_listenerAddressMismatch()
public void onChanged_listenerIsNull()
throws ExecutionException, InterruptedException, TimeoutException {
xdsServerWrapper = new XdsServerWrapper("10.1.2.3:1", mockBuilder, listener,
selectorManager, new FakeXdsClientPoolFactory(xdsClient),
filterRegistry, executor.getScheduledExecutorService());
final SettableFuture<Server> start = SettableFuture.create();
Executors.newSingleThreadExecutor().execute(new Runnable() {
@Override
public void run() {
try {
start.set(xdsServerWrapper.start());
} catch (Exception ex) {
start.setException(ex);
}
}
});
String ldsResource = xdsClient.ldsResource.get(5, TimeUnit.SECONDS);
assertThat(ldsResource).isEqualTo("grpc/server?udpa.resource.listening_address=10.1.2.3:1");
VirtualHost virtualHost =
VirtualHost.create(
"virtual-host", Collections.singletonList("auth"), new ArrayList<Route>(),
ImmutableMap.<String, FilterConfig>of());

xdsClient.deliverLdsUpdateWithApiListener(0L, Arrays.asList(virtualHost));

verify(listener, timeout(10000)).onNotServing(any());
}

@Test
public void onChanged_listenerAddressMissingPort()
throws ExecutionException, InterruptedException, TimeoutException {
xdsServerWrapper = new XdsServerWrapper("10.1.2.3:1", mockBuilder, listener,
selectorManager, new FakeXdsClientPoolFactory(xdsClient),
Expand Down Expand Up @@ -570,7 +599,7 @@ public void run() {
"filter-chain-foo", createMatch(), httpConnectionManager, createTls(),
mock(TlsContextManager.class));
LdsUpdate listenerUpdate = LdsUpdate.forTcpListener(
Listener.create("listener", "20.3.4.5:1",
Listener.create("listener", "20.3.4.5:",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ImmutableList.copyOf(Collections.singletonList(filterChain)), null, Protocol.TCP));

xdsClient.deliverLdsUpdate(listenerUpdate);
Expand All @@ -579,7 +608,7 @@ public void run() {
}

@Test
public void onChanged_listenerIsNull()
public void onChanged_listenerAddressMismatch()
throws ExecutionException, InterruptedException, TimeoutException {
xdsServerWrapper = new XdsServerWrapper("10.1.2.3:1", mockBuilder, listener,
selectorManager, new FakeXdsClientPoolFactory(xdsClient),
Expand All @@ -601,8 +630,16 @@ public void run() {
VirtualHost.create(
"virtual-host", Collections.singletonList("auth"), new ArrayList<Route>(),
ImmutableMap.<String, FilterConfig>of());
HttpConnectionManager httpConnectionManager = HttpConnectionManager.forVirtualHosts(
0L, Collections.singletonList(virtualHost), new ArrayList<NamedFilterConfig>());
EnvoyServerProtoData.FilterChain filterChain = EnvoyServerProtoData.FilterChain.create(
"filter-chain-foo", createMatch(), httpConnectionManager, createTls(),
mock(TlsContextManager.class));
LdsUpdate listenerUpdate = LdsUpdate.forTcpListener(
Listener.create("listener", "20.3.4.5:1",
ImmutableList.copyOf(Collections.singletonList(filterChain)), null, Protocol.TCP));

xdsClient.deliverLdsUpdateWithApiListener(0L, Arrays.asList(virtualHost));
xdsClient.deliverLdsUpdate(listenerUpdate);

verify(listener, timeout(10000)).onNotServing(any());
}
Expand Down