Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: listener type validation #11933

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

xds: listener type validation #11933

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4eb625a
XdsNameResolver and XdsServerWrapper checks listener type
shivaspeaks Feb 17, 2025
ad90963
XdsNameResolver and XdsServerWrapper check listener type
shivaspeaks Feb 24, 2025
01927a6
xds listener type validation
shivaspeaks Feb 25, 2025
8780c52
xds listener type validation
shivaspeaks Mar 3, 2025
76270a4
Address comments
shivaspeaks Mar 6, 2025
da0d5ab
Address comments
shivaspeaks Mar 6, 2025
ba8af22
Merge branch 'master' into listener-type-validation
shivaspeaks Mar 6, 2025
694c2f1
Pull from master
shivaspeaks Mar 6, 2025
506bbe4
remove wildcard handling
shivaspeaks Mar 7, 2025
87aad6f
rebase
shivaspeaks Mar 7, 2025
239f167
typo
shivaspeaks Mar 7, 2025
ca5bb14
address comments
shivaspeaks Mar 18, 2025
cb41652
communicate protocol through EnvoyServerData.Address
shivaspeaks Mar 19, 2025
e07158f
Merge branch 'master' into listener-type-validation
shivaspeaks Mar 19, 2025
9f79e8c
resolve conflicts
shivaspeaks Mar 19, 2025
d8fb13a
resolve conflicts
shivaspeaks Mar 19, 2025
6873d59
Final code
shivaspeaks Mar 19, 2025
b3908f1
remove unwanted line
shivaspeaks Mar 19, 2025
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
4 changes: 4 additions & 0 deletions xds/src/main/java/io/grpc/xds/XdsNameResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,10 @@ public void onChanged(final XdsListenerResource.LdsUpdate update) {
}
logger.log(XdsLogLevel.INFO, "Receive LDS resource update: {0}", update);
HttpConnectionManager httpConnectionManager = update.httpConnectionManager();
if (httpConnectionManager == null) {
onResourceDoesNotExist("API Listener: httpConnectionManager");
return;
}
List<VirtualHost> virtualHosts = httpConnectionManager.virtualHosts();
String rdsName = httpConnectionManager.rdsName();
ImmutableList<NamedFilterConfig> filterConfigs = httpConnectionManager.httpFilterConfigs();
Expand Down
37 changes: 33 additions & 4 deletions xds/src/main/java/io/grpc/xds/XdsServerWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.net.HostAndPort;
import com.google.common.net.InetAddresses;
import com.google.common.util.concurrent.SettableFuture;
import io.grpc.Attributes;
import io.grpc.InternalServerInterceptors;
Expand Down Expand Up @@ -57,6 +59,7 @@
import io.grpc.xds.client.XdsClient.ResourceWatcher;
import io.grpc.xds.internal.security.SslContextProviderSupplier;
import java.io.IOException;
import java.net.InetAddress;
import java.net.SocketAddress;
import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -383,7 +386,20 @@ public void onChanged(final LdsUpdate update) {
return;
}
logger.log(Level.FINEST, "Received Lds update {0}", update);
checkNotNull(update.listener(), "update");
if (update.listener() == null) {
onResourceDoesNotExist("Non-API");
return;
}

String ldsAddress = update.listener().address();
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my education: what would be the "best/most appropriate" behaviour if the String ldsAddress is the empty string or null?

Currently, if ldsAddress is null, we skip ipAddressesMatch entirely and proceed with the remaining logic. Would this still be fine?

Likewise, if ldsAddress is the empty string, we check ipAddressesMatch, AFAIK, this would throw an exception and would this be fine? HostAndPort.fromString would return a HostAndPort(host="", port=-1, hasBracketlessColons=false) then InetAddresses.forString would throw an IllegalArgumentException

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. Good point. I couldn't find any mention about this case in gRFC A36.

Copy link
Member

Choose a reason for hiding this comment

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

ldsAddress == null means (based on XdsListenerResource) that the address type was not a SocketAddress, which is covered in the gRFC.

In most NR/LB logic, exceptions cause the channel to go into panic mode. NR/LB should only throw in case of a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Empty address is disallowed in the proto, but is not in our validation so we should add validation. The ip matching logic here should also be checking that it is a TCP listener, which is not being communicated through EnvoyServerData.Address.

And the validation should probably be NACKing (throw ResourceInvalidException) NAMED_PORT as gRPC does not support resolver_name, which is mentioned as being required to use it in its documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we check if it is a TCP listener?

if ("raw_buffer".equals(update.listener().filterChains().get(0).filterChainMatch().transportProtocol())) this could tell us but I'm not very sure about this. I could have pasted the above condition with default filter chain but that is a Nullable field.

