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

> 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-amazo... > >> >> >> > > _______________________________________________ > >> >> >> > > 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 > >> >> > > >> >> > > >> > > >> > > > > >