Skip to content

null guards for xds server and channel #1110

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

Merged
bohhyang merged 2 commits into from
Oct 3, 2025
Merged

null guards for xds server and channel #1110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1dde85
null guards for xds server and channel
bohhyang Oct 2, 2025
4dfb9b6
fix test
bohhyang Oct 2, 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
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ and what APIs have changed, if applicable.

## [Unreleased]

## [29.79.1] - 2025-10-02
- null guards for xds server and channel

## [29.79.0] - 2025-10-01
- Preliminary readiness management

Expand Down Expand Up @@ -5909,7 +5912,8 @@ patch operations can re-use these classes for generating patch messages.

## [0.14.1]

[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.79.0...master
[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.79.1...master
[29.79.1]: https://github.com/linkedin/rest.li/compare/v29.79.0...v29.79.1
[29.79.0]: https://github.com/linkedin/rest.li/compare/v29.78.0...v29.79.0
[29.78.0]: https://github.com/linkedin/rest.li/compare/v29.77.0...v29.78.0
[29.77.0]: https://github.com/linkedin/rest.li/compare/v29.76.0...v29.77.0
Expand Down
25 changes: 23 additions & 2 deletions d2/src/main/java/com/linkedin/d2/balancer/D2ClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static com.google.common.base.Preconditions.*;


/**
* ATTENTION: Using this class MUST be reading from INDIS instead of Zookeeper. ZK read will crash in October 2025.
Expand Down Expand Up @@ -244,8 +246,7 @@ public D2Client build()
);

final LoadBalancerWithFacilitiesFactory loadBalancerFactory = (_config.lbWithFacilitiesFactory == null) ?
new ZKFSLoadBalancerWithFacilitiesFactory() :
_config.lbWithFacilitiesFactory;
new ZKFSLoadBalancerWithFacilitiesFactory() : _config.lbWithFacilitiesFactory;

// log error for not using INDIS in raw d2 client
if (_config.isLiRawD2Client && !loadBalancerFactory.isIndisOnly())
Expand All @@ -261,6 +262,12 @@ public D2Client build()
+ "Using in stack: {}", stackTrace);
}

if (loadBalancerFactory.isIndisOnly() && cfg.xdsServer == null)
{
throw new IllegalStateException("xdsServer is null. Call setXdsServer with a valid indis server address. "
+ "Reference go/onboardindis for guidelines.");
}

LoadBalancerWithFacilities loadBalancer = loadBalancerFactory.create(cfg);

D2Client d2Client = new DynamicClient(loadBalancer, loadBalancer, _restOverStream);
Expand Down Expand Up @@ -352,6 +359,7 @@ public D2ClientBuilder setZkHosts(String zkHosts)

public D2ClientBuilder setXdsServer(String xdsServer)
{
checkNotNull(xdsServer, "xdsServer");
Copy link
Collaborator

@brycezhongqing brycezhongqing Oct 2, 2025

Choose a reason for hiding this comment

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

We don't need this right ? See my other comment we could directly to throw Exception there.

Copy link
Contributor Author

@bohhyang bohhyang Oct 2, 2025

Choose a reason for hiding this comment

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

this is called earlier than when createChannel is called. This can throw as soon as anyone is setting null, instead of wait until XdsChannelFactory actually try to use the null.

Copy link
Collaborator

@brycezhongqing brycezhongqing Oct 2, 2025

Choose a reason for hiding this comment

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

IIRC, without the nullable annotation, the parameter should already be treated as non-null.

Copy link
Contributor Author

@bohhyang bohhyang Oct 2, 2025

Choose a reason for hiding this comment

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

For coding wise, yes, but it won't prevent any user from actually calling it with a null. That's what happened to some espresso-pub users. So we need this explicit null check and throw.

_config.xdsServer = xdsServer;
return this;
}
Expand Down Expand Up @@ -869,6 +877,19 @@ public D2ClientBuilder setActionOnPrecheckFailure(XdsClientValidator.ActionOnPre
return this;
}

/**
* Disable the detection of LI raw D2 client. This is intended for non-LI users who want to use this class to build
* a D2 client. All LI apps/jobs should NEVER set this to true (unless for test apps from service discovery team).
* When hostName and d2JmxManagerPrefix are not set, the app/job will be detected as a LI raw D2 client.
* @param disableDetectLiRawD2Client true to disable the detection, false to enable it.
* @return this builder.
*/
public D2ClientBuilder setDisableDetectLiRawD2Client(boolean disableDetectLiRawD2Client)
{
_config.disableDetectLiRawD2Client = disableDetectLiRawD2Client;
return this;
}

private Map<String, TransportClientFactory> createDefaultTransportClientFactories()
{
final Map<String, TransportClientFactory> clientFactories = new HashMap<>();
Expand Down
2 changes: 2 additions & 0 deletions d2/src/main/java/com/linkedin/d2/xds/XdsClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static com.google.common.base.Preconditions.*;

/**
* Implementation of {@link XdsClient} interface.
Expand Down Expand Up @@ -199,6 +200,7 @@ public XdsClientImpl(Node node,
{
_readyTimeoutMillis = readyTimeoutMillis;
_node = node;
checkNotNull(managedChannel, "managedChannel");
_managedChannel = managedChannel;
_executorService = executorService;
_subscribeToUriGlobCollection = subscribeToUriGlobCollection;
Expand Down
3 changes: 2 additions & 1 deletion d2/src/test/java/com/linkedin/d2/xds/TestXdsClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.linkedin.util.clock.Time;
import indis.XdsD2;
import io.envoyproxy.envoy.service.discovery.v3.Resource;
import io.grpc.ManagedChannel;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -1221,7 +1222,7 @@ private static final class XdsClientImplFixture

_executorService = spy(Executors.newScheduledThreadPool(1));

_xdsClientImpl = spy(new XdsClientImpl(null, null,
_xdsClientImpl = spy(new XdsClientImpl(null, mock(ManagedChannel.class),
_executorService,
0, useGlobCollections, _serverMetricsProvider, useIRV));
_xdsClientImpl._adsStream = _adsStream;
Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version=29.79.0
version=29.79.1
group=com.linkedin.pegasus
org.gradle.configureondemand=true
org.gradle.parallel=true
Expand Down