Copy link
Member

Choose a reason for hiding this comment

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

This gRPC code is always TCP. So we need to not match when xDS tells us to use UDP.

Copy link
Member

Choose a reason for hiding this comment

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

The transport protocol doesn't seem to be about TCP vs UDP. That's raw_buffer or tls: the higher-level protocol. And that is part of filter chain matches; used to select which filter chain to use. You should never loop through all the matchers and then copy details about it to other matchers.

We were talking about the address, and SocketAddress has a TCP vs UDP protocol.

Copy link
Member Author

@shivaspeaks shivaspeaks Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Got it.

You said we need not match addresses when it's UDP, so I assume we don't call handleConfigNotFoundOrMismatch when it's UDP and continue?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. If you don't call handleConfigNotFoundOrMismatch, that's the same as matching... If !ipAddressesMatch() you call handleConfigNotFoundOrMismatch. Why would you not call it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. I was thinking right at first before editing the comment and then I messed up with thoughts.

if (ldsAddress != null && !ipAddressesMatch(ldsAddress)) {
handleConfigNotFoundOrMismatch(
Status.UNKNOWN.withDescription(
String.format(
"Listener address mismatch: expected %s, but got %s.",
listenerAddress, ldsAddress)).asException());
return;
}
if (!pendingRds.isEmpty()) {
// filter chain state has not yet been applied to filterChainSelectorManager and there
// are two sets of sslContextProviderSuppliers, so we release the old ones.
Expand Down Expand Up @@ -432,15 +448,28 @@ 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 (listenerIp.isAnyLocalAddress()) {
Copy link
Member

Choose a reason for hiding this comment

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

As I said before, remove the wildcard handling. IPv4 and IPv6 wildcards behave differently, and you aren't checking the port here. And if we need to be comparing wildcards, we would really need to spell out how that is done in the gRFC to be consistent cross-language.

return true;
}
return listenerIp.equals(ldsIp) && ldsAddressHnP.getPort() == listenerAddressHnP.getPort();
}

@Override
public void onResourceDoesNotExist(final String resourceName) {
if (stopped) {
return;
}
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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

xdsClient.getBootstrapInfo().node().getId())).asException();
handleConfigNotFound(statusException);
handleConfigNotFoundOrMismatch(statusException);
}

@Override
Expand Down Expand Up @@ -673,7 +702,7 @@ public <ReqT, RespT> Listener<ReqT> interceptCall(ServerCall<ReqT, RespT> call,
};
}

private void handleConfigNotFound(StatusException exception) {
private void handleConfigNotFoundOrMismatch(StatusException exception) {
cleanUpRouteDiscoveryStates();
shutdownActiveFilters();
List<SslContextProviderSupplier> toRelease = getSuppliersInUse();
Expand Down
43 changes: 42 additions & 1 deletion xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,15 @@
import io.grpc.testing.TestMethodDescriptors;
import io.grpc.xds.EnvoyServerProtoData.CidrRange;
import io.grpc.xds.EnvoyServerProtoData.FilterChain;
import io.grpc.xds.EnvoyServerProtoData.FilterChainMatch;
import io.grpc.xds.EnvoyServerProtoData.Listener;
import io.grpc.xds.Filter.FilterConfig;
import io.grpc.xds.Filter.NamedFilterConfig;
import io.grpc.xds.FilterChainMatchingProtocolNegotiators.FilterChainMatchingHandler.FilterChainSelector;
import io.grpc.xds.StatefulFilter.Config;
import io.grpc.xds.VirtualHost.Route;
import io.grpc.xds.VirtualHost.Route.RouteMatch;
import io.grpc.xds.VirtualHost.Route.RouteMatch.PathMatcher;
import io.grpc.xds.XdsListenerResource.LdsUpdate;
import io.grpc.xds.XdsRouteConfigureResource.RdsUpdate;
import io.grpc.xds.XdsServerBuilder.XdsServingStatusListener;
import io.grpc.xds.XdsServerTestHelper.FakeXdsClient;
Expand Down Expand Up @@ -538,6 +539,46 @@ public void run() {
verify(mockServer).start();
}

@Test
public void onChanged_listenerAddressMismatch()
throws ExecutionException, InterruptedException, TimeoutException {

when(mockBuilder.build()).thenReturn(mockServer);
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());
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));
xdsClient.deliverLdsUpdate(listenerUpdate);
verify(listener, timeout(10000)).onNotServing(any());
}

@Test
public void discoverState_rds() throws Exception {
final SettableFuture<Server> start = SettableFuture.create();
Expand Down
Loading