Re: [hibernate-dev] Introducing a new API in Hibernate Search to "sign" HTTP requests to Elasticsearch

Anyways, I've had my say :)

I reckon you are making fun of me. Anyways, I've had my say :) 2017-06-02 15:03 GMT+02:00 Yoann Rodiere <yoann(a)hibernate.org&gt;: >> What's the benefit of this catch-up game? > > Not tainting our SPI with RestClient :) > > Yoann Rodière > Hibernate NoORM Team > yoann(a)hibernate.org > > On 2 June 2017 at 14:59, Gunnar Morling <gunnar(a)hibernate.org&gt; wrote: >> >> Sure, as I said, an implementor of that SPI will very likely deal with >> the HTTP client. But our SPI isn't tainted by that. >> >> I also don't see much value in re-defining all the options from >> RestClientBuilder. ElasticsearchHttpClientConfigurer resembles >> HttpClientConfigCallback. Would you also re-define >> RequestConfigCallback? What if new options get added to >> RestClientBuilder? What's the benefit of this catch-up game? >> >> >> >> >> 2017-06-02 14:47 GMT+02:00 Yoann Rodiere <yoann(a)hibernate.org&gt;: >> >> There's no exposure of HTTP Client in this SPI. Yes, if people need to >> >> customize the HTTP client to be used by the returned RestClient >> >> instance, they'll naturally depend on that. But this SPI isn't tied to >> >> such detail of how RestClient works - if ES folks decided to use >> >> OkHttp instead, our SPI contract won't be affected (of course user's >> >> implementations will need to change if they were customizing the HTTP >> >> client before). >> > >> > I would feel dishonest arguing this to users... Frankly, there's little >> > point in returning a custom RestClient without customizing stuff related >> > to >> > Apache HTTP Client. See RestClientBuilder: apart from methods tied to >> > Apache >> > HTTP Client, and from options already provided by Hibernate Search, you >> > only >> > have access to these: >> > >> > org.elasticsearch.client.RestClientBuilder. setDefaultHeaders(Header[]) >> > >> > org.elasticsearch.client.RestClientBuilder.setFailureListener( FailureListener) >> > org.elasticsearch.client.RestClientBuilder.setPathPrefix(String) >> > >> > ... and that's all. >> > >> > So yes, this SPI doesn't have a direct dependency to Apache HTTP Client, >> > but >> > any practical use of it will have one. At the end of the day, that's >> > what >> > really matters, right? >> > >> > Yoann Rodière >> > Hibernate NoORM Team >> > yoann(a)hibernate.org >> > >> > On 2 June 2017 at 14:25, Gunnar Morling <gunnar(a)hibernate.org&gt; wrote: >> >> >> >> 2017-06-02 12:49 GMT+02:00 Yoann Rodiere <yoann(a)hibernate.org&gt;: >> >> >> There's an important difference: one exposes Apache HTTP client in >> >> >> HSEARCH's SPI, whereas the other just requires usage of Apache HTTP >> >> >> client within one specific implementation. For users it doesn't >> >> >> change >> >> >> much, but the latter is cleaner from HSEARCH's perspective. >> >> > >> >> > RestClient is not an interface, it's an implementation. There's no >> >> > interface. So yes, we would just be exposing Apache HTTP Client on >> >> > top >> >> > of >> >> > RestClient. >> >> > >> >> >> >> Here's what I'd have done: >> >> >> >> package org.hibernate.search.elasticsearch.client.spi; >> >> >> >> public interface RestClientFactory { >> >> RestClient buildRestClient(SomeContext ctx); >> >> } >> >> >> >> There's no exposure of HTTP Client in this SPI. Yes, if people need to >> >> customize the HTTP client to be used by the returned RestClient >> >> instance, they'll naturally depend on that. But this SPI isn't tied to >> >> such detail of how RestClient works - if ES folks decided to use >> >> OkHttp instead, our SPI contract won't be affected (of course user's >> >> implementations will need to change if they were customizing the HTTP >> >> client before). >> >> >> >> >> I'm not quite following on that. If people are in control of the >> >> >> RestClient entirely, they can do whatever they want? >> >> > >> >> > As mentioned above, exposing RestClient is even worse than just >> >> > exposing >> >> > the >> >> > Apache HTTP client. >> >> >> >> I don't think it's worse, it's better actually. It just exposes our >> >> direct dependency in the SPI and not any of its details. >> >> >> >> > So I was suggesting to use a proper abstraction, namely our own >> >> > interface, >> >> > org.hibernate.search.elasticsearch.client.impl. ElasticsearchClient. >> >> > >> >> > I suppose your remark concerned this paragraph: >> >> > >> >> >> On top of that, this solution would not allow multiple third-party >> >> >> customizations to work well together (for instance AWS >> >> >> authentication >> >> >> provided by us or a third party + some performance tweaking by the >> >> >> user)... >> >> >> which is something the SPI we're planning in the PR could allow. >> >> > >> >> > If we did allow users to re-implement ElasticsearchClient, yes, one >> >> > implementor would be able to do whatever he wants... if he >> >> > re-implements >> >> > it. >> >> > He wouldn't be able to re-use other extensions. >> >> > Take for example the AWS authentication. You're suggestion that we >> >> > provide >> >> > an alternative client allowing to connect to AWS. Fine, we do that, >> >> > and >> >> > users can use it. But what if a users wants AWS authentication and >> >> > say, >> >> > configure a proxy? Then he can't reuse our AWS client, since this >> >> > client >> >> > is >> >> > just an implementation of ElasticsearchClient, and we don't want to >> >> > expose >> >> > anything related to Apache HTTP Client. So he must implement AWS >> >> > authentication. Just to configure a proxy. >> >> > >> >> > The thing is, there are tons of things a user may want to do with >> >> > Apache >> >> > HTTP Client, and we can't possibly provide access to each and every >> >> > option >> >> > through an abstraction layer. >> >> >> >> Right, hence I wouldn't bother to do that in the first place. Just let >> >> users customize how RestClient is instantiated. Let's them do all they >> >> want. We can provide examples which show how to do the AWS signing >> >> etc. >> >> >> >> > So at some point, if we want to allow >> >> > configuration (and in the case of an HTTP client, I'm afraid we have >> >> > to), >> >> > we'll have to expose internals. We just have to make sure this is >> >> > done >> >> > in a >> >> > controlled way (expose as little as possible). >> >> > >> >> > >> >> > >> >> > Yoann Rodière >> >> > Hibernate NoORM Team >> >> > yoann(a)hibernate.org >> >> > >> >> > On 2 June 2017 at 12:26, Gunnar Morling <gunnar(a)hibernate.org&gt; wrote: >> >> >> >> >> >> 2017-06-02 11:58 GMT+02:00 Yoann Rodiere <yoann(a)hibernate.org&gt;: >> >> >> >> Did you instead consider to just let users provide their custom >> >> >> >> instance of org.elasticsearch.client.RestClient? It's still >> >> >> >> leaking >> >> >> >> an >> >> >> >> implementation detail of Hibernate Search, but at least it's one >> >> >> >> indirection less. >> >> >> > >> >> >> > >> >> >> > The only way to add AWS authentication to the RestClient is to use >> >> >> > Apache >> >> >> > HTTP Client classes, so this solution would still bind users to >> >> >> > the >> >> >> > implementation details. We'd just shove it under the carpet :) >> >> >> >> >> >> There's an important difference: one exposes Apache HTTP client in >> >> >> HSEARCH's SPI, whereas the other just requires usage of Apache HTTP >> >> >> client within one specific implementation. For users it doesn't >> >> >> change >> >> >> much, but the latter is cleaner from HSEARCH's perspective. >> >> >> > >> >> >> > Also, I'd argue this would be an even bigger implementation leak: >> >> >> > at >> >> >> > least >> >> >> > with the current solution with can switch to an alternative >> >> >> > Elasticsearch >> >> >> > client, as long as we still use Apache HTTP Client. If we expose >> >> >> > RestClient, >> >> >> > we're stuck with it and whatever underlying technologies it uses. >> >> >> > >> >> >> > On the other hand, we could ask them to re-implement >> >> >> > >> >> >> > org.hibernate.search.elasticsearch.client.impl. ElasticsearchClient, >> >> >> > which is >> >> >> > our own façade over the Elasticsearch client. But I find it a bit >> >> >> > much >> >> >> > just >> >> >> > to tweak some settings or to add a new authentication scheme... >> >> >> > On top of that, this solution would not allow multiple third-party >> >> >> > customizations to work well together (for instance AWS >> >> >> > authentication >> >> >> > provided by us or a third party + some performance tweaking by the >> >> >> > user)... >> >> >> > which is something the SPI we're planning in the PR could allow. >> >> >> >> >> >> I'm not quite following on that. If people are in control of the >> >> >> RestClient entirely, they can do whatever they want? >> >> >> >> >> >> > >> >> >> > Yoann Rodière >> >> >> > Hibernate NoORM Team >> >> >> > yoann(a)hibernate.org >> >> >> > >> >> >> > On 2 June 2017 at 10:30, Sanne Grinovero <sanne(a)hibernate.org&gt; >> >> >> > wrote: >> >> >> >> >> >> >> >> On Fri, 2 Jun 2017, 08:56 Gunnar Morling, < gunnar(a)hibernate.org&gt; >> >> >> >> wrote: >> >> >> >> >> >> >> >> > Hi, >> >> >> >> > >> >> >> >> > I find the exposure of an implementation detail (usage of >> >> >> >> > Apache >> >> >> >> > HTTP >> >> >> >> > client) of the Elasticsearch client a bit problematic. If they >> >> >> >> > change >> >> >> >> > this to another HTTP client, our SPI would break. >> >> >> >> > >> >> >> >> >> >> >> >> Yes the very point of exposing that detail is the reason for this >> >> >> >> thread. >> >> >> >> >> >> >> >> Still, our SPI being guaranteed only for a minor, that gives a >> >> >> >> lot >> >> >> >> of >> >> >> >> flexibility? >> >> >> >> >> >> >> >> The Elasticsearch client exposing this itself, I don't expect >> >> >> >> them >> >> >> >> to >> >> >> >> switch implementation in a micro release to make some bugfix. If >> >> >> >> they >> >> >> >> change it in a major or even minor version, we're ok to not >> >> >> >> support >> >> >> >> that >> >> >> >> version until our next minor. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> > Did you instead consider to just let users provide their custom >> >> >> >> > instance of org.elasticsearch.client.RestClient? It's still >> >> >> >> > leaking >> >> >> >> > an >> >> >> >> > implementation detail of Hibernate Search, but at least it's >> >> >> >> > one >> >> >> >> > indirection less. >> >> >> >> > >> >> >> >> > People wishing to have a custom RestClient would have to >> >> >> >> > implement >> >> >> >> > a >> >> >> >> > few more bits themselves (the logic from >> >> >> >> > DefaultElasticsearchClientFactory#customizeHttpClientConfig( )), >> >> >> >> > but >> >> >> >> > I'd find that acceptable for the sake of a less detail-exposing >> >> >> >> > SPI, >> >> >> >> > plus it grants more flexibility in terms of configuring the >> >> >> >> > RestClient. >> >> >> >> > >> >> >> >> >> >> >> >> >> >> >> >> N.B. The client factory already is a Service so any advanced user >> >> >> >> already >> >> >> >> can override it. >> >> >> >> >> >> >> >> We want to make it easier for cloud users. Focusing on AWS now as >> >> >> >> we've >> >> >> >> had >> >> >> >> user requests for this - not least our own CI - but I'd expect >> >> >> >> other >> >> >> >> clouds >> >> >> >> to have similar features (today or tomorrow). I just don't expect >> >> >> >> other >> >> >> >> use >> >> >> >> cases to need this so we might provide them all eventually, but >> >> >> >> at >> >> >> >> this >> >> >> >> point my goal is to leave an appealing SPI for contributors to >> >> >> >> join >> >> >> >> on >> >> >> >> that. >> >> >> >> >> >> >> >> With that I mean: >> >> >> >> 1# this might evolve but we need something simple to use for >> >> >> >> people >> >> >> >> to >> >> >> >> not >> >> >> >> get stuck. >> >> >> >> 2# I expect integrator implementors to contribute them back >> >> >> >> 3# People won't have this low level dependency in their projects >> >> >> >> for >> >> >> >> long >> >> >> >> >> >> >> >> Having them re-implement the client wouldn't encourage this ;) >> >> >> >> >> >> >> >> Thanks! >> >> >> >> Sanne >> >> >> >> >> >> >> >> >> >> >> >> > --Gunnar >> >> >> >> > >> >> >> >> > >> >> >> >> > 2017-06-01 19:11 GMT+02:00 Sanne Grinovero >> >> >> >> > <sanne(a)hibernate.org&gt;: >> >> >> >> > > Yoann has been working on allowing Hibernate Search users to >> >> >> >> > > use >> >> >> >> > > Elasticsearch on AWS. >> >> >> >> > > >> >> >> >> > > Specifically on AWS the Elasticsearch security can be >> >> >> >> > > configured >> >> >> >> > > to >> >> >> >> > > use application identities, which implies the requests need >> >> >> >> > > to >> >> >> >> > > be >> >> >> >> > > signed. >> >> >> >> > > >> >> >> >> > > A good background read can be found here [1]. >> >> >> >> > > >> >> >> >> > > We planned to allow people to use this but were not planning >> >> >> >> > > to >> >> >> >> > > include AWS specific libraries as dependencies - but since >> >> >> >> > > Yoann >> >> >> >> > > implemented an actual AWS signer in the tests I suppose it >> >> >> >> > > would >> >> >> >> > > be >> >> >> >> > > selfish to not ship it.. >> >> >> >> > > >> >> >> >> > > Please see the API proposal on github (with the PR): >> >> >> >> > > - https://github.com/hibernate/hibernate-search/pull/1426 >> >> >> >> > > >> >> >> >> > > Thanks, >> >> >> >> > > Sanne >> >> >> >> > > >> >> >> >> > > [1] - >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> >> > https://aws.amazon.com/blogs/security/how-to-control- access-to-your-amazon-elasticsearch-service-domain/ >> >> >> >> > > _______________________________________________ >> >> >> >> > > hibernate-dev mailing list >> >> >> >> > > hibernate-dev(a)lists.jboss.org >> >> >> >> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> >> >> >> > >> >> >> >> _______________________________________________ >> >> >> >> hibernate-dev mailing list >> >> >> >> hibernate-dev(a)lists.jboss.org >> >> >> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev >> >> >> > >> >> >> > >> >> > >> >> > >> > >> > > >