Update InstantiatingGrpcChannelProvider to be tolerant of ipv6 addresses#768
Update InstantiatingGrpcChannelProvider to be tolerant of ipv6 addresses#768BenWhitehead merged 4 commits intogoogleapis:masterfrom BenWhitehead:ipv6-fix
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #768 +/- ##
============================================
+ Coverage 78.15% 78.38% +0.23%
- Complexity 1101 1106 +5
============================================
Files 198 198
Lines 4833 4886 +53
Branches 383 385 +2
============================================
+ Hits 3777 3830 +53
- Misses 886 887 +1
+ Partials 170 169 -1
Continue to review full report at Codecov.
|
chingor13
left a comment
chingor13
left a comment
There was a problem hiding this comment.
Is it possible to set the endpoint to an ipv6 ip address without a port?
chingor13
requested review from
vam-google
and removed request for
garrettjonesgoogle
| new GrpcMetadataHandlerInterceptor(); | ||
|
|
||
| int colon = endpoint.indexOf(':'); | ||
| int colon = endpoint.lastIndexOf(':'); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
It is currently not possible to pass in an endpoint without a port defined. In fact, there is an existing test |
|
@chingor13 Can you review this again? |
chingor13
left a comment
chingor13
left a comment
There was a problem hiding this comment.
Can we add documentation that the endpoint must follow [host]:[port]? We can add similar documentation to the google-cloud-core options that expose endpoint override.
We should also still investigate ManagedChannelBuilder.forTarget(String) but that can be done separately.
|
@chingor13 I've added the requested javadocs. If everything looks good, I'll rebase-squash this and we can merge it afterward. |
…pcChannelProvider.java Co-Authored-By: Jeff Ching <[email protected]>
4 participants
No description provided.