Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Update InstantiatingGrpcChannelProvider to be tolerant of ipv6 addresses #768

Merged
BenWhitehead merged 4 commits into from
Aug 20, 2019
Merged

Update InstantiatingGrpcChannelProvider to be tolerant of ipv6 addresses #768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81b3cbe
Update InstantiatingGrpcChannelProvider to be tolerant of ipv6 addresses
BenWhitehead Aug 1, 2019
a806232
Remote leftover printf
BenWhitehead Aug 5, 2019
19e552c
Add javadoc to withEndpoint
BenWhitehead Aug 19, 2019
67339e9
Update gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGr…
BenWhitehead Aug 19, 2019
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
12 changes: 10 additions & 2 deletions gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,14 @@ public boolean needsEndpoint() {
return endpoint == null;
}

/**
* Specify the endpoint the channel should connect to.
*
* <p>The value of {@code endpoint} must be of the form {@code host:port}.
*
* @param endpoint The endpoint to connect to
* @return A new {@link InstantiatingGrpcChannelProvider} with the specified endpoint configured
*/
@Override
public TransportChannelProvider withEndpoint(String endpoint) {
validateEndpoint(endpoint);
Expand Down Expand Up @@ -213,7 +221,7 @@ private ManagedChannel createSingleChannel() throws IOException {
GrpcMetadataHandlerInterceptor metadataHandlerInterceptor =
new GrpcMetadataHandlerInterceptor();

int colon = endpoint.indexOf(':');
int colon = endpoint.lastIndexOf(':');
Copy link
Copy Markdown
Contributor

@vam-google vam-google Aug 1, 2019

Choose a reason for hiding this comment

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

It looks like in case if there is no port at all, this code will try to treat the last segment of ipv6 as port number. For example in
2001:db8:85a3:8d3:1319:8a2e:370:7348
7348 becomes a port number.

I understand that it is unlikely case, but please consider handling it (i.e. add proper logic for both ipv4 and ipv6 cases, also colon is porbably a bad name here ans it should clearly indicate that we are searching for a port here).

Copy link
Copy Markdown
Contributor Author

@BenWhitehead BenWhitehead Aug 5, 2019

Choose a reason for hiding this comment

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

The usage of port below (1 and 2) explicitly necessitates a port being provided as part of endpoint since there is no form of default port handling due to not having scheme be part of endpoint. All tests defined in the corresponding test class always specify hostname:port.

I think if we wanted to update this class to support connections without port specified it'd be a lot more work than this.

if (colon < 0) {
throw new IllegalStateException("invalid endpoint - should have been validated: " + endpoint);
}
Expand Down Expand Up @@ -531,7 +539,7 @@ public ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> getChannelConfi
}

private static void validateEndpoint(String endpoint) {
int colon = endpoint.indexOf(':');
int colon = endpoint.lastIndexOf(':');
if (colon < 0) {
throw new IllegalArgumentException(
String.format("invalid endpoint, expecting \"<host>:<port>\""));
Expand Down
17 changes: 17 additions & 0 deletions gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,4 +343,21 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {

provider.getTransportChannel().shutdownNow();
}

@Test
public void testWithIPv6Address() throws IOException {
ScheduledExecutorService executor = new ScheduledThreadPoolExecutor(1);
executor.shutdown();

TransportChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.build()
.withExecutor(executor)
.withHeaders(Collections.<String, String>emptyMap())
.withEndpoint("[::1]:8080");
assertThat(provider.needsEndpoint()).isFalse();

// Make sure we can create channels OK.
provider.getTransportChannel().shutdownNow();
}
}