One more requirement surfaced in the scope of introducing the above mentioned improvements - the need for SQL Gateway requests to be authenticated. I propose to introduce an environmental variable, FLINK_REST_CLIENT_HEADERS that could contain a list of HTTPS headers ( including authentication cookies) to be passed along with each RestClient request.
Any objections or alternative suggestions? Best, Alex On Mon, 8 May 2023 at 20:14, Alexander Fedulov <alexander.fedu...@gmail.com> wrote: > Hi Thomas, > > Thanks for the feedback. I created two separate tickets to track the > related work: > https://issues.apache.org/jira/browse/FLINK-32030 > https://issues.apache.org/jira/browse/FLINK-32035 > > Best, > Alex Fedulov > > On Wed, 3 May 2023 at 20:25, Thomas Weise <t...@apache.org> wrote: > >> Hi Alex, >> >> Thanks for the investigation and for the ideas on how to improve the SQL >> gateway REST client. >> >> I think the solution could come in 2 parts. Near term the existing client >> can be patched to support URL mapping and HTTPS based on a standard URL. >> If >> I understand your proposal correctly, that can be implemented in a >> backward >> compatible manner and w/o introducing additional dependencies. >> >> Long term it would make sense to switch to a specialized REST client, or >> at >> least the HTTP client that comes with Java 11 (after Flink says bye to JDK >> 8 support). >> >> Do you have a JIRA for this work? >> >> Thanks, >> Thomas >> >> >> On Wed, May 3, 2023 at 11:46 AM Alexander Fedulov < >> alexander.fedu...@gmail.com> wrote: >> >> > CC'ing some people who worked on the original implementation - curious >> to >> > hear your thoughts. >> > >> > Alex >> > >> > On Fri, 28 Apr 2023 at 14:41, Alexander Fedulov < >> > alexander.fedu...@gmail.com> >> > wrote: >> > >> > > I would like to discuss the current implementation of the SQL Gateway >> > > support in SQL Cli Client and how it can be improved. >> > > >> > > 1) *hosname:port/v1 vs >> > > https://hostname:port/flink-clusters/session-cluster-1/v1 * >> > > Currently, the *--endpoint* parameter needs to be specified in the >> > > *InetSocketAddress* format, i.e. *hostname:port.* While this works >> fine >> > > for basic use cases, it does not support the placement of the gateway >> > > behind a proxy or using an Ingress for routing to a specific Flink >> > cluster >> > > based on the URL path. I.e. it expects *some.hostname.com:9001 >> > > <http://some.hostname.com:9001>* to directly serve requests on * >> > some.hostname.com:9001/v1 >> > > <http://some.hostname.com:9001/v1>* . Mapping to a non-root location, >> > > i.e. *some.hostname.com:9001/flink-clusters/sql-preview-cluster-1/v1 >> > > < >> http://some.hostname.com:9001/flink-clusters/sql-preview-cluster-1/v1>* >> > > is not supported. Since the client talks to the gateway via its REST >> > > endpoint, I believe that the right format for the *--endpoint* >> parameter >> > > is *URL*, not *InetSocketAddress* . >> > > >> > > 2) *HTTPS support* >> > > Another related issue is that internally SQL Client uses Flink’s >> > > *RestClient* [1]. This client decides whether to enable SSL not on >> the >> > > basis of the URL schema (https://...), but based on Flink >> configuration, >> > > namely a global *security.ssl.rest.enabled* parameter [2] (which is >> also >> > > used for the REST server-side configuration ). When this parameter is >> set >> > > to true, it automatically requires user-supplied >> > > *security.ssl.rest.truststore* and *security.ssl.rest.keystore* to be >> > > configured - there is no default option to use certificates from JDK. >> I >> > was >> > > wondering if there is any real benefit in handling the low-level Netty >> > > channels and certificates manually for the use case of connecting >> between >> > > SQL Cli Client and SQL Gateway REST API. There is already a >> dependency >> > on >> > > *OkHttpClient* in *flink-metrics*. I would like to hear what you >> think >> > > about switching to *OkHttp* and adding the ability to optionally load >> > > custom certificates there rather than patching *RestClient*. >> > > >> > > [1] >> > > >> > >> https://github.com/apache/flink/blob/5dddc0dba2be20806e67769314eecadf56b87a53/flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/ExecutorImpl.java#L359 >> > > [2] >> > > >> > >> https://github.com/apache/flink/blob/5d9e63a16f079399c6b51547284bb96db0326bdb/flink-runtime/src/main/java/org/apache/flink/runtime/rest/RestClientConfiguration.java#L103 >> > > >> > > Best, >> > > Alex Fedulov >> > > >> > >> >
