From a1c41e3d30b7621b0fa626107013597568c189e2 Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Mon, 7 Feb 2022 13:29:09 -0800 Subject: [PATCH] xds/federation: fix percent encoding on server side Overlooked in #8857 on server side. Since `XdsNameResolver.percentEncodePath()` will be also used for server side, I moved the method to `XdsClient`. --- xds/src/main/java/io/grpc/xds/XdsClient.java | 10 ++++++++ .../java/io/grpc/xds/XdsNameResolver.java | 14 +---------- .../java/io/grpc/xds/XdsServerWrapper.java | 3 +-- .../io/grpc/xds/ClientXdsClientDataTest.java | 25 +++++++++++++++++++ .../java/io/grpc/xds/XdsNameResolverTest.java | 25 ------------------- 5 files changed, 37 insertions(+), 40 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsClient.java b/xds/src/main/java/io/grpc/xds/XdsClient.java index ba9c58153a..1c231b83a1 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClient.java +++ b/xds/src/main/java/io/grpc/xds/XdsClient.java @@ -24,6 +24,7 @@ import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; +import com.google.common.net.UrlEscapers; import com.google.common.util.concurrent.ListenableFuture; import com.google.protobuf.Any; import io.grpc.Status; @@ -106,6 +107,15 @@ abstract class XdsClient { return resourceName.replace(rawQuery, canonifiedQuery); } + static String percentEncodePath(String input) { + Iterable pathSegs = Splitter.on('/').split(input); + List encodedSegs = new ArrayList<>(); + for (String pathSeg : pathSegs) { + encodedSegs.add(UrlEscapers.urlPathSegmentEscaper().escape(pathSeg)); + } + return Joiner.on('/').join(encodedSegs); + } + @AutoValue abstract static class LdsUpdate implements ResourceUpdate { // Http level api listener configuration. diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 56b3dbe059..b15af62225 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -22,12 +22,10 @@ import static io.grpc.xds.Bootstrapper.XDSTP_SCHEME; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; -import com.google.common.base.Splitter; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; -import com.google.common.net.UrlEscapers; import com.google.gson.Gson; import com.google.protobuf.util.Durations; import io.grpc.Attributes; @@ -193,7 +191,7 @@ final class XdsNameResolver extends NameResolver { } String replacement = serviceAuthority; if (listenerNameTemplate.startsWith(XDSTP_SCHEME)) { - replacement = percentEncodePath(replacement); + replacement = XdsClient.percentEncodePath(replacement); } String ldsResourceName = expandPercentS(listenerNameTemplate, replacement); if (!XdsClient.isResourceNameValid(ldsResourceName, ResourceType.LDS.typeUrl()) @@ -208,16 +206,6 @@ final class XdsNameResolver extends NameResolver { resolveState.start(); } - @VisibleForTesting - static String percentEncodePath(String input) { - Iterable pathSegs = Splitter.on('/').split(input); - List encodedSegs = new ArrayList<>(); - for (String pathSeg : pathSegs) { - encodedSegs.add(UrlEscapers.urlPathSegmentEscaper().escape(pathSeg)); - } - return Joiner.on('/').join(encodedSegs); - } - private static String expandPercentS(String template, String replacement) { return template.replace("%s", replacement); } diff --git a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java index 0946f62148..58c9692fa6 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java @@ -24,7 +24,6 @@ import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.net.UrlEscapers; import com.google.common.util.concurrent.SettableFuture; import io.grpc.Attributes; import io.grpc.InternalServerInterceptors; @@ -196,7 +195,7 @@ final class XdsServerWrapper extends Server { } String replacement = listenerAddress; if (listenerTemplate.startsWith(XDSTP_SCHEME)) { - replacement = UrlEscapers.urlFragmentEscaper().escape(replacement); + replacement = XdsClient.percentEncodePath(replacement); } discoveryState = new DiscoveryState(listenerTemplate.replaceAll("%s", replacement)); } diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java index c41773dfd9..b195a63eec 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java @@ -2619,6 +2619,31 @@ public class ClientXdsClientDataTest { .isEqualTo(expectedCanonifiedName); } + /** + * Tests compliance with RFC 3986 section 3.3 + * https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 + */ + @Test + public void percentEncodePath() { + String unreserved = "aAzZ09-._~"; + assertThat(XdsClient.percentEncodePath(unreserved)).isEqualTo(unreserved); + + String subDelims = "!$&'(*+,;/="; + assertThat(XdsClient.percentEncodePath(subDelims)).isEqualTo(subDelims); + + String colonAndAt = ":@"; + assertThat(XdsClient.percentEncodePath(colonAndAt)).isEqualTo(colonAndAt); + + String needBeEncoded = "?#[]"; + assertThat(XdsClient.percentEncodePath(needBeEncoded)).isEqualTo("%3F%23%5B%5D"); + + String ipv4 = "0.0.0.0:8080"; + assertThat(XdsClient.percentEncodePath(ipv4)).isEqualTo(ipv4); + + String ipv6 = "[::1]:8080"; + assertThat(XdsClient.percentEncodePath(ipv6)).isEqualTo("%5B::1%5D:8080"); + } + private static Filter buildHttpConnectionManagerFilter(HttpFilter... httpFilters) { return Filter.newBuilder() .setName("envoy.http_connection_manager") diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index d42cd07b1b..a17a043af5 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -1955,31 +1955,6 @@ public class XdsNameResolverTest { .isFalse(); } - /** - * Tests compliance with RFC 3986 section 3.3 - * https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 - */ - @Test - public void percentEncodePath() { - String unreserved = "aAzZ09-._~"; - assertThat(XdsNameResolver.percentEncodePath(unreserved)).isEqualTo(unreserved); - - String subDelims = "!$&'(*+,;/="; - assertThat(XdsNameResolver.percentEncodePath(subDelims)).isEqualTo(subDelims); - - String colonAndAt = ":@"; - assertThat(XdsNameResolver.percentEncodePath(colonAndAt)).isEqualTo(colonAndAt); - - String needBeEncoded = "?#[]"; - assertThat(XdsNameResolver.percentEncodePath(needBeEncoded)).isEqualTo("%3F%23%5B%5D"); - - String ipv4 = "0.0.0.0:8080"; - assertThat(XdsNameResolver.percentEncodePath(ipv4)).isEqualTo(ipv4); - - String ipv6 = "[::1]:8080"; - assertThat(XdsNameResolver.percentEncodePath(ipv6)).isEqualTo("%5B::1%5D:8080"); - } - private final class FakeXdsClientPoolFactory implements XdsClientPoolFactory { Map bootstrap;