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

Jump to bottom

xds: Add server features support to Bootstrapper #7271

Merged
merged 7 commits into from
Jul 31, 2020
Merged

xds: Add server features support to Bootstrapper #7271

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b1b9d9
xds: Add server features support to Bootstrapper
dapengzhang0 Jul 30, 2020
fe503de
refactor XdsChannelFactor return type
dapengzhang0 Jul 30, 2020
cef5941
expose only proto from ServerInfo
dapengzhang0 Jul 30, 2020
d8a8d70
read env
dapengzhang0 Jul 30, 2020
c700e81
return list of String for server_features
dapengzhang0 Jul 30, 2020
f4de628
parse boolean
dapengzhang0 Jul 31, 2020
f592405
format
dapengzhang0 Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 16 additions & 3 deletions xds/src/main/java/io/grpc/xds/Bootstrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public abstract class Bootstrapper {
@VisibleForTesting
static final String CLIENT_FEATURE_DISABLE_OVERPROVISIONING =
"envoy.lb.does_not_support_overprovisioning";
static final String XDS_V3_SERVER_FEATURE = "xds_v3";

private static final Bootstrapper DEFAULT_INSTANCE = new Bootstrapper() {
@Override
Expand Down Expand Up @@ -111,7 +112,11 @@ static BootstrapInfo parseConfig(String rawData) throws IOException {
channelCredsOptions.add(creds);
}
}
servers.add(new ServerInfo(serverUri, channelCredsOptions));
List<?> serverFeatures = JsonUtil.getList(serverConfig, "server_features");
if (serverFeatures != null && serverFeatures.contains(XDS_V3_SERVER_FEATURE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the format of server_features is undefined. Maybe it's better to just parse it to a String list.

The parser should not try to interpret what is being parsed. It's just a list of string. "xds_v3" should be defined in the consumer class, which is XdsChannelFactory (e.g., String PROTOCOL_VERSION = "xds_v3). The channel factory looks up the parsed server features and check if the expected protocol version setting is there.

Copy link
Contributor

Choose a reason for hiding this comment

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

The log here should just print out what is parsed literally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that the format of server_features is undefined. Maybe it's better to just parse it to a String list.

What do you mean actually? The following?

List<String> serverFeatures = JsonUtil.getListOfStrings(serverConfig, "server_features");

Or just pick all string type items in the list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, just use JsonUtil.getListOfStrings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

logger.log(XdsLogLevel.INFO, "Server feature: {0}", XDS_V3_SERVER_FEATURE);
}
servers.add(new ServerInfo(serverUri, channelCredsOptions, serverFeatures));
}

Node.Builder nodeBuilder = Node.newBuilder();
Expand Down Expand Up @@ -196,11 +201,14 @@ String getType() {
static class ServerInfo {
private final String serverUri;
private final List<ChannelCreds> channelCredsList;
@Nullable
private final List<?> serverFeatures;

@VisibleForTesting
ServerInfo(String serverUri, List<ChannelCreds> channelCredsList) {
ServerInfo(String serverUri, List<ChannelCreds> channelCredsList, List<?> serverFeatures) {
this.serverUri = serverUri;
this.channelCredsList = channelCredsList;
this.serverFeatures = serverFeatures;
}

String getServerUri() {
Expand All @@ -210,6 +218,12 @@ String getServerUri() {
List<ChannelCreds> getChannelCredentials() {
return Collections.unmodifiableList(channelCredsList);
}

List<?> getServerFeatures() {
return serverFeatures == null
? Collections.emptyList()
: Collections.unmodifiableList(serverFeatures);
}
}

/**
Expand Down Expand Up @@ -240,6 +254,5 @@ List<ServerInfo> getServers() {
public Node getNode() {
return node;
}

}
}
16 changes: 16 additions & 0 deletions xds/src/main/java/io/grpc/xds/XdsClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ public synchronized XdsClient returnObject(Object object) {
*/
abstract static class XdsChannelFactory {
private static final XdsChannelFactory DEFAULT_INSTANCE = new XdsChannelFactory() {
List<?> serverFeatures;

/**
* Creates a channel to the first server in the given list.
Expand All @@ -611,6 +612,7 @@ ManagedChannel createChannel(List<ServerInfo> servers) {
checkArgument(!servers.isEmpty(), "No management server provided.");
XdsLogger logger = XdsLogger.withPrefix("xds-client-channel-factory");
ServerInfo serverInfo = servers.get(0);
serverFeatures = serverInfo.getServerFeatures();
String serverUri = serverInfo.getServerUri();
logger.log(XdsLogLevel.INFO, "Creating channel to {0}", serverUri);
List<ChannelCreds> channelCredsList = serverInfo.getChannelCredentials();
Expand All @@ -633,6 +635,11 @@ ManagedChannel createChannel(List<ServerInfo> servers) {
.keepAliveTime(5, TimeUnit.MINUTES)
.build();
}

@Override
List<?> getSelectedServerFeatures() {
return serverFeatures;
}
};

static XdsChannelFactory getInstance() {
Expand All @@ -643,5 +650,14 @@ static XdsChannelFactory getInstance() {
* Creates a channel to one of the provided management servers.
*/
abstract ManagedChannel createChannel(List<ServerInfo> servers);

/**
* Gets features of the server that the channel is created for. Value is only available
* after {@link #createChannel} is called.
*/
@Nullable
List<?> getSelectedServerFeatures() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't make sense to put such an API here. This makes a factory class stateful.

Maybe it is time to eliminate XdsChannelFactory, instead we do need something like Helper.createResolvingOobChannel(), but accessible inside NameResolver.

Last time when I was doing some local e2e test for traffic splitting and routing, the gRPC channel is using plaintext, but the xDS channel is fixed to use TLS (if not google_default) instead of falling back to use the same configuration as its parent channel. This is broken for Java. It is not bitting us because we are always using TLS if not google_default in production.

We should support such a feature correctly. Basically, the creation of xDS channel will be done outside XdsClientImpl. For EDS only's code path, we are roughly able to do it with Helper.createResolvingOobChannel(), so not a problem for migrating.

For now, I think a better way is to change the createChannel API for XdsChannelFactory. You could define a simple class that contains a Channel and server features.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is coming from the factory or whatever structure being used for XdsClientIml consumes a list of server_urls. When you create the channel from it, you must know the index of the server_url that is selected, so that you can know the corresponding server feature. If it's only a single server_url, there is no problem at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

A workaround is we pass just one ServerInfo instead of List<ServerInfo> to XdsClientImpl as well as XdsChannelFactory. Because we are actually not supporting multiple management server yet, and the current XdsChannelFactory has a hack to pick the hard coded first ServerInfo in the list.

Copy link
Contributor

@voidzcy voidzcy Jul 30, 2020 •
edited

Choose a reason for hiding this comment

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

This is what I mean:

class XdsChannel {
  ManagedChannel channel;
  boolean useProtocolV3;  // or something similar if you have a better idea
}

XdsChannel createChannel(List<ServerInfo> servers) {
  ...
}

The output of creating channel contains the server feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also thought about that, which introduces yet another class and I had thought you wouldn't like it. I'm totally okay with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

:( Why would you think introducing a new class is bad? 😢 This looks fairly natural with channel and protocol feature encapsulated together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

return null;
}
}
}
4 changes: 4 additions & 0 deletions xds/src/test/java/io/grpc/xds/BootstrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ public void parseBootstrap_validData_multipleXdsServers() throws IOException {
+ " \"server_uri\": \"trafficdirector-foo.googleapis.com:443\",\n"
+ " \"channel_creds\": [\n"
+ " {\"type\": \"tls\"}, {\"type\": \"loas\"}, {\"type\": \"google_default\"}\n"
+ " ],\n"
+ " \"server_features\": [\n"
+ " \"xds_v3\", {\"foo\": \"bar\"}\n"
+ " ]\n"
+ " },\n"
+ " {\n"
Expand All @@ -134,6 +137,7 @@ public void parseBootstrap_validData_multipleXdsServers() throws IOException {
assertThat(serverInfoList.get(0).getChannelCredentials().get(2).getType())
.isEqualTo("google_default");
assertThat(serverInfoList.get(0).getChannelCredentials().get(2).getConfig()).isNull();
assertThat(serverInfoList.get(0).getServerFeatures()).contains("xds_v3");
assertThat(serverInfoList.get(1).getServerUri())
.isEqualTo("trafficdirector-bar.googleapis.com:443");
assertThat(serverInfoList.get(1).getChannelCredentials()).isEmpty();
Expand Down
5 changes: 2 additions & 3 deletions xds/src/test/java/io/grpc/xds/EdsLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,8 @@ public StreamObserver<DiscoveryRequest> streamAggregatedResources(
.forName(serverName)
.directExecutor()
.build());
final List<ServerInfo> serverList =
ImmutableList.of(
new ServerInfo("trafficdirector.googleapis.com", ImmutableList.<ChannelCreds>of()));
final List<ServerInfo> serverList = ImmutableList.of(
new ServerInfo("trafficdirector.googleapis.com", ImmutableList.<ChannelCreds>of(), null));
Node node = Node.newBuilder().build();
BootstrapInfo bootstrapInfo = new BootstrapInfo(serverList, node);
doReturn(bootstrapInfo).when(bootstrapper).readBootstrap();
Expand Down
2 changes: 1 addition & 1 deletion xds/src/test/java/io/grpc/xds/XdsClientImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ public void cancelled(Context context) {
cleanupRule.register(InProcessChannelBuilder.forName(serverName).directExecutor().build());

List<ServerInfo> servers =
ImmutableList.of(new ServerInfo(serverName, ImmutableList.<ChannelCreds>of()));
ImmutableList.of(new ServerInfo(serverName, ImmutableList.<ChannelCreds>of(), null));
XdsChannelFactory channelFactory = new XdsChannelFactory() {
@Override
ManagedChannel createChannel(List<ServerInfo> servers) {
Expand Down
2 changes: 1 addition & 1 deletion xds/src/test/java/io/grpc/xds/XdsClientImplTestForListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ public void cancelled(Context context) {
cleanupRule.register(InProcessChannelBuilder.forName(serverName).directExecutor().build());

List<ServerInfo> servers =
ImmutableList.of(new ServerInfo(serverName, ImmutableList.<ChannelCreds>of()));
ImmutableList.of(new ServerInfo(serverName, ImmutableList.<ChannelCreds>of(), null));
XdsChannelFactory channelFactory = new XdsChannelFactory() {
@Override
ManagedChannel createChannel(List<ServerInfo> servers) {
Expand Down
18 changes: 18 additions & 0 deletions xds/src/test/java/io/grpc/xds/XdsClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;

import com.google.common.collect.ImmutableList;
import io.grpc.ManagedChannel;
import io.grpc.xds.Bootstrapper.ChannelCreds;
import io.grpc.xds.Bootstrapper.ServerInfo;
import io.grpc.xds.XdsClient.RefCountedXdsClientObjectPool;
import io.grpc.xds.XdsClient.XdsChannelFactory;
import io.grpc.xds.XdsClient.XdsClientFactory;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -102,4 +107,17 @@ XdsClient createXdsClient() {
XdsClient xdsClient2 = xdsClientPool.getObject();
assertThat(xdsClient2).isNotSameInstanceAs(xdsClient1);
}

@Test
public void defaultChannelFactorySelectsServerFeatures() {
XdsChannelFactory xdsChannelFactory = XdsChannelFactory.getInstance();
ManagedChannel channel = xdsChannelFactory.createChannel(
ImmutableList.of(
new ServerInfo(
"xdsserver.com", ImmutableList.<ChannelCreds>of(), ImmutableList.of("foo", "bar")),
new ServerInfo(
"xdsserver2.com", ImmutableList.<ChannelCreds>of(), ImmutableList.of("baz"))));
channel.shutdown();
assertThat(xdsChannelFactory.getSelectedServerFeatures()).containsExactly("foo", "bar");
}
}
4 changes: 1 addition & 3 deletions xds/src/test/java/io/grpc/xds/XdsNameResolverIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,7 @@ ManagedChannel createChannel(List<ServerInfo> servers) {
@Override
public BootstrapInfo readBootstrap() {
List<ServerInfo> serverList =
ImmutableList.of(
new ServerInfo(serverName,
ImmutableList.<ChannelCreds>of()));
ImmutableList.of(new ServerInfo(serverName, ImmutableList.<ChannelCreds>of(), null));
return new BootstrapInfo(serverList, FAKE_BOOTSTRAP_NODE);
}
};
Expand Down