You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/10/09 23:47:05 UTC
[GitHub] [drill] jduo opened a new pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
jduo opened a new pull request #2107:
URL: https://github.com/apache/drill/pull/2107
Also set the TLS SNI property to the host field if the hostname wasn't overriddeen.
# [DRILL-7795](https://issues.apache.org/jira/browse/DRILL-7795): Add support for overriding hostname to C++ client
## Description
Add the property overrideHostname to the C++ client. If this property is not set, still set the TLS SNI property to the hostname being connected to.
## Documentation
The C++ client now sees
## Testing
Using the ODBC Driver, with this custom client substituted in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #2107:
URL: https://github.com/apache/drill/pull/2107
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] sanel commented on a change in pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
sanel commented on a change in pull request #2107:
URL: https://github.com/apache/drill/pull/2107#discussion_r504936707
##########
File path: contrib/native/client/src/clientlib/errmsgs.hpp
##########
@@ -59,7 +59,8 @@ namespace Drill{
#define ERR_CONN_SSL_CN DRILL_ERR_START+27
#define ERR_CONN_SSL_CERTVERIFY DRILL_ERR_START+28
#define ERR_CONN_SSL_PROTOVER DRILL_ERR_START+29
-#define ERR_CONN_MAX DRILL_ERR_START+29
+#define ERR_CONN_SSL_SNI DRILL_ERR_START+30
+#define ERR_CONN_MAX DRILL_ERR_START+30
Review comment:
In that case I'd just add comment for the last entry. Easy to overlook.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jduo commented on a change in pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #2107:
URL: https://github.com/apache/drill/pull/2107#discussion_r504833233
##########
File path: contrib/native/client/src/clientlib/errmsgs.hpp
##########
@@ -59,7 +59,8 @@ namespace Drill{
#define ERR_CONN_SSL_CN DRILL_ERR_START+27
#define ERR_CONN_SSL_CERTVERIFY DRILL_ERR_START+28
#define ERR_CONN_SSL_PROTOVER DRILL_ERR_START+29
-#define ERR_CONN_MAX DRILL_ERR_START+29
+#define ERR_CONN_SSL_SNI DRILL_ERR_START+30
+#define ERR_CONN_MAX DRILL_ERR_START+30
Review comment:
Looking through the history, it looks like ERR_CONN_MAX is always set the value of the highest ERR_CONN code (as in it is an inclusive boundary, not exclusive boundary). So this seems like it should be DRILL_ERR_START+30
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] sanel commented on a change in pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
sanel commented on a change in pull request #2107:
URL: https://github.com/apache/drill/pull/2107#discussion_r504817135
##########
File path: contrib/native/client/src/clientlib/errmsgs.hpp
##########
@@ -59,7 +59,8 @@ namespace Drill{
#define ERR_CONN_SSL_CN DRILL_ERR_START+27
#define ERR_CONN_SSL_CERTVERIFY DRILL_ERR_START+28
#define ERR_CONN_SSL_PROTOVER DRILL_ERR_START+29
-#define ERR_CONN_MAX DRILL_ERR_START+29
+#define ERR_CONN_SSL_SNI DRILL_ERR_START+30
+#define ERR_CONN_MAX DRILL_ERR_START+30
Review comment:
Should it be `DRILL_ERR_START+31`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] sanel commented on a change in pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
sanel commented on a change in pull request #2107:
URL: https://github.com/apache/drill/pull/2107#discussion_r504816572
##########
File path: contrib/native/client/src/clientlib/channel.cpp
##########
@@ -284,7 +287,10 @@ connectionStatus_t Channel::connect(){
<< m_pEndpoint->getHost()
<< ":" << m_pEndpoint->getPort()
<< "." << std::endl;
- ret=this->connectInternal();
+ ret=this->setSocketInformation();
Review comment:
formatting (spaces around `=`)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2107:
URL: https://github.com/apache/drill/pull/2107#issuecomment-718343452
@jduo
Can you please squash commits and we'll merge this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jduo commented on pull request #2107: DRILL-7795: Add support for overriding hostname to C++ client
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #2107:
URL: https://github.com/apache/drill/pull/2107#issuecomment-718478087
> @jduo
> Can you please squash commits and we'll merge this.
Done @cgivre .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org